From c86507eff2cc4b61a9c2a109ae88ed56dd501a06 Mon Sep 17 00:00:00 2001 From: TCeason Date: Tue, 1 Apr 2025 13:00:07 +0800 Subject: [PATCH 1/3] chore: use Usage privilege optimize USEDB Plan --- src/meta/app/src/principal/user_privilege.rs | 4 +- .../interpreters/access/privilege_access.rs | 46 +++++++++++---- src/query/users/src/visibility_checker.rs | 13 ++++- .../base/05_ddl/05_0017_ddl_grant_role.test | 4 +- .../18_rbac/18_0014_use_db_priv_check.result | 15 +++++ .../18_rbac/18_0014_use_db_priv_check.sh | 56 +++++++++++++++++++ 6 files changed, 123 insertions(+), 15 deletions(-) create mode 100644 tests/suites/0_stateless/18_rbac/18_0014_use_db_priv_check.result create mode 100755 tests/suites/0_stateless/18_rbac/18_0014_use_db_priv_check.sh diff --git a/src/meta/app/src/principal/user_privilege.rs b/src/meta/app/src/principal/user_privilege.rs index 3dd691f695dfc..d00bd481943a7 100644 --- a/src/meta/app/src/principal/user_privilege.rs +++ b/src/meta/app/src/principal/user_privilege.rs @@ -220,7 +220,9 @@ impl UserPrivilegeSet { /// Currently the privileges available to a database and a table are the same, it might becomes /// some differences in the future. pub fn available_privileges_on_database(available_ownership: bool) -> Self { - UserPrivilegeSet::available_privileges_on_table(available_ownership) + (UserPrivilegeSet::available_privileges_on_table(available_ownership).privileges + | make_bitflags!(UserPrivilegeType::{ Usage })) + .into() } /// The all privileges global which available to the table object diff --git a/src/query/service/src/interpreters/access/privilege_access.rs b/src/query/service/src/interpreters/access/privilege_access.rs index 239f661b05830..1c82a986c663b 100644 --- a/src/query/service/src/interpreters/access/privilege_access.rs +++ b/src/query/service/src/interpreters/access/privilege_access.rs @@ -758,7 +758,7 @@ impl AccessChecker for PrivilegeAccess { ObjectId::Database(db_id) => { (db_id, None) } }; - if has_priv(&tenant, database, None, show_db_id, table_id, grant_set).await? { + if has_priv(&tenant, database, None, show_db_id, table_id, grant_set, false).await? { return Ok(()) } @@ -777,7 +777,7 @@ impl AccessChecker for PrivilegeAccess { ObjectId::Table(db_id, table_id) => { (db_id, Some(table_id)) } ObjectId::Database(db_id) => { (db_id, None) } }; - if has_priv(&tenant, database, None, show_db_id, table_id, grant_set).await? { + if has_priv(&tenant, database, None, show_db_id, table_id, grant_set, true).await? { return Ok(()); } let user_api = UserApiProvider::instance(); @@ -803,7 +803,7 @@ impl AccessChecker for PrivilegeAccess { ObjectId::Table(db_id, table_id) => { (db_id, Some(table_id)) } ObjectId::Database(db_id) => { (db_id, None) } }; - let has_priv = has_priv(&tenant, database, Some(table), db_id, table_id, grant_set).await?; + let has_priv = has_priv(&tenant, database, Some(table), db_id, table_id, grant_set, false).await?; return if has_priv { Ok(()) } else { @@ -932,7 +932,7 @@ impl AccessChecker for PrivilegeAccess { ObjectId::Table(db_id, table_id) => { (db_id, Some(table_id)) } ObjectId::Database(db_id) => { (db_id, None) } }; - if has_priv(&tenant, &plan.database, None, show_db_id, None, grant_set).await? { + if has_priv(&tenant, &plan.database, None, show_db_id, None, grant_set, true).await? { return Ok(()); } let user_api = UserApiProvider::instance(); @@ -1434,6 +1434,7 @@ async fn has_priv( db_id: u64, table_id: Option, grant_set: UserGrantSet, + valid_usage_priv: bool, ) -> Result { if db_name.to_lowercase() == "information_schema" { return Ok(true); @@ -1461,13 +1462,38 @@ async fn has_priv( if db_name.to_lowercase() == "system" { return true; } - e.privileges().iter().any(|privilege| { - UserPrivilegeSet::available_privileges_on_database(false) - .has_privilege(privilege) - }) + if valid_usage_priv { + e.privileges().iter().any(|privilege| { + UserPrivilegeSet::available_privileges_on_database(false) + .has_privilege(privilege) + }) + } else { + !(e.privileges().len() == 1 + && e.privileges().contains(UserPrivilegeType::Usage)) + && e.privileges().iter().any(|privilege| { + UserPrivilegeSet::available_privileges_on_database(false) + .has_privilege(privilege) + }) + } + } + GrantObject::Database(_, ldb) => { + if valid_usage_priv { + *ldb == db_name + } else { + !(e.privileges().len() == 1 + && e.privileges().contains(UserPrivilegeType::Usage)) + && *ldb == db_name + } + } + GrantObject::DatabaseById(_, ldb) => { + if valid_usage_priv { + *ldb == db_id + } else { + !(e.privileges().len() == 1 + && e.privileges().contains(UserPrivilegeType::Usage)) + && *ldb == db_id + } } - GrantObject::Database(_, ldb) => *ldb == db_name, - GrantObject::DatabaseById(_, ldb) => *ldb == db_id, GrantObject::Table(_, ldb, ltab) => { if let Some(table) = table_name { *ldb == db_name && *ltab == table diff --git a/src/query/users/src/visibility_checker.rs b/src/query/users/src/visibility_checker.rs index cce5c5fde03dd..63b42fee51a57 100644 --- a/src/query/users/src/visibility_checker.rs +++ b/src/query/users/src/visibility_checker.rs @@ -133,10 +133,19 @@ impl GrantObjectVisibilityChecker { ); } GrantObject::Database(catalog, db) => { - granted_databases.insert((catalog.to_string(), db.to_string())); + if !(ent.privileges().len() == 1 + && ent.privileges().contains(UserPrivilegeType::Usage)) + { + granted_databases.insert((catalog.to_string(), db.to_string())); + } } GrantObject::DatabaseById(catalog, db) => { - granted_databases_id.insert((catalog.to_string(), *db)); + // If only has db level usage privilege means only support use db. + if !(ent.privileges().len() == 1 + && ent.privileges().contains(UserPrivilegeType::Usage)) + { + granted_databases_id.insert((catalog.to_string(), *db)); + } } GrantObject::Table(catalog, db, table) => { granted_tables.insert(( diff --git a/tests/sqllogictests/suites/base/05_ddl/05_0017_ddl_grant_role.test b/tests/sqllogictests/suites/base/05_ddl/05_0017_ddl_grant_role.test index 5a8b5cfe9a4b9..ba3c3c83bbcc0 100644 --- a/tests/sqllogictests/suites/base/05_ddl/05_0017_ddl_grant_role.test +++ b/tests/sqllogictests/suites/base/05_ddl/05_0017_ddl_grant_role.test @@ -177,8 +177,8 @@ OWNERSHIP s1 NULL ROLE account_admin (empty) query TT select * EXCLUDE(object_id) from show_grants('database', 'db1', 'default'); ---- -CREATE,SELECT,INSERT,UPDATE,DELETE,DROP,ALTER,GRANT db1 ROLE role2 (empty) -CREATE,SELECT,INSERT,UPDATE,DELETE,DROP,ALTER,GRANT db1 ROLE role3 (empty) +USAGE,CREATE,SELECT,INSERT,UPDATE,DELETE,DROP,ALTER,GRANT db1 ROLE role2 (empty) +USAGE,CREATE,SELECT,INSERT,UPDATE,DELETE,DROP,ALTER,GRANT db1 ROLE role3 (empty) OWNERSHIP db1 ROLE account_admin (empty) query TT diff --git a/tests/suites/0_stateless/18_rbac/18_0014_use_db_priv_check.result b/tests/suites/0_stateless/18_rbac/18_0014_use_db_priv_check.result new file mode 100644 index 0000000000000..c6665b7862e52 --- /dev/null +++ b/tests/suites/0_stateless/18_rbac/18_0014_use_db_priv_check.result @@ -0,0 +1,15 @@ +=== show db === +a +b +information_schema +system +=== use db === +=== show tables from a === +t1 +=== show tables from b === +t +=== show columns === +Error: APIError: QueryFailed: [1063]Permission denied: User 'test'@'%' does not have the required privileges for table 'a.t' +ida1 INT YES NULL NULL +idb INT YES NULL NULL +Error: APIError: QueryFailed: [1063]Permission denied: User 'test'@'%' does not have the required privileges for table 'b.t1' diff --git a/tests/suites/0_stateless/18_rbac/18_0014_use_db_priv_check.sh b/tests/suites/0_stateless/18_rbac/18_0014_use_db_priv_check.sh new file mode 100755 index 0000000000000..9064e399dbd60 --- /dev/null +++ b/tests/suites/0_stateless/18_rbac/18_0014_use_db_priv_check.sh @@ -0,0 +1,56 @@ +#!/usr/bin/env bash + +CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +. "$CURDIR"/../../../shell_env.sh + + +export TEST_USER_PASSWORD="password" +export USER_TEST_CONNECT="bendsql --user=test --password=password --host=${QUERY_MYSQL_HANDLER_HOST} --port ${QUERY_HTTP_HANDLER_PORT}" + + +echo "drop user if exists test" | $BENDSQL_CLIENT_CONNECT +echo "create user test identified by '$TEST_USER_PASSWORD'" | $BENDSQL_CLIENT_CONNECT + +echo "create or replace database a" | $BENDSQL_CLIENT_CONNECT +echo "create or replace database b" | $BENDSQL_CLIENT_CONNECT +echo "create or replace database c" | $BENDSQL_CLIENT_CONNECT +echo "create or replace table a.t(ida int)" | $BENDSQL_CLIENT_CONNECT +echo "create or replace table a.t1(ida1 int)" | $BENDSQL_CLIENT_CONNECT +echo "create or replace table b.t(idb int)" | $BENDSQL_CLIENT_CONNECT +echo "create or replace table b.t1(idb1 int)" | $BENDSQL_CLIENT_CONNECT +echo "drop role if exists test_role1" | $BENDSQL_CLIENT_CONNECT +echo "drop role if exists test_role2" | $BENDSQL_CLIENT_CONNECT +echo "create role test_role1" | $BENDSQL_CLIENT_CONNECT +echo "create role test_role2" | $BENDSQL_CLIENT_CONNECT +echo "grant usage on a.* to role test_role1" | $BENDSQL_CLIENT_CONNECT +echo "grant usage on b.* to role test_role1" | $BENDSQL_CLIENT_CONNECT +echo "grant ownership on b.t to role test_role1" | $BENDSQL_CLIENT_CONNECT + +echo "grant role test_role1 to test" | $BENDSQL_CLIENT_CONNECT +echo "grant select on a.t1 to test" | $BENDSQL_CLIENT_CONNECT + +echo "=== show db ===" +echo "show databases" | $USER_TEST_CONNECT + + +echo "=== use db ===" +echo "use a" | $USER_TEST_CONNECT +echo "use b" | $USER_TEST_CONNECT + +echo "=== show tables from a ===" +echo "show tables from a" | $USER_TEST_CONNECT # only display a.t1 +echo "=== show tables from b ===" +echo "show tables from b" | $USER_TEST_CONNECT # only display b.t + +echo "=== show columns ===" +echo "show columns from t from a" | $USER_TEST_CONNECT +echo "show columns from t1 from a" | $USER_TEST_CONNECT +echo "show columns from t from b" | $USER_TEST_CONNECT +echo "show columns from t1 from b" | $USER_TEST_CONNECT + +echo "drop user if exists test" | $BENDSQL_CLIENT_CONNECT +echo "drop role if exists test_role1" | $BENDSQL_CLIENT_CONNECT +echo "drop role if exists test_role2" | $BENDSQL_CLIENT_CONNECT +echo "drop database a" | $BENDSQL_CLIENT_CONNECT +echo "drop database b" | $BENDSQL_CLIENT_CONNECT +echo "drop database c" | $BENDSQL_CLIENT_CONNECT From f639360a79e4efea2fc5691b32c489815a16e42c Mon Sep 17 00:00:00 2001 From: TCeason Date: Wed, 2 Apr 2025 17:56:59 +0800 Subject: [PATCH 2/3] add forward test with revoke --- .github/actions/test_compat_fuse/action.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/actions/test_compat_fuse/action.yml b/.github/actions/test_compat_fuse/action.yml index 3e981f465455d..2c08a39acf4e0 100644 --- a/.github/actions/test_compat_fuse/action.yml +++ b/.github/actions/test_compat_fuse/action.yml @@ -24,6 +24,7 @@ runs: bash ./tests/compat_fuse/test_compat_fuse_forward.sh 1.2.318 1.2.527 rbac bash ./tests/compat_fuse/test_compat_fuse.sh 1.2.680 1.2.680 udf bash ./tests/compat_fuse/test_compat_fuse_forward.sh 1.2.680 1.2.680 udf + bash ./tests/compat_fuse/test_compat_fuse_forward.sh 1.2.680 1.2.680 revoke - name: Upload failure if: failure() uses: ./.github/actions/artifact_failure From ab36eef33eb56b2e2e079f37a9e624a2f7f51727 Mon Sep 17 00:00:00 2001 From: TCeason Date: Wed, 2 Apr 2025 18:52:10 +0800 Subject: [PATCH 3/3] no need to add test --- .github/actions/test_compat_fuse/action.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/actions/test_compat_fuse/action.yml b/.github/actions/test_compat_fuse/action.yml index 2c08a39acf4e0..3e981f465455d 100644 --- a/.github/actions/test_compat_fuse/action.yml +++ b/.github/actions/test_compat_fuse/action.yml @@ -24,7 +24,6 @@ runs: bash ./tests/compat_fuse/test_compat_fuse_forward.sh 1.2.318 1.2.527 rbac bash ./tests/compat_fuse/test_compat_fuse.sh 1.2.680 1.2.680 udf bash ./tests/compat_fuse/test_compat_fuse_forward.sh 1.2.680 1.2.680 udf - bash ./tests/compat_fuse/test_compat_fuse_forward.sh 1.2.680 1.2.680 revoke - name: Upload failure if: failure() uses: ./.github/actions/artifact_failure