From 1f6d45753389d8dad991771499bc935a56cf0394 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Wed, 1 Jul 2026 19:30:54 +0200 Subject: [PATCH 01/22] Add custom role permissions: Manage Users, Groups, Policies --- .gitignore | 3 + .../down.sql | 3 + .../up.sql | 3 + .../down.sql | 3 + .../up.sql | 3 + .../down.sql | 3 + .../up.sql | 3 + src/api/core/organizations.rs | 121 +++++++++++------- src/auth.rs | 84 ++++++++++++ src/db/models/organization.rs | 73 +++++------ src/db/schema.rs | 3 + .../templates/scss/vaultwarden.scss.hbs | 6 +- 12 files changed, 218 insertions(+), 90 deletions(-) create mode 100644 migrations/mysql/2026-06-30-120000_add_custom_role_permissions/down.sql create mode 100644 migrations/mysql/2026-06-30-120000_add_custom_role_permissions/up.sql create mode 100644 migrations/postgresql/2026-06-30-120000_add_custom_role_permissions/down.sql create mode 100644 migrations/postgresql/2026-06-30-120000_add_custom_role_permissions/up.sql create mode 100644 migrations/sqlite/2026-06-30-120000_add_custom_role_permissions/down.sql create mode 100644 migrations/sqlite/2026-06-30-120000_add_custom_role_permissions/up.sql diff --git a/.gitignore b/.gitignore index e991430e..06d677ca 100644 --- a/.gitignore +++ b/.gitignore @@ -14,3 +14,6 @@ data # Web vault web-vault + +ssl/ +vcpkg/ 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..f1979ae7 --- /dev/null +++ b/migrations/mysql/2026-06-30-120000_add_custom_role_permissions/down.sql @@ -0,0 +1,3 @@ +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..11094951 --- /dev/null +++ b/migrations/mysql/2026-06-30-120000_add_custom_role_permissions/up.sql @@ -0,0 +1,3 @@ +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; 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..f1979ae7 --- /dev/null +++ b/migrations/postgresql/2026-06-30-120000_add_custom_role_permissions/down.sql @@ -0,0 +1,3 @@ +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..11094951 --- /dev/null +++ b/migrations/postgresql/2026-06-30-120000_add_custom_role_permissions/up.sql @@ -0,0 +1,3 @@ +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; 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..f1979ae7 --- /dev/null +++ b/migrations/sqlite/2026-06-30-120000_add_custom_role_permissions/down.sql @@ -0,0 +1,3 @@ +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..11094951 --- /dev/null +++ b/migrations/sqlite/2026-06-30-120000_add_custom_role_permissions/up.sql @@ -0,0 +1,3 @@ +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; diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index dd68cd5b..c3940b3c 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -11,7 +11,10 @@ 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, Headers, ManageGroupsHeaders, ManagePoliciesHeaders, ManageUsersHeaders, ManagerHeaders, + ManagerHeadersLoose, OrgMemberHeaders, OwnerHeaders, decode_invite, + }, db::{ DbConn, models::{ @@ -389,7 +392,13 @@ 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 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. + let can_read_collection_list = headers.membership.has_full_access() + || headers.membership.manage_users + || headers.membership.manage_groups + || headers.membership.manage_policies; + if !can_read_collection_list { err_code!("Resource not found.", "User does not have full access", rocket::http::Status::NotFound.code); } @@ -1030,7 +1039,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 { @@ -1174,7 +1183,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 { @@ -1209,7 +1218,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 { @@ -1340,7 +1349,7 @@ struct BulkConfirmData { async fn bulk_confirm_invite( org_id: OrganizationId, data: Json, - headers: AdminHeaders, + headers: ManageUsersHeaders, conn: DbConn, nt: Notify<'_>, ) -> JsonResult { @@ -1384,7 +1393,7 @@ async fn confirm_invite( org_id: OrganizationId, member_id: MembershipId, data: Json, - headers: AdminHeaders, + headers: ManageUsersHeaders, conn: DbConn, nt: Notify<'_>, ) -> EmptyResult { @@ -1397,7 +1406,7 @@ async fn confirm_invite_impl( org_id: &OrganizationId, member_id: &MembershipId, key: &str, - headers: &AdminHeaders, + headers: &ManageUsersHeaders, conn: &DbConn, nt: &Notify<'_>, ) -> EmptyResult { @@ -1482,7 +1491,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 { @@ -1513,7 +1522,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 @@ -1524,7 +1533,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 { @@ -1532,23 +1541,30 @@ 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 + // For a Custom role, the "Manage all collections" parent checkbox is not sent to + // the server; we derive access_all from its three child checkboxes. Admins/Owners + // implicitly have access to all collections. let access_all = new_type >= MembershipType::Admin - || (raw_type.eq("4") + || (new_type == MembershipType::Custom && data.permissions.get("editAnyCollection") == Some(&json!(true)) && data.permissions.get("deleteAnyCollection") == Some(&json!(true)) && data.permissions.get("createNewCollections") == Some(&json!(true))); + // Read the explicit Custom-role management permissions. These only apply to the + // Custom type; for every other type they are forced to false so that changing a + // member away from Custom clears any previously granted flags. + let perm = |key: &str| { + new_type == MembershipType::Custom && data.permissions.get(key) == Some(&json!(true)) + }; + let manage_users = perm("manageUsers"); + let manage_groups = perm("manageGroups"); + let manage_policies = perm("managePolicies"); + 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") }; @@ -1574,7 +1590,18 @@ async fn edit_member( } } + // Security: only Admins and Owners may grant the granular custom-role management + // permissions. A custom user with manage_users must not be able to grant these + // permissions (to themselves or others), which would be a privilege escalation. + if headers.membership_type < MembershipType::Admin + && (manage_users || manage_groups || manage_policies) + { + err!("Only Admins or Owners can grant custom management permissions") + } member_to_edit.access_all = access_all; + member_to_edit.manage_users = manage_users; + member_to_edit.manage_groups = manage_groups; + member_to_edit.manage_policies = manage_policies; 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 @@ -1631,7 +1658,7 @@ async fn edit_member( async fn bulk_delete_member( org_id: OrganizationId, data: Json, - headers: AdminHeaders, + headers: ManageUsersHeaders, conn: DbConn, nt: Notify<'_>, ) -> JsonResult { @@ -1667,7 +1694,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 { @@ -1677,7 +1704,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 { @@ -1722,7 +1749,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 { @@ -1931,8 +1958,8 @@ 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; @@ -1997,7 +2024,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"); } @@ -2025,7 +2052,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 { @@ -2153,7 +2180,7 @@ async fn put_policy_vnext( org_id: OrganizationId, pol_type: i32, data: Json, - headers: AdminHeaders, + headers: ManagePoliciesHeaders, conn: DbConn, ) -> JsonResult { let data: PolicyDataVnext = data.into_inner(); @@ -2232,7 +2259,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 @@ -2242,7 +2269,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 { @@ -2281,7 +2308,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 { @@ -2325,7 +2352,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. @@ -2337,7 +2364,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 @@ -2347,7 +2374,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 { @@ -2381,7 +2408,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 { @@ -2528,7 +2555,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 @@ -2537,7 +2564,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 { @@ -2572,7 +2599,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 { @@ -2613,7 +2640,7 @@ async fn add_update_group( collections: Vec, members: Vec, org_id: OrganizationId, - headers: &AdminHeaders, + headers: &ManageGroupsHeaders, conn: &DbConn, ) -> JsonResult { group.save(conn).await?; @@ -2653,7 +2680,7 @@ async fn add_update_group( async fn get_group_details( org_id: OrganizationId, group_id: GroupId, - headers: AdminHeaders, + headers: ManageGroupsHeaders, conn: DbConn, ) -> JsonResult { if org_id != headers.org_id { @@ -2674,21 +2701,21 @@ 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 { @@ -2720,7 +2747,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 { @@ -2739,7 +2766,7 @@ 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"); } @@ -2758,7 +2785,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 { @@ -2785,7 +2812,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 { @@ -2833,7 +2860,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 { diff --git a/src/auth.rs b/src/auth.rs index 2ad95036..66e9bbe7 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -693,6 +693,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, @@ -714,6 +715,24 @@ 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. + fn can_manage_users(&self) -> bool { + self.is_confirmed() + && (self.membership_type >= MembershipType::Admin || self.membership.manage_users) + } + fn can_manage_groups(&self) -> bool { + self.is_confirmed() + && (self.membership_type >= MembershipType::Admin || self.membership.manage_groups) + } + fn can_manage_policies(&self) -> bool { + self.is_confirmed() + && (self.membership_type >= MembershipType::Admin || self.membership.manage_policies) + } } // org_id is usually the second path param ("/organizations/"), @@ -827,6 +846,71 @@ 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" +); + // 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. diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index d604add4..0fbc45a1 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -57,6 +57,9 @@ 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, } #[derive(Identifiable, Queryable, Insertable, AsChangeset)] @@ -98,21 +101,17 @@ 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, } } @@ -121,11 +120,15 @@ impl MembershipType { 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] = [ + // Custom is treated as a low-privilege base role (same level as Manager for + // ordering purposes); its elevated capabilities are governed by the explicit + // manage_* permission flags on the Membership, not by this ordering. + const ACCESS_LEVEL: [i32; 5] = [ 3, // Owner 2, // Admin 0, // User - 1, // Manager && Custom + 1, // Manager + 1, // Custom ]; ACCESS_LEVEL[*self as usize].cmp(&ACCESS_LEVEL[*other as usize]) } @@ -267,6 +270,9 @@ impl Membership { atype: MembershipType::User as i32, reset_password_key: None, external_id: None, + manage_users: false, + manage_groups: false, + manage_policies: false, } } @@ -306,15 +312,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,14 +440,10 @@ 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 + // The 3 Collection roles below are linked to the access_all permission "accessEventLogs": false, "accessImportExport": false, "accessReports": false, @@ -458,10 +451,10 @@ impl Membership { "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, + "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) }); @@ -607,28 +600,24 @@ impl Membership { Vec::with_capacity(0) }; - // 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. A custom member + // may have access_all (the 3 collection roles) and/or any of the explicit + // manage_* flags; otherwise Bitwarden assumes all-false defaults. + 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.access_all, + "editAnyCollection": self.access_all, + "deleteAnyCollection": self.access_all, + "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 +717,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, diff --git a/src/db/schema.rs b/src/db/schema.rs index af342186..15c43b5f 100644 --- a/src/db/schema.rs +++ b/src/db/schema.rs @@ -242,6 +242,9 @@ table! { atype -> Integer, reset_password_key -> Nullable, external_id -> Nullable, + manage_users -> Bool, + manage_groups -> Bool, + manage_policies -> Bool, } } diff --git a/src/static/templates/scss/vaultwarden.scss.hbs b/src/static/templates/scss/vaultwarden.scss.hbs index 477cdd34..c5f790ac 100644 --- a/src/static/templates/scss/vaultwarden.scss.hbs +++ b/src/static/templates/scss/vaultwarden.scss.hbs @@ -116,8 +116,12 @@ app-security > app-two-factor-setup > form { } /* Hide unsupported Custom Role options */ +/* Note: manageUsers and managePolicies are supported by Vaultwarden + and are intentionally NOT hidden here. */ bit-dialog div.tw-ml-4:has(bit-form-control input), -bit-dialog div.tw-col-span-4:has(input[formcontrolname*="access"], input[formcontrolname*="manage"]) { +bit-dialog div.tw-col-span-4:has(input[formcontrolname*="access"]), +bit-dialog bit-form-control:has(input[formcontrolname="manageSso"]), +bit-dialog bit-form-control:has(input[formcontrolname="manageResetPassword"]) { @extend %vw-hide; } From df18711b49984a99de872ec62a9fce9568fccf27 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Wed, 1 Jul 2026 19:39:10 +0200 Subject: [PATCH 02/22] Silence struct_excessive_bools lint for Membership --- src/db/models/organization.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index 0fbc45a1..c2ba53b6 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -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, From 4d6b667ffcf72d8b3e90df8bcc6f35859c3e8d5e Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Thu, 2 Jul 2026 17:12:23 +0200 Subject: [PATCH 03/22] Server-side collection/group permission hardening; works without web-vault changes --- src/api/core/organizations.rs | 175 ++++++++++++++++++++++++++-------- 1 file changed, 136 insertions(+), 39 deletions(-) diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index c3940b3c..f4c22203 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -430,6 +430,12 @@ 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 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. + let can_read_collection_list = + member.manage_users || member.manage_groups || member.manage_policies; + // 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) @@ -453,8 +459,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 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 { + 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; } @@ -514,7 +532,10 @@ async fn post_organization_collections( let data: FullCollectionData = data.into_inner(); data.validate(&org_id, &conn).await?; - if headers.membership.atype == MembershipType::Manager && !headers.membership.access_all { + // Managers and custom users may only create collections if they have full access. + // (A custom user with manage_users/manage_groups/manage_policies but no collection + // access must not be able to create collections.) + if !headers.membership.has_full_access() && headers.membership.atype < MembershipType::Admin { err!("You don't have permission to create collections") } @@ -585,6 +606,13 @@ async fn post_bulk_access_collections( err!("Can't find organization details") } + // Security: only callers who can actually manage collections (Admins/Owners, or users with + // full access) may change collection access in bulk. A custom user with only manage_users / + // manage_groups / manage_policies must not be able to modify collection assignments here. + if !headers.membership.has_full_access() { + err!("You don't have permission to modify collection access") + } + 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") @@ -1150,8 +1178,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"), @@ -1170,9 +1207,20 @@ 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.manage_groups, + None => false, + }; + + if caller_can_manage_groups { + for group_id in &data.groups { + let mut group_entry = GroupUser::new(group_id.clone(), new_member.uuid.clone()); + group_entry.save(&conn).await?; + } } } @@ -1598,6 +1646,7 @@ async fn edit_member( { err!("Only Admins or Owners can grant custom management permissions") } + member_to_edit.access_all = access_all; member_to_edit.manage_users = manage_users; member_to_edit.manage_groups = manage_groups; @@ -1608,36 +1657,59 @@ async fn edit_member( // 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?; + // 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, + }; + + 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?; + } + + // 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?; + } } } } } - 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.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 { + 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?; + } } log_event( @@ -2591,7 +2663,17 @@ async fn post_groups( ) .await; - add_update_group(group, group_request.collections, group_request.users, org_id, &headers, &conn).await + // 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 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).await } #[put("/organizations//groups/", data = "")] @@ -2618,7 +2700,19 @@ async fn put_group( let updated_group = group_request.update_group(group); - CollectionGroup::delete_all_by_group(&group_id, &org_id, &conn).await?; + // 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, + }; + + if caller_can_manage_collections { + CollectionGroup::delete_all_by_group(&group_id, &org_id, &conn).await?; + } GroupUser::delete_all_by_group(&group_id, &org_id, &conn).await?; log_event( @@ -2632,7 +2726,10 @@ 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).await } async fn add_update_group( @@ -2680,10 +2777,10 @@ async fn add_update_group( async fn get_group_details( org_id: OrganizationId, group_id: GroupId, - headers: ManageGroupsHeaders, + headers: ManagerHeadersLoose, conn: DbConn, ) -> JsonResult { - if org_id != headers.org_id { + if org_id != headers.membership.org_uuid { err!("Organization not found", "Organization id's do not match"); } if !CONFIG.org_groups_enabled() { @@ -2766,8 +2863,8 @@ async fn bulk_delete_groups( } #[get("/organizations//groups/", rank = 2)] -async fn get_group(org_id: OrganizationId, group_id: GroupId, headers: ManageGroupsHeaders, conn: DbConn) -> JsonResult { - if org_id != headers.org_id { +async fn get_group(org_id: OrganizationId, group_id: GroupId, headers: ManagerHeadersLoose, conn: DbConn) -> JsonResult { + if org_id != headers.membership.org_uuid { err!("Organization not found", "Organization id's do not match"); } if !CONFIG.org_groups_enabled() { @@ -2785,10 +2882,10 @@ async fn get_group(org_id: OrganizationId, group_id: GroupId, headers: ManageGro async fn get_group_members( org_id: OrganizationId, group_id: GroupId, - headers: ManageGroupsHeaders, + headers: ManagerHeadersLoose, conn: DbConn, ) -> JsonResult { - if org_id != headers.org_id { + if org_id != headers.membership.org_uuid { err!("Organization not found", "Organization id's do not match"); } if !CONFIG.org_groups_enabled() { From e72c97c30d8ffca4b6f04a63823393fc4a96f0e3 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Tue, 7 Jul 2026 11:59:44 +0200 Subject: [PATCH 04/22] Harden custom-role permissions: block indirect collection access via groups; block manage_users revoke/restore of admins; cargo fmt --- src/api/core/organizations.rs | 164 +++++++++++++++++++++++++--------- src/auth.rs | 9 +- 2 files changed, 126 insertions(+), 47 deletions(-) diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index f4c22203..ba8ab681 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -433,8 +433,7 @@ async fn get_org_collections_details(org_id: OrganizationId, headers: ManagerHea // Custom users with a 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. - let can_read_collection_list = - member.manage_users || member.manage_groups || member.manage_policies; + let can_read_collection_list = member.manage_users || member.manage_groups || member.manage_policies; // 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. @@ -1606,9 +1605,7 @@ async fn edit_member( // Read the explicit Custom-role management permissions. These only apply to the // Custom type; for every other type they are forced to false so that changing a // member away from Custom clears any previously granted flags. - let perm = |key: &str| { - new_type == MembershipType::Custom && data.permissions.get(key) == Some(&json!(true)) - }; + let perm = |key: &str| new_type == MembershipType::Custom && data.permissions.get(key) == Some(&json!(true)); let manage_users = perm("manageUsers"); let manage_groups = perm("manageGroups"); let manage_policies = perm("managePolicies"); @@ -1641,9 +1638,7 @@ async fn edit_member( // Security: only Admins and Owners may grant the granular custom-role management // permissions. A custom user with manage_users must not be able to grant these // permissions (to themselves or others), which would be a privilege escalation. - if headers.membership_type < MembershipType::Admin - && (manage_users || manage_groups || manage_policies) - { + if headers.membership_type < MembershipType::Admin && (manage_users || manage_groups || manage_policies) { err!("Only Admins or Owners can grant custom management permissions") } @@ -2391,6 +2386,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") } @@ -2491,6 +2492,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") } @@ -2650,7 +2657,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, @@ -2663,17 +2684,22 @@ async fn post_groups( ) .await; - // 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 collections_to_apply = - if caller_can_manage_collections { group_request.collections } else { Vec::new() }; + 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).await + add_update_group( + group, + collections_to_apply, + group_request.users, + org_id, + &headers, + &conn, + caller_can_manage_collections, + ) + .await } #[put("/organizations//groups/", data = "")] @@ -2698,8 +2724,6 @@ 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 @@ -2710,10 +2734,18 @@ async fn put_group( None => false, }; + // 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?; } - GroupUser::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, @@ -2727,9 +2759,21 @@ async fn put_group( .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).await + 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 } async fn add_update_group( @@ -2739,6 +2783,7 @@ async fn add_update_group( org_id: OrganizationId, headers: &ManageGroupsHeaders, conn: &DbConn, + caller_can_manage_collections: bool, ) -> JsonResult { group.save(conn).await?; @@ -2747,20 +2792,31 @@ async fn add_update_group( 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!({ @@ -2805,7 +2861,12 @@ async fn post_delete_group( } #[delete("/organizations//groups/")] -async fn delete_group(org_id: OrganizationId, group_id: GroupId, headers: ManageGroupsHeaders, 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 } @@ -2863,7 +2924,12 @@ async fn bulk_delete_groups( } #[get("/organizations//groups/", rank = 2)] -async fn get_group(org_id: OrganizationId, group_id: GroupId, headers: ManagerHeadersLoose, conn: DbConn) -> JsonResult { +async fn get_group( + org_id: OrganizationId, + group_id: GroupId, + headers: ManagerHeadersLoose, + conn: DbConn, +) -> JsonResult { if org_id != headers.membership.org_uuid { err!("Organization not found", "Organization id's do not match"); } @@ -2920,8 +2986,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(); diff --git a/src/auth.rs b/src/auth.rs index 66e9bbe7..75fe78da 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -722,16 +722,13 @@ impl OrgHeaders { // permission; a Custom member holds a permission only if the matching flag // is set on their Membership. fn can_manage_users(&self) -> bool { - self.is_confirmed() - && (self.membership_type >= MembershipType::Admin || self.membership.manage_users) + self.is_confirmed() && (self.membership_type >= MembershipType::Admin || self.membership.manage_users) } fn can_manage_groups(&self) -> bool { - self.is_confirmed() - && (self.membership_type >= MembershipType::Admin || self.membership.manage_groups) + self.is_confirmed() && (self.membership_type >= MembershipType::Admin || self.membership.manage_groups) } fn can_manage_policies(&self) -> bool { - self.is_confirmed() - && (self.membership_type >= MembershipType::Admin || self.membership.manage_policies) + self.is_confirmed() && (self.membership_type >= MembershipType::Admin || self.membership.manage_policies) } } From 9f45aa77e7998411dc45f831226a22d42ac859a3 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Tue, 7 Jul 2026 14:12:36 +0200 Subject: [PATCH 05/22] Fix privilege escalation: gate access_all in edit_member A Custom member with only manage_users could set the "manage all collections" child permissions (createNewCollections / editAnyCollection / deleteAnyCollection) on any member, including themselves, via POST /organizations//users/. edit_member wrote member_to_edit.access_all unconditionally, so the resulting access_all=true granted full access to every collection's contents - defeating the "manage users without collection access" guarantee. Gate the access_all change on caller_can_manage_collections (Admins/Owners or full-access members), mirroring how put_group preserves a group's access_all for callers without collection rights. For everyone else the member's existing access_all is left untouched. The collection- and group-assignment paths were already gated; this closes the remaining direct path. --- src/api/core/organizations.rs | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index ba8ab681..e8403fc1 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -1642,16 +1642,6 @@ async fn edit_member( err!("Only Admins or Owners can grant custom management permissions") } - member_to_edit.access_all = access_all; - member_to_edit.manage_users = manage_users; - member_to_edit.manage_groups = manage_groups; - member_to_edit.manage_policies = manage_policies; - 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?; - // 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 @@ -1662,6 +1652,23 @@ async fn edit_member( 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 set the Custom "manage all collections" child boxes 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; + } + member_to_edit.manage_users = manage_users; + member_to_edit.manage_groups = manage_groups; + member_to_edit.manage_policies = manage_policies; + 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?; + 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 { From 43fb19f190e75b00ab45cba31effe95bee50252f Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Wed, 8 Jul 2026 08:11:27 +0200 Subject: [PATCH 06/22] Harden custom-role permission checks Security fixes and hardening following a review of the custom-role permissions feature: - Gate the manage_* flags on the Custom membership type via new Membership::has_manage_* helpers. Previously a stale flag (e.g. left over after changing a member's type through the admin panel, which does not go through edit_member) would keep granting management permissions to a member of any type. - Clear the manage_* flags in admin::update_membership_type when a member is changed away from the Custom type. - edit_member: reject any *change* to the manage_* flags by non-admin callers. This keeps the previous protection against granting flags, and additionally prevents a manage_users member from silently stripping flags an Admin/Owner granted to fellow Custom members. Unchanged flags still pass, so the regular edit dialog keeps working. - list_policies: restrict the full policy list to Admins/Owners and Custom members holding at least one management permission. The previous loosening to ManagerHeadersLoose also exposed all policies (including their configuration data) to plain Managers. - Collection list metadata (get_org_collections{,_details}) is now only readable with manage_users or manage_groups; manage_policies does not need the collection list. - find_confirmed_and_manage_all_by_org: include Custom members with access_all, matching the pre-existing behaviour for Managers. - Fix the partial_cmp_MembershipType unit test (Custom is no longer converted to Manager) and document that Manager and Custom share an access level in Ord while remaining distinct roles under PartialEq. - Replace the remaining stale "HACK" comments and the raw "4" type comparison in send_invite; drop unrelated .gitignore entries. --- .gitignore | 3 -- src/api/admin.rs | 7 ++++ src/api/core/organizations.rs | 70 +++++++++++++++++++++-------------- src/auth.rs | 10 +++-- src/db/models/organization.rs | 32 ++++++++++++++-- 5 files changed, 85 insertions(+), 37 deletions(-) diff --git a/.gitignore b/.gitignore index 06d677ca..e991430e 100644 --- a/.gitignore +++ b/.gitignore @@ -14,6 +14,3 @@ data # Web vault web-vault - -ssl/ -vcpkg/ diff --git a/src/api/admin.rs b/src/api/admin.rs index 7037bfb1..c16fb866 100644 --- a/src/api/admin.rs +++ b/src/api/admin.rs @@ -567,6 +567,13 @@ async fn update_membership_type(data: Json, token: AdminToke } member_to_edit.atype = new_type; + // The manage_* permission flags only apply to the Custom role; clear them on any other + // type so a member changed away from Custom does not retain stale management permissions. + if new_type != MembershipType::Custom { + member_to_edit.manage_users = false; + member_to_edit.manage_groups = false; + member_to_edit.manage_policies = false; + } // 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?; diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index e8403fc1..8d63d75d 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -392,12 +392,12 @@ async fn get_org_collections(org_id: OrganizationId, headers: ManagerHeadersLoos err!("Organization not found", "Organization id's do not match"); } - // Custom users with a 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. + // 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.manage_users - || headers.membership.manage_groups - || headers.membership.manage_policies; + || headers.membership.has_manage_users() + || headers.membership.has_manage_groups(); if !can_read_collection_list { err_code!("Resource not found.", "User does not have full access", rocket::http::Status::NotFound.code); } @@ -430,10 +430,11 @@ 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 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. - let can_read_collection_list = member.manage_users || member.manage_groups || member.manage_policies; + // 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(); // 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. @@ -1075,13 +1076,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") }; @@ -1089,11 +1085,11 @@ 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 + // For a Custom role, the "Manage all collections" parent checkbox is not sent to + // the server; we derive access_all from its three child checkboxes. Admins/Owners + // implicitly have access to all collections. let access_all = new_type >= MembershipType::Admin - || (raw_type.eq("4") + || (new_type == MembershipType::Custom && data.permissions.get("editAnyCollection") == Some(&json!(true)) && data.permissions.get("deleteAnyCollection") == Some(&json!(true)) && data.permissions.get("createNewCollections") == Some(&json!(true))); @@ -1135,7 +1131,7 @@ 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; new_member.status = member_status; new_member.save(&conn).await?; @@ -1211,7 +1207,7 @@ async fn send_invite( // 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.manage_groups, + Some(m) => m.has_manage_groups(), None => false, }; @@ -1635,11 +1631,17 @@ async fn edit_member( } } - // Security: only Admins and Owners may grant the granular custom-role management - // permissions. A custom user with manage_users must not be able to grant these - // permissions (to themselves or others), which would be a privilege escalation. - if headers.membership_type < MembershipType::Admin && (manage_users || manage_groups || manage_policies) { - err!("Only Admins or Owners can grant custom management permissions") + // Security: only Admins and Owners may change the granular custom-role management + // 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 + && (manage_users != member_to_edit.manage_users + || manage_groups != member_to_edit.manage_groups + || manage_policies != member_to_edit.manage_policies) + { + err!("Only Admins or Owners can change custom management permissions") } // Security: only callers who can actually manage collections (Admins/Owners, or users @@ -1701,7 +1703,7 @@ async fn edit_member( // 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.manage_groups, + Some(m) => m.has_manage_groups(), None => false, }; @@ -2036,6 +2038,20 @@ async fn list_policies(org_id: OrganizationId, headers: ManagerHeadersLoose, con if org_id != headers.membership.org_uuid { err!("Organization not found", "Organization id's do not match"); } + + // Security: only Admins/Owners, or Custom members holding at least one management + // permission, may read the full policy list (the Admin Console needs it to load). + // Plain Managers and Custom members without any permission keep the pre-existing + // behaviour of having no access here. + let membership = &headers.membership; + if !(membership.atype >= MembershipType::Admin + || membership.has_manage_users() + || membership.has_manage_groups() + || membership.has_manage_policies()) + { + err!("You don't have permission to view policies") + } + let policies = OrgPolicy::find_by_org(&org_id, &conn).await; let policies_json: Vec = policies.iter().map(OrgPolicy::to_json).collect(); diff --git a/src/auth.rs b/src/auth.rs index 75fe78da..d3c74899 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -720,15 +720,17 @@ impl OrgHeaders { } // 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. + // is set on their Membership. The has_manage_* 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.manage_users) + 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.manage_groups) + 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.manage_policies) + self.is_confirmed() + && (self.membership_type >= MembershipType::Admin || self.membership.has_manage_policies()) } } diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index c2ba53b6..92ec4973 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -124,6 +124,11 @@ impl Ord for MembershipType { // Custom is treated as a low-privilege base role (same level as Manager for // ordering purposes); its elevated capabilities are governed by the explicit // manage_* permission flags on the Membership, not by this ordering. + // + // NOTE: Manager and Custom therefore share an access level while being distinct + // variants: the derived `PartialEq` compares the role itself (Manager != Custom), + // while this ordering compares access levels (neither is greater than the other). + // Keep that in mind before relying on `cmp() == Equal` implying equality. const ACCESS_LEVEL: [i32; 5] = [ 3, // Owner 2, // Admin @@ -809,6 +814,21 @@ impl Membership { (self.access_all || self.atype >= MembershipType::Admin) && self.has_status(MembershipStatus::Confirmed) } + // The granular manage_* 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 async fn find_by_uuid(uuid: &MembershipId, conn: &DbConn) -> Option { conn.run(move |conn| { users_organizations::table.filter(users_organizations::uuid.eq(uuid)).first::(conn).ok() @@ -915,7 +935,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 @@ -925,7 +945,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) @@ -1260,6 +1280,12 @@ mod tests { assert!(MembershipType::Owner > MembershipType::Admin); assert!(MembershipType::Admin > MembershipType::Manager); assert!(MembershipType::Manager > MembershipType::User); - assert!(MembershipType::Manager == MembershipType::from_str("4").unwrap()); + assert!(MembershipType::Custom == MembershipType::from_str("4").unwrap()); + // Manager and Custom share the same access level, but are distinct roles + assert!(MembershipType::Manager != MembershipType::Custom); + assert!(MembershipType::Manager >= MembershipType::Custom); + assert!(MembershipType::Custom >= MembershipType::Manager); + assert!(MembershipType::Custom > MembershipType::User); + assert!(MembershipType::Admin > MembershipType::Custom); } } From 02b6c2c205f1d673b9c7d68cb3f3413050779888 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Wed, 8 Jul 2026 13:24:51 +0200 Subject: [PATCH 07/22] Restrict group reads to manage_groups and hide policy contents without manage_policies Tighten two custom-role read paths that were broader than intended: - get_group, get_group_details and get_group_members only required ManagerHeadersLoose, exposing group metadata, collection mappings and membership to any confirmed Manager/Custom member. Require ManageGroupsHeaders (Admin/Owner or manage_groups) instead. - list_policies returned the full policy configuration to any manage_* member. Keep the endpoint reachable so the Admin Console still loads, but return an empty list to callers without manage_policies. --- src/api/core/organizations.rs | 39 ++++++++++++++++------------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index 8d63d75d..24e82f77 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -2039,21 +2039,18 @@ async fn list_policies(org_id: OrganizationId, headers: ManagerHeadersLoose, con err!("Organization not found", "Organization id's do not match"); } - // Security: only Admins/Owners, or Custom members holding at least one management - // permission, may read the full policy list (the Admin Console needs it to load). - // Plain Managers and Custom members without any permission keep the pre-existing - // behaviour of having no access here. - let membership = &headers.membership; - if !(membership.atype >= MembershipType::Admin - || membership.has_manage_users() - || membership.has_manage_groups() - || membership.has_manage_policies()) - { - err!("You don't have permission to view policies") - } - - 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, @@ -2856,10 +2853,10 @@ async fn add_update_group( async fn get_group_details( org_id: OrganizationId, group_id: GroupId, - headers: ManagerHeadersLoose, + headers: ManageGroupsHeaders, 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"); } if !CONFIG.org_groups_enabled() { @@ -2950,10 +2947,10 @@ async fn bulk_delete_groups( async fn get_group( org_id: OrganizationId, group_id: GroupId, - headers: ManagerHeadersLoose, + headers: ManageGroupsHeaders, 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"); } if !CONFIG.org_groups_enabled() { @@ -2971,10 +2968,10 @@ async fn get_group( async fn get_group_members( org_id: OrganizationId, group_id: GroupId, - headers: ManagerHeadersLoose, + headers: ManageGroupsHeaders, 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"); } if !CONFIG.org_groups_enabled() { From 1dcd3ea26f32d73cab9ff181e55acb2c31037591 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Wed, 8 Jul 2026 15:51:57 +0200 Subject: [PATCH 08/22] Block collection access via group assignment in edit_member and send_invite A Custom member holding manage_users + manage_groups (but without collection access) could add a member -- including themselves via edit_member, or an attacker-controlled invitee via send_invite -- to an access_all or collection-bearing group, and thereby indirectly gain read access to those collections' cipher contents. This bypassed the collection-access boundary already enforced in put_group_members and add_update_group. Gate both member-centric group-assignment paths on collection-management rights via a shared `may_change_group_membership` predicate: callers who cannot manage collections may only add/remove membership of groups that confer no collection access, and collection-bearing memberships are left untouched (neither granted nor revoked). Admins/Owners and full-access callers keep the previous full-replace behaviour. Add a regression unit test for the predicate. --- src/api/core/organizations.rs | 89 +++++++++++++++++++++++++++++++++-- 1 file changed, 85 insertions(+), 4 deletions(-) diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index 8aae8b97..e0776826 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -1218,6 +1218,14 @@ async fn send_invite( 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?; } @@ -1713,11 +1721,42 @@ async fn edit_member( }; if caller_can_manage_groups { - GroupUser::delete_all_by_member(&member_to_edit.uuid, &conn).await?; + 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?; + 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?; + } } } @@ -2809,6 +2848,25 @@ async fn put_group( .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 +} + +/// 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, + } +} + async fn add_update_group( mut group: Group, collections: Vec, @@ -3439,3 +3497,26 @@ async fn rotate_api_key( ) -> JsonResult { api_key(&org_id, data, true, headers, conn).await } + +#[cfg(test)] +mod tests { + use super::may_change_group_membership; + + #[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)); + } +} From 81b76d9782aa84cfb8e5a9dbf5b7342456b8139f Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Wed, 8 Jul 2026 18:29:52 +0200 Subject: [PATCH 09/22] Persist manage_* permission flags on invite send_invite computed access_all from the invite permissions but never wrote the manage_users / manage_groups / manage_policies flags onto the new membership, so a Custom member invited with a management permission checked was created without it (the web-vault sends everything in a single invite POST, with no follow-up edit). Mirror the handling in edit_member and persist the flags at invite time. Only Owners can invite Custom members, so the caller is always authorized to grant these; the flags are gated on the Custom type and forced false for every other type. --- src/api/core/organizations.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index e0776826..9528a756 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -1095,6 +1095,14 @@ async fn send_invite( && data.permissions.get("deleteAnyCollection") == Some(&json!(true)) && data.permissions.get("createNewCollections") == Some(&json!(true))); + // Read the explicit Custom-role management permissions. These only apply to the + // Custom type; for every other type they are forced to false. Only Owners can invite + // Custom members (checked above), so the caller is always authorized to grant these. + let perm = |key: &str| new_type == MembershipType::Custom && data.permissions.get(key) == Some(&json!(true)); + let manage_users = perm("manageUsers"); + let manage_groups = perm("manageGroups"); + let manage_policies = perm("managePolicies"); + let mut user_created: bool = false; for email in &data.emails { let mut member_status = MembershipStatus::Invited as i32; @@ -1137,6 +1145,9 @@ 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 as i32; + new_member.manage_users = manage_users; + new_member.manage_groups = manage_groups; + new_member.manage_policies = manage_policies; new_member.status = member_status; new_member.save(&conn).await?; From 8a65c6631a9a93842e73551daf6ba96c608c3b57 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Wed, 8 Jul 2026 19:39:49 +0200 Subject: [PATCH 10/22] Security: gate group delete/member-removal on collection access A Custom member with only the manage_groups permission could revoke other members' collection access via two endpoints that were missing the collection-access check enforced elsewhere (put_group_members, edit_member): - POST /organizations//groups//delete-user/ (post_delete_group_member) removed a member from any group, including collection-bearing ones. - DELETE /organizations//groups/ and its bulk variant (delete_group_impl / bulk_delete_groups) deleted collection-bearing groups outright, revoking access for all their members. Neither path can grant access, so confidentiality was never at risk, but both let a manage_groups-only user tamper with other members' collection access, contradicting the permission's invariant. Both now require Admin/Owner or full collection access before touching a group that confers collection access (via access_all or assigned collections). --- src/api/core/organizations.rs | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index 9528a756..73465b80 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -2989,6 +2989,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, @@ -3159,6 +3176,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, From b571efcaddeeb7b33a84c1baae243041b621e7e4 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Thu, 9 Jul 2026 16:20:16 +0200 Subject: [PATCH 11/22] Migrate legacy Manager members to the Custom type Before this PR every member created with the Custom role was stored as Manager (3) and masqueraded as Custom (4) in API responses. With the masquerade removed, those members would suddenly surface as type 3, which current clients no longer support and the admin panel cannot render. Convert them to the now-persisted Custom type; access_all is preserved and the new manage_* flags stay false, matching the exact capabilities and appearance these members had before. The down migration converts Custom members back to Manager so older server versions (which cannot load type 4) keep working after a rollback. --- .../2026-06-30-120000_add_custom_role_permissions/down.sql | 3 +++ .../2026-06-30-120000_add_custom_role_permissions/up.sql | 6 ++++++ .../2026-06-30-120000_add_custom_role_permissions/down.sql | 3 +++ .../2026-06-30-120000_add_custom_role_permissions/up.sql | 6 ++++++ .../2026-06-30-120000_add_custom_role_permissions/down.sql | 3 +++ .../2026-06-30-120000_add_custom_role_permissions/up.sql | 6 ++++++ 6 files changed, 27 insertions(+) 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 index f1979ae7..9ac54bfb 100644 --- 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 @@ -1,3 +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 index 11094951..6ffdca13 100644 --- 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 @@ -1,3 +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-06-30-120000_add_custom_role_permissions/down.sql b/migrations/postgresql/2026-06-30-120000_add_custom_role_permissions/down.sql index f1979ae7..9ac54bfb 100644 --- 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 @@ -1,3 +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 index 11094951..6ffdca13 100644 --- 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 @@ -1,3 +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-06-30-120000_add_custom_role_permissions/down.sql b/migrations/sqlite/2026-06-30-120000_add_custom_role_permissions/down.sql index f1979ae7..9ac54bfb 100644 --- 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 @@ -1,3 +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 index 11094951..6ffdca13 100644 --- 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 @@ -1,3 +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; From 011e5c005bcbb62bbe0ad3aa593d9ac0935f81f3 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Thu, 9 Jul 2026 16:20:36 +0200 Subject: [PATCH 12/22] Add the Custom role to the admin panel The admin panel only knew types 0/1/2 plus the masqueraded "4": Manager mapping. A Custom member would have been shown as "Manager" with the Manager radio (value 3) preselected in the role dialog, so saving it silently converted the member to Manager and wiped their manage_* flags. Any remaining legacy Manager member (type 3) would have thrown a TypeError and broken the dialog entirely. Add a proper Custom (4) entry and radio button, and keep a Manager (3) entry for members created through older clients. --- src/static/scripts/admin_users.js | 6 +++++- src/static/templates/admin/users.hbs | 3 +++ 2 files changed, 8 insertions(+), 1 deletion(-) 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 @@
+
+ +
From 84b8de124e640540fdbdb17d3445f55471a4a6ba Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Thu, 9 Jul 2026 16:20:37 +0200 Subject: [PATCH 13/22] Keep per-collection manage working for Custom members The manage flag in collection JSON was gated on atype == Manager, a raw comparison that now excludes Custom (4). A Custom member holding an explicit per-collection manage assignment (or full read/write access) would have lost the manage capability in /sync and the collection details endpoints - before this PR they were stored as Manager and matched. Compare by access level (>= Manager, which Manager and Custom share) instead, restoring the exact pre-PR behavior for migrated members. Admins/Owners are unaffected: they are caught by the earlier has_full_access / >= Admin arms. --- src/db/models/collection.rs | 11 ++++++----- src/db/models/organization.rs | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) 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/organization.rs b/src/db/models/organization.rs index 92ec4973..11789ec9 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -584,7 +584,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 From 4b90b47dec79841b3f4debe671133552140e91ef Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Sat, 11 Jul 2026 00:31:18 +0200 Subject: [PATCH 14/22] Fix privilege escalation: restrict role-type changes to Admins/Owners in edit_member A Custom member with the manage_users permission reaches edit_member via ManageUsersHeaders. Every mutated field there (access_all, the manage_* flags, collection and group assignments) is gated behind an Admin/collection-management check -- except the role type itself, which was written unconditionally aside from the existing Admin/Owner-elevation guard. Because collection "manage" rights are role-derived (`atype >= Manager` grants manage on any collection the member can write), a manage_users caller with no collection access could promote a plain User to Manager/Custom to grant them collection administration (rename/delete/re-share), or demote to revoke it -- a separation-of-duties break between the user-management and data planes. Add a may_change_member_type() guard so callers below Admin may only submit an unchanged role (keeping the regular edit dialog working), and cover it with a regression unit test. --- src/api/core/organizations.rs | 55 ++++++++++++++++++++++++++++++++++- 1 file changed, 54 insertions(+), 1 deletion(-) diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index 0b0df1ac..b2a2a6a0 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -1641,6 +1641,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 manage_* + // 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") } @@ -2869,6 +2881,20 @@ fn may_change_group_membership(caller_can_manage_collections: bool, group_confer 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 { @@ -3542,7 +3568,34 @@ async fn rotate_api_key( #[cfg(test)] mod tests { - use super::may_change_group_membership; + use super::{may_change_group_membership, may_change_member_type}; + use crate::db::models::MembershipType; + + #[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() { From 44139eb0c79ea429fc5e6e021686bc442b7a7fbf Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Thu, 16 Jul 2026 18:18:49 +0200 Subject: [PATCH 15/22] Add granular custom collection permissions Persist create, edit-any, and delete-any collection grants independently for Custom members. Enforce dedicated create/delete guards, preserve edit-any cipher access, cover imports and metadata reads, and add fail-closed role transitions, migrations, and regression tests. --- .../down.sql | 9 + .../up.sql | 11 + .../down.sql | 9 + .../up.sql | 11 + .../down.sql | 9 + .../up.sql | 11 + src/api/admin.rs | 86 +++++- src/api/core/organizations.rs | 274 ++++++++++++------ src/auth.rs | 216 +++++++++++++- src/db/models/organization.rs | 201 +++++++++++-- src/db/schema.rs | 3 + .../templates/scss/vaultwarden.scss.hbs | 5 +- 12 files changed, 716 insertions(+), 129 deletions(-) create mode 100644 migrations/mysql/2026-07-16-120000_add_custom_collection_permissions/down.sql create mode 100644 migrations/mysql/2026-07-16-120000_add_custom_collection_permissions/up.sql create mode 100644 migrations/postgresql/2026-07-16-120000_add_custom_collection_permissions/down.sql create mode 100644 migrations/postgresql/2026-07-16-120000_add_custom_collection_permissions/up.sql create mode 100644 migrations/sqlite/2026-07-16-120000_add_custom_collection_permissions/down.sql create mode 100644 migrations/sqlite/2026-07-16-120000_add_custom_collection_permissions/up.sql 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..c13662c4 --- /dev/null +++ b/migrations/mysql/2026-07-16-120000_add_custom_collection_permissions/up.sql @@ -0,0 +1,11 @@ +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; 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..c13662c4 --- /dev/null +++ b/migrations/postgresql/2026-07-16-120000_add_custom_collection_permissions/up.sql @@ -0,0 +1,11 @@ +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; 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..c13662c4 --- /dev/null +++ b/migrations/sqlite/2026-07-16-120000_add_custom_collection_permissions/up.sql @@ -0,0 +1,11 @@ +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; diff --git a/src/api/admin.rs b/src/api/admin.rs index c16fb866..0c85d562 100644 --- a/src/api/admin.rs +++ b/src/api/admin.rs @@ -544,6 +544,31 @@ struct MembershipTypeData { org_uuid: OrganizationId, } +fn apply_membership_type_change(membership: &mut Membership, new_type: MembershipType) { + let was_custom = membership.atype == MembershipType::Custom; + + // 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 +578,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,14 +589,7 @@ async fn update_membership_type(data: Json, token: AdminToke } } - member_to_edit.atype = new_type; - // The manage_* permission flags only apply to the Custom role; clear them on any other - // type so a member changed away from Custom does not retain stale management permissions. - if new_type != MembershipType::Custom { - member_to_edit.manage_users = false; - member_to_edit.manage_groups = false; - member_to_edit.manage_policies = false; - } + 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?; @@ -876,6 +892,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() { @@ -900,4 +924,44 @@ 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); + + let mut custom = membership(MembershipType::Custom); + custom.access_all = true; + custom.edit_any_collection = true; + apply_membership_type_change(&mut custom, MembershipType::Manager); + assert_eq!(custom.atype, MembershipType::Manager as i32); + assert!(custom.access_all); + assert!(!custom.edit_any_collection); + } } diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index b2a2a6a0..63d142e8 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -12,8 +12,9 @@ use crate::{ core::{CipherSyncData, CipherSyncType, accept_org_invite, log_event, two_factor}, }, auth::{ - AdminHeaders, Headers, ManageGroupsHeaders, ManagePoliciesHeaders, ManageUsersHeaders, ManagerHeaders, - ManagerHeadersLoose, OrgMemberHeaders, OwnerHeaders, decode_invite, + AdminHeaders, CollectionDeleteHeaders, CollectionReadHeaders, Headers, ManageGroupsHeaders, + ManagePoliciesHeaders, ManageUsersHeaders, ManagerHeaders, ManagerHeadersLoose, OrgMemberHeaders, OwnerHeaders, + decode_invite, }, db::{ DbConn, @@ -398,7 +399,8 @@ async fn get_org_collections(org_id: OrganizationId, headers: ManagerHeadersLoos // 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_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); } @@ -435,7 +437,12 @@ async fn get_org_collections_details(org_id: OrganizationId, headers: ManagerHea // (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(); + 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. @@ -464,7 +471,7 @@ async fn get_org_collections_details(org_id: OrganizationId, headers: ManagerHea // 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 { + 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); @@ -530,16 +537,16 @@ async fn post_organization_collections( if org_id != headers.membership.org_uuid { err!("Organization not found", "Organization id's do not match"); } - let data: FullCollectionData = data.into_inner(); - data.validate(&org_id, &conn).await?; - // Managers and custom users may only create collections if they have full access. - // (A custom user with manage_users/manage_groups/manage_policies but no collection - // access must not be able to create collections.) - if !headers.membership.has_full_access() && headers.membership.atype < MembershipType::Admin { + // 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?; + let collection = Collection::new(org_id.clone(), data.name, data.external_id); collection.save(&conn).await?; @@ -743,7 +750,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 { @@ -769,7 +776,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 @@ -779,7 +786,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 @@ -805,7 +812,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?; @@ -817,23 +824,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 @@ -867,7 +870,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); @@ -884,7 +887,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 { @@ -1035,6 +1038,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 { @@ -1086,22 +1144,10 @@ async fn send_invite( err!("Only Owners can invite Managers, Admins or Owners") } - // For a Custom role, the "Manage all collections" parent checkbox is not sent to - // the server; we derive access_all from its three child checkboxes. Admins/Owners - // implicitly have access to all collections. - let access_all = new_type >= MembershipType::Admin - || (new_type == MembershipType::Custom - && data.permissions.get("editAnyCollection") == Some(&json!(true)) - && data.permissions.get("deleteAnyCollection") == Some(&json!(true)) - && data.permissions.get("createNewCollections") == Some(&json!(true))); - - // Read the explicit Custom-role management permissions. These only apply to the - // Custom type; for every other type they are forced to false. Only Owners can invite - // Custom members (checked above), so the caller is always authorized to grant these. - let perm = |key: &str| new_type == MembershipType::Custom && data.permissions.get(key) == Some(&json!(true)); - let manage_users = perm("manageUsers"); - let manage_groups = perm("manageGroups"); - let manage_policies = perm("managePolicies"); + // 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 { @@ -1145,9 +1191,7 @@ 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 as i32; - new_member.manage_users = manage_users; - new_member.manage_groups = manage_groups; - new_member.manage_policies = manage_policies; + custom_permissions.apply_to(&mut new_member); new_member.status = member_status; new_member.save(&conn).await?; @@ -1613,22 +1657,8 @@ async fn edit_member( err!("Invalid type") }; - // For a Custom role, the "Manage all collections" parent checkbox is not sent to - // the server; we derive access_all from its three child checkboxes. Admins/Owners - // implicitly have access to all collections. - let access_all = new_type >= MembershipType::Admin - || (new_type == MembershipType::Custom - && data.permissions.get("editAnyCollection") == Some(&json!(true)) - && data.permissions.get("deleteAnyCollection") == Some(&json!(true)) - && data.permissions.get("createNewCollections") == Some(&json!(true))); - - // Read the explicit Custom-role management permissions. These only apply to the - // Custom type; for every other type they are forced to false so that changing a - // member away from Custom clears any previously granted flags. - let perm = |key: &str| new_type == MembershipType::Custom && data.permissions.get(key) == Some(&json!(true)); - let manage_users = perm("manageUsers"); - let manage_groups = perm("manageGroups"); - let manage_policies = perm("managePolicies"); + 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") @@ -1645,10 +1675,10 @@ async fn edit_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 manage_* - // 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. + // 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") } @@ -1667,17 +1697,13 @@ async fn edit_member( } } - // Security: only Admins and Owners may change the granular custom-role management - // 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 - && (manage_users != member_to_edit.manage_users - || manage_groups != member_to_edit.manage_groups - || manage_policies != member_to_edit.manage_policies) - { - err!("Only Admins or Owners can change custom management permissions") + // 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 @@ -1692,15 +1718,13 @@ async fn edit_member( // 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 set the Custom "manage all collections" child boxes 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). + // 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; } - member_to_edit.manage_users = manage_users; - member_to_edit.manage_groups = manage_groups; - member_to_edit.manage_policies = manage_policies; + 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 @@ -1992,13 +2016,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 }; @@ -3568,8 +3600,12 @@ async fn rotate_api_key( #[cfg(test)] mod tests { - use super::{may_change_group_membership, may_change_member_type}; - use crate::db::models::MembershipType; + use std::collections::HashMap; + + use serde_json::{Value, json}; + + use super::{CustomRolePermissions, may_change_group_membership, may_change_member_type}; + use crate::db::models::{Membership, MembershipStatus, MembershipType}; #[test] fn manage_users_caller_cannot_change_member_role() { @@ -3614,4 +3650,74 @@ mod tests { // 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 b7e02f89..bdaf2b6e 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -735,7 +735,7 @@ impl OrgHeaders { } // 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_manage_* helpers gate the flags on the + // 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()) @@ -744,8 +744,7 @@ impl OrgHeaders { 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()) + self.is_confirmed() && (self.membership_type >= MembershipType::Admin || self.membership.has_manage_policies()) } } @@ -944,9 +943,28 @@ 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 CollectionDeleteAccess { + Any, + ManagedOnly, + Denied, +} + +fn collection_delete_access(membership: &Membership) -> CollectionDeleteAccess { + if membership.can_delete_any_collection() { + CollectionDeleteAccess::Any + } else if membership.has_status(MembershipStatus::Confirmed) && membership.has_type(MembershipType::Manager) { + // Preserve the legacy Manager role's pre-existing per-collection deletion behavior. + CollectionDeleteAccess::ManagedOnly + } else { + CollectionDeleteAccess::Denied + } +} + +/// 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, @@ -967,7 +985,9 @@ impl<'r> FromRequest<'r> for ManagerHeaders { err_handler!("Error getting DB") }; - if !Collection::is_coll_manageable_by_user(&col_id, &headers.membership.user_uuid, &conn).await { + if !headers.membership.has_edit_any_collection() + && !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") } } else { @@ -987,6 +1007,131 @@ 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 can_read_any_collection = headers.is_confirmed_and_admin() + || headers.membership.has_edit_any_collection() + || headers.membership.has_delete_any_collection(); + + if !can_read_any_collection { + 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") + } + } + + 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 Custom members require the explicit Delete any collection +/// permission. The legacy Manager role retains its previous per-collection behavior. +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) { + CollectionDeleteAccess::Any => {} + CollectionDeleteAccess::Denied => { + // Custom is a distinct, fail-closed role. In particular, Edit any collection and + // access_all must not satisfy a Delete request without the explicit delete flag. + err_handler!("You need the 'Delete any collection' permission to call this endpoint") + } + CollectionDeleteAccess::ManagedOnly => { + 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") + } + } + } + + 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 { @@ -1039,22 +1184,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 == CollectionDeleteAccess::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 == CollectionDeleteAccess::ManagedOnly + && !Collection::is_coll_manageable_by_user(col_id, &h.membership.user_uuid, 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, @@ -1396,3 +1551,42 @@ pub async fn refresh_tokens( Ok((device, auth_tokens)) } + +#[cfg(test)] +mod tests { + use super::{CollectionDeleteAccess, collection_delete_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 collection_delete_permission_is_independent_from_edit_and_access_all() { + let mut custom = membership(MembershipType::Custom); + custom.edit_any_collection = true; + custom.access_all = true; + assert_eq!(collection_delete_access(&custom), CollectionDeleteAccess::Denied); + + custom.delete_any_collection = true; + assert_eq!(collection_delete_access(&custom), CollectionDeleteAccess::Any); + + custom.status = MembershipStatus::Accepted as i32; + assert_eq!(collection_delete_access(&custom), CollectionDeleteAccess::Denied); + } + + #[test] + fn collection_delete_permission_preserves_admin_and_legacy_manager_behavior() { + let admin = membership(MembershipType::Admin); + assert_eq!(collection_delete_access(&admin), CollectionDeleteAccess::Any); + + let manager = membership(MembershipType::Manager); + assert_eq!(collection_delete_access(&manager), CollectionDeleteAccess::ManagedOnly); + + let user = membership(MembershipType::User); + assert_eq!(collection_delete_access(&user), CollectionDeleteAccess::Denied); + } +} diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index 78228588..3fbfc789 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -61,6 +61,9 @@ pub struct Membership { 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)] @@ -123,7 +126,7 @@ impl Ord for MembershipType { // For easy comparison, map each variant to an access level (where 0 is lowest). // Custom is treated as a low-privilege base role (same level as Manager for // ordering purposes); its elevated capabilities are governed by the explicit - // manage_* permission flags on the Membership, not by this ordering. + // custom permission flags on the Membership, not by this ordering. // // NOTE: Manager and Custom therefore share an access level while being distinct // variants: the derived `PartialEq` compares the role itself (Manager != Custom), @@ -279,6 +282,9 @@ impl Membership { manage_users: false, manage_groups: false, manage_policies: false, + create_new_collections: false, + edit_any_collection: false, + delete_any_collection: false, } } @@ -449,22 +455,26 @@ impl Membership { let membership_type = self.atype; let permissions = json!({ - // The 3 Collection roles below are linked to 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": self.manage_groups, - "managePolicies": self.manage_policies, + "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": self.manage_users, + "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, @@ -508,8 +518,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, @@ -608,18 +617,16 @@ impl Membership { let membership_type = self.atype; - // Only return a permissions object for custom-type members. A custom member - // may have access_all (the 3 collection roles) and/or any of the explicit - // manage_* flags; otherwise Bitwarden assumes all-false defaults. + // 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!({ "accessEventLogs": false, "accessImportExport": false, "accessReports": false, - // If the following 3 Collection roles are set to true a custom user has access all permission - "createNewCollections": self.access_all, - "editAnyCollection": self.access_all, - "deleteAnyCollection": self.access_all, + "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 @@ -811,10 +818,11 @@ 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 manage_* permission flags are only meaningful while the membership is of + // 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 { @@ -829,6 +837,63 @@ impl Membership { 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 + } + + /// `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 { conn.run(move |conn| { users_organizations::table.filter(users_organizations::uuid.eq(uuid)).first::(conn).ok() @@ -1274,6 +1339,13 @@ 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() { @@ -1288,4 +1360,93 @@ mod tests { assert!(MembershipType::Custom > MembershipType::User); assert!(MembershipType::Admin > MembershipType::Custom); } + + #[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 15c43b5f..e8840acd 100644 --- a/src/db/schema.rs +++ b/src/db/schema.rs @@ -245,6 +245,9 @@ table! { 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/templates/scss/vaultwarden.scss.hbs b/src/static/templates/scss/vaultwarden.scss.hbs index c5f790ac..6cf97cb6 100644 --- a/src/static/templates/scss/vaultwarden.scss.hbs +++ b/src/static/templates/scss/vaultwarden.scss.hbs @@ -116,9 +116,8 @@ app-security > app-two-factor-setup > form { } /* Hide unsupported Custom Role options */ -/* Note: manageUsers and managePolicies are supported by Vaultwarden - and are intentionally NOT hidden here. */ -bit-dialog div.tw-ml-4:has(bit-form-control input), +/* The collection permission group plus manageUsers, manageGroups, managePolicies are supported + by Vaultwarden and are intentionally not hidden here. */ bit-dialog div.tw-col-span-4:has(input[formcontrolname*="access"]), bit-dialog bit-form-control:has(input[formcontrolname="manageSso"]), bit-dialog bit-form-control:has(input[formcontrolname="manageResetPassword"]) { From e67fb30c5ffb36bd33e9da817b12d2a799197ccf Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Thu, 16 Jul 2026 19:31:49 +0200 Subject: [PATCH 16/22] Fix delete-only collection access in web vault --- src/api/core/organizations.rs | 72 ++++++++++++++++++++++++++++++++++- 1 file changed, 70 insertions(+), 2 deletions(-) diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index 63d142e8..bf78e0d8 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -51,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, @@ -917,6 +918,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 { @@ -3604,8 +3648,32 @@ mod tests { use serde_json::{Value, json}; - use super::{CustomRolePermissions, may_change_group_membership, may_change_member_type}; - use crate::db::models::{Membership, MembershipStatus, MembershipType}; + use super::{ + CustomRolePermissions, filter_ciphers_for_organization, may_change_group_membership, may_change_member_type, + }; + use crate::db::models::{Cipher, Membership, MembershipStatus, MembershipType, OrganizationId}; + + #[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() { From a992695c0075d0b0c91d5c71bf49d8a9f8b8a9d6 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Thu, 16 Jul 2026 21:51:29 +0200 Subject: [PATCH 17/22] Fix cross-tenant and revoke authorization bypasses from security audit Addresses the confirmed High/Medium findings of the granular-collection- permissions security audit: - H-1: bind direct (non-sync) cipher access to a confirmed membership in the cipher's organization, and harden the user/group collection access-flag queries to require org consistency plus confirmed status. Revoked/invited members can no longer reach ciphers via stale assignment rows. - H-2: validate group ids against the org before mutating group membership in edit_member, and reject cross-org group<->membership links in GroupUser::save and Group::is_in_full_access_group. - H-3: fully pre-validate collections/groups/users in bulk-access and collection create before any mutation, and reject cross-org collection<->group links in CollectionGroup::save. - M-1: require Manage Users for the full member list and Manage Users/Groups for group details (new ManageUsersOrGroupsHeaders guard). - M-2: validate the whole bulk-access request before the destructive delete/replace so an invalid element can't leave partial state behind. - M-3: bounds-validate import collection relationships before writing and propagate cipher-save errors instead of discarding them. --- src/api/core/organizations.rs | 126 +++++++++++++++++++++++++++------- src/auth.rs | 14 ++++ src/db/models/cipher.rs | 59 ++++++++++++++-- src/db/models/group.rs | 35 +++++++++- 4 files changed, 202 insertions(+), 32 deletions(-) diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index bf78e0d8..4b2e7187 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -13,8 +13,8 @@ use crate::{ }, auth::{ AdminHeaders, CollectionDeleteHeaders, CollectionReadHeaders, Headers, ManageGroupsHeaders, - ManagePoliciesHeaders, ManageUsersHeaders, ManagerHeaders, ManagerHeadersLoose, OrgMemberHeaders, OwnerHeaders, - decode_invite, + ManagePoliciesHeaders, ManageUsersHeaders, ManageUsersOrGroupsHeaders, ManagerHeaders, ManagerHeadersLoose, + OrgMemberHeaders, OwnerHeaders, decode_invite, }, db::{ DbConn, @@ -548,6 +548,20 @@ async fn post_organization_collections( let data: FullCollectionData = data.into_inner(); data.validate(&org_id, &conn).await?; + // 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); collection.save(&conn).await?; @@ -622,8 +636,25 @@ async fn post_bulk_access_collections( err!("You don't have permission to modify collection access") } - for col_id in data.collection_ids { - let Some(collection) = Collection::find_by_uuid_and_org(&col_id, &org_id, &conn).await else { + // 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") }; @@ -631,6 +662,12 @@ 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; + // update collection modification date collection.save(&conn).await?; @@ -645,14 +682,14 @@ 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?; } - 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") @@ -662,7 +699,7 @@ async fn post_bulk_access_collections( continue; } - CollectionUser::save(&member.user_uuid, &col_id, user.read_only, user.hide_passwords, user.manage, &conn) + CollectionUser::save(&member.user_uuid, col_id, user.read_only, user.hide_passwords, user.manage, &conn) .await?; } } @@ -1024,10 +1061,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(); @@ -1812,6 +1853,16 @@ async fn edit_member( }; 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?; @@ -2046,6 +2097,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()); @@ -2095,6 +2162,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, @@ -2104,15 +2174,16 @@ 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?; } @@ -2696,15 +2767,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()); @@ -2732,14 +2795,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)] diff --git a/src/auth.rs b/src/auth.rs index bdaf2b6e..c50b3d4c 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -746,6 +746,15 @@ impl OrgHeaders { 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/"), @@ -923,6 +932,11 @@ generate_manage_headers!( 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. diff --git a/src/db/models/cipher.rs b/src/db/models/cipher.rs index 8357c9eb..40b27d5a 100644 --- a/src/db/models/cipher.rs +++ b/src/db/models/cipher.rs @@ -604,6 +604,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. @@ -669,16 +687,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) @@ -691,9 +727,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 @@ -701,13 +744,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/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: From 1cc4238654f85c82c0307d9402ec5d420a31a4a1 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Fri, 17 Jul 2026 14:35:44 +0200 Subject: [PATCH 18/22] Fix collection delete for legacy Managers migrated to Custom The custom-role migration converts every legacy Manager (atype 3) into a Custom member (atype 4) with all collection flags false. collection_delete_access only granted the per-collection (ManagedOnly) path to atype == Manager, so those migrated members silently lost the ability to delete collections they still manage via an explicit users_collections.manage / collections_groups.manage grant (audit finding F-1). Grant ManagedOnly to confirmed Custom members as well, but only when they hold neither edit_any_collection nor access_all, so Edit any collection (which is mirrored onto access_all) can never become a blanket delete. The downstream is_coll_manageable_by_user check still requires a real per-collection Manage grant, so a flagless Custom member without any assignment gains nothing; a blanket "delete any collection" still requires the explicit delete_any_collection permission handled by the Any branch. Adds regression test migrated_legacy_manager_retains_managed_collection_delete. --- src/auth.rs | 65 +++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 61 insertions(+), 4 deletions(-) diff --git a/src/auth.rs b/src/auth.rs index c50b3d4c..131b7ad2 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -967,8 +967,24 @@ enum CollectionDeleteAccess { fn collection_delete_access(membership: &Membership) -> CollectionDeleteAccess { if membership.can_delete_any_collection() { CollectionDeleteAccess::Any - } else if membership.has_status(MembershipStatus::Confirmed) && membership.has_type(MembershipType::Manager) { - // Preserve the legacy Manager role's pre-existing per-collection deletion behavior. + } else if membership.has_status(MembershipStatus::Confirmed) + && (membership.has_type(MembershipType::Manager) + // A member holding the per-collection Manage grant may delete the collections they + // manage (Bitwarden's per-collection "Manage" includes deletion). This covers the + // legacy Manager role as well as legacy Managers that the custom-role migration + // converted into all-flags-false Custom members (audit finding F-1); without it those + // members would silently lose the ability to delete collections they still manage. + // + // The per-collection Manage decision is made downstream by `is_coll_manageable_by_user`, + // which also treats `access_all` as manage-everything. `edit_any_collection` is mirrored + // onto `access_all`, so a Custom member holding Edit any collection (or access_all) is + // deliberately excluded here: Delete must stay independent from Edit and must never + // become a blanket "delete any collection" without the explicit `delete_any_collection` + // permission handled by the `Any` branch above. + || (membership.has_type(MembershipType::Custom) + && !membership.has_edit_any_collection() + && !membership.access_all)) + { CollectionDeleteAccess::ManagedOnly } else { CollectionDeleteAccess::Denied @@ -1083,8 +1099,11 @@ impl From for Headers { } /// Delete is intentionally independent from Edit any collection. Vaultwarden advertises -/// limitCollectionDeletion=true, so Custom members require the explicit Delete any collection -/// permission. The legacy Manager role retains its previous per-collection behavior. +/// 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 — the legacy Manager role and +/// the Custom members the migration produced from legacy Managers — but Edit any collection and +/// access_all never satisfy a delete on their own. pub struct CollectionDeleteHeaders { pub host: String, pub device: Device, @@ -1580,6 +1599,10 @@ mod tests { #[test] fn collection_delete_permission_is_independent_from_edit_and_access_all() { + // Edit any collection (which mirrors onto access_all) must never satisfy a delete on its + // own: it maps to `Denied` here, not `ManagedOnly`, so it can't ride the per-collection + // manage path into deleting collections. Only the explicit Delete any collection flag + // (handled by the `Any` branch) turns Edit-any members into collection deleters. let mut custom = membership(MembershipType::Custom); custom.edit_any_collection = true; custom.access_all = true; @@ -1603,4 +1626,38 @@ mod tests { let user = membership(MembershipType::User); assert_eq!(collection_delete_access(&user), CollectionDeleteAccess::Denied); } + + #[test] + fn migrated_legacy_manager_retains_managed_collection_delete() { + // Regression (audit finding F-1): the custom-role migration turns every legacy Manager + // (including those with access_all=false and an explicit per-collection Manage grant) into a + // Custom member with all collection flags false. Such a member must still reach the + // per-collection Manage check (`ManagedOnly`) instead of being denied outright, otherwise it + // silently loses the ability to delete the collections it manages. `ManagedOnly` is not a + // blanket grant: the downstream `is_coll_manageable_by_user` check still requires an actual + // Manage assignment (users_collections.manage / collections_groups.manage), which a plain + // Custom member without any assignment does not have. + let confirmed_custom = membership(MembershipType::Custom); + assert!(!confirmed_custom.access_all); + assert!(!confirmed_custom.edit_any_collection); + assert!(!confirmed_custom.delete_any_collection); + assert_eq!(collection_delete_access(&confirmed_custom), CollectionDeleteAccess::ManagedOnly); + + // create_new_collections alone must not change the delete decision either way. + let mut create_only = membership(MembershipType::Custom); + create_only.create_new_collections = true; + assert_eq!(collection_delete_access(&create_only), CollectionDeleteAccess::ManagedOnly); + + // Edit any collection / access_all still map to Denied (see the independence test above), + // so a Custom member gains a blanket delete only through delete_any_collection. + let mut edit_any = membership(MembershipType::Custom); + edit_any.edit_any_collection = true; + edit_any.access_all = true; + assert_eq!(collection_delete_access(&edit_any), CollectionDeleteAccess::Denied); + + // The membership must be confirmed; an invited/accepted member is always denied. + let mut unconfirmed = membership(MembershipType::Custom); + unconfirmed.status = MembershipStatus::Accepted as i32; + assert_eq!(collection_delete_access(&unconfirmed), CollectionDeleteAccess::Denied); + } } From 3cd4dd8bfa880269a5a53aa984670c795e895e7d Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Sat, 18 Jul 2026 14:04:32 +0200 Subject: [PATCH 19/22] Fix custom collection authorization --- .../up.sql | 16 ++ .../up.sql | 16 ++ .../up.sql | 16 ++ src/api/core/organizations.rs | 5 +- src/auth.rs | 224 ++++++++++-------- src/db/models/organization.rs | 129 +++++++--- 6 files changed, 279 insertions(+), 127 deletions(-) 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 index c13662c4..da66070a 100644 --- 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 @@ -9,3 +9,19 @@ 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-07-16-120000_add_custom_collection_permissions/up.sql b/migrations/postgresql/2026-07-16-120000_add_custom_collection_permissions/up.sql index c13662c4..da66070a 100644 --- 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 @@ -9,3 +9,19 @@ 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-07-16-120000_add_custom_collection_permissions/up.sql b/migrations/sqlite/2026-07-16-120000_add_custom_collection_permissions/up.sql index c13662c4..da66070a 100644 --- 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 @@ -9,3 +9,19 @@ 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/core/organizations.rs b/src/api/core/organizations.rs index 4b2e7187..ca30766e 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -2182,7 +2182,10 @@ async fn post_org_import( // any future drift fails closed with an error instead of panicking. for (cipher_index, col_index) in relations { 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") + err!( + "Invalid collection relationship", + "A collection relationship references a non-existent cipher or collection" + ) }; CollectionCipher::save(cipher_id, col_id, &conn).await?; } diff --git a/src/auth.rs b/src/auth.rs index 131b7ad2..786f864c 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -958,36 +958,61 @@ fn get_col_id(request: &Request<'_>) -> Option { } #[derive(Clone, Copy, Debug, Eq, PartialEq)] -enum CollectionDeleteAccess { +enum CollectionManageAccess { Any, - ManagedOnly, + LegacyManager, + ExplicitManage, Denied, } -fn collection_delete_access(membership: &Membership) -> CollectionDeleteAccess { - if membership.can_delete_any_collection() { - CollectionDeleteAccess::Any - } else if membership.has_status(MembershipStatus::Confirmed) - && (membership.has_type(MembershipType::Manager) - // A member holding the per-collection Manage grant may delete the collections they - // manage (Bitwarden's per-collection "Manage" includes deletion). This covers the - // legacy Manager role as well as legacy Managers that the custom-role migration - // converted into all-flags-false Custom members (audit finding F-1); without it those - // members would silently lose the ability to delete collections they still manage. - // - // The per-collection Manage decision is made downstream by `is_coll_manageable_by_user`, - // which also treats `access_all` as manage-everything. `edit_any_collection` is mirrored - // onto `access_all`, so a Custom member holding Edit any collection (or access_all) is - // deliberately excluded here: Delete must stay independent from Edit and must never - // become a blanket "delete any collection" without the explicit `delete_any_collection` - // permission handled by the `Any` branch above. - || (membership.has_type(MembershipType::Custom) - && !membership.has_edit_any_collection() - && !membership.access_all)) - { - CollectionDeleteAccess::ManagedOnly - } else { - CollectionDeleteAccess::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, } } @@ -1011,14 +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 !headers.membership.has_edit_any_collection() - && !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") @@ -1062,16 +1088,14 @@ impl<'r> FromRequest<'r> for CollectionReadHeaders { err_handler!("Error getting the collection id") }; - let can_read_any_collection = headers.is_confirmed_and_admin() - || headers.membership.has_edit_any_collection() - || headers.membership.has_delete_any_collection(); + let access = collection_read_access(&headers.membership); - if !can_read_any_collection { + if access != CollectionManageAccess::Any { 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 { + if !can_manage_collection(access, &headers.membership, &col_id, &conn).await { err_handler!("The current user isn't a manager for this collection") } } @@ -1101,9 +1125,9 @@ impl From for Headers { /// 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 — the legacy Manager role and -/// the Custom members the migration produced from legacy Managers — but Edit any collection and -/// access_all never satisfy a delete on their own. +/// 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, @@ -1127,18 +1151,18 @@ impl<'r> FromRequest<'r> for CollectionDeleteHeaders { }; match collection_delete_access(&headers.membership) { - CollectionDeleteAccess::Any => {} - CollectionDeleteAccess::Denied => { - // Custom is a distinct, fail-closed role. In particular, Edit any collection and - // access_all must not satisfy a Delete request without the explicit delete flag. + 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") } - CollectionDeleteAccess::ManagedOnly => { + access @ (CollectionManageAccess::LegacyManager | CollectionManageAccess::ExplicitManage) => { 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 { + if !can_manage_collection(access, &headers.membership, &col_id, &conn).await { err_handler!("The current user isn't a manager for this collection") } } @@ -1224,7 +1248,7 @@ impl CollectionDeleteHeaders { conn: &DbConn, ) -> Result { let delete_access = collection_delete_access(&h.membership); - if delete_access == CollectionDeleteAccess::Denied { + if delete_access == CollectionManageAccess::Denied { err!("You need the 'Delete any collection' permission to call this endpoint") } @@ -1235,8 +1259,8 @@ impl CollectionDeleteHeaders { 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 == CollectionDeleteAccess::ManagedOnly - && !Collection::is_coll_manageable_by_user(col_id, &h.membership.user_uuid, conn).await + 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") } @@ -1587,7 +1611,7 @@ pub async fn refresh_tokens( #[cfg(test)] mod tests { - use super::{CollectionDeleteAccess, collection_delete_access}; + 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 { @@ -1598,66 +1622,68 @@ mod tests { } #[test] - fn collection_delete_permission_is_independent_from_edit_and_access_all() { - // Edit any collection (which mirrors onto access_all) must never satisfy a delete on its - // own: it maps to `Denied` here, not `ManagedOnly`, so it can't ride the per-collection - // manage path into deleting collections. Only the explicit Delete any collection flag - // (handled by the `Any` branch) turns Edit-any members into collection deleters. - let mut custom = membership(MembershipType::Custom); - custom.edit_any_collection = true; - custom.access_all = true; - assert_eq!(collection_delete_access(&custom), CollectionDeleteAccess::Denied); + 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); + } - custom.delete_any_collection = true; - assert_eq!(collection_delete_access(&custom), CollectionDeleteAccess::Any); + #[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); - custom.status = MembershipStatus::Accepted as i32; - assert_eq!(collection_delete_access(&custom), CollectionDeleteAccess::Denied); + 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 collection_delete_permission_preserves_admin_and_legacy_manager_behavior() { - let admin = membership(MembershipType::Admin); - assert_eq!(collection_delete_access(&admin), CollectionDeleteAccess::Any); - + fn exact_legacy_manager_keeps_broad_helper() { let manager = membership(MembershipType::Manager); - assert_eq!(collection_delete_access(&manager), CollectionDeleteAccess::ManagedOnly); + 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_delete_access(&user), CollectionDeleteAccess::Denied); + assert_eq!(collection_edit_access(&user), CollectionManageAccess::Denied); + assert_eq!(collection_delete_access(&user), CollectionManageAccess::Denied); } #[test] - fn migrated_legacy_manager_retains_managed_collection_delete() { - // Regression (audit finding F-1): the custom-role migration turns every legacy Manager - // (including those with access_all=false and an explicit per-collection Manage grant) into a - // Custom member with all collection flags false. Such a member must still reach the - // per-collection Manage check (`ManagedOnly`) instead of being denied outright, otherwise it - // silently loses the ability to delete the collections it manages. `ManagedOnly` is not a - // blanket grant: the downstream `is_coll_manageable_by_user` check still requires an actual - // Manage assignment (users_collections.manage / collections_groups.manage), which a plain - // Custom member without any assignment does not have. - let confirmed_custom = membership(MembershipType::Custom); - assert!(!confirmed_custom.access_all); - assert!(!confirmed_custom.edit_any_collection); - assert!(!confirmed_custom.delete_any_collection); - assert_eq!(collection_delete_access(&confirmed_custom), CollectionDeleteAccess::ManagedOnly); - - // create_new_collections alone must not change the delete decision either way. - let mut create_only = membership(MembershipType::Custom); - create_only.create_new_collections = true; - assert_eq!(collection_delete_access(&create_only), CollectionDeleteAccess::ManagedOnly); - - // Edit any collection / access_all still map to Denied (see the independence test above), - // so a Custom member gains a blanket delete only through delete_any_collection. - let mut edit_any = membership(MembershipType::Custom); - edit_any.edit_any_collection = true; - edit_any.access_all = true; - assert_eq!(collection_delete_access(&edit_any), CollectionDeleteAccess::Denied); + 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); - // The membership must be confirmed; an invited/accepted member is always denied. let mut unconfirmed = membership(MembershipType::Custom); unconfirmed.status = MembershipStatus::Accepted as i32; - assert_eq!(collection_delete_access(&unconfirmed), CollectionDeleteAccess::Denied); + assert_eq!(collection_edit_access(&unconfirmed), CollectionManageAccess::Denied); + assert_eq!(collection_delete_access(&unconfirmed), CollectionManageAccess::Denied); } } diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index 3fbfc789..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, @@ -119,27 +119,25 @@ impl MembershipType { _ => 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). - // Custom is treated as a low-privilege base role (same level as Manager for - // ordering purposes); its elevated capabilities are governed by the explicit - // custom permission flags on the Membership, not by this ordering. - // - // NOTE: Manager and Custom therefore share an access level while being distinct - // variants: the derived `PartialEq` compares the role itself (Manager != Custom), - // while this ordering compares access levels (neither is greater than the other). - // Keep that in mind before relying on `cmp() == Equal` implying equality. - const ACCESS_LEVEL: [i32; 5] = [ - 3, // Owner - 2, // Admin - 0, // User - 1, // Manager - 1, // 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))) } } @@ -849,6 +847,69 @@ impl Membership { 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 { @@ -1347,18 +1408,32 @@ mod tests { } #[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::Custom == MembershipType::from_str("4").unwrap()); - // Manager and Custom share the same access level, but are distinct roles - assert!(MembershipType::Manager != MembershipType::Custom); - assert!(MembershipType::Manager >= MembershipType::Custom); + + // Permission comparisons continue to treat Custom as manager-level and below Admin. assert!(MembershipType::Custom >= MembershipType::Manager); - assert!(MembershipType::Custom > MembershipType::User); - assert!(MembershipType::Admin > MembershipType::Custom); + 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] From 84edbf23b0b7ae368e222af9526e7a6b5d943431 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Sat, 18 Jul 2026 23:19:10 +0200 Subject: [PATCH 20/22] Fix Custom Role selectors for new dialogs --- src/static/templates/scss/vaultwarden.scss.hbs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/static/templates/scss/vaultwarden.scss.hbs b/src/static/templates/scss/vaultwarden.scss.hbs index 6cf97cb6..11bcbad6 100644 --- a/src/static/templates/scss/vaultwarden.scss.hbs +++ b/src/static/templates/scss/vaultwarden.scss.hbs @@ -118,9 +118,9 @@ app-security > app-two-factor-setup > form { /* Hide unsupported Custom Role options */ /* The collection permission group plus manageUsers, manageGroups, managePolicies are supported by Vaultwarden and are intentionally not hidden here. */ -bit-dialog div.tw-col-span-4:has(input[formcontrolname*="access"]), -bit-dialog bit-form-control:has(input[formcontrolname="manageSso"]), -bit-dialog bit-form-control:has(input[formcontrolname="manageResetPassword"]) { +: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; } From 8289e2dcff314853e5cc8c0fdac27edb9535284c Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Sun, 19 Jul 2026 20:20:15 +0200 Subject: [PATCH 21/22] Fix custom-role privilege escalation and align read guards (security review) Follow-up hardening on the custom-role work, found during a static review: - Admin panel type change (apply_membership_type_change): when converting a Custom member to the legacy Manager role, only preserve `access_all` if the member actually held the full "manage all collections" grant (all three collection flags). Previously an Edit-any-collection-only Custom member (whose access_all is just the Edit mirror) became a Manager with a broad access_all grant, silently escalating Edit-only into Create + Edit + Delete. Mirrors the collection-permissions down-migration. Unit test updated. - Bulk collection access (post_bulk_access_collections): drop the blanket has_full_access() requirement and rely on the existing per-collection is_manageable_by_user check (as the single-collection endpoint and the pre-existing behavior do). Pure manage_users / manage_groups / manage_policies Custom members hold no per-collection manage grant and are still rejected, while a Manager/Custom member who manages some collections regains the ability to bulk-edit exactly those collections. - Group details (GET /organizations//groups//details): align the guard with the list endpoint (/groups/details) to ManageUsersOrGroups, so a manage_users member is not denied the single-group view of the same data it can already read in bulk. --- src/api/admin.rs | 42 +++++++++++++++++++++++++++++------ src/api/core/organizations.rs | 20 +++++++++++------ 2 files changed, 48 insertions(+), 14 deletions(-) diff --git a/src/api/admin.rs b/src/api/admin.rs index 0c85d562..8c60e0c7 100644 --- a/src/api/admin.rs +++ b/src/api/admin.rs @@ -547,6 +547,16 @@ struct MembershipTypeData { 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. @@ -956,12 +966,30 @@ mod tests { apply_membership_type_change(&mut user, MembershipType::Admin); assert!(user.access_all); - let mut custom = membership(MembershipType::Custom); - custom.access_all = true; - custom.edit_any_collection = true; - apply_membership_type_change(&mut custom, MembershipType::Manager); - assert_eq!(custom.atype, MembershipType::Manager as i32); - assert!(custom.access_all); - assert!(!custom.edit_any_collection); + // 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 ca30766e..4945f3dc 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -629,12 +629,13 @@ async fn post_bulk_access_collections( err!("Can't find organization details") } - // Security: only callers who can actually manage collections (Admins/Owners, or users with - // full access) may change collection access in bulk. A custom user with only manage_users / - // manage_groups / manage_policies must not be able to modify collection assignments here. - if !headers.membership.has_full_access() { - err!("You don't have permission to modify collection access") - } + // 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 @@ -3110,11 +3111,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: ManageGroupsHeaders, + headers: ManageUsersOrGroupsHeaders, conn: DbConn, ) -> JsonResult { if org_id != headers.org_id { From 5fda3e4dac8a04ada142ebc7e41dabe4a4c6eb9a Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Tue, 21 Jul 2026 23:07:09 +0200 Subject: [PATCH 22/22] Fix privilege escalation: Edit any collection could reach Delete any collection A Custom member holding only `edit_any_collection` (whose `access_all` mirror makes every collection "manageable") could grant a per-collection `manage` row to a group it belongs to -- via post_bulk_access_collections, post_organization_collection_update, add_update_group or edit_member -- and then delete that collection through CollectionDeleteHeaders' `has_explicit_collection_manage_access` path, despite holding no `delete_any_collection` permission. This defeated the intended edit != delete separation (confirmed end-to-end: the delete returned 200 before this change). Gate every caller-controlled collection-assignment `manage` write behind a new `caller_may_grant_collection_manage()` check that mirrors the delete authorization exactly: a caller may confer `manage` (which carries delete authority) on a collection only if it could delete that collection itself -- Admin/Owner or `delete_any_collection` always; an exact legacy Manager via its per-collection manage helper; any other Custom member only with a real persisted `users_collections.manage` / `collections_groups.manage` grant. `edit_any_collection`'s `access_all` mirror deliberately does not count. The change is strictly subtractive: it can only ever downgrade a requested `manage` to false, never grant it, so it opens no new access and leaves Admins/Owners and genuinely delete-capable members unaffected. The create, organization-import and invite paths are intentionally left alone (new-collection creators must manage their own collection; non-owner invites can only create plain Users, which cannot delete). Adds a unit test for the collection-independent part of the gate. --- src/api/core/organizations.rs | 175 ++++++++++++++++++++++++++++++++-- 1 file changed, 165 insertions(+), 10 deletions(-) diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index 4945f3dc..c9f68110 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -669,6 +669,11 @@ async fn post_bulk_access_collections( 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?; @@ -685,9 +690,15 @@ async fn post_bulk_access_collections( 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?; @@ -700,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?; } } @@ -760,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?; } @@ -779,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)) @@ -1823,18 +1856,28 @@ async fn edit_member( 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, - col.manage, + manage, &conn, ) .await?; @@ -3058,6 +3101,64 @@ async fn group_confers_collection_access(group_id: &GroupId, org_id: &Organizati } } +/// 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( mut group: Group, collections: Vec, @@ -3069,8 +3170,20 @@ async fn add_update_group( ) -> 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?; } @@ -3732,10 +3845,52 @@ mod tests { use serde_json::{Value, json}; use super::{ - CustomRolePermissions, filter_ciphers_for_organization, may_change_group_membership, may_change_member_type, + 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();