feat(iam): avoid duplicate statements in policy documents (#2254)

Remove duplicate statements from policy documents

Fixes #1777
Fixes #2168

packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-code-build-multiple-inputs-outputs.expected.json

@@ -191,20 +191,6 @@
                 }
               ]
             },
-            {
-              "Action": [
-                "codebuild:BatchGetBuilds",
-                "codebuild:StartBuild",
-                "codebuild:StopBuild"
-              ],
-              "Effect": "Allow",
-              "Resource": {
-                "Fn::GetAtt": [
-                  "MyBuildProject30DB9D6E",
-                  "Arn"
-                ]
-              }
-            },
             {
               "Action": [
                 "codebuild:BatchGetBuilds",
@@ -531,39 +517,6 @@
                 }
               ]
             },
-            {
-              "Action": [
-                "s3:GetObject*",
-                "s3:GetBucket*",
-                "s3:List*",
-                "s3:DeleteObject*",
-                "s3:PutObject*",
-                "s3:Abort*"
-              ],
-              "Effect": "Allow",
-              "Resource": [
-                {
-                  "Fn::GetAtt": [
-                    "MyBucketF68F3FF0",
-                    "Arn"
-                  ]
-                },
-                {
-                  "Fn::Join": [
-                    "",
-                    [
-                      {
-                        "Fn::GetAtt": [
-                          "MyBucketF68F3FF0",
-                          "Arn"
-                        ]
-                      },
-                      "/*"
-                    ]
-                  ]
-                }
-              ]
-            },
             {
               "Action": [
                 "s3:GetObject*",

packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-code-commit-build.expected.json

@@ -278,20 +278,6 @@
                 ]
               }
             },
-            {
-              "Action": [
-                "codebuild:BatchGetBuilds",
-                "codebuild:StartBuild",
-                "codebuild:StopBuild"
-              ],
-              "Effect": "Allow",
-              "Resource": {
-                "Fn::GetAtt": [
-                  "MyBuildProject30DB9D6E",
-                  "Arn"
-                ]
-              }
-            },
             {
               "Action": [
                 "codebuild:BatchGetBuilds",

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

@@ -3,15 +3,15 @@ import { Default, RegionInfo } from '@aws-cdk/region-info';
 import { IPrincipal } from './principals';
 import { mergePrincipal } from './util';
 
-export class PolicyDocument extends cdk.Token {
+export class PolicyDocument extends cdk.Token implements cdk.IResolvedValuePostProcessor {
   private statements = new Array<PolicyStatement>();
 
   /**
    * Creates a new IAM policy document.
    * @param defaultDocument An IAM policy document to use as an initial
    * policy. All statements of this document will be copied in.
    */
-  constructor(private readonly baseDocument?: any) {
+  constructor(private readonly baseDocument: any = {}) {
     super();
   }
 
@@ -20,13 +20,40 @@ export class PolicyDocument extends cdk.Token {
       return undefined;
     }
 
-    const doc = this.baseDocument || { };
-    doc.Statement = doc.Statement || [ ];
-    doc.Version = doc.Version || '2012-10-17';
-    doc.Statement = doc.Statement.concat(this.statements);
+    const doc = {
+      ...this.baseDocument,
+      Statement: (this.baseDocument.Statement || []).concat(this.statements),
+      Version: this.baseDocument.Version || '2012-10-17'
+    };
+
     return doc;
   }
 
+  /**
+   * Removes duplicate statements
+   */
+  public postProcess(input: any, _context: cdk.ResolveContext): any {
+    if (!input || !input.Statement) {
+      return input;
+    }
+
+    const jsonStatements = new Set<string>();
+    const uniqueStatements: PolicyStatement[] = [];
+
+    for (const statement of input.Statement) {
+      const jsonStatement = JSON.stringify(statement);
+      if (!jsonStatements.has(jsonStatement)) {
+        uniqueStatements.push(statement);
+        jsonStatements.add(jsonStatement);
+      }
+    }
+
+    return {
+      ...input,
+      Statement: uniqueStatements
+    };
+  }
+
   get isEmpty(): boolean {
     return this.statements.length === 0;
   }
@@ -39,6 +66,11 @@ export class PolicyDocument extends cdk.Token {
     return this.statements.length;
   }
 
+  /**
+   * Adds a statement to the policy document.
+   *
+   * @param statement the statement to add.
+   */
   public addStatement(statement: PolicyStatement): PolicyDocument {
     this.statements.push(statement);
     return this;

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

@@ -212,30 +212,58 @@ export = {
   'statementCount returns the number of statement in the policy document'(test: Test) {
     const p = new PolicyDocument();
     test.equal(p.statementCount, 0);
-    p.addStatement(new PolicyStatement());
+    p.addStatement(new PolicyStatement().addAction('action1'));
     test.equal(p.statementCount, 1);
-    p.addStatement(new PolicyStatement());
+    p.addStatement(new PolicyStatement().addAction('action2'));
     test.equal(p.statementCount, 2);
     test.done();
   },
 
-  'the { AWS: "*" } principal is represented as `Anyone` or `AnyPrincipal`'(test: Test) {
-    const stack = new Stack();
-    const p = new PolicyDocument();
+  '{ AWS: "*" } principal': {
+    'is represented as `Anyone`'(test: Test) {
+      const stack = new Stack();
+      const p = new PolicyDocument();
 
-    p.addStatement(new PolicyStatement().addPrincipal(new Anyone()));
-    p.addStatement(new PolicyStatement().addPrincipal(new AnyPrincipal()));
-    p.addStatement(new PolicyStatement().addAnyPrincipal());
+      p.addStatement(new PolicyStatement().addPrincipal(new Anyone()));
 
-    test.deepEqual(stack.node.resolve(p), {
-      Statement: [
-        { Effect: 'Allow', Principal: '*' },
-        { Effect: 'Allow', Principal: '*' },
-        { Effect: 'Allow', Principal: '*' }
-      ],
-      Version: '2012-10-17'
-    });
-    test.done();
+      test.deepEqual(stack.node.resolve(p), {
+        Statement: [
+          { Effect: 'Allow', Principal: '*' }
+        ],
+        Version: '2012-10-17'
+      });
+      test.done();
+    },
+
+    'is represented as `AnyPrincipal`'(test: Test) {
+      const stack = new Stack();
+      const p = new PolicyDocument();
+
+      p.addStatement(new PolicyStatement().addPrincipal(new AnyPrincipal()));
+
+      test.deepEqual(stack.node.resolve(p), {
+        Statement: [
+          { Effect: 'Allow', Principal: '*' }
+        ],
+        Version: '2012-10-17'
+      });
+      test.done();
+    },
+
+    'is represented as `addAnyPrincipal`'(test: Test) {
+      const stack = new Stack();
+      const p = new PolicyDocument();
+
+      p.addStatement(new PolicyStatement().addAnyPrincipal());
+
+      test.deepEqual(stack.node.resolve(p), {
+        Statement: [
+          { Effect: 'Allow', Principal: '*' }
+        ],
+        Version: '2012-10-17'
+      });
+      test.done();
+    }
   },
 
   'addAwsPrincipal/addArnPrincipal are the aliases'(test: Test) {
@@ -425,4 +453,85 @@ export = {
       test.done();
     }
   },
+
+  'duplicate statements': {
+
+    'without tokens'(test: Test) {
+      // GIVEN
+      const stack = new Stack();
+      const p = new PolicyDocument();
+
+      const statement = new PolicyStatement()
+        .addResources('resource1', 'resource2')
+        .addActions('action1', 'action2')
+        .addServicePrincipal('service')
+        .addConditions({
+          a: {
+            b: 'c'
+          },
+          d: {
+            e: 'f'
+          }
+        });
+
+      // WHEN
+      p.addStatement(statement);
+      p.addStatement(statement);
+      p.addStatement(statement);
+
+      // THEN
+      test.equal(stack.node.resolve(p).Statement.length, 1);
+      test.done();
+    },
+
+    'with tokens'(test: Test) {
+      // GIVEN
+      const stack = new Stack();
+      const p = new PolicyDocument();
+
+      const statement1 = new PolicyStatement()
+        .addResource(new Token(() => 'resource').toString())
+        .addAction(new Token(() => 'action').toString());
+      const statement2 = new PolicyStatement()
+        .addResource(new Token(() => 'resource').toString())
+        .addAction(new Token(() => 'action').toString());
+
+      // WHEN
+      p.addStatement(statement1);
+      p.addStatement(statement2);
+
+      // THEN
+      test.equal(stack.node.resolve(p).Statement.length, 1);
+      test.done();
+    },
+
+    'with base document'(test: Test) {
+      // GIVEN
+      const stack = new Stack();
+
+      // WHEN
+      const p = new PolicyDocument({
+        Statement: [
+          {
+            Action: 'action',
+            Effect: 'Allow',
+            Resource: 'resource'
+          },
+          {
+            Action: 'action',
+            Effect: 'Allow',
+            Resource: 'resource'
+          }
+        ]
+      });
+
+      p.addStatement(new PolicyStatement()
+        .addAction('action')
+        .addResource('resource'));
+
+      // THEN
+      test.equal(stack.node.resolve(p).Statement.length, 1);
+      test.done();
+    }
+  }
 };

packages/@aws-cdk/aws-lambda/lib/log-retention.ts

@@ -39,16 +39,14 @@ export class LogRetention extends cdk.Construct {
       lambdaPurpose: 'LogRetention',
     });
 
-    if (provider.role && !provider.role.node.tryFindChild('DefaultPolicy')) { // Avoid duplicate statements
-      provider.role.addToPolicy(
-        new iam.PolicyStatement()
-          .addActions('logs:PutRetentionPolicy', 'logs:DeleteRetentionPolicy')
-          // We need '*' here because we will also put a retention policy on
-          // the log group of the provider function. Referencing it's name
-          // creates a CF circular dependency.
-          .addAllResources()
-      );
-    }
+    provider.addToRolePolicy( // Duplicate statements will be deduplicated by `PolicyDocument`
+      new iam.PolicyStatement()
+        .addActions('logs:PutRetentionPolicy', 'logs:DeleteRetentionPolicy')
+        // We need '*' here because we will also put a retention policy on
+        // the log group of the provider function. Referencing it's name
+        // creates a CF circular dependency.
+        .addAllResources()
+    );
 
     // Need to use a CfnResource here to prevent lerna dependency cycles
     // @aws-cdk/aws-cloudformation -> @aws-cdk/aws-lambda -> @aws-cdk/aws-cloudformation