ODClient.loadCurrentClient() can get to a state where it can never load the current client.

[friedtofu]

Repro step:

• Sign in using MSA.
• Test to see if you can get ODClient.loadCurrentClient() simply by just calling it. (Good)
• Kill the app/re-start the app.
• Attempt to refresh the token or call anything that triggers “storeAccount” in ODAccountStore such as making the token expires so that it will automatically refreshes the token.
• Kill the app/re-start the app
• You can no longer access ODClient.loadCurrentClient()
• Now it gets into an unrecoverable state.

Reason:

The OneDriveSDK upon sign in does this:

Sign in:

• Get a response back , parse and store the tokens and userId (where userId is lowercase) in the keychain.
• There is a map of accountId:serviceInfo (accountId is case sensitive) that gets stored in a file via NSKeyedArchiver.
• There is a field called “currentSession” which is also the accountId (case sensitive) gets stored in a file via NSKeyedArchiver
• When you call ODClient.loadCurrentClient(), everything works as expected because all of the sessions are still in memory.

What happens when we kill and relaunch the app and attempt to access the current client?

• It calls loadAllAccounts, which deserialize the ‘currentSession’ and the ‘accountId:serviceInfo' map into memory.
  Because both of the currentSession and accountId stored in the map are case-sensitive , the look up will succeed. We can find all of the account information associate with this session.
• It then loads all of the secured information from keychain.
• The problem here occurs when the code attempts to assign the “currentSession" to the session we found in the keychain. The session’s account id is now LOWERCASE

IF NOTHING triggers “storeAccounts”, then everything will work fine because nothing new gets saved.
However, since the OneDriveSDK supports automatic refreshing token, upon succeeding the refresh, it will call ‘storeAccounts’ , and rewrite the “currentSession” to a file in lowercase.
The next time the application loads, it will not be able to find a matching account information due to case-mismatch.

We can no longer call ODClient.loadCurrentClient()

[kevklam]

Hi @friedtofu, thanks for reporting this issue.

I'm unable to reproduce the issue as described above - my steps were as follows:

1. [ODClient clientWithCompletion], and in the completion block execute [[ODAccountStore defaultAccountStore] storeCurrentAccount:[client.authProvider accountSession]]
2. Kill the app
3. Launch the app again - upon launching, invokes "self.client = [ODClient loadCurrentClient];"

Upon relaunching the app, it correctly loads the last session and is able to execute requests against the service.

The problem here occurs when the code attempts to assign the “currentSession" to the session we found in the keychain. The session’s account id is now LOWERCASE

I have a couple questions here - first, when you say 'account id' are you referring to the long (~32 character) hexadecimal string, or are you referring to the user's email address, or something else?

I'm also not clear on which part of the code you are referring to. Are you referring to ODAccountStore.m where it performs the following:

if (currentAccountId){
    [self.logger logWithLevel:ODLogVerbose message:@"Loading %@ as current account", currentAccountId];
    ODAccountSession *currentSession = self.accountSessions[currentAccountId];

Or are you referring to something inside keychainWrapper?

Finally:

You can no longer access ODClient.loadCurrentClient()
Now it gets into an unrecoverable state.

What is the behavior you observe when you try to call ODClient.loadCurrentClient once it enters this state?

Thanks,
Kevin

[friedtofu]

Hi Kevin,
Do the same step as you have above. THEN call storeAccount: one more time then kill the app.
If you launch the app again, you will notice everything will fail.

There is a bug in ODAccountStore in especially this part:

        ODAccountSession *currentSession = self.accountSessions[currentAccountId];
        if (currentSession){
            self.currentAccountSession = currentSession;
        }

the currentSession.accountId is lowercase due to the fact that the keychain saves it that way.
However, when you do attempt to find the 'current session' with the current accountId, it is case-sensitive.

The variable currentAccountId above will be caseSensitive (upper and lower case)
The dictionary self.accountSessions contains {lowercaseAccountId: Session}

If you do the steps I describe above, you should be able to hit this state.

It will always hit this part of the conditional statement:

            [self.logger logWithLevel:ODLogError message:@"Failed to load %@ as current account", currentAccountId];

[friedtofu]

If it helps, look at the definition of this function in KeyChainWrapper
[self.keychainStore addOrUpdateItem:accountItem error:&authError];

In the ADALib , they lowercase the accountItem userId , where the ODAccountStore serialize/deserialize the session as-in with full-case characters.

[kevklam]

See the referenced PR. I still have not been able to reproduce the issue because the account ID's returned by the service seem to be lowercase for me already, but I believe this should resolve the issue you described.

Just to be sure - are the account ID's that you see hexadecimal strings, but uppercase? Or are they some other format altogether? If they are some other format I may need to follow up with the MSA team to ensure we aren't breaking any semantics by normalizing like this.

[friedtofu]

An example string of what the account ID is this : AAAAAAAAAAAAAAAAAAAAAGK3KXtotU-qFymJnsZ7FUI
It doesn't look like hexadecimal since there are G and K ..etc
This breaks every single time the token refreshes, which I thought was pretty critical.

I ended up having to write my own custom AccountStore.

[kevklam]

Agreed, this is pretty serious - I'm surprised we haven't seen it before. I will do some digging on my end.

[kevklam]

Issue should be resolved with latest commit - if it recurs, please reopen this issue. Thanks!

[gregyoung14]

@friedtofu @kevklam I'm grabbing the latest branch through cocoapods but I'm still having the same issue with "OneDrive SDK ERROR : Failed to load AAAAAAAAAAAAAAAAAAAAAGj1ZW6pev9i2e1yo_9l0hY as current account" every time I call loadCurrentClient and loadClients remains empty, after successfully calling clientWithCompletion, and then inside setCurrentClient and storeCurrentAccount. What am I missing?

[deco1515]

I have the same issue