-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Storage Browser Default Auth #13866
Storage Browser Default Auth #13866
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adapter will most likely be moved to UI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adapter will most likely be moved to UI
packages/storage/src/internals/amplifyAuthConfigAdapter/createAmplifyListLocationsHandler.ts
Show resolved
Hide resolved
packages/storage/src/internals/amplifyAuthConfigAdapter/createAmplifyListLocationsHandler.ts
Outdated
Show resolved
Hide resolved
packages/storage/src/internals/amplifyAuthConfigAdapter/generateLocationsFromPaths.ts
Outdated
Show resolved
Hide resolved
packages/storage/src/internals/amplifyAuthConfigAdapter/generateLocationsFromPaths.ts
Outdated
Show resolved
Hide resolved
packages/storage/src/internals/amplifyAuthConfigAdapter/generateLocationsFromPaths.ts
Outdated
Show resolved
Hide resolved
…plify-js into feat/sb-default-auth
/** | ||
* @internal | ||
*/ | ||
export interface PathAccess { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping this separate for now. In a separate PR will align this and managed Auth Location access into one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Only have 1 nit comment.
@@ -50,6 +50,8 @@ export interface AmplifyOutputsStorageBucketProperties { | |||
bucket_name: string; | |||
/** Region for the bucket */ | |||
aws_region: string; | |||
/** Paths to object with access permissions */ | |||
paths?: Record<string, Record<string, string[] | undefined>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The in-line type is a bit hard the follow, what about using a interface with property names:
type Permission = string;
interface StorageBucketPaths {
[pathPrefix: string]: {
[accessType: string]: Permission[] | undefined;
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our ESLint does not allow for index signature. But i think we can create alias and align if we want. I can look into this in my follow up PR for pagination if there are no other comments :)
packages/storage/src/internals/amplifyAuthConfigAdapter/createAmplifyAuthConfigAdapter.ts
Show resolved
Hide resolved
e515d24
into
aws-amplify:storage-browser/integrity
Description of changes
TODO
Description of how you validated changes
Checklist
yarn test
passesChecklist for repo maintainers
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.