diff --git a/src/handlers/http/apikeys.rs b/src/handlers/http/apikeys.rs index fa2c79999..ed4b908d9 100644 --- a/src/handlers/http/apikeys.rs +++ b/src/handlers/http/apikeys.rs @@ -15,7 +15,7 @@ use crate::{ rbac::{ Users, map::{read_user_groups, roles, users}, - role::model::{DefaultPrivilege, Role}, + role::model::{DefaultPrivilege, Role, RoleType}, user::{User, UserType}, }, utils::{get_user_and_tenant_from_request, is_admin}, @@ -240,29 +240,43 @@ 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, 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) + 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))); + } + // 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() + }) }) - .cloned() - .collect(); - if missing.is_empty() { - Ok(()) - } else { - Err(ApiKeyError::Rbac(RBACError::RolesDoNotExist(missing))) + { + return Err(ApiKeyError::Rbac(RBACError::ProtectedRole)); } + Ok(()) } /// POST /api/prism/v1/apikeys @@ -276,8 +290,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. @@ -861,4 +875,161 @@ mod tests { assert_eq!(names(&unknown, None), ["orphan-key"]); assert!(names(&unknown, reader()).is_empty()); } + + mod role_escalation { + use actix_web::{HttpMessage, ResponseError, test::TestRequest}; + use chrono::TimeDelta; + + use super::*; + use crate::rbac::user::Basic; + + 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. + const ELEVATED_ROLE: &str = "elevated"; + + /// A fresh tenant laid out like the default tenant after `map::init`. + fn tenant() -> String { + 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 }]), + ), + ( + INTERNAL_ROLE.to_owned(), + Role::create_internal_role(vec![DefaultPrivilege::Admin]), + ), + ( + 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 _serial = blocking_rbac_state(); + let tenant = Some(tenant()); + assert!(validate_roles(&role_set(READER_ROLE), &tenant).is_ok()); + } + + #[test] + fn validate_roles_rejects_internal_role() { + let _serial = blocking_rbac_state(); + let tenant = Some(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:?}" + ); + } + + #[test] + fn validate_roles_rejects_super_admin_privilege_role() { + let _serial = blocking_rbac_state(); + 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:?}" + ); + } + + #[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; + 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_rbac.rs b/src/handlers/http/modal/query/querier_rbac.rs index 3bd829827..c11a24a36 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,21 @@ pub async fn post_user( if !non_existent_roles.is_empty() { return Err(RBACError::RolesDoNotExist(non_existent_roles)); } - let _guard = UPDATE_LOCK.lock().await; + // 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| { + 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 @@ -109,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); @@ -222,6 +241,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) @@ -232,11 +260,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()); @@ -301,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) @@ -432,3 +475,298 @@ pub async fn post_gen_password( Ok(new_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}; + use chrono::TimeDelta; + use parking_lot::RwLock; + use ulid::Ulid; + + use super::*; + use crate::{ + handlers::TENANT_ID, + rbac::{ + map::{ + ROLES, SESSIONS, SessionKey, Sessions, TEST_SERIAL, 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()]) + ); + } + + /// 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 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`. + 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`. + 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 321efa9b0..e01cf29cd 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}"); } @@ -202,3 +202,142 @@ pub async fn delete( Ok(HttpResponse::Ok().finish()) } + +#[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}; + use chrono::TimeDelta; + use parking_lot::RwLock; + use ulid::Ulid; + + use super::*; + use crate::{ + handlers::TENANT_ID, + rbac::{ + Users, + map::{ + ROLES, SESSIONS, SessionKey, Sessions, TEST_SERIAL, 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() { + // 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]); + + 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" + ); + } + + #[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`. + let _serial = TEST_SERIAL.lock().await; + 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" + ); + } + } +} diff --git a/src/handlers/http/role.rs b/src/handlers/http/role.rs index 1c064dfe2..3ce94d2d3 100644 --- a/src/handlers/http/role.rs +++ b/src/handlers/http/role.rs @@ -397,3 +397,52 @@ 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, TEST_SERIAL}, + 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() { + // 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() + .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" + ); + } +} diff --git a/src/rbac/mod.rs b/src/rbac/mod.rs index 30c044ff0..1bdc21b62 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, @@ -1418,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()) + ) + ); + } +}