-
Notifications
You must be signed in to change notification settings - Fork 136
Fix msal test app after merging with the fadi/release/5.4.0 #2118
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
Conversation
.../src/main/java/com/microsoft/identity/client/testapp/nativeauth/EmailSignInSignUpFragment.kt
Outdated
Show resolved
Hide resolved
fc422b6 to
d0b67ea
Compare
Fix the MSAL test app to support custom attribute test cases without the need to modify the source code. [ADO Ticket (2979358)](https://dev.azure.com/IdentityDivision/Engineering/_boards/board/t/DevExDub%20-B2C/Backlog%20items/?workitem=2979358) --------- Co-authored-by: Harold Karibiye <[email protected]>
# Conflicts: # common
changelog
Outdated
|
|
||
| Version 5.4.1-RC1 | ||
| --------- | ||
| - [PATCH] Update common @17.6.0-RC1 |
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.
This doesn't belong here. MSAL changelog is for changes that happened in MSAL.
msal/build.gradle
Outdated
| } | ||
| // Used for testfixtures | ||
| def common4jVersion = "1.0.+" | ||
| def common4jVersion = "14.6.0-RC1" |
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 this is right either? Neither the above.
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.
That might because I merged release branch into the test app branch in order to have both test environment. I will check the content in the dev branch and revert it back.
msal/versioning/version.properties
Outdated
| @@ -1,3 +1,3 @@ | |||
| #Wed Aug 01 15:24:11 PDT 2018 | |||
| versionName=5.4.0 | |||
| versionName=5.4.1-RC1 | |||
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.
This is not right either.
| val actionResult = authClient.signUp( | ||
| username = email, | ||
| password = password, | ||
| // password = password, |
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.
Please remove commented out code.
| username = email, | ||
| password = password | ||
| password = password, | ||
| scopes = listOf("User.Read") |
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'm not against it, but is it necessary?
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.
[use case 2.2.6] Ability to provide scope to control auth strength of the token
In this test case, we need to modify the code, providing scopes = listOf("User.Read") . If it had been there all along, the testers wouldn't have needed any modifications
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.
Got it.
| binding.resultAccessToken.text = | ||
| getString(R.string.result_access_token_text) + accessToken | ||
|
|
||
| Log.d("AccessToken", accessToken) |
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.
Why is this needed? If not, let's not log this. This is sensitive data and we should be setting an example.
| binding.resultAccessToken.text = | ||
| getString(R.string.result_access_token_text) + accessToken | ||
|
|
||
| Log.d("AccessToken", accessToken) |
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.
Same here
# Conflicts: # changelog # msal/versioning/version.properties
…es UI for email OTP attributes case
…es UI for email OTP attributes case
Fix update according to the following team feedback:
Batch2
[hero scenario 6, use case 2.2.1] Use email and OTP to get token and sign in
getParcelable(Constants.STATE)
[use case 2.2.6] Ability to provide scope to control auth strength of the token
Update the scope of profile to User.Read
Update the test app EmailSignInSignUp page: signIn(scopes = User.Read)
[use case 1.2.2] User is not registered with given email
Update test app EmailPasswordSignInSignUp page: displayDialog("Unexpected result", actionResult.errorMessage) in sign in function
[use case 1.2.6] Ability to provide scope to control auth strength of the token
Update the test app EmailPasswordSignInSignUp page: signIn(scopes = User.Read)
Update test app EmailPasswordSignInSignUp page: displayDialog("Unexpected result", actionResult.errorMessage) in sign in function
[hero scenario 2, use case 2.1.2] Signup user with custom attributes with verify OTP as last step
Update the test app SignUpCode page: currentState = (bundle?.getParcelable(Constants.STATE) as? SignUpCodeRequiredState)!!
[hero scenario 2, use case 2.1.2] Signup user with custom attributes with verify OTP as last step
Update the test app SignUpCode page: currentState = (bundle?.getParcelable(Constants.STATE) as? SignUpCodeRequiredState)!!
[hero scenario 2, use case 2.1.2] Signup user with custom attributes with verify OTP as last step
[hero scenario 3, use case 2.1.3] Verify email OTP first and then collect custom attributes
Update the test app SignUpAttributes page: currentState = (bundle?.getParcelable(Constants.STATE) as? SignUpAttributesRequiredState)!!
SignUpAttributesUI disorder
[use case 1.1.4] Verify email address using email OTP and then set password
In SignUpCodeFragment modify to currentState = (bundle?.getParcelable(Constants.STATE) as? SignUpCodeRequiredState)!!
In SignUpPasswordFragment modify to currentState = (bundle?.getParcelable(Constants.STATE) as? SignUpPasswordRequiredState)!!
Batch1
No error is popped up in UI (Probably this is a bug of the test app)
App crashing
No error is popped up in UI (Probably this is a bug of the test app)
When submitting attributes (had to hard-code some), the test application doesn't return to the sign in / sign up screen
EmailSignInSignUpFragment does not have logic for handling redirects