feat(cognito): validate oidc provider name #28802

msambol · 2024-01-21T21:39:36Z

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

msambol · 2024-01-21T21:40:00Z

packages/aws-cdk-lib/aws-cognito/lib/user-pool-idps/oidc.ts

@@ -107,10 +107,6 @@ export class UserPoolIdentityProviderOidc extends UserPoolIdentityProviderBase {
  constructor(scope: Construct, id: string, props: UserPoolIdentityProviderOidcProps) {
    super(scope, id, props);

-    if (props.name && !Token.isUnresolved(props.name) && (props.name.length < 3 || props.name.length > 32)) {
-      throw new Error(`Expected provider name to be between 3 and 32 characters, received ${props.name} (${props.name.length} characters)`);
-    }


This isn't needed because the validation is done in getProviderName below.

packages/@aws-cdk-testing/framework-integ/test/aws-cognito/test/integ.user-pool-idp.oidc.ts

laurelmay

Thanks! I do think I've seen a few recent validation additions get marked as feat instead of chore; not sure whether that applies here.

packages/aws-cdk-lib/aws-cognito/lib/user-pool-idps/oidc.ts

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

msambol · 2024-01-22T01:35:58Z

Exemption Request: I only added validation, I don't believe a README update is needed.

IkeNefcy · 2024-01-22T02:10:29Z

Does this function check work? From what I'm reading PCRE is not natural in js/ts. The string CFN uses to check is in PCRE but .match() uses ECMAScript right? I think the string would need to be translated to this regex dialect first for this to work.

My concern is mainly with the escape characters. In PCRE \p{} indicates special characters, for instance \p{Z} is a space. Checking with https://regex101.com/ works. But with js regex, it would act like https://regexr.com/ , where it's checking the character p and Z as if they are letters

laurelmay · 2024-01-22T03:29:12Z

Does this function check work? From what I'm reading PCRE is not natural in js/ts. The string CFN uses to check is in PCRE but .match() uses ECMAScript right? I think the string would need to be translated to this regex dialect first for this to work.

Good catch! I believe that per the specification the correct thing to do here to enable the u flag on the regex for unicode support. In this situation, both sites you linked to (regex101 with the JS engine selected and regexr) show a proper parsing of the regex.

packages/aws-cdk-lib/aws-cognito/lib/user-pool-idps/oidc.ts

msambol · 2024-01-22T20:18:13Z

@kylelaker / @IkeNefcy I updated the pattern with the u. Thanks!

...s-cognito/test/integ.user-pool-idp.oidc.js.snapshot/integ-user-pool-idp-google.template.json

Co-authored-by: Kyle Laker <kyle@laker.email>

msambol · 2024-01-29T18:29:34Z

@GavinZZ I removed the snapshot changes.

GavinZZ · 2024-01-29T20:08:12Z

We've had some issue with CodeBuild which is why the CI is not passing. We're actively working on a fix, sorry for any inconvenience.

GavinZZ · 2024-01-29T21:47:10Z

@mergify update

mergify · 2024-01-29T21:47:14Z

update

✅ Branch has been successfully updated

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

GavinZZ

LGTM, thanks for contributing!

mergify · 2024-01-29T23:16:08Z

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

aws-cdk-automation · 2024-01-29T23:42:51Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 0208b59
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2024-01-29T23:43:26Z

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

Closes aws#28667. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

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

aws-cdk-automation requested a review from a team January 21, 2024 21:39

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 star-contributor [Pilot] contributed between 25-49 PRs to the CDK labels Jan 21, 2024

msambol commented Jan 21, 2024

View reviewed changes

packages/@aws-cdk-testing/framework-integ/test/aws-cognito/test/integ.user-pool-idp.oidc.ts Outdated Show resolved Hide resolved

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jan 21, 2024

laurelmay approved these changes Jan 22, 2024

View reviewed changes

packages/aws-cdk-lib/aws-cognito/lib/user-pool-idps/oidc.ts Outdated Show resolved Hide resolved

aws-cdk-automation added pr/needs-maintainer-review This PR needs a review from a Core Team Member and removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Jan 22, 2024

msambol force-pushed the 28667 branch from 201cf23 to fdcc426 Compare January 22, 2024 01:32

msambol changed the title ~~chore(cognito): validate oidc provider name~~ feat(cognito): validate oidc provider name Jan 22, 2024

aws-cdk-automation previously requested changes Jan 22, 2024

View reviewed changes

aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Jan 22, 2024

laurelmay reviewed Jan 22, 2024

View reviewed changes

packages/aws-cdk-lib/aws-cognito/lib/user-pool-idps/oidc.ts Outdated Show resolved Hide resolved

msambol force-pushed the 28667 branch from d88d338 to df76de8 Compare January 25, 2024 01:36

GavinZZ reviewed Jan 25, 2024

View reviewed changes

...s-cognito/test/integ.user-pool-idp.oidc.js.snapshot/integ-user-pool-idp-google.template.json Outdated Show resolved Hide resolved

GavinZZ added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Jan 29, 2024

msambol and others added 6 commits January 29, 2024 10:26

chore(cognito): validate oidc provider name

0467e33

update snapshot

00946da

add link to docs

c10ce48

Update packages/aws-cdk-lib/aws-cognito/lib/user-pool-idps/oidc.ts

a1383d2

Co-authored-by: Kyle Laker <kyle@laker.email>

add int test changes

571b0a2

revert snap changes

024f623

msambol force-pushed the 28667 branch from 85c3ed2 to 024f623 Compare January 29, 2024 18:29

mergify bot and others added 2 commits January 29, 2024 21:47

Merge branch 'main' into 28667

b5eaaeb

Merge branch 'main' into 28667

20c3582

GavinZZ added the pr-linter/exempt-readme The PR linter will not require README changes label Jan 29, 2024

GavinZZ approved these changes Jan 29, 2024

View reviewed changes

Merge branch 'main' into 28667

0208b59

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 29, 2024

mergify bot merged commit 534794c into aws:main Jan 29, 2024
12 checks passed

github-actions bot mentioned this pull request Feb 1, 2024

Monthly PR metrics report - Jan 2024 #28952

Closed

SankyRed pushed a commit that referenced this pull request Feb 8, 2024

feat(cognito): validate oidc provider name (#28802)

812d7af

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cognito): validate oidc provider name #28802

feat(cognito): validate oidc provider name #28802

msambol commented Jan 21, 2024

msambol Jan 21, 2024

laurelmay left a comment

aws-cdk-automation left a comment

msambol commented Jan 22, 2024

IkeNefcy commented Jan 22, 2024 •

edited

Loading

laurelmay commented Jan 22, 2024

msambol commented Jan 22, 2024

msambol commented Jan 29, 2024

GavinZZ commented Jan 29, 2024

GavinZZ commented Jan 29, 2024

mergify bot commented Jan 29, 2024

GavinZZ left a comment

mergify bot commented Jan 29, 2024

aws-cdk-automation commented Jan 29, 2024

mergify bot commented Jan 29, 2024

feat(cognito): validate oidc provider name #28802

feat(cognito): validate oidc provider name #28802

Conversation

msambol commented Jan 21, 2024

msambol Jan 21, 2024

Choose a reason for hiding this comment

laurelmay left a comment

Choose a reason for hiding this comment

aws-cdk-automation left a comment

Choose a reason for hiding this comment

msambol commented Jan 22, 2024

IkeNefcy commented Jan 22, 2024 • edited Loading

laurelmay commented Jan 22, 2024

msambol commented Jan 22, 2024

msambol commented Jan 29, 2024

GavinZZ commented Jan 29, 2024

GavinZZ commented Jan 29, 2024

mergify bot commented Jan 29, 2024

✅ Branch has been successfully updated

GavinZZ left a comment

Choose a reason for hiding this comment

mergify bot commented Jan 29, 2024

aws-cdk-automation commented Jan 29, 2024

AWS CodeBuild CI Report

mergify bot commented Jan 29, 2024

IkeNefcy commented Jan 22, 2024 •

edited

Loading