-
Notifications
You must be signed in to change notification settings - Fork 129
Use parameter instead of config when initialization #2313
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
Use parameter instead of config when initialization #2313
Conversation
❌ 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 refactors native authentication initialization to use a standalone NativeAuthConfigParameters
object (with capabilities
) instead of relying on the config file, and adds validation and serialization support for the new capabilities field.
- Introduced
capabilities
inNativeAuthConfigParameters
and madePublicClientApplication
accept it. - Extended
NativeAuthPublicClientApplicationConfiguration
to parse, merge, serialize, and validate capabilities. - Updated tests (
setupPCA
calls) and added unit tests for missing, invalid, and correct capabilities.
Reviewed Changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
msal/src/test/java/.../NativeAuthPublicClientApplicationConfigurationTest.kt | Added tests for missing/invalid/case-insensitive capabilities |
msal/src/test/java/.../NativeAuthOAuth2ConfigurationTest.kt | Passed capabilities = null into OAuth2 config |
msal/src/test/java/.../SignUpEmailPasswordTest.kt | Updated setupPCA calls to include defaultCapabilities |
msal/src/test/java/.../SignUpEmailPasswordAttributesTest.kt | Updated setup to include defaultCapabilities |
msal/src/test/java/.../SignUpEmailOTPTest.kt | Updated setupPCA calls and removed old setup |
msal/src/test/java/.../SignUpEmailOTPAttributesTest.kt | Updated setup to include defaultCapabilities |
msal/src/test/java/.../SignInMFATest.kt | Passed defaultCapabilities to setupPCA |
msal/src/test/java/.../SignInJITTest.kt | Passed defaultCapabilities and added JIT error test |
msal/src/test/java/.../SignInEmailPasswordTest.kt | Passed defaultCapabilities to setupPCA |
msal/src/test/java/.../SignInEmailOTPTest.kt | Added defaultCapabilities and updated setup |
msal/src/test/java/.../SSPRTest.kt | Passed defaultCapabilities to setupPCA |
msal/src/test/java/.../NativeAuthPublicClientApplicationAbstractTest.kt | Extended setupPCA signature to take capabilities |
msal/src/test/java/.../GetTokenTests.kt | Updated setup to include defaultCapabilities |
msal/src/main/java/.../SignUpStates.kt | Switched to use redirectReason in errors and separated cases |
msal/src/main/java/.../SignInStates.kt | Switched to use redirectReason and separated cases |
msal/src/main/java/.../ResetPasswordStates.kt | Switched to use redirectReason and separated cases |
msal/src/main/java/.../MFAStates.kt | Switched to use redirectReason and separated cases |
msal/src/main/java/.../JITStates.kt | Switched to use redirectReason and separated cases |
msal/src/main/java/.../SignInErrors.kt | Made SignInSubmitPasswordError extend BrowserRequiredError |
msal/src/main/java/.../ResetPasswordErrors.kt | Made ResetPasswordSubmitPasswordError extend BrowserRequiredError |
msal/src/main/java/.../MFAErrors.kt | Made MFASubmitChallengeError extend BrowserRequiredError |
msal/src/main/java/.../JITErrors.kt | Made JIT errors extend BrowserRequiredError |
msal/src/main/java/.../GetAccessTokenError.kt | Made GetAccessTokenError extend BrowserRequiredError |
msal/src/main/java/.../Error.kt | Made ResendCodeError extend BrowserRequiredError |
msal/src/main/java/.../NativeAuthRegisterStrongAuthVerificationRequiredResultParameters.kt | Added missing copyright header |
msal/src/main/java/.../NativeAuthConfigParameters.kt | New parameter class for native-auth init (includes capabilities) |
msal/src/main/java/.../NativeAuthChallengeAuthMethodParameters.kt | Added missing copyright header |
msal/src/main/java/.../NativeAuthPublicClientApplicationConfiguration.kt | Added capabilities field, serialization, merge, and validation |
msal/src/main/java/.../CommandParametersAdapter.java | Propagated capabilities into command parameters |
msal/src/main/java/.../MsalClientException.java | Defined invalid capability error code/message |
msal/src/main/java/.../PublicClientApplicationConfiguration.java | Added import for ArrayList (no functional change) |
msal/src/main/java/.../PublicClientApplication.java | Added overload to accept NativeAuthConfigParameters and handle capabilities |
common | Updated submodule commit |
Comments suppressed due to low confidence (1)
msal/src/main/java/com/microsoft/identity/nativeauth/NativeAuthPublicClientApplicationConfiguration.kt:207
- The comment refers to capabilities but sits in
validateChallengeTypes
; it should read "Remove duplicate challenge types".
// Remove duplicate capabilities
@@ -154,7 +155,7 @@ class SignUpEmailPasswordTest : NativeAuthPublicClientApplicationAbstractTest() | |||
@Test | |||
fun testSuccessOTPResend() { | |||
config = getConfig(defaultConfigType) | |||
application = setupPCA(config, defaultChallengeTypes) | |||
application = setupPCA(config, defaultChallengeTypes, defaultChallengeTypes) |
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 third argument should be defaultCapabilities
instead of defaultChallengeTypes
to pass the correct capabilities list.
application = setupPCA(config, defaultChallengeTypes, defaultChallengeTypes) | |
application = setupPCA(config, defaultChallengeTypes, defaultCapabilities) |
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.
Done.
@@ -242,7 +243,7 @@ class SignUpEmailPasswordTest : NativeAuthPublicClientApplicationAbstractTest() | |||
@Test | |||
fun testErrorInvalidPasswordFormat() { | |||
config = getConfig(defaultConfigType) | |||
application = setupPCA(config, defaultChallengeTypes) | |||
application = setupPCA(config, defaultChallengeTypes, defaultChallengeTypes) |
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.
This call mistakenly uses defaultChallengeTypes
for both challenge types and capabilities; replace the last parameter with defaultCapabilities
.
application = setupPCA(config, defaultChallengeTypes, defaultChallengeTypes) | |
application = setupPCA(config, defaultChallengeTypes, defaultCapabilities) |
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.
Done.
package com.microsoft.identity.nativeauth.parameters | ||
|
||
|
||
class NativeAuthConfigParameters ( |
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 about NativeAuthPublicClientApplicationParameters.
Better to mention the public client application and do not mention the word configuration. This is the parameters to create the native auth public client application, not the configuration
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 make the path under the nativeauth folder.
/** | ||
* The authorityUrl to be used for the authority. | ||
*/ | ||
var authorityUrl: String, |
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.
can this be val?
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.
package com.microsoft.identity.nativeauth.parameters | ||
|
||
|
||
class NativeAuthConfigParameters ( |
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.
Can you add the "public" keyword?
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.
@@ -991,17 +995,18 @@ public static INativeAuthPublicClientApplication createNativeAuthPublicClientApp | |||
*/ | |||
public static INativeAuthPublicClientApplication createNativeAuthPublicClientApplication( | |||
@NonNull final Context context, | |||
@NonNull final NativeAuthPublicClientApplicationConfiguration config) throws MsalException { | |||
@NonNull final NativeAuthConfigParameters configParameters) throws MsalException { |
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.
Similarly to other comments
@NonNull final NativeAuthConfigParameters configParameters) throws MsalException { | |
@NonNull final NativeAuthConfigParameters parameters) throws MsalException { |
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.
validateNonNullArgument(context, NONNULL_CONSTANTS.CONTEXT); | ||
validateNonNullArgument(config, NONNULL_CONSTANTS.CONFIG); | ||
validateNonNullArgument(parameters, NONNULL_CONSTANTS.CONFIG_PARAMETER); |
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.
Does this constant need to match with the name of the parameter that is "parameters" ?
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. Name it as CLIENT_PARAMETER
7d5d10a
into
feature/backward-compatibility
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
Context:
Deal with a comment under
#2305