diff --git a/src/metabase/models/database.clj b/src/metabase/models/database.clj
index 4bc9db68ddcee81b7f825a964fba29a8fc2ef793..1262ff2a1ff69f524a90bf8f7588c05ff1c17126 100644
--- a/src/metabase/models/database.clj
+++ b/src/metabase/models/database.clj
@@ -1,6 +1,7 @@
(ns metabase.models.database
(:require [cheshire.generate :refer [add-encoder encode-map]]
[clojure.tools.logging :as log]
+ [java-time :as t]
[medley.core :as m]
[metabase.api.common :refer [*current-user*]]
[metabase.db.util :as mdb.u]
@@ -9,6 +10,7 @@
[metabase.models.interface :as i]
[metabase.models.permissions :as perms]
[metabase.models.permissions-group :as perm-group]
+ [metabase.models.secret :as secret]
[metabase.plugins.classloader :as classloader]
[metabase.util :as u]
[metabase.util.i18n :refer [trs]]
@@ -69,10 +71,65 @@
(catch Throwable e
(log/error e (trs "Error sending database deletion notification")))))
-;; TODO - this logic would make more sense in post-update if such a method existed
+(defn- handle-db-details-secret-prop!
+ "Helper fn for reducing over a map of all the secret connection-properties, keyed by name. This is side effecting. At
+ each iteration step, if there is a -value suffixed property set in the details to be persisted, then we instead insert
+ (or update an existing) Secret instance and point to the inserted -id instead."
+ [database details conn-prop-nm conn-prop]
+ (let [sub-prop (fn [suffix]
+ (keyword (str conn-prop-nm suffix)))
+ id-kw (sub-prop "-id")
+ new-name (format "%s for %s" (:display-name conn-prop) (:name database))
+ ;; in the future, when secret values can simply be changed by passing
+ ;; in a new ID (as opposed to a new value), this behavior will change,
+ ;; but for now, we should simply look for the value
+ path-kw (sub-prop "-path")
+ value-kw (sub-prop "-value")
+ value (if-let [v (value-kw details)] ; the -value suffix was specified; use that
+ v
+ (when-let [path (path-kw details)] ; the -path suffix was specified; this is actually a :file-path
+ path))
+ kind (:secret-kind conn-prop)
+ source (when (path-kw details)
+ :file-path)] ; set the :source due to the -path suffix (see above)
+ (if (nil? value) ;; secret value for this conn prop was not changed
+ details
+ (let [{:keys [id creator_id created_at]} (secret/upsert-secret-value!
+ (id-kw details)
+ new-name
+ kind
+ source
+ value)]
+ ;; remove the -value keyword (since in the persisted details blob, we only ever want to store the -id)
+ (-> details
+ (dissoc value-kw)
+ (assoc id-kw id)
+ (assoc (sub-prop "-source") source)
+ (assoc (sub-prop "-creator-id") creator_id)
+ (assoc (sub-prop "-created-at") (t/format :iso-offset-date-time created_at)))))))
+
+(defn- handle-secrets-changes [{:keys [details] :as database}]
+ (let [conn-props-fn (get-method driver/connection-properties (driver.u/database->driver database))]
+ (cond (nil? conn-props-fn)
+ database ; no connection-properties multimethod defined; can't check secret types so do nothing
+
+ details ; we have details populated in this Database instance, so handle them
+ (let [conn-props (conn-props-fn (driver.u/database->driver database))
+ conn-secrets-by-name (->> (filter #(= "secret" (:type %)) conn-props)
+ (reduce (fn [acc prop] (assoc acc (:name prop) prop)) {}))
+ ;; saved-details (db/select-field :details Database id)
+ updated-details (reduce-kv (partial handle-db-details-secret-prop! database)
+ details
+ conn-secrets-by-name)]
+ (assoc database :details updated-details))
+
+ :else ; no details populated; do nothing
+ database)))
+
(defn- pre-update
[{new-metadata-schedule :metadata_sync_schedule, new-fieldvalues-schedule :cache_field_values_schedule, :as database}]
- (u/prog1 database
+ (u/prog1 (handle-secrets-changes database)
+ ;; TODO - this logic would make more sense in post-update if such a method existed
;; if the sync operation schedules have changed, we need to reschedule this DB
(when (or new-metadata-schedule new-fieldvalues-schedule)
(let [{old-metadata-schedule :metadata_sync_schedule
@@ -101,6 +158,8 @@
:metadata_sync_schedule new-metadata-schedule
:cache_field_values_schedule new-fieldvalues-schedule)))))))
+(defn- pre-insert [database]
+ (handle-secrets-changes database))
(defn- perms-objects-set [database _]
#{(perms/data-perms-path (u/the-id database))})
@@ -118,6 +177,7 @@
:properties (constantly {:timestamped? true})
:post-insert post-insert
:post-select post-select
+ :pre-insert pre-insert
:pre-update pre-update
:pre-delete pre-delete})
i/IObjectPermissions
diff --git a/src/metabase/models/secret.clj b/src/metabase/models/secret.clj
index 0d3a44468ad436d77b653c1094812bfed9239003..c4d742a3ec7f72fac8e47af08bf7686d541b1755 100644
--- a/src/metabase/models/secret.clj
+++ b/src/metabase/models/secret.clj
@@ -1,7 +1,9 @@
(ns metabase.models.secret
(:require [cheshire.generate :refer [add-encoder encode-map]]
+ [metabase.api.common :as api]
[metabase.models.interface :as i]
[metabase.util :as u]
+ [toucan.db :as db]
[toucan.models :as models]))
;;; ----------------------------------------------- Entity & Lifecycle -----------------------------------------------
@@ -25,6 +27,46 @@
;;; ---------------------------------------------- Hydration / Util Fns ----------------------------------------------
;; none yet
+(def
+ ^{:doc "The attributes of a secret which, if changed, will result in a version bump" :private true}
+ bump-version-keys
+ [:kind :source :value])
+
+(defn latest-for-id
+ "Returns the latest Secret instance for the given `id` (meaning the one with the highest `version`)."
+ {:added "0.41.0"}
+ [id]
+ (db/select-one Secret :id id {:order-by [[:version :desc]]}))
+
+(defn upsert-secret-value!
+ "Inserts a new secret value, or updates an existing one, for the given parameters.
+ * if there is no existing Secret instance, inserts with the given field values
+ * if there is an existing latest Secret instance, and the value (or any of the supporting fields, like kind or
+ source) has changed, then inserts a new version with the given parameters.
+ * if there is an existing latest Secret instance, but none of the aforementioned fields changed, then update it"
+ {:added "0.41.0"}
+ [existing-id nm kind source value]
+ (let [insert-new (fn [id v]
+ (let [inserted (db/insert! Secret (cond-> {:version v
+ :name nm
+ :kind kind
+ :source source
+ :value value
+ :creator_id api/*current-user-id*}
+ id
+ (assoc :id id)))]
+ ;; Toucan doesn't support composite primary keys, so adding a new record with incremented
+ ;; version for an existing ID won't return a result from db/insert!, hence we may need to
+ ;; manually select it here
+ (or inserted (db/select-one Secret :id id :version v))))
+ latest-version (when existing-id (latest-for-id existing-id))]
+ (if latest-version
+ (if (= (select-keys latest-version bump-version-keys) [kind source value])
+ (db/update-where! Secret {:id existing-id :version (:version latest-version)}
+ :name nm)
+ (insert-new (u/the-id latest-version) (inc (:version latest-version))))
+ (insert-new nil 1))))
+
;;; -------------------------------------------------- JSON Encoder --------------------------------------------------
(add-encoder SecretInstance (fn [secret json-generator]
diff --git a/test/metabase/models/database_test.clj b/test/metabase/models/database_test.clj
index dab9668e3b8b32ea8933910e48ca9f94f4d17500..ed55108e33b6a15a03a4acfd7172e4811016065f 100644
--- a/test/metabase/models/database_test.clj
+++ b/test/metabase/models/database_test.clj
@@ -2,19 +2,24 @@
(:require [cheshire.core :refer [decode encode]]
[clojure.string :as str]
[clojure.test :refer :all]
+ [metabase.api.common :as api]
[metabase.driver :as driver]
[metabase.driver.util :as driver.u]
[metabase.models :refer [Database]]
[metabase.models.database :as mdb]
[metabase.models.permissions :as perms]
+ [metabase.models.secret :as secret]
[metabase.models.user :as user]
[metabase.server.middleware.session :as mw.session]
[metabase.task :as task]
[metabase.task.sync-databases :as task.sync-databases]
[metabase.test :as mt]
+ [metabase.test.fixtures :as fixtures]
[schema.core :as s]
[toucan.db :as db]))
+(use-fixtures :once (fixtures/initialize :db :plugins :test-drivers))
+
(defn- trigger-for-db [db-id]
(some (fn [{trigger-key :key, :as trigger}]
(when (str/ends-with? trigger-key (str \. db-id))
@@ -155,3 +160,44 @@
(mdb/sensitive-fields-for-db nil)))
(is (= driver.u/default-sensitive-fields
(mdb/sensitive-fields-for-db {})))))
+
+(deftest secret-resolution-test
+ (mt/with-driver :secret-test-driver
+ (binding [api/*current-user-id* (mt/user->id :crowberto)]
+ (letfn [(check-db-fn [{:keys [details] :as database} exp-secret]
+ (is (not (contains? details :password-value)) "password-value was removed from details")
+ (is (some? (:password-created-at details)) "password-created-at was populated in details")
+ (is (= (mt/user->id :crowberto) (:password-creator-id details))
+ "password-creator-id was populated in details")
+ (is (= (if-let [src (:source exp-secret)]
+ (name src)
+ nil) (:password-source details)) "password-source matches the value from the secret")
+ (is (contains? details :password-id) "password-id was added to details")
+ (let [{:keys [created_at updated_at] :as secret} (secret/latest-for-id (:password-id details))]
+ (is (some? secret) "Loaded Secret instance by ID")
+ (is (some? created_at) "created_at populated for the secret instance")
+ (is (some? updated_at) "updated_at populated for the secret instance")
+ (doseq [[exp-key exp-val] exp-secret]
+ (testing (format "%s=%s in secret" exp-key exp-val)
+ (is (= exp-val (cond-> (exp-key secret)
+ (string? exp-val)
+ (String.)
+
+ :else
+ identity)))))))]
+ (testing "values for referenced secret IDs are resolved in a new DB"
+ (mt/with-temp Database [{:keys [id details] :as database} {:engine :secret-test-driver
+ :name "Test DB with secrets"
+ :details {:host "localhost"
+ :password-value "new-password"}}]
+ (testing " and saved db-details looks correct"
+ (check-db-fn database {:kind :password
+ :source nil
+ :version 1
+ :value "new-password"})
+ (testing " updating the value works as expected"
+ (db/update! Database id :details (assoc details :password-path "/path/to/my/password-file"))
+ (check-db-fn (Database id) {:kind :password
+ :source :file-path
+ :version 2
+ :value "/path/to/my/password-file"})))))))))
diff --git a/test/metabase/test/initialize.clj b/test/metabase/test/initialize.clj
index bf14f7e9983eae380b26becb894afb2b393770e2..0c67488227ceec74e43d81d5d286bbde322df664 100644
--- a/test/metabase/test/initialize.clj
+++ b/test/metabase/test/initialize.clj
@@ -90,7 +90,7 @@
(define-initialization :test-drivers
(classloader/require 'metabase.test.initialize.plugins)
((resolve 'metabase.test.initialize.plugins/init-test-drivers!)
- [:driver-deprecation-test-legacy :driver-deprecation-test-new]))
+ [:driver-deprecation-test-legacy :driver-deprecation-test-new :secret-test-driver]))
;; initializing the DB also does setup needed so the scheduler will work correctly. (Remember that the scheduler uses
;; a JDBC backend!)
diff --git a/test_modules/drivers/secret-test-driver/resources/metabase-plugin.yaml b/test_modules/drivers/secret-test-driver/resources/metabase-plugin.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..b65a491c52097ed699cb9a3c3f2a4cc30120906e
--- /dev/null
+++ b/test_modules/drivers/secret-test-driver/resources/metabase-plugin.yaml
@@ -0,0 +1,19 @@
+info:
+ name: Secret Test Driver
+ version: 0.0.0-SNAPSHOT
+ description: Test driver for secrets
+driver:
+ name: secret-test-driver
+ display-name: Secret Test Driver
+ lazy-load: true
+ parent: sql
+ connection-properties:
+ - name: host
+ display-name: Host
+ placeholder: localhost
+ - name: password
+ display-name: Password
+ type: secret
+ secret-kind: password
+ placeholder: foo
+ required: true