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 index 3186fe6a..a20f7543 100644 --- 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 @@ -1,15 +1,33 @@ --- 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 +-- A normal User with the historical membership-level access_all bit reached every collection of the +-- organization with full read/write, but held no collection-management authority. Mapping that onto +-- the Custom role would add authority, clearing the bit would remove existing access — so instead, +-- materialize the reach as explicit per-collection assignments while the source bit still exists. +-- `manage` stays FALSE, so no management authority is invented. This is the same approach Bitwarden +-- took when it retired `accessAll`; the one behavioral difference is that the access is no longer +-- dynamic, i.e. collections created later are not added automatically. +-- +-- Step 1: a pre-existing assignment was overridden by access_all (full read/write regardless of +-- read_only/hide_passwords), so relax it to match what the member actually had. +UPDATE users_collections +SET read_only = FALSE, + hide_passwords = FALSE +WHERE EXISTS ( + SELECT 1 + FROM users_organizations AS uo + INNER JOIN collections AS c ON c.org_uuid = uo.org_uuid + WHERE uo.atype = 2 + AND uo.access_all = TRUE + AND uo.user_uuid = users_collections.user_uuid + AND c.uuid = users_collections.collection_uuid ); -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; + +-- Step 2: add the assignments that did not exist yet. Existing rows are left to step 1. +INSERT IGNORE INTO users_collections (user_uuid, collection_uuid, read_only, hide_passwords, manage) +SELECT uo.user_uuid, c.uuid, FALSE, FALSE, FALSE +FROM users_organizations AS uo +INNER JOIN collections AS c ON c.org_uuid = uo.org_uuid +WHERE uo.atype = 2 + AND uo.access_all = TRUE; -- 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, 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 index 01295bf5..a2035691 100644 --- 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 @@ -1,6 +1,13 @@ -- 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`. +-- the invariant the immediately preceding schema relies on: access_all == access to every collection. +-- That is exactly Owners/Admins, plus Custom members holding `edit_any_collection`. +-- +-- NOTE: this only holds for reverting *this* migration. Reverting further down the chain, +-- 2026-07-16 deliberately recomputes access_all as (create AND edit AND delete) for Custom members, +-- because in that older schema access_all also meant the legacy Manager "Manage all collections" +-- authority -- so a member who only held `edit_any_collection` comes out as a Manager *without* +-- access_all rather than silently gaining collection deletion. That is intentional and fail-closed; +-- the full rollback is blocked by 2026-07-24-140000/down.sql anyway. 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-140000_guard_custom_role_downgrade/up.sql b/migrations/mysql/2026-07-24-140000_guard_custom_role_downgrade/up.sql index af5fed1b..f3f3a7a7 100644 --- 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 @@ -1,3 +1,7 @@ --- Forward migration marker. Its down migration intentionally blocks an automatic lossy downgrade +-- Forward migration marker: its down migration intentionally blocks an automatic lossy downgrade -- before any granular permission column is removed. -SELECT 1; +-- +-- It also cleans up after 2026-07-15: the same-run bookkeeping table has served its purpose by now +-- (2026-07-23 consumed the marker), so it is not left behind in every database. A single DDL +-- statement is safe even on MySQL, where DDL commits implicitly -- re-running it is a no-op. +DROP TABLE IF EXISTS __vw_custom_role_same_run_0716; 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 index 6d75889c..e75897fa 100644 --- 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 @@ -1,15 +1,34 @@ --- 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 +-- A normal User with the historical membership-level access_all bit reached every collection of the +-- organization with full read/write, but held no collection-management authority. Mapping that onto +-- the Custom role would add authority, clearing the bit would remove existing access — so instead, +-- materialize the reach as explicit per-collection assignments while the source bit still exists. +-- `manage` stays FALSE, so no management authority is invented. This is the same approach Bitwarden +-- took when it retired `accessAll`; the one behavioral difference is that the access is no longer +-- dynamic, i.e. collections created later are not added automatically. +-- +-- Step 1: a pre-existing assignment was overridden by access_all (full read/write regardless of +-- read_only/hide_passwords), so relax it to match what the member actually had. +UPDATE users_collections +SET read_only = FALSE, + hide_passwords = FALSE +WHERE EXISTS ( + SELECT 1 + FROM users_organizations AS uo + INNER JOIN collections AS c ON c.org_uuid = uo.org_uuid + WHERE uo.atype = 2 + AND uo.access_all = TRUE + AND uo.user_uuid = users_collections.user_uuid + AND c.uuid = users_collections.collection_uuid ); -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; + +-- Step 2: add the assignments that did not exist yet. Existing rows are left to step 1. +INSERT INTO users_collections (user_uuid, collection_uuid, read_only, hide_passwords, manage) +SELECT uo.user_uuid, c.uuid, FALSE, FALSE, FALSE +FROM users_organizations AS uo +INNER JOIN collections AS c ON c.org_uuid = uo.org_uuid +WHERE uo.atype = 2 + AND uo.access_all = TRUE +ON CONFLICT (user_uuid, collection_uuid) DO NOTHING; -- 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, 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 index 01295bf5..a2035691 100644 --- 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 @@ -1,6 +1,13 @@ -- 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`. +-- the invariant the immediately preceding schema relies on: access_all == access to every collection. +-- That is exactly Owners/Admins, plus Custom members holding `edit_any_collection`. +-- +-- NOTE: this only holds for reverting *this* migration. Reverting further down the chain, +-- 2026-07-16 deliberately recomputes access_all as (create AND edit AND delete) for Custom members, +-- because in that older schema access_all also meant the legacy Manager "Manage all collections" +-- authority -- so a member who only held `edit_any_collection` comes out as a Manager *without* +-- access_all rather than silently gaining collection deletion. That is intentional and fail-closed; +-- the full rollback is blocked by 2026-07-24-140000/down.sql anyway. 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-140000_guard_custom_role_downgrade/up.sql b/migrations/postgresql/2026-07-24-140000_guard_custom_role_downgrade/up.sql index af5fed1b..f3f3a7a7 100644 --- 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 @@ -1,3 +1,7 @@ --- Forward migration marker. Its down migration intentionally blocks an automatic lossy downgrade +-- Forward migration marker: its down migration intentionally blocks an automatic lossy downgrade -- before any granular permission column is removed. -SELECT 1; +-- +-- It also cleans up after 2026-07-15: the same-run bookkeeping table has served its purpose by now +-- (2026-07-23 consumed the marker), so it is not left behind in every database. A single DDL +-- statement is safe even on MySQL, where DDL commits implicitly -- re-running it is a no-op. +DROP TABLE IF EXISTS __vw_custom_role_same_run_0716; 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 index 6d75889c..6a66682a 100644 --- 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 @@ -1,15 +1,33 @@ --- 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 +-- A normal User with the historical membership-level access_all bit reached every collection of the +-- organization with full read/write, but held no collection-management authority. Mapping that onto +-- the Custom role would add authority, clearing the bit would remove existing access — so instead, +-- materialize the reach as explicit per-collection assignments while the source bit still exists. +-- `manage` stays FALSE, so no management authority is invented. This is the same approach Bitwarden +-- took when it retired `accessAll`; the one behavioral difference is that the access is no longer +-- dynamic, i.e. collections created later are not added automatically. +-- +-- Step 1: a pre-existing assignment was overridden by access_all (full read/write regardless of +-- read_only/hide_passwords), so relax it to match what the member actually had. +UPDATE users_collections +SET read_only = FALSE, + hide_passwords = FALSE +WHERE EXISTS ( + SELECT 1 + FROM users_organizations AS uo + INNER JOIN collections AS c ON c.org_uuid = uo.org_uuid + WHERE uo.atype = 2 + AND uo.access_all = TRUE + AND uo.user_uuid = users_collections.user_uuid + AND c.uuid = users_collections.collection_uuid ); -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; + +-- Step 2: add the assignments that did not exist yet. Existing rows are left to step 1. +INSERT OR IGNORE INTO users_collections (user_uuid, collection_uuid, read_only, hide_passwords, manage) +SELECT uo.user_uuid, c.uuid, FALSE, FALSE, FALSE +FROM users_organizations AS uo +INNER JOIN collections AS c ON c.org_uuid = uo.org_uuid +WHERE uo.atype = 2 + AND uo.access_all = TRUE; -- 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, 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 index 01295bf5..a2035691 100644 --- 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 @@ -1,6 +1,13 @@ -- 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`. +-- the invariant the immediately preceding schema relies on: access_all == access to every collection. +-- That is exactly Owners/Admins, plus Custom members holding `edit_any_collection`. +-- +-- NOTE: this only holds for reverting *this* migration. Reverting further down the chain, +-- 2026-07-16 deliberately recomputes access_all as (create AND edit AND delete) for Custom members, +-- because in that older schema access_all also meant the legacy Manager "Manage all collections" +-- authority -- so a member who only held `edit_any_collection` comes out as a Manager *without* +-- access_all rather than silently gaining collection deletion. That is intentional and fail-closed; +-- the full rollback is blocked by 2026-07-24-140000/down.sql anyway. 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-140000_guard_custom_role_downgrade/up.sql b/migrations/sqlite/2026-07-24-140000_guard_custom_role_downgrade/up.sql index af5fed1b..f3f3a7a7 100644 --- 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 @@ -1,3 +1,7 @@ --- Forward migration marker. Its down migration intentionally blocks an automatic lossy downgrade +-- Forward migration marker: its down migration intentionally blocks an automatic lossy downgrade -- before any granular permission column is removed. -SELECT 1; +-- +-- It also cleans up after 2026-07-15: the same-run bookkeeping table has served its purpose by now +-- (2026-07-23 consumed the marker), so it is not left behind in every database. A single DDL +-- statement is safe even on MySQL, where DDL commits implicitly -- re-running it is a no-op. +DROP TABLE IF EXISTS __vw_custom_role_same_run_0716; diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index 045fa562..72a57e22 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -400,7 +400,10 @@ async fn get_org_collections(org_id: OrganizationId, headers: ManagerHeadersLoos let can_read_collection_list = headers.membership.has_full_access() || headers.membership.has_manage_users() || headers.membership.has_manage_groups() - || headers.membership.has_delete_any_collection(); + || headers.membership.has_delete_any_collection() + // Create new collections needs the list too: the client resolves the parent of a nested + // collection against it and refreshes it after a create. + || headers.membership.has_create_new_collections(); if !can_read_collection_list { err_code!("Resource not found.", "User does not have full access", rocket::http::Status::NotFound.code); } @@ -437,8 +440,10 @@ async fn get_org_collections_details(org_id: OrganizationId, headers: ManagerHea // (metadata only) so the web client can render member/group collection assignments // without crashing on collections it can't otherwise see. This exposes names/ids // only, never cipher contents. manage_policies does not need the collection list. - let can_read_collection_list = - member.has_manage_users() || member.has_manage_groups() || member.has_delete_any_collection(); + let can_read_collection_list = member.has_manage_users() + || member.has_manage_groups() + || member.has_delete_any_collection() + || member.has_create_new_collections(); // Delete any collection can reveal collection access metadata, matching Bitwarden's // ReadAllWithAccess behavior, but still does not grant cipher access. Manage Users/Groups // retain the narrower metadata-only view introduced by the base PR. @@ -1047,64 +1052,78 @@ async fn get_assigned_org_details(data: OrgIdData, headers: Headers, conn: DbCon ); } - let ciphers = filter_ciphers_for_organization( - Cipher::find_by_user_visible(&headers.user.uuid, &conn).await, - &data.organization_id, - ); - let cipher_sync_data = CipherSyncData::new(&headers.user.uuid, CipherSyncType::User, &conn).await; + Ok(Json(json!({ + "data": assigned_org_ciphers_json(&data.organization_id, &headers.host, &headers.user.uuid, &conn).await?, + "object": "list", + "continuationToken": null, + }))) +} + +// Serialize exactly the organization ciphers the user is actually assigned to, directly or via a +// group. `CipherSyncType::User` keeps the per-cipher access restrictions in place, so nothing outside +// the caller's own collections is returned and every cipher carries its real `edit`/`viewPassword` +// flags. +// +// NOTE: as everywhere else in Vaultwarden (and in Bitwarden), a `hidePasswords` assignment is +// reported to the client as `viewPassword: false` rather than redacted server-side. This therefore +// returns exactly what the same member already receives from `/api/sync` — never more. +async fn assigned_org_ciphers_json( + org_id: &OrganizationId, + host: &str, + user_id: &UserId, + conn: &DbConn, +) -> Result { + let ciphers = filter_ciphers_for_organization(Cipher::find_by_user_visible(user_id, conn).await, org_id); + let cipher_sync_data = CipherSyncData::new(user_id, CipherSyncType::User, conn).await; let mut ciphers_json = Vec::with_capacity(ciphers.len()); for cipher in ciphers { - ciphers_json.push( - cipher - .to_json(&headers.host, &headers.user.uuid, Some(&cipher_sync_data), CipherSyncType::User, &conn) - .await?, - ); + ciphers_json.push(cipher.to_json(host, user_id, Some(&cipher_sync_data), CipherSyncType::User, conn).await?); } - Ok(Json(json!({ - "data": ciphers_json, - "object": "list", - "continuationToken": null, - }))) + Ok(Value::Array(ciphers_json)) } -// Returns every cipher in the organization, serialized with `CipherSyncType::Organization` — which -// deliberately skips the per-cipher access restrictions, so `readOnly`/`hidePasswords` are not -// applied and collection assignments are ignored. Whoever passes the check below reads the whole -// organization vault. +// The organization cipher list the clients use for the admin vault view and for computing every +// report (Exposed/Reused/Weak Passwords, Unsecured Websites, Inactive 2FA, ...) locally — Vaultwarden +// has no server-side reports. +// +// Two different answers, depending on how much the caller may actually read: // -// `accessReports` is therefore, by design, a full organization *read* permission and not merely -// "may open the reports screen". Vaultwarden implements no server-side reports: the clients fetch -// this list and compute Exposed/Reused/Weak Passwords, Unsecured Websites, Inactive 2FA etc. -// locally, so the permission cannot be satisfied with less data. +// * Members who already reach every collection (Admin/Owner, or Custom + `editAnyCollection`) get +// the whole organization, serialized with `CipherSyncType::Organization` which deliberately skips +// the per-cipher access restrictions. This is unchanged behavior. // -// This matches Bitwarden upstream, which grants the same endpoint to Owner/Admin and to Custom -// members holding AccessImportExport, EditAnyCollection *or* AccessReports: -// https://github.com/bitwarden/server/blob/main/src/Api/Vault/Controllers/CiphersController.cs -// (`CanAccessAllCiphersAsync`) +// * `accessReports` opens the endpoint *without* widening what may be read: the response is built +// from the caller's own assignments with `CipherSyncType::User`, so `readOnly`/`hidePasswords` +// still apply and collections the member is not assigned to never appear. Their reports therefore +// cover exactly their own collections. // -// We are intentionally *stricter* than Bitwarden for `accessImportExport`: it does not open this -// endpoint, and `get_org_export` scopes its output to the caller's own collections, so -// "may export" never widens what a member can read. Granting `accessReports` does widen it — that -// is the documented trade-off of staying Bitwarden-compatible, and administrators must treat -// `accessReports` as equivalent to read access to every collection in the organization. +// This mirrors `accessImportExport`/`get_org_export`: a permission decides *whether* a member may use +// a feature, never *what* they may read. Bitwarden upstream is more permissive here (its +// `CanAccessAllCiphersAsync` grants the full organization to AccessReports as well); we deliberately +// deviate so that ticking "Access reports" cannot hand out read access to every password in the +// organization. #[get("/ciphers/organization-details?")] async fn get_org_details(data: OrgIdData, headers: ManagerHeadersLoose, conn: DbConn) -> JsonResult { if data.organization_id != headers.membership.org_uuid { err_code!("Resource not found.", "Organization id's do not match", rocket::http::Status::NotFound.code); } - if !headers.membership.has_full_access() && !headers.membership.has_access_reports() { + let ciphers_json = if headers.membership.has_full_access() { + get_org_details_impl(&data.organization_id, &headers.host, &headers.user.uuid, &conn).await? + } else if headers.membership.has_access_reports() { + assigned_org_ciphers_json(&data.organization_id, &headers.host, &headers.user.uuid, &conn).await? + } else { err_code!( "Resource not found.", - "User does not have permission to access all organization ciphers", + "User does not have permission to read the organization ciphers", rocket::http::Status::NotFound.code ); - } + }; Ok(Json(json!({ - "data": get_org_details_impl(&data.organization_id, &headers.host, &headers.user.uuid, &conn).await?, + "data": ciphers_json, "object": "list", "continuationToken": null, }))) @@ -1372,7 +1391,7 @@ async fn send_invite( err!("Invalid type") }; - if !may_manage_member_type(headers.membership_type, new_type) { + if !may_provision_member_type(headers.membership_type, new_type) { err!("You don't have permission to invite this role") } @@ -1781,7 +1800,7 @@ async fn confirm_invite_impl( err!("The specified user isn't a member of the organization") }; - if !may_manage_stored_member_type(headers.membership_type, member_to_confirm.atype) { + if !may_provision_stored_member_type(headers.membership_type, member_to_confirm.atype) { err!("You don't have permission to confirm this user") } @@ -2192,7 +2211,7 @@ async fn delete_member_impl( err!("User to delete isn't member of the organization") }; - if !may_manage_stored_member_type(headers.membership_type, member_to_delete.atype) { + if !may_provision_stored_member_type(headers.membership_type, member_to_delete.atype) { err!("You don't have permission to delete this user") } @@ -2305,17 +2324,19 @@ async fn post_org_import( 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" - ) + // NOTE: no `accessImportExport` gate here on purpose. Bitwarden requires that permission for an + // organization import, but Vaultwarden has always authorized this endpoint per target collection, + // and adding an up-front role check would take a capability away from ordinary members that they + // have today. The real boundary is enforced below and is unchanged: an existing collection must be + // writable for the caller (`Collection::is_writable_by_user`), and creating a new one requires the + // independent `createNewCollections` permission. `accessImportExport` therefore governs the export + // side only. + // + // A confirmed membership is required though: both checks below are confirmed-gated, so an + // invited/accepted member could otherwise only import ciphers without any collection — which lands + // unreachable, unmanaged ciphers in the organization. + if !headers.membership.has_status(MembershipStatus::Confirmed) { + err!("You need to be a confirmed member of this organization to import into it") } let data: ImportData = data.into_inner(); @@ -3154,14 +3175,25 @@ async fn post_groups( None => false, }; - let mut group = group_request.to_group(&org_id); // Security: `access_all` grants the group access to every collection, so it is a // collection-access grant just like assigning collections. A custom user without // collection-management rights must not be able to create an access_all group. + // + // API consistency: reject instead of silently creating a group without the requested access, so a + // caller never believes it granted something the server dropped. A request that grants nothing + // (no access_all, no collections) is still accepted, which is what the plain "new group" dialog + // sends for such a caller. if !caller_can_manage_collections { - group.access_all = false; + if group_request.access_all { + err!("You don't have permission to create a group with access to all collections") + } + if !group_request.collections.is_empty() { + err!("You don't have permission to assign collections to a group") + } } + let group = group_request.to_group(&org_id); + log_event( EventType::GroupCreated as i32, &group.uuid, @@ -3215,22 +3247,36 @@ async fn put_group( // Security: only callers who can actually manage collections (Admins/Owners, or users with // full access) may change a group's collection assignments. A custom user with only - // manage_groups must not be able to add/remove collection access. For them we keep the - // group's existing collection assignments untouched (neither cleared nor overwritten). + // manage_groups must not be able to add/remove collection access. let caller_can_manage_collections = headers.membership_type >= MembershipType::Admin || match Membership::find_by_user_and_org(&headers.user.uuid, &org_id, &conn).await { Some(m) => m.has_full_access(), None => false, }; - // Preserve the current `access_all` grant for callers who can't manage collections, so a - // manage_groups-only user cannot turn a group into an access_all (all-collections) grant. - let previous_access_all = group.access_all; - let mut updated_group = group_request.update_group(group); + // API consistency: reject a collection-access change this caller may not make instead of + // answering 200 and silently keeping the old value — the same rule `edit_member` and + // `send_invite` follow. Only an actual difference is rejected (the regular group dialog echoes + // the current assignments back and has to keep working), and per-assignment flag differences + // (readOnly, hidePasswords, manage) stay ignored, exactly as for a member's assignments. if !caller_can_manage_collections { - updated_group.access_all = previous_access_all; + if group_request.access_all != group.access_all { + err!("You don't have permission to change a group's access to all collections") + } + + let requested: HashSet = group_request.collections.iter().map(|c| c.id.clone()).collect(); + let current: HashSet = CollectionGroup::find_by_group(&group_id, &org_id, &conn) + .await + .into_iter() + .map(|cg| cg.collections_uuid) + .collect(); + if requested != current { + err!("You don't have permission to change this group's collection assignments") + } } + let updated_group = group_request.update_group(group); + if caller_can_manage_collections { CollectionGroup::delete_all_by_group(&group_id, &org_id, &conn).await?; } @@ -3319,6 +3365,30 @@ fn may_manage_stored_member_type(caller_type: MembershipType, target_atype: i32) MembershipType::from_i32(target_atype).is_some_and(|target_type| may_manage_member_type(caller_type, target_type)) } +/// Whether a caller may *provision* a membership of `target_type` — create it (invite), activate it +/// (confirm) or remove it (delete). +/// +/// This is deliberately stricter than [`may_manage_member_type`] and preserves the pre-existing +/// Vaultwarden rule that only Owners bring Admin (or Owner) memberships into or out of existence +/// ("Only Owners can invite Managers, Admins or Owners" / "Only Owners can delete Admins or Owners"). +/// `edit_member` keeps that boundary too, via its dedicated Owner-only guard on Admin/Owner role +/// transitions, so an Admin must not be able to route around it by inviting a fresh Admin instead. +/// State changes that leave the membership in place (reinvite, revoke, restore, edit) keep using +/// [`may_manage_member_type`], which is what Vaultwarden allowed for them before this feature. +fn may_provision_member_type(caller_type: MembershipType, target_type: MembershipType) -> bool { + match caller_type { + MembershipType::Owner => true, + MembershipType::Admin => target_type < MembershipType::Admin, + MembershipType::Custom => target_type == MembershipType::User, + MembershipType::User => false, + } +} + +fn may_provision_stored_member_type(caller_type: MembershipType, target_atype: i32) -> bool { + MembershipType::from_i32(target_atype) + .is_some_and(|target_type| may_provision_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 { @@ -3400,6 +3470,26 @@ async fn add_update_group( conn: &DbConn, caller_can_manage_collections: bool, ) -> JsonResult { + // Security: assigning members to a group that grants collection access (via `access_all` or + // assigned collections) would indirectly grant those members access to the collections' contents, + // and removing them would revoke it. Only callers who can manage collections may change the + // membership of such a group. + // + // API consistency: reject a membership change this caller may not make instead of answering 200 + // and keeping the old membership — same rule as `edit_member`/`send_invite`. Checked before the + // first write so a rejected request leaves nothing behind. On create the group is brand new, so + // it grants no collection access yet and this never triggers. + if !caller_can_manage_collections + && (group.access_all || !CollectionGroup::find_by_group(&group.uuid, &org_id, conn).await.is_empty()) + { + let requested: HashSet<&MembershipId> = members.iter().collect(); + let current_members = GroupUser::find_by_group(&group.uuid, &org_id, conn).await; + let current: HashSet<&MembershipId> = current_members.iter().map(|gu| &gu.users_organizations_uuid).collect(); + if requested != current { + err!("You don't have permission to change the membership of a group that grants collection access") + } + } + group.save(conn).await?; // Security (F-1): a `collections_groups.manage` grant carries collection delete authority, so a @@ -4096,6 +4186,7 @@ mod tests { CustomRolePermissions, caller_manage_grant_role_check, collection_bearing_membership_unchanged, filter_ciphers_for_organization, may_change_group_membership, may_change_member_type, may_export_entire_organization, may_manage_member_type, may_manage_stored_member_type, + may_provision_member_type, may_provision_stored_member_type, }; use crate::db::models::{Cipher, GroupId, Membership, MembershipStatus, MembershipType, OrganizationId}; @@ -4244,6 +4335,43 @@ mod tests { assert!(may_manage_stored_member_type(MembershipType::Custom, MembershipType::User as i32)); } + #[test] + fn only_owners_provision_admin_memberships() { + // REGRESSION: bringing an Admin (or Owner) membership into or out of existence stays + // Owner-only, exactly as before this feature ("Only Owners can invite Managers, Admins or + // Owners" / "Only Owners can delete Admins or Owners"). Otherwise an Admin could route around + // the Owner-only role-change guard in `edit_member` by inviting a fresh Admin instead. + for target in [MembershipType::Owner, MembershipType::Admin, MembershipType::Custom, MembershipType::User] { + assert!(may_provision_member_type(MembershipType::Owner, target)); + } + + assert!(!may_provision_member_type(MembershipType::Admin, MembershipType::Owner)); + assert!(!may_provision_member_type(MembershipType::Admin, MembershipType::Admin)); + assert!(may_provision_member_type(MembershipType::Admin, MembershipType::Custom)); + assert!(may_provision_member_type(MembershipType::Admin, MembershipType::User)); + + // A Custom member with manage_users stays limited to ordinary Users, as for every other + // lifecycle action. + assert!(may_provision_member_type(MembershipType::Custom, MembershipType::User)); + for target in [MembershipType::Owner, MembershipType::Admin, MembershipType::Custom] { + assert!(!may_provision_member_type(MembershipType::Custom, target)); + } + for target in [MembershipType::Owner, MembershipType::Admin, MembershipType::Custom, MembershipType::User] { + assert!(!may_provision_member_type(MembershipType::User, target)); + } + + // Provisioning is strictly narrower than the state-change matrix: an Admin may still revoke, + // restore or edit a peer Admin (which Vaultwarden allowed before), but no longer create, + // confirm or delete one. + assert!(may_manage_member_type(MembershipType::Admin, MembershipType::Admin)); + assert!(!may_provision_member_type(MembershipType::Admin, MembershipType::Admin)); + + assert!(may_provision_stored_member_type(MembershipType::Admin, MembershipType::User as i32)); + assert!(!may_provision_stored_member_type(MembershipType::Admin, MembershipType::Admin as i32)); + // An unknown stored role never qualifies. + assert!(!may_provision_stored_member_type(MembershipType::Owner, i32::MAX)); + } + #[test] fn only_collection_bearing_group_changes_are_rejected() { let plain: GroupId = "plain".to_owned().into(); diff --git a/src/db/mod.rs b/src/db/mod.rs index 75c07d2c..65b39eab 100644 --- a/src/db/mod.rs +++ b/src/db/mod.rs @@ -471,26 +471,67 @@ 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_MANAGE_PERMISSIONS_MIGRATION: &str = "20260630120000"; +const CUSTOM_ACCESS_PERMISSIONS_MIGRATION: &str = "20260724130000"; const CUSTOM_ROLE_SAME_RUN_MARKER_TABLE: &str = "__vw_custom_role_same_run_0716"; -const LEGACY_USER_ACCESS_ALL_RECOVERY_SQL: &str = concat!( - "\n\nReview every affected membership with this SQLite/MySQL/PostgreSQL-compatible query:\n", - "SELECT uuid, user_uuid, org_uuid, status\n", - "FROM users_organizations\n", - "WHERE atype = 2 AND access_all = TRUE;\n\n", - "After an organization owner has decided the intended outcome, replace and run exactly one ", - "guarded statement for that membership while every Vaultwarden instance is stopped. Do not bulk-promote these ", - "records.\n\n", - "Keep the User role and revoke organization-wide vault access:\n", - "UPDATE users_organizations\n", - "SET access_all = FALSE\n", - "WHERE uuid = '' AND atype = 2 AND access_all = TRUE;\n\n", - "Preserve organization-wide vault access by intentionally granting Custom Create/Edit/Delete-any collection ", - "authority:\n", - "UPDATE users_organizations\n", - "SET atype = 3\n", - "WHERE uuid = '' AND atype = 2 AND access_all = TRUE;\n\n", - "The second statement deliberately adds collection-management authority: the repair migration copies the retained ", - "access_all value to all three collection permissions before converting legacy role 3 to Custom role 4." + +/// One of the three groups of granular permission columns, each added by its own migration. +/// +/// A partially present group means the migration was interrupted between its `ALTER TABLE` +/// statements. On MySQL/MariaDB that is reachable because DDL commits implicitly, so the ledger entry +/// can be missing while some columns already exist; re-running the migration then fails forever with +/// `Duplicate column name`. Detect it and hand the operator an unambiguous fix instead. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum PermissionColumnGroup { + Manage, + Collection, + Access, +} + +impl PermissionColumnGroup { + const fn migration(self) -> &'static str { + match self { + Self::Manage => CUSTOM_ROLE_MANAGE_PERMISSIONS_MIGRATION, + Self::Collection => CUSTOM_COLLECTION_PERMISSIONS_MIGRATION, + Self::Access => CUSTOM_ACCESS_PERMISSIONS_MIGRATION, + } + } + + /// SQL list literal of the group's column names, for the `IN (...)` lookups. + const fn column_list(self) -> &'static str { + match self { + Self::Manage => "'manage_users', 'manage_groups', 'manage_policies'", + Self::Collection => "'create_new_collections', 'edit_any_collection', 'delete_any_collection'", + Self::Access => "'access_event_logs', 'access_import_export', 'access_reports'", + } + } + + const fn description(self) -> &'static str { + match self { + Self::Manage => "custom management-permission", + Self::Collection => "custom collection-permission", + Self::Access => "custom access-permission", + } + } +} + +const PARTIAL_PERMISSION_COLUMNS_RECOVERY: &str = concat!( + "\n\nThis happens when a migration was interrupted between its ALTER TABLE statements (on ", + "MySQL/MariaDB every DDL statement commits on its own, so columns can exist without the ledger ", + "entry). The leftover columns only ever hold their FALSE default at this point, so dropping them ", + "loses nothing and lets the migration run again from a clean state.\n\n", + "List the columns that are already present:\n", + "SELECT column_name\n", + "FROM information_schema.columns\n", + "WHERE table_name = 'users_organizations'\n", + " AND column_name IN ('manage_users', 'manage_groups', 'manage_policies',\n", + " 'create_new_collections', 'edit_any_collection', 'delete_any_collection',\n", + " 'access_event_logs', 'access_import_export', 'access_reports');\n\n", + "(On SQLite: SELECT name FROM pragma_table_info('users_organizations');)\n\n", + "Then, with every Vaultwarden instance stopped and a backup taken, drop exactly the columns of ", + "the affected group that the message above names, e.g.:\n", + "ALTER TABLE users_organizations DROP COLUMN ;\n\n", + "Afterwards restart Vaultwarden so the migration applies the whole group in one go." ); const AMBIGUOUS_DIRECT_PERMISSIONS_RECOVERY_SQL: &str = concat!( @@ -545,15 +586,35 @@ struct CustomRoleMigrationFacts { memberships_table_exists: bool, migration_table_exists: bool, access_all_column_exists: bool, + manage_permission_columns: i64, + manage_permissions_migration_applied: bool, collection_permission_columns: i64, collection_permissions_migration_applied: bool, + access_permission_columns: i64, + access_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, } +impl CustomRoleMigrationFacts { + /// `(columns present, migration recorded)` for one permission column group. + const fn permission_columns(self, group: PermissionColumnGroup) -> (i64, bool) { + match group { + PermissionColumnGroup::Manage => { + (self.manage_permission_columns, self.manage_permissions_migration_applied) + } + PermissionColumnGroup::Collection => { + (self.collection_permission_columns, self.collection_permissions_migration_applied) + } + PermissionColumnGroup::Access => { + (self.access_permission_columns, self.access_permissions_migration_applied) + } + } + } +} + #[derive(Clone, Copy, Debug, Eq, PartialEq)] enum CustomRolePreflightDecision { Proceed, @@ -561,10 +622,9 @@ enum CustomRolePreflightDecision { RefuseAlreadyDropped, RefuseMissingAccessAll, RefuseMissingMigrationLedger, - RefuseLegacyUserAccessAll, RefuseAmbiguousDirectPermissions, - RefusePartialCollectionSchema, - RefuseCollectionLedgerMismatch, + RefusePartialPermissionSchema(PermissionColumnGroup), + RefusePermissionLedgerMismatch(PermissionColumnGroup), } fn custom_role_preflight_decision( @@ -587,21 +647,27 @@ fn custom_role_preflight_decision( 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 + // Every permission column group must be either completely absent (its migration is still pending) + // or completely present with its ledger entry. Anything else is an interrupted migration whose + // re-run would fail with `Duplicate column name`, so refuse with an actionable message. The single + // historical exception is the collection group on MySQL, where the known-good partial state is + // completed in place. + for group in [PermissionColumnGroup::Manage, PermissionColumnGroup::Collection, PermissionColumnGroup::Access] { + match facts.permission_columns(group) { + (0, false) | (3, true) => {} + (3, false) if group == PermissionColumnGroup::Collection && can_complete_mysql_partial_migration => { + return CustomRolePreflightDecision::CompleteMysqlCollectionMigration; + } + (_, true) => return CustomRolePreflightDecision::RefusePermissionLedgerMismatch(group), + _ => return CustomRolePreflightDecision::RefusePartialPermissionSchema(group), } - (_, true) => CustomRolePreflightDecision::RefuseCollectionLedgerMismatch, - _ => CustomRolePreflightDecision::RefusePartialCollectionSchema, } + + CustomRolePreflightDecision::Proceed } fn custom_role_preflight_error(decision: CustomRolePreflightDecision, facts: CustomRoleMigrationFacts) -> Error { @@ -621,36 +687,35 @@ fn custom_role_preflight_error(decision: CustomRolePreflightDecision, facts: Cus 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::RefusePartialPermissionSchema(group) => format!( + "Found {} of the three {} columns ({}) without a completed {} migration. The migration \ + was interrupted between its ALTER TABLE statements.", + facts.permission_columns(group).0, + group.description(), + group.column_list(), + group.migration() ), - CustomRolePreflightDecision::RefuseCollectionLedgerMismatch => format!( - "Migration {CUSTOM_COLLECTION_PERMISSIONS_MIGRATION} is recorded, but only {} of its \ - three collection-permission columns exist.", - facts.collection_permission_columns + CustomRolePreflightDecision::RefusePermissionLedgerMismatch(group) => format!( + "Migration {} is recorded, but only {} of its three {} columns ({}) exist.", + group.migration(), + facts.permission_columns(group).0, + group.description(), + group.column_list() ), CustomRolePreflightDecision::Proceed | CustomRolePreflightDecision::CompleteMysqlCollectionMigration => { unreachable!("successful preflight decisions do not produce errors") } }; let recovery = match decision { - CustomRolePreflightDecision::RefuseLegacyUserAccessAll => LEGACY_USER_ACCESS_ALL_RECOVERY_SQL, CustomRolePreflightDecision::RefuseAmbiguousDirectPermissions => AMBIGUOUS_DIRECT_PERMISSIONS_RECOVERY_SQL, + CustomRolePreflightDecision::RefusePartialPermissionSchema(_) + | CustomRolePreflightDecision::RefusePermissionLedgerMismatch(_) => PARTIAL_PERMISSION_COLUMNS_RECOVERY, CustomRolePreflightDecision::RefuseAlreadyDropped => ALREADY_DROPPED_RECOVERY, _ => "", }; @@ -761,14 +826,28 @@ mod sqlite_migrations { "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 permission_columns = |connection: &mut diesel::sqlite::SqliteConnection, + group: super::PermissionColumnGroup| + -> Result { + count( + connection, + format!( + "SELECT COUNT(*) AS count FROM pragma_table_info('users_organizations') \ + WHERE name IN ({})", + group.column_list() + ), + ) + }; + let manage_permission_columns = permission_columns(connection, super::PermissionColumnGroup::Manage)?; + let collection_permission_columns = permission_columns(connection, super::PermissionColumnGroup::Collection)?; + let access_permission_columns = permission_columns(connection, super::PermissionColumnGroup::Access)?; + let manage_permissions_migration_applied = + migration_table_exists && migration_applied(connection, super::CUSTOM_ROLE_MANAGE_PERMISSIONS_MIGRATION)?; let collection_permissions_migration_applied = migration_table_exists && migration_applied(connection, super::CUSTOM_COLLECTION_PERMISSIONS_MIGRATION)?; + let access_permissions_migration_applied = + migration_table_exists && migration_applied(connection, super::CUSTOM_ACCESS_PERMISSIONS_MIGRATION)?; let repair_migration_applied = migration_table_exists && migration_applied(connection, super::CUSTOM_ROLE_REPAIR_MIGRATION)?; let access_all_drop_migration_applied = @@ -780,16 +859,6 @@ mod sqlite_migrations { 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, @@ -808,11 +877,14 @@ mod sqlite_migrations { memberships_table_exists, migration_table_exists, access_all_column_exists, + manage_permission_columns, + manage_permissions_migration_applied, collection_permission_columns, collection_permissions_migration_applied, + access_permission_columns, + access_permissions_migration_applied, repair_migration_applied, access_all_drop_migration_applied, - legacy_user_access_all_count, ambiguous_direct_permission_count, same_run_0716_marker, }; @@ -968,17 +1040,27 @@ mod mysql_migrations { 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 permission_columns = |connection: &mut diesel::mysql::MysqlConnection, + group: super::PermissionColumnGroup| + -> Result { + count( + connection, + format!( + "SELECT COUNT(*) AS count FROM information_schema.columns WHERE table_schema = DATABASE() AND table_name = 'users_organizations' AND column_name IN ({})", + group.column_list() + ), + ) + }; + let manage_permission_columns = permission_columns(connection, super::PermissionColumnGroup::Manage)?; + let collection_permission_columns = permission_columns(connection, super::PermissionColumnGroup::Collection)?; + let access_permission_columns = permission_columns(connection, super::PermissionColumnGroup::Access)?; + let manage_permissions_migration_applied = + migration_table_exists && migration_applied(connection, super::CUSTOM_ROLE_MANAGE_PERMISSIONS_MIGRATION)?; let collection_permissions_migration_applied = migration_table_exists && migration_applied(connection, super::CUSTOM_COLLECTION_PERMISSIONS_MIGRATION)?; + let access_permissions_migration_applied = + migration_table_exists && migration_applied(connection, super::CUSTOM_ACCESS_PERMISSIONS_MIGRATION)?; let repair_migration_applied = migration_table_exists && migration_applied(connection, super::CUSTOM_ROLE_REPAIR_MIGRATION)?; let access_all_drop_migration_applied = @@ -990,16 +1072,6 @@ mod mysql_migrations { 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, @@ -1018,11 +1090,14 @@ mod mysql_migrations { memberships_table_exists, migration_table_exists, access_all_column_exists, + manage_permission_columns, + manage_permissions_migration_applied, collection_permission_columns, collection_permissions_migration_applied, + access_permission_columns, + access_permissions_migration_applied, repair_migration_applied, access_all_drop_migration_applied, - legacy_user_access_all_count, ambiguous_direct_permission_count, same_run_0716_marker, }; @@ -1111,17 +1186,27 @@ mod postgresql_migrations { 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 permission_columns = |connection: &mut diesel::pg::PgConnection, + group: super::PermissionColumnGroup| + -> Result { + count( + connection, + format!( + "SELECT COUNT(*) AS count FROM information_schema.columns WHERE table_schema = current_schema() AND table_name = 'users_organizations' AND column_name IN ({})", + group.column_list() + ), + ) + }; + let manage_permission_columns = permission_columns(connection, super::PermissionColumnGroup::Manage)?; + let collection_permission_columns = permission_columns(connection, super::PermissionColumnGroup::Collection)?; + let access_permission_columns = permission_columns(connection, super::PermissionColumnGroup::Access)?; + let manage_permissions_migration_applied = + migration_table_exists && migration_applied(connection, super::CUSTOM_ROLE_MANAGE_PERMISSIONS_MIGRATION)?; let collection_permissions_migration_applied = migration_table_exists && migration_applied(connection, super::CUSTOM_COLLECTION_PERMISSIONS_MIGRATION)?; + let access_permissions_migration_applied = + migration_table_exists && migration_applied(connection, super::CUSTOM_ACCESS_PERMISSIONS_MIGRATION)?; let repair_migration_applied = migration_table_exists && migration_applied(connection, super::CUSTOM_ROLE_REPAIR_MIGRATION)?; let access_all_drop_migration_applied = @@ -1133,16 +1218,6 @@ mod postgresql_migrations { 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, @@ -1161,11 +1236,14 @@ mod postgresql_migrations { memberships_table_exists, migration_table_exists, access_all_column_exists, + manage_permission_columns, + manage_permissions_migration_applied, collection_permission_columns, collection_permissions_migration_applied, + access_permission_columns, + access_permissions_migration_applied, repair_migration_applied, access_all_drop_migration_applied, - legacy_user_access_all_count, ambiguous_direct_permission_count, same_run_0716_marker, }; @@ -1295,35 +1373,70 @@ mod custom_role_migration_preflight_tests { assert!(message.contains("Restore the database backup")); } + // REGRESSION: a legacy `User` membership with the historical access_all bit must NOT stop the + // upgrade. The 2026-07-23 migration materializes that reach as explicit per-collection + // assignments, so the preflight has nothing left to decide and every other fact stays untouched. #[test] - fn legacy_user_access_all_requires_an_operator_decision() { - let facts = Facts { - legacy_user_access_all_count: 1, - ..pending_repair() - }; - let decision = custom_role_preflight_decision(facts, false); - assert_eq!(decision, Decision::RefuseLegacyUserAccessAll); + fn legacy_user_access_all_no_longer_blocks_the_upgrade() { + 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, + manage_permission_columns: 3, + manage_permissions_migration_applied: true, + ..pending_repair() + }, + false, + ), + Decision::Proceed + ); + } - let error = custom_role_preflight_error(decision, facts); - let message = error.source().expect("preflight error should retain its I/O error source").to_string(); - assert!(message.contains("1 legacy User membership(s)")); - assert!(message.contains( - "SELECT uuid, user_uuid, org_uuid, status\n\ - FROM users_organizations\n\ - WHERE atype = 2 AND access_all = TRUE;" - )); - assert!(message.contains( - "SET access_all = FALSE\n\ - WHERE uuid = '' AND atype = 2 AND access_all = TRUE;" - )); - assert!(message.contains( - "SET atype = 3\n\ - WHERE uuid = '' AND atype = 2 AND access_all = TRUE;" - )); - assert!(message.contains("run exactly one guarded statement")); - assert!(message.contains("Do not bulk-promote")); - assert!(message.contains("converting legacy role 3 to Custom role 4")); - assert!(!message.contains("SET atype = 4")); + #[test] + fn a_partial_permission_column_group_is_refused_with_an_actionable_message() { + // Every group is checked, not just the collection one: an interrupted MySQL migration can + // leave `manage_*` or `access_*` columns behind, and re-running it would fail forever with + // `Duplicate column name`. + for (facts, group, expected) in [ + ( + Facts { + manage_permission_columns: 2, + ..pending_repair() + }, + "manage_users", + Decision::RefusePartialPermissionSchema(super::PermissionColumnGroup::Manage), + ), + ( + Facts { + manage_permission_columns: 3, + manage_permissions_migration_applied: true, + access_permission_columns: 3, + ..pending_repair() + }, + "access_event_logs", + Decision::RefusePartialPermissionSchema(super::PermissionColumnGroup::Access), + ), + ( + Facts { + manage_permission_columns: 1, + manage_permissions_migration_applied: true, + ..pending_repair() + }, + "manage_users", + Decision::RefusePermissionLedgerMismatch(super::PermissionColumnGroup::Manage), + ), + ] { + // `true` = MySQL: only the historical collection-group state is auto-completed, never these. + assert_eq!(custom_role_preflight_decision(facts, true), expected); + assert_eq!(custom_role_preflight_decision(facts, false), expected); + + let error = custom_role_preflight_error(expected, facts); + let message = error.source().expect("preflight error should retain its I/O error source").to_string(); + assert!(message.contains(group), "message should name the affected columns: {message}"); + assert!(message.contains("ALTER TABLE users_organizations DROP COLUMN")); + } } #[test] @@ -1375,7 +1488,10 @@ mod custom_role_migration_preflight_tests { ..pending_repair() }; assert_eq!(custom_role_preflight_decision(facts, true), Decision::CompleteMysqlCollectionMigration); - assert_eq!(custom_role_preflight_decision(facts, false), Decision::RefusePartialCollectionSchema); + assert_eq!( + custom_role_preflight_decision(facts, false), + Decision::RefusePartialPermissionSchema(super::PermissionColumnGroup::Collection) + ); } #[test] @@ -1406,7 +1522,7 @@ mod custom_role_migration_preflight_tests { }, true, ), - Decision::RefusePartialCollectionSchema + Decision::RefusePartialPermissionSchema(super::PermissionColumnGroup::Collection) ); assert_eq!( custom_role_preflight_decision( @@ -1417,7 +1533,7 @@ mod custom_role_migration_preflight_tests { }, true, ), - Decision::RefuseCollectionLedgerMismatch + Decision::RefusePermissionLedgerMismatch(super::PermissionColumnGroup::Collection) ); } } diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index 3630baaf..f74e0aca 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -15,7 +15,7 @@ use crate::{ db::{ DbConn, schema::{ - ciphers, ciphers_collections, collections, collections_groups, groups, groups_users, org_policies, + ciphers_collections, collections, collections_groups, groups, groups_users, org_policies, organization_api_key, organizations, users, users_collections, users_organizations, }, }, @@ -1295,27 +1295,6 @@ impl Membership { .await } - pub async fn user_has_ge_admin_access_to_cipher(user_uuid: &UserId, cipher_uuid: &CipherId, conn: &DbConn) -> bool { - conn.run(move |conn| { - users_organizations::table - .inner_join( - ciphers::table.on(ciphers::uuid - .eq(cipher_uuid) - .and(ciphers::organization_uuid.eq(users_organizations::org_uuid.nullable()))), - ) - .filter(users_organizations::user_uuid.eq(user_uuid)) - .filter( - users_organizations::atype.eq_any(vec![MembershipType::Owner as i32, MembershipType::Admin as i32]), - ) - .count() - .first::(conn) - .ok() - .unwrap_or(0) - != 0 - }) - .await - } - pub async fn find_by_collection_and_org( collection_uuid: &CollectionId, org_uuid: &OrganizationId,