fix(s3): revert incorrect account used for S3 event source mapping (#…

…29405) Reverts #29365 as it causes test failure.
aws · Mar 7, 2024 · ba41996 · ba41996
1 parent 3fb0254
commit ba41996
Show file tree

Hide file tree

Showing 3 changed files with 8 additions and 96 deletions.
diff --git a/packages/aws-cdk-lib/aws-lambda-event-sources/test/s3.test.ts b/packages/aws-cdk-lib/aws-lambda-event-sources/test/s3.test.ts
@@ -228,86 +228,4 @@ describe('S3EventSource', () => {
       },
     });
   });
-  test('Cross account buckect access', () => {
-    // GIVEN
-    const app = new cdk.App();
-    const stack = new cdk.Stack(app, 'stack');
-    const fn = new TestFunction(stack, 'Fn');
-
-    let accountB = '1234567';
-    //WHEN
-    const foreignBucket =
-      s3.Bucket.fromBucketAttributes(stack, 'ImportedBucket', {
-        bucketArn: 'arn:aws:s3:::some-bucket-not-in-this-account',
-        // The account the bucket really lives in
-        account: accountB,
-      });
-
-    // This will generate the IAM bindings
-    fn.addEventSource(new sources.S3EventSource(foreignBucket as s3.Bucket,
-      { events: [s3.EventType.OBJECT_CREATED] }));
-
-    // THEN
-    Template.fromStack(stack).hasResourceProperties('AWS::Lambda::Permission', {
-      'Principal': 's3.amazonaws.com',
-      'SourceAccount': '1234567',
-      'SourceArn': 'arn:aws:s3:::some-bucket-not-in-this-account',
-    });
-  });
-
-  test('Test bucket account is referenced intrinsicly', () => {
-    // GIVEN
-    const stack = new cdk.Stack();
-    const fn = new TestFunction(stack, 'Fn');
-    const bucket = new s3.Bucket(stack, 'B');
-
-    // WHEN
-    fn.addEventSource(new sources.S3EventSource(bucket, {
-      events: [s3.EventType.OBJECT_CREATED, s3.EventType.OBJECT_REMOVED],
-      filters: [
-        { prefix: 'prefix/' },
-        { suffix: '.png' },
-      ],
-    }));
-
-    // THEN
-    Template.fromStack(stack).hasResourceProperties('AWS::Lambda::Permission', {
-      'Principal': 's3.amazonaws.com',
-      'SourceAccount': {
-        'Ref': 'AWS::AccountId',
-      },
-      'SourceArn': {
-        'Fn::GetAtt': ['B08E7C7AF', 'Arn'],
-      },
-    });
-  });
-
-  test('Default to stack account if bucket account doesnt exist', () => {
-    // GIVEN
-    const app = new cdk.App();
-    const stack = new cdk.Stack(app, 'stack');
-    const fn = new TestFunction(stack, 'Fn');
-
-    let accountB = '';
-    //WHEN
-    const foreignBucket =
-      s3.Bucket.fromBucketAttributes(stack, 'ImportedBucket', {
-        bucketArn: 'arn:aws:s3:::some-bucket-not-in-this-account',
-        // The account the bucket really lives in
-        account: accountB,
-      });
-
-    // This will generate the IAM bindings
-    fn.addEventSource(new sources.S3EventSource(foreignBucket as s3.Bucket,
-      { events: [s3.EventType.OBJECT_CREATED] }));
-
-    // THEN
-    Template.fromStack(stack).hasResourceProperties('AWS::Lambda::Permission', {
-      'Principal': 's3.amazonaws.com',
-      'SourceAccount': {
-        'Ref': 'AWS::AccountId',
-      },
-      'SourceArn': 'arn:aws:s3:::some-bucket-not-in-this-account',
-    });
-  });
 });
diff --git a/packages/aws-cdk-lib/aws-s3-notifications/lib/lambda.ts b/packages/aws-cdk-lib/aws-s3-notifications/lib/lambda.ts
@@ -21,7 +21,7 @@ export class LambdaDestination implements s3.IBucketNotificationDestination {
 
     if (bucket.node.tryFindChild(permissionId) === undefined) {
       this.fn.addPermission(permissionId, {
-        sourceAccount: bucket.account || Stack.of(bucket).account,
+        sourceAccount: Stack.of(bucket).account,
         principal: new iam.ServicePrincipal('s3.amazonaws.com'),
         sourceArn: bucket.bucketArn,
         // Placing the permissions node in the same scope as the s3 bucket.

diff --git a/packages/aws-cdk-lib/aws-s3/lib/bucket.ts b/packages/aws-cdk-lib/aws-s3/lib/bucket.ts
@@ -95,11 +95,6 @@ export interface IBucket extends IResource {
    */
   policy?: BucketPolicy;
 
-  /**
-   * The account bucket belongs to.
-   */
-  readonly account?: string;
-
   /**
    * Adds a statement to the resource policy for a principal (i.e.
    * account/role/service) to perform actions on this bucket and/or its
@@ -1759,7 +1754,6 @@ export class Bucket extends BucketBase {
       public readonly bucketWebsiteNewUrlFormat = attrs.bucketWebsiteNewUrlFormat ?? false;
       public readonly encryptionKey = attrs.encryptionKey;
       public readonly isWebsite = attrs.isWebsite ?? false;
-      public readonly account = attrs.account;
       public policy?: BucketPolicy = undefined;
       protected autoCreatePolicy = false;
       protected disallowPublicAccess = false;
@@ -1973,11 +1967,11 @@ export class Bucket extends BucketBase {
 
     if (props.serverAccessLogsBucket instanceof Bucket) {
       props.serverAccessLogsBucket.allowLogDelivery(this, props.serverAccessLogsPrefix);
-      // It is possible that `serverAccessLogsBucket` was specified but is some other `IBucket`
-      // that cannot have the ACLs or bucket policy applied. In that scenario, we should only
-      // setup log delivery permissions to `this` if a bucket was not specified at all, as documented.
-      // For example, we should not allow log delivery to `this` if given an imported bucket or
-      // another situation that causes `instanceof` to fail
+    // It is possible that `serverAccessLogsBucket` was specified but is some other `IBucket`
+    // that cannot have the ACLs or bucket policy applied. In that scenario, we should only
+    // setup log delivery permissions to `this` if a bucket was not specified at all, as documented.
+    // For example, we should not allow log delivery to `this` if given an imported bucket or
+    // another situation that causes `instanceof` to fail
     } else if (!props.serverAccessLogsBucket && props.serverAccessLogsPrefix) {
       this.allowLogDelivery(this, props.serverAccessLogsPrefix);
     } else if (props.serverAccessLogsBucket) {
@@ -2231,7 +2225,7 @@ export class Bucket extends BucketBase {
     function parseLifecycleRule(rule: LifecycleRule): CfnBucket.RuleProperty {
       const enabled = rule.enabled ?? true;
       if ((rule.expiredObjectDeleteMarker)
-        && (rule.expiration || rule.expirationDate || self.parseTagFilters(rule.tagFilters))) {
+      && (rule.expiration || rule.expirationDate || self.parseTagFilters(rule.tagFilters))) {
         // ExpiredObjectDeleteMarker cannot be specified with ExpirationInDays, ExpirationDate, or TagFilters.
         throw new Error('ExpiredObjectDeleteMarker cannot be specified with expiration, ExpirationDate, or TagFilters.');
       }
@@ -2356,7 +2350,7 @@ export class Bucket extends BucketBase {
     }
 
     if (accessControlRequiresObjectOwnership && this.objectOwnership === ObjectOwnership.BUCKET_OWNER_ENFORCED) {
-      throw new Error(`objectOwnership must be set to "${ObjectOwnership.OBJECT_WRITER}" when accessControl is "${this.accessControl}"`);
+      throw new Error (`objectOwnership must be set to "${ObjectOwnership.OBJECT_WRITER}" when accessControl is "${this.accessControl}"`);
     }
 
     return {