Ensure uploaded secrets are stable

src/metabase/models/secret.clj

@@ -1,5 +1,6 @@
 (ns metabase.models.secret
   (:require [cheshire.generate :refer [add-encoder encode-map]]
+            [clojure.core.memoize :as memoize]
             [clojure.java.io :as io]
             [clojure.string :as str]
             [clojure.tools.logging :as log]
@@ -54,7 +55,7 @@
   (->> (filter #(= :secret (keyword (:type %))) conn-props)
     (reduce (fn [acc prop] (assoc acc (:name prop) prop)) {})))
 
-(defn value->file!
+(defn value->file!*
   "Returns the value of the given `secret` instance in the form of a file. If the given instance has a `:file-path` as
   its source, a `File` referring to that is returned. Otherwise, the `:value` is written to a temporary file, which is
   then returned.
@@ -100,6 +101,24 @@
           (.write out v)))
       tmp-file)))
 
+(def
+  ^java.io.File
+  ^{:arglists '([{:keys [connection-property-name id value] :as secret} driver?])}
+  value->file!
+  "Returns the value of the given `secret` instance in the form of a file. If the given instance has a `:file-path` as
+  its source, a `File` referring to that is returned. Otherwise, the `:value` is written to a temporary file, which is
+  then returned.
+
+  `driver?` is an optional argument that is only used if an ostensibly existing file value (i.e. `:file-path`) can't be
+  resolved, in order to render a more user-friendly error message (by looking up the display names of the connection
+  properties involved)."
+  (memoize/memo
+   (with-meta value->file!*
+     {::memoize/args-fn (fn [[secret _driver?]]
+                          ;; not clear if value->string could return nil due to the cond so we'll just cache on a key
+                          ;; that is unique
+                          [(vec (:value secret))])})))
+
 (defn get-sub-props
   "Return a map of secret subproperties for the property `connection-property-name`."
   [connection-property-name]

test/metabase/driver/sql_jdbc/connection_test.clj

@@ -6,7 +6,7 @@
             [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
             [metabase.driver.sql-jdbc.test-util :as sql-jdbc.tu]
             [metabase.driver.util :as driver.u]
-            [metabase.models.database :refer [Database]]
+            [metabase.models :refer [Database Secret]]
             [metabase.sync :as sync]
             [metabase.test :as mt]
             [metabase.test.data :as data]
@@ -153,4 +153,24 @@
                     (is (not= db-hash-1 db-hash-2)))))))
           (finally
             ;; restore the original test DB details, no matter what just happened
-            (db/update! Database (mt/id) :details (:details db))))))))
+            (db/update! Database (mt/id) :details (:details db)))))))
+  (testing "postgres secrets are stable (#23034)"
+    (mt/with-temp* [Secret [secret {:name   "file based secret"
+                                    :kind   :perm-cert
+                                    :source nil
+                                    :value  (.getBytes "super secret")}]]
+      (let [db {:engine :postgres
+                :details {:ssl true
+                          :ssl-mode "verify-ca"
+                          :ssl-root-cert-options "uploaded"
+                          :ssl-root-cert-creator-id (mt/user->id :crowberto)
+                          :ssl-root-cert-source nil
+                          :ssl-root-cert-id (:id secret)
+                          :ssl-root-cert-created-at "2022-07-25T15:57:51.556-05:00"}}]
+        (is (instance? java.io.File
+                       (:sslrootcert (#'sql-jdbc.conn/connection-details->spec :postgres
+                                                                               (:details db))))
+            "Secrets not loaded for db connections")
+        (is (= (#'sql-jdbc.conn/jdbc-spec-hash db)
+               (#'sql-jdbc.conn/jdbc-spec-hash db))
+            "Same db produced different hashes due to secrets")))))

test/metabase/models/secret_test.clj

@@ -120,7 +120,30 @@
                                   (let [result (byte-array (.length val-file))]
                                     (with-open [in (DataInputStream. (io/input-stream val-file))]
                                       (.readFully in result))
-                                    result))))))))))
+                                    result)))))))))
+  (testing "value->file! returns the same file for secrets"
+    (testing "for file paths"
+      (let [file-secret-val "dingbat"
+            ^File tmp-file  (doto (File/createTempFile "value-to-file-test_" ".txt")
+                              (.deleteOnExit))]
+        (spit tmp-file file-secret-val)
+        (mt/with-temp* [Secret [secret {:name   "file based secret"
+                                        :kind   :perm-cert
+                                        :source "file-path"
+                                        :value  (.getAbsolutePath tmp-file)}]]
+          (is (instance? java.io.File (secret/value->file! secret nil)))
+          (is (= (secret/value->file! secret nil)
+                 (secret/value->file! secret nil))
+              "Secret did not return the same file"))))
+    (testing "for upload files (#23034)"
+      (mt/with-temp* [Secret [secret {:name   "file based secret"
+                                      :kind   :perm-cert
+                                      :source nil
+                                      :value  (.getBytes "super secret")}]]
+        (is (instance? java.io.File (secret/value->file! secret nil)))
+        (is (= (secret/value->file! secret nil)
+               (secret/value->file! secret nil))
+            "Secret did not return the same file")))))
 
 (defn- decode-ssl-db-property [content mime-type property]
   (let [value-key (keyword (str property "-value"))