diff --git a/migrations/mysql/2026-07-24-120000_drop_membership_access_all/down.sql b/migrations/mysql/2026-07-24-120000_drop_membership_access_all/down.sql new file mode 100644 index 00000000..01295bf5 --- /dev/null +++ b/migrations/mysql/2026-07-24-120000_drop_membership_access_all/down.sql @@ -0,0 +1,6 @@ +-- Recreate the column and repopulate it from the role/permission model that replaced it, restoring +-- the invariant older server versions rely on: access_all == access to every collection. That is +-- exactly Owners/Admins, plus Custom members holding `edit_any_collection`. +ALTER TABLE users_organizations ADD COLUMN access_all BOOLEAN NOT NULL DEFAULT FALSE; +UPDATE users_organizations SET access_all = TRUE WHERE atype IN (0, 1); +UPDATE users_organizations SET access_all = TRUE WHERE atype = 4 AND edit_any_collection = TRUE; diff --git a/migrations/mysql/2026-07-24-120000_drop_membership_access_all/up.sql b/migrations/mysql/2026-07-24-120000_drop_membership_access_all/up.sql new file mode 100644 index 00000000..e11fb611 --- /dev/null +++ b/migrations/mysql/2026-07-24-120000_drop_membership_access_all/up.sql @@ -0,0 +1,5 @@ +-- The membership `access_all` flag was Vaultwarden's pre-permissions patch for "this member can +-- reach every collection". It is now fully represented by the role model: Owners/Admins hold it +-- implicitly, and a Custom member holds it via `edit_any_collection`. Drop the redundant column. +-- This only concerns users_organizations; groups.access_all is a separate, still-supported feature. +ALTER TABLE users_organizations DROP COLUMN access_all; diff --git a/migrations/postgresql/2026-07-24-120000_drop_membership_access_all/down.sql b/migrations/postgresql/2026-07-24-120000_drop_membership_access_all/down.sql new file mode 100644 index 00000000..01295bf5 --- /dev/null +++ b/migrations/postgresql/2026-07-24-120000_drop_membership_access_all/down.sql @@ -0,0 +1,6 @@ +-- Recreate the column and repopulate it from the role/permission model that replaced it, restoring +-- the invariant older server versions rely on: access_all == access to every collection. That is +-- exactly Owners/Admins, plus Custom members holding `edit_any_collection`. +ALTER TABLE users_organizations ADD COLUMN access_all BOOLEAN NOT NULL DEFAULT FALSE; +UPDATE users_organizations SET access_all = TRUE WHERE atype IN (0, 1); +UPDATE users_organizations SET access_all = TRUE WHERE atype = 4 AND edit_any_collection = TRUE; diff --git a/migrations/postgresql/2026-07-24-120000_drop_membership_access_all/up.sql b/migrations/postgresql/2026-07-24-120000_drop_membership_access_all/up.sql new file mode 100644 index 00000000..e11fb611 --- /dev/null +++ b/migrations/postgresql/2026-07-24-120000_drop_membership_access_all/up.sql @@ -0,0 +1,5 @@ +-- The membership `access_all` flag was Vaultwarden's pre-permissions patch for "this member can +-- reach every collection". It is now fully represented by the role model: Owners/Admins hold it +-- implicitly, and a Custom member holds it via `edit_any_collection`. Drop the redundant column. +-- This only concerns users_organizations; groups.access_all is a separate, still-supported feature. +ALTER TABLE users_organizations DROP COLUMN access_all; diff --git a/migrations/sqlite/2026-07-24-120000_drop_membership_access_all/down.sql b/migrations/sqlite/2026-07-24-120000_drop_membership_access_all/down.sql new file mode 100644 index 00000000..01295bf5 --- /dev/null +++ b/migrations/sqlite/2026-07-24-120000_drop_membership_access_all/down.sql @@ -0,0 +1,6 @@ +-- Recreate the column and repopulate it from the role/permission model that replaced it, restoring +-- the invariant older server versions rely on: access_all == access to every collection. That is +-- exactly Owners/Admins, plus Custom members holding `edit_any_collection`. +ALTER TABLE users_organizations ADD COLUMN access_all BOOLEAN NOT NULL DEFAULT FALSE; +UPDATE users_organizations SET access_all = TRUE WHERE atype IN (0, 1); +UPDATE users_organizations SET access_all = TRUE WHERE atype = 4 AND edit_any_collection = TRUE; diff --git a/migrations/sqlite/2026-07-24-120000_drop_membership_access_all/up.sql b/migrations/sqlite/2026-07-24-120000_drop_membership_access_all/up.sql new file mode 100644 index 00000000..e11fb611 --- /dev/null +++ b/migrations/sqlite/2026-07-24-120000_drop_membership_access_all/up.sql @@ -0,0 +1,5 @@ +-- The membership `access_all` flag was Vaultwarden's pre-permissions patch for "this member can +-- reach every collection". It is now fully represented by the role model: Owners/Admins hold it +-- implicitly, and a Custom member holds it via `edit_any_collection`. Drop the redundant column. +-- This only concerns users_organizations; groups.access_all is a separate, still-supported feature. +ALTER TABLE users_organizations DROP COLUMN access_all; diff --git a/src/api/admin.rs b/src/api/admin.rs index 8c60e0c7..b03946af 100644 --- a/src/api/admin.rs +++ b/src/api/admin.rs @@ -545,35 +545,13 @@ 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. - if new_type == MembershipType::Custom && !was_custom { + // 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. Any non-Custom role carries no custom flags at all. Only a member + // that is already Custom and stays Custom keeps its existing flags. + let stays_custom = new_type == MembershipType::Custom && membership.atype == MembershipType::Custom; + if !stays_custom { membership.clear_custom_permissions(); - membership.access_all = false; - } - if new_type != MembershipType::Custom { - membership.clear_custom_permissions(); - } - - // Prevent stale access_all from surviving a demotion to User. Admins/Owners have implicit - // full access, while legacy Manager access_all is intentionally preserved for compatibility. - match new_type { - MembershipType::Owner | MembershipType::Admin => membership.access_all = true, - MembershipType::User => membership.access_all = false, - MembershipType::Manager | MembershipType::Custom => {} } membership.atype = new_type as i32; @@ -936,9 +914,8 @@ mod tests { } #[test] - fn admin_type_changes_clear_custom_permissions_and_stale_access() { + fn admin_type_changes_clear_custom_permissions() { let mut custom = membership(MembershipType::Custom); - custom.access_all = true; custom.manage_users = true; custom.create_new_collections = true; custom.edit_any_collection = true; @@ -946,50 +923,36 @@ mod tests { apply_membership_type_change(&mut custom, MembershipType::User); assert_eq!(custom.atype, MembershipType::User as i32); - assert!(!custom.access_all); assert!(!custom.manage_users); assert!(!custom.create_new_collections); assert!(!custom.edit_any_collection); assert!(!custom.delete_any_collection); + // Entering Custom through the admin panel is fail-closed: no granular permissions are set. let mut admin = membership(MembershipType::Admin); - admin.access_all = true; apply_membership_type_change(&mut admin, MembershipType::Custom); assert_eq!(admin.atype, MembershipType::Custom as i32); - assert!(!admin.access_all, "entering Custom through this UI must be fail-closed"); assert!(!admin.has_manage_all_collections()); + assert!(!admin.edit_any_collection); } #[test] - fn admin_and_legacy_manager_access_all_behavior_is_preserved() { - let mut user = membership(MembershipType::User); - apply_membership_type_change(&mut user, MembershipType::Admin); - assert!(user.access_all); - - // 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); + fn admin_custom_to_custom_keeps_flags_but_other_transitions_clear() { + // A member kept as Custom retains its granular flags: the admin panel does not touch them; + // they are managed through the regular organization member dialog. + let mut custom = membership(MembershipType::Custom); + custom.manage_users = true; + custom.edit_any_collection = true; + apply_membership_type_change(&mut custom, MembershipType::Custom); + assert_eq!(custom.atype, MembershipType::Custom as i32); + assert!(custom.manage_users); + assert!(custom.edit_any_collection); + + // Promoting to Admin/Owner drops any stale custom flags. + let mut promo = membership(MembershipType::Custom); + promo.edit_any_collection = true; + apply_membership_type_change(&mut promo, MembershipType::Admin); + assert_eq!(promo.atype, MembershipType::Admin as i32); + assert!(!promo.edit_any_collection); } } diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index edc88881..b5309f9d 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -219,7 +219,6 @@ async fn create_organization(headers: Headers, data: Json, conn: DbConn let collection = Collection::new(org.uuid.clone(), data.collection_name, None); member.akey = data.key; - member.access_all = true; member.atype = MembershipType::Owner as i32; member.status = MembershipStatus::Confirmed as i32; @@ -539,8 +538,8 @@ async fn post_organization_collections( err!("Organization not found", "Organization id's do not match"); } - // Create is independent from Edit/Delete. In particular, Edit any collection's internal - // access_all representation must not implicitly grant this endpoint. + // Create is independent from Edit/Delete. In particular, Edit any collection (full access to + // every collection) must not implicitly grant this endpoint. if !headers.membership.can_create_new_collections() { err!("You don't have permission to create collections") } @@ -582,9 +581,8 @@ async fn post_organization_collections( // bulk-access endpoints apply. Create is deliberately independent from Edit/Delete, so a Custom // member holding only `create_new_collections` must not be able to hand a manage row to another // member or to a group (nor to itself) while creating the collection. For such callers the - // requested `manage` is forced to false; Admin/Owner, Custom-with-`delete_any_collection` and the - // legacy access_all Manager keep it. Evaluated after the collection exists so the per-collection - // lookup sees it. + // requested `manage` is forced to false; Admin/Owner and Custom-with-`delete_any_collection` + // keep it. Evaluated after the collection exists so the per-collection lookup sees it. let may_grant_manage = caller_may_grant_collection_manage(&headers.membership, &collection.uuid, &conn).await; for group in data.groups { @@ -604,7 +602,7 @@ async fn post_organization_collections( err!("User is not part of organization") }; - if member.access_all { + if member.grants_access_to_all_collections() { continue; } @@ -648,13 +646,12 @@ async fn post_bulk_access_collections( // Security (F-1): authorization is enforced per collection below via `auth::can_edit_collection`, // the exact same Custom-aware check the single-collection edit endpoint (`ManagerHeaders`) uses. - // Edit any collection (or Admin/Owner) may bulk-edit every collection; a legacy Manager keeps its - // broad per-collection helper; any other Custom member must hold a real per-collection Manage - // grant. In particular a Custom member's membership/group `access_all` does NOT satisfy this here - // (it did under the previous `is_manageable_by_user` check, which diverged from the single-edit - // endpoint). A custom user with only manage_users / manage_groups / manage_policies holds no such - // grant and is rejected, while a member who manages some collections keeps the ability to - // bulk-edit exactly those. + // Edit any collection (or Admin/Owner) may bulk-edit every collection; any other Custom member + // must hold a real per-collection Manage grant. In particular a Custom member's group + // `access_all` does NOT satisfy this here (it did under the previous `is_manageable_by_user` + // check, which diverged from the single-edit endpoint). A custom user with only manage_users / + // manage_groups / manage_policies holds no such grant and is rejected, while a member who manages + // some collections keeps the ability to bulk-edit exactly those. // 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 @@ -726,7 +723,7 @@ async fn post_bulk_access_collections( err!("User is not part of organization") }; - if member.access_all { + if member.grants_access_to_all_collections() { continue; } @@ -826,7 +823,7 @@ async fn post_organization_collection_update( err!("User is not part of organization") }; - if member.access_all { + if member.grants_access_to_all_collections() { continue; } @@ -1211,10 +1208,11 @@ impl CustomRolePermissions { } } - /// Bitwarden grants a Custom member with Edit any collection full read/edit/manage access to - /// organization ciphers. Vaultwarden's existing access_all flag is the internal data-plane - /// representation of that capability. Create and Delete remain completely independent. - fn access_all_for(self, member_type: MembershipType) -> bool { + /// Whether the requested role/permissions give this member access to *every* collection in the + /// org: Admins/Owners implicitly, and a Custom member holding Edit any collection. Such members + /// do not need (and must not be given) individual per-collection assignments. Create and Delete + /// remain completely independent of this. + fn grants_full_collection_access(self, member_type: MembershipType) -> bool { member_type >= MembershipType::Admin || (member_type == MembershipType::Custom && self.edit_any_collection) } @@ -1285,13 +1283,14 @@ async fn send_invite( }; if new_type != MembershipType::User && headers.membership_type != MembershipType::Owner { - err!("Only Owners can invite Managers, Admins or Owners") + err!("Only Owners can invite Admins, Owners or Custom members") } - // manageAllCollections is a client-only aggregate. Persist its three children independently; - // only Edit any collection maps to the existing all-cipher access representation. + // manageAllCollections is a client-only aggregate. Persist its three children independently. + // Whether the member reaches every collection (Admin/Owner, or Custom + Edit any collection) + // decides whether we skip creating individual per-collection assignments below. let custom_permissions = CustomRolePermissions::from_request(new_type, &data.permissions); - let access_all = custom_permissions.access_all_for(new_type); + let grants_full_access = custom_permissions.grants_full_collection_access(new_type); let mut user_created: bool = false; for email in &data.emails { @@ -1333,7 +1332,6 @@ 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 as i32; custom_permissions.apply_to(&mut new_member); new_member.status = member_status; @@ -1386,8 +1384,8 @@ async fn send_invite( None => false, }; - // If no accessAll, add the collections received - if !access_all && caller_can_manage_collections { + // If the member does not already reach every collection, add the collections received + if !grants_full_access && caller_can_manage_collections { // Security (F-1): a per-collection `manage` grant carries delete authority, so the // caller may only confer it on collections they could delete themselves. Otherwise a // caller acting via Edit-any-collection could invite an account they control with a @@ -1688,7 +1686,7 @@ async fn confirm_invite_impl( }; if member_to_confirm.atype != MembershipType::User && headers.membership_type != MembershipType::Owner { - err!("Only Owners can confirm Managers, Admins or Owners") + err!("Only Owners can confirm Admins, Owners or Custom members") } if member_to_confirm.status != MembershipStatus::Accepted as i32 { @@ -1813,7 +1811,7 @@ async fn edit_member( }; let custom_permissions = CustomRolePermissions::from_request(new_type, &data.permissions); - let access_all = custom_permissions.access_all_for(new_type); + let grants_full_access = custom_permissions.grants_full_collection_access(new_type); let Some(mut member_to_edit) = Membership::find_by_uuid_and_org(&member_id, &org_id, &conn).await else { err!("The specified user isn't member of the organization") @@ -1827,13 +1825,13 @@ async fn edit_member( } // Security: only Admins and Owners may change a member's role type at all. A Custom member - // with manage_users must not change roles: raising a member to Manager/Custom grants - // collection-"manage" on every collection they can already write (see the `atype >= Manager` - // branch in `Collection`/`Membership` json), and lowering it revokes that access — both are - // collection-access changes this caller is not entitled to make, even though the custom - // permission flags and access_all are already gated below. Requests that leave the role - // unchanged are allowed, so such members can still use the regular edit dialog. The - // Admin/Owner guard above still governs Admin/Owner transitions for Owners. + // with manage_users must not change roles: raising a member to Custom grants collection-"manage" + // on every collection they can already write (see the `atype >= Custom` branch in + // `Collection`/`Membership` json), and lowering it revokes that access — both are collection- + // access changes this caller is not entitled to make, even though the custom permission flags + // are already gated below. Requests that leave the role unchanged are allowed, so such members + // can still use the regular edit dialog. The Admin/Owner guard above still governs Admin/Owner + // transitions for Owners. if !may_change_member_type(headers.membership_type, member_to_edit.atype, new_type) { err!("Only Admins or Owners can change a member's role") } @@ -1871,14 +1869,10 @@ async fn edit_member( None => false, }; - // Security: `access_all` grants full access to every collection, so only callers who may - // manage collections are allowed to change it. Otherwise a custom user with only manage_users - // could enable Edit any collection on any member (including themselves) to grant full - // collection access — a privilege escalation. For everyone else we keep the member's existing - // access_all grant untouched (neither granted nor revoked). - if caller_can_manage_collections { - member_to_edit.access_all = access_all; - } + // Edit any collection (the successor of the removed access_all flag) grants full access to + // every collection. It is part of the granular custom permissions applied here, and the + // differs_from guard above already prevents a non-Admin caller from changing it — so a Custom + // member with only manage_users can never grant themselves or others full collection access. custom_permissions.apply_to(&mut member_to_edit); member_to_edit.atype = new_type as i32; @@ -1897,8 +1891,8 @@ async fn edit_member( // Edit-any-collection thus cannot hand another member a manage/delete grant it lacks. let caller = Membership::find_by_user_and_org(&headers.user.uuid, &org_id, &conn).await; - // If no accessAll, add the collections received - if !access_all { + // If the member does not already reach every collection, add the collections received + if !grants_full_access { for col in data.collections.iter().flatten() { match Collection::find_by_uuid_and_org(&col.id, &org_id, &conn).await { None => err!("Collection not found in Organization"), @@ -2209,7 +2203,7 @@ async fn post_org_import( } else { // Collection creation through an organization import is governed by the same // independent permission as the regular create endpoint. In particular, - // Edit any collection's access_all mirror must not satisfy this check. + // Edit any collection (full access to every collection) must not satisfy this check. if !headers.membership.can_create_new_collections() { err!(Compact, "The current user isn't allowed to create new collections") } @@ -3142,31 +3136,25 @@ async fn group_confers_collection_access(group_id: &GroupId, org_id: &Organizati /// /// Security (F-1, edit-any -> delete-any escalation): a `manage` grant carries collection *delete* /// authority — `CollectionDeleteHeaders` accepts it via `has_explicit_collection_manage_access`. -/// Without this gate a Custom member holding only `edit_any_collection` (whose `access_all` mirror -/// makes every collection "manageable") could, through the collection-access / group endpoints, -/// hand a `manage` row to a group they belong to (or to a manager-level member) and thereby gain -/// deletion — a capability `edit_any_collection` must never imply. +/// Without this gate a Custom member holding only `edit_any_collection` (which grants full access to +/// every collection) could, through the collection-access / group endpoints, hand a `manage` row to +/// a group they belong to (or to a manager-level member) and thereby gain deletion — a capability +/// `edit_any_collection` must never imply. /// /// We therefore allow granting `manage` on a collection only to a caller who could delete that same /// collection themselves, mirroring `collection_delete_access` exactly so it can never hand out a -/// right the caller lacks: Admin/Owner and Custom-with-`delete_any_collection` always qualify; an -/// exact legacy Manager uses its per-collection manage helper; any other Custom member must hold a -/// real explicit manage grant. This is strictly subtractive — it can only ever downgrade a requested -/// `manage` to `false`, never grant it — so it opens no new access, and delete-capable members -/// (including all Admins/Owners) are unaffected. +/// right the caller lacks: Admin/Owner and Custom-with-`delete_any_collection` always qualify; any +/// other Custom member must hold a real explicit manage grant. This is strictly subtractive — it can +/// only ever downgrade a requested `manage` to `false`, never grant it — so it opens no new access, +/// and delete-capable members (including all Admins/Owners) are unaffected. async fn caller_may_grant_collection_manage(caller: &Membership, col_id: &CollectionId, conn: &DbConn) -> bool { match caller_manage_grant_role_check(caller) { // Role alone decides it (Admin/Owner or delete_any -> yes; User/unknown/unconfirmed -> no). Some(decision) => decision, - // Manager/Custom: the answer is per-collection and must reflect a *real* manage grant. + // Custom without delete_any: the answer is per-collection and must reflect a *real* manage + // grant. A Custom member must prove a real users_collections.manage / + // collections_groups.manage grant; Edit any collection deliberately does not count here. None => match MembershipType::from_i32(caller.atype) { - // The exact legacy Manager keeps its broad per-collection manage helper (which also - // honors membership/group access_all), matching its pre-existing delete authorization. - Some(MembershipType::Manager) => { - Collection::is_coll_manageable_by_user(col_id, &caller.user_uuid, conn).await - } - // A Custom member must prove a real users_collections.manage / collections_groups.manage - // grant; edit_any_collection's access_all mirror deliberately does not count here. Some(MembershipType::Custom) => caller.has_explicit_collection_manage_access(col_id, conn).await, _ => false, }, @@ -3190,7 +3178,7 @@ fn caller_manage_grant_role_check(caller: &Membership) -> Option { return Some(false); } match MembershipType::from_i32(caller.atype) { - Some(MembershipType::Manager | MembershipType::Custom) => None, + Some(MembershipType::Custom) => None, _ => Some(false), } } @@ -3910,12 +3898,11 @@ mod tests { // manufacturing. This is what stops edit-any from escalating into delete-any. let mut edit_any = confirmed_member(MembershipType::Custom); edit_any.edit_any_collection = true; - edit_any.access_all = true; // the internal mirror of edit_any must not shortcut to yes assert_eq!(caller_manage_grant_role_check(&edit_any), None); - // A flagless Custom / exact Manager also defer to the per-collection DB check. + // A flagless Custom member (this is what a migrated legacy Manager becomes) also defers to + // the per-collection DB check. assert_eq!(caller_manage_grant_role_check(&confirmed_member(MembershipType::Custom)), None); - assert_eq!(caller_manage_grant_role_check(&confirmed_member(MembershipType::Manager)), None); // Plain User never qualifies. assert_eq!(caller_manage_grant_role_check(&confirmed_member(MembershipType::User)), Some(false)); @@ -3952,27 +3939,23 @@ mod tests { #[test] fn manage_users_caller_cannot_change_member_role() { let user = MembershipType::User as i32; - let manager = MembershipType::Manager as i32; let custom = MembershipType::Custom as i32; // Admins and Owners may change a member's role. - assert!(may_change_member_type(MembershipType::Owner, user, MembershipType::Manager)); + assert!(may_change_member_type(MembershipType::Owner, user, MembershipType::Custom)); assert!(may_change_member_type(MembershipType::Admin, user, MembershipType::Custom)); - // A below-Admin caller (Manager / Custom-with-manage_users) may only submit an unchanged - // role, so the regular edit dialog keeps working. + // A below-Admin caller (Custom-with-manage_users) may only submit an unchanged role, so the + // regular edit dialog keeps working. assert!(may_change_member_type(MembershipType::Custom, user, MembershipType::User)); assert!(may_change_member_type(MembershipType::Custom, custom, MembershipType::Custom)); - assert!(may_change_member_type(MembershipType::Manager, manager, MembershipType::Manager)); // REGRESSION (privilege escalation, PR #7397 / finding F1): a caller below Admin must NOT - // be able to change a member's role. Promoting User -> Manager/Custom grants that member - // collection-"manage" on their writable collections (atype >= Manager), and demoting + // be able to change a member's role. Promoting User -> Custom grants that member + // collection-"manage" on their writable collections (atype >= Custom), and demoting // revokes it — collection-access changes a manage_users caller is not entitled to make. - assert!(!may_change_member_type(MembershipType::Custom, user, MembershipType::Manager)); assert!(!may_change_member_type(MembershipType::Custom, user, MembershipType::Custom)); - assert!(!may_change_member_type(MembershipType::Custom, manager, MembershipType::User)); - assert!(!may_change_member_type(MembershipType::Manager, custom, MembershipType::User)); + assert!(!may_change_member_type(MembershipType::Custom, custom, MembershipType::User)); } #[test] @@ -4009,8 +3992,8 @@ mod tests { assert_eq!(parsed.create_new_collections, create, "mask={mask:03b}"); assert_eq!(parsed.edit_any_collection, edit, "mask={mask:03b}"); assert_eq!(parsed.delete_any_collection, delete, "mask={mask:03b}"); - // Only Edit any collection maps to all-cipher access. Create/Delete must never do so. - assert_eq!(parsed.access_all_for(MembershipType::Custom), edit, "mask={mask:03b}"); + // Only Edit any collection maps to all-collection access. Create/Delete must never do so. + assert_eq!(parsed.grants_full_collection_access(MembershipType::Custom), edit, "mask={mask:03b}"); } } @@ -4035,11 +4018,11 @@ mod tests { let user = CustomRolePermissions::from_request(MembershipType::User, &permissions); assert_eq!(user, CustomRolePermissions::default()); - assert!(!user.access_all_for(MembershipType::User)); + assert!(!user.grants_full_collection_access(MembershipType::User)); let admin = CustomRolePermissions::from_request(MembershipType::Admin, &permissions); assert_eq!(admin, CustomRolePermissions::default()); - assert!(admin.access_all_for(MembershipType::Admin)); + assert!(admin.grants_full_collection_access(MembershipType::Admin)); } #[test] diff --git a/src/api/core/public.rs b/src/api/core/public.rs index 33189e78..f58cd6da 100644 --- a/src/api/core/public.rs +++ b/src/api/core/public.rs @@ -118,7 +118,6 @@ async fn ldap_import(data: Json, token: PublicToken, conn: DbConn let mut new_member = Membership::new(user.uuid.clone(), org_id.clone(), Some(org_email.clone())); new_member.set_external_id(Some(user_data.external_id.clone())); - new_member.access_all = false; new_member.atype = MembershipType::User as i32; new_member.status = member_status; diff --git a/src/auth.rs b/src/auth.rs index 4716b6e2..05cdbd78 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -724,8 +724,10 @@ impl OrgHeaders { fn is_confirmed_and_admin(&self) -> bool { self.membership_status == MembershipStatus::Confirmed && self.membership_type >= MembershipType::Admin } + // "Manager-level or above": a confirmed Custom, Admin or Owner member. (The legacy Manager role + // has been folded into Custom, which shares the same authorization rank.) fn is_confirmed_and_manager(&self) -> bool { - self.membership_status == MembershipStatus::Confirmed && self.membership_type >= MembershipType::Manager + self.membership_status == MembershipStatus::Confirmed && self.membership_type >= MembershipType::Custom } fn is_confirmed_and_owner(&self) -> bool { self.membership_status == MembershipStatus::Confirmed && self.membership_type == MembershipType::Owner @@ -960,7 +962,6 @@ fn get_col_id(request: &Request<'_>) -> Option { #[derive(Clone, Copy, Debug, Eq, PartialEq)] enum CollectionManageAccess { Any, - LegacyManager, ExplicitManage, Denied, } @@ -972,9 +973,6 @@ fn collection_access_by_role(membership: &Membership, custom_has_any_access: boo match MembershipType::from_i32(membership.atype) { Some(MembershipType::Owner | MembershipType::Admin) => CollectionManageAccess::Any, - // Keep the pre-Custom role's broad behavior isolated to an exact legacy Manager. Its - // existing helper intentionally accepts membership/group access_all. - Some(MembershipType::Manager) => CollectionManageAccess::LegacyManager, Some(MembershipType::Custom) if custom_has_any_access => CollectionManageAccess::Any, // A Custom member must prove an actual users_collections.manage or // collections_groups.manage assignment. In particular, groups.access_all is not Manage. @@ -1006,9 +1004,6 @@ async fn can_manage_collection( ) -> bool { match access { CollectionManageAccess::Any => true, - CollectionManageAccess::LegacyManager => { - Collection::is_coll_manageable_by_user(collection_uuid, &membership.user_uuid, conn).await - } CollectionManageAccess::ExplicitManage => { membership.has_explicit_collection_manage_access(collection_uuid, conn).await } @@ -1035,7 +1030,7 @@ pub(crate) async fn can_edit_collection( } /// ManagerHeaders authorizes collection updates. A Custom member with Edit any collection can -/// update every collection; otherwise the caller must be at least a legacy Manager and have the +/// update every collection; otherwise the caller must be a Custom member (or above) holding the /// per-collection Manage permission. Read and delete use separate guards so Edit cannot /// accidentally imply Delete. pub struct ManagerHeaders { @@ -1144,8 +1139,8 @@ impl From for Headers { /// limitCollectionDeletion=true, so deleting *any* collection requires the explicit Delete any /// collection permission (or Admin/Owner). Deleting an individual collection is additionally /// allowed for members holding the per-collection Manage grant on it. Custom members use the -/// explicit assignment only; unlike the exact legacy Manager path, membership/group access_all -/// never counts as their per-collection Manage grant. +/// explicit assignment only; a group `access_all` grant never counts as their per-collection Manage +/// grant. pub struct CollectionDeleteHeaders { pub host: String, pub device: Device, @@ -1171,11 +1166,11 @@ impl<'r> FromRequest<'r> for CollectionDeleteHeaders { match collection_delete_access(&headers.membership) { CollectionManageAccess::Any => {} CollectionManageAccess::Denied => { - // Custom is a distinct, fail-closed role. Edit any collection and access_all alone - // must not satisfy a Delete request without either Delete any or explicit Manage. + // Custom is a distinct, fail-closed role. Edit any collection alone must not satisfy + // a Delete request without either Delete any or an explicit per-collection Manage. err_handler!("You need the 'Delete any collection' permission to call this endpoint") } - access @ (CollectionManageAccess::LegacyManager | CollectionManageAccess::ExplicitManage) => { + access @ CollectionManageAccess::ExplicitManage => { let Outcome::Success(conn) = DbConn::from_request(request).await else { err_handler!("Error getting DB") }; @@ -1641,26 +1636,20 @@ mod tests { #[test] fn flagless_custom_requires_explicit_manage_for_edit_read_and_delete() { + // A flagless Custom member (this is what a migrated legacy Manager becomes) must prove a + // real per-collection Manage grant for every collection operation. ExplicitManage invokes + // the database helper that only accepts users_collections.manage / collections_groups.manage + // — an external groups.access_all grant deliberately does not switch to a broad helper. let custom = membership(MembershipType::Custom); assert_eq!(collection_edit_access(&custom), CollectionManageAccess::ExplicitManage); assert_eq!(collection_read_access(&custom), CollectionManageAccess::ExplicitManage); assert_eq!(collection_delete_access(&custom), CollectionManageAccess::ExplicitManage); - - // Neither a stale membership access_all value nor an external groups.access_all grant may - // switch a Custom member to the legacy broad helper. ExplicitManage invokes the database - // helper that only accepts users_collections.manage / collections_groups.manage. - let mut access_all = membership(MembershipType::Custom); - access_all.access_all = true; - assert_eq!(collection_edit_access(&access_all), CollectionManageAccess::ExplicitManage); - assert_eq!(collection_read_access(&access_all), CollectionManageAccess::ExplicitManage); - assert_eq!(collection_delete_access(&access_all), CollectionManageAccess::ExplicitManage); } #[test] fn custom_any_permissions_remain_independent() { let mut edit_any = membership(MembershipType::Custom); edit_any.edit_any_collection = true; - edit_any.access_all = true; assert_eq!(collection_edit_access(&edit_any), CollectionManageAccess::Any); assert_eq!(collection_read_access(&edit_any), CollectionManageAccess::Any); // Edit-any alone is not blanket Delete. It still permits deletion of an explicitly managed @@ -1675,18 +1664,15 @@ mod tests { } #[test] - fn exact_legacy_manager_keeps_broad_helper() { - let manager = membership(MembershipType::Manager); - assert_eq!(collection_edit_access(&manager), CollectionManageAccess::LegacyManager); - assert_eq!(collection_read_access(&manager), CollectionManageAccess::LegacyManager); - assert_eq!(collection_delete_access(&manager), CollectionManageAccess::LegacyManager); - + fn admin_and_user_collection_access_roles() { let admin = membership(MembershipType::Admin); assert_eq!(collection_edit_access(&admin), CollectionManageAccess::Any); + assert_eq!(collection_read_access(&admin), CollectionManageAccess::Any); assert_eq!(collection_delete_access(&admin), CollectionManageAccess::Any); let user = membership(MembershipType::User); assert_eq!(collection_edit_access(&user), CollectionManageAccess::Denied); + assert_eq!(collection_read_access(&user), CollectionManageAccess::Denied); assert_eq!(collection_delete_access(&user), CollectionManageAccess::Denied); } diff --git a/src/db/models/cipher.rs b/src/db/models/cipher.rs index 69600e5b..77301514 100644 --- a/src/db/models/cipher.rs +++ b/src/db/models/cipher.rs @@ -881,7 +881,12 @@ impl Cipher { .and(collections_groups::groups_uuid.eq(groups::uuid))), ) .filter(ciphers::user_uuid.eq(user_uuid)) // Cipher owner - .or_filter(users_organizations::access_all.eq(true)) // access_all in org + // Edit any collection (Custom) or org admin/owner — the successor of access_all + .or_filter( + users_organizations::edit_any_collection + .eq(true) + .or(users_organizations::atype.le(MembershipType::Admin as i32)), + ) .or_filter(users_collections::user_uuid.eq(user_uuid)) // Access to collection .or_filter(groups::access_all.eq(true)) // Access via groups .or_filter(collections_groups::collections_uuid.is_not_null()) // Access via groups @@ -918,7 +923,12 @@ impl Cipher { .and(users_organizations::user_uuid.eq(users_collections::user_uuid))), ) .filter(ciphers::user_uuid.eq(user_uuid)) // Cipher owner - .or_filter(users_organizations::access_all.eq(true)) // access_all in org + // Edit any collection (Custom) or org admin/owner — the successor of access_all + .or_filter( + users_organizations::edit_any_collection + .eq(true) + .or(users_organizations::atype.le(MembershipType::Admin as i32)), + ) .or_filter(users_collections::user_uuid.eq(user_uuid)) // Access to collection .into_boxed(); @@ -1041,8 +1051,9 @@ impl Cipher { .and(collections_groups::groups_uuid.eq(groups::uuid))), ) .filter( - users_organizations::access_all - .eq(true) // User has access all + users_organizations::edit_any_collection + .eq(true) // Custom "Edit any collection" (successor of access_all) + .or(users_organizations::atype.le(MembershipType::Admin as i32)) // or org admin/owner .or(users_collections::user_uuid .eq(user_uuid) // User has access to collection .and(users_collections::read_only.eq(false))) @@ -1072,8 +1083,9 @@ impl Cipher { .and(users_collections::user_uuid.eq(user_uuid.clone()))), ) .filter( - users_organizations::access_all - .eq(true) // User has access all + users_organizations::edit_any_collection + .eq(true) // Custom "Edit any collection" (successor of access_all) + .or(users_organizations::atype.le(MembershipType::Admin as i32)) // or org admin/owner .or(users_collections::user_uuid .eq(user_uuid) // User has access to collection .and(users_collections::read_only.eq(false))), @@ -1116,8 +1128,9 @@ impl Cipher { .and(collections_groups::groups_uuid.eq(groups::uuid))), ) .filter( - users_organizations::access_all - .eq(true) // User has access all + users_organizations::edit_any_collection + .eq(true) // Custom "Edit any collection" (successor of access_all) + .or(users_organizations::atype.le(MembershipType::Admin as i32)) // or org admin/owner .or(users_collections::user_uuid .eq(user_uuid) // User has access to collection .and(users_collections::read_only.eq(false))) @@ -1148,8 +1161,9 @@ impl Cipher { .and(users_collections::user_uuid.eq(user_uuid.clone()))), ) .filter( - users_organizations::access_all - .eq(true) // User has access all + users_organizations::edit_any_collection + .eq(true) // Custom "Edit any collection" (successor of access_all) + .or(users_organizations::atype.le(MembershipType::Admin as i32)) // or org admin/owner .or(users_collections::user_uuid .eq(user_uuid) // User has access to collection .and(users_collections::read_only.eq(false))) @@ -1194,7 +1208,7 @@ impl Cipher { .and(collections_groups::groups_uuid.eq(groups::uuid))), ) .or_filter(users_collections::user_uuid.eq(user_uuid)) // User has access to collection - .or_filter(users_organizations::access_all.eq(true)) // User has access all + .or_filter(users_organizations::edit_any_collection.eq(true)) // Custom "Edit any collection" (successor of access_all) .or_filter(users_organizations::atype.le(MembershipType::Admin as i32)) // User is admin or owner .or_filter(groups::access_all.eq(true)) //Access via group .or_filter(collections_groups::collections_uuid.is_not_null()) //Access via group diff --git a/src/db/models/collection.rs b/src/db/models/collection.rs index 51a8d2c2..8145be69 100644 --- a/src/db/models/collection.rs +++ b/src/db/models/collection.rs @@ -98,13 +98,13 @@ impl Collection { ) -> Value { let (read_only, hide_passwords, manage) = if let Some(cipher_sync_data) = cipher_sync_data { match cipher_sync_data.members.get(&self.org_uuid) { - // Only for Manager types Bitwarden returns true for the manage option - // Owners and Admins always have true. Users are not able to have full access - Some(m) if m.has_full_access() => (false, false, m.atype >= MembershipType::Manager), + // Only for manager-level (Custom) members does Bitwarden return true for the manage + // option. Owners and Admins always have true. Users cannot have full access. + Some(m) if m.has_full_access() => (false, false, m.atype >= MembershipType::Custom), Some(m) => { - // Only let a manager-level member (Manager or Custom) manage collections + // Only let a manager-level (Custom) member manage collections // when they have full read/write access - let is_manager = m.atype >= MembershipType::Manager; + let is_manager = m.atype >= MembershipType::Custom; if let Some(cu) = cipher_sync_data.user_collections.get(&self.uuid) { ( cu.read_only, @@ -125,12 +125,12 @@ impl Collection { } } else { match Membership::find_confirmed_by_user_and_org(user_uuid, &self.org_uuid, conn).await { - Some(m) if m.has_full_access() => (false, false, m.atype >= MembershipType::Manager), - Some(m) if m.atype >= MembershipType::Manager && self.is_manageable_by_user(user_uuid, conn).await => { + Some(m) if m.has_full_access() => (false, false, m.atype >= MembershipType::Custom), + Some(m) if m.atype >= MembershipType::Custom && self.is_manageable_by_user(user_uuid, conn).await => { (false, false, true) } Some(m) => { - let is_manager = m.atype >= MembershipType::Manager; + let is_manager = m.atype >= MembershipType::Custom; let read_only = !self.is_writable_by_user(user_uuid, conn).await; let hide_passwords = self.hide_passwords_for_user(user_uuid, conn).await; (read_only, hide_passwords, is_manager && !read_only && !hide_passwords) @@ -255,8 +255,11 @@ impl Collection { users_collections::user_uuid .eq(user_uuid) .or( - // Directly accessed collection - users_organizations::access_all.eq(true), // access_all in Organization + // Full-access member: Custom "Edit any collection" or org admin/owner + // (successor of the removed membership access_all) + users_organizations::edit_any_collection + .eq(true) + .or(users_organizations::atype.le(MembershipType::Admin as i32)), ) .or( groups::access_all.eq(true), // access_all in groups @@ -288,10 +291,15 @@ impl Collection { .and(users_organizations::user_uuid.eq(user_uuid.clone()))), ) .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) - .filter(users_collections::user_uuid.eq(user_uuid).or( - // Directly accessed collection - users_organizations::access_all.eq(true), // access_all in Organization - )) + .filter( + users_collections::user_uuid.eq(user_uuid).or( + // Full-access member: Custom "Edit any collection" or org admin/owner + // (successor of the removed membership access_all) + users_organizations::edit_any_collection + .eq(true) + .or(users_organizations::atype.le(MembershipType::Admin as i32)), + ), + ) .select(collections::all_columns) .distinct() .load::(conn) @@ -375,8 +383,8 @@ impl Collection { .eq(uuid) .or( // Directly accessed collection - users_organizations::access_all.eq(true).or( - // access_all in Organization + users_organizations::edit_any_collection.eq(true).or( + // Custom "Edit any collection" or org admin/owner (successor of access_all) users_organizations::atype.le(MembershipType::Admin as i32), // Org admin or owner ), ) @@ -411,8 +419,8 @@ impl Collection { .filter(collections::uuid.eq(uuid)) .filter(users_collections::collection_uuid.eq(uuid).or( // Directly accessed collection - users_organizations::access_all.eq(true).or( - // access_all in Organization + users_organizations::edit_any_collection.eq(true).or( + // Custom "Edit any collection" or org admin/owner (successor of access_all) users_organizations::atype.le(MembershipType::Admin as i32), // Org admin or owner ), )) @@ -456,7 +464,7 @@ impl Collection { .filter( users_organizations::atype .le(MembershipType::Admin as i32) // Org admin or owner - .or(users_organizations::access_all.eq(true)) // access_all via membership + .or(users_organizations::edit_any_collection.eq(true)) // Custom "Edit any collection" (successor of access_all) .or(users_collections::collection_uuid .eq(&self.uuid) // write access given to collection .and(users_collections::read_only.eq(false))) @@ -489,7 +497,7 @@ impl Collection { .filter( users_organizations::atype .le(MembershipType::Admin as i32) // Org admin or owner - .or(users_organizations::access_all.eq(true)) // access_all via membership + .or(users_organizations::edit_any_collection.eq(true)) // Custom "Edit any collection" (successor of access_all) .or(users_collections::collection_uuid .eq(&self.uuid) // write access given to collection .and(users_collections::read_only.eq(false))), @@ -536,8 +544,8 @@ impl Collection { .and(users_collections::hide_passwords.eq(true)) .or( // Directly accessed collection - users_organizations::access_all.eq(true).or( - // access_all in Organization + users_organizations::edit_any_collection.eq(true).or( + // Custom "Edit any collection" or org admin/owner (successor of access_all) users_organizations::atype.le(MembershipType::Admin as i32), // Org admin or owner ), ) @@ -595,8 +603,8 @@ impl Collection { .and(users_collections::manage.eq(true)) .or( // Directly accessed collection - users_organizations::access_all.eq(true).or( - // access_all in Organization + users_organizations::edit_any_collection.eq(true).or( + // Custom "Edit any collection" or org admin/owner (successor of access_all) users_organizations::atype.le(MembershipType::Admin as i32), // Org admin or owner ), ) @@ -946,7 +954,7 @@ impl CollectionMembership { "hidePasswords": self.hide_passwords, "manage": membership_type >= MembershipType::Admin || self.manage - || (membership_type >= MembershipType::Manager + || (membership_type >= MembershipType::Custom && !self.read_only && !self.hide_passwords), }) diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index 81ceb15c..d55d44b6 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -52,7 +52,6 @@ pub struct Membership { pub invited_by_email: Option, - pub access_all: bool, pub akey: String, pub status: i32, pub atype: i32, @@ -104,7 +103,10 @@ pub enum MembershipType { Owner = 0, Admin = 1, User = 2, - Manager = 3, + // NOTE: the legacy Manager role (wire value 3) has been folded into Custom. It is no longer a + // distinct variant: it is never persisted or emitted, and an incoming value 3 is mapped onto + // Custom for backward compatibility (see `from_str`). The Custom discriminant stays 4 because + // that is the only role modern Bitwarden clients understand as carrying custom permissions. Custom = 4, } @@ -114,8 +116,10 @@ impl MembershipType { "0" | "Owner" => Some(MembershipType::Owner), "1" | "Admin" => Some(MembershipType::Admin), "2" | "User" => Some(MembershipType::User), - "3" | "Manager" => Some(MembershipType::Manager), - "4" | "Custom" => Some(MembershipType::Custom), + // "3"/"Manager" is the legacy Manager role. Modern clients no longer offer it, but an old + // client or stored request may still send value 3. Custom supersedes Manager, so accept + // and fold it onto Custom. + "3" | "Manager" | "4" | "Custom" => Some(MembershipType::Custom), _ => None, } } @@ -123,7 +127,7 @@ impl MembershipType { const fn access_rank(self) -> u8 { match self { Self::User => 0, - Self::Manager | Self::Custom => 1, + Self::Custom => 1, Self::Admin => 2, Self::Owner => 3, } @@ -132,11 +136,9 @@ impl MembershipType { impl Ord for MembershipType { fn cmp(&self, other: &MembershipType) -> Ordering { - // Manager and Custom intentionally share the same authorization rank. A total ordering - // still has to distinguish unequal enum variants, otherwise `Ord` would disagree with - // `Eq` and ordered maps/sets could collapse one role into the other. The discriminant is a - // stable tie-breaker and places Custom after Manager, preserving `Custom >= Manager` while - // keeping both roles below Admin. + // Roles are ordered by their authorization rank, not by their raw discriminant (Custom's + // discriminant is 4 but it ranks between User and Admin). The discriminant is kept as a + // stable tie-breaker so `Ord` never disagrees with `Eq`. self.access_rank().cmp(&other.access_rank()).then_with(|| (*self as i32).cmp(&(*other as i32))) } } @@ -271,7 +273,6 @@ impl Membership { org_uuid, invited_by_email, - access_all: false, akey: String::new(), status: MembershipStatus::Accepted as i32, atype: MembershipType::User as i32, @@ -467,10 +468,9 @@ impl Membership { "manageScim": false // Not supported (Not AGPLv3 Licensed) }); - // edit_any_collection is internally mirrored to access_all to provide Bitwarden-compatible - // cipher access, but it must not accidentally grant collection creation. The client treats - // limitCollectionCreation=false as an independent create grant, so compute it from the - // actual role/permission rather than access_all for Custom members. + // Edit any collection grants full read/edit access to every collection, but it must not + // accidentally grant collection creation. The client treats limitCollectionCreation=false as + // an independent create grant, so compute it from the actual role/permission. let limit_collection_creation = self.limit_collection_creation(); // https://github.com/bitwarden/server/blob/9ebe16587175b1c0e9208f84397bb75d0d595510/src/Api/AdminConsole/Models/Response/ProfileOrganizationResponseModel.cs @@ -565,7 +565,9 @@ impl Membership { CONFIG.org_groups_enabled() && Group::is_in_full_access_group(&self.user_uuid, &self.org_uuid, conn).await; // If collections are to be included, only include them if the user does not have full access via a group or defined to the user it self - let collections: Vec = if include_collections && !(full_access_group || self.access_all) { + let collections: Vec = if include_collections + && !(full_access_group || self.grants_access_to_all_collections()) + { // Get all collections for the user here already to prevent more queries let cu: HashMap = CollectionUser::find_by_organization_and_user_uuid(&self.org_uuid, &self.user_uuid, conn) @@ -586,12 +588,12 @@ impl Membership { .into_iter() .filter_map(|c| { let (read_only, hide_passwords, manage) = if self.has_full_access() { - (false, false, self.atype >= MembershipType::Manager) + (false, false, self.atype >= MembershipType::Custom) } else if let Some(cu) = cu.get(&c.uuid) { ( cu.read_only, cu.hide_passwords, - cu.manage || (self.atype >= MembershipType::Manager && !cu.read_only && !cu.hide_passwords), + cu.manage || (self.atype >= MembershipType::Custom && !cu.read_only && !cu.hide_passwords), ) // If previous checks failed it might be that this user has access via a group, but we should not return those elements here // Those are returned via a special group endpoint @@ -648,7 +650,9 @@ impl Membership { "status": status, "type": membership_type, - "accessAll": self.access_all, + // `access_all` no longer exists as a stored flag; report the effective all-collection + // access so clients that still read this obsolete field keep seeing a consistent value. + "accessAll": self.grants_access_to_all_collections(), "twoFactorEnabled": twofactor_enabled, "resetPasswordEnrolled": self.reset_password_key.is_some(), "hasMasterPassword": !user.password_hash.is_empty(), @@ -675,7 +679,7 @@ impl Membership { } pub async fn to_json_details(&self, conn: &DbConn) -> Value { - let coll_uuids = if self.access_all { + let coll_uuids = if self.grants_access_to_all_collections() { vec![] // If we have complete access, no need to fill the array } else { let collections = @@ -707,7 +711,8 @@ impl Membership { "status": status, "type": self.atype, - "accessAll": self.access_all, + // Obsolete stored flag removed; report the effective all-collection access instead. + "accessAll": self.grants_access_to_all_collections(), "collections": coll_uuids, "object": "organizationUserDetails", @@ -816,10 +821,19 @@ impl Membership { } pub fn has_full_access(&self) -> bool { - (self.access_all || self.has_edit_any_collection() || self.atype >= MembershipType::Admin) + (self.has_edit_any_collection() || self.atype >= MembershipType::Admin) && self.has_status(MembershipStatus::Confirmed) } + /// Whether this membership reaches every collection in the org regardless of per-collection + /// assignments — Admins/Owners implicitly, and Custom members holding `edit_any_collection`. + /// This is the successor of the removed `access_all` flag: it backs the `accessAll` field the + /// Bitwarden clients still read, and it intentionally does not gate on status, matching the old + /// column's semantics. Authorization decisions use the status-aware `has_full_access` instead. + pub fn grants_access_to_all_collections(&self) -> bool { + self.atype >= MembershipType::Admin || self.has_edit_any_collection() + } + // The granular custom 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. @@ -916,9 +930,8 @@ impl Membership { self.has_create_new_collections() && self.has_edit_any_collection() && self.has_delete_any_collection() } - /// Match Vaultwarden's existing collection-creation policy while keeping the new Custom - /// permission independent from edit/delete. Legacy Manager memberships retain their former - /// access_all-based behavior. + /// Match Vaultwarden's existing collection-creation policy while keeping the Custom + /// permission independent from edit/delete. pub fn can_create_new_collections(&self) -> bool { if !self.has_status(MembershipStatus::Confirmed) { return false; @@ -926,7 +939,6 @@ impl Membership { match MembershipType::from_i32(self.atype) { Some(MembershipType::Owner | MembershipType::Admin) => true, - Some(MembershipType::Manager) => self.access_all, Some(MembershipType::Custom) => self.create_new_collections, Some(MembershipType::User) | None => false, } @@ -935,7 +947,6 @@ impl Membership { pub fn limit_collection_creation(&self) -> bool { match MembershipType::from_i32(self.atype) { Some(MembershipType::Owner | MembershipType::Admin) => false, - Some(MembershipType::Manager) => !self.access_all, Some(MembershipType::Custom) => !self.create_new_collections, Some(MembershipType::User) | None => true, } @@ -1061,7 +1072,7 @@ impl Membership { .await } - // Get all users which are either owner or admin, or a manager/custom member which can manage/access all + // Get all users which are either owner or admin, or a Custom member which can access all collections pub async fn find_confirmed_and_manage_all_by_org(org_uuid: &OrganizationId, conn: &DbConn) -> Vec { conn.run(move |conn| { users_organizations::table @@ -1071,8 +1082,8 @@ impl Membership { users_organizations::atype .eq_any(vec![MembershipType::Owner as i32, MembershipType::Admin as i32]) .or(users_organizations::atype - .eq_any(vec![MembershipType::Manager as i32, MembershipType::Custom as i32]) - .and(users_organizations::access_all.eq(true))), + .eq(MembershipType::Custom as i32) + .and(users_organizations::edit_any_collection.eq(true))), ) .load::(conn) .unwrap_or_default() @@ -1196,10 +1207,12 @@ impl Membership { .eq(users_collections::collection_uuid) .and(ciphers_collections::cipher_uuid.eq(&cipher_uuid))), ) - .filter(users_organizations::access_all.eq(true).or( - // AccessAll.. - ciphers_collections::cipher_uuid.eq(&cipher_uuid), // ..or access to collection with cipher - )) + .filter( + users_organizations::edit_any_collection + .eq(true) // Custom "Edit any collection" (successor of access_all) + .or(users_organizations::atype.le(MembershipType::Admin as i32)) // or org admin/owner + .or(ciphers_collections::cipher_uuid.eq(&cipher_uuid)), // ..or access to collection with cipher + ) .select(users_organizations::all_columns) .distinct() .load::(conn) @@ -1272,10 +1285,12 @@ impl Membership { users_organizations::table .filter(users_organizations::org_uuid.eq(org_uuid)) .left_join(users_collections::table.on(users_collections::user_uuid.eq(users_organizations::user_uuid))) - .filter(users_organizations::access_all.eq(true).or( - // AccessAll.. - users_collections::collection_uuid.eq(&collection_uuid), // ..or access to collection with cipher - )) + .filter( + users_organizations::edit_any_collection + .eq(true) // Custom "Edit any collection" (successor of access_all) + .or(users_organizations::atype.le(MembershipType::Admin as i32)) // or org admin/owner + .or(users_collections::collection_uuid.eq(&collection_uuid)), // ..or access to collection + ) .select(users_organizations::all_columns) .load::(conn) .expect("Error loading user organizations") @@ -1411,23 +1426,17 @@ mod tests { fn membership_type_order_preserves_access_rank_and_ord_contract() { assert!(MembershipType::Owner > MembershipType::Admin); assert!(MembershipType::Admin > MembershipType::Custom); - assert!(MembershipType::Custom > MembershipType::Manager); - assert!(MembershipType::Manager > MembershipType::User); + assert!(MembershipType::Custom > MembershipType::User); assert!(MembershipType::Custom == MembershipType::from_str("4").unwrap()); + // The legacy Manager wire value (3) is accepted and folded onto Custom. + assert!(MembershipType::Custom == MembershipType::from_str("3").unwrap()); // Permission comparisons continue to treat Custom as manager-level and below Admin. - assert!(MembershipType::Custom >= MembershipType::Manager); let custom = MembershipType::Custom as i32; - assert!(custom >= MembershipType::Manager); + assert!(custom >= MembershipType::Custom); assert!(custom < MembershipType::Admin); - let types = [ - MembershipType::Owner, - MembershipType::Admin, - MembershipType::User, - MembershipType::Manager, - MembershipType::Custom, - ]; + let types = [MembershipType::Owner, MembershipType::Admin, MembershipType::User, MembershipType::Custom]; for lhs in types { for rhs in types { assert_eq!(lhs.cmp(&rhs) == Ordering::Equal, lhs == rhs); @@ -1473,23 +1482,20 @@ mod tests { fn edit_any_collection_does_not_imply_create_or_delete() { let mut custom = membership(MembershipType::Custom); custom.edit_any_collection = true; - // The persisted access_all mirror is intentionally tested too: client-facing create and - // delete decisions must still use their dedicated permissions. - custom.access_all = true; + // Edit any collection grants full (read/edit) access to every collection, but client-facing + // create and delete decisions must still use their own dedicated permissions. assert!(custom.has_full_access()); + assert!(custom.grants_access_to_all_collections()); assert!(!custom.can_create_new_collections()); assert!(custom.limit_collection_creation()); assert!(!custom.can_delete_any_collection()); - let mut manager = membership(MembershipType::Manager); - manager.access_all = true; - assert!(manager.can_create_new_collections()); - let admin = membership(MembershipType::Admin); assert!(admin.can_create_new_collections()); assert!(!admin.limit_collection_creation()); assert!(admin.can_delete_any_collection()); + assert!(admin.grants_access_to_all_collections()); } #[test] diff --git a/src/db/schema.rs b/src/db/schema.rs index e8840acd..8e53e099 100644 --- a/src/db/schema.rs +++ b/src/db/schema.rs @@ -236,7 +236,6 @@ table! { user_uuid -> Text, org_uuid -> Text, invited_by_email -> Nullable, - access_all -> Bool, akey -> Text, status -> Integer, atype -> Integer,