diff --git a/packages/@aws-cdk/aws-ecr/jest.config.js b/packages/@aws-cdk/aws-ecr/jest.config.js index f4f042f9d2c29..2533dd6b2e2e9 100644 --- a/packages/@aws-cdk/aws-ecr/jest.config.js +++ b/packages/@aws-cdk/aws-ecr/jest.config.js @@ -5,6 +5,7 @@ module.exports = { global: { ...baseConfig.coverageThreshold.global, branches: 70, + statements: 79, }, }, }; diff --git a/packages/@aws-cdk/aws-ecr/lib/repository.ts b/packages/@aws-cdk/aws-ecr/lib/repository.ts index 1955a3447e18b..8bb09aeef5c1c 100644 --- a/packages/@aws-cdk/aws-ecr/lib/repository.ts +++ b/packages/@aws-cdk/aws-ecr/lib/repository.ts @@ -2,7 +2,7 @@ import { EOL } from 'os'; 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 { ArnFormat, IResource, Lazy, RemovalPolicy, Resource, Stack, Token } from '@aws-cdk/core'; +import { ArnFormat, IResource, Lazy, RemovalPolicy, Resource, Stack, Token, TokenComparison } from '@aws-cdk/core'; import { IConstruct, Construct } from 'constructs'; import { CfnRepository } from './ecr.generated'; import { LifecycleRule, TagStatus } from './lifecycle'; @@ -279,17 +279,41 @@ export abstract class RepositoryBase extends Resource implements IRepository { rule.addTarget(options.target); return rule; } + /** * Grant the given principal identity permissions to perform the actions on this repository */ public grant(grantee: iam.IGrantable, ...actions: string[]) { - return iam.Grant.addToPrincipalOrResource({ - grantee, - actions, - resourceArns: [this.repositoryArn], - resourceSelfArns: [], - resource: this, - }); + const accountToTrust = this.principalCannotBeSafelyAddedToResourcePolicy(grantee); + if (accountToTrust) { + // If the principal is from a different account, + // that means addToPrincipalOrResource() will update the Resource Policy of this repo to trust that principal. + // However, ECR verifies that the principal used in the Policy exists, + // and will error out if it doesn't. + // Because of that, if the principal is a newly created resource, + // and there is not a dependency relationship between the Stacks of this repo and the principal, + // trust the entire account of the principal instead + // (otherwise, deploying this repo will fail) + this.addToResourcePolicy(new iam.PolicyStatement({ + actions, + principals: [new iam.AccountPrincipal(accountToTrust)], + })); + + return iam.Grant.addToPrincipal({ + grantee, + actions, + resourceArns: [this.repositoryArn], + scope: this, + }); + } else { + return iam.Grant.addToPrincipalOrResource({ + grantee, + actions, + resourceArns: [this.repositoryArn], + resourceSelfArns: [], + resource: this, + }); + } } /** @@ -319,6 +343,43 @@ export abstract class RepositoryBase extends Resource implements IRepository { 'ecr:UploadLayerPart', 'ecr:CompleteLayerUpload'); } + + /** + * Returns the account ID of the account to put into the Resource Policy + * if we cannot put a reference to the principal itself there, + * and 'undefined' in case we can. + */ + private principalCannotBeSafelyAddedToResourcePolicy(grantee: iam.IGrantable): string | undefined { + // A principal cannot be safely added to the Resource Policy of this repo if: + // 1. The principal is from a different account. + // 2. The principal is a new resource (meaning, not imported). + // 3. The Stack this repo belongs to doesn't depend on the Stack the principal belongs to. + + // condition #1 + const principal = grantee.grantPrincipal; + const principalAccount = principal.principalAccount; + if (!principalAccount) { + return undefined; + } + const repoAndPrincipalAccountCompare = Token.compareStrings(this.env.account, principalAccount); + if (repoAndPrincipalAccountCompare === TokenComparison.BOTH_UNRESOLVED || + repoAndPrincipalAccountCompare === TokenComparison.SAME) { + return undefined; + } + + // condition #2 + if (!iam.principalIsOwnedResource(principal)) { + return undefined; + } + + // condition #3 + const principalStack = Stack.of(principal); + if (this.stack.dependencies.includes(principalStack)) { + return undefined; + } + + return principalAccount; + } } /** @@ -491,7 +552,6 @@ export class Repository extends RepositoryBase { }); } - private static validateRepositoryName(physicalName: string) { const repositoryName = physicalName; if (!repositoryName || Token.isUnresolved(repositoryName)) { diff --git a/packages/@aws-cdk/aws-ecr/test/repository.test.ts b/packages/@aws-cdk/aws-ecr/test/repository.test.ts index 432ab41a2e8c1..748316d65ff18 100644 --- a/packages/@aws-cdk/aws-ecr/test/repository.test.ts +++ b/packages/@aws-cdk/aws-ecr/test/repository.test.ts @@ -444,6 +444,36 @@ describe('repository', () => { }).toThrow('encryptionKey is specified, so \'encryption\' must be set to KMS (value: AES256)'); }); + test('removal policy is "Retain" by default', () => { + // GIVEN + const stack = new cdk.Stack(); + + // WHEN + new ecr.Repository(stack, 'Repo'); + + // THEN + Template.fromStack(stack).hasResource('AWS::ECR::Repository', { + 'Type': 'AWS::ECR::Repository', + 'DeletionPolicy': 'Retain', + }); + }); + + test('"Delete" removal policy can be set explicitly', () => { + // GIVEN + const stack = new cdk.Stack(); + + // WHEN + new ecr.Repository(stack, 'Repo', { + removalPolicy: cdk.RemovalPolicy.DESTROY, + }); + + // THEN + Template.fromStack(stack).hasResource('AWS::ECR::Repository', { + 'Type': 'AWS::ECR::Repository', + 'DeletionPolicy': 'Delete', + }); + }); + describe('events', () => { test('onImagePushed without imageTag creates the correct event', () => { const stack = new cdk.Stack(); @@ -701,43 +731,43 @@ describe('repository', () => { repositoryName: 'a//a-a', })).toThrow(/must follow the specified pattern/); }); + }); - test('return value addToResourcePolicy', () => { - // GIVEN - const stack = new cdk.Stack(); - const policyStmt1 = new iam.PolicyStatement({ - actions: ['*'], - principals: [new iam.AnyPrincipal()], - }); - const policyStmt2 = new iam.PolicyStatement({ - effect: iam.Effect.DENY, - actions: ['ecr:BatchGetImage', 'ecr:GetDownloadUrlForLayer'], - principals: [new iam.AnyPrincipal()], - }); - const policyText1 = '{"Statement": [{"Action": "*", "Effect": "Allow", "Principal": {"AWS": "*"}}], "Version": "2012-10-17"}'; - const policyText2 = `{"Statement": [ - {"Action": "*", "Effect": "Allow", "Principal": {"AWS": "*"}}, - {"Action": ["ecr:BatchGetImage", "ecr:GetDownloadUrlForLayer"], "Effect": "Deny", "Principal": {"AWS": "*"}} - ], "Version": "2012-10-17"}`; + test('return value addToResourcePolicy', () => { + // GIVEN + const stack = new cdk.Stack(); + const policyStmt1 = new iam.PolicyStatement({ + actions: ['*'], + principals: [new iam.AnyPrincipal()], + }); + const policyStmt2 = new iam.PolicyStatement({ + effect: iam.Effect.DENY, + actions: ['ecr:BatchGetImage', 'ecr:GetDownloadUrlForLayer'], + principals: [new iam.AnyPrincipal()], + }); + const policyText1 = '{"Statement": [{"Action": "*", "Effect": "Allow", "Principal": {"AWS": "*"}}], "Version": "2012-10-17"}'; + const policyText2 = `{"Statement": [ + {"Action": "*", "Effect": "Allow", "Principal": {"AWS": "*"}}, + {"Action": ["ecr:BatchGetImage", "ecr:GetDownloadUrlForLayer"], "Effect": "Deny", "Principal": {"AWS": "*"}} + ], "Version": "2012-10-17"}`; - // WHEN - const artifact1 = new ecr.Repository(stack, 'Repo1').addToResourcePolicy(policyStmt1); - const repo = new ecr.Repository(stack, 'Repo2'); - repo.addToResourcePolicy(policyStmt1); - const artifact2 =repo.addToResourcePolicy(policyStmt2); + // WHEN + const artifact1 = new ecr.Repository(stack, 'Repo1').addToResourcePolicy(policyStmt1); + const repo = new ecr.Repository(stack, 'Repo2'); + repo.addToResourcePolicy(policyStmt1); + const artifact2 =repo.addToResourcePolicy(policyStmt2); - // THEN - expect(stack.resolve(artifact1.statementAdded)).toEqual(true); - expect(stack.resolve(artifact1.policyDependable)).toEqual(JSON.parse(policyText1)); - Template.fromStack(stack).hasResourceProperties('AWS::ECR::Repository', { - RepositoryPolicyText: JSON.parse(policyText1), - }); + // THEN + expect(stack.resolve(artifact1.statementAdded)).toEqual(true); + expect(stack.resolve(artifact1.policyDependable)).toEqual(JSON.parse(policyText1)); + Template.fromStack(stack).hasResourceProperties('AWS::ECR::Repository', { + RepositoryPolicyText: JSON.parse(policyText1), + }); - expect(stack.resolve(artifact2.statementAdded)).toEqual(true); - expect(stack.resolve(artifact2.policyDependable)).toEqual(JSON.parse(policyText2)); - Template.fromStack(stack).hasResourceProperties('AWS::ECR::Repository', { - RepositoryPolicyText: JSON.parse(policyText2), - }); + expect(stack.resolve(artifact2.statementAdded)).toEqual(true); + expect(stack.resolve(artifact2.policyDependable)).toEqual(JSON.parse(policyText2)); + Template.fromStack(stack).hasResourceProperties('AWS::ECR::Repository', { + RepositoryPolicyText: JSON.parse(policyText2), }); }); }); diff --git a/packages/@aws-cdk/aws-ecs/test/images/tag-parameter-container-image.test.ts b/packages/@aws-cdk/aws-ecs/test/images/tag-parameter-container-image.test.ts index 9ff3023b1e965..ef013e978970c 100644 --- a/packages/@aws-cdk/aws-ecs/test/images/tag-parameter-container-image.test.ts +++ b/packages/@aws-cdk/aws-ecs/test/images/tag-parameter-container-image.test.ts @@ -77,52 +77,39 @@ describe('tag parameter container image', () => { 'Fn::Join': ['', [ 'arn:', { Ref: 'AWS::Partition' }, - ':iam::service-account:role/servicestackionexecutionrolee7e2d9a783a54eb795f4', + ':iam::service-account:root', ]], }, }, }], }, }); - Template.fromStack(serviceStack).hasResourceProperties('AWS::IAM::Role', { - RoleName: 'servicestackionexecutionrolee7e2d9a783a54eb795f4', - }); - Template.fromStack(serviceStack).hasResourceProperties('AWS::ECS::TaskDefinition', { - ContainerDefinitions: [ - Match.objectLike({ - Image: { - 'Fn::Join': ['', [ - { - 'Fn::Select': [4, { - 'Fn::Split': [':', { - 'Fn::Join': ['', [ - 'arn:', - { Ref: 'AWS::Partition' }, - `:ecr:us-west-1:pipeline-account:repository/${repositoryName}`, - ]], - }], - }], - }, - '.dkr.ecr.', - { - 'Fn::Select': [3, { - 'Fn::Split': [':', { - 'Fn::Join': ['', [ - 'arn:', - { Ref: 'AWS::Partition' }, - `:ecr:us-west-1:pipeline-account:repository/${repositoryName}`, - ]], - }], - }], - }, - '.', - { Ref: 'AWS::URLSuffix' }, - `/${repositoryName}:`, - { Ref: 'ServiceTaskDefinitionContainerImageTagParamCEC9D0BA' }, - ]], - }, - }), - ], + + Template.fromStack(serviceStack).hasResourceProperties('AWS::IAM::Policy', { + PolicyDocument: Match.objectLike({ + Statement: Match.arrayWith([ + Match.objectLike({ + Action: [ + 'ecr:BatchCheckLayerAvailability', + 'ecr:GetDownloadUrlForLayer', + 'ecr:BatchGetImage', + ], + Effect: 'Allow', + Resource: { + 'Fn::Join': ['', [ + 'arn:', + { Ref: 'AWS::Partition' }, + `:ecr:us-west-1:pipeline-account:repository/${repositoryName}`, + ]], + }, + }), + Match.objectLike({ + Action: 'ecr:GetAuthorizationToken', + Effect: 'Allow', + Resource: '*', + }), + ]), + }), }); }); }); diff --git a/packages/@aws-cdk/aws-iam/lib/group.ts b/packages/@aws-cdk/aws-iam/lib/group.ts index ea8784524c5c1..0b81c6c572158 100644 --- a/packages/@aws-cdk/aws-iam/lib/group.ts +++ b/packages/@aws-cdk/aws-iam/lib/group.ts @@ -6,8 +6,8 @@ import { IManagedPolicy } from './managed-policy'; import { Policy } from './policy'; import { PolicyStatement } from './policy-statement'; import { AddToPrincipalPolicyResult, ArnPrincipal, IPrincipal, PrincipalPolicyFragment } from './principals'; +import { AttachedPolicies } from './private/util'; import { IUser } from './user'; -import { AttachedPolicies } from './util'; /** * Represents an IAM Group. diff --git a/packages/@aws-cdk/aws-iam/lib/index.ts b/packages/@aws-cdk/aws-iam/lib/index.ts index 7b13245b49f39..bf402b8037830 100644 --- a/packages/@aws-cdk/aws-iam/lib/index.ts +++ b/packages/@aws-cdk/aws-iam/lib/index.ts @@ -14,6 +14,7 @@ export * from './oidc-provider'; export * from './permissions-boundary'; export * from './saml-provider'; export * from './access-key'; +export * from './utils'; // AWS::IAM CloudFormation Resources: export * from './iam.generated'; diff --git a/packages/@aws-cdk/aws-iam/lib/managed-policy.ts b/packages/@aws-cdk/aws-iam/lib/managed-policy.ts index e8803ab6f0a8b..577fa0765c1aa 100644 --- a/packages/@aws-cdk/aws-iam/lib/managed-policy.ts +++ b/packages/@aws-cdk/aws-iam/lib/managed-policy.ts @@ -4,9 +4,9 @@ import { IGroup } from './group'; import { CfnManagedPolicy } from './iam.generated'; import { PolicyDocument } from './policy-document'; import { PolicyStatement } from './policy-statement'; +import { undefinedIfEmpty } from './private/util'; import { IRole } from './role'; import { IUser } from './user'; -import { undefinedIfEmpty } from './util'; /** * A managed policy diff --git a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts index 42e415158ec6d..bb751e3f8c432 100644 --- a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts +++ b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts @@ -6,7 +6,7 @@ import { FederatedPrincipal, IPrincipal, PrincipalBase, PrincipalPolicyFragment, ServicePrincipal, ServicePrincipalOpts, validateConditionObject, } from './principals'; import { normalizeStatement } from './private/postprocess-policy-document'; -import { LITERAL_STRING_KEY, mergePrincipal, sum } from './util'; +import { LITERAL_STRING_KEY, mergePrincipal, sum } from './private/util'; const ensureArrayOrUndefined = (field: any) => { if (field === undefined) { diff --git a/packages/@aws-cdk/aws-iam/lib/policy.ts b/packages/@aws-cdk/aws-iam/lib/policy.ts index 2de04f0990b73..f46afbc151ec3 100644 --- a/packages/@aws-cdk/aws-iam/lib/policy.ts +++ b/packages/@aws-cdk/aws-iam/lib/policy.ts @@ -4,9 +4,9 @@ import { IGroup } from './group'; import { CfnPolicy } from './iam.generated'; import { PolicyDocument } from './policy-document'; import { PolicyStatement } from './policy-statement'; +import { generatePolicyName, undefinedIfEmpty } from './private/util'; import { IRole } from './role'; import { IUser } from './user'; -import { generatePolicyName, undefinedIfEmpty } from './util'; /** * Represents an IAM Policy diff --git a/packages/@aws-cdk/aws-iam/lib/principals.ts b/packages/@aws-cdk/aws-iam/lib/principals.ts index 3a411287d6114..9bdb3c9d8ac69 100644 --- a/packages/@aws-cdk/aws-iam/lib/principals.ts +++ b/packages/@aws-cdk/aws-iam/lib/principals.ts @@ -5,8 +5,8 @@ import { IOpenIdConnectProvider } from './oidc-provider'; import { PolicyDocument } from './policy-document'; import { Condition, Conditions, PolicyStatement } from './policy-statement'; import { defaultAddPrincipalToAssumeRole } from './private/assume-role-policy'; +import { LITERAL_STRING_KEY, mergePrincipal } from './private/util'; import { ISamlProvider } from './saml-provider'; -import { LITERAL_STRING_KEY, mergePrincipal } from './util'; /** * Any object that has an associated principal that a permission can be granted to diff --git a/packages/@aws-cdk/aws-iam/lib/private/util.ts b/packages/@aws-cdk/aws-iam/lib/private/util.ts new file mode 100644 index 0000000000000..aa4bb0a1288a4 --- /dev/null +++ b/packages/@aws-cdk/aws-iam/lib/private/util.ts @@ -0,0 +1,143 @@ +import { captureStackTrace, DefaultTokenResolver, IPostProcessor, IResolvable, IResolveContext, Lazy, StringConcat, Token, Tokenization } from '@aws-cdk/core'; +import { IConstruct } from 'constructs'; +import { IPolicy } from '../policy'; + +const MAX_POLICY_NAME_LEN = 128; + +export const LITERAL_STRING_KEY = 'LiteralString'; + +export function undefinedIfEmpty(f: () => string[]): string[] { + return Lazy.list({ + produce: () => { + const array = f(); + return (array && array.length > 0) ? array : undefined; + }, + }); +} + +/** + * Used to generate a unique policy name based on the policy resource construct. + * The logical ID of the resource is a great candidate as long as it doesn't exceed + * 128 characters, so we take the last 128 characters (in order to make sure the hash + * is there). + */ +export function generatePolicyName(scope: IConstruct, logicalId: string): string { + // as logicalId is itself a Token, resolve it first + const resolvedLogicalId = Tokenization.resolve(logicalId, { + scope, + resolver: new DefaultTokenResolver(new StringConcat()), + }); + return lastNCharacters(resolvedLogicalId, MAX_POLICY_NAME_LEN); +} + +/** + * Returns a string composed of the last n characters of str. + * If str is shorter than n, returns str. + * + * @param str the string to return the last n characters of + * @param n how many characters to return + */ +function lastNCharacters(str: string, n: number) { + const startIndex = Math.max(str.length - n, 0); + return str.substring(startIndex, str.length); +} + +/** + * Helper class that maintains the set of attached policies for a principal. + */ +export class AttachedPolicies { + private policies = new Array(); + + /** + * Adds a policy to the list of attached policies. + * + * If this policy is already, attached, returns false. + * If there is another policy attached with the same name, throws an exception. + */ + public attach(policy: IPolicy) { + if (this.policies.find(p => p === policy)) { + return; // already attached + } + + if (this.policies.find(p => p.policyName === policy.policyName)) { + throw new Error(`A policy named "${policy.policyName}" is already attached`); + } + + this.policies.push(policy); + } +} + +/** + * Merge two dictionaries that represent IAM principals + * + * Does an in-place merge. + */ +export function mergePrincipal(target: { [key: string]: string[] }, source: { [key: string]: string[] }) { + // If one represents a literal string, the other one must be empty + const sourceKeys = Object.keys(source); + const targetKeys = Object.keys(target); + + if ((LITERAL_STRING_KEY in source && targetKeys.some(k => k !== LITERAL_STRING_KEY)) || + (LITERAL_STRING_KEY in target && sourceKeys.some(k => k !== LITERAL_STRING_KEY))) { + throw new Error(`Cannot merge principals ${JSON.stringify(target)} and ${JSON.stringify(source)}; if one uses a literal principal string the other one must be empty`); + } + + for (const key of sourceKeys) { + target[key] = target[key] ?? []; + + let value = source[key]; + if (!Array.isArray(value)) { + value = [value]; + } + + target[key].push(...value); + } + + return target; +} + +/** + * Lazy string set token that dedupes entries + * + * Needs to operate post-resolve, because the inputs could be + * `[ '${Token[TOKEN.9]}', '${Token[TOKEN.10]}', '${Token[TOKEN.20]}' ]`, which + * still all resolve to the same string value. + * + * Needs to JSON.stringify() results because strings could resolve to literal + * strings but could also resolve to `{ Fn::Join: [...] }`. + */ +export class UniqueStringSet implements IResolvable, IPostProcessor { + public static from(fn: () => string[]) { + return Token.asList(new UniqueStringSet(fn)); + } + + public readonly creationStack: string[]; + + private constructor(private readonly fn: () => string[]) { + this.creationStack = captureStackTrace(); + } + + public resolve(context: IResolveContext) { + context.registerPostProcessor(this); + return this.fn(); + } + + public postProcess(input: any, _context: IResolveContext) { + if (!Array.isArray(input)) { return input; } + if (input.length === 0) { return undefined; } + + const uniq: Record = {}; + for (const el of input) { + uniq[JSON.stringify(el)] = el; + } + return Object.values(uniq); + } + + public toString(): string { + return Token.asString(this); + } +} + +export function sum(xs: number[]) { + return xs.reduce((a, b) => a + b, 0); +} diff --git a/packages/@aws-cdk/aws-iam/lib/role.ts b/packages/@aws-cdk/aws-iam/lib/role.ts index edf527647e8e9..9ac900c4ac2b3 100644 --- a/packages/@aws-cdk/aws-iam/lib/role.ts +++ b/packages/@aws-cdk/aws-iam/lib/role.ts @@ -11,7 +11,7 @@ import { AddToPrincipalPolicyResult, ArnPrincipal, IPrincipal, PrincipalPolicyFr import { defaultAddPrincipalToAssumeRole } from './private/assume-role-policy'; import { ImmutableRole } from './private/immutable-role'; import { MutatingPolicyDocumentAdapter } from './private/policydoc-adapter'; -import { AttachedPolicies, UniqueStringSet } from './util'; +import { AttachedPolicies, UniqueStringSet } from './private/util'; const MAX_INLINE_SIZE = 10000; const MAX_MANAGEDPOL_SIZE = 6000; diff --git a/packages/@aws-cdk/aws-iam/lib/user.ts b/packages/@aws-cdk/aws-iam/lib/user.ts index ece9c206a8c56..7909fd3d92404 100644 --- a/packages/@aws-cdk/aws-iam/lib/user.ts +++ b/packages/@aws-cdk/aws-iam/lib/user.ts @@ -7,7 +7,7 @@ import { IManagedPolicy } from './managed-policy'; import { Policy } from './policy'; import { PolicyStatement } from './policy-statement'; import { AddToPrincipalPolicyResult, ArnPrincipal, IPrincipal, PrincipalPolicyFragment } from './principals'; -import { AttachedPolicies, undefinedIfEmpty } from './util'; +import { AttachedPolicies, undefinedIfEmpty } from './private/util'; /** * Represents an IAM user diff --git a/packages/@aws-cdk/aws-iam/lib/utils.ts b/packages/@aws-cdk/aws-iam/lib/utils.ts new file mode 100644 index 0000000000000..021977d3e5114 --- /dev/null +++ b/packages/@aws-cdk/aws-iam/lib/utils.ts @@ -0,0 +1,38 @@ +import { Resource } from '@aws-cdk/core'; +import { Construct, IConstruct } from 'constructs'; +import { IPrincipal } from './principals'; + +/** + * Determines whether the given Principal is a newly created resource managed by the CDK, + * or if it's only an imported existing resource. + * + * @param principal the Principal to check + * @returns true if the Principal is a newly created resource, false otherwise. + * Additionally, the type of the principal will now also be IConstruct + * (because a newly created resource cannot possibly not be a construct) + */ +export function principalIsOwnedResource(principal: IPrincipal): principal is IPrincipal & IConstruct { + // a newly created resource will for sure be a construct + if (!isConstruct(principal)) { + return false; + } + + return Resource.isOwnedResource(principal); +} + +/** + * Whether the given object is a Construct + * + * Normally we'd do `x instanceof Construct`, but that is not robust against + * multiple copies of the `constructs` library on disk. This can happen + * when upgrading and downgrading between v2 and v1, and in the use of CDK + * Pipelines is going to an error that says "Can't use Pipeline/Pipeline/Role in + * a cross-environment fashion", which is very confusing. + */ +function isConstruct(x: any): x is IConstruct { + const sym = Symbol.for('constructs.Construct.node'); + return (typeof x === 'object' && x && + (x instanceof Construct // happy fast case + || !!(x as any).node // constructs v10 + || !!(x as any)[sym])); // constructs v3 +} diff --git a/packages/@aws-cdk/aws-kms/lib/key.ts b/packages/@aws-cdk/aws-kms/lib/key.ts index 0dc523fb73c37..2eb86115dd534 100644 --- a/packages/@aws-cdk/aws-kms/lib/key.ts +++ b/packages/@aws-cdk/aws-kms/lib/key.ts @@ -203,12 +203,9 @@ abstract class KeyBase extends Resource implements IKey { */ private granteeStackDependsOnKeyStack(grantee: iam.IGrantable): string | undefined { const grantPrincipal = grantee.grantPrincipal; - if (!isConstruct(grantPrincipal)) { - return undefined; - } // this logic should only apply to newly created // (= not imported) resources - if (!Resource.isOwnedResource(grantPrincipal)) { + if (!iam.principalIsOwnedResource(grantPrincipal)) { return undefined; } const keyStack = Stack.of(this); @@ -223,20 +220,20 @@ abstract class KeyBase extends Resource implements IKey { } private isGranteeFromAnotherRegion(grantee: iam.IGrantable): boolean { - if (!isConstruct(grantee)) { + if (!iam.principalIsOwnedResource(grantee.grantPrincipal)) { return false; } const bucketStack = Stack.of(this); - const identityStack = Stack.of(grantee); + const identityStack = Stack.of(grantee.grantPrincipal); return bucketStack.region !== identityStack.region; } private isGranteeFromAnotherAccount(grantee: iam.IGrantable): boolean { - if (!isConstruct(grantee)) { + if (!iam.principalIsOwnedResource(grantee.grantPrincipal)) { return false; } const bucketStack = Stack.of(this); - const identityStack = Stack.of(grantee); + const identityStack = Stack.of(grantee.grantPrincipal); return bucketStack.account !== identityStack.account; } } @@ -725,20 +722,3 @@ export class Key extends KeyBase { })); } } - -/** - * Whether the given object is a Construct - * - * Normally we'd do `x instanceof Construct`, but that is not robust against - * multiple copies of the `constructs` library on disk. This can happen - * when upgrading and downgrading between v2 and v1, and in the use of CDK - * Pipelines is going to an error that says "Can't use Pipeline/Pipeline/Role in - * a cross-environment fashion", which is very confusing. - */ -function isConstruct(x: any): x is Construct { - const sym = Symbol.for('constructs.Construct.node'); - return (typeof x === 'object' && x && - (x instanceof Construct // happy fast case - || !!(x as any).node // constructs v10 - || !!(x as any)[sym])); // constructs v3 -} \ No newline at end of file