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

@@ -14,6 +14,3 @@ data
 
 # Web vault
 web-vault
-
-ssl/
-vcpkg/

src/api/admin.rs

@@ -567,6 +567,13 @@ async fn update_membership_type(data: Json<MembershipTypeData>, 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?;
 

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)
+    // Custom users with a user/group manage permission need to read the collection list
-    // to be able to assign collections to groups/members. This does NOT expose cipher contents.
+    // (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.has_manage_users()
-        || headers.membership.manage_groups
+        || headers.membership.has_manage_groups();
-        || headers.membership.manage_policies;
     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)
+    // Custom users with a user/group manage permission need the full collection list
-    // so the web client can render member/group collection assignments without crashing on
+    // (metadata only) so the web client can render member/group collection assignments
-    // collections it can't otherwise see. This exposes names/ids only, never cipher contents.
+    // without crashing on collections it can't otherwise see. This exposes names/ids
-    let can_read_collection_list = member.manage_users || member.manage_groups || member.manage_policies;
+    // 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 Some(new_type) = MembershipType::from_str(raw_type) else {
-    let new_type = if let Some(new_type) = MembershipType::from_str(raw_type) {
-        new_type as i32
-    } 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
+    // For a Custom role, the "Manage all collections" parent checkbox is not sent to
-    // Since the parent checkbox is not sent to the server we need to check and verify the child checkboxes
+    // the server; we derive access_all from its three child checkboxes. Admins/Owners
-    // If the box is not checked, the user will still be a manager, but not with the access_all permission
+    // 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
+    // Security: only Admins and Owners may change the granular custom-role management
-    // permissions. A custom user with manage_users must not be able to grant these
+    // permissions. A Custom member with manage_users must not be able to grant them (to
-    // permissions (to themselves or others), which would be a privilege escalation.
+    // themselves or others — a privilege escalation) nor strip flags an Admin/Owner has
-    if headers.membership_type < MembershipType::Admin && (manage_users || manage_groups || manage_policies) {
+    // granted to fellow Custom members. Requests that leave the flags unchanged are
-        err!("Only Admins or Owners can grant custom management permissions")
+    // 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<Value> = policies.iter().map(OrgPolicy::to_json).collect();
 

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())
     }
 }
 

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<Self> {
         conn.run(move |conn| {
             users_organizations::table.filter(users_organizations::uuid.eq(uuid)).first::<Self>(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<Self> {
         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::<Self>(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);
     }
 }