fix(codebuild): cannot use immutable roles for Project

Immutably imported `Role`s could not be used for CodeBuild
`Project`s, because they would create a policy but be unable
to attach it to the Role. That leaves an unattached Policy,
which is invalid.

Fix this by making `Policy` objects only render to an `AWS::IAM::Policy`
resource if they actually have any effect. It is perfectly allowed to
create new unattached Policy objects, or have empty Policy objects.
Only if and when they actually need to mutate the policy of an IAM
identity will they render themselves to the CloudFormation template.
Being able to abstract away these kinds of concerns is exactly the value
of a higher-level programming model.

To allow for the rare cases where an empty Policy object would be
considered a programming error, we still have the flag `mustCreate`
which triggers the legacy behavior of alwyas creating the
`AWS::IAM::Policy` resource which must have a statement and be
attached to an identity.

Fixes #1408.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

<!-- 
Please read the contribution guidelines and follow the pull-request checklist:
https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md
 -->

packages/@aws-cdk/aws-codebuild/lib/project.ts

@@ -7,7 +7,7 @@ import * as iam from '@aws-cdk/aws-iam';
 import * as kms from '@aws-cdk/aws-kms';
 import * as s3 from '@aws-cdk/aws-s3';
 import * as secretsmanager from '@aws-cdk/aws-secretsmanager';
-import { Aws, CfnResource, Construct, Duration, IResource, Lazy, PhysicalName, Resource, Stack } from '@aws-cdk/core';
+import { Aws, Construct, Duration, IResource, Lazy, PhysicalName, Resource, Stack } from '@aws-cdk/core';
 import { IArtifacts } from './artifacts';
 import { BuildSpec } from './build-spec';
 import { Cache } from './cache';
@@ -973,10 +973,9 @@ export class Project extends ProjectBase {
     this.role.attachInlinePolicy(policy);
 
     // add an explicit dependency between the EC2 Policy and this Project -
-    // otherwise, creating the Project fails,
-    // as it requires these permissions to be already attached to the Project's Role
-    const cfnPolicy = policy.node.findChild('Resource') as CfnResource;
-    project.addDependsOn(cfnPolicy);
+    // otherwise, creating the Project fails, as it requires these permissions
+    // to be already attached to the Project's Role
+    project.node.addDependency(policy);
   }
 
   private validateCodePipelineSettings(artifacts: IArtifacts) {

packages/@aws-cdk/aws-codebuild/test/test.project.ts

@@ -1,4 +1,4 @@
-import { expect, haveResource, haveResourceLike, not } from '@aws-cdk/assert';
+import { countResources, expect, haveResource, haveResourceLike, not, ResourcePart } from '@aws-cdk/assert';
 import * as ec2 from '@aws-cdk/aws-ec2';
 import * as iam from '@aws-cdk/aws-iam';
 import * as s3 from '@aws-cdk/aws-s3';
@@ -271,4 +271,34 @@ export = {
 
     test.done();
   },
-};
+
+  'can use an imported Role with mutable = false for a Project within a VPC'(test: Test) {
+    const stack = new cdk.Stack();
+
+    const importedRole = iam.Role.fromRoleArn(stack, 'Role',
+        'arn:aws:iam::1234567890:role/service-role/codebuild-bruiser-service-role', {
+      mutable: false,
+    });
+    const vpc = new ec2.Vpc(stack, 'Vpc');
+
+    new codebuild.Project(stack, 'Project', {
+      source: codebuild.Source.gitHubEnterprise({
+        httpsCloneUrl: 'https://mygithub-enterprise.com/myuser/myrepo',
+      }),
+      role: importedRole,
+      vpc,
+    });
+
+    expect(stack).to(countResources('AWS::IAM::Policy', 0));
+
+    // Check that the CodeBuild project does not have a DependsOn
+    expect(stack).to(haveResource('AWS::CodeBuild::Project', (res: any) => {
+      if (res.DependsOn && res.DependsOn.length > 0) {
+        throw new Error(`CodeBuild project should have no DependsOn, but got: ${JSON.stringify(res, undefined, 2)}`);
+      }
+      return true;
+    }, ResourcePart.CompleteDefinition));
+
+    test.done();
+  },
+};

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

@@ -56,6 +56,22 @@ export interface PolicyProps {
    * @default - No statements.
    */
   readonly statements?: PolicyStatement[];
+
+  /**
+   * Force creation of an `AWS::IAM::Policy`
+   *
+   * Unless set to `true`, this `Policy` construct will not materialize to an
+   * `AWS::IAM::Policy` CloudFormation resource in case it would have no effect
+   * (for example, if it remains unattached to an IAM identity or if it has no
+   * statements). This is generally desired behavior, since it prevents
+   * creating invalid--and hence undeployable--CloudFormation templates.
+   *
+   * In cases where you know the policy must be created and it is actually
+   * an error if no statements have been added to it, you can se this to `true`.
+   *
+   * @default false
+   */
+  readonly force?: boolean;
 }
 
 /**
@@ -79,16 +95,12 @@ export class Policy extends Resource implements IPolicy {
    */
   public readonly document = new PolicyDocument();
 
-  /**
-   * The name of this policy.
-   *
-   * @attribute
-   */
-  public readonly policyName: string;
-
+  private readonly _policyName: string;
   private readonly roles = new Array<IRole>();
   private readonly users = new Array<IUser>();
   private readonly groups = new Array<IGroup>();
+  private readonly force: boolean;
+  private referenceTaken = false;
 
   constructor(scope: Construct, id: string, props: PolicyProps = {}) {
     super(scope, id, {
@@ -107,7 +119,8 @@ export class Policy extends Resource implements IPolicy {
       groups: undefinedIfEmpty(() => this.groups.map(g => g.groupName)),
     });
 
-    this.policyName = this.physicalName!;
+    this._policyName = this.physicalName!;
+    this.force = props.force !== undefined ? props.force : false;
 
     if (props.users) {
       props.users.forEach(u => this.attachToUser(u));
@@ -160,19 +173,54 @@ export class Policy extends Resource implements IPolicy {
     group.attachInlinePolicy(this);
   }
 
+  /**
+   * The name of this policy.
+   *
+   * @attribute
+   */
+  public get policyName(): string {
+    this.referenceTaken = true;
+    return this._policyName;
+  }
+
   protected validate(): string[] {
     const result = new Array<string>();
 
     // validate that the policy document is not empty
     if (this.document.isEmpty) {
-      result.push('Policy is empty. You must add statements to the policy');
+      if (this.force) {
+        result.push('Policy created with force=true is empty. You must add statements to the policy');
+      }
+      if (!this.force && this.referenceTaken) {
+        result.push('This Policy has been referenced by a resource, so it must contain at least one statement.');
+      }
     }
 
     // validate that the policy is attached to at least one principal (role, user or group).
-    if (this.groups.length + this.users.length + this.roles.length === 0) {
-      result.push(`Policy must be attached to at least one principal: user, group or role`);
+    if (!this.isAttached) {
+      if (this.force) {
+        result.push(`Policy created with force=true must be attached to at least one principal: user, group or role`);
+      }
+      if (!this.force && this.referenceTaken) {
+        result.push('This Policy has been referenced by a resource, so it must be attached to at least one user, group or role.');
+      }
     }
 
     return result;
   }
+
+  protected prepare() {
+    // Remove the resource if it shouldn't exist. This will prevent it from being rendered to the template.
+    const shouldExist = this.force || this.referenceTaken || (!this.document.isEmpty && this.isAttached);
+    if (!shouldExist) {
+      this.node.tryRemoveChild('Resource');
+    }
+  }
+
+  /**
+   * Whether the policy resource has been attached to any identity
+   */
+  private get isAttached() {
+    return this.groups.length + this.users.length + this.roles.length > 0;
+  }
 }

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

@@ -1,22 +1,26 @@
+import { ResourcePart } from '@aws-cdk/assert';
 import '@aws-cdk/assert/jest';
-import { App, Stack } from '@aws-cdk/core';
+import { App, CfnResource, Stack } from '@aws-cdk/core';
 import { AnyPrincipal, CfnPolicy, Group, Policy, PolicyStatement, Role, ServicePrincipal, User } from '../lib';
 
 // tslint:disable:object-literal-key-quotes
 
 describe('IAM policy', () => {
-  test('fails when policy is empty', () => {
-    const app = new App();
-    const stack = new Stack(app, 'MyStack');
-    new Policy(stack, 'MyPolicy');
+  let app: App;
+  let stack: Stack;
 
-    expect(() => app.synth()).toThrow(/Policy is empty/);
+  beforeEach(() => {
+    app = new App();
+    stack = new Stack(app, 'MyStack');
   });
 
-  test('policy with statements', () => {
-    const app = new App();
-    const stack = new Stack(app, 'MyStack');
+  test('fails when "forced" policy is empty', () => {
+    new Policy(stack, 'MyPolicy', { force: true });
+
+    expect(() => app.synth()).toThrow(/is empty/);
+  });
 
+  test('policy with statements', () => {
     const policy = new Policy(stack, 'MyPolicy', { policyName: 'MyPolicyName' });
     policy.addStatements(new PolicyStatement({ resources: ['*'], actions: ['sqs:SendMessage'] }));
     policy.addStatements(new PolicyStatement({ resources: ['arn'], actions: ['sns:Subscribe'] }));
@@ -39,9 +43,6 @@ describe('IAM policy', () => {
   });
 
   test('policy name can be omitted, in which case the logical id will be used', () => {
-    const app = new App();
-    const stack = new Stack(app, 'MyStack');
-
     const policy = new Policy(stack, 'MyPolicy');
     policy.addStatements(new PolicyStatement({ resources: ['*'], actions: ['sqs:SendMessage'] }));
     policy.addStatements(new PolicyStatement({ resources: ['arn'], actions: ['sns:Subscribe'] }));
@@ -64,10 +65,6 @@ describe('IAM policy', () => {
   });
 
   test('policy can be attached users, groups and roles and added permissions via props', () => {
-    const app = new App();
-
-    const stack = new Stack(app, 'MyStack');
-
     const user1 = new User(stack, 'User1');
     const group1 = new Group(stack, 'Group1');
     const role1 = new Role(stack, 'Role1', {
@@ -108,8 +105,6 @@ describe('IAM policy', () => {
   });
 
   test('idempotent if a principal (user/group/role) is attached twice', () => {
-    const app = new App();
-    const stack = new Stack(app, 'MyStack');
     const p = new Policy(stack, 'MyPolicy');
     p.addStatements(new PolicyStatement({ actions: ['*'], resources: ['*'] }));
 
@@ -130,10 +125,6 @@ describe('IAM policy', () => {
   });
 
   test('users, groups, roles and permissions can be added using methods', () => {
-    const app = new App();
-
-    const stack = new Stack(app, 'MyStack');
-
     const p = new Policy(stack, 'MyTestPolicy', {
       policyName: 'Foo',
     });
@@ -171,9 +162,6 @@ describe('IAM policy', () => {
   });
 
   test('policy can be attached to users, groups or role via methods on the principal', () => {
-    const app = new App();
-    const stack = new Stack(app, 'MyStack');
-
     const policy = new Policy(stack, 'MyPolicy');
     const user = new User(stack, 'MyUser');
     const group = new Group(stack, 'MyGroup');
@@ -210,9 +198,6 @@ describe('IAM policy', () => {
   });
 
   test('fails if policy name is not unique within a user/group/role', () => {
-    const app = new App();
-    const stack = new Stack(app, 'MyStack');
-
     // create two policies named Foo and attach them both to the same user/group/role
     const p1 = new Policy(stack, 'P1', { policyName: 'Foo' });
     const p2 = new Policy(stack, 'P2', { policyName: 'Foo' });
@@ -236,16 +221,12 @@ describe('IAM policy', () => {
     p3.attachToRole(role);
   });
 
-  test('fails if policy is not attached to a principal', () => {
-    const app = new App();
-    const stack = new Stack(app, 'MyStack');
-    new Policy(stack, 'MyPolicy');
-    expect(() => app.synth()).toThrow(/Policy must be attached to at least one principal: user, group or role/);
+  test('fails if "forced" policy is not attached to a principal', () => {
+    new Policy(stack, 'MyPolicy', { force: true });
+    expect(() => app.synth()).toThrow(/attached to at least one principal: user, group or role/);
   });
 
   test("generated policy name is the same as the logical id if it's shorter than 128 characters", () => {
-    const stack = new Stack();
-
     createPolicyWithLogicalId(stack, 'Foo');
 
     expect(stack).toHaveResourceLike('AWS::IAM::Policy', {
@@ -254,8 +235,6 @@ describe('IAM policy', () => {
   });
 
   test('generated policy name only uses the last 128 characters of the logical id', () => {
-    const stack = new Stack();
-
     const logicalId128 = 'a' + dup(128 - 2) + 'a';
     const logicalIdOver128 = 'PREFIX' + logicalId128;
 
@@ -273,6 +252,64 @@ describe('IAM policy', () => {
       return r;
     }
   });
+
+  test('force=false, dependency on empty Policy never materializes', () => {
+    // GIVEN
+    const pol = new Policy(stack, 'Pol', { force: false });
+
+    const res = new CfnResource(stack, 'Resource', {
+      type: 'Some::Resource',
+    });
+
+    // WHEN
+    res.node.addDependency(pol);
+
+    // THEN
+    expect(stack).toMatchTemplate({
+      Resources: {
+        Resource: {
+          Type: 'Some::Resource',
+        }
+      }
+    });
+  });
+
+  test('force=false, dependency on attached and non-empty Policy can be taken', () => {
+    // GIVEN
+    const pol = new Policy(stack, 'Pol', { force: false });
+    pol.addStatements(new PolicyStatement({
+      actions: ['s3:*'],
+      resources: ['*'],
+    }));
+    pol.attachToUser(new User(stack, 'User'));
+
+    const res = new CfnResource(stack, 'Resource', {
+      type: 'Some::Resource',
+    });
+
+    // WHEN
+    res.node.addDependency(pol);
+
+    // THEN
+    expect(stack).toHaveResource("Some::Resource", {
+      Type: "Some::Resource",
+      DependsOn: [ "Pol0FE9AD5D" ]
+    }, ResourcePart.CompleteDefinition);
+  });
+
+  test('empty policy is OK if force=false', () => {
+    new Policy(stack, 'Pol', { force: false });
+
+    app.synth();
+    // If we got here, all OK
+  });
+
+  test('reading policyName forces a Policy to materialize', () => {
+    const pol = new Policy(stack, 'Pol', { force: false });
+    Array.isArray(pol.policyName);
+
+    expect(() => app.synth()).toThrow(/must contain at least one statement/);
+  });
 });
 
 function createPolicyWithLogicalId(stack: Stack, logicalId: string): void {

packages/@aws-cdk/core/lib/construct.ts

@@ -438,6 +438,18 @@ export class ConstructNode {
     return ret;
   }
 
+  /**
+   * Remove the child with the given name, if present.
+   *
+   * @returns Whether a child with the given name was deleted.
+   * @experimental
+   */
+  public tryRemoveChild(childName: string): boolean {
+    if (!(childName in this._children)) { return false; }
+    delete this._children[childName];
+    return true;
+  }
+
   /**
    * Locks this construct from allowing more children to be added. After this
    * call, no more children can be added to this construct or to any children.