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

Dynamic group auth cannot share a field with an index #61

Closed ChapterSevenSeeds closed 9 months ago

ChapterSevenSeeds commented 2 years ago

Before opening, please confirm:

How did you install the Amplify CLI?

npm i -g @aws-amplify/cli

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

14.19.1

Amplify CLI Version

7.6.26

What operating system are you using?

Windows 11

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

No manual changes as far as I know.

Amplify Categories

api

Amplify Commands

codegen, publish, push

Describe the bug

Given a simple tenant/user authorization based workflow with AWS Cognito, consider the following schema:

type Todo
@model
@auth(rules: [{allow: groups, groupsField: "tenant", operations: [create, read, update, delete]}])
{
  id: ID!
  tenant: ID! @index(name: "todosByTenant", sortKeyFields: ["title"], queryField: "todosByTenant")
  title: String!
  body: String!
}

Both amplify api gql-compile and amplify codegen work fine. A subsequent amplify push also works. The Todo table gets created with the proper secondary index. However, when querying the todosByTenant index, authorization forever fails. This is what my query looks like:

query GetTodos {
  todosByTenant(tenant: "my-tenant") {
    items {
      id
    }
  }
}

The query errors out with the following response message: "Not Authorized to access todosByTenant on type ModelTodoConnection". I have found that a workaround is to write the schema like this:

type Todo
@model
@auth(rules: [{allow: groups, groupsField: "tenant", operations: [create, read, update, delete]}])
{
  id: ID!
  tenant: ID!
  tenantName: String! @index(name: "todosByTenant", sortKeyFields: ["name"], queryField: "todosByTenant")
  title: String!
  body: String!
}

Only when the schema is written like this does the same query above return the Todos associated with my tenant. This was not an issue with GraphQL Transformer V1. The issue also occurs if the groupsField for the @auth directive is the primary key of the table. Is this behavior expected? In case you're wondering, the reason we don't just use the listTodos query is because, if the table is large enough with sparse tenants, we may have to paginate through multiple empty responses in order to get at least one item back from the table.

Expected behavior

Querying by the todosByTenant index in the first schema example shown in the bug description does not cause an unauthorized error. It should return the todos associated with that tenant.

Reproduction steps

  1. Start a new Amplify project.
  2. Add some basic Cognito auth to said project.
  3. Create a GraphQL API for the project.
  4. Place the first example schema in the bug description into your schema.
  5. Push all changes to the cloud.
  6. Attempt to query by the todosByTenant index.

GraphQL schema(s)

type Todo
@model
@auth(rules: [{allow: groups, groupsField: "tenant", operations: [create, read, update, delete]}])
{
  id: ID!
  tenant: ID! @index(name: "todosByTenant", sortKeyFields: ["title"], queryField: "todosByTenant")
  title: String!
  body: String!
}

Log output

Not Authorized to access todosByTenant on type ModelTodoConnection

Additional information

Each Cognito user has a custom field called custom:tenant which specifies what tenant to which the user pertains. With this, we also have a pre token generation lambda function integrated into Cognito which will place this tenant and the user's role into the cognito groups portion of the access token. The payload of the JWT token that is passed along to Appsync calls is decoded like so (with some redactions):

{
  "cognito:groups": [
    "my-tenant",
    "Admin"
  ],
  "username": "my-username",
  "exp": 1649343139,
  // ...other JWT token fields
}
jayKayEss commented 2 years ago

I can confirm that this issue persists with Amplify 8.0.1.

This also seems to be a workaround (although it's an obvious security risk since it allows any signed-in user to read the data)

  @auth(
    rules: [
      { allow: private, operations: [read] }
      {
        allow: groups
        groupsField: "tenant"
        operations: [create, update, delete]
      }
    ]
  ) {

This is a serious bug since having an index on a tenant ID field that's also used for @auth rules seems like a very common use case.

ykethan commented 2 years ago

Hello @ChapterSevenSeeds, Thank you for reaching out. Apologies for the delay in a response. I was able to replicate the error in my Amplify application and confirmed the workaround as well. But I noticed that the sortKeyFields uses name field which is a unknown property. I modified the query to use title as sortKeyFields and deployed the API.

I tested the API for Mutation and list queries as well and observed the todosByTenant throws Not Authorized to access todosByTenant on type ModelTodoConnection error.

Marking this as bug.

admangan commented 2 years ago

Just wanted to jump in here and confirm that I have faced this too. It took a while to debug but the only difference was that one of my dynamic groups grouFields was referencing @primaryKey and the other referencing an indexed value.

My solution was to create a duplicate index-free field that the group value is copied into with a lambda trigger and this is what is referenced for dynamic grouping.

Was very frustrating, but ultimately figured it out by looking at the VTL which was way more complicated than the ones not referencing an index.

naedx commented 2 years ago

I'm also interested in a solution to this due to my related problem.

For the moment, I've resorted to creating a field to hold the same information I need . But it requires extra care to make sure the value is being set consistently, everywhere, including resolver overrides just to set this value.

blbigelow commented 2 years ago

My company would really like to upgrade to the latest version of amplify but we are stuck on an old version until this gets worked out.

rachziger commented 2 years ago

Any Update on this?

Gzing commented 2 years ago

Facing this issue on the latest amplify, not sure if there are any workarounds where we do not need to create a separate field and duplicate the data.

olivertang commented 2 years ago

Facing this issue as well - can we please get this fixed soon? We have a workaround that's causing some us to inherit some tech debt

dkwong commented 2 years ago

Facing the same issue as others have, would love an update on ETA.

Kokonaut commented 2 years ago

+1. Also dealing with this issue.

naedx commented 2 years ago

I'm still waiting for it as well.

naedx commented 2 years ago

I'm on version 10.3.0 now and it appears to me that the issue is fixed.

Does anyone else have the same experience with this?

naedx commented 2 years ago

Sorry guys. False alarm. The issue still persists.

:(

MtthwBrwng commented 1 year ago

I'm also having this issue CLI version 10.2.3, current workaround is duplicating a field specifically for tenant.

biller-aivy commented 1 year ago

Any update here?

alharris-at commented 1 year ago

Hi folks, unfortunately no update here. This is currently not prioritized against other items, but if anybody is willing to propose a change, we'd be happy to discuss merging a fix. It's worth noting that authorization changes require some additional internal scrutiny, so if you have a proposal, I'd recommend writing it up and submitting a PR w/ the design before moving onto implementation, so we can circle up internally and review.

hendrickson-tyler commented 11 months ago

We're not seeing this issue anymore using v12.8.2 of the CLI, can somebody verify if this is fixed? Our schema is the following:

type Contact
  @model
  @auth(
    rules: [
      {
        allow: groups
         groupsField: "tenantId"
         operations: [create, read, update, delete]
      }
    ]
  ) {
  id: ID!
  tenantId: ID!
    @index(
      name: "contactsByTenant"
       sortKeyFields: ["lastName"]
       queryField: "contactsByTenant"
    )
  firstName: String!
  lastName: String!
  updatedBy: String!
}

When running the following query, contacts belonging to my user's tenant are returned perfectly fine. No additional field required:

query ContactsByTenant {
  contactsByTenant(tenantId: "development") {
    items {
      id
      tenantId
      firstName
      lastName
    }
  }
}

When I try to query for contacts belonging to another tenant, I get the expected "Unauthorized" error.

Am I missing something or has this actually been fixed?