Prevent secret records from being orphaned (#18714)

Delete Secret instances from the app DB, when the corresponding Database instance is deleted

Updating existing test (and renamed it to `secret-db-details-integration-test`) to confirm this behavior

src/metabase/models/database.clj

@@ -10,7 +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.models.secret :as secret :refer [Secret]]
             [metabase.plugins.classloader :as classloader]
             [metabase.util :as u]
             [metabase.util.i18n :refer [trs]]
@@ -60,12 +60,41 @@
           (driver/normalize-db-details driver db*)
           db*))))
 
-(defn- pre-delete [{id :id, driver :engine, :as database}]
+(defn- conn-props->secret-props-by-name
+  "For the given `conn-props` (output of `driver/connection-properties`), return a map of all secret type properties,
+  keyed by property name."
+  [conn-props]
+  (->> (filter #(= "secret" (:type %)) conn-props)
+    (reduce (fn [acc prop] (assoc acc (:name prop) prop)) {})))
+
+(defn- delete-orphaned-secrets!
+  "Delete Secret instances from the app DB, that will become orphaned when `database` is deleted. For now, this will
+  simply delete any Secret whose ID appears in the details blob, since every Secret instance that is currently created
+  is exclusively associated with a single Database.
+
+  In the future, if/when we allow arbitrary association of secret instances to database instances, this will need to
+  change and become more complicated (likely by consulting a many-to-many join table)."
+  [{:keys [id details] :as database}]
+  (when-let [conn-props-fn (get-method driver/connection-properties (driver.u/database->driver database))]
+    (let [conn-props                 (conn-props-fn (driver.u/database->driver database))
+          possible-secret-prop-names (keys (conn-props->secret-props-by-name conn-props))]
+      (doseq [secret-id (reduce (fn [acc prop-name]
+                                  (when-let [secret-id (get details (keyword (str prop-name "-id")))]
+                                    (conj acc secret-id)))
+                                []
+                                possible-secret-prop-names)]
+        (log/info (trs "Deleting secret ID {0} from app DB because the owning database ({1}) is being deleted"
+                       secret-id
+                       id))
+        (db/delete! Secret :id secret-id)))))
+
+(defn- pre-delete [{id :id, driver :engine, details :details :as database}]
   (unschedule-tasks! database)
   (db/execute! {:delete-from (db/resolve-model 'Permissions)
                 :where       [:or
                               [:like :object (str (perms/data-perms-path id) "%")]
                               [:= :object (perms/database-block-perms-path id)]]})
+  (delete-orphaned-secrets! database)
   (try
     (driver/notify-database-updated driver database)
     (catch Throwable e
@@ -115,10 +144,8 @@
 
           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)
+                conn-secrets-by-name  (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))

test/metabase/models/database_test.clj

@@ -8,7 +8,7 @@
             [metabase.models :refer [Database]]
             [metabase.models.database :as mdb]
             [metabase.models.permissions :as perms]
-            [metabase.models.secret :as secret]
+            [metabase.models.secret :as secret :refer [Secret]]
             [metabase.models.user :as user]
             [metabase.server.middleware.session :as mw.session]
             [metabase.task :as task]
@@ -161,43 +161,54 @@
     (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.)
+(deftest secret-db-details-integration-test
+  (testing "manipulating secret values in db-details works correctly"
+    (mt/with-driver :secret-test-driver
+      (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")
+                            (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 [secret-id                                  (:password-id details)
+                                  {:keys [created_at updated_at] :as secret} (secret/latest-for-id secret-id)]
+                              (swap! secret-ids conj secret-id)
+                              (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"})))))))))
+                                                   :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"}))))
+            (testing "Secret instances are deleted from the app DB when the DatabaseInstance is deleted"
+              (is (seq @secret-ids) "At least one Secret instance should have been created")
+              (doseq [secret-id @secret-ids]
+                (testing (format "Secret ID %d should have been deleted after the Database was" secret-id)
+                  (is (nil? (db/select-one Secret :id secret-id))
+                      (format "Secret ID %d was not removed from the app DB" secret-id)))))))))))