Make sure you cannot edit perms for descendants of Personal Collections

src/metabase/models/collection.clj

@@ -823,13 +823,31 @@
    (for [collection-id collection-ids]
      {collection-id (perms-type-for-collection permissions-set collection-id)})))
 
+(s/defn ^:private non-personal-collection-ids :- #{su/IntGreaterThanZero}
+  "Return a set of IDs of all Collections that are neither Personal Collections nor descendants of Personal
+  Collections (i.e., things that you can set Permissions for, and that should go in the graph.)"
+  []
+  (->> (db/query
+        (merge
+         {:select [[:id :id]]
+          :from   [Collection]}
+         (when-let [personal-collection-ids (seq (db/select-ids Collection :personal_owner_id [:not= nil]))]
+           {:where (apply
+                    vector
+                    :and
+                    [:not-in :id personal-collection-ids]
+                    (for [id personal-collection-ids]
+                      [:not [:like :location (format "/%d/%%" id)]]))})))
+       (map :id)
+       set))
+
 (s/defn graph :- PermissionsGraph
   "Fetch a graph representing the current permissions status for every group and all permissioned collections. This
   works just like the function of the same name in `metabase.models.permissions`; see also the documentation for that
   function."
   []
   (let [group-id->perms (group-id->permissions-set)
-        collection-ids  (db/select-ids 'Collection, :personal_owner_id nil)] ; exclude personal collections!
+        collection-ids  (non-personal-collection-ids)]
     {:revision (collection-revision/latest-id)
      :groups   (into {} (for [group-id (db/select-ids 'PermissionsGroup)]
                           {group-id (group-permissions-graph (group-id->perms group-id) collection-ids)}))}))

src/metabase/models/permissions.clj

@@ -427,7 +427,7 @@
   [group-id :- su/IntGreaterThanZero, database-id :- su/IntGreaterThanZero]
   (grant-permissions! group-id (object-path database-id)))
 
-(defn- check-not-personal-collection
+(defn- check-not-personal-collection-or-descendant
   "Check whether `collection-or-id` refers to a Personal Collection; if so, throw an Exception. This is done because we
   *should* never be editing granting/etc. permissions for *Personal* Collections to entire Groups! Their owner will
   get implicit permissions automatically, and of course admins will be able to see them,but a whole group should never
@@ -436,25 +436,28 @@
   ;; don't apply this check to the Root Collection, because it's never personal
   (when-not (:metabase.models.collection/is-root? collection-or-id)
     ;; ok, once we've confirmed this isn't the Root Collection, see if it's in the DB with a personal_owner_id
-    (when (db/exists? 'Collection :id (u/get-id collection-or-id), :personal_owner_id [:not= nil])
-      (throw (Exception. (str (tru "You cannot edit permissions for a Personal Collection.")))))))
+    (when ((resolve 'metabase.models.collection/is-personal-collection-or-descendant-of-one?)
+           (if (map? collection-or-id)
+             collection-or-id
+             (db/select-one 'Collection :id (u/get-id collection-or-id))))
+      (throw (Exception. (str (tru "You cannot edit permissions for a Personal Collection or its descendants.")))))))
 
 (defn revoke-collection-permissions!
   "Revoke all access for `group-or-id` to a Collection."
   [group-or-id collection-or-id]
-  (check-not-personal-collection collection-or-id)
+  (check-not-personal-collection-or-descendant collection-or-id)
   (delete-related-permissions! group-or-id (collection-readwrite-path collection-or-id)))
 
 (defn grant-collection-readwrite-permissions!
   "Grant full access to a Collection, which means a user can view all Cards in the Collection and add/remove Cards."
   [group-or-id collection-or-id]
-  (check-not-personal-collection collection-or-id)
+  (check-not-personal-collection-or-descendant collection-or-id)
   (grant-permissions! (u/get-id group-or-id) (collection-readwrite-path collection-or-id)))
 
 (defn grant-collection-read-permissions!
   "Grant read access to a Collection, which means a user can view all Cards in the Collection."
   [group-or-id collection-or-id]
-  (check-not-personal-collection collection-or-id)
+  (check-not-personal-collection-or-descendant collection-or-id)
   (grant-permissions! (u/get-id group-or-id) (collection-read-path collection-or-id)))
 
 

test/metabase/models/collection_test.clj

@@ -25,6 +25,9 @@
   (doseq [username [:rasta :lucky :crowberto :trashbird]]
     (collection/user->personal-collection (test-users/user->id username))))
 
+(defn- lucky-collection-children-location []
+  (collection/children-location (collection/user->personal-collection (test-users/user->id :lucky))))
+
 ;; test that we can create a new Collection with valid inputs
 (expect
   {:name              "My Favorite Cards"
@@ -288,13 +291,10 @@
 ;; Make sure that if we try to be sneaky and edit a Personal Collection via the graph, an Exception is thrown
 (expect
   Exception
-  (do
-    (force-create-personal-collections!)
-    (let [lucky-personal-collection-id (db/select-one-id Collection
-                                         :personal_owner_id (test-users/user->id :lucky))]
-      (collection/update-graph! (assoc-in (graph :clear-revisions? true)
-                                          [:groups (u/get-id (group/all-users)) lucky-personal-collection-id]
-                                          :read)))))
+  (let [lucky-personal-collection-id (u/get-id (collection/user->personal-collection (test-users/user->id :lucky)))]
+    (collection/update-graph! (assoc-in (graph :clear-revisions? true)
+                                        [:groups (u/get-id (group/all-users)) lucky-personal-collection-id]
+                                        :read))))
 
 ;; double-check that the graph is unchanged
 (expect
@@ -303,15 +303,34 @@
               (u/get-id (group/metabot))   {:root :none}
               (u/get-id (group/admin))     {:root :write}}}
   (do
-    (force-create-personal-collections!)
     (u/ignore-exceptions
-      (let [lucky-personal-collection-id (db/select-one-id Collection
-                                           :personal_owner_id (test-users/user->id :lucky))]
+      (let [lucky-personal-collection-id (u/get-id (collection/user->personal-collection (test-users/user->id :lucky)))]
         (collection/update-graph! (assoc-in (graph :clear-revisions? true)
                                             [:groups (u/get-id (group/all-users)) lucky-personal-collection-id]
                                             :read))))
     (graph)))
 
+;; Make sure descendants of Personal Collections do not come back as part of the graph either...
+(expect
+  {:revision 0
+   :groups   {(u/get-id (group/all-users)) {:root :none}
+              (u/get-id (group/metabot))   {:root :none}
+              (u/get-id (group/admin))     {:root :write}}}
+  (tt/with-temp Collection [_ {:location (lucky-collection-children-location)}]
+    (graph)))
+
+;; ...and that you can't be sneaky and try to edit them either...
+(expect
+  Exception
+  (tt/with-temp Collection [collection {:location (lucky-collection-children-location)}]
+    (let [lucky-personal-collection-id (u/get-id (collection/user->personal-collection (test-users/user->id :lucky)))]
+      (collection/update-graph! (assoc-in (graph :clear-revisions? true)
+                                          [:groups
+                                           (u/get-id (group/all-users))
+                                           lucky-personal-collection-id
+                                           (u/get-id collection)]
+                                          :read)))))
+
 
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                     Nested Collections Helper Fns & Macros                                     |
@@ -1400,9 +1419,6 @@
       (group->perms [parent child] group))))
 
 ;; Make sure that when creating a new Collection as child of a Personal Collection, no group permissions are created
-(defn- lucky-collection-children-location []
-  (collection/children-location (collection/user->personal-collection (test-users/user->id :lucky))))
-
 (expect
   false
   (tt/with-temp Collection [child {:name "{child}", :location (lucky-collection-children-location)}]

test/metabase/models/permissions_test.clj

 (ns metabase.models.permissions-test
   (:require [expectations :refer :all]
             [metabase.models
-             [collection :as collection]
+             [collection :as collection :refer [Collection]]
+             [collection-test :as collection-test]
              [database :refer [Database]]
              [permissions :as perms]
              [permissions-group :as group :refer [PermissionsGroup]]
@@ -583,26 +584,56 @@
 (expect
   Exception
   (do
-    ((resolve 'metabase.models.collection-test/force-create-personal-collections!))
+    (collection-test/force-create-personal-collections!)
     (perms/revoke-collection-permissions!
      (group/all-users)
      (u/get-id (db/select-one 'Collection :personal_owner_id (test-users/user->id :lucky))))))
 
+;; (should apply to descendants as well)
+(expect
+  Exception
+  (tt/with-temp Collection [collection {:location (collection/children-location
+                                                   (collection/user->personal-collection
+                                                    (test-users/user->id :lucky)))}]
+    (perms/revoke-collection-permissions!
+     (group/all-users)
+     collection)))
+
 ;; Make sure if you try to use the helper function to grant read perms for a Personal Collection, you get an Exception
 (expect
   Exception
   (do
-    ((resolve 'metabase.models.collection-test/force-create-personal-collections!))
+    (collection-test/force-create-personal-collections!)
     (perms/grant-collection-read-permissions!
      (group/all-users)
      (u/get-id (db/select-one 'Collection :personal_owner_id (test-users/user->id :lucky))))))
 
+;; (should apply to descendants as well)
+(expect
+  Exception
+  (tt/with-temp Collection [collection {:location (collection/children-location
+                                                   (collection/user->personal-collection
+                                                    (test-users/user->id :lucky)))}]
+    (perms/grant-collection-read-permissions!
+     (group/all-users)
+     collection)))
+
 ;; Make sure if you try to use the helper function to grant readwrite perms for a Personal Collection, you get an
 ;; Exception
 (expect
   Exception
   (do
-    ((resolve 'metabase.models.collection-test/force-create-personal-collections!))
+    (collection-test/force-create-personal-collections!)
     (perms/grant-collection-readwrite-permissions!
      (group/all-users)
      (u/get-id (db/select-one 'Collection :personal_owner_id (test-users/user->id :lucky))))))
+
+;; (should apply to descendants as well)
+(expect
+  Exception
+  (tt/with-temp Collection [collection {:location (collection/children-location
+                                                   (collection/user->personal-collection
+                                                    (test-users/user->id :lucky)))}]
+    (perms/grant-collection-readwrite-permissions!
+     (group/all-users)
+     collection)))