Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add storage access rules to outputs #1927

Merged
merged 15 commits into from
Sep 26, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions packages/backend-output-schemas/src/storage/v1.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,6 @@
import { z } from 'zod';

const storageAccessActionEnum = z.enum([
'read',
'get',
'list',
'write',
'delete',
]);
const storageAccessActionEnum = z.enum(['get', 'list', 'write', 'delete']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


const pathSchema = z.record(
z.string(),
Expand Down
3 changes: 2 additions & 1 deletion packages/backend-storage/src/access_builder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
ResourceProvider,
} from '@aws-amplify/plugin-types';
import { StorageAccessBuilder } from './types.js';
import { entityIdSubstitution } from './constants.js';

export const roleAccessBuilder: StorageAccessBuilder = {
authenticated: {
Expand Down Expand Up @@ -69,7 +70,7 @@ export const roleAccessBuilder: StorageAccessBuilder = {
},
],
actions,
idSubstitution: '${cognito-identity.amazonaws.com:sub}',
idSubstitution: entityIdSubstitution,
}),
}),
resource: (other) => ({
Expand Down
1 change: 1 addition & 0 deletions packages/backend-storage/src/constants.ts
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
export const entityIdPathToken = '{entity_id}';
export const entityIdSubstitution = '${cognito-identity.amazonaws.com:sub}';
2 changes: 1 addition & 1 deletion packages/backend-storage/src/private_types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,5 @@ export type InternalStorageAction = Exclude<StorageAction, 'read'>;
/**
* Storage access types intended to be used to map storage access to storage outputs
*/
export type StorageAccessConfig = Record<string, StorageAction[]>;
export type StorageAccessConfig = Record<string, InternalStorageAction[]>;
export type StorageAccessDefinitionOutput = Record<string, StorageAccessConfig>;
42 changes: 21 additions & 21 deletions packages/backend-storage/src/storage_access_orchestrator.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { ConstructFactoryGetInstanceProps } from '@aws-amplify/plugin-types';
import { App, Stack } from 'aws-cdk-lib';
import { Bucket } from 'aws-cdk-lib/aws-s3';
import assert from 'node:assert';
import { entityIdPathToken } from './constants.js';
import { entityIdPathToken, entityIdSubstitution } from './constants.js';
import { StorageAccessPolicyFactory } from './storage_access_policy_factory.js';
import { StorageAccessDefinition } from './types.js';

Expand Down Expand Up @@ -299,7 +299,7 @@ void describe('StorageAccessOrchestrator', () => {
acceptResourceAccess: acceptResourceAccessMock,
}),
],
idSubstitution: '{testOwnerSub}',
idSubstitution: entityIdSubstitution,
uniqueDefinitionIdValidations:
accessDefinitionTestDefaults('acceptor')
.uniqueDefinitionIdValidations,
Expand All @@ -321,12 +321,12 @@ void describe('StorageAccessOrchestrator', () => {
{
Action: 's3:GetObject',
Effect: 'Allow',
Resource: `${bucket.bucketArn}/test/{testOwnerSub}/*`,
Resource: `${bucket.bucketArn}/test/${entityIdSubstitution}/*`,
},
{
Action: 's3:PutObject',
Effect: 'Allow',
Resource: `${bucket.bucketArn}/test/{testOwnerSub}/*`,
Resource: `${bucket.bucketArn}/test/${entityIdSubstitution}/*`,
},
],
Version: '2012-10-17',
Expand All @@ -337,7 +337,7 @@ void describe('StorageAccessOrchestrator', () => {
ssmEnvironmentEntriesStub
);
assert.deepStrictEqual(storageAccessDefinitionOutput, {
'test/{testOwnerSub}/*': {
[`test/${entityIdSubstitution}/*`]: {
acceptor: ['get', 'write'],
},
});
Expand Down Expand Up @@ -451,7 +451,7 @@ void describe('StorageAccessOrchestrator', () => {
{
actions: ['write', 'delete'],
getResourceAccessAcceptors: [authenticatedResourceAccessAcceptor],
idSubstitution: '{idSub}',
idSubstitution: entityIdSubstitution,
uniqueDefinitionIdValidations:
accessDefinitionTestDefaults('auth-with-id')
.uniqueDefinitionIdValidations,
Expand All @@ -478,17 +478,17 @@ void describe('StorageAccessOrchestrator', () => {
{
Action: 's3:PutObject',
Effect: 'Allow',
Resource: `${bucket.bucketArn}/foo/{idSub}/*`,
Resource: `${bucket.bucketArn}/foo/${entityIdSubstitution}/*`,
},
{
Action: 's3:DeleteObject',
Effect: 'Allow',
Resource: `${bucket.bucketArn}/foo/{idSub}/*`,
Resource: `${bucket.bucketArn}/foo/${entityIdSubstitution}/*`,
},
{
Action: 's3:GetObject',
Effect: 'Allow',
Resource: `${bucket.bucketArn}/foo/*/*`,
Resource: `${bucket.bucketArn}/foo/*`,
},
],
Version: '2012-10-17',
Expand All @@ -499,11 +499,11 @@ void describe('StorageAccessOrchestrator', () => {
ssmEnvironmentEntriesStub
);
assert.deepStrictEqual(storageAccessDefinitionOutput, {
'foo/*/*': {
'foo/*': {
'auth-with-id': ['write', 'delete'],
auth: ['get'],
},
'foo/{idSub}/*': {
[`foo/${entityIdSubstitution}/*`]: {
'auth-with-id': ['write', 'delete'],
},
});
Expand Down Expand Up @@ -538,7 +538,7 @@ void describe('StorageAccessOrchestrator', () => {
{
actions: ['get'],
getResourceAccessAcceptors: [getResourceAccessAcceptorStub2],
idSubstitution: '{idSub}',
idSubstitution: entityIdSubstitution,
uniqueDefinitionIdValidations:
accessDefinitionTestDefaults('stub2')
.uniqueDefinitionIdValidations,
Expand All @@ -559,7 +559,7 @@ void describe('StorageAccessOrchestrator', () => {
{
actions: ['get', 'write', 'delete'],
getResourceAccessAcceptors: [getResourceAccessAcceptorStub2],
idSubstitution: '{idSub}',
idSubstitution: entityIdSubstitution,
uniqueDefinitionIdValidations:
accessDefinitionTestDefaults('stub2')
.uniqueDefinitionIdValidations,
Expand Down Expand Up @@ -588,7 +588,7 @@ void describe('StorageAccessOrchestrator', () => {
Effect: 'Allow',
Resource: [
`${bucket.bucketArn}/foo/*`,
`${bucket.bucketArn}/other/*/*`,
`${bucket.bucketArn}/other/*`,
],
},
{
Expand Down Expand Up @@ -628,18 +628,18 @@ void describe('StorageAccessOrchestrator', () => {
Effect: 'Allow',
Resource: [
`${bucket.bucketArn}/foo/bar/*`,
`${bucket.bucketArn}/other/{idSub}/*`,
`${bucket.bucketArn}/other/${entityIdSubstitution}/*`,
],
},
{
Action: 's3:PutObject',
Effect: 'Allow',
Resource: `${bucket.bucketArn}/other/{idSub}/*`,
Resource: `${bucket.bucketArn}/other/${entityIdSubstitution}/*`,
},
{
Action: 's3:DeleteObject',
Effect: 'Allow',
Resource: `${bucket.bucketArn}/other/{idSub}/*`,
Resource: `${bucket.bucketArn}/other/${entityIdSubstitution}/*`,
},
],
Version: '2012-10-17',
Expand All @@ -655,11 +655,11 @@ void describe('StorageAccessOrchestrator', () => {
'foo/baz/*': {
stub1: ['get'],
},
'other/*/*': {
'other/*': {
stub1: ['get'],
stub2: ['get', 'write', 'delete'],
},
'other/{idSub}/*': {
[`other/${entityIdSubstitution}/*`]: {
stub2: ['get', 'write', 'delete'],
},
});
Expand Down Expand Up @@ -767,10 +767,10 @@ void describe('StorageAccessOrchestrator', () => {
);
assert.deepStrictEqual(storageAccessDefinitionOutput, {
'foo/bar/*': {
auth: ['read', 'get', 'list'],
auth: ['get', 'list'],
},
'other/baz/*': {
auth: ['read'],
auth: ['get', 'list'],
},
});
});
Expand Down
56 changes: 40 additions & 16 deletions packages/backend-storage/src/storage_access_orchestrator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
StorageAccessGenerator,
StoragePath,
} from './types.js';
import { entityIdPathToken } from './constants.js';
import { entityIdPathToken, entityIdSubstitution } from './constants.js';
import { StorageAccessPolicyFactory } from './storage_access_policy_factory.js';
import { validateStorageAccessPaths as _validateStorageAccessPaths } from './validate_storage_access_paths.js';
import { roleAccessBuilder as _roleAccessBuilder } from './access_builder.js';
Expand Down Expand Up @@ -102,6 +102,16 @@ export class StorageAccessOrchestrator {
// iterate over all of the access definitions for a given prefix
const accessConfig: StorageAccessConfig = {};
accessPermissions.forEach((permission) => {
// replace "read" with "get" and "list" in actions
const replaceReadWithGetAndList = permission.actions.flatMap(
(action) => (action === 'read' ? ['get', 'list'] : [action])
) as InternalStorageAction[];

// ensure the actions list has no duplicates
const noDuplicateActions = Array.from(
new Set(replaceReadWithGetAndList)
);

// iterate over all uniqueDefinitionIdValidations and ensure uniqueness within this path prefix
permission.uniqueDefinitionIdValidations.forEach(
({ uniqueDefinitionId, validationErrorOptions }) => {
Expand All @@ -114,30 +124,20 @@ export class StorageAccessOrchestrator {
uniqueDefinitionIdSet.add(uniqueDefinitionId);
}

accessConfig[uniqueDefinitionId] = permission.actions;
accessConfig[uniqueDefinitionId] = noDuplicateActions;
}
);
// make the owner placeholder substitution in the s3 prefix
const prefix = s3Prefix.replaceAll(
entityIdPathToken,
const prefix = placeholderSubstitution(
s3Prefix,
permission.idSubstitution
) as StoragePath;
);

storageOutputAccessDefinition[prefix] = {
...storageOutputAccessDefinition[prefix],
...accessConfig,
};

// replace "read" with "get" and "list" in actions
const replaceReadWithGetAndList = permission.actions.flatMap(
(action) => (action === 'read' ? ['get', 'list'] : [action])
) as InternalStorageAction[];

// ensure the actions list has no duplicates
const noDuplicateActions = Array.from(
new Set(replaceReadWithGetAndList)
);

// set an entry that maps this permission to each resource acceptor
permission.getResourceAccessAcceptors.forEach(
(getResourceAccessAcceptor) => {
Expand Down Expand Up @@ -212,7 +212,11 @@ export class StorageAccessOrchestrator {
const allPaths = Array.from(this.prefixDenyMap.keys());
allPaths.forEach((storagePath) => {
const parent = findParent(storagePath, allPaths);
if (!parent) {
// do not add to prefix deny map if there is no parent or the path is a subpath with entity id
if (
!parent ||
parent === storagePath.replaceAll(`${entityIdSubstitution}/`, '')
) {
return;
}
// if a parent path is defined, invoke the denyByDefault callback on this subpath for all policies that exist on the parent path
Expand Down Expand Up @@ -275,6 +279,26 @@ export class StorageAccessOrchestratorFactory {
);
}

/**
* Performs the owner placeholder substitution in the s3 prefix
*/
const placeholderSubstitution = (
s3Prefix: string,
idSubstitution: string
): StoragePath => {
const prefix = s3Prefix.replaceAll(
entityIdPathToken,
idSubstitution
) as StoragePath;

// for auth/unauth roles where prefix ends with '/*/*' remove the last wildcard
if (prefix.endsWith('/*/*')) {
return prefix.slice(0, -2) as StoragePath;
}

return prefix as StoragePath;
};

/**
* Returns the element in paths that is a prefix of path, if any
* Note that there can only be one at this point because of upstream validation
Expand Down
16 changes: 16 additions & 0 deletions packages/backend-storage/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,17 +41,29 @@ export type StorageAccessBuilder = {
/**
* Configure storage access for authenticated users. Requires `defineAuth` in the backend definition.
* @see https://docs.amplify.aws/gen2/build-a-backend/storage/#authenticated-user-access
*
* When configuring access for paths with the `{entity_id}` token, the token is replaced with a wildcard (`*`).
* For a path like `media/profile-pictures/{entity_id}/*`, this means access is configured for authenticated users for any file within
* `media/profile-pictures/*`.
*/
authenticated: StorageActionBuilder;
/**
* Configure storage access for guest (unauthenticated) users. Requires `defineAuth` in the backend definition.
* @see https://docs.amplify.aws/gen2/build-a-backend/storage/#guest-user-access
*
* When configuring access for paths with the `{entity_id}` token, the token is replaced with a wildcard (`*`).
* For a path like `media/profile-pictures/{entity_id}/*`, this means access is configured for guest users for any file within
* `media/profile-pictures/*`.
*/
guest: StorageActionBuilder;
/**
* Configure storage access for User Pool groups. Requires `defineAuth` with groups config in the backend definition.
* @see https://docs.amplify.aws/gen2/build-a-backend/storage/#user-group-access
* @param groupName The User Pool group name to configure access for
*
* When configuring access for paths with the `{entity_id}` token, the token is replaced with a wildcard (`*`).
* For a path like `media/profile-pictures/{entity_id}/*`, this means access is configured for that specific group for any file within
* `media/profile-pictures/*`.
*/
groups: (groupNames: string[]) => StorageActionBuilder;
/**
Expand All @@ -64,6 +76,10 @@ export type StorageAccessBuilder = {
* Grant other resources in the Amplify backend access to storage.
* @see https://docs.amplify.aws/gen2/build-a-backend/storage/#grant-function-access
* @param other The target resource to grant access to. Currently only the return value of `defineFunction` is supported.
*
* When configuring access for paths with the `{entity_id}` token, the token is replaced with a wildcard (`*`).
* For a path like `media/profile-pictures/{entity_id}/*`, this means access is configured for resources for any file within
* `media/profile-pictures/*`.
*/
resource: (
other: ConstructFactory<ResourceProvider & ResourceAccessAcceptorFactory>
Expand Down
2 changes: 1 addition & 1 deletion packages/client-config/API.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ type AmazonPinpointChannels_2 = 'IN_APP_MESSAGING' | 'FCM' | 'APNS' | 'EMAIL' |
type AmazonPinpointChannels_3 = 'IN_APP_MESSAGING' | 'FCM' | 'APNS' | 'EMAIL' | 'SMS';

// @public (undocumented)
type AmplifyStorageAccessActions = 'read' | 'get' | 'list' | 'write' | 'delete';
type AmplifyStorageAccessActions = 'get' | 'list' | 'write' | 'delete';

// @public
interface AmplifyStorageAccessRule {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -542,7 +542,7 @@ void describe('storage client config contributor v1', () => {
storageRegion: 'testRegion',
paths: {
'path/*': {
guest: ['read'],
guest: ['get'],
},
},
}),
Expand All @@ -569,7 +569,7 @@ void describe('storage client config contributor v1', () => {
aws_region: 'testRegion',
paths: {
'path/*': {
guest: ['read'],
guest: ['get'],
},
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,7 @@ export type AmazonPinpointChannels =
| 'APNS'
| 'EMAIL'
| 'SMS';
export type AmplifyStorageAccessActions =
| 'read'
| 'get'
| 'list'
| 'write'
| 'delete';
export type AmplifyStorageAccessActions = 'get' | 'list' | 'write' | 'delete';

/**
* Config format for Amplify Gen 2 client libraries to communicate with backend services.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,7 @@
"$defs": {
"amplify_storage_access_actions": {
"type": "string",
"enum": ["read", "get", "list", "write", "delete"]
"enum": ["get", "list", "write", "delete"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
"amplify_storage_access_rule": {
"type": "object",
Expand Down
Loading