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): add credentials store scafolding and update types #13558

Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0

describe('createLocationCredentialsStore', () => {
it.todo('should create a store');
describe('created store', () => {
describe('getValue()', () => {
it.todo('should call getValue() from store');
it.todo(
'should validate credentials location with resolved common scope',
);
});
describe('destroy()', () => {
it.todo('should call removeStore() from store');
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/* eslint-disable unused-imports/no-unused-vars */

// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0

describe('createStore', () => {
it.todo('should create a store with given capacity, refresh Handler');
it.todo('should return a symbol to refer the store instance');
});

describe('getValue', () => {
it.todo('should look up a cache value for given location and permission');
it.todo(
'should look up a cache value for given location and READWRITE permission',
);
it.todo('should invoke the refresh handler if look up returns null');
it.todo(
'should invoke refresh handler only once if multiple look up for same location returns null',
);
it.todo('should throw if refresh handler throws');
it.todo(
'should invoke the refresh handler if the refresh handler previously fails',
);
});

describe('removeStore', () => {
it.todo('should remove the store with given symbol');
it.todo('should not throw if store with given symbol does not exist');
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0

describe('initStore', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Love all these todos!

it.todo(
'should create a store with given capacity, refresh Handler and cache',
);
it.todo('should create a store with default capacity if not provided');
it.todo('should throw if capacity is not > 0');
});

describe('getCacheKey', () => {
it.todo('should return a cache key for given location and permission');
});

describe('getCacheValue', () => {
it.todo('should return a cache value for given location and permission');
it.todo('should return null if cache value is not found');
it.todo('should return null if cache value is expired');
it.todo('should return null if cache value is not valid');
});
10 changes: 9 additions & 1 deletion packages/storage/src/providers/s3/types/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,20 @@ import {
*/
export type Permission = 'READ' | 'READWRITE' | 'WRITE';

/**
* @internal
*/
export interface BucketLocation {
bucket: string;
path: string;
}

/**
* @internal
*/
export type LocationCredentialsProvider = (options: {
forceRefresh?: boolean;
locations: { bucket: string; path: string }[];
locations: BucketLocation[];
permission: Permission;
}) => Promise<{ credentials: AWSCredentials }>;

Expand Down
6 changes: 5 additions & 1 deletion packages/storage/src/storage-browser/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,8 @@ export {
ListCallerAccessGrantsOutput,
} from './listCallerAccessGrants';
export { createLocationCredentialsHandler } from './createLocationCredentialsHandler';
export { createLocationCredentialsStore } from './locationCredentialsStore/createLocationCredentialsStore';
export { createLocationCredentialsStore } from './locationCredentialsStore/create';
Copy link
Contributor

@jimblanc jimblanc Jul 3, 2024

Choose a reason for hiding this comment

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

Nit: I think I prefer the old way, naming the file the same as the utility it contains is a pretty normal pattern in the lib (don't feel strongly, not a blocker)

export {
managedAuthAdapter,
ManagedAuthAdapterInput,
} from './managedAuthAdapter';
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/* eslint-disable unused-imports/no-unused-vars */

// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0

import {
CredentialsLocation,
LocationCredentialsHandler,
LocationCredentialsStore,
} from '../types';
import { LocationCredentialsProvider } from '../../providers/s3/types/options';

import { createStore, getValue, removeStore } from './registry';

export const createLocationCredentialsStore = (input: {
handler: LocationCredentialsHandler;
}): LocationCredentialsStore => {
const storeReference = createStore(input.handler);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit since this isn't actually a reference to the store

Suggested change
const storeReference = createStore(input.handler);
const storeSymbol = createStore(input.handler);

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. will address this in the implementation PR.


const store = {
getProvider(providerLocation: CredentialsLocation) {
const locationCredentialsProvider = async ({
permission,
locations,
forceRefresh = false,
}: Parameters<LocationCredentialsProvider>[0]) => {
// TODO(@AllanZhengYP) validate input

return getValue({
storeReference,
location: { ...providerLocation },
forceRefresh,
});
};

return locationCredentialsProvider;
},

destroy() {
removeStore(storeReference);
},
};

return store;
};

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/* eslint-disable unused-imports/no-unused-vars */

// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
import { AWSCredentials } from '@aws-amplify/core/internals/utils';

import { CredentialsLocation, LocationCredentialsHandler } from '../types';

import { LocationCredentialsStore, initStore } from './store';

/**
* Keep all cache records for all instances of credentials store in a singleton
* so we can reliably de-reference from the memory when we destroy a store
* instance.
*/
const storeRegistry = new Map<symbol, LocationCredentialsStore>();
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 to use WeakMap to ensure an automatic GC when the registry key is dereferenced?


export const createStore = (
refreshHandler: LocationCredentialsHandler,
size?: number,
) => {
const storeInstanceSymbol = Symbol('LocationCredentialsStore');
storeRegistry.set(storeInstanceSymbol, initStore(refreshHandler, size));

return storeInstanceSymbol;
};

export const getValue = async (input: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be getStore to align with createStore?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. This is already done in the following implementation PR ☺️

storeReference: symbol;
location: CredentialsLocation;
forceRefresh: boolean;
}): Promise<{ credentials: AWSCredentials }> => {
// TODO(@AllanZhengYP): get location credentials from store.
throw new Error('Not implemented');
};

export const removeStore = (storeReference: symbol) => {
storeRegistry.delete(storeReference);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/* eslint-disable unused-imports/no-unused-vars */

// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0

import { AWSCredentials } from '@aws-amplify/core/internals/utils';

import { Permission } from '../../providers/s3/types/options';
import { CredentialsLocation, LocationCredentialsHandler } from '../types';

const CREDENTIALS_STORE_DEFAULT_SIZE = 10;

export interface StoreValue extends CredentialsLocation {
credentials?: AWSCredentials;
inflightCredentials?: Promise<{ credentials: AWSCredentials }>;
}

type S3Url = string;

/**
* @internal
*/
export type CacheKey = `${S3Url}_${Permission}`;

/**
* @internal
*/
export const getCacheKey = (compositeKey: CredentialsLocation): CacheKey =>
`${compositeKey.scope}_${compositeKey.permission}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Sanity check: Will weird customer defined delimiters mess with this at all? E.g. if a customer uses ` or _ Everything would already be URL encoded right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Delimiter does not affect the caching key since a different delimiter refers to a different object.


/**
* LRU implementation for Location Credentials Store
* O(n) for get and set for simplicity.
*
* @internal
*/
export interface LocationCredentialsStore {
capacity: number;
refreshHandler: LocationCredentialsHandler;
values: Map<CacheKey, StoreValue>;
}

/**
* @internal
*/
export const initStore = (
refreshHandler: LocationCredentialsHandler,
size = CREDENTIALS_STORE_DEFAULT_SIZE,
): LocationCredentialsStore => {
// TODO(@AllanZhengYP) create StorageError
if (size <= 0) {
throw new Error('Invalid Cache size');
}

return {
capacity: size,
refreshHandler,
values: new Map<CacheKey, StoreValue>(),
};
};

export const getCacheValue = (
store: LocationCredentialsStore,
key: CacheKey,
): AWSCredentials | null => {
throw new Error('Not implemented');
};
27 changes: 27 additions & 0 deletions packages/storage/src/storage-browser/managedAuthAdapter.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
import {
CredentialsProvider,
ListLocations,
LocationCredentialsHandler,
} from './types';

export interface ManagedAuthAdapterInput {
accountId: string;
region: string;
credentialsProvider: CredentialsProvider;
}

export interface ManagedAuthAdapterOutput {
listLocations: ListLocations;
getLocationCredentials: LocationCredentialsHandler;
region: string;
}

export const managedAuthAdapter = (
// eslint-disable-next-line unused-imports/no-unused-vars
input: ManagedAuthAdapterInput,
): ManagedAuthAdapterOutput => {
// TODO(@AllanZhengYP)
throw new Error('Not implemented');
};
43 changes: 22 additions & 21 deletions packages/storage/src/storage-browser/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,31 +15,39 @@ export type CredentialsProvider = (options?: {
forceRefresh?: boolean;
}) => Promise<{ credentials: AWSCredentials }>;

type LocationAccessType = 'BUCKET' | 'PREFIX' | 'OBJECT';

/**
* @internal
*/
export interface LocationAccess {
export type LocationType = 'BUCKET' | 'PREFIX' | 'OBJECT';

export interface CredentialsLocation {
AllanZhengYP marked this conversation as resolved.
Show resolved Hide resolved
/**
* Scope of storage location. For S3 service, it's the S3 path of the data to
* which the access is granted.
* which the access is granted. It can be in following formats:
*
* @example 's3://MY-BUCKET-NAME/prefix/*'
* @example Bucket 's3://<bucket>/*'
* @example Prefix 's3://<bucket>/<prefix-with-path>*'
* @example Object 's3://<bucket>/<prefix-with-path>/<object>'
*/
readonly scope: string;
/**
* The type of access granted to your Storage data. Can be either of READ,
* WRITE or READWRITE
*/
readonly permission: Permission;
}

/**
* @internal
*/
export interface LocationAccess extends CredentialsLocation {
/**
* parse location type parsed from scope format:
* * bucket: `'s3://<bucket>/*'`
* * prefix: `'s3://<bucket>/<prefix-with-path>*'`
* * object: `'s3://<bucket>/<prefix-with-path>/<object>'`
* Parse location type parsed from scope format:
* * BUCKET: `'s3://<bucket>/*'`
* * PREFIX: `'s3://<bucket>/<prefix-with-path>*'`
* * OBJECT: `'s3://<bucket>/<prefix-with-path>/<object>'`
*/
readonly type: LocationAccessType;
readonly type: LocationType;
}

export interface AccessGrant extends LocationAccess {
Expand All @@ -64,24 +72,17 @@ export interface ListLocationsOutput<T extends LocationAccess> {
export type ListLocations = () => Promise<ListLocationsOutput<LocationAccess>>;

// Interface for getLocationCredentials() handler.
export type LocationCredentialsHandler = (input: {
scope: string;
permission: Permission;
}) => Promise<{ credentials: AWSCredentials; scope?: string }>;
export type LocationCredentialsHandler = (
input: CredentialsLocation,
) => Promise<{ credentials: AWSCredentials }>;

export interface LocationCredentialsStore {
/**
* Get location-specific credentials. It uses a cache internally to optimize performance when
* getting credentials for the same location. It will refresh credentials if they expire or
* when forced to.
*
* If specific credentials scope `option` is omitted, the store will attempt to resolve
* locations-specific credentials from the input bucket and full path.
*/
getProvider(option?: {
scope: string;
permission: Permission;
}): LocationCredentialsProvider;
getProvider(option: CredentialsLocation): LocationCredentialsProvider;
/**
* Invalidate cached credentials and force subsequent calls to get location-specific
* credentials to throw. It also makes subsequent calls to `getCredentialsProviderForLocation`
Expand Down
Loading