Added api/collection/{tree} flag to remove other users' collections from results (#28204)

Added post-collection retrieval function `remove-other-users-personal-collections` that will filter out any personal collection that doesn't belong to the current user when the `exclude-other-user-collections=true` path param is used. Also added unit tests for both the `api/collection/` and `api/collection/tree` endpoint.

.cljfmt/indents.clj

@@ -0,0 +1,2 @@
+{metabase.api.collection/foo [[:inner 0]]
+ toucan.db/insert!           [[:block 1]]}

.pre-commit-config.yaml

@@ -0,0 +1,14 @@
+repos:
+  -   repo: https://github.com/clj-kondo/clj-kondo
+      rev: v2022.12.10
+      hooks:
+        - id: clj-kondo
+#  - repo: https://github.com/allie-jo/ns-sort.git
+#    rev: v1.1.3
+#    hooks:
+#     - id: ns-sort-docker
+#  - repo: https://github.com/allie-jo/cljfmt-docker
+#    rev: v0.9.1
+#    hooks:
+#      - id: cljfmt-docker
+#        args: [--indents 2]

bin/cljfmt.sh

@@ -0,0 +1,7 @@
+#! /usr/bin/env bash
+
+set -euxo pipefail
+
+clojure -M:cljfmt \
+--indents ./.cljfmt/indents.clj \
+src/metabase/api/collection.clj

frontend/src/metabase/collections/containers/CollectionContent.jsx

@@ -318,7 +318,11 @@ function CollectionContent({
 export default _.compose(
   Bookmark.loadList(),
   Collection.loadList({
-    query: () => ({ tree: true }),
+    query: {
+      tree: true,
+      "exclude-other-user-collections": true,
+      "exclude-archived": true,
+    },
     loadingAndErrorWrapper: false,
   }),
   Collection.load({

frontend/src/metabase/containers/DataPicker/CardPicker/CardPickerContainer.tsx

@@ -152,7 +152,7 @@ export default _.compose(
     loadingAndErrorWrapper: false,
   }),
   Collections.loadList({
-    query: () => ({ tree: true }),
+    query: () => ({ tree: true, "exclude-archived": true }),
     listName: "collectionTree",
   }),
   Collections.loadList({

frontend/src/metabase/entities/questions.js

@@ -51,7 +51,13 @@ const Questions = createEntity({
           ),
         );
         dispatch(
-          Collections.actions.fetchList({ tree: true }, { reload: true }),
+          Collections.actions.fetchList(
+            {
+              tree: true,
+              "exclude-archived": true,
+            },
+            { reload: true },
+          ),
         );
 
         const card = result?.payload?.question;

frontend/src/metabase/nav/containers/MainNavbar/MainNavbarContainer/MainNavbarContainer.tsx

@@ -189,7 +189,11 @@ export default _.compose(
     loadingAndErrorWrapper: false,
   }),
   Collections.loadList({
-    query: () => ({ tree: true, "exclude-archived": true }),
+    query: () => ({
+      tree: true,
+      "exclude-other-user-collections": true,
+      "exclude-archived": true,
+    }),
     loadingAndErrorWrapper: false,
   }),
   Databases.loadList({

frontend/src/metabase/query_builder/components/DataSelector/saved-question-picker/SavedQuestionPicker.jsx

@@ -152,7 +152,7 @@ export default _.compose(
     loadingAndErrorWrapper: false,
   }),
   Collection.loadList({
-    query: () => ({ tree: true }),
+    query: () => ({ tree: true, "exclude-archived": true }),
   }),
   connect(mapStateToProps),
 )(SavedQuestionPicker);

src/metabase/api/collection.clj

@@ -45,17 +45,36 @@
 
 (declare root-collection)
 
+(defn- remove-other-users-personal-collections
+  [user-id collections]
+  (let [personal-ids (into #{} (comp (filter :personal_owner_id)
+                                     (remove (comp #{user-id} :personal_owner_id))
+                                     (map :id))
+                           collections)
+        prefixes     (into #{} (map (fn [id] (format "/%d/" id))) personal-ids)
+        personal?    (fn [{^String location :location id :id}]
+                       (or (personal-ids id)
+                           (prefixes (re-find #"^/\d+/" location))))]
+    (if (seq prefixes)
+      (remove personal? collections)
+      collections)))
+
 #_{:clj-kondo/ignore [:deprecated-var]}
 (api/defendpoint-schema GET "/"
   "Fetch a list of all Collections that the current user has read permissions for (`:can_write` is returned as an
   additional property of each Collection so you can tell which of these you have write permissions for.)
 
   By default, this returns non-archived Collections, but instead you can show archived ones by passing
-  `?archived=true`."
+  `?archived=true`.
-  [archived namespace]
+
-  {archived  (s/maybe su/BooleanString)
+  By default, admin users will see all collections. To hide other user's collections pass in
+  `?exclude-other-user-collections=true`."
+  [archived exclude-other-user-collections namespace]
+  {archived                       (s/maybe su/BooleanString)
+   exclude-other-user-collections (s/maybe su/BooleanString)
    namespace (s/maybe su/NonBlankString)}
-  (let [archived? (Boolean/parseBoolean archived)]
+  (let [archived? (Boolean/parseBoolean archived)
+        exclude-other-user-collections? (Boolean/parseBoolean exclude-other-user-collections)]
     (as-> (db/select Collection
             {:where    [:and
                         [:= :archived archived?]
@@ -64,7 +83,11 @@
                          :id
                          (collection/permissions-set->visible-collection-ids @api/*current-user-permissions-set*))]
              :order-by [[:%lower.name :asc]]}) collections
-      ;; include Root Collection at beginning or results if archived isn't `true`
+          ;; Remove other users' personal collections
+          (if exclude-other-user-collections?
+            (remove-other-users-personal-collections api/*current-user-id* collections)
+            collections)
+          ;; include Root Collection at beginning or results if archived isn't `true`
       (if archived?
         collections
         (let [root (root-collection namespace)]
@@ -101,26 +124,59 @@
 
   The here and below keys indicate the types of items at this particular level of the tree (here) and in its
   subtree (below)."
-  [exclude-archived namespace]
+  [exclude-archived exclude-other-user-collections namespace]
   {exclude-archived (s/maybe su/BooleanString)
+   exclude-other-user-collections (s/maybe su/BooleanString)
    namespace        (s/maybe su/NonBlankString)}
-  (let [coll-type-ids (reduce (fn [acc {:keys [collection_id dataset] :as _x}]
+  (let [exclude-archived? (Boolean/parseBoolean exclude-archived)
+        exclude-other-user-collections? (Boolean/parseBoolean exclude-other-user-collections)
+        coll-type-ids (reduce (fn [acc {:keys [collection_id dataset] :as _x}]
                                 (update acc (if dataset :dataset :card) conj collection_id))
                               {:dataset #{}
                                :card    #{}}
                               (mdb.query/reducible-query {:select-distinct [:collection_id :dataset]
                                                           :from            [:report_card]
                                                           :where           [:= :archived false]}))
-        colls         (db/select Collection
+        colls (cond->>
-                        {:where [:and
+                (db/select Collection
-                                 (when exclude-archived
+                  {:where [:and
-                                   [:= :archived false])
+                           (when exclude-archived?
-                                 [:= :namespace namespace]
+                             [:= :archived false])
-                                 (collection/visible-collection-ids->honeysql-filter-clause
+                           [:= :namespace namespace]
-                                  :id
+                           (collection/visible-collection-ids->honeysql-filter-clause
-                                  (collection/permissions-set->visible-collection-ids @api/*current-user-permissions-set*))]})
+                            :id
-        colls         (map collection/personal-collection-with-ui-details colls)]
+                            (collection/permissions-set->visible-collection-ids @api/*current-user-permissions-set*))]})
-    (collection/collections->tree coll-type-ids colls)))
+                exclude-other-user-collections?
+                (remove-other-users-personal-collections api/*current-user-id*))
+        colls-with-details (map collection/personal-collection-with-ui-details colls)]
+    (collection/collections->tree coll-type-ids colls-with-details)))
+
+(comment
+  (binding [api/*current-user-permissions-set* (delay #{"/"})
+            api/*current-user-id* 1]
+    (time
+     (let [exclude-archived?               false
+           exclude-other-user-collections? true
+           coll-type-ids                   (reduce (fn [acc {:keys [collection_id dataset] :as _x}]
+                                                     (update acc (if dataset :dataset :card) conj collection_id))
+                                                   {:dataset #{}
+                                                    :card    #{}}
+                                                   (mdb.query/reducible-query {:select-distinct [:collection_id :dataset]
+                                                                               :from            [:report_card]
+                                                                               :where           [:= :archived false]}))
+           colls                           (cond->>
+                                             (db/select Collection
+                                               {:where [:and
+                                                        (when exclude-archived?
+                                                          [:= :archived false])
+                                                        [:= :namespace nil]
+                                                        (collection/visible-collection-ids->honeysql-filter-clause
+                                                         :id
+                                                         (collection/permissions-set->visible-collection-ids @api/*current-user-permissions-set*))]})
+                                             exclude-other-user-collections?
+                                             (remove-other-users-personal-collections api/*current-user-id*))
+           colls-with-details              (map collection/personal-collection-with-ui-details colls)]
+       (collection/collections->tree coll-type-ids colls-with-details)))))
 
 ;;; --------------------------------- Fetching a single Collection & its 'children' ----------------------------------
 

test/metabase/api/collection_test.clj

@@ -34,7 +34,8 @@
    [metabase.util :as u]
    [metabase.util.schema :as su]
    [schema.core :as s]
-   [toucan.db :as db])
+   [toucan.db :as db]
+   [toucan2.tools.with-temp :as t2.with-temp])
   (:import
    (java.time ZonedDateTime ZoneId)))
 
@@ -103,10 +104,41 @@
                 "Rasta Toucan's Personal Collection"
                 "Trash Bird's Personal Collection"]
                (->> (mt/user-http-request :crowberto :get 200 "collection")
+                    (filter #((set (map mt/user->id [:crowberto :lucky :rasta :trashbird])) (:personal_owner_id %)))
+                    (map :name)
+                    sort))))
+
+      (testing "...or we are *admins* but exclude other user's collections"
+        (is (= ["Crowberto Corv's Personal Collection"]
+               (->> (mt/user-http-request :crowberto :get 200 "collection" :exclude-other-user-collections true)
                     (filter #((set (map mt/user->id [:crowberto :lucky :rasta :trashbird])) (:personal_owner_id %)))
                     (map :name)
                     sort)))))
 
+    (testing "You should only see your collection and public collections"
+      (let [admin-user-id  (u/the-id (test.users/fetch-user :crowberto))
+            crowberto-root (db/select-one Collection :personal_owner_id admin-user-id)]
+        (t2.with-temp/with-temp [Collection collection          {}
+                                 Collection {collection-id :id} {:name "Collection with Items"}
+                                 Collection _                   {:name            "subcollection"
+                                                                 :location        (format "/%d/" collection-id)
+                                                                 :authority_level "official"}
+                                 Collection _                   {:name     "Crowberto's Child Collection"
+                                                                 :location (collection/location-path crowberto-root)}]
+                                (let [public-collections       #{"Our analytics" (:name collection) "Collection with Items" "subcollection"}
+                                      crowbertos               (set (map :name (mt/user-http-request :crowberto :get 200 "collection")))
+                                      crowbertos-with-excludes (set (map :name (mt/user-http-request :crowberto :get 200 "collection" :exclude-other-user-collections true)))
+                                      luckys                   (set (map :name (mt/user-http-request :lucky :get 200 "collection")))]
+                                  (is (= (into (set (map :name (db/select Collection))) public-collections)
+                                         crowbertos))
+                                  (is (= (into public-collections #{"Crowberto Corv's Personal Collection" "Crowberto's Child Collection"})
+                                         crowbertos-with-excludes))
+                                  (is (true? (contains? crowbertos "Lucky Pigeon's Personal Collection")))
+                                  (is (false? (contains? crowbertos-with-excludes "Lucky Pigeon's Personal Collection")))
+                                  (is (= (conj public-collections (:name collection) "Lucky Pigeon's Personal Collection")
+                                         luckys))
+                                  (is (false? (contains? luckys "Crowberto Corv's Personal Collection")))))))
+
     (testing "Personal Collection's name and slug should be returned in user's locale"
       (with-french-user-and-personal-collection user _collection
         (is (= [{:name "Collection personnelle de Taco Bell"
@@ -117,8 +149,8 @@
 
     (testing "check that we don't see collections if we don't have permissions for them"
       (mt/with-non-admin-groups-no-root-collection-perms
-        (mt/with-temp* [Collection [collection-1  {:name "Collection 1"}]
+        (mt/with-temp* [Collection [collection-1 {:name "Collection 1"}]
-                        Collection [_             {:name "Collection 2"}]]
+                        Collection [_ {:name "Collection 2"}]]
           (perms/grant-collection-read-permissions! (perms-group/all-users) collection-1)
           (is (= ["Collection 1"
                   "Rasta Toucan's Personal Collection"]
@@ -240,7 +272,47 @@
                 response (mt/user-http-request :rasta :get 200
                                                "collection/tree?exclude-archived=true")]
             (is (= [{:name "A" :children []}]
-                   (collection-tree-view ids response)))))))
+                   (collection-tree-view ids response))))))
+      (testing "Excludes other user collections"
+        (let [admin-collection (collection/user->personal-collection (mt/user->id :crowberto))
+              lucky-collection (collection/user->personal-collection (mt/user->id :lucky))]
+          (mt/with-temp* [Collection [ac {:name "Admin Child" :location (collection/location-path admin-collection)}]
+                          Collection [lc {:name "Lucky Child" :location (collection/location-path lucky-collection)}]
+                          Collection [a {:name "A"}]
+                          Collection [b {:name     "B"
+                                         :location (collection/location-path a)}]
+                          Collection [c {:name "C"}]]
+            (let [ids                   (set (map :id [admin-collection lucky-collection ac lc a b c]))
+                  admin-response        (mt/user-http-request :crowberto :get 200
+                                                              "collection/tree")
+                  admin-response-ex     (mt/user-http-request :crowberto :get 200
+                                                              "collection/tree?exclude-other-user-collections=true")
+                  non-admin-response    (mt/user-http-request :lucky :get 200
+                                                              "collection/tree")
+                  non-admin-response-ex (mt/user-http-request :lucky :get 200
+                                                              "collection/tree?exclude-other-user-collections=true")]
+              ;; By default, our admin can see everything
+              (is (= [{:name "A", :children [{:name "B", :children []}]}
+                      {:name "C", :children []}
+                      {:name "Crowberto Corv's Personal Collection", :children [{:name "Admin Child", :children []}]}
+                      {:name "Lucky Pigeon's Personal Collection", :children [{:name "Lucky Child", :children []}]}]
+                     (collection-tree-view ids admin-response)))
+              ;; When excluding other user collections, the admin only sees their own collections and shared collections
+              (is (=
+                   [{:name "A", :children [{:name "B", :children []}]}
+                    {:name "C", :children []}
+                    {:name "Crowberto Corv's Personal Collection", :children [{:name "Admin Child", :children []}]}]
+                   (collection-tree-view ids admin-response-ex)))
+              ;; A non admin only sees their own collections without the flag...
+              (is (= [{:name "A", :children [{:name "B", :children []}]}
+                      {:name "C", :children []}
+                      {:name "Lucky Pigeon's Personal Collection", :children [{:name "Lucky Child", :children []}]}]
+                     (collection-tree-view ids non-admin-response)))
+              ;; ...as well as with the flag
+              (is (= [{:name "A", :children [{:name "B", :children []}]}
+                      {:name "C", :children []}
+                      {:name "Lucky Pigeon's Personal Collection", :children [{:name "Lucky Child", :children []}]}]
+                     (collection-tree-view ids non-admin-response-ex))))))))
 
     (testing "for personal collections, it should return name and slug in user's locale"
       (with-french-user-and-personal-collection user collection
@@ -274,7 +346,11 @@
           (perms/grant-collection-readwrite-permissions! (perms-group/all-users) child-collection)
           (is (= [{:name "Child", :children []}]
                  (collection-tree-view (map :id [parent-collection child-collection])
-                                       (mt/user-http-request :rasta :get 200 "collection/tree")))))))
+                                       (mt/user-http-request :rasta :get 200 "collection/tree"))))
+          (is (= [{:name "Child", :children []}]
+                 (collection-tree-view (map :id [parent-collection child-collection])
+                                       (mt/user-http-request :rasta :get 200 "collection/tree"
+                                                             :exclude-other-user-collections true)))))))
 
     (testing "Namespace parameter"
       (mt/with-temp* [Collection [{normal-id :id} {:name "Normal Collection"}]