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