From 32098ca7d1b94abce848b03e66545442a5d0e836 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa?= Date: Wed, 23 Sep 2026 23:22:43 +0200 Subject: [PATCH] Require confirmed membership for user access checks, not for admin views (#7763) --- src/api/core/ciphers.rs | 16 +++---- src/api/core/organizations.rs | 22 ++++++---- src/db/models/cipher.rs | 5 +++ src/db/models/collection.rs | 19 ++++++++- src/db/models/group.rs | 2 + src/db/models/organization.rs | 80 ++++++++++------------------------- 6 files changed, 68 insertions(+), 76 deletions(-) diff --git a/src/api/core/ciphers.rs b/src/api/core/ciphers.rs index a5b7e58b..a8c6aea0 100644 --- a/src/api/core/ciphers.rs +++ b/src/api/core/ciphers.rs @@ -2180,8 +2180,15 @@ impl CipherSyncData { } } + // Generate a HashMap with the Organization UUID as key and the Membership record + let members: HashMap = Membership::find_confirmed_by_user(user_id, conn) + .await + .into_iter() + .map(|m| (m.org_uuid.clone(), m)) + .collect(); + // Generate a list of Cipher UUID's containing a Vec with one or more Attachment records - let orgs = Membership::get_orgs_by_user(user_id, conn).await; + let orgs: Vec = members.keys().cloned().collect(); let attachments = Attachment::find_all_by_user_and_orgs(user_id, &orgs, conn).await; let mut cipher_attachments: HashMap> = HashMap::with_capacity(attachments.len()); for attachment in attachments { @@ -2196,13 +2203,6 @@ impl CipherSyncData { cipher_collections.entry(cipher).or_default().push(collection); } - // Generate a HashMap with the Organization UUID as key and the Membership record - let members: HashMap = Membership::find_confirmed_by_user(user_id, conn) - .await - .into_iter() - .map(|m| (m.org_uuid.clone(), m)) - .collect(); - // Generate a HashMap with the User_Collections UUID as key and the CollectionUser record let user_collections: HashMap = CollectionUser::find_by_user(user_id, conn) .await diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index 36297d30..046be793 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -415,8 +415,9 @@ async fn get_org_collections_details(org_id: OrganizationId, headers: ManagerHea let col_users = CollectionUser::find_by_organization_swap_user_uuid_with_member_uuid(&org_id, &conn).await; // Generate a HashMap to get the correct MembershipType per user to determine the manage permission // We use the uuid instead of the user_uuid here, since that is what is used in CollectionUser + // This lists other members for admins, so it must not depend on the membership status let membership_type: HashMap = - Membership::find_confirmed_by_org(&org_id, &conn).await.into_iter().map(|m| (m.uuid, m.atype)).collect(); + Membership::find_by_org(&org_id, &conn).await.into_iter().map(|m| (m.uuid, m.atype)).collect(); // check if current user has full access to the organization (either directly or via any group) let has_full_access_to_org = member.has_full_access() @@ -818,11 +819,9 @@ async fn get_org_collection_detail( // Generate a HashMap to get the correct MembershipType per user to determine the manage permission // We use the uuid instead of the user_uuid here, since that is what is used in CollectionUser - let membership_type: HashMap = Membership::find_confirmed_by_org(&org_id, &conn) - .await - .into_iter() - .map(|m| (m.uuid, m.atype)) - .collect(); + // This lists other members for admins, so it must not depend on the membership status + let membership_type: HashMap = + Membership::find_by_org(&org_id, &conn).await.into_iter().map(|m| (m.uuid, m.atype)).collect(); let users: Vec = CollectionUser::find_by_org_and_coll_swap_user_uuid_with_member_uuid(&org_id, &collection.uuid, &conn) @@ -960,8 +959,9 @@ async fn get_members( let mut users_json = Vec::new(); for u in Membership::find_by_org(&org_id, &conn).await { + // The user can be a manager instead of an admin, but we've checked above that they have full access users_json.push( - u.to_json_user_details( + u.to_json_details_for_admin( data.include_collections.unwrap_or(false), data.include_groups.unwrap_or(false), &conn, @@ -1516,7 +1516,9 @@ async fn get_user( // In this case, when groups are requested we also need to include collections. // Else these will not be shown in the interface, and could lead to missing collections when saved. let include_groups = data.include_groups.unwrap_or(false); - Ok(Json(user.to_json_user_details(data.include_collections.unwrap_or(include_groups), include_groups, &conn).await)) + Ok(Json( + user.to_json_details_for_admin(data.include_collections.unwrap_or(include_groups), include_groups, &conn).await, + )) } #[derive(Deserialize)] @@ -1835,6 +1837,10 @@ async fn post_org_import( if org_id != headers.membership.org_uuid { err!("Organization not found", "Organization id's do not match"); } + // OrgMemberHeaders also allows invited and accepted members, which are not allowed to import + if headers.membership.status != MembershipStatus::Confirmed as i32 { + err!("You need to be a Member of the Organization to call this endpoint") + } let data: ImportData = data.into_inner(); // Validate the import before continuing diff --git a/src/db/models/cipher.rs b/src/db/models/cipher.rs index fae8aaec..4a8ba1c1 100644 --- a/src/db/models/cipher.rs +++ b/src/db/models/cipher.rs @@ -1003,6 +1003,7 @@ impl Cipher { .eq(ciphers_collections::collection_uuid) .and(collections_groups::groups_uuid.eq(groups::uuid))), ) + .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) .filter( users_organizations::access_all .eq(true) // User has access all @@ -1034,6 +1035,7 @@ impl Cipher { .eq(ciphers_collections::collection_uuid) .and(users_collections::user_uuid.eq(user_uuid.clone()))), ) + .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) .filter( users_organizations::access_all .eq(true) // User has access all @@ -1078,6 +1080,7 @@ impl Cipher { .eq(ciphers_collections::collection_uuid) .and(collections_groups::groups_uuid.eq(groups::uuid))), ) + .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) .filter( users_organizations::access_all .eq(true) // User has access all @@ -1110,6 +1113,7 @@ impl Cipher { .eq(ciphers_collections::collection_uuid) .and(users_collections::user_uuid.eq(user_uuid.clone()))), ) + .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) .filter( users_organizations::access_all .eq(true) // User has access all @@ -1161,6 +1165,7 @@ impl Cipher { .or_filter(users_organizations::atype.le(MembershipType::Admin as i32)) // User is admin or owner .or_filter(groups::access_all.eq(true)) //Access via group .or_filter(collections_groups::collections_uuid.is_not_null()) //Access via group + .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) .select(ciphers_collections::all_columns) .distinct() .load::<(CipherId, CollectionId)>(conn) diff --git a/src/db/models/collection.rs b/src/db/models/collection.rs index be108f13..21387e4d 100644 --- a/src/db/models/collection.rs +++ b/src/db/models/collection.rs @@ -367,6 +367,7 @@ impl Collection { .and(collections_groups::collections_uuid.eq(collections::uuid))), ) .filter(collections::uuid.eq(uuid)) + .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) .filter( users_collections::collection_uuid .eq(uuid) @@ -406,6 +407,7 @@ impl Collection { .and(users_organizations::user_uuid.eq(user_uuid))), ) .filter(collections::uuid.eq(uuid)) + .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) .filter(users_collections::collection_uuid.eq(uuid).or( // Directly accessed collection users_organizations::access_all.eq(true).or( @@ -430,7 +432,8 @@ impl Collection { .inner_join( users_organizations::table.on(collections::org_uuid .eq(users_organizations::org_uuid) - .and(users_organizations::user_uuid.eq(user_uuid.clone()))), + .and(users_organizations::user_uuid.eq(user_uuid.clone())) + .and(users_organizations::status.eq(MembershipStatus::Confirmed as i32))), ) .left_join( users_collections::table.on(users_collections::collection_uuid @@ -476,7 +479,8 @@ impl Collection { .inner_join( users_organizations::table.on(collections::org_uuid .eq(users_organizations::org_uuid) - .and(users_organizations::user_uuid.eq(user_uuid.clone()))), + .and(users_organizations::user_uuid.eq(user_uuid.clone())) + .and(users_organizations::status.eq(MembershipStatus::Confirmed as i32))), ) .left_join( users_collections::table.on(users_collections::collection_uuid @@ -527,6 +531,7 @@ impl Collection { .and(collections_groups::collections_uuid.eq(collections::uuid))), ) .filter(collections::uuid.eq(&self.uuid)) + .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) .filter( users_collections::collection_uuid .eq(&self.uuid) @@ -586,6 +591,7 @@ impl Collection { .and(collections_groups::collections_uuid.eq(collections::uuid))), ) .filter(collections::uuid.eq(&uuid)) + .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) .filter( users_collections::collection_uuid .eq(&uuid) @@ -650,6 +656,7 @@ impl Collection { .and(collections_groups::collections_uuid.eq(collections::uuid))), ) .filter(collections::org_uuid.eq(&org_uuid)) + .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) .filter( // Manage permission on a collection assigned directly or via a group. users_collections::manage.eq(true).or(collections_groups::manage.eq(true)), @@ -819,10 +826,18 @@ impl CollectionUser { .await } + // Only returns the collections of organizations the user is a confirmed member of pub async fn find_by_user(user_uuid: &UserId, conn: &DbConn) -> Vec { conn.run(move |conn| { users_collections::table + .inner_join(collections::table.on(collections::uuid.eq(users_collections::collection_uuid))) + .inner_join( + users_organizations::table.on(users_organizations::org_uuid + .eq(collections::org_uuid) + .and(users_organizations::user_uuid.eq(users_collections::user_uuid))), + ) .filter(users_collections::user_uuid.eq(user_uuid)) + .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) .select(users_collections::all_columns) .load::(conn) .expect("Error loading users_collections") diff --git a/src/db/models/group.rs b/src/db/models/group.rs index d39a15e7..347b3235 100644 --- a/src/db/models/group.rs +++ b/src/db/models/group.rs @@ -249,6 +249,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() @@ -387,6 +388,7 @@ impl CollectionGroup { .and(collections::org_uuid.eq(groups::organizations_uuid))), ) .filter(users_organizations::user_uuid.eq(user_uuid)) + .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) .select(collections_groups::all_columns) .load::(conn) .expect("Error loading user collection groups") diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index d615a3fc..353a406e 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -1,7 +1,4 @@ -use std::{ - cmp::Ordering, - collections::{HashMap, HashSet}, -}; +use std::cmp::Ordering; use chrono::{NaiveDateTime, Utc}; use derive_more::{AsRef, Deref, Display, From}; @@ -24,8 +21,8 @@ use crate::{ use macros::UuidFromParam; use super::{ - Cipher, CipherId, Collection, CollectionGroup, CollectionId, CollectionUser, Group, GroupId, GroupUser, OrgPolicy, - OrgPolicyType, TwoFactor, User, UserId, + Cipher, CipherId, Collection, CollectionId, CollectionUser, Group, GroupId, GroupUser, OrgPolicy, OrgPolicyType, + TwoFactor, User, UserId, }; #[derive(Identifiable, Queryable, Insertable, AsChangeset)] @@ -536,7 +533,13 @@ impl Membership { }) } - pub async fn to_json_user_details(&self, include_collections: bool, include_groups: bool, conn: &DbConn) -> Value { + // Used by admins to view other members, so nothing here may depend on this member's status + pub async fn to_json_details_for_admin( + &self, + include_collections: bool, + include_groups: bool, + conn: &DbConn, + ) -> Value { let user = User::find_by_uuid(&self.user_uuid, conn).await.unwrap(); // Because BitWarden want the status to be -1 for revoked users we need to catch that here. @@ -559,52 +562,23 @@ impl Membership { // Check if a user is in a group which has access to all collections // If that is the case, we should not return individual collections! + // This is used by admins to view other members, so it must not depend on the membership status let full_access_group = - CONFIG.org_groups_enabled() && Group::is_in_full_access_group(&self.user_uuid, &self.org_uuid, conn).await; + CONFIG.org_groups_enabled() && GroupUser::has_full_access_by_member(&self.org_uuid, &self.uuid, conn).await; // If collections are to be included, only include them if the user does not have full access via a group or defined to the user it self + // Only the collections assigned directly are returned, the ones assigned via a group are returned via a special group endpoint let collections: Vec = if include_collections && !(full_access_group || self.access_all) { - // Get all collections for the user here already to prevent more queries - let cu: HashMap = - CollectionUser::find_by_organization_and_user_uuid(&self.org_uuid, &self.user_uuid, conn) - .await - .into_iter() - .map(|cu| (cu.collection_uuid.clone(), cu)) - .collect(); - - // Get all collection groups for this user to prevent there inclusion - let cg: HashSet = CollectionGroup::find_by_user(&self.user_uuid, conn) - .await - .into_iter() - .map(|cg| cg.collections_uuid) - .collect(); - - Collection::find_by_organization_and_user_uuid(&self.org_uuid, &self.user_uuid, conn) + CollectionUser::find_by_organization_and_user_uuid(&self.org_uuid, &self.user_uuid, conn) .await .into_iter() - .filter_map(|c| { - let (read_only, hide_passwords, manage) = if self.has_full_access() { - (false, false, self.atype >= MembershipType::Manager) - } else if let Some(cu) = cu.get(&c.uuid) { - ( - 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 - } else if cg.contains(&c.uuid) { - return None; - } else { - (true, true, false) - }; - - Some(json!({ - "id": c.uuid, - "readOnly": read_only, - "hidePasswords": hide_passwords, - "manage": manage, - })) + .map(|cu| { + json!({ + "id": cu.collection_uuid, + "readOnly": cu.read_only, + "hidePasswords": cu.hide_passwords, + "manage": cu.manage || (self.atype == MembershipType::Manager && !cu.read_only && !cu.hide_passwords), + }) }) .collect() } else { @@ -1032,17 +1006,6 @@ impl Membership { .await } - pub async fn get_orgs_by_user(user_uuid: &UserId, conn: &DbConn) -> Vec { - conn.run(move |conn| { - users_organizations::table - .filter(users_organizations::user_uuid.eq(user_uuid)) - .select(users_organizations::org_uuid) - .load::(conn) - .unwrap_or_default() - }) - .await - } - pub async fn find_by_user_and_policy(user_uuid: &UserId, policy_type: OrgPolicyType, conn: &DbConn) -> Vec { conn.run(move |conn| { users_organizations::table @@ -1126,6 +1089,7 @@ impl Membership { .and(ciphers::organization_uuid.eq(users_organizations::org_uuid.nullable()))), ) .filter(users_organizations::user_uuid.eq(user_uuid)) + .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) .filter( users_organizations::atype.eq_any(vec![MembershipType::Owner as i32, MembershipType::Admin as i32]), )