Skip to content
Snippets Groups Projects
Unverified Commit b9d8f70c authored by dpsutton's avatar dpsutton Committed by GitHub
Browse files

Revert "Mb db connection uri parsing (#14716)" (#14856) 

* Revert "Mb db connection uri parsing (#14716)"

This reverts commit b779333f.

* Readd the parsing of connection strings for db.env
parent 525c994b
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/db/env.clj
+ 34
− 155
View file @ b9d8f70c
......@@ -4,10 +4,6 @@
enviornment variables e.g. `MB_DB_TYPE`, `MB_DB_HOST`, etc. `MB_DB_CONNECTION_URI` is used preferentially if both
are specified.
The `MB_DB_CONNECTION_URI` is unparsed and passed to the jdbc driver with once exception: if it includes credentials
like `username:password@host:port`. In this case we parse this and log. This functionality will be removed at some
point so.
There are two ways we specify JDBC connection information in Metabase code:
1. As a 'connection details' map that is meant to be UI-friendly; this is the actual map we save when creating a
......@@ -22,15 +18,13 @@
Normally you should use the equivalent functions in `metabase.db.connection` which can be overridden rather than
using this namespace directly."
(:require [clojure.java.io :as io]
[clojure.string :as str]
[clojure.tools.logging :as log]
[clojure.walk :as walk]
[metabase.config :as config]
[metabase.db.spec :as db.spec]
[metabase.util :as u]
[metabase.util.i18n :refer [trs]]
[ring.util.codec :as codec])
(:import java.net.URI))
[ring.util.codec :as codec]))
(defn- get-db-file
"Takes a filename and converts it to H2-compatible filename."
......@@ -59,166 +53,50 @@
(get-db-file db-file-name)))))
(def ^:private jdbc-connection-regex
"Regex to be used only when `:mb-db-connection-uri` includes credentials like `username:password@host:port`."
#"^(jdbc:)?([^:/@]+)://(?:([^:/@]+)(?::([^:@]+))?@)?([^:@]+)(?::(\d+))?/([^/?]+)(?:\?(.*))?$")
(declare connection-details->jdbc-spec)
(defn- suspicious-postgres-details?
"If postgres connection seems iffy. #8908
https://github.com/metabase/metabase/issues/8908"
[details]
(and (= (:ssl details) "true")
(not (:sslmode details))))
(defn- parse-connection-string
"Parse a DB connection URI like
`postgres://cam@localhost.com:5432/cams_cool_db?ssl=true&sslfactory=org.postgresql.ssl.NonValidatingFactory` and
return a broken-out map. This should not be used unless the connection string uses the old style where the
credentials are included in the like `username:password@host:port`."
return a broken-out map."
[uri]
(when-let [[_ _ protocol user pass host port db query] (re-matches jdbc-connection-regex uri)]
(let [protocol (case (keyword protocol)
:postgres :postgres
:postgresql :postgres
:mysql :mysql
:h2 :h2)
details
(connection-details->jdbc-spec
protocol
(merge {:type protocol}
(case (keyword protocol)
:h2 {:db db}
{:user user
:password pass
:host host
:port port
:dbname db})
(some-> query
codec/form-decode
walk/keywordize-keys)))]
(u/prog1 (merge {:type (case (keyword protocol)
:postgres :postgres
:postgresql :postgres
:mysql :mysql
:h2 :h2)}
(case (keyword protocol)
:h2 {:db db}
{:user user
:password pass
:host host
:port port
:dbname db})
(some-> query
codec/form-decode
walk/keywordize-keys))
;; If someone is using Postgres and specifies `ssl=true` they might need to specify `sslmode=require`. Let's let
;; them know about that to make their lives a little easier. See
;; https://github.com/metabase/metabase/issues/8908 for more details.
{:connection details
:diags (cond-> #{:env.warning/inline-credentials}
(and (= protocol :postgres) (suspicious-postgres-details? details))
(conj :env.warning/postgres-ssl))})))
(defn- fixup-connection-string
"When we allow a raw connection string as our connection, we still perform a few fixups:
- ensure it begins with jdbc:
- we allow `postgres:` and must change that to `postgresql:`
- warn if postgres ssl settings might cause issues
Return is a map of {:connection string|spec :diags #{info or warnings}}"
[connection-uri]
(when connection-uri
(reduce (fn [{:keys [connection] :as m} {:keys [pred diag fix]}]
(if (pred connection)
(-> m (update :connection fix) (update :diags conj diag))
m))
{:connection connection-uri
:diags #{}}
[{:pred #(not (.startsWith ^String % "jdbc:"))
:diag :env.info/prepend-jdbc
:fix #(str "jdbc:" %)}
{:pred #(re-find #"postgres:" %)
:diag :env.info/change-to-postgresql
:fix #(str/replace % "postgres:" "postgresql:")}
{:pred (fn parse-query-for-postgres
[^String conn]
(when (re-find #"postgres(?:ql)?:" conn)
;; jdbc:postgresql: is an opaque URI not subject to further parsing. Strip that off and we can
;; use the structural .getQuery from the URI rather than parsing ourselves
(when-let [details (some-> (.getQuery (URI. (str/replace conn #"^jdbc:" "")))
codec/form-decode
walk/keywordize-keys)]
(suspicious-postgres-details? details))))
:fix identity
:diag :env.warning/postgres-ssl}])))
(defn old-credential-style?
"Parse a jdbc connection uri to check for older style credential passing like:
mysql://foo:password@172.17.0.2:3306/metabase"
[connection-uri]
(when connection-uri
;; strip the jdbc prefix off so its a parseable and not opaque URI
(let [uri (URI. (str/replace connection-uri #"^jdbc:" ""))]
;; this is how clojure.java.jdbc does it
(some? (.getUserInfo uri)))))
(defn- connection-from-jdbc-string
"If connection string uses the form `username:password@host:port`, use our custom parsing to return a jdbc spec and
warn about this deprecated behavior. If not, return the jdbc string as is since our parsing does not offer all of
the options of using a raw jdbc string.
Return is a map of {:connection string|spec :diags #{info or warnings}}"
[conn-string]
(when conn-string
(if (old-credential-style? conn-string)
;; prefer not parsing as we don't handle all features of connection strings
(parse-connection-string conn-string)
(fixup-connection-string conn-string))))
(defn- log-inline-credentials! []
(log/warn
(u/format-color 'red
(str
(trs "Warning: using credentials provided inline is deprecated. ")
(trs "Change to using the credentials as a query parameter: `?password=your-password&user=user`.")))))
(defn- log-postgres-ssl []
(log/warn (trs "Warning: Postgres connection string with `ssl=true` detected.")
(trs "You may need to add `?sslmode=require` to your application DB connection string.")
(trs "If Metabase fails to launch, please add it and try again.")
(trs "See https://github.com/metabase/metabase/issues/8908 for more details.")))
(defn- emit-diags! [diagnostic]
(case diagnostic
:env.info/change-to-postgresql (log/info (trs "Replaced 'postgres:' with 'postgresql:' in connection string"))
:env.info/prepend-jdbc (log/info (trs "Prepended 'jdbc:' onto connection string"))
:env.warning/inline-credentials (log-inline-credentials!)
:env.warning/postgres-ssl (log-postgres-ssl)
(log/warn (trs "Unknown diagnostic in db connection: {0}" diagnostic))))
(def ^:private connection-string-or-spec
"Uses `:mb-db-connection-uri` and is either:
- the jdbc connection string if it does not use inline credentials, or
- a db-spec parsed by this code if it does use inline credentials, or
- nil when this value is not set."
(delay (when-let [conn-uri (config/config-str :mb-db-connection-uri)]
(let [{:keys [connection diags]} (connection-from-jdbc-string conn-uri)]
(run! emit-diags! diags)
connection))))
(defn- connection-string-or-spec->db-type [x]
(cond
(map? x) (:type x)
(string? x)
(let [[_ subprotocol] (re-find #"^(?:jdbc:)?([^:]+):" x)]
(try
(case (keyword subprotocol)
:postgres :postgres
:postgresql :postgres
:mysql :mysql
:h2 :h2)
(catch java.lang.IllegalArgumentException e
(throw (ex-info (str (trs "Unsupported application database type: {0} is not currently supported."
(pr-str subprotocol))
" "
(trs "Check the value of MB_DB_CONNECTION_URI."))
{:subprotocol subprotocol})))))))
(def ^:private connection-string-db-type
(delay (connection-string-or-spec->db-type @connection-string-or-spec)))
;; them know about that to make their lives a little easier. See https://github.com/metabase/metabase/issues/8908
;; for more details.
(when (and (= (:type <>) :postgres)
(= (:ssl <>) "true")
(not (:sslmode <>)))
(log/warn (trs "Warning: Postgres connection string with `ssl=true` detected.")
(trs "You may need to add `?sslmode=require` to your application DB connection string.")
(trs "If Metabase fails to launch, please add it and try again.")
(trs "See https://github.com/metabase/metabase/issues/8908 for more details."))))))
(def ^:private connection-string-details
(delay (when-let [uri (config/config-str :mb-db-connection-uri)]
(parse-connection-string uri))))
(def db-type
"Keyword type name of the application DB details specified by environment variables. Matches corresponding driver
name e.g. `:h2`, `:mysql`, or `:postgres`."
(delay
(or @connection-string-db-type
(or (:type @connection-string-details)
(config/config-kw :mb-db-type))))
(def db-connection-details
......@@ -260,5 +138,6 @@
(def jdbc-spec
"`clojure.java.jdbc` spec map for the application DB, using the details map derived from environment variables."
(delay
(or @connection-string-or-spec
(or (when-let [details @connection-string-details]
(connection-details->jdbc-spec @db-type details))
(connection-details->jdbc-spec @db-type @db-connection-details))))
test/metabase/db/env_test.clj
+ 39
− 101
View file @ b9d8f70c
......@@ -2,107 +2,45 @@
(:require [clojure.test :refer :all]
[metabase.db.env :as mdb.env]))
(deftest connection-string-or-spec->db-type-test
(doseq [[subprotocol expected] {"postgres" :postgres
"postgresql" :postgres
"mysql" :mysql
"h2" :h2}
protocol [subprotocol (str "jdbc:" subprotocol)]
url [(str protocol "://abc")
(str protocol ":abc")
(str protocol ":cam@localhost/my_db?password=123456")
(str protocol "://localhost/my_db")]]
(testing (pr-str (list 'connection-string-or-spec->db-type url))
(is (= expected
(#'mdb.env/connection-string-or-spec->db-type url)))))
(testing "Should throw an Exception for an unsupported subprotocol"
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
#"Unsupported application database type: \"sqlserver\""
(#'mdb.env/connection-string-or-spec->db-type "jdbc:sqlserver://bad"))))
(testing "Finds type from jdbc-spec"
(let [spec {:password "password",
:characterSetResults "UTF8",
:characterEncoding "UTF8",
:type :mysql,
:classname "org.mariadb.jdbc.Driver",
:subprotocol "mysql",
:useSSL false,
:zeroDateTimeBehavior "convertToNull",
:user "foo",
:subname "//172.17.0.2:3306/metabase",
:useCompression true,
:useUnicode true}]
(is (= :mysql (#'mdb.env/connection-string-or-spec->db-type spec))))))
(deftest parse-connection-string-test
(testing "can parse with/without jdbc, uri-encoded passwords, passwords in host, etc"
(are [conn expected] (= expected (#'mdb.env/parse-connection-string conn))
;; password in host
"postgres://dan:password@localhost:5432/metabase"
{:type :postgres, :user "dan", :password "password",
:host "localhost", :port "5432", :dbname "metabase"}
(deftest fixup-connection-string-test
(let [correct-uri "jdbc:postgresql://localhost:metabase?username=johndoe"]
(doseq [[input expected-diags expected]
[["postgres://localhost:metabase?username=johndoe"
#{:env.info/prepend-jdbc :env.info/change-to-postgresql}
correct-uri]
["jdbc:postgres://localhost:metabase?username=johndoe"
#{:env.info/change-to-postgresql}
correct-uri]
[correct-uri #{} correct-uri]
[nil nil nil]]]
(let [{:keys [connection diags]} (#'mdb.env/fixup-connection-string input)]
(is (= expected connection) input)
(is (= expected-diags diags) input)))))
;; password in params
"postgresql://localhost:5432/metabase?user=dan&password=password"
{:type :postgres, :user "dan", :password "password",
:host "localhost", :port "5432", :dbname "metabase"}
(deftest old-credential-style?-test
(are [old? conn-uri] (is (= old? (#'mdb.env/old-credential-style? conn-uri)) conn-uri)
false "jdbc:mysql://172.17.0.2:3306/metabase?password=password&user=user"
false "mysql://172.17.0.2:3306/metabase?password=password&user=user"
;; prefixed with jdbc makes an "opaque" URI which doesn't parse
true "jdbc:mysql://foo@172.17.0.2:3306/metabase?password=password"
true "mysql://foo@172.17.0.2:3306/metabase?password=password"
true "mysql://foo:password@172.17.0.2:3306/metabase"))
;; includes uri encoded password
"mysql://localhost/metabase?user=newuser&password=metabase%21%40%2540%26amp%3B-%2F%3A"
{:type :mysql, :user "newuser", :password "metabase!@%40&amp;-/:",
:host "localhost", :port nil, :dbname "metabase"}
(deftest connection-from-jdbc-string-test
(let [parsed {:classname "org.mariadb.jdbc.Driver"
:subprotocol "mysql"
:subname "//172.17.0.2:3306/metabase"
:type :mysql
:user "foo"
:password "password"
:dbname "metabase"}]
(testing "handles mixed (just password in query params) old style just fine"
(let [conn-uri "mysql://foo@172.17.0.2:3306/metabase?password=password"]
(is (= {:connection parsed
:diags #{:env.warning/inline-credentials}}
(#'mdb.env/connection-from-jdbc-string conn-uri)))))
(testing "When using old-style passwords parses and returns jdbc specs"
(let [conn-uri "mysql://foo:password@172.17.0.2:3306/metabase"]
(is (= {:connection parsed
:diags #{:env.warning/inline-credentials}}
(#'mdb.env/connection-from-jdbc-string conn-uri))))))
(testing "handles credentials in query params"
(let [conn-uri "mysql://172.17.0.2:3306/metabase?user=user&password=password"]
(is (= {:connection (str "jdbc:" conn-uri)
:diags #{:env.info/prepend-jdbc}}
(#'mdb.env/connection-from-jdbc-string conn-uri)))))
(testing "warns about postgres ssl issue #8908"
(testing "when it parses due to inline credentials"
(let [conn-uri "postgresql://mb@172.17.0.2:5432/metabase?ssl=true"]
(is (= {:connection
{:ssl "true",
:OpenSourceSubProtocolOverride true,
:password nil,
:type :postgres,
:classname "org.postgresql.Driver",
:subprotocol "postgresql",
:dbname "metabase",
:user "mb",
:subname "//172.17.0.2:5432/metabase"},
:diags #{:env.warning/postgres-ssl :env.warning/inline-credentials}}
(#'mdb.env/connection-from-jdbc-string conn-uri)))))
(testing "when it doesn't parse as well"
(doseq [conn-uri ["jdbc:postgresql://172.17.0.2:5432/metabase?user=mb&password=pw&ssl=true"
"postgresql://172.17.0.2:5432/metabase?user=mb&password=pw&ssl=true"
"postgres://172.17.0.2:5432/metabase?user=mb&password=pw&ssl=true"]]
(let [{:keys [connection diags]} (#'mdb.env/connection-from-jdbc-string conn-uri)]
(is (= "jdbc:postgresql://172.17.0.2:5432/metabase?user=mb&password=pw&ssl=true" connection))
(is (contains? diags :env.warning/postgres-ssl))))))
(testing "handles nil"
(is (nil? (#'mdb.env/connection-from-jdbc-string nil)))))
;; extra options ssl=true
"jdbc:postgresql://localhost/test?user=fred&password=secret&ssl=true"
{:type :postgres, :user "fred", :password "secret",
:host "localhost", :port nil, :dbname "test", :ssl "true"}
;; copied from previous tests
"postgres://localhost/toms_cool_db"
{:type :postgres, :user nil, :password nil, :host "localhost", :port nil, :dbname "toms_cool_db" }
"postgresql://localhost/toms_cool_db"
{:type :postgres, :user nil, :password nil, :host "localhost", :port nil, :dbname "toms_cool_db"}
(str "postgres://tom:1234@localhost:5432/toms_cool_db"
"?ssl=true&sslfactory=org.postgresql.ssl.NonValidatingFactory")
{:type :postgres, :user "tom", :password "1234", :host "localhost", :port "5432", :dbname "toms_cool_db",
:ssl "true", :sslfactory "org.postgresql.ssl.NonValidatingFactory"}
(str "jdbc:postgres://tom:1234@localhost:5432/toms_cool_db"
"?ssl=true&sslfactory=org.postgresql.ssl.NonValidatingFactory")
{:type :postgres, :user "tom", :password "1234", :host "localhost", :port "5432", :dbname "toms_cool_db",
:ssl "true", :sslfactory "org.postgresql.ssl.NonValidatingFactory"}))
(testing "throws if unsupported db type"
(let [invalid-conn "oracle://dan:password@localhost:5432/metabase"]
(is (thrown? Throwable (#'mdb.env/parse-connection-string invalid-conn))))))
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