GraphQL - Roles defined without read action returns error

[severussundar]

When a role is defined without read action, mutations performed in the context of that role, returns an error response. However, at the database level, the mutation(update,insert, etc. action) succeeds.

Repro steps:

1. In the config file available in the main branch, remove the read action for the Anonymous role
2. Execute the following mutation request in the context of Anonymous role

mutation {
     updatebook(id: 1, item: {title: "Harry Potter and the Goblet of Fire"}){
          id
          title
     }
}

Config file:

{
  "$schema": "https://github.com/Azure/data-api-builder/releases/download/vmajor.minor.patch/dab.draft.schema.json",
  "data-source": {
    "database-type": "mssql",
    "connection-string": "...",
    "options": {
      "set-session-context": false
    }
  },
  "runtime": {
    "rest": {
      "enabled": true,
      "path": "/api"
    },
    "graphql": {
      "enabled": true,
      "path": "/graphql",
      "allow-introspection": true
    },
    "host": {
      "cors": {
        "origins": [],
        "allow-credentials": false
      },
      "authentication": {
        "provider": "StaticWebApps"
      },
      "mode": "development"
    }
  },
  "entities": {
    "Book": {
      "source": {
        "object": "books",
        "type": "table"
      },
      "graphql": {
        "enabled": true,
        "type": {
          "singular": "book",
          "plural": "books"
        }
      },
      "rest": {
        "enabled": true
      },
      "permissions": [
        {
          "role": "anonymous",
          "actions": [
            {
              "action": "create"
            },
            {
              "action": "update"
            },
            {
              "action": "delete"
            }
          ]
        },
        {
          "role": "authenticated",
          "actions": [
            {
              "action": "create"
            },
            {
              "action": "read"
            },
            {
              "action": "update"
            },
            {
              "action": "delete"
            }
          ]
        }
      ]
    }
  }
}

The mutation was executed in the context of Anonymous role.

In these requests,
a) The database operation goes through when the corresponding permission is setup (create action for a create mutation, update action for update mutation, etc.)
b) The lack of read action causes the authz error to be thrown.

Note We observe this behavior only in SQL DB types. With cosmos, even when read permission is not setup, valid response (response with values for fields requested in the selection set) is returned.

Tasks

Beta Give feedback

1. GraphQL - Roles defined without read action returns valid data #1603
   bug cosmos graphql hacktoberfest +4
   

[severussundar]

In this case, the database operation succeeds (the update/insert/delete goes through without any issues), but we return error since the read action is not defined for the role. This is ambiguous as it is natural to conclude that the update/insert/delete did not go through since error is being returned.

Note: We don't see this behavior with Cosmos. Even if read action is not defined for the role, valid response (response with values for the fields specified in the selection set) is returned.

To make the behavior more consistent, we could do one of the following things.
a) Check if read action is defined for the role and if not defined, the update/insert/delete operation at the database layer need not be executed (The AuthZ error can be thrown before executing the update/insert/delete database query). So, this will force users to setup read action in addition to the create/update/delete for that role, thereby preventing this inconsistent behavior. Or a slight varation of this could be to allow when the mutation when only __typename is present in the selection set. When any of the entity fields are present, then the mutation request can be errored out.

b) The AuthZ checks before executing the select statement as part of the mutation can be relaxed to validate if the corresponding mutation action (Ex: update action for an update mutation, delete action for delete mutation and create action for a create mutation) is setup and not require the read action to be setup.
At the moment, the AuthZ checks for REST requests work this way.
For ex: for a given role, if only create, update and delete actions are setup, the PATCH request works fine and returns a response with updated/created item details.
c) The error message can be more explicit to state that the database operation did succeed, just that the lack of read action leads to authz error response.

@yorek @JerryNixon @Aniruddh25 @seantleonard @sajeetharan Please kindly share your thoughts

[seantleonard]

We'll need to see if HotChocolate supports grouping the update/read-result to be protected by the runtime config defined permissions for the update operation, out of the box.

Since the selection set is a read operation, HotChocolate would enforce the authorization rules we set on the Object Type definition in the GraphQL schema for read operations.

[JerryNixon]

Desired behavior

If READ is not supported, rewrite the select to primary keys only.

[Aniruddh25]

@JerryNixon, shouldnt the select be allowed to all the fields that are allowed for the mutation: e.g. title and id in this case, not just the primary key?

[aaronpowell]

From a GraphQL standpoint, the behaviour of SQL backends is correct while Cosmos is incorrect, based on the auth config provided the anonymous user is allows to write but it can't read data back out, and a mutation is a two-part process, mutate the database then read the database.

In a custom GraphQL server where you aren't doing one-to-one data mapping a mutation doesn't have to represent a data construct in our backend, it's indicating a concept that you're performing. Using a trivia game example, you could have a mutation createGame which doesn't take any parameters but it uses that information to go and create a record in the database with some initial game state, and then there's a selection set you can pick fields from whatever data structure that is deemed right at that point. This will be implemented as "modify database" and then "query database", and if you don't have read permissions you'd get the above outlined error.

So is it more of an education piece, people aren't aware that they are really performing two database operations CREATE (or UPDATE or DELETE) followed by a SELECT? And if I am denied permissions for SELECT then it'd make sense I get an error, unless I try to just get back __typename (since that doesn't hit the database).