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*

Author: corymhall

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