[ISSUE-2894][PATCH-1] Add parser on GRANT .. TO `database`.`table` #2901

flaneur2020 · 2021-11-19T15:28:58Z

I hereby agree to the terms of the CLA available at: https://databend.rs/policies/cla/

Summary

add parser about grant privilege on database.table
add GrantObject in the grant plan
sanity check on interpreting the grant privilege plan
test cases on the sanity check

maybe in another PR:

grant user privilege on database & tables logic in the user manager

Changelog

Improvement

Related Issues

Fixes #2894

Test Plan

Unit Tests

Stateless Tests

databend-bot · 2021-11-19T15:29:02Z

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

codecov-commenter · 2021-11-19T15:49:57Z

Codecov Report

Merging #2901 (a232378) into main (9565a41) will increase coverage by 0%.
The diff coverage is 73%.

@@          Coverage Diff           @@
##            main   #2901    +/-   ##
======================================
  Coverage     68%     68%            
======================================
  Files        635     636     +1     
  Lines      33599   33680    +81     
======================================
+ Hits       22885   23006   +121     
+ Misses     10714   10674    -40

Impacted Files	Coverage Δ
...ry/src/interpreters/interpreter_grant_privilege.rs	`64% <33%> (-16%)`	⬇️
common/meta/types/src/grant_object.rs	`40% <40%> (ø)`
query/src/sql/statements/statement_grant.rs	`75% <66%> (-17%)`	⬇️
query/src/sql/sql_parser_test.rs	`79% <81%> (+<1%)`	⬆️
query/src/sql/sql_parser.rs	`84% <95%> (+<1%)`	⬆️
common/planners/src/plan_grant_privilege.rs	`88% <100%> (+1%)`	⬆️
metasrv/src/meta_service/network.rs	`78% <0%> (+1%)`	⬆️
query/src/catalogs/catalog.rs	`36% <0%> (+4%)`	⬆️
common/base/src/http_shutdown_handlers.rs	`49% <0%> (+40%)`	⬆️
metasrv/src/api/http_service.rs	`72% <0%> (+62%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9565a41...a232378. Read the comment docs.

BohuTANG · 2021-11-20T07:24:09Z

query/src/catalogs/impls/catalog/overlaid_catalog.rs

@@ -233,4 +233,21 @@ impl Catalog for OverlaidCatalog {
            }
        }
    }
+
+    async fn exists_table(


How these codes is shown in this patch, I rember that it's in another patch

in the earlier days there's only exists_database but no exists_table yet, there's another PR working on this?

if it's true i'd avoid adding exists_table func in this PR to avoid conflicting q.q

I've reverted the changes on the catalog trait, plz help review again

BohuTANG · 2021-11-20T07:31:03Z

There is a suggesstion and a question:

Suggesstion:
It would be nice to do add an exists_table method to the Catalog trait (failed on code reusing on meeting an warning about in another patch not this one.

Question:
From my understand, the privileges is grant to the role not the user, in this patch it still grant to user, we will do it in another patch(like the ... create role... patch) in future?

flaneur2020 · 2021-11-20T07:46:35Z

From my understand, the privileges is grant to the role not the user, in this patch it still grant to user,

the privileges is both granted to the user and role? 🤔 in mysql the privileges one user finally gets is the privileges she directly get granted add the user's roles' privileges

we will do it in another patch(like the ... create role... patch) in future?

yes, we'd also add the logic about grant the privileges to roles in the future pull requests

BohuTANG · 2021-11-20T08:03:27Z

From my understand, the privileges is grant to the role not the user, in this patch it still grant to user,

the privileges is both granted to the user and role? thinking in mysql the privileges one user finally gets is the privileges she directly get granted add the user's roles' privileges

Privileges are granted to roles, and roles are granted to users.

the privileges is both granted to the user and role? -- This is for forward compatibility, they had no role before.

we will do it in another patch(like the ... create role... patch) in future?

yes, we'd also add the logic about grant the privileges to roles in the future pull requests

BohuTANG · 2021-11-20T08:05:18Z

in mysql the privileges one user finally gets is the privileges she directly get granted add the user's roles' privileges

Not true.
Thinking a case:
if a role is dropped, the user role is drop and his pivileges has gone also, not revoke the privileges from the user.

Updated:
I misunderstood it, sorry

flaneur2020 · 2021-11-20T08:12:07Z

in mysql the privileges one user finally gets is the privileges she directly get granted add the user's roles' privileges

Not true. Thinking a case: if a role is dropped, the user role is drop and his pivileges has gone also, not revoke the privileges from the user.

it's right, when a role is dropped, the user stil has his own privileges left:

consider a scenary like a user has got a role with CREATE ON db1 privilege, and SELECT ON db2 which directly granted to the user.

after dropping the role, she stil remains the SELECT privilege on db2

BohuTANG · 2021-11-20T08:14:39Z

in mysql the privileges one user finally gets is the privileges she directly get granted add the user's roles' privileges
Not true. Thinking a case: if a role is dropped, the user role is drop and his pivileges has gone also, not revoke the privileges from the user.

it's right, when a role is dropped, the user stil has his own privileges left:

consider a scenary like a user has got a role with CREATE ON db1 privilege, and SELECT ON db2 which directly granted to the user.

If the CREATE ON db1 privilege is role1, SELECT ON db2 is role2, a user can only grant one of them at a time.

after dropping the role, she stil remains the SELECT privilege on db2

BohuTANG · 2021-11-20T08:16:10Z

The follow model is more simple for us(ClickHouse did them too, see):
Privileges are granted to roles, and roles are granted to users.

CREATE USER bohu@'%';
CREATE ROLE systemadmin;
GRANT SELECT ON system.* TO systemadmin;
GRANT systemadmin TO bohu;

flaneur2020 · 2021-11-20T08:24:14Z

the privileges is both granted to the user and role? -- This is for forward compatibility, they had no role before.

the user privilege + role privileges is commonly used in the popular DBMS? 🤔

it's also available in clickhouse: https://clickhouse.com/docs/en/sql-reference/statements/grant/

GRANT [ON CLUSTER cluster_name] privilege[(column_name [,...])] [,...] ON {db.table|db.*|*.*|table|*} TO {user | role | CURRENT_USER} [,...] [WITH GRANT OPTION] [WITH REPLACE OPTION]

a GRANT statement can works to user or role. it seems postgres is also the same: https://tableplus.com/blog/2018/04/postgresql-how-to-grant-access-to-users.html

imo RBAC is powerful but it has a bit higher bar than the user privileges, it has its own scenary 🤔

BohuTANG · 2021-11-20T08:28:11Z

For many DBMS, user and role is called object, and the privileges can grant to object, I guess they keep it, but it's more complex.
We can follow the easy way, make it work, make it right :D

flaneur2020 · 2021-11-20T08:40:53Z

For many DBMS, user and role is called object, and the privileges can grant to object, I guess they keep it, but it's more complex. We can follow the easy way, make it work, make it right :D

in the current code base, adding GRANT to role needs some works in a sequence of PRs, in my previous thought, we can follow these steps:

we already has a code base about grant an user some privilege
add fine grained privileges on users
the checking privilege logics
allow grant privilege to roles, and grant roles to user
allow grant roles to role

user privilege + rbac has more complexity than the RBAC only solution at last, but RBAC has more complexity than the user privilege upfront 🤔

BohuTANG · 2021-11-20T09:14:56Z

query/src/interpreters/interpreter_grant_privilege.rs

@@ -51,6 +73,40 @@ impl Interpreter for GrantPrivilegeInterpreter {
        _input_stream: Option<SendableDataBlockStream>,
    ) -> Result<SendableDataBlockStream> {
        let plan = self.plan.clone();
+
+        // *.`table` is not allowed


case by case match is more complex, but not quite sure if there is a regular check for the pattern?

Or we can first to check database_pattern and then table_pattern

it seems we can seperate the grant object (global, database, or table) in the parsing phase

how about adding a GrantObject enum on this?

it'd has three hands:

Global

Database(String)

Table(Option, String) // if the database is not specified, taking the current db in session

@BohuTANG I've refactored out an GrantObject enum, and checked *.table as invalid syntax in the parsing phase

please help review again when you get time 🤝

…table

BohuTANG · 2021-11-22T04:11:21Z

/lgtm

databend-bot · 2021-11-22T04:11:24Z

Wait for another reviewer approval

BohuTANG · 2021-11-22T04:16:55Z

I saw some sub-tasks in #2894, it would be better to create a sub-task issue of the main issue #2894 for each PR, the PR will close the sub-task when it closed

I re-open the issue #2894

databend-bot added pr-improvement need-review labels Nov 19, 2021

flaneur2020 changed the title ~~WIP: Allow grant privilege on database.table [PATCH-1]~~ WIP: [ISSUE-2894][PATCH-1] Add parser on GRANT .. TO database.table Nov 19, 2021

flaneur2020 marked this pull request as draft November 19, 2021 15:30

flaneur2020 mentioned this pull request Nov 19, 2021

Fine-grained user privilege on database & table #2894

Closed

9 tasks

flaneur2020 added 5 commits November 20, 2021 15:08

parse the database & table pattern in the grant statement

7af335a

add exists_table in the catalog trait

e533d3e

add database_pattern & table_pattern in the grant plan

7222366

add sanity check about database_pattern and table_pattern

b75e8ad

refactor: move the same exists_database code to Catalog trait

89a382c

flaneur2020 force-pushed the add-grant-database-table branch from d5d61a6 to 3fbcaa5 Compare November 20, 2021 07:08

flaneur2020 changed the title ~~WIP: [ISSUE-2894][PATCH-1] Add parser on GRANT .. TO database.table~~ [ISSUE-2894][PATCH-1] Add parser on GRANT .. TO database.table Nov 20, 2021

flaneur2020 marked this pull request as ready for review November 20, 2021 07:09

flaneur2020 requested a review from BohuTANG as a code owner November 20, 2021 07:09

flaneur2020 added 3 commits November 20, 2021 15:11

add stateless test cases on grant non existed database or tables

9d29b07

fix cargo check on where_clauses_object_safety

294e1f0

fix error code on sanitizing the grant object

3f52661

flaneur2020 force-pushed the add-grant-database-table branch from 3fbcaa5 to 3f52661 Compare November 20, 2021 07:12

BohuTANG reviewed Nov 20, 2021

View reviewed changes

combinate the parser func instead of duplicate the parsing logic

b2e78d1

flaneur2020 force-pushed the add-grant-database-table branch from 360ace9 to b2e78d1 Compare November 20, 2021 07:53

revert the catalog change in this pr

cd236a1

BohuTANG reviewed Nov 20, 2021

View reviewed changes

flaneur2020 added 4 commits November 20, 2021 17:26

add grant object

4208c12

tune the parsing logic on parse_grant_object

c6b37ec

refactor the interpreter logic

9ec112f

seperate DfGrantObject and GrantObject

43592e7

flaneur2020 force-pushed the add-grant-database-table branch from ffa3a14 to 43592e7 Compare November 20, 2021 10:22

flaneur2020 added 3 commits November 20, 2021 18:32

fix unittest and stateless test

735a743

add todo about the next job

9dfc6ae

fix cargo check

6c7d0c4

flaneur2020 force-pushed the add-grant-database-table branch from e798ac3 to 3671b5e Compare November 20, 2021 12:45

grant on * should be granted to database level

4462dd5

flaneur2020 force-pushed the add-grant-database-table branch from 3671b5e to 4462dd5 Compare November 20, 2021 13:17

flaneur2020 added 4 commits November 20, 2021 21:17

Merge remote-tracking branch 'upstream/main' into add-grant-database-…

6a4aca8

…table

cleanup tmp check code after datafuselabs#2917

8637b88

merge upstream/main

dc6c785

merge upstream/main

a232378

flaneur2020 requested a review from BohuTANG November 22, 2021 04:08

databend-bot approved these changes Nov 22, 2021

View reviewed changes

BohuTANG merged commit d182f26 into datafuselabs:main Nov 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ISSUE-2894][PATCH-1] Add parser on GRANT .. TO `database`.`table` #2901

[ISSUE-2894][PATCH-1] Add parser on GRANT .. TO `database`.`table` #2901

flaneur2020 commented Nov 19, 2021 •

edited

databend-bot commented Nov 19, 2021

codecov-commenter commented Nov 19, 2021 •

edited

BohuTANG Nov 20, 2021 •

edited

flaneur2020 Nov 20, 2021

flaneur2020 Nov 20, 2021

BohuTANG commented Nov 20, 2021

flaneur2020 commented Nov 20, 2021 •

edited

BohuTANG commented Nov 20, 2021

BohuTANG commented Nov 20, 2021 •

edited

flaneur2020 commented Nov 20, 2021

BohuTANG commented Nov 20, 2021

BohuTANG commented Nov 20, 2021

flaneur2020 commented Nov 20, 2021

BohuTANG commented Nov 20, 2021

flaneur2020 commented Nov 20, 2021

BohuTANG Nov 20, 2021

BohuTANG Nov 20, 2021

flaneur2020 Nov 20, 2021

flaneur2020 Nov 20, 2021

flaneur2020 Nov 21, 2021

BohuTANG commented Nov 22, 2021

databend-bot commented Nov 22, 2021

BohuTANG commented Nov 22, 2021

[ISSUE-2894][PATCH-1] Add parser on GRANT .. TO database.table #2901

[ISSUE-2894][PATCH-1] Add parser on GRANT .. TO database.table #2901

Conversation

flaneur2020 commented Nov 19, 2021 • edited

Summary

Changelog

Related Issues

Test Plan

databend-bot commented Nov 19, 2021

codecov-commenter commented Nov 19, 2021 • edited

Codecov Report

BohuTANG Nov 20, 2021 • edited

Choose a reason for hiding this comment

flaneur2020 Nov 20, 2021

Choose a reason for hiding this comment

flaneur2020 Nov 20, 2021

Choose a reason for hiding this comment

BohuTANG commented Nov 20, 2021

flaneur2020 commented Nov 20, 2021 • edited

BohuTANG commented Nov 20, 2021

BohuTANG commented Nov 20, 2021 • edited

flaneur2020 commented Nov 20, 2021

BohuTANG commented Nov 20, 2021

BohuTANG commented Nov 20, 2021

flaneur2020 commented Nov 20, 2021

BohuTANG commented Nov 20, 2021

flaneur2020 commented Nov 20, 2021

BohuTANG Nov 20, 2021

Choose a reason for hiding this comment

BohuTANG Nov 20, 2021

Choose a reason for hiding this comment

flaneur2020 Nov 20, 2021

Choose a reason for hiding this comment

flaneur2020 Nov 20, 2021

Choose a reason for hiding this comment

flaneur2020 Nov 21, 2021

Choose a reason for hiding this comment

BohuTANG commented Nov 22, 2021

databend-bot commented Nov 22, 2021

BohuTANG commented Nov 22, 2021

[ISSUE-2894][PATCH-1] Add parser on GRANT .. TO `database`.`table` #2901

[ISSUE-2894][PATCH-1] Add parser on GRANT .. TO `database`.`table` #2901

flaneur2020 commented Nov 19, 2021 •

edited

codecov-commenter commented Nov 19, 2021 •

edited

BohuTANG Nov 20, 2021 •

edited

flaneur2020 commented Nov 20, 2021 •

edited

BohuTANG commented Nov 20, 2021 •

edited