feat(iam): PolicyStatements can be frozen (#20911)

PolicyStatements now have a `freeze()` method, which prevents further
modification. `freeze()` is called just prior to rendering and other
statement manipulation.

This has two benefits:

- Construct authors can `freeze()` statements and be sure that consumer
  code holding a reference to the statement can no longer mutate it
  later.
- Third-party library authors that generate IAM statements lazily
  (specifically, `cdk-iam-floyd`) can hook into the `freeze()` method
  to do their generation.


----

### All Submissions:

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

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/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/main/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: rix0rrr

packages/@aws-cdk/aws-iam/lib/policy-document.ts

@@ -76,6 +76,7 @@ export class PolicyDocument implements cdk.IResolvable {
   }
 
   public resolve(context: cdk.IResolveContext): any {
+    this.freezeStatements();
     this._maybeMergeStatements(context.scope);
 
     // In the previous implementation of 'merge', sorting of actions/resources on
@@ -212,6 +213,7 @@ export class PolicyDocument implements cdk.IResolvable {
     const newDocs: PolicyDocument[] = [];
 
     // Maps final statements to original statements
+    this.freezeStatements();
     let statementsToOriginals = new Map(this.statements.map(s => [s, [s]]));
 
     // We always run 'mergeStatements' to minimize the policy before splitting.
@@ -298,4 +300,13 @@ export class PolicyDocument implements cdk.IResolvable {
   private shouldMerge(scope: IConstruct) {
     return this.minimize ?? cdk.FeatureFlags.of(scope).isEnabled(cxapi.IAM_MINIMIZE_POLICIES) ?? false;
   }
+
+  /**
+   * Freeze all statements
+   */
+  private freezeStatements() {
+    for (const statement of this.statements) {
+      statement.freeze();
+    }
+  }
 }

packages/@aws-cdk/aws-iam/lib/policy-statement.ts

@@ -69,28 +69,21 @@ export class PolicyStatement {
     return ret;
   }
 
-  /**
-   * Statement ID for this statement
-   */
-  public sid?: string;
-
-  /**
-   * Whether to allow or deny the actions in this statement
-   */
-  public effect: Effect;
-
   private readonly _action = new Array<string>();
   private readonly _notAction = new Array<string>();
   private readonly _principal: { [key: string]: any[] } = {};
   private readonly _notPrincipal: { [key: string]: any[] } = {};
   private readonly _resource = new Array<string>();
   private readonly _notResource = new Array<string>();
   private readonly _condition: { [key: string]: any } = { };
+  private _sid?: string;
+  private _effect: Effect;
   private principalConditionsJson?: string;
 
   // Hold on to those principals
   private readonly _principals = new Array<IPrincipal>();
   private readonly _notPrincipals = new Array<IPrincipal>();
+  private _frozen = false;
 
   constructor(props: PolicyStatementProps = {}) {
     // Validate actions
@@ -101,8 +94,8 @@ export class PolicyStatement {
       }
     }
 
-    this.sid = props.sid;
-    this.effect = props.effect || Effect.ALLOW;
+    this._sid = props.sid;
+    this._effect = props.effect || Effect.ALLOW;
 
     this.addActions(...props.actions || []);
     this.addNotActions(...props.notActions || []);
@@ -115,6 +108,36 @@ export class PolicyStatement {
     }
   }
 
+  /**
+   * Statement ID for this statement
+   */
+  public get sid(): string | undefined {
+    return this._sid;
+  }
+
+  /**
+   * Set Statement ID for this statement
+   */
+  public set sid(sid: string | undefined) {
+    this.assertNotFrozen('sid');
+    this._sid = sid;
+  }
+
+  /**
+   * Whether to allow or deny the actions in this statement
+   */
+  public get effect(): Effect {
+    return this._effect;
+  }
+
+  /**
+   * Set effect for this statement
+   */
+  public set effect(effect: Effect) {
+    this.assertNotFrozen('effect');
+    this._effect = effect;
+  }
+
   //
   // Actions
   //
@@ -127,6 +150,7 @@ export class PolicyStatement {
    * @param actions actions that will be allowed.
    */
   public addActions(...actions: string[]) {
+    this.assertNotFrozen('addActions');
     if (actions.length > 0 && this._notAction.length > 0) {
       throw new Error('Cannot add \'Actions\' to policy statement if \'NotActions\' have been added');
     }
@@ -142,6 +166,7 @@ export class PolicyStatement {
    * @param notActions actions that will be denied. All other actions will be permitted.
    */
   public addNotActions(...notActions: string[]) {
+    this.assertNotFrozen('addNotActions');
     if (notActions.length > 0 && this._action.length > 0) {
       throw new Error('Cannot add \'NotActions\' to policy statement if \'Actions\' have been added');
     }
@@ -167,6 +192,7 @@ export class PolicyStatement {
    * @param principals IAM principals that will be added
    */
   public addPrincipals(...principals: IPrincipal[]) {
+    this.assertNotFrozen('addPrincipals');
     this._principals.push(...principals);
     if (Object.keys(principals).length > 0 && Object.keys(this._notPrincipal).length > 0) {
       throw new Error('Cannot add \'Principals\' to policy statement if \'NotPrincipals\' have been added');
@@ -188,6 +214,7 @@ export class PolicyStatement {
    * @param notPrincipals IAM principals that will be denied access
    */
   public addNotPrincipals(...notPrincipals: IPrincipal[]) {
+    this.assertNotFrozen('addNotPrincipals');
     this._notPrincipals.push(...notPrincipals);
     if (Object.keys(notPrincipals).length > 0 && Object.keys(this._principal).length > 0) {
       throw new Error('Cannot add \'NotPrincipals\' to policy statement if \'Principals\' have been added');
@@ -280,6 +307,7 @@ export class PolicyStatement {
    * @param arns Amazon Resource Names (ARNs) of the resources that this policy statement applies to
    */
   public addResources(...arns: string[]) {
+    this.assertNotFrozen('addResources');
     if (arns.length > 0 && this._notResource.length > 0) {
       throw new Error('Cannot add \'Resources\' to policy statement if \'NotResources\' have been added');
     }
@@ -295,6 +323,7 @@ export class PolicyStatement {
    * @param arns Amazon Resource Names (ARNs) of the resources that this policy statement does not apply to
    */
   public addNotResources(...arns: string[]) {
+    this.assertNotFrozen('addNotResources');
     if (arns.length > 0 && this._resource.length > 0) {
       throw new Error('Cannot add \'NotResources\' to policy statement if \'Resources\' have been added');
     }
@@ -344,6 +373,7 @@ export class PolicyStatement {
    * ```
    */
   public addCondition(key: string, value: Condition) {
+    this.assertNotFrozen('addCondition');
     const existingValue = this._condition[key];
     this._condition[key] = existingValue ? { ...existingValue, ...value } : value;
   }
@@ -544,6 +574,29 @@ export class PolicyStatement {
     return { ...this._condition };
   }
 
+  /**
+   * Make the PolicyStatement immutable
+   *
+   * After calling this, any of the `addXxx()` methods will throw an exception.
+   *
+   * Libraries that lazily generate statement bodies can override this method to
+   * fill the actual PolicyStatement fields. Be aware that this method may be called
+   * multiple times.
+   */
+  public freeze(): PolicyStatement {
+    this._frozen = true;
+    return this;
+  }
+
+  /**
+   * Whether the PolicyStatement has been frozen
+   *
+   * The statement object is frozen when `freeze()` is called.
+   */
+  public get frozen(): boolean {
+    return this._frozen;
+  }
+
   /**
    * Estimate the size of this policy statement
    *
@@ -577,6 +630,15 @@ export class PolicyStatement {
       }
     }
   }
+
+  /**
+   * Throw an exception when the object is frozen
+   */
+  private assertNotFrozen(method: string) {
+    if (this._frozen) {
+      throw new Error(`${method}: freeze() has been called on this PolicyStatement previously, so it can no longer be modified`);
+    }
+  }
 }
 
 /**

packages/@aws-cdk/aws-iam/test/merge-statements.test.ts

@@ -471,6 +471,25 @@ test('keep merging even if it requires multiple passes', () => {
   ]);
 });
 
+test('lazily generated statements are merged correctly', () => {
+  assertMerged([
+    new LazyStatement((s) => {
+      s.addActions('service:A');
+      s.addResources('R1');
+    }),
+    new LazyStatement((s) => {
+      s.addActions('service:B');
+      s.addResources('R1');
+    }),
+  ], [
+    {
+      Effect: 'Allow',
+      Action: ['service:A', 'service:B'],
+      Resource: 'R1',
+    },
+  ]);
+});
+
 function assertNoMerge(statements: iam.PolicyStatement[]) {
   const app = new App();
   const stack = new Stack(app, 'Stack');
@@ -499,3 +518,17 @@ function assertMerged(statements: iam.PolicyStatement[], expected: any[]) {
 function assertMergedC(doMerge: boolean, statements: iam.PolicyStatement[], expected: any[]) {
   return doMerge ? assertMerged(statements, expected) : assertNoMerge(statements);
 }
+
+/**
+ * A statement that fills itself only when freeze() is called.
+ */
+class LazyStatement extends iam.PolicyStatement {
+  constructor(private readonly modifyMe: (x: iam.PolicyStatement) => void) {
+    super();
+  }
+
+  public freeze() {
+    this.modifyMe(this);
+    return super.freeze();
+  }
+}

packages/@aws-cdk/aws-iam/test/policy-statement.test.ts

@@ -1,5 +1,5 @@
 import { Stack } from '@aws-cdk/core';
-import { AnyPrincipal, Group, PolicyDocument, PolicyStatement } from '../lib';
+import { AnyPrincipal, Group, PolicyDocument, PolicyStatement, Effect } from '../lib';
 
 describe('IAM policy statement', () => {
 
@@ -214,4 +214,31 @@ describe('IAM policy statement', () => {
     expect(() => policyStatement.addNotPrincipals(group))
       .toThrow(/Cannot use an IAM Group as the 'Principal' or 'NotPrincipal' in an IAM Policy/);
   });
+
+
+  test('a frozen policy statement cannot be modified any more', () => {
+    // GIVEN
+    const statement = new PolicyStatement({
+      actions: ['action:a'],
+      resources: ['*'],
+    });
+    statement.freeze();
+
+    // WHEN
+    const modifications = [
+      () => statement.sid = 'asdf',
+      () => statement.effect = Effect.DENY,
+      () => statement.addActions('abc:def'),
+      () => statement.addNotActions('abc:def'),
+      () => statement.addResources('*'),
+      () => statement.addNotResources('*'),
+      () => statement.addPrincipals(new AnyPrincipal()),
+      () => statement.addNotPrincipals(new AnyPrincipal()),
+      () => statement.addCondition('key', 'value'),
+    ];
+
+    for (const mod of modifications) {
+      expect(mod).toThrow(/can no longer be modified/);
+    }
+  });
 });