From e65b4b2a0409b25b3ae15863d1642a337d3fdd57 Mon Sep 17 00:00:00 2001 From: August Date: Mon, 6 Feb 2023 17:33:05 +0800 Subject: [PATCH 1/3] fix: grant default conn action for new created user --- src/frontend/src/handler/create_user.rs | 27 +++++++++++++++++++++--- src/meta/src/rpc/service/user_service.rs | 2 -- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/src/frontend/src/handler/create_user.rs b/src/frontend/src/handler/create_user.rs index 5db3f52cc047..643140b43d62 100644 --- a/src/frontend/src/handler/create_user.rs +++ b/src/frontend/src/handler/create_user.rs @@ -15,12 +15,13 @@ use pgwire::pg_response::{PgResponse, StatementType}; use risingwave_common::error::ErrorCode::PermissionDenied; use risingwave_common::error::Result; -use risingwave_pb::user::UserInfo; +use risingwave_pb::user::grant_privilege::{Action, ActionWithGrantOption, Object}; +use risingwave_pb::user::{GrantPrivilege, UserInfo}; use risingwave_sqlparser::ast::{CreateUserStatement, UserOption, UserOptions}; use super::RwPgResponse; use crate::binder::Binder; -use crate::catalog::CatalogError; +use crate::catalog::{CatalogError, DatabaseId}; use crate::handler::HandlerArgs; use crate::user::user_authentication::encrypted_password; @@ -28,6 +29,7 @@ fn make_prost_user_info( user_name: String, options: &UserOptions, session_user: &UserInfo, + database_id: DatabaseId, ) -> Result { if !session_user.is_super { let require_super = options @@ -45,10 +47,22 @@ fn make_prost_user_info( } } + // Since we don't have concept of PUBLIC group yet, here we simply grant new user with CONNECT + // action of session database. + let grant_privileges = vec![GrantPrivilege { + action_with_opts: vec![ActionWithGrantOption { + action: Action::Connect as i32, + with_grant_option: false, + granted_by: session_user.id, + }], + object: Some(Object::DatabaseId(database_id)), + }]; + let mut user_info = UserInfo { name: user_name, // the LOGIN option is implied if it is not explicitly specified. can_login: true, + grant_privileges, ..Default::default() }; @@ -85,6 +99,13 @@ pub async fn handle_create_user( stmt: CreateUserStatement, ) -> Result { let session = handler_args.session; + let database_id = { + let catalog_reader = session.env().catalog_reader().read_guard(); + catalog_reader + .get_database_by_name(session.database()) + .expect("session database should exist") + .id() + }; let user_info = { let user_name = Binder::resolve_user_name(stmt.user_name)?; let user_reader = session.env().user_info_reader().read_guard(); @@ -96,7 +117,7 @@ pub async fn handle_create_user( .get_user_by_name(session.user_name()) .ok_or_else(|| CatalogError::NotFound("user", session.user_name().to_string()))?; - make_prost_user_info(user_name, &stmt.with_options, session_user)? + make_prost_user_info(user_name, &stmt.with_options, session_user, database_id)? }; let user_info_writer = session.env().user_info_writer(); diff --git a/src/meta/src/rpc/service/user_service.rs b/src/meta/src/rpc/service/user_service.rs index b950d8c9327f..be5e8f0851be 100644 --- a/src/meta/src/rpc/service/user_service.rs +++ b/src/meta/src/rpc/service/user_service.rs @@ -27,8 +27,6 @@ use crate::manager::{CatalogManagerRef, IdCategory, MetaSrvEnv}; use crate::storage::MetaStore; use crate::MetaResult; -// TODO: Change user manager as a part of the catalog manager, to ensure that operations on Catalog -// and User are transactional. pub struct UserServiceImpl { env: MetaSrvEnv, From ef8c3035e7f5fae6edda50cc4984a63aa1732107 Mon Sep 17 00:00:00 2001 From: August Date: Mon, 6 Feb 2023 19:03:22 +0800 Subject: [PATCH 2/3] remove unnecessary check --- src/meta/src/manager/catalog/mod.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/meta/src/manager/catalog/mod.rs b/src/meta/src/manager/catalog/mod.rs index 618c23ec26e9..e7d808fbdc83 100644 --- a/src/meta/src/manager/catalog/mod.rs +++ b/src/meta/src/manager/catalog/mod.rs @@ -1614,12 +1614,6 @@ where user.name ))); } - if !user.grant_privileges.is_empty() { - return Err(MetaError::permission_denied(format!( - "Cannot drop user {} with privileges", - id - ))); - } if user_core .user_grant_relation .get(&id) From e6a09e796a48d257d7b80891a87448ff97848109 Mon Sep 17 00:00:00 2001 From: August Date: Mon, 6 Feb 2023 19:17:53 +0800 Subject: [PATCH 3/3] fix ut --- src/frontend/src/handler/create_user.rs | 2 +- src/frontend/src/handler/handle_privilege.rs | 60 ++++++++++++++------ 2 files changed, 45 insertions(+), 17 deletions(-) diff --git a/src/frontend/src/handler/create_user.rs b/src/frontend/src/handler/create_user.rs index 643140b43d62..04df58e48e3e 100644 --- a/src/frontend/src/handler/create_user.rs +++ b/src/frontend/src/handler/create_user.rs @@ -52,7 +52,7 @@ fn make_prost_user_info( let grant_privileges = vec![GrantPrivilege { action_with_opts: vec![ActionWithGrantOption { action: Action::Connect as i32, - with_grant_option: false, + with_grant_option: true, granted_by: session_user.id, }], object: Some(Object::DatabaseId(database_id)), diff --git a/src/frontend/src/handler/handle_privilege.rs b/src/frontend/src/handler/handle_privilege.rs index 7319eeeebe39..63db1c842aa7 100644 --- a/src/frontend/src/handler/handle_privilege.rs +++ b/src/frontend/src/handler/handle_privilege.rs @@ -241,10 +241,16 @@ mod tests { .await .unwrap(); - let database_id = { + let (session_database_id, database_id) = { let catalog_reader = session.env().catalog_reader(); let reader = catalog_reader.read_guard(); - reader.get_database_by_name("db1").unwrap().id() + ( + reader + .get_database_by_name(session.database()) + .unwrap() + .id(), + reader.get_database_by_name("db1").unwrap().id(), + ) }; { @@ -253,21 +259,31 @@ mod tests { let user_info = reader.get_user_by_name("user1").unwrap(); assert_eq!( user_info.grant_privileges, - vec![ProstPrivilege { - action_with_opts: vec![ - ActionWithGrantOption { + vec![ + ProstPrivilege { + action_with_opts: vec![ActionWithGrantOption { action: Action::Connect as i32, with_grant_option: true, - granted_by: DEFAULT_SUPER_USER_ID, - }, - ActionWithGrantOption { - action: Action::Create as i32, - with_grant_option: true, - granted_by: DEFAULT_SUPER_USER_ID, - } - ], - object: Some(ProstObject::DatabaseId(database_id)), - }] + granted_by: session.user_id(), + }], + object: Some(ProstObject::DatabaseId(session_database_id)), + }, + ProstPrivilege { + action_with_opts: vec![ + ActionWithGrantOption { + action: Action::Connect as i32, + with_grant_option: true, + granted_by: DEFAULT_SUPER_USER_ID, + }, + ActionWithGrantOption { + action: Action::Create as i32, + with_grant_option: true, + granted_by: DEFAULT_SUPER_USER_ID, + } + ], + object: Some(ProstObject::DatabaseId(database_id)), + } + ] ); } @@ -282,6 +298,7 @@ mod tests { assert!(user_info .grant_privileges .iter() + .filter(|gp| gp.object == Some(ProstObject::DatabaseId(database_id))) .all(|p| p.action_with_opts.iter().all(|ao| !ao.with_grant_option))); } @@ -293,7 +310,18 @@ mod tests { let user_reader = session.env().user_info_reader(); let reader = user_reader.read_guard(); let user_info = reader.get_user_by_name("user1").unwrap(); - assert!(user_info.grant_privileges.is_empty()); + assert_eq!( + user_info.grant_privileges, + vec![ProstPrivilege { + action_with_opts: vec![ActionWithGrantOption { + action: Action::Connect as i32, + with_grant_option: true, + granted_by: session.user_id(), + }], + object: Some(ProstObject::DatabaseId(session_database_id)), + }] + ); } + frontend.run_sql("DROP USER user1").await.unwrap(); } }