@W-20026518: [MSDK Android] Null OAuth Refresh Token Parameters During User App Upgrade From MSDK 12.0.1 To 13.0.2 Cause Unexpected Logout #2794
Conversation
…g User App Upgrade From MSDK 12.0.1 To 13.0.2 Cause Unexpected Logout
Generated by 🚫 Danger |
| if (addlParams != null ) { | ||
| for (final Map.Entry<String,String> entry : addlParams.entrySet()) { | ||
| builder.add(entry.getKey(),entry.getValue()); | ||
| // Safely ignore missing values since, for instance, a user account that is being upgraded may not have received that value yet. |
There was a problem hiding this comment.
I'm trying to see the connection between this change and the earlier comments.
Why would parent sid or token format be sent up to the server doing refresh?
There was a problem hiding this comment.
It looks like the UserAccount properties are added to the result of UserAccount.getAdditionalOauthValues() which is then passed as the final parameter of refreshAuthToken(). Take a look at UserAccount.toBundle() and UserAccount.toJson(), which seem to reference both PARENT_SID and TOKEN_FORMAT. I believe those are adding the keys which get to getAdditionalOauthValues() and refreshAuthToken().
There was a problem hiding this comment.
getAdditionalOauthValues() is driven by SalesforceSDKManager.getInstance().getAdditionalOauthKeys() which is defined by the application to indicate additional fields it want to save from the token response in the user account.
Now that parent_sid is a "first class citizen" meaning that the code explicitly reads it from the response and save it into the user account, it no longer needs to be one of the oauth keys provided by the application.
Honestly I don't know why we send all of those up during the refresh token call to the server. Those are not required params for the refresh token endpoint.
In short:
- MSDK should probably not send additional oauth values to the server on refresh (we are doing the same on iOS but the refresh endpoint does not seem to do anything with those parameters.
- Apps no longer need to specify parent_sid as an oauthKey to save in the UserAccount, MSDK 13 does it automatically.
- If the upgraded app starts asking for it (meaning it has it in its additional oauth keys) but the user logged in with a version of the app that did not ask for it, then the getAdditionalOauthKeys() will have a null value, causing the issue above. Your change is safe but doing (1) or (2) would also avoid the issue.
| final String sidCookieName = decryptUserData(account, AuthenticatorService.KEY_SID_COOKIE_NAME, encryptionKey); | ||
| final String clientId = decryptUserData(account, AuthenticatorService.KEY_CLIENT_ID, encryptionKey); | ||
|
|
||
| // "Parent SID" and "Token Format" were introduced in Salesforce Mobile SDK 13.0.0. May be null for user accounts of previous versions, but will be populated after successful log in or access token refresh. |
There was a problem hiding this comment.
I would remove that comment - it's redundant with the one line 507.
Also a new parent sid is only an issue if the app has "parent_sid" and its additional auth keys (which it shouldn't with MSDK 13) and because we send those to the refresh end point (which we shouldn't).
There was a problem hiding this comment.
👍🏻 Done!
…g User App Upgrade From MSDK 12.0.1 To 13.0.2 Cause Unexpected Logout (Remove Unneeded Comment)
…g User App Upgrade From MSDK 12.0.1 To 13.0.2 Cause Unexpected Logout (Escalate Some Token Refresh Exceptions From Warn To Error For Consistency)
| return null; | ||
| } catch (Exception e) { | ||
| SalesforceSDKLogger.w(TAG, "Exception thrown while getting new auth token", e); | ||
| SalesforceSDKLogger.e(TAG, "Exception thrown while getting new auth token", e); |
There was a problem hiding this comment.
@wmathurin - I added one more change after your approval. Sorry! I remembered from last night that the key reason we didn't see this a little sooner was because the most relevant exception was warn rather than error. One already was error out of three cases, so now they all are error for easier diagnosis.
JohnsonEricAtSalesforce
merged commit 28c0d83
into
forcedotcom:dev
JohnsonEricAtSalesforce
deleted the
bugfix/w-20026518_msdk-android-null-oauth-refresh-token-parameters-during-user-app-upgrade-from-msdk-12.0.1-to-13.0.2-cause-unexpected-logout
branch
2 participants
🌘 Ready For Review! 🌒
Salesforce Mobile SDK introduced two new user account properties in version 13 for JWT support: "Parent SID" and "Token Format". These are not saved in the user account properties of user accounts upgrading from previous versions. In the case of some apps where the SDK is initialized in a specific sequence and the user's access token has expired, the token refresh will run first and have missing values for the two new properties. This causes token refresh to fail and the user to be unexpectedly logged out.
This update adds a default null-guard to the access token refresh to ignore missing parameter values, which is likely a safe default since the user account will be refreshed and the missing values populated. I verified this in the affected app at runtime.
An update also adds maintainer notes where future new values may need default values.