From 2abb1367c5359502175031a63d04a57d784d7f1a Mon Sep 17 00:00:00 2001 From: AllanZhengYP Date: Fri, 15 Sep 2023 10:39:24 -0700 Subject: [PATCH] feat: disallow targetIdentityId option in all S3 write APIs and category API (#12034) --------- Co-authored-by: Jim Blanchard --- .../__tests__/providers/s3/apis/copy.test.ts | 12 +- .../providers/s3/apis/downloadData.test.ts | 6 +- .../providers/s3/apis/getProperties.test.ts | 4 +- .../providers/s3/apis/getUrl.test.ts | 6 +- .../__tests__/providers/s3/apis/list.test.ts | 16 ++- .../storage/src/providers/s3/types/index.ts | 2 + .../storage/src/providers/s3/types/inputs.ts | 8 +- .../storage/src/providers/s3/types/options.ts | 127 ++++++++++-------- packages/storage/src/types/index.ts | 2 - packages/storage/src/types/inputs.ts | 17 ++- packages/storage/src/types/options.ts | 20 +-- 11 files changed, 121 insertions(+), 99 deletions(-) diff --git a/packages/storage/__tests__/providers/s3/apis/copy.test.ts b/packages/storage/__tests__/providers/s3/apis/copy.test.ts index 41f40ac660a..dc33c10276c 100644 --- a/packages/storage/__tests__/providers/s3/apis/copy.test.ts +++ b/packages/storage/__tests__/providers/s3/apis/copy.test.ts @@ -2,13 +2,13 @@ // SPDX-License-Identifier: Apache-2.0 import { Credentials } from '@aws-sdk/types'; -import { Amplify, StorageAccessLevel } from '@aws-amplify/core'; +import { Amplify } from '@aws-amplify/core'; import { copyObject } from '../../../../src/providers/s3/utils/client'; import { copy } from '../../../../src/providers/s3/apis'; import { - StorageCopySourceOptions, - StorageCopyDestinationOptions, -} from '../../../../src/types'; + CopySourceOptions, + CopyDestinationOptions, +} from '../../../../src/providers/s3/types'; jest.mock('../../../../src/providers/s3/utils/client'); jest.mock('@aws-amplify/core', () => ({ @@ -153,11 +153,11 @@ describe('copy API', () => { expect( await copy({ source: { - ...(source as StorageCopySourceOptions), + ...(source as CopySourceOptions), key: sourceKey, }, destination: { - ...(destination as StorageCopyDestinationOptions), + ...(destination as CopyDestinationOptions), key: destinationKey, }, }) diff --git a/packages/storage/__tests__/providers/s3/apis/downloadData.test.ts b/packages/storage/__tests__/providers/s3/apis/downloadData.test.ts index a4b095a6d90..e107f05e6ad 100644 --- a/packages/storage/__tests__/providers/s3/apis/downloadData.test.ts +++ b/packages/storage/__tests__/providers/s3/apis/downloadData.test.ts @@ -6,7 +6,7 @@ import { Amplify } from '@aws-amplify/core'; import { getObject } from '../../../../src/providers/s3/utils/client'; import { downloadData } from '../../../../src/providers/s3'; import { createDownloadTask } from '../../../../src/providers/s3/utils'; -import { StorageOptions } from '../../../../src/types'; +import { DownloadDataOptions } from '../../../../src/providers/s3/types'; jest.mock('../../../../src/providers/s3/utils/client'); jest.mock('../../../../src/providers/s3/utils'); @@ -93,10 +93,10 @@ describe('downloadData', () => { downloadData({ key, options: { - ...(options as StorageOptions), + ...options, useAccelerateEndpoint: true, onProgress, - }, + } as DownloadDataOptions, }); const job = mockCreateDownloadTask.mock.calls[0][0].job; await job(); diff --git a/packages/storage/__tests__/providers/s3/apis/getProperties.test.ts b/packages/storage/__tests__/providers/s3/apis/getProperties.test.ts index 60765c2a95f..946eac7c245 100644 --- a/packages/storage/__tests__/providers/s3/apis/getProperties.test.ts +++ b/packages/storage/__tests__/providers/s3/apis/getProperties.test.ts @@ -5,7 +5,7 @@ import { headObject } from '../../../../src/providers/s3/utils/client'; import { getProperties } from '../../../../src/providers/s3'; import { Credentials } from '@aws-sdk/types'; import { Amplify } from '@aws-amplify/core'; -import { StorageOptions } from '../../../../src/types'; +import { GetPropertiesOptions } from '../../../../src/providers/s3/types'; jest.mock('../../../../src/providers/s3/utils/client'); jest.mock('@aws-amplify/core', () => ({ @@ -107,7 +107,7 @@ describe('getProperties api', () => { expect( await getProperties({ key, - options: options as StorageOptions, + options: options as GetPropertiesOptions, }) ).toEqual(expected); expect(headObject).toBeCalledTimes(1); diff --git a/packages/storage/__tests__/providers/s3/apis/getUrl.test.ts b/packages/storage/__tests__/providers/s3/apis/getUrl.test.ts index 74b992f18c2..5fb759584e8 100644 --- a/packages/storage/__tests__/providers/s3/apis/getUrl.test.ts +++ b/packages/storage/__tests__/providers/s3/apis/getUrl.test.ts @@ -8,7 +8,7 @@ import { getPresignedGetObjectUrl, headObject, } from '../../../../src/providers/s3/utils/client'; -import { StorageOptions } from '../../../../src/types'; +import { GetUrlOptions } from '../../../../src/providers/s3/types'; jest.mock('../../../../src/providers/s3/utils/client'); jest.mock('@aws-amplify/core', () => ({ @@ -106,9 +106,9 @@ describe('getUrl test', () => { const result = await getUrl({ key, options: { - ...(options as StorageOptions), + ...options, validateObjectExistence: true, - }, + } as GetUrlOptions, }); expect(getPresignedGetObjectUrl).toBeCalledTimes(1); expect(headObject).toBeCalledTimes(1); diff --git a/packages/storage/__tests__/providers/s3/apis/list.test.ts b/packages/storage/__tests__/providers/s3/apis/list.test.ts index fecad15e696..c1bc773127d 100644 --- a/packages/storage/__tests__/providers/s3/apis/list.test.ts +++ b/packages/storage/__tests__/providers/s3/apis/list.test.ts @@ -4,8 +4,11 @@ import { Credentials } from '@aws-sdk/types'; import { Amplify } from '@aws-amplify/core'; import { listObjectsV2 } from '../../../../src/providers/s3/utils/client'; -import { list } from '../../../../src/providers/s3/apis'; -import { StorageOptions } from '../../../../src/types'; +import { list } from '../../../../src/providers/s3'; +import { + ListAllOptions, + ListPaginateOptions, +} from '../../../../src/providers/s3/types'; jest.mock('../../../../src/providers/s3/utils/client'); jest.mock('@aws-amplify/core', () => ({ @@ -135,7 +138,7 @@ describe('list API', () => { expect.assertions(4); let response = await list({ prefix: path, - options: options as StorageOptions, + options: options as ListPaginateOptions, }); expect(response.items).toEqual([ { ...listResultItem, key: path ?? '' }, @@ -170,7 +173,7 @@ describe('list API', () => { const response = await list({ prefix: path, options: { - ...(options as StorageOptions), + ...(options as ListPaginateOptions), pageSize: customPageSize, nextToken: nextToken, }, @@ -202,9 +205,10 @@ describe('list API', () => { expect.assertions(3); let response = await list({ prefix: path, - options: options as StorageOptions, + options: options as ListPaginateOptions, }); expect(response.items).toEqual([]); + // expect(response.nextToken).toEqual(undefined); expect(listObjectsV2).toHaveBeenCalledWith(listObjectClientConfig, { Bucket: bucket, @@ -225,7 +229,7 @@ describe('list API', () => { mockListObjectsV2ApiWithPages(3); const result = await list({ prefix: path, - options: { ...(options as StorageOptions), listAll: true }, + options: { ...options, listAll: true } as ListAllOptions, }); const listResult = { ...listResultItem, key: path ?? '' }; diff --git a/packages/storage/src/providers/s3/types/index.ts b/packages/storage/src/providers/s3/types/index.ts index 17c64167090..4366ee48383 100644 --- a/packages/storage/src/providers/s3/types/index.ts +++ b/packages/storage/src/providers/s3/types/index.ts @@ -9,6 +9,8 @@ export { ListPaginateOptions, RemoveOptions, DownloadDataOptions, + CopyDestinationOptions, + CopySourceOptions, } from './options'; export { DownloadDataOutput, diff --git a/packages/storage/src/providers/s3/types/inputs.ts b/packages/storage/src/providers/s3/types/inputs.ts index 6596925cbc5..51c25ab4b3f 100644 --- a/packages/storage/src/providers/s3/types/inputs.ts +++ b/packages/storage/src/providers/s3/types/inputs.ts @@ -18,12 +18,18 @@ import { RemoveOptions, DownloadDataOptions, UploadDataOptions, + CopyDestinationOptions, + CopySourceOptions, } from '../types'; +// TODO: support use accelerate endpoint option /** * Input type for S3 copy API. */ -export type CopyInput = StorageCopyInput; +export type CopyInput = StorageCopyInput< + CopySourceOptions, + CopyDestinationOptions +>; /** * Input type for S3 getProperties API. diff --git a/packages/storage/src/providers/s3/types/options.ts b/packages/storage/src/providers/s3/types/options.ts index b2bd747e1e9..5082cfd4542 100644 --- a/packages/storage/src/providers/s3/types/options.ts +++ b/packages/storage/src/providers/s3/types/options.ts @@ -1,20 +1,17 @@ // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 +import { StorageAccessLevel } from '@aws-amplify/core'; // TODO(ashwinkumar6) this uses V5 Credentials, update to V6. import { Credentials } from '@aws-sdk/types'; import { TransferProgressEvent } from '../../../types'; import { - StorageOptions, StorageListAllOptions, StorageListPaginateOptions, } from '../../../types/options'; -/** - * Input options type for S3 Storage operations. - */ -export type Options = StorageOptions & { +type CommonOptions = { /** * Whether to use accelerate endpoint. * @default false @@ -22,68 +19,101 @@ export type Options = StorageOptions & { useAccelerateEndpoint?: boolean; }; +type ReadOptions = + | { accessLevel?: 'guest' | 'private' } + | { accessLevel: 'protected'; targetIdentityId?: string }; + +type WriteOptions = { + accessLevel?: StorageAccessLevel; +}; + +/** + * Transfer-related options type for S3 downloadData, uploadData APIs. + */ +type TransferOptions = { + /** + * Callback function tracking the upload/download progress. + */ + onProgress?: (event: TransferProgressEvent) => void; +}; + /** * Input options type for S3 getProperties API. */ -export type GetPropertiesOptions = Options; +export type GetPropertiesOptions = ReadOptions & CommonOptions; /** * Input options type for S3 getProperties API. */ -export type RemoveOptions = Options; +export type RemoveOptions = WriteOptions & CommonOptions; /** * Input options type for S3 list API. */ -export type ListAllOptions = StorageListAllOptions; +export type ListAllOptions = StorageListAllOptions & + ReadOptions & + CommonOptions; /** * Input options type for S3 list API. */ -export type ListPaginateOptions = StorageListPaginateOptions; +export type ListPaginateOptions = StorageListPaginateOptions & + ReadOptions & + CommonOptions; /** - * Input options type for S3 downloadData API. + * Input options type for S3 getUrl API. */ -export type DownloadDataOptions = TransferOptions; +export type GetUrlOptions = ReadOptions & + CommonOptions & { + /** + * Whether to head object to make sure the object existence before downloading. + * @default false + */ + validateObjectExistence?: boolean; + /** + * Number of seconds till the URL expires. + * @default 900 (15 minutes) + */ + expiresIn?: number; + }; /** - * Input options type for S3 getUrl API. + * Input options type for S3 downloadData API. */ -export type GetUrlOptions = Options & { - /** - * Whether to head object to make sure the object existence before downloading. - * @default false - */ - validateObjectExistence?: boolean; - /** - * Number of seconds till the URL expires. - * @default 900 (15 minutes) - */ - expiresIn?: number; +export type DownloadDataOptions = ReadOptions & CommonOptions & TransferOptions; + +export type UploadDataOptions = WriteOptions & + CommonOptions & + TransferOptions & { + /** + * The default content-disposition header value of the file when downloading it. + * @see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Disposition + */ + contentDisposition?: string; + /** + * The default content-encoding header value of the file when downloading it. + * @see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Encoding + */ + contentEncoding?: string; + /** + * The default content-type header value of the file when downloading it. + * @see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Type + */ + contentType?: string; + /** + * The user-defined metadata for the object uploaded to S3. + * @see https://docs.aws.amazon.com/AmazonS3/latest/userguide/UsingMetadata.html#UserMetadata + */ + metadata?: Record; + }; + +export type CopySourceOptions = ReadOptions & { + key: string; }; -export type UploadDataOptions = Omit & { - /** - * The default content-disposition header value of the file when downloading it. - * @see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Disposition - */ - contentDisposition?: string; - /** - * The default content-encoding header value of the file when downloading it. - * @see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Encoding - */ - contentEncoding?: string; - /** - * The default content-type header value of the file when downloading it. - * @see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Type - */ - contentType?: string; - /** - * The user-defined metadata for the object uploaded to S3. - * @see https://docs.aws.amazon.com/AmazonS3/latest/userguide/UsingMetadata.html#UserMetadata - */ - metadata?: Record; +export type CopyDestinationOptions = WriteOptions & { + key: string; }; /** @@ -98,12 +128,3 @@ export type ResolvedS3Config = { forcePathStyle?: boolean; useAccelerateEndpoint?: boolean; }; -/** - * Input options type for S3 downloadData, uploadData APIs. - */ -type TransferOptions = Options & { - /** - * Callback function tracking the upload/download progress. - */ - onProgress?: (event: TransferProgressEvent) => void; -}; diff --git a/packages/storage/src/types/index.ts b/packages/storage/src/types/index.ts index c85523a9c77..39bb1c049a3 100644 --- a/packages/storage/src/types/index.ts +++ b/packages/storage/src/types/index.ts @@ -23,8 +23,6 @@ export { StorageRemoveOptions, StorageListAllOptions, StorageListPaginateOptions, - StorageCopySourceOptions, - StorageCopyDestinationOptions, } from './options'; export { StorageItem, diff --git a/packages/storage/src/types/inputs.ts b/packages/storage/src/types/inputs.ts index 61ed132335b..9ef767450d6 100644 --- a/packages/storage/src/types/inputs.ts +++ b/packages/storage/src/types/inputs.ts @@ -5,8 +5,6 @@ import { StorageOptions, StorageListAllOptions, StorageListPaginateOptions, - StorageCopySourceOptions, - StorageCopyDestinationOptions, } from './options'; export type StorageOperationInput = { @@ -17,8 +15,10 @@ export type StorageOperationInput = { export type StorageGetPropertiesInput = StorageOperationInput; -export type StorageRemoveInput = - StorageOperationInput; +export type StorageRemoveInput = { + key: string; + options?: Options; +}; export type StorageListInput< Options extends StorageListAllOptions | StorageListPaginateOptions @@ -38,9 +38,12 @@ export type StorageUploadDataInput = data: StorageUploadDataPayload; }; -export type StorageCopyInput = { - source: StorageCopySourceOptions; - destination: StorageCopyDestinationOptions; +export type StorageCopyInput< + SourceOptions extends StorageOptions, + DestinationOptions extends StorageOptions +> = { + source: SourceOptions; + destination: DestinationOptions; }; /** diff --git a/packages/storage/src/types/options.ts b/packages/storage/src/types/options.ts index 3a95ffa764d..9962084dfde 100644 --- a/packages/storage/src/types/options.ts +++ b/packages/storage/src/types/options.ts @@ -3,12 +3,9 @@ import { StorageAccessLevel } from '@aws-amplify/core'; -export type StorageOptions = - | { accessLevel?: 'guest' | 'private' } - | { - accessLevel: 'protected'; - targetIdentityId?: string; - }; +export type StorageOptions = { + accessLevel?: StorageAccessLevel; +}; export type StorageListAllOptions = StorageOptions & { listAll: true; @@ -20,13 +17,4 @@ export type StorageListPaginateOptions = StorageOptions & { nextToken?: string; }; -export type StorageRemoveOptions = Omit; - -export type StorageCopySourceOptions = { - key: string; -} & StorageOptions; - -export type StorageCopyDestinationOptions = { - key: string; - accessLevel?: StorageAccessLevel; -}; +export type StorageRemoveOptions = StorageOptions;