From 8a65c6631a9a93842e73551daf6ba96c608c3b57 Mon Sep 17 00:00:00 2001
From: tom27052006 <83423411+tom27052006@users.noreply.github.com>
Date: Wed, 8 Jul 2026 19:39:49 +0200
Subject: [PATCH] Security: gate group delete/member-removal on collection
 access

A Custom member with only the manage_groups permission could revoke other
members' collection access via two endpoints that were missing the
collection-access check enforced elsewhere (put_group_members, edit_member):

- POST /organizations/<org>/groups/<group>/delete-user/<member>
  (post_delete_group_member) removed a member from any group, including
  collection-bearing ones.
- DELETE /organizations/<org>/groups/<group> and its bulk variant
  (delete_group_impl / bulk_delete_groups) deleted collection-bearing groups
  outright, revoking access for all their members.

Neither path can grant access, so confidentiality was never at risk, but both
let a manage_groups-only user tamper with other members' collection access,
contradicting the permission's invariant. Both now require Admin/Owner or full
collection access before touching a group that confers collection access (via
access_all or assigned collections).
---
 src/api/core/organizations.rs | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs
index 9528a756..73465b80 100644
--- a/src/api/core/organizations.rs
+++ b/src/api/core/organizations.rs
@@ -2989,6 +2989,23 @@ async fn delete_group_impl(
         err!("Group not found", "Group uuid is invalid or does not belong to the organization")
     };
 
+    // Security: deleting a group that grants collection access (via `access_all` or assigned
+    // collections) revokes that access for all its members. A custom user with only manage_groups
+    // must not be able to affect collection access, so only callers who can actually manage
+    // collections (Admins/Owners or users with full access) may delete such a group. Mirrors the
+    // restriction in put_group_members / post_delete_group_member. Also covers bulk_delete_groups,
+    // which funnels through this function.
+    let caller_can_manage_collections = headers.membership_type >= MembershipType::Admin
+        || match Membership::find_by_user_and_org(&headers.user.uuid, org_id, conn).await {
+            Some(m) => m.has_full_access(),
+            None => false,
+        };
+    if !caller_can_manage_collections
+        && (group.access_all || !CollectionGroup::find_by_group(group_id, org_id, conn).await.is_empty())
+    {
+        err!("You don't have permission to delete a group that grants collection access")
+    }
+
     log_event(
         EventType::GroupDeleted as i32,
         &group.uuid,
@@ -3159,6 +3176,20 @@ async fn post_delete_group_member(
         err!("Group could not be found or does not belong to the organization.");
     }
 
+    // Security: removing a member from a group that grants collection access (via `access_all`
+    // or assigned collections) revokes that member's collection access. A custom user with only
+    // manage_groups must not be able to affect collection access, so only callers who can actually
+    // manage collections (Admins/Owners or users with full access) may do this. Mirrors the
+    // restriction enforced in put_group_members.
+    let caller_can_manage_collections = headers.membership_type >= MembershipType::Admin
+        || match Membership::find_by_user_and_org(&headers.user.uuid, &org_id, &conn).await {
+            Some(m) => m.has_full_access(),
+            None => false,
+        };
+    if !caller_can_manage_collections && group_confers_collection_access(&group_id, &org_id, &conn).await {
+        err!("You don't have permission to change the membership of a group that grants collection access")
+    }
+
     log_event(
         EventType::OrganizationUserUpdatedGroups as i32,
         &member_id,