aws-amplify / amplify-category-api

The AWS Amplify CLI is a toolchain for simplifying serverless web and mobile development. This plugin provides functionality for the API category, allowing for the creation and management of GraphQL and REST based backends for your amplify project.
https://docs.amplify.aws/
Apache License 2.0
89 stars 79 forks source link

Incorrect warning "owners may reassign ownership for the following model..." #872

Open AlessioVallero opened 2 years ago

AlessioVallero commented 2 years ago

Before opening, please confirm:

JavaScript Framework

React Native

Amplify APIs

GraphQL API

Amplify Categories

auth

Environment information

``` # Put output below this line System: OS: macOS 12.5.1 CPU: (12) x64 Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz Memory: 1.94 GB / 16.00 GB Shell: 5.8.1 - /bin/zsh Binaries: Node: 16.17.0 - ~/.nvm/versions/node/v16.17.0/bin/node Yarn: 1.22.19 - ~/core/www/node_modules/.bin/yarn npm: 8.19.2 - ~/.nvm/versions/node/v16.17.0/bin/npm Browsers: Chrome: 106.0.5249.103 Firefox: 101.0 Safari: 15.6.1 npmPackages: @apollo/client: ^3.5.7 => 3.5.7 @apollo/client/cache: undefined () @apollo/client/core: undefined () @apollo/client/errors: undefined () @apollo/client/link/batch: undefined () @apollo/client/link/batch-http: undefined () @apollo/client/link/context: undefined () @apollo/client/link/core: undefined () @apollo/client/link/error: undefined () @apollo/client/link/http: undefined () @apollo/client/link/persisted-queries: undefined () @apollo/client/link/retry: undefined () @apollo/client/link/schema: undefined () @apollo/client/link/utils: undefined () @apollo/client/link/ws: undefined () @apollo/client/react: undefined () @apollo/client/react/components: undefined () @apollo/client/react/context: undefined () @apollo/client/react/hoc: undefined () @apollo/client/react/hooks: undefined () @apollo/client/react/parser: undefined () @apollo/client/react/ssr: undefined () @apollo/client/testing: undefined () @apollo/client/testing/core: undefined () @apollo/client/utilities: undefined () @apollo/client/utilities/globals: undefined () @aws-amplify/auth: ^4.3.20 => 4.3.20 (4.6.8) @aws-amplify/datastore: ^3.7.4 => 3.7.4 (3.12.12) @aws-sdk/client-cognito-identity: ^3.54.1 => 3.54.1 (3.6.1) @aws-sdk/client-kinesis: ^3.54.1 => 3.54.1 (3.6.1) @aws-sdk/client-location: ^3.54.1 => 3.54.1 (3.48.0) @aws-sdk/credential-provider-cognito-identity: ^3.54.1 => 3.54.1 (3.6.1) @aws-sdk/credential-providers: ^3.54.1 => 3.54.1 @aws-sdk/property-provider: ^3.54.1 => 3.54.1 (3.6.1, 3.47.2, 3.171.0) @babel/core: ^7.12.9 => 7.16.12 (7.12.3) @babel/plugin-proposal-class-properties: ^7.16.7 => 7.16.7 @babel/plugin-proposal-logical-assignment-operators: ^7.16.7 => 7.16.7 @babel/plugin-syntax-class-properties: ^7.12.13 => 7.12.13 @babel/plugin-syntax-dynamic-import: ^7.8.3 => 7.8.3 @babel/preset-typescript: ^7.16.7 => 7.16.7 @babel/runtime: ^7.12.5 => 7.16.7 (7.19.0) @googlemaps/google-maps-services-js: ^3.3.6 => 3.3.6 @mapbox/mapbox-gl-style-spec: 13.18.0-dev @react-native-async-storage/async-storage: ^1.15.15 => 1.15.17 @react-native-community/eslint-config: ^1.1.0 => 1.1.0 @react-native-community/geolocation: ^2.0.2 => 2.0.2 @react-native-community/netinfo: ^7.1.7 => 7.1.7 @sentry/react: ^6.16.1 => 6.19.7 @sentry/serverless: ^6.16.1 => 6.19.7 @sentry/tracing: ^6.16.1 => 6.19.7 @sentry/webpack-plugin: ^1.18.3 => 1.18.4 @stripe/react-stripe-js: ^1.7.0 => 1.7.0 @stripe/stripe-js: ^1.22.0 => 1.22.0 @stripe/stripe-react-native: 0.2.2 => 0.2.2 @types/aws-lambda: ^8.10.90 => 8.10.91 @types/aws-sdk: ^2.7.0 => 2.7.0 @types/clone-deep: ^4.0.1 => 4.0.1 @types/color: ^3.0.3 => 3.0.3 @types/google-libphonenumber: ^7.4.23 => 7.4.23 @types/jest: ^27.4.0 => 27.4.0 @types/jest-in-case: ^1.0.5 => 1.0.5 @types/luxon: ^2.3.0 => 2.4.0 @types/mapbox-gl: ^2.6.0 => 2.6.0 @types/node: ^17.0.10 => 17.0.10 (14.18.9, 18.8.2) @types/node-fetch: ^2.6.1 => 2.6.1 (2.5.12) @types/nodemailer: ^6.4.4 => 6.4.4 @types/pino: ^7.0.4 => 7.0.5 @types/pretty: ^2.0.1 => 2.0.1 @types/react: ^17.0.38 => 17.0.38 @types/react-dom: ^17.0.11 => 17.0.11 @types/react-native: 0.66.12 => 0.66.12 (0.66.15) @types/react-native-vector-icons: ^6.4.10 => 6.4.10 @types/react-router-dom: ^5.3.2 => 5.3.3 @types/react-router-native: ^5.1.2 => 5.1.3 @types/stripe-v3: ^3.1.26 => 3.1.26 @types/uuid: ^8.3.4 => 8.3.4 @types/validator: ^13.7.1 => 13.7.1 @typescript-eslint/eslint-plugin: ^4.15.1 => 4.33.0 (2.34.0) @typescript-eslint/parser: ^4.15.1 => 4.33.0 (2.34.0) HelloWorld: 0.0.1 amazon-cognito-identity-js: ^5.2.4 => 5.2.4 (5.2.10) apollo: ^2.34.0 => 2.34.0 aws-amplify: ^4.3.37 => 4.3.37 aws-appsync-auth-link: ^3.0.7 => 3.0.7 axios: ^0.26.1 => 0.26.1 (0.26.0, 0.21.4, 0.24.0) babel-eslint: ^10.1.0 => 10.1.0 babel-loader: 8.1.0 => 8.1.0 cheerio: ^1.0.0-rc.10 => 1.0.0-rc.10 clone-deep: ^4.0.1 => 4.0.1 color: ^4.2.3 => 4.2.3 (3.2.1) core-js: 2 => 2.6.12 (3.21.0, 3.20.3) date-fns: ^2.28.0 => 2.28.0 (1.30.1) email-addresses: ^5.0.0 => 5.0.0 eslint: ^7.20.0 => 7.32.0 eslint-config-airbnb: ^18.2.1 => 18.2.1 eslint-config-airbnb-typescript: ^12.3.1 => 12.3.1 eslint-config-prettier: ^7.2.0 => 7.2.0 (6.15.0) eslint-plugin-import: ^2.22.1 => 2.25.4 eslint-plugin-jest: ^24.1.5 => 24.7.0 (22.4.1) eslint-plugin-jsx-a11y: ^6.4.1 => 6.5.1 eslint-plugin-prettier: ^3.3.1 => 3.4.1 (3.1.2) eslint-plugin-react: ^7.22.0 => 7.28.0 (7.19.0) eslint-plugin-react-hooks: ^4.2.0 => 4.3.0 (3.0.0) events: ^3.3.0 => 3.3.0 (1.1.1) exceljs: ^4.3.0 => 4.3.0 fast-deep-equal: ^3.1.3 => 3.1.3 fuse.js: ^6.5.3 => 6.5.3 geolib: ^3.3.3 => 3.3.3 google-libphonenumber: ^3.2.28 => 3.2.28 hermes-inspector-msggen: 1.0.0 immutability-helper: ^3.1.1 => 3.1.1 import-sort-style-module: ^6.0.0 => 6.0.0 jest: 26.6.0 => 26.6.0 jest-in-case: ^1.0.2 => 1.0.2 js-sha256: ^0.9.0 => 0.9.0 json2csv: ^5.0.6 => 5.0.6 khshared: link:./shared => 0.0.1 luxon: ^2.3.0 => 2.3.0 mapbox-gl: 1.13.2 => 1.13.2 metro-react-native-babel-preset: ^0.66.2 => 0.66.2 node-fetch: 2.6.7 => 2.6.7 nodemailer: ^6.7.2 => 6.7.2 nullthrows: ^1.1.1 => 1.1.1 pdf-lib: ^1.17.1 => 1.17.1 pino: ^7.6.3 => 7.6.4 prettier: 2.2.1 => 2.2.1 (2.5.1) prettier-plugin-import-sort: ^0.0.7 => 0.0.7 pretty: ^2.0.0 => 2.0.0 react: ^17.0.2 => 17.0.2 react-app-rewired: ^2.1.11 => 2.1.11 react-dom: ^17.0.2 => 17.0.2 react-native: ^0.66.4 => 0.66.4 react-native-config: ^1.4.5 => 1.4.5 react-native-document-picker: ^7.1.3 => 7.1.3 react-native-encrypted-storage: ^4.0.2 => 4.0.2 react-native-file-viewer: ^2.1.5 => 2.1.5 react-native-geolocation-service: ^5.2.0 => 5.2.0 react-native-get-random-values: ^1.7.2 => 1.7.2 react-native-image-picker: ^4.7.3 => 4.7.3 react-native-image-resizer: ^1.4.5 => 1.4.5 react-native-keychain: ^8.0.0 => 8.0.0 react-native-maps: ^0.30.1 => 0.30.1 react-native-mask-input: ^1.1.0 => 1.1.0 react-native-paper: 4.11.2 => 4.11.2 react-native-touch-id: ^4.4.1 => 4.4.1 react-native-url-polyfill: ^1.3.0 => 1.3.0 react-native-vector-icons: ^9.0.0 => 9.0.0 react-native-web: ^0.17.5 => 0.17.5 react-native-webview: ^11.16.0 => 11.17.0 react-router: ^5.2.1 => 5.2.1 react-router-dom: ^5.2.1 => 5.3.0 react-router-native: ^5.2.1 => 5.2.1 react-scripts: ^4.0.3 => 4.0.3 react-test-renderer: 17.0.2 => 17.0.2 stripe: ^8.197.0 => 8.199.0 transport: 0.0.1 ts-jest: ^26.5.6 => 26.5.6 ts-loader: ^9.2.6 => 9.2.6 ts-node: ^10.7.0 => 10.7.0 (9.1.1) typescript: 4.2.4 => 4.2.4 (3.9.10) uuid: ^8.3.2 => 8.3.2 (3.4.0, 3.3.2, 7.0.3) validator: ^13.7.0 => 13.7.0 npmGlobalPackages: @aws-amplify/cli: 9.2.1 @vercel/ncc: 0.34.0 corepack: 0.12.1 graphql-ttl-transformer: 2.0.0 npm: 8.19.2 ```

Describe the bug

Since I installed Amplify version 10.0.0, when running amplify api gql-compile, I've seen the following warning:

Screen Shot 2022-10-12 at 5 17 50 PM

In our model, a Coordinator has a primary key of type id: ID! that we use when running the autogenerated updateCoordinator function to update other fields. Can you clarify why an updateCoordinator, which needs the id to identify the record to update and that does not, in fact, mutate the id, still shows the warning? I cannot remove the permission to update otherwise I lose the possibility to update.

Expected behavior

A primary key field should be expected to be used to perform update mutations on a model and it shouldn't show the warning.

Reproduction steps

  1. Install Amplify CLI: npm install -g @aws-amplify/cli@10.2.1
  2. Init your environment with amplify init
  3. Create a GraphQL schema with a type such as
    type Coordinator @model @auth(
    rules: [
      { allow: owner, ownerField: "id" }
    ]) {
    id: ID!
    something: String
    }
  4. Run amplify api gql-compile and you should see the warning

Code Snippet

// Put your code below this line.

Log output

``` // Put your logs below this line ⚠️ WARNING: owners may reassign ownership for the following model(s) and role(s): Coordinator: [id]. If this is not intentional, you may want to apply field-level authorization rules to these fields. To read more: https://docs.amplify.aws/cli/graphql/authorization-rules/#per-user--owner-based-data-access. ```

aws-exports.js

No response

Manual configuration

No response

Additional configuration

No response

Mobile Device

No response

Mobile Operating System

No response

Mobile Browser

No response

Mobile Browser Version

No response

Additional information and screenshots

No response

chrisbonifacio commented 2 years ago

Hi @AlessioVallero 👋 thanks for raising this issue.

This is just a warning that can be ignored if you're okay with owners having the ability to update the id field. The problem is simply that the id field is acting as two important mechanisms here. It is both the unique identifier for the record and the field that determines ownership. So, the value of id in this case is probably the user's cognito sub::username. Please correct me if I'm wrong, but that's usually how the owner auth rule works.

Now, if you want to get rid of the warning because you don't want the owner of a Coordinator record to be able to update the id field, thus potentially re-assigning ownership, you can protect the id with a field level auth rule granting them only create and read permissions instead. They should still be able to update the record, just not the id. The other fields inherit the model level auth rule.

type Coordinator @model @auth(rules: [{ allow: owner, ownerField: "id" }]) {
  id: ID!
    @auth(
      rules: [{ allow: owner, ownerField: "id", operations: [create, read] }]
    )
  something: String
}

I should note that when using field level auth rules, you have to either apply field-level authorization rules to all required fields where all rules have read access ["id"], make those fields nullable, or disable subscriptions for "Coordinator" (setting level to off or public).

Let me know if that helps!

AlessioVallero commented 2 years ago

Hi @AlessioVallero 👋 thanks for raising this issue.

This is just a warning that can be ignored if you're okay with owners having the ability to update the id field. The problem is simply that the id field is acting as two important mechanisms here. It is both the unique identifier for the record and the field that determines ownership. So, the value of id in this case is probably the user's cognito sub::username. Please correct me if I'm wrong, but that's usually how the owner auth rule works.

Now, if you want to get rid of the warning because you don't want the owner of a Coordinator record to be able to update the id field, thus potentially re-assigning ownership, you can protect the id with a field level auth rule granting them only create and read permissions instead. They should still be able to update the record, just not the id. The other fields inherit the model level auth rule.


type Coordinator @model @auth(rules: [{ allow: owner, ownerField: "id" }]) {

  id: ID!

    @auth(

      rules: [{ allow: owner, ownerField: "id", operations: [create, read] }]

    )

  something: String

}

I should note that when using field level auth rules, you have to either apply field-level authorization rules to all required fields where all rules have read access ["id"], make those fields nullable, or disable subscriptions for "Coordinator" (setting level to off or public).

Let me know if that helps!

Thank you @chrisbonifacio, that helps and I appreciate your quick response I guess my only question is the following: using the field-level authorization exactly as you just shared, I'm unable to perform an updateCoordinator({ id: <the_id>, something: "Hello world"}). The point is that id is needed in order to invoke updateCoordinator. How can I still invoke the function without getting an access denied from using the id?

chrisbonifacio commented 2 years ago

Oh, I see, the id is a required part of the UpdateCoordinatorInput so makes sense. So, I think there are two options - either you ignore the warning as it's just pointing out a potentially unwanted permission (allowing users to re-assign ownership). This might be the only option if your schema is already in production. OR - if your schema isn't in production yet - you'd have to handle ownership differently by using a field other than id.

Do you need the id field to act as both the identifier and the owner field? If not, you could create a separate owner field that is protected as I described and doesn't need to be included in your input variables. But we'd be doing all this to simply avoid the warning, which shouldn't really affect your ability to perform updates on a record.

0x24a537r9 commented 2 years ago

Hey Chris—appreciate the quick response as well. I think it's worth pointing out that an update field-level permission is inherently nonsensical on a primary key field (as id is in this case, which yes, you correctly deduced we use to store the user's Cognito sub). That is, insofar as the primary key field of the mutation input specifies which record we want to update, it's not possible to use that same field in the mutation to specify a different value, since a field can only have one value (instead one must delete and recreate the record). In short, ignoring auth rules completely, it's not possible to reassign ownership of a model if the owner field is also the primary key because it's not possible to update the primary key of a model.

For that reason, I'd still suggest that this particular arrangement is safe, and thus the warning shouldn't fire.

(As an aside, I definitely appreciate the Amplify team going the extra step to help devs identify nonobvious security risks like unintentional ownership reassignability! While it may not be correct in this instance, it's been super helpful for locking down our other models.)

alharris-at commented 2 years ago

Hi @0x24a537r9, What you're describing here seems sound, and should be a minor update to this warning method https://github.com/aws-amplify/amplify-category-api/blob/main/packages/amplify-graphql-auth-transformer/src/utils/warnings.ts#L32

For the comment you'd made on that related PR, I'd anticipated that if your primaryKey and ownerField collided you were getting this issue around making the id required for updates, etc, but I think I misunderstood slightly. It sounds like the issue only arises when when you add the field level auth though.

My recommendation in the docs was focused around separating your PK and the owner info, potentially adding a secondary index to query by owner if necessary, e.g.

type User @model @auth(rules:[{ allow: owner, ownerField: "owner"}]) {
    owner: String @index(name: "byOwner", queryField: "userByOwner")
    additionalAttributesBecauseCognitoWontAllowItNatively: String 
}