diff --git a/src/handlers/http/modal/query/querier_rbac.rs b/src/handlers/http/modal/query/querier_rbac.rs index c11a24a36..4cf624eb2 100644 --- a/src/handlers/http/modal/query/querier_rbac.rs +++ b/src/handlers/http/modal/query/querier_rbac.rs @@ -442,14 +442,17 @@ pub async fn post_gen_password( req: HttpRequest, userid: web::Path, ) -> Result { - let username = userid.into_inner(); + let userid = userid.into_inner(); let mut new_password = String::default(); let mut new_hash = String::default(); let tenant_id = get_tenant_id_from_request(&req); let caller_userid = get_user_from_request(&req)?; + if Users.is_protected(&userid, &tenant_id) == Some(true) { + return Err(RBACError::ProtectedUser); + } + let _guard = UPDATE_LOCK.lock().await; let mut metadata = get_metadata(&tenant_id).await?; - let _guard = UPDATE_LOCK.lock().await; let user::PassCode { password, hash } = user::Basic::gen_new_password(); new_password.clone_from(&password); new_hash.clone_from(&hash); @@ -460,16 +463,16 @@ pub async fn post_gen_password( user::UserType::Native(ref mut user) => Some(user), _ => None, }) - .find(|user| user.username == username) + .find(|user| user.username == userid) { user.password_hash.clone_from(&hash); } else { return Err(RBACError::UserDoesNotExist); } put_metadata(&metadata, &tenant_id).await?; - Users.change_password_hash(&username, &new_hash, &tenant_id); + Users.change_password_hash(&userid, &new_hash, &tenant_id); - if let Err(e) = sync_password_reset_with_ingestors(req, &username, &caller_userid).await { + if let Err(e) = sync_password_reset_with_ingestors(req, &userid, &caller_userid).await { tracing::error!("{e}"); }; @@ -591,6 +594,45 @@ mod role_escalation { Role::create_user_role(vec![DefaultPrivilege::Reader { resource: None }]) } + #[tokio::test] + async fn post_gen_password_rejects_protected_user_before_storage() { + let _serial = TEST_SERIAL.lock().await; + let (tenant, _, req) = tenant_request(ordinary_role()); + let target = insert_protected_user(&tenant, "admin"); + let before = Users.get_user(&target, &Some(tenant.clone())).unwrap(); + + let Err(err) = post_gen_password(req, web::Path::from(target.clone())).await else { + panic!("a protected user's password must not be reset"); + }; + assert!(matches!(err, RBACError::ProtectedUser), "{err:?}"); + assert_eq!(err.status_code(), StatusCode::BAD_REQUEST); + let after = Users.get_user(&target, &Some(tenant)).unwrap(); + let (UserType::Native(before), UserType::Native(after)) = (before.ty, after.ty) else { + panic!("the protected user must remain native"); + }; + assert_eq!(after.password_hash, before.password_hash); + } + + #[tokio::test] + async fn post_gen_password_waits_for_update_lock_before_storage() { + let _serial = TEST_SERIAL.lock().await; + let (_, userid, req) = tenant_request(ordinary_role()); + let _guard = UPDATE_LOCK.lock().await; + + // Cancel at the lock: resuming would initialize PARSEABLE and make clap + // parse the test arguments. Without the lock before get_metadata, this + // test exits with code 2 instead of timing out. + assert!( + tokio::time::timeout( + std::time::Duration::from_millis(50), + post_gen_password(req, web::Path::from(userid)), + ) + .await + .is_err(), + "password generation must wait for UPDATE_LOCK before reading metadata" + ); + } + #[tokio::test] async fn post_user_rejects_role_held_by_protected_user() { // Other modules' tests watch the RBAC map locks; see `map::TEST_SERIAL`. diff --git a/src/handlers/http/modal/query/querier_role.rs b/src/handlers/http/modal/query/querier_role.rs index e01cf29cd..205e3d796 100644 --- a/src/handlers/http/modal/query/querier_role.rs +++ b/src/handlers/http/modal/query/querier_role.rs @@ -129,6 +129,7 @@ pub async fn delete( req: HttpRequest, name: web::Path, ) -> Result { + let guard = UPDATE_LOCK.lock().await; let name = name.into_inner(); let tenant_id = get_tenant_id_from_request(&req); let tenant = tenant_id.as_deref().unwrap_or(DEFAULT_TENANT); @@ -144,6 +145,9 @@ pub async fn delete( // check if the role is being used by any user or group let mut metadata = get_metadata(&tenant_id).await?; + if metadata.default_role.as_deref() == Some(name.as_str()) { + return Err(RoleError::DefaultRoleInUse); + } if metadata.users.iter().any(|user| user.roles.contains(&name)) { return Err(RoleError::RoleInUse); } @@ -196,6 +200,8 @@ pub async fn delete( } } + drop(guard); + if let Err(e) = sync_role_delete(&req, name.clone(), &tenant_id, &caller_userid).await { tracing::error!("Failed to sync role deletion to cluster nodes: {e}"); } @@ -273,6 +279,42 @@ mod tests { (tenant, req) } + #[tokio::test] + async fn delete_waits_for_update_lock_before_checking_role() { + let _serial = TEST_SERIAL.lock().await; + let (tenant, req) = tenant_request(); + let internal_role = Role::create_internal_role(vec![DefaultPrivilege::Admin]); + mut_roles() + .entry(tenant.clone()) + .or_default() + .insert(ROLE.to_owned(), internal_role.clone()); + let guard = UPDATE_LOCK.lock().await; + let deletion = delete(req, web::Path::from(ROLE.to_owned())); + tokio::pin!(deletion); + + // Poll the handler while the lock is held. An internal role lets it + // complete after release without initializing PARSEABLE or storage. + assert!( + tokio::time::timeout(std::time::Duration::from_millis(50), &mut deletion) + .await + .is_err(), + "role deletion must wait for UPDATE_LOCK" + ); + drop(guard); + + let result = tokio::time::timeout(std::time::Duration::from_secs(1), &mut deletion) + .await + .expect("role deletion must resume after UPDATE_LOCK is released"); + let Err(err) = result else { + panic!("deleting an internal role must be rejected"); + }; + assert!(matches!(err, RoleError::ProtectedRole), "{err:?}"); + assert_eq!( + roles().get(&tenant).and_then(|r| r.get(ROLE)).cloned(), + Some(internal_role) + ); + } + #[tokio::test] async fn rejected_super_admin_update_leaves_live_role_unchanged() { // Other modules' tests watch the RBAC map locks; see `map::TEST_SERIAL`. diff --git a/src/handlers/http/rbac.rs b/src/handlers/http/rbac.rs index c93835d60..98b8b02b3 100644 --- a/src/handlers/http/rbac.rs +++ b/src/handlers/http/rbac.rs @@ -248,9 +248,9 @@ pub async fn post_gen_password( } let mut new_password = String::default(); let mut new_hash = String::default(); + let _guard = UPDATE_LOCK.lock().await; let mut metadata = get_metadata(&tenant_id).await?; - let _guard = UPDATE_LOCK.lock().await; let user::PassCode { password, hash } = user::Basic::gen_new_password(); new_password.clone_from(&password); new_hash.clone_from(&hash); @@ -688,3 +688,35 @@ pub struct RolesResponse { #[serde(skip_serializing_if = "Option::is_none")] pub oidc: Option, } + +#[cfg(test)] +mod tests { + use actix_web::test::TestRequest; + use parking_lot::RwLock; + + use super::*; + use crate::rbac::map::{TEST_SERIAL, USERS}; + + #[tokio::test] + async fn post_gen_password_waits_for_update_lock_before_storage() { + let _serial = TEST_SERIAL.lock().await; + USERS.get_or_init(|| RwLock::new(crate::rbac::map::Users::default())); + let _guard = UPDATE_LOCK.lock().await; + + // Cancel at the lock: resuming would initialize PARSEABLE and make clap + // parse the test arguments. Without the lock before get_metadata, this + // test exits with code 2 instead of timing out. + assert!( + tokio::time::timeout( + std::time::Duration::from_millis(50), + post_gen_password( + TestRequest::default().to_http_request(), + web::Path::from("nobody".to_owned()), + ), + ) + .await + .is_err(), + "password generation must wait for UPDATE_LOCK before reading metadata" + ); + } +}