From d509298d803c202edfd5f90c92a08e3307a3af96 Mon Sep 17 00:00:00 2001 From: Stefan Melmuk Date: Fri, 5 Jan 2024 02:48:26 +0100 Subject: [PATCH] improve check if a manager has access via groups instead of returning the two lists of member ids and later checking if they contain the uuid of the current user, we really only care if the current user has full access via a group or if they have access to a given collection via a group --- src/api/core/organizations.rs | 55 +++++++++++++++++------------------ src/db/models/group.rs | 28 +++++++++--------- 2 files changed, 40 insertions(+), 43 deletions(-) diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index a7629d742b5..d7730807091 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -321,38 +321,24 @@ async fn get_org_collections_details(org_id: &str, headers: ManagerHeadersLoose, None => err!("User is not part of organization"), }; + // get all collection memberships for the current organization let coll_users = CollectionUser::find_by_organization(org_id, &mut conn).await; - // uuids of users in groups having access to all collections - let has_full_access_via_group = if CONFIG.org_groups_enabled() { - GroupUser::get_members_of_full_access_groups(org_id, &mut conn).await - } else { - vec![] - }; - let has_full_access = user_org.access_all || has_full_access_via_group.contains(&user_org.uuid); + // check if current user has full access to the organization (either directly or via any group) + let has_full_access_via_group = + CONFIG.org_groups_enabled() && GroupUser::has_full_access_by_member(org_id, &user_org.uuid, &mut conn).await; + let has_full_access_to_org = user_org.access_all || has_full_access_via_group; for col in Collection::find_by_organization(org_id, &mut conn).await { - let groups: Vec = if CONFIG.org_groups_enabled() { - CollectionGroup::find_by_collection(&col.uuid, &mut conn) - .await - .iter() - .map(|collection_group| { - SelectionReadOnly::to_collection_group_details_read_only(collection_group).to_json() - }) - .collect() - } else { - // The Bitwarden clients seem to call this API regardless of whether groups are enabled, - // so just act as if there are no groups. - Vec::with_capacity(0) - }; + // assigned indicates whether the current user has access to the given collection + let mut assigned = has_full_access_to_org; - let mut assigned = has_full_access; + // get the users assigned directly to the given collection let users: Vec = coll_users .iter() .filter(|collection_user| collection_user.collection_uuid == col.uuid) .map(|collection_user| { - // Remember `user_uuid` is swapped here with the `user_org.uuid` with a join during the `CollectionUser::find_by_organization` call. - // We check here if the current user is assigned to this collection or not. + // check if the current user is assigned to this collection directly if collection_user.user_uuid == user_org.uuid { assigned = true; } @@ -360,12 +346,23 @@ async fn get_org_collections_details(org_id: &str, headers: ManagerHeadersLoose, }) .collect(); - // if the current user is not assigned and groups are enabled, - // check if they have access to the given collection via a group - if !assigned && CONFIG.org_groups_enabled() - { - assigned = GroupUser::get_group_members_for_collection(&col.uuid, &mut conn).await.contains(&user_org.uuid); - } + // check if the current user has access to the given collection via a group + if !assigned && CONFIG.org_groups_enabled() { + assigned = GroupUser::has_access_to_collection_by_member(&col.uuid, &user_org.uuid, &mut conn).await; + } + + // get the group details for the given collection + let groups: Vec = if CONFIG.org_groups_enabled() { + CollectionGroup::find_by_collection(&col.uuid, &mut conn) + .await + .iter() + .map(|collection_group| { + SelectionReadOnly::to_collection_group_details_read_only(collection_group).to_json() + }) + .collect() + } else { + Vec::with_capacity(0) + }; let mut json_object = col.to_json(); json_object["Assigned"] = json!(assigned); diff --git a/src/db/models/group.rs b/src/db/models/group.rs index 01e8911cc81..e50853e2453 100644 --- a/src/db/models/group.rs +++ b/src/db/models/group.rs @@ -486,23 +486,25 @@ impl GroupUser { }} } - pub async fn get_group_members_for_collection(collection_uuid: &str, conn: &mut DbConn) -> Vec { + pub async fn has_access_to_collection_by_member( + collection_uuid: &str, + member_uuid: &str, + conn: &mut DbConn, + ) -> bool { db_run! { conn: { groups_users::table .inner_join(collections_groups::table.on( collections_groups::groups_uuid.eq(groups_users::groups_uuid) )) .filter(collections_groups::collections_uuid.eq(collection_uuid)) - .select(groups_users::users_organizations_uuid) - .distinct() - .load::(conn) - .expect("Error loading group users for collection") + .filter(groups_users::users_organizations_uuid.eq(member_uuid)) + .count() + .first::(conn) + .unwrap_or(0) != 0 }} - .into_iter() - .collect() } - pub async fn get_members_of_full_access_groups(org_uuid: &str, conn: &mut DbConn) -> Vec { + pub async fn has_full_access_by_member(org_uuid: &str, member_uuid: &str, conn: &mut DbConn) -> bool { db_run! { conn: { groups_users::table .inner_join(groups::table.on( @@ -510,13 +512,11 @@ impl GroupUser { )) .filter(groups::organizations_uuid.eq(org_uuid)) .filter(groups::access_all.eq(true)) - .select(groups_users::users_organizations_uuid) - .distinct() - .load::(conn) - .expect("Error loading all access group users for organization") + .filter(groups_users::users_organizations_uuid.eq(member_uuid)) + .count() + .first::(conn) + .unwrap_or(0) != 0 }} - .into_iter() - .collect() } pub async fn update_user_revision(&self, conn: &mut DbConn) {