diff --git a/packages/@aws-cdk/aws-ecr/lib/repository.ts b/packages/@aws-cdk/aws-ecr/lib/repository.ts index af8547ba5edaf..d9e9df57c2d74 100644 --- a/packages/@aws-cdk/aws-ecr/lib/repository.ts +++ b/packages/@aws-cdk/aws-ecr/lib/repository.ts @@ -1,7 +1,7 @@ import { EOL } from 'os'; import * as events from '@aws-cdk/aws-events'; import * as iam from '@aws-cdk/aws-iam'; -import { IResource, Lazy, RemovalPolicy, Resource, Stack, Token } from '@aws-cdk/core'; +import { 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'; @@ -252,17 +252,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, + }); + } } /** @@ -292,6 +316,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.principalIsANewlyCreatedResource(principal)) { + return undefined; + } + + // condition #3 + const principalStack = Stack.of(principal); + if (this.stack.dependencies.includes(principalStack)) { + return undefined; + } + + return principalAccount; + } } /** @@ -441,7 +502,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 37cc58a8485eb..c4eeedef17493 100644 --- a/packages/@aws-cdk/aws-ecr/test/repository.test.ts +++ b/packages/@aws-cdk/aws-ecr/test/repository.test.ts @@ -1,5 +1,5 @@ import { EOL } from 'os'; -import { expect as expectCDK, haveResource, haveResourceLike, ResourcePart } from '@aws-cdk/assert-internal'; +import { arrayWith, expect as expectCDK, haveResource, haveResourceLike, objectLike, ResourcePart } from '@aws-cdk/assert-internal'; import * as iam from '@aws-cdk/aws-iam'; import * as cdk from '@aws-cdk/core'; import * as ecr from '../lib'; @@ -330,6 +330,36 @@ describe('repository', () => { expect(() => app.synth()).toThrow(/A PolicyStatement used in a resource-based policy must specify at least one IAM principal/); }); + test('removal policy is "Retain" by default', () => { + // GIVEN + const stack = new cdk.Stack(); + + // WHEN + new ecr.Repository(stack, 'Repo'); + + // THEN + expectCDK(stack).to(haveResource('AWS::ECR::Repository', { + 'Type': 'AWS::ECR::Repository', + 'DeletionPolicy': 'Retain', + }, ResourcePart.CompleteDefinition)); + }); + + 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 + expectCDK(stack).to(haveResource('AWS::ECR::Repository', { + 'Type': 'AWS::ECR::Repository', + 'DeletionPolicy': 'Delete', + }, ResourcePart.CompleteDefinition)); + }); + describe('events', () => { test('onImagePushed without imageTag creates the correct event', () => { const stack = new cdk.Stack(); @@ -462,63 +492,6 @@ describe('repository', () => { 'State': 'ENABLED', })); }); - - test('removal policy is "Retain" by default', () => { - // GIVEN - const stack = new cdk.Stack(); - - // WHEN - new ecr.Repository(stack, 'Repo'); - - // THEN - expectCDK(stack).to(haveResource('AWS::ECR::Repository', { - 'Type': 'AWS::ECR::Repository', - 'DeletionPolicy': 'Retain', - }, ResourcePart.CompleteDefinition)); - }); - - 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 - expectCDK(stack).to(haveResource('AWS::ECR::Repository', { - 'Type': 'AWS::ECR::Repository', - 'DeletionPolicy': 'Delete', - }, ResourcePart.CompleteDefinition)); - }); - - test('grant adds appropriate resource-*', () => { - // GIVEN - const stack = new cdk.Stack(); - const repo = new ecr.Repository(stack, 'TestHarnessRepo'); - - // WHEN - repo.grantPull(new iam.AnyPrincipal()); - - // THEN - expectCDK(stack).to(haveResource('AWS::ECR::Repository', { - 'RepositoryPolicyText': { - 'Statement': [ - { - 'Action': [ - 'ecr:BatchCheckLayerAvailability', - 'ecr:GetDownloadUrlForLayer', - 'ecr:BatchGetImage', - ], - 'Effect': 'Allow', - 'Principal': { AWS: '*' }, - }, - ], - 'Version': '2012-10-17', - }, - })); - }); }); describe('repository name validation', () => { @@ -588,4 +561,237 @@ describe('repository', () => { })).toThrow(/must follow the specified pattern/); }); }); + + describe('permission grants', () => { + test('grant adds appropriate resource-*', () => { + // GIVEN + const stack = new cdk.Stack(); + const repo = new ecr.Repository(stack, 'TestHarnessRepo'); + + // WHEN + repo.grantPull(new iam.AnyPrincipal()); + + // THEN + expectCDK(stack).to(haveResource('AWS::ECR::Repository', { + 'RepositoryPolicyText': { + 'Statement': [ + { + 'Action': [ + 'ecr:BatchCheckLayerAvailability', + 'ecr:GetDownloadUrlForLayer', + 'ecr:BatchGetImage', + ], + 'Effect': 'Allow', + 'Principal': { AWS: '*' }, + }, + ], + 'Version': '2012-10-17', + }, + })); + }); + + describe('across accounts', () => { + let repoStack: cdk.Stack; + let repo: ecr.IRepository; + + let principalStack: cdk.Stack; + let newPrincipal: iam.IGrantable; + let existingPrincipal: iam.IGrantable; + + beforeEach(() => { + const app = new cdk.App(); + + repoStack = new cdk.Stack(app, 'RepoStack', { + env: { + account: 'repo-account', + region: 'us-west-2', + }, + }); + repo = new ecr.Repository(repoStack, 'TestHarnessRepo', { + repositoryName: cdk.PhysicalName.GENERATE_IF_NEEDED, + }); + + principalStack = new cdk.Stack(app, 'PrincipalStack', { + env: { + account: 'principal-account', + region: 'us-west-2', + }, + }); + newPrincipal = new iam.Role(principalStack, 'Role', { + assumedBy: new iam.AccountRootPrincipal(), + roleName: cdk.PhysicalName.GENERATE_IF_NEEDED, + }); + existingPrincipal = iam.Role.fromRoleArn(principalStack, 'ImportedRole', + 'arn:aws:iam::principal-account:role/my-principal-role'); + }); + + test("puts the new principal in the repo's resource policy if the principal Stack's is a dependency of its Stack", () => { + // GIVEN + repoStack.addDependency(principalStack); + + // WHEN + repo.grantPull(newPrincipal); + + // THEN + expectCDK(repoStack).to(haveResource('AWS::ECR::Repository', { + 'RepositoryPolicyText': objectLike({ + 'Statement': arrayWith(objectLike({ + 'Action': [ + 'ecr:BatchCheckLayerAvailability', + 'ecr:GetDownloadUrlForLayer', + 'ecr:BatchGetImage', + ], + 'Effect': 'Allow', + 'Principal': { + 'AWS': { + 'Fn::Join': ['', [ + 'arn:', + { 'Ref': 'AWS::Partition' }, + ':iam::principal-account:role/principalstackincipalstackrole7c6dfa81516e80f323e3', + ]], + }, + }, + })), + }), + })); + + expectCDK(principalStack).to(haveResource('AWS::IAM::Policy', { + 'PolicyDocument': objectLike({ + 'Statement': arrayWith( + objectLike({ + 'Action': [ + 'ecr:BatchCheckLayerAvailability', + 'ecr:GetDownloadUrlForLayer', + 'ecr:BatchGetImage', + ], + 'Effect': 'Allow', + 'Resource': { + 'Fn::Join': ['', [ + 'arn:', + { 'Ref': 'AWS::Partition' }, + ':ecr:us-west-2:repo-account:repository/repostackktestharnessrepo2a7c0b92dc5ee3ff7310', + ]], + }, + }), + objectLike({ + 'Action': 'ecr:GetAuthorizationToken', + 'Effect': 'Allow', + 'Resource': '*', + }), + ), + }), + })); + }); + + test("puts the new principal's account in the repo's resource policy if the principal Stack's is _not_ a dependency of its Stack", () => { + // GIVEN + // we do _not_ add a dependency on principalStack to repoStack + + // WHEN + repo.grantPull(newPrincipal); + + // THEN + expectCDK(repoStack).to(haveResource('AWS::ECR::Repository', { + 'RepositoryPolicyText': objectLike({ + 'Statement': arrayWith(objectLike({ + 'Action': [ + 'ecr:BatchCheckLayerAvailability', + 'ecr:GetDownloadUrlForLayer', + 'ecr:BatchGetImage', + ], + 'Effect': 'Allow', + 'Principal': { + 'AWS': { + 'Fn::Join': ['', [ + 'arn:', + { 'Ref': 'AWS::Partition' }, + ':iam::principal-account:root', + ]], + }, + }, + })), + }), + })); + + expectCDK(principalStack).to(haveResource('AWS::IAM::Policy', { + 'PolicyDocument': objectLike({ + 'Statement': arrayWith( + objectLike({ + 'Action': [ + 'ecr:BatchCheckLayerAvailability', + 'ecr:GetDownloadUrlForLayer', + 'ecr:BatchGetImage', + ], + 'Effect': 'Allow', + 'Resource': { + 'Fn::Join': ['', [ + 'arn:', + { 'Ref': 'AWS::Partition' }, + ':ecr:us-west-2:repo-account:repository/repostackktestharnessrepo2a7c0b92dc5ee3ff7310', + ]], + }, + }), + objectLike({ + 'Action': 'ecr:GetAuthorizationToken', + 'Effect': 'Allow', + 'Resource': '*', + }), + ), + }), + })); + }); + + test("puts the existing principal in the repo's resource policy, even if the principal Stack's is not a dependency of its Stack", () => { + // GIVEN + // we do _not_ add a dependency on principalStack to repoStack + + // WHEN + repo.grantPull(existingPrincipal); + + // THEN + expectCDK(repoStack).to(haveResource('AWS::ECR::Repository', { + 'RepositoryPolicyText': objectLike({ + 'Statement': arrayWith(objectLike({ + 'Action': [ + 'ecr:BatchCheckLayerAvailability', + 'ecr:GetDownloadUrlForLayer', + 'ecr:BatchGetImage', + ], + 'Effect': 'Allow', + 'Principal': { + 'AWS': 'arn:aws:iam::principal-account:role/my-principal-role', + }, + })), + }), + })); + + expectCDK(principalStack).to(haveResource('AWS::IAM::Policy', { + 'PolicyDocument': objectLike({ + 'Statement': arrayWith( + objectLike({ + 'Action': [ + 'ecr:BatchCheckLayerAvailability', + 'ecr:GetDownloadUrlForLayer', + 'ecr:BatchGetImage', + ], + 'Effect': 'Allow', + 'Resource': { + 'Fn::Join': ['', [ + 'arn:', + { 'Ref': 'AWS::Partition' }, + ':ecr:us-west-2:repo-account:repository/repostackktestharnessrepo2a7c0b92dc5ee3ff7310', + ]], + }, + }), + objectLike({ + 'Action': 'ecr:GetAuthorizationToken', + 'Effect': 'Allow', + 'Resource': '*', + }), + ), + }), + })); + }); + }); + }); }); 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 3f409739d76ed..ce047e5043fc7 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 @@ -1,5 +1,5 @@ import '@aws-cdk/assert-internal/jest'; -import { SynthUtils } from '@aws-cdk/assert-internal'; +import { arrayWith, objectLike, SynthUtils } from '@aws-cdk/assert-internal'; import * as ecr from '@aws-cdk/aws-ecr'; import * as cdk from '@aws-cdk/core'; import * as ecs from '../../lib'; @@ -82,55 +82,40 @@ describe('tag parameter container image', () => { 'Fn::Join': ['', [ 'arn:', { Ref: 'AWS::Partition' }, - ':iam::service-account:role/servicestackionexecutionrolee7e2d9a783a54eb795f4', + ':iam::service-account:root', ]], }, }, }], }, }); - expect(serviceStack).toHaveResourceLike('AWS::IAM::Role', { - RoleName: 'servicestackionexecutionrolee7e2d9a783a54eb795f4', - }); - expect(serviceStack).toHaveResourceLike('AWS::ECS::TaskDefinition', { - ContainerDefinitions: [ - { - 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' }, - ]], - }, - }, - ], - }); - + expect(serviceStack).toHaveResourceLike('AWS::IAM::Policy', { + PolicyDocument: objectLike({ + Statement: arrayWith( + 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}`, + ]], + }, + }), + 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 eca266f6975dd..0c70bc9be739b 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 06c2a9bb6cdcd..ce737fbd2f670 100644 --- a/packages/@aws-cdk/aws-iam/lib/index.ts +++ b/packages/@aws-cdk/aws-iam/lib/index.ts @@ -13,6 +13,7 @@ export * from './unknown-principal'; export * from './oidc-provider'; export * from './permissions-boundary'; export * from './saml-provider'; +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 cc8bddc0e8a17..db5f5316b02f6 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 14ca172de5506..62175666b3f7e 100644 --- a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts +++ b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts @@ -4,7 +4,7 @@ import { AccountPrincipal, AccountRootPrincipal, Anyone, ArnPrincipal, CanonicalUserPrincipal, FederatedPrincipal, IPrincipal, PrincipalBase, PrincipalPolicyFragment, ServicePrincipal, ServicePrincipalOpts, } from './principals'; -import { mergePrincipal } from './util'; +import { mergePrincipal } 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 60862dca07c56..8278b472f35fd 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 2c89f96749324..151d135830f52 100644 --- a/packages/@aws-cdk/aws-iam/lib/principals.ts +++ b/packages/@aws-cdk/aws-iam/lib/principals.ts @@ -2,8 +2,8 @@ import * as cdk from '@aws-cdk/core'; import { Default, RegionInfo } from '@aws-cdk/region-info'; import { IOpenIdConnectProvider } from './oidc-provider'; import { Condition, Conditions, PolicyStatement } from './policy-statement'; +import { mergePrincipal } from './private/util'; import { ISamlProvider } from './saml-provider'; -import { 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/util.ts b/packages/@aws-cdk/aws-iam/lib/private/util.ts similarity index 98% rename from packages/@aws-cdk/aws-iam/lib/util.ts rename to packages/@aws-cdk/aws-iam/lib/private/util.ts index 19fcbffe09639..929848837ffd0 100644 --- a/packages/@aws-cdk/aws-iam/lib/util.ts +++ b/packages/@aws-cdk/aws-iam/lib/private/util.ts @@ -1,6 +1,6 @@ import { captureStackTrace, DefaultTokenResolver, IPostProcessor, IResolvable, IResolveContext, Lazy, StringConcat, Token, Tokenization } from '@aws-cdk/core'; import { IConstruct } from 'constructs'; -import { IPolicy } from './policy'; +import { IPolicy } from '../policy'; const MAX_POLICY_NAME_LEN = 128; @@ -123,4 +123,4 @@ export class UniqueStringSet implements IResolvable, IPostProcessor { public toString(): string { return Token.asString(this); } -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/aws-iam/lib/role.ts b/packages/@aws-cdk/aws-iam/lib/role.ts index f142940b2b3ef..3d2155fa0675a 100644 --- a/packages/@aws-cdk/aws-iam/lib/role.ts +++ b/packages/@aws-cdk/aws-iam/lib/role.ts @@ -9,7 +9,7 @@ import { PolicyDocument } from './policy-document'; import { PolicyStatement } from './policy-statement'; import { AddToPrincipalPolicyResult, ArnPrincipal, IPrincipal, PrincipalPolicyFragment } from './principals'; import { ImmutableRole } from './private/immutable-role'; -import { AttachedPolicies, UniqueStringSet } from './util'; +import { AttachedPolicies, UniqueStringSet } from './private/util'; /** * Properties for defining an IAM Role @@ -540,4 +540,4 @@ export interface WithoutPolicyUpdatesOptions { * @default false */ readonly addGrantsToResources?: boolean; -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/aws-iam/lib/user.ts b/packages/@aws-cdk/aws-iam/lib/user.ts index 4874e84f791df..975b9d9a8aed4 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..a35481ba5f931 --- /dev/null +++ b/packages/@aws-cdk/aws-iam/lib/utils.ts @@ -0,0 +1,44 @@ +import { Construct, IConstruct } from 'constructs'; +import { Group } from './group'; +import { IPrincipal } from './principals'; +import { Role } from './role'; +import { User } from './user'; + +/** + * 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 principalIsANewlyCreatedResource(principal: IPrincipal): principal is IPrincipal & IConstruct { + // a newly created resource will for sure be a construct + if (!isConstruct(principal)) { + return false; + } + + // yes, this sucks + // this is just a temporary stopgap to stem the bleeding while we work on a proper fix + return principal instanceof Role || + principal instanceof User || + principal instanceof Group; +} + +/** + * 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 aae71efb460a7..2e434364e59db 100644 --- a/packages/@aws-cdk/aws-kms/lib/key.ts +++ b/packages/@aws-cdk/aws-kms/lib/key.ts @@ -1,7 +1,7 @@ import * as iam from '@aws-cdk/aws-iam'; import { FeatureFlags, IResource, Lazy, RemovalPolicy, Resource, Stack, Duration } from '@aws-cdk/core'; import * as cxapi from '@aws-cdk/cx-api'; -import { IConstruct, Construct } from 'constructs'; +import { Construct } from 'constructs'; import { Alias } from './alias'; import { CfnKey } from './kms.generated'; import * as perms from './private/perms'; @@ -201,15 +201,11 @@ 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 (!this.principalIsANewlyCreatedResource(grantPrincipal)) { + if (!iam.principalIsANewlyCreatedResource(grantPrincipal)) { return undefined; } - // return undefined; const keyStack = Stack.of(this); const granteeStack = Stack.of(grantPrincipal); if (keyStack === granteeStack) { @@ -220,29 +216,21 @@ abstract class KeyBase extends Resource implements IKey { : undefined; } - private principalIsANewlyCreatedResource(principal: IConstruct): boolean { - // yes, this sucks - // this is just a temporary stopgap to stem the bleeding while we work on a proper fix - return principal instanceof iam.Role || - principal instanceof iam.User || - principal instanceof iam.Group; - } - private isGranteeFromAnotherRegion(grantee: iam.IGrantable): boolean { - if (!isConstruct(grantee)) { + if (!iam.principalIsANewlyCreatedResource(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.principalIsANewlyCreatedResource(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; } } @@ -675,20 +663,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