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
87 stars 73 forks source link

Field-level using Groups returns null on CREATE/UPDATE calls #2433

Open benkirbycodes opened 2 years ago

benkirbycodes commented 2 years ago

Before opening, please confirm:

JavaScript Framework

React

Amplify APIs

GraphQL API

Amplify Categories

auth, function, api

Environment information

``` # Put output below this line System: OS: macOS 12.0.1 CPU: (8) x64 Apple M1 Memory: 137.84 MB / 16.00 GB Shell: 5.8 - /bin/zsh Binaries: Node: 14.18.1 - /usr/local/bin/node Yarn: 1.22.10 - /usr/local/bin/yarn npm: 8.1.0 - /usr/local/bin/npm Browsers: Chrome: 96.0.4664.55 Firefox Developer Edition: 95.0 Safari: 15.1 npmPackages: @aws-amplify/cli: ^7.3.1 => 7.3.1 @aws-amplify/ui-react: ^1.2.24 => 1.2.24 @babel/eslint-parser: ^7.15.8 => 7.16.3 @date-io/core: ^2.10.7 => 2.11.0 (1.3.13) @date-io/moment: 1.x => 1.3.13 @material-ui/core: ^4.12.3 => 4.12.3 @material-ui/icons: ^4.11.2 => 4.11.2 @material-ui/lab: ^4.0.0-alpha.57 => 4.0.0-alpha.60 @material-ui/pickers: ^3.3.10 => 3.3.10 @mui/x-data-grid: ^4.0.1 => 4.0.2 @reduxjs/toolkit: ^1.6.1 => 1.6.2 @reduxjs/toolkit-query: 1.0.0 @reduxjs/toolkit-query-react: 1.0.0 @testing-library/dom: ^7.29.4 => 7.31.2 @testing-library/jest-dom: ^5.11.9 => 5.15.0 @testing-library/react: ^11.2.5 => 11.2.7 @testing-library/user-event: ^12.7.1 => 12.8.3 @typescript-eslint/eslint-plugin: ^4.0.0 => 4.33.0 @typescript-eslint/parser: ^4.0.0 => 4.33.0 aws-amplify: ^4.3.6 => 4.3.6 babel-eslint: ^10.0.0 => 10.1.0 blob-stream: ^0.1.3 => 0.1.3 chart.js: ^2.9.4 => 2.9.4 core-js: ^3.8.3 => 3.19.1 (2.6.12) deep-equal: ^2.0.5 => 2.0.5 (1.1.1) eslint: ^7.5.0 => 7.32.0 eslint-config-react-app: ^6.0.0 => 6.0.0 eslint-config-standard: ^16.0.3 => 16.0.3 eslint-plugin-flowtype: ^5.2.0 => 5.10.0 eslint-plugin-import: ^2.22.0 => 2.25.3 eslint-plugin-jsx-a11y: ^6.3.1 => 6.5.1 eslint-plugin-node: ^11.1.0 => 11.1.0 eslint-plugin-promise: ^5.1.0 => 5.1.1 eslint-plugin-react: ^7.20.3 => 7.27.0 eslint-plugin-react-hooks: ^4.0.8 => 4.3.0 example: 1.0.0 fetch-mock: ^9.3.1 => 9.11.0 file-saver: ^2.0.5 => 2.0.5 formik: ^2.2.6 => 2.2.9 jsonexport: ^3.2.0 => 3.2.0 material-ui-phone-number: ^2.2.6 => 2.2.6 moment: ^2.29.1 => 2.29.1 new-plugin-package: 1.0.0 node-fetch: ^2.6.1 => 2.6.6 (2.1.2, 1.7.3) pdfkit-browserify: ^0.8.3-R2 => 0.8.3-R2 prettier: ^1.19.1 => 1.19.1 prop-types: ^15.7.2 => 15.7.2 react: ^17.0.1 => 17.0.2 (16.14.0) react-app-polyfill: ^2.0.0 => 2.0.0 react-chartjs-2: ^2.11.1 => 2.11.2 react-cookie: ^4.0.3 => 4.1.1 react-credit-cards: ^0.8.3 => 0.8.3 react-dnd: ^11.1.3 => 11.1.3 react-dnd-html5-backend: ^11.1.3 => 11.1.3 react-dom: ^16.13.1 => 16.14.0 react-draggable: ^4.4.3 => 4.4.4 react-fullstory: ^1.4.0 => 1.4.0 react-ga: ^3.1.2 => 3.3.0 react-material-ui-carousel: ^2.3.5 => 2.3.8 react-recaptcha: ^2.3.10 => 2.3.10 react-redux: ^7.2.0 => 7.2.6 react-router-dom: ^5.1.2 => 5.3.0 react-scripts: ^4.0.2 => 4.0.3 react-sliding-pane: ^7.0.0 => 7.1.0 react-square-payment-form: ^0.7.2 => 0.7.2 react-stack-grid: ^0.7.1 => 0.7.1 redux: ^4.0.5 => 4.1.2 redux-devtools: ^3.5.0 => 3.7.0 redux-devtools-extension: ^2.13.8 => 2.13.9 redux-immutable-state-invariant: ^2.1.0 => 2.1.0 redux-mock-store: ^1.5.4 => 1.5.4 redux-thunk: ^2.3.0 => 2.4.0 reselect: ^4.0.0 => 4.1.2 uuid: ^8.3.2 => 8.3.2 (3.4.0, 3.3.2) yup: ^0.29.3 => 0.29.3 zxcvbn: ^4.4.2 => 4.4.2 npmGlobalPackages: @aws-amplify/cli: 7.3.1 eslint: 8.0.1 gulp-cli: 2.3.0 n: 7.5.0 npm: 8.1.0 stable: 0.1.8 yarn: 1.22.10 ```

Describe the bug

We have fields on a model that are protected by a field-level auth declaration. The declaration checks whether the user is in a group that has permissions to get the field value.

In the results of a GET request, these declarations are respected. In the results of both CREATE and UPDATE requests though, all users receive null for these fields, regardless of their group.

Expected behavior

I would expect that the results of CREATE and UPDATE would contain the same fields as a GET request.

This may be a misunderstanding on my part, would love some further info if so

Reproduction steps

  1. Create a new type in a graphqlAPI with the @model attribute.
  2. Add a top-level auth declaration that checks for a custom:tenant value stored in Cognito (not sure if this is relevant to the issue).
  3. Add a field-level auth declaration that checks for a user's group in order to return the field value.

Code Snippet

type Resource
  @model
  @auth (
    rules: [
      { allow: owner, ownerField: "tenantId", identityClaim: "custom:tenant" }
    ]
  )
{
  id: ID!
  tenantId: ID
  actualHourlyRate: Float @auth(rules: [{ allow: groups, groups: ["AllowedFinancials"] }])
}

Log output

``` // Put your logs below this line ```

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

jo2 commented 2 years ago

I'm facing the same error in my application using vue. But this error can also be replicated using AWS AppSync, so it might not be a problem with this project.

chrisbonifacio commented 2 years ago

@benkirbycodes I got this error from the CLI when I tried pushing with your Resource type.

Screen Shot 2021-12-09 at 12 12 04 AM

Do you still experience this behavior if you do one of the actions mentioned? I just added the auth directive to the required ID field and it allowed me to push.

benkirbycodes commented 2 years ago

@chrisbonifacio We're actually not getting an error on push. Everything builds fine and we actually get the desired behavior for READ reqs. The fields are just always null for CREATE/UPDATE. The fields that we're trying to lockdown via field-level auth are not required, so that solution wouldn't really be workable for us.

chrisbonifacio commented 2 years ago

I wasn't able to reproduce the exact behavior described in this issue but I ran into other issues on the client side.

On the AppSync console, queries and mutations seem to behave as expected.

Creating a Resource returns the data

Screen Shot 2022-01-11 at 12 42 21 PM

So does updating a Resource

Screen Shot 2022-01-11 at 12 44 02 PM

However, trying to create a resource on the client (logged in as the same CognitoUser) results in the "custom:tenant" attribute not being set as the owner. Instead, it seems ___xamznone____ is being set as the tenantId.

Screen Shot 2022-01-11 at 1 18 52 PM

I checked my CognitoUser data and the custom:tenant, of which the value was correctly used as the tenantId by the AppSync console earlier, is present.

Screen Shot 2022-01-11 at 1 22 57 PM

I'm labeling this as a bug for the team to look into further.

Some of the other strange behavior I ran into:

The tenantId for the records created on the client, where the tenantId was set to ___xamznone____ are returned as null in the AppSync console.

Screen Shot 2022-01-11 at 1 26 28 PM

On the client, all records are returned in the response data, but with an Unauthorized error for the records where the tenantId doesn't match

Screen Shot 2022-01-11 at 1 28 54 PM
alharris-at commented 2 years ago

It looks like this issue is related to aws-amplify/amplify-category-api#64

PlusA2M commented 1 year ago

This issue might be caused by the auto-generated VTL template for the field-level auth directive. As a example:

## [Start] Checking for allowed operations which can return this field. **
#set( $operation = $util.defaultIfNull($ctx.source.get("__operation"), null) )
#if( $operation == "Mutation" )
  $util.toJson(null)
#else
  $util.toJson($context.source["fieldName"])
#end
## [End] Checking for allowed operations which can return this field. **

If I understand the code well, it just return whenever it is an Mutation, which is quite weird in this case.

dpilch commented 1 year ago

I believe you are on the right track @PlusA2M. The fix for this issue is likely entirely in https://github.com/aws-amplify/amplify-category-api.

I'll leave this issue open for visibility.

chrisbonifacio commented 4 months ago

Transferring this over to category-api repo as it seems the root cause is in the generated VTL templates