Support `MB_DB_USER` and/or `MB_DB_PASS` in combination with `MB_DB_CONNECTION_URI` (#20135)

docs/operations-guide/configuring-application-database.md

@@ -50,17 +50,26 @@ You can change the application database to use Postgres using a few simple envir
 
 Metabase will not create this database for you. Example command to create the database:
 
-    createdb --encoding=UTF8 -e metabase 
+    createdb --encoding=UTF8 -e metabase
 
-This will tell Metabase to look for its application database using the supplied Postgres connection information. Metabase also supports providing a full JDBC connection URI if you have additional parameters:
+This will tell Metabase to look for its application database using the supplied Postgres connection information.
+Metabase also supports providing a full JDBC connection string if you have additional parameters:
 
-    export MB_DB_CONNECTION_URI="postgres://localhost:5432/metabase?user=<username>&password=<password>"
+    export MB_DB_CONNECTION_URI="jdbc:postgresql://localhost:5432/metabase?user=<username>&password=<password>"
+    java -jar metabase.jar
+
+`MB_DB_CONNECTION_URI` can also be used in combination with `MB_DB_USER` and/or `MB_DB_PASS` if you want to pass one
+or both separately from the rest of the JDBC connection string (useful if the password contains special characters):
+
+    export MB_DB_CONNECTION_URI="jdbc:postgresql://localhost:5432/metabase"
+    export MB_DB_USER=<username>
+    export MB_DB_PASS=<password>
     java -jar metabase.jar
 
 
 #### Upgrading from a Metabase version pre-0.38
 
-If you’re upgrading from a previous version of Metabase, note that for Metabase 0.38 we've removed the use of the PostgreSQL `NonValidatingFactory` for SSL validation. It’s possible that you could experience a failure either at startup (if you're using a PostgreSQL application database) or when querying a PostgreSQL data warehouse. 
+If you’re upgrading from a previous version of Metabase, note that for Metabase 0.38 we've removed the use of the PostgreSQL `NonValidatingFactory` for SSL validation. It’s possible that you could experience a failure either at startup (if you're using a PostgreSQL application database) or when querying a PostgreSQL data warehouse.
 
 You can resolve this failure in one of two ways:
 
@@ -85,7 +94,7 @@ export MB_DB_CONNECTION_URI="postgres://localhost:5432/metabase?user=<username>&
 
 **For Postgres data warehouse databases**
 
-You can do the same inside the Metabase Admin page for the connection to your Postgres database. Add the following to the end of your JDBC connection URI for your database:
+You can do the same inside the Metabase Admin page for the connection to your Postgres database. Add the following to the end of your JDBC connection string for your database:
 
 ```
 &sslmode=verify-ca&sslrootcert=<path to CA root or intermediate root certificate>
@@ -97,13 +106,15 @@ If that does not work, you can enable `NonValidatingFactory` by adding the follo
 &ssl=true&sslfactory=org.postgresql.ssl.NonValidatingFactory
 ```
 
-For more options to further tune the SSL connection parameters, 
+For more options to further tune the SSL connection parameters,
 see the [PostgreSQL SSL client documentation](https://jdbc.postgresql.org/documentation/head/ssl-client.html).
 
 
 #### [MySQL](https://www.mysql.com/) or [MariaDB](https://www.mariadb.org/)
 
-If you prefer to use MySQL or MariaDB we've got you covered. The minimum recommended version is MySQL 5.7.7 or MariaDB 10.2.2, and the `utf8mb4` character set is required. You can change the application database to use MySQL using environment variables like this:
+If you prefer to use MySQL or MariaDB we've got you covered. The minimum recommended version is MySQL 5.7.7 or MariaDB
+10.2.2, and the `utf8mb4` character set is required. You can change the application database to use MySQL using
+environment variables like this:
 
     export MB_DB_TYPE=mysql
     export MB_DB_DBNAME=metabase
@@ -116,8 +127,17 @@ If you prefer to use MySQL or MariaDB we've got you covered. The minimum recomme
 Metabase will not create this database for you. Example SQL statement to create the database:
 
     CREATE DATABASE metabase CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci;
-    
-This will tell Metabase to look for its application database using the supplied MySQL connection information. Metabase also supports providing a full JDBC connection URI if you have additional parameters:
 
-    export MB_DB_CONNECTION_URI="mysql://localhost:3306/metabase?user=<username>&password=<password>"
+This will tell Metabase to look for its application database using the supplied MySQL connection information. Metabase
+also supports providing a full JDBC connection string if you have additional parameters:
+
+    export MB_DB_CONNECTION_URI="jdbc:mysql://localhost:3306/metabase?user=<username>&password=<password>"
+    java -jar metabase.jar
+
+As with Postgres, `MB_DB_CONNECTION_URI` can also be used in combination with `MB_DB_USER` and/or `MB_DB_PASS` if you
+want to pass one or both separately from the rest of the JDBC connection string:
+
+    export MB_DB_CONNECTION_URI="jdbc:mysql://localhost:5432/metabase"
+    export MB_DB_USER=<username>
+    export MB_DB_PASS=<password>
     java -jar metabase.jar

src/metabase/cmd/dump_to_h2.clj

@@ -23,7 +23,7 @@
   "Transfer data from existing database specified by connection string to the H2 DB specified by env vars. Intended as a
   tool for migrating from one instance to another using H2 as serialization target.
 
-  Defaults to using `@metabase.db.env/db-file` as the connection string.
+  Defaults to using [[metabase.db.env/db-file]] as the connection string.
 
   Target H2 DB will be deleted if it exists, unless `keep-existing?` is truthy."
   ([h2-filename]

src/metabase/cmd/load_from_h2.clj

@@ -26,9 +26,9 @@
   "Transfer data from existing H2 database to a newly created (presumably MySQL or Postgres) DB. Intended as a tool for
   upgrading from H2 to a 'real' database.
 
-  Defaults to using `@metabase.db.env/db-file` as the source H2 database if `h2-filename` is `nil`."
+  Defaults to using [[metabase.db.env/db-file]] as the source H2 database if `h2-filename` is `nil`."
   ([]
-   (load-from-h2! @mdb.env/db-file))
+   (load-from-h2! (mdb.env/db-file)))
   ([h2-filename]
    (let [h2-filename    (str h2-filename ";IFEXISTS=TRUE")
          h2-data-source (copy.h2/h2-data-source h2-filename)]

src/metabase/db/data_source.clj

 (ns metabase.db.data-source
   (:require [clojure.set :as set]
             [clojure.string :as str]
+            [clojure.tools.logging :as log]
             [metabase.config :as config]
             [metabase.connection-pool :as pool]
             [metabase.db.spec :as db.spec]
@@ -32,36 +33,51 @@
   (equals [_ another]
     (and (instance? DataSource another)
          (= (.url ^DataSource another) url)
-         (= (.properties ^DataSource another) properties))))
+         (= (.properties ^DataSource another) properties)))
+
+  (toString [this]
+    (pr-str (pretty/pretty this))))
 
 (alter-meta! #'->DataSource assoc :private true)
 
 (defn raw-connection-string->DataSource
   "Return a [[javax.sql.DataSource]] given a raw JDBC connection string."
-  ^javax.sql.DataSource [s]
-  {:pre [(string? s)]}
-  ;; normalize the protocol in case someone is trying to trip us up. Heroku is known for this and passes stuff in
-  ;; like `postgres:...` to screw with us.
-  (let [s (cond-> s
-            (str/starts-with? s "postgres:")   (str/replace-first #"^postgres:" "postgresql:")
-            (not (str/starts-with? s "jdbc:")) (str/replace-first #"^" "jdbc:"))]
-    ;; Even tho they're invalid we need to handle strings like `postgres://user:password@host:port` for legacy reasons.
-    ;; (I think this is also how some places like Heroku ship them in order to make our lives hard) So strip those out
-    ;; with the absolute minimum of parsing we can get away with and then pass them in separately -- see #14678 and
-    ;; #20121
-    ;;
-    ;; NOTE: if password is URL-encoded this isn't going to work, since we're not URL-decoding it. I don't think that's
-    ;; a problem we really have to worry about, and at any rate we have never supported it. We did URL-decode things at
-    ;; one point, but that was only because [[clojure.java.jdbc]] tries to parse connection strings itself if you let it
-    ;; -- see #14836. We never let it see connection strings anymore, so that shouldn't be a problem. At any rate #20122
-    ;; would probably solve most people's problems if their password contains special characters.
-    (if-let [[_ subprotocol user password more] (re-find #"^jdbc:((?:postgresql)|(?:mysql))://([^:@]+)(?::([^@:]+))?@(.+$)" s)]
-      (->DataSource (str "jdbc:" subprotocol "://" more)
-                    (pool/map->properties
-                     (merge {:user user}
-                            (when (seq password)
-                              {:password password}))))
-      (->DataSource s nil))))
+  (^javax.sql.DataSource [s]
+   (raw-connection-string->DataSource s nil nil))
+
+  (^javax.sql.DataSource [s username password]
+   {:pre [(string? s)]}
+   ;; normalize the protocol in case someone is trying to trip us up. Heroku is known for this and passes stuff in
+   ;; like `postgres:...` to screw with us.
+   (let [s     (cond-> s
+                 (str/starts-with? s "postgres:")   (str/replace-first #"^postgres:" "postgresql:")
+                 (not (str/starts-with? s "jdbc:")) (str/replace-first #"^" "jdbc:"))
+         ;; Even tho they're invalid we need to handle strings like `postgres://user:password@host:port` for legacy
+         ;; reasons. (I think this is also how some places like Heroku ship them in order to make our lives hard) So
+         ;; strip those out with the absolute minimum of parsing we can get away with and then pass them in separately
+         ;; -- see #14678 and #20121
+         ;;
+         ;; NOTE: if password is URL-encoded this isn't going to work, since we're not URL-decoding it. I don't think
+         ;; that's a problem we really have to worry about, and at any rate we have never supported it. We did
+         ;; URL-decode things at one point, but that was only because [[clojure.java.jdbc]] tries to parse connection
+         ;; strings itself if you let it -- see #14836. We never let it see connection strings anymore, so that
+         ;; shouldn't be a problem. At any rate #20122 would probably solve most people's problems if their password
+         ;; contains special characters.
+         [s m] (if-let [[_ subprotocol user password more] (re-find #"^jdbc:((?:postgresql)|(?:mysql))://([^:@]+)(?::([^@:]+))?@(.+$)" s)]
+                 [(str "jdbc:" subprotocol "://" more)
+                  (merge {:user user}
+                         (when (seq password)
+                           {:password password}))]
+                 [s nil])
+         ;; these can't be i18n'ed because the app DB isn't set up yet
+         _     (when (and (:user m) (seq username))
+                 (log/error "Connection string contains a username, but MB_DB_USER is specified. MB_DB_USER will be used."))
+         _     (when (and (:password m) (seq password))
+                 (log/error "Connection string contains a password, but MB_DB_PASS is specified. MB_DB_PASS will be used."))
+         m     (cond-> m
+                 (seq username) (assoc :user username)
+                 (seq password) (assoc :password password))]
+     (->DataSource s (some-> (not-empty m) pool/map->properties)))))
 
 (defn broken-out-details->DataSource
   "Return a [[javax.sql.DataSource]] given a broken-out Metabase connection details."

src/metabase/db/env.clj

@@ -4,17 +4,20 @@
   enviornment variables e.g. `MB_DB_TYPE`, `MB_DB_HOST`, etc. `MB_DB_CONNECTION_URI` is used preferentially if both
   are specified.
 
-  There are two ways we specify JDBC connection information in Metabase code:
+  There are three ways you can specify application JDBC connection information for Metabase:
 
-  1. As a 'connection details' map that is meant to be UI-friendly; this is the actual `:details` map we save when creating a
-     [[metabase.models.Database]] object and the one you can go edit from the admin page. For application DB code,
-     this representation is only used in this namespace.
+  1. As broken-out connection details -- see [[env]] for a list of env vars. This is basically the same
+    format the actual `:details` map we save when creating a [[metabase.models.Database]] object. We convert this to
+    a [[clojure.java.jdbc]] spec map using [[metabase.db.spec/spec]] and then to create a [[javax.sql.DataSource]] from
+    it. See [[mdb.data-source/broken-out-details->DataSource]].
 
-  2. As a [[clojure.java.jdbc]] connection spec map. This is used internally by lower-level JDBC stuff. We have to
-     convert the connections details maps to JDBC specs at some point; Metabase driver code normally handles this.
+  2. As a JDBC connection string specified by `MB_DB_CONNECTION_URI`. This is used to create
+     a [[javax.sql.DataSource]]. See [[mdb.data-source/raw-connection-string->DataSource]].
 
-  There are functions for fetching both types of connection details below.
+  3. As a JDBC connection string (`MB_DB_CONNECTION_URI`) with username (`MB_DB_USER`) and/or password (`MB_DB_PASS`)
+     passed separately. Support for this was added in Metabase 0.43.0 -- see #20122.
 
+  This namespace exposes the vars [[db-type]] and [[data-source]] based on the aforementioned environment variables.
   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]
@@ -24,6 +27,24 @@
             [metabase.db.data-source :as mdb.data-source]
             [metabase.util :as u]))
 
+;;;; [[env->db-type]]
+
+(defn- raw-connection-string->type [s]
+  (when (seq s)
+    (when-let [[_protocol subprotocol] (re-find #"^(?:jdbc:)?([^:]+):" s)]
+      (condp = subprotocol
+        "postgresql" :postgres
+        (keyword subprotocol)))))
+
+(defn- env->db-type
+  [{:keys [mb-db-connection-uri mb-db-type]}]
+  {:post [(#{:postgres :mysql :h2} %)]}
+  (or (some-> mb-db-connection-uri raw-connection-string->type)
+      mb-db-type))
+
+
+;;;; [[env->DataSource]]
+
 (defn- get-db-file
   "Takes a filename and converts it to H2-compatible filename."
   [db-file-name]
@@ -38,47 +59,52 @@
          (.getAbsolutePath (io/file db-file-name))
          options)))
 
-(def db-file
-  "Path to our H2 DB file from env var or app config."
+(defn- env->db-file
+  [{:keys [mb-db-in-memory mb-db-file]}]
   ;; see https://h2database.com/html/features.html for explanation of options
-  (if (config/config-bool :mb-db-in-memory)
+  (if mb-db-in-memory
     ;; In-memory (i.e. test) DB
     ;; DB_CLOSE_DELAY=-1 = don't close the Database until the JVM shuts down
     "mem:metabase;DB_CLOSE_DELAY=-1"
     ;; File-based DB
-    (let [db-file-name (config/config-str :mb-db-file)]
-      (get-db-file db-file-name))))
+    (get-db-file mb-db-file)))
 
-(def ^:private raw-connection-string
-  (config/config-str :mb-db-connection-uri))
+(defn- broken-out-details
+  "Connection details that can be used when pretending the Metabase DB is itself a `Database` (e.g., to use the Generic
+  SQL driver functions on the Metabase DB itself)."
+  [db-type {:keys [mb-db-dbname mb-db-host mb-db-pass mb-db-port mb-db-user], :as env-vars}]
+  (if (= db-type :h2)
+    {:db (env->db-file env-vars)}
+    {:host     mb-db-host
+     :port     mb-db-port
+     :db       mb-db-dbname
+     :user     mb-db-user
+     :password mb-db-pass}))
 
-(defn- raw-connection-string->type [s]
-  (when (seq s)
-    (when-let [[_protocol subprotocol] (re-find #"^(?:jdbc:)?([^:]+):" s)]
-      (condp = subprotocol
-        "postgresql" :postgres
-        (keyword subprotocol)))))
+(defn- env->DataSource
+  [db-type {:keys [mb-db-connection-uri mb-db-user mb-db-pass], :as env-vars}]
+  (if mb-db-connection-uri
+    (mdb.data-source/raw-connection-string->DataSource mb-db-connection-uri mb-db-user mb-db-pass)
+    (mdb.data-source/broken-out-details->DataSource db-type (broken-out-details db-type env-vars))))
 
-(def ^:private raw-connection-string-type
-  (raw-connection-string->type raw-connection-string))
 
-;; 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 #8908 for more details.
-(when (and (= raw-connection-string-type :postgres)
-           (str/includes? raw-connection-string "ssl=true")
-           (not (str/includes? raw-connection-string "sslmode=require")))
-  ;; Unfortunately this can't be i18n'ed because the application DB hasn't been initialized yet at the time we log this
-  ;; and thus the site locale is unavailable.
-  (log/warn (str/join " " ["Warning: Postgres connection string with `ssl=true` detected."
-                           "You may need to add `?sslmode=require` to your application DB connection string."
-                           "If Metabase fails to launch, please add it and try again."
-                           "See https://github.com/metabase/metabase/issues/8908 for more details."])))
+;;;; exports: [[db-type]], [[db-file]], and [[data-source]] created using enviornment variables.
+
+(def ^:private env
+  {:mb-db-type           (config/config-kw :mb-db-type)
+   :mb-db-in-memory      (config/config-bool :mb-db-in-memory)
+   :mb-db-file           (config/config-str :mb-db-file)
+   :mb-db-connection-uri (config/config-str :mb-db-connection-uri)
+   :mb-db-host           (config/config-str :mb-db-host)
+   :mb-db-port           (config/config-int :mb-db-port)
+   :mb-db-dbname         (config/config-str :mb-db-dbname)
+   :mb-db-user           (config/config-str :mb-db-user)
+   :mb-db-pass           (config/config-str :mb-db-pass)})
 
 (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`."
-  (or raw-connection-string-type
-      (config/config-kw :mb-db-type)))
+  (env->db-type env))
 
 (when (= db-type :h2)
   (log/warn
@@ -93,19 +119,24 @@
       "If you decide to continue to use H2, please be sure to back up the database file regularly."
       "For more information, see https://metabase.com/docs/latest/operations-guide/migrating-from-h2.html"]))))
 
-(def ^:private broken-out-details
-  "Connection details that can be used when pretending the Metabase DB is itself a `Database` (e.g., to use the Generic
-  SQL driver functions on the Metabase DB itself)."
-  (if (= db-type :h2)
-    {:db db-file}
-    {:host     (config/config-str :mb-db-host)
-     :port     (config/config-int :mb-db-port)
-     :db       (config/config-str :mb-db-dbname)
-     :user     (config/config-str :mb-db-user)
-     :password (config/config-str :mb-db-pass)}))
+(defn db-file
+  "Path to our H2 DB file from env var or app config."
+  []
+  (env->db-file env))
+
+;; 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 #8908 for more details.
+(when-let [raw-connection-string (not-empty (:mb-db-connection-uri env))]
+  (when (and (= db-type :postgres)
+             (str/includes? raw-connection-string "ssl=true")
+             (not (str/includes? raw-connection-string "sslmode=require")))
+    ;; Unfortunately this can't be i18n'ed because the application DB hasn't been initialized yet at the time we log
+    ;; this and thus the site locale is unavailable.
+    (log/warn (str/join " " ["Warning: Postgres connection string with `ssl=true` detected."
+                             "You may need to add `?sslmode=require` to your application DB connection string."
+                             "If Metabase fails to launch, please add it and try again."
+                             "See https://github.com/metabase/metabase/issues/8908 for more details."]))))
 
 (def ^javax.sql.DataSource data-source
   "A [[javax.sql.DataSource]] ultimately derived from the environment variables."
-  (if raw-connection-string
-    (mdb.data-source/raw-connection-string->DataSource raw-connection-string)
-    (mdb.data-source/broken-out-details->DataSource db-type broken-out-details)))
+  (env->DataSource db-type env))

test/metabase/db/data_source_test.clj

@@ -94,6 +94,26 @@
                   {"user" "cam"})
                  (mdb.data-source/raw-connection-string->DataSource (str subprotocol "://cam@localhost/metabase?password=1234")))))))))
 
+(deftest raw-connection-string-with-separate-username-and-password-test
+  (testing "Raw connection string should support separate username and/or password (#20122)"
+    (testing "username and password"
+      (is (= (->DataSource
+              "jdbc:postgresql://metabase"
+              {"user" "cam", "password" "1234"})
+             (mdb.data-source/raw-connection-string->DataSource "postgres://metabase" "cam" "1234"))))
+    (testing "username only"
+      (is (= (->DataSource
+              "jdbc:postgresql://metabase"
+              {"user" "cam"})
+             (mdb.data-source/raw-connection-string->DataSource "postgres://metabase" "cam" nil)
+             (mdb.data-source/raw-connection-string->DataSource "postgres://metabase" "cam" ""))))
+    (testing "password only"
+      (is (= (->DataSource
+              "jdbc:postgresql://metabase"
+              {"password" "1234"})
+             (mdb.data-source/raw-connection-string->DataSource "postgres://metabase" nil "1234")
+             (mdb.data-source/raw-connection-string->DataSource "postgres://metabase" "" "1234"))))))
+
 (deftest equality-test
   (testing "Two DataSources with the same URL should be equal"
     (is (= (mdb.data-source/raw-connection-string->DataSource "ABCD")

test/metabase/db/env_test.clj

 (ns metabase.db.env-test
   (:require [clojure.test :refer :all]
+            [metabase.db.data-source :as mdb.data-source]
             [metabase.db.env :as mdb.env]))
 
 (deftest raw-connection-string->type-test
@@ -8,3 +9,21 @@
     "postgres:wow"        :postgres
     "jdbc:postgresql:wow" :postgres
     "postgresql:wow"      :postgres))
+
+(deftest connection-string-data-source-test
+  (is (= (mdb.data-source/raw-connection-string->DataSource "jdbc:postgresql://metabase?user=cam&password=1234")
+         (#'mdb.env/env->DataSource :postgres {:mb-db-connection-uri "postgres://metabase?user=cam&password=1234"})
+         (#'mdb.env/env->DataSource :postgres {:mb-db-connection-uri "postgres://metabase?user=cam&password=1234", :mb-db-user "", :mb-db-pass ""})))
+
+  (testing "Raw connection string should support separate username and/or password (#20122)"
+    (testing "username and password"
+      (is (= (mdb.data-source/raw-connection-string->DataSource "jdbc:postgresql://metabase" "cam" "1234")
+             (#'mdb.env/env->DataSource :postgres {:mb-db-connection-uri "postgres://metabase", :mb-db-user "cam", :mb-db-pass "1234"}))))
+    (testing "username only"
+      (is (= (mdb.data-source/raw-connection-string->DataSource "jdbc:postgresql://metabase" "cam" nil)
+             (#'mdb.env/env->DataSource :postgres {:mb-db-connection-uri "postgres://metabase", :mb-db-user "cam"})
+             (#'mdb.env/env->DataSource :postgres {:mb-db-connection-uri "postgres://metabase", :mb-db-user "cam", :mb-db-pass ""}))))
+    (testing "password only"
+      (is (= (mdb.data-source/raw-connection-string->DataSource "jdbc:postgresql://metabase" nil "1234")
+             (#'mdb.env/env->DataSource :postgres {:mb-db-connection-uri "postgres://metabase", :mb-db-pass "1234"})
+             (#'mdb.env/env->DataSource :postgres {:mb-db-connection-uri "postgres://metabase", :mb-db-user  "", :mb-db-pass "1234"}))))))