-
Notifications
You must be signed in to change notification settings - Fork 128
Native Auth Backward Compatibility feature, Fixes AB#3287604 #2305
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
…nitialization, Fixes AB#3287412 (#2303) Fixes [AB#3287412](https://identitydivision.visualstudio.com/Engineering/_workitems/edit/3287412) Related common PR: AzureAD/microsoft-authentication-library-common-for-android#2656 The Capabilities was added in the common repo however the MSAL repository is pointing to the dev branch by default, not our feature branch. Will change the MSAL feature branch to point to the Common feature branch to ensure pipeline validation of the merge to dev, but doing similar on individual branches tends to introduce unwanted reference errors
…o the developers, Fixes AB#3288191 (#2304) Fixes [AB#3288191](https://identitydivision.visualstudio.com/Engineering/_workitems/edit/3288191) Related common PR: AzureAD/microsoft-authentication-library-common-for-android#2660 Pipeline failed: common reference
… code to include defaultCapabilities
…ClientApplication
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds backward compatibility for native authentication by introducing support for client capabilities and updating state machines to use redirectReason
for Browser-required errors, alongside new configuration APIs.
- Introduce a
capabilities
field in configuration, tests, and validation logic - Update state machine error handling to separate
Redirect
andAPIError
cases and useredirectReason
- Expose a new overload of
createNativeAuthPublicClientApplication
and provide a user-facing factory method
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
msal/src/test/java/.../SSPRTest.kt | Added defaultCapabilities and passed it into setupPCA |
msal/src/test/java/.../NativeAuthPublicClientApplicationAbstractTest.kt | Extended setupPCA to accept capabilities and build the config |
msal/src/test/java/.../GetTokenTests.kt | Updated tests to include defaultCapabilities in setupPCA |
msal/src/main/java/.../SignUpStates.kt | Handle Redirect separately and use redirectReason in errors |
msal/src/main/java/.../SignInStates.kt | Refactored redirect/APIError branching and switched to redirectReason |
msal/src/main/java/.../ResetPasswordStates.kt | Added explicit Redirect handling and use of redirectReason |
msal/src/main/java/.../MFAStates.kt | Introduced Redirect case and migrated to redirectReason |
msal/src/main/java/.../JITStates.kt | Added Redirect branches for JIT states with redirectReason |
msal/src/main/java/.../SignInErrors.kt | Marked error classes as BrowserRequiredError implementations |
msal/src/main/java/.../ResetPasswordErrors.kt | Annotated ResetPasswordSubmitPasswordError with BrowserRequiredError |
msal/src/main/java/.../MFAErrors.kt | Added BrowserRequiredError to MFASubmitChallengeError |
msal/src/main/java/.../JITErrors.kt | Updated JIT error classes to implement BrowserRequiredError |
msal/src/main/java/.../GetAccessTokenError.kt | Annotated GetAccessTokenError with BrowserRequiredError |
msal/src/main/java/.../Error.kt | Extended ResendCodeError to implement BrowserRequiredError |
msal/src/main/java/.../NativeAuthPublicClientApplicationConfigurationFactory.kt | Exposed initializeNativeAuthConfiguration overload |
msal/src/main/java/.../NativeAuthPublicClientApplicationConfiguration.kt | Added capabilities field, getters/setters, and validation logic |
msal/src/main/java/.../MsalClientException.java | Defined new error code/message for invalid capability |
msal/src/main/java/.../PublicClientApplicationConfiguration.java | Ensured getAuthorities initializes its list if null |
msal/src/main/java/.../PublicClientApplication.java | Deprecated old factory and added new overload accepting config |
common | Updated submodule commit |
Comments suppressed due to low confidence (1)
msal/src/test/java/com/microsoft/identity/client/e2e/tests/network/nativeauth/GetTokenTests.kt:58
- Tests cover the happy path with valid capabilities but do not include a case for invalid or unsupported capabilities. Consider adding a negative test to verify that invalid capability values trigger the expected error path.
application = setupPCA(config, defaultChallengeTypes, defaultCapabilities)
...ain/java/com/microsoft/identity/nativeauth/NativeAuthPublicClientApplicationConfiguration.kt
Show resolved
Hide resolved
msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/SignInStates.kt
Show resolved
Hide resolved
…iguration, mAuthorities could be null or mAuthorities.size() == 0 Make changes under getDefaultAuthority() to pass the test
A hint for the change log update and sub module reference after common PR get merged. |
@@ -247,6 +248,9 @@ public void setClientId(final String clientId) { | |||
* @return The List of current Authorities. | |||
*/ | |||
public List<Authority> getAuthorities() { | |||
if (mAuthorities == null) { | |||
mAuthorities = new ArrayList<>(); | |||
} | |||
return mAuthorities; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May need Redmon team for confirmation since it's an important class. Or maybe there's another, better way to fix it.
Why the changes?
- developer's authorities config cannot be null.
- the default status of authorities is null.
getAuthorities()
throws error when authorities is null.- The transformation of null to empty list depends on the
defaultMsalConfiguration(R.raw.msal_default_config)
and merging, clearing operations.
What's the problem?
val nativeAuthConfig = NativeAuthPublicClientApplicationConfiguration()
nativeAuthConfig.clientId = config.clientId
val authorityObject = Authority.getAuthorityFromAuthorityUrl(
config.authorityUrl,
config.clientId
)
authorityObject.setDefault(true)
nativeAuthConfig.getAuthorities().add(authorityObject)
nativeAuthConfig.setChallengeTypes(challengeTypes)
nativeAuthConfig.setCapabilities(capabilities)
nativeAuthConfig.getAuthorities()
will have null pointer error if authorities is null. However, Authorities
does not open the set method.
Previous deprecated method
return createNativeAuthApplication(
Companion.initializeNativeAuthConfiguration(context),
clientId,
authority,
redirectUri,
challengeTypes
);
which use
fun initializeNativeAuthConfiguration(context: Context): NativeAuthPublicClientApplicationConfiguration {
return initializeNativeAuthConfigurationInternal(context, null)
}
Since the developerConfig
parameter is null, thus will skip the developerConfig.validateConfiguration()
step inside initializeNativeAuthConfigurationInternal(context: Context, developerConfig: NativeAuthPublicClientApplicationConfiguration?)
method, where if (authorities == null || authorities.size == 0)
will throw an exception.
The authorities keep null until merging and clearing
private fun initializeNativeAuthConfigurationInternal(context: Context, developerConfig: NativeAuthPublicClientApplicationConfiguration?): NativeAuthPublicClientApplicationConfiguration {
val defaultMsalConfiguration: PublicClientApplicationConfiguration = PublicClientApplicationConfigurationFactory.loadConfiguration(context, R.raw.msal_default_config)
// one authorities
val defaultNativeAuthConfiguration = loadDefaultNativeAuthConfiguration(context)
// no authorities
val config = NativeAuthPublicClientApplicationConfiguration()
config.appContext = context
// Merge base MSAL defaults
config.mergeConfiguration(defaultMsalConfiguration) // authorities.size() == 1
// Marge Native Auth defaults
config.mergeConfiguration(defaultNativeAuthConfiguration) // authorities.size() == 1
config.authorities.clear() // authorities.size() == 0 but authorities is not null
// Check for developerConfig
if (developerConfig != null) { // skip the validation since developer config is null
// Merge developer Native Auth configuration
config.mergeConfiguration(developerConfig)
// Validate Native Auth configuration
config.validateConfiguration()
}
Then structure the authority from the URL and append it into the authorities list
private static NativeAuthPublicClientApplication createNativeAuthApplication(@NonNull final NativeAuthPublicClientApplicationConfiguration config,
@Nullable final String clientId,
@Nullable final String authorityURL,
@Nullable final String redirectUri,
@Nullable final List<String> challengeTypes) throws BaseException {
if (authorityURL != null) {
config.getAuthorities().clear();
final Authority authorityObject = Authority.getAuthorityFromAuthorityUrl(authorityURL, clientId);
authorityObject.setDefault(true);
config.getAuthorities().add(authorityObject);
}
When passing in Config using ConfigFile and ResourceId, if authoriries
field doesn't exist in the JSON file, the gson.fromJson(config, NativeAuthPublicClientApplicationConfiguration::class.java)
will initialize it to null and fail the validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please refer to #2313 for the new solution.
Fixes AB#{3287604} |
|
||
// SignIn specifying authentication context as claim | ||
val result = application.signIn(params) | ||
assertResult<SignInResult.MFARequired>(result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the expected result here is "redirect" no "MFA_required"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. And updated the others.
* - Return redirect with reason mfa required was not supplied | ||
* | ||
*/ | ||
@Ignore("Retrieving OTP code failure.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no OTP code should be sent for this test, so it should be enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Use "Backward compatibility feature not available in eSTS production" instead.
} | ||
|
||
@Test | ||
fun testRepeatedCorrectCapabilities() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what this test is exactly testing? Where are you checking that capabilities duplicates are removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. Add Assert.assertEquals(spyConfig.getCapabilities()?.size,2)
and also for challenge types.
Context: Deal with a comment under #2305
Related msal PR: AzureAD/microsoft-authentication-library-for-android#2305 Fixes [AB#{3287604}](https://dev.azure.com/IdentityDivision/Engineering/_workitems/edit/3287604) Native Auth Backward Compatibility feature includes: - [Add capabilities parameter to native auth public client application initialization, Fixes AB#3287412, Closed AB#3287412](ca1ed52) - [Add capabilities parameter to the flow start operations, Fixes AB#3288182, Closed AB#3288182](1a93a53) - [Handle redirect across all endpoints and return the redirect reason to the developers, Closed AB#3288191](0220406) The main addition to this PR is the testing section
# Conflicts: # common
# Conflicts: # common
Fixes AB#{3287604}
Related common PR: AzureAD/microsoft-authentication-library-common-for-android#2669
Native Auth Backward Compatibility feature includes:
The main addition to this PR is the testing section