From 876ed8ad1726d6b77e7450eadbd1a4ded8236544 Mon Sep 17 00:00:00 2001
From: Rico Huijbers <rix0rrr@gmail.com>
Date: Tue, 5 Apr 2022 17:58:17 +0200
Subject: [PATCH] fix(iam): policies aren't minimized as far as possible
 (#19764)

IAM Policies were being correctly minimized; however, the minimization
was being performed in one pass across all statements.

It can be that after one pass, statements have ended up in forms that
allow for more merging. Example:

```
[{ A1, R1 }, { A2, R1 }, { A1, R2 }, { A2, R2 }]
// -> (pass one, combine actions)
[{ [A1, A2], R1}, { [A1, A2], R2 }]
// -> (pass two, combine resources)
[{ [A1, A2], [R1, R2] }]
```

Change to perform minimization passes until nothing changes anymore.

Fixes #19751.


----

### 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 `cdk-integ` to deploy the infrastructure and generate the snapshot (i.e. `cdk-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*
---
 .../aws-iam/lib/private/merge-statements.ts   | 39 +++++++++++--------
 .../aws-iam/test/merge-statements.test.ts     | 30 ++++++++++++++
 2 files changed, 53 insertions(+), 16 deletions(-)

diff --git a/packages/@aws-cdk/aws-iam/lib/private/merge-statements.ts b/packages/@aws-cdk/aws-iam/lib/private/merge-statements.ts
index f7ef33b1ea026..c79ecd6a8a814 100644
--- a/packages/@aws-cdk/aws-iam/lib/private/merge-statements.ts
+++ b/packages/@aws-cdk/aws-iam/lib/private/merge-statements.ts
@@ -18,26 +18,33 @@ import { StatementSchema, normalizeStatement, IamValue } from './postprocess-pol
 export function mergeStatements(statements: StatementSchema[]): StatementSchema[] {
   const compStatements = statements.map(makeComparable);
 
-  let i = 0;
-  while (i < compStatements.length) {
-    let didMerge = false;
-
-    for (let j = i + 1; j < compStatements.length; j++) {
-      const merged = tryMerge(compStatements[i], compStatements[j]);
-      if (merged) {
-        compStatements[i] = merged;
-        compStatements.splice(j, 1);
-        didMerge = true;
-        break;
+  // Keep trying until nothing changes anymore
+  while (onePass()) { /* again */ }
+  return compStatements.map(renderComparable);
+
+  // Do one optimization pass, return 'true' if we merged anything
+  function onePass() {
+    let ret = false;
+    let i = 0;
+    while (i < compStatements.length) {
+      let didMerge = false;
+
+      for (let j = i + 1; j < compStatements.length; j++) {
+        const merged = tryMerge(compStatements[i], compStatements[j]);
+        if (merged) {
+          compStatements[i] = merged;
+          compStatements.splice(j, 1);
+          ret = didMerge = true;
+          break;
+        }
       }
-    }
 
-    if (!didMerge) {
-      i++;
+      if (!didMerge) {
+        i++;
+      }
     }
+    return ret;
   }
-
-  return compStatements.map(renderComparable);
 }
 
 /**
diff --git a/packages/@aws-cdk/aws-iam/test/merge-statements.test.ts b/packages/@aws-cdk/aws-iam/test/merge-statements.test.ts
index f3114955ecd61..061db0e134d02 100644
--- a/packages/@aws-cdk/aws-iam/test/merge-statements.test.ts
+++ b/packages/@aws-cdk/aws-iam/test/merge-statements.test.ts
@@ -441,6 +441,36 @@ test('fail merging typed and untyped principals', () => {
   ]);
 });
 
+test('keep merging even if it requires multiple passes', () => {
+  // [A, R1], [B, R1], [A, R2], [B, R2]
+  // -> [{A, B}, R1], [{A, B], R2]
+  // -> [{A, B}, {R1, R2}]
+  assertMerged([
+    new iam.PolicyStatement({
+      actions: ['service:A'],
+      resources: ['R1'],
+    }),
+    new iam.PolicyStatement({
+      actions: ['service:B'],
+      resources: ['R1'],
+    }),
+    new iam.PolicyStatement({
+      actions: ['service:A'],
+      resources: ['R2'],
+    }),
+    new iam.PolicyStatement({
+      actions: ['service:B'],
+      resources: ['R2'],
+    }),
+  ], [
+    {
+      Effect: 'Allow',
+      Action: ['service:A', 'service:B'],
+      Resource: ['R1', 'R2'],
+    },
+  ]);
+});
+
 function assertNoMerge(statements: iam.PolicyStatement[]) {
   const app = new App();
   const stack = new Stack(app, 'Stack');