Skip to content
Snippets Groups Projects
Commit 46de1271 authored by Allen Gilliland's avatar Allen Gilliland
Browse files

Merge pull request #1476 from metabase/dont_return_db_password_in_api 

Dont return db password in api
parents cfac5b54 96d48237
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
frontend/src/admin/databases/databases.controllers.js
+ 2
− 34
View file @ 46de1271
......@@ -102,42 +102,10 @@ DatabasesControllers.controller('DatabaseEdit', ['$scope', '$routeParams', '$loc
};
var save = function(database, details) {
// validate_connection needs engine so add it to request body
details.engine = database.engine;
function handleError(error) {
$scope.$broadcast("form:api-error", error);
throw error;
}
// for an existing DB check that connection is valid before save
if ($routeParams.databaseId) {
return Metabase.validate_connection(details).$promise.catch(handleError).then(function() {
return update(database, details);
});
// for a new DB we want to infer SSL support. First try to connect w/ SSL. If that fails, disable SSL
return update(database, details);
} else {
const engineSupportsSSL = _.contains(_.map($scope.engines[database.engine]['details-fields'], 'name'),
'ssl');
function createDB() {
console.log('Successfully connected to database with SSL = ' + details.ssl + '.');
return create(database, details);
}
// if the engine supports SSL, try connecting with SSL first, and then without
if (engineSupportsSSL) {
details.ssl = true;
return Metabase.validate_connection(details).$promise.catch(function() {
console.log('Unable to connect with SSL. Trying with SSL = false.');
details.ssl = false;
return Metabase.validate_connection(details).$promise;
}).then(createDB).catch(handleError);
} else {
delete details.ssl;
return Metabase.validate_connection(details).$promise.catch(handleError).then(createDB);
}
return create(database, details);
}
};
......
frontend/src/services.js
+ 0
− 4
View file @ 46de1271
......@@ -392,10 +392,6 @@ CoreServices.factory('Metabase', ['$resource', '$cookies', 'MetabaseCore', funct
url: '/api/database/',
method: 'POST'
},
validate_connection: {
url: '/api/database/validate/',
method: 'POST'
},
db_add_sample_dataset: {
url: '/api/database/sample_dataset',
method: 'POST'
......
src/metabase/api/database.clj
+ 71
− 30
View file @ 46de1271
......@@ -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,35 @@
[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}]
(when (not (metabase.config/is-test?))
(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 +59,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 +87,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 +95,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`."
......
src/metabase/models/database.clj
+ 4
− 0
View file @ 46de1271
......@@ -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)
......
test/metabase/api/database_test.clj
+ 7
− 12
View file @ 46de1271
......@@ -19,7 +19,8 @@
:details {:host "localhost"
:port 5432
:dbname "fakedb"
:user "cam"}}))
:user "cam"
:ssl false}}))
;; # DB LIFECYCLE ENDPOINTS
......@@ -59,7 +60,7 @@
{:created_at $
:engine "postgres" ; string because it's coming back from API instead of DB
:id $
:details {:host "localhost", :port 5432, :dbname "fakedb", :user "cam"}
:details {:host "localhost", :port 5432, :dbname "fakedb", :user "cam", :ssl true}
:updated_at $
:name db-name
:is_sample false
......@@ -83,23 +84,17 @@
(expect-let [[old-name new-name] (repeatedly 2 random-name)
{db-id :id} (create-db old-name)
sel-db (fn [] (sel :one :fields [Database :name :engine :details] :id db-id))]
[{:details {:host "localhost", :port 5432, :dbname "fakedb", :user "cam"}
[{:details {:host "localhost", :port 5432, :dbname "fakedb", :user "cam", :ssl true}
:engine :postgres
:name old-name}
{:details {:host "localhost", :port 5432, :dbname "fakedb", :user "rastacan"}
:engine :h2
:name new-name}
{:details {:host "localhost", :port 5432, :dbname "fakedb", :user "rastacan"}
:engine :h2
:name old-name}]
:name new-name}]
[(sel-db)
;; Check that we can update all the fields
(do ((user->client :crowberto) :put 200 (format "database/%d" db-id) {:name new-name
:engine "h2"
:details {:host "localhost", :port 5432, :dbname "fakedb", :user "rastacan"}})
(sel-db))
;; Check that we can update just a single field
(do ((user->client :crowberto) :put 200 (format "database/%d" db-id) {:name old-name})
:engine "h2"
:details {:host "localhost", :port 5432, :dbname "fakedb", :user "rastacan"}})
(sel-db))])
;; # DATABASES FOR ORG
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment