Fix connecting to SQL Server DBs with passwords with special characters

[ci drivers]

src/metabase/db.clj

@@ -19,6 +19,7 @@
             metabase.util.honeysql-extensions) ; this needs to be loaded so the `:h2` quoting style gets added
   (:import java.io.StringWriter
            java.sql.Connection
+           java.util.Properties
            com.mchange.v2.c3p0.ComboPooledDataSource
            liquibase.Liquibase
            (liquibase.database DatabaseFactory Database)
@@ -268,7 +269,7 @@
                  (.setTestConnectionOnCheckin      false)
                  (.setTestConnectionOnCheckout     false)
                  (.setPreferredTestQuery           nil)
-                 (.setProperties                   (u/prog1 (java.util.Properties.)
+                 (.setProperties                   (u/prog1 (Properties.)
                                                      (doseq [[k v] (dissoc spec :classname :subprotocol :subname :naming :delimiters :alias-delimiter
                                                                                 :excess-timeout :minimum-pool-size :idle-connection-test-period)]
                                                        (.setProperty <> (name k) (str v))))))})

src/metabase/db/spec.clj

 (ns metabase.db.spec
-  "Functions for creating JDBC DB specs for a given engine.")
+  "Functions for creating JDBC DB specs for a given engine.
+   Only databases that are supported as application DBs should have functions in this namespace;
+   otherwise, similar functions are only needed by drivers, and belong in those namespaces.")
 
 (defn h2
   "Create a database specification for a h2 database. Opts should include a key
@@ -36,38 +38,3 @@
           :subname (str "//" host ":" port "/" db)
           :delimiters "`"}
          (dissoc opts :host :port :db)))
-
-
-;; TODO - These other ones can acutally be moved directly into their respective drivers themselves since they're not supported as backing DBs
-
-(defn mssql
-  "Create a database specification for a mssql database. Opts should include keys
-  for :db, :user, and :password. You can also optionally set host and port."
-  [{:keys [user password db host port]
-    :or {user "dbuser", password "dbpassword", db "", host "localhost", port 1433}
-    :as opts}]
-  (merge {:classname "com.microsoft.sqlserver.jdbc.SQLServerDriver" ; must be in classpath
-          :subprotocol "sqlserver"
-          :subname (str "//" host ":" port ";database=" db ";user=" user ";password=" password)}
-         (dissoc opts :host :port :db)))
-
-(defn sqlite3
-  "Create a database specification for a SQLite3 database. Opts should include a
-  key for :db which is the path to the database file."
-  [{:keys [db]
-    :or {db "sqlite.db"}
-    :as opts}]
-  (merge {:classname "org.sqlite.JDBC" ; must be in classpath
-          :subprotocol "sqlite"
-          :subname db}
-         (dissoc opts :db)))
-
-(defn oracle
-  "Create a database specification for an Oracle database. Opts should include keys
-  for :user and :password. You can also optionally set host and port."
-  [{:keys [host port]
-    :or {host "localhost", port 1521}
-    :as opts}]
-  (merge {:subprotocol "oracle:thin"
-          :subname     (str "@" host ":" port)}
-         (dissoc opts :host :port)))

src/metabase/driver/crate.clj

@@ -40,16 +40,16 @@
 
 (def ^:private ^:const now (hsql/call :current_timestamp 3))
 
-(defn- crate-spec
+(defn- connection-details->spec
   [{:keys [hosts]
-    :as   opts}]
+    :as   details}]
   (merge {:classname   "io.crate.client.jdbc.CrateDriver" ; must be in classpath
           :subprotocol "crate"
           :subname     (str "//" hosts "/")}
-         (dissoc opts :hosts)))
+         (dissoc details :hosts)))
 
 (defn- can-connect? [details]
-  (let [connection-spec (crate-spec details)]
+  (let [connection-spec (connection-details->spec details)]
     (= 1 (first (vals (first (jdbc/query connection-spec ["select 1"])))))))
 
 (defn- string-length-fn [field-key]
@@ -71,7 +71,7 @@
           :features       (comp (u/rpartial disj :foreign-keys) sql/features)})
   sql/ISQLDriver
   (merge (sql/ISQLDriverDefaultsMixin)
-         {:connection-details->spec  (u/drop-first-arg crate-spec)
+         {:connection-details->spec  (u/drop-first-arg connection-details->spec)
           :column->base-type         (u/drop-first-arg column->base-type)
           :string-length-fn          (u/drop-first-arg string-length-fn)
           :date                      crate-util/date

src/metabase/driver/oracle.clj

@@ -7,7 +7,6 @@
                       [helpers :as h])
             [metabase.config :as config]
             [toucan.db :as db]
-            [metabase.db.spec :as dbspec]
             [metabase.driver :as driver]
             [metabase.driver.generic-sql :as sql]
             [metabase.driver.generic-sql.query-processor :as sqlqp]
@@ -42,8 +41,15 @@
    [#"URI"         :type/Text]
    [#"XML"         :type/*]])
 
-(defn- connection-details->spec [{:keys [sid], :as details}]
-  (update (dbspec/oracle details) :subname (u/rpartial str \: sid)))
+(defn- connection-details->spec
+  "Create a database specification for an Oracle database. DETAILS should include keys
+  for `:user`, `:password`, and `:sid`. You can also optionally set `:host` and `:port`."
+  [{:keys [host port sid]
+    :or   {host "localhost", port 1521}
+    :as   details}]
+  (merge {:subprotocol "oracle:thin"
+          :subname     (str "@" host ":" port ":" sid)}
+         (dissoc details :host :port)))
 
 (defn- can-connect? [details]
   (let [connection (connection-details->spec details)]

src/metabase/driver/sqlite.clj

@@ -4,12 +4,22 @@
             (honeysql [core :as hsql]
                       [format :as hformat])
             [metabase.config :as config]
-            [metabase.db.spec :as dbspec]
             [metabase.driver :as driver]
             [metabase.driver.generic-sql :as sql]
             [metabase.util :as u]
             [metabase.util.honeysql-extensions :as hx]))
 
+(defn- connection-details->spec
+  "Create a database specification for a SQLite3 database. DETAILS should include a
+  key for `:db` which is the path to the database file."
+  [{:keys [db]
+    :or   {db "sqlite.db"}
+    :as   details}]
+  (merge {:classname   "org.sqlite.JDBC"
+          :subprotocol "sqlite"
+          :subname     db}
+         (dissoc details :db)))
+
 ;; We'll do regex pattern matching here for determining Field types
 ;; because SQLite types can have optional lengths, e.g. NVARCHAR(100) or NUMERIC(10,5)
 ;; See also http://www.sqlite.org/datatype3.html
@@ -158,7 +168,7 @@
   (merge (sql/ISQLDriverDefaultsMixin)
          {:active-tables             sql/post-filtered-active-tables
           :column->base-type         (sql/pattern-based-column->base-type pattern->type)
-          :connection-details->spec  (u/drop-first-arg dbspec/sqlite3)
+          :connection-details->spec  (u/drop-first-arg connection-details->spec)
           :current-datetime-fn       (constantly (hsql/call :datetime (hx/literal :now)))
           :date                      (u/drop-first-arg date)
           :prepare-sql-param         (u/drop-first-arg prepare-sql-param)

src/metabase/driver/sqlserver.clj

 (ns metabase.driver.sqlserver
   (:require [clojure.string :as s]
             [honeysql.core :as hsql]
-            [metabase.db.spec :as dbspec]
             [metabase.driver :as driver]
             [metabase.driver.generic-sql :as sql]
             [metabase.util :as u]
@@ -48,27 +47,26 @@
     :xml              :type/*
     (keyword "int identity") :type/Integer} column-type)) ; auto-incrementing integer (ie pk) field
 
-(defn- connection-details->spec [{:keys [domain instance ssl], :as details}]
-  (-> ;; Having the `:ssl` key present, even if it is `false`, will make the driver attempt to connect with SSL
-      (dbspec/mssql (if ssl
-                      details
-                      (dissoc details :ssl)))
-      ;; swap out Microsoft Driver details for jTDS ones
-      (assoc :classname   "net.sourceforge.jtds.jdbc.Driver"
-             :subprotocol "jtds:sqlserver")
-
-      ;; adjust the connection URL to match up with the jTDS format (see http://jtds.sourceforge.net/faq.html#urlFormat)
-      (update :subname (fn [subname]
-                         ;; jTDS uses a "/" instead of ";database="
-                         (cond-> (s/replace subname #";database=" "/")
-                           ;; and add the ;instance= option if applicable
-                           (seq instance) (str ";instance=" instance)
-
-                           ;; add Windows domain for Windows domain authentication if applicable. useNTLMv2 = send LMv2/NTLMv2 responses when using Windows auth
-                           (seq domain) (str ";domain=" domain ";useNTLMv2=true")
-
-                           ;; If SSL is specified append ;ssl=require, which enables SSL and throws exception if SSL connection cannot be made
-                           ssl (str ";ssl=require"))))))
+
+(defn- connection-details->spec [{:keys [user password db host port instance domain ssl]
+                                  :or   {user "dbuser", password "dbpassword", db "", host "localhost", port 1433}
+                                  :as   details}]
+  {:classname    "net.sourceforge.jtds.jdbc.Driver"
+   :subprotocol  "jtds:sqlserver"
+   :loginTimeout 5 ; Wait up to 10 seconds for connection success. If we get no response by then, consider the connection failed
+   :subname      (str "//" host ":" port "/" db)
+   ;; everything else gets passed as `java.util.Properties` to the JDBC connection. See full list of properties here: `http://jtds.sourceforge.net/faq.html#urlFormat`
+   ;; (passing these as Properties instead of part of the `:subname` is preferable because they support things like passwords with special characters)
+   :user         user
+   :password     password
+   :instance     instance
+   :domain       domain
+   :useNTLMv2    (boolean domain) ; if domain is specified, send LMv2/NTLMv2 responses when using Windows authentication
+   ;; for whatever reason `ssl=request` doesn't work with RDS (it hangs indefinitely), so just set ssl=off (disabled) if SSL isn't being used
+   :ssl          (if ssl
+                   "require"
+                   "off")})
+
 
 (defn- date-part [unit expr]
   (hsql/call :datepart (hsql/raw (name unit)) expr))

src/metabase/driver/vertica.clj

@@ -32,22 +32,13 @@
    (keyword "Long Varchar")   :type/Text
    (keyword "Long Varbinary") :type/*})
 
-(defn- vertica-spec [{:keys [host port db]
-                      :or   {host "localhost", port 5433, db ""}
-                      :as   opts}]
+(defn- connection-details->spec [{:keys [host port db dbname]
+                                  :or   {host "localhost", port 5433, db ""}
+                                  :as   details}]
   (merge {:classname   "com.vertica.jdbc.Driver"
           :subprotocol "vertica"
-          :subname     (str "//" host ":" port "/" db)}
-         (dissoc opts :host :port :db :ssl)))
-
-(defn- connection-details->spec [details-map]
-  (-> details-map
-      (update :port (fn [port]
-                      (if (string? port)
-                        (Integer/parseInt port)
-                        port)))
-      (rename-keys {:dbname :db})
-      vertica-spec))
+          :subname     (str "//" host ":" port "/" (or dbname db))}
+         (dissoc details :host :port :dbname :db :ssl)))
 
 (defn- unix-timestamp->timestamp [expr seconds-or-milliseconds]
   (case seconds-or-milliseconds

test/metabase/sync_database_test.clj

@@ -334,7 +334,6 @@
         (let [driver       (driver/engine->driver :h2)
               spec         (sql/connection-details->spec driver details)
               exec!        #(doseq [statement %]
-                              (println "[H2]" statement) ; NOCOMMIT
                               (println (jdbc/execute! spec [statement])))
               insert-range #(str "INSERT INTO blueberries_consumed (num) VALUES "
                                  (str/join ", " (for [n %]