Add check expresion to update permissions (close #384) #3804

paf31 · 2020-01-30T13:36:27Z

Description

Add a check constraint property to the update permission data structure
Validate the check constraint after updates

The console UI has not yet been updated and I was hoping someone could pair with me on that. It is still possible to test this using direct changes to hdb_permission right now, or presumably using the CLI.

Left for a later PR:

Console UI changes
Update documentation for permissions in the console

Affected components

Server
Tests
Console

Related Issues

Solution and Design

See https://github.com/0x777/graphql-engine/blob/rfc-update-permission-check-condition/rfcs/update-permission-check-condition.md

Steps to test and verify

Limitations, known bugs & workarounds

Server checklist

Catalog upgrade

Does this PR change Hasura Catalog version?

No

Metadata

Does this PR add a new Metadata feature?

Yes
- Does run_sql auto manages the new metadata through schema diffing?
  - Yes
  - Not required
- Does run_sql auto manages the definitions of metadata on renaming?
  - Yes
  - Not required
- Does export_metadata/replace_metadata supports the new metadata added?
  - Yes
  - Not required

GraphQL

No new GraphQL schema is generated

Breaking changes

No Breaking changes

hasura-bot · 2020-01-30T13:49:28Z

Review app for commit 53318dd deployed to Heroku: https://hge-ci-pull-3804.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3804-53318dd4

hasura-bot · 2020-01-30T15:25:46Z

Review app for commit 228f398 deployed to Heroku: https://hge-ci-pull-3804.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3804-228f3986

hasura-bot · 2020-01-30T15:58:08Z

Review app for commit 680e836 deployed to Heroku: https://hge-ci-pull-3804.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3804-680e836a

hasura-bot · 2020-01-30T16:27:25Z

Review app for commit 54d4a85 deployed to Heroku: https://hge-ci-pull-3804.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3804-54d4a85e

hasura-bot · 2020-02-04T12:56:21Z

Review app for commit f84f874 deployed to Heroku: https://hge-ci-pull-3804.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3804-f84f8746

hasura-bot · 2020-02-05T04:08:00Z

Review app for commit da5a362 deployed to Heroku: https://hge-ci-pull-3804.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3804-da5a362a

paf31 · 2020-02-05T18:31:03Z

@0x777 Current status: I'm trying to figure out why xmax works in some cases but not in every case (see the test failures for example).

Edit: the problem is that xmax doesn't work for views, so in particular, if a user is using automatically updatable views for inserts, xmax won't work.

0x777 · 2020-02-06T11:39:43Z

@paf31

Edit: the problem is that xmax doesn't work for views, so in particular, if a user is using automatically updatable views for inserts, xmax won't work.

Ah! That makes sense! So if we unconditionally include the CASE xmax = 0 .. in returning, inserting into a view will result in errors. However, if we only use the xmax column based CASE expression in returning when on_conflict is used, we should be able to avoid these errors? Because on_conflict doesn't make sense on views anyway (in fact, we shouldn't even generate on_conflict arguments in the generated graphql schema for a view). What do you think?

paf31 · 2020-02-06T22:29:20Z

ON CONFLICT does seem to work on automatically-updatable views for me, but if we don't support that in the API, then I think that solution is fine. Edit: I checked, and we don't, so I think that solution is good.

paf31 · 2020-02-06T22:44:34Z

@0x777 One more question is what to do if there is an insert check, but no update check, and the ON CONFLICT behavior is used. In the old code, we would use the insert check, but now we will not. This is technically breaking, so do you think we should keep the old behavior?

netlify · 2020-02-06T23:43:56Z

Deploy preview for hasura-docs ready!

Built with commit c0a42ef

https://deploy-preview-3804--hasura-docs.netlify.com

hasura-bot · 2020-02-07T01:25:36Z

Review app for commit 148dfd2 deployed to Heroku: https://hge-ci-pull-3804.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3804-148dfd26

0x777 · 2020-02-07T11:52:46Z

ON CONFLICT does seem to work on automatically-updatable views for me

Interesting! I did a little bit of digging:

db> create table genre (id serial primary key, name text not null unique);
CREATE TABLE
db> create view genre_view as select * from genre;
CREATE VIEW
db> insert into genre (id, name) values (DEFAULT, 'drama');
INSERT 0 1
db> insert into genre_view (id, name) values (DEFAULT, 'crime');
INSERT 0 1
db> insert into genre (id, name) values (DEFAULT, 'drama') on conflict on constraint genre_name_key DO NOTHING;
INSERT 0 0
db> insert into genre_view (id, name) values (DEFAULT, 'drama') on conflict on constraint genre_name_key DO NOTHING;
ERROR:  constraint "genre_name_key" for table "genre_view" does not exist

What's interesting is that this works:

db> insert into genre_view (id, name) values (DEFAULT, 'drama') on conflict (name) DO NOTHING;
INSERT 0 0

on conflict with unique index inference seems to be supported on views. So our current on_conflict
API (which relies on unique constraint names) is limited when it comes to views which seems
to work in our favour. Just to summarize, in case if we ever support unique index inference in our on_conflict API, we won't be able to enforce permissions on views if the insert and update permission's check constraints differ, right?

In the old code, we would use the insert check, but now we will not. This is technically breaking, so do you think we should keep the old behavior?

Oh yes, I guess it makes sense to use insert check constraint if the update check constraint is missing.

paf31 · 2020-02-07T18:40:31Z

Just to summarize, in case if we ever support unique index inference in our on_conflict API, we won't be able to enforce permissions on views if the insert and update permission's check constraints differ, right?

Correct.

hasura-bot · 2020-02-07T18:53:09Z

Review app for commit 5772eee deployed to Heroku: https://hge-ci-pull-3804.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3804-5772eeea

hasura-bot · 2020-02-07T19:10:38Z

Review app for commit 5f0fb0c deployed to Heroku: https://hge-ci-pull-3804.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3804-5f0fb0cf

paf31 · 2020-02-07T22:02:35Z

@0x777 This is ready for another review when you have time, please. I'm not planning to implement the console changes or documentation changes yet (since the docs only seem to describe the console UI anyway), but I think this could be merged without those while we decide on the right approach to the UI.

beerose

I'm approving since there are no changes in the console code except for linting.

0x777 · 2020-02-11T09:00:19Z

server/tests-py/queries/graphql_mutation/update/permissions/user_cannot_unpublish.yaml

@@ -0,0 +1,26 @@
+description: Try to change the published flag on a published article


This test doesn't seem to be testing the check constraint defined above. We should testing for both a success and an error.

Oops, thanks. I forgot to finish this and got confused because the tests were passing and I had uncommitted work locally.

0x777 · 2020-02-11T09:01:12Z

@paf31 LGTM except for the test case.

hasura-bot · 2020-02-11T18:10:13Z

Review app for commit b08c277 deployed to Heroku: https://hge-ci-pull-3804.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3804-b08c2776

0x777 · 2020-02-12T13:04:39Z

server/tests-py/queries/graphql_mutation/update/permissions/user_cannot_publish.yaml

+  - extensions:
+      path: $
+      code: permission-error
+    message: Check constraint violation. insert check constraint failed


I think we'll need to change the message appropriately for update checks, .. update check constraint failed?

hasura-bot · 2020-02-12T18:32:55Z

Review app for commit 9881cac deployed to Heroku: https://hge-ci-pull-3804.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3804-9881cac5

hasura-bot · 2020-02-13T07:06:55Z

Review app for commit 0766959 deployed to Heroku: https://hge-ci-pull-3804.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3804-0766959d

hasura-bot · 2020-02-13T07:38:52Z

Review app https://hge-ci-pull-3804.herokuapp.com is deleted

paf31 requested a review from 0x777 January 30, 2020 13:36

paf31 force-pushed the 384 branch from 53318dd to 228f398 Compare January 30, 2020 15:12

paf31 force-pushed the 384 branch from 228f398 to 680e836 Compare January 30, 2020 15:45

Add check expresion to update permissions (close hasura#384)

54d4a85

paf31 force-pushed the 384 branch from 680e836 to 54d4a85 Compare January 30, 2020 16:12

Merge branch 'master' into 384

f84f874

paf31 added 2 commits February 4, 2020 18:51

wip on conflict behavior

162a7b8

Merge branch '384' of github.com:paf31/graphql-engine into 384

da5a362

0x777 added the c/server Related to server label Feb 6, 2020

Handle upserts for views properly

c0a42ef

Merge remote-tracking branch 'origin' into 384

148dfd2

Merge branch 'master' into 384

5772eee

Use insert check if there is no update check

5f0fb0c

paf31 marked this pull request as ready for review February 8, 2020 00:04

paf31 requested review from beerose and lexi-lambda as code owners February 8, 2020 00:04

beerose previously approved these changes Feb 10, 2020

View reviewed changes

0x777 reviewed Feb 11, 2020

View reviewed changes

paf31 dismissed beerose’s stale review via 25c066b February 11, 2020 17:56

Fix the test

b08c277

paf31 force-pushed the 384 branch from 25c066b to b08c277 Compare February 11, 2020 17:58

paf31 requested a review from 0x777 February 11, 2020 17:58

0x777 reviewed Feb 12, 2020

View reviewed changes

Improve error message slightly

9881cac

0x777 approved these changes Feb 13, 2020

View reviewed changes

Merge branch 'master' into 384

0766959

beerose approved these changes Feb 13, 2020

View reviewed changes

Merge branch 'master' into 384

681750f

0x777 merged commit f615abd into hasura:master Feb 13, 2020

rakeshkky mentioned this pull request Mar 11, 2020

fix postgres query error when computed fields included in mutation response (fix #4035) #4080

Merged

17 tasks

tirumaraiselvan mentioned this pull request Mar 18, 2020

console: incorporate filter and check for update permissions #4142

Closed

beerose mentioned this pull request Apr 7, 2020

console: allow setting filter and check for update permissions #4313

Merged

2 tasks

		@@ -0,0 +1,26 @@
		description: Try to change the published flag on a published article

Add check expresion to update permissions (close #384) #3804

Add check expresion to update permissions (close #384) #3804

Uh oh!

Conversation

paf31 commented Jan 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Affected components

Related Issues

Solution and Design

Steps to test and verify

Limitations, known bugs & workarounds

Server checklist

Catalog upgrade

Metadata

GraphQL

Breaking changes

Uh oh!

hasura-bot commented Jan 30, 2020

Uh oh!

hasura-bot commented Jan 30, 2020

Uh oh!

hasura-bot commented Jan 30, 2020

Uh oh!

hasura-bot commented Jan 30, 2020

Uh oh!

hasura-bot commented Feb 4, 2020

Uh oh!

hasura-bot commented Feb 5, 2020

Uh oh!

paf31 commented Feb 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0x777 commented Feb 6, 2020

Uh oh!

paf31 commented Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paf31 commented Feb 6, 2020

Uh oh!

netlify bot commented Feb 6, 2020

Uh oh!

hasura-bot commented Feb 7, 2020

Uh oh!

0x777 commented Feb 7, 2020

Uh oh!

paf31 commented Feb 7, 2020

Uh oh!

hasura-bot commented Feb 7, 2020

Uh oh!

hasura-bot commented Feb 7, 2020

Uh oh!

paf31 commented Feb 7, 2020

Uh oh!

beerose left a comment

Choose a reason for hiding this comment

Uh oh!

0x777 Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

paf31 Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

0x777 commented Feb 11, 2020

Uh oh!

hasura-bot commented Feb 11, 2020

Uh oh!

0x777 Feb 12, 2020

Choose a reason for hiding this comment

Uh oh!

hasura-bot commented Feb 12, 2020

Uh oh!

hasura-bot commented Feb 13, 2020

Uh oh!

hasura-bot commented Feb 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

paf31 commented Jan 30, 2020 •

edited

Loading

paf31 commented Feb 5, 2020 •

edited

Loading

paf31 commented Feb 6, 2020 •

edited

Loading