feat(cognito): user pools - non-ascii email domains #11099

wtho · 2020-10-25T16:40:38Z

If an email address for cognito email settings contains a non-ascii character, the cognito cdk package applies punycode encoding, which cloudformation will pick up properly.

This pull request adds the punycode package to the dependencies, as previously discussed in #8473. The package occupies 42KB in node_modules after installation. I am not sure if I configured the package correctly to be included in the cdk build. The project itself added punycode to aws-cdk-lib and monocdk, I had to re-run yarn install in these packages.

Unit- and integration tests are added, but notice that I could not manage a manual e2e test, I think I need to own a domain with non-ascii letters to do so, which I currently do not.

A similar approach as in this PR will also be interesting for other cdk packages which deal with email addresses and domains, like hosted zones.

According to the Cfn Domain Name Format Docs, characters with code point below 33, or in [127, 255] should be escaped with an octal code, which is not applied in this pull request.

Fixes #8473

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

gitpod-io · 2020-10-25T16:40:41Z

nija-at

Thanks for submitting this PR. Looks pretty good. Some minor suggestions below.

You will also need to merge latest from 'master' and resolve the merge conflict.

packages/@aws-cdk/aws-cognito/README.md

packages/@aws-cdk/aws-cognito/lib/user-pool.ts

packages/@aws-cdk/aws-cognito/test/integ.user-pool-encoding-charset-unicode.ts

packages/@aws-cdk/aws-cognito/lib/user-pool.ts

If an email address for cognito email settings contains a non-ascii character, the cognito cdk package applies punycode encoding, which cloudformation will pick up properly.

Pull request has been modified.

nija-at

I'd like to wait for this PR to be merged - #11195 - and rebase this change on top, so that we can correctly add attributions to 3rd party bundled dependencies.

Pull request has been modified.

wtho · 2020-10-30T18:48:57Z

Added the lint rule
Rebased on chore: attributions for all bundled third party dependencies #11195 and added the license for punycode to cognito and monocdk

I added the lint rule in cognito's eslintrc.js, but we could move it to the common lint rule configuration if you prefer so.

nija-at

Thanks for your patience with this.
A couple of more adjustments to the eslint rule.

packages/@aws-cdk/aws-cognito/.eslintrc.js

packages/@aws-cdk/aws-cognito/package.json

packages/monocdk/NOTICE

Pull request has been modified.

packages/@aws-cdk/aws-cognito/test/user-pool.test.ts

nija-at

Sorry for the delay. I was on vacation earlier in the week.

nija-at · 2020-11-06T13:58:30Z

packages/@aws-cdk/aws-cognito/lib/user-pool.ts

+function encodeDomain(email: string | undefined): string | undefined {
+  const splitEmail = email?.split('@');
+  if (splitEmail === undefined || splitEmail.length !== 2) {
+    return email;
+  }
+  const [local, domain] = splitEmail!;
+  return `${local}@${punycodeEncode(domain)}`;
+}


Can't we just use the toASCII() API instead. According to the documentation, it takes care of emails.

Yeah, but it also encodes the part before the @, which for AWS/Cloudformation should not be encoded. See rix0rr's earlier comment and the documentation.

Not according to the example in the docs that I've linked above. Specifically example#3 shows that the first half of the email address is NOT encoded.

Does the library behave differently? Can you test this and report?

You are correct! Did not go through the documentation properly!

I reverted back to the toASCII method, as previously.

Pull request has been modified.

mergify · 2020-11-09T14:12:32Z

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 · 2020-11-09T15:47:46Z

AWS CodeBuild CI Report

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

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

mergify · 2020-11-09T15:47:55Z

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

wtho force-pushed the wtho/cognito-userpool-punycode-encoding branch from c6c65b6 to 64d2a5f Compare October 25, 2020 20:12

SomayaB assigned nija-at Oct 26, 2020

nija-at changed the title ~~feat(cognito): auto-encode non-ascii domains~~ feat(cognito): user pool - non-ascii email domains Oct 28, 2020

nija-at changed the title ~~feat(cognito): user pool - non-ascii email domains~~ feat(cognito): user pools - non-ascii email domains Oct 28, 2020

nija-at previously requested changes Oct 28, 2020

View reviewed changes

wtho added 2 commits October 28, 2020 17:23

feat(cognito): auto-encode non-ascii domains

35ad9a9

If an email address for cognito email settings contains a non-ascii character, the cognito cdk package applies punycode encoding, which cloudformation will pick up properly.

apply suggestions from code review

d7301f4

wtho force-pushed the wtho/cognito-userpool-punycode-encoding branch from 64d2a5f to d7301f4 Compare October 28, 2020 16:31

Merge branch 'master' into wtho/cognito-userpool-punycode-encoding

77cb367

nija-at previously requested changes Oct 29, 2020

View reviewed changes

Merge branch 'master' into wtho/cognito-userpool-punycode-encoding

675124c

wtho force-pushed the wtho/cognito-userpool-punycode-encoding branch 2 times, most recently from bdd9a31 to 308f32c Compare October 30, 2020 18:08

nija-at previously requested changes Nov 2, 2020

View reviewed changes

packages/@aws-cdk/aws-cognito/.eslintrc.js Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-cognito/package.json Outdated Show resolved Hide resolved

packages/monocdk/NOTICE Outdated Show resolved Hide resolved

wtho force-pushed the wtho/cognito-userpool-punycode-encoding branch from 74af44c to 2b9dfa1 Compare November 2, 2020 16:14

rix0rrr reviewed Nov 3, 2020

View reviewed changes

packages/@aws-cdk/aws-cognito/test/user-pool.test.ts Outdated Show resolved Hide resolved

wtho force-pushed the wtho/cognito-userpool-punycode-encoding branch 2 times, most recently from bcdf43b to 7369218 Compare November 3, 2020 19:21

wtho requested review from rix0rrr and nija-at November 4, 2020 15:16

nija-at previously requested changes Nov 6, 2020

View reviewed changes

feat: add restricted punycode import eslint rule

79cba93

wtho force-pushed the wtho/cognito-userpool-punycode-encoding branch from 7369218 to 79cba93 Compare November 6, 2020 18:02

Merge branch 'master' into wtho/cognito-userpool-punycode-encoding

be3ec3f

nija-at approved these changes Nov 9, 2020

View reviewed changes

Merge branch 'master' into wtho/cognito-userpool-punycode-encoding

319795a

mergify bot merged commit 5d907b6 into aws:master Nov 9, 2020

wtho deleted the wtho/cognito-userpool-punycode-encoding branch November 9, 2020 17:47

nija-at mentioned this pull request Apr 21, 2021

(@aws-cdk/aws-cognito): import 'punycode/' #14290

Closed

carlgieringer mentioned this pull request Jul 3, 2023

All API requests fail with Cannot find module 'punycode/' Howdju/howdju#444

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cognito): user pools - non-ascii email domains #11099

feat(cognito): user pools - non-ascii email domains #11099

wtho commented Oct 25, 2020 •

edited

Loading

gitpod-io bot commented Oct 25, 2020 •

edited

Loading

nija-at left a comment •

edited

Loading

nija-at left a comment

wtho commented Oct 30, 2020

nija-at left a comment

nija-at left a comment

nija-at Nov 6, 2020

wtho Nov 6, 2020

nija-at Nov 6, 2020 •

edited

Loading

wtho Nov 6, 2020

mergify bot commented Nov 9, 2020

aws-cdk-automation commented Nov 9, 2020

mergify bot commented Nov 9, 2020

feat(cognito): user pools - non-ascii email domains #11099

feat(cognito): user pools - non-ascii email domains #11099

Conversation

wtho commented Oct 25, 2020 • edited Loading

gitpod-io bot commented Oct 25, 2020 • edited Loading

nija-at left a comment • edited Loading

Choose a reason for hiding this comment

nija-at left a comment

Choose a reason for hiding this comment

wtho commented Oct 30, 2020

nija-at left a comment

Choose a reason for hiding this comment

nija-at left a comment

Choose a reason for hiding this comment

nija-at Nov 6, 2020

Choose a reason for hiding this comment

wtho Nov 6, 2020

Choose a reason for hiding this comment

nija-at Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

wtho Nov 6, 2020

Choose a reason for hiding this comment

mergify bot commented Nov 9, 2020

aws-cdk-automation commented Nov 9, 2020

AWS CodeBuild CI Report

mergify bot commented Nov 9, 2020

wtho commented Oct 25, 2020 •

edited

Loading

gitpod-io bot commented Oct 25, 2020 •

edited

Loading

nija-at left a comment •

edited

Loading

nija-at Nov 6, 2020 •

edited

Loading