diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index 8e7c8057..045fa562 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -1382,6 +1382,33 @@ async fn send_invite( let custom_permissions = CustomRolePermissions::from_request(new_type, &data.permissions); let grants_full_access = custom_permissions.grants_full_collection_access(new_type); + // Security: only callers who can manage collections (Admins/Owners, or users with full access) + // may assign collection access when inviting. A custom user with only manage_users can invite + // members, but cannot grant them collection access. Assigning groups is gated separately, + // because a collection-bearing group grants that access indirectly. + let caller = Membership::find_by_user_and_org(&headers.user.uuid, &org_id, &conn).await; + let caller_can_manage_collections = + headers.membership_type >= MembershipType::Admin || caller.as_ref().is_some_and(Membership::has_full_access); + let caller_can_manage_groups = + headers.membership_type >= MembershipType::Admin || caller.as_ref().is_some_and(Membership::has_manage_groups); + + // API consistency: these fields used to be dropped silently while the invite still reported + // success, so the caller believed access had been granted. Reject the request instead, and do it + // before the loop below creates any user, invitation or membership row. + if !grants_full_access && !caller_can_manage_collections && data.collections.iter().flatten().next().is_some() { + err!("You don't have permission to assign collections to invited members") + } + if !caller_can_manage_groups && !data.groups.is_empty() { + err!("You don't have permission to assign groups to invited members") + } + if !caller_can_manage_collections { + for group_id in &data.groups { + if group_confers_collection_access(group_id, &org_id, &conn).await { + err!("You don't have permission to assign a group that grants collection access") + } + } + } + let mut user_created: bool = false; for email in &data.emails { let mut member_status = MembershipStatus::Invited as i32; @@ -1465,23 +1492,12 @@ async fn send_invite( ) .await; - // Security: only callers who can manage collections (Admins/Owners, or users with full - // access) may assign collection access when inviting. A custom user with only manage_users - // can invite members, but cannot grant them collection access. - let caller_can_manage_collections = headers.membership_type >= MembershipType::Admin - || match Membership::find_by_user_and_org(&headers.user.uuid, &org_id, &conn).await { - Some(m) => m.has_full_access(), - None => false, - }; - // If the member does not already reach every collection, add the collections received if !grants_full_access && caller_can_manage_collections { // Security (F-1): a per-collection `manage` grant carries delete authority, so the // caller may only confer it on collections they could delete themselves. Otherwise a // caller acting via Edit-any-collection could invite an account they control with a // `manage` row and reach Delete-any-collection through it. - let caller = Membership::find_by_user_and_org(&headers.user.uuid, &org_id, &conn).await; - for col in data.collections.iter().flatten() { match Collection::find_by_uuid_and_org(&col.id, &org_id, &conn).await { None => err!("Collection not found in Organization"), @@ -1505,27 +1521,11 @@ async fn send_invite( } } - // Security: assigning groups can indirectly grant collection access via the groups' - // collections. Only callers who may manage groups (Admins/Owners or users with - // manage_groups) are allowed to assign groups when inviting. // NOTE: every requested group was already validated against this organization in - // `InviteData::validate` above, before any record was created. - let caller_can_manage_groups = headers.membership_type >= MembershipType::Admin - || match Membership::find_by_user_and_org(&headers.user.uuid, &org_id, &conn).await { - Some(m) => m.has_manage_groups(), - None => false, - }; - + // `InviteData::validate`, and both the manage_groups permission and the collection-bearing + // group restriction were rejected up front, before any record was created. if caller_can_manage_groups { for group_id in &data.groups { - // Security: a caller who cannot manage collections must not grant collection - // access to the invitee by placing them into a collection-bearing group. - if !may_change_group_membership( - caller_can_manage_collections, - group_confers_collection_access(group_id, &org_id, &conn).await, - ) { - continue; - } let mut group_entry = GroupUser::new(group_id.clone(), new_member.uuid.clone()); group_entry.save(&conn).await?; } @@ -1935,6 +1935,14 @@ async fn edit_member( err!("Only Owners can edit Owner users") } + // Security: apply the same actor/target role matrix as every other member endpoint (reinvite, + // confirm, revoke, restore, delete). Without it `edit_member` was the only path on which a + // Custom member holding manage_users could aim at an Admin or at a fellow Custom membership, as + // long as the request left the role unchanged. + if !may_manage_stored_member_type(headers.membership_type, member_to_edit.atype) { + err!("You don't have permission to edit this member") + } + if member_to_edit.atype == MembershipType::Owner && new_type != MembershipType::Owner && member_to_edit.status == MembershipStatus::Confirmed as i32 @@ -1964,6 +1972,24 @@ async fn edit_member( None => false, }; + // API consistency: dropping these fields silently while still answering 200 let client and + // server drift apart after an apparently saved change. Reject the request instead — but only + // when it would actually add or remove an assignment, because the regular edit dialog echoes the + // current assignments back and has to keep working. Flag-only differences (readOnly, + // hidePasswords, manage) remain ignored for these callers. + if !caller_can_manage_collections && !grants_full_access { + let requested: HashSet = data.collections.iter().flatten().map(|c| c.id.clone()).collect(); + let current: HashSet = + CollectionUser::find_by_organization_and_user_uuid(&org_id, &member_to_edit.user_uuid, &conn) + .await + .into_iter() + .map(|c| c.collection_uuid) + .collect(); + if requested != current { + err!("You don't have permission to change this member's collection assignments") + } + } + // Edit any collection (the successor of the removed access_all flag) grants full access to // every collection. It is part of the granular custom permissions applied here, and the // differs_from guard above already prevents a non-Admin caller from changing it — so a Custom @@ -2021,6 +2047,26 @@ async fn edit_member( None => false, }; + // API consistency, as for the collection assignments above: reject group changes this caller may + // not make instead of silently dropping them. + let requested_groups: HashSet = data.groups.iter().flatten().cloned().collect(); + let current_groups: HashSet = + GroupUser::find_by_member(&member_to_edit.uuid, &conn).await.into_iter().map(|gu| gu.groups_uuid).collect(); + if !caller_can_manage_groups && requested_groups != current_groups { + err!("You don't have permission to change this member's group assignments") + } + if caller_can_manage_groups && !caller_can_manage_collections { + let mut collection_bearing: HashSet = HashSet::new(); + for group_id in requested_groups.union(¤t_groups) { + if group_confers_collection_access(group_id, &org_id, &conn).await { + collection_bearing.insert(group_id.clone()); + } + } + if !collection_bearing_membership_unchanged(&requested_groups, ¤t_groups, &collection_bearing) { + err!("You don't have permission to change memberships in groups that grant collection access") + } + } + if caller_can_manage_groups { // Security (audit H-2): validate that every requested group belongs to this organization // *before* mutating any group membership. Otherwise a caller could link the member to a @@ -3229,6 +3275,21 @@ fn may_change_group_membership(caller_can_manage_collections: bool, group_confer caller_can_manage_collections || !group_confers_collection_access } +/// Whether `requested` and `current` agree on every group that confers collection access. +/// +/// A caller who may manage groups but not collections may only change memberships in groups that +/// confer no collection access. Anything else has to be rejected with an error rather than skipped +/// silently, so a save that appears to succeed never means something different on the server. +fn collection_bearing_membership_unchanged( + requested: &HashSet, + current: &HashSet, + collection_bearing: &HashSet, +) -> bool { + let restrict = + |set: &HashSet| -> HashSet { set.intersection(collection_bearing).cloned().collect() }; + restrict(requested) == restrict(current) +} + /// 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` @@ -4027,16 +4088,16 @@ async fn rotate_api_key( #[cfg(test)] mod tests { - use std::collections::HashMap; + use std::collections::{HashMap, HashSet}; use serde_json::{Value, json}; 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_manage_member_type, - may_manage_stored_member_type, + CustomRolePermissions, caller_manage_grant_role_check, collection_bearing_membership_unchanged, + filter_ciphers_for_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}; + use crate::db::models::{Cipher, GroupId, Membership, MembershipStatus, MembershipType, OrganizationId}; fn confirmed_member(member_type: MembershipType) -> Membership { let mut m = Membership::new("test-user".to_owned().into(), "test-org".to_owned().into(), None); @@ -4172,6 +4233,36 @@ mod tests { assert!(may_manage_stored_member_type(MembershipType::Admin, MembershipType::Custom as i32)); assert!(!may_manage_stored_member_type(MembershipType::Owner, i32::MAX)); + + // edit_member applies the same matrix as reinvite/confirm/revoke/restore/delete, so a + // Custom caller cannot target an Admin or a fellow Custom member even when the requested + // role equals the stored one. + for target in [MembershipType::Owner, MembershipType::Admin, MembershipType::Custom] { + assert!(may_change_member_type(MembershipType::Custom, target as i32, target)); + assert!(!may_manage_stored_member_type(MembershipType::Custom, target as i32)); + } + assert!(may_manage_stored_member_type(MembershipType::Custom, MembershipType::User as i32)); + } + + #[test] + fn only_collection_bearing_group_changes_are_rejected() { + let plain: GroupId = "plain".to_owned().into(); + let bearing: GroupId = "bearing".to_owned().into(); + let collection_bearing = HashSet::from([bearing.clone()]); + + let set = |ids: &[&GroupId]| -> HashSet { ids.iter().map(|id| (*id).clone()).collect() }; + + // Adding, removing or keeping a group without collections is fine. + for (requested, current) in + [(set(&[&plain]), set(&[])), (set(&[]), set(&[&plain])), (set(&[&plain, &bearing]), set(&[&bearing]))] + { + assert!(collection_bearing_membership_unchanged(&requested, ¤t, &collection_bearing)); + } + + // Adding or removing a collection-bearing group is not. + for (requested, current) in [(set(&[&bearing]), set(&[])), (set(&[&plain]), set(&[&plain, &bearing]))] { + assert!(!collection_bearing_membership_unchanged(&requested, ¤t, &collection_bearing)); + } } #[test] diff --git a/src/db/mod.rs b/src/db/mod.rs index 622235a2..75c07d2c 100644 --- a/src/db/mod.rs +++ b/src/db/mod.rs @@ -493,6 +493,49 @@ const LEGACY_USER_ACCESS_ALL_RECOVERY_SQL: &str = concat!( "access_all value to all three collection permissions before converting legacy role 3 to Custom role 4." ); +const AMBIGUOUS_DIRECT_PERMISSIONS_RECOVERY_SQL: &str = concat!( + "\n\nList every affected membership with this SQLite/MySQL/PostgreSQL-compatible query:\n", + "SELECT uuid, user_uuid, org_uuid, atype, status\n", + "FROM users_organizations\n", + "WHERE atype IN (3, 4)\n", + " AND access_all = FALSE\n", + " AND create_new_collections = FALSE\n", + " AND edit_any_collection = TRUE\n", + " AND delete_any_collection = TRUE;\n\n", + "To see which of them still have an organization-local full-access group as a plausible source of the pattern, ", + "run the query below. On MySQL/MariaDB the reserved word `groups` has to be quoted with backticks:\n", + "SELECT uo.uuid, uo.org_uuid, g.uuid AS group_uuid, g.name AS group_name\n", + "FROM users_organizations uo\n", + "INNER JOIN groups_users gu ON gu.users_organizations_uuid = uo.uuid\n", + "INNER JOIN groups g ON g.uuid = gu.groups_uuid AND g.organizations_uuid = uo.org_uuid\n", + "WHERE g.access_all = TRUE\n", + " AND uo.atype IN (3, 4)\n", + " AND uo.access_all = FALSE\n", + " AND uo.create_new_collections = FALSE\n", + " AND uo.edit_any_collection = TRUE\n", + " AND uo.delete_any_collection = TRUE;\n\n", + "An organization owner has to decide per membership which of the two meanings applies. Replace ", + " and run exactly one guarded statement for that membership while every Vaultwarden instance is ", + "stopped. Do not bulk-apply either statement.\n\n", + "The pattern was a group-derived copy, or the authority is no longer wanted: drop it and let the group (if any) ", + "remain the only source of access.\n", + "UPDATE users_organizations\n", + "SET edit_any_collection = FALSE,\n", + " delete_any_collection = FALSE\n", + "WHERE uuid = '' AND access_all = FALSE AND create_new_collections = FALSE;\n\n", + "The pattern was an intentional direct grant that has to survive: make it unambiguous so the migration can pass.\n", + "UPDATE users_organizations\n", + "SET create_new_collections = TRUE\n", + "WHERE uuid = '' AND access_all = FALSE AND edit_any_collection = TRUE;\n\n", + "Note that the second statement also grants Create-any-collection, because a 0/1/1 pattern is exactly the state ", + "the migration cannot attribute. If that member must not be able to create collections, start the server once so ", + "the migration completes, then set create_new_collections back to FALSE for that membership." +); +const ALREADY_DROPPED_RECOVERY: &str = concat!( + "\n\nThe permission values cannot be recomputed from the current schema. Restore the database backup taken ", + "before the upgrade and run the upgrade again against that restored copy." +); + #[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] #[expect( clippy::struct_excessive_bools, @@ -605,10 +648,11 @@ fn custom_role_preflight_error(decision: CustomRolePreflightDecision, facts: Cus unreachable!("successful preflight decisions do not produce errors") } }; - let recovery = if decision == CustomRolePreflightDecision::RefuseLegacyUserAccessAll { - LEGACY_USER_ACCESS_ALL_RECOVERY_SQL - } else { - "" + let recovery = match decision { + CustomRolePreflightDecision::RefuseLegacyUserAccessAll => LEGACY_USER_ACCESS_ALL_RECOVERY_SQL, + CustomRolePreflightDecision::RefuseAmbiguousDirectPermissions => AMBIGUOUS_DIRECT_PERMISSIONS_RECOVERY_SQL, + CustomRolePreflightDecision::RefuseAlreadyDropped => ALREADY_DROPPED_RECOVERY, + _ => "", }; std::io::Error::other(format!( @@ -1216,6 +1260,41 @@ mod custom_role_migration_preflight_tests { ); } + #[test] + fn ambiguous_direct_permissions_error_carries_a_recovery_path() { + let facts = Facts { + ambiguous_direct_permission_count: 2, + ..pending_repair() + }; + let decision = custom_role_preflight_decision(facts, false); + assert_eq!(decision, Decision::RefuseAmbiguousDirectPermissions); + + 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("2 membership(s)")); + // The operator needs the affected memberships and their possible group source ... + assert!(message.contains("SELECT uuid, user_uuid, org_uuid, atype, status")); + assert!(message.contains("WHERE g.access_all = TRUE")); + // ... plus both decisions, and the note that keeping the grant also grants Create. + assert!(message.contains("SET edit_any_collection = FALSE,\n delete_any_collection = FALSE")); + assert!(message.contains("SET create_new_collections = TRUE")); + assert!(message.contains("also grants Create-any-collection")); + } + + #[test] + fn already_dropped_error_points_at_the_backup() { + let facts = Facts { + access_all_drop_migration_applied: true, + ..pending_repair() + }; + let decision = custom_role_preflight_decision(facts, false); + assert_eq!(decision, Decision::RefuseAlreadyDropped); + + 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("Restore the database backup")); + } + #[test] fn legacy_user_access_all_requires_an_operator_decision() { let facts = Facts {