diff --git a/frontend/src/metabase/entities/databases/forms.js b/frontend/src/metabase/entities/databases/forms.js
index e0584efa8a4fc8c44c86f4ecafd54a912957e665..b060f4c91f92769f40656f426c8aec5c3ececf13 100644
--- a/frontend/src/metabase/entities/databases/forms.js
+++ b/frontend/src/metabase/entities/databases/forms.js
@@ -10,22 +10,22 @@ import MetadataSyncScheduleWidget from "metabase/admin/databases/components/widg
import CacheFieldValuesScheduleWidget from "metabase/admin/databases/components/widgets/CacheFieldValuesScheduleWidget";
const DATABASE_DETAIL_OVERRIDES = {
- "tunnel-enabled": (engine, details) => ({
+ "tunnel-enabled": (engine, details, id) => ({
title: t`Use an SSH-tunnel for database connections`,
description: t`Some database installations can only be accessed by connecting through an SSH bastion host. This option also provides an extra layer of security when a VPN is not available. Enabling this is usually slower than a direct connection.`,
}),
- "use-jvm-timezone": (engine, details) => ({
+ "use-jvm-timezone": (engine, details, id) => ({
title: t`Use the Java Virtual Machine (JVM) timezone`,
description: t`We suggest you leave this off unless you're doing manual timezone casting in many or most of your queries with this data.`,
}),
- "use-srv": (engine, details) => ({
+ "use-srv": (engine, details, id) => ({
title: t`Use DNS SRV when connecting`,
description: t`Using this option requires that provided host is a FQDN. If connecting to an Atlas cluster, you might need to enable this option. If you don't know what this means, leave this disabled.`,
}),
- "client-id": (engine, details) => ({
+ "client-id": (engine, details, id) => ({
description: getClientIdDescription(engine, details),
}),
- "auth-code": (engine, details) => ({
+ "auth-code": (engine, details, id) => ({
description: (
<div>
<div>{getAuthCodeLink(engine, details)}</div>
@@ -33,8 +33,13 @@ const DATABASE_DETAIL_OVERRIDES = {
</div>
),
}),
- "service-account-json": (engine, details) => ({
+ "service-account-json": (engine, details, id) => ({
validate: value => {
+ // this field is only required if this is a new entry
+ if (id) {
+ return null;
+ }
+
if (!value) {
return t`required`;
}
@@ -46,22 +51,22 @@ const DATABASE_DETAIL_OVERRIDES = {
return null;
},
}),
- "tunnel-private-key": (engine, details) => ({
+ "tunnel-private-key": (engine, details, id) => ({
title: t`SSH private key`,
placeholder: t`Paste the contents of your ssh private key here`,
type: "text",
}),
- "tunnel-private-key-passphrase": (engine, details) => ({
+ "tunnel-private-key-passphrase": (engine, details, id) => ({
title: t`Passphrase for the SSH private key`,
}),
- "tunnel-auth-option": (engine, details) => ({
+ "tunnel-auth-option": (engine, details, id) => ({
title: t`SSH Authentication`,
options: [
{ name: t`SSH Key`, value: "ssh-key" },
{ name: t`Password`, value: "password" },
],
}),
- "ssl-cert": (engine, details) => ({
+ "ssl-cert": (engine, details, id) => ({
title: t`Server SSL certificate chain`,
placeholder: t`Paste the contents of the server's SSL certificate chain here`,
type: "text",
@@ -234,7 +239,7 @@ function getFieldsForEngine(engine, details, id) {
horizontal: field.type === "boolean",
initial: field.default,
readOnly: field.readOnly || false,
- ...(overrides && overrides(engine, details)),
+ ...(overrides && overrides(engine, details, id)),
});
}
return fields;
diff --git a/src/metabase/api/database.clj b/src/metabase/api/database.clj
index 4de66157cc461ebbf2394660a8482d598d9c4f38..97379a3a8bb74d1655f1223f86115c6f23d979c1 100644
--- a/src/metabase/api/database.clj
+++ b/src/metabase/api/database.clj
@@ -3,6 +3,7 @@
(:require [clojure.string :as str]
[clojure.tools.logging :as log]
[compojure.core :refer [DELETE GET POST PUT]]
+ [medley.core :as m]
[metabase
[config :as config]
[driver :as driver]
@@ -533,6 +534,19 @@
;;; --------------------------------------------- PUT /api/database/:id ----------------------------------------------
+(defn upsert-sensitive-fields
+ "Replace any sensitive values not overriden in the PUT with the original values"
+ [database details]
+ (when details
+ (merge (:details database)
+ (reduce
+ (fn [details k]
+ (if (= protected-password (get details k))
+ (m/update-existing details k (constantly (get-in database [:details k])))
+ details))
+ details
+ database/sensitive-fields))))
+
(api/defendpoint PUT "/:id"
"Update a `Database`."
[id :as {{:keys [name engine details is_full_sync is_on_demand description caveats points_of_interest schedules
@@ -547,9 +561,7 @@
auto_run_queries (s/maybe s/Bool)}
(api/check-superuser)
(api/let-404 [database (Database id)]
- (let [details (if-not (= protected-password (:password details))
- details
- (assoc details :password (get-in database [:details :password])))
+ (let [details (upsert-sensitive-fields database details)
conn-error (when (some? details)
(assert (some? engine))
(test-database-connection engine details))
@@ -565,18 +577,18 @@
;; that seems like the kind of thing that will almost never work in any practical way
;; TODO - this means one cannot unset the description. Does that matter?
(api/check-500 (db/update-non-nil-keys! Database id
- (merge
- {:name name
- :engine engine
- :details details
- :is_full_sync full-sync?
- :is_on_demand (boolean is_on_demand)
- :description description
- :caveats caveats
- :points_of_interest points_of_interest
- :auto_run_queries auto_run_queries}
- (when schedules
- (schedule-map->cron-strings schedules)))))
+ (merge
+ {:name name
+ :engine engine
+ :details details
+ :is_full_sync full-sync?
+ :is_on_demand (boolean is_on_demand)
+ :description description
+ :caveats caveats
+ :points_of_interest points_of_interest
+ :auto_run_queries auto_run_queries}
+ (when schedules
+ (schedule-map->cron-strings schedules)))))
(let [db (Database id)]
(events/publish-event! :database-update db)
;; return the DB with the expanded schedules back in place
diff --git a/test/metabase/api/database_test.clj b/test/metabase/api/database_test.clj
index f0c3697d3cc02a93d8119739e60b236ee30abcda..214711b7cff96dd42864c823df4e3f8c27ce99f2 100644
--- a/test/metabase/api/database_test.clj
+++ b/test/metabase/api/database_test.clj
@@ -13,6 +13,7 @@
[metabase.driver.util :as driver.u]
[metabase.mbql.schema :as mbql.s]
[metabase.models
+ [database :as database :refer [protected-password]]
[permissions :as perms]
[permissions-group :as perms-group]]
[metabase.sync
@@ -197,16 +198,17 @@
:features (driver.u/features :h2)}
(into {} (db/select-one [Database :name :engine :details :is_full_sync], :id db-id)))))))))
- (testing "should be able to set `auto_run_queries`"
- (testing "when creating a Database"
- (is (= {:auto_run_queries false}
- (select-keys (create-db-via-api! {:auto_run_queries false}) [:auto_run_queries]))))
- (testing "when updating a Database"
- (mt/with-temp Database [{db-id :id} {:engine ::test-driver}]
- (let [updates {:auto_run_queries false}]
- ((mt/user->client :crowberto) :put 200 (format "database/%d" db-id) updates))
- (is (= false
- (db/select-one-field :auto_run_queries Database, :id db-id))))))))
+ (mt/with-log-level :info
+ (testing "should be able to set `auto_run_queries`"
+ (testing "when creating a Database"
+ (is (= {:auto_run_queries false}
+ (select-keys (create-db-via-api! {:auto_run_queries false}) [:auto_run_queries]))))
+ (testing "when updating a Database"
+ (mt/with-temp Database [{db-id :id} {:engine ::test-driver}]
+ (let [updates {:auto_run_queries false}]
+ ((mt/user->client :crowberto) :put 200 (format "database/%d" db-id) updates))
+ (is (= false
+ (db/select-one-field :auto_run_queries Database, :id db-id)))))))))
(deftest fetch-database-metadata-test
(testing "GET /api/database/:id/metadata"
@@ -941,3 +943,114 @@
(is (schema= [{:schema (s/eq schema-name)
s/Keyword s/Any}]
((mt/user->client :rasta) :get 200 url))))))))))))
+
+(deftest upsert-sensitive-values-test
+ (testing "empty maps are okay"
+ (is (= {}
+ (database-api/upsert-sensitive-fields {} {}))))
+ (testing "no details updates are okay"
+ (is (= nil
+ (database-api/upsert-sensitive-fields nil nil))))
+ (testing "fields are replaced"
+ (is (= {:use-service-account nil
+ :dataset-id "dacort"
+ :use-jvm-timezone false
+ :service-account-json "{\"foo\": \"bar\"}"
+ :password "foo"
+ :pass "bar"
+ :tunnel-pass "quux"
+ :tunnel-private-key "foobar"
+ :tunnel-private-key-passphrase "fooquux"
+ :access-token "foobarfoo"
+ :refresh-token "foobarquux"}
+ (database-api/upsert-sensitive-fields {:description nil
+ :name "customer success BQ"
+ :details {:use-service-account nil
+ :dataset-id "dacort"
+ :service-account-json "{}"
+ :use-jvm-timezone false
+ :password "password"
+ :pass "pass"
+ :tunnel-pass "tunnel-pass"
+ :tunnel-private-key "tunnel-private-key"
+ :tunnel-private-key-passphrase "tunnel-private-key-passphrase"
+ :access-token "access-token"
+ :refresh-token "refresh-token"}
+ :id 2}
+ {:service-account-json "{\"foo\": \"bar\"}"
+ :password "foo"
+ :pass "bar"
+ :tunnel-pass "quux"
+ :tunnel-private-key "foobar"
+ :tunnel-private-key-passphrase "fooquux"
+ :access-token "foobarfoo"
+ :refresh-token "foobarquux"}))))
+ (testing "no fields are replaced"
+ (is (= {:use-service-account nil
+ :dataset-id "dacort"
+ :use-jvm-timezone false
+ :service-account-json "{}"
+ :password "password"
+ :pass "pass"
+ :tunnel-pass "tunnel-pass"
+ :tunnel-private-key "tunnel-private-key"
+ :tunnel-private-key-passphrase "tunnel-private-key-passphrase"
+ :access-token "access-token"
+ :refresh-token "refresh-token"}
+ (database-api/upsert-sensitive-fields {:description nil
+ :name "customer success BQ"
+ :details {:use-service-account nil
+ :dataset-id "dacort"
+ :use-jvm-timezone false
+ :service-account-json "{}"
+ :password "password"
+ :pass "pass"
+ :tunnel-pass "tunnel-pass"
+ :tunnel-private-key "tunnel-private-key"
+ :tunnel-private-key-passphrase "tunnel-private-key-passphrase"
+ :access-token "access-token"
+ :refresh-token "refresh-token"}
+ :id 2}
+ {:service-account-json protected-password
+ :password protected-password
+ :pass protected-password
+ :tunnel-pass protected-password
+ :tunnel-private-key protected-password
+ :tunnel-private-key-passphrase protected-password
+ :access-token protected-password
+ :refresh-token protected-password}))))
+
+ (testing "only one field is replaced"
+ (is (= {:use-service-account nil
+ :dataset-id "dacort"
+ :use-jvm-timezone false
+ :service-account-json "{}"
+ :password "new-password"
+ :pass "pass"
+ :tunnel-pass "tunnel-pass"
+ :tunnel-private-key "tunnel-private-key"
+ :tunnel-private-key-passphrase "tunnel-private-key-passphrase"
+ :access-token "access-token"
+ :refresh-token "refresh-token"}
+ (database-api/upsert-sensitive-fields {:description nil
+ :name "customer success BQ"
+ :details {:use-service-account nil
+ :dataset-id "dacort"
+ :use-jvm-timezone false
+ :service-account-json "{}"
+ :password "password"
+ :pass "pass"
+ :tunnel-pass "tunnel-pass"
+ :tunnel-private-key "tunnel-private-key"
+ :tunnel-private-key-passphrase "tunnel-private-key-passphrase"
+ :access-token "access-token"
+ :refresh-token "refresh-token"}
+ :id 2}
+ {:service-account-json protected-password
+ :password "new-password"
+ :pass protected-password
+ :tunnel-pass protected-password
+ :tunnel-private-key protected-password
+ :tunnel-private-key-passphrase protected-password
+ :access-token protected-password
+ :refresh-token protected-password})))))
\ No newline at end of file