Skip to content

Commit

Permalink
Return invalid username error instead of unknown error (#2020)
Browse files Browse the repository at this point in the history
If the username is empty or malformed then a InvalidUsername error is returned instead of UnknownError

Corresponding PR in common repo:
AzureAD/microsoft-authentication-library-common-for-android#2297
  • Loading branch information
SaurabhMSFT authored Jan 29, 2024
1 parent 7ebbe76 commit 56fd7d2
Show file tree
Hide file tree
Showing 10 changed files with 88 additions and 29 deletions.
2 changes: 1 addition & 1 deletion common
Submodule common updated 25 files
+3 −0 changelog.txt
+0 −1 common/build.gradle
+15 −5 common/src/main/java/com/microsoft/identity/common/adal/internal/AuthenticationConstants.java
+1 −0 common/src/main/java/com/microsoft/identity/common/internal/broker/ipc/AccountManagerAddAccountStrategy.java
+215 −0 .../microsoft/identity/common/internal/broker/ipc/AccountManagerBackupIpcStrategyTargetingSpecificBrokerApp.kt
+9 −2 common/src/main/java/com/microsoft/identity/common/internal/broker/ipc/BrokerOperationBundle.java
+5 −1 common/src/main/java/com/microsoft/identity/common/internal/controllers/BrokerMsalController.java
+12 −7 common/src/main/java/com/microsoft/identity/common/internal/util/AccountManagerUtil.java
+8 −0 common/src/main/java/com/microsoft/identity/common/logging/Logger.java
+2 −2 common/src/main/java/com/microsoft/identity/common/nativeauth/internal/controllers/NativeAuthMsalController.kt
+254 −0 ...rosoft/identity/common/internal/broker/ipc/AccountManagerBackupIpcStrategyTargetingSpecificBrokerAppTest.kt
+1 −1 ...m/microsoft/identity/common/internal/providers/microsoft/nativeauth/integration/SignUpOAuth2StrategyTest.kt
+0 −1 common4j/build.gradle
+6 −2 common4j/src/main/com/microsoft/identity/common/java/logging/Logger.java
+10 −0 ...on4j/src/main/com/microsoft/identity/common/java/nativeauth/controllers/results/INativeAuthCommandResult.kt
+0 −9 common4j/src/main/com/microsoft/identity/common/java/nativeauth/controllers/results/SignUpCommandResult.kt
+0 −1 ...com/microsoft/identity/common/java/nativeauth/providers/requests/resetpassword/ResetPasswordStartRequest.kt
+0 −1 ...j/src/main/com/microsoft/identity/common/java/nativeauth/providers/requests/signin/SignInInitiateRequest.kt
+0 −1 ...on4j/src/main/com/microsoft/identity/common/java/nativeauth/providers/requests/signup/SignUpStartRequest.kt
+3 −5 ...src/main/com/microsoft/identity/common/java/nativeauth/providers/responses/signup/SignUpStartApiResponse.kt
+1 −1 ...j/src/main/com/microsoft/identity/common/java/nativeauth/providers/responses/signup/SignUpStartApiResult.kt
+1 −1 common4j/src/main/com/microsoft/identity/common/java/nativeauth/util/ApiErrorResponseUtil.kt
+20 −0 common4j/src/test/com/microsoft/identity/common/java/logging/LoggerTest.java
+8 −8 common4j/src/test/com/microsoft/identity/common/java/nativeauth/providers/NativeAuthRequestHandlerTest.kt
+1 −2 common4j/src/test/com/microsoft/identity/common/java/nativeauth/providers/NativeAuthResponseHandlerTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,14 @@ class NativeAuthPublicClientApplication(

verifyNoUserIsSignedIn()

if (username.isBlank()) {
return@withContext SignInError(
errorType = ErrorTypes.INVALID_USERNAME,
errorMessage = "Empty or blank username",
correlationId = "UNSET"
)
}

val hasPassword = password?.isNotEmpty() == true

val params =
Expand Down Expand Up @@ -357,6 +365,15 @@ class NativeAuthPublicClientApplication(
channel = result.challengeChannel
)
}
is INativeAuthCommandResult.InvalidUsername -> {
SignInError(
errorType = ErrorTypes.INVALID_USERNAME,
errorMessage = result.errorDescription,
error = result.error,
correlationId = result.correlationId,
errorCodes = result.errorCodes
)
}
is SignInCommandResult.PasswordRequired -> {
if (hasPassword) {
Logger.warn(
Expand Down Expand Up @@ -491,6 +508,14 @@ class NativeAuthPublicClientApplication(
)
}

if (username.isBlank()) {
return@withContext SignUpError(
errorType = ErrorTypes.INVALID_USERNAME,
errorMessage = "Empty or blank username",
correlationId = "UNSET"
)
}

val parameters =
CommandParametersAdapter.createSignUpStartCommandParameters(
nativeAuthConfig,
Expand Down Expand Up @@ -604,9 +629,9 @@ class NativeAuthPublicClientApplication(
)
}

is SignUpCommandResult.InvalidEmail -> {
is INativeAuthCommandResult.InvalidUsername -> {
SignUpError(
errorType = SignUpErrorTypes.INVALID_USERNAME,
errorType = ErrorTypes.INVALID_USERNAME,
error = result.error,
errorMessage = result.errorDescription,
correlationId = result.correlationId
Expand Down Expand Up @@ -687,6 +712,14 @@ class NativeAuthPublicClientApplication(
)
}

if (username.isBlank()) {
return@withContext ResetPasswordError(
errorType = ErrorTypes.INVALID_USERNAME,
errorMessage = "Empty or blank username",
correlationId = "UNSET"
)
}

val parameters = CommandParametersAdapter.createResetPasswordStartCommandParameters(
nativeAuthConfig,
nativeAuthConfig.oAuth2TokenCache,
Expand Down Expand Up @@ -724,6 +757,16 @@ class NativeAuthPublicClientApplication(
)
}

is INativeAuthCommandResult.InvalidUsername -> {
ResetPasswordError(
errorType = ErrorTypes.INVALID_USERNAME,
errorMessage = result.errorDescription,
error = result.error,
correlationId = result.correlationId,
errorCodes = result.errorCodes
)
}

is INativeAuthCommandResult.UnknownError -> {
ResetPasswordError(
error = result.error,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,12 @@ internal class ErrorTypes () {
* The password should be re-submitted.
*/
const val INVALID_PASSWORD = "invalid_password"

/*
* The INVALID_USERNAME value indicates the username provided by the user is not acceptable to the server.
* If this occurs, the flow should be restarted.
*/
const val INVALID_USERNAME = "invalid_username"
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ class ResetPasswordError(
override var exception: Exception? = null
): ResetPasswordResult, ResetPasswordStartResult, BrowserRequiredError, Error(errorType = errorType, error = error, errorMessage= errorMessage, correlationId = correlationId, errorCodes = errorCodes, exception = exception) {
fun isUserNotFound() : Boolean = this.errorType == ErrorTypes.USER_NOT_FOUND

fun isInvalidUsername(): Boolean = this.errorType == ErrorTypes.INVALID_USERNAME
}

/**
Expand Down Expand Up @@ -87,4 +89,6 @@ class ResetPasswordSubmitPasswordError(
fun isInvalidPassword() : Boolean = this.errorType == ErrorTypes.INVALID_PASSWORD

fun isPasswordResetFailed() : Boolean = this.errorType == ResetPasswordErrorTypes.PASSWORD_RESET_FAILED

fun isInvalidUsername() : Boolean = this.errorType == ErrorTypes.INVALID_USERNAME
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ open class SignInError(
fun isUserNotFound(): Boolean = this.errorType == ErrorTypes.USER_NOT_FOUND

fun isInvalidCredentials(): Boolean = this.errorType == SignInErrorTypes.INVALID_CREDENTIALS

fun isInvalidUsername(): Boolean = this.errorType == ErrorTypes.INVALID_USERNAME
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,6 @@ internal class SignUpErrorTypes {
*/
const val USER_ALREADY_EXISTS = "user_already_exists"

/* The INVALID_USERNAME value indicates the username provided by the user is not acceptable to the server.
* If this occurs, the flow should be restarted.
*/
const val INVALID_USERNAME = "invalid_username"

/*
* The INVALID_ATTRIBUTES value indicates one or more attributes that were sent failed input validation.
* The attributes should be resubmitted.
Expand Down Expand Up @@ -80,7 +75,7 @@ open class SignUpError (

fun isUserAlreadyExists(): Boolean = this.errorType == SignUpErrorTypes.USER_ALREADY_EXISTS

fun isInvalidUsername(): Boolean = this.errorType == SignUpErrorTypes.INVALID_USERNAME
fun isInvalidUsername(): Boolean = this.errorType == ErrorTypes.INVALID_USERNAME

fun isInvalidAttributes(): Boolean = this.errorType == SignUpErrorTypes.INVALID_ATTRIBUTES

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

package com.microsoft.identity.nativeauth.statemachine.results

import com.microsoft.identity.nativeauth.statemachine.errors.ErrorTypes
import com.microsoft.identity.nativeauth.statemachine.states.ResetPasswordCodeRequiredState
import com.microsoft.identity.nativeauth.statemachine.states.ResetPasswordPasswordRequiredState
import com.microsoft.identity.nativeauth.statemachine.states.SignInContinuationState
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import com.microsoft.identity.nativeauth.NativeAuthPublicClientApplicationConfig
import com.microsoft.identity.nativeauth.UserAttributes
import com.microsoft.identity.client.exception.MsalException
import com.microsoft.identity.client.internal.CommandParametersAdapter
import com.microsoft.identity.nativeauth.statemachine.results.SignInResult
import com.microsoft.identity.nativeauth.statemachine.results.SignUpResendCodeResult
import com.microsoft.identity.nativeauth.statemachine.results.SignUpResult
import com.microsoft.identity.nativeauth.statemachine.results.SignUpSubmitAttributesResult
Expand Down Expand Up @@ -438,7 +437,7 @@ class SignUpPasswordRequiredState internal constructor(
}

// This should be caught earlier in the flow, so throwing UnexpectedError
is SignUpCommandResult.InvalidEmail -> {
is INativeAuthCommandResult.InvalidUsername -> {
Logger.warn(
TAG,
"Submit password received unexpected result: $result"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1117,10 +1117,6 @@ public void onError(@NonNull BaseException exception) {
/**
* Check that we don't get a type casting exception thrown when we get it,
* should get error result instead.
*
* @throws ExecutionException
* @throws InterruptedException
* @throws TimeoutException
*/
@Test
public void testSignInEmptyUsernameNoException() throws ExecutionException, InterruptedException, TimeoutException {
Expand Down Expand Up @@ -1159,6 +1155,7 @@ public void onError(@NonNull BaseException exception) {

SignInError error = (SignInError)result;

assertTrue(error.isInvalidUsername());
assertFalse(error.isBrowserRequired());
assertFalse(error.isUserNotFound());
assertFalse(error.isInvalidCredentials());
Expand Down Expand Up @@ -1710,8 +1707,7 @@ public void testSSPREmptyUsernameNoException() throws ExecutionException, Interr
ResetPasswordStartResult resetPasswordResult = resetPasswordStartTestCallback.get();

assertTrue(resetPasswordResult instanceof ResetPasswordError);
assertFalse(((ResetPasswordError) resetPasswordResult).isBrowserRequired());
assertFalse(((ResetPasswordError) resetPasswordResult).isUserNotFound());
assertTrue(((ResetPasswordError) resetPasswordResult).isInvalidUsername());
}

// Helper methods
Expand Down Expand Up @@ -2650,9 +2646,9 @@ public void testSignUpEmptyUsernameNoException() throws ExecutionException, Inte
SignUpResult signUpResult = signUpTestCallback.get();

assertTrue(signUpResult instanceof SignUpError);
assertTrue(((SignUpError) signUpResult).isInvalidUsername());
assertFalse(((SignUpError) signUpResult).isBrowserRequired());
assertFalse(((SignUpError) signUpResult).isInvalidPassword());
assertFalse(((SignUpError) signUpResult).isInvalidUsername());
assertFalse(((SignUpError) signUpResult).isInvalidAttributes());
assertFalse(((SignUpError) signUpResult).isUserAlreadyExists());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ import com.microsoft.identity.common.java.interfaces.IPlatformComponents
import com.microsoft.identity.common.java.nativeauth.BuildValues
import com.microsoft.identity.common.java.util.ResultFuture
import com.microsoft.identity.internal.testutils.TestUtils
import com.microsoft.identity.nativeauth.statemachine.errors.ErrorTypes
import com.microsoft.identity.nativeauth.statemachine.states.SignInContinuationState
import com.microsoft.identity.nativeauth.statemachine.errors.SignInContinuationError
import kotlinx.coroutines.ExperimentalCoroutinesApi
Expand Down Expand Up @@ -1261,10 +1262,6 @@ class NativeAuthPublicClientApplicationKotlinTest : PublicClientApplicationAbstr
/**
* Check that we don't get a type casting exception thrown when we get it,
* should get error result instead.
*
* @throws ExecutionException
* @throws InterruptedException
* @throws TimeoutException
*/
@Test
fun testSignInEmptyUsernameNoException() = runTest {
Expand All @@ -1276,7 +1273,7 @@ class NativeAuthPublicClientApplicationKotlinTest : PublicClientApplicationAbstr

val result = application.signIn(emptyString, password)
assertTrue(result is SignInError)
assertTrue((result as SignInError).errorType == null)
assertTrue((result as SignInError).errorType == ErrorTypes.INVALID_USERNAME)
}

// Helper methods
Expand Down Expand Up @@ -2067,10 +2064,6 @@ class NativeAuthPublicClientApplicationKotlinTest : PublicClientApplicationAbstr
/**
* Check that we don't get a type casting exception thrown when we get it,
* should get error result instead.
*
* @throws ExecutionException
* @throws InterruptedException
* @throws TimeoutException
*/
@Test
fun testSignUpNullUsernameNoException() = runTest {
Expand All @@ -2082,10 +2075,30 @@ class NativeAuthPublicClientApplicationKotlinTest : PublicClientApplicationAbstr
responseType = MockApiResponseType.CHALLENGE_TYPE_REDIRECT
)

// 1b. Call SDK interface
//Call SDK interface
val result = application.signUp(emptyString)
assertTrue(result is SignUpError)
assertTrue((result as SignUpError).errorType == null)
assertTrue((result as SignUpError).errorType == ErrorTypes.INVALID_USERNAME)
}

/**
* Check that we don't get a type casting exception thrown when we get it,
* should get error result instead.
*/
@Test
fun testResetPasswordNullUsernameNoException() = runTest {
val correlationId = UUID.randomUUID().toString()

configureMockApi(
endpointType = MockApiEndpoint.SSPRStart,
correlationId = correlationId,
responseType = MockApiResponseType.CHALLENGE_TYPE_REDIRECT
)

// 1b. Call SDK interface
val result = application.resetPassword(emptyString)
assertTrue(result is ResetPasswordError)
assertTrue((result as ResetPasswordError).errorType == ErrorTypes.INVALID_USERNAME)
}

@Test
Expand Down

0 comments on commit 56fd7d2

Please sign in to comment.