Retain property file path when revisiting database edit page (#20199)

* Retain property file path when revisiting database edit page

Ensuring that the file path (if that is the secret type) is returned via the API response to the admin, so the UX on the form edit page is better

This involved a lot of refactoring to clean up some stuff:

* removed the population of inferred secret properties (ex: last updated timestamp and creator ID) from `handle-db-details-secret-prop!`, since that stores those values into the details blob, defeating some of the purpose of normalizing secrets into a separate table
* moved the logic for injecting such inferred values to a new function, `expand-inferred-secret-values`, which lives within the secret model namespace
* delegating existing calls that require such expansion to the new function
* adding a new helper function, `reduce-over-details-secret-values`, to handle some common logic that is now showing up in a few places (reducing over db-details on secret values and doing some kind of manipulation)

Adding new database API test to ensure that the file path value is, in fact, returned from the API

src/metabase/api/database.clj

@@ -20,6 +20,7 @@
             [metabase.models.field-values :refer [FieldValues]]
             [metabase.models.interface :as mi]
             [metabase.models.permissions :as perms]
+            [metabase.models.secret :as secret]
             [metabase.models.table :refer [Table]]
             [metabase.public-settings :as public-settings]
             [metabase.sample-data :as sample-data]
@@ -271,12 +272,17 @@
 
 (api/defendpoint GET "/:id"
   "Get a single Database with `id`. Optionally pass `?include=tables` or `?include=tables.fields` to include the Tables
-  belonging to this database, or the Tables and Fields, respectively."
+  belonging to this database, or the Tables and Fields, respectively.  If the requestor is an admin, then certain
+  inferred secret values will also be included in the returned details (see
+  [[metabase.models.secret/admin-expand-db-details-inferred-secret-values]] for full details)."
   [id include]
   {include (s/maybe (s/enum "tables" "tables.fields"))}
-  (-> (api/read-check Database id)
-      add-expanded-schedules
-      (get-database-hydrate-include include)))
+  (cond-> (-> (api/read-check Database id)
+              add-expanded-schedules
+              (get-database-hydrate-include include))
+
+    api/*is-superuser?* ; for admins, expand inferred secret values in db-details
+    secret/admin-expand-db-details-inferred-secret-values))
 
 
 ;;; ----------------------------------------- GET /api/database/:id/metadata -----------------------------------------

src/metabase/models/database.clj

 (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]
@@ -110,38 +109,30 @@
         ;; but for now, we should simply look for the value
         secret-map (secret/db-details-prop->secret-map details conn-prop-nm)
         value      (:value secret-map)
-        source     (:source secret-map)]                     ; set the :source due to the -path suffix (see above)
+        src        (:source secret-map)] ; 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)
+      (let [{:keys [id] :as secret*} (secret/upsert-secret-value!
+                                       (id-kw details)
+                                       new-name
+                                       kind
+                                       src
+                                       value)]
         (-> details
-          (dissoc value-kw (sub-prop "-path"))
-          (assoc id-kw id)
-          (assoc (sub-prop "-source") source) ; TODO: figure out why this is needed
-          (assoc (sub-prop "-creator-id") creator_id)
-          (assoc (sub-prop "-created-at") (t/format :iso-offset-date-time created_at)))))))
+            ;; remove the -value keyword (since in the persisted details blob, we only ever want to store the -id),
+            ;; but the value may be re-added by expand-inferred-secret-values below (if appropriate)
+            (dissoc value-kw (sub-prop "-path"))
+            (assoc id-kw id)
+            (secret/expand-inferred-secret-values conn-prop-nm conn-prop secret*))))))
 
 (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  (secret/conn-props->secret-props-by-name conn-props)
-                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)))
+  (if (map? details)
+    (let [updated-details (secret/reduce-over-details-secret-values
+                            (driver.u/database->driver database)
+                            details
+                            (partial handle-db-details-secret-prop! database))]
+      (assoc database :details updated-details))
+    database))
 
 (defn- pre-update
   [{new-metadata-schedule :metadata_sync_schedule, new-fieldvalues-schedule :cache_field_values_schedule, :as database}]
@@ -177,8 +168,8 @@
 
 (defn- pre-insert [database]
   (-> database
-   handle-secrets-changes
-   (assoc :initial_sync_status "incomplete")))
+      handle-secrets-changes
+      (assoc :initial_sync_status "incomplete")))
 
 (defn- perms-objects-set [database _]
   #{(perms/data-perms-path (u/the-id database))})

src/metabase/models/secret.clj

@@ -2,8 +2,10 @@
   (:require [cheshire.generate :refer [add-encoder encode-map]]
             [clojure.java.io :as io]
             [clojure.tools.logging :as log]
+            [java-time :as t]
             [metabase.api.common :as api]
             [metabase.driver :as driver]
+            [metabase.driver.util :as driver.u]
             [metabase.models.interface :as i]
             [metabase.public-settings.premium-features :as premium-features]
             [metabase.util :as u]
@@ -159,12 +161,12 @@
        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.42.0"}
-  [existing-id nm kind source value]
+  [existing-id nm kind src value]
   (let [insert-new     (fn [id v]
                          (let [inserted (db/insert! Secret (cond-> {:version    v
                                                                     :name       nm
                                                                     :kind       kind
-                                                                    :source     source
+                                                                    :source     src
                                                                     :value      value
                                                                     :creator_id api/*current-user-id*}
                                                              id
@@ -172,15 +174,89 @@
                            ;; 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))))
+                           (db/select-one Secret :id (or id (u/the-id inserted)) :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])
+      (if (= (select-keys latest-version bump-version-keys) [kind src 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))))
 
+(defn reduce-over-details-secret-values
+  "Reduces over the given `db-details` (a Database details map), for any secret type connection properties under the
+  given `driver`, using the given `reduce-fn`, and returns the accumulated result.
+
+  `reduce-fn` is the reduction fn (i.e. the first arg to [[clojure.core/reduce-kv]]), and is therefore expected to have
+  a 3-arity.  Its first param is the accumulated `db-details`, its 2nd param (a String) is the connection property
+  name, and the 3rd param (a map) is the connection property map itself (containing the `:name`, `:type`, etc.).  This
+  function will only be invoked with connection properties that are of the secret type.
+
+  In essence, this is a utility function to provide a generic mechanism for transforming db-details containing secret
+  values."
+  {:added "0.42.0"}
+  [driver db-details reduce-fn]
+  (let [conn-props-fn (get-method driver/connection-properties driver)]
+    (if (and (map? db-details) (fn? conn-props-fn))
+        (let [conn-props            (conn-props-fn driver)
+              conn-secrets-by-name  (conn-props->secret-props-by-name conn-props)]
+          (reduce-kv reduce-fn db-details conn-secrets-by-name))
+        db-details)))
+
+(defn expand-inferred-secret-values
+  "Expand certain secret sub-properties in the `db-details`, depending on the secret type, for admin purposes.  This is
+  invoked as part of a KV reduction over secret type connection-properties, so `conn-prop-nm` (a String), and
+  `conn-prop` (a map containing the connection property definition) are also passed as parameters.
+
+  `secret-or-id?` is an optional param that, if passed, will be used to look up the derived secret values (to avoid a
+  redundant app DB query if the caller already has this; if a nil param value is passed, then the secret ID will be
+  looked up from the `:db-details` map at `conn-prop-nm`).
+
+  The keys/value pairs that may be added into `db-details`:
+
+   - <prop>-value - the secret value itself, in the case that the secret is a file-path type (as opposed to a value we
+                    store directly); the purpose of expanding this is to repopulate the file paths in the UI at the
+                    cost of \"exposing\" the file path (which itself shouldn't be too risky, especially since it will
+                    only be shown to admins); only populated for file type secret values
+   - <prop>-creator-id - the ID of the user who \"created\" the secret value for <prop> (i.e. the last person who
+                         updated it), for audit purposes; only populated for non-file type secret values
+   - <prop>-created-at - the timestamp of the last time the secret value for <prop> was changed or updated; only
+                         populated for non-file type secret values"
+  {:added "0.42.0"}
+  [db-details conn-prop-nm _conn-prop & [secret-or-id]]
+  (let [subprop (fn [prop-nm]
+                  (keyword (str conn-prop-nm prop-nm)))
+        secret* (cond (int? secret-or-id)
+                      (Secret secret-or-id)
+
+                      (instance? (class Secret) secret-or-id)
+                      secret-or-id
+
+                      true ; default; app DB look up from the ID in db-details
+                      (latest-for-id (get db-details (subprop "-id"))))
+        src     (:source secret*)]
+    ;; always populate the -source, -creator-id, and -created-at sub properties
+    (cond-> (assoc db-details (subprop "-source") src
+                              (subprop "-creator-id") (:creator_id secret*))
+
+      (some? (:created_at secret*))
+      (assoc (subprop "-created-at") (t/format :iso-offset-date-time (:created_at secret*)))
+
+      (= :file-path src) ; for file path sources only, populate the value
+      (assoc (subprop "-value") (value->string secret*)))))
+
+(defn admin-expand-db-details-inferred-secret-values
+  "Expand certain inferred secret sub-properties in the `database` `:details`, for the purpose of serving admin
+  requests (ex: to edit an existing database or view its current details).  This is to populate certain values that
+  shouldn't be stored in the details blob itself, but which can be derived from the details->secret association itself.
+  Refer to the docstring for [[expand-inferred-secret-values]] for full details."
+  {:added "0.42.0"}
+  [database]
+  (update database :details (fn [details]
+                              (reduce-over-details-secret-values (driver.u/database->driver database)
+                                                                 details
+                                                                 expand-inferred-secret-values))))
+
 ;;; -------------------------------------------------- JSON Encoder --------------------------------------------------
 
 (add-encoder SecretInstance (fn [secret json-generator]

test/metabase/api/database_test.clj

@@ -1198,3 +1198,18 @@
                                              :get
                                              200
                                              "database/db-ids-with-deprecated-drivers"))))))))
+
+(deftest secret-file-paths-returned-by-api-test
+  (mt/with-driver :secret-test-driver
+    (testing "File path values for secrets are returned as plaintext in the API (#20030)"
+      (mt/with-temp Database [database {:engine  :secret-test-driver
+                                        :name    "Test secret DB with password path"
+                                        :details {:host           "localhost"
+                                                  :password-path "/path/to/password.txt"}}]
+        (is (= {:password-source "file-path"
+                :password-value  "/path/to/password.txt"}
+               (as-> (u/the-id database) d
+                     (format "database/%d" d)
+                     (mt/user-http-request :crowberto :get 200 d)
+                     (:details d)
+                     (select-keys d [:password-source :password-value]))))))))

test/metabase/models/database_test.clj

@@ -169,13 +169,14 @@
       (binding [api/*current-user-id* (mt/user->id :crowberto)]
         (let [secret-ids  (atom #{}) ; keep track of all secret IDs created with the temp database
               check-db-fn (fn [{:keys [details] :as database} exp-secret]
-                            (is (not (contains? details :password-value)) "password-value was removed from details")
+                            (when (not= :file-path (:source exp-secret))
+                              (is (not (contains? details :password-value))
+                                  "password-value was removed from details when not a file-path"))
                             (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))
+                            (is (= (some-> (:source exp-secret) name)
+                                   (:password-source details))
                                 "password-source matches the value from the secret")
                             (is (contains? details :password-id) "password-id was added to details")
                             (let [secret-id                                  (:password-id details)