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

Group claim auth for secondary index query is not consistent with other auth #2311

Open dpilch opened 8 months ago

dpilch commented 8 months ago

How did you install the Amplify CLI?

npm

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

v18.19.0

Amplify CLI Version

12.10.1

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.

No

Describe the bug

All queries with group claim will fail with unauthorized.

There is mismatch in types between the custom claims (list of integers) when matched to a string field. Resulting queries being unauthorized.

#if( !$util.isNull($ctx.args.projectId) )
      #if( $util.isString($ctx.args.projectId) )
        #set( $projectIdCondition = (($projectIdClaim == $ctx.args.projectId) || $ownerClaimsList2.contains($ctx.args.projectId)) )
      #else

Expected behavior

Query should be authorized when appropriate.

Replacing contains with a for loop will fix this issue.

#if( !$util.isNull($ctx.args.projectId) )
      #if( $util.isString($ctx.args.projectId) )
        $util.log.info("Is String")
        #foreach( $ownerClaim in $ownerClaimsList2 )
          #if( $ownerClaim == $ctx.args.projectId )
            #set( $projectIdCondition = true )
            #break
          #end
        #end
        $util.log.info("projectIdCondition {}", $projectIdCondition)
      #else

Reproduction steps

  1. Create a model with auth rule {allow: groups, groupsField: "projectId", groupClaim: "custom:Managing", operations: [create, update, delete, read]}
  2. Login as user with Cognito Id token with custom claim
  3. Attempt a list on the model

Project Identifier

No response

Log output

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

Additional information

The workaround for this issue is to modify the vtl with suggested for loop.

Related Issues:

Before submitting, please confirm:

BuildingFiles commented 8 months ago

Hello, this is Mike from Case ID 170299609101233 who found this issue and the for loop solution.

I am currently manually editing in the for loop for all custom parameters in these secondary index resolvers.

But I was thinking about this issue after my recent vacation and came up with what I think might be the core question for this issue.

In ALL the other resolver auth files the custom parameters all use some form of the $authFiler.add for the actual authentication process. This got me thinking that this may be the reason theirs a issue.

Also the $ctx.identity.clams.get ends with [] which I assume specifies a default as an array.

Like this:

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

Look how small and clean that authentication code is.

But these secondary named index resolvers created by the graphql @index function do not use the authFilter in any way.

Generated by the @index function

projectId: ID! @index(name: "ListFeedById", queryField: "ListFeedById", sortKeyFields: ["updatedAt"])

This instead generates a $ctx.identity.clams.get that ends with null for some reason.

Adds in a bunch of code. A large portion of which we determined never could be executed by the script since the ID! parameter will always be a string.

Additionally, the $authFilter is explicitly set to Null.

This is the original code generated for the custom auth check in the secondary resolvers.

#set( $projectIdClaim = $util.defaultIfNull($ctx.identity.claims.get("custom:Projects"), null) ) #* Why null and not [] like all other esolvers? *#
  #if( !$util.isNull($projectIdClaim) )

    #set( $ownerClaimsList2 = [] )
    #if( $util.isString($projectIdClaim) )
      #if( $util.isList($util.parseJson($projectIdClaim)) )
        #set( $projectIdClaim = $util.parseJson($projectIdClaim) )
      #else
        #set( $projectIdClaim = [$projectIdClaim] )
      #end
    #end
    $util.qr($ownerClaimsList2.addAll($projectIdClaim))
    #if( !$util.isNull($ctx.args.projectId) )
      #if( $util.isString($ctx.args.projectId) ) #* if ID! is always a string the following else code never runs. *#
        #set( $projectIdCondition = (($projectIdClaim == $ctx.args.projectId) || $ownerClaimsList2.contains($ctx.args.projectId)) )
      #else
        #set( $projectIdCondition = ($projectIdClaim == $util.defaultIfNull($ctx.args.projectId.get("eq"), null) || $ownerClaimsList2.contains($util.defaultIfNull($ctx.args.projectId.get("eq"), null))) )
        #if( !$projectIdCondition )
          #set( $entityValues = 0 )
          #foreach( $argEntity in $ctx.args.projectId.get("eq") )
            #if( $ownerClaimsList2.contains($argEntity) )
              #set( $entityValues = $entityValues + 1 )
            #end
          #end
          #if( $entityValues == $ctx.args.projectId.get("eq").size() )
            #set( $projectIdCondition = true )
          #end
        #end
      #end
      #if( $projectIdCondition )
        #set( $isAuthorized = true )
        $util.qr($ctx.stash.put("authFilter", null))
      #end
    #else
      $util.qr($primaryFieldMap.add({ "projectId": { "in": $projectIdClaim } }) )
    #end

So, the ultimate question is why do all other auth resolvers use the filters, but these secondary @index resolvers don't?

If there's some reason the filter method is taboo in this specific auth case. The for loop is still a valid fix. But maybe it would be simpler to just use the authFilter like everything else does?