Browse Source

Fix custom-role audit findings

Type-gate edit-any queries and add legacy access_all recovery SQL.
pull/7397/head
tom27052006 2 weeks ago
parent
commit
ceeba0bf35
  1. 64
      src/db/mod.rs
  2. 21
      src/db/models/cipher.rs
  3. 18
      src/db/models/collection.rs
  4. 52
      src/db/models/organization.rs

64
src/db/mod.rs

@ -472,6 +472,26 @@ const CUSTOM_ROLE_REPAIR_MIGRATION: &str = "20260723120000";
const CUSTOM_COLLECTION_PERMISSIONS_MIGRATION: &str = "20260716120000"; const CUSTOM_COLLECTION_PERMISSIONS_MIGRATION: &str = "20260716120000";
const DROP_MEMBERSHIP_ACCESS_ALL_MIGRATION: &str = "20260724120000"; 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_SAME_RUN_MARKER_TABLE: &str = "__vw_custom_role_same_run_0716";
const LEGACY_USER_ACCESS_ALL_RECOVERY_SQL: &str = concat!(
"\n\nReview every affected membership with this SQLite/MySQL/PostgreSQL-compatible query:\n",
"SELECT uuid, user_uuid, org_uuid, status\n",
"FROM users_organizations\n",
"WHERE atype = 2 AND access_all = TRUE;\n\n",
"After an organization owner has decided the intended outcome, replace <MEMBERSHIP_UUID> and run exactly one ",
"guarded statement for that membership while every Vaultwarden instance is stopped. Do not bulk-promote these ",
"records.\n\n",
"Keep the User role and revoke organization-wide vault access:\n",
"UPDATE users_organizations\n",
"SET access_all = FALSE\n",
"WHERE uuid = '<MEMBERSHIP_UUID>' AND atype = 2 AND access_all = TRUE;\n\n",
"Preserve organization-wide vault access by intentionally granting Custom Create/Edit/Delete-any collection ",
"authority:\n",
"UPDATE users_organizations\n",
"SET atype = 3\n",
"WHERE uuid = '<MEMBERSHIP_UUID>' AND atype = 2 AND access_all = TRUE;\n\n",
"The second statement deliberately adds collection-management authority: the repair migration copies the retained ",
"access_all value to all three collection permissions before converting legacy role 3 to Custom role 4."
);
#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] #[derive(Clone, Copy, Debug, Default, Eq, PartialEq)]
#[expect( #[expect(
@ -585,10 +605,15 @@ fn custom_role_preflight_error(decision: CustomRolePreflightDecision, facts: Cus
unreachable!("successful preflight decisions do not produce errors") unreachable!("successful preflight decisions do not produce errors")
} }
}; };
let recovery = if decision == CustomRolePreflightDecision::RefuseLegacyUserAccessAll {
LEGACY_USER_ACCESS_ALL_RECOVERY_SQL
} else {
""
};
std::io::Error::other(format!( std::io::Error::other(format!(
"Custom-role migration preflight stopped startup: {detail} Back up the database and resolve \ "Custom-role migration preflight stopped startup: {detail} Back up the database and resolve \
the legacy membership state manually before restarting." the legacy membership state manually before restarting.{recovery}"
)) ))
.into() .into()
} }
@ -1122,9 +1147,11 @@ mod postgresql_migrations {
#[cfg(test)] #[cfg(test)]
mod custom_role_migration_preflight_tests { mod custom_role_migration_preflight_tests {
use std::error::Error as _;
use super::{ use super::{
CustomRoleMigrationFacts as Facts, CustomRolePreflightDecision as Decision, custom_role_preflight_decision, CustomRoleMigrationFacts as Facts, CustomRolePreflightDecision as Decision, custom_role_preflight_decision,
mysql_partial_unexpected_values_query, custom_role_preflight_error, mysql_partial_unexpected_values_query,
}; };
fn pending_repair() -> Facts { fn pending_repair() -> Facts {
@ -1191,16 +1218,33 @@ mod custom_role_migration_preflight_tests {
#[test] #[test]
fn legacy_user_access_all_requires_an_operator_decision() { fn legacy_user_access_all_requires_an_operator_decision() {
assert_eq!( let facts = Facts {
custom_role_preflight_decision(
Facts {
legacy_user_access_all_count: 1, legacy_user_access_all_count: 1,
..pending_repair() ..pending_repair()
}, };
false, let decision = custom_role_preflight_decision(facts, false);
), assert_eq!(decision, Decision::RefuseLegacyUserAccessAll);
Decision::RefuseLegacyUserAccessAll
); let error = custom_role_preflight_error(decision, facts);
let message = error.source().expect("preflight error should retain its I/O error source").to_string();
assert!(message.contains("1 legacy User membership(s)"));
assert!(message.contains(
"SELECT uuid, user_uuid, org_uuid, status\n\
FROM users_organizations\n\
WHERE atype = 2 AND access_all = TRUE;"
));
assert!(message.contains(
"SET access_all = FALSE\n\
WHERE uuid = '<MEMBERSHIP_UUID>' AND atype = 2 AND access_all = TRUE;"
));
assert!(message.contains(
"SET atype = 3\n\
WHERE uuid = '<MEMBERSHIP_UUID>' AND atype = 2 AND access_all = TRUE;"
));
assert!(message.contains("run exactly one guarded statement"));
assert!(message.contains("Do not bulk-promote"));
assert!(message.contains("converting legacy role 3 to Custom role 4"));
assert!(!message.contains("SET atype = 4"));
} }
#[test] #[test]

21
src/db/models/cipher.rs

@ -26,6 +26,7 @@ use macros::UuidFromParam;
use super::{ use super::{
Archive, Attachment, CollectionCipher, CollectionId, Favorite, FolderCipher, FolderId, Group, Membership, Archive, Attachment, CollectionCipher, CollectionId, Favorite, FolderCipher, FolderId, Group, Membership,
MembershipStatus, MembershipType, OrganizationId, User, UserId, MembershipStatus, MembershipType, OrganizationId, User, UserId,
organization::custom_membership_with_edit_any_collection,
}; };
#[derive(Identifiable, Queryable, Insertable, AsChangeset)] #[derive(Identifiable, Queryable, Insertable, AsChangeset)]
@ -891,8 +892,7 @@ impl Cipher {
.filter(ciphers::user_uuid.eq(user_uuid)) // Cipher owner .filter(ciphers::user_uuid.eq(user_uuid)) // Cipher owner
// Edit any collection (Custom) or org admin/owner — the successor of access_all // Edit any collection (Custom) or org admin/owner — the successor of access_all
.or_filter( .or_filter(
users_organizations::edit_any_collection custom_membership_with_edit_any_collection()
.eq(true)
.or(users_organizations::atype.le(MembershipType::Admin as i32)), .or(users_organizations::atype.le(MembershipType::Admin as i32)),
) )
.or_filter(users_collections::user_uuid.eq(user_uuid)) // Access to collection .or_filter(users_collections::user_uuid.eq(user_uuid)) // Access to collection
@ -933,8 +933,7 @@ impl Cipher {
.filter(ciphers::user_uuid.eq(user_uuid)) // Cipher owner .filter(ciphers::user_uuid.eq(user_uuid)) // Cipher owner
// Edit any collection (Custom) or org admin/owner — the successor of access_all // Edit any collection (Custom) or org admin/owner — the successor of access_all
.or_filter( .or_filter(
users_organizations::edit_any_collection custom_membership_with_edit_any_collection()
.eq(true)
.or(users_organizations::atype.le(MembershipType::Admin as i32)), .or(users_organizations::atype.le(MembershipType::Admin as i32)),
) )
.or_filter(users_collections::user_uuid.eq(user_uuid)) // Access to collection .or_filter(users_collections::user_uuid.eq(user_uuid)) // Access to collection
@ -1059,8 +1058,7 @@ impl Cipher {
.and(collections_groups::groups_uuid.eq(groups::uuid))), .and(collections_groups::groups_uuid.eq(groups::uuid))),
) )
.filter( .filter(
users_organizations::edit_any_collection custom_membership_with_edit_any_collection() // Custom "Edit any collection" (successor of access_all)
.eq(true) // Custom "Edit any collection" (successor of access_all)
.or(users_organizations::atype.le(MembershipType::Admin as i32)) // or org admin/owner .or(users_organizations::atype.le(MembershipType::Admin as i32)) // or org admin/owner
.or(users_collections::user_uuid .or(users_collections::user_uuid
.eq(user_uuid) // User has access to collection .eq(user_uuid) // User has access to collection
@ -1091,8 +1089,7 @@ impl Cipher {
.and(users_collections::user_uuid.eq(user_uuid.clone()))), .and(users_collections::user_uuid.eq(user_uuid.clone()))),
) )
.filter( .filter(
users_organizations::edit_any_collection custom_membership_with_edit_any_collection() // Custom "Edit any collection" (successor of access_all)
.eq(true) // Custom "Edit any collection" (successor of access_all)
.or(users_organizations::atype.le(MembershipType::Admin as i32)) // or org admin/owner .or(users_organizations::atype.le(MembershipType::Admin as i32)) // or org admin/owner
.or(users_collections::user_uuid .or(users_collections::user_uuid
.eq(user_uuid) // User has access to collection .eq(user_uuid) // User has access to collection
@ -1136,8 +1133,7 @@ impl Cipher {
.and(collections_groups::groups_uuid.eq(groups::uuid))), .and(collections_groups::groups_uuid.eq(groups::uuid))),
) )
.filter( .filter(
users_organizations::edit_any_collection custom_membership_with_edit_any_collection() // Custom "Edit any collection" (successor of access_all)
.eq(true) // Custom "Edit any collection" (successor of access_all)
.or(users_organizations::atype.le(MembershipType::Admin as i32)) // or org admin/owner .or(users_organizations::atype.le(MembershipType::Admin as i32)) // or org admin/owner
.or(users_collections::user_uuid .or(users_collections::user_uuid
.eq(user_uuid) // User has access to collection .eq(user_uuid) // User has access to collection
@ -1169,8 +1165,7 @@ impl Cipher {
.and(users_collections::user_uuid.eq(user_uuid.clone()))), .and(users_collections::user_uuid.eq(user_uuid.clone()))),
) )
.filter( .filter(
users_organizations::edit_any_collection custom_membership_with_edit_any_collection() // Custom "Edit any collection" (successor of access_all)
.eq(true) // Custom "Edit any collection" (successor of access_all)
.or(users_organizations::atype.le(MembershipType::Admin as i32)) // or org admin/owner .or(users_organizations::atype.le(MembershipType::Admin as i32)) // or org admin/owner
.or(users_collections::user_uuid .or(users_collections::user_uuid
.eq(user_uuid) // User has access to collection .eq(user_uuid) // User has access to collection
@ -1216,7 +1211,7 @@ impl Cipher {
.and(collections_groups::groups_uuid.eq(groups::uuid))), .and(collections_groups::groups_uuid.eq(groups::uuid))),
) )
.or_filter(users_collections::user_uuid.eq(user_uuid)) // User has access to collection .or_filter(users_collections::user_uuid.eq(user_uuid)) // User has access to collection
.or_filter(users_organizations::edit_any_collection.eq(true)) // Custom "Edit any collection" (successor of access_all) .or_filter(custom_membership_with_edit_any_collection()) // Custom "Edit any collection" (successor of access_all)
.or_filter(users_organizations::atype.le(MembershipType::Admin as i32)) // User is admin or owner .or_filter(users_organizations::atype.le(MembershipType::Admin as i32)) // User is admin or owner
.or_filter(groups::access_all.eq(true)) //Access via group .or_filter(groups::access_all.eq(true)) //Access via group
.or_filter(collections_groups::collections_uuid.is_not_null()) //Access via group .or_filter(collections_groups::collections_uuid.is_not_null()) //Access via group

18
src/db/models/collection.rs

@ -19,7 +19,7 @@ use macros::UuidFromParam;
use super::{ use super::{
CipherId, CollectionGroup, GroupUser, Membership, MembershipId, MembershipStatus, MembershipType, OrganizationId, CipherId, CollectionGroup, GroupUser, Membership, MembershipId, MembershipStatus, MembershipType, OrganizationId,
User, UserId, User, UserId, organization::custom_membership_with_edit_any_collection,
}; };
// See (v2026.7.0): https://github.com/bitwarden/server/blob/5d4461aa42cadbacfef8fe2166c5453a5c52773a/src/Core/AdminConsole/Entities/Collection.cs // See (v2026.7.0): https://github.com/bitwarden/server/blob/5d4461aa42cadbacfef8fe2166c5453a5c52773a/src/Core/AdminConsole/Entities/Collection.cs
@ -265,8 +265,7 @@ impl Collection {
.or( .or(
// Full-access member: Custom "Edit any collection" or org admin/owner // Full-access member: Custom "Edit any collection" or org admin/owner
// (successor of the removed membership access_all) // (successor of the removed membership access_all)
users_organizations::edit_any_collection custom_membership_with_edit_any_collection()
.eq(true)
.or(users_organizations::atype.le(MembershipType::Admin as i32)), .or(users_organizations::atype.le(MembershipType::Admin as i32)),
) )
.or( .or(
@ -303,8 +302,7 @@ impl Collection {
users_collections::user_uuid.eq(user_uuid).or( users_collections::user_uuid.eq(user_uuid).or(
// Full-access member: Custom "Edit any collection" or org admin/owner // Full-access member: Custom "Edit any collection" or org admin/owner
// (successor of the removed membership access_all) // (successor of the removed membership access_all)
users_organizations::edit_any_collection custom_membership_with_edit_any_collection()
.eq(true)
.or(users_organizations::atype.le(MembershipType::Admin as i32)), .or(users_organizations::atype.le(MembershipType::Admin as i32)),
), ),
) )
@ -391,7 +389,7 @@ impl Collection {
.eq(uuid) .eq(uuid)
.or( .or(
// Directly accessed collection // Directly accessed collection
users_organizations::edit_any_collection.eq(true).or( custom_membership_with_edit_any_collection().or(
// Custom "Edit any collection" or org admin/owner (successor of access_all) // Custom "Edit any collection" or org admin/owner (successor of access_all)
users_organizations::atype.le(MembershipType::Admin as i32), // Org admin or owner users_organizations::atype.le(MembershipType::Admin as i32), // Org admin or owner
), ),
@ -427,7 +425,7 @@ impl Collection {
.filter(collections::uuid.eq(uuid)) .filter(collections::uuid.eq(uuid))
.filter(users_collections::collection_uuid.eq(uuid).or( .filter(users_collections::collection_uuid.eq(uuid).or(
// Directly accessed collection // Directly accessed collection
users_organizations::edit_any_collection.eq(true).or( custom_membership_with_edit_any_collection().or(
// Custom "Edit any collection" or org admin/owner (successor of access_all) // Custom "Edit any collection" or org admin/owner (successor of access_all)
users_organizations::atype.le(MembershipType::Admin as i32), // Org admin or owner users_organizations::atype.le(MembershipType::Admin as i32), // Org admin or owner
), ),
@ -472,7 +470,7 @@ impl Collection {
.filter( .filter(
users_organizations::atype users_organizations::atype
.le(MembershipType::Admin as i32) // Org admin or owner .le(MembershipType::Admin as i32) // Org admin or owner
.or(users_organizations::edit_any_collection.eq(true)) // Custom "Edit any collection" (successor of access_all) .or(custom_membership_with_edit_any_collection()) // Custom "Edit any collection" (successor of access_all)
.or(users_collections::collection_uuid .or(users_collections::collection_uuid
.eq(&self.uuid) // write access given to collection .eq(&self.uuid) // write access given to collection
.and(users_collections::read_only.eq(false))) .and(users_collections::read_only.eq(false)))
@ -505,7 +503,7 @@ impl Collection {
.filter( .filter(
users_organizations::atype users_organizations::atype
.le(MembershipType::Admin as i32) // Org admin or owner .le(MembershipType::Admin as i32) // Org admin or owner
.or(users_organizations::edit_any_collection.eq(true)) // Custom "Edit any collection" (successor of access_all) .or(custom_membership_with_edit_any_collection()) // Custom "Edit any collection" (successor of access_all)
.or(users_collections::collection_uuid .or(users_collections::collection_uuid
.eq(&self.uuid) // write access given to collection .eq(&self.uuid) // write access given to collection
.and(users_collections::read_only.eq(false))), .and(users_collections::read_only.eq(false))),
@ -552,7 +550,7 @@ impl Collection {
.and(users_collections::hide_passwords.eq(true)) .and(users_collections::hide_passwords.eq(true))
.or( .or(
// Directly accessed collection // Directly accessed collection
users_organizations::edit_any_collection.eq(true).or( custom_membership_with_edit_any_collection().or(
// Custom "Edit any collection" or org admin/owner (successor of access_all) // Custom "Edit any collection" or org admin/owner (successor of access_all)
users_organizations::atype.le(MembershipType::Admin as i32), // Org admin or owner users_organizations::atype.le(MembershipType::Admin as i32), // Org admin or owner
), ),

52
src/db/models/organization.rs

@ -68,6 +68,17 @@ pub struct Membership {
pub access_reports: bool, pub access_reports: bool,
} }
/// Diesel equivalent of [`Membership::has_edit_any_collection`].
///
/// Keep the role check in this shared predicate so a stale flag on any non-Custom membership
/// remains inert in every collection-access query.
pub(super) fn custom_membership_with_edit_any_collection() -> diesel::dsl::And<
diesel::dsl::Eq<users_organizations::atype, i32>,
diesel::dsl::Eq<users_organizations::edit_any_collection, bool>,
> {
users_organizations::atype.eq(MembershipType::Custom as i32).and(users_organizations::edit_any_collection.eq(true))
}
#[derive(Identifiable, Queryable, Insertable, AsChangeset)] #[derive(Identifiable, Queryable, Insertable, AsChangeset)]
#[diesel(table_name = organization_api_key)] #[diesel(table_name = organization_api_key)]
#[diesel(primary_key(uuid, org_uuid))] #[diesel(primary_key(uuid, org_uuid))]
@ -1112,9 +1123,7 @@ impl Membership {
.filter( .filter(
users_organizations::atype users_organizations::atype
.eq_any(vec![MembershipType::Owner as i32, MembershipType::Admin as i32]) .eq_any(vec![MembershipType::Owner as i32, MembershipType::Admin as i32])
.or(users_organizations::atype .or(custom_membership_with_edit_any_collection()),
.eq(MembershipType::Custom as i32)
.and(users_organizations::edit_any_collection.eq(true))),
) )
.load::<Self>(conn) .load::<Self>(conn)
.unwrap_or_default() .unwrap_or_default()
@ -1239,8 +1248,7 @@ impl Membership {
.and(ciphers_collections::cipher_uuid.eq(&cipher_uuid))), .and(ciphers_collections::cipher_uuid.eq(&cipher_uuid))),
) )
.filter( .filter(
users_organizations::edit_any_collection custom_membership_with_edit_any_collection() // Custom "Edit any collection" (successor of access_all)
.eq(true) // Custom "Edit any collection" (successor of access_all)
.or(users_organizations::atype.le(MembershipType::Admin as i32)) // or org admin/owner .or(users_organizations::atype.le(MembershipType::Admin as i32)) // or org admin/owner
.or(ciphers_collections::cipher_uuid.eq(&cipher_uuid)), // ..or access to collection with cipher .or(ciphers_collections::cipher_uuid.eq(&cipher_uuid)), // ..or access to collection with cipher
) )
@ -1317,8 +1325,7 @@ impl Membership {
.filter(users_organizations::org_uuid.eq(org_uuid)) .filter(users_organizations::org_uuid.eq(org_uuid))
.left_join(users_collections::table.on(users_collections::user_uuid.eq(users_organizations::user_uuid))) .left_join(users_collections::table.on(users_collections::user_uuid.eq(users_organizations::user_uuid)))
.filter( .filter(
users_organizations::edit_any_collection custom_membership_with_edit_any_collection() // Custom "Edit any collection" (successor of access_all)
.eq(true) // Custom "Edit any collection" (successor of access_all)
.or(users_organizations::atype.le(MembershipType::Admin as i32)) // or org admin/owner .or(users_organizations::atype.le(MembershipType::Admin as i32)) // or org admin/owner
.or(users_collections::collection_uuid.eq(&collection_uuid)), // ..or access to collection .or(users_collections::collection_uuid.eq(&collection_uuid)), // ..or access to collection
) )
@ -1509,6 +1516,37 @@ mod tests {
assert!(!member.has_full_access()); assert!(!member.has_full_access());
} }
#[cfg(sqlite)]
#[test]
fn diesel_edit_any_collection_predicate_is_custom_type_gated() {
use diesel::{Connection, connection::SimpleConnection, sqlite::SqliteConnection};
let mut conn = SqliteConnection::establish(":memory:").unwrap();
conn.batch_execute(
"CREATE TABLE users_organizations (
atype INTEGER NOT NULL,
edit_any_collection BOOLEAN NOT NULL
);
INSERT INTO users_organizations (atype, edit_any_collection) VALUES
(0, TRUE),
(1, TRUE),
(2, TRUE),
(3, TRUE),
(4, FALSE),
(4, TRUE),
(5, TRUE);",
)
.unwrap();
let matching_types = users_organizations::table
.select(users_organizations::atype)
.filter(custom_membership_with_edit_any_collection())
.load::<i32>(&mut conn)
.unwrap();
assert_eq!(matching_types, vec![MembershipType::Custom as i32]);
}
#[test] #[test]
fn edit_any_collection_does_not_imply_create_or_delete() { fn edit_any_collection_does_not_imply_create_or_delete() {
let mut custom = membership(MembershipType::Custom); let mut custom = membership(MembershipType::Custom);

Loading…
Cancel
Save