fix(iam): attaching a policy is not idempotent with imported resources (

aws#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 aws#28101. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
chenjane-dev · Dec 5, 2023 · 9aa0377 · 9aa0377
1 parent 995beb1
commit 9aa0377
Show file tree

Hide file tree

Showing 13 changed files with 258 additions and 48 deletions.
diff --git a/....managed-policy.js.snapshot/ManagedPolicyIntegDefaultTestDeployAssert27007DC6.assets.json b/....managed-policy.js.snapshot/ManagedPolicyIntegDefaultTestDeployAssert27007DC6.assets.json
diff --git a/...test/aws-iam/test/integ.managed-policy.js.snapshot/aws-cdk-iam-managed-policy.assets.json b/...test/aws-iam/test/integ.managed-policy.js.snapshot/aws-cdk-iam-managed-policy.assets.json
diff --git a/...st/aws-iam/test/integ.managed-policy.js.snapshot/aws-cdk-iam-managed-policy.template.json b/...st/aws-iam/test/integ.managed-policy.js.snapshot/aws-cdk-iam-managed-policy.template.json
@@ -25,6 +25,9 @@
   "OneManagedPolicy77F9185F": {
    "Type": "AWS::IAM::ManagedPolicy",
    "Properties": {
+    "Description": "My Policy",
+    "ManagedPolicyName": "Default",
+    "Path": "/some/path/",
     "PolicyDocument": {
      "Statement": [
       {
@@ -45,9 +48,11 @@
      ],
      "Version": "2012-10-17"
     },
-    "Description": "My Policy",
-    "ManagedPolicyName": "Default",
-    "Path": "/some/path/",
+    "Roles": [
+     {
+      "Ref": "Role1ABCC5F0"
+     }
+    ],
     "Users": [
      {
       "Ref": "MyUserDC45028B"
@@ -58,6 +63,8 @@
   "TwoManagedPolicy7E701864": {
    "Type": "AWS::IAM::ManagedPolicy",
    "Properties": {
+    "Description": "",
+    "Path": "/",
     "PolicyDocument": {
      "Statement": [
       {
@@ -77,9 +84,7 @@
       }
      ],
      "Version": "2012-10-17"
-    },
-    "Description": "",
-    "Path": "/"
+    }
    }
   },
   "Role1ABCC5F0": {

diff --git a/...ws-cdk-testing/framework-integ/test/aws-iam/test/integ.managed-policy.js.snapshot/cdk.out b/...ws-cdk-testing/framework-integ/test/aws-iam/test/integ.managed-policy.js.snapshot/cdk.out
diff --git a/...cdk-testing/framework-integ/test/aws-iam/test/integ.managed-policy.js.snapshot/integ.json b/...cdk-testing/framework-integ/test/aws-iam/test/integ.managed-policy.js.snapshot/integ.json
diff --git a/...-testing/framework-integ/test/aws-iam/test/integ.managed-policy.js.snapshot/manifest.json b/...-testing/framework-integ/test/aws-iam/test/integ.managed-policy.js.snapshot/manifest.json
diff --git a/...-cdk-testing/framework-integ/test/aws-iam/test/integ.managed-policy.js.snapshot/tree.json b/...-cdk-testing/framework-integ/test/aws-iam/test/integ.managed-policy.js.snapshot/tree.json
diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-iam/test/integ.managed-policy.ts b/packages/@aws-cdk-testing/framework-integ/test/aws-iam/test/integ.managed-policy.ts
@@ -27,6 +27,12 @@ const role = new Role(stack, 'Role', { assumedBy: new AccountRootPrincipal() });
 role.grantAssumeRole(policy.grantPrincipal);
 Grant.addToPrincipal({ actions: ['iam:*'], resourceArns: [role.roleArn], grantee: policy2 });
 
+policy.attachToRole(role);
+
+// Idempotent with imported roles, see https://github.com/aws/aws-cdk/issues/28101
+const importedRole = Role.fromRoleArn(stack, 'ImportedRole', role.roleArn);
+policy.attachToRole(importedRole);
+
 new IntegTest(app, 'ManagedPolicyInteg', {
   testCases: [stack],
 });
diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-iam/test/integ.role.ts b/packages/@aws-cdk-testing/framework-integ/test/aws-iam/test/integ.role.ts
@@ -16,6 +16,10 @@ const policy = new Policy(stack, 'HelloPolicy', { policyName: 'Default' });
 policy.addStatements(new PolicyStatement({ actions: ['ec2:*'], resources: ['*'] }));
 policy.attachToRole(role);
 
+// Idempotent with imported roles, see https://github.com/aws/aws-cdk/issues/28101
+const importedRole = Role.fromRoleArn(stack, 'TestImportedRole', role.roleArn);
+policy.attachToRole(importedRole);
+
 // Role with an external ID
 new Role(stack, 'TestRole2', {
   assumedBy: new AccountRootPrincipal(),

diff --git a/packages/aws-cdk-lib/aws-iam/lib/managed-policy.ts b/packages/aws-cdk-lib/aws-iam/lib/managed-policy.ts
@@ -282,23 +282,23 @@ export class ManagedPolicy extends Resource implements IManagedPolicy, IGrantabl
    * Attaches this policy to a user.
    */
   public attachToUser(user: IUser) {
-    if (this.users.find(u => u === user)) { return; }
+    if (this.users.find(u => u.userArn === user.userArn)) { return; }
     this.users.push(user);
   }
 
   /**
    * Attaches this policy to a role.
    */
   public attachToRole(role: IRole) {
-    if (this.roles.find(r => r === role)) { return; }
+    if (this.roles.find(r => r.roleArn === role.roleArn)) { return; }
     this.roles.push(role);
   }
 
   /**
    * Attaches this policy to a group.
    */
   public attachToGroup(group: IGroup) {
-    if (this.groups.find(g => g === group)) { return; }
+    if (this.groups.find(g => g.groupArn === group.groupArn)) { return; }
     this.groups.push(group);
   }