Browse Source

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.
pull/7397/head
tom27052006 6 days ago
parent
commit
634d9b2bd3
  1. 4
      migrations/mysql/2026-07-23-120000_reconcile_legacy_custom_roles/up.sql
  2. 4
      migrations/postgresql/2026-07-23-120000_reconcile_legacy_custom_roles/up.sql
  3. 4
      migrations/sqlite/2026-07-23-120000_reconcile_legacy_custom_roles/up.sql
  4. 43
      migrations/sqlite/2026-07-24-120000_drop_membership_access_all/up.sql
  5. 26
      src/api/core/organizations.rs
  6. 16
      src/auth.rs
  7. 219
      src/db/mod.rs
  8. 22
      src/db/models/collection.rs
  9. 55
      src/db/models/organization.rs

4
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 -- 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, -- 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 UPDATE users_organizations
SET edit_any_collection = FALSE, SET edit_any_collection = FALSE,
delete_any_collection = FALSE delete_any_collection = FALSE

4
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 -- 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, -- 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 UPDATE users_organizations
SET edit_any_collection = FALSE, SET edit_any_collection = FALSE,
delete_any_collection = FALSE delete_any_collection = FALSE

4
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 -- 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, -- 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 UPDATE users_organizations
SET edit_any_collection = FALSE, SET edit_any_collection = FALSE,
delete_any_collection = FALSE delete_any_collection = FALSE

43
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 -- 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. -- 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. -- 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;

26
src/api/core/organizations.rs

@ -2324,13 +2324,15 @@ async fn post_org_import(
err!("Organization not found", "Organization id's do not match"); err!("Organization not found", "Organization id's do not match");
} }
// NOTE: no `accessImportExport` gate here on purpose. Bitwarden requires that permission for an // NOTE: no `accessImportExport` gate here on purpose. Bitwarden does not require the permission
// organization import, but Vaultwarden has always authorized this endpoint per target collection, // either — `ImportCiphersController.CheckOrgImportPermissionAsync` authorizes an organization
// and adding an up-front role check would take a capability away from ordinary members that they // import on `AccessImportExport` *or* per-collection Create/ImportCiphers authority. Vaultwarden
// have today. The real boundary is enforced below and is unchanged: an existing collection must be // has always authorized this endpoint per target collection, so an up-front role check would take
// writable for the caller (`Collection::is_writable_by_user`), and creating a new one requires the // a capability away from ordinary members that they have today. The real boundary is enforced
// independent `createNewCollections` permission. `accessImportExport` therefore governs the export // below and is unchanged: an existing collection must be writable for the caller
// side only. // (`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 // 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 // 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) { match caller_manage_grant_role_check(caller) {
// Role alone decides it (Admin/Owner or delete_any -> yes; User/unknown/unconfirmed -> no). // Role alone decides it (Admin/Owner or delete_any -> yes; User/unknown/unconfirmed -> no).
Some(decision) => decision, Some(decision) => decision,
// Custom without delete_any: the answer is per-collection and must reflect a *real* manage // Custom without delete_any: the answer is per-collection and must mirror
// grant. A Custom member must prove a real users_collections.manage / // `collection_delete_access` exactly, so it can never hand out a right the caller lacks —
// collections_groups.manage grant; Edit any collection deliberately does not count here. // 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) { 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, _ => false,
}, },
} }

16
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) { match MembershipType::from_i32(membership.atype) {
Some(MembershipType::Owner | MembershipType::Admin) => CollectionManageAccess::Any, Some(MembershipType::Owner | MembershipType::Admin) => CollectionManageAccess::Any,
Some(MembershipType::Custom) if custom_has_any_access => CollectionManageAccess::Any, Some(MembershipType::Custom) if custom_has_any_access => CollectionManageAccess::Any,
// A Custom member must prove an actual users_collections.manage or // A Custom member must prove an actual users_collections.manage / collections_groups.manage
// collections_groups.manage assignment. In particular, groups.access_all is not 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::Custom) => CollectionManageAccess::ExplicitManage,
Some(MembershipType::User) | None => CollectionManageAccess::Denied, Some(MembershipType::User) | None => CollectionManageAccess::Denied,
} }
@ -1039,7 +1040,7 @@ async fn can_manage_collection(
match access { match access {
CollectionManageAccess::Any => true, CollectionManageAccess::Any => true,
CollectionManageAccess::ExplicitManage => { CollectionManageAccess::ExplicitManage => {
membership.has_explicit_collection_manage_access(collection_uuid, conn).await membership.has_collection_manage_authority(collection_uuid, conn).await
} }
CollectionManageAccess::Denied => false, CollectionManageAccess::Denied => false,
} }
@ -1695,10 +1696,11 @@ mod tests {
#[test] #[test]
fn flagless_custom_requires_explicit_manage_for_edit_read_and_delete() { 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 // A flagless Custom member (this is what a migrated legacy Manager becomes) never gets
// real per-collection Manage grant for every collection operation. ExplicitManage invokes // blanket collection authority from its role alone: every collection operation has to be
// the database helper that only accepts users_collections.manage / collections_groups.manage // answered per collection. ExplicitManage invokes the database helper that accepts a real
// — an external groups.access_all grant deliberately does not switch to a broad helper. // 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); let custom = membership(MembershipType::Custom);
assert_eq!(collection_edit_access(&custom), CollectionManageAccess::ExplicitManage); assert_eq!(collection_edit_access(&custom), CollectionManageAccess::ExplicitManage);
assert_eq!(collection_read_access(&custom), CollectionManageAccess::ExplicitManage); assert_eq!(collection_read_access(&custom), CollectionManageAccess::ExplicitManage);

219
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 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." "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!( const ALREADY_DROPPED_RECOVERY: &str = concat!(
"\n\nThe permission values cannot be recomputed from the current schema. Restore the database backup taken ", "\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." "before the upgrade and run the upgrade again against that restored copy."
@ -619,10 +637,13 @@ impl CustomRoleMigrationFacts {
enum CustomRolePreflightDecision { enum CustomRolePreflightDecision {
Proceed, Proceed,
CompleteMysqlCollectionMigration, CompleteMysqlCollectionMigration,
CompleteInterruptedAccessAllDrop,
RefuseAlreadyDropped, RefuseAlreadyDropped,
RefuseMissingAccessAll, RefuseMissingAccessAll,
RefuseMissingMigrationLedger, RefuseMissingMigrationLedger,
RefuseAmbiguousDirectPermissions, RefuseAmbiguousDirectPermissions,
RefuseInterruptedAccessAllDrop,
RefuseAccessAllDropLedgerMismatch,
RefusePartialPermissionSchema(PermissionColumnGroup), RefusePartialPermissionSchema(PermissionColumnGroup),
RefusePermissionLedgerMismatch(PermissionColumnGroup), RefusePermissionLedgerMismatch(PermissionColumnGroup),
} }
@ -631,24 +652,48 @@ fn custom_role_preflight_decision(
facts: CustomRoleMigrationFacts, facts: CustomRoleMigrationFacts,
can_complete_mysql_partial_migration: bool, can_complete_mysql_partial_migration: bool,
) -> CustomRolePreflightDecision { ) -> CustomRolePreflightDecision {
if !facts.memberships_table_exists || facts.repair_migration_applied { if !facts.memberships_table_exists {
return CustomRolePreflightDecision::Proceed; return CustomRolePreflightDecision::Proceed;
} }
if !facts.migration_table_exists { if !facts.migration_table_exists {
return CustomRolePreflightDecision::RefuseMissingMigrationLedger; return CustomRolePreflightDecision::RefuseMissingMigrationLedger;
} }
// Once access_all has been dropped, its former value and the provenance of 0/1/1 // The legacy reconstruction below only makes sense while the repair migration is still ahead of
// collection permissions can no longer be reconstructed. Never guess at either. // us. Everything *after* it -- the access_all drop and the third permission column group -- still
if facts.access_all_drop_migration_applied { // has to be checked on every start: both run after the repair, and on MySQL/MariaDB each DDL
return CustomRolePreflightDecision::RefuseAlreadyDropped; // 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
if !facts.access_all_column_exists { // those states, and the generic Diesel retry then fails on every following start with
return CustomRolePreflightDecision::RefuseMissingAccessAll; // `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 { if facts.ambiguous_direct_permission_count != 0 && !facts.same_run_0716_marker {
return CustomRolePreflightDecision::RefuseAmbiguousDirectPermissions; return CustomRolePreflightDecision::RefuseAmbiguousDirectPermissions;
}
} }
// Every permission column group must be either completely absent (its migration is still pending) // 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.description(),
group.column_list() 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") unreachable!("successful preflight decisions do not produce errors")
} }
}; };
@ -717,6 +773,8 @@ fn custom_role_preflight_error(decision: CustomRolePreflightDecision, facts: Cus
CustomRolePreflightDecision::RefusePartialPermissionSchema(_) CustomRolePreflightDecision::RefusePartialPermissionSchema(_)
| CustomRolePreflightDecision::RefusePermissionLedgerMismatch(_) => PARTIAL_PERMISSION_COLUMNS_RECOVERY, | CustomRolePreflightDecision::RefusePermissionLedgerMismatch(_) => PARTIAL_PERMISSION_COLUMNS_RECOVERY,
CustomRolePreflightDecision::RefuseAlreadyDropped => ALREADY_DROPPED_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(()) 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> { fn preflight(connection: &mut diesel::mysql::MysqlConnection) -> Result<(), super::Error> {
let memberships_table_exists = table_exists(connection, "users_organizations")?; let memberships_table_exists = table_exists(connection, "users_organizations")?;
if !memberships_table_exists { if !memberships_table_exists {
@ -1107,6 +1182,9 @@ mod mysql_migrations {
super::CustomRolePreflightDecision::CompleteMysqlCollectionMigration => { super::CustomRolePreflightDecision::CompleteMysqlCollectionMigration => {
complete_partial_collection_migration(connection, same_run_0716_marker) 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)), 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] #[test]
fn repair_marker_makes_completed_state_idempotent() { 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!( assert_eq!(
custom_role_preflight_decision( custom_role_preflight_decision(
Facts { Facts {
memberships_table_exists: true, access_permission_columns: 2,
migration_table_exists: true, ..fully_migrated()
repair_migration_applied: true, },
access_all_drop_migration_applied: true, true
collection_permission_columns: 3, ),
..Facts::default() 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, 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] #[test]
fn a_historical_drop_without_the_repair_is_refused() { fn a_historical_drop_without_the_repair_is_refused() {
assert_eq!( assert_eq!(

22
src/db/models/collection.rs

@ -135,12 +135,26 @@ impl Collection {
// Owners manage implicitly; Custom members still need an explicit stored grant. // 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) if m.has_full_access() => (false, false, assignment_manage_for_member(m.atype, false)),
Some(m) => { 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) { 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) { } 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 { } else {
(false, false, false) (false, false, legacy_group_manage)
} }
} }
_ => (true, true, false), _ => (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.has_full_access() => (false, false, assignment_manage_for_member(m.atype, false)),
Some(m) Some(m)
if m.atype >= MembershipType::Custom 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) (false, false, true)
} }

55
src/db/models/organization.rs

@ -964,6 +964,61 @@ impl Membership {
.await .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::<i64>(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 /// `manageAllCollections` is a client-side aggregate checkbox, not a separately persisted
/// Bitwarden permission. It is selected exactly when all three child permissions are selected. /// Bitwarden permission. It is selected exactly when all three child permissions are selected.
pub fn has_manage_all_collections(&self) -> bool { pub fn has_manage_all_collections(&self) -> bool {

Loading…
Cancel
Save