@auth does not work for update mutations with owner and group combo

[hisham]

Describe the bug
If a @model has @auth rules of "allow: owner" and "allow: groups, groups: ["Manager"], mutations: [update]", updates won't actually work when a user belonging to the Manager group performs an update mutation. Instead you get following dynamo error: The conditional request failed (Service: AmazonDynamoDBv2; Status Code: 400; Error Code: ConditionalCheckFailedException

To Reproduce
Steps to reproduce the behavior:

Create model in graphql as follows:

type Task
  @model
  @auth(rules: [
  {allow: owner},
  {allow: groups, groups: ["Manager"], mutations: [update], queries: [get, list]}
])
  {
    id: ID!
    title: String!
    description: String
    status: String
  }

Then as a user belonging to the Manager group try to do an update mutation, you'll get the error.

Expected behavior
If a user belongs to an authorized group for updates then owner check should be skipped.

Additional context

The reason this bug is happening is because the ownership condition is appended to the dynamodb query on updates regardless of what group the user belongs to. To fix, static group authorization code on the vtl should run first, then ownership condition should only be added if $isAuthorized is false. Actually the more I look at the resolver code, even the actual owner (who may not belong to Managers group) wont be able to update the model. $isAuthorized is set to true only if user belongs to the defined group. So it seems this scenario of combo owner and groups @auth rule was not tested and is not currently supported.

This is the start of Mutation.updateTask.request:

## START: Prepare Ownership Condition. **
#if( !$authCondition )
  #set( $authCondition = {
  "expression": "#owner = :username",
  "expressionNames": {
      "#owner": "owner"
  },
  "expressionValues": {
      ":username": {
          "S": "$ctx.identity.username"
    }
  }
} )
#else
  #set( $authCondition.expression = "($authCondition.expression) AND #owner = :username" )
$util.qr($authCondition.expressionNames.put("#owner", "owner"))
$util.qr($authCondition.expressionValues.put(":username", { "S": "$ctx.identity.username"}))
#end
## END: Prepare Ownership Condition. **

## START: Static Group Authorization. **
#set( $userGroups = $ctx.identity.claims.get("cognito:groups") )
#set( $allowedGroups = ["Manager"] )
#set($isAuthorized = $util.defaultIfNull($isAuthorized, false))
#foreach( $userGroup in $userGroups )
  #foreach( $allowedGroup in $allowedGroups )
    #if( $allowedGroup == $userGroup )
      #set( $isAuthorized = true )
    #end
  #end
#end
## END: Static Group Authorization. **

## START: Throw if Unauthorized. **
#if( !$isAuthorized )
$util.unauthorized()
#end
## END: Throw if Unauthorized. **

#if( $authCondition )
  #set( $condition = $authCondition )

And this is the dynamo query:

{
  "version": "2017-02-28",
  "operation": "UpdateItem",
  "key": {
      "id": {
          "S": "$context.args.input.id"
    }
  },
  "update": $util.toJson($update),
  "condition": $util.toJson($condition)
}

[hisham]

To fix, I modified the update resolver code like below. Basically I changed $isAuthorized flag to $isGroupAuthorized, removed the $util.unauthorized(), and instead only add the owner $authcondition if $isGroupAuthorized is false.

## START: Static Group Authorization. **
#set( $userGroups = $ctx.identity.claims.get("cognito:groups") )
#set( $allowedGroups = ["MyGroup"] )
## This generated code is modified to workaround https://github.com/aws-amplify/amplify-cli/issues/305 bug
##set($isAuthorized = $util.defaultIfNull($isAuthorized, false))
#set($isGroupAuthorized = $util.defaultIfNull($isGroupAuthorized, false))
#foreach( $userGroup in $userGroups )
  #foreach( $allowedGroup in $allowedGroups )
    #if( $allowedGroup == $userGroup )
##      #set( $isAuthorized = true )
      #set( $isGroupAuthorized = true )
    #end
  #end
#end
## END: Static Group Authorization. **

## START: Throw if Unauthorized. **
##if( !$isAuthorized )
##$util.unauthorized()
###end
## END: Throw if Unauthorized. **
##if( $authCondition )
#if( !$isGroupAuthorized && $authCondition )
  #set( $condition = $authCondition ) 

[mikeparisstuff]

@hisham This has been fixed in #285.

The above PR revamps the auth logic so it is more efficient (and now correct) for complex auth rules that involve multiple rules on the same operation.

[hisham]

Great, thank you @mikeparisstuff for this and the rest of your work. :)

[github-actions]

This issue has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels for those types of questions.