aws-amplify / amplify-js

A declarative JavaScript library for application development using cloud services.
https://docs.amplify.aws/lib/q/platform/js
Apache License 2.0
9.43k stars 2.13k forks source link

Cannot access protected files in S3 by group pool #10296

Closed conor909 closed 2 years ago

conor909 commented 2 years ago

Before opening, please confirm:

JavaScript Framework

React

Amplify APIs

Storage

Amplify Categories

No response

Environment information

``` System: OS: macOS 12.4 CPU: (12) x64 Intel(R) Core(TM) i7-8750H CPU @ 2.20GHz Memory: 291.12 MB / 16.00 GB Shell: 5.8.1 - /bin/zsh Binaries: Node: 16.14.2 - ~/.nvm/versions/node/v16.14.2/bin/node Yarn: 1.22.19 - /usr/local/bin/yarn npm: 8.5.0 - ~/.nvm/versions/node/v16.14.2/bin/npm Watchman: 2022.07.04.00 - /usr/local/bin/watchman Browsers: Chrome: 105.0.5195.102 Safari: 15.5 npmPackages: @ampproject/toolbox-optimizer: undefined () @aws-amplify/ui-react: ^3.0.2 => 3.0.2 @aws-amplify/ui-react-internal: undefined () @aws-amplify/ui-react-legacy: undefined () @babel/core: undefined () @babel/runtime: 7.15.4 @hapi/accept: undefined () @napi-rs/triples: undefined () @next/react-dev-overlay: undefined () @next/react-refresh-utils: 12.1.6 @peculiar/webcrypto: undefined () @stripe/react-stripe-js: ^1.10.0 => 1.10.0 @stripe/stripe-js: ^1.35.0 => 1.35.0 @testing-library/jest-dom: ^4.2.4 => 4.2.4 @testing-library/react: ^9.3.2 => 9.5.0 @testing-library/user-event: ^7.1.2 => 7.2.1 @vercel/nft: undefined () abort-controller: undefined () acorn: undefined () amphtml-validator: undefined () arg: undefined () assert: undefined () async-retry: undefined () async-sema: undefined () aws-amplify: ^4.3.26 => 4.3.26 aws-sdk: ^2.1168.0 => 2.1168.0 axios: ^0.20.0 => 0.20.0 (0.26.0) babel-packages: undefined () babel-plugin-macros: ^3.1.0 => 3.1.0 (2.8.0) browserify-zlib: undefined () browserslist: undefined () buffer: undefined () bytes: undefined () chalk: undefined () ci-info: undefined () cli-select: undefined () comment-json: undefined () compression: undefined () conf: undefined () constants-browserify: undefined () content-disposition: undefined () content-type: undefined () cookie: undefined () cross-spawn: undefined () crypto-browserify: undefined () cssnano-simple: undefined () date-fns: ^2.28.0 => 2.28.0 debug: undefined () devalue: undefined () domain-browser: undefined () dotenv: ^8.2.0 => 8.6.0 (8.2.0) downloadjs: ^1.4.7 => 1.4.7 eslint: 8.18.0 => 8.18.0 (6.8.0) eslint-config-next: 12.1.6 => 12.1.6 etag: undefined () events: undefined () find-cache-dir: undefined () find-up: undefined () formdata-node: undefined () fresh: undefined () get-orientation: undefined () glob: undefined () gzip-size: undefined () history: ^5.0.0 => 5.3.0 (4.10.1) http-proxy: undefined () https-browserify: undefined () icss-utils: undefined () ignore-loader: undefined () image-size: undefined () is-animated: undefined () is-docker: undefined () is-wsl: undefined () jest-worker: undefined () js-file-download: ^0.4.12 => 0.4.12 json5: undefined () jsonwebtoken: undefined () loader-utils: undefined () lodash.curry: undefined () lru-cache: undefined () micromatch: undefined () mini-css-extract-plugin: undefined () nanoid: undefined () native-url: undefined () neo-async: undefined () next: 12.1.6 => 12.1.6 node-fetch: undefined () node-html-parser: undefined () ora: undefined () os-browserify: undefined () p-limit: undefined () path-browserify: undefined () postcss-flexbugs-fixes: undefined () postcss-modules-extract-imports: undefined () postcss-modules-local-by-default: undefined () postcss-modules-scope: undefined () postcss-modules-values: undefined () postcss-preset-env: undefined () postcss-safe-parser: undefined () postcss-scss: undefined () postcss-value-parser: undefined () process: undefined () punycode: undefined () querystring-es3: undefined () raw-body: undefined () react: 18.2.0 => 18.2.0 react-dom: 18.2.0 => 18.2.0 react-icons: ^4.4.0 => 4.4.0 react-is: 17.0.2 react-refresh: 0.12.0 react-router-dom: ^5.2.0 => 5.3.3 react-scripts: 3.4.3 => 3.4.3 react-server-dom-webpack: undefined () react-toastify: ^6.0.9 => 6.2.0 regenerator-runtime: 0.13.4 sass: ^1.53.0 => 1.53.0 sass-loader: undefined () schema-utils: undefined () semver: undefined () send: undefined () setimmediate: undefined () source-map: undefined () stream-browserify: undefined () stream-http: undefined () string-hash: undefined () string_decoder: undefined () strip-ansi: undefined () terser: undefined () text-table: undefined () timers-browserify: undefined () tty-browserify: undefined () ua-parser-js: undefined () unistore: undefined () use-subscription: undefined () util: undefined () uuid: ^8.3.1 => 8.3.2 (3.4.0, 3.3.2, 8.0.0, ) vm-browserify: undefined () watchpack: undefined () web-streams-polyfill: undefined () web-vitals: undefined () webpack: undefined () webpack-sources: undefined () ws: undefined () npmGlobalPackages: @aws-amplify/cli: 9.2.1 corepack: 0.10.0 eas-cli: 0.55.1 eslint: 8.20.0 npm: 8.5.0 ```

Describe the bug

When a user uploads a file as protected, an Admin cannot access it because aws-amplify adds a prefix of user region and user id to the location in storage, but returns only the filename as the key.

Expected behavior

I'm not sure if this behaviour is intended or not? I would of thought amplify would know the current logged in user and permissions on the object its attempting to download. So it seems burying protected objects in folders that dont match the object Key is counter intuitive.

Reproduction steps

Upload a file as a logged in user with:

      const result = await Storage.put('my-file.js', file, {
        level: 'protected',
        contentType: 'application/pdf'
      })

Log in as an admin and try to download the file:

      const result = await Storage.get(key, {
        download: true,
        level: 'protected'
      })

Code Snippet

Upload as user:
```js
import { Storage } from 'aws-amplify'

const result = Storage.put('file.pdf', file, { level: 'protected', contentType: 'application/pdf' })

// result:
// { key: 'file.pdf' }

// path in aws bucket:
// {user-aws-region}:{user-cognito-identity-id}/file.pdf

I save the key to an object, but when the Admin goes to download it, it says key not found, I presume because it's searching for

{admin-region}:{admin-identity-id}/filename.pdf

instead of

{owner-region}:{owner-identity-id}/filename.pdf

Is it not possible to download the file without having to save the owners identity Id somewhere for the Admin to use? Since I’ve granted access to the bucket to “Admin” Cognito User Group Pool in the cli, and amplify knows it’s an Admin user.

Additional information and screenshots

I've asked this question on StackOverflow but have not had any replies: https://stackoverflow.com/questions/73557587/amplify-storage-api-cannot-access-protected-files-by-group-pool

conor909 commented 2 years ago

I think this is related to this github issue on Amplify Cli repo

nadetastic commented 2 years ago

Hi @conor909 👋,

When trying to access another users protected object, you need to include their identity id. Under the hood, the identity id is used to determine the location of the protected file since the id is used as part of the key/path when uploading with Storage.put(). If you don't include one, it defaults to the current users identity id, which explains why you get the not found message.

Storage.get(key,{
    level: 'protected',
    identityId: '<region>:<random_value>'
});

https://docs.amplify.aws/lib/storage/upload/q/platform/js/#protected-level

conor909 commented 2 years ago

@nadetastic thanks for the explanation!

So is it best practice to save the owners identity ID to the same object we receive the key? Then do a check to see if the current user is an Admin, if so, add the identity ID prop, if not, just use the Key. It feels a bit manual, because Amplify knows who’s signed in, yet we have to save extra props in places to help retrieve the files.

nadetastic commented 2 years ago

@conor909 Not quite sure what you mean by saving the owners identity ID to the object - by default, Amplify will add the owners identity ID to the key/path in the S3 bucket: protected/<identity-id>/object-name.

However, you would need to have the owners identity ID before calling Storage.get (or Storage.list) in order to include it with the request.

nadetastic commented 2 years ago

Following up with you on this, @conor909 do you have any more questions or need additional help?

conor909 commented 2 years ago

@nadetastic I guess the answer is that you need the identity ID of the owner to access the file, how you get that ID is up to you.