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

Dead code in GraphQL API's auto-generated resolver VTL template #1747

Closed dorontal closed 1 year ago

dorontal 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

12.2.3

What operating system are you using?

Debian 12

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

I was about to clone a VTL resolver and make changes to it, when I noticed this - no changes have been made yet and nothing has been cloned yet!

Describe the bug

I wanted to clone a VTL resolver and customize it according to https://docs.amplify.aws/cli/graphql/custom-business-logic/#override-amplify-generated-resolvers

Looking at the VTL code, it seems like a large section there that seems important (for authentication) is actually dead code. Before making modifications to this file, I'd like to straighten this issue out please and then if it is fixed work by modifying the more correct VTL template.

The model - in the GraphQL schema - for which this VTL resolver code was generated only has one simple @auth rule to allow the owner full CRUD access:

type Project
  @model(subscriptions: { level: off })
  @auth(
    rules: [
      { allow: owner }
    ]
  ) {
  id: ID!
  owner: ID @index(name: "projectsByOwner", sortKeyFields: ["updatedAt"]) @auth(rules: [{ allow: owner, operations: [create, read, delete] }])
  user: User! @belongsTo(fields: ["owner"])
  title: String!
  visibility: ProjectVisibility!
  license: ProjectLicense!
  description: String
  artist: String
  coverartUrl: String
  parentProjectId: ID @index(name: "projectsByParentProject", sortKeyFields: ["updatedAt"])
  forks: [Project] @hasMany(indexName: "projectsByParentProject", fields: ["id"])
  nPlays: Int
  nTracks: Int
  nForks: Int
  duration: Float
  tracks: [Track] @hasMany(indexName: "tracksByProject", fields: ["id"])
  createdAt: AWSDateTime!
  updatedAt: AWSDateTime!
}

And here is the generated VTL resolver file Project.user.auth.1.req.vtl context:

# [Start] Authorization Steps. **
$util.qr($ctx.stash.put("hasAuth", true))
#set( $isAuthorized = false )
#if( $util.authType() == "IAM Authorization" )
  #if( !$isAuthorized )
    #if( $ctx.identity.userArn == $ctx.stash.unauthRole )
      #set( $isAuthorized = true )
    #end
  #end
#end
#if( $util.authType() == "User Pool Authorization" )
  #set( $isAuthorized = true )
  #if( !$isAuthorized )
    #set( $authFilter = [] )
    #set( $ownerClaim0 = $util.defaultIfNull($ctx.identity.claims.get("sub"), null) )
    #set( $currentClaim1 = $util.defaultIfNull($ctx.identity.claims.get("username"), $util.defaultIfNull($ctx.identity.claims.get("cognito:username"), null)) )
    #if( !$util.isNull($ownerClaim0) && !$util.isNull($currentClaim1) )
      #set( $ownerClaim0 = "$ownerClaim0::$currentClaim1" )
      #if( !$util.isNull($ownerClaim0) )
        $util.qr($authFilter.add({"owner": { "eq": $ownerClaim0 }}))
      #end
    #end
    #set( $role0_0 = $util.defaultIfNull($ctx.identity.claims.get("sub"), null) )
    #if( !$util.isNull($role0_0) )
      $util.qr($authFilter.add({"owner": { "eq": $role0_0 }}))
    #end
    #set( $role0_1 = $util.defaultIfNull($ctx.identity.claims.get("username"), $util.defaultIfNull($ctx.identity.claims.get("cognito:username"), null)) )
    #if( !$util.isNull($role0_1) )
      $util.qr($authFilter.add({"owner": { "eq": $role0_1 }}))
    #end
    #if( !$authFilter.isEmpty() )
      $util.qr($ctx.stash.put("authFilter", { "or": $authFilter }))
    #end
  #end
#end
#if( !$isAuthorized && $util.isNull($ctx.stash.authFilter) )
$util.unauthorized()
#end
$util.toJson({"version":"2018-05-29","payload":{}})
## [End] Authorization Steps. **

You'll notice above that the second #if statement:

#if( $util.authType() == "User Pool Authorization" )
  #set( $isAuthorized = true )
  #if( !$isAuthorized )
    ...

appears to simply skip the entire block (in ... above). This block seems important, for authenticating Cognito User Pool users.

Just wanted to point that out, in hope that this gets fixed and that I can modify a more "correct" VTL template before starting.

Expected behavior

I expected the VTL not to have large blocks of dead code in it. I expected that code to make sense before attempting to modify it for the first time.

Reproduction steps

Just generate a model with similar @auth permissions to the above and look at its auto-generated VTL.

Project Identifier

860b46330dca368b2540f2cdb87a3485

Log output

N/A

Additional information

No response

Before submitting, please confirm:

dorontal commented 1 year ago

Just realized this is not the VTL template I needed to modify - this VTL template is just for one field of the Project model (the field named user) - leaving this issue open just in case you find the dead code to be something that needs to be taken care of.

dorontal commented 1 year ago

Also I have no idea how my username was able to add the pending-triage label - I did not add that label myself. I don't believe I have the authority to add labels to issues here and did not have the intention to add any labels, or if this happened via something I did then I am not able to reverse it, apologies.

dpilch commented 1 year ago

@dorontal I was not able to reproduce this behavior myself, however this does look like a bug.

Would you be able to provide your User schema as well. Or use amplify diagnose --send-report and comment your project identifier to provide additional details. https://docs.amplify.aws/cli/reference/diagnose/

P.S. pending-triage is automatically added to all new issues opened.

dorontal commented 1 year ago

@dpilch thanks for your response, which I just saw now! Here is the project ID after amplify diagnose --send-report for the schema repored here: 860b46330dca368b2540f2cdb87a3485

dpilch commented 1 year ago

This is actually expected behavior. The resolver generation could be improved to remove the dead section, but the end logic is correct.

https://github.com/aws-amplify/amplify-category-api/blob/07ac802ad3e346b18b6a4c4fd9777009771b1fc3/packages/amplify-graphql-auth-transformer/src/resolvers/mutation.delete.ts#L96-L99

This is because you have { allow: private, operations: [read] } on your User model that is connected via user: User! @belongsTo(fields: ["owner"]) in Project model. Therefore the user field on the Project models does not need additional auth checks.