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
82 stars 71 forks source link

It forces us to give read permission to all authorizations.  #2628

Open jewells07 opened 1 month ago

jewells07 commented 1 month ago

How did you install the Amplify CLI?

npm

If applicable, what version of Node.js are you using?

v20.14.0

Amplify CLI Version

12.12.2

What operating system are you using?

Windows

Did you make any manual changes to the cloud resources managed by Amplify? Please describe the changes made.

No manual changes made

Describe the bug

Failed to instantiate data construct
Caused By: When using field-level authorization rules you need to add rules to all of the model's required fields with at least read permissions. Found model "Booking" with required fields ["price"] missing field-level authorization rules.\n\nFor more information visit https://docs.amplify.aws/cli/graphql/authorization-rules/#field-level-authorization-rules

Resolution: See the underlying error message for more details.
 Booking: a
    .model({
      ....
      price: a.float().required(),
      .....
    })
    .authorization((allow) => [
      allow.authenticated().to(['read', 'create']),
      allow.owner().to(['read']),
      allow.group('Admin'),
    ]),

It forces us to give read permission to all authorizations. Above code I need to add read permission to allow.authenticated().to(['read', 'create']).

Expected behavior

 Booking: a
    .model({
      ....
      price: a.float().required(),
      .....
    })
    .authorization((allow) => [
      allow.authenticated().to([ 'create']),
      allow.owner().to(['read']),
      allow.group('Admin'),
    ]),

Only Owner and Admins should read the Booking and all authenticated users

Reproduction steps

  1. create amplify backend
  2. create schema like Booking: a .model({ .... price: a.float().required(), ..... }) .authorization((allow) => [ allow.authenticated().to([ 'create']), allow.owner().to(['read']), allow.group('Admin'), ])
  3. it will show the error image

Project Identifier

No response

Log output

``` # Put your logs below this line ```

Additional information

No response

Before submitting, please confirm:

kekami commented 1 month ago

You are defining price as required, which means it must never be null. But it will be null if someone lacks read permission, hence the conflict.

A solution is to either make price optional, or add a field level permission allowing everyone to read that field.

AnilMaktala commented 1 month ago

Hey @jewells07, Thanks for raising this. We are trying to reproduce the issue. Will you be able to share the full schema?

jewells07 commented 1 month ago

You are defining price as required, which means it must never be null. But it will be null if someone lacks read permission, hence the conflict.

A solution is to either make price optional, or add a field level permission allowing everyone to read that field.

It has required so on create it won't be null but in read why we need it? Example: If I book a flight, why would I need to show it to all users? Similar goes to the message: I should not give read permission of messages to all users

jewells07 commented 1 month ago

Hey @jewells07, Thanks for raising this. We are trying to reproduce the issue. Will you be able to share the full schema?

Booking: a
    .model({
      slug: a
        .string()
        .required()
        .authorization((allow) => [
          allow.authenticated().to(['create', 'read']),
          allow.owner().to(['read']),
          allow.group('Admin').to(['create', 'read']),
        ]),
      price: a.float().required(),
    })
    .authorization((allow) => [
      allow.authenticated().to(['create']),
      allow.owner().to(['read']),
      allow.group('Admin'),
    ]),
AnilMaktala commented 1 month ago

Hey @jewells07, Thanks for sharing the model. Currently, this is not supported. Hence, marking this as a feature request for the team to evaluate further.

naedx commented 3 weeks ago

I'm also interested in this use case being supported.