Storage Browser Default Auth #13866
Storage Browser Default Auth #13866
Conversation
ashika112
requested review from
haverchuck,
cshfang,
jimblanc,
HuiSF and
a team
as code owners
There was a problem hiding this comment.
Adapter will most likely be moved to UI
There was a problem hiding this comment.
Adapter will most likely be moved to UI
| import { generateLocationsFromPaths } from './generateLocationsFromPaths'; | ||
|
|
||
| export const createAmplifyListLocationsHandler = (): ListPaths => { | ||
| const { buckets } = Amplify.getConfig().Storage!.S3!; |
There was a problem hiding this comment.
Sanity check: the default bucket exists in buckets as well right?
| const { tokens, identityId } = await fetchAuthSession(); | ||
| const userGroups = tokens?.accessToken.payload['cognito:groups']; | ||
|
|
||
| const locations = generateLocationsFromPaths({ |
There was a problem hiding this comment.
Nit: this name is not straightforward because this function does not take in paths input. What about renaming it to something like:
const authSession = await fetchAuthSession();
const locations = resolveAllowedLocationsFromSession( {buckets, authSession })
There was a problem hiding this comment.
Yes i have been trying to find the right naming too. I will updated this
| /** | ||
| * @internal | ||
| */ | ||
| export type StorageAccess = 'read' | 'get' | 'list' | 'write' | 'delete'; |
There was a problem hiding this comment.
Sanity check: does this match the types in the backend package?
| scope: { | ||
| bucketName: string; | ||
| path: string; | ||
| }; |
There was a problem hiding this comment.
Subject to API review tho but it's better to align the scope here with the scope from other API: https://github.com/ashika112/amplify-js/blob/9cbc71d8a78ce8944db9642d7222efaec55f99e1/packages/storage/src/internals/types/credentials.ts#L81
There was a problem hiding this comment.
@AllanZhengYP I check in with Caleb on this, he proposed a different change in interface for both managedAuth and AmplifyAuth locations. Will raise a sep PR when we make that update everywhere
There was a problem hiding this comment.
Interface we want to move to should be along the lines of:
export interface LocationData {
bucket: string;
permission: Permission;
prefix: string;
type: LocationType;
}
| const { bucketName, paths } = bucketInfo; | ||
| if (!paths) { | ||
| // Todo: Verify behavior | ||
| return locations; |
There was a problem hiding this comment.
Does this mean default bucket? Should it be continue?
There was a problem hiding this comment.
This is for when there is no paths. So locations will be [] at that point.
| ...resolvePermissions(accessRules, tokens, userGroup), | ||
| scope: { bucketName, path }, | ||
| }; | ||
| if (location.permission) locations.push(location as PathAccess); |
There was a problem hiding this comment.
Sanity check, what are the cases when a path is omitted from locations? I'm wondering if we should fail-fast in these scenarios.
There was a problem hiding this comment.
Nit: Looking at our doc it seems backend also support Functions access type. It make sense to not support it here, but just make sure we document it.
There was a problem hiding this comment.
what are the cases when a path is omitted from locations?
- There is logic in
resolvePermissionson what permission to pick for what use cases like authenticated, unAuthenticated, user group, entity etc. Whereever we could fail fast that is done. This logic here is more santizing the array after resolvePermission.
There was a problem hiding this comment.
Nit: Looking at our doc it seems backend also support Functions access type. It make sense to not support it here, but just make sure we document it.
This will result in empty paths it actually would not even make it to buckets but for both of these scenarios we fail fast so it would not even get to this point
| return locations; | ||
| } | ||
| for (const [path, accessRules] of Object.entries(paths)) { | ||
| if (tokens && identityId && path.includes(ENTITY_IDENTITY_URL)) { |
There was a problem hiding this comment.
Nit: this can be moved to a function similar to resolvePermissions
Description of changes
TODO
Description of how you validated changes
Checklist
yarn testpassesChecklist for repo maintainers
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.