Take UPDATE_LOCK in querier role DELETE and before password resets

src/handlers/http/modal/query/querier_rbac.rs

@@ -442,14 +442,17 @@ pub async fn post_gen_password(
     req: HttpRequest,
     userid: web::Path<String>,
 ) -> Result<impl Responder, RBACError> {
-    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`.

src/handlers/http/modal/query/querier_role.rs

@@ -129,6 +129,7 @@ pub async fn delete(
     req: HttpRequest,
     name: web::Path<String>,
 ) -> Result<impl Responder, RoleError> {
+    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`.

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<serde_json::Value>,
 }
+
+#[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"
+        );
+    }
+}