-
Notifications
You must be signed in to change notification settings - Fork 123
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.
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
Add ClientException wrapper for native auth #2080
Conversation
…nError in getAccessToken
msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt
Outdated
Show resolved
Hide resolved
msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt
Outdated
Show resolved
Hide resolved
… signout and NativeAuthPublicClientApplication.kt getCurrentAccount as they have no associated errors.
This reverts commit 7edaee8.
This reverts commit 7b1994e.
This reverts commit cb3f621.
# Conflicts: # msal/src/main/java/com/microsoft/identity/nativeauth/NativeAuthPublicClientApplication.kt # msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt # msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/ResetPasswordStates.kt # msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/SignInStates.kt # msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/SignUpStates.kt
correlationId = result.correlationId | ||
) | ||
} | ||
// This should be caught earlier in the flow, so throwing UnexpectedError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: let's update this comment: // This should be caught earlier in the flow, so returning generic error
...c/test/java/com/microsoft/identity/nativeauth/NativeAuthPublicClientApplicationJavaTest.java
Show resolved
Hide resolved
...c/test/java/com/microsoft/identity/nativeauth/NativeAuthPublicClientApplicationKotlinTest.kt
Show resolved
Hide resolved
val submitCodeState = spy((result as SignUpResult.CodeRequired).nextState) | ||
val submitCodeResult = submitCodeState.submitCode(emptyString) // Empty code will trigger ArgUtils.validateNonNullArg(oob, "oob") of the SignUpContinueRequest to thrown ClientException | ||
assertTrue(submitCodeResult is SubmitCodeError) | ||
assertTrue((submitCodeResult as SubmitCodeError).error.equals(ErrorTypes.UNSUCCESSFUL_COMMAND_ERROR)) // ClientException will be caught in CommandResultUtil.kt and converted to generic error in interface layer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is ErrorTypes.UNSUCCESSFUL_COMMAND_ERROR
set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It set under CommandResultUtil.kt const val UNSUCCESSFUL_COMMAND_ERROR = "unsuccessful_command"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think those changes are pushed, I don't see them in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this PR also introduces const val UNSUCCESSFUL_COMMAND_ERROR = "unsuccessful_command"
in Error.kt. Is that one used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't used in the SDK code base because this PR just adds try catch block with ErrorType = "client_exception".
ErryType = "unsuccessful_command" is defined wrapping exceptions in common repo under CommandResultUtil.
However, the existing "unsuccessful_command" has some difference from "client_exception" in my opinion. They can't replace each other.
"unsuccessful_command" is used in the appending test testEmptyRequestParametersToGenericErrorNotThrownException() and will be displayed in the sample app to user SubmitError(error= "unsuccessful_command").
We cannot use assertTrue((submitCodeResult as SubmitCodeError).error.equals(CommandResultUtil.UNSUCCESSFUL_COMMAND_ERROR))
instead of assertTrue((submitCodeResult as SubmitCodeError).error.equals(ErrorTypes.UNSUCCESSFUL_COMMAND_ERROR))
in the NativeAuthPublicClientApplicationKotlinTest
We can either remove testEmptyRequestParametersToGenericErrorNotThrownException() test or remove the introduction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Offline discussion conclusion: Use direct string in the test file instead of adding new definition in the code base.
…st file. remove useless import in the test file as well.
Goal:
The SDK currently throws Exceptions in some flows (e.g. ClientException when an empty username is passed, refresh token exceptions, etc.). We should refactor this and replace it with exceptions being wrapped into an Error class and returned as a normal SDK response. Calling applications shouldn't crash, and shouldn't require try-catch blocks.
This will also involve updating the sample app (removing try-catch surrounding SDK methods being called)
Investigation summary:
BaseNativeAuthController throws ClientException. They could only be called in NativeAuthMsalController and we didn't.
NativeAuthMsalController throws IOException::class, ClientException::class, ServiceException::class ArgumentException::class, ServiceException::class. mainly from acquireTokenSilent & renewAT used by AcquireTokenNoFixedScopesCommand (accountState.getAccessToken). It will be handled by
is Exception
in getAccessToken`AcquireTokenNoFixedScopesCommandParameters throws ArgumentException -> this parameter will be delted
Request throws ClientException -> NativeAuthRequestHandlerTest include
verifyNoUserIsSignedIn() throws MsalClientException
Changes summary:
Company PRs:
native sample app: Azure-Samples/ms-identity-ciam-native-auth-android-sample#24