Skip to content

Commit 0abd0b5

Browse files
authored
fix(iam): attaching a policy is not idempotent with imported resources (#28129)
Fixes `attachToUser`, `attachToRole`, and `attachToGroup` for `Policy` and `ManagedPolicy` to use ARNs as a discriminant for resource equality to prevent duplicates on imported resources. Closes #28101. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent 4867294 commit 0abd0b5

13 files changed

+258
-48
lines changed

packages/@aws-cdk-testing/framework-integ/test/aws-iam/test/integ.managed-policy.js.snapshot/ManagedPolicyIntegDefaultTestDeployAssert27007DC6.assets.json

+1-1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/@aws-cdk-testing/framework-integ/test/aws-iam/test/integ.managed-policy.js.snapshot/aws-cdk-iam-managed-policy.assets.json

+3-3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/@aws-cdk-testing/framework-integ/test/aws-iam/test/integ.managed-policy.js.snapshot/aws-cdk-iam-managed-policy.template.json

+11-6
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,9 @@
2525
"OneManagedPolicy77F9185F": {
2626
"Type": "AWS::IAM::ManagedPolicy",
2727
"Properties": {
28+
"Description": "My Policy",
29+
"ManagedPolicyName": "Default",
30+
"Path": "/some/path/",
2831
"PolicyDocument": {
2932
"Statement": [
3033
{
@@ -45,9 +48,11 @@
4548
],
4649
"Version": "2012-10-17"
4750
},
48-
"Description": "My Policy",
49-
"ManagedPolicyName": "Default",
50-
"Path": "/some/path/",
51+
"Roles": [
52+
{
53+
"Ref": "Role1ABCC5F0"
54+
}
55+
],
5156
"Users": [
5257
{
5358
"Ref": "MyUserDC45028B"
@@ -58,6 +63,8 @@
5863
"TwoManagedPolicy7E701864": {
5964
"Type": "AWS::IAM::ManagedPolicy",
6065
"Properties": {
66+
"Description": "",
67+
"Path": "/",
6168
"PolicyDocument": {
6269
"Statement": [
6370
{
@@ -77,9 +84,7 @@
7784
}
7885
],
7986
"Version": "2012-10-17"
80-
},
81-
"Description": "",
82-
"Path": "/"
87+
}
8388
}
8489
},
8590
"Role1ABCC5F0": {

packages/@aws-cdk-testing/framework-integ/test/aws-iam/test/integ.managed-policy.js.snapshot/cdk.out

+1-1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/@aws-cdk-testing/framework-integ/test/aws-iam/test/integ.managed-policy.js.snapshot/integ.json

+1-1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/@aws-cdk-testing/framework-integ/test/aws-iam/test/integ.managed-policy.js.snapshot/manifest.json

+4-2
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/@aws-cdk-testing/framework-integ/test/aws-iam/test/integ.managed-policy.js.snapshot/tree.json

+41-28
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/@aws-cdk-testing/framework-integ/test/aws-iam/test/integ.managed-policy.ts

+6
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,12 @@ const role = new Role(stack, 'Role', { assumedBy: new AccountRootPrincipal() });
2727
role.grantAssumeRole(policy.grantPrincipal);
2828
Grant.addToPrincipal({ actions: ['iam:*'], resourceArns: [role.roleArn], grantee: policy2 });
2929

30+
policy.attachToRole(role);
31+
32+
// Idempotent with imported roles, see https://github.com/aws/aws-cdk/issues/28101
33+
const importedRole = Role.fromRoleArn(stack, 'ImportedRole', role.roleArn);
34+
policy.attachToRole(importedRole);
35+
3036
new IntegTest(app, 'ManagedPolicyInteg', {
3137
testCases: [stack],
3238
});

packages/@aws-cdk-testing/framework-integ/test/aws-iam/test/integ.role.ts

+4
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,10 @@ const policy = new Policy(stack, 'HelloPolicy', { policyName: 'Default' });
1616
policy.addStatements(new PolicyStatement({ actions: ['ec2:*'], resources: ['*'] }));
1717
policy.attachToRole(role);
1818

19+
// Idempotent with imported roles, see https://github.com/aws/aws-cdk/issues/28101
20+
const importedRole = Role.fromRoleArn(stack, 'TestImportedRole', role.roleArn);
21+
policy.attachToRole(importedRole);
22+
1923
// Role with an external ID
2024
new Role(stack, 'TestRole2', {
2125
assumedBy: new AccountRootPrincipal(),

packages/aws-cdk-lib/aws-iam/lib/managed-policy.ts

+3-3
Original file line numberDiff line numberDiff line change
@@ -282,23 +282,23 @@ export class ManagedPolicy extends Resource implements IManagedPolicy, IGrantabl
282282
* Attaches this policy to a user.
283283
*/
284284
public attachToUser(user: IUser) {
285-
if (this.users.find(u => u === user)) { return; }
285+
if (this.users.find(u => u.userArn === user.userArn)) { return; }
286286
this.users.push(user);
287287
}
288288

289289
/**
290290
* Attaches this policy to a role.
291291
*/
292292
public attachToRole(role: IRole) {
293-
if (this.roles.find(r => r === role)) { return; }
293+
if (this.roles.find(r => r.roleArn === role.roleArn)) { return; }
294294
this.roles.push(role);
295295
}
296296

297297
/**
298298
* Attaches this policy to a group.
299299
*/
300300
public attachToGroup(group: IGroup) {
301-
if (this.groups.find(g => g === group)) { return; }
301+
if (this.groups.find(g => g.groupArn === group.groupArn)) { return; }
302302
this.groups.push(group);
303303
}
304304

0 commit comments

Comments
 (0)