add storage access rules to outputs #1927

rtpascual · 2024-08-27T23:31:19Z

Problem

Storage access rules defined in defineStorage are not in amplify_outputs.json.

Issue number, if available:

Changes

Adds storage access rules to amplify_outputs.json. Considering the following backend code:

// amplify/storage/resource.ts

export const storage = defineStorage({
  name: 'myBucket',
  access: (allow) => ({
    'media/*': [allow.authenticated.to(['read', 'write', 'delete'])],
    'other/*': [
      allow.guest.to(['read']),
      allow.authenticated.to(['read', 'write'])
    ]
  })
});

The outputs for storage will be (note: read is replaced with get and list):

"storage": {
  "aws_region": "us-east-1",
  "bucket_name": "amplify-myapp-mybucket",
  "buckets": [
    {
      "name": "myBucket",
      "bucket_name": "amplify-myapp-mybucket",
      "aws_region": "us-east-1",
      "paths": {
        "media/*": {
          "authenticated": ["get", "list", "write", "delete"]
        },
        "other/*": {
          "guest": ["get", "list"],
          "authenticated": ["get", "list", "write"]
        }
      }
    }
  ]
}

Corresponding docs PR, if applicable:

Validation

E2E and unit tests

Checklist

If this PR includes a functional change to the runtime behavior of the code, I have added or updated automated test coverage for this change.
If this PR requires a change to the Project Architecture README, I have included that update in this PR.
If this PR requires a docs update, I have linked to that docs PR above.
If this PR modifies E2E tests, makes changes to resource provisioning, or makes SDK calls, I have run the PR checks with the run-e2e label set.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

changeset-bot · 2024-08-27T23:31:23Z

🦋 Changeset detected

Latest commit: 2b23793

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages

Name	Type
@aws-amplify/client-config	Minor
@aws-amplify/backend-output-schemas	Patch
@aws-amplify/integration-tests	Patch
@aws-amplify/backend-storage	Patch
@aws-amplify/backend	Patch
@aws-amplify/backend-cli	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

ashika112

Overall change looks good to me.

Question on npx ampx generate outputs, will customer be able to specify and generate version like 1, 1.1, 1.2?

rtpascual · 2024-09-04T20:12:54Z

Question on npx ampx generate outputs, will customer be able to specify and generate version like 1, 1.1, 1.2?

Yes they can specify using the --outputs-version option

AllanZhengYP

AllanZhengYP · 2024-09-06T07:29:43Z

The DX from the PR description makes sense. The JS library can resolve which of the authenticated, guest, owner, group the caller belongs to, but I'm not sure about the behavior of Functions. How does the client side know whether the client has the permission based on the Functions? I'm referring to this doc: https://docs.amplify.aws/flutter/build-a-backend/storage/authorization/#access-types

rtpascual · 2024-09-06T18:25:00Z

I notice the access can be 1 of enum value from 2 sets: read | write | delete or get | list | write | delete. Is it possible the output file only use the later one? It would make client side logic a lot easier.

Yes we can make this change

rtpascual · 2024-09-06T18:38:43Z

The DX from the PR description makes sense. The JS library can resolve which of the authenticated, guest, owner, group the caller belongs to, but I'm not sure about the behavior of Functions. How does the client side know whether the client has the permission based on the Functions? I'm referring to this doc: https://docs.amplify.aws/flutter/build-a-backend/storage/authorization/#access-types

Functions specifically have separate logic through environment variables that reference the resource they are given access to to avoid circular dependencies. For more information see https://docs.amplify.aws/flutter/build-a-backend/functions/grant-access-to-other-resources/.

…nd list

sobolk · 2024-09-23T17:03:39Z

packages/backend-output-schemas/src/storage/v1.ts

-  'write',
-  'delete',
-]);
+const storageAccessActionEnum = z.enum(['get', 'list', 'write', 'delete']);


wouldn't this be a breaking change for deployed apps ?

You're right, I forgot to think about impact on existing apps, adding this back in.

sobolk · 2024-09-23T17:05:48Z

packages/client-config/src/client-config-schema/schema_v1.2.json

@@ -356,7 +356,7 @@
  "$defs": {
    "amplify_storage_access_actions": {
      "type": "string",
-      "enum": ["read", "get", "list", "write", "delete"]
+      "enum": ["get", "list", "write", "delete"]


we may deprecate and not use read but we should keep it.

Amplifiyer · 2024-09-23T17:25:34Z

packages/client-config/src/client-config-contributor/client_config_contributor_v1.ts

 */
+// eslint-disable-next-line @typescript-eslint/naming-convention


This should not be needed here.

Amplifiyer · 2024-09-23T17:28:50Z

packages/client-config/src/client-config-schema/client_config_v1.2.ts

+  /**
+   * JSON schema
+   */
+  $schema?: string;


we typically remove this from the code-gen

Amplifiyer · 2024-09-23T17:31:34Z

packages/client-config/src/client-config-schema/client_config_v1.2.ts

+export interface AmazonLocationServiceConfig {
+  /**
+   * Map style
+   */
+  style?: string;
+}


It seems the name property is missing from this type?

I might have misconfigured or am using a different version of json-schema-to-typescript. The closest I would get to the existing client config was running:
json2ts packages/client-config/src/client-config-schema/schema_v1.2.json -o packages/client-config/src/client-config-schema/client_config_v1.2.ts --style.singleQuote --ignoreMinAndMaxItems

I'll update this manually.

Amplifiyer · 2024-09-23T17:40:16Z

packages/backend-storage/src/storage_outputs_aspect.ts

@@ -104,6 +103,7 @@ export class StorageOutputsAspect implements IAspect {
          name: node.name,
          bucketName: node.resources.bucket.bucketName,
          storageRegion: Stack.of(node).region,
+          paths: node.accessDefinition,


We didn't need to make changes here

amplify-backend/packages/backend-output-schemas/src/storage/v1.ts

Lines 3 to 7 in c99636f

const bucketSchema = z.object({

name: z.string(),

bucketName: z.string(),

storageRegion: z.string(),

});

?

We might also be missing some automated testing if we didn't catch this failure, or Zod is not strict here?

This is needed to add paths to the amplify_outputs.json file. paths is optional so storage with no access rules defined will not fail and not have paths in their amplify_outputs.json.

Shouldn't the paths be added as optional since we are adding that in the bucketSchema object before stringifying.

Got it, and yup I believe Zod is not strict enough here, I'll add a test to make sure we cover for both cases of storage with access and without.

Amplifiyer · 2024-09-23T17:44:20Z

packages/client-config/src/client-config-contributor/client_config_contributor_v1.test.ts

+        paths: {
+          'path/*': {
+            guest: ['get'],
+          },
+        },


Can you add all five types of access rules we support here.

For groups, entity and resource, do we print the actual substituted value (for the group, entity and resource) or just the key name groups, entity etc.

Will add all the types of access rules.

For group and entity, we append the group name or entityId to the key name.

For resource since we use environment variables they go through different logic since we don't create storage policies.

Is this updated? I was thinking this is where we should update https://github.com/aws-amplify/amplify-backend/pull/1927/files#diff-cc440dd1098daee47d41294de3ae883a0c8e2570e516061b54623fa84f7ff8edR136-R139

I added all the access rules but we won't see much benefit from adding more to this test since we are just mapping paths from the payload to the config. For key names, this is handled in access_builder.test.ts and tested there and for verifying the paths is built correctly that is done when we assert storageAccessDefinitionOutput in storage_access_orchestrator.test.ts.

ashika112

Approving from JS client side. The outputs generated works as expected without breaking our existing outputs parsing logic.

add storage access rules to outputs

5cf3823

fix tests

1284ce1

rtpascual added the run-e2e Label that will include e2e tests in PR checks workflow label Aug 28, 2024

Merge branch 'main' into storage-access-outputs

6ef5567

rtpascual marked this pull request as ready for review August 28, 2024 17:15

rtpascual requested review from a team as code owners August 28, 2024 17:15

sobolk previously approved these changes Aug 28, 2024

View reviewed changes

AllanZhengYP requested review from AllanZhengYP and ashika112 September 4, 2024 18:49

ashika112 previously approved these changes Sep 4, 2024

View reviewed changes

AllanZhengYP reviewed Sep 6, 2024

View reviewed changes

rtpascual added 2 commits September 6, 2024 12:02

Merge branch 'main' into storage-access-outputs

dfcfde5

remove read action from storage access rule outputs in favor of get a…

98662a8

…nd list

rtpascual dismissed stale reviews from ashika112 and sobolk via 98662a8 September 6, 2024 19:12

rtpascual added 7 commits September 16, 2024 11:36

Merge branch 'main' into storage-access-outputs

54da34d

update api.md

dddd410

Merge branch 'main' into storage-access-outputs

eedd44b

update permissions for non entity roles for entity_id paths

3b77458

Merge branch 'main' into storage-access-outputs

0e9497d

add entity id substitution constant

3471c97

add comments for new behavior with owner paths

c426a0d

sobolk reviewed Sep 23, 2024

View reviewed changes

add back read

12db2ca

sobolk previously approved these changes Sep 23, 2024

View reviewed changes

Amplifiyer reviewed Sep 23, 2024

View reviewed changes

pr feedback

69dbc7b

rtpascual dismissed sobolk’s stale review via 69dbc7b September 23, 2024 23:43

update api.md

2b23793

sobolk approved these changes Sep 24, 2024

View reviewed changes

ashika112 approved these changes Sep 26, 2024

View reviewed changes

Amplifiyer approved these changes Sep 26, 2024

View reviewed changes

rtpascual merged commit d538ecc into main Sep 26, 2024
60 of 62 checks passed

rtpascual deleted the storage-access-outputs branch September 26, 2024 20:06

rtpascual mentioned this pull request Sep 26, 2024

update e2e test for storage access outputs #2063

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add storage access rules to outputs #1927

add storage access rules to outputs #1927

rtpascual commented Aug 27, 2024 •

edited

Loading

changeset-bot bot commented Aug 27, 2024 •

edited

Loading

ashika112 left a comment

rtpascual commented Sep 4, 2024

AllanZhengYP left a comment •

edited

Loading

AllanZhengYP commented Sep 6, 2024

rtpascual commented Sep 6, 2024

rtpascual commented Sep 6, 2024

sobolk Sep 23, 2024

rtpascual Sep 23, 2024

sobolk Sep 23, 2024

Amplifiyer Sep 23, 2024

Amplifiyer Sep 23, 2024

Amplifiyer Sep 23, 2024

rtpascual Sep 23, 2024

Amplifiyer Sep 23, 2024

rtpascual Sep 23, 2024

Amplifiyer Sep 23, 2024

rtpascual Sep 23, 2024 •

edited

Loading

Amplifiyer Sep 23, 2024

rtpascual Sep 23, 2024 •

edited

Loading

Amplifiyer Sep 24, 2024

rtpascual Sep 24, 2024

ashika112 left a comment

		*/
		// eslint-disable-next-line @typescript-eslint/naming-convention

	const bucketSchema = z.object({
	name: z.string(),
	bucketName: z.string(),
	storageRegion: z.string(),
	});

add storage access rules to outputs #1927

add storage access rules to outputs #1927

Conversation

rtpascual commented Aug 27, 2024 • edited Loading

Problem

Changes

Validation

Checklist

changeset-bot bot commented Aug 27, 2024 • edited Loading

🦋 Changeset detected

ashika112 left a comment

Choose a reason for hiding this comment

rtpascual commented Sep 4, 2024

AllanZhengYP left a comment • edited Loading

Choose a reason for hiding this comment

AllanZhengYP commented Sep 6, 2024

rtpascual commented Sep 6, 2024

rtpascual commented Sep 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtpascual Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtpascual Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashika112 left a comment

Choose a reason for hiding this comment

rtpascual commented Aug 27, 2024 •

edited

Loading

changeset-bot bot commented Aug 27, 2024 •

edited

Loading

AllanZhengYP left a comment •

edited

Loading

rtpascual Sep 23, 2024 •

edited

Loading

rtpascual Sep 23, 2024 •

edited

Loading