From 5558b418015c16895fe7f272263602087228b22f Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Thu, 23 Jul 2026 22:15:32 +0200 Subject: [PATCH 1/9] Remove membership access_all flag and fold Manager role into Custom The per-membership `access_all` flag was Vaultwarden's pre-permissions patch for "this member reaches 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`. Every authorization query that read `users_organizations.access_all` now reads `edit_any_collection = true OR atype <= Admin` instead, which is exactly the set the flag ever identified, so admin/owner and edit-any access is preserved. The column is dropped via a new migration (down-migration restores it from the role/permission model). `groups.access_all` is a separate, still-supported group feature and is untouched. The legacy Manager role (wire value 3) is folded into Custom: the `MembershipType::Manager` variant is removed, all `>= Manager` rank checks become `>= Custom` (identical authorization rank), the `LegacyManager` collection-access path collapses into the Custom rules, and an incoming wire `type=3` is mapped onto Custom for backward compatibility. Custom stays `type=4` because that is the only role modern Bitwarden clients render with custom permissions. Existing type-3 members were already converted to Custom by the earlier migration. Server-only change; verified against the unmodified web-vault. Unit tests, clippy and rustfmt pass; migration up/down verified against SQLite. --- .../down.sql | 6 + .../up.sql | 5 + .../down.sql | 6 + .../up.sql | 5 + .../down.sql | 6 + .../up.sql | 5 + src/api/admin.rs | 89 ++++------- src/api/core/organizations.rs | 147 ++++++++---------- src/api/core/public.rs | 1 - src/auth.rs | 46 ++---- src/db/models/cipher.rs | 36 +++-- src/db/models/collection.rs | 58 ++++--- src/db/models/organization.rs | 118 +++++++------- src/db/schema.rs | 1 - 14 files changed, 260 insertions(+), 269 deletions(-) create mode 100644 migrations/mysql/2026-07-24-120000_drop_membership_access_all/down.sql create mode 100644 migrations/mysql/2026-07-24-120000_drop_membership_access_all/up.sql create mode 100644 migrations/postgresql/2026-07-24-120000_drop_membership_access_all/down.sql create mode 100644 migrations/postgresql/2026-07-24-120000_drop_membership_access_all/up.sql create mode 100644 migrations/sqlite/2026-07-24-120000_drop_membership_access_all/down.sql create mode 100644 migrations/sqlite/2026-07-24-120000_drop_membership_access_all/up.sql 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, From bf56c9b169ae2fd2ca7e1f732b84d00644377e7e Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Thu, 23 Jul 2026 23:00:15 +0200 Subject: [PATCH 2/9] Add accessEventLogs, accessImportExport and accessReports Custom permissions Implements the three remaining Bitwarden Custom-role permissions on top of the existing set. Each is an independent, persisted flag on the membership, gated on the Custom role in code (stale flags on other roles grant nothing); Owners/Admins hold every permission implicitly. They are parsed from and emitted in the `permissions` object (replacing the previously hard-coded `false`) so the unmodified web-vault shows and round-trips them. Server-side enforcement: - accessEventLogs: the organization event-log endpoints (`GET .../events` and `GET .../users//events`) now use a new `AccessEventLogsHeaders` guard (Admin/Owner or the permission) instead of `AdminHeaders`. - accessImportExport: `GET .../export` uses a new `AccessImportExportHeaders` guard, and `post_org_import` gains an explicit permission check. NOTE: this tightens org import, which previously accepted any confirmed member (with per-collection gating). It now requires Admin/Owner or the permission, matching Bitwarden and the web-vault, which only offers org import to permitted members. Flagged here for maintainer review. - accessReports has no server endpoint in Vaultwarden (reports are computed client-side from vault data the member already has), so it is stored and reported in the permissions object and enforced by the client UI, matching Bitwarden's own model. No server route gates it. New migration adds the three columns (down drops them). Unit tests cover independence, type-gating, parsing and change-detection; a black-box probe over HTTP confirms the event-log/export/import gating and the permission round-trip (16/16), and the existing custom-role suite still passes (30/30). --- .../down.sql | 3 + .../up.sql | 5 ++ .../down.sql | 3 + .../up.sql | 5 ++ .../down.sql | 3 + .../up.sql | 5 ++ src/api/core/events.rs | 11 +++- src/api/core/organizations.rs | 46 ++++++++++++-- src/auth.rs | 21 +++++++ src/db/models/organization.rs | 62 +++++++++++++++++-- src/db/schema.rs | 3 + 11 files changed, 154 insertions(+), 13 deletions(-) create mode 100644 migrations/mysql/2026-07-24-130000_add_custom_access_permissions/down.sql create mode 100644 migrations/mysql/2026-07-24-130000_add_custom_access_permissions/up.sql create mode 100644 migrations/postgresql/2026-07-24-130000_add_custom_access_permissions/down.sql create mode 100644 migrations/postgresql/2026-07-24-130000_add_custom_access_permissions/up.sql create mode 100644 migrations/sqlite/2026-07-24-130000_add_custom_access_permissions/down.sql create mode 100644 migrations/sqlite/2026-07-24-130000_add_custom_access_permissions/up.sql diff --git a/migrations/mysql/2026-07-24-130000_add_custom_access_permissions/down.sql b/migrations/mysql/2026-07-24-130000_add_custom_access_permissions/down.sql new file mode 100644 index 00000000..f276ea5b --- /dev/null +++ b/migrations/mysql/2026-07-24-130000_add_custom_access_permissions/down.sql @@ -0,0 +1,3 @@ +ALTER TABLE users_organizations DROP COLUMN access_event_logs; +ALTER TABLE users_organizations DROP COLUMN access_import_export; +ALTER TABLE users_organizations DROP COLUMN access_reports; diff --git a/migrations/mysql/2026-07-24-130000_add_custom_access_permissions/up.sql b/migrations/mysql/2026-07-24-130000_add_custom_access_permissions/up.sql new file mode 100644 index 00000000..9d9c31ff --- /dev/null +++ b/migrations/mysql/2026-07-24-130000_add_custom_access_permissions/up.sql @@ -0,0 +1,5 @@ +-- Three additional Bitwarden Custom-role permissions. They are only meaningful for Custom members +-- (gated on the role in code); Owners/Admins hold every permission implicitly. +ALTER TABLE users_organizations ADD COLUMN access_event_logs BOOLEAN NOT NULL DEFAULT FALSE; +ALTER TABLE users_organizations ADD COLUMN access_import_export BOOLEAN NOT NULL DEFAULT FALSE; +ALTER TABLE users_organizations ADD COLUMN access_reports BOOLEAN NOT NULL DEFAULT FALSE; diff --git a/migrations/postgresql/2026-07-24-130000_add_custom_access_permissions/down.sql b/migrations/postgresql/2026-07-24-130000_add_custom_access_permissions/down.sql new file mode 100644 index 00000000..f276ea5b --- /dev/null +++ b/migrations/postgresql/2026-07-24-130000_add_custom_access_permissions/down.sql @@ -0,0 +1,3 @@ +ALTER TABLE users_organizations DROP COLUMN access_event_logs; +ALTER TABLE users_organizations DROP COLUMN access_import_export; +ALTER TABLE users_organizations DROP COLUMN access_reports; diff --git a/migrations/postgresql/2026-07-24-130000_add_custom_access_permissions/up.sql b/migrations/postgresql/2026-07-24-130000_add_custom_access_permissions/up.sql new file mode 100644 index 00000000..9d9c31ff --- /dev/null +++ b/migrations/postgresql/2026-07-24-130000_add_custom_access_permissions/up.sql @@ -0,0 +1,5 @@ +-- Three additional Bitwarden Custom-role permissions. They are only meaningful for Custom members +-- (gated on the role in code); Owners/Admins hold every permission implicitly. +ALTER TABLE users_organizations ADD COLUMN access_event_logs BOOLEAN NOT NULL DEFAULT FALSE; +ALTER TABLE users_organizations ADD COLUMN access_import_export BOOLEAN NOT NULL DEFAULT FALSE; +ALTER TABLE users_organizations ADD COLUMN access_reports BOOLEAN NOT NULL DEFAULT FALSE; diff --git a/migrations/sqlite/2026-07-24-130000_add_custom_access_permissions/down.sql b/migrations/sqlite/2026-07-24-130000_add_custom_access_permissions/down.sql new file mode 100644 index 00000000..f276ea5b --- /dev/null +++ b/migrations/sqlite/2026-07-24-130000_add_custom_access_permissions/down.sql @@ -0,0 +1,3 @@ +ALTER TABLE users_organizations DROP COLUMN access_event_logs; +ALTER TABLE users_organizations DROP COLUMN access_import_export; +ALTER TABLE users_organizations DROP COLUMN access_reports; diff --git a/migrations/sqlite/2026-07-24-130000_add_custom_access_permissions/up.sql b/migrations/sqlite/2026-07-24-130000_add_custom_access_permissions/up.sql new file mode 100644 index 00000000..9d9c31ff --- /dev/null +++ b/migrations/sqlite/2026-07-24-130000_add_custom_access_permissions/up.sql @@ -0,0 +1,5 @@ +-- Three additional Bitwarden Custom-role permissions. They are only meaningful for Custom members +-- (gated on the role in code); Owners/Admins hold every permission implicitly. +ALTER TABLE users_organizations ADD COLUMN access_event_logs BOOLEAN NOT NULL DEFAULT FALSE; +ALTER TABLE users_organizations ADD COLUMN access_import_export BOOLEAN NOT NULL DEFAULT FALSE; +ALTER TABLE users_organizations ADD COLUMN access_reports BOOLEAN NOT NULL DEFAULT FALSE; diff --git a/src/api/core/events.rs b/src/api/core/events.rs index 698a890f..3fea281e 100644 --- a/src/api/core/events.rs +++ b/src/api/core/events.rs @@ -7,7 +7,7 @@ use serde_json::Value; use crate::{ CONFIG, api::{EmptyResult, JsonResult}, - auth::{AdminHeaders, Headers}, + auth::{AccessEventLogsHeaders, Headers}, db::{ DbConn, DbPool, models::{Cipher, CipherId, Event, Membership, MembershipId, OrganizationId, UserId}, @@ -31,7 +31,12 @@ struct EventRange { // Upstream: https://github.com/bitwarden/server/blob/9ebe16587175b1c0e9208f84397bb75d0d595510/src/Api/AdminConsole/Controllers/EventsController.cs#L87 #[get("/organizations//events?")] -async fn get_org_events(org_id: OrganizationId, data: EventRange, headers: AdminHeaders, conn: DbConn) -> JsonResult { +async fn get_org_events( + org_id: OrganizationId, + data: EventRange, + headers: AccessEventLogsHeaders, + conn: DbConn, +) -> JsonResult { if org_id != headers.org_id { err!("Organization not found", "Organization id's do not match"); } @@ -93,7 +98,7 @@ async fn get_user_events( org_id: OrganizationId, member_id: MembershipId, data: EventRange, - headers: AdminHeaders, + headers: AccessEventLogsHeaders, conn: DbConn, ) -> JsonResult { if org_id != headers.org_id { diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index edc88881..001b546f 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -12,9 +12,9 @@ use crate::{ core::{CipherSyncData, CipherSyncType, accept_org_invite, log_event, two_factor}, }, auth::{ - AdminHeaders, CollectionDeleteHeaders, CollectionReadHeaders, Headers, ManageGroupsHeaders, - ManagePoliciesHeaders, ManageUsersHeaders, ManageUsersOrGroupsHeaders, ManagerHeaders, ManagerHeadersLoose, - OrgMemberHeaders, OwnerHeaders, decode_invite, + AccessImportExportHeaders, AdminHeaders, CollectionDeleteHeaders, CollectionReadHeaders, Headers, + ManageGroupsHeaders, ManagePoliciesHeaders, ManageUsersHeaders, ManageUsersOrGroupsHeaders, ManagerHeaders, + ManagerHeadersLoose, OrgMemberHeaders, OwnerHeaders, decode_invite, }, db::{ DbConn, @@ -1192,6 +1192,9 @@ struct CustomRolePermissions { create_new_collections: bool, edit_any_collection: bool, delete_any_collection: bool, + access_event_logs: bool, + access_import_export: bool, + access_reports: bool, } impl CustomRolePermissions { @@ -1208,6 +1211,9 @@ impl CustomRolePermissions { create_new_collections: enabled("createNewCollections"), edit_any_collection: enabled("editAnyCollection"), delete_any_collection: enabled("deleteAnyCollection"), + access_event_logs: enabled("accessEventLogs"), + access_import_export: enabled("accessImportExport"), + access_reports: enabled("accessReports"), } } @@ -1225,6 +1231,9 @@ impl CustomRolePermissions { || self.create_new_collections != membership.create_new_collections || self.edit_any_collection != membership.edit_any_collection || self.delete_any_collection != membership.delete_any_collection + || self.access_event_logs != membership.access_event_logs + || self.access_import_export != membership.access_import_export + || self.access_reports != membership.access_reports } fn apply_to(self, membership: &mut Membership) { @@ -1234,6 +1243,9 @@ impl CustomRolePermissions { membership.create_new_collections = self.create_new_collections; membership.edit_any_collection = self.edit_any_collection; membership.delete_any_collection = self.delete_any_collection; + membership.access_event_logs = self.access_event_logs; + membership.access_import_export = self.access_import_export; + membership.access_reports = self.access_reports; } } @@ -2169,6 +2181,20 @@ async fn post_org_import( if org_id != headers.membership.org_uuid { err!("Organization not found", "Organization id's do not match"); } + + // accessImportExport: importing into the organization requires the permission (or Admin/Owner), + // mirroring the export endpoint and the Bitwarden permission model. The web-vault only offers org + // import to members holding this permission; enforcing it server-side keeps the two consistent. + // NOTE: this tightens the previous member-level behaviour (any confirmed member could import into + // collections they could write) — see the branch notes. + if !(headers.membership.has_status(MembershipStatus::Confirmed) + && (headers.membership.atype >= MembershipType::Admin || headers.membership.has_access_import_export())) + { + err!( + "You need the 'Access Import/Export' permission, or to be an Admin or Owner, to import into this organization" + ) + } + let data: ImportData = data.into_inner(); // Validate the import before continuing @@ -3806,7 +3832,7 @@ async fn put_reset_password_enrollment( // Vaultwarden does not yet support exporting only managed collections! // https://github.com/bitwarden/server/blob/9ebe16587175b1c0e9208f84397bb75d0d595510/src/Api/Tools/Controllers/OrganizationExportController.cs#L52 #[get("/organizations//export")] -async fn get_org_export(org_id: OrganizationId, headers: AdminHeaders, conn: DbConn) -> JsonResult { +async fn get_org_export(org_id: OrganizationId, headers: AccessImportExportHeaders, conn: DbConn) -> JsonResult { if org_id != headers.org_id { err!("Organization not found", "Organization id's do not match"); } @@ -4023,6 +4049,9 @@ mod tests { ("createNewCollections".to_owned(), json!(true)), ("editAnyCollection".to_owned(), json!(true)), ("deleteAnyCollection".to_owned(), json!(true)), + ("accessEventLogs".to_owned(), json!(true)), + ("accessImportExport".to_owned(), json!(true)), + ("accessReports".to_owned(), json!(true)), ]); let custom = CustomRolePermissions::from_request(MembershipType::Custom, &permissions); @@ -4032,6 +4061,9 @@ mod tests { assert!(custom.create_new_collections); assert!(custom.edit_any_collection); assert!(custom.delete_any_collection); + assert!(custom.access_event_logs); + assert!(custom.access_import_export); + assert!(custom.access_reports); let user = CustomRolePermissions::from_request(MembershipType::User, &permissions); assert_eq!(user, CustomRolePermissions::default()); @@ -4052,6 +4084,9 @@ mod tests { create_new_collections: true, edit_any_collection: true, delete_any_collection: true, + access_event_logs: true, + access_import_export: true, + access_reports: true, ..CustomRolePermissions::default() }; @@ -4061,5 +4096,8 @@ mod tests { assert!(membership.create_new_collections); assert!(membership.edit_any_collection); assert!(membership.delete_any_collection); + assert!(membership.access_event_logs); + assert!(membership.access_import_export); + assert!(membership.access_reports); } } diff --git a/src/auth.rs b/src/auth.rs index 4716b6e2..9e89a16b 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -755,6 +755,14 @@ impl OrgHeaders { || self.membership.has_manage_users() || self.membership.has_manage_groups()) } + fn can_access_event_logs(&self) -> bool { + self.is_confirmed() + && (self.membership_type >= MembershipType::Admin || self.membership.has_access_event_logs()) + } + fn can_access_import_export(&self) -> bool { + self.is_confirmed() + && (self.membership_type >= MembershipType::Admin || self.membership.has_access_import_export()) + } } // org_id is usually the second path param ("/organizations/"), @@ -839,6 +847,9 @@ impl<'r> FromRequest<'r> for OrgHeaders { } pub struct AdminHeaders { + // Kept for parity with the other org header guards (and possible future use); the org export + // endpoint that used to read this now goes through `AccessImportExportHeaders` instead. + #[allow(dead_code)] pub host: String, pub device: Device, pub user: User, @@ -937,6 +948,16 @@ generate_manage_headers!( can_manage_users_or_groups, "You need the 'Manage Users' or 'Manage Groups' permission, or to be an Admin or Owner, to call this endpoint" ); +generate_manage_headers!( + AccessEventLogsHeaders, + can_access_event_logs, + "You need the 'Access Event Logs' permission, or to be an Admin or Owner, to call this endpoint" +); +generate_manage_headers!( + AccessImportExportHeaders, + can_access_import_export, + "You need the 'Access Import/Export' permission, or to be an Admin or Owner, to call this endpoint" +); // col_id is usually the fourth path param ("/organizations//collections/"), // but there could be cases where it is a query value. diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index 81ceb15c..0ddfcc80 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -64,6 +64,9 @@ pub struct Membership { pub create_new_collections: bool, pub edit_any_collection: bool, pub delete_any_collection: bool, + pub access_event_logs: bool, + pub access_import_export: bool, + pub access_reports: bool, } #[derive(Identifiable, Queryable, Insertable, AsChangeset)] @@ -283,6 +286,9 @@ impl Membership { create_new_collections: false, edit_any_collection: false, delete_any_collection: false, + access_event_logs: false, + access_import_export: false, + access_reports: false, } } @@ -453,9 +459,9 @@ impl Membership { let membership_type = self.atype; let permissions = json!({ - "accessEventLogs": false, - "accessImportExport": false, - "accessReports": false, + "accessEventLogs": membership_type == MembershipType::Custom as i32 && self.access_event_logs, + "accessImportExport": membership_type == MembershipType::Custom as i32 && self.access_import_export, + "accessReports": membership_type == MembershipType::Custom as i32 && self.access_reports, "createNewCollections": membership_type == MembershipType::Custom as i32 && self.create_new_collections, "editAnyCollection": membership_type == MembershipType::Custom as i32 && self.edit_any_collection, "deleteAnyCollection": membership_type == MembershipType::Custom as i32 && self.delete_any_collection, @@ -619,9 +625,9 @@ impl Membership { // all-false defaults and the role itself supplies any elevated capabilities. let permissions = if membership_type == MembershipType::Custom as i32 { json!({ - "accessEventLogs": false, - "accessImportExport": false, - "accessReports": false, + "accessEventLogs": self.access_event_logs, + "accessImportExport": self.access_import_export, + "accessReports": self.access_reports, "createNewCollections": self.create_new_collections, "editAnyCollection": self.edit_any_collection, "deleteAnyCollection": self.delete_any_collection, @@ -847,6 +853,18 @@ impl Membership { self.has_type(MembershipType::Custom) && self.delete_any_collection } + pub fn has_access_event_logs(&self) -> bool { + self.has_type(MembershipType::Custom) && self.access_event_logs + } + + pub fn has_access_import_export(&self) -> bool { + self.has_type(MembershipType::Custom) && self.access_import_export + } + + pub fn has_access_reports(&self) -> bool { + self.has_type(MembershipType::Custom) && self.access_reports + } + /// Check for an explicit per-collection Manage grant without treating any `access_all` value /// as such a grant. Custom-role collection guards use this instead of the legacy broad helper, /// because membership/group `access_all` must not manufacture a per-collection Manage grant. @@ -953,6 +971,9 @@ impl Membership { self.create_new_collections = false; self.edit_any_collection = false; self.delete_any_collection = false; + self.access_event_logs = false; + self.access_import_export = false; + self.access_reports = false; } pub async fn find_by_uuid(uuid: &MembershipId, conn: &DbConn) -> Option { @@ -1514,6 +1535,9 @@ mod tests { member.create_new_collections = true; member.edit_any_collection = true; member.delete_any_collection = true; + member.access_event_logs = true; + member.access_import_export = true; + member.access_reports = true; member.clear_custom_permissions(); @@ -1523,5 +1547,31 @@ mod tests { assert!(!member.create_new_collections); assert!(!member.edit_any_collection); assert!(!member.delete_any_collection); + assert!(!member.access_event_logs); + assert!(!member.access_import_export); + assert!(!member.access_reports); + } + + #[test] + fn custom_access_permissions_are_independent_and_type_gated() { + let mut member = membership(MembershipType::Custom); + member.access_event_logs = true; + assert!(member.has_access_event_logs()); + assert!(!member.has_access_import_export()); + assert!(!member.has_access_reports()); + + member.access_import_export = true; + member.access_reports = true; + assert!(member.has_access_import_export()); + assert!(member.has_access_reports()); + // None of them imply collection or management capabilities. + assert!(!member.has_full_access()); + assert!(!member.has_manage_users()); + + // Stale flags on a non-Custom role grant nothing. + member.atype = MembershipType::User as i32; + assert!(!member.has_access_event_logs()); + assert!(!member.has_access_import_export()); + assert!(!member.has_access_reports()); } } diff --git a/src/db/schema.rs b/src/db/schema.rs index e8840acd..aee26991 100644 --- a/src/db/schema.rs +++ b/src/db/schema.rs @@ -248,6 +248,9 @@ table! { create_new_collections -> Bool, edit_any_collection -> Bool, delete_any_collection -> Bool, + access_event_logs -> Bool, + access_import_export -> Bool, + access_reports -> Bool, } } From aa58ef576c6c175f2502103326facfa13b5d922f Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Sat, 25 Jul 2026 20:25:12 +0200 Subject: [PATCH 3/9] Scope the organization export to what the caller may actually read A security review of this branch (finding F1) pointed out that the 'Access Import/Export' permission, which this branch added and which opens GET /organizations//export via AccessImportExportHeaders, decided *whether* a member may export but not *what* they get: the handler always dumped Cipher::find_by_org(), and CipherSyncType:: Organization deliberately skips the per-cipher access restrictions. A confirmed Custom member holding only accessImportExport - assigned to no collection at all - therefore received every cipher of the organization, including collections they were explicitly excluded from. Every confirmed member holds the organization key, so the exported blobs are decryptable by the caller. The export is now built from the caller's own assignments unless they already reach every collection anyway (Admin/Owner, or a Custom member with 'Edit any collection'), which is what Bitwarden's export controller does. Group-based access is covered: both scoped queries honour group assignments and group access_all. * new helper may_export_entire_organization() names the decision and keeps it unit-testable * get_org_details_impl() split into ciphers_to_org_json() so the export can serialize an already-authorized cipher list; the serializer's doc comment states that requirement * new regression test access_import_export_alone_does_not_widen_the_export Verified: cargo check, cargo clippy --features sqlite (clean), cargo test --features sqlite (48 passed), cargo fmt --all -- --check. --- src/api/core/organizations.rs | 76 ++++++++++++++++++++++++++++++++--- 1 file changed, 70 insertions(+), 6 deletions(-) diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index 6481d003..50754dd8 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -1077,7 +1077,18 @@ async fn get_org_details_impl( user_id: &UserId, conn: &DbConn, ) -> Result { - let ciphers = Cipher::find_by_org(org_id, conn).await; + ciphers_to_org_json(Cipher::find_by_org(org_id, conn).await, host, user_id, conn).await +} + +// Serialize an already-authorized set of organization ciphers. The caller decides which ciphers go +// in: `CipherSyncType::Organization` skips the per-cipher access restrictions, so this must never be +// handed a cipher the user is not allowed to see. +async fn ciphers_to_org_json( + ciphers: Vec, + host: &str, + user_id: &UserId, + conn: &DbConn, +) -> Result { let cipher_sync_data = CipherSyncData::new(user_id, CipherSyncType::Organization, conn).await; let mut ciphers_json = Vec::with_capacity(ciphers.len()); @@ -3218,6 +3229,17 @@ async fn caller_may_grant_collection_manage(caller: &Membership, col_id: &Collec } } +/// Whether `caller` may export the *entire* organization instead of only their own assignments. +/// +/// Security (audit F1): the `AccessImportExportHeaders` guard on `get_org_export` decides whether a +/// member may export at all; it must not decide *what* they get. Only members who already reach +/// every collection — Admins/Owners, and Custom members holding `edit_any_collection` — may receive +/// the full organization dump. For anyone else the export is built from their own assigned +/// collections/ciphers, so 'Access Import/Export' can never turn into a full vault read. +fn may_export_entire_organization(caller: &Membership) -> bool { + caller.has_full_access() +} + /// Pure, collection-independent part of `caller_may_grant_collection_manage`. /// /// `Some(true)` -> the caller may grant `manage` on *any* collection (Admin/Owner, or a Custom @@ -3847,8 +3869,9 @@ async fn put_reset_password_enrollment( // NOTE: It seems clients can't handle uppercase-first keys!! // We need to convert all keys so they have the first character to be a lowercase. // Else the export will be just an empty JSON file. -// We currently only support exports by members of the Admin or Owner status. -// Vaultwarden does not yet support exporting only managed collections! +// Members with full access to the organization (Admin/Owner, or a Custom member with +// 'Edit any collection') export the whole organization; everyone else exports only what they can +// actually reach, like Bitwarden's export controller does. // https://github.com/bitwarden/server/blob/9ebe16587175b1c0e9208f84397bb75d0d595510/src/Api/Tools/Controllers/OrganizationExportController.cs#L52 #[get("/organizations//export")] async fn get_org_export(org_id: OrganizationId, headers: AccessImportExportHeaders, conn: DbConn) -> JsonResult { @@ -3856,9 +3879,24 @@ async fn get_org_export(org_id: OrganizationId, headers: AccessImportExportHeade err!("Organization not found", "Organization id's do not match"); } + // Security (audit F1): 'Access Import/Export' decides *whether* a member may export, it must not + // widen *what* they may read. Without this scoping a Custom member holding only this permission + // — assigned to no collection at all — would receive every cipher in the organization, because + // the organization sync type deliberately skips the per-cipher access restrictions. + let (collections, ciphers) = if may_export_entire_organization(&headers.membership) { + (Collection::find_by_organization(&org_id, &conn).await, Cipher::find_by_org(&org_id, &conn).await) + } else { + ( + Collection::find_by_organization_and_user_uuid(&org_id, &headers.user.uuid, &conn).await, + filter_ciphers_for_organization(Cipher::find_by_user_visible(&headers.user.uuid, &conn).await, &org_id), + ) + }; + + let collections_json: Value = collections.iter().map(Collection::to_json).collect(); + Ok(Json(json!({ - "collections": convert_json_key_lcase_first(get_org_collections_impl(&org_id, &conn).await), - "ciphers": convert_json_key_lcase_first(get_org_details_impl(&org_id, &headers.host, &headers.user.uuid, &conn).await?), + "collections": convert_json_key_lcase_first(collections_json), + "ciphers": convert_json_key_lcase_first(ciphers_to_org_json(ciphers, &headers.host, &headers.user.uuid, &conn).await?), }))) } @@ -3927,7 +3965,7 @@ mod tests { use super::{ CustomRolePermissions, caller_manage_grant_role_check, filter_ciphers_for_organization, - may_change_group_membership, may_change_member_type, + may_change_group_membership, may_change_member_type, may_export_entire_organization, }; use crate::db::models::{Cipher, Membership, MembershipStatus, MembershipType, OrganizationId}; @@ -3971,6 +4009,32 @@ mod tests { assert_eq!(caller_manage_grant_role_check(&unconfirmed), Some(false)); } + #[test] + fn access_import_export_alone_does_not_widen_the_export() { + // REGRESSION (audit F1): 'Access Import/Export' opens the export endpoint, but a Custom + // member holding only that permission reaches no collection of their own, so the export + // must be built from their assignments — never from the whole organization. + let mut import_export_only = confirmed_member(MembershipType::Custom); + import_export_only.access_import_export = true; + assert!(!may_export_entire_organization(&import_export_only)); + + // Custom members who already reach every collection keep the full dump. + let mut edit_any = confirmed_member(MembershipType::Custom); + edit_any.edit_any_collection = true; + edit_any.access_import_export = true; + assert!(may_export_entire_organization(&edit_any)); + + // Admins and Owners are unaffected. + assert!(may_export_entire_organization(&confirmed_member(MembershipType::Admin))); + assert!(may_export_entire_organization(&confirmed_member(MembershipType::Owner))); + + // An unconfirmed membership never qualifies, whatever its flags say. + let mut unconfirmed = confirmed_member(MembershipType::Custom); + unconfirmed.edit_any_collection = true; + unconfirmed.status = MembershipStatus::Accepted as i32; + assert!(!may_export_entire_organization(&unconfirmed)); + } + #[test] fn assigned_cipher_response_is_scoped_to_requested_organization() { let requested_org: OrganizationId = "requested-org".to_owned().into(); From fc96fbe8d38ffc4353b22afcc4c1526bf10414f3 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Sun, 26 Jul 2026 00:56:40 +0200 Subject: [PATCH 4/9] Harden custom-role authorization and migrations --- docs/custom-role-migration-recovery.md | 200 +++++ .../down.sql | 1 + .../up.sql | 13 + .../down.sql | 3 + .../up.sql | 67 ++ .../down.sql | 7 + .../up.sql | 3 + .../down.sql | 1 + .../up.sql | 13 + .../down.sql | 3 + .../up.sql | 65 ++ .../down.sql | 7 + .../up.sql | 3 + .../down.sql | 1 + .../up.sql | 12 + .../down.sql | 3 + .../up.sql | 65 ++ .../down.sql | 7 + .../up.sql | 3 + src/api/admin.rs | 27 +- src/api/core/events.rs | 328 +++++++- src/api/core/organizations.rs | 247 ++++-- src/db/mod.rs | 768 +++++++++++++++++- src/db/models/collection.rs | 135 +-- src/db/models/organization.rs | 102 ++- src/static/scripts/admin_users.js | 16 +- src/static/templates/admin/users.hbs | 5 +- .../templates/scss/vaultwarden.scss.hbs | 6 +- src/util.rs | 10 +- 29 files changed, 1864 insertions(+), 257 deletions(-) create mode 100644 docs/custom-role-migration-recovery.md create mode 100644 migrations/mysql/2026-07-15-120000_mark_pending_custom_collection_migration/down.sql create mode 100644 migrations/mysql/2026-07-15-120000_mark_pending_custom_collection_migration/up.sql create mode 100644 migrations/mysql/2026-07-23-120000_reconcile_legacy_custom_roles/down.sql create mode 100644 migrations/mysql/2026-07-23-120000_reconcile_legacy_custom_roles/up.sql create mode 100644 migrations/mysql/2026-07-24-140000_guard_custom_role_downgrade/down.sql create mode 100644 migrations/mysql/2026-07-24-140000_guard_custom_role_downgrade/up.sql create mode 100644 migrations/postgresql/2026-07-15-120000_mark_pending_custom_collection_migration/down.sql create mode 100644 migrations/postgresql/2026-07-15-120000_mark_pending_custom_collection_migration/up.sql create mode 100644 migrations/postgresql/2026-07-23-120000_reconcile_legacy_custom_roles/down.sql create mode 100644 migrations/postgresql/2026-07-23-120000_reconcile_legacy_custom_roles/up.sql create mode 100644 migrations/postgresql/2026-07-24-140000_guard_custom_role_downgrade/down.sql create mode 100644 migrations/postgresql/2026-07-24-140000_guard_custom_role_downgrade/up.sql create mode 100644 migrations/sqlite/2026-07-15-120000_mark_pending_custom_collection_migration/down.sql create mode 100644 migrations/sqlite/2026-07-15-120000_mark_pending_custom_collection_migration/up.sql create mode 100644 migrations/sqlite/2026-07-23-120000_reconcile_legacy_custom_roles/down.sql create mode 100644 migrations/sqlite/2026-07-23-120000_reconcile_legacy_custom_roles/up.sql create mode 100644 migrations/sqlite/2026-07-24-140000_guard_custom_role_downgrade/down.sql create mode 100644 migrations/sqlite/2026-07-24-140000_guard_custom_role_downgrade/up.sql diff --git a/docs/custom-role-migration-recovery.md b/docs/custom-role-migration-recovery.md new file mode 100644 index 00000000..64fad205 --- /dev/null +++ b/docs/custom-role-migration-recovery.md @@ -0,0 +1,200 @@ +# Custom-role migration recovery + +Vaultwarden deliberately stops the Custom-role migration when the old database state cannot be +translated without either removing access or adding new management authority. A failed preflight +does not authorize Vaultwarden to choose between those outcomes. + +## Before doing anything + +1. Stop every Vaultwarden instance that uses the database. Do not perform this migration during a + rolling deployment. +2. Take and verify a full database backup. +3. Keep the complete startup error. It identifies the state that needs review. +4. Do not add or delete rows in `__diesel_schema_migrations` merely to bypass the preflight. + +The relevant versions are: + +| Version | Purpose | +|---|---| +| `2026-07-15-120000` | Mark that `2026-07-16` is pending in the same migration sequence | +| `2026-07-16-120000` | Add the three collection-permission columns | +| `2026-07-23-120000` | Reconcile legacy Manager/Custom membership permissions | +| `2026-07-24-120000` | Drop membership-level `access_all` | +| `2026-07-24-130000` | Add the three Custom Access permissions | +| `2026-07-24-140000` | Refuse a lossy Custom-role downgrade | + +Diesel stores these directory versions without punctuation in `__diesel_schema_migrations` (for +example, `2026-07-16-120000` is stored as `20260716120000`). +The immutable `2026-06-30-120000` migration is an earlier prerequisite; this table focuses on the +new marker/repair/drop/downgrade safety sequence. + +## Legacy User with membership `access_all` + +Find the affected memberships before the source column is dropped: + +```sql +SELECT uuid, user_uuid, org_uuid, status +FROM users_organizations +WHERE atype = 2 AND access_all = TRUE; +``` + +This state was accepted by older Vaultwarden versions. It has no exact representation in the new +nine-bit Custom-role model: + +- clearing `access_all` keeps the User role but removes organization-wide vault access; +- changing the member to Custom with all three collection permissions preserves broad vault access, + but also grants collection-management capabilities the old User role did not have. + +An organization owner must decide the intended role and permissions for each result. Make that +change on the backed-up pre-drop database and record the decision: + +- to keep the member a normal User, set that membership's `access_all` to false; +- to intentionally promote the member to Custom with Create/Edit/Delete-any authority, set that + membership's `atype` to the legacy Manager value `3` and keep `access_all` true. The repair copies + the bit to all three collection permissions before converting `atype` to `4`. + +Apply either change by exact membership UUID while all Vaultwarden instances are stopped. Do not +bulk-promote these records automatically. + +## Group-derived legacy collection management + +An organization-local `groups.access_all` relationship is safe when the membership has no direct +collection permissions. During a normal upgrade, the older `2026-07-16` migration temporarily +copies that relationship to the exact direct `create/edit/delete = 0/1/1` pattern. The new repair +recognizes the still-present, organization-bound source and deterministically resets the direct +Edit/Delete bits to false **only** when the durable `2026-07-15` marker proves that `2026-07-16` was +pending in the same migration sequence. Vault access remains group-derived, so removing the member +from the group also removes that access. + +The marker survives a process failure between `2026-07-16` and `2026-07-23`, allowing the next +startup to finish the deterministic repair. `2026-07-23` transactionally clears the marker row only +after all guards and data updates succeed. The empty internal bookkeeping table is intentionally +retained so MySQL does not introduce a DDL commit boundary. Do not create, remove, or populate +`__vw_custom_role_same_run_0716` manually. + +The preflight stops only when the three columns already exist and it finds a `0/1/1` pattern. At +that point the values may be either an intentional direct Edit+Delete grant or an older group +backfill whose source group was removed; the database has no provenance bit that can distinguish +them. + +For each stopped `0/1/1` membership, the owner must choose one of these executable outcomes: + +- **Group-derived or obsolete:** set `edit_any_collection` and `delete_any_collection` to false for + that exact membership. Leave the intended group relationship in place if access should remain + group-derived. The next preflight can then proceed. +- **Intentionally direct Edit+Delete:** while every server is stopped, temporarily set + `create_new_collections` to true for that exact membership. The unambiguous `1/1/1` state passes + the repair and is not treated as a group backfill. Run the migration in a maintenance instance + that is not reachable by clients, stop it as soon as all six recovery-sequence versions listed + above are recorded, then set `create_new_collections` back to false before normal service resumes. + This restores the explicitly reviewed direct `0/1/1` state after the repair marker exists. + +The organization boundary used to identify a current group source is: + +```sql +SELECT DISTINCT uo.uuid, uo.org_uuid, g.uuid AS group_uuid +FROM users_organizations AS uo +INNER JOIN groups_users AS gu ON gu.users_organizations_uuid = uo.uuid +INNER JOIN groups AS g ON g.uuid = gu.groups_uuid +WHERE uo.atype IN (3, 4) + AND uo.access_all = FALSE + AND g.organizations_uuid = uo.org_uuid + AND g.access_all = TRUE; +``` + +On MySQL, quote the table as `` `groups` ``. Review direct `0/1/1` records separately: + +```sql +SELECT uuid, user_uuid, org_uuid +FROM users_organizations +WHERE atype IN (3, 4) + AND access_all = FALSE + AND create_new_collections = FALSE + AND edit_any_collection = TRUE + AND delete_any_collection = TRUE; +``` + +Because an explicit Edit+Delete assignment has the same stored values as the historical derived +state, Vaultwarden cannot classify those records automatically. Never use the temporary Create bit +while a server is accepting client traffic. + +## The `access_all` column was already dropped + +If `2026-07-24-120000` is recorded but `2026-07-23-120000` is not, restore a backup from before the +drop and migrate again after resolving the cases above. The old membership bit is no longer present, +so a later migration cannot prove which members had it. + +If no such backup exists, perform a membership-by-membership authorization review using +administrative records before changing roles or flags. Only after the final state has been reviewed +may an operator mark the repair version as resolved. Vaultwarden intentionally provides no automatic +command for this irreversible case. + +## Historical MySQL partial `2026-07-16` migration + +An older branch revision could fail on the unquoted `groups` identifier after MySQL had already +committed all three `ADD COLUMN` statements. The migration version was not recorded, so a normal +retry then failed on duplicate columns. + +Vaultwarden automatically completes this state only when all of the following are true: + +- `2026-07-16-120000` is absent from the ledger; +- all three expected columns exist, are non-null booleans, and default to false; +- `access_all` still exists and `2026-07-24-120000` has not run; +- the stored values are either the untouched false defaults, the values produced by the canonical + membership-`access_all` copy, or exact `0/1/1` values accompanied by both the durable same-run + marker and a current same-organization `groups.access_all` source; and +- neither a legacy User/access-all case nor ambiguous group provenance exists. + +It then reapplies the canonical membership data copy and inserts the ledger row in one transaction. +For the narrowly accepted same-run `0/1/1` crash state, that copy first reconstructs `0/0/0`; the +pending canonical group backfill and `2026-07-23` repair then run normally. A missing group source, +any other partial column set, changed definition, or unexpected value stops startup. Preserve that +database and repair it manually from the verified backup; do not drop columns that may contain +independently changed permissions. + +## Verification after recovery + +After a successful start, verify: + +```sql +SELECT version +FROM __diesel_schema_migrations +WHERE version IN ( + '20260715120000', + '20260716120000', + '20260723120000', + '20260724120000', + '20260724130000', + '20260724140000' +) +ORDER BY version; + +SELECT COUNT(*) AS invalid_manager_types +FROM users_organizations +WHERE atype = 3; +``` + +All six versions must be present and `invalid_manager_types` must be zero. Then test a fresh login, +sync, collection read/edit/delete, and group removal for every membership that was reviewed. + +## Downgrade guard + +The old schema cannot encode nine independent permissions in its single membership `access_all` +bit. Even a state that currently happens to use only `0/0/0` or `1/1/1` could be changed after a +one-step guard was reverted and before a later incremental downgrade. A conditional guard would +therefore create false confidence. + +The newest migration always stops an automatic downgrade with a duplicate-key error in the +`__vw_custom_role_downgrade_guard` temporary table, before any production permission column or +migration-ledger row is removed. This mechanism is enforced by primary keys on SQLite, PostgreSQL, +MySQL 5.7+, and MariaDB; it does not rely on historically ignored MySQL `CHECK` constraints. This is +intentional. + +Rollback requires either: + +- restoring a verified database backup taken before the Custom-role upgrade; or +- an explicit offline transformation plan that exports all permissions, defines the accepted + semantic loss or role changes membership by membership, and is tested against a disposable copy + on the same database backend. + +Do not delete the `20260724140000` ledger row merely to bypass this protection. diff --git a/migrations/mysql/2026-07-15-120000_mark_pending_custom_collection_migration/down.sql b/migrations/mysql/2026-07-15-120000_mark_pending_custom_collection_migration/down.sql new file mode 100644 index 00000000..04346743 --- /dev/null +++ b/migrations/mysql/2026-07-15-120000_mark_pending_custom_collection_migration/down.sql @@ -0,0 +1 @@ +DROP TABLE IF EXISTS __vw_custom_role_same_run_0716; diff --git a/migrations/mysql/2026-07-15-120000_mark_pending_custom_collection_migration/up.sql b/migrations/mysql/2026-07-15-120000_mark_pending_custom_collection_migration/up.sql new file mode 100644 index 00000000..1ba47e9d --- /dev/null +++ b/migrations/mysql/2026-07-15-120000_mark_pending_custom_collection_migration/up.sql @@ -0,0 +1,13 @@ +-- Record whether 2026-07-16 is about to run in this migration sequence. The durable marker lets a +-- retry distinguish its deterministic group-derived 0/1/1 backfill from older, ambiguous data. +CREATE TABLE IF NOT EXISTS __vw_custom_role_same_run_0716 ( + marker INTEGER NOT NULL PRIMARY KEY +); +INSERT IGNORE INTO __vw_custom_role_same_run_0716 (marker) +SELECT 1 +FROM DUAL +WHERE NOT EXISTS ( + SELECT 1 + FROM __diesel_schema_migrations + WHERE version = '20260716120000' +); diff --git a/migrations/mysql/2026-07-23-120000_reconcile_legacy_custom_roles/down.sql b/migrations/mysql/2026-07-23-120000_reconcile_legacy_custom_roles/down.sql new file mode 100644 index 00000000..b9d4e9e6 --- /dev/null +++ b/migrations/mysql/2026-07-23-120000_reconcile_legacy_custom_roles/down.sql @@ -0,0 +1,3 @@ +-- This is an idempotent data repair. Reverting it must not remove permissions or recreate the +-- invalid persisted Manager type; the older-schema migration performs its own safe conversion. +SELECT 1; diff --git a/migrations/mysql/2026-07-23-120000_reconcile_legacy_custom_roles/up.sql b/migrations/mysql/2026-07-23-120000_reconcile_legacy_custom_roles/up.sql new file mode 100644 index 00000000..3186fe6a --- /dev/null +++ b/migrations/mysql/2026-07-23-120000_reconcile_legacy_custom_roles/up.sql @@ -0,0 +1,67 @@ +-- A normal User with the historical membership-level access_all bit cannot be mapped to the +-- Custom role without adding collection-management authority. Stop before dropping the source bit. +CREATE TEMPORARY TABLE __vw_legacy_user_access_all_guard ( + blocked INTEGER NOT NULL PRIMARY KEY +); +INSERT INTO __vw_legacy_user_access_all_guard (blocked) VALUES (1); +INSERT INTO __vw_legacy_user_access_all_guard (blocked) +SELECT 1 +FROM users_organizations +WHERE atype = 2 AND access_all = TRUE +LIMIT 1; +DROP TEMPORARY TABLE __vw_legacy_user_access_all_guard; + +-- The current 2026-07-16 migration copied a legacy full-access group's dynamic authority to the +-- exact direct 0/1/1 pattern. While the same organization-local source group is still present, +-- remove that deterministic copy so later group removal also revokes the authority. +UPDATE users_organizations +SET edit_any_collection = FALSE, + delete_any_collection = FALSE +WHERE atype IN (3, 4) + AND access_all = FALSE + AND create_new_collections = FALSE + AND edit_any_collection = TRUE + AND delete_any_collection = TRUE + AND EXISTS (SELECT 1 FROM __vw_custom_role_same_run_0716 WHERE marker = 1) + AND EXISTS ( + SELECT 1 + FROM groups_users AS gu + INNER JOIN `groups` AS g ON g.uuid = gu.groups_uuid + WHERE gu.users_organizations_uuid = users_organizations.uuid + AND g.organizations_uuid = users_organizations.org_uuid + AND g.access_all = TRUE + ); + +-- A remaining 0/1/1 pattern may be either an intentional direct grant or an older derived grant +-- whose source group has already been removed. Do not guess which one it is. +CREATE TEMPORARY TABLE __vw_legacy_group_access_guard ( + blocked INTEGER NOT NULL PRIMARY KEY +); +INSERT INTO __vw_legacy_group_access_guard (blocked) VALUES (1); +INSERT INTO __vw_legacy_group_access_guard (blocked) +SELECT 1 +FROM users_organizations +WHERE atype IN (3, 4) + AND access_all = FALSE + AND create_new_collections = FALSE + AND edit_any_collection = TRUE + AND delete_any_collection = TRUE +LIMIT 1; +DROP TEMPORARY TABLE __vw_legacy_group_access_guard; + +-- Membership access_all on a legacy Manager/Custom represented all three collection capabilities. +-- Set only TRUE values so this repair never removes independently configured permissions. +UPDATE users_organizations +SET create_new_collections = TRUE, + edit_any_collection = TRUE, + delete_any_collection = TRUE +WHERE atype IN (3, 4) + AND access_all = TRUE; + +-- Convert only after the legacy bit has been copied. +UPDATE users_organizations SET atype = 4 WHERE atype = 3; + +-- Clear only the marker row as transactional DML. Keeping the empty bookkeeping table avoids +-- MySQL DDL implicit commits, so the permission repair, marker clear, and Diesel ledger insert +-- either commit together or are all retried. +DELETE FROM __vw_custom_role_same_run_0716 WHERE marker = 1; diff --git a/migrations/mysql/2026-07-24-140000_guard_custom_role_downgrade/down.sql b/migrations/mysql/2026-07-24-140000_guard_custom_role_downgrade/down.sql new file mode 100644 index 00000000..4eb19e97 --- /dev/null +++ b/migrations/mysql/2026-07-24-140000_guard_custom_role_downgrade/down.sql @@ -0,0 +1,7 @@ +-- Nine independent Custom-role permissions cannot be represented losslessly by the legacy +-- role/access_all schema. Always stop before any older down migration removes permission data. +CREATE TEMPORARY TABLE __vw_custom_role_downgrade_guard ( + blocked INTEGER NOT NULL PRIMARY KEY +); +INSERT INTO __vw_custom_role_downgrade_guard (blocked) VALUES (1); +INSERT INTO __vw_custom_role_downgrade_guard (blocked) VALUES (1); diff --git a/migrations/mysql/2026-07-24-140000_guard_custom_role_downgrade/up.sql b/migrations/mysql/2026-07-24-140000_guard_custom_role_downgrade/up.sql new file mode 100644 index 00000000..af5fed1b --- /dev/null +++ b/migrations/mysql/2026-07-24-140000_guard_custom_role_downgrade/up.sql @@ -0,0 +1,3 @@ +-- Forward migration marker. Its down migration intentionally blocks an automatic lossy downgrade +-- before any granular permission column is removed. +SELECT 1; diff --git a/migrations/postgresql/2026-07-15-120000_mark_pending_custom_collection_migration/down.sql b/migrations/postgresql/2026-07-15-120000_mark_pending_custom_collection_migration/down.sql new file mode 100644 index 00000000..04346743 --- /dev/null +++ b/migrations/postgresql/2026-07-15-120000_mark_pending_custom_collection_migration/down.sql @@ -0,0 +1 @@ +DROP TABLE IF EXISTS __vw_custom_role_same_run_0716; diff --git a/migrations/postgresql/2026-07-15-120000_mark_pending_custom_collection_migration/up.sql b/migrations/postgresql/2026-07-15-120000_mark_pending_custom_collection_migration/up.sql new file mode 100644 index 00000000..f4f6862e --- /dev/null +++ b/migrations/postgresql/2026-07-15-120000_mark_pending_custom_collection_migration/up.sql @@ -0,0 +1,13 @@ +-- Record whether 2026-07-16 is about to run in this migration sequence. The durable marker lets a +-- retry distinguish its deterministic group-derived 0/1/1 backfill from older, ambiguous data. +CREATE TABLE IF NOT EXISTS __vw_custom_role_same_run_0716 ( + marker INTEGER NOT NULL PRIMARY KEY +); +INSERT INTO __vw_custom_role_same_run_0716 (marker) +SELECT 1 +WHERE NOT EXISTS ( + SELECT 1 + FROM __diesel_schema_migrations + WHERE version = '20260716120000' +) +ON CONFLICT (marker) DO NOTHING; diff --git a/migrations/postgresql/2026-07-23-120000_reconcile_legacy_custom_roles/down.sql b/migrations/postgresql/2026-07-23-120000_reconcile_legacy_custom_roles/down.sql new file mode 100644 index 00000000..b9d4e9e6 --- /dev/null +++ b/migrations/postgresql/2026-07-23-120000_reconcile_legacy_custom_roles/down.sql @@ -0,0 +1,3 @@ +-- This is an idempotent data repair. Reverting it must not remove permissions or recreate the +-- invalid persisted Manager type; the older-schema migration performs its own safe conversion. +SELECT 1; diff --git a/migrations/postgresql/2026-07-23-120000_reconcile_legacy_custom_roles/up.sql b/migrations/postgresql/2026-07-23-120000_reconcile_legacy_custom_roles/up.sql new file mode 100644 index 00000000..6d75889c --- /dev/null +++ b/migrations/postgresql/2026-07-23-120000_reconcile_legacy_custom_roles/up.sql @@ -0,0 +1,65 @@ +-- A normal User with the historical membership-level access_all bit cannot be mapped to the +-- Custom role without adding collection-management authority. Stop before dropping the source bit. +CREATE TEMPORARY TABLE __vw_legacy_user_access_all_guard ( + blocked INTEGER NOT NULL PRIMARY KEY +); +INSERT INTO __vw_legacy_user_access_all_guard (blocked) VALUES (1); +INSERT INTO __vw_legacy_user_access_all_guard (blocked) +SELECT 1 +FROM users_organizations +WHERE atype = 2 AND access_all = TRUE +LIMIT 1; +DROP TABLE __vw_legacy_user_access_all_guard; + +-- The current 2026-07-16 migration copied a legacy full-access group's dynamic authority to the +-- exact direct 0/1/1 pattern. While the same organization-local source group is still present, +-- remove that deterministic copy so later group removal also revokes the authority. +UPDATE users_organizations +SET edit_any_collection = FALSE, + delete_any_collection = FALSE +WHERE atype IN (3, 4) + AND access_all = FALSE + AND create_new_collections = FALSE + AND edit_any_collection = TRUE + AND delete_any_collection = TRUE + AND EXISTS (SELECT 1 FROM __vw_custom_role_same_run_0716 WHERE marker = 1) + AND EXISTS ( + SELECT 1 + FROM groups_users AS gu + INNER JOIN "groups" AS g ON g.uuid = gu.groups_uuid + WHERE gu.users_organizations_uuid = users_organizations.uuid + AND g.organizations_uuid = users_organizations.org_uuid + AND g.access_all = TRUE + ); + +-- A remaining 0/1/1 pattern may be either an intentional direct grant or an older derived grant +-- whose source group has already been removed. Do not guess which one it is. +CREATE TEMPORARY TABLE __vw_legacy_group_access_guard ( + blocked INTEGER NOT NULL PRIMARY KEY +); +INSERT INTO __vw_legacy_group_access_guard (blocked) VALUES (1); +INSERT INTO __vw_legacy_group_access_guard (blocked) +SELECT 1 +FROM users_organizations +WHERE atype IN (3, 4) + AND access_all = FALSE + AND create_new_collections = FALSE + AND edit_any_collection = TRUE + AND delete_any_collection = TRUE +LIMIT 1; +DROP TABLE __vw_legacy_group_access_guard; + +-- Membership access_all on a legacy Manager/Custom represented all three collection capabilities. +-- Set only TRUE values so this repair never removes independently configured permissions. +UPDATE users_organizations +SET create_new_collections = TRUE, + edit_any_collection = TRUE, + delete_any_collection = TRUE +WHERE atype IN (3, 4) + AND access_all = TRUE; + +-- Convert only after the legacy bit has been copied. +UPDATE users_organizations SET atype = 4 WHERE atype = 3; + +-- Clear the same-run marker only after every guard and permission update succeeds. +DELETE FROM __vw_custom_role_same_run_0716 WHERE marker = 1; diff --git a/migrations/postgresql/2026-07-24-140000_guard_custom_role_downgrade/down.sql b/migrations/postgresql/2026-07-24-140000_guard_custom_role_downgrade/down.sql new file mode 100644 index 00000000..4eb19e97 --- /dev/null +++ b/migrations/postgresql/2026-07-24-140000_guard_custom_role_downgrade/down.sql @@ -0,0 +1,7 @@ +-- Nine independent Custom-role permissions cannot be represented losslessly by the legacy +-- role/access_all schema. Always stop before any older down migration removes permission data. +CREATE TEMPORARY TABLE __vw_custom_role_downgrade_guard ( + blocked INTEGER NOT NULL PRIMARY KEY +); +INSERT INTO __vw_custom_role_downgrade_guard (blocked) VALUES (1); +INSERT INTO __vw_custom_role_downgrade_guard (blocked) VALUES (1); diff --git a/migrations/postgresql/2026-07-24-140000_guard_custom_role_downgrade/up.sql b/migrations/postgresql/2026-07-24-140000_guard_custom_role_downgrade/up.sql new file mode 100644 index 00000000..af5fed1b --- /dev/null +++ b/migrations/postgresql/2026-07-24-140000_guard_custom_role_downgrade/up.sql @@ -0,0 +1,3 @@ +-- Forward migration marker. Its down migration intentionally blocks an automatic lossy downgrade +-- before any granular permission column is removed. +SELECT 1; diff --git a/migrations/sqlite/2026-07-15-120000_mark_pending_custom_collection_migration/down.sql b/migrations/sqlite/2026-07-15-120000_mark_pending_custom_collection_migration/down.sql new file mode 100644 index 00000000..04346743 --- /dev/null +++ b/migrations/sqlite/2026-07-15-120000_mark_pending_custom_collection_migration/down.sql @@ -0,0 +1 @@ +DROP TABLE IF EXISTS __vw_custom_role_same_run_0716; diff --git a/migrations/sqlite/2026-07-15-120000_mark_pending_custom_collection_migration/up.sql b/migrations/sqlite/2026-07-15-120000_mark_pending_custom_collection_migration/up.sql new file mode 100644 index 00000000..53fd7671 --- /dev/null +++ b/migrations/sqlite/2026-07-15-120000_mark_pending_custom_collection_migration/up.sql @@ -0,0 +1,12 @@ +-- Record whether 2026-07-16 is about to run in this migration sequence. The durable marker lets a +-- retry distinguish its deterministic group-derived 0/1/1 backfill from older, ambiguous data. +CREATE TABLE IF NOT EXISTS __vw_custom_role_same_run_0716 ( + marker INTEGER NOT NULL PRIMARY KEY +); +INSERT OR IGNORE INTO __vw_custom_role_same_run_0716 (marker) +SELECT 1 +WHERE NOT EXISTS ( + SELECT 1 + FROM __diesel_schema_migrations + WHERE version = '20260716120000' +); diff --git a/migrations/sqlite/2026-07-23-120000_reconcile_legacy_custom_roles/down.sql b/migrations/sqlite/2026-07-23-120000_reconcile_legacy_custom_roles/down.sql new file mode 100644 index 00000000..b9d4e9e6 --- /dev/null +++ b/migrations/sqlite/2026-07-23-120000_reconcile_legacy_custom_roles/down.sql @@ -0,0 +1,3 @@ +-- This is an idempotent data repair. Reverting it must not remove permissions or recreate the +-- invalid persisted Manager type; the older-schema migration performs its own safe conversion. +SELECT 1; diff --git a/migrations/sqlite/2026-07-23-120000_reconcile_legacy_custom_roles/up.sql b/migrations/sqlite/2026-07-23-120000_reconcile_legacy_custom_roles/up.sql new file mode 100644 index 00000000..6d75889c --- /dev/null +++ b/migrations/sqlite/2026-07-23-120000_reconcile_legacy_custom_roles/up.sql @@ -0,0 +1,65 @@ +-- A normal User with the historical membership-level access_all bit cannot be mapped to the +-- Custom role without adding collection-management authority. Stop before dropping the source bit. +CREATE TEMPORARY TABLE __vw_legacy_user_access_all_guard ( + blocked INTEGER NOT NULL PRIMARY KEY +); +INSERT INTO __vw_legacy_user_access_all_guard (blocked) VALUES (1); +INSERT INTO __vw_legacy_user_access_all_guard (blocked) +SELECT 1 +FROM users_organizations +WHERE atype = 2 AND access_all = TRUE +LIMIT 1; +DROP TABLE __vw_legacy_user_access_all_guard; + +-- The current 2026-07-16 migration copied a legacy full-access group's dynamic authority to the +-- exact direct 0/1/1 pattern. While the same organization-local source group is still present, +-- remove that deterministic copy so later group removal also revokes the authority. +UPDATE users_organizations +SET edit_any_collection = FALSE, + delete_any_collection = FALSE +WHERE atype IN (3, 4) + AND access_all = FALSE + AND create_new_collections = FALSE + AND edit_any_collection = TRUE + AND delete_any_collection = TRUE + AND EXISTS (SELECT 1 FROM __vw_custom_role_same_run_0716 WHERE marker = 1) + AND EXISTS ( + SELECT 1 + FROM groups_users AS gu + INNER JOIN "groups" AS g ON g.uuid = gu.groups_uuid + WHERE gu.users_organizations_uuid = users_organizations.uuid + AND g.organizations_uuid = users_organizations.org_uuid + AND g.access_all = TRUE + ); + +-- A remaining 0/1/1 pattern may be either an intentional direct grant or an older derived grant +-- whose source group has already been removed. Do not guess which one it is. +CREATE TEMPORARY TABLE __vw_legacy_group_access_guard ( + blocked INTEGER NOT NULL PRIMARY KEY +); +INSERT INTO __vw_legacy_group_access_guard (blocked) VALUES (1); +INSERT INTO __vw_legacy_group_access_guard (blocked) +SELECT 1 +FROM users_organizations +WHERE atype IN (3, 4) + AND access_all = FALSE + AND create_new_collections = FALSE + AND edit_any_collection = TRUE + AND delete_any_collection = TRUE +LIMIT 1; +DROP TABLE __vw_legacy_group_access_guard; + +-- Membership access_all on a legacy Manager/Custom represented all three collection capabilities. +-- Set only TRUE values so this repair never removes independently configured permissions. +UPDATE users_organizations +SET create_new_collections = TRUE, + edit_any_collection = TRUE, + delete_any_collection = TRUE +WHERE atype IN (3, 4) + AND access_all = TRUE; + +-- Convert only after the legacy bit has been copied. +UPDATE users_organizations SET atype = 4 WHERE atype = 3; + +-- Clear the same-run marker only after every guard and permission update succeeds. +DELETE FROM __vw_custom_role_same_run_0716 WHERE marker = 1; diff --git a/migrations/sqlite/2026-07-24-140000_guard_custom_role_downgrade/down.sql b/migrations/sqlite/2026-07-24-140000_guard_custom_role_downgrade/down.sql new file mode 100644 index 00000000..4eb19e97 --- /dev/null +++ b/migrations/sqlite/2026-07-24-140000_guard_custom_role_downgrade/down.sql @@ -0,0 +1,7 @@ +-- Nine independent Custom-role permissions cannot be represented losslessly by the legacy +-- role/access_all schema. Always stop before any older down migration removes permission data. +CREATE TEMPORARY TABLE __vw_custom_role_downgrade_guard ( + blocked INTEGER NOT NULL PRIMARY KEY +); +INSERT INTO __vw_custom_role_downgrade_guard (blocked) VALUES (1); +INSERT INTO __vw_custom_role_downgrade_guard (blocked) VALUES (1); diff --git a/migrations/sqlite/2026-07-24-140000_guard_custom_role_downgrade/up.sql b/migrations/sqlite/2026-07-24-140000_guard_custom_role_downgrade/up.sql new file mode 100644 index 00000000..af5fed1b --- /dev/null +++ b/migrations/sqlite/2026-07-24-140000_guard_custom_role_downgrade/up.sql @@ -0,0 +1,3 @@ +-- Forward migration marker. Its down migration intentionally blocks an automatic lossy downgrade +-- before any granular permission column is removed. +SELECT 1; diff --git a/src/api/admin.rs b/src/api/admin.rs index b03946af..5989efdb 100644 --- a/src/api/admin.rs +++ b/src/api/admin.rs @@ -557,6 +557,19 @@ fn apply_membership_type_change(membership: &mut Membership, new_type: Membershi membership.atype = new_type as i32; } +fn parse_admin_membership_type(user_type: NumberOrString) -> Option { + let raw_type = user_type.into_string(); + + // The public API still accepts the legacy Manager representation for compatibility and folds + // it into Custom. The admin panel must not do that: treating an apparent Manager demotion as a + // Custom-to-Custom update would preserve the member's existing granular permissions. + if matches!(raw_type.as_str(), "3" | "Manager") { + return None; + } + + MembershipType::from_str(&raw_type) +} + #[post("/users/org_type", format = "application/json", data = "")] async fn update_membership_type(data: Json, token: AdminToken, conn: DbConn) -> EmptyResult { let data: MembershipTypeData = data.into_inner(); @@ -566,7 +579,7 @@ async fn update_membership_type(data: Json, token: AdminToke err!("The specified user isn't member of the organization") }; - let Some(new_type) = MembershipType::from_str(&data.user_type.into_string()) else { + let Some(new_type) = parse_admin_membership_type(data.user_type) else { err!("Invalid type") }; @@ -955,4 +968,16 @@ mod tests { assert_eq!(promo.atype, MembershipType::Admin as i32); assert!(!promo.edit_any_collection); } + + #[test] + fn admin_type_parser_rejects_legacy_manager_before_normalization() { + assert!(parse_admin_membership_type(NumberOrString::Number(3)).is_none()); + assert!(parse_admin_membership_type(NumberOrString::String("3".to_owned())).is_none()); + assert!(parse_admin_membership_type(NumberOrString::String("Manager".to_owned())).is_none()); + + assert!(parse_admin_membership_type(NumberOrString::Number(4)) == Some(MembershipType::Custom)); + assert!( + parse_admin_membership_type(NumberOrString::String("Custom".to_owned())) == Some(MembershipType::Custom) + ); + } } diff --git a/src/api/core/events.rs b/src/api/core/events.rs index 012be88f..b856176c 100644 --- a/src/api/core/events.rs +++ b/src/api/core/events.rs @@ -10,9 +10,12 @@ use crate::{ auth::{AccessEventLogsHeaders, Headers}, db::{ DbConn, DbPool, - models::{Cipher, CipherId, Event, Membership, MembershipId, OrganizationId, UserId}, + models::{ + Cipher, CipherId, Event, EventType, Membership, MembershipId, MembershipStatus, MembershipType, + OrganizationId, UserId, + }, }, - util::parse_date, + util::try_parse_date, }; /// ############################################################################################################### @@ -29,6 +32,28 @@ struct EventRange { continuation_token: Option, } +fn parse_event_date(date: &str, field: &str) -> Result { + try_parse_date(date) + .map_err(|error| crate::Error::new("Invalid event date", format!("Invalid RFC 3339 {field}: {error}"))) +} + +fn parse_event_range(data: &EventRange) -> Result<(NaiveDateTime, NaiveDateTime), crate::Error> { + let start_date = parse_event_date(&data.start, "start date")?; + + let end_date = if let Some(continuation_token) = &data.continuation_token { + try_parse_date(continuation_token).map_err(|error| { + crate::Error::new( + "Invalid continuation token", + format!("Continuation token is not a valid RFC 3339 date: {error}"), + ) + })? + } else { + parse_event_date(&data.end, "end date")? + }; + + Ok((start_date, end_date)) +} + // Upstream: https://github.com/bitwarden/server/blob/9ebe16587175b1c0e9208f84397bb75d0d595510/src/Api/AdminConsole/Controllers/EventsController.cs#L87 #[get("/organizations//events?")] async fn get_org_events( @@ -44,12 +69,7 @@ async fn get_org_events( // Return an empty vec when we org events are disabled. // This prevents client errors let events_json: Vec = if CONFIG.org_events_enabled() { - let start_date = parse_date(&data.start); - let end_date = if let Some(before_date) = &data.continuation_token { - parse_date(before_date) - } else { - parse_date(&data.end) - }; + let (start_date, end_date) = parse_event_range(&data)?; Event::find_by_organization_uuid(&org_id, &start_date, &end_date, &conn) .await @@ -67,21 +87,70 @@ async fn get_org_events( }))) } +#[derive(Debug, Eq, PartialEq)] +enum CipherEventScope { + Organization(OrganizationId), + Personal, +} + +impl CipherEventScope { + fn includes(&self, event: &Event) -> bool { + match self { + Self::Organization(org_id) => event.org_uuid.as_ref() == Some(org_id), + Self::Personal => event.org_uuid.is_none(), + } + } +} + +fn membership_can_access_event_logs(membership: &Membership) -> bool { + membership.has_status(MembershipStatus::Confirmed) + && (membership.atype >= MembershipType::Admin || membership.has_access_event_logs()) +} + +fn cipher_event_scope(cipher: &Cipher, user_id: &UserId, membership: Option<&Membership>) -> Option { + match &cipher.organization_uuid { + Some(org_id) + if membership.is_some_and(|membership| { + membership.user_uuid == *user_id + && membership.org_uuid == *org_id + && membership_can_access_event_logs(membership) + }) => + { + Some(CipherEventScope::Organization(org_id.clone())) + } + None if cipher.is_owned_by_user(user_id) => Some(CipherEventScope::Personal), + _ => None, + } +} + #[get("/ciphers//events?")] async fn get_cipher_events(cipher_id: CipherId, data: EventRange, headers: Headers, conn: DbConn) -> JsonResult { // Return an empty vec when org events are disabled. // This prevents client errors - let events_json: Vec = if CONFIG.org_events_enabled() - && Membership::user_has_ge_admin_access_to_cipher(&headers.user.uuid, &cipher_id, &conn).await - { - let start_date = parse_date(&data.start); - let end_date = if let Some(before_date) = &data.continuation_token { - parse_date(before_date) + let events_json: Vec = if CONFIG.org_events_enabled() { + let (start_date, end_date) = parse_event_range(&data)?; + + let scope = if let Some(cipher) = Cipher::find_by_uuid(&cipher_id, &conn).await { + let membership = if let Some(org_id) = &cipher.organization_uuid { + Membership::find_by_user_and_org(&headers.user.uuid, org_id, &conn).await + } else { + None + }; + cipher_event_scope(&cipher, &headers.user.uuid, membership.as_ref()) } else { - parse_date(&data.end) + None }; - Event::find_by_cipher_uuid(&cipher_id, &start_date, &end_date, &conn).await.iter().map(Event::to_json).collect() + if let Some(scope) = scope { + Event::find_by_cipher_uuid(&cipher_id, &start_date, &end_date, &conn) + .await + .iter() + .filter(|event| scope.includes(event)) + .map(Event::to_json) + .collect() + } else { + Vec::new() + } } else { Vec::new() }; @@ -104,15 +173,11 @@ async fn get_user_events( if org_id != headers.org_id { err!("Organization not found", "Organization id's do not match"); } + // Return an empty vec when we org events are disabled. // This prevents client errors let events_json: Vec = if CONFIG.org_events_enabled() { - let start_date = parse_date(&data.start); - let end_date = if let Some(before_date) = &data.continuation_token { - parse_date(before_date) - } else { - parse_date(&data.end) - }; + let (start_date, end_date) = parse_event_range(&data)?; Event::find_by_org_and_member(&org_id, &member_id, &start_date, &end_date, &conn) .await @@ -163,6 +228,48 @@ struct EventCollection { organization_id: Option, } +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum ClientEventKind { + User, + Cipher, + Organization, +} + +const MAX_CLIENT_EVENT_BATCH_SIZE: usize = 1_000; + +fn validate_client_event_batch_size(event_count: usize) -> Result<(), crate::Error> { + if event_count > MAX_CLIENT_EVENT_BATCH_SIZE { + return Err(crate::Error::new( + "Event batch is too large", + format!("At most {MAX_CLIENT_EVENT_BATCH_SIZE} events are accepted per request"), + )); + } + Ok(()) +} + +fn client_event_kind(event_type: i32) -> Option { + match event_type { + event_type if event_type == EventType::UserClientExportedVault as i32 => Some(ClientEventKind::User), + event_type + if event_type == EventType::CipherClientViewed as i32 + || event_type == EventType::CipherClientToggledPasswordVisible as i32 + || event_type == EventType::CipherClientToggledHiddenFieldVisible as i32 + || event_type == EventType::CipherClientToggledCardCodeVisible as i32 + || event_type == EventType::CipherClientCopiedPassword as i32 + || event_type == EventType::CipherClientCopiedHiddenField as i32 + || event_type == EventType::CipherClientCopiedCardCode as i32 + || event_type == EventType::CipherClientAutofilled as i32 + || event_type == EventType::CipherClientToggledCardNumberVisible as i32 => + { + Some(ClientEventKind::Cipher) + } + event_type if event_type == EventType::OrganizationClientExportedVault as i32 => { + Some(ClientEventKind::Organization) + } + _ => None, + } +} + // Upstream: // https://github.com/bitwarden/server/blob/9ebe16587175b1c0e9208f84397bb75d0d595510/src/Events/Controllers/CollectController.cs // https://github.com/bitwarden/server/blob/9ebe16587175b1c0e9208f84397bb75d0d595510/src/Core/AdminConsole/Services/Implementations/EventService.cs @@ -172,10 +279,25 @@ async fn post_events_collect(data: Json>, headers: Headers, return Ok(()); } + // Official clients normally submit small batches (upstream explicitly exercises batches of + // 100). Keep ample headroom while preventing one authenticated request from causing an + // effectively unbounded sequence of database reads and writes under the shared 20 MiB JSON + // limit. + validate_client_event_batch_size(data.len())?; + + // Validate all accepted client events before writing any of them. Unsupported event types are + // ignored, matching upstream, while malformed dates on accepted events produce a controlled + // 400 response instead of panicking after a partially processed batch. + let mut accepted_events = Vec::new(); for event in data.iter() { - let event_date = parse_date(&event.date); - match event.r#type { - 1000..=1099 => { + if let Some(kind) = client_event_kind(event.r#type) { + accepted_events.push((event, kind, parse_event_date(&event.date, "event date")?)); + } + } + + for (event, kind, event_date) in accepted_events { + match kind { + ClientEventKind::User => { log_user_event_impl( event.r#type, &headers.user.uuid, @@ -186,7 +308,7 @@ async fn post_events_collect(data: Json>, headers: Headers, ) .await; } - 1600..=1699 => { + ClientEventKind::Organization => { // Only allow logging events for an organization the user is actually a member of. if let Some(org_id) = &event.organization_id && Membership::find_confirmed_by_user_and_org(&headers.user.uuid, org_id, &conn).await.is_some() @@ -204,7 +326,7 @@ async fn post_events_collect(data: Json>, headers: Headers, .await; } } - _ => { + ClientEventKind::Cipher => { // The cipher determines the organization the event is logged to, so make sure the // user can actually access it instead of trusting the provided cipher uuid. if let Some(cipher_uuid) = &event.cipher_id @@ -230,6 +352,158 @@ async fn post_events_collect(data: Json>, headers: Headers, Ok(()) } +#[cfg(test)] +mod tests { + use super::*; + + fn membership(member_type: MembershipType, status: MembershipStatus) -> Membership { + let mut membership = Membership::new("test-user".to_owned().into(), "test-org".to_owned().into(), None); + membership.atype = member_type as i32; + membership.status = status as i32; + membership + } + + #[test] + fn cipher_event_access_requires_confirmed_admin_or_access_event_logs() { + for member_type in [MembershipType::Owner, MembershipType::Admin] { + assert!(membership_can_access_event_logs(&membership(member_type, MembershipStatus::Confirmed))); + assert!(!membership_can_access_event_logs(&membership(member_type, MembershipStatus::Invited))); + assert!(!membership_can_access_event_logs(&membership(member_type, MembershipStatus::Accepted))); + assert!(!membership_can_access_event_logs(&membership(member_type, MembershipStatus::Revoked))); + } + + let mut custom = membership(MembershipType::Custom, MembershipStatus::Confirmed); + assert!(!membership_can_access_event_logs(&custom)); + custom.access_event_logs = true; + assert!(membership_can_access_event_logs(&custom)); + + custom.status = MembershipStatus::Revoked as i32; + assert!(!membership_can_access_event_logs(&custom)); + assert!(!membership_can_access_event_logs(&membership(MembershipType::User, MembershipStatus::Confirmed))); + } + + #[test] + fn cipher_event_scope_is_bound_to_cipher_org_or_personal_owner() { + let user_id: UserId = "test-user".to_owned().into(); + let org_id: OrganizationId = "test-org".to_owned().into(); + let mut cipher = Cipher::new(1, "test-cipher".to_owned()); + cipher.organization_uuid = Some(org_id.clone()); + + let admin = membership(MembershipType::Admin, MembershipStatus::Confirmed); + assert_eq!( + cipher_event_scope(&cipher, &user_id, Some(&admin)), + Some(CipherEventScope::Organization(org_id.clone())) + ); + + let accepted_admin = membership(MembershipType::Admin, MembershipStatus::Accepted); + assert_eq!(cipher_event_scope(&cipher, &user_id, Some(&accepted_admin)), None); + + let mut foreign_membership = membership(MembershipType::Admin, MembershipStatus::Confirmed); + foreign_membership.org_uuid = "other-org".to_owned().into(); + assert_eq!(cipher_event_scope(&cipher, &user_id, Some(&foreign_membership)), None); + + cipher.organization_uuid = None; + cipher.user_uuid = Some(user_id.clone()); + assert_eq!(cipher_event_scope(&cipher, &user_id, None), Some(CipherEventScope::Personal)); + assert_eq!(cipher_event_scope(&cipher, &"other-user".to_owned().into(), None), None); + } + + #[test] + fn cipher_event_rows_must_match_the_authorized_scope() { + let org_id: OrganizationId = "test-org".to_owned().into(); + let mut event = Event::new(EventType::CipherClientViewed as i32, None); + + assert!(CipherEventScope::Personal.includes(&event)); + event.org_uuid = Some(org_id.clone()); + assert!(!CipherEventScope::Personal.includes(&event)); + assert!(CipherEventScope::Organization(org_id).includes(&event)); + assert!(!CipherEventScope::Organization("other-org".to_owned().into()).includes(&event)); + } + + #[test] + fn event_range_rejects_invalid_dates_and_continuation_tokens() { + let valid = EventRange { + start: "2026-07-25T10:00:00Z".to_owned(), + end: "2026-07-25T11:00:00Z".to_owned(), + continuation_token: None, + }; + assert!(parse_event_range(&valid).is_ok()); + + let invalid_start = EventRange { + start: "not-a-date".to_owned(), + ..valid + }; + assert!(parse_event_range(&invalid_start).is_err()); + + let invalid_end = EventRange { + start: "2026-07-25T10:00:00Z".to_owned(), + end: "not-a-date".to_owned(), + continuation_token: None, + }; + assert!(parse_event_range(&invalid_end).is_err()); + + let invalid_token = EventRange { + start: "2026-07-25T10:00:00Z".to_owned(), + end: "2026-07-25T11:00:00Z".to_owned(), + continuation_token: Some("not-a-date".to_owned()), + }; + assert!(parse_event_range(&invalid_token).is_err()); + + let token_supersedes_end = EventRange { + start: "2026-07-25T10:00:00Z".to_owned(), + end: "legacy-client-value-that-is-not-used".to_owned(), + continuation_token: Some("2026-07-25T10:30:00Z".to_owned()), + }; + assert!(parse_event_range(&token_supersedes_end).is_ok()); + } + + #[test] + fn collect_accepts_only_official_client_generated_event_types() { + assert_eq!(client_event_kind(EventType::UserClientExportedVault as i32), Some(ClientEventKind::User)); + for event_type in [ + EventType::CipherClientViewed, + EventType::CipherClientToggledPasswordVisible, + EventType::CipherClientToggledHiddenFieldVisible, + EventType::CipherClientToggledCardCodeVisible, + EventType::CipherClientCopiedPassword, + EventType::CipherClientCopiedHiddenField, + EventType::CipherClientCopiedCardCode, + EventType::CipherClientAutofilled, + EventType::CipherClientToggledCardNumberVisible, + ] { + assert_eq!(client_event_kind(event_type as i32), Some(ClientEventKind::Cipher)); + } + assert_eq!( + client_event_kind(EventType::OrganizationClientExportedVault as i32), + Some(ClientEventKind::Organization) + ); + + for event_type in [ + EventType::UserLoggedIn, + EventType::UserChangedPassword, + EventType::CipherCreated, + EventType::CipherUpdated, + EventType::CipherDeleted, + EventType::OrganizationUpdated, + EventType::OrganizationPurgedVault, + EventType::PolicyUpdated, + ] { + assert_eq!(client_event_kind(event_type as i32), None); + } + assert_eq!(client_event_kind(1099), None); + assert_eq!(client_event_kind(1199), None); + assert_eq!(client_event_kind(1699), None); + } + + #[test] + fn collect_batch_limit_preserves_normal_batches_and_rejects_excess() { + assert!(validate_client_event_batch_size(0).is_ok()); + assert!(validate_client_event_batch_size(100).is_ok()); + assert!(validate_client_event_batch_size(MAX_CLIENT_EVENT_BATCH_SIZE).is_ok()); + assert!(validate_client_event_batch_size(MAX_CLIENT_EVENT_BATCH_SIZE + 1).is_err()); + } +} + pub async fn log_user_event(event_type: i32, user_id: &UserId, device_type: i32, ip: &IpAddr, conn: &DbConn) { if !CONFIG.org_events_enabled() { return; diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index 50754dd8..7d05787c 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -564,26 +564,25 @@ async fn post_organization_collections( let collection = Collection::new(org_id.clone(), data.name, data.external_id); collection.save(&conn).await?; - log_event( - EventType::CollectionCreated as i32, - &collection.uuid, - &org_id, - &headers.user.uuid, - headers.device.atype, - &headers.ip.ip, - &conn, - ) - .await; - // Security (F-3): a `manage` grant carries collection *delete*/administer authority // (`has_explicit_collection_manage_access` -> CollectionDeleteHeaders/ManagerHeaders), so only a // caller who could delete this collection may confer it — the same rule the collection-update and // 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 and Custom-with-`delete_any_collection` - // keep it. Evaluated after the collection exists so the per-collection lookup sees it. + // member or to a group while creating the collection. For such callers the requested `manage` + // is forced to false; Admin/Owner and Custom-with-`delete_any_collection` keep it. The creator's + // own object-scoped ownership is added separately below. 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; + let creator_needs_assignment = !headers.membership.has_full_access(); + + // Persist the creator's object-scoped ownership before secondary assignments. If a later + // assignment write fails, the otherwise non-transactional create path still leaves the new + // collection recoverably manageable by its creator. An explicit self-assignment below is + // skipped so it cannot weaken this grant. + if creator_needs_assignment { + CollectionUser::save(&headers.membership.user_uuid, &collection.uuid, false, false, true, &conn).await?; + } for group in data.groups { CollectionGroup::new( @@ -605,6 +604,9 @@ async fn post_organization_collections( if member.grants_access_to_all_collections() { continue; } + if member.user_uuid == headers.membership.user_uuid && creator_needs_assignment { + continue; + } CollectionUser::save( &member.user_uuid, @@ -617,6 +619,19 @@ async fn post_organization_collections( .await?; } + // Emit the success event only after all requested assignments and the creator's object-scoped + // manage grant have been persisted. A later write failure must not leave a false audit record. + log_event( + EventType::CollectionCreated as i32, + &collection.uuid, + &org_id, + &headers.user.uuid, + headers.device.atype, + &headers.ip.ip, + &conn, + ) + .await; + Ok(Json(collection.to_json_details(&headers.membership.user_uuid, None, &conn).await)) } @@ -1139,10 +1154,6 @@ async fn get_members( err!("Organization not found", "Organization id's do not match"); } - if !headers.membership.has_full_access() { - err_code!("Resource not found.", "User does not have full access", rocket::http::Status::NotFound.code); - } - let mut users_json = Vec::new(); for u in Membership::find_by_org(&org_id, &conn).await { users_json.push( @@ -1226,7 +1237,9 @@ impl CustomRolePermissions { delete_any_collection: enabled("deleteAnyCollection"), access_event_logs: enabled("accessEventLogs"), access_import_export: enabled("accessImportExport"), - access_reports: enabled("accessReports"), + // Vaultwarden has no report endpoints yet. Keep the compatibility field in the + // database/DTO, but never accept a permission that cannot be enforced server-side. + access_reports: false, } } @@ -1238,6 +1251,34 @@ impl CustomRolePermissions { member_type >= MembershipType::Admin || (member_type == MembershipType::Custom && self.edit_any_collection) } + /// Parse permissions for an existing member without treating an omitted permissions object as + /// an instruction to clear every Custom-role grant. Older clients send legacy role value `3` + /// without the modern object; that value is normalized to Custom for compatibility. + fn from_edit_request( + member_type: MembershipType, + permissions: Option<&HashMap>, + membership: &Membership, + ) -> Self { + match permissions { + Some(permissions) => Self::from_request(member_type, permissions), + None if member_type == MembershipType::Custom && membership.atype == MembershipType::Custom as i32 => { + Self { + manage_users: membership.manage_users, + manage_groups: membership.manage_groups, + manage_policies: membership.manage_policies, + create_new_collections: membership.create_new_collections, + edit_any_collection: membership.edit_any_collection, + delete_any_collection: membership.delete_any_collection, + access_event_logs: membership.access_event_logs, + access_import_export: membership.access_import_export, + // Reports are unsupported and therefore never preserved as an active grant. + access_reports: false, + } + } + None => Self::default(), + } + } + fn differs_from(self, membership: &Membership) -> bool { self.manage_users != membership.manage_users || self.manage_groups != membership.manage_groups @@ -1247,7 +1288,6 @@ impl CustomRolePermissions { || self.delete_any_collection != membership.delete_any_collection || self.access_event_logs != membership.access_event_logs || self.access_import_export != membership.access_import_export - || self.access_reports != membership.access_reports } fn apply_to(self, membership: &mut Membership) { @@ -1310,8 +1350,8 @@ async fn send_invite( err!("Invalid type") }; - if new_type != MembershipType::User && headers.membership_type != MembershipType::Owner { - err!("Only Owners can invite Admins, Owners or Custom members") + if !may_manage_member_type(headers.membership_type, new_type) { + err!("You don't have permission to invite this role") } // manageAllCollections is a client-only aggregate. Persist its three children independently. @@ -1487,7 +1527,7 @@ async fn bulk_reinvite_members( let mut bulk_response = Vec::new(); for member_id in data.ids { - let err_msg = match reinvite_member_impl(&org_id, &member_id, &headers.user.email, &conn).await { + let err_msg = match reinvite_member_impl(&org_id, &member_id, &headers, &conn).await { Ok(()) => String::new(), Err(e) => format!("{e:?}"), }; @@ -1518,19 +1558,23 @@ async fn reinvite_member( if org_id != headers.org_id { err!("Organization not found", "Organization id's do not match"); } - reinvite_member_impl(&org_id, &member_id, &headers.user.email, &conn).await + reinvite_member_impl(&org_id, &member_id, &headers, &conn).await } async fn reinvite_member_impl( org_id: &OrganizationId, member_id: &MembershipId, - invited_by_email: &str, + headers: &ManageUsersHeaders, conn: &DbConn, ) -> EmptyResult { let Some(member) = Membership::find_by_uuid_and_org(member_id, org_id, conn).await else { err!("The user hasn't been invited to the organization.") }; + if !may_manage_stored_member_type(headers.membership_type, member.atype) { + err!("You don't have permission to reinvite this user") + } + if member.status != MembershipStatus::Invited as i32 { err!("The user is already accepted or confirmed to the organization") } @@ -1550,7 +1594,7 @@ async fn reinvite_member_impl( }; if CONFIG.mail_enabled() { - mail::send_invite(&user, org_id.clone(), member.uuid, &org_name, Some(invited_by_email.to_owned())).await?; + mail::send_invite(&user, org_id.clone(), member.uuid, &org_name, Some(headers.user.email.clone())).await?; } else if user.password_hash.is_empty() { let invitation = Invitation::new(&user.email); invitation.save(conn).await?; @@ -1715,8 +1759,8 @@ async fn confirm_invite_impl( err!("The specified user isn't a member of the organization") }; - if member_to_confirm.atype != MembershipType::User && headers.membership_type != MembershipType::Owner { - err!("Only Owners can confirm Admins, Owners or Custom members") + if !may_manage_stored_member_type(headers.membership_type, member_to_confirm.atype) { + err!("You don't have permission to confirm this user") } if member_to_confirm.status != MembershipStatus::Accepted as i32 { @@ -1807,8 +1851,7 @@ struct EditUserData { r#type: NumberOrString, collections: Option>, groups: Option>, - #[serde(default)] - permissions: HashMap, + permissions: Option>, } #[put("/organizations//users/", data = "", rank = 1)] @@ -1840,13 +1883,14 @@ async fn edit_member( err!("Invalid type") }; - let custom_permissions = CustomRolePermissions::from_request(new_type, &data.permissions); - 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") }; + let custom_permissions = + CustomRolePermissions::from_edit_request(new_type, data.permissions.as_ref(), &member_to_edit); + let grants_full_access = custom_permissions.grants_full_collection_access(new_type); + if new_type != member_to_edit.atype && (member_to_edit.atype >= MembershipType::Admin || new_type >= MembershipType::Admin) && headers.membership_type != MembershipType::Owner @@ -1855,13 +1899,12 @@ 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 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. + // with manage_users must not change roles: raising a member to Custom can activate existing + // explicit collection-Manage assignments and other Custom-only authorization paths, while + // lowering it revokes them. Those authority changes are outside Manage Users even though + // granular permission changes are independently 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") } @@ -2081,8 +2124,8 @@ async fn delete_member_impl( err!("User to delete isn't member of the organization") }; - if member_to_delete.atype != MembershipType::User && headers.membership_type != MembershipType::Owner { - err!("Only Owners can delete Admins or Owners") + if !may_manage_stored_member_type(headers.membership_type, member_to_delete.atype) { + err!("You don't have permission to delete this user") } if member_to_delete.atype == MembershipType::Owner && member_to_delete.status == MembershipStatus::Confirmed as i32 @@ -2751,15 +2794,9 @@ async fn revoke_member_impl( if member.user_uuid == headers.user.uuid { err!("You cannot revoke yourself") } - // Security: a Custom user with manage_users must not be able to revoke Admins or - // Owners. Mirrors the restriction in delete_member_impl; the Owner-specific check - // below still guards Admin-vs-Owner actions. - if member.atype != MembershipType::User && headers.membership_type < MembershipType::Admin { + if !may_manage_stored_member_type(headers.membership_type, member.atype) { err!("You don't have permission to revoke this user") } - if member.atype == MembershipType::Owner && headers.membership_type != MembershipType::Owner { - err!("Only owners can revoke other owners") - } if member.atype == MembershipType::Owner && Membership::count_confirmed_by_org_and_type(org_id, MembershipType::Owner, conn).await <= 1 { @@ -2857,15 +2894,9 @@ async fn restore_member_impl( if member.user_uuid == headers.user.uuid { err!("You cannot restore yourself") } - // Security: a Custom user with manage_users must not be able to restore Admins or - // Owners. Mirrors the restriction in delete_member_impl; the Owner-specific check - // below still guards Admin-vs-Owner actions. - if member.atype != MembershipType::User && headers.membership_type < MembershipType::Admin { + if !may_manage_stored_member_type(headers.membership_type, member.atype) { err!("You don't have permission to restore this user") } - if member.atype == MembershipType::Owner && headers.membership_type != MembershipType::Owner { - err!("Only owners can restore other owners") - } member.restore(); // This check is also done at accept_invite, _confirm_invite, _activate_member, edit_member, admin::update_membership_type @@ -3179,17 +3210,32 @@ fn may_change_group_membership(caller_can_manage_collections: bool, group_confer /// Whether a caller of `edit_member` may change a member's role type. /// /// Only Admins and Owners may change a member's role at all. A Custom member with `manage_users` -/// must not, because the role type has collection-access side effects: a member of type -/// `Manager`/`Custom` gains collection-"manage" on every collection they can write (the -/// `atype >= Manager` branches in `Collection`/`Membership`), so promoting grants that access and -/// demoting revokes it. `manage_users` covers the user lifecycle, not the data plane, so role -/// changes are reserved for Admins/Owners. Leaving the role unchanged is always allowed so +/// must not, because the role type changes organization-wide collection reach and which granular +/// permissions are effective. `manage_users` covers the user lifecycle, not the data plane, so +/// role changes are reserved for Admins/Owners. Leaving the role unchanged is always allowed so /// `manage_users` members can still use the regular edit dialog. Admin/Owner transitions are /// additionally governed by the dedicated Owner-only guard in `edit_member`. fn may_change_member_type(caller_type: MembershipType, current_atype: i32, new_type: MembershipType) -> bool { caller_type >= MembershipType::Admin || new_type == current_atype } +/// Whether a caller with user-management access may perform lifecycle actions on a target role. +/// +/// Owners may manage every role. Admins may manage Admin, Custom, and User memberships, but never +/// Owners. Custom members holding `manage_users` are limited to ordinary Users. +fn may_manage_member_type(caller_type: MembershipType, target_type: MembershipType) -> bool { + match caller_type { + MembershipType::Owner => true, + MembershipType::Admin => target_type != MembershipType::Owner, + MembershipType::Custom => target_type == MembershipType::User, + MembershipType::User => false, + } +} + +fn may_manage_stored_member_type(caller_type: MembershipType, target_atype: i32) -> bool { + MembershipType::from_i32(target_atype).is_some_and(|target_type| may_manage_member_type(caller_type, target_type)) +} + /// Returns true if being a member of `group_id` confers collection access — either because the /// group has `access_all` set, or because it has collections assigned. async fn group_confers_collection_access(group_id: &GroupId, org_id: &OrganizationId, conn: &DbConn) -> bool { @@ -3965,7 +4011,8 @@ mod tests { use super::{ CustomRolePermissions, caller_manage_grant_role_check, filter_ciphers_for_organization, - may_change_group_membership, may_change_member_type, may_export_entire_organization, + may_change_group_membership, may_change_member_type, may_export_entire_organization, may_manage_member_type, + may_manage_stored_member_type, }; use crate::db::models::{Cipher, Membership, MembershipStatus, MembershipType, OrganizationId}; @@ -4072,13 +4119,39 @@ mod tests { assert!(may_change_member_type(MembershipType::Custom, custom, MembershipType::Custom)); // REGRESSION (privilege escalation, PR #7397 / finding F1): a caller below Admin must NOT - // 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. + // be able to change a member's role. Promoting User -> Custom can activate explicit + // collection-Manage assignments and Custom-only authorization paths; demoting revokes + // them. A manage_users caller is not entitled to either authority change. assert!(!may_change_member_type(MembershipType::Custom, user, MembershipType::Custom)); assert!(!may_change_member_type(MembershipType::Custom, custom, MembershipType::User)); } + #[test] + fn member_lifecycle_permissions_follow_the_role_hierarchy() { + let roles = [MembershipType::Owner, MembershipType::Admin, MembershipType::Custom, MembershipType::User]; + + for target in roles { + assert!(may_manage_member_type(MembershipType::Owner, target)); + } + + assert!(!may_manage_member_type(MembershipType::Admin, MembershipType::Owner)); + assert!(may_manage_member_type(MembershipType::Admin, MembershipType::Admin)); + assert!(may_manage_member_type(MembershipType::Admin, MembershipType::Custom)); + assert!(may_manage_member_type(MembershipType::Admin, MembershipType::User)); + + assert!(!may_manage_member_type(MembershipType::Custom, MembershipType::Owner)); + assert!(!may_manage_member_type(MembershipType::Custom, MembershipType::Admin)); + assert!(!may_manage_member_type(MembershipType::Custom, MembershipType::Custom)); + assert!(may_manage_member_type(MembershipType::Custom, MembershipType::User)); + + for target in roles { + assert!(!may_manage_member_type(MembershipType::User, target)); + } + + assert!(may_manage_stored_member_type(MembershipType::Admin, MembershipType::Custom as i32)); + assert!(!may_manage_stored_member_type(MembershipType::Owner, i32::MAX)); + } + #[test] fn manage_groups_caller_cannot_grant_collection_access_via_groups() { // A caller who can manage collections may change membership of any group. @@ -4141,7 +4214,7 @@ mod tests { assert!(custom.delete_any_collection); assert!(custom.access_event_logs); assert!(custom.access_import_export); - assert!(custom.access_reports); + assert!(!custom.access_reports, "unsupported report access must remain fail-closed"); let user = CustomRolePermissions::from_request(MembershipType::User, &permissions); assert_eq!(user, CustomRolePermissions::default()); @@ -4164,7 +4237,6 @@ mod tests { delete_any_collection: true, access_event_logs: true, access_import_export: true, - access_reports: true, ..CustomRolePermissions::default() }; @@ -4176,6 +4248,45 @@ mod tests { assert!(membership.delete_any_collection); assert!(membership.access_event_logs); assert!(membership.access_import_export); - assert!(membership.access_reports); + assert!(!membership.access_reports); + } + + #[test] + fn omitted_edit_permissions_preserve_supported_custom_grants() { + let mut membership = confirmed_member(MembershipType::Custom); + membership.manage_users = true; + membership.manage_groups = true; + membership.manage_policies = true; + membership.create_new_collections = true; + membership.edit_any_collection = true; + membership.delete_any_collection = true; + membership.access_event_logs = true; + membership.access_import_export = true; + membership.access_reports = true; + + let preserved = CustomRolePermissions::from_edit_request(MembershipType::Custom, None, &membership); + assert!(preserved.manage_users); + assert!(preserved.manage_groups); + assert!(preserved.manage_policies); + assert!(preserved.create_new_collections); + assert!(preserved.edit_any_collection); + assert!(preserved.delete_any_collection); + assert!(preserved.access_event_logs); + assert!(preserved.access_import_export); + assert!(!preserved.access_reports); + assert!( + !preserved.differs_from(&membership), + "a stale unsupported reports bit must not block an otherwise unchanged legacy-client update" + ); + + let explicit_reset = HashMap::new(); + assert_eq!( + CustomRolePermissions::from_edit_request(MembershipType::Custom, Some(&explicit_reset), &membership), + CustomRolePermissions::default() + ); + assert_eq!( + CustomRolePermissions::from_edit_request(MembershipType::User, None, &membership), + CustomRolePermissions::default() + ); } } diff --git a/src/db/mod.rs b/src/db/mod.rs index 2eae3f3c..4f57147b 100644 --- a/src/db/mod.rs +++ b/src/db/mod.rs @@ -468,6 +468,169 @@ impl<'r> FromRequest<'r> for DbConn { } } +const CUSTOM_ROLE_REPAIR_MIGRATION: &str = "20260723120000"; +const CUSTOM_COLLECTION_PERMISSIONS_MIGRATION: &str = "20260716120000"; +const DROP_MEMBERSHIP_ACCESS_ALL_MIGRATION: &str = "20260724120000"; +const CUSTOM_ROLE_SAME_RUN_MARKER_TABLE: &str = "__vw_custom_role_same_run_0716"; +const CUSTOM_ROLE_MIGRATION_RECOVERY_DOC: &str = "docs/custom-role-migration-recovery.md"; + +#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] +#[expect( + clippy::struct_excessive_bools, + reason = "These are independent facts read from a historical database schema and migration ledger" +)] +struct CustomRoleMigrationFacts { + memberships_table_exists: bool, + migration_table_exists: bool, + access_all_column_exists: bool, + collection_permission_columns: i64, + collection_permissions_migration_applied: bool, + repair_migration_applied: bool, + access_all_drop_migration_applied: bool, + legacy_user_access_all_count: i64, + ambiguous_direct_permission_count: i64, + same_run_0716_marker: bool, +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum CustomRolePreflightDecision { + Proceed, + CompleteMysqlCollectionMigration, + RefuseAlreadyDropped, + RefuseMissingAccessAll, + RefuseMissingMigrationLedger, + RefuseLegacyUserAccessAll, + RefuseAmbiguousDirectPermissions, + RefusePartialCollectionSchema, + RefuseCollectionLedgerMismatch, +} + +fn custom_role_preflight_decision( + facts: CustomRoleMigrationFacts, + can_complete_mysql_partial_migration: bool, +) -> CustomRolePreflightDecision { + if !facts.memberships_table_exists || facts.repair_migration_applied { + return CustomRolePreflightDecision::Proceed; + } + if !facts.migration_table_exists { + return CustomRolePreflightDecision::RefuseMissingMigrationLedger; + } + + // Once access_all has been dropped, its former value and the provenance of 0/1/1 + // collection permissions can no longer be reconstructed. Never guess at either. + if facts.access_all_drop_migration_applied { + return CustomRolePreflightDecision::RefuseAlreadyDropped; + } + if !facts.access_all_column_exists { + return CustomRolePreflightDecision::RefuseMissingAccessAll; + } + + if facts.legacy_user_access_all_count != 0 { + return CustomRolePreflightDecision::RefuseLegacyUserAccessAll; + } + if facts.ambiguous_direct_permission_count != 0 && !facts.same_run_0716_marker { + return CustomRolePreflightDecision::RefuseAmbiguousDirectPermissions; + } + + match (facts.collection_permission_columns, facts.collection_permissions_migration_applied) { + (0, false) | (3, true) => CustomRolePreflightDecision::Proceed, + (3, false) if can_complete_mysql_partial_migration => { + CustomRolePreflightDecision::CompleteMysqlCollectionMigration + } + (_, true) => CustomRolePreflightDecision::RefuseCollectionLedgerMismatch, + _ => CustomRolePreflightDecision::RefusePartialCollectionSchema, + } +} + +fn custom_role_preflight_error(decision: CustomRolePreflightDecision, facts: CustomRoleMigrationFacts) -> Error { + let detail = match decision { + CustomRolePreflightDecision::RefuseAlreadyDropped => format!( + "The membership access_all column was already dropped by migration \ + {DROP_MEMBERSHIP_ACCESS_ALL_MIGRATION}, but the required repair migration \ + {CUSTOM_ROLE_REPAIR_MIGRATION} is not recorded. The former permission values cannot \ + be reconstructed safely." + ), + CustomRolePreflightDecision::RefuseMissingAccessAll => format!( + "The membership access_all column is missing before repair migration \ + {CUSTOM_ROLE_REPAIR_MIGRATION}; refusing to infer deleted permissions." + ), + CustomRolePreflightDecision::RefuseMissingMigrationLedger => { + "The users_organizations table exists, but the Diesel migration ledger does not. \ + Refusing to guess which schema and data migrations were previously applied." + .to_owned() + } + CustomRolePreflightDecision::RefuseLegacyUserAccessAll => format!( + "{} legacy User membership(s) still have membership access_all=true. Mapping these \ + records to Custom/EditAny would add management authority, while clearing the bit \ + would remove existing vault access.", + facts.legacy_user_access_all_count + ), + CustomRolePreflightDecision::RefuseAmbiguousDirectPermissions => format!( + "Found {} membership(s) with an ambiguous 0/1/1 collection-permission pattern. It is \ + not possible to distinguish an older group-derived backfill from an intentional \ + direct Edit+Delete assignment.", + facts.ambiguous_direct_permission_count + ), + CustomRolePreflightDecision::RefusePartialCollectionSchema => format!( + "Found {} of the three custom collection-permission columns without a completed \ + {CUSTOM_COLLECTION_PERMISSIONS_MIGRATION} migration. This is not an automatically \ + recoverable state for this database backend.", + facts.collection_permission_columns + ), + CustomRolePreflightDecision::RefuseCollectionLedgerMismatch => format!( + "Migration {CUSTOM_COLLECTION_PERMISSIONS_MIGRATION} is recorded, but only {} of its \ + three collection-permission columns exist.", + facts.collection_permission_columns + ), + CustomRolePreflightDecision::Proceed | CustomRolePreflightDecision::CompleteMysqlCollectionMigration => { + unreachable!("successful preflight decisions do not produce errors") + } + }; + + std::io::Error::other(format!( + "Custom-role migration preflight stopped startup: {detail} Back up the database and follow \ + {CUSTOM_ROLE_MIGRATION_RECOVERY_DOC}." + )) + .into() +} + +#[cfg(any(mysql, test))] +fn mysql_partial_unexpected_values_query(allow_same_run_group_derived: bool) -> String { + let same_run_group_derived = if allow_same_run_group_derived { + " OR \ + (atype = 4 \ + AND access_all = FALSE \ + AND create_new_collections = FALSE \ + AND edit_any_collection = TRUE \ + AND delete_any_collection = TRUE \ + AND EXISTS ( \ + SELECT 1 \ + FROM groups_users AS gu \ + INNER JOIN `groups` AS g ON g.uuid = gu.groups_uuid \ + WHERE gu.users_organizations_uuid = users_organizations.uuid \ + AND g.organizations_uuid = users_organizations.org_uuid \ + AND g.access_all = TRUE \ + ))" + } else { + "" + }; + + format!( + "SELECT COUNT(*) AS count FROM users_organizations \ + WHERE NOT ( \ + (create_new_collections = FALSE \ + AND edit_any_collection = FALSE \ + AND delete_any_collection = FALSE) \ + OR \ + (atype = 4 \ + AND create_new_collections = access_all \ + AND edit_any_collection = access_all \ + AND delete_any_collection = access_all) \ + {same_run_group_derived} \ + )" + ) +} + // Embed the migrations from the migrations folder into the application // This way, the program automatically migrates the database to the latest version // https://docs.rs/diesel_migrations/*/diesel_migrations/macro.embed_migrations.html @@ -477,11 +640,130 @@ mod sqlite_migrations { use diesel_migrations::{EmbeddedMigrations, MigrationHarness}; pub const MIGRATIONS: EmbeddedMigrations = embed_migrations!("migrations/sqlite"); + #[derive(diesel::QueryableByName)] + struct Count { + #[diesel(sql_type = diesel::sql_types::BigInt)] + count: i64, + } + + fn count( + connection: &mut diesel::sqlite::SqliteConnection, + query: impl Into, + ) -> Result { + diesel::sql_query(query).get_result::(connection).map(|row| row.count) + } + + fn table_exists( + connection: &mut diesel::sqlite::SqliteConnection, + table: &str, + ) -> Result { + count( + connection, + format!( + "SELECT COUNT(*) AS count FROM sqlite_master \ + WHERE type = 'table' AND name = '{table}'" + ), + ) + .map(|value| value != 0) + } + + fn migration_applied( + connection: &mut diesel::sqlite::SqliteConnection, + version: &str, + ) -> Result { + count( + connection, + format!( + "SELECT COUNT(*) AS count FROM __diesel_schema_migrations \ + WHERE version = '{version}'" + ), + ) + .map(|value| value != 0) + } + + fn preflight(connection: &mut diesel::sqlite::SqliteConnection) -> Result<(), super::Error> { + let memberships_table_exists = table_exists(connection, "users_organizations")?; + if !memberships_table_exists { + return Ok(()); + } + + let migration_table_exists = table_exists(connection, "__diesel_schema_migrations")?; + let access_all_column_exists = count( + connection, + "SELECT COUNT(*) AS count FROM pragma_table_info('users_organizations') \ + WHERE name = 'access_all'", + )? != 0; + let collection_permission_columns = count( + connection, + "SELECT COUNT(*) AS count FROM pragma_table_info('users_organizations') \ + WHERE name IN ('create_new_collections', 'edit_any_collection', 'delete_any_collection')", + )?; + + let collection_permissions_migration_applied = + migration_table_exists && migration_applied(connection, super::CUSTOM_COLLECTION_PERMISSIONS_MIGRATION)?; + let repair_migration_applied = + migration_table_exists && migration_applied(connection, super::CUSTOM_ROLE_REPAIR_MIGRATION)?; + let access_all_drop_migration_applied = + migration_table_exists && migration_applied(connection, super::DROP_MEMBERSHIP_ACCESS_ALL_MIGRATION)?; + let same_run_marker_table_exists = table_exists(connection, super::CUSTOM_ROLE_SAME_RUN_MARKER_TABLE)?; + let same_run_0716_marker = same_run_marker_table_exists + && count( + connection, + format!("SELECT COUNT(*) AS count FROM {} WHERE marker = 1", super::CUSTOM_ROLE_SAME_RUN_MARKER_TABLE), + )? != 0; + + let legacy_user_access_all_count = if access_all_column_exists { + count( + connection, + "SELECT COUNT(*) AS count FROM users_organizations \ + WHERE atype = 2 AND access_all = TRUE", + )? + } else { + 0 + }; + + let ambiguous_direct_permission_count = if access_all_column_exists && collection_permission_columns == 3 { + count( + connection, + "SELECT COUNT(*) AS count FROM users_organizations \ + WHERE atype IN (3, 4) \ + AND access_all = FALSE \ + AND create_new_collections = FALSE \ + AND edit_any_collection = TRUE \ + AND delete_any_collection = TRUE", + )? + } else { + 0 + }; + + let facts = super::CustomRoleMigrationFacts { + memberships_table_exists, + migration_table_exists, + access_all_column_exists, + collection_permission_columns, + collection_permissions_migration_applied, + repair_migration_applied, + access_all_drop_migration_applied, + legacy_user_access_all_count, + ambiguous_direct_permission_count, + same_run_0716_marker, + }; + + let decision = super::custom_role_preflight_decision(facts, false); + if decision == super::CustomRolePreflightDecision::Proceed { + Ok(()) + } else { + Err(super::custom_role_preflight_error(decision, facts)) + } + } + pub fn run_migrations(db_url: &str) -> Result<(), super::Error> { // Establish a connection to the sqlite database (this will create a new one, if it does // not exist, and exit if there is an error). let mut connection = diesel::sqlite::SqliteConnection::establish(db_url)?; + preflight(&mut connection)?; + // Run the migrations after successfully establishing a connection // Disable Foreign Key Checks during migration // Scoped to a connection. @@ -505,10 +787,194 @@ mod mysql_migrations { use diesel_migrations::{EmbeddedMigrations, MigrationHarness}; pub const MIGRATIONS: EmbeddedMigrations = embed_migrations!("migrations/mysql"); + #[derive(diesel::QueryableByName)] + struct Count { + #[diesel(sql_type = diesel::sql_types::BigInt)] + count: i64, + } + + fn count( + connection: &mut diesel::mysql::MysqlConnection, + query: impl Into, + ) -> Result { + diesel::sql_query(query).get_result::(connection).map(|row| row.count) + } + + fn table_exists( + connection: &mut diesel::mysql::MysqlConnection, + table: &str, + ) -> Result { + count( + connection, + format!( + "SELECT COUNT(*) AS count FROM information_schema.tables \ + WHERE table_schema = DATABASE() AND table_name = '{table}'" + ), + ) + .map(|value| value != 0) + } + + fn migration_applied( + connection: &mut diesel::mysql::MysqlConnection, + version: &str, + ) -> Result { + count( + connection, + format!( + "SELECT COUNT(*) AS count FROM __diesel_schema_migrations \ + WHERE version = '{version}'" + ), + ) + .map(|value| value != 0) + } + + fn complete_partial_collection_migration( + connection: &mut diesel::mysql::MysqlConnection, + allow_same_run_group_derived: bool, + ) -> Result<(), super::Error> { + // MySQL implicitly committed the three historical ALTER TABLE statements before the + // unquoted `groups` identifier made the migration fail. Complete that exact, known state + // without dropping columns or inventing values. + let matching_column_definitions = count( + connection, + "SELECT COUNT(*) AS count FROM information_schema.columns \ + WHERE table_schema = DATABASE() \ + AND table_name = 'users_organizations' \ + AND column_name IN \ + ('create_new_collections', 'edit_any_collection', 'delete_any_collection') \ + AND data_type = 'tinyint' \ + AND is_nullable = 'NO' \ + AND LOWER(COALESCE(CAST(column_default AS CHAR), '')) IN ('0', 'false')", + )?; + let unexpected_values = + count(connection, super::mysql_partial_unexpected_values_query(allow_same_run_group_derived))?; + + if matching_column_definitions != 3 || unexpected_values != 0 { + return Err(std::io::Error::other(format!( + "Custom-role migration preflight found the historical MySQL partial \ + {version} schema, but its column definitions or data were modified \ + (matching columns: {matching_column_definitions}/3, unexpected rows: \ + {unexpected_values}). Refusing automatic recovery. Back up the database and \ + follow {doc}.", + version = super::CUSTOM_COLLECTION_PERMISSIONS_MIGRATION, + doc = super::CUSTOM_ROLE_MIGRATION_RECOVERY_DOC, + )) + .into()); + } + + connection.transaction::<(), diesel::result::Error, _>(|connection| { + // This is the first data statement from the canonical migration. It also resets an + // exact, same-run group-derived 0/1/1 row to 0/0/0; that authority remains dynamically + // derived from the group, and the separate 07-23 repair then reconciles the role. + diesel::sql_query( + "UPDATE users_organizations \ + SET create_new_collections = access_all, \ + edit_any_collection = access_all, \ + delete_any_collection = access_all \ + WHERE atype = 4", + ) + .execute(connection)?; + + diesel::sql_query(format!( + "INSERT INTO __diesel_schema_migrations (version) \ + VALUES ('{}')", + super::CUSTOM_COLLECTION_PERMISSIONS_MIGRATION + )) + .execute(connection)?; + Ok(()) + })?; + + Ok(()) + } + + fn preflight(connection: &mut diesel::mysql::MysqlConnection) -> Result<(), super::Error> { + let memberships_table_exists = table_exists(connection, "users_organizations")?; + if !memberships_table_exists { + return Ok(()); + } + + let migration_table_exists = table_exists(connection, "__diesel_schema_migrations")?; + let access_all_column_exists = count( + connection, + "SELECT COUNT(*) AS count FROM information_schema.columns \ + WHERE table_schema = DATABASE() \ + AND table_name = 'users_organizations' \ + AND column_name = 'access_all'", + )? != 0; + let collection_permission_columns = count( + connection, + "SELECT COUNT(*) AS count FROM information_schema.columns \ + WHERE table_schema = DATABASE() \ + AND table_name = 'users_organizations' \ + AND column_name IN \ + ('create_new_collections', 'edit_any_collection', 'delete_any_collection')", + )?; + + let collection_permissions_migration_applied = + migration_table_exists && migration_applied(connection, super::CUSTOM_COLLECTION_PERMISSIONS_MIGRATION)?; + let repair_migration_applied = + migration_table_exists && migration_applied(connection, super::CUSTOM_ROLE_REPAIR_MIGRATION)?; + let access_all_drop_migration_applied = + migration_table_exists && migration_applied(connection, super::DROP_MEMBERSHIP_ACCESS_ALL_MIGRATION)?; + let same_run_marker_table_exists = table_exists(connection, super::CUSTOM_ROLE_SAME_RUN_MARKER_TABLE)?; + let same_run_0716_marker = same_run_marker_table_exists + && count( + connection, + format!("SELECT COUNT(*) AS count FROM {} WHERE marker = 1", super::CUSTOM_ROLE_SAME_RUN_MARKER_TABLE), + )? != 0; + + let legacy_user_access_all_count = if access_all_column_exists { + count( + connection, + "SELECT COUNT(*) AS count FROM users_organizations \ + WHERE atype = 2 AND access_all = TRUE", + )? + } else { + 0 + }; + + let ambiguous_direct_permission_count = if access_all_column_exists && collection_permission_columns == 3 { + count( + connection, + "SELECT COUNT(*) AS count FROM users_organizations \ + WHERE atype IN (3, 4) \ + AND access_all = FALSE \ + AND create_new_collections = FALSE \ + AND edit_any_collection = TRUE \ + AND delete_any_collection = TRUE", + )? + } else { + 0 + }; + + let facts = super::CustomRoleMigrationFacts { + memberships_table_exists, + migration_table_exists, + access_all_column_exists, + collection_permission_columns, + collection_permissions_migration_applied, + repair_migration_applied, + access_all_drop_migration_applied, + legacy_user_access_all_count, + ambiguous_direct_permission_count, + same_run_0716_marker, + }; + + match super::custom_role_preflight_decision(facts, true) { + super::CustomRolePreflightDecision::Proceed => Ok(()), + super::CustomRolePreflightDecision::CompleteMysqlCollectionMigration => { + complete_partial_collection_migration(connection, same_run_0716_marker) + } + decision => Err(super::custom_role_preflight_error(decision, facts)), + } + } + pub fn run_migrations(db_url: &str) -> Result<(), super::Error> { // Make sure the database is up to date (create if it doesn't exist, or run the migrations) let mut connection = diesel::mysql::MysqlConnection::establish(db_url)?; + preflight(&mut connection)?; + // Disable Foreign Key Checks during migration // Scoped to a connection/session. diesel::sql_query("SET FOREIGN_KEY_CHECKS = 0") @@ -522,15 +988,315 @@ mod mysql_migrations { #[cfg(postgresql)] mod postgresql_migrations { - use diesel::Connection; + use diesel::{Connection, RunQueryDsl}; use diesel_migrations::{EmbeddedMigrations, MigrationHarness}; pub const MIGRATIONS: EmbeddedMigrations = embed_migrations!("migrations/postgresql"); + #[derive(diesel::QueryableByName)] + struct Count { + #[diesel(sql_type = diesel::sql_types::BigInt)] + count: i64, + } + + fn count( + connection: &mut diesel::pg::PgConnection, + query: impl Into, + ) -> Result { + diesel::sql_query(query).get_result::(connection).map(|row| row.count) + } + + fn table_exists(connection: &mut diesel::pg::PgConnection, table: &str) -> Result { + count( + connection, + format!( + "SELECT COUNT(*) AS count FROM information_schema.tables \ + WHERE table_schema = current_schema() AND table_name = '{table}'" + ), + ) + .map(|value| value != 0) + } + + fn migration_applied( + connection: &mut diesel::pg::PgConnection, + version: &str, + ) -> Result { + count( + connection, + format!( + "SELECT COUNT(*) AS count FROM __diesel_schema_migrations \ + WHERE version = '{version}'" + ), + ) + .map(|value| value != 0) + } + + fn preflight(connection: &mut diesel::pg::PgConnection) -> Result<(), super::Error> { + let memberships_table_exists = table_exists(connection, "users_organizations")?; + if !memberships_table_exists { + return Ok(()); + } + + let migration_table_exists = table_exists(connection, "__diesel_schema_migrations")?; + let access_all_column_exists = count( + connection, + "SELECT COUNT(*) AS count FROM information_schema.columns \ + WHERE table_schema = current_schema() \ + AND table_name = 'users_organizations' \ + AND column_name = 'access_all'", + )? != 0; + let collection_permission_columns = count( + connection, + "SELECT COUNT(*) AS count FROM information_schema.columns \ + WHERE table_schema = current_schema() \ + AND table_name = 'users_organizations' \ + AND column_name IN \ + ('create_new_collections', 'edit_any_collection', 'delete_any_collection')", + )?; + + let collection_permissions_migration_applied = + migration_table_exists && migration_applied(connection, super::CUSTOM_COLLECTION_PERMISSIONS_MIGRATION)?; + let repair_migration_applied = + migration_table_exists && migration_applied(connection, super::CUSTOM_ROLE_REPAIR_MIGRATION)?; + let access_all_drop_migration_applied = + migration_table_exists && migration_applied(connection, super::DROP_MEMBERSHIP_ACCESS_ALL_MIGRATION)?; + let same_run_marker_table_exists = table_exists(connection, super::CUSTOM_ROLE_SAME_RUN_MARKER_TABLE)?; + let same_run_0716_marker = same_run_marker_table_exists + && count( + connection, + format!("SELECT COUNT(*) AS count FROM {} WHERE marker = 1", super::CUSTOM_ROLE_SAME_RUN_MARKER_TABLE), + )? != 0; + + let legacy_user_access_all_count = if access_all_column_exists { + count( + connection, + "SELECT COUNT(*) AS count FROM users_organizations \ + WHERE atype = 2 AND access_all = TRUE", + )? + } else { + 0 + }; + + let ambiguous_direct_permission_count = if access_all_column_exists && collection_permission_columns == 3 { + count( + connection, + "SELECT COUNT(*) AS count FROM users_organizations \ + WHERE atype IN (3, 4) \ + AND access_all = FALSE \ + AND create_new_collections = FALSE \ + AND edit_any_collection = TRUE \ + AND delete_any_collection = TRUE", + )? + } else { + 0 + }; + + let facts = super::CustomRoleMigrationFacts { + memberships_table_exists, + migration_table_exists, + access_all_column_exists, + collection_permission_columns, + collection_permissions_migration_applied, + repair_migration_applied, + access_all_drop_migration_applied, + legacy_user_access_all_count, + ambiguous_direct_permission_count, + same_run_0716_marker, + }; + + let decision = super::custom_role_preflight_decision(facts, false); + if decision == super::CustomRolePreflightDecision::Proceed { + Ok(()) + } else { + Err(super::custom_role_preflight_error(decision, facts)) + } + } + pub fn run_migrations(db_url: &str) -> Result<(), super::Error> { // Make sure the database is up to date (create if it doesn't exist, or run the migrations) let mut connection = diesel::pg::PgConnection::establish(db_url)?; + preflight(&mut connection)?; + connection.run_pending_migrations(MIGRATIONS).expect("Error running migrations"); Ok(()) } } + +#[cfg(test)] +mod custom_role_migration_preflight_tests { + use super::{ + CustomRoleMigrationFacts as Facts, CustomRolePreflightDecision as Decision, custom_role_preflight_decision, + mysql_partial_unexpected_values_query, + }; + + fn pending_repair() -> Facts { + Facts { + memberships_table_exists: true, + migration_table_exists: true, + access_all_column_exists: true, + ..Facts::default() + } + } + + #[test] + fn empty_database_can_run_normal_migrations() { + assert_eq!(custom_role_preflight_decision(Facts::default(), false), Decision::Proceed); + } + + #[test] + fn existing_schema_without_a_ledger_is_not_guessed() { + assert_eq!( + custom_role_preflight_decision( + Facts { + memberships_table_exists: true, + access_all_column_exists: true, + ..Facts::default() + }, + false, + ), + Decision::RefuseMissingMigrationLedger + ); + } + + #[test] + fn repair_marker_makes_completed_state_idempotent() { + assert_eq!( + custom_role_preflight_decision( + Facts { + memberships_table_exists: true, + migration_table_exists: true, + repair_migration_applied: true, + access_all_drop_migration_applied: true, + collection_permission_columns: 3, + ..Facts::default() + }, + false, + ), + Decision::Proceed + ); + } + + #[test] + fn a_historical_drop_without_the_repair_is_refused() { + assert_eq!( + custom_role_preflight_decision( + Facts { + access_all_drop_migration_applied: true, + access_all_column_exists: false, + ..pending_repair() + }, + false, + ), + Decision::RefuseAlreadyDropped + ); + } + + #[test] + fn legacy_user_access_all_requires_an_operator_decision() { + assert_eq!( + custom_role_preflight_decision( + Facts { + legacy_user_access_all_count: 1, + ..pending_repair() + }, + false, + ), + Decision::RefuseLegacyUserAccessAll + ); + } + + #[test] + fn group_derived_zero_permissions_are_safe_but_ambiguous_direct_permissions_are_refused() { + assert_eq!(custom_role_preflight_decision(pending_repair(), false), Decision::Proceed); + assert_eq!( + custom_role_preflight_decision( + Facts { + collection_permission_columns: 3, + collection_permissions_migration_applied: true, + ..pending_repair() + }, + false, + ), + Decision::Proceed + ); + assert_eq!( + custom_role_preflight_decision( + Facts { + collection_permission_columns: 3, + collection_permissions_migration_applied: true, + ambiguous_direct_permission_count: 1, + ..pending_repair() + }, + false, + ), + Decision::RefuseAmbiguousDirectPermissions + ); + assert_eq!( + custom_role_preflight_decision( + Facts { + collection_permission_columns: 3, + collection_permissions_migration_applied: true, + ambiguous_direct_permission_count: 1, + same_run_0716_marker: true, + ..pending_repair() + }, + false, + ), + Decision::Proceed + ); + } + + #[test] + fn exact_mysql_partial_schema_uses_only_the_mysql_completion_path() { + let facts = Facts { + collection_permission_columns: 3, + collection_permissions_migration_applied: false, + ..pending_repair() + }; + assert_eq!(custom_role_preflight_decision(facts, true), Decision::CompleteMysqlCollectionMigration); + assert_eq!(custom_role_preflight_decision(facts, false), Decision::RefusePartialCollectionSchema); + } + + #[test] + fn historical_mysql_partial_query_does_not_require_the_new_marker_table() { + let query = mysql_partial_unexpected_values_query(false); + assert!(!query.contains(super::CUSTOM_ROLE_SAME_RUN_MARKER_TABLE)); + assert!(!query.contains("groups_users")); + } + + #[test] + fn same_run_mysql_partial_query_requires_the_current_group_source() { + let query = mysql_partial_unexpected_values_query(true); + assert!(query.contains("access_all = FALSE")); + assert!(query.contains("edit_any_collection = TRUE")); + assert!(query.contains("delete_any_collection = TRUE")); + assert!(query.contains("INNER JOIN `groups` AS g")); + assert!(query.contains("g.organizations_uuid = users_organizations.org_uuid")); + assert!(query.contains("g.access_all = TRUE")); + } + + #[test] + fn incomplete_columns_and_ledger_mismatch_are_refused() { + assert_eq!( + custom_role_preflight_decision( + Facts { + collection_permission_columns: 2, + ..pending_repair() + }, + true, + ), + Decision::RefusePartialCollectionSchema + ); + assert_eq!( + custom_role_preflight_decision( + Facts { + collection_permission_columns: 2, + collection_permissions_migration_applied: true, + ..pending_repair() + }, + true, + ), + Decision::RefuseCollectionLedgerMismatch + ); + } +} diff --git a/src/db/models/collection.rs b/src/db/models/collection.rs index 6983a4f2..17dfa090 100644 --- a/src/db/models/collection.rs +++ b/src/db/models/collection.rs @@ -1,5 +1,6 @@ use derive_more::{AsRef, Deref, Display, From}; use diesel::prelude::*; +use num_traits::FromPrimitive; use serde_json::Value; use crate::{ @@ -52,6 +53,16 @@ pub struct CollectionCipher { pub collection_uuid: CollectionId, } +/// Serialize the assignment-level `manage` capability using the same role boundary as the +/// collection mutation guards. Read/write access is deliberately not management authority. +pub(super) fn assignment_manage_for_member(membership_type: i32, stored_manage: bool) -> bool { + match MembershipType::from_i32(membership_type) { + Some(MembershipType::Owner | MembershipType::Admin) => true, + Some(MembershipType::Custom) => stored_manage, + Some(MembershipType::User) | None => false, + } +} + /// Local methods impl Collection { pub fn new(org_uuid: OrganizationId, name: String, external_id: Option) -> Self { @@ -104,25 +115,14 @@ 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-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), + // Full collection visibility is not collection-management authority. Admins and + // Owners manage implicitly; Custom members still need an explicit stored grant. + Some(m) if m.has_full_access() => (false, false, assignment_manage_for_member(m.atype, false)), Some(m) => { - // Only let a manager-level (Custom) member manage collections - // when they have full read/write access - let is_manager = m.atype >= MembershipType::Custom; if let Some(cu) = cipher_sync_data.user_collections.get(&self.uuid) { - ( - cu.read_only, - cu.hide_passwords, - is_manager && (cu.manage || (!cu.read_only && !cu.hide_passwords)), - ) + (cu.read_only, cu.hide_passwords, assignment_manage_for_member(m.atype, cu.manage)) } else if let Some(cg) = cipher_sync_data.user_collections_groups.get(&self.uuid) { - ( - cg.read_only, - cg.hide_passwords, - is_manager && (cg.manage || (!cg.read_only && !cg.hide_passwords)), - ) + (cg.read_only, cg.hide_passwords, assignment_manage_for_member(m.atype, cg.manage)) } else { (false, false, false) } @@ -131,15 +131,17 @@ 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::Custom), - Some(m) if m.atype >= MembershipType::Custom && self.is_manageable_by_user(user_uuid, conn).await => { + Some(m) if m.has_full_access() => (false, false, assignment_manage_for_member(m.atype, false)), + Some(m) + if m.atype >= MembershipType::Custom + && m.has_explicit_collection_manage_access(&self.uuid, conn).await => + { (false, false, true) } - Some(m) => { - let is_manager = m.atype >= MembershipType::Custom; + Some(_) => { 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) + (read_only, hide_passwords, false) } _ => (true, true, false), } @@ -576,71 +578,8 @@ impl Collection { .await } - pub async fn is_coll_manageable_by_user(uuid: &CollectionId, user_uuid: &UserId, conn: &DbConn) -> bool { - let uuid = uuid.to_string(); - let user_uuid = user_uuid.to_string(); - conn.run(move |conn| { - collections::table - .left_join( - users_collections::table.on(users_collections::collection_uuid - .eq(collections::uuid) - .and(users_collections::user_uuid.eq(user_uuid.clone()))), - ) - .left_join( - users_organizations::table.on(collections::org_uuid - .eq(users_organizations::org_uuid) - .and(users_organizations::user_uuid.eq(user_uuid))), - ) - .left_join(groups_users::table.on(groups_users::users_organizations_uuid.eq(users_organizations::uuid))) - .left_join( - groups::table.on(groups::uuid - .eq(groups_users::groups_uuid) - .and(groups::organizations_uuid.eq(users_organizations::org_uuid))), - ) - .left_join( - collections_groups::table.on(collections_groups::groups_uuid - .eq(groups_users::groups_uuid) - .and(collections_groups::collections_uuid.eq(collections::uuid))), - ) - .filter(collections::uuid.eq(&uuid)) - .filter( - users_collections::collection_uuid - .eq(&uuid) - .and(users_collections::manage.eq(true)) - .or( - // Directly accessed collection - 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 - ), - ) - .or( - groups::access_all.eq(true), // access_all in groups - ) - .or( - // access via groups - groups_users::users_organizations_uuid.eq(users_organizations::uuid).and( - collections_groups::collections_uuid - .is_not_null() - .and(collections_groups::manage.eq(true)), - ), - ), - ) - .count() - .first::(conn) - .ok() - .unwrap_or(0) - != 0 - }) - .await - } - - pub async fn is_manageable_by_user(&self, user_uuid: &UserId, conn: &DbConn) -> bool { - Self::is_coll_manageable_by_user(&self.uuid, user_uuid, conn).await - } - // Whether the user has manage access to at least one collection in the org, directly or via a - // group. Org-scoped counterpart of is_coll_manageable_by_user. + // group. pub async fn has_manageable_collection_by_user( org_uuid: &OrganizationId, user_uuid: &UserId, @@ -667,6 +606,8 @@ impl Collection { .and(collections_groups::collections_uuid.eq(collections::uuid))), ) .filter(collections::org_uuid.eq(&org_uuid)) + .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) + .filter(users_organizations::atype.eq(MembershipType::Custom as i32)) .filter( // Manage permission on a collection assigned directly or via a group. users_collections::manage.eq(true).or(collections_groups::manage.eq(true)), @@ -999,11 +940,7 @@ impl CollectionMembership { "id": self.membership_uuid, "readOnly": self.read_only, "hidePasswords": self.hide_passwords, - "manage": membership_type >= MembershipType::Admin - || self.manage - || (membership_type >= MembershipType::Custom - && !self.read_only - && !self.hide_passwords), + "manage": assignment_manage_for_member(membership_type, self.manage), }) } } @@ -1037,3 +974,21 @@ impl From for CollectionMembership { UuidFromParam, )] pub struct CollectionId(String); + +#[cfg(test)] +mod tests { + use super::assignment_manage_for_member; + use crate::db::models::MembershipType; + + #[test] + fn assignment_manage_matches_collection_guard_role_boundaries() { + for role in [MembershipType::Owner, MembershipType::Admin] { + assert!(assignment_manage_for_member(role as i32, false)); + } + + assert!(assignment_manage_for_member(MembershipType::Custom as i32, true)); + assert!(!assignment_manage_for_member(MembershipType::Custom as i32, false)); + assert!(!assignment_manage_for_member(MembershipType::User as i32, true)); + assert!(!assignment_manage_for_member(i32::MAX, true)); + } +} diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index 066f3574..410ba7d0 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -25,7 +25,7 @@ use macros::UuidFromParam; use super::{ Cipher, CipherId, Collection, CollectionGroup, CollectionId, CollectionUser, Group, GroupId, GroupUser, OrgPolicy, - OrgPolicyType, TwoFactor, User, UserId, + OrgPolicyType, TwoFactor, User, UserId, collection::assignment_manage_for_member as assignment_manage, }; #[derive(Identifiable, Queryable, Insertable, AsChangeset)] @@ -469,7 +469,9 @@ impl Membership { let permissions = json!({ "accessEventLogs": membership_type == MembershipType::Custom as i32 && self.access_event_logs, "accessImportExport": membership_type == MembershipType::Custom as i32 && self.access_import_export, - "accessReports": membership_type == MembershipType::Custom as i32 && self.access_reports, + // Reports are not implemented server-side. Advertising a stored bit as usable + // would make the permission contract misleading, so this stays fail-closed. + "accessReports": false, "createNewCollections": membership_type == MembershipType::Custom as i32 && self.create_new_collections, "editAnyCollection": membership_type == MembershipType::Custom as i32 && self.edit_any_collection, "deleteAnyCollection": membership_type == MembershipType::Custom as i32 && self.delete_any_collection, @@ -584,55 +586,50 @@ 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.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) + 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) + .await + .into_iter() + .map(|cu| (cu.collection_uuid.clone(), cu)) + .collect(); + + // Get all collection groups for this user to prevent there inclusion + let cg: HashSet = CollectionGroup::find_by_user(&self.user_uuid, conn) .await .into_iter() - .map(|cu| (cu.collection_uuid.clone(), cu)) + .map(|cg| cg.collections_uuid) .collect(); - // Get all collection groups for this user to prevent there inclusion - let cg: HashSet = CollectionGroup::find_by_user(&self.user_uuid, conn) - .await - .into_iter() - .map(|cg| cg.collections_uuid) - .collect(); - - Collection::find_by_organization_and_user_uuid(&self.org_uuid, &self.user_uuid, conn) - .await - .into_iter() - .filter_map(|c| { - let (read_only, hide_passwords, manage) = if self.has_full_access() { - (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::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 - } else if cg.contains(&c.uuid) { - return None; - } else { - (true, true, false) - }; - - Some(json!({ - "id": c.uuid, - "readOnly": read_only, - "hidePasswords": hide_passwords, - "manage": manage, - })) - }) - .collect() - } else { - Vec::new() - }; + Collection::find_by_organization_and_user_uuid(&self.org_uuid, &self.user_uuid, conn) + .await + .into_iter() + .filter_map(|c| { + let (read_only, hide_passwords, manage) = if self.has_full_access() { + (false, false, assignment_manage(self.atype, false)) + } else if let Some(cu) = cu.get(&c.uuid) { + (cu.read_only, cu.hide_passwords, assignment_manage(self.atype, cu.manage)) + // 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 + } else if cg.contains(&c.uuid) { + return None; + } else { + (true, true, false) + }; + + Some(json!({ + "id": c.uuid, + "readOnly": read_only, + "hidePasswords": hide_passwords, + "manage": manage, + })) + }) + .collect() + } else { + Vec::new() + }; let membership_type = self.atype; @@ -642,7 +639,7 @@ impl Membership { json!({ "accessEventLogs": self.access_event_logs, "accessImportExport": self.access_import_export, - "accessReports": self.access_reports, + "accessReports": false, "createNewCollections": self.create_new_collections, "editAnyCollection": self.edit_any_collection, "deleteAnyCollection": self.delete_any_collection, @@ -888,10 +885,6 @@ impl Membership { self.has_type(MembershipType::Custom) && self.access_import_export } - pub fn has_access_reports(&self) -> bool { - self.has_type(MembershipType::Custom) && self.access_reports - } - /// Check for an explicit per-collection Manage grant without treating any `access_all` value /// as such a grant. Custom-role collection guards use this instead of the legacy broad helper, /// because membership/group `access_all` must not manufacture a per-collection Manage grant. @@ -915,6 +908,7 @@ impl Membership { .filter(users_organizations::user_uuid.eq(user_uuid.clone())) .filter(users_organizations::org_uuid.eq(org_uuid.clone())) .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) + .filter(users_organizations::atype.eq(MembershipType::Custom as i32)) .filter(collections::uuid.eq(collection_uuid.clone())) .filter(users_collections::manage.eq(true)) .count() @@ -945,6 +939,7 @@ impl Membership { .filter(users_organizations::user_uuid.eq(user_uuid)) .filter(users_organizations::org_uuid.eq(org_uuid)) .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) + .filter(users_organizations::atype.eq(MembershipType::Custom as i32)) .filter(collections::uuid.eq(collection_uuid)) .filter(collections_groups::manage.eq(true)) .count() @@ -1577,12 +1572,10 @@ mod tests { member.access_event_logs = true; assert!(member.has_access_event_logs()); assert!(!member.has_access_import_export()); - assert!(!member.has_access_reports()); member.access_import_export = true; member.access_reports = true; assert!(member.has_access_import_export()); - assert!(member.has_access_reports()); // None of them imply collection or management capabilities. assert!(!member.has_full_access()); assert!(!member.has_manage_users()); @@ -1591,6 +1584,5 @@ mod tests { member.atype = MembershipType::User as i32; assert!(!member.has_access_event_logs()); assert!(!member.has_access_import_export()); - assert!(!member.has_access_reports()); } } diff --git a/src/static/scripts/admin_users.js b/src/static/scripts/admin_users.js index 1bae0aa3..03ec9712 100644 --- a/src/static/scripts/admin_users.js +++ b/src/static/scripts/admin_users.js @@ -174,10 +174,6 @@ const ORG_TYPES = { "name": "User", "bg": "blue" }, - "3": { - "name": "Manager", - "bg": "green" - }, "4": { "name": "Custom", "bg": "teal" @@ -215,12 +211,13 @@ jQuery.extend(jQuery.fn.dataTableExt.oSort, { const userOrgTypeDialog = document.getElementById("userOrgTypeDialog"); // Fill the form and title userOrgTypeDialog.addEventListener("show.bs.modal", function(event) { + document.getElementById("userOrgTypeForm").reset(); + // Get shared values const userEmail = event.relatedTarget.parentNode.dataset.vwUserEmail; const userUuid = event.relatedTarget.parentNode.dataset.vwUserUuid; // Get org specific values const userOrgType = event.relatedTarget.dataset.vwOrgType; - const userOrgTypeName = ORG_TYPES[userOrgType]["name"]; const orgName = event.relatedTarget.dataset.vwOrgName; const orgUuid = event.relatedTarget.dataset.vwOrgUuid; @@ -228,7 +225,9 @@ userOrgTypeDialog.addEventListener("show.bs.modal", function(event) { document.getElementById("userOrgTypeDialogUserEmail").textContent = userEmail; document.getElementById("userOrgTypeUserUuid").value = userUuid; document.getElementById("userOrgTypeOrgUuid").value = orgUuid; - document.getElementById(`userOrgType${userOrgTypeName}`).checked = true; + if (ORG_TYPES[userOrgType] !== undefined) { + document.getElementById(`userOrgType${ORG_TYPES[userOrgType].name}`).checked = true; + } }, false); // Prevent accidental submission of the form with valid elements after the modal has been hidden. @@ -255,7 +254,10 @@ function updateUserOrgType(event) { function initUserTable() { // Color all the org buttons per type document.querySelectorAll("button[data-vw-org-type]").forEach(function(e) { - const orgType = ORG_TYPES[e.dataset.vwOrgType]; + const orgType = ORG_TYPES[e.dataset.vwOrgType] ?? { + "name": "Unknown membership type", + "bg": "gray" + }; e.style.backgroundColor = orgType.bg; if (orgType.font !== undefined) { e.style.color = orgType.font; diff --git a/src/static/templates/admin/users.hbs b/src/static/templates/admin/users.hbs index 3bd63446..d848d894 100644 --- a/src/static/templates/admin/users.hbs +++ b/src/static/templates/admin/users.hbs @@ -130,10 +130,7 @@