fix(cognito): imported userpool not retaining environment from arn #13715

DaWyz · 2021-03-21T07:29:15Z

Importing a UserPool using fromUserPoolArn() method would not retain the account/region from the ARN and would instead use the environment from the scope it is imported to.

Closes #13691

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

gitpod-io · 2021-03-21T07:29:17Z

nija-at

@DaWyz - Thanks for submitting this PR.

See my comments below.

nija-at · 2021-03-22T13:39:37Z

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

+  userPoolArn: string
+}
+
+class ImportedUserPool extends UserPoolBase {


Could you try and retain the original structure of keeping the 'Import' class local to the fromXXX() methods?

You should be able to move the full implementation to within fromUserPoolArn() and call this method from fromUserPoolId(). Something like -

public static fromUserPoolId(...) { return fromUserPoolArn(..., Stack.of(scope).formatArn(...)); }

@nija-at Are we sure we want to do this ?
I'm asking for the two following reasons:

I've been asked in the past to extract the Import class (That was for EventBus) and it's done that way in a few other modules (search for class Imported in the codebase).

I would need to find a different name for scope and id since I'm getting eslint(@typescript-eslint/no-shadow) eslint error.

I prefer the class to be inline since this acts as an added check to ensure that we don't export this class into the public API. If we extracted this out, it would be easy to just add an 'export' keyword and could easily be missed in reviews.
However if you feel strongly on extracting and keeping this separate, that's fine with me.

The shadow variable issue is minor. We can easily find different variable names.

However, I would either rename of not use the UserPoolAttributes interface. This is reserved for specific types of import APIs - https://github.com/aws/aws-cdk/blob/master/docs/DESIGN_GUIDELINES.md#from-attributes - and I wouldn't want this to be incorrectly used.

That makes sense. I'm moving the Imported class within the fromUserPoolArn. Thanks for the explanation @nija-at

nija-at · 2021-03-22T13:40:54Z

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

  }

  /**
   * Import an existing user pool based on its ARN.
   */
  public static fromUserPoolArn(scope: Construct, id: string, userPoolArn: string): IUserPool {
-    return UserPool.fromUserPoolId(scope, id, Stack.of(scope).parseArn(userPoolArn).resourceName!);
+    return new ImportedUserPool(scope, id, {
+      userPoolId: Stack.of(scope).parseArn(userPoolArn).resourceName!,


Avoid the !.

Instead add a conditional that throws an error is resourceName is undefined.

Updated the code to check for resourceName + added a test.

Pull request has been modified.

nija-at · 2021-03-25T12:20:54Z

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

+  userPoolArn: string
+}
+
+class ImportedUserPool extends UserPoolBase {


I prefer the class to be inline since this acts as an added check to ensure that we don't export this class into the public API. If we extracted this out, it would be easy to just add an 'export' keyword and could easily be missed in reviews.
However if you feel strongly on extracting and keeping this separate, that's fine with me.

The shadow variable issue is minor. We can easily find different variable names.

However, I would either rename of not use the UserPoolAttributes interface. This is reserved for specific types of import APIs - https://github.com/aws/aws-cdk/blob/master/docs/DESIGN_GUIDELINES.md#from-attributes - and I wouldn't want this to be incorrectly used.

nija-at · 2021-03-25T12:21:25Z

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

+    let userPoolId = Stack.of(scope).parseArn(userPoolArn).resourceName;
+
+    if (!userPoolId) {
+      throw new Error('UserPool.fromUserPoolArn requires an arn with a resourceName.');


Suggested change

throw new Error('UserPool.fromUserPoolArn requires an arn with a resourceName.');

throw new Error('invalid user pool ARN');

Pull request has been modified.

nija-at

Thanks for the change @DaWyz. A couple of small suggestions.

nija-at · 2021-03-26T14:52:12Z

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

+    class ImportedUserPool extends UserPoolBase {
+      public readonly userPoolArn: string;
+      public readonly userPoolId: string;
+      constructor(_scope: Construct, _id: string, attrs: {
+        userPoolId: string;
+        userPoolArn: string
+      }) {
+        const arnParts = Stack.of(_scope).parseArn(attrs.userPoolArn);
+        super(_scope, _id, {
+          account: arnParts.account,
+          region: arnParts.region,
+        });
+
+        this.userPoolArn = attrs.userPoolArn;
+        this.userPoolId = attrs.userPoolId;
+      }
+    }
+
+    return new ImportedUserPool(scope, id, {
+      userPoolId: userPoolId,
+      userPoolArn: userPoolArn,
+    });


Is this simpler? Fewer lines of code to achieve the same thing and removes the need for _scope and _id.

Suggested change

class ImportedUserPool extends UserPoolBase {

public readonly userPoolArn: string;

public readonly userPoolId: string;

constructor(_scope: Construct, _id: string, attrs: {

userPoolId: string;

userPoolArn: string

}) {

const arnParts = Stack.of(_scope).parseArn(attrs.userPoolArn);

super(_scope, _id, {

account: arnParts.account,

region: arnParts.region,

});

this.userPoolArn = attrs.userPoolArn;

this.userPoolId = attrs.userPoolId;

}

}

return new ImportedUserPool(scope, id, {

userPoolId: userPoolId,

userPoolArn: userPoolArn,

});

class ImportedUserPool extends UserPoolBase {

public readonly userPoolArn = userPoolArn;

public readonly userPoolId = userPoolId;

constructor() {

const arnParts = Stack.of(scope).parseArn(userPoolArn);

super(scope, id, {

account: arnParts.account,

region: arnParts.region,

});

}

}

return new ImportedUserPool();

nija-at · 2021-03-26T14:54:27Z

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

+    // WHEN
+    const pool = UserPool.fromUserPoolArn(stack, 'userpool', userPoolArn);
+    expect(pool.env.account).toEqual('0123456789012');
+    expect(pool.env.region).toEqual('us-east-1');


Can you also verify the arn -

Suggested change

expect(pool.env.region).toEqual('us-east-1');

expect(pool.env.region).toEqual('us-east-1');

expect(pool.userPoolArn).toEqual('arn:aws:cognito-idp:us-east-1:0123456789012:userpool/test-user-pool');

Pull request has been modified.

DaWyz · 2021-03-26T23:52:50Z

Updated the code following the comments !

mergify · 2021-03-27T08:25:41Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2021-03-27T09:00:37Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 677fbb0
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2021-03-27T09:00:57Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…ws#13715) Importing a `UserPool` using `fromUserPoolArn()` method would not retain the account/region from the ARN and would instead use the environment from the scope it is imported to. Closes aws#13691 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

fix(cognito): imported userpool not retraining arn environment

60cb31a

github-actions bot added the @aws-cdk/aws-cognito Related to Amazon Cognito label Mar 21, 2021

github-actions bot assigned nija-at Mar 21, 2021

nija-at previously requested changes Mar 22, 2021

View reviewed changes

nija-at changed the title ~~fix(cognito): imported userpool not retraining arn environment~~ fix(cognito): imported userpool not retaining environment from arn Mar 22, 2021

validate resourceName for UserPool.fromUserPoolArn

ec3aad4

nija-at previously requested changes Mar 25, 2021

View reviewed changes

Move ImportedUserPool class within fromUserPoolArn method

dadc8cc

nija-at previously requested changes Mar 26, 2021

View reviewed changes

refactor importedUserPool class

9ef5d89

DaWyz requested a review from nija-at March 26, 2021 23:52

nija-at approved these changes Mar 27, 2021

View reviewed changes

Merge branch 'master' into DaWyz/fix-imported-userpool-env

677fbb0

mergify bot merged commit aa9fd9c into aws:master Mar 27, 2021

DaWyz deleted the DaWyz/fix-imported-userpool-env branch March 27, 2021 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cognito): imported userpool not retaining environment from arn #13715

fix(cognito): imported userpool not retaining environment from arn #13715

DaWyz commented Mar 21, 2021

gitpod-io bot commented Mar 21, 2021 •

edited

Loading

nija-at left a comment

nija-at Mar 22, 2021

DaWyz Mar 23, 2021

nija-at Mar 25, 2021 •

edited

Loading

DaWyz Mar 26, 2021

nija-at Mar 22, 2021

DaWyz Mar 23, 2021

nija-at Mar 25, 2021 •

edited

Loading

nija-at Mar 25, 2021

nija-at left a comment

nija-at Mar 26, 2021

nija-at Mar 26, 2021

DaWyz commented Mar 26, 2021

mergify bot commented Mar 27, 2021

aws-cdk-automation commented Mar 27, 2021

mergify bot commented Mar 27, 2021

	throw new Error('UserPool.fromUserPoolArn requires an arn with a resourceName.');
	throw new Error('invalid user pool ARN');

	expect(pool.env.region).toEqual('us-east-1');
	expect(pool.env.region).toEqual('us-east-1');
	expect(pool.userPoolArn).toEqual('arn:aws:cognito-idp:us-east-1:0123456789012:userpool/test-user-pool');

fix(cognito): imported userpool not retaining environment from arn #13715

fix(cognito): imported userpool not retaining environment from arn #13715

Conversation

DaWyz commented Mar 21, 2021

gitpod-io bot commented Mar 21, 2021 • edited Loading

nija-at left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nija-at Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nija-at Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nija-at left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaWyz commented Mar 26, 2021

mergify bot commented Mar 27, 2021

aws-cdk-automation commented Mar 27, 2021

AWS CodeBuild CI Report

mergify bot commented Mar 27, 2021

gitpod-io bot commented Mar 21, 2021 •

edited

Loading

nija-at Mar 25, 2021 •

edited

Loading

nija-at Mar 25, 2021 •

edited

Loading