feat(cognito): support provider details for UserPoolIdentityProviderSaml#29588
feat(cognito): support provider details for UserPoolIdentityProviderSaml#29588mergify[bot] merged 10 commits intoaws:mainfrom
UserPoolIdentityProviderSaml#29588Conversation
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.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
UserPoolIdentityProviderSaml
| MetadataURL: metadataType === UserPoolIdentityProviderSamlMetadataType.URL ? metadataContent : undefined, | ||
| MetadataFile: metadataType === UserPoolIdentityProviderSamlMetadataType.FILE ? metadataContent : undefined, | ||
| EncryptedResponses: props.encryptedResponses ?? undefined, | ||
| RequestSigningAlgorithm: props.signingRequests ? 'rsa-sha256' : undefined, |
There was a problem hiding this comment.
I could not find any information about what algorithm is acceptable for RequestSigningAlgorithm without rsa-sha256.
SAML
Create or update request with Metadata URL: "ProviderDetails": { "IDPInit": "true", "IDPSignout": "true", "EncryptedResponses" : "true", "MetadataURL": "https://auth.example.com/sso/saml/metadata", "RequestSigningAlgorithm": "rsa-sha256" }
I defined signingRequest as boolean but it may be better to define SigningAlgorithm enum and use it.
I would like to hear the reviewer's opinion.
There was a problem hiding this comment.
I'm also unable to find documentation regarding other availabilities, maybe someone from AWS can clear it up if they have internal docs regarding this.
Regardless, even if rsa-sha256 is the only current available signing algorithm, adding an enum right away gives us more extensibility if and when other algorithms are made available. The boolean would become redundant.
There was a problem hiding this comment.
@nmussy Thank you for your opinion!!
I will create a new enum and remove Boolean definition.
There was a problem hiding this comment.
This would have been my one comment on this PR. Great suggestion.
7361eb9 to
0e68618
Compare
0e68618 to
816d08c
Compare
TheRealAmazonKendra
left a comment
There was a problem hiding this comment.
Great work! I have... no notes? That's weird.
Thanks for making this change and thank you to @nmussy for your initial round of feedback!
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). |
Issue # (if applicable)
Closes #29494.
Closes #29598.
#29598 is really close issue and I tried to resolve it in this PR.
If it is not good to resolve multiple issues in 1 PR, I would separate this PR.
Reason for this change
UserPoolIdentityProviderSamlcan configureProviderDetailsbut there are some items that is not configurable from AWS CDK.EncryptedResponsesRequestSigningAlgorithmIDPInitDescription of changes
Add 3 properties to
UserPoolIdentityProviderSamlProps.encryptedResponsesrequestSigningAlgorithmidpInitiatedDescription of how you validated changes
Added both unit and integ tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license