From ca3d13546c781a78b452f24ea1df96ca57866a3f Mon Sep 17 00:00:00 2001 From: Bohu Date: Sat, 27 Jan 2024 22:59:10 +0800 Subject: [PATCH] Revert "feat(rbac): ensure the builtin roles on auth" (#14497) --- src/meta/app/src/principal/user_grant.rs | 10 --- src/meta/app/tests/it/user_grant.rs | 4 - src/query/management/src/role/role_api.rs | 2 +- src/query/management/src/role/role_mgr.rs | 4 +- src/query/service/src/auth.rs | 7 -- .../interpreters/interpreter_role_grant.rs | 2 +- .../src/interpreters/interpreter_setting.rs | 59 +++++++------ .../interpreters/interpreter_show_grants.rs | 4 +- .../settings/src/settings_getter_setter.rs | 9 +- src/query/users/src/role_cache_mgr.rs | 2 +- src/query/users/src/role_mgr.rs | 83 ++----------------- src/query/users/src/user_api.rs | 25 +++++- src/query/users/tests/it/role_mgr.rs | 40 +-------- 13 files changed, 78 insertions(+), 173 deletions(-) diff --git a/src/meta/app/src/principal/user_grant.rs b/src/meta/app/src/principal/user_grant.rs index 183f8f947182..5610e121fc47 100644 --- a/src/meta/app/src/principal/user_grant.rs +++ b/src/meta/app/src/principal/user_grant.rs @@ -232,16 +232,6 @@ impl UserGrantSet { self.roles.remove(role); } - pub fn find_object_granted_privileges(&self, object: &GrantObject) -> UserPrivilegeSet { - let mut privileges = UserPrivilegeSet::empty(); - for entry in self.entries.iter() { - if entry.matches_entry(object) { - privileges |= entry.privileges.into(); - } - } - privileges - } - pub fn verify_privilege( &self, object: &GrantObject, diff --git a/src/meta/app/tests/it/user_grant.rs b/src/meta/app/tests/it/user_grant.rs index afc6c61e3a0e..40aed214ec94 100644 --- a/src/meta/app/tests/it/user_grant.rs +++ b/src/meta/app/tests/it/user_grant.rs @@ -185,9 +185,6 @@ fn test_user_grant_entry() -> Result<()> { fn test_user_grant_set() -> Result<()> { let mut grants = UserGrantSet::empty(); - // GRANT CREATE ON *.* TO 'user1'; - // GRANT INSERT ON *.* TO 'user1'; - // GRANT SELECT, CREATE ON db1.table1 TO 'user1'; grants.grant_privileges( &GrantObject::Global, make_bitflags!(UserPrivilegeType::{Create}).into(), @@ -206,7 +203,6 @@ fn test_user_grant_set() -> Result<()> { ); assert_eq!(2, grants.entries().len()); - // REVOKE INSERT ON *.* FROM 'user1'; grants.revoke_privileges( &GrantObject::Global, make_bitflags!(UserPrivilegeType::{Insert}).into(), diff --git a/src/query/management/src/role/role_api.rs b/src/query/management/src/role/role_api.rs index 34769811a08d..ad402fa0b03a 100644 --- a/src/query/management/src/role/role_api.rs +++ b/src/query/management/src/role/role_api.rs @@ -36,7 +36,7 @@ pub trait RoleApi: Sync + Send { /// /// Seq number ensures there is no other write happens between get and set. #[allow(clippy::ptr_arg)] - async fn update_role_with(&self, role: &str, seq: MatchSeq, f: F) -> Result> + async fn update_role_with(&self, role: &String, seq: MatchSeq, f: F) -> Result> where F: FnOnce(&mut RoleInfo) + Send; /// Grant ownership would transfer ownership of a object from one role to another role diff --git a/src/query/management/src/role/role_mgr.rs b/src/query/management/src/role/role_mgr.rs index 8077b296e1a1..ac816f7c1e7f 100644 --- a/src/query/management/src/role/role_mgr.rs +++ b/src/query/management/src/role/role_mgr.rs @@ -249,7 +249,7 @@ impl RoleApi for RoleMgr { #[minitrace::trace] async fn update_role_with( &self, - role: &str, + role: &String, seq: MatchSeq, f: F, ) -> Result, ErrorCode> @@ -260,7 +260,7 @@ impl RoleApi for RoleMgr { seq, data: mut role_info, .. - } = self.get_role(&role.to_string(), seq).await?; + } = self.get_role(role, seq).await?; f(&mut role_info); diff --git a/src/query/service/src/auth.rs b/src/query/service/src/auth.rs index 84a651c8a576..a123f9dc7a9c 100644 --- a/src/query/service/src/auth.rs +++ b/src/query/service/src/auth.rs @@ -121,9 +121,6 @@ impl AuthMgr { } }; - UserApiProvider::instance() - .ensure_builtin_roles(&tenant) - .await?; session.set_authed_user(user, jwt.custom.role).await?; } Credential::Password { @@ -164,13 +161,9 @@ impl AuthMgr { authed?; - UserApiProvider::instance() - .ensure_builtin_roles(&tenant) - .await?; session.set_authed_user(user, None).await?; } }; - Ok(()) } } diff --git a/src/query/service/src/interpreters/interpreter_role_grant.rs b/src/query/service/src/interpreters/interpreter_role_grant.rs index 1733a01a9791..02dbc460567e 100644 --- a/src/query/service/src/interpreters/interpreter_role_grant.rs +++ b/src/query/service/src/interpreters/interpreter_role_grant.rs @@ -56,7 +56,7 @@ impl Interpreter for GrantRoleInterpreter { // TODO: check privileges // Check if the grant role exists. - user_mgr.get_role(&tenant, &plan.role).await?; + user_mgr.get_role(&tenant, plan.role.clone()).await?; match plan.principal { PrincipalIdentity::User(user) => { user_mgr diff --git a/src/query/service/src/interpreters/interpreter_setting.rs b/src/query/service/src/interpreters/interpreter_setting.rs index 2762dbbeed96..a742423fa042 100644 --- a/src/query/service/src/interpreters/interpreter_setting.rs +++ b/src/query/service/src/interpreters/interpreter_setting.rs @@ -15,12 +15,9 @@ use std::sync::Arc; use chrono_tz::Tz; -use databend_common_config::GlobalConfig; use databend_common_exception::ErrorCode; use databend_common_exception::Result; use databend_common_sql::plans::SettingPlan; -use databend_common_sql::plans::VarValue; -use databend_common_users::UserApiProvider; use crate::interpreters::Interpreter; use crate::pipelines::PipelineBuildResult; @@ -37,19 +34,6 @@ impl SettingInterpreter { pub fn try_create(ctx: Arc, set: SettingPlan) -> Result { Ok(SettingInterpreter { ctx, set }) } - - async fn set_setting_by_var(&self, var: &VarValue, value: String) -> Result<()> { - let settings = self.ctx.get_shared_settings(); - - match var.is_global { - true => { - settings - .set_global_setting(var.variable.clone(), value) - .await - } - false => settings.set_setting(var.variable.clone(), value).await, - } - } } #[async_trait::async_trait] @@ -74,26 +58,39 @@ impl Interpreter for SettingInterpreter { let _ = tz.parse::().map_err(|_| { ErrorCode::InvalidTimezone(format!("Invalid Timezone: {}", var.value)) })?; + let settings = self.ctx.get_shared_settings(); - self.set_setting_by_var(&var, tz.to_string()).await?; - true - } - "sandbox_tenant" => { - // only used in sqlogictest, it will create a sandbox tenant on every sqlogictest cases - // and switch to it by SET sandbox_tenant = xxx; - let config = GlobalConfig::instance(); - let tenant = var.value.clone(); - if config.query.internal_enable_sandbox_tenant && !tenant.is_empty() { - UserApiProvider::instance() - .ensure_builtin_roles(&tenant) - .await?; - } + match var.is_global { + true => { + settings + .set_global_setting(var.variable.clone(), tz.to_string()) + .await + } + false => { + settings + .set_setting(var.variable.clone(), tz.to_string()) + .await + } + }?; - self.set_setting_by_var(&var, var.value.clone()).await?; true } _ => { - self.set_setting_by_var(&var, var.value.clone()).await?; + let settings = self.ctx.get_shared_settings(); + + match var.is_global { + true => { + settings + .set_global_setting(var.variable.clone(), var.value.clone()) + .await + } + false => { + settings + .set_setting(var.variable.clone(), var.value.clone()) + .await + } + }?; + true } }; diff --git a/src/query/service/src/interpreters/interpreter_show_grants.rs b/src/query/service/src/interpreters/interpreter_show_grants.rs index 93f3ad0a67cc..f3800577f432 100644 --- a/src/query/service/src/interpreters/interpreter_show_grants.rs +++ b/src/query/service/src/interpreters/interpreter_show_grants.rs @@ -65,7 +65,9 @@ impl Interpreter for ShowGrantsInterpreter { (user.identity().to_string(), user.grants) } PrincipalIdentity::Role(role) => { - let role = UserApiProvider::instance().get_role(&tenant, role).await?; + let role = UserApiProvider::instance() + .get_role(&tenant, role.clone()) + .await?; (format!("ROLE `{}`", role.identity()), role.grants) } }, diff --git a/src/query/settings/src/settings_getter_setter.rs b/src/query/settings/src/settings_getter_setter.rs index 0c2924a2d343..51b18e712f6d 100644 --- a/src/query/settings/src/settings_getter_setter.rs +++ b/src/query/settings/src/settings_getter_setter.rs @@ -13,9 +13,11 @@ // limitations under the License. use databend_common_ast::Dialect; +use databend_common_config::GlobalConfig; use databend_common_exception::ErrorCode; use databend_common_exception::Result; use databend_common_meta_app::principal::UserSettingValue; +use databend_common_users::UserApiProvider; use crate::settings::Settings; use crate::settings_default::DefaultSettings; @@ -117,7 +119,12 @@ impl Settings { let (key, value) = DefaultSettings::convert_value(k.clone(), v)?; if key == "sandbox_tenant" { - log::info!("switch sandbox tenant to {}", value); + let config = GlobalConfig::instance(); + let tenant = value.as_string(); + if config.query.internal_enable_sandbox_tenant && !tenant.is_empty() { + UserApiProvider::try_create_simple(config.meta.to_meta_grpc_client_conf(), &tenant) + .await?; + } } self.changes.insert(key, ChangeValue { diff --git a/src/query/users/src/role_cache_mgr.rs b/src/query/users/src/role_cache_mgr.rs index 2d8ffdc184a9..9d7e0e4409ab 100644 --- a/src/query/users/src/role_cache_mgr.rs +++ b/src/query/users/src/role_cache_mgr.rs @@ -125,7 +125,7 @@ impl RoleCacheManager { }; // cache manager would not look into built-in roles. - let role = self.user_manager.get_role(tenant, &owner.role).await?; + let role = self.user_manager.get_role(tenant, owner.role).await?; Ok(Some(role)) } diff --git a/src/query/users/src/role_mgr.rs b/src/query/users/src/role_mgr.rs index 48455c14fec2..6b613e073730 100644 --- a/src/query/users/src/role_mgr.rs +++ b/src/query/users/src/role_mgr.rs @@ -23,7 +23,6 @@ use databend_common_meta_app::principal::OwnershipObject; use databend_common_meta_app::principal::RoleInfo; use databend_common_meta_app::principal::UserPrivilegeSet; use databend_common_meta_types::MatchSeq; -use log::info; use crate::role_util::find_all_related_roles; use crate::UserApiProvider; @@ -34,29 +33,12 @@ pub const BUILTIN_ROLE_PUBLIC: &str = "public"; impl UserApiProvider { // Get one role from by tenant. #[async_backtrace::framed] - pub async fn get_role(&self, tenant: &str, role: &str) -> Result { + pub async fn get_role(&self, tenant: &str, role: String) -> Result { let client = self.get_role_api_client(tenant)?; - let role_data = client - .get_role(&role.to_string(), MatchSeq::GE(0)) - .await? - .data; + let role_data = client.get_role(&role, MatchSeq::GE(0)).await?.data; Ok(role_data) } - #[async_backtrace::framed] - pub async fn get_role_optional(&self, tenant: &str, role: &str) -> Result> { - match self.get_role(tenant, role).await { - Ok(r) => Ok(Some(r)), - Err(err) => { - if err.code() == ErrorCode::UNKNOWN_ROLE { - Ok(None) - } else { - return Err(err); - } - } - } - } - // Get the tenant all roles list. #[async_backtrace::framed] pub async fn get_roles(&self, tenant: &str) -> Result> { @@ -70,55 +52,6 @@ impl UserApiProvider { Ok(roles) } - /// Ensure the builtin roles account_admin and public exists. Please note that there's - /// a corner case that if we added another privilege type, we should add it to the - /// existed account_admin role. - /// - /// This function have two calling places: - /// 1. when the server starts - /// 2. when the user is authenticated - #[async_backtrace::framed] - pub async fn ensure_builtin_roles(&self, tenant: &str) -> Result<()> { - // CREATE ROLE IF NOT EXISTS public; - let public = RoleInfo::new(BUILTIN_ROLE_PUBLIC); - self.add_role(tenant, public, true).await?; - - // if not exists, create account_admin. - // if new privilege type on Global added, grant it to account_admin. - let existed_account_admin = self - .get_role_optional(tenant, BUILTIN_ROLE_ACCOUNT_ADMIN) - .await?; - if existed_account_admin.is_none() { - let account_admin = { - let mut r = RoleInfo::new(BUILTIN_ROLE_ACCOUNT_ADMIN); - r.grants.grant_privileges( - &GrantObject::Global, - UserPrivilegeSet::available_privileges_on_global(), - ); - r - }; - self.add_role(tenant, account_admin, true).await?; - } else if existed_account_admin - .unwrap() - .grants - .find_object_granted_privileges(&GrantObject::Global) - != UserPrivilegeSet::available_privileges_on_global() - { - info!( - "new privilege type on GrantObject::Global detected, sync it to role account_admin" - ); - self.grant_privileges_to_role( - tenant, - BUILTIN_ROLE_ACCOUNT_ADMIN, - GrantObject::Global, - UserPrivilegeSet::available_privileges_on_global(), - ) - .await?; - } - - Ok(()) - } - #[async_backtrace::framed] pub async fn get_ownerships(&self, tenant: &str) -> Result> { let seq_owns = self @@ -135,7 +68,7 @@ impl UserApiProvider { } #[async_backtrace::framed] - pub async fn exists_role(&self, tenant: &str, role: &str) -> Result { + pub async fn exists_role(&self, tenant: &str, role: String) -> Result { match self.get_role(tenant, role).await { Ok(_) => Ok(true), Err(e) => { @@ -156,7 +89,7 @@ impl UserApiProvider { role_info: RoleInfo, if_not_exists: bool, ) -> Result { - if if_not_exists && self.exists_role(tenant, &role_info.name).await? { + if if_not_exists && self.exists_role(tenant, role_info.name.clone()).await? { return Ok(0); } @@ -182,7 +115,7 @@ impl UserApiProvider { new_role: &str, ) -> Result<()> { // from and to role must exists - self.get_role(tenant, new_role).await?; + self.get_role(tenant, new_role.to_string()).await?; let client = self.get_role_api_client(tenant)?; client @@ -209,7 +142,7 @@ impl UserApiProvider { pub async fn grant_privileges_to_role( &self, tenant: &str, - role: &str, + role: &String, object: GrantObject, privileges: UserPrivilegeSet, ) -> Result> { @@ -226,7 +159,7 @@ impl UserApiProvider { pub async fn revoke_privileges_from_role( &self, tenant: &str, - role: &str, + role: &String, object: GrantObject, privileges: UserPrivilegeSet, ) -> Result> { @@ -271,7 +204,7 @@ impl UserApiProvider { pub async fn revoke_role_from_role( &self, tenant: &str, - role: &str, + role: &String, revoke_role: &String, ) -> Result> { let client = self.get_role_api_client(tenant)?; diff --git a/src/query/users/src/user_api.rs b/src/query/users/src/user_api.rs index 36d005245c0f..3040676e9b7a 100644 --- a/src/query/users/src/user_api.rs +++ b/src/query/users/src/user_api.rs @@ -39,6 +39,9 @@ use databend_common_management::UdfMgr; use databend_common_management::UserApi; use databend_common_management::UserMgr; use databend_common_meta_app::principal::AuthInfo; +use databend_common_meta_app::principal::GrantObject; +use databend_common_meta_app::principal::RoleInfo; +use databend_common_meta_app::principal::UserPrivilegeSet; use databend_common_meta_app::tenant::TenantQuota; use databend_common_meta_kvapi::kvapi; use databend_common_meta_store::MetaStore; @@ -47,6 +50,8 @@ use databend_common_meta_types::MatchSeq; use databend_common_meta_types::MetaError; use crate::idm_config::IDMConfig; +use crate::BUILTIN_ROLE_ACCOUNT_ADMIN; +use crate::BUILTIN_ROLE_PUBLIC; pub struct UserApiProvider { meta: MetaStore, @@ -85,8 +90,24 @@ impl UserApiProvider { idm_config, }; - // create builtin roles account_admin & public if not exists. - user_mgr.ensure_builtin_roles(tenant).await?; + // init built-in role + // Currently we have two builtin roles: + // 1. ACCOUNT_ADMIN, which has the equivalent privileges of `GRANT ALL ON *.* TO ROLE account_admin`, + // it also contains all roles. ACCOUNT_ADMIN can access the data objects which owned by any role. + // 2. PUBLIC, on the other side only includes the public accessible privileges, but every role + // contains the PUBLIC role. The data objects which owned by PUBLIC can be accessed by any role. + { + let mut account_admin = RoleInfo::new(BUILTIN_ROLE_ACCOUNT_ADMIN); + account_admin.grants.grant_privileges( + &GrantObject::Global, + UserPrivilegeSet::available_privileges_on_global(), + ); + user_mgr.add_role(tenant, account_admin, true).await?; + + let public = RoleInfo::new(BUILTIN_ROLE_PUBLIC); + user_mgr.add_role(tenant, public, true).await?; + } + Ok(Arc::new(user_mgr)) } diff --git a/src/query/users/tests/it/role_mgr.rs b/src/query/users/tests/it/role_mgr.rs index 14782cbb3b95..d7bb4ed05c69 100644 --- a/src/query/users/tests/it/role_mgr.rs +++ b/src/query/users/tests/it/role_mgr.rs @@ -53,7 +53,7 @@ async fn test_role_manager() -> Result<()> { // get role { - let role = role_mgr.get_role(tenant, &role_name).await?; + let role = role_mgr.get_role(tenant, role_name.clone()).await?; assert_eq!(role.name, "test-role1"); } @@ -79,7 +79,7 @@ async fn test_role_manager() -> Result<()> { UserPrivilegeSet::all_privileges(), ) .await?; - let role = role_mgr.get_role(tenant, &role_name).await?; + let role = role_mgr.get_role(tenant, role_name.clone()).await?; assert!( role.grants .verify_privilege(&GrantObject::Global, vec![UserPrivilegeType::Alter]) @@ -97,43 +97,9 @@ async fn test_role_manager() -> Result<()> { ) .await?; - let role = role_mgr.get_role(tenant, &role_name).await?; + let role = role_mgr.get_role(tenant, role_name.clone()).await?; assert_eq!(role.grants.entries().len(), 0); } Ok(()) } - -#[tokio::test(flavor = "multi_thread", worker_threads = 1)] -async fn test_ensure_builtin_roles() -> Result<()> { - let conf = RpcClientConf::default(); - let tenant = "tenant1"; - let role_mgr = UserApiProvider::try_create_simple(conf, tenant).await?; - - // revoke super privilege from account_admin - role_mgr - .revoke_privileges_from_role( - tenant, - "account_admin", - GrantObject::Global, - UserPrivilegeType::Super.into(), - ) - .await?; - let account_admin = role_mgr.get_role(tenant, "account_admin").await?; - assert!( - !account_admin - .grants - .verify_privilege(&GrantObject::Global, vec![UserPrivilegeType::Super]) - ); - - // ensure builtin roles, account_admin should recover the super privilege - role_mgr.ensure_builtin_roles(tenant).await?; - let account_admin = role_mgr.get_role(tenant, "account_admin").await?; - assert!( - account_admin - .grants - .verify_privilege(&GrantObject::Global, vec![UserPrivilegeType::Super]) - ); - - Ok(()) -}