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 77 forks source link

Owner- and group-rules failing in V2 search resolvers #58

Open lseemann opened 2 years ago

lseemann commented 2 years ago

Before opening, please confirm:

How did you install the Amplify CLI?

npm

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

14

Amplify CLI Version

7.6.26

What operating system are you using?

Mac

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

Custom Cognito attributes

Amplify Categories

api

Amplify Commands

Not applicable

Describe the bug

When making a search query with owner- or group rules, the query is failing and not returning any results.

For the ForeignNational model below, a user with a custom:person_id should be able to read any ForeignNational whose relatives field includes the claim.

But here is a user with person_id of 9af7... searching for his relatives and it’s empty. In the same query, I'm doing a get query for a known relative Zdf... to show that the user does indeed have rights to their record..

Screen Shot 2022-04-14 at 9 08 37 AM

Here is the identical query made as a member of the Admins group, showing the expected response:

Screen Shot 2022-04-14 at 9 09 27 AM

In the Query.searchForeignNationals.req.vtl resolver that the CLI generates, I can tell that it is making the following query to OpenSearch:

  "bool": {
    "must": [
      {
        "bool": {
          "should": [
            {
              "terms_set": {
                "companyID.keyword": {
                  "terms": ["___xamznone____"],
                  "minimum_should_match_script": { "source": "1" }
                }
              }
            },
            {
              "terms_set": {
                "id.keyword": {
                  "terms": ["9af7e450-a8c2-4808-bfef-dfb878ba628d"],
                  "minimum_should_match_script": { "source": "1" }
                }
              }
            },
            {
              "terms_set": {
                "relatives": {
                  "terms": ["9af7e450-a8c2-4808-bfef-dfb878ba628d"],
                  "minimum_should_match_script": { "source": "1" }
                }
              }
            },
            {
              "terms_set": {
                "relativeAdmins": {
                  "terms": ["9af7e450-a8c2-4808-bfef-dfb878ba628d"],
                  "minimum_should_match_script": { "source": "1" }
                }
              }
            }
          ]
        }
      },
      { "match": { "relatives": "9af7e450-a8c2-4808-bfef-dfb878ba628d" } }
    ]
  }
}

Perhaps this filter is malformed? I don't know enough about Query DSL to diagnose.

Expected behavior

Search queries should correctly respect the owner and group rules in models.

Reproduction steps

  1. Create model with owner and group auth rules
  2. Search for records employing those rules

GraphQL schema(s)

```graphql # Put schemas below this line type ForeignNational implements Person @model( queries: { get: "getForeignNational", list: null } subscriptions: null ) @searchable @auth( rules: [ { allow: groups groups: ["Admins", "Team", "Employers"] # TODO: Remove employer } { allow: groups groupsField: "companyID" groupClaim: "custom:company_access" operations: [read, update, delete] } { allow: owner ownerField: "id" identityClaim: "custom:person_id" operations: [read, update, delete] } { allow: owner ownerField: "relatives" identityClaim: "custom:person_id" operations: [read] } { allow: owner ownerField: "relativeAdmins" identityClaim: "custom:person_id" operations: [read, update, delete] } ] ) { id: ID! firstname: String relatives: [String!] relativeAdmins: [String!] ... } ```

Log output

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

Additional information

No response

kolodi commented 2 years ago

Here is my use cases and observations.

So I have a model like this:

type Asset
    @model(subscriptions: null)
    @auth(
        rules: [
            # Users of the owner org can do all the operations
            { allow: groups, groupsField: "org_owner" }
            # Users in group specified in "set_name" field will have read only access
            { allow: groups, groupsField: "set_name", operations: [read] }
        ]
    ) {
    id: ID! @primaryKey
    org_owner: String @index(name: "byOrganization", queryField: "assetsByOrganization")
    set_name: String @index(name: "byContentSetName", queryField: "assetsByContentSet")
    ItemName: String @default(value: "No Name")
}

The authorization rules works fine for all the operations.

But

As you can see I set indexes on fields used for the groups. When doing operations with assetsByOrganization and assetsByContentSet there is a problem with implementing the auth rules. That is because the auth rules are implemented as filters in auth resolver. Example for simple listing:

#if( !$isAuthorized )
  #set( $authFilter = [] )
  #set( $role0 = $util.defaultIfNull($ctx.identity.claims.get("cognito:groups"), []) )
  #if( $util.isString($role0) )
    #if( $util.isList($util.parseJson($role0)) )
      #set( $role0 = $util.parseJson($role0) )
    #else
      #set( $role0 = [$role0] )
    #end
  #end
  #if( !$role0.isEmpty() )
    $util.qr($authFilter.add({ "org_owner": { "in": $role0 } }))
  #end
  #set( $role1 = $util.defaultIfNull($ctx.identity.claims.get("cognito:groups"), []) )
  #if( $util.isString($role1) )
    #if( $util.isList($util.parseJson($role1)) )
      #set( $role1 = $util.parseJson($role1) )
    #else
      #set( $role1 = [$role1] )
    #end
  #end
  #if( !$role1.isEmpty() )
    $util.qr($authFilter.add({ "set_name": { "in": $role1 } }))
  #end
  #if( !$authFilter.isEmpty() )
    $util.qr($ctx.stash.put("authFilter", { "or": $authFilter }))
  #end
#end

You can see the auth resolver adding those rules for both org_owner and set_name group fields It results in this filter statement in DynamoDB query (user is in group called std_lib in my case):

{
    "expression": "(contains(:or_0_org_owner_in, #org_owner) OR contains(:or_1_set_name_in, #set_name))",
    "expressionNames": {
        "#org_owner": "org_owner",
        "#set_name": "set_name"
    },
    "expressionValues": {
        ":or_0_org_owner_in": {
        "L": [
            {
            "S": "std_lib"
            }
        ]
        },
        ":or_1_set_name_in": {
        "L": [
            {
            "S": "std_lib"
            }
        ]
        }
    }
}

But for the "indexed" queries, 1 of the group fields become a primary key, which cannot be used in filter expression to DynamoDB. So I have noticed this field is just not being added in auth resolver (generated automatically) which makes the DynamoDB request valid BUT the authorization rules are now broken and you often get not items from DB because now there is no the second part of the filtering expression (there is no "OR" now).

As a workaround I have modified the auth resolver specifically for my assetsByContentSet case:

$util.qr($ctx.stash.put("hasAuth", true))
#set( $isAuthorized = false )
#if( $util.authType() == "User Pool Authorization" )
    #set ($userGroups = $util.defaultIfNull($ctx.identity.claims.get("cognito:groups"), []))
    #set( $authFilter = [] )
    #set ($targetGroup = $ctx.args.set_name)
    #set ($userIsInTargetGroup = $userGroups.contains("${targetGroup}") )
    #if( $userIsInTargetGroup )
        #set( $isAuthorized = true )
    #else
        #if( !$userGroups.isEmpty() )
            $util.qr($authFilter.add({ "org_owner": { "in": $userGroups } }))
        #end
    #end
    #if( !$authFilter.isEmpty() )
        $util.qr($ctx.stash.put("authFilter", { "or": $authFilter }))
    #end
#end
#if( !$isAuthorized && $util.isNull($ctx.stash.authFilter) )
    $util.unauthorized()
#end
$util.toJson({"version":"2018-05-29","payload":{}})

Here I knowthat there will be always a required argument set_name and a user can be in a group with the same name, I can authorize those users right away without any filtering. Otherwise I add the filtering for the second group field org_owner.

I think this approach can be somehow generalized for any group filed (array or string) that happen to be a primary key when generating auth resolvers.

Even before I wen into this issue with indexes, there is another fail case I hit. Initially, I had no indexes but one of my group fields was also a sort key of the table. And there was a similar problem regarding the authorization rules.

lseemann commented 2 years ago

So my search appears to work if I rewrite the resolver to generate match queries instead of terms_set, such as the following instead of the filter pasted above:

{
  "bool": {
    "must": [
      {
        "bool": {
          "should": [
            { "match": { "companyID.keyword": "___xamznone____" } },
            {
              "match": { "id.keyword": "9af7e450-a8c2-4808-bfef-dfb878ba628d" }
            },
            {
              "match": { "relatives": "9af7e450-a8c2-4808-bfef-dfb878ba628d" }
            },
            {
              "match": {
                "relativeAdmins": "9af7e450-a8c2-4808-bfef-dfb878ba628d"
              }
            }
          ]
        }
      },
      { "match": { "relatives": "9af7e450-a8c2-4808-bfef-dfb878ba628d" } }
    ]
  }
}

But something tells me match is going to have unintended consequences here.

josefaidt commented 2 years ago

Hey @lseemann :wave: I was able to successfully reproduce this issue using the following users:

image image

calling from the user:

image

calling from an admin:

image

Marking as a bug 🙂

kylekirkby commented 2 years ago

Hi @alharris-at ,

Any update on this? This has just hit us... A suggested workaround would be great.

Cheers!

Kyle

AaronZyLee commented 1 year ago

This is probably due to the github issue. The relatives and relativeAdmins have hyphens in the value, which were manipulated by open search for search efficiency. The temporary solution is to add .keyword in the query field.

maintainnow commented 2 months ago

Any update on this issue ? we recently enabled searchable and getting similar issue for using groupsField & groupClaim for record access. thanks