From 8f0c6a62b3cfc434ac86b58873e745352e0b03d6 Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Wed, 16 Feb 2022 18:30:35 -0800
Subject: [PATCH] Replace `add-admin-group-root-entry` with Liquibase
migrations (#20400)
* Rework add-admin-group-root-entry migration
* Add test for the migration
* Dump/load should not copy the root perms entry for the admin group
---
resources/migrations/000_migrations.yaml | 27 +++++++++++++++++++++
src/metabase/cmd/copy.clj | 5 +++-
src/metabase/db/data_migrations.clj | 9 -------
src/metabase/models/permissions_group.clj | 2 +-
test/metabase/db/schema_migrations_test.clj | 20 ++++++++++++++-
5 files changed, 51 insertions(+), 12 deletions(-)
diff --git a/resources/migrations/000_migrations.yaml b/resources/migrations/000_migrations.yaml
index ee5099d7d96..1269cffc5e1 100644
--- a/resources/migrations/000_migrations.yaml
+++ b/resources/migrations/000_migrations.yaml
@@ -10004,6 +10004,33 @@ databaseChangeLog:
WHERE u.is_superuser = true
AND pgm.id IS NULL;
+ #
+ # This migration replaces metabase.db.data-migrations/add-admin-group-root-entry, added 0.20.0
+ #
+ # Create root permissions entry for admin magic Permissions Group. Admin Group has a single entry that lets it
+ # access to everything
+ - changeSet:
+ id: v43.00-006
+ author: camsaul
+ comment: >-
+ Added 0.43.0. Create root '/' permissions entry for the 'Administrators' magic Permissions Group if needed.
+ changes:
+ - sql:
+ sql: >-
+ INSERT INTO permissions (group_id, object)
+ SELECT
+ admin_group.id AS group_id,
+ '/' AS object
+ FROM (
+ SELECT id
+ FROM permissions_group
+ WHERE name = 'Administrators'
+ ) admin_group
+ LEFT JOIN permissions p
+ ON admin_group.id = p.group_id
+ AND 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/cmd/copy.clj b/src/metabase/cmd/copy.clj
index 4bce65f48ec..3a46b1fd330 100644
--- a/src/metabase/cmd/copy.clj
+++ b/src/metabase/cmd/copy.clj
@@ -121,7 +121,10 @@
{;; ensure ID order to ensure that parent fields are inserted before children
"metabase_field" "ORDER BY id ASC"
;; don't copy the magic Permissions Groups. They get created by Liquibase migrations.
- "permissions_group" (format "WHERE name NOT IN ('%s', '%s')" group/all-users-group-name group/admin-group-name)})
+ "permissions_group" (format "WHERE name NOT IN ('%s', '%s')" group/all-users-group-name group/admin-group-name)
+ ;; don't copy over root permissions entries. Only the Administrators group should have this entry, and it gets
+ ;; created automatically by a Liquibase migration.
+ "permissions" "WHERE object <> '/'"})
(defn- copy-data! [^javax.sql.DataSource source-data-source target-db-type ^java.sql.Connection target-db-conn]
(with-open [source-conn (.getConnection source-data-source)]
diff --git a/src/metabase/db/data_migrations.clj b/src/metabase/db/data_migrations.clj
index dc7d7f16b1a..9ef648250d5 100644
--- a/src/metabase/db/data_migrations.clj
+++ b/src/metabase/db/data_migrations.clj
@@ -74,15 +74,6 @@
;;; | PERMISSIONS v1 |
;;; +----------------------------------------------------------------------------------------------------------------+
-;; admin group has a single entry that lets it access to everything
-(defmigration ^{:author "camsaul", :added "0.20.0"} add-admin-group-root-entry
- (binding [perms/*allow-admin-permissions-changes* true
- perms/*allow-root-entries* true]
- (u/ignore-exceptions
- (db/insert! Permissions
- :group_id (:id (perm-group/admin))
- :object "/"))))
-
;; 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
diff --git a/src/metabase/models/permissions_group.clj b/src/metabase/models/permissions_group.clj
index 92b7b39c7d2..b65675107f4 100644
--- a/src/metabase/models/permissions_group.clj
+++ b/src/metabase/models/permissions_group.clj
@@ -49,7 +49,7 @@
"Administrators")
(def ^{:arglists '([])} admin
- "Fetch the `Administators` permissions group, creating it if needed."
+ "Fetch the `Administrators` permissions group, creating it if needed."
(magic-group admin-group-name))
(defn- ^:deprecated get-or-create-magic-group! [group-name]
diff --git a/test/metabase/db/schema_migrations_test.clj b/test/metabase/db/schema_migrations_test.clj
index a5b60bb1ee3..8391628da10 100644
--- a/test/metabase/db/schema_migrations_test.clj
+++ b/test/metabase/db/schema_migrations_test.clj
@@ -14,8 +14,9 @@
[clojure.test :refer :all]
[metabase.db.schema-migrations-test.impl :as impl]
[metabase.driver :as driver]
- [metabase.models :refer [Database Field Setting Table]]
+ [metabase.models :refer [Database Field Permissions PermissionsGroup Setting Table]]
[metabase.models.interface :as mi]
+ [metabase.models.permissions-group :as group]
[metabase.models.user :refer [User]]
[metabase.test :as mt]
[metabase.test.fixtures :as fixtures]
@@ -317,6 +318,23 @@
(finally
(db/simple-delete! Database :name "Legacy BigQuery driver DB"))))))
+(deftest create-root-permissions-entry-for-admin-test
+ (testing "Migration v0.43.00-006: Add root permissions entry for 'Administrators' magic group"
+ (doseq [existing-entry? [true false]]
+ (testing (format "Existing root entry? %s" (pr-str existing-entry?))
+ (impl/test-migrations "v43.00-006" [migrate!]
+ (let [[{admin-group-id :id}] (db/query {:select [:id], :from [PermissionsGroup], :where [:= :name group/admin-group-name]})]
+ (is (integer? admin-group-id))
+ (when existing-entry?
+ (db/execute! {:insert-into Permissions
+ :values [{:object "/"
+ :group_id admin-group-id}]}))
+ (migrate!)
+ (is (= [{:object "/"}]
+ (db/query {:select [:object]
+ :from [Permissions]
+ :where [:= :group_id admin-group-id]})))))))))
+
(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)"
(impl/test-migrations ["v43.00-008"] [migrate!]
--
GitLab