Skip to content

Commit 876ed8a

Browse files
authored
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*
1 parent 5ce0ad3 commit 876ed8a

File tree

2 files changed

+53
-16
lines changed

2 files changed

+53
-16
lines changed

packages/@aws-cdk/aws-iam/lib/private/merge-statements.ts

Lines changed: 23 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -18,26 +18,33 @@ import { StatementSchema, normalizeStatement, IamValue } from './postprocess-pol
1818
export function mergeStatements(statements: StatementSchema[]): StatementSchema[] {
1919
const compStatements = statements.map(makeComparable);
2020

21-
let i = 0;
22-
while (i < compStatements.length) {
23-
let didMerge = false;
24-
25-
for (let j = i + 1; j < compStatements.length; j++) {
26-
const merged = tryMerge(compStatements[i], compStatements[j]);
27-
if (merged) {
28-
compStatements[i] = merged;
29-
compStatements.splice(j, 1);
30-
didMerge = true;
31-
break;
21+
// Keep trying until nothing changes anymore
22+
while (onePass()) { /* again */ }
23+
return compStatements.map(renderComparable);
24+
25+
// Do one optimization pass, return 'true' if we merged anything
26+
function onePass() {
27+
let ret = false;
28+
let i = 0;
29+
while (i < compStatements.length) {
30+
let didMerge = false;
31+
32+
for (let j = i + 1; j < compStatements.length; j++) {
33+
const merged = tryMerge(compStatements[i], compStatements[j]);
34+
if (merged) {
35+
compStatements[i] = merged;
36+
compStatements.splice(j, 1);
37+
ret = didMerge = true;
38+
break;
39+
}
3240
}
33-
}
3441

35-
if (!didMerge) {
36-
i++;
42+
if (!didMerge) {
43+
i++;
44+
}
3745
}
46+
return ret;
3847
}
39-
40-
return compStatements.map(renderComparable);
4148
}
4249

4350
/**

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -441,6 +441,36 @@ test('fail merging typed and untyped principals', () => {
441441
]);
442442
});
443443

444+
test('keep merging even if it requires multiple passes', () => {
445+
// [A, R1], [B, R1], [A, R2], [B, R2]
446+
// -> [{A, B}, R1], [{A, B], R2]
447+
// -> [{A, B}, {R1, R2}]
448+
assertMerged([
449+
new iam.PolicyStatement({
450+
actions: ['service:A'],
451+
resources: ['R1'],
452+
}),
453+
new iam.PolicyStatement({
454+
actions: ['service:B'],
455+
resources: ['R1'],
456+
}),
457+
new iam.PolicyStatement({
458+
actions: ['service:A'],
459+
resources: ['R2'],
460+
}),
461+
new iam.PolicyStatement({
462+
actions: ['service:B'],
463+
resources: ['R2'],
464+
}),
465+
], [
466+
{
467+
Effect: 'Allow',
468+
Action: ['service:A', 'service:B'],
469+
Resource: ['R1', 'R2'],
470+
},
471+
]);
472+
});
473+
444474
function assertNoMerge(statements: iam.PolicyStatement[]) {
445475
const app = new App();
446476
const stack = new Stack(app, 'Stack');

0 commit comments

Comments
 (0)