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(cognito): user pool - link style email verification fails to deploy #6938

Merged
merged 3 commits into from Mar 24, 2020

Conversation

nija-at
Copy link
Contributor

@nija-at nija-at commented Mar 23, 2020

Commit Message

fix(cognito): user pool - link style email verification fails to deploy (#6938)

A couple of changes -

  • emailVerificationMessage and emailVerificationSubject
    properties are not set when link-style verification is used.
    This is root cause for the error message reported by the user.

  • The defaults emailBody and emailSubject are modified when
    link-style verification is used. They will now correctly contain the
    template placeholder {##Verify Email##}.

  • Validations are now added that email and SMS verification
    messages have the mandatory placeholders.

fixes #6811

End Commit Message


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

A couple of things happen with this change -
- `emailVerificationMessage`, `emailVerificationSubject` and
`smsVerificationMessage` properties are not set when link-style
verification is used. This is root cause for the error message reported
by the user.
- The defaults `emailBody` and `emailSubject` are modified when
link-style verification is used. They will now correctly contian the
template placeholder '{##Verify Email##}'.

fixes #6811
@nija-at nija-at added pr/do-not-merge This PR should not be merged at this time. contribution/core This is a PR that came from AWS. labels Mar 23, 2020
@nija-at nija-at requested a review from a team March 23, 2020 11:22
@nija-at nija-at self-assigned this Mar 23, 2020
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: aeaf6d9
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

throw new Error('SMS message cannot be configured when emailStyle is configured to CODE');
}
const emailMessage = props.userVerification?.emailBody ?? 'Hello {username}, Verify your account by clicking on {##Verify Email##}';
if (emailMessage.indexOf('{##Verify Email##}') < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't like this placeholder very much. Do we have control over it, or is this what Cognito decided upon?

In any case shouldn't these all be in constants somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cognito's placeholders - https://docs.aws.amazon.com/cognito/latest/developerguide/cognito-user-pool-settings-message-templates.html
And the {##Verify Email##} is an undocumented one.

In any case shouldn't these all be in constants somewhere?

Yep, will change

@nija-at nija-at removed the pr/do-not-merge This PR should not be merged at this time. label Mar 24, 2020
nija-at pushed a commit that referenced this pull request Mar 24, 2020
Configuring the correct properties for sign up and testing it is hard.

This PR - #6938 - shows how it can be
easily mis-configured.

Testing the correct configuration is hard and requires a certain amount
of set up and the correct set of calls to the 'SignUp' API.

The integ tests here encode all of this in a way that can be easily
replicated when any part of sign up is being modified or needs to be
tested.
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: d63203c
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Mar 24, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify
Copy link
Contributor

mergify bot commented Mar 24, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit b5c60d5 into master Mar 24, 2020
@mergify mergify bot deleted the nija-at/cognito-fix-link-verification branch March 24, 2020 12:33
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 8f37185
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify bot added a commit that referenced this pull request Mar 25, 2020
* chore(cognito): integ tests for sign up

Configuring the correct properties for sign up and testing it is hard.

This PR - #6938 - shows how it can be
easily mis-configured.

Testing the correct configuration is hard and requires a certain amount
of set up and the correct set of calls to the 'SignUp' API.

The integ tests here encode all of this in a way that can be easily
replicated when any part of sign up is being modified or needs to be
tested.

* fixup breaking tests

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

user pool - link style email verification fails to deploy
3 participants