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

fix parameters in sendMFASelectionAnswer #6418

Merged
merged 5 commits into from
Aug 25, 2020

Conversation

donaldpipowitch
Copy link
Contributor

@donaldpipowitch donaldpipowitch commented Jul 27, 2020

The wrong parameters were passed to the callbacks of sendMFASelectionAnswer.

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

@codecov
Copy link

codecov bot commented Jul 27, 2020

Codecov Report

Merging #6418 into main will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #6418   +/-   ##
=======================================
  Coverage   73.23%   73.23%           
=======================================
  Files         208      208           
  Lines       12941    12941           
  Branches     2529     2529           
=======================================
  Hits         9477     9477           
  Misses       3272     3272           
  Partials      192      192           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9ad830...4029ae5. Read the comment docs.

@ericclemmons ericclemmons requested a review from amhinson August 11, 2020 22:57
@ericclemmons
Copy link
Contributor

@donaldpipowitch Can you share some sample code on how you're using CognitoUser directly or indirectly so we can validate this PR?

Thanks!

@ericclemmons ericclemmons added the not-reproducible Not able to reproduce the issue label Aug 12, 2020
@donaldpipowitch
Copy link
Contributor Author

donaldpipowitch commented Aug 24, 2020

@donaldpipowitch Can you share some sample code on how you're using CognitoUser directly or indirectly so we can validate this PR?

cognitoUser.sendMFASelectionAnswer('SMS_MFA', {
  mfaRequired: (challengeName, challengeParameters) => {
    // those params are always undefined without the fix
  },
});

If you search for those params in the file you'll see that the API returns ChallengeParameters/ChallengeName and not challengeParameters/challengeName.

image

Copy link
Contributor

@amhinson amhinson left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @donaldpipowitch! This does appear to be a bug, as it uses the correct key name in other responses in the same file, as you showed with your screenshot. Also, the documentation shows the correct casing as ChallengeName/ChallengeParameters.

@amhinson amhinson self-assigned this Aug 24, 2020
@amhinson amhinson merged commit 794c1da into aws-amplify:main Aug 25, 2020
nubpro pushed a commit to nubpro/amplify-js that referenced this pull request Oct 2, 2020
@github-actions
Copy link

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants