SignIn code for Native Auth #1919

SaurabhMSFT · 2023-10-10T12:02:37Z

This PR includes the work for SignIn relates features of Native Auth

Companion PR in MSAL Common repo is AzureAD/microsoft-authentication-library-common-for-android#2207

msal/src/main/java/com/microsoft/identity/client/INativeAuthPublicClientApplication.kt

msal/src/main/java/com/microsoft/identity/client/NativeAuthPublicClientApplication.kt

msal/src/main/java/com/microsoft/identity/client/statemachine/results/SignInResult.kt

shahzaibj · 2023-10-17T23:20:18Z

msal/src/main/java/com/microsoft/identity/client/NativeAuthPublicClientApplication.kt

+            if (cacheRecords.isNullOrEmpty()) {
+                return null
+            }
+            val account = AccountAdapter.adapt(cacheRecords)


Sounds like this is an accountList as opposed to an account, right? So let's name it appropriately

shahzaibj · 2023-10-17T23:42:37Z

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

@@ -250,6 +283,37 @@ public static DeviceCodeFlowCommandParameters createDeviceCodeFlowWithClaimsComm
        return commandParameters;
    }

+    public static AcquireTokenNoFixedScopesCommandParameters createAcquireTokenNoFixedScopesCommandParameters(


javadoc for all public methods

shahzaibj · 2023-10-17T23:42:59Z

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

@@ -51,6 +82,8 @@
 import java.util.List;
 import java.util.Map;

+import androidx.annotation.Nullable;


Probably run Optimize imports once on all files in the PR

shahzaibj · 2023-10-17T23:43:40Z

msal/src/main/java/com/microsoft/identity/client/statemachine/results/BaseResults.kt

+import com.microsoft.identity.client.statemachine.Error
+import com.microsoft.identity.client.statemachine.states.State
+
+interface Result {


shahzaibj · 2023-10-17T23:45:15Z

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

@@ -82,7 +82,7 @@ class PasswordIncorrectError(
    override val errorMessage: String,
    override val correlationId: String,
    override val errorCodes: List<Int>
-) : GeneralError(errorMessage = errorMessage, error = error, correlationId = correlationId, errorCodes = errorCodes)
+) : Error(errorMessage = errorMessage, error = error, correlationId = correlationId, errorCodes = errorCodes)


Why are there so many identical classes?

You can easily write one Error class and then use different Strings or enums to represent different error codes within that Error object

I will discuss it with the team but won't be able to make changes as part of this PR as it will be significant refactoring.

shahzaibj · 2023-10-17T23:48:22Z

msal/src/main/java/com/microsoft/identity/client/statemachine/results/SignInResult.kt

+     * @param error [com.microsoft.identity.client.statemachine.Error].
+     */
+    class UnexpectedError(override val error: Error) :
+        Result.ErrorResult(error = error),


Mentioned in another place about not having so many unnecessary duplicated classes, and instead one result or error class that can hold different error codes.

I will discuss it with the team but won't be able to make changes as part of this PR as it will be significant refactoring.

shahzaibj · 2023-10-17T23:50:09Z

msal/src/main/java/com/microsoft/identity/client/statemachine/states/BaseStates.kt

+        }
+    }
+
+    interface SignOutCallback : Callback<SignOutResult>


Also what's inside this interface? Seems to be empty

I mean I don't see any curly braces...so not sure if the below methods are in this interface or on the AccountResult class or perhaps on the parent interface i.e. BaseStates?

An interface in Kotlin doesn't need a body so thats why it works.

shahzaibj · 2023-10-17T23:51:09Z

msal/src/main/java/com/microsoft/identity/client/statemachine/states/BaseStates.kt

+
+sealed interface State
+
+abstract class BaseState(internal open val flowToken: String?)


shahzaibj · 2023-10-17T23:54:01Z

msal/src/main/java/com/microsoft/identity/client/statemachine/states/Callback.kt

+
+import com.microsoft.identity.common.java.exception.BaseException
+
+interface Callback <T> {


Javadoc everywhere

shahzaibj · 2023-10-17T23:54:57Z

msal/src/test/java/com/microsoft/identity/client/nativeauth/NativeAuthCIAMAuthorityTest.kt

@@ -0,0 +1,154 @@
+package com.microsoft.identity.client.nativeauth


shahzaibj · 2023-10-18T00:01:57Z

.../src/test/java/com/microsoft/identity/client/nativeauth/NativeAuthOAuth2ConfigurationTest.kt

@@ -0,0 +1,47 @@
+package com.microsoft.identity.client.nativeauth


Missing LICENSE

shahzaibj

Missing LICENSEs, Missing Javadocs, duplicated code around classes that hold Error and result objects

fadidurah · 2023-11-04T03:29:56Z

msal/src/main/java/com/microsoft/identity/client/statemachine/states/SignInStates.kt

@@ -266,7 +266,7 @@ class SignInPasswordRequiredState(
     * @param callback [com.microsoft.identity.client.statemachine.states.SignInPasswordRequiredState.SubmitPasswordCallback] to receive the result on.
     * @return The results of the submit password action.
     */
-    fun submitPassword(password: String, callback: SubmitPasswordCallback) {
+    fun submitPassword(password: CharArray, callback: SubmitPasswordCallback) {
        LogSession.logMethodCall(TAG, "${TAG}.submitPassword")
        NativeAuthPublicClientApplication.pcaScope.launch {
            try {


Not sure exactly where we need to add StringUtil.overwriteWithZero(params.password)

Every place password is passed as param? If so, then we need it in this method as well i think

Maybe right after we get the result

This password is coming from the calling application and it must be their responsibility to clear the password char array. As a rule, I am clearing password char arrays in the object we are creating.

shahzaibj · 2023-11-07T17:14:27Z

msal/src/main/java/com/microsoft/identity/client/NativeAuthPublicClientApplication.kt

+                    null
+                )
+                throw MsalClientException(
+                    MsalClientException.INVALID_PARAMETER,


Is INVALID_PARAMETER the right error in this case?

This is the most suitable error from existing codes.

shahzaibj · 2023-11-07T17:15:36Z

msal/src/main/java/com/microsoft/identity/client/NativeAuthPublicClientApplication.kt

+                    null
+                )
+                throw MsalClientException(
+                    MsalClientException.INVALID_PARAMETER,


Is INVALID_PARAMETER the right error in this case?

This is the most suitable error from existing codes.

shahzaibj · 2023-11-07T17:16:59Z

msal/src/main/java/com/microsoft/identity/client/NativeAuthPublicClientApplication.kt

+            val doesAccountExist = checkForPersistedAccount().get()
+            if (doesAccountExist) {
+                Logger.error(
+                    TAG,
+                    "An account is already signed in.",
+                    null
+                )
+                throw MsalClientException(
+                    MsalClientException.INVALID_PARAMETER,
+                    "An account is already signed in."
+                )
+            }


I saw this exact code block in another method - seems like an opportunity for code sharing

I have moved it to a common function.

shahzaibj · 2023-11-07T17:17:19Z

msal/src/main/java/com/microsoft/identity/client/exception/MsalUserNotFoundException.java

+
+package com.microsoft.identity.client.exception;
+
+public class MsalUserNotFoundException extends MsalException {


Perhaps you can simply leverage MsalServiceException with a new error code for user_not_found?

This file has been removed.

shahzaibj · 2023-11-07T17:21:41Z

msal/src/main/java/com/microsoft/identity/client/statemachine/states/BaseStates.kt

+     *
+     * @param callback [com.microsoft.identity.client.statemachine.states.AccountResult.SignOutCallback] to receive the result on.
+     */
+    fun signOut(callback: SignOutCallback) {


So signOut is a method on a base state..why?

Does this mean that you can sign out from any given state? What if the user was never even signed in to begin with?

If the user does not have a signed in account then we will throw MsalClientException.NO_CURRENT_ACCOUNT error.

Ok, to me it sounds like that signOut should only be accessible from a signedInState... is there a reason it has to stay on a base state?

@shahzaibj this method (as well as getAccessToken(), and other methods) are only available in the AccountResult state, which is practically the same as the SignInState you mention. They're not part of BaseState, this is just an interface.

The reason we've called it AccountResult rather than SignedInResult is because "account" makes more sense to us in this context: on an account object you can retrieve tokens, sign out (the act of signing out an account), etc. This was aligned with our PM.

I have moved the AccountResult to its own file so its clear that BaseStates interface doesn't have the signOut and getAccessToken method.

shahzaibj · 2023-11-07T17:22:41Z

msal/src/main/java/com/microsoft/identity/client/statemachine/states/SignInStates.kt

+import kotlinx.coroutines.withContext
+import java.io.Serializable
+
+class SignInCodeRequiredState internal constructor(


shahzaibj · 2023-11-07T17:22:48Z

msal/src/main/java/com/microsoft/identity/client/statemachine/states/SignInStates.kt

+) : BaseState(flowToken), State, Serializable {
+    private val TAG: String = SignInCodeRequiredState::class.java.simpleName
+
+    interface SubmitCodeCallback : Callback<SignInSubmitCodeResult>


shahzaibj · 2023-11-07T17:23:43Z

msal/src/main/java/com/microsoft/identity/client/statemachine/states/SignInStates.kt

+    }
+}
+
+class SignInPasswordRequiredState(


shahzaibj · 2023-11-07T17:24:31Z

msal/src/main/java/com/microsoft/identity/client/statemachine/states/SignInStates.kt

+    }
+}
+
+abstract class SignInAfterSignUpBaseState(


shahzaibj · 2023-11-09T00:37:56Z

msal/src/main/java/com/microsoft/identity/client/statemachine/states/BaseStates.kt

+     * @throws [MsalClientException] If the the account doesn't exist in the cache.
+     * @throws [ServiceException] If the refresh token doesn't exist in the cache/is expired, or the refreshing fails.
+     */
+    fun getAccessToken(forceRefresh: Boolean = false, callback: GetAccessTokenCallback) {


You can get access token from a state where you never signed in?

Same reply as for the other comment: this method is part of AccountResult, which is a result/state that is returned when you're signed in.

I have moved the AccountResult to its own file so its clear that BaseStates interface doesn't have the signOut and getAccessToken method.

This PR includes the work for SignIn relates features of Native Auth. Mockito-kotlin has been added as dependency for test projects in common and testutils project. Powermock has been added as a dependency to testutils project. Companion PR in MSAL repo is AzureAD/microsoft-authentication-library-for-android#1919

This PR is to merge the SignIn, SIgnup and SSPR code into dev branch. This code was previously reviewed in #1897, #1919, #1949 and #1950 . A technical overview of Native Auth is available at https://dev.azure.com/IdentityDivision/DevEx/_git/AuthLibrariesApiReview?path=/%5BAndroid%5D%20Native%20authentication/technical_overview.md&version=GBsammy/native-authentication&_a=preview A companion PR in common repo is AzureAD/microsoft-authentication-library-common-for-android#2233 --------- Co-authored-by: melissaahn <[email protected]> Co-authored-by: iamgusain <[email protected]> Co-authored-by: Dome Pongmongkol <[email protected]> Co-authored-by: Sowmya Malayanur <[email protected]> Co-authored-by: Mohit <[email protected]> Co-authored-by: tanmaymanolkar1 <[email protected]> Co-authored-by: amoyixi <[email protected]> Co-authored-by: yixilin <[email protected]> Co-authored-by: fadidurah <[email protected]> Co-authored-by: Dean Xu <[email protected]> Co-authored-by: Neha Goel <[email protected]> Co-authored-by: Dome Pongmongkol <[email protected]> Co-authored-by: pedro romero vargas <[email protected]>

Code for NativeAuth Ciam

0768b6e

github-actions bot added the msal label Oct 10, 2023

SaurabhMSFT mentioned this pull request Oct 10, 2023

SignIn features for Native Auth AzureAD/microsoft-authentication-library-common-for-android#2207

Merged

SaurabhMSFT marked this pull request as ready for review October 10, 2023 12:05

SaurabhMSFT requested a review from a team as a code owner October 10, 2023 12:05

fadidurah reviewed Oct 16, 2023

View reviewed changes

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

fadidurah reviewed Oct 16, 2023

View reviewed changes

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

fadidurah reviewed Oct 16, 2023

View reviewed changes

msal/src/main/java/com/microsoft/identity/client/NativeAuthPublicClientApplication.kt Outdated Show resolved Hide resolved

fadidurah reviewed Oct 16, 2023

View reviewed changes

msal/src/main/java/com/microsoft/identity/client/NativeAuthPublicClientApplication.kt Outdated Show resolved Hide resolved

fadidurah reviewed Oct 16, 2023

View reviewed changes

msal/src/main/java/com/microsoft/identity/client/statemachine/results/SignInResult.kt Outdated Show resolved Hide resolved

Add javadoc

1f107c9

shahzaibj reviewed Oct 17, 2023

View reviewed changes

shahzaibj reviewed Oct 18, 2023

View reviewed changes

shahzaibj requested changes Oct 18, 2023

View reviewed changes

SaurabhMSFT added 4 commits October 18, 2023 16:01

Add license and javadoc

1ba1321

Add javadoc

94aaee3

Refactor class name

c0cbf8f

Use char array instead of string

21e7c80

fadidurah reviewed Nov 4, 2023

View reviewed changes

SaurabhMSFT added 2 commits November 6, 2023 13:49

Use try finally

e524600

Added a comment for PBI

e79735a

shahzaibj reviewed Nov 7, 2023

View reviewed changes

fadidurah approved these changes Nov 7, 2023

View reviewed changes

SaurabhMSFT added 4 commits November 8, 2023 10:34

Merge branch 'ciam-feature' into saugautam/2642912

ae36dff

Fix build

ead4af7

More comments

0c8a4f0

Fix version file

8685f9e

shahzaibj reviewed Nov 9, 2023

View reviewed changes

Move AccountResult to its own file

bc3ef8c

shahzaibj approved these changes Nov 9, 2023

View reviewed changes

SaurabhMSFT merged commit 872c867 into ciam-feature Nov 9, 2023
9 checks passed

SaurabhMSFT deleted the saugautam/2642912 branch November 9, 2023 18:50

SaurabhMSFT mentioned this pull request Nov 9, 2023

Native Auth SignIn, Signup and SSPR features #1947

Merged


		sealed interface State

		abstract class BaseState(internal open val flowToken: String?)


		import com.microsoft.identity.common.java.exception.BaseException

		interface Callback <T> {

		@@ -0,0 +1,154 @@
		package com.microsoft.identity.client.nativeauth

		@@ -0,0 +1,47 @@
		package com.microsoft.identity.client.nativeauth


		package com.microsoft.identity.client.exception;

		public class MsalUserNotFoundException extends MsalException {

SignIn code for Native Auth #1919

SignIn code for Native Auth #1919

Conversation

SaurabhMSFT commented Oct 10, 2023 • 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

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

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

shahzaibj left a comment

Choose a reason for hiding this comment

fadidurah Nov 4, 2023 • 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

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

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

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 commented Oct 10, 2023 •

edited

Loading

fadidurah Nov 4, 2023 •

edited

Loading