Fix mongo password handling (#38959)

* Fix mongo password handling

* Update test, add credential check

* Address the review remarks

modules/drivers/mongo/src/metabase/driver/mongo/connection.clj

@@ -10,7 +10,7 @@
    [metabase.util.log :as log]
    [metabase.util.ssh :as ssh])
   (:import
-   (com.mongodb ConnectionString MongoClientSettings MongoClientSettings$Builder)
+   (com.mongodb ConnectionString MongoClientSettings MongoClientSettings$Builder MongoCredential)
    (com.mongodb.connection SslSettings$Builder)))
 
 (set! *warn-on-reflection* true)
@@ -20,11 +20,8 @@
   nil)
 
 (defn db-details->connection-string
-  "Generate connection string from database details.
-
-   - `?authSource` is always prestent because we are using `dbname`.
-   - We let the user override options we are passing in by means of `additional-options`."
-  [{:keys [use-conn-uri conn-uri host port user authdb pass dbname additional-options use-srv ssl] :as _db-details}]
+  "Generate connection string from database details."
+  [{:keys [use-conn-uri conn-uri host port dbname additional-options use-srv ssl] :as _db-details}]
   ;; Connection string docs:
   ;; http://mongodb.github.io/mongo-java-driver/4.11/apidocs/mongodb-driver-core/com/mongodb/ConnectionString.html
   (if use-conn-uri
@@ -32,14 +29,12 @@
     (str
      (if use-srv "mongodb+srv" "mongodb")
      "://"
-     (when (seq user) (str user (when (seq pass) (str ":" pass)) "@"))
+     ;; credentials are added into MongoClientSettings in [[db-details->mongo-client-settings]]
      host
      (when (and (not use-srv) (some? port)) (str ":" port))
      "/"
      dbname
-     "?authSource=" (if (empty? authdb) "admin" authdb)
-     "&appName=" config/mb-app-id-string
-     "&connectTimeoutMS=" (driver.u/db-connection-timeout-ms)
+     "?connectTimeoutMS=" (driver.u/db-connection-timeout-ms)
      "&serverSelectionTimeoutMS=" (driver.u/db-connection-timeout-ms)
      (when ssl "&ssl=true")
      (when (seq additional-options) (str "&" additional-options)))))
@@ -65,17 +60,27 @@
 
 (defn db-details->mongo-client-settings
   "Generate `MongoClientSettings` from `db-details`. `ConnectionString` is generated and applied to
-   `MongoClientSettings$Builder` first. Then ssl context is udated in the `builder` object.
+   `MongoClientSettings$Builder` first. Then credentials are set and ssl context is updated in the `builder` object.
    Afterwards, `MongoClientSettings` are built using `.build`."
   ^MongoClientSettings
-  [{:keys [use-conn-uri ssl] :as db-details}]
+  [{:keys [authdb user pass use-conn-uri ssl] :as db-details}]
   (let [connection-string (-> db-details
                               db-details->connection-string
                               ConnectionString.)
         builder (com.mongodb.MongoClientSettings/builder)]
+    (.applicationName builder config/mb-app-id-string)
     (.applyConnectionString builder connection-string)
-    (when (and ssl (not use-conn-uri))
-      (maybe-add-ssl-context-to-builder! builder db-details))
+    (when-not use-conn-uri
+      ;; NOTE: authSource connection parameter is the second argument of `createCredential`. We currently set it only
+      ;;       when some credentials are used (ie. user is not empty), previously we did that in all cases. I've
+      ;;       manually verified that's not necessary.
+      (when (seq user)
+        (.credential builder
+                     (MongoCredential/createCredential user
+                                                       (or (not-empty authdb) "admin")
+                                                       (char-array pass))))
+      (when ssl
+        (maybe-add-ssl-context-to-builder! builder db-details)))
     (.build builder)))
 
 (defn do-with-mongo-client

modules/drivers/mongo/test/metabase/driver/mongo/connection_test.clj

@@ -2,17 +2,16 @@
   (:require
    [clojure.string :as str]
    [clojure.test :refer :all]
+   [metabase.config :as config]
    [metabase.driver.mongo.connection :as mongo.connection]
    [metabase.driver.mongo.database :as mongo.db]
    [metabase.driver.mongo.util :as mongo.util]
    [metabase.driver.util :as driver.u]
    [metabase.test :as mt])
   (:import
-   (com.mongodb ServerAddress)
+   (com.mongodb MongoCredential ServerAddress)
    (com.mongodb.client MongoDatabase)))
 
-;;;; TODO: move some tests to db-test?
-
 (set! *warn-on-reflection* true)
 
 (def ^:private mock-details
@@ -39,7 +38,12 @@
                     :use-srv true}]
     (testing "mongo+srv connection string used when :use-srv is thruthy"
       (is (str/includes? (mongo.connection/db-details->connection-string db-details)
-                         "mongodb+srv://test-user:test-pass@test-host.place.com/datadb?authSource=authdb")))
+                         "mongodb+srv://test-host.place.com/datadb"))
+      (let [settings (mongo.connection/db-details->mongo-client-settings db-details)
+            ^MongoCredential credential (.getCredential settings)]
+        (is (= "test-user" (.getUserName credential)))
+        (is (= "test-pass" (str/join "" (.getPassword credential))))
+        (is (= "authdb" (.getSource credential)))))
     (testing "Only fqdn may be used with mongo+srv"
       (is (thrown-with-msg? Throwable
                             #"Using DNS SRV requires a FQDN for host"
@@ -139,3 +143,27 @@
                 (or (when (instance? java.net.ConnectException e)
                       (throw e))
                     (some-> (.getCause e) recur))))))))))
+
+(deftest hard-password-test
+  (mt/test-driver
+   :mongo
+   (testing "Passwords and usernames containing `$ : / ? # [ ] @` are usable (#38697)"
+     (let [s "$ : / ? # [ ] @"
+           settings (mongo.connection/db-details->mongo-client-settings {:user s
+                                                                         :pass s
+                                                                         :host "localhost"
+                                                                         :dbname "justdb"})
+           ^MongoCredential credential (.getCredential settings)]
+       (is (= s (.getUserName credential)))
+       (is (= s (str/join "" (.getPassword credential))))
+       (is (= "admin" (.getSource credential)))))))
+
+(deftest application-name-test
+  (mt/test-driver
+   :mongo
+   (with-redefs [config/mb-app-id-string "$ : / ? # [ ] @"]
+     (let [db-details {:host "test-host.place.com"
+                       :dbname "datadb"}
+           settings (mongo.connection/db-details->mongo-client-settings db-details)]
+       (is (= "$ : / ? # [ ] @"
+              (.getApplicationName settings)))))))