Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cognito-identitypool-alpha): inconvenient IdentityPoolProviderUrl.userPool() #29025

Merged
merged 6 commits into from
Apr 17, 2024

Conversation

Tietew
Copy link
Contributor

@Tietew Tietew commented Feb 8, 2024

Reason for this change

IdentityPoolProviderUrl.userPool() requires a string url currently.
The description is "User Pool Provider Url".
It should be `${userPool.userPoolProviderName}:${userPoolClient.userPoolClientId}`.

UserPool has an attribute userPoolProviderUrl which description is "User Pool Provider Url", but confusingly, it cannot be specified to IdentityPoolProviderUrl.userPool().

The format of the identity provider identifier isn't well documented.
See SetIdentityPoolRoles API reference for example of User Pool's identity provider identifier.

Description of changes

This PR fixes IdentityPoolProviderUrl.userPool() to accept UserPool and UserPoolClient instead of a string url.
It generates a correct identifier described above.

Description of how you validated changes

Existing integration test generates an identifier as described above.
The snapshot won't be changed by this PR.

Checklist

BREAKING CHANGE: The argument of IdentityPoolProviderUrl.userPool() has been changed from url: string to userPool: UserPool, userPoolClient: UserPoolClient. If you want to specify custom identifier string, use IdentityPoolProviderUrl.custom() instead.


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

@github-actions github-actions bot added the p2 label Feb 8, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team February 8, 2024 04:50
@github-actions github-actions bot added the valued-contributor [Pilot] contributed between 6-12 PRs to the CDK label Feb 8, 2024
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Tietew
Copy link
Contributor Author

Tietew commented Feb 8, 2024

Exemption Request
The snapshot should not be changed.

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Feb 8, 2024
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Feb 8, 2024
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple probing questions here to start @Tietew

@@ -155,7 +157,8 @@ export class IdentityPoolProviderUrl {
}

/** User Pool Provider Url */
public static userPool(url: string): IdentityPoolProviderUrl {
public static userPool(userPool: UserPool, userPoolClient: UserPoolClient): IdentityPoolProviderUrl {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we're ok with breaking changes, but this seems like it would ruin anyone who was simply supplying a url here that was not connected to a specific UserPool or UserPoolCliet construct. I'm not sure what the difference would be if the IdentityPoolProviderType turns from USER_POOL to CUSTOM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IdentityPoolProviderType (IdentityPoolProviderUrl.type) is not used since #16190 (implemented PR).
Changing .userPool(somthing) to .custom(something) makes no differences in CloudFormation template.

@@ -440,7 +440,7 @@ describe('role mappings', () => {
const providerUrl = Fn.importValue('ProviderUrl');
expect(() => new IdentityPool(stack, 'TestIdentityPoolRoleMappingErrors', {
roleMappings: [{
providerUrl: IdentityPoolProviderUrl.userPool(providerUrl),
providerUrl: IdentityPoolProviderUrl.custom(providerUrl),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would change the IdentityPoolProviderTyle right? How does that affect things?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above. No affects in CloudFormation template.

@Tietew
Copy link
Contributor Author

Tietew commented Feb 22, 2024

Updated BREAKING CHANGE in description

@aws-cdk-automation
Copy link
Collaborator

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@aws-cdk-automation
Copy link
Collaborator

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@aws-cdk-automation aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Mar 9, 2024
@paulhcsun paulhcsun reopened this Apr 8, 2024
@paulhcsun paulhcsun added pr-linter/do-not-close The PR linter will not close this PR while this label is present and removed closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. labels Apr 8, 2024
@TheRealAmazonKendra TheRealAmazonKendra added pr-linter/exempt-integ-test The PR linter will not require integ test changes and removed pr-linter/do-not-close The PR linter will not close this PR while this label is present pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. labels Apr 17, 2024
@aws-cdk-automation aws-cdk-automation dismissed their stale review April 17, 2024 01:42

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

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Apr 17, 2024
Copy link
Contributor

mergify bot commented Apr 17, 2024

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

@TheRealAmazonKendra
Copy link
Contributor

@Mergifyio update

Copy link
Contributor

mergify bot commented Apr 17, 2024

update

❌ Mergify doesn't have permission to update

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/request-cli-integ-test.yml without workflows permission

@mergify mergify bot dismissed TheRealAmazonKendra’s stale review April 17, 2024 18:02

Pull request has been modified.

Copy link
Contributor

mergify bot commented Apr 17, 2024

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
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify mergify bot merged commit 90a7734 into aws:main Apr 17, 2024
9 checks passed
Copy link
Contributor

mergify bot commented Apr 17, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes valued-contributor [Pilot] contributed between 6-12 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants