Copy permissions for parent when creating new Collection

src/metabase/models/collection.clj

@@ -512,11 +512,80 @@
 ;;; |                                       Toucan IModel & Perms Method Impls                                       |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
+(def ^:private CollectionWithLocationIDAndPersonalOwnerID
+  {:id                su/IntGreaterThanZero
+   :location          LocationPath
+   :personal_owner_id (s/maybe su/IntGreaterThanZero)
+   s/Keyword          s/Any})
+
+(s/defn ^:private is-personal-collection-or-descendant-of-one? :- s/Bool
+  "Is `collection` a Personal Collection, or a descendant of one?"
+  [collection :- CollectionWithLocationIDAndPersonalOwnerID]
+  (boolean
+   (or
+    ;; If collection has an owner ID we're already done here, we know it's a Personal Collection
+    (:personal_owner_id collection)
+    ;; Otherwise try to get the ID of its highest-level ancestor, e.g. if `location` is `/1/2/3/` we would get `1`.
+    ;; Then see if the root-level ancestor is a Personal Collection (Personal Collections can only got in the Root
+    ;; Collection.)
+    (db/exists? Collection
+      :id                (first (location-path->ids (:location collection)))
+      :personal_owner_id [:not= nil]))))
+
+
+;;; ----------------------------------------------------- INSERT -----------------------------------------------------
+
 (defn- pre-insert [{collection-name :name, color :color, :as collection}]
   (assert-valid-location collection)
   (assert-valid-hex-color color)
   (assoc collection :slug (slugify collection-name)))
 
+(defn- copy-collection-permissions!
+  "Grant read permissions to a destination Collection for every Group with read permissions for a source Collection, and
+  write perms for every Group with write perms for the source Collection."
+  [source-collection-or-id dest-collection-or-id]
+  ;; figure out who has permissions for the source Collection...
+  (let [group-ids-with-read-perms  (db/select-field :group_id Permissions
+                                     :object (perms/collection-read-path source-collection-or-id))
+        group-ids-with-write-perms (db/select-field :group_id Permissions
+                                     :object (perms/collection-readwrite-path source-collection-or-id))
+        read-path      (perms/collection-read-path      (u/get-id dest-collection-or-id))
+        readwrite-path (perms/collection-readwrite-path (u/get-id dest-collection-or-id))]
+    (println "group-ids-with-read-perms:" group-ids-with-read-perms) ; NOCOMMIT
+    (println "group-ids-with-write-perms:" group-ids-with-write-perms) ; NOCOMMIT
+    ;; ...and insert corresponding rows for the destination Collection
+    (db/insert-many! Permissions
+      (concat
+       ;; insert all the new read-perms records
+       (for [group-id group-ids-with-read-perms]
+         {:group_id group-id, :object read-path})
+       ;; ...and all the new write-perms records
+       (for [group-id group-ids-with-write-perms]
+         {:group_id group-id, :object readwrite-path})))))
+
+(defn- copy-parent-permissions!
+  "When creating a new Collection, we shall copy the Permissions entries for its parent. That way, Groups who can see
+  its parent can see it; and Groups who can 'curate' its parent can 'curate' it, as a default state. (Of course,
+  admins can change these permissions after the fact.)
+
+  This does *not* apply to Collections that are created inside a Personal Collection or one of its descendants.
+  Descendants of Personal Collections, like Personal Collections themselves, cannot have permissions entries in the
+  application database.
+
+  For newly created Collections at the root-level, copy the existing permissions for the Root Collection."
+  [{:keys [location id], :as collection}]
+  (when-not (is-personal-collection-or-descendant-of-one? collection)
+    (let [parent-collection-id (location-path->parent-id location)]
+      (println "parent-collection-id:" parent-collection-id) ; NOCOMMIT
+      (copy-collection-permissions! (or parent-collection-id root-collection) id))))
+
+(defn- post-insert [collection]
+  (u/prog1 collection
+    (copy-parent-permissions! collection)))
+
+
+;;; ----------------------------------------------------- UPDATE -----------------------------------------------------
+
 (s/defn ^:private check-changes-allowed-for-personal-collection
   "If we're trying to UPDATE a Personal Collection, make sure the proposed changes are allowed. Personal Collections
   have lots of restrictions -- you can't archive them, for example, nor can you transfer them to other Users."
@@ -580,6 +649,9 @@
     (cond-> collection-updates
       collection-name (assoc :slug (slugify collection-name)))))
 
+
+;;; ----------------------------------------------------- DELETE -----------------------------------------------------
+
 (def ^:dynamic *allow-deleting-personal-collections*
   "Whether to allow deleting Personal Collections. Normally we should *never* allow this, but in the single case of
   deleting a User themselves, we need to allow this. (Note that in normal usage, Users never get deleted, but rather
@@ -604,6 +676,9 @@
                               [:= :object (perms/collection-readwrite-path collection)]
                               [:= :object (perms/collection-read-path collection)]]}))
 
+
+;;; -------------------------------------------------- IModel Impl ---------------------------------------------------
+
 (defn perms-objects-set
   "Return the required set of permissions to `read-or-write` `collection-or-id`."
   [collection-or-id read-or-write]
@@ -620,6 +695,7 @@
          {:hydration-keys (constantly [:collection])
           :types          (constantly {:name :clob, :description :clob})
           :pre-insert     pre-insert
+          :post-insert    post-insert
           :pre-update     pre-update
           :pre-delete     pre-delete})
   i/IObjectPermissions

test/metabase/models/collection_test.clj

@@ -7,7 +7,7 @@
              [card :refer [Card]]
              [collection :as collection :refer [Collection]]
              [dashboard :refer [Dashboard]]
-             [permissions :as perms]
+             [permissions :refer [Permissions] :as perms]
              [permissions-group :as group :refer [PermissionsGroup]]
              [pulse :refer [Pulse]]
              [user :refer [User]]]
@@ -1301,6 +1301,115 @@
 ;; TODO - can you unarchive a Card that is inside an archived Collection??
 
 
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                                     Permissions Inheritance Upon Creation!                                     |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(defn- group->perms
+  "Return the perms paths for a `perms-group`, replacing the ID of any `collections` in any entries with their name."
+  [collections perms-group]
+  ;; we can reuse the `perms-path-ids->names` helper function from above, just need to stick `collection` in a map
+  ;; to simulate the output of the `with-collection-hierarchy` macro
+  (perms-path-ids->names
+   (zipmap (map :name collections)
+           collections)
+   (db/select-field :object Permissions :group_id (u/get-id perms-group))))
+
+;; Make sure that when creating a new Collection at the Root Level, we copy the group permissions for the Root
+;; Collection
+(expect
+  #{"/collection/{new}/"
+    "/collection/root/"}
+  (tt/with-temp PermissionsGroup [group]
+    (perms/grant-collection-readwrite-permissions! group collection/root-collection)
+    (tt/with-temp Collection [collection {:name "{new}"}]
+      (group->perms [collection] group))))
+
+(expect
+  #{"/collection/{new}/read/"
+    "/collection/root/read/"}
+  (tt/with-temp PermissionsGroup [group]
+    (perms/grant-collection-read-permissions! group collection/root-collection)
+    (tt/with-temp Collection [collection {:name "{new}"}]
+      (group->perms [collection] group))))
+
+;; Needless to say, no perms before hand = no perms after
+(expect
+  #{}
+  (tt/with-temp PermissionsGroup [group]
+    (tt/with-temp Collection [collection {:name "{new}"}]
+      (group->perms [collection] group))))
+
+;; ...and granting perms after shouldn't affect Collections already created
+(expect
+  #{"/collection/root/read/"}
+  (tt/with-temp* [PermissionsGroup [group]
+                  Collection [collection {:name "{new}"}]]
+    (perms/grant-collection-read-permissions! group collection/root-collection)
+    (group->perms [collection] group)))
+
+;; Make sure that when creating a new Collection as a child of another, we copy the group permissions for its parent
+(expect
+  #{"/collection/{parent}/"
+    "/collection/{child}/"}
+  (tt/with-temp* [PermissionsGroup [group]
+                  Collection       [parent {:name "{parent}"}]]
+    (perms/grant-collection-readwrite-permissions! group parent)
+    (tt/with-temp Collection [child {:name "{child}", :location (collection/children-location parent)}]
+      (group->perms [parent child] group))))
+
+(expect
+  #{"/collection/{parent}/read/"
+    "/collection/{child}/read/"}
+  (tt/with-temp* [PermissionsGroup [group]
+                  Collection       [parent {:name "{parent}"}]]
+    (perms/grant-collection-read-permissions! group parent)
+    (tt/with-temp Collection [child {:name "{child}", :location (collection/children-location parent)}]
+      (group->perms [parent child] group))))
+
+(expect
+  #{}
+  (tt/with-temp* [PermissionsGroup [group]
+                  Collection       [parent {:name "{parent}"}]
+                  Collection       [child {:name "{child}", :location (collection/children-location parent)}]]
+    (group->perms [parent child] group)))
+
+(expect
+  #{"/collection/{parent}/read/"}
+  (tt/with-temp* [PermissionsGroup [group]
+                  Collection       [parent {:name "{parent}"}]
+                  Collection       [child {:name "{child}", :location (collection/children-location parent)}]]
+    (perms/grant-collection-read-permissions! group parent)
+    (group->perms [parent child] group)))
+
+;; If we have Root Collection perms they shouldn't be copied for a Child
+(expect
+  #{"/collection/root/read/"}
+  (tt/with-temp* [PermissionsGroup [group]
+                  Collection       [parent {:name "{parent}"}]]
+    (perms/grant-collection-read-permissions! group collection/root-collection)
+    (tt/with-temp Collection [child {:name "{child}", :location (collection/children-location parent)}]
+      (group->perms [parent child] group))))
+
+;; Make sure that when creating a new Collection as child of a Personal Collection, no group permissions are created
+(expect
+  false
+  (tt/with-temp Collection [child {:name "{child}", :location (collection/children-location
+                                                               (collection/user->personal-collection
+                                                                (test-users/user->id :lucky)))}]
+    (db/exists? Permissions :object [:like (format "/collection/%d/%%" (u/get-id child))])))
+
+;; Make sure that when creating a new Collection as grandchild of a Personal Collection, no group permissions are
+;; created
+(expect
+  false
+  (tt/with-temp* [Collection [child {:location (collection/children-location
+                                                (collection/user->personal-collection (test-users/user->id :lucky)))}]
+                  Collection [grandchild {:location (collection/children-location child)}]]
+    (or (db/exists? Permissions :object [:like (format "/collection/%d/%%" (u/get-id child))])
+        (db/exists? Permissions :object [:like (format "/collection/%d/%%" (u/get-id grandchild))]))))
+
+
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                              Personal Collections                                              |
 ;;; +----------------------------------------------------------------------------------------------------------------+