From aefa8bc048c3260ac8660431f49a8533289998c6 Mon Sep 17 00:00:00 2001 From: VibeSWE <338779468+VibeSWE@users.noreply.github.com> Date: Fri, 23 Oct 2026 22:58:13 -0500 Subject: [PATCH 01/13] Add failing tests for API key role escalation and querier role update Expected RED until fixed: - apikeys validate_roles_rejects_internal_role and validate_roles_rejects_super_admin_privilege_role: validate_roles only checks that roles exist, so an internal role or one holding SuperAdmin is accepted (Ok(())) instead of RBACError::ProtectedRole. - apikeys tenant_admin_cannot_create_super_admin_api_key: a tenant admin (Admin, not SuperAdmin) reaches create_api_key with role super-admin or a SuperAdmin user role and gets past the role check. A duplicate key name stops the vulnerable handler with 409 before storage; the fix should return 400 ProtectedRole first. - querier_role rejected_super_admin_update_leaves_live_role_unchanged: put writes the body into the live roles map before the SuperAdmin check, so the request returns SuperAdminPrivilege but the in-memory role now holds SuperAdmin. Expected GREEN (guards): - apikeys validate_roles_accepts_ordinary_role. - role put_tests rejected_super_admin_update_leaves_live_role_unchanged: the standalone handler validates the body before touching any state. Co-Authored-By: Claude Opus 5.5 --- src/handlers/http/apikeys.rs | 145 ++++++++++++++++++ src/handlers/http/modal/query/querier_role.rs | 87 +++++++++++ src/handlers/http/role.rs | 43 ++++++ 3 files changed, 275 insertions(+) diff --git a/src/handlers/http/apikeys.rs b/src/handlers/http/apikeys.rs index fc2320efb..05977a5f8 100644 --- a/src/handlers/http/apikeys.rs +++ b/src/handlers/http/apikeys.rs @@ -389,4 +389,149 @@ mod tests { assert!(detail.get("expiresAt").unwrap().is_null()); assert!(entry.get("expiresAt").unwrap().is_null()); } + + mod role_escalation { + use std::collections::HashMap; + + use actix_web::{HttpMessage, ResponseError, http::StatusCode, test::TestRequest}; + use chrono::TimeDelta; + use parking_lot::RwLock; + + use super::*; + use crate::rbac::{ + map::{ + ROLES, SESSIONS, SessionKey, Sessions, USER_GROUPS, USERS, UserGroups, + Users as UserMap, mut_roles, mut_users, + }, + role::model::{DefaultPrivilege, Role}, + user::Basic, + }; + + const ADMIN_ROLE: &str = "tenant-admin"; + const READER_ROLE: &str = "reader"; + // Mirrors the internal role `rbac::map::init` adds to the default tenant. + const SUPER_ADMIN_ROLE: &str = "super-admin"; + // A user-typed role holding SuperAdmin, e.g. from legacy metadata. + const ELEVATED_ROLE: &str = "elevated"; + + // `rbac::map::init` needs `PARSEABLE`, so tests create the maps directly. + fn init_auth_maps() { + USERS.get_or_init(|| RwLock::new(UserMap::default())); + SESSIONS.get_or_init(|| RwLock::new(Sessions::default())); + USER_GROUPS.get_or_init(|| RwLock::new(UserGroups::default())); + ROLES.get_or_init(|| RwLock::new(HashMap::new())); + } + + /// A fresh tenant laid out like the default tenant after `map::init`. + fn tenant() -> String { + init_auth_maps(); + let tenant = format!("apikey-escalation-{}", Ulid::new()); + mut_roles().insert( + tenant.clone(), + HashMap::from([ + ( + ADMIN_ROLE.to_owned(), + Role::create_user_role(vec![DefaultPrivilege::Admin]), + ), + ( + READER_ROLE.to_owned(), + Role::create_user_role(vec![DefaultPrivilege::Reader { resource: None }]), + ), + ( + SUPER_ADMIN_ROLE.to_owned(), + Role::create_internal_role(vec![DefaultPrivilege::SuperAdmin]), + ), + ( + ELEVATED_ROLE.to_owned(), + Role::create_user_role(vec![DefaultPrivilege::SuperAdmin]), + ), + ]), + ); + tenant + } + + fn role_set(name: &str) -> HashSet { + HashSet::from([name.to_owned()]) + } + + /// A request from a tenant admin (Admin, not SuperAdmin) with a live session. + fn tenant_admin_request(tenant: &str) -> HttpRequest { + let admin = User { + ty: UserType::Native(Basic { + username: format!("admin-{}", Ulid::new()), + password_hash: String::new(), + email: None, + }), + roles: role_set(ADMIN_ROLE), + user_groups: HashSet::new(), + tenant: Some(tenant.to_owned()), + protected: false, + }; + mut_users().insert(admin.clone()); + let session = SessionKey::SessionId(Ulid::new()); + Users.new_session(&admin, session.clone(), TimeDelta::minutes(5)); + let req = TestRequest::default().to_http_request(); + req.extensions_mut().insert(session); + req + } + + #[test] + fn validate_roles_accepts_ordinary_role() { + let tenant = Some(tenant()); + assert!(validate_roles(&role_set(READER_ROLE), &tenant).is_ok()); + } + + #[test] + fn validate_roles_rejects_internal_role() { + let tenant = Some(tenant()); + let result = validate_roles(&role_set(SUPER_ADMIN_ROLE), &tenant); + assert!( + matches!(result, Err(ApiKeyError::Rbac(RBACError::ProtectedRole))), + "internal role must be rejected like the native-user path, got {result:?}" + ); + } + + #[test] + fn validate_roles_rejects_super_admin_privilege_role() { + let tenant = Some(tenant()); + let result = validate_roles(&role_set(ELEVATED_ROLE), &tenant); + assert!( + matches!(result, Err(ApiKeyError::Rbac(RBACError::ProtectedRole))), + "role holding SuperAdmin must be rejected like the native-user path, got {result:?}" + ); + } + + #[tokio::test] + async fn tenant_admin_cannot_create_super_admin_api_key() { + let tenant = tenant(); + let req = tenant_admin_request(&tenant); + // An existing key with the same name makes the vulnerable handler stop + // with 409 before it reaches storage, which tests cannot initialise. + let key_name = "escalation".to_owned(); + mut_users().insert(User::new_api_key( + Ulid::new(), + format!("existing-{}", Ulid::new()), + key_name.clone(), + HashSet::new(), + "admin".into(), + Some(tenant.clone()), + )); + + for role in [SUPER_ADMIN_ROLE, ELEVATED_ROLE] { + let body = CreateApiKeyRequest { + key_name: key_name.clone(), + roles: role_set(role), + ttl_seconds: None, + }; + let Err(err) = create_api_key(req.clone(), web::Json(body)).await else { + panic!("API key with role {role} must be rejected"); + }; + assert!( + matches!(err, ApiKeyError::Rbac(RBACError::ProtectedRole)), + "role {role}: expected ProtectedRole, got {err:?}" + ); + assert_eq!(err.status_code(), StatusCode::BAD_REQUEST, "role {role}"); + } + } + } } diff --git a/src/handlers/http/modal/query/querier_role.rs b/src/handlers/http/modal/query/querier_role.rs index 321efa9b0..7196b67e1 100644 --- a/src/handlers/http/modal/query/querier_role.rs +++ b/src/handlers/http/modal/query/querier_role.rs @@ -202,3 +202,90 @@ pub async fn delete( Ok(HttpResponse::Ok().finish()) } + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + + use actix_web::{HttpMessage, ResponseError, http::StatusCode, test::TestRequest}; + use chrono::TimeDelta; + use parking_lot::RwLock; + use ulid::Ulid; + + use super::*; + use crate::{ + handlers::TENANT_ID, + rbac::{ + Users, + map::{ + ROLES, SESSIONS, SessionKey, Sessions, USER_GROUPS, USERS, UserGroups, + Users as UserMap, mut_users, + }, + role::model::DefaultPrivilege, + user::{Basic, User, UserType}, + }, + }; + + const ROLE: &str = "readers"; + + // `rbac::map::init` needs `PARSEABLE`, so tests create the maps directly. + fn init_auth_maps() { + USERS.get_or_init(|| RwLock::new(UserMap::default())); + SESSIONS.get_or_init(|| RwLock::new(Sessions::default())); + USER_GROUPS.get_or_init(|| RwLock::new(UserGroups::default())); + ROLES.get_or_init(|| RwLock::new(HashMap::new())); + } + + fn reader_role() -> Role { + Role::create_user_role(vec![DefaultPrivilege::Reader { resource: None }]) + } + + /// A fresh tenant holding one ordinary role, and a request from a user with + /// a live session in that tenant. + fn tenant_request() -> (String, HttpRequest) { + init_auth_maps(); + let tenant = format!("querier-role-{}", Ulid::new()); + mut_roles() + .entry(tenant.clone()) + .or_default() + .insert(ROLE.to_owned(), reader_role()); + let caller = User { + ty: UserType::Native(Basic { + username: format!("caller-{}", Ulid::new()), + password_hash: String::new(), + email: None, + }), + roles: HashSet::from([ROLE.to_owned()]), + user_groups: HashSet::new(), + tenant: Some(tenant.clone()), + protected: false, + }; + mut_users().insert(caller.clone()); + let session = SessionKey::SessionId(Ulid::new()); + Users.new_session(&caller, session.clone(), TimeDelta::minutes(5)); + let req = TestRequest::default() + .insert_header((TENANT_ID, tenant.as_str())) + .to_http_request(); + req.extensions_mut().insert(session); + (tenant, req) + } + + #[tokio::test] + async fn rejected_super_admin_update_leaves_live_role_unchanged() { + let (tenant, req) = tenant_request(); + let body = Role::create_user_role(vec![DefaultPrivilege::SuperAdmin]); + + let Err(err) = put(req, web::Path::from(ROLE.to_owned()), Json(body)).await else { + panic!("SuperAdmin role update must be rejected"); + }; + assert!(matches!(err, RoleError::SuperAdminPrivilege), "{err:?}"); + assert_eq!(err.status_code(), StatusCode::BAD_REQUEST); + + let live = roles().get(&tenant).and_then(|r| r.get(ROLE)).cloned(); + assert_eq!( + live, + Some(reader_role()), + "rejected update must not change the live role" + ); + } +} diff --git a/src/handlers/http/role.rs b/src/handlers/http/role.rs index 1c064dfe2..0339e6377 100644 --- a/src/handlers/http/role.rs +++ b/src/handlers/http/role.rs @@ -397,3 +397,46 @@ mod oidc_default_tests { assert!(validate_default_role("", &roles).is_err()); } } + +#[cfg(test)] +mod put_tests { + use actix_web::{ResponseError, test::TestRequest}; + use parking_lot::RwLock; + use ulid::Ulid; + + use super::*; + use crate::{handlers::TENANT_ID, rbac::map::ROLES, rbac::role::model::DefaultPrivilege}; + + const ROLE: &str = "readers"; + + fn reader_role() -> Role { + Role::create_user_role(vec![DefaultPrivilege::Reader { resource: None }]) + } + + #[tokio::test] + async fn rejected_super_admin_update_leaves_live_role_unchanged() { + ROLES.get_or_init(|| RwLock::new(HashMap::new())); + let tenant = format!("role-put-{}", Ulid::new()); + mut_roles() + .entry(tenant.clone()) + .or_default() + .insert(ROLE.to_owned(), reader_role()); + let req = TestRequest::default() + .insert_header((TENANT_ID, tenant.as_str())) + .to_http_request(); + let body = Role::create_user_role(vec![DefaultPrivilege::SuperAdmin]); + + let Err(err) = put(req, web::Path::from(ROLE.to_owned()), Json(body)).await else { + panic!("SuperAdmin role update must be rejected"); + }; + assert!(matches!(err, RoleError::SuperAdminPrivilege), "{err:?}"); + assert_eq!(err.status_code(), StatusCode::BAD_REQUEST); + + let live = roles().get(&tenant).and_then(|r| r.get(ROLE)).cloned(); + assert_eq!( + live, + Some(reader_role()), + "rejected update must not change the live role" + ); + } +} From d5ffa911ab75e41f4207833454d98029699e76a0 Mon Sep 17 00:00:00 2001 From: VibeSWE <338779468+VibeSWE@users.noreply.github.com> Date: Fri, 23 Oct 2026 23:10:52 -0500 Subject: [PATCH 02/13] Add regressions for protected roles on querier endpoints Cover Internal and SuperAdmin roles on querier user creation and role addition, plus Internal role PUT rejection without live-map mutation. The assignment tests currently reach PARSEABLE and terminate on CLI parsing instead of rejecting before storage. The Internal PUT test currently returns SuperAdminPrivilege rather than ProtectedRole. Co-Authored-By: GPT-6.1 Sol --- src/handlers/http/modal/query/querier_rbac.rs | 111 ++++++++++++++++++ src/handlers/http/modal/query/querier_role.rs | 22 ++++ 2 files changed, 133 insertions(+) diff --git a/src/handlers/http/modal/query/querier_rbac.rs b/src/handlers/http/modal/query/querier_rbac.rs index 3bd829827..4a2f33de5 100644 --- a/src/handlers/http/modal/query/querier_rbac.rs +++ b/src/handlers/http/modal/query/querier_rbac.rs @@ -432,3 +432,114 @@ pub async fn post_gen_password( Ok(new_password) } + +#[cfg(test)] +mod role_escalation { + use std::collections::HashMap; + + use actix_web::{HttpMessage, ResponseError, http::StatusCode, test::TestRequest}; + use chrono::TimeDelta; + use parking_lot::RwLock; + use ulid::Ulid; + + use super::*; + use crate::{ + handlers::TENANT_ID, + rbac::{ + map::{ + ROLES, SESSIONS, SessionKey, Sessions, USER_GROUPS, USERS, UserGroups, + Users as UserMap, mut_roles, mut_users, + }, + role::model::{DefaultPrivilege, Role}, + user::{Basic, User}, + }, + }; + + fn tenant_request(role: Role) -> (String, String, HttpRequest) { + USERS.get_or_init(|| RwLock::new(UserMap::default())); + SESSIONS.get_or_init(|| RwLock::new(Sessions::default())); + USER_GROUPS.get_or_init(|| RwLock::new(UserGroups::default())); + ROLES.get_or_init(|| RwLock::new(HashMap::new())); + let tenant = format!("querier-user-{}", Ulid::new()); + mut_roles().insert( + tenant.clone(), + HashMap::from([ + ("elevated".to_owned(), role), + ( + "admin".to_owned(), + Role::create_user_role(vec![DefaultPrivilege::Admin]), + ), + ]), + ); + let caller = User { + ty: UserType::Native(Basic { + username: format!("admin-{}", Ulid::new()), + password_hash: String::new(), + email: None, + }), + roles: HashSet::from(["admin".to_owned()]), + user_groups: HashSet::new(), + tenant: Some(tenant.clone()), + protected: false, + }; + mut_users().insert(caller.clone()); + let session = SessionKey::SessionId(Ulid::new()); + Users.new_session(&caller, session.clone(), TimeDelta::minutes(5)); + let req = TestRequest::default() + .insert_header((TENANT_ID, tenant.as_str())) + .to_http_request(); + req.extensions_mut().insert(session); + (tenant, caller.userid().to_owned(), req) + } + + async fn rejects_creation(role: Role) { + let (tenant, _, req) = tenant_request(role); + let body = web::Json(serde_json::json!(["elevated"])); + let Err(err) = post_user(req, web::Path::from("new-user".to_owned()), Some(body)).await + else { + panic!("protected role assignment must be rejected before storage"); + }; + assert!(matches!(err, RBACError::ProtectedRole), "{err:?}"); + assert_eq!(err.status_code(), StatusCode::BAD_REQUEST); + assert!(!Users.contains("new-user", &Some(tenant))); + } + + async fn rejects_addition(role: Role) { + let (tenant, userid, req) = tenant_request(role); + let Err(err) = add_roles_to_user( + req, + web::Path::from(userid.clone()), + web::Json(HashSet::from(["elevated".to_owned()])), + ) + .await + else { + panic!("protected role assignment must be rejected before storage"); + }; + assert!(matches!(err, RBACError::ProtectedRole), "{err:?}"); + assert_eq!(err.status_code(), StatusCode::BAD_REQUEST); + assert_eq!( + Users.get_user(&userid, &Some(tenant)).unwrap().roles, + HashSet::from(["admin".to_owned()]) + ); + } + + #[tokio::test] + async fn post_user_rejects_internal_role() { + rejects_creation(Role::create_internal_role(vec![DefaultPrivilege::Admin])).await; + } + + #[tokio::test] + async fn post_user_rejects_super_admin_role() { + rejects_creation(Role::create_user_role(vec![DefaultPrivilege::SuperAdmin])).await; + } + + #[tokio::test] + async fn add_roles_rejects_internal_role() { + rejects_addition(Role::create_internal_role(vec![DefaultPrivilege::Admin])).await; + } + + #[tokio::test] + async fn add_roles_rejects_super_admin_role() { + rejects_addition(Role::create_user_role(vec![DefaultPrivilege::SuperAdmin])).await; + } +} diff --git a/src/handlers/http/modal/query/querier_role.rs b/src/handlers/http/modal/query/querier_role.rs index 7196b67e1..f579d50c8 100644 --- a/src/handlers/http/modal/query/querier_role.rs +++ b/src/handlers/http/modal/query/querier_role.rs @@ -288,4 +288,26 @@ mod tests { "rejected update must not change the live role" ); } + + #[tokio::test] + async fn rejected_internal_update_leaves_live_role_unchanged() { + for privilege in [ + DefaultPrivilege::SuperAdmin, + DefaultPrivilege::Reader { resource: None }, + ] { + let (tenant, req) = tenant_request(); + let body = Role::create_internal_role(vec![privilege]); + + let Err(err) = put(req, web::Path::from(ROLE.to_owned()), Json(body)).await else { + panic!("internal role update must be rejected"); + }; + assert!(matches!(err, RoleError::ProtectedRole), "{err:?}"); + assert_eq!(err.status_code(), StatusCode::BAD_REQUEST); + assert_eq!( + roles().get(&tenant).and_then(|r| r.get(ROLE)).cloned(), + Some(reader_role()), + "rejected update must not change the live role" + ); + } + } } From d041fa4d835717303cd9fcd16c619b7c865b2386 Mon Sep 17 00:00:00 2001 From: VibeSWE <338779468+VibeSWE@users.noreply.github.com> Date: Fri, 23 Oct 2026 23:13:26 -0500 Subject: [PATCH 03/13] Isolate Internal role rejection from SuperAdmin in API key test The previous Internal-role fixture also held SuperAdmin, so rejecting only SuperAdmin privileges could incorrectly satisfy both regressions. Use an Internal role with Admin privileges for this test while preserving the realistic super-admin fixture for the handler test. Confirmed RED: validation still returns Ok(()). Co-Authored-By: GPT-6.1 Sol --- src/handlers/http/apikeys.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/handlers/http/apikeys.rs b/src/handlers/http/apikeys.rs index 05977a5f8..301337b99 100644 --- a/src/handlers/http/apikeys.rs +++ b/src/handlers/http/apikeys.rs @@ -409,6 +409,7 @@ mod tests { const ADMIN_ROLE: &str = "tenant-admin"; const READER_ROLE: &str = "reader"; + const INTERNAL_ROLE: &str = "internal"; // Mirrors the internal role `rbac::map::init` adds to the default tenant. const SUPER_ADMIN_ROLE: &str = "super-admin"; // A user-typed role holding SuperAdmin, e.g. from legacy metadata. @@ -437,6 +438,10 @@ mod tests { READER_ROLE.to_owned(), Role::create_user_role(vec![DefaultPrivilege::Reader { resource: None }]), ), + ( + INTERNAL_ROLE.to_owned(), + Role::create_internal_role(vec![DefaultPrivilege::Admin]), + ), ( SUPER_ADMIN_ROLE.to_owned(), Role::create_internal_role(vec![DefaultPrivilege::SuperAdmin]), @@ -484,7 +489,7 @@ mod tests { #[test] fn validate_roles_rejects_internal_role() { let tenant = Some(tenant()); - let result = validate_roles(&role_set(SUPER_ADMIN_ROLE), &tenant); + let result = validate_roles(&role_set(INTERNAL_ROLE), &tenant); assert!( matches!(result, Err(ApiKeyError::Rbac(RBACError::ProtectedRole))), "internal role must be rejected like the native-user path, got {result:?}" From 71dffd5ffd5dc11f875cd54e9aa1b1920662e1c8 Mon Sep 17 00:00:00 2001 From: VibeSWE <338779468+VibeSWE@users.noreply.github.com> Date: Fri, 23 Oct 2026 23:14:25 -0500 Subject: [PATCH 04/13] Reject protected role assignments for API keys and querier users Reject Internal and SuperAdmin roles with ProtectedRole after checking role existence. API key creation validates before duplicate-key detection and metadata access. Apply the same checks to querier user creation and role addition; validate creation under UPDATE_LOCK before reading metadata. Co-Authored-By: GPT-6.1 Sol --- src/handlers/http/apikeys.rs | 23 ++++++++++++------ src/handlers/http/modal/query/querier_rbac.rs | 24 +++++++++++++++++-- 2 files changed, 38 insertions(+), 9 deletions(-) diff --git a/src/handlers/http/apikeys.rs b/src/handlers/http/apikeys.rs index 301337b99..9ae6474d4 100644 --- a/src/handlers/http/apikeys.rs +++ b/src/handlers/http/apikeys.rs @@ -15,6 +15,7 @@ use crate::{ rbac::{ Users, map::{roles, users}, + role::model::RoleType, user::{User, UserType}, }, utils::{get_user_and_tenant_from_request, is_admin}, @@ -140,7 +141,7 @@ fn collect_tenant_api_keys(tenant_id: &Option) -> Vec { .collect() } -/// Validate that every role in the request exists in the tenant's role map. +/// Validate that every requested role exists and can be assigned in this tenant. /// Mirrors the check performed by the native-user CRUD endpoint. fn validate_roles( role_names: &HashSet, @@ -158,11 +159,19 @@ fn validate_roles( }) .cloned() .collect(); - if missing.is_empty() { - Ok(()) - } else { - Err(ApiKeyError::Rbac(RBACError::RolesDoNotExist(missing))) + if !missing.is_empty() { + return Err(ApiKeyError::Rbac(RBACError::RolesDoNotExist(missing))); } + if let Some(tenant_roles) = tenant_roles.get(tenant) + && role_names.iter().any(|name| { + tenant_roles.get(name).is_some_and(|role| { + role.role_type() == &RoleType::Internal || role.deny_super_admin() + }) + }) + { + return Err(ApiKeyError::Rbac(RBACError::ProtectedRole)); + } + Ok(()) } /// POST /api/prism/v1/apikeys @@ -176,8 +185,8 @@ pub async fn create_api_key( let guard = UPDATE_LOCK.lock().await; - // Reject any role names that don't exist in this tenant while holding the - // update lock so a concurrent role update cannot invalidate the check. + // Validate role assignments under the update lock so a concurrent role + // update cannot invalidate the check. validate_roles(&body.roles, &tenant_id)?; // Duplicate key-name detection inside the same tenant. diff --git a/src/handlers/http/modal/query/querier_rbac.rs b/src/handlers/http/modal/query/querier_rbac.rs index 4a2f33de5..3cc967343 100644 --- a/src/handlers/http/modal/query/querier_rbac.rs +++ b/src/handlers/http/modal/query/querier_rbac.rs @@ -33,6 +33,7 @@ use crate::{ rbac::{ Users, map::{read_user_groups, roles, users, write_user_groups}, + role::model::RoleType, user::{self, UserType}, }, tenants::TENANT_METADATA, @@ -51,7 +52,7 @@ pub async fn post_user( let tenant_id = get_tenant_id_from_request(&req); let caller_userid = get_user_from_request(&req)?; validator::user_role_name(&username)?; - let mut metadata = get_metadata(&tenant_id).await?; + let _guard = UPDATE_LOCK.lock().await; let user_roles: HashSet = if let Some(body) = body { serde_json::from_value(body.into_inner())? @@ -73,7 +74,16 @@ pub async fn post_user( if !non_existent_roles.is_empty() { return Err(RBACError::RolesDoNotExist(non_existent_roles)); } - let _guard = UPDATE_LOCK.lock().await; + if let Some(tenant_roles) = roles().get(tenant_id.as_deref().unwrap_or(DEFAULT_TENANT)) + && user_roles.iter().any(|name| { + tenant_roles.get(name).is_some_and(|role| { + role.role_type() == &RoleType::Internal || role.deny_super_admin() + }) + }) + { + return Err(RBACError::ProtectedRole); + } + let mut metadata = get_metadata(&tenant_id).await?; if Users.contains(&username, &tenant_id) || metadata .users @@ -247,6 +257,16 @@ pub async fn add_roles_to_user( return Err(RBACError::RolesDoNotExist(non_existent_roles)); } + if let Some(tenant_roles) = roles().get(tenant_id.as_deref().unwrap_or(DEFAULT_TENANT)) + && roles_to_add.iter().any(|name| { + tenant_roles.get(name).is_some_and(|role| { + role.role_type() == &RoleType::Internal || role.deny_super_admin() + }) + }) + { + return Err(RBACError::ProtectedRole); + } + // update parseable.json first let mut metadata = get_metadata(&tenant_id).await?; if let Some(user) = metadata From e7860973c422d0f193e6e0bfd606bcca10af3d8d Mon Sep 17 00:00:00 2001 From: VibeSWE <338779468+VibeSWE@users.noreply.github.com> Date: Fri, 23 Oct 2026 23:15:16 -0500 Subject: [PATCH 05/13] Validate querier role updates before mutating live state Reject Internal bodies and SuperAdmin privileges before metadata access. Inspect existing roles through a read lock and insert into the live map only after metadata persistence succeeds. Serialize the local update with UPDATE_LOCK as standalone PUT does, releasing it before cluster synchronization. Co-Authored-By: GPT-6.1 Sol --- src/handlers/http/modal/query/querier_role.rs | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/handlers/http/modal/query/querier_role.rs b/src/handlers/http/modal/query/querier_role.rs index f579d50c8..6f3456a44 100644 --- a/src/handlers/http/modal/query/querier_role.rs +++ b/src/handlers/http/modal/query/querier_role.rs @@ -27,6 +27,7 @@ use crate::{ handlers::http::{ cluster::{sync_role_delete, sync_role_update}, modal::utils::rbac_utils::{get_metadata, put_metadata}, + rbac::UPDATE_LOCK, role::RoleError, }, parseable::DEFAULT_TENANT, @@ -44,8 +45,16 @@ use crate::{ pub async fn put( req: HttpRequest, name: web::Path, - Json(body): Json, + Json(role): Json, ) -> Result { + let guard = UPDATE_LOCK.lock().await; + // internal role manipulation not allowed + if role.role_type().eq(&RoleType::Internal) { + return Err(RoleError::ProtectedRole); + } + if role.deny_super_admin() { + return Err(RoleError::SuperAdminPrivilege); + } let name = name.into_inner(); let tenant_id = get_tenant_id_from_request(&req); let tenant = tenant_id.as_deref().unwrap_or(DEFAULT_TENANT); @@ -56,22 +65,11 @@ pub async fn put( validator::user_role_name(&name).map_err(RoleError::ValidationError)?; // if role exists, then it should not be an internal role - let role = if let Some(tenant_roles) = mut_roles().get_mut(tenant) - && let Some(role) = tenant_roles.get_mut(&name) + if let Some(tenant_roles) = roles().get(tenant) + && let Some(existing_role) = tenant_roles.get(&name) + && existing_role.role_type().eq(&RoleType::Internal) { - if role.role_type().eq(&RoleType::Internal) { - return Err(RoleError::ProtectedRole); - } else { - // role exists and is not internal, can proceed with modification - *role = body; - role.clone() - } - } else { - body - }; - - if role.deny_super_admin() { - return Err(RoleError::SuperAdminPrivilege); + return Err(RoleError::ProtectedRole); } let mut metadata = get_metadata(&tenant_id).await?; @@ -116,6 +114,8 @@ pub async fn put( } } + drop(guard); + if let Err(e) = sync_role_update(&req, name.clone(), role, &tenant_id, &caller_userid).await { tracing::error!("Failed to sync role update to cluster nodes: {e}"); } From 8fd7930ae39f7703dd81808f0fd907ce2e5add1e Mon Sep 17 00:00:00 2001 From: VibeSWE <338779468+VibeSWE@users.noreply.github.com> Date: Fri, 23 Oct 2026 23:18:59 -0500 Subject: [PATCH 06/13] Make API key auth test roles independent of map initialization order The RBAC regression fixtures initialize ROLES without the middleware test role. The middleware helper previously installed its role only inside get_or_init, yielding four repeatable 403 failures in the full suite. Insert that fixture role independently of global initialization. Confirmed RED twice (870 passed, 4 failed, 2 ignored) and GREEN for all 19 RBAC/authentication tests together. Co-Authored-By: GPT-6.1 Sol --- src/handlers/http/middleware.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/handlers/http/middleware.rs b/src/handlers/http/middleware.rs index 4507a9b8d..44944962b 100644 --- a/src/handlers/http/middleware.rs +++ b/src/handlers/http/middleware.rs @@ -1123,7 +1123,8 @@ mod api_key_auth_tests { use super::*; use crate::rbac::{ map::{ - ROLES, SESSIONS, Sessions, USER_GROUPS, USERS, UserGroups, Users as UserMap, mut_users, + ROLES, SESSIONS, Sessions, USER_GROUPS, USERS, UserGroups, Users as UserMap, mut_roles, + mut_users, }, role::model::{DefaultPrivilege, Role}, }; @@ -1154,11 +1155,14 @@ mod api_key_auth_tests { USERS.get_or_init(|| RwLock::new(UserMap::default())); SESSIONS.get_or_init(|| RwLock::new(Sessions::default())); USER_GROUPS.get_or_init(|| RwLock::new(UserGroups::default())); - ROLES.get_or_init(|| { - let role = Role::create_internal_role(vec![DefaultPrivilege::Admin]); - let roles = HashMap::from([(ROLE.to_owned(), role)]); - RwLock::new(HashMap::from([(DEFAULT_TENANT.to_owned(), roles)])) - }); + ROLES.get_or_init(|| RwLock::new(HashMap::new())); + mut_roles() + .entry(DEFAULT_TENANT.to_owned()) + .or_default() + .insert( + ROLE.to_owned(), + Role::create_internal_role(vec![DefaultPrivilege::Admin]), + ); CLUSTER_SECRET.get_or_init(|| { let salt = SaltString::encode_b64(b"api-key-expiry-salt").unwrap(); let hash = Argon2::default() From 36af50e57a97160bffd596a848c64cfcc46ccda7 Mon Sep 17 00:00:00 2001 From: VibeSWE <338779468+VibeSWE@users.noreply.github.com> Date: Sat, 24 Oct 2026 19:24:33 -0500 Subject: [PATCH 07/13] Serialize querier and role handler tests that write the shared RBAC maps These tests insert into USERS and ROLES, which rbac::lock_order_tests watch for queued writers. Hold map::TEST_SERIAL like the other modules' tests do. Co-Authored-By: Claude Opus 5.5 --- src/handlers/http/modal/query/querier_rbac.rs | 10 +++++++++- src/handlers/http/modal/query/querier_role.rs | 6 +++++- src/handlers/http/role.rs | 8 +++++++- 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/handlers/http/modal/query/querier_rbac.rs b/src/handlers/http/modal/query/querier_rbac.rs index 3cc967343..4ae40b6ac 100644 --- a/src/handlers/http/modal/query/querier_rbac.rs +++ b/src/handlers/http/modal/query/querier_rbac.rs @@ -467,7 +467,7 @@ mod role_escalation { handlers::TENANT_ID, rbac::{ map::{ - ROLES, SESSIONS, SessionKey, Sessions, USER_GROUPS, USERS, UserGroups, + ROLES, SESSIONS, SessionKey, Sessions, TEST_SERIAL, USER_GROUPS, USERS, UserGroups, Users as UserMap, mut_roles, mut_users, }, role::model::{DefaultPrivilege, Role}, @@ -545,21 +545,29 @@ mod role_escalation { #[tokio::test] async fn post_user_rejects_internal_role() { + // Other modules' tests watch the RBAC map locks; see `map::TEST_SERIAL`. + let _serial = TEST_SERIAL.lock().await; rejects_creation(Role::create_internal_role(vec![DefaultPrivilege::Admin])).await; } #[tokio::test] async fn post_user_rejects_super_admin_role() { + // Other modules' tests watch the RBAC map locks; see `map::TEST_SERIAL`. + let _serial = TEST_SERIAL.lock().await; rejects_creation(Role::create_user_role(vec![DefaultPrivilege::SuperAdmin])).await; } #[tokio::test] async fn add_roles_rejects_internal_role() { + // Other modules' tests watch the RBAC map locks; see `map::TEST_SERIAL`. + let _serial = TEST_SERIAL.lock().await; rejects_addition(Role::create_internal_role(vec![DefaultPrivilege::Admin])).await; } #[tokio::test] async fn add_roles_rejects_super_admin_role() { + // Other modules' tests watch the RBAC map locks; see `map::TEST_SERIAL`. + let _serial = TEST_SERIAL.lock().await; rejects_addition(Role::create_user_role(vec![DefaultPrivilege::SuperAdmin])).await; } } diff --git a/src/handlers/http/modal/query/querier_role.rs b/src/handlers/http/modal/query/querier_role.rs index 6f3456a44..3fab74c67 100644 --- a/src/handlers/http/modal/query/querier_role.rs +++ b/src/handlers/http/modal/query/querier_role.rs @@ -218,7 +218,7 @@ mod tests { rbac::{ Users, map::{ - ROLES, SESSIONS, SessionKey, Sessions, USER_GROUPS, USERS, UserGroups, + ROLES, SESSIONS, SessionKey, Sessions, TEST_SERIAL, USER_GROUPS, USERS, UserGroups, Users as UserMap, mut_users, }, role::model::DefaultPrivilege, @@ -272,6 +272,8 @@ mod tests { #[tokio::test] async fn rejected_super_admin_update_leaves_live_role_unchanged() { + // Other modules' tests watch the RBAC map locks; see `map::TEST_SERIAL`. + let _serial = TEST_SERIAL.lock().await; let (tenant, req) = tenant_request(); let body = Role::create_user_role(vec![DefaultPrivilege::SuperAdmin]); @@ -291,6 +293,8 @@ mod tests { #[tokio::test] async fn rejected_internal_update_leaves_live_role_unchanged() { + // Other modules' tests watch the RBAC map locks; see `map::TEST_SERIAL`. + let _serial = TEST_SERIAL.lock().await; for privilege in [ DefaultPrivilege::SuperAdmin, DefaultPrivilege::Reader { resource: None }, diff --git a/src/handlers/http/role.rs b/src/handlers/http/role.rs index 0339e6377..3ce94d2d3 100644 --- a/src/handlers/http/role.rs +++ b/src/handlers/http/role.rs @@ -405,7 +405,11 @@ mod put_tests { use ulid::Ulid; use super::*; - use crate::{handlers::TENANT_ID, rbac::map::ROLES, rbac::role::model::DefaultPrivilege}; + use crate::{ + handlers::TENANT_ID, + rbac::map::{ROLES, TEST_SERIAL}, + rbac::role::model::DefaultPrivilege, + }; const ROLE: &str = "readers"; @@ -415,6 +419,8 @@ mod put_tests { #[tokio::test] async fn rejected_super_admin_update_leaves_live_role_unchanged() { + // Other modules' tests watch the RBAC map locks; see `map::TEST_SERIAL`. + let _serial = TEST_SERIAL.lock().await; ROLES.get_or_init(|| RwLock::new(HashMap::new())); let tenant = format!("role-put-{}", Ulid::new()); mut_roles() From f3459732038abc77dfc704ced83dba3f4121366d Mon Sep 17 00:00:00 2001 From: VibeSWE <338779468+VibeSWE@users.noreply.github.com> Date: Sat, 24 Oct 2026 19:52:25 -0500 Subject: [PATCH 08/13] test: reject updates to existing internal querier roles Cover an ordinary user-typed update targeting an internal role, including the protected-role error, HTTP 400, and unchanged live role. Co-Authored-By: GPT-6.1 Sol --- src/handlers/http/modal/query/querier_role.rs | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/handlers/http/modal/query/querier_role.rs b/src/handlers/http/modal/query/querier_role.rs index 3fab74c67..f6560ad92 100644 --- a/src/handlers/http/modal/query/querier_role.rs +++ b/src/handlers/http/modal/query/querier_role.rs @@ -291,6 +291,30 @@ mod tests { ); } + #[tokio::test] + async fn rejected_existing_internal_role_update_leaves_live_role_unchanged() { + // Other modules' tests watch the RBAC map locks; see `map::TEST_SERIAL`. + 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 Err(err) = put(req, web::Path::from(ROLE.to_owned()), Json(reader_role())).await + else { + panic!("updating an existing internal role must be rejected"); + }; + assert!(matches!(err, RoleError::ProtectedRole), "{err:?}"); + assert_eq!(err.status_code(), StatusCode::BAD_REQUEST); + assert_eq!( + roles().get(&tenant).and_then(|r| r.get(ROLE)).cloned(), + Some(internal_role), + "rejected update must not change the live internal role" + ); + } + #[tokio::test] async fn rejected_internal_update_leaves_live_role_unchanged() { // Other modules' tests watch the RBAC map locks; see `map::TEST_SERIAL`. From 7c7af59b5bac29ecee92fd5593bba4218ecb0f11 Mon Sep 17 00:00:00 2001 From: VibeSWE <338779468+VibeSWE@users.noreply.github.com> Date: Sat, 24 Oct 2026 19:52:32 -0500 Subject: [PATCH 09/13] test: document querier role guard regression failures Explain that removing a protected-role check reaches storage and initializes PARSEABLE, causing clap to exit the test binary with code 2. Co-Authored-By: GPT-6.1 Sol --- src/handlers/http/modal/query/querier_role.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/handlers/http/modal/query/querier_role.rs b/src/handlers/http/modal/query/querier_role.rs index f6560ad92..e01cf29cd 100644 --- a/src/handlers/http/modal/query/querier_role.rs +++ b/src/handlers/http/modal/query/querier_role.rs @@ -205,6 +205,9 @@ pub async fn delete( #[cfg(test)] mod tests { + // Removing a protected-role check lets the handler reach storage and initialize + // PARSEABLE, which parses the test binary's args. Clap then exits the process + // with code 2 (`unrecognized subcommand`); this symptom means a check was removed. use std::collections::HashMap; use actix_web::{HttpMessage, ResponseError, http::StatusCode, test::TestRequest}; @@ -302,8 +305,7 @@ mod tests { .or_default() .insert(ROLE.to_owned(), internal_role.clone()); - let Err(err) = put(req, web::Path::from(ROLE.to_owned()), Json(reader_role())).await - else { + let Err(err) = put(req, web::Path::from(ROLE.to_owned()), Json(reader_role())).await else { panic!("updating an existing internal role must be rejected"); }; assert!(matches!(err, RoleError::ProtectedRole), "{err:?}"); From bf1056a9cc64b06e9c0735505836534736d4491e Mon Sep 17 00:00:00 2001 From: VibeSWE <338779468+VibeSWE@users.noreply.github.com> Date: Sat, 24 Oct 2026 19:53:04 -0500 Subject: [PATCH 10/13] Add failing tests for protected user and role checks on querier and API keys Querier post_user and add_roles_to_user, and apikeys::validate_roles, do not yet reject roles held by protected users or protected target users, and querier add_roles_to_user reports missing roles before protected ones. Co-Authored-By: Claude Opus 5.5 --- src/handlers/http/apikeys.rs | 22 +++ src/handlers/http/modal/query/querier_rbac.rs | 137 ++++++++++++++++++ 2 files changed, 159 insertions(+) diff --git a/src/handlers/http/apikeys.rs b/src/handlers/http/apikeys.rs index 5a34399a1..774abe8ef 100644 --- a/src/handlers/http/apikeys.rs +++ b/src/handlers/http/apikeys.rs @@ -970,6 +970,28 @@ mod tests { ); } + #[test] + fn validate_roles_rejects_role_held_by_protected_user() { + let _serial = blocking_rbac_state(); + let tenant = tenant(); + mut_users().insert(User { + ty: UserType::Native(Basic { + username: format!("protected-{}", Ulid::new()), + password_hash: String::new(), + email: None, + }), + roles: role_set(READER_ROLE), + user_groups: HashSet::new(), + tenant: Some(tenant.clone()), + protected: true, + }); + let result = validate_roles(&role_set(READER_ROLE), &Some(tenant)); + assert!( + matches!(result, Err(ApiKeyError::Rbac(RBACError::ProtectedRole))), + "a protected user's role must be rejected like the native-user path, got {result:?}" + ); + } + #[tokio::test] async fn tenant_admin_cannot_create_super_admin_api_key() { let _serial = rbac_state().await; diff --git a/src/handlers/http/modal/query/querier_rbac.rs b/src/handlers/http/modal/query/querier_rbac.rs index 4ae40b6ac..8e5f0e958 100644 --- a/src/handlers/http/modal/query/querier_rbac.rs +++ b/src/handlers/http/modal/query/querier_rbac.rs @@ -543,6 +543,143 @@ mod role_escalation { ); } + /// Adds a protected user holding `role` to `tenant`. + fn insert_protected_user(tenant: &str, role: &str) -> String { + let protected = User { + ty: UserType::Native(Basic { + username: format!("protected-{}", Ulid::new()), + password_hash: String::new(), + email: None, + }), + roles: HashSet::from([role.to_owned()]), + user_groups: HashSet::new(), + tenant: Some(tenant.to_owned()), + protected: true, + }; + mut_users().insert(protected.clone()); + protected.userid().to_owned() + } + + fn ordinary_role() -> Role { + Role::create_user_role(vec![DefaultPrivilege::Reader { resource: None }]) + } + + #[tokio::test] + async fn post_user_rejects_role_held_by_protected_user() { + // Other modules' tests watch the RBAC map locks; see `map::TEST_SERIAL`. + let _serial = TEST_SERIAL.lock().await; + let (tenant, _, req) = tenant_request(ordinary_role()); + insert_protected_user(&tenant, "elevated"); + let body = web::Json(serde_json::json!(["elevated"])); + let Err(err) = post_user(req, web::Path::from("new-user".to_owned()), Some(body)).await + else { + panic!("a protected user's role must be rejected before storage"); + }; + assert!(matches!(err, RBACError::ProtectedRole), "{err:?}"); + assert!(!Users.contains("new-user", &Some(tenant))); + } + + #[tokio::test] + async fn add_roles_rejects_role_held_by_protected_user() { + // Other modules' tests watch the RBAC map locks; see `map::TEST_SERIAL`. + let _serial = TEST_SERIAL.lock().await; + let (tenant, userid, req) = tenant_request(ordinary_role()); + insert_protected_user(&tenant, "elevated"); + let Err(err) = add_roles_to_user( + req, + web::Path::from(userid.clone()), + web::Json(HashSet::from(["elevated".to_owned()])), + ) + .await + else { + panic!("a protected user's role must be rejected before storage"); + }; + assert!(matches!(err, RBACError::ProtectedRole), "{err:?}"); + assert_eq!( + Users.get_user(&userid, &Some(tenant)).unwrap().roles, + HashSet::from(["admin".to_owned()]) + ); + } + + #[tokio::test] + async fn add_roles_rejects_protected_target_user() { + // Other modules' tests watch the RBAC map locks; see `map::TEST_SERIAL`. + let _serial = TEST_SERIAL.lock().await; + let (tenant, _, req) = tenant_request(ordinary_role()); + let target = insert_protected_user(&tenant, "admin"); + let Err(err) = add_roles_to_user( + req, + web::Path::from(target.clone()), + web::Json(HashSet::from(["elevated".to_owned()])), + ) + .await + else { + panic!("a protected user must be rejected before storage"); + }; + assert!(matches!(err, RBACError::ProtectedUser), "{err:?}"); + assert_eq!( + Users.get_user(&target, &Some(tenant)).unwrap().roles, + HashSet::from(["admin".to_owned()]) + ); + } + + // Like the standalone handler, protected checks run before the existence + // check, so a request mixing a protected and a missing role is a 400 for + // the protected one rather than a list of missing roles. + #[tokio::test] + async fn add_roles_checks_protected_target_before_missing_roles() { + // Other modules' tests watch the RBAC map locks; see `map::TEST_SERIAL`. + let _serial = TEST_SERIAL.lock().await; + let (tenant, _, req) = tenant_request(ordinary_role()); + let target = insert_protected_user(&tenant, "admin"); + let Err(err) = add_roles_to_user( + req, + web::Path::from(target), + web::Json(HashSet::from(["missing".to_owned()])), + ) + .await + else { + panic!("a protected user must be rejected"); + }; + assert!(matches!(err, RBACError::ProtectedUser), "{err:?}"); + } + + #[tokio::test] + async fn add_roles_checks_protected_role_before_missing_roles() { + // Other modules' tests watch the RBAC map locks; see `map::TEST_SERIAL`. + let _serial = TEST_SERIAL.lock().await; + let (_, userid, req) = + tenant_request(Role::create_internal_role(vec![DefaultPrivilege::Admin])); + let Err(err) = add_roles_to_user( + req, + web::Path::from(userid), + web::Json(HashSet::from(["elevated".to_owned(), "missing".to_owned()])), + ) + .await + else { + panic!("an internal role must be rejected"); + }; + assert!(matches!(err, RBACError::ProtectedRole), "{err:?}"); + } + + #[tokio::test] + async fn add_roles_checks_protected_user_role_before_missing_roles() { + // Other modules' tests watch the RBAC map locks; see `map::TEST_SERIAL`. + let _serial = TEST_SERIAL.lock().await; + let (tenant, userid, req) = tenant_request(ordinary_role()); + insert_protected_user(&tenant, "elevated"); + let Err(err) = add_roles_to_user( + req, + web::Path::from(userid), + web::Json(HashSet::from(["elevated".to_owned(), "missing".to_owned()])), + ) + .await + else { + panic!("a protected user's role must be rejected"); + }; + assert!(matches!(err, RBACError::ProtectedRole), "{err:?}"); + } + #[tokio::test] async fn post_user_rejects_internal_role() { // Other modules' tests watch the RBAC map locks; see `map::TEST_SERIAL`. From ad4b09d26e45279d12871484d39729e65bc24680 Mon Sep 17 00:00:00 2001 From: VibeSWE <338779468+VibeSWE@users.noreply.github.com> Date: Sat, 24 Oct 2026 22:01:33 -0500 Subject: [PATCH 11/13] Port standalone protected-user checks to querier user endpoints and API keys Querier post_user and add_roles_to_user, and API key role validation, now reject roles held by protected users, and add_roles_to_user rejects a protected target user. add_roles_to_user runs the protected checks before the unknown-role check, matching standalone. Co-Authored-By: Claude Opus 5.5 --- src/handlers/http/apikeys.rs | 30 ++++++++------ src/handlers/http/modal/query/querier_rbac.rs | 40 ++++++++++++++----- src/rbac/mod.rs | 15 +++++++ 3 files changed, 62 insertions(+), 23 deletions(-) diff --git a/src/handlers/http/apikeys.rs b/src/handlers/http/apikeys.rs index 774abe8ef..ed4b908d9 100644 --- a/src/handlers/http/apikeys.rs +++ b/src/handlers/http/apikeys.rs @@ -247,21 +247,27 @@ fn validate_roles( tenant_id: &Option, ) -> Result<(), ApiKeyError> { let tenant = tenant_or_default(tenant_id); - let tenant_roles = roles(); - let missing: Vec = role_names - .iter() - .filter(|r| { - tenant_roles - .get(tenant) - .map(|m| !m.contains_key(*r)) - .unwrap_or(true) - }) - .cloned() - .collect(); + let missing: Vec = { + let tenant_roles = roles(); + role_names + .iter() + .filter(|r| { + tenant_roles + .get(tenant) + .map(|m| !m.contains_key(*r)) + .unwrap_or(true) + }) + .cloned() + .collect() + }; if !missing.is_empty() { return Err(ApiKeyError::Rbac(RBACError::RolesDoNotExist(missing))); } - if let Some(tenant_roles) = tenant_roles.get(tenant) + // Takes USERS, so no ROLES guard may be held here. + if Users.any_role_held_by_protected_user(role_names, tenant_id) { + return Err(ApiKeyError::Rbac(RBACError::ProtectedRole)); + } + if let Some(tenant_roles) = roles().get(tenant) && role_names.iter().any(|name| { tenant_roles.get(name).is_some_and(|role| { role.role_type() == &RoleType::Internal || role.deny_super_admin() diff --git a/src/handlers/http/modal/query/querier_rbac.rs b/src/handlers/http/modal/query/querier_rbac.rs index 8e5f0e958..922b0f7b4 100644 --- a/src/handlers/http/modal/query/querier_rbac.rs +++ b/src/handlers/http/modal/query/querier_rbac.rs @@ -74,6 +74,11 @@ pub async fn post_user( if !non_existent_roles.is_empty() { return Err(RBACError::RolesDoNotExist(non_existent_roles)); } + // Standalone reads protected users from metadata; the in-memory map is + // loaded from it and kept in step, and is checked before any storage call. + if Users.any_role_held_by_protected_user(&user_roles, &tenant_id) { + return Err(RBACError::ProtectedRole); + } if let Some(tenant_roles) = roles().get(tenant_id.as_deref().unwrap_or(DEFAULT_TENANT)) && user_roles.iter().any(|name| { tenant_roles.get(name).is_some_and(|role| { @@ -232,6 +237,15 @@ pub async fn add_roles_to_user( return Err(RBACError::UserDoesNotExist); }; + // Protected checks come before the existence check, as in the standalone handler. + if Users.is_protected(&userid, &tenant_id) == Some(true) { + return Err(RBACError::ProtectedUser); + } + + if Users.any_role_held_by_protected_user(&roles_to_add, &tenant_id) { + return Err(RBACError::ProtectedRole); + } + let tenant = tenant_id.as_deref().unwrap_or(DEFAULT_TENANT); // find username by userid, for native users, username is userid, for oauth users, we need to look up let username = if let Some(users) = users().get(tenant) @@ -242,11 +256,21 @@ pub async fn add_roles_to_user( return Err(RBACError::UserDoesNotExist); }; + if let Some(tenant_roles) = roles().get(tenant) + && roles_to_add.iter().any(|name| { + tenant_roles.get(name).is_some_and(|role| { + role.role_type() == &RoleType::Internal || role.deny_super_admin() + }) + }) + { + return Err(RBACError::ProtectedRole); + } + let mut non_existent_roles = Vec::new(); // check if the role exists roles_to_add.iter().for_each(|r| { - if let Some(tenant_roles) = roles().get(tenant_id.as_deref().unwrap_or(DEFAULT_TENANT)) + if let Some(tenant_roles) = roles().get(tenant) && tenant_roles.get(r).is_none() { non_existent_roles.push(r.clone()); @@ -257,16 +281,6 @@ pub async fn add_roles_to_user( return Err(RBACError::RolesDoNotExist(non_existent_roles)); } - if let Some(tenant_roles) = roles().get(tenant_id.as_deref().unwrap_or(DEFAULT_TENANT)) - && roles_to_add.iter().any(|name| { - tenant_roles.get(name).is_some_and(|role| { - role.role_type() == &RoleType::Internal || role.deny_super_admin() - }) - }) - { - return Err(RBACError::ProtectedRole); - } - // update parseable.json first let mut metadata = get_metadata(&tenant_id).await?; if let Some(user) = metadata @@ -455,6 +469,10 @@ pub async fn post_gen_password( #[cfg(test)] mod role_escalation { + // Removing a protected check lets the handler reach `get_metadata`, which + // initialises PARSEABLE and parses the test binary's args. Clap then exits + // the process with code 2 (`unrecognized subcommand`); that symptom means a + // check was removed. use std::collections::HashMap; use actix_web::{HttpMessage, ResponseError, http::StatusCode, test::TestRequest}; diff --git a/src/rbac/mod.rs b/src/rbac/mod.rs index 30c044ff0..0cf74d51c 100644 --- a/src/rbac/mod.rs +++ b/src/rbac/mod.rs @@ -127,6 +127,21 @@ impl Users { .map(|users| users.get(userid).unwrap().protected) } + /// Whether a protected user in the tenant holds any of `role_names`. + /// Takes the USERS lock, so callers must not hold ROLES or USER_GROUPS. + pub fn any_role_held_by_protected_user( + &self, + role_names: &HashSet, + tenant_id: &Option, + ) -> bool { + let tenant_id = tenant_id.as_deref().unwrap_or(DEFAULT_TENANT); + users().get(tenant_id).is_some_and(|users| { + users + .values() + .any(|user| user.protected && !user.roles.is_disjoint(role_names)) + }) + } + pub fn collect_user From<&'a User> + 'static>( &self, tenant_id: &Option, From 7c4630f991e784e6c539f1b686513c90fefa0bbd Mon Sep 17 00:00:00 2001 From: VibeSWE <338779468+VibeSWE@users.noreply.github.com> Date: Sat, 24 Oct 2026 22:05:08 -0500 Subject: [PATCH 12/13] test: cover unprotected holders and tenant scoping in any_role_held_by_protected_user Co-Authored-By: Claude Opus 5.5 --- src/rbac/mod.rs | 79 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/src/rbac/mod.rs b/src/rbac/mod.rs index 0cf74d51c..1bdc21b62 100644 --- a/src/rbac/mod.rs +++ b/src/rbac/mod.rs @@ -1433,3 +1433,82 @@ mod lock_order_tests { } } } + +#[cfg(test)] +mod protected_holder_tests { + use parking_lot::RwLock; + use ulid::Ulid; + + use super::*; + use crate::rbac::map::{TEST_SERIAL, USERS}; + use crate::rbac::user::Basic; + + fn insert_user(tenant: Option, roles: &[&str], protected: bool) { + mut_users().insert(User { + ty: UserType::Native(Basic { + username: format!("user-{}", Ulid::new()), + password_hash: String::new(), + email: None, + }), + roles: roles.iter().map(|role| role.to_string()).collect(), + user_groups: HashSet::new(), + tenant, + protected, + }); + } + + fn names(roles: &[&str]) -> HashSet { + roles.iter().map(|role| role.to_string()).collect() + } + + /// Unique role names keep these tests apart from others sharing the default tenant. + fn role() -> String { + format!("role-{}", Ulid::new()) + } + + #[test] + fn only_protected_holders_of_a_requested_role_count() { + let _serial = TEST_SERIAL.blocking_lock(); + USERS.get_or_init(|| RwLock::new(map::Users::default())); + let tenant = Some(format!("protected-holder-{}", Ulid::new())); + let (held, unprotected, other) = (role(), role(), role()); + insert_user(tenant.clone(), &[&unprotected], false); + insert_user(tenant.clone(), &[&held], true); + + assert!(!Users.any_role_held_by_protected_user(&names(&[&unprotected]), &tenant)); + assert!(!Users.any_role_held_by_protected_user(&names(&[&other]), &tenant)); + assert!(Users.any_role_held_by_protected_user(&names(&[&other, &held]), &tenant)); + } + + #[test] + fn protected_holders_in_other_tenants_do_not_count() { + let _serial = TEST_SERIAL.blocking_lock(); + USERS.get_or_init(|| RwLock::new(map::Users::default())); + let held = role(); + insert_user( + Some(format!("protected-holder-{}", Ulid::new())), + &[&held], + true, + ); + + let other_tenant = Some(format!("protected-holder-{}", Ulid::new())); + assert!(!Users.any_role_held_by_protected_user(&names(&[&held]), &other_tenant)); + assert!(!Users.any_role_held_by_protected_user(&names(&[&held]), &None)); + } + + #[test] + fn no_tenant_means_the_default_tenant() { + let _serial = TEST_SERIAL.blocking_lock(); + USERS.get_or_init(|| RwLock::new(map::Users::default())); + let held = role(); + insert_user(None, &[&held], true); + + assert!(Users.any_role_held_by_protected_user(&names(&[&held]), &None)); + assert!( + Users.any_role_held_by_protected_user( + &names(&[&held]), + &Some(DEFAULT_TENANT.to_owned()) + ) + ); + } +} From 8988e9559688b5e9c0beff9fdcefd191d11207a9 Mon Sep 17 00:00:00 2001 From: VibeSWE <338779468+VibeSWE@users.noreply.github.com> Date: Sat, 24 Oct 2026 22:07:49 -0500 Subject: [PATCH 13/13] Reject protected users in querier delete_user and remove_roles_from_user Completes the port of the standalone protected-user checks to the Query node's user endpoints. Co-Authored-By: Claude Opus 5.5 --- src/handlers/http/modal/query/querier_rbac.rs | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/src/handlers/http/modal/query/querier_rbac.rs b/src/handlers/http/modal/query/querier_rbac.rs index 922b0f7b4..c11a24a36 100644 --- a/src/handlers/http/modal/query/querier_rbac.rs +++ b/src/handlers/http/modal/query/querier_rbac.rs @@ -124,6 +124,10 @@ pub async fn delete_user( let caller_userid = get_user_from_request(&req)?; let tenant = tenant_id.as_deref().unwrap_or(DEFAULT_TENANT); let _guard = UPDATE_LOCK.lock().await; + // fail this request if the user is protected + if Users.is_protected(&userid, &tenant_id) == Some(true) { + return Err(RBACError::ProtectedUser); + } // fail this request if the user does not exist if !Users.contains(&userid, &tenant_id) { return Err(RBACError::UserDoesNotExist); @@ -335,6 +339,11 @@ pub async fn remove_roles_from_user( return Err(RBACError::UserDoesNotExist); }; + // fail this request if the user is protected + if Users.is_protected(&userid, &tenant_id) == Some(true) { + return Err(RBACError::ProtectedUser); + } + // find username by userid, for native users, username is userid, for oauth users, we need to look up let username = if let Some(users) = users().get(tenant) && let Some(user) = users.get(&userid) @@ -662,6 +671,41 @@ mod role_escalation { assert!(matches!(err, RBACError::ProtectedUser), "{err:?}"); } + #[tokio::test] + async fn delete_user_rejects_protected_user() { + // Other modules' tests watch the RBAC map locks; see `map::TEST_SERIAL`. + let _serial = TEST_SERIAL.lock().await; + let (tenant, _, req) = tenant_request(ordinary_role()); + let target = insert_protected_user(&tenant, "admin"); + let Err(err) = delete_user(req, web::Path::from(target.clone())).await else { + panic!("a protected user must be rejected before storage"); + }; + assert!(matches!(err, RBACError::ProtectedUser), "{err:?}"); + assert!(Users.contains(&target, &Some(tenant))); + } + + #[tokio::test] + async fn remove_roles_rejects_protected_user() { + // Other modules' tests watch the RBAC map locks; see `map::TEST_SERIAL`. + let _serial = TEST_SERIAL.lock().await; + let (tenant, _, req) = tenant_request(ordinary_role()); + let target = insert_protected_user(&tenant, "admin"); + let Err(err) = remove_roles_from_user( + req, + web::Path::from(target.clone()), + web::Json(HashSet::from(["admin".to_owned()])), + ) + .await + else { + panic!("a protected user must be rejected before storage"); + }; + assert!(matches!(err, RBACError::ProtectedUser), "{err:?}"); + assert_eq!( + Users.get_user(&target, &Some(tenant)).unwrap().roles, + HashSet::from(["admin".to_owned()]) + ); + } + #[tokio::test] async fn add_roles_checks_protected_role_before_missing_roles() { // Other modules' tests watch the RBAC map locks; see `map::TEST_SERIAL`.