diff --git a/src/api/core/ciphers.rs b/src/api/core/ciphers.rs index d4e3ddf5..dc1269e8 100644 --- a/src/api/core/ciphers.rs +++ b/src/api/core/ciphers.rs @@ -2439,8 +2439,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 { @@ -2455,13 +2462,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 429c0d6e..a3c6d6e1 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -442,8 +442,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() @@ -915,11 +916,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) @@ -1176,8 +1175,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, @@ -1848,7 +1848,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)] @@ -2167,7 +2169,6 @@ async fn post_org_import( if org_id != headers.membership.org_uuid { err!("Organization not found", "Organization id's do not match"); } - // AccessImportExport authorizes the complete organization import. Other confirmed members keep // the regular per-target Create/Update authorization. if !headers.membership.has_status(MembershipStatus::Confirmed) { diff --git a/src/db/models/cipher.rs b/src/db/models/cipher.rs index 1df857b5..9a73d533 100644 --- a/src/db/models/cipher.rs +++ b/src/db/models/cipher.rs @@ -855,6 +855,9 @@ impl Cipher { .and(collections::org_uuid.nullable().eq(ciphers::organization_uuid))), ) .filter(users_organizations::user_uuid.eq(user_uuid)) + // Only allow group access via a confirmed membership, since + // groups_users rows are kept when a membership is revoked. + .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) .select((collections_groups::read_only, collections_groups::hide_passwords, collections_groups::manage)) .load::<(bool, bool, bool)>(conn) .expect("Error getting group access restrictions") @@ -1159,6 +1162,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( custom_membership_with_edit_any_collection() // Custom "Edit any collection" (successor of access_all) .or(users_organizations::atype.eq_any(ORG_ADMIN_ATYPES)) // or org admin/owner @@ -1190,6 +1194,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( custom_membership_with_edit_any_collection() // Custom "Edit any collection" (successor of access_all) .or(users_organizations::atype.eq_any(ORG_ADMIN_ATYPES)) // or org admin/owner @@ -1234,6 +1239,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( custom_membership_with_edit_any_collection() // Custom "Edit any collection" (successor of access_all) .or(users_organizations::atype.eq_any(ORG_ADMIN_ATYPES)) // or org admin/owner @@ -1266,6 +1272,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( custom_membership_with_edit_any_collection() // Custom "Edit any collection" (successor of access_all) .or(users_organizations::atype.eq_any(ORG_ADMIN_ATYPES)) // or org admin/owner @@ -1317,6 +1324,7 @@ impl Cipher { .or_filter(users_organizations::atype.eq_any(ORG_ADMIN_ATYPES)) // 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 f1f237d4..5b38ce82 100644 --- a/src/db/models/collection.rs +++ b/src/db/models/collection.rs @@ -406,6 +406,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) @@ -445,6 +446,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 custom_membership_with_edit_any_collection().or( @@ -469,7 +471,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 @@ -515,7 +518,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 @@ -566,6 +570,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) @@ -797,10 +802,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 35b47dc3..f06a83a9 100644 --- a/src/db/models/group.rs +++ b/src/db/models/group.rs @@ -267,6 +267,8 @@ impl Group { .and(users_organizations::org_uuid.eq(groups::organizations_uuid))), ) .filter(users_organizations::user_uuid.eq(user_uuid)) + // Only allow full access via a confirmed membership, since + // groups_users rows are kept when a membership is revoked. .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) .filter(groups::organizations_uuid.eq(org_uuid)) .filter(groups::access_all.eq(true)) @@ -393,6 +395,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 b1b840c0..288fcae0 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -605,7 +605,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. @@ -1184,17 +1190,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