Fix #14114 (#14251)

* Fix #14114

* Test fix 

.dir-locals.el

@@ -26,7 +26,8 @@
                               (p.types/def-abstract-type '(1 (:defn)))
                               (p.types/deftype+ '(2 nil nil (:defn)))
                               (p/def-map-type '(2 nil nil (:defn)))
-                              (p.types/defrecord+ '(2 nil nil (:defn))))))
+                              (p.types/defrecord+ '(2 nil nil (:defn)))
+                              (tools.macro/macrolet '(1 (:defn))))))
                   ;; if you're using clj-refactor (highly recommended!), prefer prefix notation when cleaning the ns form
                   (cljr-favor-prefix-notation . t)
                   ;; prefer keeping source width about ~118, GitHub seems to cut off stuff at either 119 or 120 and

frontend/test/metabase/scenarios/collections/collections.cy.spec.js

@@ -289,7 +289,7 @@ describe("scenarios > collection_defaults", () => {
       signInAsAdmin();
     });
 
-    it.skip("should see a child collection in a sidebar even with revoked access to its parent (metabase#14114)", () => {
+    it("should see a child collection in a sidebar even with revoked access to its parent (metabase#14114)", () => {
       // Create Parent collection within `Our analytics`
       cy.request("POST", "/api/collection", {
         name: "Parent",

src/metabase/models/collection.clj

@@ -1004,32 +1004,39 @@
                               :children [{:name \"G\"}]}]}]}
      {:name \"H\"}]"
   [collections]
-  (transduce
-   identity
-   (fn ->tree
-     ;; 1. We'll use a map representation to start off with to make building the tree easier. Keyed by Collection ID
-     ;; e.g.
-     ;;
-     ;; {1 {:name "A"
-     ;;     :children {2 {:name "B"}, ...}}}
-     ([] {})
-     ;; 2. For each as we come across it, put it in the correct location in the tree. Convert it's `:location` (e.g.
-     ;; `/1/`) plus its ID to a key path e.g. `[1 :children 2]`
-     ([m collection]
-      (let [path (interpose :children (concat (location-path->ids (:location collection))
-                                              [(:id collection)]))]
-        (assoc-in m path collection)))
-     ;; 3. Once we've build the entire tree structure, go in and convert each ID->Collection map into a flat sequence,
-     ;; sorted by the lowercased Collection name. Do this recursively for the `:children` of each Collection e.g.
-     ;;
-     ;; {1 {:name "A"
-     ;;     :children {2 {:name "B"}, ...}}}
-     ;; ->
-     ;; [{:name "A"
-     ;;   :children [{:name "B"}, ...]}]
-     ([m]
-      (let [vs (for [v (vals m)]
-                 (cond-> v
-                   (:children v) (update :children ->tree)))]
-        (sort-by (comp (fnil u/lower-case-en "") :name) vs))))
-   collections))
+  (let [all-visible-ids (set (map :id collections))]
+    (transduce
+     identity
+     (fn ->tree
+       ;; 1. We'll use a map representation to start off with to make building the tree easier. Keyed by Collection ID
+       ;; e.g.
+       ;;
+       ;; {1 {:name "A"
+       ;;     :children {2 {:name "B"}, ...}}}
+       ([] {})
+       ;; 2. For each as we come across it, put it in the correct location in the tree. Convert it's `:location` (e.g.
+       ;; `/1/`) plus its ID to a key path e.g. `[1 :children 2]`
+       ;;
+       ;; If any ancestor Collections are not present in `collections`, just remove their IDs from the path,
+       ;; effectively "pulling" a Collection up to a higher level. e.g. if we have A > B > C and we can't see B then
+       ;; the tree should come back as A > C.
+       ([m collection]
+        (let [path (as-> (location-path->ids (:location collection)) ids
+                     (filter all-visible-ids ids)
+                     (concat ids [(:id collection)])
+                     (interpose :children ids))]
+          (assoc-in m path collection)))
+       ;; 3. Once we've build the entire tree structure, go in and convert each ID->Collection map into a flat sequence,
+       ;; sorted by the lowercased Collection name. Do this recursively for the `:children` of each Collection e.g.
+       ;;
+       ;; {1 {:name "A"
+       ;;     :children {2 {:name "B"}, ...}}}
+       ;; ->
+       ;; [{:name "A"
+       ;;   :children [{:name "B"}, ...]}]
+       ([m]
+        (let [vs (for [v (vals m)]
+                   (cond-> v
+                     (:children v) (update :children ->tree)))]
+          (sort-by (comp (fnil u/lower-case-en "") :name) vs))))
+     collections)))

test/metabase/api/collection_test.clj

@@ -4,7 +4,7 @@
              [string :as str]
              [test :refer :all]]
             [metabase
-             [models :refer [Card Collection Dashboard DashboardCard NativeQuerySnippet Permissions PermissionsGroup
+             [models :refer [Card Collection Dashboard DashboardCard NativeQuerySnippet PermissionsGroup
                              PermissionsGroupMembership Pulse PulseCard PulseChannel PulseChannelRecipient]]
              [test :as mt]
              [util :as u]]
@@ -119,6 +119,15 @@
 ;;; |                                              GET /collection/tree                                              |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
+(defn- collection-tree-names-only
+  "Keep just the names of Collections in `collection-ids-to-keep` in the response returned by the Collection tree
+  endpoint."
+  [collection-ids-to-keep collections]
+  (for [collection collections
+        :when      (contains? (set collection-ids-to-keep) (:id collection))]
+    (cond-> (select-keys collection [:name :children])
+      (:children collection) (update :children (partial collection-tree-names-only collection-ids-to-keep)))))
+
 (deftest collection-tree-test
   (testing "GET /api/collection/tree"
     (with-collection-hierarchy [a b c d e f g]
@@ -126,21 +135,15 @@
                                          [a b c d e f g])))
             response (mt/user-http-request :rasta :get 200 "collection/tree")]
         (testing "Make sure overall tree shape of the response is as is expected"
-          (letfn [(collection-names [collections]
-                    (for [collection collections
-                          :when      (contains? ids (:id collection))]
-                      (cond-> (select-keys collection [:name :children])
-                        (:children collection) (update :children collection-names))))]
-            (testing "GET /api/collection/tree"
-              (is (= [{:name     "A"
-                       :children [{:name "B"}
-                                  {:name     "C"
-                                   :children [{:name     "D"
-                                               :children [{:name "E"}]}
-                                              {:name     "F"
-                                               :children [{:name "G"}]}]}]}
-                      {:name "Rasta Toucan's Personal Collection"}]
-                     (collection-names response))))))
+          (is (= [{:name     "A"
+                   :children [{:name "B"}
+                              {:name     "C"
+                               :children [{:name     "D"
+                                           :children [{:name "E"}]}
+                                          {:name     "F"
+                                           :children [{:name "G"}]}]}]}
+                  {:name "Rasta Toucan's Personal Collection"}]
+                 (collection-tree-names-only ids response))))
         (testing "Make sure each Collection comes back with the expected keys"
           (is (= {:description       nil
                   :archived          false
@@ -155,6 +158,24 @@
                           %)
                        response))))))))
 
+(deftest collection-tree-child-permissions-test
+  (testing "GET /api/collection/tree"
+    (testing "Tree endpoint should still return Collections if we don't have perms for the parent Collection (#14114)"
+      ;; Create a hierarchy like:
+      ;;
+      ;; + Our analytics (Revoke permissions to All Users)
+      ;; +--+ Parent collection (Revoke permissions to All Users)
+      ;;    +--+ Child collection (Give All Users group Curate access)
+      (mt/with-non-admin-groups-no-root-collection-perms
+        (mt/with-temp* [Collection [parent-collection {:name "Parent"}]
+                        Collection [child-collection  {:name "Child", :location (format "/%d/" (:id parent-collection))}]]
+          (perms/revoke-collection-permissions! (group/all-users) parent-collection)
+          (perms/grant-collection-readwrite-permissions! (group/all-users) child-collection)
+          (is (= [{:name "Child"}]
+                 (collection-tree-names-only (map :id [parent-collection child-collection])
+                                             (mt/user-http-request :rasta :get 200 "collection/tree")))))))))
+
+
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                              GET /collection/:id                                               |
 ;;; +----------------------------------------------------------------------------------------------------------------+