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

VTL resolvers not correctly parsing Cognito custom claims that are arrays of values #1345

Open ritxweb opened 1 year ago

ritxweb commented 1 year ago

How did you install the Amplify CLI?

npm

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

v18.13.0

Amplify CLI Version

11.0.0

What operating system are you using?

Windows 10

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

No

Describe the bug

I am creating a custom claim in Cognito in my JWT to add a list of groups that the user belongs to and defines their access to certain resources when querying AppSync. I am using the lambda pre token generation function to add this list (array) of groups (strings) to this custom claim but as already mentioned in this StackOverflow question:

https://stackoverflow.com/questions/49556221/how-to-add-array-values-in-claims-of-idtoken-in-cognito-using-claimstoaddoroverr

it is not possible to add objects like arrays of strings to custom Cognito claims like it works with the cognito:groups claim for instance. Only strings are allowed. The solution then is to JSON.stringify this array and add this to the claim. This works fine.

Now let's say we create a simple model in AppSync that looks like this:

type MyModel @model @auth(rules: [{allow: owner}, {allow: groups, groupsField: "myGroupsField", groupClaim: "myCustomClaim"}]) {
  id: ID!
  myGroupsField: [String!]
}

When pushing this changes to Amplify it automatically generates VTL resolvers for sync and get objects of type MyModel. Here comes the problem. As I have seen, this behaviour of Cognito forcing to stringify the list of groups in my custom claim is correctly observed and treated in the generated VTL resolver function called QuerygetMyModelauth0Function that parses to JSON this claim and transforms it into a list that can be correctly treated by the resolver:

#set( $role1 = $util.defaultIfNull($ctx.identity.claims.get("myCustomClaim"), []) )
#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({ "myGroupsField": { "in": $role1 } }))
#end

But in the resolver called QuerysyncMyModelsauth0Function (and some others) it uses this code:

#foreach( $group in $util.defaultIfNull($ctx.identity.claims.get("myCustomClaim"), []) )
  #if( !$group.isEmpty() )
    $util.qr($authFilter.add({"myGroupsField": { "contains": $group }}))
  #end
#end

Without calling to $util.parseJson with the claim that makes that the variable $group never gets a value so this claim is never checked against the myGroupsField, therefore denying access to the object.

Expected behavior

As Cognito doesn't currently allow to add arrays of strings to custom claims, all the VTL resolvers should parse this claims to correctly read permissions based on lists of groups and allow or deny access.

Reproduction steps

  1. Create a simple model that uses @auth directives with custom Cognito claims
  2. Push the changes to Amplify
  3. Check the generated VTL resolvers

Project Identifier

No response

Log output

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

Additional information

No response

Before submitting, please confirm:

phani-srikar commented 1 year ago

Hi @ritxweb, thanks for contacting us. Given your use-case which is to override the user groups in their claim via a pre-token generation lambda, have you tried using groupOverrideDetails in your lambda's response as mentioned in the docs here:

"groupOverrideDetails": {
     "groupsToOverride": [
          "custom-group-1",
          "custom-group-2"
      ]
  }

You might need to modify your schema to static group based Auth as below:

type MyModel @model @auth(rules: [{allow: owner}, {allow: groups, groups: ["custom-group-1", "custom-group-2"]}]) {
id: ID!
myGroupsField: [String!]
}

Please let us know if this works. Thanks.

alex-vladut commented 1 year ago

I'm also facing this issue with dynamic groups (i.e. custom claims), which has been discussed many times before in other GitHub issues. I think your suggestion won't solve the problem because I want to use dynamic groups and those claims are not always parsed to a proper array. I gave it a try to fix some of these occurrences in this PR, but it didn't receive much attention so far: https://github.com/aws-amplify/amplify-category-api/pull/682. For reference, the issues were described before here https://github.com/aws-amplify/amplify-category-api/issues/132 and https://github.com/aws-amplify/amplify-category-api/issues/110

The problem, as I understand it, is that the transformer doesn't treat those claims in a consistent way: sometimes it's assuming those are strings, other times it assumes it's an array while other times it parses the stringified array accordingly.

ritxweb commented 1 year ago

Of course it works and obviously that is not an option. If I have a variable and high number of groups or if these groups change during the use of the app, I have to regenerate my schema every time? That's the whole idea of custom claims and dynamic group attributes. I don't know why instead of proposing users this let's say not very usefull workaround, you don't fix this clear error on your side that is as easy as replacing this line in your resolver:

foreach( $group in $util.defaultIfNull($ctx.identity.claims.get("myCustomClaim"), []) )

with something similar to what you have in the other function:

set( $role1 = $util.defaultIfNull($ctx.identity.claims.get("myCustomClaim"), []) )

if( $util.isString($role1) )

if( $util.isList($util.parseJson($role1)) )

#set( $role1 = $util.parseJson($role1) )

else

#set( $role1 = [$role1] )

end

end

if( !$role1.isEmpty() )

foreach( $group in $role1 ) )

......

I don't know if you are able to propose this fix/change request but if not I would apreciate that you escalate this issue to someone who can because I'm starting to feel very disappointed with Amplify. I started a project using this technology months ago, trusting AWS, and all these issues are critically slowing down the development. And I don't want this case to fall into oblivion like happened to @alex-vladut or with this other clear issue [ #1172] that has been abandoned for almost two months.

By the way, after correcting this failure in my resolvers with the solution I mentioned before, I started receiving new critical Appsync errors related with the subscriptions to changes on this model. These errors were all pointing to the same line on your resolver functions SubscriptiononCreateMyModelauth0Function, SubscriptiononUpdateMyModelauth0Function and SubscriptiononDeleteMyModelauth0Function:

$util.qr($authGroupRuntimeFilter.add({ "myGroupsField": { "containsAny": $groupClaim2 } }))

That failed everytime that myCustomClaim was empty or doesn't exists and looks like some kind of problem with the "containsAny" operator that again I had to fix on my own by changing this line in the resolver to:

if( !$groupClaim2.isEmpty() )

$util.qr($authGroupRuntimeFilter.add({ "myGroupsField": { "containsAny": $groupClaim2 } }))

end

It looks in general like a problem with using custom claims with group attributes that are arrays of elements. Again I would really apreciate that you escalate these issues to someone actually able to propose these change requests. Right now my program has around 700 resolvers and I can not check and fix all and any of them.

And lastly I would like to point out an error in your documentation page about extensions and operators https://docs.aws.amazon.com/appsync/latest/devguide/extensions.html#extensions-setSubscriptionInvalidationFilter. In the definition of Field: operator, if you select the tab of the operator "containsAny", the example of possible value types says:

{ "fieldName" : "seats", "operator" : "contains", "value" : [10, 15] }

where clearly the "operator" field should be:

"operator" : "containsAny",

This is a minor mistake but it could lead to misunderstandings so please fix it as well.

phani-srikar commented 1 year ago

Hi @ritxweb, thanks for sharing more details. I have prioritized this appropriately and sorry that this has been around for quite sometime. I will bring this up with my team so that this gets picked up in upcoming sprints.

ritxweb commented 1 year ago

Thanks @phani-srikar. No need to apologize but I would really appreciate this to be solved ASAP.

Again I have to point out another error related with custom claims. While I was checking all the resolvers to find out more errors, I found a lot of errors that involve some of the resolvers which names ended in ...auth0Function that are resolvers to get an attribute that is a list of models of another model like for instance the resolver to get myModel2List field in the following model:

type MyModel @model @auth(rules: [{allow: owner}, {allow: groups, groupsField: "id", groupClaim: "myCustomClaim"}]) { id: ID! myModel2List: [MyModel2!] ] @ hasMany }

In this case the resolver would be called MyModelmyModel2Listauth0Function. Well, this resolvers sometimes treat this custom claims properly with the following code:

#set( $groupEntity1 = $util.defaultIfNull($ctx.source.id, null) )
#set( $groupClaim1 = $util.defaultIfNull($ctx.identity.claims.get("myCustomClaim"), []) )
#if( $util.isString($groupClaim1) )
  #if( $util.isList($util.parseJson($groupClaim1)) )
    #set( $groupClaim1 = $util.parseJson($groupClaim1) )
  #else
    #set( $groupClaim1 = [$groupClaim1] )
  #end
#end
#if( $groupClaim1.contains($groupEntity1) )
  #set( $isAuthorized = true )
#end

While in a lot of other resolvers, I don't know why, it uses the following weird code:

#set( $primaryRole1 = $util.defaultIfNull($ctx.identity.claims.get("myCustomClaim"), null) )
#if( !$util.isNull($primaryRole1) )

  #set( $ownerClaimsList1 = [] )
  #if( (!$util.isNull($ctx.args.id)) && (($ctx.args.id == $primaryRole1) || $ownerClaimsList1.contains($ctx.args.id)) )
    #set( $isAuthorized = true )
    $util.qr($ctx.stash.put("authFilter", null))
  #else
    #if( !$isAuthorized && $util.isNull($ctx.stash.authFilter) )
      $util.qr($ctx.args.put("id", $primaryRole1))
      #set( $isAuthorized = true )
    #end
  #end
#end

That fails in the following points:

Sadly I colud not find another pattern to identify those resolvers with these errors more than that they are resolvers for attributes that are lists of models in another model and their names end in ...auth0Function. I hope this helps to find the transformers that behave in such weird way. The solution is as easy as use always the code in the first resolver mentioned.

I would appreciate that this issue remains open till all these errors get fixed.

sundersc commented 1 year ago

Hey @ritxweb - We recently addressed a similar issue with @index queries in PR #1363. I'm looking into the sync resolvers to see if it has a similar problem.

Could you please post your schema? I'm trying to figure out the list of scenarios where it generates incorrect group claim check.

sundersc commented 1 year ago

I see that the problem occurs only when the groups auth field is also part of primary key or GSI (example schema below). PR #1363 should fix the problem with the sync queries as well.

type Todo @model @searchable @auth(rules: [{allow: groups, groupsField: "groups"}]) {
  id: ID! @primaryKey(sortKeyFields: ["groups"])
  name: String!
  description: String
  groups: String!
}
naedx commented 1 year ago

I have also had the issue where some dynamic group claims are parsed into lists and others where they aren't even when they refer to the same groupsField which is of type list. This was frustrating as I've had to add a duplicate field across my entire project that I now have to manage just to avoid the issue where it doesn't parse properly if the groupsField is a key/index field.

I just resorted to patching it myself by overriding the resolvers to get my project moving forward.

I hope to see a durable resolution to this.

ritxweb commented 1 year ago

The schema was mentioned in the first post:

type MyModel @model @auth(rules: [{allow: owner}, {allow: groups, groupsField: "myGroupsField", groupClaim: "myCustomClaim"}]) {
    id: ID!
    myGroupsField: [String!]
}

No, the groups auth field is not part of primary key or GSI. Please look further for the error

unknown1337 commented 1 year ago

Hey @ritxweb - We recently addressed a similar issue with @index queries in PR #1363. I'm looking into the sync resolvers to see if it has a similar problem.

Could you please post your schema? I'm trying to figure out the list of scenarios where it generates incorrect group claim check.

Wow, thanks! Im curious when your PR will be released to the amplify cli :)

ritxweb commented 1 year ago

Hello. Any news about this? Should we abandon Amplify for real projects till it become stable?

naedx commented 1 year ago

This issue still exists in v12.0.3

naedx commented 1 year ago

This issue still exists in v12.7.0. Still suffering.

Related to #110 , #132

A fix was attempted in https://github.com/aws-amplify/amplify-cli/pull/9466 but it seems the solution just missed something.

ritxweb commented 12 months ago

Hello. Any news about this?

naedx commented 9 months ago

I still have this issue on version 12.10.1.

Are there any updates?

ritxweb commented 7 months ago

Hello. Any news about this?