Perms checking logic for Collection recursive moves/archiving

src/metabase/api/card.clj

@@ -251,26 +251,23 @@
 
 (defn- check-allowed-to-modify-query
   "If the query is being modified, check that we have data permissions to run the query."
-  [card query]
-  (when (and query
-             (not= query (:dataset_query card)))
-    (check-data-permissions-for-query query)))
+  [card-before-updates card-updates]
+  (when (api/column-will-change? :dataset_query card-before-updates card-updates)
+    (check-data-permissions-for-query (:dataset_query card-updates))))
 
 (defn- check-allowed-to-unarchive
   "When unarchiving a Card, make sure we have data permissions for the Card query before doing so."
-  [card archived?]
-  (when (and (false? archived?)
-             (:archived card))
-    (check-data-permissions-for-query (:dataset_query card))))
+  [card-before-updates card-updates]
+  (when (and (api/column-will-change? :archived card-before-updates card-updates)
+             (:archived card-before-updates))
+    (check-data-permissions-for-query (:dataset_query card-before-updates))))
 
 (defn- check-allowed-to-change-embedding
   "You must be a superuser to change the value of `enable_embedding` or `embedding_params`. Embedding must be
   enabled."
-  [card enable-embedding? embedding-params]
-  (when (or (and (some? enable-embedding?)
-                 (not= enable-embedding? (:enable_embedding card)))
-            (and embedding-params
-                 (not= embedding-params (:embedding_params card))))
+  [card-before-updates card-updates]
+  (when (or (api/column-will-change? :enable_embedding card-before-updates card-updates)
+            (api/column-will-change? :embedding_params card-before-updates card-updates))
     (api/check-embedding-enabled)
     (api/check-superuser)))
 
@@ -398,9 +395,9 @@
   (let [card-before-update (api/write-check Card id)]
     ;; Do various permissions checks
     (collection/check-allowed-to-change-collection card-before-update card-updates)
-    (check-allowed-to-modify-query card-before-update dataset_query)
-    (check-allowed-to-unarchive card-before-update archived)
-    (check-allowed-to-change-embedding card-before-update enable_embedding embedding_params)
+    (check-allowed-to-modify-query                 card-before-update card-updates)
+    (check-allowed-to-unarchive                    card-before-update card-updates)
+    (check-allowed-to-change-embedding             card-before-update card-updates)
     ;; make sure we have the correct `result_metadata`
     (let [card-updates (assoc card-updates
                          :result_metadata (result-metadata-for-updating card-before-update dataset_query

src/metabase/api/collection.clj

@@ -136,23 +136,54 @@
      (when parent_id
        {:location (collection/children-location (db/select-one [Collection :location :id] :id parent_id))}))))
 
-(defn- move-collection-if-needed! [collection-before-update collection-updates]
+;; TODO - I'm not 100% sure it makes sense that moving a Collection requires a special call to `move-collection!`,
+;; while archiving is handled automatically as part of the `pre-update` logic when you change a Collection's
+;; `archived` value. They are both recursive operations; couldn't we just have moving happen automatically when you
+;; change a `:location` as well?
+(defn- move-collection-if-needed!
+  "If input the `PUT /api/collection/:id` endpoint (`collection-updates`) specify that we should *move* a Collection, do
+  appropriate permissions checks and move it (and its descendants)."
+  [collection-before-update collection-updates]
   ;; is a [new] parent_id update specified in the PUT request?
   (when (contains? collection-updates :parent_id)
     (let [orig-location (:location collection-before-update)
           new-parent-id (:parent_id collection-updates)
-          new-location  (collection/children-location (if new-parent-id
-                                                        (db/select-one [Collection :location :id] :id new-parent-id)
-                                                        collection/root-collection))]
+          new-parent    (if new-parent-id
+                          (db/select-one [Collection :location :id] :id new-parent-id)
+                          collection/root-collection)
+          new-location  (collection/children-location new-parent)]
       ;; check and make sure we're actually supposed to be moving something
       (when (not= orig-location new-location)
-        ;; ok, make sure we have perms to move something out of the original parent Collection
-        (write-check-collection-or-root-collection (collection/location-path->parent-id orig-location))
-        ;; now make sure we have perms to move something into the new parent Collection
-        (write-check-collection-or-root-collection new-parent-id)
+        ;; ok, make sure we have perms to do this operation
+        (api/check-403
+         (perms/set-has-full-permissions-for-set? @api/*current-user-permissions-set*
+           (collection/perms-for-moving collection-before-update new-parent)))
         ;; ok, we're good to move!
         (collection/move-collection! collection-before-update new-location)))))
 
+(defn- check-allowed-to-archive-or-unarchive
+  "If input the `PUT /api/collection/:id` endpoint (`collection-updates`) specify that we should change the `archived`
+  status of a Collection, do appropriate permissions checks. (Actual recurisve (un)archiving logic is handled by
+  Collection's `pre-update`, so we do not need to manually call `collection/archive-collection!` and the like in this
+  namespace.)"
+  [collection-before-update collection-updates]
+  (when (api/column-will-change? :archived collection-before-update collection-updates)
+    ;; Check that we have approprate perms
+    (api/check-403
+     (perms/set-has-full-permissions-for-set? @api/*current-user-permissions-set*
+       (collection/perms-for-archiving collection-before-update)))))
+
+(defn- maybe-send-archived-notificaitons!
+  "When a collection is archived, all of it's cards are also marked as archived, but this is down in the model layer
+  which will not cause the archive notification code to fire. This will delete the relevant alerts and notify the
+  users just as if they had be archived individually via the card API."
+  [collection-before-update collection-updates]
+  (when (api/column-will-change? :archived collection-before-update collection-updates)
+    (when-let [alerts (seq (apply pulse/retrieve-alerts-for-cards (db/select-ids Card
+                                                                    :collection_id (u/get-id collection-before-update))))]
+        (card-api/delete-alert-and-notify-archived! alerts))))
+
+
 (api/defendpoint PUT "/:id"
   "Modify an existing Collection, including archiving or unarchiving it, or moving it."
   [id, :as {{:keys [name color description archived parent_id], :as collection-updates} :body}]
@@ -163,21 +194,18 @@
    parent_id   (s/maybe su/IntGreaterThanZero)}
   ;; do we have perms to edit this Collection?
   (let [collection-before-update (api/write-check Collection id)]
-    ;; ok, go ahead and update it! Only update keys that were specified in the `body`
+    ;; if we're trying to *archive* the Collection, make sure we're allowed to do that
+    (check-allowed-to-archive-or-unarchive collection-before-update collection-updates)
+    ;; ok, go ahead and update it! Only update keys that were specified in the `body`. But not `parent_id` since
+    ;; that's not actually a property of Collection, and since we handle moving a Collection separately below.
     (let [updates (u/select-keys-when collection-updates :present [:name :color :description :archived])]
       (when (seq updates)
         (db/update! Collection id updates)))
     ;; if we're trying to *move* the Collection (instead or as well) go ahead and do that
     (move-collection-if-needed! collection-before-update collection-updates)
-    ;; Check and see if if the Collection is switiching to archived
-    (when (and (not (:archived collection-before-update))
-               archived)
-      (when-let [alerts (seq (apply pulse/retrieve-alerts-for-cards (db/select-ids Card, :collection_id id)))]
-        ;; When a collection is archived, all of it's cards are also marked as archived, but this is down in the model
-        ;; layer which will not cause the archive notification code to fire. This will delete the relevant alerts and
-        ;; notify the users just as if they had be archived individually via the card API
-        (card-api/delete-alert-and-notify-archived! alerts))))
-  ;; return the updated object
+    ;; if we *did* end up archiving this Collection, we most post a few notifications
+    (maybe-send-archived-notificaitons! collection-before-update collection-updates))
+  ;; finally, return the updated object
   (Collection id))
 
 

src/metabase/api/common.clj

@@ -13,6 +13,7 @@
              [util :as u]]
             [metabase.api.common.internal :refer :all]
             [metabase.models.interface :as mi]
+            [metabase.util.schema :as su]
             [puppetlabs.i18n.core :refer [trs tru]]
             [ring.core.protocols :as protocols]
             [ring.util.response :as response]
@@ -494,3 +495,19 @@
     (check-404 object)
     (check (not (:archived object))
       [404 {:message (tru "The object has been archived."), :error_code "archived"}])))
+
+(s/defn column-will-change? :- s/Bool
+  "Helper for PATCH-style operations to see if a column is set to change when `object-updates` (i.e., the input to the
+  endpoint) is applied.
+
+    ;; assuming we have a Collection 10, that is not currently archived...
+    (api/column-will-change? :archived (Collection 10) {:archived true}) ; -> true, because value will change
+
+    (api/column-will-change? :archived (Collection 10) {:archived false}) ; -> false, because value did not change
+
+    (api/column-will-change? :archived (Collection 10) {}) ; -> false; value not specified in updates (request body)"
+  [k :- s/Keyword, object-before-updates :- su/Map, object-updates :- su/Map]
+  (boolean
+   (and (contains? object-updates k)
+        (not= (get object-before-updates k)
+              (get object-updates k)))))

src/metabase/api/dashboard.clj

@@ -202,11 +202,9 @@
 (defn- check-allowed-to-change-embedding
   "You must be a superuser to change the value of `enable_embedding` or `embedding_params`. Embedding must be
   enabled."
-  [dash enable-embedding? embedding-params]
-  (when (or (and (some? enable-embedding?)
-                 (not= enable-embedding? (:enable_embedding dash)))
-            (and embedding-params
-                 (not= embedding-params (:embedding_params dash))))
+  [dash-before-update dash-updates]
+  (when (or (api/column-will-change? :enable_embedding dash-before-update dash-updates)
+            (api/column-will-change? :embedding_params dash-before-update dash-updates))
     (api/check-embedding-enabled)
     (api/check-superuser)))
 
@@ -234,7 +232,7 @@
   (let [dash-before-update (api/write-check Dashboard id)]
     ;; Do various permissions checks as needed
     (collection/check-allowed-to-change-collection dash-before-update dash-updates)
-    (check-allowed-to-change-embedding dash-before-update enable_embedding embedding_params))
+    (check-allowed-to-change-embedding dash-before-update dash-updates))
   (api/check-500
    (db/update! Dashboard id
      ;; description, position, collection_id, and collection_position are allowed to be `nil`. Everything else must be

src/metabase/models/collection.clj

@@ -240,13 +240,15 @@
 
 (defn- is-root-collection? [m]
   (boolean (::is-root? m)))
-
-(def ^:private CollectionWithLocation
+(def ^:private CollectionWithLocationAndID
+  "Schema for a Collection Instance that has a valid Location *and* ID, or the Root Collection special placeholder
+  object."
   (s/pred (fn [collection]
             (and (map? collection)
                  (or (::is-root? collection)
-                     (valid-location-path? (:location collection)))))
-          "Collection with a valid `:location` or the Root Collection"))
+                     (and (valid-location-path? (:location collection))
+                          (integer? (:id collection))))))
+          "Collection with a valid `:location` and `:id`, or the Root Collection."))
 
 (s/defn children-location :- LocationPath
   "Given a `collection` return a location path that should match the `:location` value of all the children of the
@@ -256,7 +258,7 @@
 
      ;; To get children of this collection:
      (db/select Collection :location \"/10/20/30/\")"
-  [{:keys [location], :as collection} :- CollectionWithLocation]
+  [{:keys [location], :as collection} :- CollectionWithLocationAndID]
   (if (is-root-collection? collection)
     "/"
     (str location (u/get-id collection) "/")))
@@ -280,7 +282,7 @@
 
   where each letter represents a Collection, and the arrows represent values of its respective `:children`
   set."
-  [collection :- CollectionWithLocation]
+  [collection :- CollectionWithLocationAndID]
   ;; first, fetch all the descendants of the `collection`, and build a map of location -> children. This will be used
   ;; so we can fetch the immediate children of each Collection
   (let [location->children (group-by :location (db/select [Collection :name :id :location]
@@ -306,7 +308,6 @@
         ;; key
         :children)))
 
-
 (s/defn effective-children :- #{CollectionInstance}
   "Return the descendant Collections of a `collection` that should be presented to the current user as the children of
   this Collection. This takes into account descendants that get filtered out when the current user can't see them. For
@@ -331,7 +332,7 @@
    the current User. This needs to be done so we can give a User a way to navigate to nodes that they are allowed to
    access, but that are children of Collections they cannot access; in the example above, E and F are such nodes."
    {:hydrate :effective_children}
-  [collection :- CollectionWithLocation]
+  [collection :- CollectionWithLocationAndID]
   ;; Hydrate `:children` if it's not already done
   (-> (for [child (if (contains? collection :children)
                     (:children collection)
@@ -350,9 +351,68 @@
 ;;; |                                    Recursive Operations: Moving & Archiving                                    |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
+(s/defn perms-for-archiving :- #{perms/ObjectPath}
+  "Return the set of Permissions needed to archive or unarchive a `collection`. Since archiving a Collection is
+  *recursive* (i.e., it applies to all the descendant Collections of that Collection), we require write ('curate')
+  permissions for the Collection itself and all its descendants, but not for its parent Collection.
+
+  For example, suppose we have a Collection hierarchy like:
+
+    A > B > C
+
+  To move or archive B, you need write permissions for both B and C, since B would be affected as well. You do *not*
+  need permissions for A, even though in some sense you are affecting to contents of A; this is allowed because we
+  want to let people archive Collections they can see (via 'effectively' pulling them up), even if they can't see the
+  parent Collection."
+  [collection :- CollectionWithLocationAndID]
+  ;; Make sure we're not trying to archive the Root Collection...
+  (when (is-root-collection? collection)
+    (throw (Exception. (str (tru "You cannot archive the Root Collection.")))))
+  ;; also make sure we're not trying to archive a PERSONAL Collection
+  (when (db/exists? Collection :id (u/get-id collection), :personal_owner_id [:not= nil])
+    (throw (Exception. (str (tru "You cannot archive a Personal Collection.")))))
+  (set
+   (for [collection-or-id (cons collection
+                                (db/select-ids Collection :location [:like (str (children-location collection) "%")]))]
+     (perms/collection-readwrite-path collection-or-id))))
+
+(s/defn perms-for-moving :- #{perms/ObjectPath}
+  "Return the set of Permissions needed to move a `collection`. Like archiving, moving is recursive, so we require
+  perms for both the Collection and its descendants; we additionally require permissions for its new parent Collection.
+
+
+  For example, suppose we have a Collection hierarchy of three Collections, A, B, and C, and a forth Collection, D,
+  and we want to move B from A to D:
+
+    A > B > C        A
+               ===>
+    D                D > B > C
+
+
+  To move or archive B, you would need write permissions for A, B, and D:
+
+  *  B, since it's the Collection we're operating on
+  *  C, since it will by definition be affected too
+  *  D, because it's the new parent Collection, and moving something into it requires write perms.
+
+  Note that, like archiving, you *do not* need any permissions for A, the original parent; you can think of this
+  operation as something done on the Collection itself, rather than 'curating' its parent."
+  [collection :- CollectionWithLocationAndID, new-parent :- CollectionWithLocationAndID]
+  ;; Make sure we're not trying to move the Root Collection...
+  (when (is-root-collection? collection)
+    (throw (Exception. (str (tru "You cannot move the Root Collection.")))))
+  ;; Needless to say, it makes no sense to move a Collection into itself or into one of its descendants. So let's make
+  ;; sure we're not doing that...
+  (when (contains? (set (location-path->ids (children-location new-parent)))
+                   (u/get-id collection))
+    (throw (Exception. (str (tru "You cannot move a Collection into itself or into one of its descendants.")))))
+  (set
+   (cons (perms/collection-readwrite-path new-parent)
+         (perms-for-archiving collection))))
+
 (s/defn move-collection!
   "Move a Collection and all its descendant Collections from its current `location` to a `new-location`."
-  [collection :- CollectionWithLocation, new-location :- LocationPath]
+  [collection :- CollectionWithLocationAndID, new-location :- LocationPath]
   (let [orig-children-location (children-location collection)
         new-children-location  (children-location (assoc collection :location new-location))]
     ;; first move this Collection
@@ -367,14 +427,14 @@
         :where  [:like :location (str orig-children-location "%")]}))))
 
 (s/defn ^:private collection->descendant-ids :- (s/maybe #{su/IntGreaterThanZero})
-  [collection :- CollectionWithLocation, & additional-conditions]
+  [collection :- CollectionWithLocationAndID, & additional-conditions]
   (apply db/select-ids Collection
          :location [:like (str (children-location collection) "%")]
          additional-conditions))
 
 (s/defn ^:private archive-collection!
   "Archive a Collection and its descendant Collections and their Cards, Dashboards, and Pulses."
-  [collection :- CollectionWithLocation]
+  [collection :- CollectionWithLocationAndID]
   (let [affected-collection-ids (cons (u/get-id collection)
                                       (collection->descendant-ids collection, :archived false))]
     (db/transaction
@@ -389,7 +449,7 @@
 
 (s/defn ^:private unarchive-collection!
   "Unarchive a Collection and its descendant Collections and their Cards, Dashboards, and Pulses."
-  [collection :- CollectionWithLocation]
+  [collection :- CollectionWithLocationAndID]
   (let [affected-collection-ids (cons (u/get-id collection)
                                       (collection->descendant-ids collection, :archived true))]
     (db/transaction
@@ -411,46 +471,42 @@
   (assert-valid-hex-color color)
   (assoc collection :slug (slugify collection-name)))
 
-(s/defn ^:private field-will-change? :- s/Bool
-  "True if a `field-kw` is present in the `collection-updates` map of changes being passed into `pre-update`, and if
-  the value is acually different from the current value in the DB."
-  [field-kw :- s/Keyword, collection-before-updates :- CollectionInstance, collection-updates :- su/Map]
-  (boolean
-   (and (contains? collection-updates field-kw)
-        (not= (get collection-before-updates field-kw)
-              (get collection-updates        field-kw)))))
-
 (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."
-  [collection-before-updates :- CollectionWithLocation, collection-updates :- su/Map]
+  [collection-before-updates :- CollectionWithLocationAndID, collection-updates :- su/Map]
   ;; you're not allowed to change the `:personal_owner_id` of a Collection!
   ;; double-check and make sure it's not just the existing value getting passed back in for whatever reason
-  (when (field-will-change? :personal_owner_id collection-before-updates collection-updates)
+  (when (api/column-will-change? :personal_owner_id collection-before-updates collection-updates)
     (throw
      (ex-info (tru "You're not allowed to change the owner of a Personal Collection.")
        {:status-code 400
         :errors      {:personal_owner_id (tru "You're not allowed to change the owner of a Personal Collection.")}})))
+  ;;
+  ;; The checks below should be redundant because the `perms-for-moving` and `perms-for-archiving` functions also
+  ;; check to make sure you're not operating on Personal Collections. But as an extra safety net it doesn't hurt to
+  ;; check here too.
+  ;;
   ;; You also definitely cannot *move* a Personal Collection
-  (when (field-will-change? :location collection-before-updates collection-updates)
+  (when (api/column-will-change? :location collection-before-updates collection-updates)
     (throw
      (ex-info (tru "You're not allowed to move a Personal Collection.")
        {:status-code 400
         :errors      {:location (tru "You're not allowed to move a Personal Collection.")}})))
   ;; You also can't archive a Personal Collection
-  (when (field-will-change? :archived collection-before-updates collection-updates)
+  (when (api/column-will-change? :archived collection-before-updates collection-updates)
     (throw
      (ex-info (tru "You cannot archive a Personal Collection!")
        {:status-code 400
-        :errors   {:archived (tru "You cannot archive a Personal Collection!")}}))))
+        :errors      {:archived (tru "You cannot archive a Personal Collection!")}}))))
 
 (s/defn ^:private maybe-archive-or-unarchive!
   "If `:archived` specified in the updates map, archive/unarchive as needed."
-  [collection-before-updates :- CollectionWithLocation, collection-updates :- su/Map]
+  [collection-before-updates :- CollectionWithLocationAndID, collection-updates :- su/Map]
   ;; If the updates map contains a value for `:archived`, see if it's actually something different than current value
-  (when (field-will-change? :archived collection-before-updates collection-updates)
+  (when (api/column-will-change? :archived collection-before-updates collection-updates)
     ;; check to make sure we're not trying to change location at the same time
-    (when (field-will-change? :location collection-before-updates collection-updates)
+    (when (api/column-will-change? :location collection-before-updates collection-updates)
       (throw (ex-info (tru "You cannot move a Collection and archive it at the same time.")
                {:status-code 400
                 :errors      {:archived (tru "You cannot move a Collection and archive it at the same time.")}})))
@@ -468,7 +524,7 @@
     ;; (2) make sure the location is valid if we're changing it
     (assert-valid-location collection-updates)
     ;; (3) make sure hex color is valid
-    (when (field-will-change? :color collection-before-updates collection-updates)
+    (when (api/column-will-change? :color collection-before-updates collection-updates)
       (assert-valid-hex-color color))
     ;; OK, AT THIS POINT THE CHANGES ARE VALIDATED. NOW START ISSUING UPDATES
     ;; (1) archive or unarchive as appropriate
@@ -641,6 +697,7 @@
                                                       collection-or-id-or-nil
                                                       root-collection)))))
 
+
 (defn check-allowed-to-change-collection
   "If we're changing the `collection_id` of an object, make sure we have write permissions for both the old and new
   Collections, or throw a 403 if not. If `collection_id` isn't present in `object-updates`, or the value is the same
@@ -656,13 +713,11 @@
     (check-allowed-to-change-collection (Card 100) http-request-body)"
   [object-before-update object-updates]
   ;; if collection_id is set to change...
-  (when (contains? object-updates :collection_id)
-    (when (not= (:collection_id object-updates)
-                (:collection_id object-before-update))
-      ;; check that we're allowed to modify the old Collection
-      (check-write-perms-for-collection (:collection_id object-before-update))
-      ;; check that we're allowed to modify the new Collection
-      (check-write-perms-for-collection (:collection_id object-updates)))))
+  (when (api/column-will-change? :collection_id object-before-update object-updates)
+    ;; check that we're allowed to modify the old Collection
+    (check-write-perms-for-collection (:collection_id object-before-update))
+    ;; check that we're allowed to modify the new Collection
+    (check-write-perms-for-collection (:collection_id object-updates))))
 
 
 ;;; +----------------------------------------------------------------------------------------------------------------+

test/metabase/api/collection_test.clj

@@ -433,7 +433,7 @@
   ((user->client :crowberto) :put 200 (str "collection/" (u/get-id collection))
    {:name "My Beautiful Collection", :color "#ABCDEF"}))
 
-;; check that non-admins aren't allowed to update a collection
+;; check that users without write perms aren't allowed to update a Collection
 (expect
   "You don't have permissions to do that."
   (tt/with-temp Collection [collection]
@@ -470,6 +470,25 @@
        :emails (et/regex-email-bodies #"the question was archived by Crowberto Corv")
        :pulse  (Pulse pulse-id)))))
 
+;; I shouldn't be allowed to archive a Collection without proper perms
+(expect
+  "You don't have permissions to do that."
+  (tt/with-temp Collection [collection]
+    ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection))
+     {:archived true})))
+
+;; Perms checking should be recursive as well...
+;;
+;; Create Collections A > B, and grant permissions for A. You should not be allowed to archive A because you would
+;; also need perms for B
+(expect
+  "You don't have permissions to do that."
+  (tt/with-temp* [Collection [collection-a]
+                  Collection [collection-b {:location (collection/children-location collection-a)}]]
+    (perms/grant-collection-readwrite-permissions! (group/all-users) collection-a)
+    ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection-a))
+     {:archived true})))
+
 ;; Can I *change* the `location` of a Collection? (i.e. move it into a different parent Colleciton)
 (expect
   {:id                true
@@ -485,3 +504,64 @@
          {:parent_id (u/get-id b)})
         (update :location collection-test/location-path-ids->names)
         (update :id integer?))))
+
+;; I shouldn't be allowed to move the Collection without proper perms.
+;; If I want to move A into B, I should need permissions for both A and B
+(expect
+  "You don't have permissions to do that."
+  (tt/with-temp* [Collection [collection-a]
+                  Collection [collection-b]]
+    (perms/grant-collection-readwrite-permissions! (group/all-users) collection-a)
+    ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection-a))
+     {:parent_id (u/get-id collection-b)})))
+
+;; Perms checking should be recursive as well...
+;;
+;; Create A, B, and C; B is a child of A. Grant perms for A and B. Moving A into C should fail because we need perms
+;; for C:
+;; (collections with readwrite perms marked below with a `*`)
+;;
+;; A* -> B*  ==>  C -> A -> B
+;; C
+(expect
+  "You don't have permissions to do that."
+  (tt/with-temp* [Collection [collection-a]
+                  Collection [collection-b {:location (collection/children-location collection-a)}]
+                  Collection [collection-c]]
+    (doseq [collection [collection-a collection-b]]
+      (perms/grant-collection-readwrite-permissions! (group/all-users) collection))
+    ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection-a))
+     {:parent_id (u/get-id collection-c)})))
+
+
+;; Create A, B, and C; B is a child of A. Grant perms for A and C. Moving A into C should fail because we need perms
+;; for B:
+;; (collections with readwrite perms marked below with a `*`)
+;;
+;; A* -> B  ==>  C -> A -> B
+;; C*
+(expect
+  "You don't have permissions to do that."
+  (tt/with-temp* [Collection [collection-a]
+                  Collection [collection-b {:location (collection/children-location collection-a)}]
+                  Collection [collection-c]]
+    (doseq [collection [collection-a collection-c]]
+      (perms/grant-collection-readwrite-permissions! (group/all-users) collection))
+    ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection-a))
+     {:parent_id (u/get-id collection-c)})))
+
+;; Create A, B, and C; B is a child of A. Grant perms for B and C. Moving A into C should fail because we need perms
+;; for A:
+;; (collections with readwrite perms marked below with a `*`)
+;;
+;; A -> B*  ==>  C -> A -> B
+;; C*
+(expect
+  "You don't have permissions to do that."
+  (tt/with-temp* [Collection [collection-a]
+                  Collection [collection-b {:location (collection/children-location collection-a)}]
+                  Collection [collection-c]]
+    (doseq [collection [collection-b collection-c]]
+      (perms/grant-collection-readwrite-permissions! (group/all-users) collection))
+    ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection-a))
+     {:parent_id (u/get-id collection-c)})))

test/metabase/models/collection_test.clj

@@ -798,6 +798,194 @@
     (with-current-user-perms-for-collections [b d e f g]
       (effective-children collection/root-collection))))
 
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                                Nested Collections: Perms for Moving & Archiving                                |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+;; The tests in this section continue to use the Collection hierarchy above. The hierarchy doesn't get modified here,
+;; so it's the same in each test:
+;;
+;;    +-> B
+;;    |
+;; A -+-> C -+-> D -> E
+;;           |
+;;           +-> F -> G
+
+(defn- perms-path-ids->names
+  "Given a set of permissions and the `collections` map returned by the `with-collection-hierarchy` macro above, replace
+  the numeric IDs in the permissions paths with corresponding Collection names, making our tests easier to read."
+  [collections perms-set]
+  ;; first build a function that will replace any instances of numeric IDs with their respective names
+  ;; e.g. /123/ would become something like /A/
+  ;; Do this by composing together a series of functions that will handle one string replacement for each ID + name
+  ;; pair
+  (let [replace-ids-with-names (reduce comp (for [{:keys [id name]} (vals collections)]
+                                              #(str/replace % (re-pattern (format "/%d/" id)) (str "/" name "/"))))]
+    (set (for [perms-path perms-set]
+           (replace-ids-with-names perms-path)))))
+
+;;; ---------------------------------------------- Perms for Archiving -----------------------------------------------
+
+;; To Archive A, you should need *write* perms for A and all of its descendants...
+(expect
+  #{"/collection/A/"
+    "/collection/B/"
+    "/collection/C/"
+    "/collection/D/"
+    "/collection/E/"
+    "/collection/F/"
+    "/collection/G/"}
+  (with-collection-hierarchy [{:keys [a], :as collections}]
+    (->> (collection/perms-for-archiving a)
+         (perms-path-ids->names collections))))
+
+;; Now let's move down a level. To archive B, you should only need permissions for B itself, since it doesn't
+;; have any descendants
+(expect
+  #{"/collection/B/"}
+  (with-collection-hierarchy [{:keys [b], :as collections}]
+    (->> (collection/perms-for-archiving b)
+         (perms-path-ids->names collections))))
+
+;; but for C, you should need perms for C, D, E, F, and G
+(expect
+  #{"/collection/C/"
+    "/collection/D/"
+    "/collection/E/"
+    "/collection/F/"
+    "/collection/G/"}
+  (with-collection-hierarchy [{:keys [c], :as collections}]
+    (->> (collection/perms-for-archiving c)
+         (perms-path-ids->names collections))))
+
+;; For D you should need D + E
+(expect
+  #{"/collection/D/"
+    "/collection/E/"}
+  (with-collection-hierarchy [{:keys [d], :as collections}]
+    (->> (collection/perms-for-archiving d)
+         (perms-path-ids->names collections))))
+
+;; If you try to calculate permissions to archive the Root Collection, throw an Exception! Because you can't do
+;; that...
+(expect
+  Exception
+  (collection/perms-for-archiving collection/root-collection))
+
+;; Let's make sure we get an Exception when we try to archive a Personal Collection
+(expect
+  Exception
+  (collection/perms-for-archiving (collection/user->personal-collection (test-users/fetch-user :lucky))))
+
+;; also you should get an Exception if you try to pull a fast one on use and pass in some sort of invalid input...
+(expect Exception (collection/perms-for-archiving nil))
+(expect Exception (collection/perms-for-archiving {}))
+(expect Exception (collection/perms-for-archiving 1))
+
+
+;;; ------------------------------------------------ Perms for Moving ------------------------------------------------
+
+;; `*` marks the things that require permissions in charts below!
+
+;; If we want to move B into C, we should need perms for B and C. B because it is being moved; C we are moving
+;; something into it. Note that we do NOT require perms for A
+;;
+;;    +-> B                             +-> B*
+;;    |                                 |
+;; A -+-> C -+-> D -> E  ===>  A -> C* -+-> D -> E
+;;           |                          |
+;;           +-> F -> G                 +-> F -> G
+
+(expect
+  #{"/collection/B/"
+    "/collection/C/"}
+  (with-collection-hierarchy [{:keys [b c], :as collections}]
+    (->> (collection/perms-for-moving b c)
+         (perms-path-ids->names collections))))
+
+;; Ok, now let's try moving something with descendants. If we move C into B, we need perms for C and all its
+;; descendants, and B, since it's the new parent; we do not need perms for A, the old parent
+;;
+;;    +-> B
+;;    |
+;; A -+-> C -+-> D -> E  ===>  A -> B* -> C* -+-> D* -> E*
+;;           |                                |
+;;           +-> F -> G                       +-> F* -> G*
+(expect
+  #{"/collection/B/"
+    "/collection/C/"
+    "/collection/D/"
+    "/collection/E/"
+    "/collection/F/"
+    "/collection/G/"}
+  (with-collection-hierarchy [{:keys [b c], :as collections}]
+    (->> (collection/perms-for-moving c b)
+         (perms-path-ids->names collections))))
+
+;; Ok, now how about moving B into the Root Collection?
+;;
+;;    +-> B                    B* [and Root*]
+;;    |
+;; A -+-> C -+-> D -> E  ===>  A -> C -+-> D -> E
+;;           |                         |
+;;           +-> F -> G                +-> F -> G
+(expect
+  #{"/collection/root/"
+    "/collection/B/"}
+  (with-collection-hierarchy [{:keys [b], :as collections}]
+    (->> (collection/perms-for-moving b collection/root-collection)
+         (perms-path-ids->names collections))))
+
+;; How about moving C into the Root Collection?
+;;
+;;    +-> B                    A -> B
+;;    |
+;; A -+-> C -+-> D -> E  ===>  C* -+-> D* -> E* [and Root*]
+;;           |                     |
+;;           +-> F -> G            +-> F* -> G*
+(expect
+  #{"/collection/root/"
+    "/collection/C/"
+    "/collection/D/"
+    "/collection/E/"
+    "/collection/F/"
+    "/collection/G/"}
+  (with-collection-hierarchy [{:keys [c], :as collections}]
+    (->> (collection/perms-for-moving c collection/root-collection)
+         (perms-path-ids->names collections))))
+
+;; If you try to calculate permissions to move or archive the Root Collection, throw an Exception! Because you can't
+;; do that...
+(expect
+  Exception
+  (with-collection-hierarchy [{:keys [a]}]
+    (collection/perms-for-moving collection/root-collection a)))
+
+;; You should also see an Exception if you try to move a Collection into itself or into one its descendants...
+(expect
+  Exception
+  (with-collection-hierarchy [{:keys [b]}]
+    (collection/perms-for-moving b b)))
+
+(expect
+  Exception
+  (with-collection-hierarchy [{:keys [a b]}]
+    (collection/perms-for-moving a b)))
+
+;; Let's make sure we get an Exception when we try to *move* a Collection
+(expect
+  Exception
+  (with-collection-hierarchy [{:keys [a]}]
+    (collection/perms-for-moving (collection/user->personal-collection (test-users/fetch-user :lucky)) a)))
+
+;; also you should get an Exception if you try to pull a fast one on use and pass in some sort of invalid input...
+(expect Exception (collection/perms-for-moving {:location "/"} nil))
+(expect Exception (collection/perms-for-moving {:location "/"} {}))
+(expect Exception (collection/perms-for-moving {:location "/"} 1))
+(expect Exception (collection/perms-for-moving nil {:location "/"}))
+(expect Exception (collection/perms-for-moving {}  {:location "/"}))
+(expect Exception (collection/perms-for-moving 1   {:location "/"}))
+
 
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                     Nested Collections: Moving Collections                                     |