-
Notifications
You must be signed in to change notification settings - Fork 751
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
fix(query): revoke need consider GrantObject::Database/Table #14567
Conversation
19173d6
to
a85ddfb
Compare
a73a469
to
5f4077e
Compare
// Note if old version `grant all on db.*/db.t to user`, the user will contains ownership privilege. | ||
// revoke all need to revoke it. | ||
let priv_types = match principal { | ||
PrincipalIdentity::User(_) => grant_object[0].available_privileges(true), |
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.
I think this modify is ok. I test on local with different query version, old query write, new query read:
Prepare data in v1.2.241:
drop user if exists a;
drop role if exists role1;
drop database if exists a;
create database a;
create user a identified by '123';
grant all on a.* to a;
create role role1;
grant ownership on a.* to role role1;
grant role role1 to a;
1.2.241: show grants can not display ownership info. This is a feature loss in 1.2.241
MySQL [(none)]> show grants for a;
+-------------------------------------------------------------+
| Grants |
+-------------------------------------------------------------+
| GRANT ALL ON 'default'.'a'.* TO 'a'@'%' |
| GRANT SELECT ON 'default'.'system'.'one' TO 'a'@'%' |
| GRANT SELECT ON 'default'.'information_schema'.* TO 'a'@'%' |
+-------------------------------------------------------------+
3 rows in set (0.007 sec)
MySQL [(none)]> show grants for role role1;
+------------------------------------------------------------------+
| Grants |
+------------------------------------------------------------------+
| GRANT SELECT ON 'default'.'system'.'one' TO ROLE `role1` |
| GRANT SELECT ON 'default'.'information_schema'.* TO ROLE `role1` |
+------------------------------------------------------------------+
2 rows in set (0.011 sec)
====
upgrade to fixed pr version, build databend-query, execute these operator.
user root login:
MySQL [(none)]> show grants for a;
+-----------------------------------------+
| Grants |
+-----------------------------------------+
| GRANT ALL ON 'default'.'a'.* TO 'a'@'%' |
+-----------------------------------------+
1 row in set (0.008 sec)
MySQL [(none)]> show grants for role role1; -- also can not display ownership info.
Empty set (0.008 sec)
create user b identified by '123' with DEFAULT_ROLE='role1';
grant role role1 to b;
revoke all on a.* from a;
user a login: After revoke all privilege from user a, also can access Database a.
mysql -ua -p123 -h127.0.0.1 -P3307
MySQL [(none)]> show roles;
+--------+-----------------+------------+------------+
| name | inherited_roles | is_current | is_default |
+--------+-----------------+------------+------------+
| public | 0 | 1 | 0 |
| role1 | 0 | 0 | 0 |
+--------+-----------------+------------+------------+
2 rows in set (0.007 sec)
MySQL [(none)]> set role role1;
Query OK, 0 rows affected (0.006 sec)
MySQL [(none)]> use a
Database changed
MySQL [a]> show tables;
Empty set (0.029 sec)
MySQL [a]> create table t(id int);
Query OK, 0 rows affected (0.068 sec)
MySQL [a]> show tables;
+-------------+
| Tables_in_a |
+-------------+
| t |
+-------------+
1 row in set (0.039 sec)
MySQL [a]> insert into t values(1);
Query OK, 1 row affected (0.143 sec)
user root login : Reovke role1 from a;
mysql -uroot -h127.0.0.1 -P3307
MySQL [(none)]> show grants for a;
+-------------------------------------------------+
| Grants |
+-------------------------------------------------+
| GRANT OWNERSHIP ON 'default'.'a'.'t' TO 'a'@'%' |
+-------------------------------------------------+
1 row in set (0.013 sec)
MySQL [(none)]> revoke role role1 from a;
Query OK, 0 rows affected (0.047 sec)
user b login : Can query Database a.
mysql -ub -p123 -h127.0.0.1 -P3307
MySQL [(none)]> show roles;
+--------+-----------------+------------+------------+
| name | inherited_roles | is_current | is_default |
+--------+-----------------+------------+------------+
| public | 0 | 0 | 0 |
| role1 | 0 | 1 | 1 |
+--------+-----------------+------------+------------+
2 rows in set (0.008 sec)
MySQL [(none)]> use a;
Reading table information for completion of table and column names
You can turn off this feature to get a quicker startup with -A
Database changed
MySQL [a]> select * from t;
+------+
| id |
+------+
| 1 |
+------+
1 row in set (0.040 sec)
user a login : Now user a can not query Database a.
mysql -ua -p123 -h127.0.0.1 -P3307
MySQL [(none)]> use a;
ERROR 1105 (HY000): PermissionDenied. Code: 1063, Text = Permission denied: User 'a'@'%' does not have the required privileges for database 'a'.
MySQL [default]> select * from a.t;
ERROR 1105 (HY000): PermissionDenied. Code: 1063, Text = Permission denied, privilege [Select] is required on 'default'.'a'.'t' for user 'a'@'%' with roles [public].
old version, all contains ownership privilege. now will not revoke ownership privilege. so the user will contains the object's ownership Then in bind_revoke, if revoke all on <object> from user, need contains ownership.
This pr delete some compat version. Please review it @drmingdrmer @dantengsky Revoke need respect old version please review it @flaneur2020 @BohuTANG Add ci test about compat please review it @everpcpc @drmingdrmer |
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.
LGTM
Try to test system table suit test. Add label ci-benchmark-suites |
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.
Reviewed 3 of 6 files at r1, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @BohuTANG, @dantengsky, and @everpcpc)
I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/
Summary
revoke need consider GrantObject::Database/Table
old version, all contains ownership privilege. now will not revoke ownership privilege.
so the user will contains the object's ownership
Then in bind_revoke, if revoke all on from user, need contains
ownership.
Tests
Type of change
This change is