Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Username checking in native auth requests #2385

Closed
wants to merge 3 commits into from
Closed

Username checking in native auth requests #2385

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f8ad1d8
username checking in requests
Yuki-YuXin Apr 16, 2024
e165b63
Add test
Yuki-YuXin Apr 16, 2024
7d66139
Fix tests related with requests username checking
Yuki-YuXin Apr 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ...tity/common/java/nativeauth/providers/requests/resetpassword/ResetPasswordStartRequest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ data class ResetPasswordStartRequest private constructor(
headers: Map<String, String?>
): ResetPasswordStartRequest {
// Check for empty Strings and empty Maps
ArgUtils.validateNonNullArg(username, "username")
Copy link
Contributor

@SammyO SammyO Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this removed in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the reason but I would prefer to keep the checking here.

ArgUtils.validateNonNullArg(clientId, "clientId")
ArgUtils.validateNonNullArg(challengeType, "challengeType")
ArgUtils.validateNonNullArg(requestUrl, "requestUrl")
Expand Down
1 change: 1 addition & 0 deletions ...rosoft/identity/common/java/nativeauth/providers/requests/signin/SignInInitiateRequest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ data class SignInInitiateRequest private constructor(
headers: Map<String, String?>
): SignInInitiateRequest {
// Check for empty Strings and empty Maps
ArgUtils.validateNonNullArg(username, "username")
ArgUtils.validateNonNullArg(clientId, "clientId")
ArgUtils.validateNonNullArg(challengeType, "challengeType")
ArgUtils.validateNonNullArg(requestUrl, "requestUrl")
Expand Down
1 change: 1 addition & 0 deletions ...microsoft/identity/common/java/nativeauth/providers/requests/signup/SignUpStartRequest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ data class SignUpStartRequest private constructor(
headers: Map<String, String?>
): SignUpStartRequest {
// Check for empty Strings and empty Maps
ArgUtils.validateNonNullArg(username, "username")
ArgUtils.validateNonNullArg(clientId, "clientId")
ArgUtils.validateNonNullArg(challengeType, "challengeType")
ArgUtils.validateNonNullArg(requestUrl, "requestUrl")
Expand Down
14 changes: 7 additions & 7 deletions ...t/com/microsoft/identity/common/java/nativeauth/providers/NativeAuthRequestHandlerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,8 @@ class NativeAuthRequestHandlerTest {
)

// signup start tests
@Test
fun testSignUpStartWithEmptyUsernameShouldNotThrowException() {
@Test(expected = ClientException::class)
fun testSignUpStartWithEmptyUsernameShouldThrowException() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't this changed (i.e. the exception removed) as part of last year's work to align some of our errors and return an InvalidUsernameError in the case of an empty string? Giannis worked on that I believe.

Copy link
Contributor Author

@Yuki-YuXin Yuki-YuXin Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I think so. But either CommandResultUtil.kt

if (this.status != ICommandResult.ResultStatus.COMPLETED) {
        var exception: Exception? = null
        var exceptionMessage: String? = ""

        if (this.result is Exception) {
            exception = this.result as Exception
            exceptionMessage = exception.message
        }

        return com.microsoft.identity.common.java.nativeauth.controllers.results.INativeAuthCommandResult.UnknownError(
            error = UNSUCCESSFUL_COMMAND_ERROR,
            errorDescription = exceptionMessage,
            exception = exception,
            correlationId = this.correlationId
        ) as ExpectedType

or NativeAuthPublicClientApplication.kt

return withContext(Dispatchers.IO) {
            try {
                verifyNoUserIsSignedIn()

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

can handle empty username.

I think the latter one act the main role of returning InvalidUsernameError before sending out the request/validating request parameters. Thus, I would prefer to keep the ThrowException here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some thoughts:

  • if we're already doing a username is empty check in the interface, we don't need to do it again in the API layer.
  • we shouldn't just be throwing exceptions. An exception is a pretty heavy mechanism to give feedback to a developer. We should aim for something more graceful than that.
  • the fact that this "throwing an exception" was removed from this API layer a few months ago, means we had a reason why. If that reason has changed, let's talk about it. We can just revert the work we did a few months ago without good reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will abandon this PR.

val commandParameters = SignUpStartCommandParameters.builder()
.platformComponents(mock<PlatformComponents>())
.username(emptyString)
Expand Down Expand Up @@ -352,8 +352,8 @@ class NativeAuthRequestHandlerTest {
}

// signin tests
@Test
fun testSignInInitiateWithEmptyUsernameShouldNotThrowException() {
@Test(expected = ClientException::class)
fun testSignInInitiateWithEmptyUsernameShouldThrowException() {
val commandParameters = SignInStartCommandParameters.builder()
.platformComponents(mock<PlatformComponents>())
.username(emptyString)
Expand Down Expand Up @@ -485,7 +485,7 @@ class NativeAuthRequestHandlerTest {
)
}

@Test
@Test(expected = ClientException::class)
fun testSignInInitiateWithPasswordCommandParametersWithEmptyUsernameShouldNotThrowException() {
val commandParameters = SignInStartCommandParameters.builder()
.platformComponents(mock<PlatformComponents>())
Expand Down Expand Up @@ -644,8 +644,8 @@ class NativeAuthRequestHandlerTest {
}

// sspr start tests
@Test
fun testResetPasswordStartWithEmptyUsernameShouldNotThrowException() {
@Test(expected = ClientException::class)
fun testResetPasswordStartWithEmptyUsernameShouldThrowException() {
val commandParameters = ResetPasswordStartCommandParameters.builder()
.platformComponents(mock<PlatformComponents>())
.username(emptyString)
Expand Down
Loading