feat: disallow targetIdentityId option in all S3 write APIs and categ…

…ory API (#12034) --------- Co-authored-by: Jim Blanchard <[email protected]>
aws-amplify · Sep 15, 2023 · 2abb136 · 2abb136
1 parent a96ade9
commit 2abb136
Show file tree

Hide file tree

Showing 11 changed files with 121 additions and 99 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -1,89 +1,119 @@
 // 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
 	 */
 	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<string, string>;
+	};
+
+export type CopySourceOptions = ReadOptions & {
+	key: string;
 };
 
-export type UploadDataOptions = Omit<TransferOptions, 'targetIdentityId'> & {
-	/**
-	 * 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<string, string>;
+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
@@ -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
@@ -5,8 +5,6 @@ import {
 	StorageOptions,
 	StorageListAllOptions,
 	StorageListPaginateOptions,
-	StorageCopySourceOptions,
-	StorageCopyDestinationOptions,
 } from './options';
 
 export type StorageOperationInput<Options extends StorageOptions> = {
@@ -17,8 +15,10 @@ export type StorageOperationInput<Options extends StorageOptions> = {
 export type StorageGetPropertiesInput<Options extends StorageOptions> =
 	StorageOperationInput<Options>;
 
-export type StorageRemoveInput<Options extends StorageOptions> =
-	StorageOperationInput<Options>;
+export type StorageRemoveInput<Options extends StorageOptions> = {
+	key: string;
+	options?: Options;
+};
 
 export type StorageListInput<
 	Options extends StorageListAllOptions | StorageListPaginateOptions
@@ -38,9 +38,12 @@ export type StorageUploadDataInput<Options extends StorageOptions> =
 		data: StorageUploadDataPayload;
 	};
 
-export type StorageCopyInput = {
-	source: StorageCopySourceOptions;
-	destination: StorageCopyDestinationOptions;
+export type StorageCopyInput<
+	SourceOptions extends StorageOptions,
+	DestinationOptions extends StorageOptions
+> = {
+	source: SourceOptions;
+	destination: DestinationOptions;
 };
 
 /**