feat(cognito): validate oidc provider name#28802
Conversation
|
|
||
| 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)`); | ||
| } |
There was a problem hiding this comment.
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
Outdated
Show resolved
Hide resolved
laurelmay
left a comment
There was a problem hiding this comment.
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.
aws-cdk-automation
left a comment
There was a problem hiding this comment.
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.
|
Exemption Request: I only added validation, I don't believe a README update is needed. |
|
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 |
Good catch! I believe that per the specification the correct thing to do here to enable the |
|
@kylelaker / @IkeNefcy I updated the pattern with the |
...s-cognito/test/integ.user-pool-idp.oidc.js.snapshot/integ-user-pool-idp-google.template.json
Show resolved
Hide resolved
Co-authored-by: Kyle Laker <kyle@laker.email>
|
@GavinZZ I removed the snapshot changes. |
|
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. |
|
@mergify update |
✅ Branch has been successfully updated |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
GavinZZ
left a comment
There was a problem hiding this comment.
LGTM, thanks for contributing!
|
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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 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*
Closes #28667.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license