MFA business logic #2160
Merged
MFA business logic #2160
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…iness-logic # Conflicts: # msal/src/main/java/com/microsoft/identity/nativeauth/NativeAuthPublicClientApplication.kt # msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/errors/SignInErrors.kt # msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/MFAStates.kt # msal/src/test/java/com/microsoft/identity/client/e2e/tests/network/nativeauth/SignInTest.kt
rmccahill
reviewed
Aug 15, 2024
msal/src/main/java/com/microsoft/identity/client/internal/CommandParametersAdapter.java
Show resolved
Hide resolved
rmccahill
reviewed
Aug 15, 2024
msal/src/main/java/com/microsoft/identity/client/internal/CommandParametersAdapter.java
Show resolved
Hide resolved
rmccahill
reviewed
Aug 15, 2024
msal/src/main/java/com/microsoft/identity/client/internal/CommandParametersAdapter.java
Show resolved
Hide resolved
nilo-ms
reviewed
Aug 15, 2024
| @@ -68,6 +70,11 @@ interface SignInResult : Result { | |||
| class PasswordRequired( | |||
| override val nextState: SignInPasswordRequiredState | |||
| ) : SignInResult, Result.SuccessResult(nextState = nextState) | |||
|
|
|||
|
|
|||
| class MFARequired( | |||
There was a problem hiding this comment.
Should this class be called MFARequiredResult?
There was a problem hiding this comment.
No. Our structure is Result., e.g. SignInResult.PasswordRequired. In this case it's SignInResult.MFARequired.
nilo-ms
reviewed
Aug 15, 2024
| * @param callback [com.microsoft.identity.nativeauth.statemachine.states.MFARequiredState.SendChallengeCallback] to receive the result on. | ||
| * @return The result of the send challenge action. | ||
| */ | ||
| fun sendChallenge(authMethodId: String? = null, callback: SendChallengeCallback) { |
There was a problem hiding this comment.
Why not consider using the authMethod instance as a parameter instead of the String id? There don't seem to be significant drawbacks, and the advantages include not having to validate the authMethodId since it's exclusively created by the SDK.
nilo-ms
reviewed
Aug 15, 2024
| /** | ||
| * RequiredUserAttribute represents details about the account attributes required by the server. | ||
| */ | ||
| data class AuthMethod( |
There was a problem hiding this comment.
nit: I don't know if you align file name to class name, but the file name has an "s" while the class name doesn't
# Conflicts: # msal/src/main/java/com/microsoft/identity/nativeauth/AuthMethods.kt # msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/errors/MFAErrors.kt # msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/results/MFAResult.kt # msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/results/SignInResult.kt # msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/MFAStates.kt # msal/src/test/java/com/microsoft/identity/client/e2e/tests/network/nativeauth/SignInTest.kt
Yuki-YuXin
approved these changes
Aug 16, 2024
Yuki-YuXin
reviewed
Aug 23, 2024
| * | ||
| * @param nextState [com.microsoft.identity.nativeauth.statemachine.states.MFARequiredState] the current state of the flow with follow-on methods. | ||
| * @param authMethods the authentication methods that can be used to complete the challenge flow. | ||
| */ | ||
| class SelectionRequired( | ||
| override val nextState: MFARequiredState, | ||
| val authMethods: List<AuthMethod> |
There was a problem hiding this comment.
Can we make auth method Parcelable?
SammyO
commented
Aug 27, 2024
| mockCorrelationId(nextState, correlationId); | ||
|
|
||
| AwaitingMFAStateSendChallengeTestCallback sendChallengeCallback = new AwaitingMFAStateSendChallengeTestCallback(); | ||
| nextState.sendChallenge(sendChallengeCallback); |
There was a problem hiding this comment.
rename to requestChallenge()
# Conflicts: # msal/src/main/java/com/microsoft/identity/client/internal/CommandParametersAdapter.java # msal/src/test/java/com/microsoft/identity/client/e2e/tests/network/nativeauth/SignInEmailPasswordTest.kt # msal/src/test/java/com/microsoft/identity/nativeauth/NativeAuthPublicClientApplicationJavaTest.java
rmccahill
approved these changes
Aug 28, 2024
SammyO
added a commit
to AzureAD/microsoft-authentication-library-common-for-android
that referenced
this pull request
Aug 28, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requires #2158 to be merged first.
MSAL common PR: AzureAD/microsoft-authentication-library-common-for-android#2474