feat(cognito): support provider details for UserPoolIdentityProviderSaml
#29588
Conversation
github-actions
bot
added
star-contributor
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.
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
badmintoncryer
changed the title
UserPoolIdentityProviderSaml
| @@ -99,6 +124,9 @@ export class UserPoolIdentityProviderSaml extends UserPoolIdentityProviderBase { | |||
| IDPSignout: props.idpSignout ?? false, | |||
| 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.
badmintoncryer
force-pushed
the
29494-samlProvider
branch
2 times, most recently
from
7361eb9
to
0e68618
Compare
badmintoncryer
force-pushed
the
29494-samlProvider
branch
from
0e68618
to
816d08c
Compare
aws-cdk-automation
added
the
pr/needs-community-review
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-cdk-automation
removed
the
pr/needs-community-review
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