From 634d9b2bd311c1d581e07c19374f30374d1ab252 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Sat, 8 Aug 2026 18:01:23 +0200 Subject: [PATCH] Fix migration portability and preserve legacy group collection authority - SQLite: the access_all drop used ALTER TABLE ... DROP COLUMN, which needs SQLite 3.35 while a sqlite_system build links whatever the host provides and libsqlite3-sys accepts 3.34.1. Use the portable table rebuild instead. - Preflight: it returned Proceed for every database carrying the repair migration. Both the access_all drop and the access-permission columns run after that repair, so their interrupted MySQL/MariaDB states went undetected and every restart failed with 1091/1060. Check schema and ledger after the repair too, complete a recorded-drop-without-ledger in place, and refuse the remaining states with a recovery path. - Keep deriving a legacy Manager's collection edit/delete from the organization-local access_all group it came from, rather than dropping the authority when the repair removes its 0/1/1 copy. Stays revocable with the group; collection creation remains the independent permission. - post_org_import: Bitwarden does not require accessImportExport either, it authorizes on that permission *or* per-collection create/import authority. --- .../up.sql | 4 +- .../up.sql | 4 +- .../up.sql | 4 +- .../up.sql | 43 +++- src/api/core/organizations.rs | 26 ++- src/auth.rs | 16 +- src/db/mod.rs | 219 ++++++++++++++++-- src/db/models/collection.rs | 22 +- src/db/models/organization.rs | 55 +++++ 9 files changed, 349 insertions(+), 44 deletions(-) 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 a20f7543..b7d93cdc 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 @@ -31,7 +31,9 @@ WHERE uo.atype = 2 -- 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. +-- remove that deterministic copy so later group removal also revokes the authority. The runtime +-- keeps deriving edit/delete from that group -- see +-- `Membership::has_legacy_group_collection_manage_access` -- so nothing is lost here. UPDATE users_organizations SET edit_any_collection = FALSE, delete_any_collection = FALSE 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 e75897fa..aca9d21d 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 @@ -32,7 +32,9 @@ 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, --- remove that deterministic copy so later group removal also revokes the authority. +-- remove that deterministic copy so later group removal also revokes the authority. The runtime +-- keeps deriving edit/delete from that group -- see +-- `Membership::has_legacy_group_collection_manage_access` -- so nothing is lost here. UPDATE users_organizations SET edit_any_collection = FALSE, delete_any_collection = FALSE 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 6a66682a..404f9fd9 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 @@ -31,7 +31,9 @@ WHERE uo.atype = 2 -- 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. +-- remove that deterministic copy so later group removal also revokes the authority. The runtime +-- keeps deriving edit/delete from that group -- see +-- `Membership::has_legacy_group_collection_manage_access` -- so nothing is lost here. UPDATE users_organizations SET edit_any_collection = FALSE, delete_any_collection = FALSE 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 index e11fb611..3638bc7a 100644 --- 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 @@ -2,4 +2,45 @@ -- 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; +-- +-- `ALTER TABLE ... DROP COLUMN` is deliberately NOT used here: it only exists since SQLite 3.35.0, +-- while a `sqlite_system` build links whatever the host provides and libsqlite3-sys accepts 3.34.1 +-- (which is what Debian 11 ships). Forward migrations have to run on every supported build, so use +-- the portable table rebuild instead -- the same pattern as +-- 2022-03-02-210038_update_devices_primary_key. Vaultwarden runs SQLite migrations with +-- `PRAGMA foreign_keys = OFF`, so dropping the old table does not cascade into groups_users. +CREATE TABLE users_organizations_new ( + uuid TEXT NOT NULL PRIMARY KEY, + user_uuid TEXT NOT NULL REFERENCES users (uuid), + org_uuid TEXT NOT NULL REFERENCES organizations (uuid), + + akey TEXT NOT NULL, + status INTEGER NOT NULL, + atype INTEGER NOT NULL, + reset_password_key TEXT, + external_id TEXT, + invited_by_email TEXT DEFAULT NULL, + manage_users BOOLEAN NOT NULL DEFAULT FALSE, + manage_groups BOOLEAN NOT NULL DEFAULT FALSE, + manage_policies BOOLEAN NOT NULL DEFAULT FALSE, + create_new_collections BOOLEAN NOT NULL DEFAULT FALSE, + edit_any_collection BOOLEAN NOT NULL DEFAULT FALSE, + delete_any_collection BOOLEAN NOT NULL DEFAULT FALSE, + + UNIQUE (user_uuid, org_uuid) +); + +INSERT INTO users_organizations_new ( + uuid, user_uuid, org_uuid, akey, status, atype, reset_password_key, external_id, + invited_by_email, manage_users, manage_groups, manage_policies, + create_new_collections, edit_any_collection, delete_any_collection +) +SELECT + uuid, user_uuid, org_uuid, akey, status, atype, reset_password_key, external_id, + invited_by_email, manage_users, manage_groups, manage_policies, + create_new_collections, edit_any_collection, delete_any_collection +FROM users_organizations; + +DROP TABLE users_organizations; + +ALTER TABLE users_organizations_new RENAME TO users_organizations; diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index 72a57e22..13c915c5 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -2324,13 +2324,15 @@ async fn post_org_import( err!("Organization not found", "Organization id's do not match"); } - // 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. + // NOTE: no `accessImportExport` gate here on purpose. Bitwarden does not require the permission + // either — `ImportCiphersController.CheckOrgImportPermissionAsync` authorizes an organization + // import on `AccessImportExport` *or* per-collection Create/ImportCiphers authority. Vaultwarden + // has always authorized this endpoint per target collection, so 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. The one deliberate difference from Bitwarden is that + // `accessImportExport` alone does not open the endpoint here; it 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 @@ -3418,11 +3420,13 @@ async fn caller_may_grant_collection_manage(caller: &Membership, col_id: &Collec match caller_manage_grant_role_check(caller) { // Role alone decides it (Admin/Owner or delete_any -> yes; User/unknown/unconfirmed -> no). Some(decision) => decision, - // 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. + // Custom without delete_any: the answer is per-collection and must mirror + // `collection_delete_access` exactly, so it can never hand out a right the caller lacks — + // a real users_collections.manage / collections_groups.manage grant, or the legacy + // organization-local `access_all` group that also confers deletion. Edit any collection + // deliberately does not count here. None => match MembershipType::from_i32(caller.atype) { - Some(MembershipType::Custom) => caller.has_explicit_collection_manage_access(col_id, conn).await, + Some(MembershipType::Custom) => caller.has_collection_manage_authority(col_id, conn).await, _ => false, }, } diff --git a/src/auth.rs b/src/auth.rs index 39533abd..ceb57816 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -1008,8 +1008,9 @@ 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, 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. + // A Custom member must prove an actual users_collections.manage / collections_groups.manage + // assignment, or the legacy organization-local `access_all` group a Manager's authority used + // to come from. Membership-level `access_all` is gone and never counted here. Some(MembershipType::Custom) => CollectionManageAccess::ExplicitManage, Some(MembershipType::User) | None => CollectionManageAccess::Denied, } @@ -1039,7 +1040,7 @@ async fn can_manage_collection( match access { CollectionManageAccess::Any => true, CollectionManageAccess::ExplicitManage => { - membership.has_explicit_collection_manage_access(collection_uuid, conn).await + membership.has_collection_manage_authority(collection_uuid, conn).await } CollectionManageAccess::Denied => false, } @@ -1695,10 +1696,11 @@ 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. + // A flagless Custom member (this is what a migrated legacy Manager becomes) never gets + // blanket collection authority from its role alone: every collection operation has to be + // answered per collection. ExplicitManage invokes the database helper that accepts a real + // users_collections.manage / collections_groups.manage grant, or the legacy + // organization-local access_all group — never the membership-level access_all that is gone. let custom = membership(MembershipType::Custom); assert_eq!(collection_edit_access(&custom), CollectionManageAccess::ExplicitManage); assert_eq!(collection_read_access(&custom), CollectionManageAccess::ExplicitManage); diff --git a/src/db/mod.rs b/src/db/mod.rs index 65b39eab..6261c1c4 100644 --- a/src/db/mod.rs +++ b/src/db/mod.rs @@ -572,6 +572,24 @@ const AMBIGUOUS_DIRECT_PERMISSIONS_RECOVERY_SQL: &str = concat!( "the migration cannot attribute. If that member must not be able to create collections, start the server once so ", "the migration completes, then set create_new_collections back to FALSE for that membership." ); +const INTERRUPTED_ACCESS_ALL_DROP_RECOVERY: &str = concat!( + "\n\nThe drop itself carries no data, so the schema is already in its intended final state and ", + "only the ledger entry is missing. Vaultwarden completes this automatically on MySQL/MariaDB, ", + "where it is reachable because DDL commits implicitly. On this backend DDL is transactional, so ", + "the state points at a manual schema change. With every Vaultwarden instance stopped and a ", + "backup taken, record the migration:\n", + "INSERT INTO __diesel_schema_migrations (version) VALUES ('20260724120000');\n\n", + "Afterwards restart Vaultwarden so the remaining migrations run." +); + +const ACCESS_ALL_DROP_MISMATCH_RECOVERY: &str = concat!( + "\n\nThis state cannot arise from a normal upgrade -- the column is removed before the migration ", + "is recorded. Verify whether the column was re-added manually. If it was, and its values are no ", + "longer needed, drop it again with every Vaultwarden instance stopped and a backup taken:\n", + "ALTER TABLE users_organizations DROP COLUMN access_all;\n\n", + "Otherwise restore the database backup taken before the upgrade and run the upgrade again." +); + const ALREADY_DROPPED_RECOVERY: &str = concat!( "\n\nThe permission values cannot be recomputed from the current schema. Restore the database backup taken ", "before the upgrade and run the upgrade again against that restored copy." @@ -619,10 +637,13 @@ impl CustomRoleMigrationFacts { enum CustomRolePreflightDecision { Proceed, CompleteMysqlCollectionMigration, + CompleteInterruptedAccessAllDrop, RefuseAlreadyDropped, RefuseMissingAccessAll, RefuseMissingMigrationLedger, RefuseAmbiguousDirectPermissions, + RefuseInterruptedAccessAllDrop, + RefuseAccessAllDropLedgerMismatch, RefusePartialPermissionSchema(PermissionColumnGroup), RefusePermissionLedgerMismatch(PermissionColumnGroup), } @@ -631,24 +652,48 @@ fn custom_role_preflight_decision( facts: CustomRoleMigrationFacts, can_complete_mysql_partial_migration: bool, ) -> CustomRolePreflightDecision { - if !facts.memberships_table_exists || facts.repair_migration_applied { + if !facts.memberships_table_exists { 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; - } + // The legacy reconstruction below only makes sense while the repair migration is still ahead of + // us. Everything *after* it -- the access_all drop and the third permission column group -- still + // has to be checked on every start: both run after the repair, and on MySQL/MariaDB each DDL + // statement commits on its own, so a crash between the statement and Diesel's ledger insert + // leaves a durable partial state. Returning early for every repaired database would hide exactly + // those states, and the generic Diesel retry then fails on every following start with + // `Unknown column` (1091) or `Duplicate column name` (1060). + if facts.repair_migration_applied { + // The drop is a single statement with no data component, so it is all-or-nothing: either the + // column is still there and the migration is pending, or the column is gone and the + // migration is recorded. + if facts.access_all_column_exists == facts.access_all_drop_migration_applied { + return if facts.access_all_drop_migration_applied { + CustomRolePreflightDecision::RefuseAccessAllDropLedgerMismatch + } else if can_complete_mysql_partial_migration { + // Only reachable on MySQL/MariaDB, and the schema is already in its intended final + // state -- just record the migration instead of stopping the operator. + CustomRolePreflightDecision::CompleteInterruptedAccessAllDrop + } else { + CustomRolePreflightDecision::RefuseInterruptedAccessAllDrop + }; + } + } else { + // 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.ambiguous_direct_permission_count != 0 && !facts.same_run_0716_marker { - return CustomRolePreflightDecision::RefuseAmbiguousDirectPermissions; + if facts.ambiguous_direct_permission_count != 0 && !facts.same_run_0716_marker { + return CustomRolePreflightDecision::RefuseAmbiguousDirectPermissions; + } } // Every permission column group must be either completely absent (its migration is still pending) @@ -708,7 +753,18 @@ fn custom_role_preflight_error(decision: CustomRolePreflightDecision, facts: Cus group.description(), group.column_list() ), - CustomRolePreflightDecision::Proceed | CustomRolePreflightDecision::CompleteMysqlCollectionMigration => { + CustomRolePreflightDecision::RefuseInterruptedAccessAllDrop => format!( + "The membership access_all column is already gone, but migration \ + {DROP_MEMBERSHIP_ACCESS_ALL_MIGRATION} is not recorded. The column was dropped without \ + its ledger entry, so re-running the migration would fail on every start." + ), + CustomRolePreflightDecision::RefuseAccessAllDropLedgerMismatch => format!( + "Migration {DROP_MEMBERSHIP_ACCESS_ALL_MIGRATION} is recorded, but the membership \ + access_all column still exists. Schema and migration ledger disagree." + ), + CustomRolePreflightDecision::Proceed + | CustomRolePreflightDecision::CompleteMysqlCollectionMigration + | CustomRolePreflightDecision::CompleteInterruptedAccessAllDrop => { unreachable!("successful preflight decisions do not produce errors") } }; @@ -717,6 +773,8 @@ fn custom_role_preflight_error(decision: CustomRolePreflightDecision, facts: Cus CustomRolePreflightDecision::RefusePartialPermissionSchema(_) | CustomRolePreflightDecision::RefusePermissionLedgerMismatch(_) => PARTIAL_PERMISSION_COLUMNS_RECOVERY, CustomRolePreflightDecision::RefuseAlreadyDropped => ALREADY_DROPPED_RECOVERY, + CustomRolePreflightDecision::RefuseInterruptedAccessAllDrop => INTERRUPTED_ACCESS_ALL_DROP_RECOVERY, + CustomRolePreflightDecision::RefuseAccessAllDropLedgerMismatch => ACCESS_ALL_DROP_MISMATCH_RECOVERY, _ => "", }; @@ -1026,6 +1084,23 @@ mod mysql_migrations { Ok(()) } + fn complete_interrupted_access_all_drop( + connection: &mut diesel::mysql::MysqlConnection, + ) -> Result<(), super::Error> { + // MySQL/MariaDB commit DDL implicitly, so the single `ALTER TABLE ... DROP COLUMN access_all` + // can be durable while Diesel's ledger insert that follows it is not. Re-running the + // migration would then fail with error 1091 (Unknown column) on every start. The statement + // has no data component and the preflight has just confirmed the column is gone, so the + // schema already is what the migration wanted: record it and let the rest of the chain run. + diesel::sql_query(format!( + "INSERT INTO __diesel_schema_migrations (version) VALUES ('{}')", + super::DROP_MEMBERSHIP_ACCESS_ALL_MIGRATION + )) + .execute(connection)?; + + Ok(()) + } + fn preflight(connection: &mut diesel::mysql::MysqlConnection) -> Result<(), super::Error> { let memberships_table_exists = table_exists(connection, "users_organizations")?; if !memberships_table_exists { @@ -1107,6 +1182,9 @@ mod mysql_migrations { super::CustomRolePreflightDecision::CompleteMysqlCollectionMigration => { complete_partial_collection_migration(connection, same_run_0716_marker) } + super::CustomRolePreflightDecision::CompleteInterruptedAccessAllDrop => { + complete_interrupted_access_all_drop(connection) + } decision => Err(super::custom_role_preflight_error(decision, facts)), } } @@ -1305,17 +1383,107 @@ mod custom_role_migration_preflight_tests { ); } + /// A database on which the whole chain has already run. + fn fully_migrated() -> Facts { + Facts { + memberships_table_exists: true, + migration_table_exists: true, + access_all_column_exists: false, + manage_permission_columns: 3, + manage_permissions_migration_applied: true, + collection_permission_columns: 3, + collection_permissions_migration_applied: true, + access_permission_columns: 3, + access_permissions_migration_applied: true, + repair_migration_applied: true, + access_all_drop_migration_applied: true, + ambiguous_direct_permission_count: 0, + same_run_0716_marker: false, + } + } + #[test] fn repair_marker_makes_completed_state_idempotent() { + assert_eq!(custom_role_preflight_decision(fully_migrated(), false), Decision::Proceed); + } + + /// The repair migration runs *before* the access_all drop and the third permission column group, + /// so a partial state of either always carries `repair_migration_applied`. Skipping the schema + /// checks for repaired databases would make them unreachable in exactly the situation they were + /// written for. + #[test] + fn interrupted_migrations_after_the_repair_are_still_detected() { + // Crash after `DROP COLUMN access_all`, before the ledger insert. MySQL/MariaDB commit DDL + // implicitly, so the column is gone for good; a retry would fail with 1091. + let interrupted_drop = Facts { + access_all_drop_migration_applied: false, + access_permission_columns: 0, + access_permissions_migration_applied: false, + ..fully_migrated() + }; + assert_eq!( + custom_role_preflight_decision(interrupted_drop, true), + Decision::CompleteInterruptedAccessAllDrop, + "MySQL/MariaDB can complete this in place" + ); + assert_eq!( + custom_role_preflight_decision(interrupted_drop, false), + Decision::RefuseInterruptedAccessAllDrop, + "backends with transactional DDL cannot reach this state by themselves" + ); + + // Crash after one of the three `ADD COLUMN` statements of the access group, before the + // ledger insert. A retry would fail with 1060. + for present in [1, 2] { + assert_eq!( + custom_role_preflight_decision( + Facts { + access_permission_columns: present, + access_permissions_migration_applied: false, + ..fully_migrated() + }, + true, + ), + Decision::RefusePartialPermissionSchema(super::PermissionColumnGroup::Access) + ); + } + + // Ledger recorded, columns missing. 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() + access_permission_columns: 2, + ..fully_migrated() + }, + true + ), + Decision::RefusePermissionLedgerMismatch(super::PermissionColumnGroup::Access) + ); + + // Drop recorded, but the column is back: schema and ledger disagree. + assert_eq!( + custom_role_preflight_decision( + Facts { + access_all_column_exists: true, + ..fully_migrated() + }, + true + ), + Decision::RefuseAccessAllDropLedgerMismatch + ); + } + + #[test] + fn a_pending_drop_after_the_repair_proceeds() { + // The repair ran, the drop is simply next in line: column present, migration not recorded. + assert_eq!( + custom_role_preflight_decision( + Facts { + access_all_column_exists: true, + access_all_drop_migration_applied: false, + access_permission_columns: 0, + access_permissions_migration_applied: false, + ..fully_migrated() }, false, ), @@ -1323,6 +1491,21 @@ mod custom_role_migration_preflight_tests { ); } + #[test] + fn interrupted_access_all_drop_error_names_the_ledger_fix() { + let facts = Facts { + access_all_drop_migration_applied: false, + access_permission_columns: 0, + access_permissions_migration_applied: false, + ..fully_migrated() + }; + let decision = custom_role_preflight_decision(facts, false); + 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(super::DROP_MEMBERSHIP_ACCESS_ALL_MIGRATION)); + assert!(message.contains("INSERT INTO __diesel_schema_migrations")); + } + #[test] fn a_historical_drop_without_the_repair_is_refused() { assert_eq!( diff --git a/src/db/models/collection.rs b/src/db/models/collection.rs index e61e9bfc..9b33dd43 100644 --- a/src/db/models/collection.rs +++ b/src/db/models/collection.rs @@ -135,12 +135,26 @@ impl Collection { // 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) => { + // A legacy organization-local `access_all` group confers collection management + // on its Custom members (see `has_legacy_group_collection_manage_access`), and + // reaches every collection without a `collections_groups` row that could carry + // the `manage` bit — so it has to be answered from the membership side. + let legacy_group_manage = m.has_type(MembershipType::Custom) + && cipher_sync_data.user_group_full_access_for_organizations.contains(&self.org_uuid); if let Some(cu) = cipher_sync_data.user_collections.get(&self.uuid) { - (cu.read_only, cu.hide_passwords, assignment_manage_for_member(m.atype, cu.manage)) + ( + cu.read_only, + cu.hide_passwords, + legacy_group_manage || 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, assignment_manage_for_member(m.atype, cg.manage)) + ( + cg.read_only, + cg.hide_passwords, + legacy_group_manage || assignment_manage_for_member(m.atype, cg.manage), + ) } else { - (false, false, false) + (false, false, legacy_group_manage) } } _ => (true, true, false), @@ -150,7 +164,7 @@ impl Collection { 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 => + && m.has_collection_manage_authority(&self.uuid, conn).await => { (false, false, true) } diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index f74e0aca..60082bc2 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -964,6 +964,61 @@ impl Membership { .await } + /// Legacy collection-management authority derived from an organization-local `access_all` group. + /// + /// Before this role model existed, a Manager who reached every collection through such a group + /// could edit and delete all of them — `Collection::is_coll_manageable_by_user` accepted + /// `groups.access_all` outright. Managers are Custom members now, so that authority has to keep + /// coming from the same place, or the upgrade would silently strip a capability from members who + /// hold no explicit per-collection grant. Deriving it live (instead of copying it into the + /// permission columns during the migration) is what keeps it revocable: remove the member from + /// the group, or clear the group's `access_all`, and the authority is gone with it. + /// + /// Deliberately not collection *creation*: that historically required membership-level + /// `access_all` and is now the independent `create_new_collections` permission. + pub async fn has_legacy_group_collection_manage_access( + &self, + collection_uuid: &CollectionId, + conn: &DbConn, + ) -> bool { + let membership_uuid = self.uuid.clone(); + let user_uuid = self.user_uuid.clone(); + let org_uuid = self.org_uuid.clone(); + let collection_uuid = collection_uuid.clone(); + + conn.run(move |conn| { + users_organizations::table + .inner_join( + groups_users::table.on(groups_users::users_organizations_uuid.eq(users_organizations::uuid)), + ) + .inner_join( + groups::table.on(groups::uuid + .eq(groups_users::groups_uuid) + .and(groups::organizations_uuid.eq(users_organizations::org_uuid))), + ) + .inner_join(collections::table.on(collections::org_uuid.eq(users_organizations::org_uuid))) + .filter(users_organizations::uuid.eq(membership_uuid)) + .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(groups::access_all.eq(true)) + .count() + .first::(conn) + .unwrap_or(0) + != 0 + }) + .await + } + + /// Whether this member may manage `collection_uuid` without holding a blanket collection + /// permission: either a real stored per-collection grant, or the legacy full-access group. + pub async fn has_collection_manage_authority(&self, collection_uuid: &CollectionId, conn: &DbConn) -> bool { + self.has_explicit_collection_manage_access(collection_uuid, conn).await + || self.has_legacy_group_collection_manage_access(collection_uuid, conn).await + } + /// `manageAllCollections` is a client-side aggregate checkbox, not a separately persisted /// Bitwarden permission. It is selected exactly when all three child permissions are selected. pub fn has_manage_all_collections(&self) -> bool {