diff --git a/migrations/mysql/2026-06-30-120000_add_custom_role_permissions/down.sql b/migrations/mysql/2026-06-30-120000_add_custom_role_permissions/down.sql new file mode 100644 index 00000000..9ac54bfb --- /dev/null +++ b/migrations/mysql/2026-06-30-120000_add_custom_role_permissions/down.sql @@ -0,0 +1,6 @@ +-- Convert Custom members back to Manager, the representation older server versions +-- expect (they masquerade Manager as Custom in API responses and cannot load type 4). +UPDATE users_organizations SET atype = 3 WHERE atype = 4; +ALTER TABLE users_organizations DROP COLUMN manage_users; +ALTER TABLE users_organizations DROP COLUMN manage_groups; +ALTER TABLE users_organizations DROP COLUMN manage_policies; diff --git a/migrations/mysql/2026-06-30-120000_add_custom_role_permissions/up.sql b/migrations/mysql/2026-06-30-120000_add_custom_role_permissions/up.sql new file mode 100644 index 00000000..6ffdca13 --- /dev/null +++ b/migrations/mysql/2026-06-30-120000_add_custom_role_permissions/up.sql @@ -0,0 +1,9 @@ +ALTER TABLE users_organizations ADD COLUMN manage_users BOOLEAN NOT NULL DEFAULT FALSE; +ALTER TABLE users_organizations ADD COLUMN manage_groups BOOLEAN NOT NULL DEFAULT FALSE; +ALTER TABLE users_organizations ADD COLUMN manage_policies BOOLEAN NOT NULL DEFAULT FALSE; +-- Previously the server stored members created with the Custom role as Manager (3) and +-- masqueraded them as Custom (4) in all API responses. Now that Custom is a real, persisted +-- type, convert those members so clients (which no longer know the Manager role) keep +-- seeing exactly what they saw before. access_all is preserved; the new flags stay FALSE, +-- which matches the capabilities these members had. +UPDATE users_organizations SET atype = 4 WHERE atype = 3; diff --git a/migrations/mysql/2026-07-16-120000_add_custom_collection_permissions/down.sql b/migrations/mysql/2026-07-16-120000_add_custom_collection_permissions/down.sql new file mode 100644 index 00000000..6506059d --- /dev/null +++ b/migrations/mysql/2026-07-16-120000_add_custom_collection_permissions/down.sql @@ -0,0 +1,9 @@ +-- The previous schema exposes access_all as the three collection permissions together. Avoid +-- turning Edit-only memberships into Create/Edit/Delete grants when rolling back. +UPDATE users_organizations +SET access_all = create_new_collections AND edit_any_collection AND delete_any_collection +WHERE atype = 4; + +ALTER TABLE users_organizations DROP COLUMN create_new_collections; +ALTER TABLE users_organizations DROP COLUMN edit_any_collection; +ALTER TABLE users_organizations DROP COLUMN delete_any_collection; diff --git a/migrations/mysql/2026-07-16-120000_add_custom_collection_permissions/up.sql b/migrations/mysql/2026-07-16-120000_add_custom_collection_permissions/up.sql new file mode 100644 index 00000000..da66070a --- /dev/null +++ b/migrations/mysql/2026-07-16-120000_add_custom_collection_permissions/up.sql @@ -0,0 +1,27 @@ +ALTER TABLE users_organizations ADD COLUMN create_new_collections BOOLEAN NOT NULL DEFAULT FALSE; +ALTER TABLE users_organizations ADD COLUMN edit_any_collection BOOLEAN NOT NULL DEFAULT FALSE; +ALTER TABLE users_organizations ADD COLUMN delete_any_collection BOOLEAN NOT NULL DEFAULT FALSE; + +-- Before these permissions were persisted independently, access_all represented the legacy +-- "Manage all collections" checkbox. Preserve that capability for existing Custom members. +UPDATE users_organizations +SET create_new_collections = access_all, + edit_any_collection = access_all, + delete_any_collection = access_all +WHERE atype = 4; + +-- A legacy Manager also managed every collection when one of their groups had access_all, +-- even if the membership itself did not. Preserve that existing edit/delete capability without +-- granting collection creation, which historically still required membership access_all. +UPDATE users_organizations +SET edit_any_collection = TRUE, + delete_any_collection = TRUE +WHERE atype = 4 + AND EXISTS ( + SELECT 1 + FROM groups_users + INNER JOIN groups ON groups.uuid = groups_users.groups_uuid + WHERE groups_users.users_organizations_uuid = users_organizations.uuid + AND groups.organizations_uuid = users_organizations.org_uuid + AND groups.access_all = TRUE + ); diff --git a/migrations/postgresql/2026-06-30-120000_add_custom_role_permissions/down.sql b/migrations/postgresql/2026-06-30-120000_add_custom_role_permissions/down.sql new file mode 100644 index 00000000..9ac54bfb --- /dev/null +++ b/migrations/postgresql/2026-06-30-120000_add_custom_role_permissions/down.sql @@ -0,0 +1,6 @@ +-- Convert Custom members back to Manager, the representation older server versions +-- expect (they masquerade Manager as Custom in API responses and cannot load type 4). +UPDATE users_organizations SET atype = 3 WHERE atype = 4; +ALTER TABLE users_organizations DROP COLUMN manage_users; +ALTER TABLE users_organizations DROP COLUMN manage_groups; +ALTER TABLE users_organizations DROP COLUMN manage_policies; diff --git a/migrations/postgresql/2026-06-30-120000_add_custom_role_permissions/up.sql b/migrations/postgresql/2026-06-30-120000_add_custom_role_permissions/up.sql new file mode 100644 index 00000000..6ffdca13 --- /dev/null +++ b/migrations/postgresql/2026-06-30-120000_add_custom_role_permissions/up.sql @@ -0,0 +1,9 @@ +ALTER TABLE users_organizations ADD COLUMN manage_users BOOLEAN NOT NULL DEFAULT FALSE; +ALTER TABLE users_organizations ADD COLUMN manage_groups BOOLEAN NOT NULL DEFAULT FALSE; +ALTER TABLE users_organizations ADD COLUMN manage_policies BOOLEAN NOT NULL DEFAULT FALSE; +-- Previously the server stored members created with the Custom role as Manager (3) and +-- masqueraded them as Custom (4) in all API responses. Now that Custom is a real, persisted +-- type, convert those members so clients (which no longer know the Manager role) keep +-- seeing exactly what they saw before. access_all is preserved; the new flags stay FALSE, +-- which matches the capabilities these members had. +UPDATE users_organizations SET atype = 4 WHERE atype = 3; diff --git a/migrations/postgresql/2026-07-16-120000_add_custom_collection_permissions/down.sql b/migrations/postgresql/2026-07-16-120000_add_custom_collection_permissions/down.sql new file mode 100644 index 00000000..6506059d --- /dev/null +++ b/migrations/postgresql/2026-07-16-120000_add_custom_collection_permissions/down.sql @@ -0,0 +1,9 @@ +-- The previous schema exposes access_all as the three collection permissions together. Avoid +-- turning Edit-only memberships into Create/Edit/Delete grants when rolling back. +UPDATE users_organizations +SET access_all = create_new_collections AND edit_any_collection AND delete_any_collection +WHERE atype = 4; + +ALTER TABLE users_organizations DROP COLUMN create_new_collections; +ALTER TABLE users_organizations DROP COLUMN edit_any_collection; +ALTER TABLE users_organizations DROP COLUMN delete_any_collection; diff --git a/migrations/postgresql/2026-07-16-120000_add_custom_collection_permissions/up.sql b/migrations/postgresql/2026-07-16-120000_add_custom_collection_permissions/up.sql new file mode 100644 index 00000000..da66070a --- /dev/null +++ b/migrations/postgresql/2026-07-16-120000_add_custom_collection_permissions/up.sql @@ -0,0 +1,27 @@ +ALTER TABLE users_organizations ADD COLUMN create_new_collections BOOLEAN NOT NULL DEFAULT FALSE; +ALTER TABLE users_organizations ADD COLUMN edit_any_collection BOOLEAN NOT NULL DEFAULT FALSE; +ALTER TABLE users_organizations ADD COLUMN delete_any_collection BOOLEAN NOT NULL DEFAULT FALSE; + +-- Before these permissions were persisted independently, access_all represented the legacy +-- "Manage all collections" checkbox. Preserve that capability for existing Custom members. +UPDATE users_organizations +SET create_new_collections = access_all, + edit_any_collection = access_all, + delete_any_collection = access_all +WHERE atype = 4; + +-- A legacy Manager also managed every collection when one of their groups had access_all, +-- even if the membership itself did not. Preserve that existing edit/delete capability without +-- granting collection creation, which historically still required membership access_all. +UPDATE users_organizations +SET edit_any_collection = TRUE, + delete_any_collection = TRUE +WHERE atype = 4 + AND EXISTS ( + SELECT 1 + FROM groups_users + INNER JOIN groups ON groups.uuid = groups_users.groups_uuid + WHERE groups_users.users_organizations_uuid = users_organizations.uuid + AND groups.organizations_uuid = users_organizations.org_uuid + AND groups.access_all = TRUE + ); diff --git a/migrations/sqlite/2026-06-30-120000_add_custom_role_permissions/down.sql b/migrations/sqlite/2026-06-30-120000_add_custom_role_permissions/down.sql new file mode 100644 index 00000000..9ac54bfb --- /dev/null +++ b/migrations/sqlite/2026-06-30-120000_add_custom_role_permissions/down.sql @@ -0,0 +1,6 @@ +-- Convert Custom members back to Manager, the representation older server versions +-- expect (they masquerade Manager as Custom in API responses and cannot load type 4). +UPDATE users_organizations SET atype = 3 WHERE atype = 4; +ALTER TABLE users_organizations DROP COLUMN manage_users; +ALTER TABLE users_organizations DROP COLUMN manage_groups; +ALTER TABLE users_organizations DROP COLUMN manage_policies; diff --git a/migrations/sqlite/2026-06-30-120000_add_custom_role_permissions/up.sql b/migrations/sqlite/2026-06-30-120000_add_custom_role_permissions/up.sql new file mode 100644 index 00000000..6ffdca13 --- /dev/null +++ b/migrations/sqlite/2026-06-30-120000_add_custom_role_permissions/up.sql @@ -0,0 +1,9 @@ +ALTER TABLE users_organizations ADD COLUMN manage_users BOOLEAN NOT NULL DEFAULT FALSE; +ALTER TABLE users_organizations ADD COLUMN manage_groups BOOLEAN NOT NULL DEFAULT FALSE; +ALTER TABLE users_organizations ADD COLUMN manage_policies BOOLEAN NOT NULL DEFAULT FALSE; +-- Previously the server stored members created with the Custom role as Manager (3) and +-- masqueraded them as Custom (4) in all API responses. Now that Custom is a real, persisted +-- type, convert those members so clients (which no longer know the Manager role) keep +-- seeing exactly what they saw before. access_all is preserved; the new flags stay FALSE, +-- which matches the capabilities these members had. +UPDATE users_organizations SET atype = 4 WHERE atype = 3; diff --git a/migrations/sqlite/2026-07-16-120000_add_custom_collection_permissions/down.sql b/migrations/sqlite/2026-07-16-120000_add_custom_collection_permissions/down.sql new file mode 100644 index 00000000..6506059d --- /dev/null +++ b/migrations/sqlite/2026-07-16-120000_add_custom_collection_permissions/down.sql @@ -0,0 +1,9 @@ +-- The previous schema exposes access_all as the three collection permissions together. Avoid +-- turning Edit-only memberships into Create/Edit/Delete grants when rolling back. +UPDATE users_organizations +SET access_all = create_new_collections AND edit_any_collection AND delete_any_collection +WHERE atype = 4; + +ALTER TABLE users_organizations DROP COLUMN create_new_collections; +ALTER TABLE users_organizations DROP COLUMN edit_any_collection; +ALTER TABLE users_organizations DROP COLUMN delete_any_collection; diff --git a/migrations/sqlite/2026-07-16-120000_add_custom_collection_permissions/up.sql b/migrations/sqlite/2026-07-16-120000_add_custom_collection_permissions/up.sql new file mode 100644 index 00000000..da66070a --- /dev/null +++ b/migrations/sqlite/2026-07-16-120000_add_custom_collection_permissions/up.sql @@ -0,0 +1,27 @@ +ALTER TABLE users_organizations ADD COLUMN create_new_collections BOOLEAN NOT NULL DEFAULT FALSE; +ALTER TABLE users_organizations ADD COLUMN edit_any_collection BOOLEAN NOT NULL DEFAULT FALSE; +ALTER TABLE users_organizations ADD COLUMN delete_any_collection BOOLEAN NOT NULL DEFAULT FALSE; + +-- Before these permissions were persisted independently, access_all represented the legacy +-- "Manage all collections" checkbox. Preserve that capability for existing Custom members. +UPDATE users_organizations +SET create_new_collections = access_all, + edit_any_collection = access_all, + delete_any_collection = access_all +WHERE atype = 4; + +-- A legacy Manager also managed every collection when one of their groups had access_all, +-- even if the membership itself did not. Preserve that existing edit/delete capability without +-- granting collection creation, which historically still required membership access_all. +UPDATE users_organizations +SET edit_any_collection = TRUE, + delete_any_collection = TRUE +WHERE atype = 4 + AND EXISTS ( + SELECT 1 + FROM groups_users + INNER JOIN groups ON groups.uuid = groups_users.groups_uuid + WHERE groups_users.users_organizations_uuid = users_organizations.uuid + AND groups.organizations_uuid = users_organizations.org_uuid + AND groups.access_all = TRUE + ); diff --git a/src/api/admin.rs b/src/api/admin.rs index 7037bfb1..8c60e0c7 100644 --- a/src/api/admin.rs +++ b/src/api/admin.rs @@ -544,6 +544,41 @@ struct MembershipTypeData { org_uuid: OrganizationId, } +fn apply_membership_type_change(membership: &mut Membership, new_type: MembershipType) { + let was_custom = membership.atype == MembershipType::Custom; + + // Leaving Custom for the legacy Manager role: `access_all` is the internal mirror of + // Edit any collection, but a Manager's `access_all` means the broad "manage all collections" + // grant (Create + Edit + Delete). Carrying an Edit-only mirror over would silently escalate the + // member to Create/Edit/Delete. Only preserve `access_all` when the member actually held the + // full manage-all grant, mirroring the collection-permissions down-migration. Must be evaluated + // before the flags are cleared below (and while the type is still Custom). + if was_custom && new_type == MembershipType::Manager { + membership.access_all = membership.has_manage_all_collections(); + } + + // Entering Custom through the Vaultwarden admin panel is deliberately fail-closed because + // that UI cannot select granular permissions; they can be granted later through the regular + // organization member dialog. + if new_type == MembershipType::Custom && !was_custom { + membership.clear_custom_permissions(); + membership.access_all = false; + } + if new_type != MembershipType::Custom { + membership.clear_custom_permissions(); + } + + // Prevent stale access_all from surviving a demotion to User. Admins/Owners have implicit + // full access, while legacy Manager access_all is intentionally preserved for compatibility. + match new_type { + MembershipType::Owner | MembershipType::Admin => membership.access_all = true, + MembershipType::User => membership.access_all = false, + MembershipType::Manager | MembershipType::Custom => {} + } + + membership.atype = new_type as i32; +} + #[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(); @@ -553,9 +588,7 @@ async fn update_membership_type(data: Json, token: AdminToke err!("The specified user isn't member of the organization") }; - let new_type = if let Some(new_type) = MembershipType::from_str(&data.user_type.into_string()) { - new_type as i32 - } else { + let Some(new_type) = MembershipType::from_str(&data.user_type.into_string()) else { err!("Invalid type") }; @@ -566,7 +599,7 @@ async fn update_membership_type(data: Json, token: AdminToke } } - member_to_edit.atype = new_type; + apply_membership_type_change(&mut member_to_edit, new_type); // This check is also done at api::organizations::{accept_invite, _confirm_invite, _activate_member, edit_member}, update_membership_type OrgPolicy::check_user_allowed(&member_to_edit, "modify", &conn).await?; @@ -869,6 +902,14 @@ impl<'r> FromRequest<'r> for AdminToken { #[cfg(test)] mod tests { use super::*; + use crate::db::models::MembershipStatus; + + fn membership(member_type: MembershipType) -> 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 = MembershipStatus::Confirmed as i32; + membership + } #[test] fn validate_web_vault_compare() { @@ -893,4 +934,62 @@ mod tests { assert!(web_vault_compare("2025.12.2+build.1", "2025.12.1+build.1") == 1); assert!(web_vault_compare("2025.12.1+build.3", "2025.12.1+build.2") == 1); } + + #[test] + fn admin_type_changes_clear_custom_permissions_and_stale_access() { + let mut custom = membership(MembershipType::Custom); + custom.access_all = true; + custom.manage_users = true; + custom.create_new_collections = true; + custom.edit_any_collection = true; + custom.delete_any_collection = true; + + apply_membership_type_change(&mut custom, MembershipType::User); + assert_eq!(custom.atype, MembershipType::User as i32); + assert!(!custom.access_all); + assert!(!custom.manage_users); + assert!(!custom.create_new_collections); + assert!(!custom.edit_any_collection); + assert!(!custom.delete_any_collection); + + let mut admin = membership(MembershipType::Admin); + admin.access_all = true; + apply_membership_type_change(&mut admin, MembershipType::Custom); + assert_eq!(admin.atype, MembershipType::Custom as i32); + assert!(!admin.access_all, "entering Custom through this UI must be fail-closed"); + assert!(!admin.has_manage_all_collections()); + } + + #[test] + fn admin_and_legacy_manager_access_all_behavior_is_preserved() { + let mut user = membership(MembershipType::User); + apply_membership_type_change(&mut user, MembershipType::Admin); + assert!(user.access_all); + + // REGRESSION (privilege escalation, PR #7397 / finding F2): a Custom member whose + // `access_all` is only the Edit-any-collection mirror must NOT be turned into a legacy + // Manager with the broad "manage all collections" `access_all` grant. That would escalate + // an Edit-only member into Create + Edit + Delete. Mirrors the collection down-migration. + let mut edit_only = membership(MembershipType::Custom); + edit_only.access_all = true; + edit_only.edit_any_collection = true; + apply_membership_type_change(&mut edit_only, MembershipType::Manager); + assert_eq!(edit_only.atype, MembershipType::Manager as i32); + assert!(!edit_only.access_all, "Edit-only Custom must not become an access_all Manager"); + assert!(!edit_only.edit_any_collection); + + // A Custom member who genuinely held the full manage-all grant (all three collection + // flags) keeps the equivalent legacy Manager `access_all`. + let mut manage_all = membership(MembershipType::Custom); + manage_all.access_all = true; + manage_all.create_new_collections = true; + manage_all.edit_any_collection = true; + manage_all.delete_any_collection = true; + apply_membership_type_change(&mut manage_all, MembershipType::Manager); + assert_eq!(manage_all.atype, MembershipType::Manager as i32); + assert!(manage_all.access_all, "full manage-all Custom keeps legacy Manager access_all"); + assert!(!manage_all.create_new_collections); + assert!(!manage_all.edit_any_collection); + assert!(!manage_all.delete_any_collection); + } } diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index c7e79aed..c9f68110 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -11,7 +11,11 @@ use crate::{ EmptyResult, JsonResult, Notify, PasswordOrOtpData, UpdateType, core::{CipherSyncData, CipherSyncType, accept_org_invite, log_event, two_factor}, }, - auth::{AdminHeaders, Headers, ManagerHeaders, ManagerHeadersLoose, OrgMemberHeaders, OwnerHeaders, decode_invite}, + auth::{ + AdminHeaders, CollectionDeleteHeaders, CollectionReadHeaders, Headers, ManageGroupsHeaders, + ManagePoliciesHeaders, ManageUsersHeaders, ManageUsersOrGroupsHeaders, ManagerHeaders, ManagerHeadersLoose, + OrgMemberHeaders, OwnerHeaders, decode_invite, + }, db::{ DbConn, models::{ @@ -47,6 +51,7 @@ pub fn routes() -> Vec { post_organization_collection_delete, bulk_delete_organization_collections, post_bulk_collections, + get_assigned_org_details, get_org_details, get_org_domain_sso_verified, get_members, @@ -390,7 +395,14 @@ async fn get_org_collections(org_id: OrganizationId, headers: ManagerHeadersLoos err!("Organization not found", "Organization id's do not match"); } - if !headers.membership.has_full_access() { + // Custom users with a user/group manage permission need to read the collection list + // (metadata only) to be able to assign collections to groups/members. This does NOT + // expose cipher contents. manage_policies does not need the collection list. + let can_read_collection_list = headers.membership.has_full_access() + || headers.membership.has_manage_users() + || headers.membership.has_manage_groups() + || headers.membership.has_delete_any_collection(); + if !can_read_collection_list { err_code!("Resource not found.", "User does not have full access", rocket::http::Status::NotFound.code); } @@ -422,6 +434,17 @@ async fn get_org_collections_details(org_id: OrganizationId, headers: ManagerHea let has_full_access_to_org = member.has_full_access() || (CONFIG.org_groups_enabled() && GroupUser::has_full_access_by_member(&org_id, &member.uuid, &conn).await); + // Custom users with a user/group manage permission need the full collection list + // (metadata only) so the web client can render member/group collection assignments + // without crashing on collections it can't otherwise see. This exposes names/ids + // only, never cipher contents. manage_policies does not need the collection list. + let can_read_collection_list = + member.has_manage_users() || member.has_manage_groups() || member.has_delete_any_collection(); + // Delete any collection can reveal collection access metadata, matching Bitwarden's + // ReadAllWithAccess behavior, but still does not grant cipher access. Manage Users/Groups + // retain the narrower metadata-only view introduced by the base PR. + let can_read_all_collection_access = member.has_edit_any_collection() || member.has_delete_any_collection(); + // Get all admins, owners and managers who can manage/access all // Those are currently not listed in the col_users but need to be listed too. let manage_all_members: Vec = Membership::find_confirmed_and_manage_all_by_org(&org_id, &conn) @@ -445,8 +468,20 @@ async fn get_org_collections_details(org_id: OrganizationId, headers: ManagerHea || (CONFIG.org_groups_enabled() && GroupUser::has_access_to_collection_by_member(&col.uuid, &member.uuid, &conn).await); - // If the user is a manager, and is not assigned to this collection, skip this and continue with the next collection - if !assigned { + // If the user is a manager and is not assigned to this collection, normally skip it. + // Exception: custom users with a manage permission get a metadata-only entry (no user + // or group access details) so the web client can resolve assignment references without + // crashing. This never exposes cipher contents. + if !assigned && !can_read_all_collection_access { + if can_read_collection_list { + let mut json_object = col.to_json_details(&headers.user.uuid, None, &conn).await; + json_object["assigned"] = json!(false); + json_object["users"] = json!(Vec::::new()); + json_object["groups"] = json!(Vec::::new()); + json_object["object"] = json!("collectionAccessDetails"); + json_object["unmanaged"] = json!(false); + data.push(json_object); + } continue; } @@ -503,11 +538,28 @@ async fn post_organization_collections( if org_id != headers.membership.org_uuid { err!("Organization not found", "Organization id's do not match"); } + + // Create is independent from Edit/Delete. In particular, Edit any collection's internal + // access_all representation must not implicitly grant this endpoint. + if !headers.membership.can_create_new_collections() { + err!("You don't have permission to create collections") + } + let data: FullCollectionData = data.into_inner(); data.validate(&org_id, &conn).await?; - if headers.membership.atype == MembershipType::Manager && !headers.membership.access_all { - err!("You don't have permission to create collections") + // Security (audit H-3): validate every referenced group and user against this organization + // *before* creating the collection or any assignment, so a foreign-tenant group can't be + // attached to the new collection and no partial state is left behind on rejection. + for group in &data.groups { + if Group::find_by_uuid_and_org(&group.id, &org_id, &conn).await.is_none() { + err!("Group not found in this organization") + } + } + for user in &data.users { + if Membership::find_by_uuid_and_org(&user.id, &org_id, &conn).await.is_none() { + err!("User is not part of organization") + } } let collection = Collection::new(org_id.clone(), data.name, data.external_id); @@ -577,8 +629,33 @@ async fn post_bulk_access_collections( err!("Can't find organization details") } - for col_id in data.collection_ids { - let Some(collection) = Collection::find_by_uuid_and_org(&col_id, &org_id, &conn).await else { + // Security: authorization is enforced per collection below via `is_manageable_by_user`, which + // requires a real manage grant on each requested collection (direct users_collections.manage, + // a manage group, membership/group access_all, or Admin/Owner). This matches the single + // collection edit endpoint and the pre-existing behavior of this endpoint. A custom user with + // only manage_users / manage_groups / manage_policies holds no such grant and is therefore + // rejected, while a Manager/Custom member who manages some collections keeps the ability to + // bulk-edit exactly those (a blanket full-access requirement would wrongly deny that here). + + // Security (audit H-3) and atomicity (audit M-2): validate the whole request against this + // organization *before* mutating anything. Every collection must exist in the org and be + // manageable by the caller, and every referenced group and user must belong to the org. Only + // once the entire request is known-valid do we begin the destructive delete/replace of + // assignments, so a foreign-tenant group can never be linked and a later invalid element can no + // longer leave earlier collections with their assignments already wiped. + for group in &data.groups { + if Group::find_by_uuid_and_org(&group.id, &org_id, &conn).await.is_none() { + err!("Group not found in this organization") + } + } + for user in &data.users { + if Membership::find_by_uuid_and_org(&user.id, &org_id, &conn).await.is_none() { + err!("User is not part of organization") + } + } + let mut collections = Vec::with_capacity(data.collection_ids.len()); + for col_id in &data.collection_ids { + let Some(collection) = Collection::find_by_uuid_and_org(col_id, &org_id, &conn).await else { err!("Collection not found") }; @@ -586,6 +663,17 @@ async fn post_bulk_access_collections( err!("Collection not found", "The current user isn't a manager for this collection") } + collections.push(collection); + } + + for collection in collections { + let col_id = &collection.uuid; + + // Security (F-1): only a caller who could delete this collection may confer a `manage` grant + // on it. Otherwise the requested `manage` is forced to false, so a caller whose access comes + // from Edit-any-collection cannot escalate into deletion by self-assigning a manage row. + let may_grant_manage = caller_may_grant_collection_manage(&headers.membership, col_id, &conn).await; + // update collection modification date collection.save(&conn).await?; @@ -600,14 +688,20 @@ async fn post_bulk_access_collections( ) .await; - CollectionGroup::delete_all_by_collection(&col_id, &org_id, &conn).await?; + CollectionGroup::delete_all_by_collection(col_id, &org_id, &conn).await?; for group in &data.groups { - CollectionGroup::new(col_id.clone(), group.id.clone(), group.read_only, group.hide_passwords, group.manage) - .save(&org_id, &conn) - .await?; + CollectionGroup::new( + col_id.clone(), + group.id.clone(), + group.read_only, + group.hide_passwords, + group.manage && may_grant_manage, + ) + .save(&org_id, &conn) + .await?; } - CollectionUser::delete_all_by_collection(&col_id, &conn).await?; + CollectionUser::delete_all_by_collection(col_id, &conn).await?; for user in &data.users { let Some(member) = Membership::find_by_uuid_and_org(&user.id, &org_id, &conn).await else { err!("User is not part of organization") @@ -617,8 +711,15 @@ async fn post_bulk_access_collections( continue; } - CollectionUser::save(&member.user_uuid, &col_id, user.read_only, user.hide_passwords, user.manage, &conn) - .await?; + CollectionUser::save( + &member.user_uuid, + col_id, + user.read_only, + user.hide_passwords, + user.manage && may_grant_manage, + &conn, + ) + .await?; } } @@ -677,10 +778,18 @@ async fn post_organization_collection_update( ) .await; + // Security (F-1): only a caller who could delete this collection may confer a `manage` grant on + // it (a `manage` row carries delete authority). For everyone else the requested `manage` is + // forced to false, so Edit-any-collection can rewrite access but never escalate into deletion. + let may_grant_manage = match Membership::find_by_user_and_org(&headers.user.uuid, &org_id, &conn).await { + Some(caller) => caller_may_grant_collection_manage(&caller, &col_id, &conn).await, + None => false, + }; + CollectionGroup::delete_all_by_collection(&col_id, &org_id, &conn).await?; for group in data.groups { - CollectionGroup::new(col_id.clone(), group.id, group.read_only, group.hide_passwords, group.manage) + CollectionGroup::new(col_id.clone(), group.id, group.read_only, group.hide_passwords, group.manage && may_grant_manage) .save(&org_id, &conn) .await?; } @@ -696,8 +805,15 @@ async fn post_organization_collection_update( continue; } - CollectionUser::save(&member.user_uuid, &col_id, user.read_only, user.hide_passwords, user.manage, &conn) - .await?; + CollectionUser::save( + &member.user_uuid, + &col_id, + user.read_only, + user.hide_passwords, + user.manage && may_grant_manage, + &conn, + ) + .await?; } Ok(Json(collection.to_json_details(&headers.user.uuid, None, &conn).await)) @@ -706,7 +822,7 @@ async fn post_organization_collection_update( async fn delete_organization_collection_impl( org_id: &OrganizationId, col_id: &CollectionId, - headers: &ManagerHeaders, + headers: &CollectionDeleteHeaders, conn: &DbConn, ) -> EmptyResult { if org_id != &headers.org_id { @@ -732,7 +848,7 @@ async fn delete_organization_collection_impl( async fn delete_organization_collection( org_id: OrganizationId, col_id: CollectionId, - headers: ManagerHeaders, + headers: CollectionDeleteHeaders, conn: DbConn, ) -> EmptyResult { delete_organization_collection_impl(&org_id, &col_id, &headers, &conn).await @@ -742,7 +858,7 @@ async fn delete_organization_collection( async fn post_organization_collection_delete( org_id: OrganizationId, col_id: CollectionId, - headers: ManagerHeaders, + headers: CollectionDeleteHeaders, conn: DbConn, ) -> EmptyResult { delete_organization_collection_impl(&org_id, &col_id, &headers, &conn).await @@ -768,7 +884,7 @@ async fn bulk_delete_organization_collections( let collections = data.ids; - let headers = ManagerHeaders::from_loose(headers, &collections, &conn).await?; + let headers = CollectionDeleteHeaders::from_loose(headers, &collections, &conn).await?; for col_id in collections { delete_organization_collection_impl(&org_id, &col_id, &headers, &conn).await?; @@ -780,23 +896,19 @@ async fn bulk_delete_organization_collections( async fn get_org_collection_detail( org_id: OrganizationId, col_id: CollectionId, - headers: ManagerHeaders, + headers: CollectionReadHeaders, conn: DbConn, ) -> JsonResult { if org_id != headers.org_id { err!("Organization not found", "Organization id's do not match"); } - match Collection::find_by_uuid_and_user(&col_id, headers.user.uuid.clone(), &conn).await { + match Collection::find_by_uuid_and_org(&col_id, &org_id, &conn).await { None => err!("Collection not found"), Some(collection) => { if collection.org_uuid != org_id { err!("Collection is not owned by organization") } - let Some(member) = Membership::find_by_user_and_org(&headers.user.uuid, &org_id, &conn).await else { - err!("User is not part of organization") - }; - let groups: Vec = if CONFIG.org_groups_enabled() { CollectionGroup::find_by_collection(&collection.uuid, &conn) .await @@ -830,7 +942,7 @@ async fn get_org_collection_detail( }) .collect(); - let assigned = Collection::can_access_collection(&member, &collection.uuid, &conn).await; + let assigned = Collection::can_access_collection(&headers.membership, &collection.uuid, &conn).await; let mut json_object = collection.to_json_details(&headers.user.uuid, None, &conn).await; json_object["assigned"] = json!(assigned); @@ -847,7 +959,7 @@ async fn get_org_collection_detail( async fn get_collection_users( org_id: OrganizationId, col_id: CollectionId, - headers: ManagerHeaders, + headers: CollectionReadHeaders, conn: DbConn, ) -> JsonResult { if org_id != headers.org_id { @@ -877,6 +989,49 @@ struct OrgIdData { organization_id: OrganizationId, } +fn filter_ciphers_for_organization(ciphers: Vec, org_id: &OrganizationId) -> Vec { + ciphers.into_iter().filter(|cipher| cipher.organization_uuid.as_ref() == Some(org_id)).collect() +} + +// The Admin Console uses this endpoint when the acting member is not allowed to read every +// cipher in the organization. In particular, a Custom member with only DeleteAnyCollection +// needs an empty successful response so the collection list can finish loading and expose its +// collection-only delete controls. +// +// Security: start from the regular user-visible cipher query and then constrain the result to +// the requested organization. DeleteAnyCollection itself must never make cipher contents visible. +#[get("/ciphers/organization-details/assigned?")] +async fn get_assigned_org_details(data: OrgIdData, headers: Headers, conn: DbConn) -> JsonResult { + if Membership::find_confirmed_by_user_and_org(&headers.user.uuid, &data.organization_id, &conn).await.is_none() { + err_code!( + "Resource not found.", + "User is not a confirmed member of the organization", + rocket::http::Status::NotFound.code + ); + } + + let ciphers = filter_ciphers_for_organization( + Cipher::find_by_user_visible(&headers.user.uuid, &conn).await, + &data.organization_id, + ); + let cipher_sync_data = CipherSyncData::new(&headers.user.uuid, CipherSyncType::User, &conn).await; + + let mut ciphers_json = Vec::with_capacity(ciphers.len()); + for cipher in ciphers { + ciphers_json.push( + cipher + .to_json(&headers.host, &headers.user.uuid, Some(&cipher_sync_data), CipherSyncType::User, &conn) + .await?, + ); + } + + Ok(Json(json!({ + "data": ciphers_json, + "object": "list", + "continuationToken": null, + }))) +} + #[get("/ciphers/organization-details?")] async fn get_org_details(data: OrgIdData, headers: ManagerHeadersLoose, conn: DbConn) -> JsonResult { if data.organization_id != headers.membership.org_uuid { @@ -940,10 +1095,14 @@ struct GetOrgUserData { async fn get_members( data: GetOrgUserData, org_id: OrganizationId, - headers: ManagerHeadersLoose, + // Security (audit M-1): the full member list exposes each member's PII, 2FA/enrollment status, + // permission flags and (optionally) collection/group assignments. Reading it requires the + // 'Manage Users' permission (or Admin/Owner), matching Bitwarden. Members who only need to + // reference other users (e.g. the collection dialog) use the member-readable mini-details. + headers: ManageUsersHeaders, conn: DbConn, ) -> JsonResult { - if org_id != headers.membership.org_uuid { + if org_id != headers.org_id { err!("Organization not found", "Organization id's do not match"); } let mut users_json = Vec::new(); @@ -998,6 +1157,61 @@ async fn post_org_keys( }))) } +#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] +// This is intentionally a permission bitmap: every field represents an independent API grant. +#[allow(clippy::struct_excessive_bools)] +struct CustomRolePermissions { + manage_users: bool, + manage_groups: bool, + manage_policies: bool, + create_new_collections: bool, + edit_any_collection: bool, + delete_any_collection: bool, +} + +impl CustomRolePermissions { + fn from_request(member_type: MembershipType, permissions: &HashMap) -> Self { + if member_type != MembershipType::Custom { + return Self::default(); + } + + let enabled = |key: &str| matches!(permissions.get(key), Some(Value::Bool(true))); + Self { + manage_users: enabled("manageUsers"), + manage_groups: enabled("manageGroups"), + manage_policies: enabled("managePolicies"), + create_new_collections: enabled("createNewCollections"), + edit_any_collection: enabled("editAnyCollection"), + delete_any_collection: enabled("deleteAnyCollection"), + } + } + + /// Bitwarden grants a Custom member with Edit any collection full read/edit/manage access to + /// organization ciphers. Vaultwarden's existing access_all flag is the internal data-plane + /// representation of that capability. Create and Delete remain completely independent. + fn access_all_for(self, member_type: MembershipType) -> bool { + member_type >= MembershipType::Admin || (member_type == MembershipType::Custom && self.edit_any_collection) + } + + fn differs_from(self, membership: &Membership) -> bool { + self.manage_users != membership.manage_users + || self.manage_groups != membership.manage_groups + || self.manage_policies != membership.manage_policies + || self.create_new_collections != membership.create_new_collections + || self.edit_any_collection != membership.edit_any_collection + || self.delete_any_collection != membership.delete_any_collection + } + + fn apply_to(self, membership: &mut Membership) { + membership.manage_users = self.manage_users; + membership.manage_groups = self.manage_groups; + membership.manage_policies = self.manage_policies; + membership.create_new_collections = self.create_new_collections; + membership.edit_any_collection = self.edit_any_collection; + membership.delete_any_collection = self.delete_any_collection; + } +} + #[derive(Deserialize)] #[serde(rename_all = "camelCase")] struct InviteData { @@ -1031,7 +1245,7 @@ impl InviteData { async fn send_invite( org_id: OrganizationId, data: Json, - headers: AdminHeaders, + headers: ManageUsersHeaders, conn: DbConn, ) -> EmptyResult { if org_id != headers.org_id { @@ -1040,13 +1254,8 @@ async fn send_invite( let data: InviteData = data.into_inner(); data.validate(&org_id, &conn).await?; - // HACK: We need the raw user-type to be sure custom role is selected to determine the access_all permission - // The from_str() will convert the custom role type into a manager role type let raw_type = &data.r#type.into_string(); - // Membership::from_str will convert custom (4) to manager (3) - let new_type = if let Some(new_type) = MembershipType::from_str(raw_type) { - new_type as i32 - } else { + let Some(new_type) = MembershipType::from_str(raw_type) else { err!("Invalid type") }; @@ -1054,14 +1263,10 @@ async fn send_invite( err!("Only Owners can invite Managers, Admins or Owners") } - // HACK: This converts the Custom role which has the `Manage all collections` box checked into an access_all flag - // Since the parent checkbox is not sent to the server we need to check and verify the child checkboxes - // If the box is not checked, the user will still be a manager, but not with the access_all permission - let access_all = new_type >= MembershipType::Admin - || (raw_type.eq("4") - && data.permissions.get("editAnyCollection") == Some(&json!(true)) - && data.permissions.get("deleteAnyCollection") == Some(&json!(true)) - && data.permissions.get("createNewCollections") == Some(&json!(true))); + // manageAllCollections is a client-only aggregate. Persist its three children independently; + // only Edit any collection maps to the existing all-cipher access representation. + let custom_permissions = CustomRolePermissions::from_request(new_type, &data.permissions); + let access_all = custom_permissions.access_all_for(new_type); let mut user_created: bool = false; for email in &data.emails { @@ -1104,7 +1309,8 @@ async fn send_invite( let mut new_member = Membership::new(user.uuid.clone(), org_id.clone(), Some(headers.user.email.clone())); new_member.access_all = access_all; - new_member.atype = new_type; + new_member.atype = new_type as i32; + custom_permissions.apply_to(&mut new_member); new_member.status = member_status; new_member.save(&conn).await?; @@ -1146,8 +1352,17 @@ 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 no accessAll, add the collections received - if !access_all { + if !access_all && caller_can_manage_collections { 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"), @@ -1166,9 +1381,28 @@ async fn send_invite( } } - for group_id in &data.groups { - let mut group_entry = GroupUser::new(group_id.clone(), new_member.uuid.clone()); - group_entry.save(&conn).await?; + // 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. + 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, + }; + + 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?; + } } } @@ -1179,7 +1413,7 @@ async fn send_invite( async fn bulk_reinvite_members( org_id: OrganizationId, data: Json, - headers: AdminHeaders, + headers: ManageUsersHeaders, conn: DbConn, ) -> JsonResult { if org_id != headers.org_id { @@ -1214,7 +1448,7 @@ async fn bulk_reinvite_members( async fn reinvite_member( org_id: OrganizationId, member_id: MembershipId, - headers: AdminHeaders, + headers: ManageUsersHeaders, conn: DbConn, ) -> EmptyResult { if org_id != headers.org_id { @@ -1345,7 +1579,7 @@ struct BulkConfirmData { async fn bulk_confirm_invite( org_id: OrganizationId, data: Json, - headers: AdminHeaders, + headers: ManageUsersHeaders, conn: DbConn, nt: Notify<'_>, ) -> JsonResult { @@ -1389,7 +1623,7 @@ async fn confirm_invite( org_id: OrganizationId, member_id: MembershipId, data: Json, - headers: AdminHeaders, + headers: ManageUsersHeaders, conn: DbConn, nt: Notify<'_>, ) -> EmptyResult { @@ -1402,7 +1636,7 @@ async fn confirm_invite_impl( org_id: &OrganizationId, member_id: &MembershipId, key: &str, - headers: &AdminHeaders, + headers: &ManageUsersHeaders, conn: &DbConn, nt: &Notify<'_>, ) -> EmptyResult { @@ -1487,7 +1721,7 @@ async fn get_user( org_id: OrganizationId, member_id: MembershipId, data: GetOrgUserData, - headers: AdminHeaders, + headers: ManageUsersHeaders, conn: DbConn, ) -> JsonResult { if org_id != headers.org_id { @@ -1518,7 +1752,7 @@ async fn put_member( org_id: OrganizationId, member_id: MembershipId, data: Json, - headers: AdminHeaders, + headers: ManageUsersHeaders, conn: DbConn, ) -> EmptyResult { edit_member(org_id, member_id, data, headers, conn).await @@ -1529,7 +1763,7 @@ async fn edit_member( org_id: OrganizationId, member_id: MembershipId, data: Json, - headers: AdminHeaders, + headers: ManageUsersHeaders, conn: DbConn, ) -> EmptyResult { if org_id != headers.org_id { @@ -1537,22 +1771,13 @@ async fn edit_member( } let data: EditUserData = data.into_inner(); - // HACK: We need the raw user-type to be sure custom role is selected to determine the access_all permission - // The from_str() will convert the custom role type into a manager role type let raw_type = &data.r#type.into_string(); - // MembershipType::from_str will convert custom (4) to manager (3) let Some(new_type) = MembershipType::from_str(raw_type) else { err!("Invalid type") }; - // HACK: This converts the Custom role which has the `Manage all collections` box checked into an access_all flag - // Since the parent checkbox is not sent to the server we need to check and verify the child checkboxes - // If the box is not checked, the user will still be a manager, but not with the access_all permission - let access_all = new_type >= MembershipType::Admin - || (raw_type.eq("4") - && data.permissions.get("editAnyCollection") == Some(&json!(true)) - && data.permissions.get("deleteAnyCollection") == Some(&json!(true)) - && data.permissions.get("createNewCollections") == Some(&json!(true))); + let custom_permissions = CustomRolePermissions::from_request(new_type, &data.permissions); + let access_all = custom_permissions.access_all_for(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") @@ -1565,6 +1790,18 @@ async fn edit_member( err!("Only Owners can grant and remove Admin or Owner privileges") } + // 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 Manager/Custom grants + // collection-"manage" on every collection they can already write (see the `atype >= Manager` + // 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 and access_all 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. + 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") + } + if member_to_edit.atype == MembershipType::Owner && headers.membership_type != MembershipType::Owner { err!("Only Owners can edit Owner users") } @@ -1579,43 +1816,134 @@ async fn edit_member( } } - member_to_edit.access_all = access_all; + // Security: only Admins and Owners may change the granular custom-role permissions. A Custom + // member with manage_users must not be able to grant them to themselves or others (a + // privilege escalation), nor strip flags an Admin/Owner has granted to fellow Custom members. + // Requests that leave the flags unchanged are allowed, so such members can still use the + // regular edit dialog. + if headers.membership_type < MembershipType::Admin && custom_permissions.differs_from(&member_to_edit) { + err!("Only Admins or Owners can change custom permissions") + } + + // Security: only callers who can actually manage collections (Admins/Owners, or users + // with full access) may change a member's collection assignments. A custom user with only + // manage_users must not be able to add/remove collection access, so we leave the existing + // assignments untouched for them. + 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, + }; + + // Security: `access_all` grants full access to every collection, so only callers who may + // manage collections are allowed to change it. Otherwise a custom user with only manage_users + // could enable Edit any collection on any member (including themselves) to grant full + // collection access — a privilege escalation. For everyone else we keep the member's existing + // access_all grant untouched (neither granted nor revoked). + if caller_can_manage_collections { + member_to_edit.access_all = access_all; + } + custom_permissions.apply_to(&mut member_to_edit); member_to_edit.atype = new_type as i32; // This check is also done at accept_invite, _confirm_invite, _activate_member, edit_member, admin::update_membership_type // We need to perform the check after changing the type since `admin` is exempt. OrgPolicy::check_user_allowed(&member_to_edit, "modify", &conn).await?; - // Delete all the odd collections - for c in CollectionUser::find_by_organization_and_user_uuid(&org_id, &member_to_edit.user_uuid, &conn).await { - c.delete(&conn).await?; - } - - // If no accessAll, add the collections received - if !access_all { - 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"), - Some(collection) => { - CollectionUser::save( - &member_to_edit.user_uuid, - &collection.uuid, - col.read_only, - col.hide_passwords, - col.manage, - &conn, - ) - .await?; + if caller_can_manage_collections { + // Delete all the odd collections + for c in CollectionUser::find_by_organization_and_user_uuid(&org_id, &member_to_edit.user_uuid, &conn).await { + c.delete(&conn).await?; + } + + // Security (F-1): a per-collection `manage` grant carries delete authority, so the caller + // may only confer it on collections they could delete themselves. A caller acting via + // Edit-any-collection thus cannot hand another member a manage/delete grant it lacks. + let caller = Membership::find_by_user_and_org(&headers.user.uuid, &org_id, &conn).await; + + // If no accessAll, add the collections received + if !access_all { + 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"), + Some(collection) => { + let manage = col.manage + && match &caller { + Some(c) => caller_may_grant_collection_manage(c, &collection.uuid, &conn).await, + None => false, + }; + CollectionUser::save( + &member_to_edit.user_uuid, + &collection.uuid, + col.read_only, + col.hide_passwords, + manage, + &conn, + ) + .await?; + } } } } } - GroupUser::delete_all_by_member(&member_to_edit.uuid, &conn).await?; + // Security: changing a member's group membership 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 change it. For others we leave group membership untouched. + 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, + }; - for group_id in data.groups.iter().flatten() { - let mut group_entry = GroupUser::new(group_id.clone(), member_to_edit.uuid.clone()); - group_entry.save(&conn).await?; + 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 + // group of a foreign tenant (e.g. an access-all group), which the direct cipher-access + // checks would then honor. Fail closed on the whole request if any group is foreign. + for group_id in data.groups.iter().flatten() { + if Group::find_by_uuid_and_org(group_id, &org_id, &conn).await.is_none() { + err!("Group not found in this organization") + } + } + + if caller_can_manage_collections { + // Caller may grant/revoke collection access via groups: full replace. + GroupUser::delete_all_by_member(&member_to_edit.uuid, &conn).await?; + + for group_id in data.groups.iter().flatten() { + let mut group_entry = GroupUser::new(group_id.clone(), member_to_edit.uuid.clone()); + group_entry.save(&conn).await?; + } + } else { + // Security: the caller may manage groups but NOT collections. They may only change the + // member's membership in groups that confer no collection access; collection-bearing + // memberships are preserved untouched (neither granted nor revoked), mirroring the + // restriction enforced in put_group_members and add_update_group. + + // Remove the member only from non-collection-bearing groups; keep collection-bearing + // memberships so this caller cannot revoke collection access either. + for gu in GroupUser::find_by_member(&member_to_edit.uuid, &conn).await { + if may_change_group_membership( + caller_can_manage_collections, + group_confers_collection_access(&gu.groups_uuid, &org_id, &conn).await, + ) { + GroupUser::delete_by_group_and_member(&gu.groups_uuid, &member_to_edit.uuid, &conn).await?; + } + } + + // Add the requested groups, skipping any that would grant collection access. + for group_id in data.groups.iter().flatten() { + 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(), member_to_edit.uuid.clone()); + group_entry.save(&conn).await?; + } + } } log_event( @@ -1636,7 +1964,7 @@ async fn edit_member( async fn bulk_delete_member( org_id: OrganizationId, data: Json, - headers: AdminHeaders, + headers: ManageUsersHeaders, conn: DbConn, nt: Notify<'_>, ) -> JsonResult { @@ -1672,7 +2000,7 @@ async fn bulk_delete_member( async fn delete_member( org_id: OrganizationId, member_id: MembershipId, - headers: AdminHeaders, + headers: ManageUsersHeaders, conn: DbConn, nt: Notify<'_>, ) -> EmptyResult { @@ -1682,7 +2010,7 @@ async fn delete_member( async fn delete_member_impl( org_id: &OrganizationId, member_id: &MembershipId, - headers: &AdminHeaders, + headers: &ManageUsersHeaders, conn: &DbConn, nt: &Notify<'_>, ) -> EmptyResult { @@ -1736,7 +2064,7 @@ async fn delete_member_impl( async fn bulk_public_keys( org_id: OrganizationId, data: Json, - headers: AdminHeaders, + headers: ManageUsersHeaders, conn: DbConn, ) -> JsonResult { if org_id != headers.org_id { @@ -1813,6 +2141,22 @@ async fn post_org_import( // TODO: See if we can optimize the whole cipher adding/importing and prevent duplicate code and checks. Cipher::validate_cipher_data(&data.ciphers)?; + // Robustness/DoS (audit M-3): validate every collection<->cipher relationship index against the + // import payload *before* creating any collection or cipher. `key` indexes into `ciphers` and + // `value` into `collections`; an out-of-range index would otherwise cause an out-of-bounds panic + // when the relations are applied below — a 500 (or a process abort under panic="abort") that + // happens after rows have already been written, leaving partial state behind. + let import_cipher_count = data.ciphers.len(); + let import_collection_count = data.collections.len(); + for relation in &data.collection_relationships { + if relation.key >= import_cipher_count || relation.value >= import_collection_count { + err!( + "Invalid collection relationship", + "A collection relationship references a non-existent cipher or collection" + ) + } + } + let existing_collections: HashSet> = Collection::find_by_organization(&org_id, &conn).await.into_iter().map(|c| Some(c.uuid)).collect(); let mut collections: Vec = Vec::with_capacity(data.collections.len()); @@ -1827,13 +2171,21 @@ async fn post_org_import( } col_id } else { - // We do not allow users or managers which can not manage all collections to create new collections - // If there is any collection other than an existing import collection, abort the import. - if headers.membership.atype <= MembershipType::Manager && !headers.membership.has_full_access() { + // Collection creation through an organization import is governed by the same + // independent permission as the regular create endpoint. In particular, + // Edit any collection's access_all mirror must not satisfy this check. + if !headers.membership.can_create_new_collections() { err!(Compact, "The current user isn't allowed to create new collections") } let new_collection = Collection::new(org_id.clone(), col.name, col.external_id); new_collection.save(&conn).await?; + // Import-created collections do not carry the regular create endpoint's user access + // selections. Give a create-only importer Manage access to the collection they just + // created, matching Bitwarden's organization-import behavior. + if !headers.membership.has_full_access() { + CollectionUser::save(&headers.membership.user_uuid, &new_collection.uuid, false, false, true, &conn) + .await?; + } new_collection.uuid }; @@ -1854,6 +2206,9 @@ async fn post_org_import( // Always clear folder_id's via an organization import cipher_data.folder_id = None; let mut cipher = Cipher::new(cipher_data.r#type, cipher_data.name.clone()); + // Propagate cipher-save failures instead of silently discarding them (audit M-3): a + // discarded error would still push the cipher id and let a relationship reference a cipher + // that was never persisted. This matches Bitwarden's all-or-nothing import semantics. update_cipher_from_data( &mut cipher, cipher_data, @@ -1863,15 +2218,19 @@ async fn post_org_import( &nt, UpdateType::None, ) - .await - .ok(); + .await?; ciphers.push(cipher.uuid); } - // Assign the collections + // Assign the collections. Indices were bounds-validated above, but use `.get()` here as well so + // any future drift fails closed with an error instead of panicking. for (cipher_index, col_index) in relations { - let cipher_id = &ciphers[cipher_index]; - let col_id = &collections[col_index]; + let (Some(cipher_id), Some(col_id)) = (ciphers.get(cipher_index), collections.get(col_index)) else { + err!( + "Invalid collection relationship", + "A collection relationship references a non-existent cipher or collection" + ) + }; CollectionCipher::save(cipher_id, col_id, &conn).await?; } @@ -1945,12 +2304,23 @@ async fn post_bulk_collections(data: Json, headers: Headers } #[get("/organizations//policies")] -async fn list_policies(org_id: OrganizationId, headers: AdminHeaders, conn: DbConn) -> JsonResult { - if org_id != headers.org_id { +async fn list_policies(org_id: OrganizationId, headers: ManagerHeadersLoose, conn: DbConn) -> JsonResult { + if org_id != headers.membership.org_uuid { err!("Organization not found", "Organization id's do not match"); } - let policies = OrgPolicy::find_by_org(&org_id, &conn).await; - let policies_json: Vec = policies.iter().map(OrgPolicy::to_json).collect(); + + // Security: only Admins/Owners, or Custom members holding the manage_policies permission, + // may see the actual policy configuration. Other Managers/Custom members (e.g. manage_users + // or manage_groups only) are still allowed to call this endpoint so the Admin Console can + // load, but they receive an empty list instead of the policy contents. + let can_view_policies = + headers.membership.atype >= MembershipType::Admin || headers.membership.has_manage_policies(); + + let policies_json: Vec = if can_view_policies { + OrgPolicy::find_by_org(&org_id, &conn).await.iter().map(OrgPolicy::to_json).collect() + } else { + Vec::new() + }; Ok(Json(json!({ "data": policies_json, @@ -2011,7 +2381,7 @@ async fn get_master_password_policy(org_id: OrganizationId, _headers: OrgMemberH } #[get("/organizations//policies/", rank = 3)] -async fn get_policy(org_id: OrganizationId, pol_type: i32, headers: AdminHeaders, conn: DbConn) -> JsonResult { +async fn get_policy(org_id: OrganizationId, pol_type: i32, headers: ManagePoliciesHeaders, conn: DbConn) -> JsonResult { if org_id != headers.org_id { err!("Organization not found", "Organization id's do not match"); } @@ -2048,7 +2418,7 @@ async fn put_policy( org_id: OrganizationId, pol_type: i32, data: Json, - headers: AdminHeaders, + headers: ManagePoliciesHeaders, conn: DbConn, ) -> JsonResult { if org_id != headers.org_id { @@ -2168,7 +2538,7 @@ async fn put_policy_vnext( org_id: OrganizationId, pol_type: i32, data: Json, - headers: AdminHeaders, + headers: ManagePoliciesHeaders, conn: DbConn, ) -> JsonResult { put_policy(org_id, pol_type, data, headers, conn).await @@ -2245,7 +2615,7 @@ struct BulkRevokeMembershipIds { async fn revoke_member( org_id: OrganizationId, member_id: MembershipId, - headers: AdminHeaders, + headers: ManageUsersHeaders, conn: DbConn, ) -> EmptyResult { revoke_member_impl(&org_id, &member_id, &headers, &conn).await @@ -2255,7 +2625,7 @@ async fn revoke_member( async fn bulk_revoke_members( org_id: OrganizationId, data: Json, - headers: AdminHeaders, + headers: ManageUsersHeaders, conn: DbConn, ) -> JsonResult { if org_id != headers.org_id { @@ -2294,7 +2664,7 @@ async fn bulk_revoke_members( async fn revoke_member_impl( org_id: &OrganizationId, member_id: &MembershipId, - headers: &AdminHeaders, + headers: &ManageUsersHeaders, conn: &DbConn, ) -> EmptyResult { if org_id != &headers.org_id { @@ -2305,6 +2675,12 @@ 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 { + 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") } @@ -2338,7 +2714,7 @@ async fn revoke_member_impl( async fn restore_member_vnext( org_id: OrganizationId, member_id: MembershipId, - headers: AdminHeaders, + headers: ManageUsersHeaders, conn: DbConn, ) -> EmptyResult { // Vaultwarden does not (yet) support the per User Collection linked to the `Enforce organization data ownership` policy. @@ -2350,7 +2726,7 @@ async fn restore_member_vnext( async fn restore_member( org_id: OrganizationId, member_id: MembershipId, - headers: AdminHeaders, + headers: ManageUsersHeaders, conn: DbConn, ) -> EmptyResult { restore_member_impl(&org_id, &member_id, &headers, &conn).await @@ -2360,7 +2736,7 @@ async fn restore_member( async fn bulk_restore_members( org_id: OrganizationId, data: Json, - headers: AdminHeaders, + headers: ManageUsersHeaders, conn: DbConn, ) -> JsonResult { if org_id != headers.org_id { @@ -2394,7 +2770,7 @@ async fn bulk_restore_members( async fn restore_member_impl( org_id: &OrganizationId, member_id: &MembershipId, - headers: &AdminHeaders, + headers: &ManageUsersHeaders, conn: &DbConn, ) -> EmptyResult { if org_id != &headers.org_id { @@ -2405,6 +2781,12 @@ 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 { + 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") } @@ -2432,15 +2814,7 @@ async fn restore_member_impl( Ok(()) } -async fn get_groups_data( - details: bool, - org_id: OrganizationId, - headers: ManagerHeadersLoose, - conn: DbConn, -) -> JsonResult { - if org_id != headers.membership.org_uuid { - err!("Organization not found", "Organization id's do not match"); - } +async fn get_groups_data(details: bool, org_id: OrganizationId, conn: DbConn) -> JsonResult { let groups: Vec = if CONFIG.org_groups_enabled() { let groups = Group::find_by_organization(&org_id, &conn).await; let mut groups_json = Vec::with_capacity(groups.len()); @@ -2468,14 +2842,25 @@ async fn get_groups_data( }))) } +// The plain group list (id, name, externalId) stays member-readable: the web vault needs it to +// render group names, and it exposes no access mappings. #[get("/organizations//groups")] async fn get_groups(org_id: OrganizationId, headers: ManagerHeadersLoose, conn: DbConn) -> JsonResult { - get_groups_data(false, org_id, headers, conn).await + if org_id != headers.membership.org_uuid { + err!("Organization not found", "Organization id's do not match"); + } + get_groups_data(false, org_id, conn).await } +// Security (audit M-1): group *details* expose accessAll, external IDs and collection mappings, so +// reading them requires the 'Manage Users' or 'Manage Groups' permission (or Admin/Owner), matching +// Bitwarden's ReadAll/ReadAllWithAccess authorization. #[get("/organizations//groups/details", rank = 1)] -async fn get_groups_details(org_id: OrganizationId, headers: ManagerHeadersLoose, conn: DbConn) -> JsonResult { - get_groups_data(true, org_id, headers, conn).await +async fn get_groups_details(org_id: OrganizationId, headers: ManageUsersOrGroupsHeaders, conn: DbConn) -> JsonResult { + if org_id != headers.org_id { + err!("Organization not found", "Organization id's do not match"); + } + get_groups_data(true, org_id, conn).await } #[derive(Deserialize)] @@ -2541,7 +2926,7 @@ async fn post_group( org_id: OrganizationId, group_id: GroupId, data: Json, - headers: AdminHeaders, + headers: ManageGroupsHeaders, conn: DbConn, ) -> JsonResult { put_group(org_id, group_id, data, headers, conn).await @@ -2550,7 +2935,7 @@ async fn post_group( #[post("/organizations//groups", data = "")] async fn post_groups( org_id: OrganizationId, - headers: AdminHeaders, + headers: ManageGroupsHeaders, data: Json, conn: DbConn, ) -> JsonResult { @@ -2564,7 +2949,21 @@ async fn post_groups( let group_request = data.into_inner(); group_request.validate(&org_id, &conn).await?; - let group = group_request.to_group(&org_id); + // Security: only callers who can manage collections may assign collections to a new group. + // A custom user with only manage_groups can create the group, but without 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, + }; + + let mut group = group_request.to_group(&org_id); + // Security: `access_all` grants the group access to every collection, so it is a + // collection-access grant just like assigning collections. A custom user without + // collection-management rights must not be able to create an access_all group. + if !caller_can_manage_collections { + group.access_all = false; + } log_event( EventType::GroupCreated as i32, @@ -2577,7 +2976,22 @@ async fn post_groups( ) .await; - add_update_group(group, group_request.collections, group_request.users, org_id, &headers, &conn).await + let collections_to_apply = if caller_can_manage_collections { + group_request.collections + } else { + Vec::new() + }; + + add_update_group( + group, + collections_to_apply, + group_request.users, + org_id, + &headers, + &conn, + caller_can_manage_collections, + ) + .await } #[put("/organizations//groups/", data = "")] @@ -2585,7 +2999,7 @@ async fn put_group( org_id: OrganizationId, group_id: GroupId, data: Json, - headers: AdminHeaders, + headers: ManageGroupsHeaders, conn: DbConn, ) -> JsonResult { if org_id != headers.org_id { @@ -2602,10 +3016,28 @@ async fn put_group( let group_request = data.into_inner(); group_request.validate(&org_id, &conn).await?; - let updated_group = group_request.update_group(group); + // Security: only callers who can actually manage collections (Admins/Owners, or users with + // full access) may change a group's collection assignments. A custom user with only + // manage_groups must not be able to add/remove collection access. For them we keep the + // group's existing collection assignments untouched (neither cleared nor overwritten). + 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, + }; - CollectionGroup::delete_all_by_group(&group_id, &org_id, &conn).await?; - GroupUser::delete_all_by_group(&group_id, &org_id, &conn).await?; + // Preserve the current `access_all` grant for callers who can't manage collections, so a + // manage_groups-only user cannot turn a group into an access_all (all-collections) grant. + let previous_access_all = group.access_all; + let mut updated_group = group_request.update_group(group); + if !caller_can_manage_collections { + updated_group.access_all = previous_access_all; + } + + if caller_can_manage_collections { + CollectionGroup::delete_all_by_group(&group_id, &org_id, &conn).await?; + } + // NOTE: group membership is replaced (and access-gated) inside add_update_group. log_event( EventType::GroupUpdated as i32, @@ -2618,7 +3050,113 @@ async fn put_group( ) .await; - add_update_group(updated_group, group_request.collections, group_request.users, org_id, &headers, &conn).await + // Only pass collection changes through if the caller is allowed to manage collections. + let collections_to_apply = if caller_can_manage_collections { + group_request.collections + } else { + Vec::new() + }; + add_update_group( + updated_group, + collections_to_apply, + group_request.users, + org_id, + &headers, + &conn, + caller_can_manage_collections, + ) + .await +} + +/// Whether a caller may change (add OR remove) a member's membership in a group. +/// +/// A caller who cannot manage collections must never touch a *collection-bearing* group's +/// membership: adding would indirectly grant collection access, removing would revoke it. +/// Callers who can manage collections may change any group's membership. This mirrors the +/// restriction already enforced inline in `put_group_members` and `add_update_group`. +fn may_change_group_membership(caller_can_manage_collections: bool, group_confers_collection_access: bool) -> bool { + caller_can_manage_collections || !group_confers_collection_access +} + +/// 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 +/// `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 +} + +/// 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 { + match Group::find_by_uuid_and_org(group_id, org_id, conn).await { + Some(group) => group.access_all || !CollectionGroup::find_by_group(group_id, org_id, conn).await.is_empty(), + None => false, + } +} + +/// Whether `caller` may set a per-collection `manage` grant (`users_collections.manage` / +/// `collections_groups.manage`) on `col_id`. +/// +/// Security (F-1, edit-any -> delete-any escalation): a `manage` grant carries collection *delete* +/// authority — `CollectionDeleteHeaders` accepts it via `has_explicit_collection_manage_access`. +/// Without this gate a Custom member holding only `edit_any_collection` (whose `access_all` mirror +/// makes every collection "manageable") could, through the collection-access / group endpoints, +/// hand a `manage` row to a group they belong to (or to a manager-level member) and thereby gain +/// deletion — a capability `edit_any_collection` must never imply. +/// +/// We therefore allow granting `manage` on a collection only to a caller who could delete that same +/// collection themselves, mirroring `collection_delete_access` exactly so it can never hand out a +/// right the caller lacks: Admin/Owner and Custom-with-`delete_any_collection` always qualify; an +/// exact legacy Manager uses its per-collection manage helper; any other Custom member must hold a +/// real explicit manage grant. This is strictly subtractive — it can only ever downgrade a requested +/// `manage` to `false`, never grant it — so it opens no new access, and delete-capable members +/// (including all Admins/Owners) are unaffected. +async fn caller_may_grant_collection_manage(caller: &Membership, col_id: &CollectionId, conn: &DbConn) -> bool { + match caller_manage_grant_role_check(caller) { + // Role alone decides it (Admin/Owner or delete_any -> yes; User/unknown/unconfirmed -> no). + Some(decision) => decision, + // Manager/Custom: the answer is per-collection and must reflect a *real* manage grant. + None => match MembershipType::from_i32(caller.atype) { + // The exact legacy Manager keeps its broad per-collection manage helper (which also + // honors membership/group access_all), matching its pre-existing delete authorization. + Some(MembershipType::Manager) => { + Collection::is_coll_manageable_by_user(col_id, &caller.user_uuid, conn).await + } + // A Custom member must prove a real users_collections.manage / collections_groups.manage + // grant; edit_any_collection's access_all mirror deliberately does not count here. + Some(MembershipType::Custom) => caller.has_explicit_collection_manage_access(col_id, conn).await, + _ => false, + }, + } +} + +/// Pure, collection-independent part of `caller_may_grant_collection_manage`. +/// +/// `Some(true)` -> the caller may grant `manage` on *any* collection (Admin/Owner, or a Custom +/// member holding `delete_any_collection`). +/// `Some(false)` -> the caller may never grant `manage` (unconfirmed, plain User, or unknown type). +/// `None` -> depends on a real per-collection manage grant, resolved against the database. +/// +/// Kept separate so the role gating — in particular that `edit_any_collection` alone yields `None` +/// (a DB check for a genuine grant) rather than `Some(true)` — is unit-testable without a DB. +fn caller_manage_grant_role_check(caller: &Membership) -> Option { + if caller.can_delete_any_collection() { + return Some(true); // Admin/Owner, or a Custom member holding delete_any_collection + } + if !caller.has_status(MembershipStatus::Confirmed) { + return Some(false); + } + match MembershipType::from_i32(caller.atype) { + Some(MembershipType::Manager | MembershipType::Custom) => None, + _ => Some(false), + } } async fn add_update_group( @@ -2626,30 +3164,54 @@ async fn add_update_group( collections: Vec, members: Vec, org_id: OrganizationId, - headers: &AdminHeaders, + headers: &ManageGroupsHeaders, conn: &DbConn, + caller_can_manage_collections: bool, ) -> JsonResult { group.save(conn).await?; + // Security (F-1): a `collections_groups.manage` grant carries collection delete authority, so a + // caller may only set it on a collection they could delete themselves. This stops a caller whose + // access derives from Edit-any-collection from creating a manage-bearing group and then joining + // it to reach Delete-any-collection. Fetched once; delete-capable callers keep `manage`. + let caller = Membership::find_by_user_and_org(&headers.user.uuid, &org_id, conn).await; for col_selection in collections { let mut collection_group = col_selection.to_collection_group(group.uuid.clone()); + if collection_group.manage { + let may_grant_manage = match &caller { + Some(c) => caller_may_grant_collection_manage(c, &collection_group.collections_uuid, conn).await, + None => false, + }; + collection_group.manage = may_grant_manage; + } collection_group.save(&org_id, conn).await?; } - for assigned_member in members { - let mut user_entry = GroupUser::new(group.uuid.clone(), assigned_member.clone()); - user_entry.save(conn).await?; + // Security: assigning members to a group that grants collection access (via `access_all` + // or assigned collections) would indirectly grant those members access to the collections' + // contents. Only callers who can manage collections may change the membership of such a + // group; for others we leave the group's membership untouched. + let group_grants_collection_access = + group.access_all || !CollectionGroup::find_by_group(&group.uuid, &org_id, conn).await.is_empty(); - log_event( - EventType::OrganizationUserUpdatedGroups as i32, - &assigned_member, - &org_id, - &headers.user.uuid, - headers.device.atype, - &headers.ip.ip, - conn, - ) - .await; + if caller_can_manage_collections || !group_grants_collection_access { + GroupUser::delete_all_by_group(&group.uuid, &org_id, conn).await?; + + for assigned_member in members { + let mut user_entry = GroupUser::new(group.uuid.clone(), assigned_member.clone()); + user_entry.save(conn).await?; + + log_event( + EventType::OrganizationUserUpdatedGroups as i32, + &assigned_member, + &org_id, + &headers.user.uuid, + headers.device.atype, + &headers.ip.ip, + conn, + ) + .await; + } } Ok(Json(json!({ @@ -2662,11 +3224,16 @@ async fn add_update_group( }))) } +// Reads a single group's details (accessAll, externalId, collection mappings). This is the same +// data the `/groups/details` list endpoint returns, so it uses the same guard: Manage Users OR +// Manage Groups (or Admin/Owner). Requiring Manage Groups here while the list only requires Manage +// Users-or-Groups would let a manage_users member read every group's details in bulk but be denied +// the single-group view of the same data. #[get("/organizations//groups//details")] async fn get_group_details( org_id: OrganizationId, group_id: GroupId, - headers: AdminHeaders, + headers: ManageUsersOrGroupsHeaders, conn: DbConn, ) -> JsonResult { if org_id != headers.org_id { @@ -2687,21 +3254,26 @@ async fn get_group_details( async fn post_delete_group( org_id: OrganizationId, group_id: GroupId, - headers: AdminHeaders, + headers: ManageGroupsHeaders, conn: DbConn, ) -> EmptyResult { delete_group_impl(&org_id, &group_id, &headers, &conn).await } #[delete("/organizations//groups/")] -async fn delete_group(org_id: OrganizationId, group_id: GroupId, headers: AdminHeaders, conn: DbConn) -> EmptyResult { +async fn delete_group( + org_id: OrganizationId, + group_id: GroupId, + headers: ManageGroupsHeaders, + conn: DbConn, +) -> EmptyResult { delete_group_impl(&org_id, &group_id, &headers, &conn).await } async fn delete_group_impl( org_id: &OrganizationId, group_id: &GroupId, - headers: &AdminHeaders, + headers: &ManageGroupsHeaders, conn: &DbConn, ) -> EmptyResult { if org_id != &headers.org_id { @@ -2715,6 +3287,23 @@ async fn delete_group_impl( err!("Group not found", "Group uuid is invalid or does not belong to the organization") }; + // Security: deleting a group that grants collection access (via `access_all` or assigned + // collections) revokes that access for all its members. A custom user with only manage_groups + // must not be able to affect collection access, so only callers who can actually manage + // collections (Admins/Owners or users with full access) may delete such a group. Mirrors the + // restriction in put_group_members / post_delete_group_member. Also covers bulk_delete_groups, + // which funnels through this function. + 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 !caller_can_manage_collections + && (group.access_all || !CollectionGroup::find_by_group(group_id, org_id, conn).await.is_empty()) + { + err!("You don't have permission to delete a group that grants collection access") + } + log_event( EventType::GroupDeleted as i32, &group.uuid, @@ -2733,7 +3322,7 @@ async fn delete_group_impl( async fn bulk_delete_groups( org_id: OrganizationId, data: Json, - headers: AdminHeaders, + headers: ManageGroupsHeaders, conn: DbConn, ) -> EmptyResult { if org_id != headers.org_id { @@ -2752,7 +3341,12 @@ async fn bulk_delete_groups( } #[get("/organizations//groups/", rank = 2)] -async fn get_group(org_id: OrganizationId, group_id: GroupId, headers: AdminHeaders, conn: DbConn) -> JsonResult { +async fn get_group( + org_id: OrganizationId, + group_id: GroupId, + headers: ManageGroupsHeaders, + conn: DbConn, +) -> JsonResult { if org_id != headers.org_id { err!("Organization not found", "Organization id's do not match"); } @@ -2771,7 +3365,7 @@ async fn get_group(org_id: OrganizationId, group_id: GroupId, headers: AdminHead async fn get_group_members( org_id: OrganizationId, group_id: GroupId, - headers: AdminHeaders, + headers: ManageGroupsHeaders, conn: DbConn, ) -> JsonResult { if org_id != headers.org_id { @@ -2798,7 +3392,7 @@ async fn get_group_members( async fn put_group_members( org_id: OrganizationId, group_id: GroupId, - headers: AdminHeaders, + headers: ManageGroupsHeaders, data: Json>, conn: DbConn, ) -> EmptyResult { @@ -2809,8 +3403,24 @@ async fn put_group_members( err!("Group support is disabled"); } - if Group::find_by_uuid_and_org(&group_id, &org_id, &conn).await.is_none() { + let Some(group) = Group::find_by_uuid_and_org(&group_id, &org_id, &conn).await else { err!("Group could not be found!", "Group uuid is invalid or does not belong to the organization") + }; + + // Security: changing the membership of a group that grants collection access (via + // `access_all` or assigned collections) indirectly grants those members access to the + // collections' contents. Only callers who can actually manage collections (Admins/Owners + // or users with full access) may do this. A custom user with only manage_groups may manage + // the membership of groups that grant no collection access, but not of collection-bearing ones. + 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, + }; + let group_grants_collection_access = + group.access_all || !CollectionGroup::find_by_group(&group_id, &org_id, &conn).await.is_empty(); + if !caller_can_manage_collections && group_grants_collection_access { + err!("You don't have permission to change the membership of a group that grants collection access") } let assigned_members = data.into_inner(); @@ -2846,7 +3456,7 @@ async fn post_delete_group_member( org_id: OrganizationId, group_id: GroupId, member_id: MembershipId, - headers: AdminHeaders, + headers: ManageGroupsHeaders, conn: DbConn, ) -> EmptyResult { if org_id != headers.org_id { @@ -2864,6 +3474,20 @@ async fn post_delete_group_member( err!("Group could not be found or does not belong to the organization."); } + // Security: removing a member from a group that grants collection access (via `access_all` + // or assigned collections) revokes that member's collection access. A custom user with only + // manage_groups must not be able to affect collection access, so only callers who can actually + // manage collections (Admins/Owners or users with full access) may do this. Mirrors the + // restriction enforced in put_group_members. + 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 !caller_can_manage_collections && group_confers_collection_access(&group_id, &org_id, &conn).await { + err!("You don't have permission to change the membership of a group that grants collection access") + } + log_event( EventType::OrganizationUserUpdatedGroups as i32, &member_id, @@ -3213,3 +3837,193 @@ async fn rotate_api_key( ) -> JsonResult { api_key(&org_id, data, true, headers, conn).await } + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + + 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, + }; + use crate::db::models::{Cipher, 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); + m.atype = member_type as i32; + m.status = MembershipStatus::Confirmed as i32; + m + } + + #[test] + fn only_delete_capable_callers_may_grant_collection_manage() { + // Admin/Owner may always confer a per-collection `manage` (delete) grant. + assert_eq!(caller_manage_grant_role_check(&confirmed_member(MembershipType::Owner)), Some(true)); + assert_eq!(caller_manage_grant_role_check(&confirmed_member(MembershipType::Admin)), Some(true)); + + // A Custom member with `delete_any_collection` may also always grant it. + let mut delete_any = confirmed_member(MembershipType::Custom); + delete_any.delete_any_collection = true; + assert_eq!(caller_manage_grant_role_check(&delete_any), Some(true)); + + // REGRESSION (F-1): a Custom member with ONLY `edit_any_collection` must NOT get a blanket + // yes. The role check returns None so the decision falls through to a real per-collection + // manage grant in the DB — which a self-assigned group/user manage row is prevented from + // manufacturing. This is what stops edit-any from escalating into delete-any. + let mut edit_any = confirmed_member(MembershipType::Custom); + edit_any.edit_any_collection = true; + edit_any.access_all = true; // the internal mirror of edit_any must not shortcut to yes + assert_eq!(caller_manage_grant_role_check(&edit_any), None); + + // A flagless Custom / exact Manager also defer to the per-collection DB check. + assert_eq!(caller_manage_grant_role_check(&confirmed_member(MembershipType::Custom)), None); + assert_eq!(caller_manage_grant_role_check(&confirmed_member(MembershipType::Manager)), None); + + // Plain User never qualifies. + assert_eq!(caller_manage_grant_role_check(&confirmed_member(MembershipType::User)), Some(false)); + + // An unconfirmed caller never qualifies, even with delete_any set. + let mut unconfirmed = confirmed_member(MembershipType::Custom); + unconfirmed.status = MembershipStatus::Accepted as i32; + unconfirmed.delete_any_collection = true; + assert_eq!(caller_manage_grant_role_check(&unconfirmed), Some(false)); + } + + #[test] + fn assigned_cipher_response_is_scoped_to_requested_organization() { + let requested_org: OrganizationId = "requested-org".to_owned().into(); + let other_org: OrganizationId = "other-org".to_owned().into(); + + let mut requested_cipher = Cipher::new(1, "requested".to_owned()); + requested_cipher.organization_uuid = Some(requested_org.clone()); + let requested_cipher_id = requested_cipher.uuid.clone(); + + let mut other_cipher = Cipher::new(1, "other".to_owned()); + other_cipher.organization_uuid = Some(other_org); + + let personal_cipher = Cipher::new(1, "personal".to_owned()); + + let filtered = + filter_ciphers_for_organization(vec![other_cipher, personal_cipher, requested_cipher], &requested_org); + + assert_eq!(filtered.len(), 1); + assert_eq!(filtered[0].uuid, requested_cipher_id); + assert_eq!(filtered[0].organization_uuid.as_ref(), Some(&requested_org)); + } + + #[test] + fn manage_users_caller_cannot_change_member_role() { + let user = MembershipType::User as i32; + let manager = MembershipType::Manager as i32; + let custom = MembershipType::Custom as i32; + + // Admins and Owners may change a member's role. + assert!(may_change_member_type(MembershipType::Owner, user, MembershipType::Manager)); + assert!(may_change_member_type(MembershipType::Admin, user, MembershipType::Custom)); + + // A below-Admin caller (Manager / Custom-with-manage_users) may only submit an unchanged + // role, so the regular edit dialog keeps working. + assert!(may_change_member_type(MembershipType::Custom, user, MembershipType::User)); + assert!(may_change_member_type(MembershipType::Custom, custom, MembershipType::Custom)); + assert!(may_change_member_type(MembershipType::Manager, manager, MembershipType::Manager)); + + // REGRESSION (privilege escalation, PR #7397 / finding F1): a caller below Admin must NOT + // be able to change a member's role. Promoting User -> Manager/Custom grants that member + // collection-"manage" on their writable collections (atype >= Manager), and demoting + // revokes it — collection-access changes a manage_users caller is not entitled to make. + assert!(!may_change_member_type(MembershipType::Custom, user, MembershipType::Manager)); + assert!(!may_change_member_type(MembershipType::Custom, user, MembershipType::Custom)); + assert!(!may_change_member_type(MembershipType::Custom, manager, MembershipType::User)); + assert!(!may_change_member_type(MembershipType::Manager, custom, MembershipType::User)); + } + + #[test] + fn manage_groups_caller_cannot_grant_collection_access_via_groups() { + // A caller who can manage collections may change membership of any group. + assert!(may_change_group_membership(true, true)); + assert!(may_change_group_membership(true, false)); + + // A caller who cannot manage collections may change membership of groups that confer no + // collection access (plain groups). + assert!(may_change_group_membership(false, false)); + + // REGRESSION (privilege escalation, PR #7397): a caller who cannot manage collections must + // NOT be able to change membership of a collection-bearing / access_all group. This is the + // vector that let a Custom user with manage_users + manage_groups add themselves to an + // access_all group and read all collection contents via edit_member / send_invite. Adding + // AND removing such memberships must be denied. + assert!(!may_change_group_membership(false, true)); + } + + #[test] + fn collection_permission_request_combinations_remain_independent() { + for mask in 0_u8..8 { + let create = mask & 0b001 != 0; + let edit = mask & 0b010 != 0; + let delete = mask & 0b100 != 0; + let permissions = HashMap::from([ + ("createNewCollections".to_owned(), json!(create)), + ("editAnyCollection".to_owned(), json!(edit)), + ("deleteAnyCollection".to_owned(), json!(delete)), + ]); + + let parsed = CustomRolePermissions::from_request(MembershipType::Custom, &permissions); + assert_eq!(parsed.create_new_collections, create, "mask={mask:03b}"); + assert_eq!(parsed.edit_any_collection, edit, "mask={mask:03b}"); + assert_eq!(parsed.delete_any_collection, delete, "mask={mask:03b}"); + // Only Edit any collection maps to all-cipher access. Create/Delete must never do so. + assert_eq!(parsed.access_all_for(MembershipType::Custom), edit, "mask={mask:03b}"); + } + } + + #[test] + fn custom_permission_parser_is_strict_and_non_custom_roles_are_fail_closed() { + let permissions = HashMap::from([ + ("manageUsers".to_owned(), Value::String("true".to_owned())), + ("manageGroups".to_owned(), json!(true)), + ("managePolicies".to_owned(), json!(true)), + ("createNewCollections".to_owned(), json!(true)), + ("editAnyCollection".to_owned(), json!(true)), + ("deleteAnyCollection".to_owned(), json!(true)), + ]); + + let custom = CustomRolePermissions::from_request(MembershipType::Custom, &permissions); + assert!(!custom.manage_users, "string values must not be accepted as booleans"); + assert!(custom.manage_groups); + assert!(custom.manage_policies); + assert!(custom.create_new_collections); + assert!(custom.edit_any_collection); + assert!(custom.delete_any_collection); + + let user = CustomRolePermissions::from_request(MembershipType::User, &permissions); + assert_eq!(user, CustomRolePermissions::default()); + assert!(!user.access_all_for(MembershipType::User)); + + let admin = CustomRolePermissions::from_request(MembershipType::Admin, &permissions); + assert_eq!(admin, CustomRolePermissions::default()); + assert!(admin.access_all_for(MembershipType::Admin)); + } + + #[test] + fn custom_permission_change_detection_covers_collection_flags() { + let mut membership = Membership::new("test-user".to_owned().into(), "test-org".to_owned().into(), None); + membership.atype = MembershipType::Custom as i32; + membership.status = MembershipStatus::Confirmed as i32; + + let requested = CustomRolePermissions { + create_new_collections: true, + edit_any_collection: true, + delete_any_collection: true, + ..CustomRolePermissions::default() + }; + + assert!(requested.differs_from(&membership)); + requested.apply_to(&mut membership); + assert!(!requested.differs_from(&membership)); + assert!(membership.create_new_collections); + assert!(membership.edit_any_collection); + assert!(membership.delete_any_collection); + } +} diff --git a/src/auth.rs b/src/auth.rs index 88a59b4b..786f864c 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -708,6 +708,7 @@ pub struct OrgHeaders { pub host: String, pub device: Device, pub user: User, + #[allow(dead_code)] pub membership_type: MembershipType, pub membership_status: MembershipStatus, pub membership: Membership, @@ -729,6 +730,31 @@ impl OrgHeaders { fn is_confirmed_and_owner(&self) -> bool { self.membership_status == MembershipStatus::Confirmed && self.membership_type == MembershipType::Owner } + fn is_confirmed(&self) -> bool { + self.membership_status == MembershipStatus::Confirmed + } + // Custom-role permission checks. Admins and Owners implicitly hold every + // permission; a Custom member holds a permission only if the matching flag + // is set on their Membership. The has_* helpers gate the flags on the + // Custom type, so stale flags on other types can never grant anything. + fn can_manage_users(&self) -> bool { + self.is_confirmed() && (self.membership_type >= MembershipType::Admin || self.membership.has_manage_users()) + } + fn can_manage_groups(&self) -> bool { + self.is_confirmed() && (self.membership_type >= MembershipType::Admin || self.membership.has_manage_groups()) + } + fn can_manage_policies(&self) -> bool { + self.is_confirmed() && (self.membership_type >= MembershipType::Admin || self.membership.has_manage_policies()) + } + // Reading the full member/group *details* (PII, 2FA status, permission flags, access mappings) + // requires the ability to manage users or groups, matching Bitwarden's `ReadAll`/`ReadAllWithAccess` + // authorization. Basic member mini-details and the plain group list remain member-readable. + fn can_manage_users_or_groups(&self) -> bool { + self.is_confirmed() + && (self.membership_type >= MembershipType::Admin + || self.membership.has_manage_users() + || self.membership.has_manage_groups()) + } } // org_id is usually the second path param ("/organizations/"), @@ -842,6 +868,76 @@ impl<'r> FromRequest<'r> for AdminHeaders { } } +// Macro to generate a request guard that permits a confirmed Admin/Owner, or a +// confirmed Custom member holding the given permission. The generated struct +// mirrors AdminHeaders so it can be used as a drop-in replacement on endpoints. +macro_rules! generate_manage_headers { + ($name:ident, $check:ident, $err:literal) => { + #[allow(dead_code)] + pub struct $name { + pub host: String, + pub device: Device, + pub user: User, + pub membership_type: MembershipType, + pub ip: ClientIp, + pub org_id: OrganizationId, + } + + #[rocket::async_trait] + impl<'r> FromRequest<'r> for $name { + type Error = &'static str; + + async fn from_request(request: &'r Request<'_>) -> Outcome { + let headers = try_outcome!(OrgHeaders::from_request(request).await); + if headers.$check() { + Outcome::Success(Self { + host: headers.host, + device: headers.device, + user: headers.user, + membership_type: headers.membership_type, + ip: headers.ip, + org_id: headers.membership.org_uuid, + }) + } else { + err_handler!($err) + } + } + } + + impl From<$name> for Headers { + fn from(h: $name) -> Headers { + Headers { + host: h.host, + device: h.device, + user: h.user, + ip: h.ip, + } + } + } + }; +} + +generate_manage_headers!( + ManageUsersHeaders, + can_manage_users, + "You need the 'Manage Users' permission, or to be an Admin or Owner, to call this endpoint" +); +generate_manage_headers!( + ManageGroupsHeaders, + can_manage_groups, + "You need the 'Manage Groups' permission, or to be an Admin or Owner, to call this endpoint" +); +generate_manage_headers!( + ManagePoliciesHeaders, + can_manage_policies, + "You need the 'Manage Policies' permission, or to be an Admin or Owner, to call this endpoint" +); +generate_manage_headers!( + ManageUsersOrGroupsHeaders, + can_manage_users_or_groups, + "You need the 'Manage Users' or 'Manage Groups' permission, or to be an Admin or Owner, to call this endpoint" +); + // col_id is usually the fourth path param ("/organizations//collections/"), // but there could be cases where it is a query value. // First check the path, if this is not a valid uuid, try the query values. @@ -861,9 +957,69 @@ fn get_col_id(request: &Request<'_>) -> Option { None } -/// The ManagerHeaders are used to check if you are at least a Manager -/// and have access to the specific collection provided via the /collections/collectionId. -/// This does strict checking on the collection_id, ManagerHeadersLoose does not. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum CollectionManageAccess { + Any, + LegacyManager, + ExplicitManage, + Denied, +} + +fn collection_access_by_role(membership: &Membership, custom_has_any_access: bool) -> CollectionManageAccess { + if !membership.has_status(MembershipStatus::Confirmed) { + return CollectionManageAccess::Denied; + } + + match MembershipType::from_i32(membership.atype) { + Some(MembershipType::Owner | MembershipType::Admin) => CollectionManageAccess::Any, + // Keep the pre-Custom role's broad behavior isolated to an exact legacy Manager. Its + // existing helper intentionally accepts membership/group access_all. + Some(MembershipType::Manager) => CollectionManageAccess::LegacyManager, + Some(MembershipType::Custom) if custom_has_any_access => CollectionManageAccess::Any, + // A Custom member must prove an actual users_collections.manage or + // collections_groups.manage assignment. In particular, groups.access_all is not Manage. + Some(MembershipType::Custom) => CollectionManageAccess::ExplicitManage, + Some(MembershipType::User) | None => CollectionManageAccess::Denied, + } +} + +fn collection_edit_access(membership: &Membership) -> CollectionManageAccess { + collection_access_by_role(membership, membership.has_edit_any_collection()) +} + +fn collection_read_access(membership: &Membership) -> CollectionManageAccess { + collection_access_by_role( + membership, + membership.has_edit_any_collection() || membership.has_delete_any_collection(), + ) +} + +fn collection_delete_access(membership: &Membership) -> CollectionManageAccess { + collection_access_by_role(membership, membership.has_delete_any_collection()) +} + +async fn can_manage_collection( + access: CollectionManageAccess, + membership: &Membership, + collection_uuid: &CollectionId, + conn: &DbConn, +) -> bool { + match access { + CollectionManageAccess::Any => true, + CollectionManageAccess::LegacyManager => { + Collection::is_coll_manageable_by_user(collection_uuid, &membership.user_uuid, conn).await + } + CollectionManageAccess::ExplicitManage => { + membership.has_explicit_collection_manage_access(collection_uuid, conn).await + } + CollectionManageAccess::Denied => false, + } +} + +/// ManagerHeaders authorizes collection updates. A Custom member with Edit any collection can +/// update every collection; otherwise the caller must be at least a legacy Manager and have the +/// per-collection Manage permission. Read and delete use separate guards so Edit cannot +/// accidentally imply Delete. pub struct ManagerHeaders { pub host: String, pub device: Device, @@ -880,12 +1036,15 @@ impl<'r> FromRequest<'r> for ManagerHeaders { let headers = try_outcome!(OrgHeaders::from_request(request).await); if headers.is_confirmed_and_manager() { if let Some(col_id) = get_col_id(request) { - let Outcome::Success(conn) = DbConn::from_request(request).await else { - err_handler!("Error getting DB") - }; - - if !Collection::is_coll_manageable_by_user(&col_id, &headers.membership.user_uuid, &conn).await { - err_handler!("The current user isn't a manager for this collection") + let access = collection_edit_access(&headers.membership); + if access != CollectionManageAccess::Any { + let Outcome::Success(conn) = DbConn::from_request(request).await else { + err_handler!("Error getting DB") + }; + + if !can_manage_collection(access, &headers.membership, &col_id, &conn).await { + err_handler!("The current user isn't a manager for this collection") + } } } else { err_handler!("Error getting the collection id") @@ -904,6 +1063,132 @@ impl<'r> FromRequest<'r> for ManagerHeaders { } } +/// Read access to collection metadata and assignment details. Delete any collection needs this +/// visibility to render the standard collection view, but it does not grant edit or cipher access. +pub struct CollectionReadHeaders { + pub host: String, + pub device: Device, + pub user: User, + pub membership: Membership, + pub ip: ClientIp, + pub org_id: OrganizationId, +} + +#[rocket::async_trait] +impl<'r> FromRequest<'r> for CollectionReadHeaders { + type Error = &'static str; + + async fn from_request(request: &'r Request<'_>) -> Outcome { + let headers = try_outcome!(OrgHeaders::from_request(request).await); + if !headers.is_confirmed_and_manager() { + err_handler!("You need collection read permission to call this endpoint") + } + + let Some(col_id) = get_col_id(request) else { + err_handler!("Error getting the collection id") + }; + + let access = collection_read_access(&headers.membership); + + if access != CollectionManageAccess::Any { + let Outcome::Success(conn) = DbConn::from_request(request).await else { + err_handler!("Error getting DB") + }; + + if !can_manage_collection(access, &headers.membership, &col_id, &conn).await { + err_handler!("The current user isn't a manager for this collection") + } + } + + Outcome::Success(Self { + host: headers.host, + device: headers.device, + user: headers.user, + ip: headers.ip, + org_id: headers.membership.org_uuid.clone(), + membership: headers.membership, + }) + } +} + +impl From for Headers { + fn from(h: CollectionReadHeaders) -> Headers { + Headers { + host: h.host, + device: h.device, + user: h.user, + ip: h.ip, + } + } +} + +/// Delete is intentionally independent from Edit any collection. Vaultwarden advertises +/// limitCollectionDeletion=true, so deleting *any* collection requires the explicit Delete any +/// collection permission (or Admin/Owner). Deleting an individual collection is additionally +/// allowed for members holding the per-collection Manage grant on it. Custom members use the +/// explicit assignment only; unlike the exact legacy Manager path, membership/group access_all +/// never counts as their per-collection Manage grant. +pub struct CollectionDeleteHeaders { + pub host: String, + pub device: Device, + pub user: User, + pub ip: ClientIp, + pub org_id: OrganizationId, +} + +#[rocket::async_trait] +impl<'r> FromRequest<'r> for CollectionDeleteHeaders { + type Error = &'static str; + + async fn from_request(request: &'r Request<'_>) -> Outcome { + let headers = try_outcome!(OrgHeaders::from_request(request).await); + if !headers.is_confirmed_and_manager() { + err_handler!("You need collection delete permission to call this endpoint") + } + + let Some(col_id) = get_col_id(request) else { + err_handler!("Error getting the collection id") + }; + + match collection_delete_access(&headers.membership) { + CollectionManageAccess::Any => {} + CollectionManageAccess::Denied => { + // Custom is a distinct, fail-closed role. Edit any collection and access_all alone + // must not satisfy a Delete request without either Delete any or explicit Manage. + err_handler!("You need the 'Delete any collection' permission to call this endpoint") + } + access @ (CollectionManageAccess::LegacyManager | CollectionManageAccess::ExplicitManage) => { + let Outcome::Success(conn) = DbConn::from_request(request).await else { + err_handler!("Error getting DB") + }; + + if !can_manage_collection(access, &headers.membership, &col_id, &conn).await { + err_handler!("The current user isn't a manager for this collection") + } + } + } + + Outcome::Success(Self { + host: headers.host, + device: headers.device, + user: headers.user, + ip: headers.ip, + org_id: headers.membership.org_uuid, + }) + } +} + +impl From for Headers { + fn from(h: CollectionDeleteHeaders) -> Headers { + Headers { + host: h.host, + device: h.device, + user: h.user, + ip: h.ip, + } + } +} + impl From for Headers { fn from(h: ManagerHeaders) -> Headers { Headers { @@ -956,22 +1241,32 @@ impl From for Headers { } } -impl ManagerHeaders { +impl CollectionDeleteHeaders { pub async fn from_loose( h: ManagerHeadersLoose, collections: &Vec, conn: &DbConn, - ) -> Result { + ) -> Result { + let delete_access = collection_delete_access(&h.membership); + if delete_access == CollectionManageAccess::Denied { + err!("You need the 'Delete any collection' permission to call this endpoint") + } + for col_id in collections { if uuid::Uuid::parse_str(col_id.as_ref()).is_err() { err!("Collection Id is malformed!"); } - if !Collection::is_coll_manageable_by_user(col_id, &h.membership.user_uuid, conn).await { + if Collection::find_by_uuid_and_org(col_id, &h.membership.org_uuid, conn).await.is_none() { + err!("Collection not found", "Collection does not exist or does not belong to this organization") + } + if delete_access != CollectionManageAccess::Any + && !can_manage_collection(delete_access, &h.membership, col_id, conn).await + { err!("Collection not found", "The current user isn't a manager for this collection") } } - Ok(ManagerHeaders { + Ok(CollectionDeleteHeaders { host: h.host, device: h.device, user: h.user, @@ -1313,3 +1608,82 @@ pub async fn refresh_tokens( Ok((device, auth_tokens)) } + +#[cfg(test)] +mod tests { + use super::{CollectionManageAccess, collection_delete_access, collection_edit_access, collection_read_access}; + use crate::db::models::{Membership, MembershipStatus, MembershipType}; + + fn membership(member_type: MembershipType) -> 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 = MembershipStatus::Confirmed as i32; + membership + } + + #[test] + fn flagless_custom_requires_explicit_manage_for_edit_read_and_delete() { + let custom = membership(MembershipType::Custom); + assert_eq!(collection_edit_access(&custom), CollectionManageAccess::ExplicitManage); + assert_eq!(collection_read_access(&custom), CollectionManageAccess::ExplicitManage); + assert_eq!(collection_delete_access(&custom), CollectionManageAccess::ExplicitManage); + + // Neither a stale membership access_all value nor an external groups.access_all grant may + // switch a Custom member to the legacy broad helper. ExplicitManage invokes the database + // helper that only accepts users_collections.manage / collections_groups.manage. + let mut access_all = membership(MembershipType::Custom); + access_all.access_all = true; + assert_eq!(collection_edit_access(&access_all), CollectionManageAccess::ExplicitManage); + assert_eq!(collection_read_access(&access_all), CollectionManageAccess::ExplicitManage); + assert_eq!(collection_delete_access(&access_all), CollectionManageAccess::ExplicitManage); + } + + #[test] + fn custom_any_permissions_remain_independent() { + let mut edit_any = membership(MembershipType::Custom); + edit_any.edit_any_collection = true; + edit_any.access_all = true; + assert_eq!(collection_edit_access(&edit_any), CollectionManageAccess::Any); + assert_eq!(collection_read_access(&edit_any), CollectionManageAccess::Any); + // Edit-any alone is not blanket Delete. It still permits deletion of an explicitly managed + // collection, which is why the result is ExplicitManage rather than Denied. + assert_eq!(collection_delete_access(&edit_any), CollectionManageAccess::ExplicitManage); + + let mut delete_any = membership(MembershipType::Custom); + delete_any.delete_any_collection = true; + assert_eq!(collection_edit_access(&delete_any), CollectionManageAccess::ExplicitManage); + assert_eq!(collection_read_access(&delete_any), CollectionManageAccess::Any); + assert_eq!(collection_delete_access(&delete_any), CollectionManageAccess::Any); + } + + #[test] + fn exact_legacy_manager_keeps_broad_helper() { + let manager = membership(MembershipType::Manager); + assert_eq!(collection_edit_access(&manager), CollectionManageAccess::LegacyManager); + assert_eq!(collection_read_access(&manager), CollectionManageAccess::LegacyManager); + assert_eq!(collection_delete_access(&manager), CollectionManageAccess::LegacyManager); + + let admin = membership(MembershipType::Admin); + assert_eq!(collection_edit_access(&admin), CollectionManageAccess::Any); + assert_eq!(collection_delete_access(&admin), CollectionManageAccess::Any); + + let user = membership(MembershipType::User); + assert_eq!(collection_edit_access(&user), CollectionManageAccess::Denied); + assert_eq!(collection_delete_access(&user), CollectionManageAccess::Denied); + } + + #[test] + fn migrated_legacy_manager_retains_explicit_collection_manage() { + // The role migration converts legacy Managers to flagless Custom members. They retain + // edit/delete only for collections with a persisted per-collection Manage assignment; + // the restrictive helper deliberately excludes group and membership access_all. + let migrated_manager = membership(MembershipType::Custom); + assert_eq!(collection_edit_access(&migrated_manager), CollectionManageAccess::ExplicitManage); + assert_eq!(collection_delete_access(&migrated_manager), CollectionManageAccess::ExplicitManage); + + let mut unconfirmed = membership(MembershipType::Custom); + unconfirmed.status = MembershipStatus::Accepted as i32; + assert_eq!(collection_edit_access(&unconfirmed), CollectionManageAccess::Denied); + assert_eq!(collection_delete_access(&unconfirmed), CollectionManageAccess::Denied); + } +} diff --git a/src/db/models/cipher.rs b/src/db/models/cipher.rs index 2fa6260a..69600e5b 100644 --- a/src/db/models/cipher.rs +++ b/src/db/models/cipher.rs @@ -592,6 +592,24 @@ impl Cipher { cipher_sync_data: Option<&CipherSyncData>, conn: &DbConn, ) -> Option<(bool, bool, bool)> { + // Security: central fail-closed check binding cipher -> organization -> confirmed membership. + // + // In the direct (non-sync) authorization path an organization cipher is only accessible to a + // user who has a *confirmed* membership in that same organization. This denies access to + // members whose collection/group assignment rows still exist after they were revoked (or are + // still only invited/accepted), and to cross-organization collection/group assignments that + // another code path might have persisted. Without it, the queries below would keep granting + // access from those stale or cross-tenant rows (security audit findings H-1, H-2, H-3). + // + // The sync path (cipher_sync_data is Some) is intentionally left to the caller: it is built + // only from confirmed memberships and evaluated below against that cached data. + if cipher_sync_data.is_none() + && let Some(ref org_uuid) = self.organization_uuid + && Membership::find_confirmed_by_user_and_org(user_uuid, org_uuid, conn).await.is_none() + { + return None; + } + // Check whether this cipher is directly owned by the user, or is in // a collection that the user has full access to. If so, there are no // access restrictions. @@ -657,16 +675,34 @@ impl Cipher { } async fn get_user_collections_access_flags(&self, user_uuid: &UserId, conn: &DbConn) -> Vec<(bool, bool, bool)> { + let cipher_uuid = self.uuid.clone(); + let user_uuid = user_uuid.clone(); conn.run(move |conn| { // Check whether this cipher is in any collections accessible to the // user. If so, retrieve the access flags for each collection. + // + // Security: bind the assignment to a *confirmed* membership in the same organization as + // both the cipher and the collection. Without this, a `users_collections` row left behind + // after a revoke, or an assignment pointing at a collection in a different organization, + // would keep granting access (defense in depth for audit findings H-1 and H-3). ciphers::table - .filter(ciphers::uuid.eq(&self.uuid)) + .filter(ciphers::uuid.eq(cipher_uuid)) .inner_join(ciphers_collections::table.on(ciphers::uuid.eq(ciphers_collections::cipher_uuid))) + .inner_join( + collections::table.on(collections::uuid + .eq(ciphers_collections::collection_uuid) + .and(collections::org_uuid.nullable().eq(ciphers::organization_uuid))), + ) .inner_join( users_collections::table.on(ciphers_collections::collection_uuid .eq(users_collections::collection_uuid) - .and(users_collections::user_uuid.eq(user_uuid))), + .and(users_collections::user_uuid.eq(user_uuid.clone()))), + ) + .inner_join( + users_organizations::table.on(users_organizations::user_uuid + .eq(user_uuid) + .and(users_organizations::org_uuid.eq(collections::org_uuid)) + .and(users_organizations::status.eq(MembershipStatus::Confirmed as i32))), ) .select((users_collections::read_only, users_collections::hide_passwords, users_collections::manage)) .load::<(bool, bool, bool)>(conn) @@ -679,9 +715,16 @@ impl Cipher { if !CONFIG.org_groups_enabled() { return Vec::new(); } + let cipher_uuid = self.uuid.clone(); + let user_uuid = user_uuid.clone(); conn.run(move |conn| { + // Security: bind the group assignment to a *confirmed* membership and require that the + // cipher, the collection, the group and the membership all belong to the same + // organization. The `collections` join in particular prevents a cross-organization + // collection<->group assignment from granting access to a foreign organization's ciphers + // (defense in depth for audit findings H-1, H-2 and H-3). ciphers::table - .filter(ciphers::uuid.eq(&self.uuid)) + .filter(ciphers::uuid.eq(cipher_uuid)) .inner_join(ciphers_collections::table.on(ciphers::uuid.eq(ciphers_collections::cipher_uuid))) .inner_join( collections_groups::table @@ -689,13 +732,21 @@ impl Cipher { ) .inner_join(groups_users::table.on(groups_users::groups_uuid.eq(collections_groups::groups_uuid))) .inner_join( - users_organizations::table.on(users_organizations::uuid.eq(groups_users::users_organizations_uuid)), + users_organizations::table.on(users_organizations::uuid + .eq(groups_users::users_organizations_uuid) + .and(users_organizations::status.eq(MembershipStatus::Confirmed as i32))), ) .inner_join( groups::table.on(groups::uuid .eq(collections_groups::groups_uuid) .and(groups::organizations_uuid.eq(users_organizations::org_uuid))), ) + .inner_join( + collections::table.on(collections::uuid + .eq(ciphers_collections::collection_uuid) + .and(collections::org_uuid.eq(groups::organizations_uuid)) + .and(collections::org_uuid.nullable().eq(ciphers::organization_uuid))), + ) .filter(users_organizations::user_uuid.eq(user_uuid)) .select((collections_groups::read_only, collections_groups::hide_passwords, collections_groups::manage)) .load::<(bool, bool, bool)>(conn) diff --git a/src/db/models/collection.rs b/src/db/models/collection.rs index f29843f7..51a8d2c2 100644 --- a/src/db/models/collection.rs +++ b/src/db/models/collection.rs @@ -102,8 +102,9 @@ impl Collection { // Owners and Admins always have true. Users are not able to have full access Some(m) if m.has_full_access() => (false, false, m.atype >= MembershipType::Manager), Some(m) => { - // Only let a manager manage collections when the have full read/write access - let is_manager = m.atype == MembershipType::Manager; + // Only let a manager-level member (Manager or Custom) manage collections + // when they have full read/write access + let is_manager = m.atype >= MembershipType::Manager; if let Some(cu) = cipher_sync_data.user_collections.get(&self.uuid) { ( cu.read_only, @@ -125,11 +126,11 @@ 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::Manager), - Some(m) if m.atype == MembershipType::Manager && self.is_manageable_by_user(user_uuid, conn).await => { + Some(m) if m.atype >= MembershipType::Manager && self.is_manageable_by_user(user_uuid, conn).await => { (false, false, true) } Some(m) => { - let is_manager = m.atype == MembershipType::Manager; + let is_manager = m.atype >= MembershipType::Manager; 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) @@ -945,7 +946,7 @@ impl CollectionMembership { "hidePasswords": self.hide_passwords, "manage": membership_type >= MembershipType::Admin || self.manage - || (membership_type == MembershipType::Manager + || (membership_type >= MembershipType::Manager && !self.read_only && !self.hide_passwords), }) diff --git a/src/db/models/group.rs b/src/db/models/group.rs index 820d3700..2f5348d7 100644 --- a/src/db/models/group.rs +++ b/src/db/models/group.rs @@ -13,7 +13,7 @@ use crate::{ }; use macros::UuidFromParam; -use super::{CollectionId, Membership, MembershipId, OrganizationId, User, UserId}; +use super::{Collection, CollectionId, Membership, MembershipId, MembershipStatus, OrganizationId, User, UserId}; #[derive(Identifiable, Queryable, Insertable, AsChangeset)] #[diesel(table_name = groups)] @@ -257,6 +257,7 @@ impl Group { .and(groups::organizations_uuid.eq(users_organizations::org_uuid))), ) .filter(users_organizations::user_uuid.eq(user_uuid)) + .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) .filter(groups::access_all.eq(true)) .select(groups::organizations_uuid) .distinct() @@ -268,12 +269,19 @@ impl Group { pub async fn is_in_full_access_group(user_uuid: &UserId, org_uuid: &OrganizationId, conn: &DbConn) -> bool { conn.run(move |conn| { + // Security: the membership linked through `groups_users` must itself belong to the same + // organization as the group and must be confirmed. Otherwise a cross-organization + // `groups_users` row (a member of org A linked to an access-all group of org B) would let + // that member pass as having full access to org B (audit finding H-2). groups::table .inner_join(groups_users::table.on(groups_users::groups_uuid.eq(groups::uuid))) .inner_join( - users_organizations::table.on(users_organizations::uuid.eq(groups_users::users_organizations_uuid)), + users_organizations::table.on(users_organizations::uuid + .eq(groups_users::users_organizations_uuid) + .and(users_organizations::org_uuid.eq(groups::organizations_uuid))), ) .filter(users_organizations::user_uuid.eq(user_uuid)) + .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) .filter(groups::organizations_uuid.eq(org_uuid)) .filter(groups::access_all.eq(true)) .select(groups::access_all) @@ -319,6 +327,17 @@ impl Group { impl CollectionGroup { pub async fn save(&mut self, org_uuid: &OrganizationId, conn: &DbConn) -> EmptyResult { + // Security (audit H-3): never persist a cross-organization link between a collection and a + // group. Both must belong to the organization this assignment is scoped to; otherwise a + // caller could attach a foreign-tenant group to this organization's collection and thereby + // grant that group's members access to it. This is a defense-in-depth guard so no route can + // create such a link even if it fails to validate its inputs. + if Collection::find_by_uuid_and_org(&self.collections_uuid, org_uuid, conn).await.is_none() + || Group::find_by_uuid_and_org(&self.groups_uuid, org_uuid, conn).await.is_none() + { + err!("Collection and group must belong to the same organization") + } + let group_users = GroupUser::find_by_group(&self.groups_uuid, org_uuid, conn).await; for group_user in group_users { group_user.update_user_revision(conn).await; @@ -493,6 +512,18 @@ impl CollectionGroup { impl GroupUser { pub async fn save(&mut self, conn: &DbConn) -> EmptyResult { + // Security (audit H-2): never persist a cross-organization link between a group and a + // membership. The group must belong to the same organization as the membership; otherwise a + // caller could grant a member of one organization full access to another organization's + // collections through an access-all group. This is a defense-in-depth guard so no route can + // create such a link even if it fails to validate its inputs. + let Some(member) = Membership::find_by_uuid(&self.users_organizations_uuid, conn).await else { + err!("Member not found while assigning to group") + }; + if Group::find_by_uuid_and_org(&self.groups_uuid, &member.org_uuid, conn).await.is_none() { + err!("Group and member must belong to the same organization") + } + self.update_user_revision(conn).await; db_run! { conn: diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index 72b1df0b..81ceb15c 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -15,8 +15,8 @@ use crate::{ db::{ DbConn, schema::{ - ciphers, ciphers_collections, collections_groups, groups, groups_users, org_policies, organization_api_key, - organizations, users, users_collections, users_organizations, + ciphers, ciphers_collections, collections, collections_groups, groups, groups_users, org_policies, + organization_api_key, organizations, users, users_collections, users_organizations, }, }, error::MapResult, @@ -44,6 +44,7 @@ pub struct Organization { #[diesel(table_name = users_organizations)] #[diesel(treat_none_as_null = true)] #[diesel(primary_key(uuid))] +#[allow(clippy::struct_excessive_bools)] pub struct Membership { pub uuid: MembershipId, pub user_uuid: UserId, @@ -57,6 +58,12 @@ pub struct Membership { pub atype: i32, pub reset_password_key: Option, pub external_id: Option, + pub manage_users: bool, + pub manage_groups: bool, + pub manage_policies: bool, + pub create_new_collections: bool, + pub edit_any_collection: bool, + pub delete_any_collection: bool, } #[derive(Identifiable, Queryable, Insertable, AsChangeset)] @@ -98,36 +105,39 @@ pub enum MembershipType { Admin = 1, User = 2, Manager = 3, + Custom = 4, } impl MembershipType { pub fn from_str(s: &str) -> Option { - #[expect( - clippy::match_same_arms, - reason = "Specifically define `4|Custom` since this is a hack, not a default" - )] match s { "0" | "Owner" => Some(MembershipType::Owner), "1" | "Admin" => Some(MembershipType::Admin), "2" | "User" => Some(MembershipType::User), "3" | "Manager" => Some(MembershipType::Manager), - // HACK: We convert the custom role to a manager role - "4" | "Custom" => Some(MembershipType::Manager), + "4" | "Custom" => Some(MembershipType::Custom), _ => None, } } + + const fn access_rank(self) -> u8 { + match self { + Self::User => 0, + Self::Manager | Self::Custom => 1, + Self::Admin => 2, + Self::Owner => 3, + } + } } impl Ord for MembershipType { fn cmp(&self, other: &MembershipType) -> Ordering { - // For easy comparison, map each variant to an access level (where 0 is lowest). - const ACCESS_LEVEL: [i32; 4] = [ - 3, // Owner - 2, // Admin - 0, // User - 1, // Manager && Custom - ]; - ACCESS_LEVEL[*self as usize].cmp(&ACCESS_LEVEL[*other as usize]) + // Manager and Custom intentionally share the same authorization rank. A total ordering + // still has to distinguish unequal enum variants, otherwise `Ord` would disagree with + // `Eq` and ordered maps/sets could collapse one role into the other. The discriminant is a + // stable tie-breaker and places Custom after Manager, preserving `Custom >= Manager` while + // keeping both roles below Admin. + self.access_rank().cmp(&other.access_rank()).then_with(|| (*self as i32).cmp(&(*other as i32))) } } @@ -267,6 +277,12 @@ impl Membership { atype: MembershipType::User as i32, reset_password_key: None, external_id: None, + manage_users: false, + manage_groups: false, + manage_policies: false, + create_new_collections: false, + edit_any_collection: false, + delete_any_collection: false, } } @@ -306,15 +322,6 @@ impl Membership { } false } - - /// HACK: Convert the manager type to a custom type - /// It will be converted back on other locations - pub fn type_manager_as_custom(&self) -> i32 { - match self.atype { - 3 => 4, - _ => self.atype, - } - } } impl OrganizationApiKey { @@ -443,29 +450,29 @@ impl Membership { pub async fn to_json(&self, conn: &DbConn) -> Value { let org = Organization::find_by_uuid(&self.org_uuid, conn).await.unwrap(); - // HACK: Convert the manager type to a custom type - // It will be converted back on other locations - let membership_type = self.type_manager_as_custom(); + let membership_type = self.atype; let permissions = json!({ - // TODO: Add full support for Custom User Roles - // See: https://bitwarden.com/help/article/user-types-access-control/#custom-role - // Currently we use the custom role as a manager role and link the 3 Collection roles to mimic the access_all permission "accessEventLogs": false, "accessImportExport": false, "accessReports": false, - // If the following 3 Collection roles are set to true a custom user has access all permission - "createNewCollections": membership_type == 4 && self.access_all, - "editAnyCollection": membership_type == 4 && self.access_all, - "deleteAnyCollection": membership_type == 4 && self.access_all, - "manageGroups": false, - "managePolicies": 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, + "manageGroups": membership_type == MembershipType::Custom as i32 && self.manage_groups, + "managePolicies": membership_type == MembershipType::Custom as i32 && self.manage_policies, "manageSso": false, // Not supported - "manageUsers": false, + "manageUsers": membership_type == MembershipType::Custom as i32 && self.manage_users, "manageResetPassword": false, "manageScim": false // Not supported (Not AGPLv3 Licensed) }); + // edit_any_collection is internally mirrored to access_all to provide Bitwarden-compatible + // cipher access, but it must not accidentally grant collection creation. The client treats + // limitCollectionCreation=false as an independent create grant, so compute it from the + // actual role/permission rather than access_all for Custom members. + let limit_collection_creation = self.limit_collection_creation(); + // https://github.com/bitwarden/server/blob/9ebe16587175b1c0e9208f84397bb75d0d595510/src/Api/AdminConsole/Models/Response/ProfileOrganizationResponseModel.cs json!({ "id": self.org_uuid, @@ -509,8 +516,7 @@ impl Membership { "familySponsorshipValidUntil": null, "familySponsorshipToDelete": null, "accessSecretsManager": false, - // limit collection creation to managers with access_all permission to prevent issues - "limitCollectionCreation": self.atype < MembershipType::Manager || !self.access_all, + "limitCollectionCreation": limit_collection_creation, "limitCollectionDeletion": true, "limitItemDeletion": false, "allowAdminAccessToAllCollectionItems": true, @@ -585,7 +591,7 @@ impl Membership { ( cu.read_only, cu.hide_passwords, - cu.manage || (self.atype == MembershipType::Manager && !cu.read_only && !cu.hide_passwords), + cu.manage || (self.atype >= MembershipType::Manager && !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 @@ -607,28 +613,22 @@ impl Membership { Vec::new() }; - // HACK: Convert the manager type to a custom type - // It will be converted back on other locations - let membership_type = self.type_manager_as_custom(); + let membership_type = self.atype; - // HACK: Only return permissions if the user is of type custom and has access_all - // Else Bitwarden will assume the defaults of all false - let permissions = if membership_type == 4 && self.access_all { + // Only return a permissions object for custom-type members. Otherwise Bitwarden assumes + // all-false defaults and the role itself supplies any elevated capabilities. + let permissions = if membership_type == MembershipType::Custom as i32 { json!({ - // TODO: Add full support for Custom User Roles - // See: https://bitwarden.com/help/article/user-types-access-control/#custom-role - // Currently we use the custom role as a manager role and link the 3 Collection roles to mimic the access_all permission "accessEventLogs": false, "accessImportExport": false, "accessReports": false, - // If the following 3 Collection roles are set to true a custom user has access all permission - "createNewCollections": true, - "editAnyCollection": true, - "deleteAnyCollection": true, - "manageGroups": false, - "managePolicies": false, + "createNewCollections": self.create_new_collections, + "editAnyCollection": self.edit_any_collection, + "deleteAnyCollection": self.delete_any_collection, + "manageGroups": self.manage_groups, + "managePolicies": self.manage_policies, "manageSso": false, // Not supported - "manageUsers": false, + "manageUsers": self.manage_users, "manageResetPassword": false, "manageScim": false // Not supported (Not AGPLv3 Licensed) }) @@ -728,7 +728,7 @@ impl Membership { json!({ "id": self.uuid, "userId": self.user_uuid, - "type": self.type_manager_as_custom(), // HACK: Convert the manager type to a custom type + "type": self.atype, "status": status, "name": user.name, "email": user.email, @@ -816,7 +816,143 @@ impl Membership { } pub fn has_full_access(&self) -> bool { - (self.access_all || self.atype >= MembershipType::Admin) && self.has_status(MembershipStatus::Confirmed) + (self.access_all || self.has_edit_any_collection() || self.atype >= MembershipType::Admin) + && self.has_status(MembershipStatus::Confirmed) + } + + // The granular custom permission flags are only meaningful while the membership is of + // the Custom type. Gating them on the type here ensures that a stale flag left over from + // a type change (e.g. via the admin panel) can never grant anything. + pub fn has_manage_users(&self) -> bool { + self.has_type(MembershipType::Custom) && self.manage_users + } + + pub fn has_manage_groups(&self) -> bool { + self.has_type(MembershipType::Custom) && self.manage_groups + } + + pub fn has_manage_policies(&self) -> bool { + self.has_type(MembershipType::Custom) && self.manage_policies + } + + pub fn has_create_new_collections(&self) -> bool { + self.has_type(MembershipType::Custom) && self.create_new_collections + } + + pub fn has_edit_any_collection(&self) -> bool { + self.has_type(MembershipType::Custom) && self.edit_any_collection + } + + pub fn has_delete_any_collection(&self) -> bool { + self.has_type(MembershipType::Custom) && self.delete_any_collection + } + + /// 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. + pub async fn has_explicit_collection_manage_access(&self, collection_uuid: &CollectionId, conn: &DbConn) -> bool { + let membership_uuid = self.uuid.clone(); + let user_uuid = self.user_uuid.clone(); + let org_uuid = self.org_uuid.clone(); + let collection_uuid = collection_uuid.clone(); + + conn.run(move |conn| { + let has_direct_manage = users_organizations::table + .inner_join( + users_collections::table.on(users_collections::user_uuid.eq(users_organizations::user_uuid)), + ) + .inner_join( + collections::table.on(collections::uuid + .eq(users_collections::collection_uuid) + .and(collections::org_uuid.eq(users_organizations::org_uuid))), + ) + .filter(users_organizations::uuid.eq(membership_uuid.clone())) + .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(collections::uuid.eq(collection_uuid.clone())) + .filter(users_collections::manage.eq(true)) + .count() + .first::(conn) + .unwrap_or(0) + != 0; + + if has_direct_manage { + return true; + } + + users_organizations::table + .inner_join( + groups_users::table.on(groups_users::users_organizations_uuid.eq(users_organizations::uuid)), + ) + .inner_join( + groups::table.on(groups::uuid + .eq(groups_users::groups_uuid) + .and(groups::organizations_uuid.eq(users_organizations::org_uuid))), + ) + .inner_join(collections_groups::table.on(collections_groups::groups_uuid.eq(groups_users::groups_uuid))) + .inner_join( + collections::table.on(collections::uuid + .eq(collections_groups::collections_uuid) + .and(collections::org_uuid.eq(users_organizations::org_uuid))), + ) + .filter(users_organizations::uuid.eq(membership_uuid)) + .filter(users_organizations::user_uuid.eq(user_uuid)) + .filter(users_organizations::org_uuid.eq(org_uuid)) + .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) + .filter(collections::uuid.eq(collection_uuid)) + .filter(collections_groups::manage.eq(true)) + .count() + .first::(conn) + .unwrap_or(0) + != 0 + }) + .await + } + + /// `manageAllCollections` is a client-side aggregate checkbox, not a separately persisted + /// Bitwarden permission. It is selected exactly when all three child permissions are selected. + pub fn has_manage_all_collections(&self) -> bool { + self.has_create_new_collections() && self.has_edit_any_collection() && self.has_delete_any_collection() + } + + /// Match Vaultwarden's existing collection-creation policy while keeping the new Custom + /// permission independent from edit/delete. Legacy Manager memberships retain their former + /// access_all-based behavior. + pub fn can_create_new_collections(&self) -> bool { + if !self.has_status(MembershipStatus::Confirmed) { + return false; + } + + match MembershipType::from_i32(self.atype) { + Some(MembershipType::Owner | MembershipType::Admin) => true, + Some(MembershipType::Manager) => self.access_all, + Some(MembershipType::Custom) => self.create_new_collections, + Some(MembershipType::User) | None => false, + } + } + + pub fn limit_collection_creation(&self) -> bool { + match MembershipType::from_i32(self.atype) { + Some(MembershipType::Owner | MembershipType::Admin) => false, + Some(MembershipType::Manager) => !self.access_all, + Some(MembershipType::Custom) => !self.create_new_collections, + Some(MembershipType::User) | None => true, + } + } + + pub fn can_delete_any_collection(&self) -> bool { + self.has_status(MembershipStatus::Confirmed) + && (self.atype >= MembershipType::Admin || self.has_delete_any_collection()) + } + + pub fn clear_custom_permissions(&mut self) { + self.manage_users = false; + self.manage_groups = false; + self.manage_policies = false; + self.create_new_collections = false; + self.edit_any_collection = false; + self.delete_any_collection = false; } pub async fn find_by_uuid(uuid: &MembershipId, conn: &DbConn) -> Option { @@ -925,7 +1061,7 @@ impl Membership { .await } - // Get all users which are either owner or admin, or a manager which can manage/access all + // Get all users which are either owner or admin, or a manager/custom member which can manage/access all pub async fn find_confirmed_and_manage_all_by_org(org_uuid: &OrganizationId, conn: &DbConn) -> Vec { conn.run(move |conn| { users_organizations::table @@ -935,7 +1071,7 @@ impl Membership { users_organizations::atype .eq_any(vec![MembershipType::Owner as i32, MembershipType::Admin as i32]) .or(users_organizations::atype - .eq(MembershipType::Manager as i32) + .eq_any(vec![MembershipType::Manager as i32, MembershipType::Custom as i32]) .and(users_organizations::access_all.eq(true))), ) .load::(conn) @@ -1264,12 +1400,128 @@ pub struct OrgApiKeyId(String); mod tests { use super::*; + fn membership(member_type: MembershipType) -> 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 = MembershipStatus::Confirmed as i32; + membership + } + #[test] - #[allow(non_snake_case)] - fn partial_cmp_MembershipType() { + fn membership_type_order_preserves_access_rank_and_ord_contract() { assert!(MembershipType::Owner > MembershipType::Admin); - assert!(MembershipType::Admin > MembershipType::Manager); + assert!(MembershipType::Admin > MembershipType::Custom); + assert!(MembershipType::Custom > MembershipType::Manager); assert!(MembershipType::Manager > MembershipType::User); - assert!(MembershipType::Manager == MembershipType::from_str("4").unwrap()); + assert!(MembershipType::Custom == MembershipType::from_str("4").unwrap()); + + // Permission comparisons continue to treat Custom as manager-level and below Admin. + assert!(MembershipType::Custom >= MembershipType::Manager); + let custom = MembershipType::Custom as i32; + assert!(custom >= MembershipType::Manager); + assert!(custom < MembershipType::Admin); + + let types = [ + MembershipType::Owner, + MembershipType::Admin, + MembershipType::User, + MembershipType::Manager, + MembershipType::Custom, + ]; + for lhs in types { + for rhs in types { + assert_eq!(lhs.cmp(&rhs) == Ordering::Equal, lhs == rhs); + assert_eq!(lhs.cmp(&rhs), rhs.cmp(&lhs).reverse()); + } + } + } + + #[test] + fn custom_collection_permissions_are_independent_and_type_gated() { + let mut member = membership(MembershipType::Custom); + member.create_new_collections = true; + + assert!(member.has_create_new_collections()); + assert!(member.can_create_new_collections()); + assert!(!member.limit_collection_creation()); + assert!(!member.has_full_access()); + assert!(!member.can_delete_any_collection()); + assert!(!member.has_manage_all_collections()); + + member.delete_any_collection = true; + assert!(member.has_delete_any_collection()); + assert!(member.can_delete_any_collection()); + assert!(!member.has_full_access()); + assert!(!member.has_manage_all_collections()); + + member.edit_any_collection = true; + assert!(member.has_edit_any_collection()); + assert!(member.has_full_access()); + assert!(member.has_manage_all_collections()); + + // Stale flags on a non-Custom role are inert. + member.atype = MembershipType::User as i32; + assert!(!member.has_create_new_collections()); + assert!(!member.has_edit_any_collection()); + assert!(!member.has_delete_any_collection()); + assert!(!member.can_create_new_collections()); + assert!(!member.can_delete_any_collection()); + assert!(!member.has_full_access()); + } + + #[test] + fn edit_any_collection_does_not_imply_create_or_delete() { + let mut custom = membership(MembershipType::Custom); + custom.edit_any_collection = true; + // The persisted access_all mirror is intentionally tested too: client-facing create and + // delete decisions must still use their dedicated permissions. + custom.access_all = true; + + assert!(custom.has_full_access()); + assert!(!custom.can_create_new_collections()); + assert!(custom.limit_collection_creation()); + assert!(!custom.can_delete_any_collection()); + + let mut manager = membership(MembershipType::Manager); + manager.access_all = true; + assert!(manager.can_create_new_collections()); + + let admin = membership(MembershipType::Admin); + assert!(admin.can_create_new_collections()); + assert!(!admin.limit_collection_creation()); + assert!(admin.can_delete_any_collection()); + } + + #[test] + fn custom_collection_permissions_require_confirmed_membership() { + let mut member = membership(MembershipType::Custom); + member.create_new_collections = true; + member.edit_any_collection = true; + member.delete_any_collection = true; + member.status = MembershipStatus::Accepted as i32; + + assert!(!member.can_create_new_collections()); + assert!(!member.can_delete_any_collection()); + assert!(!member.has_full_access()); + } + + #[test] + fn clearing_custom_permissions_clears_every_flag() { + let mut member = membership(MembershipType::Custom); + member.manage_users = true; + member.manage_groups = true; + member.manage_policies = true; + member.create_new_collections = true; + member.edit_any_collection = true; + member.delete_any_collection = true; + + member.clear_custom_permissions(); + + assert!(!member.manage_users); + assert!(!member.manage_groups); + assert!(!member.manage_policies); + assert!(!member.create_new_collections); + assert!(!member.edit_any_collection); + assert!(!member.delete_any_collection); } } diff --git a/src/db/schema.rs b/src/db/schema.rs index af342186..e8840acd 100644 --- a/src/db/schema.rs +++ b/src/db/schema.rs @@ -242,6 +242,12 @@ table! { atype -> Integer, reset_password_key -> Nullable, external_id -> Nullable, + manage_users -> Bool, + manage_groups -> Bool, + manage_policies -> Bool, + create_new_collections -> Bool, + edit_any_collection -> Bool, + delete_any_collection -> Bool, } } diff --git a/src/static/scripts/admin_users.js b/src/static/scripts/admin_users.js index 99e39aab..1bae0aa3 100644 --- a/src/static/scripts/admin_users.js +++ b/src/static/scripts/admin_users.js @@ -174,10 +174,14 @@ const ORG_TYPES = { "name": "User", "bg": "blue" }, - "4": { + "3": { "name": "Manager", "bg": "green" }, + "4": { + "name": "Custom", + "bg": "teal" + }, }; // Special sort function to sort dates in ISO format diff --git a/src/static/templates/admin/users.hbs b/src/static/templates/admin/users.hbs index 4c91bc0e..3bd63446 100644 --- a/src/static/templates/admin/users.hbs +++ b/src/static/templates/admin/users.hbs @@ -135,6 +135,9 @@
+
+ +
diff --git a/src/static/templates/scss/vaultwarden.scss.hbs b/src/static/templates/scss/vaultwarden.scss.hbs index 5bbe5db2..11bcbad6 100644 --- a/src/static/templates/scss/vaultwarden.scss.hbs +++ b/src/static/templates/scss/vaultwarden.scss.hbs @@ -116,8 +116,11 @@ app-security > app-two-factor-setup > form { } /* Hide unsupported Custom Role options */ -:is(bit-dialog, [bit-dialog]) div.tw-ml-4:has(bit-form-control input), -:is(bit-dialog, [bit-dialog]) div.tw-col-span-4:has(input[formcontrolname*="access"], input[formcontrolname*="manage"]) { +/* 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"]), +: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; }