From 43455f13b7a05db38aaca84e8150fc68020cd3b2 Mon Sep 17 00:00:00 2001 From: taichong Date: Tue, 6 Feb 2024 12:57:05 +0800 Subject: [PATCH] add table test and add some comment about why not modify get_ownerships --- src/query/service/src/sessions/session_privilege_mgr.rs | 9 ++++----- src/query/users/src/role_mgr.rs | 4 ++++ .../0_stateless/18_rbac/18_0002_ownership_cover.result | 4 ++++ .../0_stateless/18_rbac/18_0002_ownership_cover.sh | 5 +++++ 4 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/query/service/src/sessions/session_privilege_mgr.rs b/src/query/service/src/sessions/session_privilege_mgr.rs index a4a750f6ad19..66581fd5460b 100644 --- a/src/query/service/src/sessions/session_privilege_mgr.rs +++ b/src/query/service/src/sessions/session_privilege_mgr.rs @@ -23,7 +23,6 @@ use databend_common_meta_app::principal::UserInfo; use databend_common_meta_app::principal::UserPrivilegeType; use databend_common_users::GrantObjectVisibilityChecker; use databend_common_users::RoleCacheManager; -use databend_common_users::UserApiProvider; use databend_common_users::BUILTIN_ROLE_ACCOUNT_ADMIN; use databend_common_users::BUILTIN_ROLE_PUBLIC; @@ -270,10 +269,10 @@ impl SessionPrivilegeManager for SessionPrivilegeManagerImpl { async fn has_ownership(&self, object: &OwnershipObject) -> Result { let role_mgr = RoleCacheManager::instance(); let tenant = self.session_ctx.get_current_tenant(); - let owner_role_name = match role_mgr.find_object_owner(&tenant, object).await? { - Some(owner_role) => owner_role, - None => BUILTIN_ROLE_ACCOUNT_ADMIN.to_string(), - }; + let owner_role_name = role_mgr + .find_object_owner(&tenant, object) + .await? + .unwrap_or_else(|| BUILTIN_ROLE_ACCOUNT_ADMIN.to_string()); let effective_roles = self.get_all_effective_roles().await?; let exists = effective_roles.iter().any(|r| r.name == owner_role_name); diff --git a/src/query/users/src/role_mgr.rs b/src/query/users/src/role_mgr.rs index e1e871189444..b812a9d30c1b 100644 --- a/src/query/users/src/role_mgr.rs +++ b/src/query/users/src/role_mgr.rs @@ -168,6 +168,10 @@ impl UserApiProvider { if let Some(owner) = ownership { // if object has ownership, but the owner role is not exists, set owner role to ACCOUNT_ADMIN, // only account_admin can access this object. + // Note: get_ownerships no need to do this check. + // Because this can cause system.table queries to slow down + // The intention is that the account admin can grant ownership. + // So system.tables will display dropped role. It's by design. if !self.exists_role(tenant, owner.role.clone()).await? { Ok(Some(OwnershipInfo { role: BUILTIN_ROLE_ACCOUNT_ADMIN.to_string(), diff --git a/tests/suites/0_stateless/18_rbac/18_0002_ownership_cover.result b/tests/suites/0_stateless/18_rbac/18_0002_ownership_cover.result index 084424e98187..ec5bd068d6d5 100644 --- a/tests/suites/0_stateless/18_rbac/18_0002_ownership_cover.result +++ b/tests/suites/0_stateless/18_rbac/18_0002_ownership_cover.result @@ -39,6 +39,10 @@ t t1 === fix_issue_14572: test drop role; grant ownership === a drop_role +t drop_role a account_admin +t drop_role a drop_role1 +t drop_role1 GRANT OWNERSHIP ON 'default'.'a'.* TO ROLE `drop_role1` +GRANT OWNERSHIP ON 'default'.'a'.'t' TO ROLE `drop_role1` diff --git a/tests/suites/0_stateless/18_rbac/18_0002_ownership_cover.sh b/tests/suites/0_stateless/18_rbac/18_0002_ownership_cover.sh index 2e827bb1d231..5d95f1ddae46 100755 --- a/tests/suites/0_stateless/18_rbac/18_0002_ownership_cover.sh +++ b/tests/suites/0_stateless/18_rbac/18_0002_ownership_cover.sh @@ -140,11 +140,16 @@ echo "grant create on *.* to u1;" | $BENDSQL_CLIENT_CONNECT export USER_U1_CONNECT="bendsql --user=u1 --password=123 --host=${QUERY_MYSQL_HANDLER_HOST} --port ${QUERY_HTTP_HANDLER_PORT}" echo "create database a" | $USER_U1_CONNECT +echo "create table a.t(id int)" | $USER_U1_CONNECT echo "select name, owner from system.databases where name='a'" | $USER_U1_CONNECT +echo "select name, owner from system.tables where database='a'" | $USER_U1_CONNECT echo "drop role drop_role;" | $BENDSQL_CLIENT_CONNECT echo "select name, owner from system.databases where name='a'" | $BENDSQL_CLIENT_CONNECT +echo "select name, owner from system.tables where database='a'" | $USER_U1_CONNECT echo "grant ownership on a.* to role drop_role1;" | $BENDSQL_CLIENT_CONNECT +echo "grant ownership on a.t to role drop_role1;" | $BENDSQL_CLIENT_CONNECT echo "select name, owner from system.databases where name='a'" | $BENDSQL_CLIENT_CONNECT +echo "select name, owner from system.tables where database='a'" | $USER_U1_CONNECT echo "show grants for role drop_role1" | $BENDSQL_CLIENT_CONNECT echo "drop role drop_role1" | $BENDSQL_CLIENT_CONNECT echo "drop user u1" | $BENDSQL_CLIENT_CONNECT