improve access to collections via groups (#4441)

* refactor get_org_collections_details

* improve access to collection check

* fix get_org_collection_detail too

src/api/core/organizations.rs

@@ -329,27 +329,19 @@ async fn get_org_collections_details(org_id: &str, headers: ManagerHeadersLoose,
             && GroupUser::has_full_access_by_member(org_id, &user_org.uuid, &mut conn).await);
 
     for col in Collection::find_by_organization(org_id, &mut conn).await {
-        // assigned indicates whether the current user has access to the given collection
+        // check whether the current user has access to the given collection
-        let mut assigned = has_full_access_to_org;
+        let assigned = has_full_access_to_org
+            || CollectionUser::has_access_to_collection_by_user(&col.uuid, &user_org.user_uuid, &mut conn).await
+            || (CONFIG.org_groups_enabled()
+                && GroupUser::has_access_to_collection_by_member(&col.uuid, &user_org.uuid, &mut conn).await);
 
         // get the users assigned directly to the given collection
         let users: Vec<Value> = coll_users
             .iter()
             .filter(|collection_user| collection_user.collection_uuid == col.uuid)
-            .map(|collection_user| {
+            .map(|collection_user| SelectionReadOnly::to_collection_user_details_read_only(collection_user).to_json())
-                // check if the current user is assigned to this collection directly
-                if collection_user.user_uuid == user_org.uuid {
-                    assigned = true;
-                }
-                SelectionReadOnly::to_collection_user_details_read_only(collection_user).to_json()
-            })
             .collect();
 
-        // 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<Value> = if CONFIG.org_groups_enabled() {
             CollectionGroup::find_by_collection(&col.uuid, &mut conn)
@@ -672,24 +664,16 @@ async fn get_org_collection_detail(
                 Vec::with_capacity(0)
             };
 
-            let mut assigned = false;
             let users: Vec<Value> =
                 CollectionUser::find_by_collection_swap_user_uuid_with_org_user_uuid(&collection.uuid, &mut conn)
                     .await
                     .iter()
                     .map(|collection_user| {
-                        // Remember `user_uuid` is swapped here with the `user_org.uuid` with a join during the `find_by_collection_swap_user_uuid_with_org_user_uuid` call.
-                        // We check here if the current user is assigned to this collection or not.
-                        if collection_user.user_uuid == user_org.uuid {
-                            assigned = true;
-                        }
                         SelectionReadOnly::to_collection_user_details_read_only(collection_user).to_json()
                     })
                     .collect();
 
-            if user_org.access_all {
+            let assigned = Collection::can_access_collection(&user_org, &collection.uuid, &mut conn).await;
-                assigned = true;
-            }
 
             let mut json_object = collection.to_json();
             json_object["Assigned"] = json!(assigned);

src/auth.rs

@@ -689,7 +689,7 @@ impl<'r> FromRequest<'r> for ManagerHeaders {
                         _ => err_handler!("Error getting DB"),
                     };
 
-                    if !can_access_collection(&headers.org_user, &col_id, &mut conn).await {
+                    if !Collection::can_access_collection(&headers.org_user, &col_id, &mut conn).await {
                         err_handler!("The current user isn't a manager for this collection")
                     }
                 }
@@ -762,10 +762,6 @@ impl From<ManagerHeadersLoose> for Headers {
         }
     }
 }
-async fn can_access_collection(org_user: &UserOrganization, col_id: &str, conn: &mut DbConn) -> bool {
-    org_user.has_full_access()
-        || Collection::has_access_by_collection_and_user_uuid(col_id, &org_user.user_uuid, conn).await
-}
 
 impl ManagerHeaders {
     pub async fn from_loose(
@@ -777,7 +773,7 @@ impl ManagerHeaders {
             if uuid::Uuid::parse_str(col_id).is_err() {
                 err!("Collection Id is malformed!");
             }
-            if !can_access_collection(&h.org_user, col_id, conn).await {
+            if !Collection::can_access_collection(&h.org_user, col_id, conn).await {
                 err!("You don't have access to all collections!");
             }
         }

src/db/models/collection.rs

@@ -1,6 +1,6 @@
 use serde_json::Value;
 
-use super::{CollectionGroup, User, UserOrgStatus, UserOrgType, UserOrganization};
+use super::{CollectionGroup, GroupUser, User, UserOrgStatus, UserOrgType, UserOrganization};
 use crate::CONFIG;
 
 db_object! {
@@ -102,6 +102,15 @@ impl Collection {
         json_object["HidePasswords"] = json!(hide_passwords);
         json_object
     }
+
+    pub async fn can_access_collection(org_user: &UserOrganization, col_id: &str, conn: &mut DbConn) -> bool {
+        org_user.has_status(UserOrgStatus::Confirmed)
+            && (org_user.has_full_access()
+                || CollectionUser::has_access_to_collection_by_user(col_id, &org_user.user_uuid, conn).await
+                || (CONFIG.org_groups_enabled()
+                    && (GroupUser::has_full_access_by_member(&org_user.org_uuid, &org_user.uuid, conn).await
+                        || GroupUser::has_access_to_collection_by_member(col_id, &org_user.uuid, conn).await)))
+    }
 }
 
 use crate::db::DbConn;
@@ -252,17 +261,6 @@ impl Collection {
         }
     }
 
-    // Check if a user has access to a specific collection
-    // FIXME: This needs to be reviewed. The query used by `find_by_user_uuid` could be adjusted to filter when needed.
-    //        For now this is a good solution without making to much changes.
-    pub async fn has_access_by_collection_and_user_uuid(
-        collection_uuid: &str,
-        user_uuid: &str,
-        conn: &mut DbConn,
-    ) -> bool {
-        Self::find_by_user_uuid(user_uuid.to_owned(), conn).await.into_iter().any(|c| c.uuid == collection_uuid)
-    }
-
     pub async fn find_by_organization_and_user_uuid(org_uuid: &str, user_uuid: &str, conn: &mut DbConn) -> Vec<Self> {
         Self::find_by_user_uuid(user_uuid.to_owned(), conn)
             .await
@@ -644,6 +642,10 @@ impl CollectionUser {
             Ok(())
         }}
     }
+
+    pub async fn has_access_to_collection_by_user(col_id: &str, user_uuid: &str, conn: &mut DbConn) -> bool {
+        Self::find_by_collection_and_user(col_id, user_uuid, conn).await.is_some()
+    }
 }
 
 /// Database methods