fix(rbac): resolve lock-order deadlocks between sessions, users, groups and roles

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

@@ -32,7 +32,7 @@ use crate::{
     parseable::{DEFAULT_TENANT, PARSEABLE},
     rbac::{
         Users,
-        map::{roles, users, write_user_groups},
+        map::{read_user_groups, roles, users, write_user_groups},
         user::{self, UserType},
     },
     tenants::TENANT_METADATA,
@@ -128,27 +128,7 @@ pub async fn delete_user(
     metadata.users.retain(|user| user.userid() != userid);
 
     // also delete from user groups
-    let user_groups = Users.get_user_groups(&userid, &tenant_id);
-    let mut groups_to_update = Vec::new();
-
-    for user_group in user_groups {
-        if let Some(groups) = write_user_groups().get_mut(tenant)
-            && let Some(ug) = groups.get_mut(&user_group)
-            && let Some(users) = users().get(tenant)
-            && let Some(user) = users.get(&userid)
-        {
-            let userid = match &user.ty {
-                UserType::Native(basic) => basic.username.clone(),
-                UserType::OAuth(oauth) => oauth.userid.clone(),
-                UserType::ApiKey(api_key) => api_key.userid.clone(),
-            };
-            ug.remove_users_by_user_ids(HashSet::from_iter([userid]))?;
-            groups_to_update.push(ug.clone());
-        } else {
-            // User not found, skip or log as needed
-            continue;
-        }
-    }
+    let groups_to_update = remove_user_from_groups(&userid, &tenant_id)?;
 
     // For each updated group, replace in place if found; otherwise push
     for updated_group in &groups_to_update {
@@ -163,6 +143,7 @@ pub async fn delete_user(
         }
     }
     put_metadata(&metadata, &tenant_id).await?;
+    apply_user_removal_to_groups(&userid, &tenant_id, &groups_to_update);
 
     if let Err(e) =
         sync_user_deletion_with_ingestors(&req, &userid, &tenant_id, &caller_userid).await
@@ -175,6 +156,57 @@ pub async fn delete_user(
     Ok(HttpResponse::Ok().json(format!("deleted user: {username}")))
 }
 
+/// Returns copies of the user's groups with the member removed, without storing
+/// them. Caller holds `UPDATE_LOCK` and persists them before applying the removal.
+pub(crate) fn remove_user_from_groups(
+    userid: &str,
+    tenant_id: &Option<String>,
+) -> Result<Vec<user::UserGroup>, RBACError> {
+    let tenant = tenant_id.as_deref().unwrap_or(DEFAULT_TENANT);
+    let Some(member_id) = Users
+        .get_user(userid, tenant_id)
+        .map(|user| match &user.ty {
+            UserType::Native(basic) => basic.username.clone(),
+            UserType::OAuth(oauth) => oauth.userid.clone(),
+            UserType::ApiKey(api_key) => api_key.userid.clone(),
+        })
+    else {
+        return Ok(Vec::new());
+    };
+    let mut groups_to_update = Vec::new();
+
+    for user_group in Users.get_user_groups(userid, tenant_id) {
+        // Removing members refreshes sessions, which comes first in the lock
+        // order (see `rbac::map`): update a copy without holding USER_GROUPS.
+        let Some(mut ug) = read_user_groups()
+            .get(tenant)
+            .and_then(|groups| groups.get(&user_group))
+            .cloned()
+        else {
+            continue;
+        };
+        ug.remove_users_by_user_ids(HashSet::from_iter([member_id.clone()]))?;
+        groups_to_update.push(ug);
+    }
+    Ok(groups_to_update)
+}
+
+pub(crate) fn apply_user_removal_to_groups(
+    userid: &str,
+    tenant_id: &Option<String>,
+    groups_to_update: &[user::UserGroup],
+) {
+    let tenant = tenant_id.as_deref().unwrap_or(DEFAULT_TENANT);
+    if let Some(groups) = write_user_groups().get_mut(tenant) {
+        for updated_group in groups_to_update {
+            if let Some(stored) = groups.get_mut(&updated_group.name) {
+                // Preserve any other membership or role changes since the copy was made.
+                stored.users.retain(|member| member.userid() != userid);
+            }
+        }
+    }
+}
+
 // Handler PATCH /user/{userid}/role/add => Add roles to a user
 pub async fn add_roles_to_user(
     req: HttpRequest,

src/handlers/http/oidc.rs

@@ -36,7 +36,7 @@ static COOKIE_REQUIRE_CROSS_SITE: AtomicBool = AtomicBool::new(false);
 pub fn set_cookie_cross_site(enabled: bool) {
     COOKIE_REQUIRE_CROSS_SITE.store(enabled, Ordering::Relaxed);
 }
-use chrono::TimeDelta;
+use chrono::{TimeDelta, Utc};
 use openid::Bearer;
 use serde::Deserialize;
 use tokio::sync::RwLock;
@@ -124,29 +124,20 @@ pub async fn login(
     let tenant_id = get_tenant_id_from_key(&session_key);
     match session_key {
         // We can exchange basic auth for session cookie
-        SessionKey::BasicAuth { username, password } => match Users.get_user(&username, &tenant_id)
-        {
-            Some(
-                ref user @ User {
-                    ty: UserType::Native(ref basic),
-                    ..
-                },
-            ) if basic.verify_password(&password) => {
-                let user_cookie = cookie_username(&username);
-                let user_id_cookie = cookie_userid(&username);
-                let session_cookie = exchange_basic_for_cookie(
-                    user,
-                    SessionKey::BasicAuth { username, password },
-                    EXPIRY_DURATION,
-                );
-
-                Ok(redirect_to_client(
-                    query.redirect.as_str(),
-                    [user_cookie, user_id_cookie, session_cookie],
-                ))
+        SessionKey::BasicAuth { username, password } => {
+            match exchange_basic_for_cookie(&username, &password, &tenant_id, EXPIRY_DURATION) {
+                Some(session_cookie) => {
+                    let user_cookie = cookie_username(&username);
+                    let user_id_cookie = cookie_userid(&username);
+
+                    Ok(redirect_to_client(
+                        query.redirect.as_str(),
+                        [user_cookie, user_id_cookie, session_cookie],
+                    ))
+                }
+                None => Err(OIDCError::BadRequest("Bad Request".to_string())),
             }
-            _ => Err(OIDCError::BadRequest("Bad Request".to_string())),
-        },
+        }
         // if it's a valid active session, just redirect back
         key @ SessionKey::SessionId(_) => {
             let resp = if Users.session_exists(&key) {
@@ -510,15 +501,28 @@ fn callback_base_url(req: &HttpRequest) -> String {
     format!("{}://{host}/", PARSEABLE.options.get_scheme())
 }
 
-fn exchange_basic_for_cookie(
-    user: &User,
-    key: SessionKey,
+/// Re-verifies the Basic credentials and swaps their session for a cookie one.
+/// `new_basic_session` re-reads the user under SESSIONS, so an admin change
+/// racing the login cannot leave a cookie session with stale credentials.
+pub(crate) fn exchange_basic_for_cookie(
+    username: &str,
+    password: &str,
+    tenant_id: &Option<String>,
     expires_in: TimeDelta,
-) -> Cookie<'static> {
+) -> Option<Cookie<'static>> {
     let id = Ulid::new();
-    Users.remove_session(&key);
-    Users.new_session(user, SessionKey::SessionId(id), expires_in);
-    cookie_session(id)
+    let mut sessions = Users.new_basic_session(
+        username,
+        password,
+        tenant_id.as_deref().unwrap_or(DEFAULT_TENANT),
+        SessionKey::SessionId(id),
+        Utc::now() + expires_in,
+    )?;
+    sessions.remove_session(&SessionKey::BasicAuth {
+        username: username.to_owned(),
+        password: password.to_owned(),
+    });
+    Some(cookie_session(id))
 }
 
 fn redirect_to_oidc_logout(mut logout_endpoint: Url, redirect: &Url) -> HttpResponse {

src/rbac/map.rs

@@ -40,6 +40,12 @@ use serde::{Deserialize, Serialize};
 
 pub type Roles = HashMap<String, HashMap<String, Role>>;
 
+// Lock order: SESSIONS -> USERS -> USER_GROUPS -> ROLES -> DEFAULT_ROLE.
+// Never acquire a map while holding one later in this order, and never take a
+// second guard on a map this thread already holds (even two reads): these
+// locks are task-fair, so a queued writer blocks every new reader.
+// `Sessions::check_auth` reads USERS, USER_GROUPS and ROLES, so its caller
+// must hold no guard other than the SESSIONS one it is called on.
 pub static USERS: OnceCell<RwLock<Users>> = OnceCell::new();
 pub static ROLES: OnceCell<RwLock<Roles>> = OnceCell::new();
 pub static DEFAULT_ROLE: Lazy<RwLock<HashMap<String, Option<String>>>> =