Other rando Groups shouldn't get perms for 'Migrated' Collections

src/metabase/db/migrations.clj

@@ -34,7 +34,8 @@
             [puppetlabs.i18n.core :refer [trs]]
             [toucan
              [db :as db]
-             [models :as models]])
+             [models :as models]]
+            [metabase.models.permissions-group :as group])
   (:import java.util.UUID))
 
 ;;; # Migration Helpers
@@ -356,22 +357,24 @@
 ;;    new collections.
 ;;
 (defmigration ^{:author "camsaul", :added "0.30.0"} add-migrated-collections
-  ;; 1. Grant Root Collection readwrite perms to all Groups. Except for admin since they already have root (`/`)
-  ;; perms, and we don't want to put extra entries in there that confuse things
-  (doseq [group-id (db/select-ids PermissionsGroup :id [:not= (u/get-id (perm-group/admin))])]
-    (perms/grant-collection-readwrite-permissions! group-id collection/root-collection))
-  ;; 2. Create the new collections.
-  (doseq [[model new-collection-name] {Dashboard (trs "Migrated Dashboards")
-                                       Pulse     (trs "Migrated Pulses")
-                                       Card      (trs "Migrated Questions")}
-          :when                       (db/exists? model :collection_id nil)
-          :let                        [new-collection (db/insert! Collection
-                                                        :name  new-collection-name
-                                                        :color "#509ee3")]] ; MB brand color
-    ;; 3. make sure the All Users group doesn't have any perms for this Collection.
-    (perms/revoke-collection-permissions! (perm-group/all-users) new-collection)
-    ;; 4. move everything not in this Collection to a new Collection
-    (log/info (trs "Moving instances of {0} that aren't in a Collection to {1} Collection {2}"
-                   (name model) new-collection-name (u/get-id new-collection)))
-    (db/update-where! model {:collection_id nil}
-      :collection_id (u/get-id new-collection))))
+  (let [non-admin-group-ids (db/select-ids PermissionsGroup :id [:not= (u/get-id (perm-group/admin))])]
+    ;; 1. Grant Root Collection readwrite perms to all Groups. Except for admin since they already have root (`/`)
+    ;; perms, and we don't want to put extra entries in there that confuse things
+    (doseq [group-id non-admin-group-ids]
+      (perms/grant-collection-readwrite-permissions! group-id collection/root-collection))
+    ;; 2. Create the new collections.
+    (doseq [[model new-collection-name] {Dashboard (trs "Migrated Dashboards")
+                                         Pulse     (trs "Migrated Pulses")
+                                         Card      (trs "Migrated Questions")}
+            :when                       (db/exists? model :collection_id nil)
+            :let                        [new-collection (db/insert! Collection
+                                                          :name  new-collection-name
+                                                          :color "#509ee3")]] ; MB brand color
+      ;; 3. make sure the non-admin groups don't have any perms for this Collection.
+      (doseq [group-id non-admin-group-ids]
+        (perms/revoke-collection-permissions! group-id new-collection))
+      ;; 4. move everything not in this Collection to a new Collection
+      (log/info (trs "Moving instances of {0} that aren't in a Collection to {1} Collection {2}"
+                     (name model) new-collection-name (u/get-id new-collection)))
+      (db/update-where! model {:collection_id nil}
+        :collection_id (u/get-id new-collection)))))

src/metabase/models/permissions.clj

@@ -5,13 +5,14 @@
             [clojure.core.match :refer [match]]
             [clojure.tools.logging :as log]
             [medley.core :as m]
+            [metabase
+             [config :as config]
+             [util :as u]]
             [metabase.api.common :refer [*current-user-id*]]
             [metabase.models
              [interface :as i]
              [permissions-group :as group]
              [permissions-revision :as perms-revision :refer [PermissionsRevision]]]
-            [metabase.util :as u]
-            [metabase.config :as config]
             [metabase.util
              [honeysql-extensions :as hx]
              [schema :as su]]

test/metabase/db/migrations_test.clj

@@ -161,3 +161,20 @@
                   :or
                   (for [migrated-collection-id (db/select-ids Collection :name [:in migrated-collection-names])]
                     [:like :object (format "/collection/%d/%%" migrated-collection-id)]))]}))))
+
+;; ...nor should other groups that happen to exist
+(expect
+  []
+  (tt/with-temp PermissionsGroup [group]
+    (with-add-migrated-collections-cleanup
+      (tt/with-temp* [Pulse     [_]
+                      Card      [_]
+                      Dashboard [_]]
+        (#'migrations/add-migrated-collections)
+        (db/select Permissions
+          {:where [:and
+                   [:= :group_id (u/get-id group)]
+                   (cons
+                    :or
+                    (for [migrated-collection-id (db/select-ids Collection :name [:in migrated-collection-names])]
+                      [:like :object (format "/collection/%d/%%" migrated-collection-id)]))]})))))