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

fix(cognito): changing installLatestAwsSdk breaks Client Secret reference #23798

Merged
merged 4 commits into from
Feb 16, 2023
Merged

fix(cognito): changing installLatestAwsSdk breaks Client Secret reference #23798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
837b8b0
fix(cognito): changing `installLatestAwsSdk` breaks Client Secret ref…
laurelmay Jan 23, 2023
ce0ddfb
Replace `onCreate` with `onUpdate`
laurelmay Jan 24, 2023
d5ff298
Merge branch 'main' into cognito-update-error
laurelmay Feb 15, 2023
2a98289
Merge branch 'main' into cognito-update-error
mergify[bot] Feb 16, 2023
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
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-cognito/lib/user-pool-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,7 +451,7 @@ export class UserPoolClient extends Resource implements IUserPoolClient {
'DescribeCognitoUserPoolClient',
{
resourceType: 'Custom::DescribeCognitoUserPoolClient',
onCreate: {
onUpdate: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should onCreate be completely removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! It seems to be relatively unknown that onCreate defaults to the value of on onUpdate when not specified (see @default for https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.custom_resources.AwsCustomResource.html#oncreate).

Please check me on this and confirm that the changes to integration test snapshots add Update but do not remove Create?

aws-cdk/packages/@aws-cdk/custom-resources/lib/aws-custom-resource/aws-custom-resource.ts

Lines 435 to 446 in 4c62f0b

const create = props.onCreate || props.onUpdate;
this.customResource = new cdk.CustomResource(this, 'Resource', {
resourceType: props.resourceType || 'Custom::AWS',
serviceToken: provider.functionArn,
pascalCaseProperties: true,
properties: {
create: create && this.encodeJson(create),
update: props.onUpdate && this.encodeJson(props.onUpdate),
delete: props.onDelete && this.encodeJson(props.onDelete),
installLatestAwsSdk,
},
});

I think this is actually a common bug (specifying onCreate but not onUpdate for AwsCustomResource), even within @aws-cdk/*.

Copy link
Contributor

Choose a reason for hiding this comment

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

I did not know that, thanks for the clarification!

Yep, OnCreate is still in the snapshots

region: Stack.of(this).region,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any circumstance where the custom resources wouldn't be in the same region? Probably a dumb question on my end, but I just want to check.

Copy link
Contributor Author

@laurelmay laurelmay Jan 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@TheRealAmazonKendra That's a great question and honestly I am not entirely sure... So the User Pool and the Client always have to be in the same region. There also isn't (currently) a fromXXX static method on UserPoolClient that lets you specify region (there's fromUserPoolClientId but not fromUserPoolClientArn). And while User Pool has a fromUserPoolArn and IUserPool has addClient, I actually feel like that would fail in CloudFormation since the pool is in another region.

And then fromUserPoolClientId actually has an implementation of get userPoolClientSecret() that always throws (probably because it doesn't take an IUserPool as a parameter to make the API call).

I am not sure that there's any situation where the region parameter's value could be different from the region where the pool/client exist (and have things actually work with the interfaces that exist today).

So I guess region could actually probably be omitted? But frankly, I just copied what had been done for onCreate.

Which, I think, means the options are to either:

  1. Keep region as-is
  2. Remove region from onCreate and onUpdate
  3. Change both to this.userPool.env.region (which at the moment should always be the stack's region)

Let me know which you'd prefer in this PR and if you'd like a second PR to make another change!

service: 'CognitoIdentityServiceProvider',
action: 'describeUserPoolClient',
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-cognito/test/integ.user-pool-client-explicit-props.js.snapshot/cdk.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"version":"22.0.0"}
{"version":"29.0.0"}
6 changes: 3 additions & 3 deletions ...-pool-client-explicit-props.js.snapshot/integ-user-pool-client-explicit-props.assets.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "22.0.0",
"version": "29.0.0",
"files": {
"a268caa53756f51bda8ad5f499be4ed8484a81b314811806fbb66f874837c476": {
"source": {
Expand All @@ -14,15 +14,15 @@
}
}
},
"be03a3e392c69c83e42480095a0bbc5f0bb315abc56be4db3af345689cf1505a": {
"734cf8b4d966e3e725d80eb9076268d2066da8cd9e460447734d6f661bb4fba7": {
"source": {
"path": "integ-user-pool-client-explicit-props.template.json",
"packaging": "file"
},
"destinations": {
"current_account-current_region": {
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
"objectKey": "be03a3e392c69c83e42480095a0bbc5f0bb315abc56be4db3af345689cf1505a.json",
"objectKey": "734cf8b4d966e3e725d80eb9076268d2066da8cd9e460447734d6f661bb4fba7.json",
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
}
}
Expand Down
24 changes: 24 additions & 0 deletions ...ool-client-explicit-props.js.snapshot/integ-user-pool-client-explicit-props.template.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,30 @@
]
]
},
"Update": {
"Fn::Join": [
"",
[
"{\"region\":\"",
{
"Ref": "AWS::Region"
},
"\",\"service\":\"CognitoIdentityServiceProvider\",\"action\":\"describeUserPoolClient\",\"parameters\":{\"UserPoolId\":\"",
{
"Ref": "myuserpool01998219"
},
"\",\"ClientId\":\"",
{
"Ref": "myuserpoolmyuserpoolclientAFB2274E"
},
"\"},\"physicalResourceId\":{\"id\":\"",
{
"Ref": "myuserpoolmyuserpoolclientAFB2274E"
},
"\"}}"
]
]
},
"InstallLatestAwsSdk": false
},
"DependsOn": [
Expand Down
2 changes: 1 addition & 1 deletion ...es/@aws-cdk/aws-cognito/test/integ.user-pool-client-explicit-props.js.snapshot/integ.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "22.0.0",
"version": "29.0.0",
"testCases": {
"integ.user-pool-client-explicit-props": {
"stacks": [
Expand Down
4 changes: 2 additions & 2 deletions ...@aws-cdk/aws-cognito/test/integ.user-pool-client-explicit-props.js.snapshot/manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "22.0.0",
"version": "29.0.0",
"artifacts": {
"integ-user-pool-client-explicit-props.assets": {
"type": "cdk:asset-manifest",
Expand All @@ -17,7 +17,7 @@
"validateOnSynth": false,
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-deploy-role-${AWS::AccountId}-${AWS::Region}",
"cloudFormationExecutionRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-cfn-exec-role-${AWS::AccountId}-${AWS::Region}",
"stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/be03a3e392c69c83e42480095a0bbc5f0bb315abc56be4db3af345689cf1505a.json",
"stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/734cf8b4d966e3e725d80eb9076268d2066da8cd9e460447734d6f661bb4fba7.json",
"requiresBootstrapStackVersion": 6,
"bootstrapStackVersionSsmParameter": "/cdk-bootstrap/hnb659fds/version",
"additionalDependencies": [
Expand Down
2 changes: 1 addition & 1 deletion ...ges/@aws-cdk/aws-cognito/test/integ.user-pool-client-explicit-props.js.snapshot/tree.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,7 @@
"path": "Tree",
"constructInfo": {
"fqn": "constructs.Construct",
"version": "10.1.189"
"version": "10.1.216"
}
}
},
Expand Down
253 changes: 253 additions & 0 deletions ....snapshot/asset.a268caa53756f51bda8ad5f499be4ed8484a81b314811806fbb66f874837c476/index.js
View file
Open in desktop

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-cognito/test/integ.user-pool-client-secret.js.snapshot/cdk.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"version":"29.0.0"}
32 changes: 32 additions & 0 deletions .../test/integ.user-pool-client-secret.js.snapshot/integ-user-pool-client-secret.assets.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
{
"version": "29.0.0",
"files": {
"a268caa53756f51bda8ad5f499be4ed8484a81b314811806fbb66f874837c476": {
"source": {
"path": "asset.a268caa53756f51bda8ad5f499be4ed8484a81b314811806fbb66f874837c476",
"packaging": "zip"
},
"destinations": {
"current_account-current_region": {
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
"objectKey": "a268caa53756f51bda8ad5f499be4ed8484a81b314811806fbb66f874837c476.zip",
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
}
}
},
"23470f9d0cab672bb28f93d0b4cc009f579dd49d76249b541091db889df6aaae": {
"source": {
"path": "integ-user-pool-client-secret.template.json",
"packaging": "file"
},
"destinations": {
"current_account-current_region": {
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
"objectKey": "23470f9d0cab672bb28f93d0b4cc009f579dd49d76249b541091db889df6aaae.json",
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
}
}
}
},
"dockerImages": {}
}
Loading