diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn
index 9993e7f739ee05ba9fda5940d596211a1f6c7373..db7ec479c45be25f740abf76fc1cd7541b41faae 100644
--- a/.clj-kondo/config.edn
+++ b/.clj-kondo/config.edn
@@ -401,7 +401,7 @@
metabase.async.api-response-test/with-response clojure.core/let
metabase.dashboard-subscription-test/with-dashboard-sub-for-card clojure.core/let
metabase.db.custom-migrations/defmigration clj-kondo.lint-as/def-catch-all
- metabase.db.custom-migrations/def-reversible-migration clj-kondo.lint-as/def-catch-all
+ metabase.db.custom-migrations/define-reversible-migration clj-kondo.lint-as/def-catch-all
metabase.db.data-migrations/defmigration clojure.core/def
metabase.db.liquibase/with-liquibase clojure.core/let
metabase.db.schema-migrations-test.impl/with-temp-empty-app-db clojure.core/let
diff --git a/bin/lint-migrations-file/src/change_set/strict.clj b/bin/lint-migrations-file/src/change_set/strict.clj
index 8003877c37515f64c62aeccddbe56a7a3fafa9d7..bf684b867431bf04921fb8f7a3dee17b5c8d1f75 100644
--- a/bin/lint-migrations-file/src/change_set/strict.clj
+++ b/bin/lint-migrations-file/src/change_set/strict.clj
@@ -56,7 +56,7 @@
:renameTable
:renameTrigger
:renameView
- ;; assumes all custom changes use the `def-migration` or `def-reversible-migration` in
+ ;; assumes all custom changes use the `def-migration` or `define-reversible-migration` in
;; metabase.db.custom-migrations
:customChange})
diff --git a/resources/migrations/000_migrations.yaml b/resources/migrations/000_migrations.yaml
index 472c9b9c7e12267922e339b876b59c5a36c9b865..f0b44a3c74b738cae51a3e7b1fa04589c2e31f48 100644
--- a/resources/migrations/000_migrations.yaml
+++ b/resources/migrations/000_migrations.yaml
@@ -14262,6 +14262,14 @@ databaseChangeLog:
- sql: UPDATE metabase_database SET engine = 'presto-jdbc' WHERE engine = 'presto'
rollback: # nothing to do, since we don't know which drivers used to be presto
+ - changeSet:
+ id: v46.00-080
+ author: noahmoss
+ comment: Migrate data permission paths from v1 to v2 (splitting them into separate data and query permissions)
+ changes:
+ - customChange:
+ class: "metabase.db.custom_migrations.SplitDataPermissions"
+
- changeSet:
id: v46.00-084
author: qnkhuat
diff --git a/src/metabase/db/custom_migrations.clj b/src/metabase/db/custom_migrations.clj
index ee0fbdb421b26529470ab69061fb4ad9cb5bff8b..8658ae61716a80a85eb0d2e0170292d66d854ff7 100644
--- a/src/metabase/db/custom_migrations.clj
+++ b/src/metabase/db/custom_migrations.clj
@@ -1,35 +1,32 @@
(ns metabase.db.custom-migrations
- (:require [metabase.util.log :as log]
- [toucan2.connection :as t2.conn])
- (:import [liquibase.change.custom CustomTaskChange CustomTaskRollback]
- liquibase.exception.ValidationErrors))
+ (:require
+ [clojure.set :as set]
+ [metabase.util.honey-sql-2 :as h2x]
+ [metabase.util.log :as log]
+ [toucan2.core :as t2]
+ [toucan2.execute :as t2.execute])
+ (:import
+ (liquibase.change.custom CustomTaskChange CustomTaskRollback)
+ (liquibase.exception ValidationErrors)))
(set! *warn-on-reflection* true)
-(defmacro def-reversible-migration
+(defmacro define-reversible-migration
"Define a reversible custom migration. Both the forward and reverse migrations are defined using the same structure,
similar to the bodies of multi-arity Clojure functions.
- The first thing in each migration body must be a one-element vector containing a binding to use for the database
- object provided by Liquibase, so that migrations have access to it if needed. This should typically not be used
- directly, however, because is also set automatically as the current connection for Toucan 2.
-
Example:
```clj
- (def-reversible-migration ExampleMigrationName
- ([_database]
- (migration-body))
-
- ([_database]
- (migration-body)))"
- [name [[db-binding-1] & migration-body] [[db-binding-2] reverse-migration-body]]
+ (define-reversible-migration ExampleMigrationName
+ (migration-body)
+ (reverse-migration-body)))
+ ```"
+ [name migration-body reverse-migration-body]
`(defrecord ~name []
CustomTaskChange
(execute [_# database#]
- (binding [toucan2.connection/*current-connectable* (.getWrappedConnection (.getConnection database#))]
- (let [~db-binding-1 database#]
- ~@migration-body)))
+ ~migration-body)
(getConfirmationMessage [_#]
(str "Custom migration: " ~name))
(setUp [_#])
@@ -39,9 +36,7 @@
CustomTaskRollback
(rollback [_# database#]
- (binding [toucan2.connection/*current-connectable* (.getWrappedConnection (.getConnection database#))]
- (let [~db-binding-2 database#]
- ~@reverse-migration-body)))))
+ ~reverse-migration-body)))
(defn no-op
"No-op logging rollback function"
@@ -51,4 +46,48 @@
(defmacro defmigration
"Define a custom migration."
[name migration-body]
- `(def-reversible-migration ~name ~migration-body ([~'_] (no-op ~(str name)))))
+ `(define-reversible-migration ~name ~migration-body (no-op ~(str name))))
+
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | MIGRATIONS |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(def ^:private base-path-regex
+ #"^(/db/\d+(?:/schema/(?:(?:[^\\/])|(?:\\/)|(?:\\\\))*(?:/table/\d+?)?)?/)((native/)|(query/(segmented/)?))?$")
+
+(defn- ->v2-paths
+ "Converts v1 data permission paths into v2 data and query permissions paths. This is similar to `->v2-path` in
+ metabase.models.permissions but somewhat simplified for the migration use case."
+ [v1-path]
+ (if-let [base-path (second (re-find base-path-regex v1-path))]
+ ;; For (almost) all v1 data paths, we simply extract the base path (e.g. "/db/1/schema/PUBLIC/table/1/")
+ ;; and construct new v2 paths by adding prefixes to the base path.
+ [(str "/data" base-path) (str "/query" base-path)]
+
+ ;; For the specific v1 path that grants full data access but no native query access, we add a
+ ;; /schema/ suffix to the corresponding v2 query permission path.
+ (when-let [db-id (second (re-find #"^/db/(\d+)/schema/$" v1-path))]
+ [(str "/data/db/" db-id "/") (str "/query/db/" db-id "/schema/")])))
+
+(define-reversible-migration SplitDataPermissions
+ (let [current-perms-set (t2/select-fn-set
+ (juxt :object :group_id)
+ :metabase.models.permissions/Permissions
+ {:where [:or
+ [:like :object (h2x/literal "/db/%")]
+ [:like :object (h2x/literal "/data/db/%")]
+ [:like :object (h2x/literal "/query/db/%")]]})
+ v2-perms-set (into #{} (mapcat
+ (fn [[v1-path group-id]]
+ (for [v2-path (->v2-paths v1-path)]
+ [v2-path group-id]))
+ current-perms-set))
+ new-v2-perms (into [] (set/difference v2-perms-set current-perms-set))]
+ (when (seq new-v2-perms)
+ (t2.execute/query-one {:insert-into :permissions
+ :columns [:object :group_id]
+ :values new-v2-perms})))
+ (t2.execute/query-one {:delete-from :permissions
+ :where [:or [:like :object (h2x/literal "/data/db/%")]
+ [:like :object (h2x/literal "/query/db/%")]]}))
diff --git a/src/metabase/db/setup.clj b/src/metabase/db/setup.clj
index a3a1b75a5ef69ce90f85e4250b6d056a540aaa67..df471fe10f3f443e5502be31d76a164c60af6dd4 100644
--- a/src/metabase/db/setup.clj
+++ b/src/metabase/db/setup.clj
@@ -134,14 +134,11 @@
(s/defn ^:private run-data-migrations!
"Do any custom code-based migrations now that the db structure is up to date."
- [db-type :- s/Keyword
- data-source :- javax.sql.DataSource]
+ []
;; TODO -- check whether we can remove the circular ref busting here.
(when-not *disable-data-migrations*
(classloader/require 'metabase.db.data-migrations)
- (binding [mdb.connection/*application-db* (mdb.connection/application-db db-type data-source :create-pool? false) ; should already be a pool.
- setting/*disable-cache* true]
- ((resolve 'metabase.db.data-migrations/run-all!)))))
+ ((resolve 'metabase.db.data-migrations/run-all!))))
;; TODO -- consider renaming to something like `verify-connection-and-migrate!`
;;
@@ -154,9 +151,11 @@
auto-migrate? :- (s/maybe s/Bool)]
(u/profile (trs "Database setup")
(u/with-us-locale
- (verify-db-connection db-type data-source)
- (run-schema-migrations! db-type data-source auto-migrate?)
- (run-data-migrations! db-type data-source)))
+ (binding [mdb.connection/*application-db* (mdb.connection/application-db db-type data-source :create-pool? false) ; should already be a pool
+ setting/*disable-cache* true]
+ (verify-db-connection db-type data-source)
+ (run-schema-migrations! db-type data-source auto-migrate?)
+ (run-data-migrations!))))
:done)
;;;; Toucan Setup.
diff --git a/test/metabase/db/schema_migrations_test.clj b/test/metabase/db/schema_migrations_test.clj
index 040ebf813cd15e30f128d7b59ee80a83d49de784..9f0add63983f06bdc2a5eb141c9f31f96cc05aab 100644
--- a/test/metabase/db/schema_migrations_test.clj
+++ b/test/metabase/db/schema_migrations_test.clj
@@ -28,6 +28,7 @@
Dimension
Field
Permissions
+ PermissionsGroup
Pulse
Setting
Table
@@ -39,7 +40,8 @@
[metabase.test.util :as tu]
[metabase.util :as u]
[toucan.db :as db]
- [toucan2.core :as t2])
+ [toucan2.core :as t2]
+ [toucan2.execute :as t2.execute])
(:import
(java.sql Connection)
(java.util UUID)))
@@ -906,3 +908,58 @@
(db/delete! Database :id db-id)))
(migrate!)
(is (db/delete! Database :id db-id))))))
+
+(deftest split-data-permission-test
+ (testing "Migration v46.00-080: split existing v1 data permission paths into v2 data and query permission paths"
+ (impl/test-migrations ["v46.00-080"] [migrate!]
+ (let [[group-1-id] (t2/insert-returning-pks! PermissionsGroup {:name "Test Group 1"})
+ [group-2-id] (t2/insert-returning-pks! PermissionsGroup {:name "Test Group 2"})
+ v1-paths-and-groups [["/db/1/" group-1-id]
+ ["/db/2/schema/" group-1-id]
+ ["/db/3/native/" group-1-id]
+ ["/db/4/schema/PUBLIC/" group-1-id]
+ ["/db/5/schema/my\\\\schema/" group-1-id]
+ ["/db/6/schema/PUBLIC/table/1/" group-1-id]
+ ["/db/7/schema/PUBLIC/table/1/query/segmented/" group-1-id]
+ ["/db/8/schema/PUBLIC/table/1/query/segmented/" group-1-id]
+ ["/db/1/" group-2-id]
+ ["invalid-path" group-2-id]]
+ _ (t2.execute/query-one {:insert-into :permissions
+ :columns [:object :group_id]
+ :values v1-paths-and-groups})
+ _ (migrate!)
+ new-paths-set (t2/select-fn-set (juxt :object :group_id)
+ :models/permissions
+ {:where [:in :group_id [group-1-id group-2-id]]})]
+ ;; Check that the full permission set for group-1 and group-2 is what we expect post-migration.
+ ;; Each v1-path from above is listed here, immediately followed by the two resulting v2 paths.
+ (is (= #{["/db/1/" group-1-id]
+ ["/data/db/1/" group-1-id]
+ ["/query/db/1/" group-1-id]
+ ["/db/2/schema/" group-1-id]
+ ["/data/db/2/" group-1-id]
+ ["/query/db/2/schema/" group-1-id]
+ ["/db/3/native/" group-1-id]
+ ["/data/db/3/" group-1-id]
+ ["/query/db/3/" group-1-id]
+ ["/db/4/schema/PUBLIC/" group-1-id]
+ ["/data/db/4/schema/PUBLIC/" group-1-id]
+ ["/query/db/4/schema/PUBLIC/" group-1-id]
+ ["/db/5/schema/my\\\\schema/" group-1-id]
+ ["/data/db/5/schema/my\\\\schema/" group-1-id]
+ ["/query/db/5/schema/my\\\\schema/" group-1-id]
+ ["/db/6/schema/PUBLIC/table/1/" group-1-id]
+ ["/data/db/6/schema/PUBLIC/table/1/" group-1-id]
+ ["/query/db/6/schema/PUBLIC/table/1/" group-1-id]
+ ["/db/7/schema/PUBLIC/table/1/query/segmented/" group-1-id]
+ ["/data/db/7/schema/PUBLIC/table/1/" group-1-id]
+ ["/query/db/7/schema/PUBLIC/table/1/" group-1-id]
+ ["/db/8/schema/PUBLIC/table/1/query/segmented/" group-1-id]
+ ["/data/db/8/schema/PUBLIC/table/1/" group-1-id]
+ ["/query/db/8/schema/PUBLIC/table/1/" group-1-id]
+ ["/db/1/" group-2-id]
+ ["/data/db/1/" group-2-id]
+ ["/query/db/1/" group-2-id]
+ ;; Invalid path is not touched but also doesn't fail the migration
+ ["invalid-path" group-2-id]}
+ new-paths-set))))))