Merge pull request #443 from getlift/storage-additions

Add `allowAcl` and `cors` options to the storage construct

Author: mnapoli

.github/workflows/ci.yml

@@ -63,4 +63,4 @@ jobs:
                   key: ${{ runner.os }}-node-${{ hashFiles('**/package.json') }}
             - run: npm i
             - name: Typescript checks
-              run: tsc --noEmit
+              run: npx tsc --noEmit

docs/storage.md

@@ -113,6 +113,53 @@ The configuration maps directly to [CloudFormation LifecycleConfiguration Rules]
 
 These rules are added to the default lifecycle rules that Lift adds (intelligent tiering and old version cleanup).
 
+### ACL support
+
+Since April 2023, S3 buckets have ACLs disabled by default. However, many tools and libraries (including PHP's [Flysystem](https://github.com/thephpleague/flysystem), used by Laravel) send ACL headers on S3 operations. Without enabling ACLs, these operations will fail.
+
+To let the bucket accept ACL headers while keeping the bucket owner in full control:
+
+```yaml
+constructs:
+    storage:
+        type: storage
+        allowAcl: true
+```
+
+This sets the S3 bucket's [Object Ownership](https://docs.aws.amazon.com/AmazonS3/latest/userguide/about-object-ownership.html) to `BucketOwnerPreferred` and grants `s3:GetObjectAcl` and `s3:PutObjectAcl` permissions to Lambda functions.
+
+### CORS
+
+To allow browser-based uploads (e.g. via presigned URLs), you can configure CORS on the bucket.
+
+**Simple form** — allow a single origin with default methods (`GET`, `PUT`, `DELETE`) and all headers:
+
+```yaml
+constructs:
+    storage:
+        type: storage
+        cors: "${construct:website.url}"
+```
+
+Use `cors: "*"` to allow all origins.
+
+**Full form** — define complete CORS rules (property names can be camelCase or PascalCase):
+
+```yaml
+constructs:
+    storage:
+        type: storage
+        cors:
+            -   allowedOrigins:
+                    - "${construct:website.url}"
+                allowedMethods:
+                    - PUT
+                allowedHeaders:
+                    - "*"
+```
+
+The full form maps directly to [CloudFormation CorsRules](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-s3-bucket-corsconfiguration-corsrule.html).
+
 ## Extensions
 
 You can specify an `extensions` property on the storage construct to extend the underlying CloudFormation resources. In the exemple below, the S3 Bucket CloudFormation resource generated by the `avatars` storage construct will be extended with the new `AccessControl: PublicRead` CloudFormation property.

package.json

@@ -9,7 +9,7 @@
     "chalk": "^4.1.1",
     "change-case": "^4.1.2",
     "cidr-split": "^0.1.2",
-    "constructs": "10.2.20",
+    "constructs": "^10.5.0",
     "inquirer": "^8.2.7",
     "js-yaml": "^3.14.2",
     "lodash": "^4.17.21",

src/constructs/aws/Storage.ts

@@ -16,14 +16,18 @@ const STORAGE_DEFINITION = {
         encryption: {
             anyOf: [{ const: "s3" }, { const: "kms" }],
         },
+        allowAcl: { type: "boolean" },
+        cors: {
+            anyOf: [{ type: "array", items: { type: "object" } }, { type: "string" }],
+        },
         lifecycleRules: {
             type: "array",
             items: { type: "object" },
         },
     },
     additionalProperties: false,
 } as const;
-const STORAGE_DEFAULTS: Required<FromSchema<typeof STORAGE_DEFINITION>> = {
+const STORAGE_DEFAULTS: Omit<Required<FromSchema<typeof STORAGE_DEFINITION>>, "allowAcl" | "cors"> = {
     type: "storage",
     archive: 45,
     encryption: "s3",
@@ -59,13 +63,15 @@ export class Storage extends AwsConstruct {
     public static schema = STORAGE_DEFINITION;
 
     private readonly bucket: Bucket;
+    private readonly allowAcl: boolean;
     // a remplacer par StorageExtensionsKeys
     private readonly bucketNameOutput: CfnOutput;
 
     constructor(scope: CdkConstruct, id: string, configuration: Configuration, private provider: AwsProvider) {
         super(scope, id);
 
         const resolvedConfiguration = Object.assign({}, STORAGE_DEFAULTS, configuration);
+        this.allowAcl = resolvedConfiguration.allowAcl === true;
 
         const encryptionOptions = {
             s3: BucketEncryption.S3_MANAGED,
@@ -113,6 +119,28 @@ export class Storage extends AwsConstruct {
             Rules: [...defaultRules, ...userRules],
         });
 
+        if (this.allowAcl) {
+            cfnBucket.addPropertyOverride("OwnershipControls", {
+                Rules: [{ ObjectOwnership: "BucketOwnerPreferred" }],
+            });
+        }
+
+        if (resolvedConfiguration.cors !== undefined) {
+            let corsRules;
+            if (typeof resolvedConfiguration.cors === "string") {
+                corsRules = [
+                    {
+                        AllowedOrigins: [resolvedConfiguration.cors],
+                        AllowedMethods: ["GET", "PUT", "DELETE"],
+                        AllowedHeaders: ["*"],
+                    },
+                ];
+            } else {
+                corsRules = resolvedConfiguration.cors.map((rule) => capitalizeKeys(rule as Record<string, unknown>));
+            }
+            cfnBucket.addPropertyOverride("CorsConfiguration", { CorsRules: corsRules });
+        }
+
         this.bucketNameOutput = new CfnOutput(this, "BucketName", {
             value: this.bucket.bucketName,
         });
@@ -126,11 +154,16 @@ export class Storage extends AwsConstruct {
     }
 
     permissions(): PolicyStatement[] {
+        const actions = ["s3:PutObject", "s3:GetObject", "s3:DeleteObject", "s3:ListBucket"];
+        if (this.allowAcl) {
+            actions.push("s3:GetObjectAcl", "s3:PutObjectAcl");
+        }
+
         return [
-            new PolicyStatement(
-                ["s3:PutObject", "s3:GetObject", "s3:DeleteObject", "s3:ListBucket"],
-                [this.bucket.bucketArn, Stack.of(this).resolve(Fn.join("/", [this.bucket.bucketArn, "*"]))]
-            ),
+            new PolicyStatement(actions, [
+                this.bucket.bucketArn,
+                Stack.of(this).resolve(Fn.join("/", [this.bucket.bucketArn, "*"])),
+            ]),
         ];
     }
 

test/fixtures/permissions/serverless.yml

@@ -13,3 +13,6 @@ functions:
 constructs:
     testStorage:
         type: storage
+        allowAcl: true
+    testStorageNoAcl:
+        type: storage

test/fixtures/storage/serverless.yml

@@ -32,6 +32,21 @@ constructs:
                                   - HEAD
                                   - PUT
                                   - POST
+    withAcl:
+        type: storage
+        allowAcl: true
+    withCorsString:
+        type: storage
+        cors: "*"
+    withCorsRules:
+        type: storage
+        cors:
+            - allowedOrigins:
+                  - "https://example.com"
+              allowedMethods:
+                  - PUT
+              allowedHeaders:
+                  - "*"
     withLifecycleRules:
         type: storage
         lifecycleRules:

test/unit/permissions.test.ts

@@ -11,7 +11,14 @@ function expectLiftStorageStatementIsAdded(cfTemplate: CfTemplate) {
         expect.arrayContaining([
             expect.objectContaining({
                 Effect: "Allow",
-                Action: ["s3:PutObject", "s3:GetObject", "s3:DeleteObject", "s3:ListBucket"],
+                Action: [
+                    "s3:PutObject",
+                    "s3:GetObject",
+                    "s3:DeleteObject",
+                    "s3:ListBucket",
+                    "s3:GetObjectAcl",
+                    "s3:PutObjectAcl",
+                ],
             }),
         ])
     );
@@ -103,6 +110,27 @@ describe("permissions", () => {
         expectLiftStorageStatementIsAdded(cfTemplate);
     });
 
+    it("should not include ACL permissions when allowAcl is not set", async () => {
+        const { cfTemplate } = await runServerless({
+            fixture: "permissions",
+            configExt: pluginConfigExt,
+            command: "package",
+        });
+        const statements = get(
+            cfTemplate.Resources.IamRoleLambdaExecution,
+            "Properties.Policies[0].PolicyDocument.Statement"
+        ) as unknown as { Action: string[] }[];
+        // testStorageNoAcl should produce a statement without ACL permissions
+        expect(statements).toEqual(
+            expect.arrayContaining([
+                expect.objectContaining({
+                    Effect: "Allow",
+                    Action: ["s3:PutObject", "s3:GetObject", "s3:DeleteObject", "s3:ListBucket"],
+                }),
+            ])
+        );
+    });
+
     it("should be possible to disable automatic permissions", async () => {
         const { cfTemplate } = await runServerless({
             fixture: "permissions",

test/unit/storage.test.ts

@@ -77,6 +77,54 @@ describe("storage", () => {
         });
     });
 
+    it("should not set OwnershipControls by default", () => {
+        expect(cfTemplate.Resources[computeLogicalId("default", "Bucket")].Properties).not.toHaveProperty(
+            "OwnershipControls"
+        );
+    });
+
+    it("should set OwnershipControls when allowAcl is true", () => {
+        expect(cfTemplate.Resources[computeLogicalId("withAcl", "Bucket")].Properties).toMatchObject({
+            OwnershipControls: {
+                Rules: [{ ObjectOwnership: "BucketOwnerPreferred" }],
+            },
+        });
+    });
+
+    it("should not set CorsConfiguration by default", () => {
+        expect(cfTemplate.Resources[computeLogicalId("default", "Bucket")].Properties).not.toHaveProperty(
+            "CorsConfiguration"
+        );
+    });
+
+    it("should configure CORS with default methods when cors is a string", () => {
+        expect(cfTemplate.Resources[computeLogicalId("withCorsString", "Bucket")].Properties).toMatchObject({
+            CorsConfiguration: {
+                CorsRules: [
+                    {
+                        AllowedOrigins: ["*"],
+                        AllowedMethods: ["GET", "PUT", "DELETE"],
+                        AllowedHeaders: ["*"],
+                    },
+                ],
+            },
+        });
+    });
+
+    it("should configure CORS with full rules when cors is an array", () => {
+        expect(cfTemplate.Resources[computeLogicalId("withCorsRules", "Bucket")].Properties).toMatchObject({
+            CorsConfiguration: {
+                CorsRules: [
+                    {
+                        AllowedOrigins: ["https://example.com"],
+                        AllowedMethods: ["PUT"],
+                        AllowedHeaders: ["*"],
+                    },
+                ],
+            },
+        });
+    });
+
     it("supports custom lifecycleRules with auto-capitalization and default Status", () => {
         const lifecycleConfig = cfTemplate.Resources[computeLogicalId("withLifecycleRules", "Bucket")].Properties
             .LifecycleConfiguration as { Rules: unknown[] };