diff --git a/src/api/admin.rs b/src/api/admin.rs index 0c85d562..8c60e0c7 100644 --- a/src/api/admin.rs +++ b/src/api/admin.rs @@ -547,6 +547,16 @@ struct MembershipTypeData { fn apply_membership_type_change(membership: &mut Membership, new_type: MembershipType) { let was_custom = membership.atype == MembershipType::Custom; + // Leaving Custom for the legacy Manager role: `access_all` is the internal mirror of + // Edit any collection, but a Manager's `access_all` means the broad "manage all collections" + // grant (Create + Edit + Delete). Carrying an Edit-only mirror over would silently escalate the + // member to Create/Edit/Delete. Only preserve `access_all` when the member actually held the + // full manage-all grant, mirroring the collection-permissions down-migration. Must be evaluated + // before the flags are cleared below (and while the type is still Custom). + if was_custom && new_type == MembershipType::Manager { + membership.access_all = membership.has_manage_all_collections(); + } + // Entering Custom through the Vaultwarden admin panel is deliberately fail-closed because // that UI cannot select granular permissions; they can be granted later through the regular // organization member dialog. @@ -956,12 +966,30 @@ mod tests { apply_membership_type_change(&mut user, MembershipType::Admin); assert!(user.access_all); - let mut custom = membership(MembershipType::Custom); - custom.access_all = true; - custom.edit_any_collection = true; - apply_membership_type_change(&mut custom, MembershipType::Manager); - assert_eq!(custom.atype, MembershipType::Manager as i32); - assert!(custom.access_all); - assert!(!custom.edit_any_collection); + // REGRESSION (privilege escalation, PR #7397 / finding F2): a Custom member whose + // `access_all` is only the Edit-any-collection mirror must NOT be turned into a legacy + // Manager with the broad "manage all collections" `access_all` grant. That would escalate + // an Edit-only member into Create + Edit + Delete. Mirrors the collection down-migration. + let mut edit_only = membership(MembershipType::Custom); + edit_only.access_all = true; + edit_only.edit_any_collection = true; + apply_membership_type_change(&mut edit_only, MembershipType::Manager); + assert_eq!(edit_only.atype, MembershipType::Manager as i32); + assert!(!edit_only.access_all, "Edit-only Custom must not become an access_all Manager"); + assert!(!edit_only.edit_any_collection); + + // A Custom member who genuinely held the full manage-all grant (all three collection + // flags) keeps the equivalent legacy Manager `access_all`. + let mut manage_all = membership(MembershipType::Custom); + manage_all.access_all = true; + manage_all.create_new_collections = true; + manage_all.edit_any_collection = true; + manage_all.delete_any_collection = true; + apply_membership_type_change(&mut manage_all, MembershipType::Manager); + assert_eq!(manage_all.atype, MembershipType::Manager as i32); + assert!(manage_all.access_all, "full manage-all Custom keeps legacy Manager access_all"); + assert!(!manage_all.create_new_collections); + assert!(!manage_all.edit_any_collection); + assert!(!manage_all.delete_any_collection); } } diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index ca30766e..4945f3dc 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -629,12 +629,13 @@ async fn post_bulk_access_collections( err!("Can't find organization details") } - // Security: only callers who can actually manage collections (Admins/Owners, or users with - // full access) may change collection access in bulk. A custom user with only manage_users / - // manage_groups / manage_policies must not be able to modify collection assignments here. - if !headers.membership.has_full_access() { - err!("You don't have permission to modify collection access") - } + // Security: authorization is enforced per collection below via `is_manageable_by_user`, which + // requires a real manage grant on each requested collection (direct users_collections.manage, + // a manage group, membership/group access_all, or Admin/Owner). This matches the single + // collection edit endpoint and the pre-existing behavior of this endpoint. A custom user with + // only manage_users / manage_groups / manage_policies holds no such grant and is therefore + // rejected, while a Manager/Custom member who manages some collections keeps the ability to + // bulk-edit exactly those (a blanket full-access requirement would wrongly deny that here). // Security (audit H-3) and atomicity (audit M-2): validate the whole request against this // organization *before* mutating anything. Every collection must exist in the org and be @@ -3110,11 +3111,16 @@ async fn add_update_group( }))) } +// Reads a single group's details (accessAll, externalId, collection mappings). This is the same +// data the `/groups/details` list endpoint returns, so it uses the same guard: Manage Users OR +// Manage Groups (or Admin/Owner). Requiring Manage Groups here while the list only requires Manage +// Users-or-Groups would let a manage_users member read every group's details in bulk but be denied +// the single-group view of the same data. #[get("/organizations//groups//details")] async fn get_group_details( org_id: OrganizationId, group_id: GroupId, - headers: ManageGroupsHeaders, + headers: ManageUsersOrGroupsHeaders, conn: DbConn, ) -> JsonResult { if org_id != headers.org_id {