ID Token should be updated when Access Token is refreshed/acquired silently #2141

spetrescu84 · 2024-05-01T10:16:47Z

Proposed changes

The Auth Tokens present in the UserAccountResult and specifically the ID Token alongside the MSALAccount get updated when a refresh token operation happens

Type of change

Risk

High – Errors could cause MAJOR regression of many scenarios. (Example: new large features or high level infrastructure changes)
Medium – Errors could cause regression of 1 or more scenarios. (Example: somewhat complex bug fixes, small new features)
Small – No issues are expected. (Example: Very small bug fixes, string changes, or configuration settings changes)

Additional information

To test this, the test case Verify split issuer needs to be run. The UserAccountResult returned when the getAccount is called, needs to have the ID Token updated when refreshToken is called and the MSALAccount should have the Account Claims updated

azure-pipelines · 2024-05-01T10:17:32Z

MSAL/src/native_auth/controllers/credentials/MSALNativeAuthCredentialsController.swift

@@ -83,7 +83,9 @@ final class MSALNativeAuthCredentialsController: MSALNativeAuthTokenController,
        return nil


This pull request does not update CHANGELOG.md.

Please consider if this change would be noticeable to a partner or user and either update CHANGELOG.md or resolve this conversation.

diegojerezba · 2024-05-01T14:54:32Z

MSAL/src/native_auth/controllers/credentials/MSALNativeAuthCredentialsController.swift

@@ -182,12 +189,40 @@ final class MSALNativeAuthCredentialsController: MSALNativeAuthTokenController,
                context: context,
                format: "Refresh Token completed successfully")
            // TODO: Handle tokenResult.refreshToken as? MSIDRefreshToken in a safer way


Now that we are changing this part, can you handle this TODO comment?

Moved to factory, so removed

diegojerezba

Left a small comment but LGTM, there isn't a reference cycle between MSALNativeAuthUserAccountResult and CredentialsController (I was a bit wary of the UserAccountResult passing itself to the CredentialsController, but it seems the object is being correctly deallocated).

diegojerezba · 2024-05-02T13:00:26Z

MSAL/src/native_auth/public/MSALNativeAuthUserAccountResult.swift

    let configuration: MSALNativeAuthConfiguration
    private let cacheAccessor: MSALNativeAuthCacheInterface

-    /// Get the ID token for the account.
+    /// Get the latest ID token for the account.
    @objc public var idToken: String? {
        authTokens.rawIdToken


@spetrescu84 did you consider retrieving the id token from the cache here? Maybe that would be a cleaner way, as you wouldn't have to make the account property mutable and would avoid most of the new changes.

I'm not sure if the performance would be affected too much, though.

If we retrieve from cache we would need an account and it could be the access token is expired so we would need to get a new one so this property would need to have a completion so I don't think it would work.
Currently the account that is retrieved is the first one from cache, but if neither exist, the MSALNativeAuthUserAccountResult will not be created.
In the following scenario:
-App retrieves from cache the MSALAccount and a MSALNativeAuthUserAccountResult gets created
-Another app deletes the cache and the MSALAccount gets deleted from cache
-We try to get it again from cache and it fails because it's no longer there
I am simplifying it because the MSALAccount actually gets created out of a IDToken but I'd say the way we do it now works best as we have it in memory and don't have to deal with the change to cache/expiration.
I don't think there's a way around making it mutable here

nilo-ms · 2024-05-02T15:47:11Z

MSAL/src/native_auth/controllers/credentials/MSALNativeAuthCredentialsController.swift

+            do {
+                let claims = try MSIDIdTokenClaims.init(rawIdToken: tokenResult.rawIdToken)
+                jsonDictionary = claims.jsonDictionary()
+                if jsonDictionary == nil {


Do claims needs to be always not nil?

Claims can be nil, MSAL Objective C logs as a warning if a MSIDTokenResult cannot be converted to a MSALResult, regardless if claims cannot be created. Account is always created.
MSID_LOG_WITH_CTX_PII(MSIDLogLevelWarning, nil, @"MSALErrorConverter could not convert MSIDTokenResult to MSALResult %@", MSID_PII_LOG_MASKABLE(resultError))
Changed the level to a warning one.

nilo-ms · 2024-05-02T15:48:38Z

MSAL/src/native_auth/controllers/credentials/MSALNativeAuthCredentialsController.swift

@@ -169,11 +174,13 @@ final class MSALNativeAuthCredentialsController: MSALNativeAuthTokenController,
        }
    }

+    // swiftlint:disable:next function_body_length


can you move the claims extraction/account creation in another private method? So, we don't need to add an exception here

No longer needed as it was all moved to the MSALNativeAuthResultBuildable

nilo-ms · 2024-05-02T15:51:13Z

MSAL/src/native_auth/controllers/credentials/MSALNativeAuthCredentialsController.swift

+                    context: context,
+                    format: "Claims for account could not be created - \(error)" )
+            }
+            guard let account = MSALAccount.init(msidAccount: tokenResult.account,


Can we reuse the code from the MSALNativeAuthResultFactory class?

Reused, created 2 new function to create the account and create the authTokens in the factory
func makeAccount(tokenResult: MSIDTokenResult, context: MSIDRequestContext) -> MSALAccount?
func makeAuthTokens(tokenResult: MSIDTokenResult, context: MSIDRequestContext) -> MSALNativeAuthTokens?

nilo-ms · 2024-05-02T15:54:52Z

MSAL/test/unit/native_auth/controllers/MSALNativeAuthCredentialsControllerTests.swift

        responseValidatorMock.tokenValidatedResponse = .success(tokenResponse)
-        cacheAccessorMock.mockUserAccounts = [account]
+        cacheAccessorMock.mockUserAccounts = [newAccount!]


can we avoid the force unwrap here?

I'd say we are ok with force unwraps in unit tests. No point to add guards here.

The only point is that if newAccount is nil, we crash, and all the next tests are not executed. While if we gracefully unwrap it only this test will fail

We have that in many tests such as MSALNativeAuthUrlRequestSerializerTests or MSALNativeAuthResponseErrorHandlerTests. The idea is for them to be succinct. I can add it but we should have all tests behave the same because if the tests in any other classes fail, it will stop execution also.

nilo-ms · 2024-05-02T15:55:55Z

MSAL/test/unit/native_auth/controllers/MSALNativeAuthCredentialsControllerTests.swift

@@ -121,13 +121,14 @@ final class MSALNativeAuthCredentialsControllerTests: MSALNativeAuthTestCase {

        let expectedContext = MSALNativeAuthRequestContext(correlationId: defaultUUID)


can you also test the case when the account claims can't be parsed?

Added, but not here, but rather in MSALNativeAuthResultFactoryTests as that's where it should be. Also changed how the code works because the MSALAccount always gets created no matter what as it's not an optional (we had it wrong because of Objective C/Swift interoperability).
Changed the names of the tests on this class to highlight the userAccountResult doesn't get created rather than the account

* dev: Update automation.yml for Azure Pipelines Update automation.yml for Azure Pipelines Update automation.yml for Azure Pipelines ID Token should be updated when Access Token is refreshed/acquired silently (#2141) So support ssh-cert flow through broker (Update submodule only) (#2134) # Conflicts: # MSAL/IdentityCore

* Update msal-release-ado-trigger.yml for Azure Pipelines * Update msal-release-ado-trigger.yml for Azure Pipelines * Update msal-release-ado-trigger.yml for Azure Pipelines * Update msal-release-ado-trigger.yml for Azure Pipelines * Update msal-release-ado-trigger.yml for Azure Pipelines * Update msal-release-ado-trigger.yml for Azure Pipelines * Delete spm-framework.yml * Update from dev * remove test * Reintroduce Minimum OS Version Requirements to Readme Reintroducing minimum version indicators that were [mistakenly?] removed in #2080 * So support ssh-cert flow through broker (Update submodule only) (#2134) * cc update * Update submodule * Update submodule * ID Token should be updated when Access Token is refreshed/acquired silently (#2141) * Changed refresh token to update account and tokens on UserAccountResult * Unit tests * Swiftlint * Updated code comment * PR Comments * Update automation.yml for Azure Pipelines * Update automation.yml for Azure Pipelines * Update automation.yml for Azure Pipelines * This PR removes the ADAL keyvault and client secret (#2150) * Update submodule * Update yml file * Revert conf file from testings * Update msal submodule (#2160) * Update core. * Bump version. * modified: CHANGELOG.md * modified: IdentityCore --------- Co-authored-by: Ameya Patil <ameyapat@buffalo.edu> Co-authored-by: Ameya <> Co-authored-by: Antonio Alwan <antonioalwan@microsoft.com> Co-authored-by: Hieu Nguyen <hiengu@microsoft.com> Co-authored-by: Ameya Patil <amepatil@microsoft.com> Co-authored-by: Hieu Nguyen <65981263+hieunguyenmsft@users.noreply.github.com> Co-authored-by: Brian Melton-Grace <iambmelton@gmail.com> Co-authored-by: Kai <kaisong1990@gmail.com> Co-authored-by: Silviu Petrescu <111577419+spetrescu84@users.noreply.github.com> Co-authored-by: Swasti Gupta <swastinitb@gmail.com>

* Update core. * Bump version. * modified: CHANGELOG.md * modified: IdentityCore * Merge release 1.3.3 to main (#2165) * Update msal-release-ado-trigger.yml for Azure Pipelines * Update msal-release-ado-trigger.yml for Azure Pipelines * Update msal-release-ado-trigger.yml for Azure Pipelines * Update msal-release-ado-trigger.yml for Azure Pipelines * Update msal-release-ado-trigger.yml for Azure Pipelines * Update msal-release-ado-trigger.yml for Azure Pipelines * Delete spm-framework.yml * Update from dev * remove test * Reintroduce Minimum OS Version Requirements to Readme Reintroducing minimum version indicators that were [mistakenly?] removed in #2080 * So support ssh-cert flow through broker (Update submodule only) (#2134) * cc update * Update submodule * Update submodule * ID Token should be updated when Access Token is refreshed/acquired silently (#2141) * Changed refresh token to update account and tokens on UserAccountResult * Unit tests * Swiftlint * Updated code comment * PR Comments * Update automation.yml for Azure Pipelines * Update automation.yml for Azure Pipelines * Update automation.yml for Azure Pipelines * This PR removes the ADAL keyvault and client secret (#2150) * Update submodule * Update yml file * Revert conf file from testings * Update msal submodule (#2160) * Update core. * Bump version. * modified: CHANGELOG.md * modified: IdentityCore --------- Co-authored-by: Ameya Patil <ameyapat@buffalo.edu> Co-authored-by: Ameya <> Co-authored-by: Antonio Alwan <antonioalwan@microsoft.com> Co-authored-by: Hieu Nguyen <hiengu@microsoft.com> Co-authored-by: Ameya Patil <amepatil@microsoft.com> Co-authored-by: Hieu Nguyen <65981263+hieunguyenmsft@users.noreply.github.com> Co-authored-by: Brian Melton-Grace <iambmelton@gmail.com> Co-authored-by: Kai <kaisong1990@gmail.com> Co-authored-by: Silviu Petrescu <111577419+spetrescu84@users.noreply.github.com> Co-authored-by: Swasti Gupta <swastinitb@gmail.com> * modified: MSAL/IdentityCore * Updating MSAL framework checksum & url for 1.3.3 [skip ci] --------- Co-authored-by: Ameya Patil <ameyapat@buffalo.edu> Co-authored-by: Antonio Alwan <antonioalwan@microsoft.com> Co-authored-by: Hieu Nguyen <hiengu@microsoft.com> Co-authored-by: Ameya Patil <amepatil@microsoft.com> Co-authored-by: Hieu Nguyen <65981263+hieunguyenmsft@users.noreply.github.com> Co-authored-by: Brian Melton-Grace <iambmelton@gmail.com> Co-authored-by: Kai <kaisong1990@gmail.com> Co-authored-by: Silviu Petrescu <111577419+spetrescu84@users.noreply.github.com> Co-authored-by: Swasti Gupta <swastinitb@gmail.com>

spetrescu84 added 3 commits April 30, 2024 15:47

Changed refresh token to update account and tokens on UserAccountResult

702f40f

Unit tests

e9c9824

Swiftlint

6fa6bff

spetrescu84 added the native-auth Code related to native authentication label May 1, 2024

spetrescu84 requested review from dualtagh, nilo-ms, diegojerezba and borgesmb May 1, 2024 10:16

spetrescu84 self-assigned this May 1, 2024

spetrescu84 requested a review from a team as a code owner May 1, 2024 10:16

azure-pipelines bot reviewed May 1, 2024

View reviewed changes

Updated code comment

ff0e5bd

diegojerezba reviewed May 1, 2024

View reviewed changes

diegojerezba approved these changes May 1, 2024

View reviewed changes

diegojerezba reviewed May 2, 2024

View reviewed changes

nilo-ms reviewed May 2, 2024

View reviewed changes

PR Comments

645ccae

nilo-ms approved these changes May 3, 2024

View reviewed changes

Merge branch 'dev' into spetrescu/idtoken-update-data

bae8bff

spetrescu84 merged commit 28bcbef into dev May 3, 2024
6 checks passed

azure-pipelines bot mentioned this pull request May 7, 2024

Automation tests failure #2149

Closed

nilo-ms mentioned this pull request Sep 25, 2024

[iOS SDK] Update current user account result with latest account and id token after refresh #2346

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ID Token should be updated when Access Token is refreshed/acquired silently #2141

ID Token should be updated when Access Token is refreshed/acquired silently #2141

spetrescu84 commented May 1, 2024

azure-pipelines bot May 1, 2024

diegojerezba May 1, 2024

spetrescu84 May 3, 2024

diegojerezba left a comment

diegojerezba May 2, 2024 •

edited

Loading

spetrescu84 May 3, 2024

nilo-ms May 2, 2024

spetrescu84 May 3, 2024 •

edited

Loading

nilo-ms May 2, 2024 •

edited

Loading

spetrescu84 May 3, 2024

nilo-ms May 2, 2024

spetrescu84 May 3, 2024

nilo-ms May 2, 2024

spetrescu84 May 3, 2024

nilo-ms May 3, 2024

spetrescu84 May 3, 2024

nilo-ms May 2, 2024

spetrescu84 May 3, 2024

		@@ -83,7 +83,9 @@ final class MSALNativeAuthCredentialsController: MSALNativeAuthTokenController,
		return nil

		@@ -121,13 +121,14 @@ final class MSALNativeAuthCredentialsControllerTests: MSALNativeAuthTestCase {

		let expectedContext = MSALNativeAuthRequestContext(correlationId: defaultUUID)

ID Token should be updated when Access Token is refreshed/acquired silently #2141

ID Token should be updated when Access Token is refreshed/acquired silently #2141

Conversation

spetrescu84 commented May 1, 2024

Proposed changes

Type of change

Risk

Additional information

azure-pipelines bot May 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegojerezba left a comment

Choose a reason for hiding this comment

diegojerezba May 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spetrescu84 May 3, 2024 • edited Loading

Choose a reason for hiding this comment

nilo-ms May 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegojerezba May 2, 2024 •

edited

Loading

spetrescu84 May 3, 2024 •

edited

Loading

nilo-ms May 2, 2024 •

edited

Loading