Skip to content

Commit 10f16a1

Browse files
TCeasonTCeason
authored
refactor(user): redesign ALTER USER implementation with improved API consistency (#18804)
* refactor(user): redesign ALTER USER implementation with improved API consistency - Add new `alter_user` method to UserApi trait for atomic user updates - Replace fragmented auth/option updates with unified user modification flow - Consolidate user validation logic in planner to prevent built-in user alterations - Update privilege checking to use new user info structure - Improve test coverage for user alteration edge cases - Ensure sequence-based consistency for concurrent user modifications * refactor(user): unify user update operations under single upsert_user_info API
1 parent 8e9d3cc commit 10f16a1

File tree

8 files changed

+139
-99
lines changed

8 files changed

+139
-99
lines changed

‎src/query/management/src/user/user_api.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,5 +49,7 @@ pub trait UserApi: Sync + Send {
4949
where
5050
F: FnOnce(&mut UserInfo) + Send;
5151

52+
async fn upsert_user_info(&self, user: &UserInfo, seq: MatchSeq) -> Result<u64>;
53+
5254
async fn drop_user(&self, user: UserIdentity, seq: MatchSeq) -> Result<()>;
5355
}

‎src/query/management/src/user/user_mgr.rs‎

Lines changed: 24 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -61,30 +61,6 @@ impl UserMgr {
6161
let ident = TenantUserIdent::new_user_host(self.tenant.clone(), "dummy", "dummy");
6262
ident.tenant_prefix()
6363
}
64-
65-
#[async_backtrace::framed]
66-
async fn upsert_user_info(
67-
&self,
68-
user_info: &UserInfo,
69-
seq: MatchSeq,
70-
) -> databend_common_exception::Result<u64> {
71-
let key = self.user_key(&user_info.name, &user_info.hostname);
72-
73-
let value = serialize_struct(user_info, ErrorCode::IllegalUserInfoFormat, || "")?;
74-
75-
let kv_api = self.kv_api.clone();
76-
let res = kv_api
77-
.upsert_kv(UpsertKV::new(&key, seq, Operation::Update(value), None))
78-
.await?;
79-
80-
match res.result {
81-
Some(SeqV { seq: s, .. }) => Ok(s),
82-
None => Err(ErrorCode::UnknownUser(format!(
83-
"User '{}' update failed: User does not exist or invalid request.",
84-
user_info.name
85-
))),
86-
}
87-
}
8864
}
8965

9066
#[async_trait::async_trait]
@@ -183,6 +159,30 @@ impl UserApi for UserMgr {
183159
Ok(Some(seq))
184160
}
185161

162+
#[async_backtrace::framed]
163+
async fn upsert_user_info(
164+
&self,
165+
user_info: &UserInfo,
166+
seq: MatchSeq,
167+
) -> databend_common_exception::Result<u64> {
168+
let key = self.user_key(&user_info.name, &user_info.hostname);
169+
170+
let value = serialize_struct(user_info, ErrorCode::IllegalUserInfoFormat, || "")?;
171+
172+
let kv_api = self.kv_api.clone();
173+
let res = kv_api
174+
.upsert_kv(UpsertKV::new(&key, seq, Operation::Update(value), None))
175+
.await?;
176+
177+
match res.result {
178+
Some(SeqV { seq: s, .. }) => Ok(s),
179+
None => Err(ErrorCode::UnknownUser(format!(
180+
"User '{}' update failed: User does not exist or invalid request.",
181+
user_info.name
182+
))),
183+
}
184+
}
185+
186186
#[async_backtrace::framed]
187187
async fn drop_user(&self, user: UserIdentity, seq: MatchSeq) -> Result<()> {
188188
let key = self.user_key(&user.username, &user.hostname);

‎src/query/service/src/interpreters/access/privilege_access.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -901,7 +901,7 @@ impl AccessChecker for PrivilegeAccess {
901901
let user = self.ctx.get_current_user()?;
902902
if let Plan::AlterUser(plan) = plan {
903903
// Alter current user's password do not need to check privileges.
904-
if plan.user.username == user.name && plan.user_option.is_none() {
904+
if plan.user_info.name == user.name && !plan.change_user_option {
905905
return Ok(());
906906
}
907907
}

‎src/query/service/src/interpreters/interpreter_user_alter.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,9 +53,9 @@ impl Interpreter for AlterUserInterpreter {
5353

5454
let plan = self.plan.clone();
5555
let tenant = self.ctx.get_tenant();
56-
if plan.auth_info.is_some() || plan.user_option.is_some() {
56+
if plan.change_auth || plan.change_user_option {
5757
UserApiProvider::instance()
58-
.update_user(&tenant, plan.user, plan.auth_info, plan.user_option)
58+
.alter_user(&tenant, &plan.user_info, plan.seq)
5959
.await?;
6060
}
6161

‎src/query/sql/src/planner/binder/ddl/account.rs‎

Lines changed: 36 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ use databend_common_meta_app::principal::GrantObject;
3737
use databend_common_meta_app::principal::PrincipalIdentity;
3838
use databend_common_meta_app::principal::ProcedureIdentity;
3939
use databend_common_meta_app::principal::ProcedureNameIdent;
40+
use databend_common_meta_app::principal::UserIdentity;
4041
use databend_common_meta_app::principal::UserOption;
4142
use databend_common_meta_app::principal::UserPrivilegeSet;
4243
use databend_common_users::UserApiProvider;
@@ -372,14 +373,29 @@ impl Binder {
372373
auth_option,
373374
user_options,
374375
} = stmt;
375-
// None means current user
376-
let user_info = if user.is_none() {
377-
self.ctx.get_current_user()?
376+
377+
let user_identity = if let Some(user) = user {
378+
UserIdentity::from(user.clone())
378379
} else {
379-
UserApiProvider::instance()
380-
.get_user(&self.ctx.get_tenant(), user.clone().unwrap().into())
381-
.await?
380+
// None means current user
381+
self.ctx.get_current_user()?.identity()
382382
};
383+
let user_api = UserApiProvider::instance();
384+
if user_api
385+
.get_configured_user(&user_identity.username)
386+
.is_some()
387+
{
388+
return Err(ErrorCode::IllegalUser(format!(
389+
"Can not alter built-in user `{}`",
390+
user_identity.username
391+
)));
392+
}
393+
394+
let user_info = UserApiProvider::instance()
395+
.get_meta_user(&self.ctx.get_tenant(), user_identity)
396+
.await?;
397+
let seq = user_info.seq;
398+
let mut user_info = user_info.data;
383399

384400
// TODO: Only user with OWNERSHIP privilege can change user options.
385401
let mut user_option = user_info.option.clone();
@@ -434,15 +450,23 @@ impl Binder {
434450
None
435451
};
436452

437-
let new_user_option = if user_option == user_info.option {
438-
None
439-
} else {
453+
let change_auth = new_auth_info.is_some();
454+
let change_user_option = user_option != user_info.option;
455+
let new_user_option = if change_user_option {
440456
Some(user_option)
457+
} else {
458+
None
441459
};
460+
461+
user_info.update_auth_option(new_auth_info.clone(), new_user_option);
462+
user_info.update_user_time();
463+
user_info.update_auth_history(new_auth_info);
464+
442465
let plan = AlterUserPlan {
443-
user: user_info.identity(),
444-
auth_info: new_auth_info,
445-
user_option: new_user_option,
466+
seq,
467+
user_info,
468+
change_auth,
469+
change_user_option,
446470
};
447471

448472
Ok(Plan::AlterUser(Box::new(plan)))

‎src/query/sql/src/planner/plans/ddl/account.rs‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ use databend_common_meta_app::principal::AuthInfo;
2424
use databend_common_meta_app::principal::GrantObject;
2525
use databend_common_meta_app::principal::PrincipalIdentity;
2626
use databend_common_meta_app::principal::UserIdentity;
27+
use databend_common_meta_app::principal::UserInfo;
2728
use databend_common_meta_app::principal::UserOption;
2829
use databend_common_meta_app::principal::UserPrivilegeSet;
2930
use databend_common_meta_app::schema::CreateOption;
@@ -40,10 +41,10 @@ pub struct CreateUserPlan {
4041

4142
#[derive(Clone, Debug, PartialEq, Eq)]
4243
pub struct AlterUserPlan {
43-
pub user: UserIdentity,
44-
// None means no change to make
45-
pub auth_info: Option<AuthInfo>,
46-
pub user_option: Option<UserOption>,
44+
pub seq: u64,
45+
pub user_info: UserInfo,
46+
pub change_auth: bool,
47+
pub change_user_option: bool,
4748
}
4849

4950
#[derive(Clone, Debug, PartialEq, Eq)]

‎src/query/users/src/user_mgr.rs‎

Lines changed: 47 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -21,15 +21,14 @@ use databend_common_config::GlobalConfig;
2121
use databend_common_exception::ErrorCode;
2222
use databend_common_exception::Result;
2323
use databend_common_management::UserApi;
24-
use databend_common_meta_app::principal::AuthInfo;
2524
use databend_common_meta_app::principal::GrantObject;
2625
use databend_common_meta_app::principal::UserIdentity;
2726
use databend_common_meta_app::principal::UserInfo;
28-
use databend_common_meta_app::principal::UserOption;
2927
use databend_common_meta_app::principal::UserPrivilegeSet;
3028
use databend_common_meta_app::schema::CreateOption;
3129
use databend_common_meta_app::tenant::Tenant;
3230
use databend_common_meta_types::MatchSeq;
31+
use databend_common_meta_types::SeqV;
3332

3433
use crate::role_mgr::BUILTIN_ROLE_ACCOUNT_ADMIN;
3534
use crate::UserApiProvider;
@@ -54,12 +53,20 @@ impl UserApiProvider {
5453
user_info.option.set_all_flag();
5554
Ok(user_info)
5655
} else {
57-
let client = self.user_api(tenant);
58-
let get_user = client.get_user(user, MatchSeq::GE(0));
59-
Ok(get_user.await?.data)
56+
Ok(self.get_meta_user(tenant, user).await?.data)
6057
}
6158
}
6259

60+
pub async fn get_meta_user(
61+
&self,
62+
tenant: &Tenant,
63+
user: UserIdentity,
64+
) -> Result<SeqV<UserInfo>> {
65+
let client = self.user_api(tenant);
66+
let get_user = client.get_user(user, MatchSeq::GE(0)).await?;
67+
Ok(get_user)
68+
}
69+
6370
// Get one user and check client ip if has network policy.
6471
#[async_backtrace::framed]
6572
pub async fn get_user_with_client_ip(
@@ -330,50 +337,37 @@ impl UserApiProvider {
330337
}
331338
}
332339

333-
// Update an user by name and hostname.
334340
#[async_backtrace::framed]
335-
pub async fn update_user(
341+
pub async fn alter_user(
336342
&self,
337343
tenant: &Tenant,
338-
user: UserIdentity,
339-
auth_info: Option<AuthInfo>,
340-
user_option: Option<UserOption>,
344+
user_info: &UserInfo,
345+
seq: u64,
341346
) -> Result<Option<u64>> {
342-
if let Some(ref user_option) = user_option {
343-
if let Some(name) = user_option.network_policy() {
344-
if self.get_network_policy(tenant, name).await.is_err() {
345-
return Err(ErrorCode::UnknownNetworkPolicy(format!(
346-
"network policy `{}` is not exist",
347-
name
348-
)));
349-
}
350-
}
351-
if let Some(name) = user_option.password_policy() {
352-
if self.get_password_policy(tenant, name).await.is_err() {
353-
return Err(ErrorCode::UnknownPasswordPolicy(format!(
354-
"password policy `{}` is not exist",
355-
name
356-
)));
357-
}
347+
let user_option = &user_info.option;
348+
if let Some(name) = user_option.network_policy() {
349+
if self.get_network_policy(tenant, name).await.is_err() {
350+
return Err(ErrorCode::UnknownNetworkPolicy(format!(
351+
"network policy `{}` is not exist",
352+
name
353+
)));
358354
}
359355
}
360-
if self.get_configured_user(&user.username).is_some() {
361-
return Err(ErrorCode::UserAlreadyExists(format!(
362-
"Built-in user `{}` cannot be updated",
363-
user.username
364-
)));
356+
if let Some(name) = user_option.password_policy() {
357+
if self.get_password_policy(tenant, name).await.is_err() {
358+
return Err(ErrorCode::UnknownPasswordPolicy(format!(
359+
"password policy `{}` is not exist",
360+
name
361+
)));
362+
}
365363
}
364+
366365
let client = self.user_api(tenant);
367-
let update_user = client
368-
.update_user_with(user, MatchSeq::GE(1), |ui: &mut UserInfo| {
369-
ui.update_auth_option(auth_info.clone(), user_option);
370-
ui.update_user_time();
371-
ui.update_auth_history(auth_info)
372-
})
366+
let seq = client
367+
.upsert_user_info(user_info, MatchSeq::Exact(seq))
373368
.await;
374-
375-
match update_user {
376-
Ok(res) => Ok(res),
369+
match seq {
370+
Ok(s) => Ok(Some(s)),
377371
Err(e) => Err(e.add_message_back("(while alter user).")),
378372
}
379373
}
@@ -386,10 +380,20 @@ impl UserApiProvider {
386380
user: UserIdentity,
387381
default_role: Option<String>,
388382
) -> Result<Option<u64>> {
389-
let mut user_info = self.get_user(tenant, user.clone()).await?;
383+
let user = self.get_meta_user(tenant, user.clone()).await?;
384+
let seq = user.seq;
385+
let mut user_info = user.data;
390386
user_info.option.set_default_role(default_role);
391-
self.update_user(tenant, user, None, Some(user_info.option))
392-
.await
387+
user_info.update_user_time();
388+
389+
let client = self.user_api(tenant);
390+
let seq = client
391+
.upsert_user_info(&user_info, MatchSeq::Exact(seq))
392+
.await;
393+
match seq {
394+
Ok(s) => Ok(Some(s)),
395+
Err(e) => Err(e.add_message_back("(while alter user).")),
396+
}
393397
}
394398

395399
#[async_backtrace::framed]

‎src/query/users/tests/it/user_mgr.rs‎

Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,11 @@ async fn test_user_manager() -> Result<()> {
198198
.add_user(&tenant, user_info.clone(), &CreateOption::Create)
199199
.await?;
200200

201-
let old_user = user_mgr.get_user(&tenant, user_info.identity()).await?;
201+
let old_user = user_mgr
202+
.get_meta_user(&tenant, user_info.identity())
203+
.await?;
204+
let seq = old_user.seq;
205+
let mut old_user = old_user.data;
202206
assert_eq!(old_user.auth_info.get_password().unwrap(), Vec::from(pwd));
203207

204208
// alter both password & password_type
@@ -208,10 +212,15 @@ async fn test_user_manager() -> Result<()> {
208212
hash_method: PasswordHashMethod::Sha256,
209213
need_change: false,
210214
};
211-
user_mgr
212-
.update_user(&tenant, user_info.identity(), Some(auth_info), None)
215+
old_user.update_auth_option(Some(auth_info.clone()), None);
216+
old_user.update_user_time();
217+
old_user.update_auth_history(Some(auth_info));
218+
user_mgr.alter_user(&tenant, &old_user, seq).await?;
219+
let new_user = user_mgr
220+
.get_meta_user(&tenant, user_info.identity())
213221
.await?;
214-
let new_user = user_mgr.get_user(&tenant, user_info.identity()).await?;
222+
let seq = new_user.seq;
223+
let mut new_user = new_user.data;
215224
assert_eq!(
216225
new_user.auth_info.get_password().unwrap(),
217226
Vec::from(new_pwd)
@@ -228,22 +237,22 @@ async fn test_user_manager() -> Result<()> {
228237
hash_method: PasswordHashMethod::Sha256,
229238
need_change: false,
230239
};
231-
user_mgr
232-
.update_user(&tenant, user_info.identity(), Some(auth_info.clone()), None)
240+
241+
new_user.update_auth_option(Some(auth_info.clone()), None);
242+
new_user.update_user_time();
243+
new_user.update_auth_history(Some(auth_info.clone()));
244+
user_mgr.alter_user(&tenant, &new_user, seq).await?;
245+
let new_new_user = user_mgr
246+
.get_meta_user(&tenant, user_info.identity())
233247
.await?;
234-
let new_new_user = user_mgr.get_user(&tenant, user_info.identity()).await?;
248+
let new_new_user = new_new_user.data;
235249
assert_eq!(
236250
new_new_user.auth_info.get_password().unwrap(),
237251
Vec::from(new_new_pwd)
238252
);
239253

240254
let not_exist = user_mgr
241-
.update_user(
242-
&tenant,
243-
UserIdentity::new("user", hostname),
244-
Some(auth_info.clone()),
245-
None,
246-
)
255+
.alter_user(&tenant, &UserInfo::new("user", hostname, auth_info), 1)
247256
.await;
248257
// ErrorCode::UnknownUser
249258
assert_eq!(not_exist.err().unwrap().code(), 2201)

0 commit comments

Comments
 (0)