Browse Source

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).
pull/7397/head
tom27052006 2 weeks ago
parent
commit
8a65c6631a
  1. 31
      src/api/core/organizations.rs

31
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,

Loading…
Cancel
Save