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(auth): Added missing UserContextData to Cognito User Pool operation SignUp #13477

Merged
merged 13 commits into from
Jun 20, 2024

Conversation

armandsyah134
Copy link
Contributor

Description of changes

  • Adding missing property UserContextData in SignUp call to Cognito User Pools (CUP)
  • UserContextData is an important property for Sign Up calls, and should be added to the SignUp operation
    • Other CUP apis are passing in UserContextData (e.g. ConfirmSignUp)
  • UserContextData is already in your API contract

Issue #, if available

N/a (will make an issue for this if required

Description of how you validated changes

  • Added unit test for validating UserContextData present in SignUp call
  • Performed local testing by linking changed library to sample NextJS app and testing sign up flow
    • Validated that UserContextData was present in our logs

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • [n/a] Relevant documentation is changed or added (and PR referenced)

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

israx
israx previously approved these changes Jun 7, 2024
Copy link
Member

@israx israx left a comment

Choose a reason for hiding this comment

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

LGTM

yarn.lock Outdated Show resolved Hide resolved
israx
israx previously approved these changes Jun 7, 2024
Copy link
Contributor

@jimblanc jimblanc left a comment

Choose a reason for hiding this comment

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

Change looks good to me, but please hold off on merging until we can discuss (blocking PR). Thank you for the contribution!

Comment on lines 77 to 81
const UserContextData = getUserContextData({
username,
userPoolId,
userPoolClientId,
});
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Can we move this directly into line 96? I think getUserContextData is expressive enough to convey the context there when used directly. Also prefer not to have the PascalCased variable as it easily gets confused with types/interfaces/classes

jimblanc
jimblanc previously approved these changes Jun 17, 2024
@armandsyah134 armandsyah134 marked this pull request as ready for review June 17, 2024 13:31
haverchuck
haverchuck previously approved these changes Jun 17, 2024
@armandsyah134 armandsyah134 dismissed stale reviews from israx, haverchuck, and jimblanc via 3839009 June 18, 2024 15:31
jimblanc
jimblanc previously approved these changes Jun 18, 2024
@jimblanc jimblanc merged commit e6c5f60 into aws-amplify:main Jun 20, 2024
30 checks passed
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.

5 participants