diff --git a/migrations/mysql/2026-07-24-140000_guard_custom_role_downgrade/down.sql b/migrations/mysql/2026-07-24-140000_guard_custom_role_downgrade/down.sql index 29b831e3..93358747 100644 --- a/migrations/mysql/2026-07-24-140000_guard_custom_role_downgrade/down.sql +++ b/migrations/mysql/2026-07-24-140000_guard_custom_role_downgrade/down.sql @@ -21,3 +21,7 @@ WHERE NOT EXISTS ( WHERE table_schema = DATABASE() AND table_name = '__vw_allow_custom_role_downgrade' ); DROP TABLE __vw_custom_role_downgrade_guard; + +-- Consume the acknowledgement: it authorized *this* revert, not every future one. After a +-- re-upgrade the next revert has to be acknowledged again. +DROP TABLE IF EXISTS __vw_allow_custom_role_downgrade; 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 f3f3a7a7..9079f661 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 @@ -5,3 +5,7 @@ -- (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; + +-- Also clear a downgrade acknowledgement left over from an earlier revert, so consent is +-- never inherited across an upgrade. +DROP TABLE IF EXISTS __vw_allow_custom_role_downgrade; diff --git a/migrations/postgresql/2026-07-24-140000_guard_custom_role_downgrade/down.sql b/migrations/postgresql/2026-07-24-140000_guard_custom_role_downgrade/down.sql index 24760fc4..787d60dc 100644 --- a/migrations/postgresql/2026-07-24-140000_guard_custom_role_downgrade/down.sql +++ b/migrations/postgresql/2026-07-24-140000_guard_custom_role_downgrade/down.sql @@ -18,3 +18,7 @@ INSERT INTO __vw_custom_role_downgrade_guard (blocked) SELECT 1 WHERE to_regclass('__vw_allow_custom_role_downgrade') IS NULL; DROP TABLE __vw_custom_role_downgrade_guard; + +-- Consume the acknowledgement: it authorized *this* revert, not every future one. After a +-- re-upgrade the next revert has to be acknowledged again. +DROP TABLE IF EXISTS __vw_allow_custom_role_downgrade; 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 f3f3a7a7..9079f661 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 @@ -5,3 +5,7 @@ -- (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; + +-- Also clear a downgrade acknowledgement left over from an earlier revert, so consent is +-- never inherited across an upgrade. +DROP TABLE IF EXISTS __vw_allow_custom_role_downgrade; diff --git a/migrations/sqlite/2026-07-24-140000_guard_custom_role_downgrade/down.sql b/migrations/sqlite/2026-07-24-140000_guard_custom_role_downgrade/down.sql index 8d149ae3..b6fb06ac 100644 --- a/migrations/sqlite/2026-07-24-140000_guard_custom_role_downgrade/down.sql +++ b/migrations/sqlite/2026-07-24-140000_guard_custom_role_downgrade/down.sql @@ -21,3 +21,7 @@ WHERE NOT EXISTS ( WHERE type = 'table' AND name = '__vw_allow_custom_role_downgrade' ); DROP TABLE __vw_custom_role_downgrade_guard; + +-- Consume the acknowledgement: it authorized *this* revert, not every future one. After a +-- re-upgrade the next revert has to be acknowledged again. +DROP TABLE IF EXISTS __vw_allow_custom_role_downgrade; 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 f3f3a7a7..9079f661 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 @@ -5,3 +5,7 @@ -- (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; + +-- Also clear a downgrade acknowledgement left over from an earlier revert, so consent is +-- never inherited across an upgrade. +DROP TABLE IF EXISTS __vw_allow_custom_role_downgrade; diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index 13c915c5..fa4e7a4b 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -3420,13 +3420,14 @@ 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 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. + // Custom without delete_any: the answer is per-collection and must reflect a *real* stored + // manage grant. Edit any collection deliberately does not count here, and neither does the + // legacy `access_all`-group authority: that one is derived from a group membership that can + // be taken away again, while a `manage` row written here outlives it. Accepting it would let + // temporary authority be laundered into a permanent grant — and with it collection deletion + // — which is exactly the escalation this clamp exists to prevent. None => match MembershipType::from_i32(caller.atype) { - Some(MembershipType::Custom) => caller.has_collection_manage_authority(col_id, conn).await, + Some(MembershipType::Custom) => caller.has_explicit_collection_manage_access(col_id, conn).await, _ => false, }, } diff --git a/src/db/models/collection.rs b/src/db/models/collection.rs index 9b33dd43..e6f74557 100644 --- a/src/db/models/collection.rs +++ b/src/db/models/collection.rs @@ -131,37 +131,52 @@ impl Collection { ) -> Value { let (read_only, hide_passwords, manage) = if let Some(cipher_sync_data) = cipher_sync_data { match cipher_sync_data.members.get(&self.org_uuid) { - // Full collection visibility is not collection-management authority. Admins and - // Owners manage implicitly; Custom members still need an explicit stored grant. - Some(m) if m.has_full_access() => (false, false, assignment_manage_for_member(m.atype, false)), Some(m) => { + // What the client is told here has to match what the collection guards actually + // allow, or it renders the wrong controls. A stored grant therefore counts even + // for a member who already reaches every collection: full visibility is not + // management authority, but it does not cancel out a real grant either. + // // 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) + && !m.has_create_new_collections() + && !m.has_edit_any_collection() + && !m.has_delete_any_collection() && 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, - 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, - legacy_group_manage || assignment_manage_for_member(m.atype, cg.manage), - ) - } else { - (false, false, legacy_group_manage) + let assignment = cipher_sync_data + .user_collections + .get(&self.uuid) + .map(|cu| (cu.read_only, cu.hide_passwords, cu.manage)) + .or_else(|| { + cipher_sync_data + .user_collections_groups + .get(&self.uuid) + .map(|cg| (cg.read_only, cg.hide_passwords, cg.manage)) + }); + let stored_manage = assignment.is_some_and(|(_, _, manage)| manage); + let manage = legacy_group_manage || assignment_manage_for_member(m.atype, stored_manage); + match assignment { + Some((read_only, hide_passwords, _)) if !m.has_full_access() => { + (read_only, hide_passwords, manage) + } + // Reaching every collection means nothing is read-only or hidden here. + _ => (false, false, manage), } } _ => (true, true, false), } } else { match Membership::find_confirmed_by_user_and_org(user_uuid, &self.org_uuid, conn).await { - Some(m) if m.has_full_access() => (false, false, assignment_manage_for_member(m.atype, false)), + // Same rule as the cached branch above: a member who reaches every collection still + // reports a real stored grant, so the serialized value matches the guards. + Some(m) if m.has_full_access() => ( + false, + false, + assignment_manage_for_member(m.atype, m.has_collection_manage_authority(&self.uuid, conn).await), + ), Some(m) if m.atype >= MembershipType::Custom && m.has_collection_manage_authority(&self.uuid, conn).await => diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index 60082bc2..d33e9b90 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -976,11 +976,22 @@ impl Membership { /// /// Deliberately not collection *creation*: that historically required membership-level /// `access_all` and is now the independent `create_new_collections` permission. + /// + /// Security: the exception is limited to members holding *none* of the three collection + /// permissions, which is exactly the shape the migration leaves a group-derived legacy Manager + /// in. Without that limit it would also cover a Custom member holding `edit_any_collection` — + /// and since `edit_any_collection` is what lets a caller create an `access_all` group in the + /// first place, such a member could grant themselves this authority and use it to persist a + /// real `collections_groups.manage` row, keeping collection deletion after leaving the group. pub async fn has_legacy_group_collection_manage_access( &self, collection_uuid: &CollectionId, conn: &DbConn, ) -> bool { + if self.create_new_collections || self.edit_any_collection || self.delete_any_collection { + return false; + } + let membership_uuid = self.uuid.clone(); let user_uuid = self.user_uuid.clone(); let org_uuid = self.org_uuid.clone(); diff --git a/tools/custom_role_rollback/README.md b/tools/custom_role_rollback/README.md index e475ff5f..7e47a8f4 100644 --- a/tools/custom_role_rollback/README.md +++ b/tools/custom_role_rollback/README.md @@ -47,7 +47,7 @@ Stop every Vaultwarden instance and take a backup first. Then: ```bash # SQLite -sqlite3 /path/to/data/db.sqlite3 < tools/custom_role_rollback/sqlite.sql +sqlite3 -bail /path/to/data/db.sqlite3 < tools/custom_role_rollback/sqlite.sql # MySQL / MariaDB mysql -u -p < tools/custom_role_rollback/mysql.sql @@ -56,10 +56,18 @@ mysql -u -p < tools/custom_role_rollback/mysql.sql psql -U -d -v ON_ERROR_STOP=1 -f tools/custom_role_rollback/postgresql.sql ``` -Run it exactly once — a second run fails because the permission columns are already gone. On -MySQL/MariaDB the statements cannot be wrapped in a transaction (DDL commits implicitly there); if -the script is interrupted, restore the backup and start over. SQLite and PostgreSQL apply the whole -script atomically. +Each script stops on its own if the database is not in the state it converts from, so running one +twice is refused rather than half-applied. + +**Do not drop the `-bail` / `ON_ERROR_STOP=1` flags, and do not run these through a client that +keeps going after a failed statement.** The sqlite3 shell continues after errors by default; the +script sets `.bail on` itself, but that is a shell command a different runner will ignore. A runner +that carries on past the failing statement would reach the `DROP TABLE` and commit an empty +`users_organizations`. + +SQLite and PostgreSQL apply the script in a single transaction, so an aborted run leaves the +database untouched. On MySQL/MariaDB the statements cannot be wrapped in a transaction (DDL commits +implicitly there); if the script is interrupted, restore the backup and start over. Afterwards start the older Vaultwarden version. Upgrading again later re-applies the seven migrations from a clean state. diff --git a/tools/custom_role_rollback/sqlite.sql b/tools/custom_role_rollback/sqlite.sql index e95a1cee..54cb237b 100644 --- a/tools/custom_role_rollback/sqlite.sql +++ b/tools/custom_role_rollback/sqlite.sql @@ -6,10 +6,30 @@ -- script has to work on the same older system SQLite the forward migrations support. Rebuilding the -- table also recreates `access_all` and drops all nine permission columns in one step. +-- Stop at the first error. Without this the sqlite3 shell keeps going after a failed statement, +-- and a second run -- where the SELECT below can no longer see the permission columns -- would +-- still reach DROP TABLE and commit an empty users_organizations. `.bail on` is a shell command; +-- a runner that is not the sqlite3 CLI has to abort on the first error and roll back by itself. +.bail on + PRAGMA foreign_keys = OFF; BEGIN; +-- Refuse to start at all unless the database is in the state this script converts *from*. A repeat +-- run would otherwise only fail somewhere in the middle. The failing CHECK names the reason. +CREATE TEMPORARY TABLE __vw_rollback_precondition ( + ok INTEGER NOT NULL CONSTRAINT + this_database_has_no_custom_role_permission_columns_to_roll_back CHECK (ok = 1) +); +INSERT INTO __vw_rollback_precondition (ok) +SELECT CASE + WHEN EXISTS (SELECT 1 FROM pragma_table_info('users_organizations') WHERE name = 'create_new_collections') + THEN 1 + ELSE 0 +END; +DROP TABLE __vw_rollback_precondition; + CREATE TABLE users_organizations_rollback ( uuid TEXT NOT NULL PRIMARY KEY, user_uuid TEXT NOT NULL REFERENCES users (uuid),