-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rfc: admin-only/backend-only insert permissions #4120
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
### Insert permissions and actions | ||
|
||
If an action is used for data validation, it is hard to re-use constraints | ||
specified through insert permissions. | ||
|
||
#### Motivation | ||
|
||
Let's consider a table with the following schema: | ||
|
||
```sql | ||
create table message ( | ||
id serial primary key, | ||
content text not null, | ||
channel_id integer not null references channel(id) | ||
) | ||
create table channel (..) | ||
create table channel_members (..) | ||
``` | ||
|
||
Let's say you want to allow a `user` role to insert into | ||
`message` table if the following `check` constraint holds: | ||
```json | ||
{"channel": {"members": {"user_id": "x-hasura-user-id"}}} | ||
``` | ||
i.e, a user can only post a message to a channel if they are a member of that | ||
particular channel. Now let's say you want to do some sort of validation on the | ||
content of the message (maybe through some external service), this is a problem | ||
that would broadly fall under the 'data validation' category (data cannot be | ||
validated with graphql-engine's permissions or through Postgres's check | ||
constraints). | ||
|
||
We recommend actions in such cases, we would suggest the developer to remove | ||
the permission for `message` table on `user` and then do the entire validation | ||
in the webhook for the action. However the webhook now has to do more than | ||
validation of the message's content - it also has to check this constraint | ||
`{"channel": {"members": {"user_id": "x-hasura-user-id"}}}` which was part of | ||
insert permissions earlier. This is inconvenient and the reusability of | ||
hasura's permission system is lost. | ||
|
||
Why should the insert permission be removed? Because if the insert permission | ||
is defined, the `insert_message` mutation is generated which can be directly | ||
used by anyone with the `user` role and thus bypassing the validation of the | ||
`message` content. | ||
|
||
This problem is originally reported | ||
[on discord](https://discordapp.com/channels/407792526867693568/516590536875048990/685462912420282546). | ||
|
||
#### Proposed solution | ||
|
||
Introduce a new field in insert permission called `admin_only`, this | ||
will remove the insert mutation field for the table from `mutation_root` for | ||
that role but the mutation can still be accessed when `admin-secret` is | ||
specified. The action's handler can now specify the `admin-secret`, set | ||
`x-hasura-role` as the user's role and execute the request so that the | ||
constraints defined on the role in insert permissions are enforced. | ||
|
||
Currently `admin-secret` + `x-hasura-role` is used in GraphiQL to mimic a | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would be a breaking change. Currently, |
||
request as a particular role. So `admin_only` insert mutations will be allowed | ||
when a request is made through GraphiQL with `admin-secret` + `x-hasura-role`. | ||
We'll need to make this clear in our documentation. | ||
|
||
|
||
|
||
|
||
|
||
|
||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is the solution affected by https://github.com/hasura/graphql-engine/blob/233ab645993ec08e981ca55d4bc8414a01bbd944/rfcs/disable-query-and-subscription-root-fields.md
Does it make sense to hide fields under
mutation_root
also? In that case, we do not needadmin_only
in the permission but we will need a header likex-hasura-include-hidden-fields: true
or something?