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.

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

feat(auth): add options to resetPassword and confirmResetPassword #1426

Merged
merged 6 commits into from
Aug 2, 2021

Conversation

Jordan-Nelson
Copy link
Contributor

Issue #, if available: N/A

Description of changes:

  • Add options to resetPassword
  • Add options to confirmResetPassword

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@Jordan-Nelson Jordan-Nelson requested a review from a team July 23, 2021 15:51
@frimfram
Copy link
Contributor

Can you please elaborate on what type of testings you have done? We are looking for something more involved than just unit testing. Have you integrated this in sample apps to see if it works?

@frimfram frimfram self-requested a review July 23, 2021 19:17
Copy link
Contributor

@frimfram frimfram left a comment

Choose a reason for hiding this comment

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

more testing required.

@Jordan-Nelson
Copy link
Contributor Author

Jordan-Nelson commented Jul 23, 2021

Hi @frimfram - For context, I am adding this for use in amplify-flutter.

I have tested an end to end reset password flow from a flutter app with the changes in aws-amplify/amplify-flutter#743 and confirmed that I can see the metadata in a lambda trigger.

If there is something specific you are looking for from a testing perspective, please let me know.

Copy link
Contributor

@rjuliano rjuliano left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

@Jordan-Nelson
Copy link
Contributor Author

@frimfram - Here are the scenarios that I tested for resetPassword and confirmResetPassword. Each of these scenarios were from a native Android app. For both APIs I first tested v1.21.0 of amplify-android to confirm existing behavior, and then tested with the changes in this PR (with and without options/metadata).

Reset Password Scenarios

Scenario Expected Result (based on v1.21.0 behavior) PR w/o metadata PR w/ metadata
No Network AuthException, cause = AmazonClientException
Non-existent Username UserNotFoundException
No Username InvalidParameterException
Unconfirmed User InvalidParameterException
5+ Attempts in a row LimitExceededException
Failure in lambda trigger AuthException, cause = UserLambdaValidationException

Confirm Reset Password Scenarios

Scenario Expected Result (based on v1.21.0 behavior) PR w/o metadata PR w/ metadata
No Network AuthException, cause = AmazonClientException
Wrong Code UserNotFoundException
No Code InvalidParameterException
5+ Attempts in a row LimitExceededException
Failure in lambda trigger AuthException, cause = UserLambdaValidationException

@@ -653,10 +657,17 @@ public void onError(Exception exception) {
@Override
public void resetPassword(
@NonNull String username,
@NonNull AuthResetPasswordOptions options,
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great if overriding is used instead of this breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resetPassword() on line 698 has the same method signature as the existing method. This will not be breaking.

@frimfram frimfram requested a review from rjuliano July 28, 2021 20:37
@frimfram frimfram self-requested a review July 29, 2021 20:02
@raphkim raphkim merged commit 7fe6e2b into aws-amplify:main Aug 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants