feat(iam): avoid duplicate statements in policy documents

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

@@ -1,5 +1,6 @@
 import cdk = require('@aws-cdk/cdk');
 import { Default, RegionInfo } from '@aws-cdk/region-info';
+import crypto = require('crypto');
 import { IPrincipal } from './principals';
 import { mergePrincipal } from './util';
 
@@ -11,7 +12,7 @@ export class PolicyDocument extends cdk.Token {
    * @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,10 +21,20 @@ 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 || [],
+      Version: this.baseDocument.Version || '2012-10-17'
+    };
+
+    const hashes: string[] = [];
+    for (const statement of this.statements) {
+      if (!hashes.includes(statement.hash)) {
+        doc.Statement.push(statement);
+      }
+      hashes.push(statement.hash);
+    }
+
     return doc;
   }
 
@@ -517,6 +528,16 @@ export class PolicyStatement extends cdk.Token {
       return result;
     }
   }
+
+  /**
+   * The hash of this statement. Used to avoid duplicates.
+   */
+  public get hash(): string {
+    return crypto
+      .createHash('sha256')
+      .update(JSON.stringify(this.toJson()))
+      .digest('hex');
+  }
 }
 
 export enum PolicyStatementEffect {

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

@@ -219,23 +219,51 @@ export = {
     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,29 @@ export = {
       test.done();
     }
   },
+
+  'duplicate statements'(test: Test) {
+    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'
+        }
+      });
+
+    p.addStatement(statement);
+    p.addStatement(statement);
+    p.addStatement(statement);
+
+    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(
+      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