From 43fb19f190e75b00ab45cba31effe95bee50252f Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Wed, 8 Jul 2026 08:11:27 +0200 Subject: [PATCH] Harden custom-role permission checks Security fixes and hardening following a review of the custom-role permissions feature: - Gate the manage_* flags on the Custom membership type via new Membership::has_manage_* helpers. Previously a stale flag (e.g. left over after changing a member's type through the admin panel, which does not go through edit_member) would keep granting management permissions to a member of any type. - Clear the manage_* flags in admin::update_membership_type when a member is changed away from the Custom type. - edit_member: reject any *change* to the manage_* flags by non-admin callers. This keeps the previous protection against granting flags, and additionally prevents a manage_users member from silently stripping flags an Admin/Owner granted to fellow Custom members. Unchanged flags still pass, so the regular edit dialog keeps working. - list_policies: restrict the full policy list to Admins/Owners and Custom members holding at least one management permission. The previous loosening to ManagerHeadersLoose also exposed all policies (including their configuration data) to plain Managers. - Collection list metadata (get_org_collections{,_details}) is now only readable with manage_users or manage_groups; manage_policies does not need the collection list. - find_confirmed_and_manage_all_by_org: include Custom members with access_all, matching the pre-existing behaviour for Managers. - Fix the partial_cmp_MembershipType unit test (Custom is no longer converted to Manager) and document that Manager and Custom share an access level in Ord while remaining distinct roles under PartialEq. - Replace the remaining stale "HACK" comments and the raw "4" type comparison in send_invite; drop unrelated .gitignore entries. --- .gitignore | 3 -- src/api/admin.rs | 7 ++++ src/api/core/organizations.rs | 70 +++++++++++++++++++++-------------- src/auth.rs | 10 +++-- src/db/models/organization.rs | 32 ++++++++++++++-- 5 files changed, 85 insertions(+), 37 deletions(-) diff --git a/.gitignore b/.gitignore index 06d677ca..e991430e 100644 --- a/.gitignore +++ b/.gitignore @@ -14,6 +14,3 @@ data # Web vault web-vault - -ssl/ -vcpkg/ diff --git a/src/api/admin.rs b/src/api/admin.rs index 7037bfb1..c16fb866 100644 --- a/src/api/admin.rs +++ b/src/api/admin.rs @@ -567,6 +567,13 @@ async fn update_membership_type(data: Json, token: AdminToke } member_to_edit.atype = new_type; + // The manage_* permission flags only apply to the Custom role; clear them on any other + // type so a member changed away from Custom does not retain stale management permissions. + if new_type != MembershipType::Custom { + member_to_edit.manage_users = false; + member_to_edit.manage_groups = false; + member_to_edit.manage_policies = false; + } // This check is also done at api::organizations::{accept_invite, _confirm_invite, _activate_member, edit_member}, update_membership_type OrgPolicy::check_user_allowed(&member_to_edit, "modify", &conn).await?; diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index e8403fc1..8d63d75d 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -392,12 +392,12 @@ async fn get_org_collections(org_id: OrganizationId, headers: ManagerHeadersLoos err!("Organization not found", "Organization id's do not match"); } - // Custom users with a manage permission need to read the collection list (metadata only) - // to be able to assign collections to groups/members. This does NOT expose cipher contents. + // Custom users with a user/group manage permission need to read the collection list + // (metadata only) to be able to assign collections to groups/members. This does NOT + // expose cipher contents. manage_policies does not need the collection list. let can_read_collection_list = headers.membership.has_full_access() - || headers.membership.manage_users - || headers.membership.manage_groups - || headers.membership.manage_policies; + || headers.membership.has_manage_users() + || headers.membership.has_manage_groups(); if !can_read_collection_list { err_code!("Resource not found.", "User does not have full access", rocket::http::Status::NotFound.code); } @@ -430,10 +430,11 @@ async fn get_org_collections_details(org_id: OrganizationId, headers: ManagerHea let has_full_access_to_org = member.has_full_access() || (CONFIG.org_groups_enabled() && GroupUser::has_full_access_by_member(&org_id, &member.uuid, &conn).await); - // Custom users with a manage permission need the full collection list (metadata only) - // so the web client can render member/group collection assignments without crashing on - // collections it can't otherwise see. This exposes names/ids only, never cipher contents. - let can_read_collection_list = member.manage_users || member.manage_groups || member.manage_policies; + // Custom users with a user/group manage permission need the full collection list + // (metadata only) so the web client can render member/group collection assignments + // without crashing on collections it can't otherwise see. This exposes names/ids + // only, never cipher contents. manage_policies does not need the collection list. + let can_read_collection_list = member.has_manage_users() || member.has_manage_groups(); // Get all admins, owners and managers who can manage/access all // Those are currently not listed in the col_users but need to be listed too. @@ -1075,13 +1076,8 @@ async fn send_invite( let data: InviteData = data.into_inner(); data.validate(&org_id, &conn).await?; - // HACK: We need the raw user-type to be sure custom role is selected to determine the access_all permission - // The from_str() will convert the custom role type into a manager role type let raw_type = &data.r#type.into_string(); - // Membership::from_str will convert custom (4) to manager (3) - let new_type = if let Some(new_type) = MembershipType::from_str(raw_type) { - new_type as i32 - } else { + let Some(new_type) = MembershipType::from_str(raw_type) else { err!("Invalid type") }; @@ -1089,11 +1085,11 @@ async fn send_invite( err!("Only Owners can invite Managers, Admins or Owners") } - // HACK: This converts the Custom role which has the `Manage all collections` box checked into an access_all flag - // Since the parent checkbox is not sent to the server we need to check and verify the child checkboxes - // If the box is not checked, the user will still be a manager, but not with the access_all permission + // For a Custom role, the "Manage all collections" parent checkbox is not sent to + // the server; we derive access_all from its three child checkboxes. Admins/Owners + // implicitly have access to all collections. let access_all = new_type >= MembershipType::Admin - || (raw_type.eq("4") + || (new_type == MembershipType::Custom && data.permissions.get("editAnyCollection") == Some(&json!(true)) && data.permissions.get("deleteAnyCollection") == Some(&json!(true)) && data.permissions.get("createNewCollections") == Some(&json!(true))); @@ -1135,7 +1131,7 @@ async fn send_invite( let mut new_member = Membership::new(user.uuid.clone(), org_id.clone(), Some(headers.user.email.clone())); new_member.access_all = access_all; - new_member.atype = new_type; + new_member.atype = new_type as i32; new_member.status = member_status; new_member.save(&conn).await?; @@ -1211,7 +1207,7 @@ async fn send_invite( // manage_groups) are allowed to assign groups when inviting. let caller_can_manage_groups = headers.membership_type >= MembershipType::Admin || match Membership::find_by_user_and_org(&headers.user.uuid, &org_id, &conn).await { - Some(m) => m.manage_groups, + Some(m) => m.has_manage_groups(), None => false, }; @@ -1635,11 +1631,17 @@ async fn edit_member( } } - // Security: only Admins and Owners may grant the granular custom-role management - // permissions. A custom user with manage_users must not be able to grant these - // permissions (to themselves or others), which would be a privilege escalation. - if headers.membership_type < MembershipType::Admin && (manage_users || manage_groups || manage_policies) { - err!("Only Admins or Owners can grant custom management permissions") + // Security: only Admins and Owners may change the granular custom-role management + // permissions. A Custom member with manage_users must not be able to grant them (to + // themselves or others — a privilege escalation) nor strip flags an Admin/Owner has + // granted to fellow Custom members. Requests that leave the flags unchanged are + // allowed, so such members can still use the regular edit dialog. + if headers.membership_type < MembershipType::Admin + && (manage_users != member_to_edit.manage_users + || manage_groups != member_to_edit.manage_groups + || manage_policies != member_to_edit.manage_policies) + { + err!("Only Admins or Owners can change custom management permissions") } // Security: only callers who can actually manage collections (Admins/Owners, or users @@ -1701,7 +1703,7 @@ async fn edit_member( // with manage_groups) are allowed to change it. For others we leave group membership untouched. let caller_can_manage_groups = headers.membership_type >= MembershipType::Admin || match Membership::find_by_user_and_org(&headers.user.uuid, &org_id, &conn).await { - Some(m) => m.manage_groups, + Some(m) => m.has_manage_groups(), None => false, }; @@ -2036,6 +2038,20 @@ async fn list_policies(org_id: OrganizationId, headers: ManagerHeadersLoose, con if org_id != headers.membership.org_uuid { err!("Organization not found", "Organization id's do not match"); } + + // Security: only Admins/Owners, or Custom members holding at least one management + // permission, may read the full policy list (the Admin Console needs it to load). + // Plain Managers and Custom members without any permission keep the pre-existing + // behaviour of having no access here. + let membership = &headers.membership; + if !(membership.atype >= MembershipType::Admin + || membership.has_manage_users() + || membership.has_manage_groups() + || membership.has_manage_policies()) + { + err!("You don't have permission to view policies") + } + let policies = OrgPolicy::find_by_org(&org_id, &conn).await; let policies_json: Vec = policies.iter().map(OrgPolicy::to_json).collect(); diff --git a/src/auth.rs b/src/auth.rs index 75fe78da..d3c74899 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -720,15 +720,17 @@ impl OrgHeaders { } // Custom-role permission checks. Admins and Owners implicitly hold every // permission; a Custom member holds a permission only if the matching flag - // is set on their Membership. + // is set on their Membership. The has_manage_* helpers gate the flags on the + // Custom type, so stale flags on other types can never grant anything. fn can_manage_users(&self) -> bool { - self.is_confirmed() && (self.membership_type >= MembershipType::Admin || self.membership.manage_users) + self.is_confirmed() && (self.membership_type >= MembershipType::Admin || self.membership.has_manage_users()) } fn can_manage_groups(&self) -> bool { - self.is_confirmed() && (self.membership_type >= MembershipType::Admin || self.membership.manage_groups) + self.is_confirmed() && (self.membership_type >= MembershipType::Admin || self.membership.has_manage_groups()) } fn can_manage_policies(&self) -> bool { - self.is_confirmed() && (self.membership_type >= MembershipType::Admin || self.membership.manage_policies) + self.is_confirmed() + && (self.membership_type >= MembershipType::Admin || self.membership.has_manage_policies()) } } diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index c2ba53b6..92ec4973 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -124,6 +124,11 @@ impl Ord for MembershipType { // Custom is treated as a low-privilege base role (same level as Manager for // ordering purposes); its elevated capabilities are governed by the explicit // manage_* permission flags on the Membership, not by this ordering. + // + // NOTE: Manager and Custom therefore share an access level while being distinct + // variants: the derived `PartialEq` compares the role itself (Manager != Custom), + // while this ordering compares access levels (neither is greater than the other). + // Keep that in mind before relying on `cmp() == Equal` implying equality. const ACCESS_LEVEL: [i32; 5] = [ 3, // Owner 2, // Admin @@ -809,6 +814,21 @@ impl Membership { (self.access_all || self.atype >= MembershipType::Admin) && self.has_status(MembershipStatus::Confirmed) } + // The granular manage_* permission flags are only meaningful while the membership is of + // the Custom type. Gating them on the type here ensures that a stale flag left over from + // a type change (e.g. via the admin panel) can never grant anything. + pub fn has_manage_users(&self) -> bool { + self.has_type(MembershipType::Custom) && self.manage_users + } + + pub fn has_manage_groups(&self) -> bool { + self.has_type(MembershipType::Custom) && self.manage_groups + } + + pub fn has_manage_policies(&self) -> bool { + self.has_type(MembershipType::Custom) && self.manage_policies + } + pub async fn find_by_uuid(uuid: &MembershipId, conn: &DbConn) -> Option { conn.run(move |conn| { users_organizations::table.filter(users_organizations::uuid.eq(uuid)).first::(conn).ok() @@ -915,7 +935,7 @@ impl Membership { .await } - // Get all users which are either owner or admin, or a manager which can manage/access all + // Get all users which are either owner or admin, or a manager/custom member which can manage/access all pub async fn find_confirmed_and_manage_all_by_org(org_uuid: &OrganizationId, conn: &DbConn) -> Vec { conn.run(move |conn| { users_organizations::table @@ -925,7 +945,7 @@ impl Membership { users_organizations::atype .eq_any(vec![MembershipType::Owner as i32, MembershipType::Admin as i32]) .or(users_organizations::atype - .eq(MembershipType::Manager as i32) + .eq_any(vec![MembershipType::Manager as i32, MembershipType::Custom as i32]) .and(users_organizations::access_all.eq(true))), ) .load::(conn) @@ -1260,6 +1280,12 @@ mod tests { assert!(MembershipType::Owner > MembershipType::Admin); assert!(MembershipType::Admin > MembershipType::Manager); assert!(MembershipType::Manager > MembershipType::User); - assert!(MembershipType::Manager == MembershipType::from_str("4").unwrap()); + assert!(MembershipType::Custom == MembershipType::from_str("4").unwrap()); + // Manager and Custom share the same access level, but are distinct roles + assert!(MembershipType::Manager != MembershipType::Custom); + assert!(MembershipType::Manager >= MembershipType::Custom); + assert!(MembershipType::Custom >= MembershipType::Manager); + assert!(MembershipType::Custom > MembershipType::User); + assert!(MembershipType::Admin > MembershipType::Custom); } }