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

feat(storage): enables location credentials provider #13605

Merged
merged 13 commits into from
Jul 22, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ import {
StorageValidationErrorCode,
validationErrorMap,
} from '../../../../../src/errors/types/validation';
import {
CallbackPathStorageInput,
DeprecatedStorageInput,
} from '../../../../../src/providers/s3/utils/resolveS3ConfigAndInput';
import { INVALID_STORAGE_INPUT } from '../../../../../src/errors/constants';

jest.mock('@aws-amplify/core', () => ({
ConsoleLogger: jest.fn(),
Expand Down Expand Up @@ -76,13 +81,11 @@ describe('resolveS3ConfigAndInput', () => {
}
});

it('should throw if identityId is not available', async () => {
it('should not throw if identityId is not available', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

The resolveS3ConfingAndInput helper is not asserting for the identityId anymore

Copy link
Member

Choose a reason for hiding this comment

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

while this might not be needed, if customers are already relyign on this message for something, would removing it cause a breaking change for them?

Copy link
Member

Choose a reason for hiding this comment

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

@ashika112 I don't think removing the validation here itself is a breaking change, unless they are using Storage APIs intentionally to check for identity ID. However, it's not the intended usage of the Storage APIs.

Copy link
Member

Choose a reason for hiding this comment

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

Oh i mean , if customer lets say is specifically finding this error to avoid making a call right now, wouldn't it break? this is propogated all the way to customer. Again coming from path callback perspective, but if this check is moved to path validation i think it is fine there. but wondering if there is any other use case like that

mockFetchAuthSession.mockResolvedValueOnce({
credentials,
});
await expect(resolveS3ConfigAndInput(Amplify, {})).rejects.toMatchObject(
validationErrorMap[StorageValidationErrorCode.NoIdentityId],
);
expect(async () => resolveS3ConfigAndInput(Amplify, {})).not.toThrow();
});

it('should resolve bucket from S3 config', async () => {
Expand Down Expand Up @@ -214,4 +217,74 @@ describe('resolveS3ConfigAndInput', () => {
});
expect(keyPrefix).toEqual('prefix');
});

describe('with locationCredentialsProvider', () => {
const mockLocationCredentialsProvider = jest
.fn()
.mockReturnValue({ credentials });
it('should resolve credentials without Amplify singleton', async () => {
mockGetConfig.mockReturnValue({
Storage: {
S3: {
bucket,
region,
},
},
});
const { s3Config } = await resolveS3ConfigAndInput(Amplify, {
locationCredentialsProvider: mockLocationCredentialsProvider,
});

if (typeof s3Config.credentials === 'function') {
const result = await s3Config.credentials();
expect(mockLocationCredentialsProvider).toHaveBeenCalled();
expect(result).toEqual(credentials);
} else {
throw new Error('Expect credentials to be a function');
}
});

describe('with deprecated or callback paths as inputs', () => {
ashika112 marked this conversation as resolved.
Show resolved Hide resolved
const key = 'mock-value';
const prefix = 'mock-value';
const path = () => 'path';
const deprecatedInputs: DeprecatedStorageInput[] = [
{ prefix },
{ key },
{
source: { key },
destination: { key },
},
];
const callbackPathInputs: CallbackPathStorageInput[] = [
{ path },
{
destination: { path },
source: { path },
},
];

const testCases = [...deprecatedInputs, ...callbackPathInputs];

it.each(testCases)('should throw when input is %s', async input => {
const { s3Config } = await resolveS3ConfigAndInput(
Amplify,
{ locationCredentialsProvider: mockLocationCredentialsProvider },
input,
);
if (typeof s3Config.credentials === 'function') {
await expect(s3Config.credentials()).rejects.toThrow(
expect.objectContaining({
israx marked this conversation as resolved.
Show resolved Hide resolved
name: INVALID_STORAGE_INPUT,
message: 'The input needs to have a path as a string value.',
recoverySuggestion:
'Please provide a valid path as a string value for the input.',
}),
);
} else {
throw new Error('Expect credentials to be a function');
}
});
});
});
});
4 changes: 4 additions & 0 deletions packages/storage/src/errors/constants.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0

export const INVALID_STORAGE_INPUT = 'InvalidStorageInput';
6 changes: 5 additions & 1 deletion packages/storage/src/providers/s3/apis/downloadData.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,11 @@ const downloadDataJob =
> => {
const { options: downloadDataOptions } = downloadDataInput;
const { bucket, keyPrefix, s3Config, identityId } =
await resolveS3ConfigAndInput(Amplify, downloadDataOptions);
await resolveS3ConfigAndInput(
israx marked this conversation as resolved.
Show resolved Hide resolved
Amplify,
downloadDataOptions,
downloadDataInput,
);
const { inputType, objectKey } = validateStorageOperationInput(
downloadDataInput,
identityId,
Expand Down
10 changes: 7 additions & 3 deletions packages/storage/src/providers/s3/apis/internal/copy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,11 @@ const copyWithPath = async (
input: CopyWithPathInput,
): Promise<CopyWithPathOutput> => {
const { source, destination } = input;
const { s3Config, bucket, identityId } =
await resolveS3ConfigAndInput(amplify);
const { s3Config, bucket, identityId } = await resolveS3ConfigAndInput(
amplify,
{},
input,
);

assertValidationError(!!source.path, StorageValidationErrorCode.NoSourcePath);
assertValidationError(
Expand Down Expand Up @@ -92,10 +95,11 @@ export const copyWithKey = async (
s3Config,
bucket,
keyPrefix: sourceKeyPrefix,
} = await resolveS3ConfigAndInput(amplify, input.source);
} = await resolveS3ConfigAndInput(amplify, input.source, input);
const { keyPrefix: destinationKeyPrefix } = await resolveS3ConfigAndInput(
amplify,
input.destination,
input,
); // resolveS3ConfigAndInput does not make extra API calls or storage access if called repeatedly.

// TODO(ashwinkumar6) V6-logger: warn `You may copy files from another user if the source level is "protected", currently it's ${srcLevel}`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export const getProperties = async (
): Promise<GetPropertiesOutput | GetPropertiesWithPathOutput> => {
const { options: getPropertiesOptions } = input;
const { s3Config, bucket, keyPrefix, identityId } =
await resolveS3ConfigAndInput(amplify, getPropertiesOptions);
await resolveS3ConfigAndInput(amplify, getPropertiesOptions, input);
const { inputType, objectKey } = validateStorageOperationInput(
input,
identityId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export const getUrl = async (
): Promise<GetUrlOutput | GetUrlWithPathOutput> => {
const { options: getUrlOptions } = input;
const { s3Config, keyPrefix, bucket, identityId } =
await resolveS3ConfigAndInput(amplify, getUrlOptions);
await resolveS3ConfigAndInput(amplify, getUrlOptions, input);
const { inputType, objectKey } = validateStorageOperationInput(
input,
identityId,
Expand Down
2 changes: 1 addition & 1 deletion packages/storage/src/providers/s3/apis/internal/list.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export const list = async (
bucket,
keyPrefix: generatedPrefix,
identityId,
} = await resolveS3ConfigAndInput(amplify, options);
} = await resolveS3ConfigAndInput(amplify, options, input);

const { inputType, objectKey } = validateStorageOperationInputWithPrefix(
input,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export const remove = async (
): Promise<RemoveOutput | RemoveWithPathOutput> => {
const { options = {} } = input ?? {};
const { s3Config, keyPrefix, bucket, identityId } =
await resolveS3ConfigAndInput(amplify, options);
await resolveS3ConfigAndInput(amplify, options, input);

const { inputType, objectKey } = validateStorageOperationInput(
input,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ export const getMultipartUploadHandlers = (
const resolvedS3Options = await resolveS3ConfigAndInput(
Amplify,
uploadDataOptions,
uploadDataInput,
);

abortController = new AbortController();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,11 @@ export const putObjectJob =
async (): Promise<ItemWithKey | ItemWithPath> => {
const { options: uploadDataOptions, data } = uploadDataInput;
const { bucket, keyPrefix, s3Config, isObjectLockEnabled, identityId } =
await resolveS3ConfigAndInput(Amplify, uploadDataOptions);
await resolveS3ConfigAndInput(
Amplify,
uploadDataOptions,
uploadDataInput,
);
const { inputType, objectKey } = validateStorageOperationInput(
uploadDataInput,
identityId,
Expand Down
6 changes: 5 additions & 1 deletion packages/storage/src/providers/s3/types/inputs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ import {
UploadDataOptionsWithPath,
} from '../types';

import { LocationCredentialsProvider } from './options';

// TODO: support use accelerate endpoint option
israx marked this conversation as resolved.
Show resolved Hide resolved
/**
* @deprecated Use {@link CopyWithPathInput} instead.
Expand All @@ -47,7 +49,9 @@ export type CopyInput = StorageCopyInputWithKey<
/**
* Input type with path for S3 copy API.
*/
export type CopyWithPathInput = StorageCopyInputWithPath;
export type CopyWithPathInput = StorageCopyInputWithPath<{
locationCredentialsProvider?: LocationCredentialsProvider;
}>;

/**
* @deprecated Use {@link GetPropertiesWithPathInput} instead.
Expand Down
11 changes: 11 additions & 0 deletions packages/storage/src/providers/s3/utils/resolveIdentityId.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// 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';

export const resolveIdentityId = (identityId?: string): string => {
assertValidationError(!!identityId, StorageValidationErrorCode.NoIdentityId);

return identityId;
};
102 changes: 99 additions & 3 deletions packages/storage/src/providers/s3/utils/resolveS3ConfigAndInput.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,27 @@ import { AmplifyClassV6, StorageAccessLevel } from '@aws-amplify/core';
import { assertValidationError } from '../../../errors/utils/assertValidationError';
import { StorageValidationErrorCode } from '../../../errors/types/validation';
import { resolvePrefix as defaultPrefixResolver } from '../../../utils/resolvePrefix';
import { ResolvedS3Config } from '../types/options';
import {
LocationCredentialsProvider,
ResolvedS3Config,
} from '../types/options';
import {
StorageCopyInputWithPath,
StorageOperationInputWithKey,
StorageOperationInputWithPath,
StorageOperationInputWithPrefix,
} from '../../../types/inputs';
import { StorageError } from '../../../errors/StorageError';
import { CopyInput } from '../types';
import { INVALID_STORAGE_INPUT } from '../../../errors/constants';

import { DEFAULT_ACCESS_LEVEL, LOCAL_TESTING_S3_ENDPOINT } from './constants';

interface S3ApiOptions {
accessLevel?: StorageAccessLevel;
targetIdentityId?: string;
useAccelerateEndpoint?: boolean;
locationCredentialsProvider?: LocationCredentialsProvider;
}

interface ResolvedS3ConfigAndInput {
Expand All @@ -23,6 +36,14 @@ interface ResolvedS3ConfigAndInput {
isObjectLockEnabled?: boolean;
identityId?: string;
}
export type DeprecatedStorageInput =
| StorageOperationInputWithKey
| StorageOperationInputWithPrefix
| CopyInput;

export type CallbackPathStorageInput =
| StorageOperationInputWithPath
| StorageCopyInputWithPath;
ashika112 marked this conversation as resolved.
Show resolved Hide resolved

/**
* resolve the common input options for S3 API handlers from Amplify configuration and library options.
Expand All @@ -38,13 +59,13 @@ interface ResolvedS3ConfigAndInput {
export const resolveS3ConfigAndInput = async (
amplify: AmplifyClassV6,
apiOptions?: S3ApiOptions,
input?: DeprecatedStorageInput | CallbackPathStorageInput,
AllanZhengYP marked this conversation as resolved.
Show resolved Hide resolved
): Promise<ResolvedS3ConfigAndInput> => {
/**
* IdentityId is always cached in memory so we can safely make calls here. It
* should be stable even for unauthenticated users, regardless of credentials.
*/
const { identityId } = await amplify.Auth.fetchAuthSession();
assertValidationError(!!identityId, StorageValidationErrorCode.NoIdentityId);
ashika112 marked this conversation as resolved.
Show resolved Hide resolved

/**
* A credentials provider function instead of a static credentials object is
Expand All @@ -53,7 +74,13 @@ export const resolveS3ConfigAndInput = async (
* credentials if they are expired.
*/
const credentialsProvider = async () => {
const { credentials } = await amplify.Auth.fetchAuthSession();
if (isLocationCredentialsProvider(apiOptions)) {
assertStorageInput(input);
}

const { credentials } = isLocationCredentialsProvider(apiOptions)
? await apiOptions.locationCredentialsProvider()
: await amplify.Auth.fetchAuthSession();
assertValidationError(
!!credentials,
StorageValidationErrorCode.NoCredentials,
Expand Down Expand Up @@ -101,3 +128,72 @@ export const resolveS3ConfigAndInput = async (
isObjectLockEnabled,
};
};

const isLocationCredentialsProvider = (
options?: S3ApiOptions,
): options is S3ApiOptions & {
locationCredentialsProvider: LocationCredentialsProvider;
} => {
return !!options?.locationCredentialsProvider;
};

const isInputWithCallbackPath = (input?: CallbackPathStorageInput) => {
return (
((input as StorageOperationInputWithPath)?.path &&
typeof (input as StorageOperationInputWithPath).path === 'function') ||
((input as StorageCopyInputWithPath)?.destination?.path &&
typeof (input as StorageCopyInputWithPath).destination?.path ===
'function') ||
israx marked this conversation as resolved.
Show resolved Hide resolved
((input as StorageCopyInputWithPath)?.source?.path &&
typeof (input as StorageCopyInputWithPath).source?.path === 'function')
);
};

const isDeprecatedInput = (
input?: DeprecatedStorageInput | CallbackPathStorageInput,
): input is DeprecatedStorageInput => {
return (
isInputWithKey(input) ||
isInputWithPrefix(input) ||
isInputWithCopySourceOrDestination(input)
);
};
const assertStorageInput = (
input?: DeprecatedStorageInput | CallbackPathStorageInput,
) => {
if (isDeprecatedInput(input) || isInputWithCallbackPath(input)) {
Copy link
Member

Choose a reason for hiding this comment

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

why not use assertValidationError here?

Copy link
Member Author

Choose a reason for hiding this comment

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

The assertValidationError uses a map and enum, expanding any of those will increase the bundle size. That is why I would like to avoid using it

Copy link
Member

Choose a reason for hiding this comment

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

Yes but any validation we already perform it through the assertValidation why change the pattern here and now?

throw new StorageError({
name: INVALID_STORAGE_INPUT,
message: 'The input needs to have a path as a string value.',
recoverySuggestion:
'Please provide a valid path as a string value for the input.',
});
}
};

const isInputWithKey = (
ashika112 marked this conversation as resolved.
Show resolved Hide resolved
input?: DeprecatedStorageInput | CallbackPathStorageInput,
): input is StorageOperationInputWithKey => {
return !!(
(input as StorageOperationInputWithKey)?.key &&
typeof (input as StorageOperationInputWithKey).key === 'string'
);
};
const isInputWithPrefix = (
input?: DeprecatedStorageInput | CallbackPathStorageInput,
): input is StorageOperationInputWithPrefix => {
return !!(
(input as StorageOperationInputWithPrefix)?.prefix &&
typeof (input as StorageOperationInputWithPrefix).prefix === 'string'
);
};
const isInputWithCopySourceOrDestination = (
input?: CopyInput | CallbackPathStorageInput,
): input is CopyInput => {
return !!(
((input as CopyInput)?.source?.key &&
typeof (input as CopyInput).source?.key === 'string') ||
((input as CopyInput)?.destination?.key &&
typeof (input as CopyInput).destination?.key === 'string')
);
};
Loading
Loading