From 06c093b3efc6d847fb13c8bde5943aeeea8029da Mon Sep 17 00:00:00 2001 From: AllanZhengYP Date: Tue, 16 Jul 2024 12:45:02 -0700 Subject: [PATCH] feat(storage): simplify the location cred provider option input (#13601) Remove the un-essetnial validation of per-API's location credentials provider input scope and permission for now. --- .../locationCredentialsStore/create.test.ts | 61 +---- .../validators.test.ts | 208 ------------------ .../storage/src/errors/types/validation.ts | 22 +- .../storage/src/providers/s3/types/options.ts | 17 +- .../locationCredentialsStore/create.ts | 63 +----- .../locationCredentialsStore/store.ts | 7 +- .../locationCredentialsStore/validators.ts | 86 -------- packages/storage/src/storageBrowser/types.ts | 10 +- 8 files changed, 21 insertions(+), 453 deletions(-) delete mode 100644 packages/storage/__tests__/storageBrowser/locationCredentialsStore/validators.test.ts delete mode 100644 packages/storage/src/storageBrowser/locationCredentialsStore/validators.ts diff --git a/packages/storage/__tests__/storageBrowser/locationCredentialsStore/create.test.ts b/packages/storage/__tests__/storageBrowser/locationCredentialsStore/create.test.ts index fa1ba483ada..6405cdb5487 100644 --- a/packages/storage/__tests__/storageBrowser/locationCredentialsStore/create.test.ts +++ b/packages/storage/__tests__/storageBrowser/locationCredentialsStore/create.test.ts @@ -8,7 +8,6 @@ import { getValue, removeStore, } from '../../../src/storageBrowser/locationCredentialsStore/registry'; -import { validateCredentialsProviderLocation } from '../../../src/storageBrowser/locationCredentialsStore/validators'; import { LocationCredentialsStore } from '../../../src/storageBrowser/types'; import { StorageValidationErrorCode, @@ -16,7 +15,6 @@ import { } from '../../../src/errors/types/validation'; jest.mock('../../../src/storageBrowser/locationCredentialsStore/registry'); -jest.mock('../../../src/storageBrowser/locationCredentialsStore/validators'); const mockedCredentials = 'MOCK_CREDS' as any as AWSCredentials; @@ -53,10 +51,7 @@ describe('createLocationCredentialsStore', () => { scope: 's3://bucket/path/*', permission: 'READ', }); - const { credentials } = await locationCredentialsProvider({ - locations: [{ bucket: 'bucket', path: 'path/to/object' }], - permission: 'READ', - }); + const { credentials } = await locationCredentialsProvider(); expect(credentials).toEqual(mockedCredentials); expect(getValue).toHaveBeenCalledWith( expect.objectContaining({ @@ -69,55 +64,6 @@ describe('createLocationCredentialsStore', () => { ); }); - it('should validate credentials location with resolved common scope', async () => { - expect.assertions(1); - jest - .mocked(getValue) - .mockResolvedValue({ credentials: mockedCredentials }); - const locationCredentialsProvider = store.getProvider({ - scope: 's3://bucket/path/*', - permission: 'READWRITE', - }); - await locationCredentialsProvider({ - locations: [ - { bucket: 'bucket', path: 'path/to/object' }, - { bucket: 'bucket', path: 'path/to/object2' }, - { bucket: 'bucket', path: 'path/folder' }, - ], - permission: 'READ', - }); - expect(validateCredentialsProviderLocation).toHaveBeenCalledWith( - { bucket: 'bucket', path: 'path/', permission: 'READ' }, - { bucket: 'bucket', path: 'path/*', permission: 'READWRITE' }, - ); - }); - - it('should throw if action requires cross-bucket permission', async () => { - expect.assertions(1); - jest - .mocked(getValue) - .mockResolvedValue({ credentials: mockedCredentials }); - const locationCredentialsProvider = store.getProvider({ - scope: 's3://bucket/path/*', - permission: 'READWRITE', - }); - try { - await locationCredentialsProvider({ - locations: [ - { bucket: 'bucket-1', path: 'path/to/object' }, - { bucket: 'bucket-2', path: 'path/to/object2' }, - ], - permission: 'READ', - }); - } catch (e: any) { - expect(e.message).toEqual( - validationErrorMap[ - StorageValidationErrorCode.LocationCredentialsCrossBucket - ].message, - ); - } - }); - it.each(['invalid-s3-uri', 's3://', 's3:///'])( 'should throw if location credentials provider scope is not a valid S3 URI "%s"', async invalidScope => { @@ -130,10 +76,7 @@ describe('createLocationCredentialsStore', () => { permission: 'READWRITE', }); try { - await locationCredentialsProvider({ - locations: [{ bucket: 'XXXXXXXX', path: 'path/to/object' }], - permission: 'READ', - }); + await locationCredentialsProvider(); } catch (e: any) { expect(e.message).toEqual( validationErrorMap[StorageValidationErrorCode.InvalidS3Uri] diff --git a/packages/storage/__tests__/storageBrowser/locationCredentialsStore/validators.test.ts b/packages/storage/__tests__/storageBrowser/locationCredentialsStore/validators.test.ts deleted file mode 100644 index 774b5663d34..00000000000 --- a/packages/storage/__tests__/storageBrowser/locationCredentialsStore/validators.test.ts +++ /dev/null @@ -1,208 +0,0 @@ -// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -import { validateCredentialsProviderLocation } from '../../../src/storageBrowser/locationCredentialsStore/validators'; -import { - StorageValidationErrorCode, - validationErrorMap, -} from '../../../src/errors/types/validation'; - -jest.mock('../../../src/storageBrowser/locationCredentialsStore/registry'); - -const mockBucket = 'MOCK_BUCKET'; - -describe('validateCredentialsProviderLocation', () => { - it('should NOT throw if action path matches credentials path prefix', () => { - expect(() => { - validateCredentialsProviderLocation( - { - bucket: mockBucket, - path: 'path/to/object', - permission: 'READ', - }, - { - bucket: mockBucket, - path: 'path/to/*', - permission: 'READ', - }, - ); - }).not.toThrow(); - }); - - it('should throw if action path does not path credentials path prefix', () => { - expect(() => { - validateCredentialsProviderLocation( - { - bucket: mockBucket, - path: 'path/to/object', - permission: 'READ', - }, - { - bucket: mockBucket, - path: 'path/to/other/*', - permission: 'READ', - }, - ); - }).toThrow( - validationErrorMap[ - StorageValidationErrorCode.LocationCredentialsPathMismatch - ].message, - ); - }); - - it('should NOT throw if action path matches credentials object path', () => { - expect(() => { - validateCredentialsProviderLocation( - { - bucket: mockBucket, - path: 'path/to/object', - permission: 'READ', - }, - { - bucket: mockBucket, - path: 'path/to/object', - permission: 'READ', - }, - ); - }).not.toThrow(); - }); - - it('should throw if action path does not match credentials object path', () => { - expect(() => { - validateCredentialsProviderLocation( - { - bucket: mockBucket, - path: 'path/to/object', - permission: 'READ', - }, - { - bucket: mockBucket, - path: 'path/to/object2', - permission: 'READ', - }, - ); - }).toThrow( - validationErrorMap[ - StorageValidationErrorCode.LocationCredentialsPathMismatch - ].message, - ); - }); - - it('should throw if action bucket and credentials bucket does not match', () => { - expect(() => { - validateCredentialsProviderLocation( - { - bucket: 'bucket-1', - path: 'path/to/object', - permission: 'READ', - }, - { - bucket: 'bucket-2', - path: 'path/to/object', - permission: 'READ', - }, - ); - }).toThrow( - validationErrorMap[ - StorageValidationErrorCode.LocationCredentialsBucketMismatch - ].message, - ); - }); - - it('should not throw if READ action permission matches READWRITE credentials permission', () => { - expect(() => { - validateCredentialsProviderLocation( - { - bucket: mockBucket, - path: 'path/to/object', - permission: 'READ', - }, - { - bucket: mockBucket, - path: 'path/to/*', - permission: 'READWRITE', - }, - ); - }).not.toThrow(); - }); - - it('should not throw if WRITE action permission matches READWRITE credentials permission', () => { - expect(() => { - validateCredentialsProviderLocation( - { - bucket: mockBucket, - path: 'path/to/object', - permission: 'WRITE', - }, - { - bucket: mockBucket, - path: 'path/to/*', - permission: 'READWRITE', - }, - ); - }).not.toThrow(); - }); - - it('should throw if READ action permission does not match WRITE credentials permission', () => { - expect(() => { - validateCredentialsProviderLocation( - { - bucket: mockBucket, - path: 'path/to/object', - permission: 'READ', - }, - { - bucket: mockBucket, - path: 'path/to/*', - permission: 'WRITE', - }, - ); - }).toThrow( - validationErrorMap[ - StorageValidationErrorCode.LocationCredentialsPermissionMismatch - ].message, - ); - }); - - it('should throw if WRITE action permission does not match READ credentials permission', () => { - expect(() => { - validateCredentialsProviderLocation( - { - bucket: mockBucket, - path: 'path/to/object', - permission: 'WRITE', - }, - { - bucket: mockBucket, - path: 'path/to/*', - permission: 'READ', - }, - ); - }).toThrow( - validationErrorMap[ - StorageValidationErrorCode.LocationCredentialsPermissionMismatch - ].message, - ); - }); - - it('should throw if READWRITE action permission does not match READ credentials permission', () => { - expect(() => { - validateCredentialsProviderLocation( - { - bucket: mockBucket, - path: 'path/to/object', - permission: 'READWRITE', - }, - { - bucket: mockBucket, - path: 'path/to/*', - permission: 'READ', - }, - ); - }).toThrow( - validationErrorMap[ - StorageValidationErrorCode.LocationCredentialsPermissionMismatch - ].message, - ); - }); -}); diff --git a/packages/storage/src/errors/types/validation.ts b/packages/storage/src/errors/types/validation.ts index 281272863de..deefdb1700b 100644 --- a/packages/storage/src/errors/types/validation.ts +++ b/packages/storage/src/errors/types/validation.ts @@ -21,17 +21,9 @@ export enum StorageValidationErrorCode { UrlExpirationMaxLimitExceed = 'UrlExpirationMaxLimitExceed', InvalidLocationCredentialsCacheSize = 'InvalidLocationCredentialsCacheSize', LocationCredentialsStoreDestroyed = 'LocationCredentialsStoreDestroyed', - LocationCredentialsBucketMismatch = 'LocationCredentialsBucketMismatch', - LocationCredentialsCrossBucket = 'LocationCredentialsCrossBucket', - LocationCredentialsPathMismatch = 'LocationCredentialsPathMismatch', - LocationCredentialsPermissionMismatch = 'LocationCredentialsPermissionMismatch', InvalidS3Uri = 'InvalidS3Uri', } -// Common error message strings to save some bytes -const LOCATION_SPECIFIC_CREDENTIALS = 'Location-specific credentials'; -const DOES_NOT_MATCH = 'does not match that required for the API call'; - export const validationErrorMap: AmplifyErrorMap = { [StorageValidationErrorCode.NoCredentials]: { message: 'Credentials should not be empty.', @@ -85,21 +77,9 @@ export const validationErrorMap: AmplifyErrorMap = { message: 'locationCredentialsCacheSize must be a positive integer.', }, [StorageValidationErrorCode.LocationCredentialsStoreDestroyed]: { - message: `${LOCATION_SPECIFIC_CREDENTIALS} store has been destroyed.`, + message: `Location-specific credentials store has been destroyed.`, }, [StorageValidationErrorCode.InvalidS3Uri]: { message: 'Invalid S3 URI.', }, - [StorageValidationErrorCode.LocationCredentialsCrossBucket]: { - message: `${LOCATION_SPECIFIC_CREDENTIALS} cannot be used across buckets.`, - }, - [StorageValidationErrorCode.LocationCredentialsBucketMismatch]: { - message: `${LOCATION_SPECIFIC_CREDENTIALS} bucket ${DOES_NOT_MATCH}.`, - }, - [StorageValidationErrorCode.LocationCredentialsPathMismatch]: { - message: `${LOCATION_SPECIFIC_CREDENTIALS} path ${DOES_NOT_MATCH}.`, - }, - [StorageValidationErrorCode.LocationCredentialsPermissionMismatch]: { - message: `${LOCATION_SPECIFIC_CREDENTIALS} permission ${DOES_NOT_MATCH}.`, - }, }; diff --git a/packages/storage/src/providers/s3/types/options.ts b/packages/storage/src/providers/s3/types/options.ts index 633366a4628..e37bcca8bbc 100644 --- a/packages/storage/src/providers/s3/types/options.ts +++ b/packages/storage/src/providers/s3/types/options.ts @@ -14,23 +14,8 @@ import { /** * @internal */ -export type Permission = 'READ' | 'READWRITE' | 'WRITE'; - -/** - * @internal - */ -export interface BucketLocation { - bucket: string; - path: string; -} - -/** - * @internal - */ -export type LocationCredentialsProvider = (options: { +export type LocationCredentialsProvider = (options?: { forceRefresh?: boolean; - locations: BucketLocation[]; - permission: Permission; }) => Promise<{ credentials: AWSCredentials }>; interface CommonOptions { diff --git a/packages/storage/src/storageBrowser/locationCredentialsStore/create.ts b/packages/storage/src/storageBrowser/locationCredentialsStore/create.ts index a54ae32a354..ce4e9126612 100644 --- a/packages/storage/src/storageBrowser/locationCredentialsStore/create.ts +++ b/packages/storage/src/storageBrowser/locationCredentialsStore/create.ts @@ -8,13 +8,9 @@ import { } from '../types'; import { StorageValidationErrorCode } from '../../errors/types/validation'; import { assertValidationError } from '../../errors/utils/assertValidationError'; -import { - BucketLocation, - LocationCredentialsProvider, -} from '../../providers/s3/types/options'; +import { LocationCredentialsProvider } from '../../providers/s3/types/options'; import { createStore, getValue, removeStore } from './registry'; -import { validateCredentialsProviderLocation } from './validators'; export const createLocationCredentialsStore = (input: { handler: GetLocationCredentials; @@ -24,23 +20,11 @@ export const createLocationCredentialsStore = (input: { const store = { getProvider(providerLocation: CredentialsLocation) { const locationCredentialsProvider = async ({ - permission, - locations, forceRefresh = false, - }: Parameters[0]) => { - const actionBucketLocation = resolveCommonBucketLocation(locations); - const providerBucketLocation = parseS3Uri(providerLocation.scope); - validateCredentialsProviderLocation( - { - ...actionBucketLocation, - permission, - }, - { - ...providerBucketLocation, - permission: providerLocation.permission, - }, - ); + }: Parameters[0] = {}) => { + validateS3Uri(providerLocation.scope); + // TODO(@AllanZhengYP): validate the action bucket and paths matches provider scope. return getValue({ storeSymbol, location: { ...providerLocation }, @@ -61,45 +45,10 @@ export const createLocationCredentialsStore = (input: { type S3Uri = string; -const parseS3Uri = (uri: S3Uri): BucketLocation => { - const s3UrlSchemaRegex = /^s3:\/\//; - // TODO(@AllanZhengYP): Provide more info to error message: url +const validateS3Uri = (uri: S3Uri): void => { + const s3UrlSchemaRegex = /^s3:\/\/[^/]+/; assertValidationError( s3UrlSchemaRegex.test(uri), StorageValidationErrorCode.InvalidS3Uri, ); - const [bucket, ...pathParts] = uri.replace(s3UrlSchemaRegex, '').split('/'); - assertValidationError(!!bucket, StorageValidationErrorCode.InvalidS3Uri); - const path = pathParts.join('/'); - - return { - bucket, - path, - }; -}; - -/** - * Given a list of bucket and path combinations, verify they have the same - * bucket and resolves the longest common prefix for multiple given paths. - */ -const resolveCommonBucketLocation = ( - locations: BucketLocation[], -): BucketLocation => { - let { bucket: commonBucket, path: commonPath } = locations[0]; - - for (const location of locations) { - const { bucket, path } = location; - assertValidationError( - bucket === commonBucket, - StorageValidationErrorCode.LocationCredentialsCrossBucket, - ); - while (commonPath !== '' && !path.startsWith(commonPath)) { - commonPath = commonPath.slice(0, -1); - } - } - - return { - bucket: commonBucket, - path: commonPath, - }; }; diff --git a/packages/storage/src/storageBrowser/locationCredentialsStore/store.ts b/packages/storage/src/storageBrowser/locationCredentialsStore/store.ts index 04e800365c7..63b88008b71 100644 --- a/packages/storage/src/storageBrowser/locationCredentialsStore/store.ts +++ b/packages/storage/src/storageBrowser/locationCredentialsStore/store.ts @@ -5,8 +5,11 @@ import { AWSCredentials } from '@aws-amplify/core/internals/utils'; -import { Permission } from '../../providers/s3/types/options'; -import { CredentialsLocation, GetLocationCredentials } from '../types'; +import { + CredentialsLocation, + GetLocationCredentials, + Permission, +} from '../types'; import { assertValidationError } from '../../errors/utils/assertValidationError'; import { StorageValidationErrorCode } from '../../errors/types/validation'; diff --git a/packages/storage/src/storageBrowser/locationCredentialsStore/validators.ts b/packages/storage/src/storageBrowser/locationCredentialsStore/validators.ts deleted file mode 100644 index 3409a899fdf..00000000000 --- a/packages/storage/src/storageBrowser/locationCredentialsStore/validators.ts +++ /dev/null @@ -1,86 +0,0 @@ -// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -import { StorageValidationErrorCode } from '../../errors/types/validation'; -import { assertValidationError } from '../../errors/utils/assertValidationError'; -import { BucketLocation, Permission } from '../../providers/s3/types/options'; - -interface CredentialsBucketLocation extends BucketLocation { - permission: Permission; -} - -/** - * @internal - */ -export const validateCredentialsProviderLocation = ( - actionLocation: CredentialsBucketLocation, - providerLocation: CredentialsBucketLocation, -): void => { - validateLocationBucket({ - actionBucket: actionLocation.bucket, - providerBucket: providerLocation.bucket, - }); - validateLocationPath({ - actionPath: actionLocation.path, - providerPath: providerLocation.path, - }); - validateLocationPermission({ - actionPermission: actionLocation.permission, - providerPermission: providerLocation.permission, - }); -}; - -const validateLocationBucket = (input: { - actionBucket: string; - providerBucket?: string; -}): void => { - const { actionBucket, providerBucket } = input; - if (!providerBucket) { - return; - } - assertValidationError( - actionBucket === providerBucket, - StorageValidationErrorCode.LocationCredentialsBucketMismatch, - ); -}; - -const validateLocationPath = (input: { - actionPath: string; - providerPath?: string; -}): void => { - const { actionPath, providerPath } = input; - if (!providerPath) { - return; - } - if (providerPath.endsWith('*')) { - // Verify if the action path has prefix required by the provider; - const providerPathPrefix = providerPath.replace(/\*$/, ''); - assertValidationError( - actionPath.startsWith(providerPathPrefix), - StorageValidationErrorCode.LocationCredentialsPathMismatch, - ); - } else { - // If provider path is scoped to an object, verify if the action path points to the same object. - // TODO(@AllanZhengYP) Provider more info in error message: actionPath, providerPath. - assertValidationError( - actionPath === providerPath, - StorageValidationErrorCode.LocationCredentialsPathMismatch, - ); - } -}; - -const validateLocationPermission = (input: { - actionPermission: Permission; - providerPermission?: Permission; -}) => { - const { actionPermission, providerPermission } = input; - if (!providerPermission) { - return; - } - // TODO(@AllanZhengYP) Provide more info in error message: `API needs permission ${actionPermission}, but provided - // location credentials provider with permission ${providerPermission}.` - assertValidationError( - providerPermission.includes(actionPermission), - StorageValidationErrorCode.LocationCredentialsPermissionMismatch, - ); -}; diff --git a/packages/storage/src/storageBrowser/types.ts b/packages/storage/src/storageBrowser/types.ts index 3f184dcc313..68fe71aadaf 100644 --- a/packages/storage/src/storageBrowser/types.ts +++ b/packages/storage/src/storageBrowser/types.ts @@ -3,10 +3,12 @@ import { AWSCredentials } from '@aws-amplify/core/internals/utils'; -import { - LocationCredentialsProvider, - Permission, -} from '../providers/s3/types/options'; +import { LocationCredentialsProvider } from '../providers/s3/types/options'; + +/** + * @internal + */ +export type Permission = 'READ' | 'READWRITE' | 'WRITE'; /** * @internal