From c380ec750a1ffcaf6b937c60bca7bd3b5e041a5b Mon Sep 17 00:00:00 2001 From: mdecimus Date: Mon, 21 Oct 2024 19:07:29 +0200 Subject: [PATCH] Verify roles and permissions when creating or modifying accounts (closes #874) --- crates/common/src/auth/roles.rs | 6 ++ .../directory/src/backend/internal/manage.rs | 56 ++++++++++++- crates/jmap/src/api/management/principal.rs | 79 ++++++++++++++++++- crates/jmap/src/auth/oauth/registration.rs | 1 + crates/trc/src/ipc/bitset.rs | 2 +- tests/src/directory/internal.rs | 17 +++- 6 files changed, 153 insertions(+), 8 deletions(-) diff --git a/crates/common/src/auth/roles.rs b/crates/common/src/auth/roles.rs index 1180ed79..d94decf1 100644 --- a/crates/common/src/auth/roles.rs +++ b/crates/common/src/auth/roles.rs @@ -167,6 +167,12 @@ impl RolePermissions { self.enabled.difference(&self.disabled); self.enabled } + + pub fn finalize_as_ref(&self) -> Permissions { + let mut enabled = self.enabled.clone(); + enabled.difference(&self.disabled); + enabled + } } fn tenant_admin_permissions() -> Arc { diff --git a/crates/directory/src/backend/internal/manage.rs b/crates/directory/src/backend/internal/manage.rs index 2fe3adb2..5241d1b9 100644 --- a/crates/directory/src/backend/internal/manage.rs +++ b/crates/directory/src/backend/internal/manage.rs @@ -16,7 +16,8 @@ use store::{ use trc::AddContext; use crate::{ - Permission, Principal, QueryBy, Type, MAX_TYPE_ID, ROLE_ADMIN, ROLE_TENANT_ADMIN, ROLE_USER, + Permission, Permissions, Principal, QueryBy, Type, MAX_TYPE_ID, ROLE_ADMIN, ROLE_TENANT_ADMIN, + ROLE_USER, }; use super::{ @@ -37,6 +38,7 @@ pub struct PrincipalList { pub struct UpdatePrincipal<'x> { query: QueryBy<'x>, + allowed_permissions: Option<&'x Permissions>, changes: Vec, tenant_id: Option, create_domains: bool, @@ -54,6 +56,7 @@ pub trait ManageDirectory: Sized { &self, principal: Principal, tenant_id: Option, + allowed_permissions: Option<&Permissions>, ) -> trc::Result; async fn update_principal(&self, params: UpdatePrincipal<'_>) -> trc::Result<()>; async fn delete_principal(&self, by: QueryBy<'_>) -> trc::Result<()>; @@ -192,6 +195,7 @@ impl ManageDirectory for Store { &self, mut principal: Principal, mut tenant_id: Option, + allowed_permissions: Option<&Permissions>, ) -> trc::Result { // Make sure the principal has a name let name = principal.name().to_lowercase(); @@ -358,7 +362,18 @@ impl ManageDirectory for Store { .id() as u64; if !permissions.contains(&permission) { - permissions.push(permission); + if allowed_permissions + .as_ref() + .map_or(true, |p| p.get(permission as usize)) + || field == PrincipalField::DisabledPermissions + { + permissions.push(permission); + } else { + return Err(error( + "Invalid permission", + format!("Your account cannot grant the {name:?} permission").into(), + )); + } } } @@ -1339,7 +1354,20 @@ impl ManageDirectory for Store { .id() as u64; if !permissions.contains(&permission) { - permissions.push(permission); + if params + .allowed_permissions + .as_ref() + .map_or(true, |p| p.get(permission as usize)) + || change.field == PrincipalField::DisabledPermissions + { + permissions.push(permission); + } else { + return Err(error( + "Invalid permission", + format!("Your account cannot grant the {name:?} permission") + .into(), + )); + } } } @@ -1363,7 +1391,19 @@ impl ManageDirectory for Store { })? .id() as u64; - principal.inner.append_int(change.field, permission); + if params + .allowed_permissions + .as_ref() + .map_or(true, |p| p.get(permission as usize)) + || change.field == PrincipalField::DisabledPermissions + { + principal.inner.append_int(change.field, permission); + } else { + return Err(error( + "Invalid permission", + format!("Your account cannot grant the {name:?} permission").into(), + )); + } } ( PrincipalAction::RemoveItem, @@ -1810,6 +1850,7 @@ impl ValidateDirectory for Store { .with_field(PrincipalField::Name, domain.to_string()) .with_field(PrincipalField::Description, domain.to_string()), tenant_id, + None, ) .await .caused_by(trc::location!()) @@ -1859,6 +1900,7 @@ impl<'x> UpdatePrincipal<'x> { changes: Vec::new(), create_domains: false, tenant_id: None, + allowed_permissions: None, } } @@ -1868,6 +1910,7 @@ impl<'x> UpdatePrincipal<'x> { changes: Vec::new(), create_domains: false, tenant_id: None, + allowed_permissions: None, } } @@ -1881,6 +1924,11 @@ impl<'x> UpdatePrincipal<'x> { self } + pub fn with_allowed_permissions(mut self, permissions: &'x Permissions) -> Self { + self.allowed_permissions = permissions.into(); + self + } + pub fn create_domains(mut self) -> Self { self.create_domains = true; self diff --git a/crates/jmap/src/api/management/principal.rs b/crates/jmap/src/api/management/principal.rs index 14f43c64..4e2bbbf6 100644 --- a/crates/jmap/src/api/management/principal.rs +++ b/crates/jmap/src/api/management/principal.rs @@ -119,12 +119,39 @@ impl PrincipalManager for Server { self.assert_supported_directory()?; } + // Validate roles + let tenant_id = access_token.tenant.map(|t| t.id); + for name in principal + .get_str_array(PrincipalField::Roles) + .unwrap_or_default() + { + if let Some(pinfo) = self + .store() + .get_principal_info(name) + .await + .caused_by(trc::location!())? + .filter(|v| v.typ == Type::Role && v.has_tenant_access(tenant_id)) + .or_else(|| PrincipalField::Roles.map_internal_roles(name)) + { + let role_permissions = + self.get_role_permissions(pinfo.id).await?.finalize_as_ref(); + let mut allowed_permissions = role_permissions.clone(); + allowed_permissions.intersection(&access_token.permissions); + if allowed_permissions != role_permissions { + return Err(manage::error( + "Invalid role", + format!("Your account cannot grant the {name:?} role").into(), + )); + } + } + } + // Create principal let result = self .core .storage .data - .create_principal(principal, access_token.tenant.map(|t| t.id)) + .create_principal(principal, tenant_id, Some(&access_token.permissions)) .await?; Ok(JsonResponse::new(json!({ @@ -416,6 +443,53 @@ impl PrincipalManager for Server { } else { expire_token = true; } + + if change.field == PrincipalField::Roles + && matches!( + change.action, + PrincipalAction::AddItem | PrincipalAction::Set + ) + { + let roles = match &change.value { + PrincipalValue::String(v) => std::slice::from_ref(v), + PrincipalValue::StringList(vec) => vec, + PrincipalValue::Integer(_) + | PrincipalValue::IntegerList(_) => continue, + }; + + // Validate roles + let tenant_id = access_token.tenant.map(|t| t.id); + for name in roles { + if let Some(pinfo) = self + .store() + .get_principal_info(name) + .await + .caused_by(trc::location!())? + .filter(|v| { + v.typ == Type::Role + && v.has_tenant_access(tenant_id) + }) + .or_else(|| { + PrincipalField::Roles.map_internal_roles(name) + }) + { + let role_permissions = self + .get_role_permissions(pinfo.id) + .await? + .finalize_as_ref(); + let mut allowed_permissions = + role_permissions.clone(); + allowed_permissions + .intersection(&access_token.permissions); + if allowed_permissions != role_permissions { + return Err(manage::error( + "Invalid role", + format!("Your account cannot grant the {name:?} role").into(), + )); + } + } + } + } } } } @@ -431,7 +505,8 @@ impl PrincipalManager for Server { .update_principal( UpdatePrincipal::by_id(account_id) .with_updates(changes) - .with_tenant(access_token.tenant.map(|t| t.id)), + .with_tenant(access_token.tenant.map(|t| t.id)) + .with_allowed_permissions(&access_token.permissions), ) .await?; diff --git a/crates/jmap/src/auth/oauth/registration.rs b/crates/jmap/src/auth/oauth/registration.rs index 5cdb7c06..c140182f 100644 --- a/crates/jmap/src/auth/oauth/registration.rs +++ b/crates/jmap/src/auth/oauth/registration.rs @@ -81,6 +81,7 @@ impl ClientRegistrationHandler for Server { .with_field(PrincipalField::Emails, request.contacts.clone()) .with_opt_field(PrincipalField::Picture, request.logo_uri.clone()), None, + None, ) .await .caused_by(trc::location!())?; diff --git a/crates/trc/src/ipc/bitset.rs b/crates/trc/src/ipc/bitset.rs index d42d603a..d5bc43d5 100644 --- a/crates/trc/src/ipc/bitset.rs +++ b/crates/trc/src/ipc/bitset.rs @@ -6,7 +6,7 @@ use super::{USIZE_BITS, USIZE_BITS_MASK}; -#[derive(Clone, Debug)] +#[derive(Clone, Debug, PartialEq, Eq)] pub struct Bitset(pub(crate) [usize; N]); impl Bitset { diff --git a/tests/src/directory/internal.rs b/tests/src/directory/internal.rs index 7fbf0195..b57dc7b9 100644 --- a/tests/src/directory/internal.rs +++ b/tests/src/directory/internal.rs @@ -33,7 +33,9 @@ async fn internal_directory() { // A principal without name should fail assert_eq!( - store.create_principal(Principal::default(), None).await, + store + .create_principal(Principal::default(), None, None) + .await, Err(manage::err_missing(PrincipalField::Name)) ); @@ -48,6 +50,7 @@ async fn internal_directory() { } .into(), None, + None, ) .await .unwrap(); @@ -61,6 +64,7 @@ async fn internal_directory() { ..Default::default() } .into(), + None, None ) .await, @@ -77,6 +81,7 @@ async fn internal_directory() { ..Default::default() } .into(), + None, None ) .await, @@ -93,6 +98,7 @@ async fn internal_directory() { } .into(), None, + None, ) .await .unwrap(); @@ -143,6 +149,7 @@ async fn internal_directory() { } .into(), None, + None, ) .await .unwrap(); @@ -201,6 +208,7 @@ async fn internal_directory() { ..Default::default() } .into(), + None, None ) .await, @@ -221,6 +229,7 @@ async fn internal_directory() { } .into(), None, + None, ) .await .unwrap(); @@ -284,6 +293,7 @@ async fn internal_directory() { } .into(), None, + None, ) .await .unwrap(); @@ -297,6 +307,7 @@ async fn internal_directory() { } .into(), None, + None, ) .await .unwrap(); @@ -755,6 +766,7 @@ impl TestInternalDirectory for Store { PrincipalValue::StringList(vec![role.to_string()]), ), None, + None, ) .await .unwrap() @@ -779,6 +791,7 @@ impl TestInternalDirectory for Store { PrincipalValue::StringList(vec!["user".to_string()]), ), None, + None, ) .await .unwrap() @@ -803,6 +816,7 @@ impl TestInternalDirectory for Store { PrincipalValue::StringList(vec![login.to_string()]), ), None, + None, ) .await .unwrap() @@ -863,6 +877,7 @@ impl TestInternalDirectory for Store { Principal::new(0, Type::Domain) .with_field(PrincipalField::Name, domain.to_string()), None, + None, ) .await .unwrap();