From 2ebb5958a1c93873124ce28f45f5150512911b85 Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Thu, 17 Feb 2022 10:24:35 -0800
Subject: [PATCH] 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 | 40 +++++++++++++++++++++
src/metabase/db/data_migrations.clj | 33 ++++-------------
test/metabase/db/schema_migrations_test.clj | 28 +++++++++++++--
3 files changed, 73 insertions(+), 28 deletions(-)
diff --git a/resources/migrations/000_migrations.yaml b/resources/migrations/000_migrations.yaml
index 1269cffc5e1..47d1d04e40e 100644
--- a/resources/migrations/000_migrations.yaml
+++ b/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
#
diff --git a/src/metabase/db/data_migrations.clj b/src/metabase/db/data_migrations.clj
index 9ef648250d5..e8b12b28b2f 100644
--- a/src/metabase/db/data_migrations.clj
+++ b/src/metabase/db/data_migrations.clj
@@ -1,4 +1,4 @@
-(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).
diff --git a/test/metabase/db/schema_migrations_test.clj b/test/metabase/db/schema_migrations_test.clj
index 8391628da10..7d06d0c16c3 100644
--- a/test/metabase/db/schema_migrations_test.clj
+++ b/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"
--
GitLab