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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
Mar 27, 2021
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
60cb31a
fix(cognito): imported userpool not retraining arn environment
DaWyz Mar 21, 2021
ec3aad4
validate resourceName for UserPool.fromUserPoolArn
DaWyz Mar 23, 2021
dadc8cc
Move ImportedUserPool class within fromUserPoolArn method
DaWyz Mar 26, 2021
9ef5d89
refactor importedUserPool class
DaWyz Mar 26, 2021
677fbb0
Merge branch 'master' into DaWyz/fix-imported-userpool-env
mergify[bot] Mar 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 47 additions & 10 deletions packages/@aws-cdk/aws-cognito/lib/user-pool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,22 +660,32 @@ export class UserPool extends UserPoolBase {
* Import an existing user pool based on its id.
*/
public static fromUserPoolId(scope: Construct, id: string, userPoolId: string): IUserPool {
class Import extends UserPoolBase {
public readonly userPoolId = userPoolId;
public readonly userPoolArn = Stack.of(this).formatArn({
service: 'cognito-idp',
resource: 'userpool',
resourceName: userPoolId,
});
}
return new Import(scope, id);
let userPoolArn = Stack.of(scope).formatArn({
service: 'cognito-idp',
resource: 'userpool',
resourceName: userPoolId,
});

return new ImportedUserPool(scope, id, {
userPoolId: userPoolId,
userPoolArn: userPoolArn,
});
}

/**
* 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!);
let userPoolId = Stack.of(scope).parseArn(userPoolArn).resourceName;

if (!userPoolId) {
throw new Error('UserPool.fromUserPoolArn requires an arn with a resourceName.');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

return new ImportedUserPool(scope, id, {
userPoolId: userPoolId,
userPoolArn: userPoolArn,
});
}

/**
Expand Down Expand Up @@ -1041,6 +1051,33 @@ export class UserPool extends UserPoolBase {
}
}

interface UserPoolAttributes {
/**
* The physical ID of this user pool resource
*/
userPoolId: string;
/**
* The ARN of the user pool
*/
userPoolArn: string
}

class ImportedUserPool extends UserPoolBase {
Copy link
Contributor

Choose a reason for hiding this comment

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

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(...));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@nija-at nija-at Mar 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

public readonly userPoolArn: string;
public readonly userPoolId: string;
constructor(scope: Construct, id: string, attrs: UserPoolAttributes) {
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;
}
}


function undefinedIfNoKeys(struct: object): object | undefined {
const allUndefined = Object.values(struct).every(val => val === undefined);
return allUndefined ? undefined : struct;
Expand Down
38 changes: 28 additions & 10 deletions packages/@aws-cdk/aws-cognito/test/user-pool.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,17 +211,35 @@ describe('User Pool', () => {
// WHEN
const pool = UserPool.fromUserPoolArn(stack, 'userpool', userPoolArn);
expect(pool.userPoolId).toEqual('test-user-pool');
expect(stack.resolve(pool.userPoolArn)).toEqual({
'Fn::Join': ['', [
'arn:',
{ Ref: 'AWS::Partition' },
':cognito-idp:',
{ Ref: 'AWS::Region' },
':',
{ Ref: 'AWS::AccountId' },
':userpool/test-user-pool',
]],
expect(stack.resolve(pool.userPoolArn)).toEqual('arn:aws:cognito-idp:us-east-1:0123456789012:userpool/test-user-pool');
});

test('import using arn without resourceName fails', () => {
// GIVEN
const stack = new Stack();
const userPoolArn = 'arn:aws:cognito-idp:us-east-1:0123456789012:*';

// WHEN
expect(() => {
UserPool.fromUserPoolArn(stack, 'userpool', userPoolArn);
}).toThrowError(/UserPool\.fromUserPoolArn requires an arn with a resourceName\./);
});

test('import from different account region using arn', () => {
// GIVEN
const userPoolArn = 'arn:aws:cognito-idp:us-east-1:0123456789012:userpool/test-user-pool';

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

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

Choose a reason for hiding this comment

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

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');

});

test('support tags', () => {
Expand Down