diff --git a/packages/@aws-cdk/aws-ecr/lib/repository.ts b/packages/@aws-cdk/aws-ecr/lib/repository.ts index 40be60237d8df..1117530fb777d 100644 --- a/packages/@aws-cdk/aws-ecr/lib/repository.ts +++ b/packages/@aws-cdk/aws-ecr/lib/repository.ts @@ -41,7 +41,7 @@ export interface IRepository extends IResource { /** * Add a policy statement to the repository's resource policy */ - addToResourcePolicy(statement: iam.PolicyStatement): void; + addToResourcePolicy(statement: iam.PolicyStatement): iam.AddToResourcePolicyResult; /** * Grant the given principal identity permissions to perform the actions on this repository @@ -114,7 +114,7 @@ export abstract class RepositoryBase extends Resource implements IRepository { /** * Add a policy statement to the repository's resource policy */ - public abstract addToResourcePolicy(statement: iam.PolicyStatement): void; + public abstract addToResourcePolicy(statement: iam.PolicyStatement): iam.AddToResourcePolicyResult; /** * The URI of this repository (represents the latest image): @@ -335,8 +335,9 @@ export class Repository extends RepositoryBase { public readonly repositoryName = attrs.repositoryName; public readonly repositoryArn = attrs.repositoryArn; - public addToResourcePolicy(_statement: iam.PolicyStatement) { + public addToResourcePolicy(_statement: iam.PolicyStatement): iam.AddToResourcePolicyResult { // dropped + return { statementAdded: false }; } } @@ -358,8 +359,9 @@ export class Repository extends RepositoryBase { public repositoryName = repositoryName; public repositoryArn = repositoryArn; - public addToResourcePolicy(_statement: iam.PolicyStatement): void { + public addToResourcePolicy(_statement: iam.PolicyStatement): iam.AddToResourcePolicyResult { // dropped + return { statementAdded: false }; } } @@ -371,8 +373,9 @@ export class Repository extends RepositoryBase { public repositoryName = repositoryName; public repositoryArn = Repository.arnForLocalRepository(repositoryName, scope); - public addToResourcePolicy(_statement: iam.PolicyStatement): void { + public addToResourcePolicy(_statement: iam.PolicyStatement): iam.AddToResourcePolicyResult { // dropped + return { statementAdded: false }; } } @@ -424,11 +427,12 @@ export class Repository extends RepositoryBase { }); } - public addToResourcePolicy(statement: iam.PolicyStatement) { + public addToResourcePolicy(statement: iam.PolicyStatement): iam.AddToResourcePolicyResult { if (this.policyDocument === undefined) { this.policyDocument = new iam.PolicyDocument(); } this.policyDocument.addStatements(statement); + return { statementAdded: false, policyDependable: this.policyDocument }; } /** diff --git a/packages/@aws-cdk/aws-elasticloadbalancingv2-targets/lib/lambda-target.ts b/packages/@aws-cdk/aws-elasticloadbalancingv2-targets/lib/lambda-target.ts index 871d8f2b3c4e0..be1f10d465b81 100644 --- a/packages/@aws-cdk/aws-elasticloadbalancingv2-targets/lib/lambda-target.ts +++ b/packages/@aws-cdk/aws-elasticloadbalancingv2-targets/lib/lambda-target.ts @@ -18,7 +18,8 @@ export class LambdaTarget implements elbv2.IApplicationLoadBalancerTarget { * load balancer. */ public attachToApplicationTargetGroup(targetGroup: elbv2.IApplicationTargetGroup): elbv2.LoadBalancerTargetProps { - this.fn.grantInvoke(new iam.ServicePrincipal('elasticloadbalancing.amazonaws.com')); + const grant = this.fn.grantInvoke(new iam.ServicePrincipal('elasticloadbalancing.amazonaws.com')); + grant.applyBefore(targetGroup); return this.attach(targetGroup); } @@ -29,7 +30,8 @@ export class LambdaTarget implements elbv2.IApplicationLoadBalancerTarget { * load balancer. */ public attachToNetworkTargetGroup(targetGroup: elbv2.INetworkTargetGroup): elbv2.LoadBalancerTargetProps { - this.fn.grantInvoke(new iam.ServicePrincipal('elasticloadbalancing.amazonaws.com')); + const grant = this.fn.grantInvoke(new iam.ServicePrincipal('elasticloadbalancing.amazonaws.com')); + grant.applyBefore(targetGroup); return this.attach(targetGroup); } diff --git a/packages/@aws-cdk/aws-iam/lib/grant.ts b/packages/@aws-cdk/aws-iam/lib/grant.ts index d78fb25c545ef..b979c5d6e9657 100644 --- a/packages/@aws-cdk/aws-iam/lib/grant.ts +++ b/packages/@aws-cdk/aws-iam/lib/grant.ts @@ -1,6 +1,7 @@ import * as cdk from '@aws-cdk/core'; import { PolicyStatement } from './policy-statement'; import { IGrantable, IPrincipal } from './principals'; +import { ConcreteDependable } from '@aws-cdk/core'; /** * Basic options for a grant operation @@ -100,7 +101,7 @@ export interface GrantOnPrincipalAndResourceOptions extends CommonGrantOptions { * This class is not instantiable by consumers on purpose, so that they will be * required to call the Grant factory functions. */ -export class Grant { +export class Grant implements cdk.IDependable { /** * Grant the given permissions to the principal * @@ -129,9 +130,13 @@ export class Grant { principals: [options.grantee!.grantPrincipal], }); - options.resource.addToResourcePolicy(statement); + const resourceResult = options.resource.addToResourcePolicy(statement); - return new Grant({ resourceStatement: statement, options }); + return new Grant({ + resourceStatement: statement, + options, + policyApplied: resourceResult.statementAdded ? resourceResult.policyDependable ?? options.resource : undefined, + }); } /** @@ -147,8 +152,9 @@ export class Grant { }); const addedToPrincipal = options.grantee.grantPrincipal.addToPolicy(statement); + const policyApplied = addedToPrincipal ? guessPolicyDependable(options.grantee.grantPrincipal) : new cdk.ConcreteDependable(); - return new Grant({ principalStatement: addedToPrincipal ? statement : undefined, options }); + return new Grant({ principalStatement: addedToPrincipal ? statement : undefined, options, policyApplied }); } /** @@ -172,9 +178,15 @@ export class Grant { principals: [options.resourcePolicyPrincipal || options.grantee!.grantPrincipal], }); - options.resource.addToResourcePolicy(statement); + const resourceResult = options.resource.addToResourcePolicy(statement); + const resourceDependable = resourceResult.statementAdded ? resourceResult.policyDependable ?? options.resource : undefined; - return new Grant({ principalStatement: statement, resourceStatement: result.resourceStatement, options }); + return new Grant({ + principalStatement: statement, + resourceStatement: result.resourceStatement, + options, + policyApplied: resourceDependable ? new CompositeDependable(result, resourceDependable) : result, + }); } /** @@ -218,6 +230,12 @@ export class Grant { this.options = props.options; this.principalStatement = props.principalStatement; this.resourceStatement = props.resourceStatement; + + cdk.DependableTrait.implement(this, { + get dependencyRoots() { + return props.policyApplied ? cdk.DependableTrait.get(props.policyApplied).dependencyRoots : []; + }, + }); } /** @@ -236,6 +254,17 @@ export class Grant { throw new Error(`${describeGrant(this.options)} could not be added on either identity or resource policy.`); } } + + /** + * Make sure this grant is applied before the given constructs are deployed + * + * The same as construct.node.addDependency(grant), but slightly nicer to read. + */ + public applyBefore(...constructs: cdk.IConstruct[]) { + for (const construct of constructs) { + construct.node.addDependency(this); + } + } } function describeGrant(options: CommonGrantOptions) { @@ -246,6 +275,13 @@ interface GrantProps { readonly options: CommonGrantOptions; readonly principalStatement?: PolicyStatement; readonly resourceStatement?: PolicyStatement; + + /** + * Constructs whose deployment applies the grant + * + * Used to add dependencies on grants + */ + readonly policyApplied?: cdk.IDependable; } /** @@ -255,5 +291,80 @@ export interface IResourceWithPolicy extends cdk.IConstruct { /** * Add a statement to the resource's resource policy */ - addToResourcePolicy(statement: PolicyStatement): void; + addToResourcePolicy(statement: PolicyStatement): AddToResourcePolicyResult; +} + +/** + * Result of calling addToResourcePolicy + */ +export interface AddToResourcePolicyResult { + /** + * Whether the statement as added + */ + readonly statementAdded: boolean; + + /** + * Dependable which allows depending on the policy change being applied + * + * @default - The resource itself applies the policy change, if `didAddStatement` + * is true. Otherwise, no dependable. + */ + readonly policyDependable?: cdk.IDependable; +} + +/** + * Guess the policy object that just got modified from a principal + * + * This is a mega-extreme hack. We SHOULD have gotten the policy that + * was modified back from the principal when doing `addToPolicy()`. However, + * that API was cast in stone to return a `boolean` and cannot be changed + * anymore since it's stable. + * + * It's also occupying prime real estate, in that `addToPolicy` is the name + * we'd want this thing to have, so deprecating it feels silly and wasteful. + * + * So we do nasty things here in order to take a good guess at finding the + * Policy object, and hope we are correct. We should at least be fine for + * the CDK built-in Principal types that have mutable Policies (Role, + * User, Group, which are the main ones that matter). + */ +function guessPolicyDependable(principal: IPrincipal): cdk.IDependable { + // Role, Group and User Constructs have a default `Policy` object. + if (cdk.Construct.isConstruct(principal)) { + const pol = principal.node.tryFindChild('Policy'); + if (pol) { return pol; } + + // LazyRole wraps another role, but that may not be reified yet. + // It is itself a construct, so we can return it. We may capture too much + // (resource and any of its policies), but not too little. It's also + // no worse than what we used to have before Grants being IDependable, + // where users would ALWAYS have to depend on the Role (in order to depend + // on its Policy). + return principal; + } + + // PrincipalWithCondition wraps another principal. + if ((principal as any).principal) { + return guessPolicyDependable((principal as any).principal); + } + + // Give up (empty dependable) + return new cdk.ConcreteDependable(); } + +/** + * Composite dependable + * + * Not as simple as eagerly getting the dependency roots from the + * inner dependables, as they may be mutable so we need to defer + * the query. + */ +export class CompositeDependable implements cdk.IDependable { + constructor(...dependables: cdk.IDependable[]) { + cdk.DependableTrait.implement(this, { + get dependencyRoots(): cdk.IConstruct[] { + return Array.prototype.concat.apply([], dependables.map(d => cdk.DependableTrait.get(d).dependencyRoots)); + }, + }); + } +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-kms/lib/alias.ts b/packages/@aws-cdk/aws-kms/lib/alias.ts index 158548856d0df..df210f40afcc1 100644 --- a/packages/@aws-cdk/aws-kms/lib/alias.ts +++ b/packages/@aws-cdk/aws-kms/lib/alias.ts @@ -73,8 +73,8 @@ abstract class AliasBase extends Resource implements IAlias { return this.aliasTargetKey.addAlias(alias); } - public addToResourcePolicy(statement: iam.PolicyStatement, allowNoOp?: boolean): void { - this.aliasTargetKey.addToResourcePolicy(statement, allowNoOp); + public addToResourcePolicy(statement: iam.PolicyStatement, allowNoOp?: boolean): iam.AddToResourcePolicyResult { + return this.aliasTargetKey.addToResourcePolicy(statement, allowNoOp); } public grant(grantee: iam.IGrantable, ...actions: string[]): iam.Grant { diff --git a/packages/@aws-cdk/aws-kms/lib/key.ts b/packages/@aws-cdk/aws-kms/lib/key.ts index 381e63e7abc5b..437ccd808106f 100644 --- a/packages/@aws-cdk/aws-kms/lib/key.ts +++ b/packages/@aws-cdk/aws-kms/lib/key.ts @@ -107,15 +107,16 @@ abstract class KeyBase extends Resource implements IKey { * defined (i.e. external key), the operation will fail. Otherwise, it will * no-op. */ - public addToResourcePolicy(statement: iam.PolicyStatement, allowNoOp = true) { + public addToResourcePolicy(statement: iam.PolicyStatement, allowNoOp = true): iam.AddToResourcePolicyResult { const stack = Stack.of(this); if (!this.policy) { - if (allowNoOp) { return; } + if (allowNoOp) { return { statementAdded: false }; } throw new Error(`Unable to add statement to IAM resource policy for KMS key: ${JSON.stringify(stack.resolve(this.keyArn))}`); } this.policy.addStatements(statement); + return { statementAdded: true, policyDependable: this.policy }; } /** diff --git a/packages/@aws-cdk/aws-lambda/lib/function-base.ts b/packages/@aws-cdk/aws-lambda/lib/function-base.ts index eea549fbb957e..65a6fe6c05c26 100644 --- a/packages/@aws-cdk/aws-lambda/lib/function-base.ts +++ b/packages/@aws-cdk/aws-lambda/lib/function-base.ts @@ -283,6 +283,8 @@ export abstract class FunctionBase extends Resource implements IFunction { principal: grantee.grantPrincipal!, action: 'lambda:InvokeFunction', }); + + return { statementAdded: true, policyDependable: this.node.findChild(identifier) } as iam.AddToResourcePolicyResult; }, node: this.node, }, diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index 62f38dab11d8e..e1e8267db7a06 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -1,7 +1,7 @@ import * as events from '@aws-cdk/aws-events'; import * as iam from '@aws-cdk/aws-iam'; import * as kms from '@aws-cdk/aws-kms'; -import { Construct, Fn, IResource, Lazy, RemovalPolicy, Resource, Stack, Token } from '@aws-cdk/core'; +import { Construct, Fn, IResource, Lazy, RemovalPolicy, Resource, Stack, Token, ConcreteDependable } from '@aws-cdk/core'; import { EOL } from 'os'; import { BucketPolicy } from './bucket-policy'; import { IBucketNotificationDestination } from './destination'; @@ -73,7 +73,7 @@ export interface IBucket extends IResource { * contents. Use `bucketArn` and `arnForObjects(keys)` to obtain ARNs for * this bucket or objects. */ - addToResourcePolicy(permission: iam.PolicyStatement): void; + addToResourcePolicy(permission: iam.PolicyStatement): iam.AddToResourcePolicyResult; /** * The https URL of an S3 object. For example: @@ -408,14 +408,17 @@ abstract class BucketBase extends Resource implements IBucket { * contents. Use `bucketArn` and `arnForObjects(keys)` to obtain ARNs for * this bucket or objects. */ - public addToResourcePolicy(permission: iam.PolicyStatement) { + public addToResourcePolicy(permission: iam.PolicyStatement): iam.AddToResourcePolicyResult { if (!this.policy && this.autoCreatePolicy) { this.policy = new BucketPolicy(this, 'Policy', { bucket: this }); } if (this.policy) { this.policy.document.addStatements(permission); + return { statementAdded: true, policyDependable: this.policy }; } + + return { statementAdded: false }; } /** diff --git a/packages/@aws-cdk/aws-secretsmanager/lib/secret.ts b/packages/@aws-cdk/aws-secretsmanager/lib/secret.ts index 6ae768c9311b8..9d0456dbd23f3 100644 --- a/packages/@aws-cdk/aws-secretsmanager/lib/secret.ts +++ b/packages/@aws-cdk/aws-secretsmanager/lib/secret.ts @@ -163,14 +163,16 @@ abstract class SecretBase extends Resource implements ISecret { }); } - public addToResourcePolicy(statement: iam.PolicyStatement) { + public addToResourcePolicy(statement: iam.PolicyStatement): iam.AddToResourcePolicyResult { if (!this.policy && this.autoCreatePolicy) { this.policy = new ResourcePolicy(this, 'Policy', { secret: this }); } if (this.policy) { this.policy.document.addStatements(statement); + return { statementAdded: true, policyDependable: this.policy }; } + return { statementAdded: false }; } public denyAccountRootDelete() { diff --git a/packages/@aws-cdk/aws-sns/lib/topic-base.ts b/packages/@aws-cdk/aws-sns/lib/topic-base.ts index ff025cb09072f..7bf1936a1c0dd 100644 --- a/packages/@aws-cdk/aws-sns/lib/topic-base.ts +++ b/packages/@aws-cdk/aws-sns/lib/topic-base.ts @@ -83,14 +83,16 @@ export abstract class TopicBase extends Resource implements ITopic { * will be automatically created upon the first call to `addToPolicy`. If * the topic is improted (`Topic.import`), then this is a no-op. */ - public addToResourcePolicy(statement: iam.PolicyStatement) { + public addToResourcePolicy(statement: iam.PolicyStatement): iam.AddToResourcePolicyResult { if (!this.policy && this.autoCreatePolicy) { this.policy = new TopicPolicy(this, 'Policy', { topics: [ this ] }); } if (this.policy) { this.policy.document.addStatements(statement); + return { statementAdded: true, policyDependable: this.policy }; } + return { statementAdded: false }; } /** diff --git a/packages/@aws-cdk/aws-sqs/lib/queue-base.ts b/packages/@aws-cdk/aws-sqs/lib/queue-base.ts index 955afd6644701..ab89616e60a7e 100644 --- a/packages/@aws-cdk/aws-sqs/lib/queue-base.ts +++ b/packages/@aws-cdk/aws-sqs/lib/queue-base.ts @@ -138,14 +138,17 @@ export abstract class QueueBase extends Resource implements IQueue { * will be automatically created upon the first call to `addToPolicy`. If * the queue is improted (`Queue.import`), then this is a no-op. */ - public addToResourcePolicy(statement: iam.PolicyStatement) { + public addToResourcePolicy(statement: iam.PolicyStatement): iam.AddToResourcePolicyResult { if (!this.policy && this.autoCreatePolicy) { this.policy = new QueuePolicy(this, 'Policy', { queues: [ this ] }); } if (this.policy) { this.policy.document.addStatements(statement); + return { statementAdded: true, policyDependable: this.policy }; } + + return { statementAdded: false }; } /**