-
Notifications
You must be signed in to change notification settings - Fork 38
Native Auth Backward Compatibility feature, Fixes AB#3287604 #2669
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
Conversation
…nitialization, Fixes AB#3287412, Closed AB#3287412 (#2656) Fixes [AB#3287412](https://identitydivision.visualstudio.com/Engineering/_workitems/edit/3287412) Related msal PR: AzureAD/microsoft-authentication-library-for-android#2303
…8182, Closed AB#3288182 (#2658) Fixes [AB#3288182](https://dev.azure.com/IdentityDivision/Engineering/_workitems/edit/3288182) 1. __BaseNativeAuthCommandParameters.java__ - Added `capabilities` field to the base command parameters class 2. __SignInInitiateRequest.kt__ - Added `capabilities` parameter to the sign-in initiate request 3. __SignUpStartRequest.kt__ - Added `capabilities` parameter to the sign-up start request 4. __ResetPasswordStartRequest.kt__ - Added `capabilities` parameter to the reset password start request 5. __NativeAuthRequestProvider.kt__ - Updated to pass capabilities from configuration to requests 6. __NativeAuthOAuth2Configuration.kt__ - Added `capabilities` property to the configuration class pipeline failed due to the broker step.
…o the developers, Closed AB#3288191 (#2660) Fixes [AB#3288191](https://identitydivision.visualstudio.com/Engineering/_workitems/edit/3288191) Related msal PR: AzureAD/microsoft-authentication-library-for-android#2304 Pipeline failed reason: NativeAuthOAuth2Strategy needs both request and response and capabilities configuration together.
❌ Work item link check failed. Description does not contain AB#{ID}. Click here to Learn more. |
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
This PR adds backward compatibility support for redirect-based authentication flows by introducing Redirect
result types, propagating a new redirectReason
field throughout requests, responses, handlers, controllers, and tests, and adds a capabilities
parameter to request configuration.
- Introduce
Redirect
cases in JIT, sign-in, sign-up, and reset-password result and response classes - Add a nullable
capabilities
parameter to request models and propagate it through providers - Update
NativeAuthResponseHandler
,NativeAuthMsalController
, and integration/test suites to handle redirect flows
Reviewed Changes
Copilot reviewed 50 out of 50 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
common4j/src/main/com/microsoft/identity/common/java/nativeauth/providers/responses/jit/JITContinueApiResult.kt | Added Redirect result type with redirectReason |
common4j/src/main/com/microsoft/identity/common/java/nativeauth/providers/responses/jit/JITContinueApiResponse.kt | Extended parsing to detect redirect and wire into new Redirect result |
common4j/src/main/com/microsoft/identity/common/java/nativeauth/providers/responses/jit/JITChallengeApiResult.kt | Added Redirect case for JIT challenge |
common4j/src/main/com/microsoft/identity/common/java/nativeauth/providers/responses/jit/JITChallengeApiResponse.kt | Updated response logic to handle redirect responses |
common4j/src/main/com/microsoft/identity/common/java/nativeauth/providers/requests/signup/SignUpStartRequest.kt | Introduced capabilities field to sign-up start request |
common4j/src/main/com/microsoft/identity/common/java/nativeauth/providers/requests/signin/SignInInitiateRequest.kt | Introduced capabilities field to sign-in initiate request |
common4j/src/main/com/microsoft/identity/common/java/nativeauth/providers/requests/resetpassword/ResetPasswordStartRequest.kt | Introduced capabilities field to reset-password start request |
common4j/src/main/com/microsoft/identity/common/java/nativeauth/providers/NativeAuthResponseHandler.kt | Populated redirectReason default/null and branched on isRedirect() |
common4j/src/main/com/microsoft/identity/common/java/nativeauth/providers/NativeAuthRequestProvider.kt | Passed config.capabilities into request constructors |
common4j/src/main/com/microsoft/identity/common/java/nativeauth/providers/NativeAuthOAuth2Configuration.kt | Added optional capabilities to configuration |
common4j/src/main/com/microsoft/identity/common/java/nativeauth/providers/NativeAuthConstants.kt | Defined Capabilities constants and documented usage |
common4j/src/main/com/microsoft/identity/common/java/nativeauth/providers/INativeAuthApiResponse.kt | Replaced IApiResponse with INativeAuthApiResponse adding redirectReason |
common4j/src/main/com/microsoft/identity/common/java/nativeauth/controllers/results/INativeAuthCommandResult.kt | Updated Redirect command result to include redirectReason |
common4j/src/main/com/microsoft/identity/common/java/nativeauth/commands/parameters/BaseNativeAuthCommandParameters.java | Added capabilities field for logging and parameter propagation |
common/src/test/java/com/microsoft/identity/common/nativeauth/util/CommandResultUtilTest.kt | Added test vector for Redirect in CommandResultUtil |
common/src/test/java/com/microsoft/identity/common/internal/providers/microsoft/nativeauth/integration/SignUpOAuth2StrategyTest.kt | New integration test for sign-up redirect flow |
common/src/test/java/com/microsoft/identity/common/internal/providers/microsoft/nativeauth/integration/SignInOAuthStrategyTest.kt | Renamed and added redirect case for sign-in challenge |
common/src/test/java/com/microsoft/identity/common/internal/providers/microsoft/nativeauth/integration/ResetPasswordOAuth2StrategyTest.kt | Added redirect test cases for reset-password flows |
common/src/test/java/com/microsoft/identity/common/internal/providers/microsoft/nativeauth/integration/JITOAuthStrategyTest.kt | New JIT integration tests covering redirect behaviour |
common/src/main/java/com/microsoft/identity/common/nativeauth/internal/controllers/NativeAuthMsalController.kt | Extended controller branches to handle Redirect results |
Comments suppressed due to low confidence (2)
common4j/src/main/com/microsoft/identity/common/java/nativeauth/providers/NativeAuthConstants.kt:74
- The comment refers to
MFARequired
andRegistrationRequired
but the constants are namedMFA_REQUIRED
andREGISTRATION_REQUIRED
; updating the docs to match the actual constant names will avoid confusion.
* MFARequired: The application can accommodate ...
common4j/src/main/com/microsoft/identity/common/java/nativeauth/providers/requests/signup/SignUpStartRequest.kt:99
- [nitpick] The
toString()
implementation omits the newcapabilities
field; consider including it to reflect the full parameters in logs.
override fun toString(): String = "NativeAuthRequestSignUpStartRequestParameters(clientId=$clientId)"
val redirectReason: String | ||
) : JITContinueApiResult { | ||
override fun toUnsanitizedString(): String { | ||
return "Redirect(correlationId=$correlationId, redirectReason=$redirectReason)" |
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.
[nitpick] Consider adding the class qualifier for clarity, e.g. JITContinueApiResult.Redirect(...)
, to distinguish this string from other Redirect
cases.
return "Redirect(correlationId=$correlationId, redirectReason=$redirectReason)" | |
return "JITContinueApiResult.Redirect(correlationId=$correlationId, redirectReason=$redirectReason)" |
Copilot uses AI. Check for mistakes.
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.
Good suggestion but tend to keep alignment the existing common practice.
@@ -89,6 +90,7 @@ class NativeAuthResponseHandler { | |||
unverifiedAttributes = null, | |||
invalidAttributes = null, | |||
challengeType = null, | |||
redirectReason = null, |
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.
[nitpick] There's a lot of repeated redirectReason = null
across handlers; consider refactoring into a helper or relying on default constructor values to reduce boilerplate.
Copilot uses AI. Check for mistakes.
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.
Good suggestion. Did default constructor before. But tend to be explicit on the parameters.
...microsoft/identity/common/java/nativeauth/providers/responses/jit/JITChallengeApiResponse.kt
Outdated
Show resolved
Hide resolved
A hit for the change log update after final review. |
…into feature/backward-compatibility
.../identity/common/internal/providers/microsoft/nativeauth/integration/JITOAuthStrategyTest.kt
Show resolved
Hide resolved
.../identity/common/internal/providers/microsoft/nativeauth/integration/JITOAuthStrategyTest.kt
Show resolved
Hide resolved
@@ -51,6 +51,9 @@ public class OAuth2StrategyParameters { | |||
@Nullable | |||
public final List<String> mChallengeTypes; | |||
|
|||
@Nullable | |||
public final List<String> mCapabilities; |
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.
Let's put a comment here for clarity
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. Updated both challenge type and capabilities.
Fixes [AB#{3287604}](https://dev.azure.com/IdentityDivision/Engineering/_workitems/edit/3287604) Related common PR: AzureAD/microsoft-authentication-library-common-for-android#2669 Native Auth Backward Compatibility feature includes: - [Add capabilities parameter to native auth public client application initialization, Fixes AB#3287412](6d55346) - [Handle redirect across all endpoints and return the redirect reason to the developers, Fixes AB#3288191](a121610) - [Use parameter instead of config when initialization](#2313) The main addition to this PR is the testing section
Related msal PR: AzureAD/microsoft-authentication-library-for-android#2305
Fixes AB#{3287604}
Native Auth Backward Compatibility feature includes:
The main addition to this PR is the testing section