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(cognito): user pool: send emails using Amazon SES #17117

Merged
merged 10 commits into from Nov 15, 2021

Conversation

corymhall
Copy link
Contributor

add support for SES integration by introducing a new property for
configuring email settings for a user pool. This feature supports both
types of integration with SES.

  1. Using the COGNITO_DEFAULT sending account, but providing a custom
    email address
  2. Using the DEVELOPER sending account

This feature does not automate any configuration on SES since that is
not currently possible with CloudFormation and requires a manual
verification step. To use the SES integration introduced in this feature
the user will have had to already configured a verified email address in
Amazon SES and followed the steps outlined here:
https://docs.aws.amazon.com/cognito/latest/developerguide/user-pool-email.html

closes #6768


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

add support for SES integration by introducing a new property for
configuring email settings for a user pool. This feature supports both
types of integration with SES.

1. Using the COGNITO_DEFAULT sending account, but providing a custom
   email address
2. Using the DEVELOPER sending account

This feature does not automate any configuration on SES since that is
not currently possible with CloudFormation and requires a manual
verification step. To use the SES integration introduced in this feature
the user will have had to already configured a verified email address in
Amazon SES and followed the steps outlined here:
https://docs.aws.amazon.com/cognito/latest/developerguide/user-pool-email.html

closes #6768
@gitpod-io
Copy link

gitpod-io bot commented Oct 22, 2021

@github-actions github-actions bot added the @aws-cdk/aws-cognito Related to Amazon Cognito label Oct 22, 2021
Copy link

@mnanchev mnanchev left a comment

Choose a reason for hiding this comment

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

Looks good, maybe just Beta1 is avoidable

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Oct 27, 2021
@nija-at nija-at changed the title feat(aws-cognito): add support for user pool SES integration feat(cognito): add support for user pool SES integration Oct 27, 2021
@nija-at nija-at changed the title feat(cognito): add support for user pool SES integration feat(cognito): support for user pool SES integration Oct 27, 2021
@corymhall corymhall requested review from nija-at and a team October 27, 2021 14:27
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

Initial comments just based on the README.

packages/@aws-cdk/aws-cognito/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cognito/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cognito/README.md Outdated Show resolved Hide resolved
Comment on lines 361 to 362
this email must be a verified email address in Amazon SES, and that email address must have an authorization policy
that allows Cognito to send emails. See the section `Step 3: Grant Email Permissions to Amazon Cognito` of the
Copy link
Contributor

Choose a reason for hiding this comment

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

must have an authorization policy that allows Cognito to send emails

can we auto-configure this policy as part of the cognito module? What would that take?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We would need to configure this policy by using a custom resource that calls put-identity-policy. A prerequisite would be creating the email in SES, but it wouldn't need to be verified or fully configured to attach this policy.

I wasn't sure about configuring this one piece in SES since it feels like SES should have it's own package that controls it's configuration. I think ideally the user experience would look like this, although I don't know if SES will get CloudFormation resources anytime soon.

const sesIdentity = new ses.Identity(this, 'Identity', {
  email: 'user@example.com',
});

sesIdentity.addAuthorization(new authorizations.CognitoAuthorization(userpool));

Copy link
Contributor

Choose a reason for hiding this comment

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

Email verification is clearly an out-of-band process, and we cannot automate that in the cdk.

Calling put-identity-policy via a custom resource is something we can do, i.e., add it to the aws-ses module and use it from here.
If you're interested, you could consider something like this in a subsequent PR. I'm ok with shipping it without this feature available yet.

packages/@aws-cdk/aws-cognito/README.md Show resolved Hide resolved
packages/@aws-cdk/aws-cognito/lib/user-pool.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cognito/README.md Outdated Show resolved Hide resolved
(before the '@') must only include ASCII characters, but the domain can
be punycode encoded.
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

Spent some time on this PR. Not complete yet.

packages/@aws-cdk/aws-cognito/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cognito/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cognito/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cognito/README.md Outdated Show resolved Hide resolved
```ts
new cognito.UserPool(this, 'myuserpool', {
email: Email.withSES({
sesRegion: SESRegion.US_EAST_1,
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need an enum for this. Region is just a string in the cdk. We can just add a validation for the list of valid regions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Still show as an enum. README needs to be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah sorry, should be fixed now.

Comment on lines 433 to 442
* Required if the UserPool region is different than the SES region.
*
* If sending emails with a Amazon SES verified email address,
* and the region that SES is configured is different than the
* region in which the UserPool is deployed, you must specify that
* region here.
*
* @default - The same region as the Cognito UserPool
*/
readonly sesRegion?: SESRegion;
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if this property is not specified and the region is not the expected SES region?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

then it will throw an error telling the user to provide this property.

Copy link
Contributor

Choose a reason for hiding this comment

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

"it" here being CloudFormation deployment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have validation logic to validate that a correct region is is provided and the library will throw an error if it is not a valid region.

https://github.com/aws/aws-cdk/pull/17117/files#diff-46f225425155cdae9873139ac778b85e5312e63ed690512df01a449a9f45c1a6R195

packages/@aws-cdk/aws-cognito/lib/user-pool.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cognito/lib/user-pool.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cognito/lib/user-pool.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cognito/lib/user-pool.ts Outdated Show resolved Hide resolved
removing regions property in favor of a const
switching sesRegions to take a string
deprecating emailSettings property
other small doc updates
@corymhall corymhall requested review from nija-at and a team November 1, 2021 17:43
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

Looks much better. See more comments below.

```ts
new cognito.UserPool(this, 'myuserpool', {
email: Email.withSES({
sesRegion: SESRegion.US_EAST_1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Still show as an enum. README needs to be updated.

/**
* Configuration for Cognito sending emails via Amazon SES
*/
export interface SESOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this module contains both user pool and identity pool, perhaps rename this as UserPoolSESOptions.

Same for other names as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated all the exported names.

packages/@aws-cdk/aws-cognito/lib/user-pool-email.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cognito/lib/user-pool-email.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cognito/lib/user-pool-email.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cognito/lib/user-pool-email.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cognito/lib/user-pool-email.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cognito/lib/user-pool-email.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cognito/lib/user-pool-email.ts Outdated Show resolved Hide resolved
Comment on lines 187 to 188
// if a custom email is provided that means that cognito is going to use an SES email
// and we need to provide the sourceArn which requires a valid region
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused. Cognito email is not SES email right? Are we mixing these up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a little confusing, maybe I can add some more explanation to the README. There are technically two modes to Cognito default email.

  1. Using the default no-reply@verificationemail.com
    In this case Cognito is handling sending the email because it is using an email address owned by AWS.

  2. Using a custom email address.
    In this case even though it is still using the Cognito default email functionality, it is actually using SES to send the email, so it does require you to have the email configured in SES.

Copy link
Contributor

@nija-at nija-at Nov 5, 2021

Choose a reason for hiding this comment

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

Can we then roll #2 into Email.withSES() instead? That would keep this simpler/ easier to explain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm that is a good point. I can't think of a reason why you would want to use Cognito default through SES instead of just SES directly. I'll update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I updated withCognito to only take the replyTo email address. You now how two options

  1. Cognito default with the default no-reply@verificationemail.com email and optionally a replyTo email address.
  2. Sending emails through SES.

I completely removed option 2 from my previous response since you don't gain anything over just configuring Cognito to use SES.

@nija-at nija-at changed the title feat(cognito): support for user pool SES integration feat(cognito): user pool: send emails using Amazon SES Nov 15, 2021
@nija-at nija-at added the pr/do-not-merge This PR should not be merged at this time. label Nov 15, 2021
packages/@aws-cdk/aws-cognito/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cognito/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cognito/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cognito/lib/user-pool-email.ts Outdated Show resolved Hide resolved
* Returns the email configuration for a Cognito UserPool
* that controls how Cognito will send emails
*/
public abstract bind(scope: Construct): UserPoolEmailConfig;
Copy link
Contributor

Choose a reason for hiding this comment

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

I like to use UserPoolEmailBindOptions as the parameter to bind().
This allows for more properties to be added, deprecated, etc. here without breaking changes.

The other option is to mark this API as @internal and not export UserPoolEmailConfig. Then changes to this API will not be considered breaking.
This option assumes that there aren't going to be many other ways in which customers will want to bind their own implementations here. Seems ok in this case since I don't expect Cognito to support other options than SES.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to be @internal

@corymhall corymhall removed the pr/do-not-merge This PR should not be merged at this time. label Nov 15, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 15, 2021

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).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: da4352d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 503720f into aws:master Nov 15, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 15, 2021

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).

mpvosseller pushed a commit to mpvosseller/aws-cdk that referenced this pull request Nov 16, 2021
add support for SES integration by introducing a new property for
configuring email settings for a user pool. This feature supports both
types of integration with SES.

1. Using the COGNITO_DEFAULT sending account, but providing a custom
   email address
2. Using the DEVELOPER sending account

This feature does not automate any configuration on SES since that is
not currently possible with CloudFormation and requires a manual
verification step. To use the SES integration introduced in this feature
the user will have had to already configured a verified email address in
Amazon SES and followed the steps outlined here:
https://docs.aws.amazon.com/cognito/latest/developerguide/user-pool-email.html

closes aws#6768

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
add support for SES integration by introducing a new property for
configuring email settings for a user pool. This feature supports both
types of integration with SES.

1. Using the COGNITO_DEFAULT sending account, but providing a custom
   email address
2. Using the DEVELOPER sending account

This feature does not automate any configuration on SES since that is
not currently possible with CloudFormation and requires a manual
verification step. To use the SES integration introduced in this feature
the user will have had to already configured a verified email address in
Amazon SES and followed the steps outlined here:
https://docs.aws.amazon.com/cognito/latest/developerguide/user-pool-email.html

closes aws#6768

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-cognito Related to Amazon Cognito contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cognito user pools - support SES integration
4 participants