From fa7a89d792d4581929f7ef4873dc21971ca3858a Mon Sep 17 00:00:00 2001
From: Jeff Evans <jeff303@users.noreply.github.com>
Date: Thu, 28 Oct 2021 21:29:25 -0500
Subject: [PATCH] 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 | 39 +++++++++--
test/metabase/models/database_test.clj | 91 +++++++++++++++-----------
2 files changed, 84 insertions(+), 46 deletions(-)
diff --git a/src/metabase/models/database.clj b/src/metabase/models/database.clj
index 1262ff2a1ff..508335a5fe4 100644
--- a/src/metabase/models/database.clj
+++ b/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))
diff --git a/test/metabase/models/database_test.clj b/test/metabase/models/database_test.clj
index ed55108e33b..d85f2736808 100644
--- a/test/metabase/models/database_test.clj
+++ b/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)))))))))))
--
GitLab