Change the signature of signup/signin methods #2010
Conversation
|
|
||
| /** | ||
| * Sign up the account starting from a username; callback variant. | ||
| * | ||
| * @param username username of the account to sign up. | ||
| * @param password password of the account to sign up. |
There was a problem hiding this comment.
let's make it explicit that this is optional, like we do for attributes.
| scopes: List<String>? | ||
| ): SignInUsingPasswordResult { | ||
| ): SignInResult { | ||
| LogSession.logMethodCall(TAG, "${TAG}.signInUsingPassword") | ||
| return withContext(Dispatchers.IO) { | ||
| LogSession.logMethodCall(TAG, "${TAG}.signInUsingPassword.withContext") |
There was a problem hiding this comment.
Update log (and perhaps also in other places)
| @NonNull final NativeAuthPublicClientApplicationConfiguration configuration, | ||
| @NonNull final OAuth2TokenCache tokenCache, | ||
| @NonNull final String username, | ||
| @NonNull final char[] password, | ||
| @Nullable final char[] password, |
There was a problem hiding this comment.
update comment (line 373) to reflect the fact that password is optional
There was a problem hiding this comment.
The password is nullable but is still required as Java does not support default values. Developers have to specify all parameters.
There was a problem hiding this comment.
This class is only used internally, but Kotlin classes (our state machine). In those cases, this field is optional. We've done the same for e.g. scopes parameter in other methods.
There was a problem hiding this comment.
Java does not have optional parameters. Optional parameters are simulated in Java by overloading functions. As this parameter is in the middle, it wouldn't be good style to overload function with reduced set of parameters.
There was a problem hiding this comment.
My point is that this method isn't called from any Java class, and so we don't have to consider the non-optionality of this parameter. You can leave it like this however, that's also fine.
| @NonNull final NativeAuthPublicClientApplicationConfiguration configuration, | ||
| @NonNull final OAuth2TokenCache tokenCache, | ||
| @NonNull final String username, | ||
| @NonNull final char[] password, | ||
| @Nullable final char[] password, |
There was a problem hiding this comment.
The password is nullable but is still required as Java does not support default values. Developers have to specify all parameters.
| nativeAuthConfig, | ||
| nativeAuthConfig.oAuth2TokenCache, | ||
| username, | ||
| password, | ||
| attributes?.toMap() | ||
| attributes?.userAttributes |
There was a problem hiding this comment.
This is what it is on line 781 in the previous version of the file.
There was a problem hiding this comment.
It seems we had an inconsistency between signUp and signUpUsingPassword when it comes to this field.
Could you validate that attributes?.userAttributes is the right way of doing this, and not attributes?.toMap(), by doing some E2E testing.
There was a problem hiding this comment.
SignUpStates.submitAttribute still uses toMap(), so this inconsistency needs to be fixed. Let's make sure we're not introducing a last minute bug in this flow.
There was a problem hiding this comment.
attributes?.userAttributes and attributes?.toMap() return the same value. I have changed the usage here to attributes?.toMap()
...c/test/java/com/microsoft/identity/nativeauth/NativeAuthPublicClientApplicationJavaTest.java
Outdated
Show resolved
Hide resolved
...c/test/java/com/microsoft/identity/nativeauth/NativeAuthPublicClientApplicationJavaTest.java
Outdated
Show resolved
Hide resolved
...c/test/java/com/microsoft/identity/nativeauth/NativeAuthPublicClientApplicationJavaTest.java
Outdated
Show resolved
Hide resolved
msal/src/main/java/com/microsoft/identity/nativeauth/NativeAuthPublicClientApplication.kt
Outdated
Show resolved
Hide resolved
msal/src/main/java/com/microsoft/identity/nativeauth/NativeAuthPublicClientApplication.kt
Outdated
Show resolved
Hide resolved
The signIn and signInUsingPassword methods have been refactored into one signIn method with optional password. The signUp and signUpUsingPassword methods have been refactored into one signUp method with optional password. PR for the MSAL repo is AzureAD/microsoft-authentication-library-for-android#2010
| @@ -414,24 +280,26 @@ class NativeAuthPublicClientApplication( | |||
| * Sign in the account using username and password; Kotlin coroutines variant. | |||
| * | |||
| * @param username username of the account to sign in. | |||
| * @param password password of the account to sign in. | |||
| * @param password (Optional) password of the account to sign in. | |||
There was a problem hiding this comment.
This needs to be removed, as the optionality doesn't apply to Java call sites. Applies in different places in the code.
There was a problem hiding this comment.
The place where the optionality doesnot apply is in an internal package and not used from the Sample app. Optional in the doc should stay here as it provides meaningful information to the developer.
The signIn and signInUsingPassword methods have been refactored into one signIn method with optional password.
The signUp and signUpUsingPassword methods have been refactored into one signUp method with optional password.
PR for the common repo is AzureAD/microsoft-authentication-library-common-for-android#2284