From f3c3667b38b568683cbc2d49735974514e68575e Mon Sep 17 00:00:00 2001
From: Allen Gilliland <agilliland@gmail.com>
Date: Fri, 13 Nov 2015 22:57:45 -0800
Subject: [PATCH] a few things happening here:
* remove the /api/database/validate endpoint
* introduce connection checking as part of the database api create/update endpoints directly
* modify the return of the Database model in the api so that if a password was provided we swap it out for a known value and prevent leaking of the real password via the api.
---
src/metabase/api/database.clj | 100 +++++++++++++++++++++----------
src/metabase/models/database.clj | 4 ++
2 files changed, 74 insertions(+), 30 deletions(-)
diff --git a/src/metabase/api/database.clj b/src/metabase/api/database.clj
index 1eaf29f7457..82b0256a81d 100644
--- a/src/metabase/api/database.clj
+++ b/src/metabase/api/database.clj
@@ -8,7 +8,7 @@
[metabase.events :as events]
(metabase.models common
[hydrate :refer [hydrate]]
- [database :refer [Database]]
+ [database :refer [Database protected-password]]
[field :refer [Field]]
[table :refer [Table]])
[metabase.sample-data :as sample-data]
@@ -19,6 +19,34 @@
[symb value :nillable]
(checkp-contains? (set (map name (keys @driver/available-drivers))) symb value))
+(defn test-database-connection
+ "Try out the connection details for a database and useful error message if connection fails, returns `nil` if connection succeeds."
+ [engine {:keys [host port] :as details}]
+ (let [engine (keyword engine)
+ details (assoc details :engine engine)
+ response-invalid (fn [field m] {:valid false
+ field m ; work with the new {:field error-message} format
+ :message m})] ; but be backwards-compatible with the UI as it exists right now
+ (try
+ (cond
+ (driver/can-connect-with-details? engine details :rethrow-exceptions) nil
+ (and host port (u/host-port-up? host port)) (response-invalid :dbname (format "Connection to '%s:%d' successful, but could not connect to DB." host port))
+ (and host (u/host-up? host)) (response-invalid :port (format "Connection to '%s' successful, but port %d is invalid." port))
+ host (response-invalid :host (format "'%s' is not reachable" host))
+ :else (response-invalid :db "Unable to connect to database."))
+ (catch Throwable e
+ (response-invalid :dbname (.getMessage e))))))
+
+(defn supports-ssl?
+ "Predicate function which determines if a given `engine` supports the `:ssl` setting."
+ [engine]
+ {:pre [(driver/is-engine? engine)]}
+ (let [driver-props (->> (driver/engine->driver engine)
+ :details-fields
+ (map :name)
+ set)]
+ (contains? driver-props "ssl")))
+
(defendpoint GET "/"
"Fetch all `Databases`."
[]
@@ -30,10 +58,26 @@
{name [Required NonEmptyString]
engine [Required DBEngine]
details [Required Dict]}
- ;; TODO - we should validate the contents of `details` here based on the engine
(check-superuser)
- (let-500 [new-db (ins Database :name name :engine engine :details details)]
- (events/publish-event :database-create new-db)))
+ ;; this function tries connecting over ssl and non-ssl to establish a connection
+ ;; if it succeeds it returns the `details` that worked, otherwise it returns an error
+ (let [try-connection (fn [engine details]
+ (let [error (test-database-connection engine details)]
+ (if (and error
+ (true? (:ssl details)))
+ (recur engine (assoc details :ssl false))
+ (or error details))))
+ details (if (supports-ssl? engine)
+ (assoc details :ssl true)
+ details)
+ details-or-error (try-connection engine details)]
+ (if-not (false? (:valid details-or-error))
+ ;; no error, proceed with creation
+ (let-500 [new-db (ins Database :name name :engine engine :details details-or-error)]
+ (events/publish-event :database-create new-db))
+ ;; failed to connect, return error
+ {:status 400
+ :body details-or-error})))
(defendpoint POST "/sample_dataset"
"Add the sample dataset as a new `Database`."
@@ -42,25 +86,6 @@
(sample-data/add-sample-dataset!)
(sel :one Database :is_sample true))
-;; Stub function that will eventually validate a connection string
-(defendpoint POST "/validate"
- "Validate that we can connect to a `Database`."
- [:as {{:keys [host port engine] :as details} :body}]
- (let [engine (keyword engine)
- details (assoc details :engine engine)
- response-invalid (fn [field m] {:status 400 :body {:valid false
- field m ; work with the new {:field error-message} format
- :message m}})] ; but be backwards-compatible with the UI as it exists right now
- (try
- (cond
- (driver/can-connect-with-details? engine details :rethrow-exceptions) {:valid true}
- (and host port (u/host-port-up? host port)) (response-invalid :dbname (format "Connection to '%s:%d' successful, but could not connect to DB." host port))
- (and host (u/host-up? host)) (response-invalid :port (format "Connection to '%s' successful, but port %d is invalid." port))
- host (response-invalid :host (format "'%s' is not reachable" host))
- :else (response-invalid :db "Unable to connect to database."))
- (catch Throwable e
- (response-invalid :dbname (.getMessage e))))))
-
(defendpoint GET "/:id"
"Get `Database` with ID."
[id]
@@ -69,13 +94,28 @@
(defendpoint PUT "/:id"
"Update a `Database`."
[id :as {{:keys [name engine details]} :body}]
- {name NonEmptyString, details Dict} ; TODO - check that engine is a valid choice
- (write-check Database id)
- (check-500 (upd-non-nil-keys Database id
- :name name
- :engine engine
- :details details))
- (Database id))
+ {name [Required NonEmptyString]
+ engine [Required DBEngine]
+ details [Required Dict]}
+ (check-superuser)
+ (let-404 [database (Database id)]
+ (let [details (if-not (= protected-password (:password details))
+ details
+ (assoc details :password (get-in database [:details :password])))
+ conn-error (test-database-connection engine details)]
+ (if-not conn-error
+ ;; no error, proceed with update
+ (do
+ ;; TODO: is there really a reason to let someone change the engine on an existing database?
+ ;; that seems like the kind of thing that will almost never work in any practical way
+ (check-500 (upd-non-nil-keys Database id
+ :name name
+ :engine engine
+ :details details))
+ (Database id))
+ ;; failed to connect, return error
+ {:status 400
+ :body conn-error}))))
(defendpoint DELETE "/:id"
"Delete a `Database`."
diff --git a/src/metabase/models/database.clj b/src/metabase/models/database.clj
index 22d203a3463..87f30a2a843 100644
--- a/src/metabase/models/database.clj
+++ b/src/metabase/models/database.clj
@@ -5,6 +5,9 @@
[metabase.models.interface :refer :all]
[metabase.util :as u]))
+(def ^:const protected-password
+ "**MetabasePass**")
+
(defrecord DatabaseInstance []
;; preserve normal IFn behavior so things like ((sel :one Database) :id) work correctly
clojure.lang.IFn
@@ -15,6 +18,7 @@
(api-serialize [this]
;; If current user isn't an admin strip out DB details which may include things like password
(cond-> this
+ (get-in this [:details :password]) (assoc-in [:details :password] protected-password)
(not (:is_superuser @*current-user*)) (dissoc :details))))
(extend-ICanReadWrite DatabaseInstance :read :always, :write :superuser)
--
GitLab