Dynamic Groups authorization check not applied on field level in GraphQL transformer v2

Before opening, please confirm:

[X] I have installed the latest version of the Amplify CLI (see above), and confirmed that the issue still persists.
[X] I have searched for duplicate or closed issues.
[X] I have read the guide for submitting bug reports.
[X] I have done my best to include a minimal, self-contained set of instructions for consistently reproducing the issue.

How did you install the Amplify CLI?

npm install -g @aws-amplify/cli

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

v14.16.0

Amplify CLI Version

7.6.5

What operating system are you using?

MacOS

Amplify Categories

api

Amplify Commands

Not applicable

Describe the bug

As I gave it a try and tested more in depth the authorization rules applied through GraphQL transformer v2, I came across a couple of issues in regard to dynamic groups authorization rules:

1) The dynamic groups based authorization logic doesn't seem to be properly applied on field level and, as a result, the resolver returns null when the user is actually allowed to access this field.

I know there was this related issue https://github.com/aws-amplify/amplify-cli/issues/9222 which was fixed with this PR https://github.com/aws-amplify/amplify-cli/pull/9264. Possibly the changes applied on the query may need to be applied as well on the relationship resolvers as far as I could understand this issue. The generated field level resolver UserMembership.organisation has this authorization section, so as I understand it, it will simply check the claim string, rather than parsing it to be a list as expected:

#if( $util.authType() == "User Pool Authorization" )
  #if( !$isAuthorized )
    #set( $primaryRole0 = $util.defaultIfNull($ctx.identity.claims.get("tenants"), "___xamznone____") )
    #if( (!$util.isNull($ctx.source.organisationId)) && ($ctx.source.organisationId == $primaryRole0) )
      #set( $isAuthorized = true )
      $util.qr($ctx.stash.put("authFilter", null))
    #else
      #if( !$isAuthorized && $util.isNull($ctx.stash.authFilter) )
        $util.qr($ctx.source.put("organisationId", $primaryRole0))
        #set( $isAuthorized = true )
      #end
    #end
  #end
#end

2) The other issue I noticed is that, even though it won't throw an error anymore due to an invalid DynamoDB filter syntax, as it happened before, it doesn't seem to apply any filtering at all and a user is able to retrieve data they shouldn't have access to. What I did was to create 2 Organisations, and a user has a UserMembership entry only for one of the Organsiations. But when querying listOrganisations (the default query automatically generated by Amplify), it will return both organisations. e.g. the JWT token will include a custom claim like this:

{
  "tenants": "[\"04916a5e-1eb6-438a-9f7d-739511c7c186\"]",
  ...
}

but it will return 2 organisations:

{
  "data": {
    "listOrganisations": {
      "items": [
        {
          "id": "04916a5e-1eb6-438a-9f7d-739511c7c186",
          "name": "MyOrg",
          "createdById": "704807c1-e409-449e-82b3-bd1b6e5f2fa1",
          "createdBy": {
            "id": "704807c1-e409-449e-82b3-bd1b6e5f2fa1"
          }
        },
        {
          "id": "30b6011c-5511-44b1-bb6b-3e70ca331fbb",
          "name": "Unauthorized org",
          "createdById": "fe038661-681c-47bf-8305-196c560ddc62",
          "createdBy": {
            "id": "704807c1-e409-449e-82b3-bd1b6e5f2fa1"
          }
        }
      ],
      "nextToken": null,
      "startedAt": null
    }
  }
}

3) There is also an owner authorization rule set on the Organisation and as I created this entity manually, I set a different createdBy ID to it. As you can see in the result above, createdBy.id ends up being set to the user's sub value even though that's different from what createdById is set to and it should probably just return a null value if there is no such User in the database with such ID. A mutation is indeed expected to set the owner to authenticated user's ID, but I think the query should not override it as the item could have been created by a different user.

4) The following query:

  query GetUser($id: ID!) {
    getUser(id: $id) {
      id
      memberships {
        items {
          id
          organisationId
          organisation {
            id
            name
          }
        }
        nextToken
        startedAt
      }
    }
  }

results in this error:

{
  "errors": [
    {
      "path": [
        "getUser",
        "memberships",
        "items",
        0,
        "organisation"
      ],
      "locations": null,
      "message": "Cannot return null for non-nullable type: 'Organisation' within parent 'UserMembership' (/getUser/memberships/items[0]/organisation)"
    }
  ]
}

I turned on CloudWatch Logs on the Appsync API, and I noticed that the $ctx.source.organisationId has a wrong format, by the look of it due to assigning the groups claim value to it (in which case it is understandable why the DynamoDB query won't match any item). This kind of behavior used to work in the previous Transformer version:

{
    "logType": "BeforeMapping",
    "path": [
        "getUser",
        "memberships",
        "items",
        "0",
        "organisation"
    ],
    "fieldName": "organisation",
    "context": {
        "arguments": {},
        "source": {
            "organisationId": "[\"bb8b0912-dfc8-4834-be63-1a7fd34785d2\"]",
            "__typename": "UserMembership",
            ...
        }
...
}

The resolver UserMembership.organisation.req.vtl makes use of this variable:

  $util.qr($GetRequest.put("query", {
  "expression": "#partitionKey = :partitionValue",
  "expressionNames": {
      "#partitionKey": "id"
  },
  "expressionValues": {
      ":partitionValue": $util.parseJson($util.dynamodb.toDynamoDBJson($util.defaultIfNullOrBlank($ctx.source.organisationId, "___xamznone____")))
  }
}))

Let me know please if I can help with any additional information, I've put together a small project here to reproduce this issue in case that helps: https://github.com/alex-vladut/aws-amplify-auth

Expected behavior

1) Should successfully resolve the field 2) Should filter out the items the authenticated user doesn't have access to 3) Should return the owner of an item, if the authenticated user is allowed to access it, or null otherwise 1) Should successfully resolve the field

Reproduction steps

git clone git@github.com:alex-vladut/aws-amplify-auth.git cd aws-amplify-auth. npm install amplify push npm start Create a new organisation by typing a name and clicking "Create organisation" button

After refreshing the page, check the browser's devtools and there should be an error like:

"message": "Cannot return null for non-nullable type: 'Organisation' within parent 'UserMembership' (/listUserMemberships/items[0]/organisation)"

To simulate the other issue, where an organisation should be filtered out due to the user not being authorized to access it, go to AWS console and in DynamoDB create a new Organisation item. Set a random UUID for createdById field so that it won't match the authenticated user.

GraphQL schema(s)

```graphql # Put schemas below this line type User @model(queries: { get: "getUser" }, subscriptions: null) @auth(rules: [{ allow: owner, ownerField: "id" }]) { id: ID! @primaryKey email: AWSEmail! phone: AWSPhone createdAt: AWSDateTime! updatedAt: AWSDateTime! } type Organisation @model(subscriptions: null) @auth( rules: [ { allow: owner, ownerField: "createdById", operations: [create] } { allow: groups groupsField: "id" groupClaim: "tenants" operations: [read, update] } ] ) { id: ID! @primaryKey name: String! createdById: ID createdBy: User @hasOne(fields: ["createdById"]) createdAt: AWSDateTime! updatedAt: AWSDateTime! } type UserMembership @model(subscriptions: null) @auth( rules: [ { allow: groups, groupsField: "organisationId", groupClaim: "tenants" } { allow: private, provider: iam } ] ) { id: ID! @primaryKey organisationId: ID! organisation: Organisation! @hasOne(fields: ["organisationId"]) userId: ID! @index( name: "byUser" queryField: "userMembershipsByUser" sortKeyFields: ["organisationId"] ) createdAt: AWSDateTime! updatedAt: AWSDateTime! } ```

Log output

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

Additional information

No response

Hi @alex-vladut, there are 4 issues here - are all of these still issues for you? I know you had a PR you were working on to address one of these as well. Which one did that map to? Thank you!

Hey @danielleadams thank you for investigating these issues. Yes, I am still facing most of these issues, as far as I'm aware the only fixes in that area were done through this PR https://github.com/aws-amplify/amplify-cli/pull/9466 so the other issues are still present. In the meantime I think I gained a better understanding of the internals of this authorization logic, so maybe I'll give it a try and summarize the cases I came across in a different form with a link to the code where I think the problem is originating from.

Firstly, most of these issues only happen due to Cognito overridden claims expecting to be a String, so we have to stringify list of tenants a user belongs to. e.g. here https://github.com/alex-vladut/aws-amplify-auth/blob/main/amplify/backend/function/awsamplifyauthc2544e60PreTokenGeneration/src/alter-claims.js

In this example below I am expecting the user to be a member of 2 organizations: "first-org" and "second-org". In order for this claim to be attached to the user's JWT token the claim's tenant value will have to be stringified and then, in the authorization rules will have to parse it. And throughout the authorization checks code the custom claims are not parsed into an array - it's either assumed the value is already an array (which I think it is automatically the case for static groups, so it might be a good assumption for that) or it works with it as a string, so it was not a consistent approach as far as I remember digging into the code.

    claimsOverrideDetails: {
      claimsToAddOrOverride: {
        tenants: JSON.stringify(["first-org", "second-org"]),
      },
    },

Use cases:

create, update and delete mutations with dynamic groups authorization checks

This use case was addressed with this PR https://github.com/aws-amplify/amplify-cli/pull/9466, as before it was throwing an Unauthorized error due to the custom claim not being parsed into an array.

queries with dynamic groups authorization groupsField set on an entity field of type string (or rather a single value, so not an array) and not part of an index

e.g. see this test here for an example https://github.com/aws-amplify/amplify-cli/pull/9466/files#diff-c4a64c1111bdde43a6fcd1ee4c6cf66b2183c6fdd569a6e5278e67ec9569dd20R76

    type Post @model @auth(rules: [{allow: groups, groupsField: "group", groupClaim: "tenants"}]) {
        id: ID!
        title: String!
        group: String!
        createdAt: String
        updatedAt: String
    }

In this case the group field in a single value one and not part of an index so the logic dealing with the corresponding authorization logic should happen in this method https://github.com/aws-amplify/amplify-cli/blob/master/packages/amplify-graphql-auth-transformer/src/resolvers/query.ts#L252. This use case is also handled by the PR linked above.

queries with groupsField set on an entity field of type array and not part of an index

e.g.

    type Post @model @auth(rules: [{allow: groups, groupsField: "groups", groupClaim: "tenants"}]) {
        id: ID!
        title: String!
        groups: [String!]
        createdAt: String
        updatedAt: String
    }

Here the groups field is an array, and the corresponding logic is handled in the same method as 2. here https://github.com/aws-amplify/amplify-cli/blob/master/packages/amplify-graphql-auth-transformer/src/resolvers/query.ts#L312. This commit was intended to address this issue https://github.com/aws-amplify/amplify-cli/pull/9535/commits/f5a93463c21698b9878ff72121f27fb48f850efe. So as a preliminary step it would parse the claim into an array rather than assuming it is already an array as it is currently the case.

queries with groupsField set on an index

e.g.

    type Post @model @auth(rules: [{allow: groups, groupsField: "id", groupClaim: "tenants"}]) {
        id: ID!
        title: String!
        createdAt: String
        updatedAt: String
    }

If I remember correctly this must be handled inside this method https://github.com/aws-amplify/amplify-cli/blob/master/packages/amplify-graphql-auth-transformer/src/resolvers/query.ts#L113 where currently it is not parsing the custom claim and instead is treating it as a string hence failing the authorization check.

relationship queries with dynamic groups authentication

It should be this method handling this logic https://github.com/aws-amplify/amplify-cli/blob/fcf9100c778e98e16f0ed695112f2a7251aca06a/packages/amplify-graphql-auth-transformer/src/resolvers/query.ts#L73

incorrectly setting the source ID to custom claim's value

This is the equivalent of points 3. and 4. in the original issue description. It overrides the source ID in the entity and sets it to the custom claim's value so then. when retrieving the relationship entity, it throws an Unauthorized error due to a wrong query condition being built.

It's been a while since I touched this code, so I'm not sure I remember what was the whole reasoning at the time, hope I got it right :). Thanks for your work, let me know if you need any other details and I am happy to discuss. I'm thinking if it's a better idea to push all the changes I have stashed locally to address such issues into the PR I have opened, in case it may provide a starting point and can later be split in multiple PRs if needed.

Thanks for this explanation @alex-vladut! This is really helpful. I will follow up with any questions.

@alex-vladut for Case 2, what is the expected behavior if the claim returns a stringified array?

Hey, I am expecting the claim's value will be parsed into an array and the value will be compared against the group field to decide if the user is allowed to perform a certain action. e.g. a list query will filter out all the items with group's value not included in the parsed array; a get query similarly may return null if the group value of the entity is not included in the array, and will return the entity otherwise.

At a more general level, I think there are 3 possible values the claim used for authorization could have (the current implementation, as I understand it, is mostly concerned with the first 2 cases and doesn't always take into account the 3rd one):

a string, e.g. "bb8b0912-dfc8-4834-be63-1a7fd34785d2"
an array, e.g. ["04916a5e-1eb6-438a-9f7d-739511c7c186"]
a stringified array, e.g. "[\"04916a5e-1eb6-438a-9f7d-739511c7c186\"]" (it should be parsed first into an array). Of course, it would be ideal to not require this case and always have the claims as either a single value or an array, but Cognito is expecting the overridden claims to be a string.

Thanks for looking into it, let me know if that makes sense to you or there is anything I can help with.

Just because @josefaidt mentioned - are the two issues (#9370, aws-amplify/amplify-cli#9883) directly related? The VTL logic seems almost the same...

Straubulous moved this from In progress to Stalled in Bug bash

... argh!! 😫 This issue really breaks my neck! How can we contribute to move this issue forward? It should not stall!

I completely described bug 4 of @alex-vladut https://github.com/aws-amplify/amplify-category-api/issues/132#issuecomment-1129353659 and also where it needs to be resolved here https://github.com/aws-amplify/amplify-category-api/issues/110#issuecomment-1129353246. I'm too bad in writing good vtl code but for someone with some background knowledge this can be fixed in under 2h. As this issue is key for several multi tenancy use cases I hope that this can be fixed soon?! So how can we push this issue up here @alharris-at @Straubulous (as you moved this issue to bug bash?!).

aws-amplify / amplify-category-api