Support for multiple tokens in native auth flows #2082

SaurabhMSFT · 2024-04-19T10:52:56Z

This PR and its companion PR (AzureAD/microsoft-authentication-library-common-for-android#2390) enables multiple access token support for Native Auth. A pair of new method have been added in the AccountState class to allow developer to retrieve different access tokens according to the requested scopes. The way these methods acquire tokens is exactly as same as the one in MSAL. The old method 'getAccessToken''s functionality hasn't been changed. It still returns the access token matching the default OIDC scope. This method though has been deprecated.

msal/src/main/java/com/microsoft/identity/client/internal/CommandParametersAdapter.java

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt

SammyO · 2024-04-30T13:16:37Z

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt

@@ -58,6 +60,7 @@ import com.microsoft.identity.nativeauth.utils.serializable
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.withContext
+import java.util.Collections

 /**


As discussed in the technical design document review, the documentation in comments needs to be updated to explain the difference between these two methods.

Comments updated

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt

SammyO · 2024-04-30T13:17:29Z

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt

+        return getAccessTokenInternal(forceRefresh, ArrayList(scopeSet2))
+    }
+
+    suspend fun getAccessTokenInternal(forceRefresh: Boolean, scopes: List<String>?): GetAccessTokenResult {


should scopes be nullable?

yes, the scopes are null from the old deprecated method.

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt

...c/test/java/com/microsoft/identity/nativeauth/NativeAuthPublicClientApplicationKotlinTest.kt

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt

...c/test/java/com/microsoft/identity/nativeauth/NativeAuthPublicClientApplicationKotlinTest.kt

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt

nilo-ms · 2024-05-01T16:21:09Z

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt

+     *
+     * @return [com.microsoft.identity.nativeauth.statemachine.results.GetAccessTokenResult] The result of the getAccessToken action
+     */
+    suspend fun getAccessToken(forceRefresh: Boolean = false, scopes: List<String>?): GetAccessTokenResult {


I think the scopes list should not be optional here

Making it null allows the developers to move to this function from the old deprecated method.

@nilo-ms can you confirm what should happen here? Is scopes optional, or not?

in current MSAL, it's not optional (i.e. if an empty scopes list is passed, the method returns an error)

if we allow empty/null scopes param, how do we communicate to developers how this method works in those scenarios?

how is iOS implementing this for native auth?

I agree with Saurabh that developers need a way to migrate from the old, now-deprecated method to the new one. If the old one doesn't take a scopes parameter, then the new one needs to have the possibility of also not passing any.

I think native auth should work in the same way as the platform specific MSAL works. For example, for MSAL Objective-C, the scope list is not optional, but the external developer can specify an empty list of scopes, no error is thrown. This is how we plan to implement it.
Given that you want to provide a way to migrate to the new method, I suggest to slightly deviate from how MSAL Android work. I propose:

adding the scope list as mandatory

If an empty scope list is passed by the external developer, add the default OIDC scopes internally (as we do for the signIn)

clarify this behaviour in the method comment

Be mindful that if you want to align with the MSAL Android behaviour, it will be considered a breaking change.

I have made the parameter as NonNull

@SaurabhMSFT can you clarify which approach you're taking:

conceptually, scopes are optional. Similar to how getAccessToken() behaves right now.

scopes are not optional. Similar to how MSAL browser-delegated behaves right now.

If we're going with 1, then the following should be implemented IMO:

scopes param may be null.

scopes list may be empty.

As these two are effectively the same, I don't see a reason to limit one but not the other.

Scopes are optional. An empty list of scopes or a null value could show the same intent. Supporting one or the other is just a matter of taste and consistency.

I disagree @SaurabhMSFT. If scopes are optional, the whole parameter should be nullable. It's a pain for developers to have to create an empty list, only because they can't pas null. both null and empty list serve the same functional purpose, so both should be allowed.

SammyO · 2024-05-07T12:14:16Z

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt

@@ -98,6 +100,19 @@ class AccountState private constructor(
        }
    }

+    private fun addDefaultScopes(scopes: List<String>?): List<String> {


Why is this done here, rather than inside the controller? The latter is what MSAL is doing currently, and wherever possible I would like to stick to existing designs/architectures.

This function wasn't used and has been removed.

That's not the question. I can see default scopes are added in the interface layer (in getAccessTokenInternal()), rather than inside the controller. Why not the latter, and align with how MSAL's browser-delegated flow operates?

I have changed the code in getAccessMethod so that empty scopes are not passed to getAccessTokenInternal obviating the need to merge scopes.

SammyO · 2024-05-07T12:15:40Z

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt

@@ -194,14 +209,16 @@ class AccountState private constructor(
    interface GetAccessTokenCallback : Callback<GetAccessTokenResult>

    /**
-     * Retrieves the access token for the currently signed in account from the cache.
+     * Retrieves the access token for the currently signed in account from the cache. If multiple


This comment doesn't make sense, as this method doesn't accept a scopes parameter.
Also, shouldn't this method be returning a token for the default scopes? If so, that should be made clear from the comments.

@nilo-ms can you review this based on your technical proposal please.

I suggest adding a comment like:
Retrieves an access token with default OIDC scopes as permission

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt

msal/src/main/java/com/microsoft/identity/client/TokenParameters.java

SammyO

As discussed on Teams, please propose how these changes should be tested by reviewers, before this is merged in.

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/errors/GetAccessTokenError.kt

SammyO · 2024-05-16T13:52:00Z

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/errors/Error.kt

+        * The INVALID_SCOPES value indicates the scopes provided by the user are not valid
+        * If this occurs, valid scopes should be resubmitted
+        */
+        const val INVALID_SCOPES = "invalid_scopes"


This is not a shared/common error, right? If so and if you agree with my other comment, please move this out of the common error class and into the one that's specific for this scenario.

This is not shared now but should be as we should return similar error from signin method.

we should return similar error from signin method

what makes you say that?

I have moved the variable to GetAccessTokenErrorTypes class

SammyO · 2024-05-16T13:52:36Z

...c/test/java/com/microsoft/identity/nativeauth/NativeAuthPublicClientApplicationKotlinTest.kt

+
+        assertTrue(accessTokenState is GetAccessTokenError)
+        assertTrue((accessTokenState as GetAccessTokenError).isInvalidScopes())
+        assertEquals((accessTokenState as GetAccessTokenError).errorType, ErrorTypes.INVALID_SCOPES)


please update this to use the utility method

There is a call to utility method on line 770. Are you looking for some other utility method?

Ah sorry, missed that. We don't need line 271, IMO, but nit.

changelog

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt

SammyO · 2024-05-23T15:15:31Z

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt

                        )
                    }

                    is Exception -> {
                        GetAccessTokenError(
                            exception = commandResult,
-                            correlationId = "UNSET"
+                            correlationId = correlationId


We should actually be using commandResult.correlationid here, for the unlikely case that the API has returned a different correlation ID. That field might be null, so we'd need to update GetAccessTokenError. And that improvement would have to be applied to more places.

This change can be done when commandResult is ServiceException if commandResult is Exception then we will use the old correlationId.

Feel free to move this into a separate PR. It's not blocking.

This PR and its companion PR (AzureAD/microsoft-authentication-library-for-android#2082) enables multiple access token support for Native Auth. A pair of new method have been added in the AccountState class to allow developer to retrieve different access tokens according to the requested scopes. The way these methods acquire tokens is exactly as same as the one in MSAL. The old method 'getAccessToken''s functionality hasn't been changed. It still returns the access token matching the default OIDC scope. This method though has been deprecated. Two classes that have become unused have been removed.

First draft of the multiple token

cf1f61b

github-actions bot added the msal label Apr 19, 2024

SaurabhMSFT mentioned this pull request Apr 19, 2024

Support for multiple tokens in native auth flows AzureAD/microsoft-authentication-library-common-for-android#2390

Merged

SammyO reviewed Apr 19, 2024

View reviewed changes

msal/src/main/java/com/microsoft/identity/client/internal/CommandParametersAdapter.java Outdated Show resolved Hide resolved

Use silenttoken classes

7e788d8

SammyO reviewed Apr 30, 2024

View reviewed changes

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt Outdated Show resolved Hide resolved

SammyO reviewed Apr 30, 2024

View reviewed changes

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt Outdated Show resolved Hide resolved

SammyO reviewed Apr 30, 2024

View reviewed changes

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt Outdated Show resolved Hide resolved

SammyO reviewed Apr 30, 2024

View reviewed changes

...c/test/java/com/microsoft/identity/nativeauth/NativeAuthPublicClientApplicationKotlinTest.kt Show resolved Hide resolved

SammyO reviewed Apr 30, 2024

View reviewed changes

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt Show resolved Hide resolved

SammyO reviewed Apr 30, 2024

View reviewed changes

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt Outdated Show resolved Hide resolved

SammyO reviewed Apr 30, 2024

View reviewed changes

...c/test/java/com/microsoft/identity/nativeauth/NativeAuthPublicClientApplicationKotlinTest.kt Outdated Show resolved Hide resolved

SammyO reviewed Apr 30, 2024

View reviewed changes

...c/test/java/com/microsoft/identity/nativeauth/NativeAuthPublicClientApplicationKotlinTest.kt Outdated Show resolved Hide resolved

Code review feedback

b9aa09e

nilo-ms reviewed May 1, 2024

View reviewed changes

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt Outdated Show resolved Hide resolved

nilo-ms reviewed May 1, 2024

View reviewed changes

Added deprecation warning

50e6c35

SammyO reviewed May 7, 2024

View reviewed changes

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt Show resolved Hide resolved

SammyO reviewed May 7, 2024

View reviewed changes

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt Outdated Show resolved Hide resolved

nilo-ms reviewed May 7, 2024

View reviewed changes

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt Outdated Show resolved Hide resolved

SaurabhMSFT added 3 commits May 7, 2024 23:36

Code review feedback

4ef097b

Merge branch 'dev' into saugautam/2906127

19130be

Update pointer

0ebf50a

SammyO requested changes May 8, 2024

View reviewed changes

msal/src/main/java/com/microsoft/identity/client/TokenParameters.java Show resolved Hide resolved

SammyO requested changes May 8, 2024

View reviewed changes

Removed unused test and merge of scopes

7d46e01

Update changelog

6957c82

SaurabhMSFT marked this pull request as ready for review May 15, 2024 14:51

SaurabhMSFT requested review from a team as code owners May 15, 2024 14:51

nilo-ms reviewed May 15, 2024

View reviewed changes

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt Show resolved Hide resolved

SaurabhMSFT added 3 commits May 15, 2024 16:16

Remove throws

90e31d6

Return error for empty scopes

1818e11

Add utility method for error

1fe903c

SaurabhMSFT commented May 16, 2024

View reviewed changes

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/errors/GetAccessTokenError.kt Show resolved Hide resolved

SaurabhMSFT added 2 commits May 16, 2024 13:48

Move error string to ErrorTypes class

681ed64

Use new variable

678a810

SammyO reviewed May 16, 2024

View reviewed changes

SaurabhMSFT added 5 commits May 16, 2024 21:06

Move error code to another type

db37b21

Set correlation id

7663b69

Update submodule pointer

267edff

Java tests

c3a2589

Merge remote-tracking branch 'origin/dev' into saugautam/2906127

3b985e4

rmccahill approved these changes May 23, 2024

View reviewed changes

SammyO reviewed May 23, 2024

View reviewed changes

changelog Outdated Show resolved Hide resolved

SammyO reviewed May 23, 2024

View reviewed changes

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt Outdated Show resolved Hide resolved

SammyO reviewed May 23, 2024

View reviewed changes

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt Outdated Show resolved Hide resolved

SammyO reviewed May 23, 2024

View reviewed changes

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt Show resolved Hide resolved

Fix whitespace

ba70fff

SammyO approved these changes May 23, 2024

View reviewed changes

SammyO reviewed May 23, 2024

View reviewed changes

Update pointer

1ba1bfd

SaurabhMSFT merged commit f357998 into dev May 23, 2024
8 checks passed

SaurabhMSFT deleted the saugautam/2906127 branch May 23, 2024 16:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for multiple tokens in native auth flows #2082

Support for multiple tokens in native auth flows #2082

SaurabhMSFT commented Apr 19, 2024 •

edited

Loading

SammyO Apr 30, 2024

SaurabhMSFT May 1, 2024

SammyO Apr 30, 2024

SaurabhMSFT Apr 30, 2024

nilo-ms May 1, 2024

SaurabhMSFT May 2, 2024

SammyO May 7, 2024

nilo-ms May 7, 2024 •

edited

Loading

SaurabhMSFT May 7, 2024

SammyO May 8, 2024

SaurabhMSFT May 8, 2024

SammyO May 9, 2024 •

edited

Loading

SammyO May 7, 2024

SaurabhMSFT May 7, 2024

SammyO May 8, 2024

SaurabhMSFT May 8, 2024

SammyO May 7, 2024

nilo-ms May 7, 2024

SaurabhMSFT May 7, 2024

SammyO left a comment

SammyO May 16, 2024

SaurabhMSFT May 16, 2024

SammyO May 16, 2024

SaurabhMSFT May 16, 2024

SammyO May 16, 2024

SaurabhMSFT May 16, 2024

SammyO May 16, 2024

SammyO May 23, 2024

SaurabhMSFT May 23, 2024 •

edited

Loading

SammyO May 23, 2024

Support for multiple tokens in native auth flows #2082

Support for multiple tokens in native auth flows #2082

Conversation

SaurabhMSFT commented Apr 19, 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

nilo-ms May 7, 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

SammyO May 9, 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

SammyO left a 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

SaurabhMSFT May 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SaurabhMSFT commented Apr 19, 2024 •

edited

Loading

nilo-ms May 7, 2024 •

edited

Loading

SammyO May 9, 2024 •

edited

Loading

SaurabhMSFT May 23, 2024 •

edited

Loading