Skip to content
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

Merged
merged 4 commits into from Feb 3, 2024

Conversation

TCeason
Copy link
Collaborator

@TCeason TCeason commented Feb 2, 2024

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

Summary

  1. revoke need consider GrantObject::Database/Table

  2. 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

  • Unit Test
  • Logic Test
  • Benchmark Test
  • No Test - Explain why

Type of change

  • Bug Fix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that could cause existing functionality not to work as expected)
  • Documentation Update
  • Refactoring
  • Performance Improvement
  • Other (please describe):

This change is Reviewable

@github-actions github-actions bot added the pr-bugfix this PR patches a bug in codebase label Feb 2, 2024
@TCeason TCeason force-pushed the fix_revoke branch 4 times, most recently from 19173d6 to a85ddfb Compare February 2, 2024 11:35
@TCeason TCeason force-pushed the fix_revoke branch 5 times, most recently from a73a469 to 5f4077e Compare February 2, 2024 13:38
// 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),
Copy link
Collaborator Author

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.
@TCeason
Copy link
Collaborator Author

TCeason commented Feb 3, 2024

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

Copy link
Member

@flaneur2020 flaneur2020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@TCeason
Copy link
Collaborator Author

TCeason commented Feb 3, 2024

Try to test system table suit test. Add label ci-benchmark-suites

Copy link
Member

@drmingdrmer drmingdrmer left a 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)

@BohuTANG BohuTANG merged commit ba033c2 into datafuselabs:main Feb 3, 2024
71 of 72 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-bugfix this PR patches a bug in codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: can't revoke privileges from users after upgrade databend-query version to v1.2.321-nightly
4 participants