fix(iam): referencing the same immutable role twice makes it mutable (#…

…20497)

The solution I went with in this PR was to try and keep the provided id
set on the `ImmutableRole` instead of the `Import` role. This should
also keep backwards compatibility by only changing the id of the
`Import` role if we are returning an `ImmutableRole`.

fixes #7255


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

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

packages/@aws-cdk/aws-iam/lib/role.ts

@@ -277,15 +277,20 @@ export class Role extends Resource implements IRole {
       throw new Error('\'addGrantsToResources\' can only be passed if \'mutable: false\'');
     }
 
-    const importedRole = new Import(scope, id);
-    const roleArnAndScopeStackAccountComparison = Token.compareStrings(importedRole.env.account, scopeStack.account);
+    const roleArnAndScopeStackAccountComparison = Token.compareStrings(roleAccount ?? '', scopeStack.account);
     const equalOrAnyUnresolved = roleArnAndScopeStackAccountComparison === TokenComparison.SAME ||
       roleArnAndScopeStackAccountComparison === TokenComparison.BOTH_UNRESOLVED ||
       roleArnAndScopeStackAccountComparison === TokenComparison.ONE_UNRESOLVED;
+
+    // if we are returning an immutable role then the 'importedRole' is just a throwaway construct
+    // so give it a different id
+    const mutableRoleId = (options.mutable !== false && equalOrAnyUnresolved) ? id : `MutableRole${id}`;
+    const importedRole = new Import(scope, mutableRoleId);
+
     // we only return an immutable Role if both accounts were explicitly provided, and different
     return options.mutable !== false && equalOrAnyUnresolved
       ? importedRole
-      : new ImmutableRole(scope, `ImmutableRole${id}`, importedRole, options.addGrantsToResources ?? false);
+      : new ImmutableRole(scope, id, importedRole, options.addGrantsToResources ?? false);
   }
 
   /**
@@ -655,4 +660,4 @@ export interface WithoutPolicyUpdatesOptions {
    * @default false
    */
   readonly addGrantsToResources?: boolean;
-}
+}

packages/@aws-cdk/aws-iam/test/immutable-role.test.ts

@@ -1,4 +1,4 @@
-import { Template } from '@aws-cdk/assertions';
+import { Template, Match } from '@aws-cdk/assertions';
 import { Stack } from '@aws-cdk/core';
 import * as iam from '../lib';
 
@@ -53,6 +53,57 @@ describe('ImmutableRole', () => {
     });
   });
 
+  test('id of mutable role remains unchanged', () => {
+    iam.Role.fromRoleName(stack, 'TestRole123', 'my-role');
+    expect(stack.node.tryFindChild('TestRole123')).not.toBeUndefined();
+    expect(stack.node.tryFindChild('MutableRoleTestRole123')).toBeUndefined();
+  });
+
+  test('remains mutable when called multiple times', () => {
+    const user = new iam.User(stack, 'User');
+    const policy = new iam.Policy(stack, 'Policy', {
+      statements: [new iam.PolicyStatement({
+        resources: ['*'],
+        actions: ['s3:*'],
+      })],
+      users: [user],
+    });
+
+    function findRole(): iam.IRole {
+      const foundRole = stack.node.tryFindChild('MyRole') as iam.IRole;
+      if (foundRole) {
+        return foundRole;
+      }
+      return iam.Role.fromRoleArn(stack, 'MyRole', 'arn:aws:iam::12345:role/role-name', { mutable: false });
+    }
+
+    let foundRole = findRole();
+    foundRole.attachInlinePolicy(policy);
+    foundRole = findRole();
+    foundRole.attachInlinePolicy(policy);
+
+    expect(stack.node.tryFindChild('MutableRoleMyRole')).not.toBeUndefined();
+    Template.fromStack(stack).hasResourceProperties('AWS::IAM::Policy', {
+      'PolicyDocument': {
+        'Statement': [
+          {
+            'Action': 's3:*',
+            'Resource': '*',
+            'Effect': 'Allow',
+          },
+        ],
+        'Version': '2012-10-17',
+      },
+      'PolicyName': 'Policy23B91518',
+      'Roles': Match.absent(),
+      'Users': [
+        {
+          'Ref': 'User00B015A1',
+        },
+      ],
+    });
+  });
+
   test('ignores calls to addManagedPolicy', () => {
     mutableRole.addManagedPolicy({ managedPolicyArn: 'Arn1' });
 
@@ -117,4 +168,4 @@ describe('ImmutableRole', () => {
     new Construct(immutableRole as unknown as Construct, 'Child');
     new Construct(mutableRole.withoutPolicyUpdates() as unknown as Construct, 'Child2');
   });
-});
+});