Skip to content

Commit

Permalink
Verify roles and permissions when creating or modifying accounts (closes
Browse files Browse the repository at this point in the history 
  #874)
  • Loading branch information
@mdecimus
mdecimus committed Oct 21, 2024
1 parent db8b536 commit c380ec7
Show file tree
Hide file tree
Showing 6 changed files with 153 additions and 8 deletions.
6 changes: 6 additions & 0 deletions crates/common/src/auth/roles.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<RolePermissions> {
Expand Down
56 changes: 52 additions & 4 deletions crates/directory/src/backend/internal/manage.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand All @@ -37,6 +38,7 @@ pub struct PrincipalList {

pub struct UpdatePrincipal<'x> {
query: QueryBy<'x>,
allowed_permissions: Option<&'x Permissions>,
changes: Vec<PrincipalUpdate>,
tenant_id: Option<u32>,
create_domains: bool,
Expand All @@ -54,6 +56,7 @@ pub trait ManageDirectory: Sized {
&self,
principal: Principal,
tenant_id: Option<u32>,
allowed_permissions: Option<&Permissions>,
) -> trc::Result<u32>;
async fn update_principal(&self, params: UpdatePrincipal<'_>) -> trc::Result<()>;
async fn delete_principal(&self, by: QueryBy<'_>) -> trc::Result<()>;
Expand Down Expand Up @@ -192,6 +195,7 @@ impl ManageDirectory for Store {
&self,
mut principal: Principal,
mut tenant_id: Option<u32>,
allowed_permissions: Option<&Permissions>,
) -> trc::Result<u32> {
// Make sure the principal has a name
let name = principal.name().to_lowercase();
Expand Down Expand Up @@ -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(),
));
}
}
}

Expand Down Expand Up @@ -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(),
));
}
}
}

Expand All @@ -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,
Expand Down Expand Up @@ -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!())
Expand Down Expand Up @@ -1859,6 +1900,7 @@ impl<'x> UpdatePrincipal<'x> {
changes: Vec::new(),
create_domains: false,
tenant_id: None,
allowed_permissions: None,
}
}

Expand All @@ -1868,6 +1910,7 @@ impl<'x> UpdatePrincipal<'x> {
changes: Vec::new(),
create_domains: false,
tenant_id: None,
allowed_permissions: None,
}
}

Expand All @@ -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
Expand Down
79 changes: 77 additions & 2 deletions crates/jmap/src/api/management/principal.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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!({
Expand Down Expand Up @@ -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(),
));
}
}
}
}
}
}
}
Expand All @@ -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?;

Expand Down
1 change: 1 addition & 0 deletions crates/jmap/src/auth/oauth/registration.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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!())?;
Expand Down
2 changes: 1 addition & 1 deletion crates/trc/src/ipc/bitset.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

use super::{USIZE_BITS, USIZE_BITS_MASK};

#[derive(Clone, Debug)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct Bitset<const N: usize>(pub(crate) [usize; N]);

impl<const N: usize> Bitset<N> {
Expand Down
17 changes: 16 additions & 1 deletion tests/src/directory/internal.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
);

Expand All @@ -48,6 +50,7 @@ async fn internal_directory() {
}
.into(),
None,
None,
)
.await
.unwrap();
Expand All @@ -61,6 +64,7 @@ async fn internal_directory() {
..Default::default()
}
.into(),
None,
None
)
.await,
Expand All @@ -77,6 +81,7 @@ async fn internal_directory() {
..Default::default()
}
.into(),
None,
None
)
.await,
Expand All @@ -93,6 +98,7 @@ async fn internal_directory() {
}
.into(),
None,
None,
)
.await
.unwrap();
Expand Down Expand Up @@ -143,6 +149,7 @@ async fn internal_directory() {
}
.into(),
None,
None,
)
.await
.unwrap();
Expand Down Expand Up @@ -201,6 +208,7 @@ async fn internal_directory() {
..Default::default()
}
.into(),
None,
None
)
.await,
Expand All @@ -221,6 +229,7 @@ async fn internal_directory() {
}
.into(),
None,
None,
)
.await
.unwrap();
Expand Down Expand Up @@ -284,6 +293,7 @@ async fn internal_directory() {
}
.into(),
None,
None,
)
.await
.unwrap();
Expand All @@ -297,6 +307,7 @@ async fn internal_directory() {
}
.into(),
None,
None,
)
.await
.unwrap();
Expand Down Expand Up @@ -755,6 +766,7 @@ impl TestInternalDirectory for Store {
PrincipalValue::StringList(vec![role.to_string()]),
),
None,
None,
)
.await
.unwrap()
Expand All @@ -779,6 +791,7 @@ impl TestInternalDirectory for Store {
PrincipalValue::StringList(vec!["user".to_string()]),
),
None,
None,
)
.await
.unwrap()
Expand All @@ -803,6 +816,7 @@ impl TestInternalDirectory for Store {
PrincipalValue::StringList(vec![login.to_string()]),
),
None,
None,
)
.await
.unwrap()
Expand Down Expand Up @@ -863,6 +877,7 @@ impl TestInternalDirectory for Store {
Principal::new(0, Type::Domain)
.with_field(PrincipalField::Name, domain.to_string()),
None,
None,
)
.await
.unwrap();
Expand Down

0 comments on commit c380ec7

Please sign in to comment.