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

Open Search group claims case sensitive + breaking with hyphens #862

Open kylekirkby opened 1 year ago

kylekirkby commented 1 year ago

Before opening, please confirm:

How did you install the Amplify CLI?

No response

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

No response

Amplify CLI Version

9.1.0

What operating system are you using?

Ubuntu

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

....

Amplify Categories

api

Amplify Commands

add, env, push

Describe the bug

We've got an Open Search instance for searching our resources added via the AppSync GraphQL schema. We had reports that users were not seeing their private resources (only visible to users in specific Cognito groups).

Some groups were working fine and others were not. After intense debugging, I realised that the issue was due to the current SearchResources VTL / Open Search setup not respect case sensitivity / hyphens in the Cognito Group names?!?!

If the user had the Testing Cognito group claim in their JWT, the visibleTo field (listing cognito groups that should be able to view the resource) would not be respected. However, if I changed the field value to use testing and changed their Cognito group membership to include a testing group. It worked! The same goes for any groups that have a dash/hyphen -. Replacing the dashes with underscores (_), fixed the issue. I couldn't find any details on this behaviour in the Open Search documentation or the Amplify documentation, am I missing something here?

My solution, so far, involves modifying Cognito Group names to be lower case and update my slugify calls in my lambda functions to use a _ instead of - as the delimiter. Unless anyone in the Amplify team knows why this is happening and can offer another solution, I'm going to need to write a script to migrate all users over to new Cognito groups and update all my DynamDB table entries that reference the old Cognito group names!


Also, the only way I could get my search query to return items based on an email claim was to update my search resolver with

Query.searchResources.auth.1.req.vtl

  #set( $owner0 = {"match": {"owner": $ctx.identity.claims.get("email")}})

Expected behavior

The Open Search streaming stack should respect hyphens and case sensitivity in Cognito group claims when checking auth rules. If case sensitivity cannot be fixed then the default auth VTL resolvers should lower case any Cognito group claims before checking them in the search results.

#set( $groupClaimsUpper = $util.defaultIfNull($ctx.identity.claims.get("cognito:groups"), ["___xamznone____"]))
  #set( $groupClaimsLower = [] )
  #foreach( $group in $groupClaimsUpper )
    $util.qr($groupClaimsLower.add($util.str.toLower($util.str.toReplace($group, "-", "_"))))
  #end
  #set( $owner0 = {"match": {"owner": $ctx.identity.claims.get("email")}})
  #set( $group1 = {
  "terms_set": {
      "visibleTo": {
          "terms": $groupClaimsLower,
          "minimum_should_match_script": {
              "source": "1"
      }
    }
  }
} )

If the hyphens issue is due to a limitation/requirement of Open Search, there should be some clear documentation in the Auth/API sections of the Amplify docs stating not to creating Cognito group names that contain hyphens.

Reproduction steps

  1. Create a Cognito group with hyphens in them.
  2. Create a AppSync schema with @searchable and Cognito group field auth.
type Resource
    @model
    @searchable
    @auth(
        rules: [
            { allow: public, operations: [read], provider: iam }
            { allow: private, operations: [create, read, update, delete], provider: iam }
            { allow: owner, operations: [read], ownerField: "owner", identityClaim: "email" }
            { allow: groups, groupsField: "visibleTo", operations: [read] }
            { allow: groups, groupsField: "adminGroups", operations: [read] }
            { allow: groups, groupsField: "contributorGroups", operations: [read] }
        ]
    ) {
    """
    Generated automatically.
    """
    id: ID!
    """
    The email of the user that uploaded the resource.
    """
    owner: String
    """
    Contains a list of cognito groups that can view the private resource. This field
    is updated when a Permission or PermissionSet is modified.
    """
    visibleTo: [String]
        @auth(rules: [{ allow: private, operations: [create, read, update, delete], provider: iam }])
}
  1. Attempt to to use the searchResources generated graphql query to pull resources only visible to a specific user (group name must contain hyphens or be camel case etc).
  2. See that no results are returned.

GraphQL schema(s)

```graphql # Put schemas below this line ```

Project Identifier

No response

Log output

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

Additional information

No response

kylekirkby commented 1 year ago

Hi @josefaidt / all,

It was a hassle... but we figured out the issue.

So if you're using group field-level claims + searchable, the current state of the auth resolver that Amplify generates for Open Search is broken.

When Open Search creates your index, it manipulates the data you give to allow it to be more easily searched. This includes lower-casing all text and splitting phrases by a hyphen. Therefore any Cognito groups using a hyphen or uppercase letters will not match on any results found; with the DSL filters provided in the auth resolver. So by simply appending keyword to the field name in the terms_set filters, we were able to get authorisation working correctly by looking at the value streamed to Open Search not that of the manipulated index.

Query.searchResources.auth.1.req.vtl

  #set( $owner0 = {"match": {"owner.keyword": $ctx.identity.claims.get("email")}})
  #set( $group1 = {
  "terms_set": {
      "visibleTo.keyword": {
          "terms": $groupClaims,
          "minimum_should_match_script": {
              "source": "1"
      }
    }
  }
} )
AnilMaktala commented 1 year ago

Hi @kylekirkby, Sorry for the delay. Thank you for raising the issue and for providing a workaround. I was able to replicate the issues using the steps you provided. Therefore, I will label this as a bug that needs further evaluation by the team.

image