From d2ddd86d0a59f64baae65b03264b22ec01c83a0e Mon Sep 17 00:00:00 2001 From: ap00rv Date: Sun, 27 Sep 2020 00:37:27 +0000 Subject: [PATCH 1/6] fix(sns): enable creating instance of TopicPolicy TopicPolicy class had a bug. It did specify a mandatory prop for IAM policy document to use in AWS::SNS::TopicPolicy fixes #7934 --- packages/@aws-cdk/aws-sns/lib/policy.ts | 17 ++++---- packages/@aws-cdk/aws-sns/lib/topic-base.ts | 2 +- packages/@aws-cdk/aws-sns/test/test.sns.ts | 45 +++++++++++++++++++++ 3 files changed, 55 insertions(+), 9 deletions(-) diff --git a/packages/@aws-cdk/aws-sns/lib/policy.ts b/packages/@aws-cdk/aws-sns/lib/policy.ts index 60aec85b13b8c..f60c567455382 100644 --- a/packages/@aws-cdk/aws-sns/lib/policy.ts +++ b/packages/@aws-cdk/aws-sns/lib/policy.ts @@ -11,6 +11,11 @@ export interface TopicPolicyProps { * The set of topics this policy applies to. */ readonly topics: ITopic[]; + /** + * IAM policy document to apply to topic(s). + */ + readonly policyDocument: PolicyDocument; + } /** @@ -20,19 +25,15 @@ export class TopicPolicy extends Resource { /** * The IAM policy document for this policy. */ - public readonly document = new PolicyDocument({ - // statements must be unique, so we use the statement index. - // potantially SIDs can change as a result of order change, but this should - // not have an impact on the policy evaluation. - // https://docs.aws.amazon.com/sns/latest/dg/AccessPolicyLanguage_SpecialInfo.html - assignSids: true, - }); + public readonly document: PolicyDocument; constructor(scope: Construct, id: string, props: TopicPolicyProps) { super(scope, id); + this.document = props.policyDocument; + new CfnTopicPolicy(this, 'Resource', { - policyDocument: this.document, + policyDocument: props.policyDocument, topics: props.topics.map(t => t.topicArn), }); } diff --git a/packages/@aws-cdk/aws-sns/lib/topic-base.ts b/packages/@aws-cdk/aws-sns/lib/topic-base.ts index bddfde6288748..4477c032a7c80 100644 --- a/packages/@aws-cdk/aws-sns/lib/topic-base.ts +++ b/packages/@aws-cdk/aws-sns/lib/topic-base.ts @@ -92,7 +92,7 @@ export abstract class TopicBase extends Resource implements ITopic { */ public addToResourcePolicy(statement: iam.PolicyStatement): iam.AddToResourcePolicyResult { if (!this.policy && this.autoCreatePolicy) { - this.policy = new TopicPolicy(this, 'Policy', { topics: [this] }); + this.policy = new TopicPolicy(this, 'Policy', { topics: [this], policyDocument: new iam.PolicyDocument({ assignSids: true }) }); } if (this.policy) { diff --git a/packages/@aws-cdk/aws-sns/test/test.sns.ts b/packages/@aws-cdk/aws-sns/test/test.sns.ts index dbf956e72153b..fc3acc35b2812 100644 --- a/packages/@aws-cdk/aws-sns/test/test.sns.ts +++ b/packages/@aws-cdk/aws-sns/test/test.sns.ts @@ -160,6 +160,51 @@ export = { test.done(); }, + 'TopicPolicy can be created'(test: Test) { + // GIVEN + const stack = new cdk.Stack(); + const topic = new sns.Topic(stack, 'MyTopic'); + const ps = new iam.PolicyStatement({ + actions: ['service:statement0'], + principals: [new iam.ArnPrincipal('arn')], + }); + + // WHEN + new sns.TopicPolicy(stack, 'topicpolicy', { topics: [topic], policyDocument: new iam.PolicyDocument({ assignSids: true, statements: [ps] }) }); + + // THEN + expect(stack).toMatch({ + 'Resources': { + 'MyTopic86869434': { + 'Type': 'AWS::SNS::Topic', + }, + 'topicpolicyF8CF12FD': { + 'Type': 'AWS::SNS::TopicPolicy', + 'Properties': { + 'PolicyDocument': { + 'Statement': [ + { + 'Action': 'service:statement0', + 'Effect': 'Allow', + 'Principal': { 'AWS': 'arn' }, + 'Sid': '0', + }, + ], + 'Version': '2012-10-17', + }, + 'Topics': [ + { + 'Ref': 'MyTopic86869434', + }, + ], + }, + }, + }, + }); + + test.done(); + }, + 'topic resource policy includes unique SIDs'(test: Test) { const stack = new cdk.Stack(); From ca65180fa7f756461fa6e45dc8486ac0ed081096 Mon Sep 17 00:00:00 2001 From: Mitchell Valine Date: Tue, 16 Mar 2021 08:44:41 -0700 Subject: [PATCH 2/6] add breaking change exclusion --- allowed-breaking-changes.txt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/allowed-breaking-changes.txt b/allowed-breaking-changes.txt index 2ca2ca5b6067f..d2e452410c1de 100644 --- a/allowed-breaking-changes.txt +++ b/allowed-breaking-changes.txt @@ -56,3 +56,7 @@ incompatible-argument:@aws-cdk/aws-ecs.TaskDefinition.addVolume # We made properties optional and it's really fine but our differ doesn't think so. weakened:@aws-cdk/cloud-assembly-schema.DockerImageSource weakened:@aws-cdk/cloud-assembly-schema.FileSource + +# Added required property to SNS TopicPolicy to prevent synth errors when not provided. +# No sensible default could be found. +strengthened:@aws-cdk/aws-sns.TopicPolicyProps From c490390f9ffcd5a9c21508a472ab1c6ca60ff3c6 Mon Sep 17 00:00:00 2001 From: Mitchell Valine Date: Tue, 16 Mar 2021 09:29:30 -0700 Subject: [PATCH 3/6] make policy document optional --- allowed-breaking-changes.txt | 4 -- packages/@aws-cdk/aws-sns/lib/policy.ts | 15 +++++-- packages/@aws-cdk/aws-sns/test/test.sns.ts | 48 +++++++++++++++++++++- 3 files changed, 58 insertions(+), 9 deletions(-) diff --git a/allowed-breaking-changes.txt b/allowed-breaking-changes.txt index d2e452410c1de..2ca2ca5b6067f 100644 --- a/allowed-breaking-changes.txt +++ b/allowed-breaking-changes.txt @@ -56,7 +56,3 @@ incompatible-argument:@aws-cdk/aws-ecs.TaskDefinition.addVolume # We made properties optional and it's really fine but our differ doesn't think so. weakened:@aws-cdk/cloud-assembly-schema.DockerImageSource weakened:@aws-cdk/cloud-assembly-schema.FileSource - -# Added required property to SNS TopicPolicy to prevent synth errors when not provided. -# No sensible default could be found. -strengthened:@aws-cdk/aws-sns.TopicPolicyProps diff --git a/packages/@aws-cdk/aws-sns/lib/policy.ts b/packages/@aws-cdk/aws-sns/lib/policy.ts index e527e7fdc38ff..03a791bd57814 100644 --- a/packages/@aws-cdk/aws-sns/lib/policy.ts +++ b/packages/@aws-cdk/aws-sns/lib/policy.ts @@ -14,8 +14,9 @@ export interface TopicPolicyProps { readonly topics: ITopic[]; /** * IAM policy document to apply to topic(s). + * @default empty policy document */ - readonly policyDocument: PolicyDocument; + readonly policyDocument?: PolicyDocument; } @@ -26,15 +27,21 @@ export class TopicPolicy extends Resource { /** * The IAM policy document for this policy. */ - public readonly document: PolicyDocument; + public readonly document = new PolicyDocument({ + // statements must be unique, so we use the statement index. + // potantially SIDs can change as a result of order change, but this should + // not have an impact on the policy evaluation. + // https://docs.aws.amazon.com/sns/latest/dg/AccessPolicyLanguage_SpecialInfo.html + assignSids: true, + }); constructor(scope: Construct, id: string, props: TopicPolicyProps) { super(scope, id); - this.document = props.policyDocument; + this.document = props.policyDocument ?? this.document; new CfnTopicPolicy(this, 'Resource', { - policyDocument: props.policyDocument, + policyDocument: this.document, topics: props.topics.map(t => t.topicArn), }); } diff --git a/packages/@aws-cdk/aws-sns/test/test.sns.ts b/packages/@aws-cdk/aws-sns/test/test.sns.ts index f637726fd27ec..5261900f9059b 100644 --- a/packages/@aws-cdk/aws-sns/test/test.sns.ts +++ b/packages/@aws-cdk/aws-sns/test/test.sns.ts @@ -276,7 +276,7 @@ export = { test.done(); }, - 'TopicPolicy can be created'(test: Test) { + 'TopicPolicy passed document'(test: Test) { // GIVEN const stack = new cdk.Stack(); const topic = new sns.Topic(stack, 'MyTopic'); @@ -321,6 +321,52 @@ export = { test.done(); }, + 'Add statements to policy'(test: Test) { + // GIVEN + const stack = new cdk.Stack(); + const topic = new sns.Topic(stack, 'MyTopic'); + + // WHEN + const topicPolicy = new sns.TopicPolicy(stack, 'TopicPolicy', { + topics: [topic], + }); + topicPolicy.document.addStatements(new iam.PolicyStatement({ + actions: ['service:statement0'], + principals: [new iam.ArnPrincipal('arn')], + })); + + // THEN + expect(stack).toMatch({ + 'Resources': { + 'MyTopic86869434': { + 'Type': 'AWS::SNS::Topic', + }, + 'TopicPolicyA24B096F': { + 'Type': 'AWS::SNS::TopicPolicy', + 'Properties': { + 'PolicyDocument': { + 'Statement': [ + { + 'Action': 'service:statement0', + 'Effect': 'Allow', + 'Principal': { 'AWS': 'arn' }, + 'Sid': '0', + }, + ], + 'Version': '2012-10-17', + }, + 'Topics': [ + { + 'Ref': 'MyTopic86869434', + }, + ], + }, + }, + }, + }); + test.done(); + }, + 'topic resource policy includes unique SIDs'(test: Test) { const stack = new cdk.Stack(); From 1cef14fdc1f1f9f199d83ad7615663163f35a12f Mon Sep 17 00:00:00 2001 From: Mitchell Valine Date: Tue, 16 Mar 2021 09:32:55 -0700 Subject: [PATCH 4/6] remove passed default in addToResourcePolicy --- packages/@aws-cdk/aws-sns/lib/topic-base.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-sns/lib/topic-base.ts b/packages/@aws-cdk/aws-sns/lib/topic-base.ts index 2369b4140caee..db262ef87e068 100644 --- a/packages/@aws-cdk/aws-sns/lib/topic-base.ts +++ b/packages/@aws-cdk/aws-sns/lib/topic-base.ts @@ -96,7 +96,7 @@ export abstract class TopicBase extends Resource implements ITopic { */ public addToResourcePolicy(statement: iam.PolicyStatement): iam.AddToResourcePolicyResult { if (!this.policy && this.autoCreatePolicy) { - this.policy = new TopicPolicy(this, 'Policy', { topics: [this], policyDocument: new iam.PolicyDocument({ assignSids: true }) }); + this.policy = new TopicPolicy(this, 'Policy', { topics: [this] }); } if (this.policy) { From 357cc5e37b7284717ffcb7cc97ee9bf2c5dcd420 Mon Sep 17 00:00:00 2001 From: Mitchell Valine Date: Tue, 16 Mar 2021 09:52:22 -0700 Subject: [PATCH 5/6] add to readme --- packages/@aws-cdk/aws-sns/README.md | 39 +++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/packages/@aws-cdk/aws-sns/README.md b/packages/@aws-cdk/aws-sns/README.md index b5d9f52c3d9b9..c0e84029788ff 100644 --- a/packages/@aws-cdk/aws-sns/README.md +++ b/packages/@aws-cdk/aws-sns/README.md @@ -131,3 +131,42 @@ codeCommitRepository.onCommit(new targets.SnsTopic(myTopic)); This will result in adding a target to the event rule and will also modify the topic resource policy to allow CloudWatch events to publish to the topic. + +## Topic Policy + +A topic policy is automatically created when `addToResourcePolicy` is called, if +one doesn't already exist. Using `addToResourcePolicy` is the simplest way to +add policies, but a `TopicPolicy` can also be created manually. + +```ts +const topic = new sns.Topic(stack, 'Topic'); +const topicPolicy = new sns.TopicPolicy(stack, 'TopicPolicy', { + topics: [topic], +}); + +topicPolicy.document.addStatements(new iam.PolicyStatement({ + actions: ["sns:Subscribe"], + principals: [new iam.AnyPrincipal()], + resources: [topic.topicArn], +})); +``` + +A policy document can also be passed on `TopicPolicy` construction +```ts +const topic = new sns.Topic(stack, 'Topic'); +const policyDocument = new iam.PolicyDocument({ + assignSids: true, + statements: [ + new iam.PolicyStatement({ + actions: ["sns:Subscribe"], + principals: [new iam.AnyPrincipal()], + resources: [topic.topicArn] + }), + ], +}); + +const topicPolicy = new sns.TopicPolicy(this, 'Policy', { + topics: [topic], + policyDocument, +}); +``` From 21779c1f63d6b5ddbc106d6f800c13200bea4b51 Mon Sep 17 00:00:00 2001 From: Mitchell Valine Date: Tue, 16 Mar 2021 10:14:33 -0700 Subject: [PATCH 6/6] lint fix --- packages/@aws-cdk/aws-sns/README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/@aws-cdk/aws-sns/README.md b/packages/@aws-cdk/aws-sns/README.md index c0e84029788ff..ca80f72f9aad3 100644 --- a/packages/@aws-cdk/aws-sns/README.md +++ b/packages/@aws-cdk/aws-sns/README.md @@ -152,6 +152,7 @@ topicPolicy.document.addStatements(new iam.PolicyStatement({ ``` A policy document can also be passed on `TopicPolicy` construction + ```ts const topic = new sns.Topic(stack, 'Topic'); const policyDocument = new iam.PolicyDocument({