fix(cognito-identitypool-alpha): cannot configure roleMappings with imported userPool and client by sakurai-ryo · Pull Request #30421 · aws/aws-cdk

sakurai-ryo · 2024-06-01T13:44:46Z

Issue # (if applicable)

Reason for this change

Currently, we cannot use imported user pools and clients for role mapping in an identity pool.
This is because the IdentityPoolProviderUrl.userPool method takes an L2 construct as its argument type instead of Interface (IUserPool, IUserPoolClient).

    const userPool = cognito.UserPool.fromUserPoolArn(this, 'CognitoUserPool', 'arn');
    const userPoolClient = cognito.UserPoolClient.fromUserPoolClientId(this, 'UserPoolClientId', 'client-id');
    const identityPool = new cognitoidp.IdentityPool(this, 'IdentityPool', {
      // ~
      roleMappings: [
        {
          mappingKey: 'cognito', 
          providerUrl: cognitoidp.IdentityPoolProviderUrl.userPool(userPool, userPoolClient), // ! type error here !
          useToken: true
        }
      ],
      allowUnauthenticatedIdentities: false
    });

Description of changes

The argument types of the IdentityPoolProviderUrl.userPool method are changed to IUserPool and IUserPoolClient.
This method requires the userPoolProviderName of the userPool, but since it does not exist for IUserPool, a property was added.
Since this property is required in the UserPool construct, it is also required in IUserPool.

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

Line 902 in c3003ab

public readonly userPoolProviderName: string;

I add a required attribute to the Interface of the aws-cognito module(stable), but I do not think this to be a breaking change.
Please let me know if it is not.

Description of how you validated changes

Unit tests and integ tests are added to verify that the imported userPool and clinet can be used.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

…giot-idp-use-imported-user-pool

TheRealAmazonKendra

Thank you for your contribution. Please see my comments inline.

TheRealAmazonKendra · 2024-06-11T18:26:04Z

packages/@aws-cdk/aws-cognito-identitypool-alpha/README.md


-declare const userPool: UserPool;
-declare const userPoolClient: UserPoolClient;
+// If you use a previously defined Cognito User Pool, use the `fromUserPoolAttributes` method instead of `fromUserPoolId` or `fromUserPoolArn`.


Please specify why this is the case. This comment alone gives me pause about these changes.

packages/@aws-cdk/aws-cognito-identitypool-alpha/lib/identitypool.ts

TheRealAmazonKendra · 2024-06-11T18:33:23Z

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

      public readonly userPoolArn = userPoolArn;
      public readonly userPoolId = userPoolId;
+
+      // In the UserPool construct, the userPoolProviderName is a required property but it is constructed from the arn and id of the user pool.


I'm not sure I'm getting this. If the name is constructed from the ARN and ID of the userpool, why can't we construct this on import functions so that it's accessible to users?

Also, wouldn't the ID be a subset of the ARN ?

TheRealAmazonKendra · 2024-06-11T20:11:36Z

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

+   * Import an existing user pool into the stack.
+   */
+  public static fromUserPoolAttributes(scope: Construct, id: string, attrs: UserPoolAttributes): IUserPool {
+    if (!attrs.userPoolArn && !attrs.userPoolId) {


I'm not sure that this function is really needed here. The fromId and fromArn functions should be able to assemble and return all of the missing attributes.

Correct me if I'm wrong but the attributes for a UserPool are:

Arn: arn:<partition>:cognito-idp:<region>:<account>:userpool/<userpoolId> ProviderName: cognito-idp.<region>.<url-suffix>/<userpoolId> ProviderURL: https://cognito-idp.<region>.<url-suffix>/userpoolId/.well-known/jwks.json (this is listed as the token signing key url, which I think is what's being referred to here?) UserPoolId: userpoolId

So, given that, you could add each of the missing fields as attributes in the IUserPool and then construct each of these (assuming same region as in the id for fromId) in the existing fromXxx functions so that you don't even need to create a new function.

Doing so would mitigate every one of my comments above.

Pull request has been modified.

sakurai-ryo · 2024-06-12T14:35:19Z

Hi @TheRealAmazonKendra
Thanks for the review.
Really sorry. I got mixed up with other PRs and misunderstood.
I fixed it by deleting the fromUserPoolAttributes method and constructing the ProviderName within the fromUserPoolArn method.
Also, I updated the PR description.

Leo10Gama

Thanks for the changes! Left a few more comments looking for a bit more clarification on these changes.

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

Leo10Gama · 2024-08-29T21:55:45Z

packages/aws-cdk-lib/aws-cognito/test/user-pool.test.ts

-    const stack = new Stack();
+    const stack = new Stack(undefined, undefined, {
+      env: { region: 'us-east-1', account: '0123456789012' },
+    });


Can we keep the stack identical for this test, or does it need these additional properties to work? If so, that would probably make these breaking changes. If not, can we update the logic that checks for the new userPoolProviderName so that it works without additional props?

Leo10Gama · 2024-08-29T21:58:04Z

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

  readonly userPoolArn: string;

+  /**
+   * User pool provider name


Can we update the docstring to be more similar to some of the other attributes? Feels redundant to have an attribute userPoolProviderName with the description User pool provider name.

Leo10Gama · 2024-08-30T19:04:55Z

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


    const userPoolId = arnParts.resourceName;
+    // ex) cognito-idp.us-east-1.amazonaws.com/us-east-1_abcdefghi
+    const providerName = `cognito-idp.${Stack.of(scope).region}.amazonaws.com/${userPoolId}`;


The formation logic of this string should be changed, since amazonaws.com is a partition that could be different for different users. Can you use the arnParts attributes for this instead? Something along the lines of:

`cognito-idp.${arnParts.region}.${arnParts.partition}/${userPoolId}`;

Pull request has been modified.

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.

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

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

paulhcsun

Nice work, thanks @sakurai-ryo and @Leo10Gama both!

paulhcsun · 2024-09-18T21:14:26Z

@Mergifyio update

mergify · 2024-09-18T21:14:35Z

update

❌ Mergify doesn't have permission to update

Details

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/github-merit-badger.yml without workflows permission

paulhcsun · 2024-09-18T21:15:00Z

@Mergifyio update

mergify · 2024-09-18T21:15:06Z

update

☑️ Nothing to do

Details

#commits-behind > 0 [📌 update requirement]
-closed [📌 update requirement]
-conflict [📌 update requirement]
queue-position = -1 [📌 update requirement]

Pull request has been modified.

mergify · 2024-09-18T21:45:39Z

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-09-18T22:16:32Z

AWS CodeBuild CI Report

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

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

mergify · 2024-09-18T22:17:02Z

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

github-actions · 2024-09-18T22:17:54Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

fix: cannot use imported userpool and client

2e346b2

github-actions bot added star-contributor [Pilot] contributed between 25-49 PRs to the CDK bug This issue is a bug. effort/medium Medium work item – several days of effort p1 labels Jun 1, 2024

aws-cdk-automation requested a review from a team June 1, 2024 13:44

Merge branch 'main' of https://github.com/sakurai-ryo/aws-cdk into co…

425c407

…giot-idp-use-imported-user-pool

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jun 1, 2024

github-actions bot mentioned this pull request Jun 3, 2024

Weekly PR metrics report #30434

Closed

TheRealAmazonKendra previously requested changes Jun 11, 2024

View reviewed changes

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

fix: delete unused method

2deebb8

github-actions bot added p2 and removed p1 labels Jun 12, 2024

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

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

Monthly PR metrics report #30715

Closed

Leo10Gama suggested changes Aug 29, 2024

View reviewed changes

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

Leo10Gama previously requested changes Aug 30, 2024

View reviewed changes

Merge branch 'main' into cogiot-idp-use-imported-user-pool

8ac1151

Leonardo Gama added 7 commits September 15, 2024 13:33

Implement requested changes

ecbb0b8

Fix failing test

b204ab9

Satisfy linter

2cae39b

Update snapshot

3d54b8b

Update logic

0cc57d4

Satisfy linter

d6df29f

Update integ tests (failing)

1a5a93a

aws-cdk-automation previously requested changes Sep 17, 2024

View reviewed changes

Integ test passes now

d88d532

Leonardo Gama added 2 commits September 17, 2024 13:39

Update snapshot

8685878

ACTUALLY update the integ test

b46a645

paulhcsun reviewed Sep 18, 2024

View reviewed changes

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

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

1ca04d3

paulhcsun previously approved these changes Sep 18, 2024

View reviewed changes

Merge branch 'main' into cogiot-idp-use-imported-user-pool

40be4b8

paulhcsun approved these changes Sep 18, 2024

View reviewed changes

Merge branch 'main' into cogiot-idp-use-imported-user-pool

c627419

mergify bot merged commit 0fdd6a9 into aws:main Sep 18, 2024

github-actions bot locked as resolved and limited conversation to collaborators Sep 18, 2024

Conversation

sakurai-ryo commented Jun 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sakurai-ryo commented Jun 12, 2024

Uh oh!

Leo10Gama left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

paulhcsun left a comment

Choose a reason for hiding this comment

Uh oh!

paulhcsun commented Sep 18, 2024

Uh oh!

mergify bot commented Sep 18, 2024

❌ Mergify doesn't have permission to update

Uh oh!

paulhcsun commented Sep 18, 2024

Uh oh!

mergify bot commented Sep 18, 2024

☑️ Nothing to do

Uh oh!

mergify bot commented Sep 18, 2024

Uh oh!

aws-cdk-automation commented Sep 18, 2024

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Sep 18, 2024

Uh oh!

github-actions bot commented Sep 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sakurai-ryo commented Jun 1, 2024 •

edited

Loading