add feadback from colifran

aws · Feb 20, 2024 · bd87f7f · bd87f7f
1 parent ca9c2cd
commit bd87f7f
Show file tree

Hide file tree

Showing 5 changed files with 30 additions and 29 deletions.
diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.ts b/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.ts
@@ -32,15 +32,14 @@ new TopicPolicy(stack, 'TopicPolicy', {
 const topicAddPolicy = new Topic(stack, 'TopicAddPolicy', {
   topicName: 'topicAddPolicy',
   displayName: 'topicDisplayNameAddPolicy',
+  enforceSSL: true,
 });
 
 topicAddPolicy.addToResourcePolicy(new PolicyStatement({
   principals: [new ServicePrincipal('s3.amazonaws.com')],
   actions: ['sns:Publish'],
   resources: [topicAddPolicy.topicArn],
-}), {
-  enforceSSL: true,
-});
+}));
 
 new IntegTest(app, 'SNSTopicPolicyInteg', {
   testCases: [stack],

diff --git a/packages/aws-cdk-lib/aws-sns/README.md b/packages/aws-cdk-lib/aws-sns/README.md
@@ -232,18 +232,18 @@ const topicPolicy = new sns.TopicPolicy(this, 'Policy', {
 });
 ```
 
-Similiarly for `addToResourcePolicy`, you can enforce SSL by setting the `enforceSSL` flag:
+Similiarly you can enforce SSL by setting the `enforceSSL` flag on the topic:
 
 ```ts
-const topic = new sns.Topic(this, 'TopicAddPolicy');
+const topic = new sns.Topic(this, 'TopicAddPolicy', {
+  enforceSSL: true,
+});
 
 topic.addToResourcePolicy(new iam.PolicyStatement({
   principals: [new iam.ServicePrincipal('s3.amazonaws.com')],
   actions: ['sns:Publish'],
   resources: [topic.topicArn],
-}), {
-  enforceSSL: true,
-});
+}));
 ```
 
 ## Delivery status logging

diff --git a/packages/aws-cdk-lib/aws-sns/lib/topic-base.ts b/packages/aws-cdk-lib/aws-sns/lib/topic-base.ts
@@ -51,28 +51,14 @@ export interface ITopic extends IResource, notifications.INotificationRuleTarget
    * will be automatically created upon the first call to `addToResourcePolicy`. If
    * the topic is imported (`Topic.import`), then this is a no-op.
    */
-  addToResourcePolicy(statement: iam.PolicyStatement, addToResourcePolicyProps?: AddToResourcePolicyProps): iam.AddToResourcePolicyResult;
+  addToResourcePolicy(statement: iam.PolicyStatement): iam.AddToResourcePolicyResult;
 
   /**
    * Grant topic publishing permissions to the given identity
    */
   grantPublish(identity: iam.IGrantable): iam.Grant;
 }
 
-/**
- * Properties for adding a resource policy
- */
-export interface AddToResourcePolicyProps {
-  /**
-   * Adds a statement to enforce encryption of data in transit when publishing to the topic.
-   *
-   * For more information, see https://docs.aws.amazon.com/sns/latest/dg/sns-security-best-practices.html#enforce-encryption-data-in-transit.
-   *
-   * @default false
-   */
-  readonly enforceSSL?: boolean;
-}
-
 /**
  * Either a new or imported Topic
  */
@@ -92,6 +78,11 @@ export abstract class TopicBase extends Resource implements ITopic {
    */
   protected abstract readonly autoCreatePolicy: boolean;
 
+  /**
+   * Adds a statement to enforce encryption of data in transit when publishing to the topic.
+   */
+  protected enforceSSL?: boolean;
+
   private policy?: TopicPolicy;
 
   constructor(scope: Construct, id: string, props: ResourceProps = {}) {
@@ -139,15 +130,15 @@ export abstract class TopicBase extends Resource implements ITopic {
    * will be automatically created upon the first call to `addToResourcePolicy`. If
    * the topic is imported (`Topic.import`), then this is a no-op.
    */
-  public addToResourcePolicy(statement: iam.PolicyStatement, addToResourcePolicyProps?: AddToResourcePolicyProps): iam.AddToResourcePolicyResult {
+  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);
 
-      if (addToResourcePolicyProps?.enforceSSL) {
+      if (this.enforceSSL) {
         this.policy.document.addStatements(this.createSSLPolicyDocument());
       }
       return { statementAdded: true, policyDependable: this.policy };

diff --git a/packages/aws-cdk-lib/aws-sns/lib/topic.ts b/packages/aws-cdk-lib/aws-sns/lib/topic.ts
@@ -67,6 +67,15 @@ export interface TopicProps {
    * @default - do not archive messages
    */
   readonly messageRetentionPeriodInDays?: number;
+
+  /**
+   * Adds a statement to enforce encryption of data in transit when publishing to the topic.
+   *
+   * For more information, see https://docs.aws.amazon.com/sns/latest/dg/sns-security-best-practices.html#enforce-encryption-data-in-transit.
+   *
+   * @default false
+   */
+  readonly enforceSSL?: boolean;
 }
 
 /**
@@ -174,6 +183,8 @@ export class Topic extends TopicBase {
       physicalName: props.topicName,
     });
 
+    this.enforceSSL = props.enforceSSL;
+
     if (props.contentBasedDeduplication && !props.fifo) {
       throw new Error('Content based deduplication can only be enabled for FIFO SNS topics.');
     }

diff --git a/packages/aws-cdk-lib/aws-sns/test/sns.test.ts b/packages/aws-cdk-lib/aws-sns/test/sns.test.ts
@@ -189,16 +189,16 @@ describe('Topic', () => {
   test('can add a policy to the topic enforcing ssl', () => {
     // GIVEN
     const stack = new cdk.Stack();
-    const topic = new sns.Topic(stack, 'Topic');
+    const topic = new sns.Topic(stack, 'Topic', {
+      enforceSSL: true,
+    });
 
     // WHEN
     topic.addToResourcePolicy(new iam.PolicyStatement({
       resources: ['*'],
       actions: ['sns:*'],
       principals: [new iam.ArnPrincipal('arn')],
-    }), {
-      enforceSSL: true,
-    });
+    }));
 
     // THEN
     Template.fromStack(stack).hasResourceProperties('AWS::SNS::TopicPolicy', {