Replace `add-databases-to-magic-permissions-groups` with Liquibase migrations (#20401)

* Rewrite add-databases-to-magic-permissions-groups

* Add a test for v0.43.00-007

* Small test tweak.

resources/migrations/000_migrations.yaml

@@ -10031,6 +10031,46 @@ databaseChangeLog:
                     AND p.object = '/'
               WHERE p.object IS NULL;
 
+  #
+  # The following migration replaces metabase.db.data-migrations/add-databases-to-magic-permissions-groups, added 0.20.0
+  #
+
+  # Add permissions entries for 'All Users' for all Databases created BEFORE we created the 'All Users' Permissions
+  # Group. This replaces the old 'add-databases-to-magic-permissions-groups' Clojure-land data migration. Only run
+  # this migration if that one hasn't been run yet.
+  - changeSet:
+      id: v43.00-007
+      author: camsaul
+      comment: >-
+        Added 0.43.0. Grant permissions for existing Databases to 'All Users' permissions group.
+      preConditions:
+        - onFail: MARK_RAN
+        - sqlCheck:
+            expectedResult: 0
+            sql: >-
+              SELECT count(*)
+              FROM data_migrations
+              WHERE id = 'add-databases-to-magic-permissions-groups';
+      changes:
+        - sql:
+            sql: >-
+              INSERT INTO permissions (object, group_id)
+              SELECT db.object, all_users.id AS group_id
+              FROM (
+                SELECT concat('/db/', id, '/') AS object
+                FROM metabase_database
+              ) db
+              LEFT JOIN (
+                SELECT id
+                FROM permissions_group
+                WHERE name = 'All Users'
+              ) all_users
+                ON true
+              LEFT JOIN permissions p
+                     ON p.group_id = all_users.id
+                    AND db.object = p.object
+              WHERE p.object IS NULL;
+
   #
   # The following migration replaces metabase.db.migrations/copy-site-url-setting-and-remove-trailing-slashes, added 0.23.0
   #

src/metabase/db/data_migrations.clj

-(ns metabase.db.data-migrations
+(ns ^:deprecated metabase.db.data-migrations
   "Clojure-land data migration definitions and fns for running them.
   These migrations are all ran once when Metabase is first launched, except when transferring data from an existing
   H2 database.  When data is transferred from an H2 database, migrations will already have been run against that data;
@@ -15,8 +15,7 @@
             [metabase.models.collection :as collection :refer [Collection]]
             [metabase.models.dashboard :refer [Dashboard]]
             [metabase.models.dashboard-card :refer [DashboardCard]]
-            [metabase.models.database :refer [Database]]
-            [metabase.models.permissions :as perms :refer [Permissions]]
+            [metabase.models.permissions :as perms]
             [metabase.models.permissions-group :as perm-group :refer [PermissionsGroup]]
             [metabase.models.pulse :refer [Pulse]]
             [metabase.models.user :refer [User]]
@@ -27,9 +26,9 @@
 
 ;;; # Migration Helpers
 
-(models/defmodel DataMigrations :data_migrations)
+(models/defmodel ^:deprecated DataMigrations :data_migrations)
 
-(defn- run-migration-if-needed!
+(defn- ^:deprecated run-migration-if-needed!
   "Run migration defined by `migration-var` if needed. `ran-migrations` is a set of migrations names that have already
   been run.
 
@@ -51,16 +50,16 @@
         :id        migration-name
         :timestamp :%now))))
 
-(def ^:private data-migrations (atom []))
+(def ^:private ^:deprecated data-migrations (atom []))
 
-(defmacro ^:private defmigration
+(defmacro ^:private ^:deprecated defmigration
   "Define a new data migration. This is just a simple wrapper around `defn-` that adds the resulting var to that
   `data-migrations` atom."
   [migration-name & body]
   `(do (defn- ~migration-name [] ~@body)
        (swap! data-migrations conj #'~migration-name)))
 
-(defn run-all!
+(defn ^:deprecated run-all!
   "Run all data migrations defined by `defmigration`."
   []
   (log/info "Running all necessary data migrations, this may take a minute.")
@@ -69,23 +68,6 @@
       (run-migration-if-needed! ran-migrations migration)))
   (log/info "Finished running data migrations."))
 
-
-;;; +----------------------------------------------------------------------------------------------------------------+
-;;; |                                                 PERMISSIONS v1                                                 |
-;;; +----------------------------------------------------------------------------------------------------------------+
-
-;; add existing databases to default permissions groups. default and metabot groups have entries for each individual
-;; DB
-(defmigration ^{:author "camsaul", :added "0.20.0"} add-databases-to-magic-permissions-groups
-  (let [db-ids (db/select-ids Database)]
-    (doseq [{group-id :id} [(perm-group/all-users)
-                            (perm-group/metabot)]
-            database-id    db-ids]
-      (u/ignore-exceptions
-        (db/insert! Permissions
-          :object   (perms/data-perms-path database-id)
-          :group_id group-id)))))
-
 ;; Before 0.30.0, we were storing the LDAP user's password in the `core_user` table (though it wasn't used).  This
 ;; migration clears those passwords and replaces them with a UUID. This is similar to a new account setup, or how we
 ;; disable passwords for Google authenticated users
@@ -94,7 +76,6 @@
     (doseq [user (db/select [User :id :password_salt] :ldap_auth [:= true])]
       (db/update! User (u/the-id user) :password (creds/hash-bcrypt (str (:password_salt user) (java.util.UUID/randomUUID)))))))
 
-
 ;; In 0.30 dashboards and pulses will be saved in collections rather than on separate list pages. Additionally, there
 ;; will no longer be any notion of saved questions existing outside of a collection (i.e. in the weird "Everything
 ;; Else" area where they can currently be saved).

test/metabase/db/schema_migrations_test.clj

@@ -335,8 +335,32 @@
                               :from      [Permissions]
                               :where     [:= :group_id admin-group-id]})))))))))
 
+(deftest create-database-entries-for-all-users-group-test
+  (testing "Migration v43.00-007: create DB entries for the 'All Users' permissions group"
+    (doseq [with-existing-data-migration? [true false]]
+      (testing (format "With existing data migration? %s" (pr-str with-existing-data-migration?))
+        (impl/test-migrations "v43.00-007" [migrate!]
+          (db/execute! {:insert-into Database
+                        :values      [{:name       "My DB"
+                                       :engine     "h2"
+                                       :created_at :%now
+                                       :updated_at :%now
+                                       :details    "{}"}]})
+          (when with-existing-data-migration?
+            (db/execute! {:insert-into :data_migrations
+                          :values      [{:id        "add-databases-to-magic-permissions-groups"
+                                         :timestamp :%now}]}))
+          (migrate!)
+          (is (= (if with-existing-data-migration?
+                   []
+                   [{:object "/db/1/"}])
+                 (db/query {:select    [:p.object]
+                            :from      [[Permissions :p]]
+                            :left-join [[PermissionsGroup :pg] [:= :p.group_id :pg.id]]
+                            :where     [:= :pg.name group/all-users-group-name]}))))))))
+
 (deftest migrate-legacy-site-url-setting-test
-  (testing "Migration v043.00-008: migrate legacy `-site-url` Setting to `site-url`; remove trailing slashes (#4123, #4188, #20402)"
+  (testing "Migration v43.00-008: migrate legacy `-site-url` Setting to `site-url`; remove trailing slashes (#4123, #4188, #20402)"
     (impl/test-migrations ["v43.00-008"] [migrate!]
       (db/execute! {:insert-into Setting
                     :values      [{:key   "-site-url"
@@ -346,7 +370,7 @@
              (db/query {:select [:*], :from [Setting], :where [:= :key "site-url"]}))))))
 
 (deftest site-url-ensure-protocol-test
-  (testing "Migration v043.00-009: ensure `site-url` Setting starts with a protocol (#20403)"
+  (testing "Migration v43.00-009: ensure `site-url` Setting starts with a protocol (#20403)"
     (impl/test-migrations ["v43.00-009"] [migrate!]
       (db/execute! {:insert-into Setting
                     :values      [{:key   "site-url"