diff --git a/src/metabase/driver/sql_jdbc/actions.clj b/src/metabase/driver/sql_jdbc/actions.clj
index 63b9354b6a3d417268774557f93f89d702d648e5..d5dcc91a65a9af2d706b9e92a74c11dae29d78ae 100644
--- a/src/metabase/driver/sql_jdbc/actions.clj
+++ b/src/metabase/driver/sql_jdbc/actions.clj
@@ -14,6 +14,7 @@
[metabase.driver.util :as driver.u]
[metabase.models.field :refer [Field]]
[metabase.query-processor :as qp]
+ [metabase.query-processor.error-type :as qp.error-type]
[metabase.query-processor.store :as qp.store]
[metabase.util :as u]
[metabase.util.honeysql-extensions :as hx]
@@ -22,7 +23,7 @@
[schema.core :as s]
[toucan.db :as db])
(:import
- (java.sql Connection)))
+ (java.sql Connection PreparedStatement)))
(set! *warn-on-reflection* true)
@@ -111,7 +112,9 @@
;;; 2. [[jdbc/with-db-transaction]] does a lot of magic that we don't necessarily want. Writing raw JDBC code is barely
;;; any more code and lets us have complete control over what happens and lets us see at a glance exactly what's
;;; happening without having to keep [[clojure.java.jdbc]] magic in mind or work around it.
-(defn- do-with-jdbc-transaction [database-id f]
+(defn do-with-jdbc-transaction
+ "Impl function for [[with-jdbc-transaction]]."
+ [database-id f]
(if *connection*
(f *connection*)
(let [jdbc-spec (sql-jdbc.conn/db->pooled-connection-spec database-id)]
@@ -134,7 +137,7 @@
(.rollback conn)
(throw e)))))))
-(defmacro ^:private with-jdbc-transaction
+(defmacro with-jdbc-transaction
"Execute `f` with a JDBC Connection for the Database with `database-id`. Uses [[*connection*]] if already bound,
otherwise fetches a new Connection from the Database's Connection pool and executes `f` inside of a transaction."
{:style/indent 1}
@@ -343,6 +346,21 @@
successes]))))
rows)))
+(defmethod driver/execute-write-query! :sql-jdbc
+ [driver {{sql :query, :keys [params]} :native}]
+ {:pre [(string? sql)]}
+ (try
+ (let [{db-id :id} (qp.store/database)]
+ (with-jdbc-transaction [conn db-id]
+ (with-open [stmt (sql-jdbc.execute/statement-or-prepared-statement driver conn sql params nil)]
+ {:rows-affected (if (instance? PreparedStatement stmt)
+ (.executeUpdate ^PreparedStatement stmt)
+ (.executeUpdate stmt sql))})))
+ (catch Throwable e
+ (throw (ex-info (tru "Error executing write query: {0}" (ex-message e))
+ {:sql sql, :params params, :type qp.error-type/invalid-query}
+ e)))))
+
;;;; `:bulk/create`
(defmethod actions/perform-action!* [:sql-jdbc :bulk/create]
diff --git a/src/metabase/driver/sql_jdbc/execute.clj b/src/metabase/driver/sql_jdbc/execute.clj
index 6fdabbe406cadd7a9679b46c832f5d73042de948..1148ad83425b5f59a9bf7a60ece0f0c558abcbda 100644
--- a/src/metabase/driver/sql_jdbc/execute.clj
+++ b/src/metabase/driver/sql_jdbc/execute.clj
@@ -6,7 +6,6 @@
`metabase.driver.sql-jdbc.execute.legacy-impl`. "
(:require
[clojure.core.async :as a]
- [clojure.java.jdbc :as jdbc]
[clojure.string :as str]
[java-time :as t]
[metabase.db.query :as mdb.query]
@@ -369,7 +368,9 @@
(doto (statement driver conn)
(wire-up-canceled-chan-to-cancel-Statement! canceled-chan)))
-(defn- statement-or-prepared-statement ^Statement [driver conn sql params canceled-chan]
+(defn statement-or-prepared-statement
+ "Create a statement or a prepared statement. Should be called from [[with-open]]."
+ ^Statement [driver conn sql params canceled-chan]
(if (use-statement? driver params)
(statement* driver conn canceled-chan)
(prepared-statement* driver conn sql params canceled-chan)))
@@ -512,27 +513,6 @@
(respond results-metadata (reducible-rows driver rs rsmeta (qp.context/canceled-chan context)))))))
-;;; +----------------------------------------------------------------------------------------------------------------+
-;;; | Actions Stuff |
-;;; +----------------------------------------------------------------------------------------------------------------+
-
-(defmethod driver/execute-write-query! :sql-jdbc
- [driver {{sql :query, :keys [params]} :native}]
- {:pre [(string? sql)]}
- (try
- (let [{:keys [details]} (qp.store/database)
- jdbc-spec (sql-jdbc.conn/connection-details->spec driver details)]
- ;; TODO -- should this be done in a transaction? Should we set the isolation level?
- (with-open [conn (jdbc/get-connection jdbc-spec)
- stmt (statement-or-prepared-statement driver conn sql params nil)]
- {:rows-affected (if (instance? PreparedStatement stmt)
- (.executeUpdate ^PreparedStatement stmt)
- (.executeUpdate stmt sql))}))
- (catch Throwable e
- (throw (ex-info (tru "Error executing write query: {0}" (ex-message e))
- {:sql sql, :params params, :type qp.error-type/invalid-query}
- e)))))
-
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Convenience Imports from Old Impl |
;;; +----------------------------------------------------------------------------------------------------------------+
diff --git a/test/metabase/actions_test.clj b/test/metabase/actions_test.clj
index 8150d009c27240443a3763a04ecc24743a25e940..7b84d0e94d83f37311af3807d8d00532995e0fb1 100644
--- a/test/metabase/actions_test.clj
+++ b/test/metabase/actions_test.clj
@@ -2,14 +2,22 @@
(:require
[clojure.test :refer :all]
[metabase.actions :as actions]
+ [metabase.actions.execution :as actions.execution]
[metabase.api.common :refer [*current-user-permissions-set*]]
[metabase.driver :as driver]
[metabase.models :refer [Database Table]]
+ [metabase.models.action :as action]
[metabase.query-processor :as qp]
[metabase.test :as mt]
[metabase.util :as u]
[metabase.util.schema :as su]
- [schema.core :as s]))
+ [schema.core :as s]
+ [toucan2.core :as t2])
+ (:import
+ (org.apache.sshd.server SshServer)
+ (org.apache.sshd.server.forward AcceptAllForwardingFilter)))
+
+(set! *warn-on-reflection* true)
(defmacro with-actions-test-data-and-actions-permissively-enabled
"Combines [[mt/with-actions-test-data-and-actions-enabled]] with full permissions."
@@ -477,3 +485,96 @@
[2 "American"]
[3 "Artisan"]]
(first-three-categories)))))))))
+
+(defn basic-auth-ssh-server ^java.io.Closeable [username password]
+ (try
+ (let [password-auth (reify org.apache.sshd.server.auth.password.PasswordAuthenticator
+ (authenticate [_ auth-username auth-password _session]
+ (and
+ (= auth-username username)
+ (= auth-password password))))
+ keypair-provider (org.apache.sshd.server.keyprovider.SimpleGeneratorHostKeyProvider.)
+ sshd (doto (SshServer/setUpDefaultServer)
+ (.setPort 0)
+ (.setKeyPairProvider keypair-provider)
+ (.setPasswordAuthenticator password-auth)
+ (.setForwardingFilter AcceptAllForwardingFilter/INSTANCE)
+ .start)]
+ sshd)
+ (catch Throwable e
+ (throw (ex-info (format "Error starting SSH mock server with password")
+ {:username username :password password}
+ e)))))
+
+(deftest actions-on-ssh-tunneled-db
+ ;; testing actions against dbs with ssh tunnels. Use an in-memory ssh server that just forwards to the correct port
+ ;; through localhost. Since it is local, it's possible for the application to ignore the ssh tunnel and just talk to
+ ;; the db on the correct port. So we do the tests twice, once expecting the correct answer, and then again with the
+ ;; wrong password for the ssh tunnel and we expect failures. This ensures that the correct result is actually going
+ ;; through the ssh tunnel
+ (mt/test-drivers (disj (mt/normal-drivers-with-feature :actions) :h2)
+ (let [username "username", password "password"]
+ (with-open [ssh-server (basic-auth-ssh-server username password)]
+ (doseq [[correct-password? ssh-password] [[true password] [false "wrong-password"]]]
+ (with-actions-test-data-and-actions-permissively-enabled
+ (let [ssh-port (.getPort ^SshServer ssh-server)]
+ (let [details (t2/select-one-fn :details 'Database :id (mt/id))]
+ (t2/update! 'Database (mt/id)
+ ;; enable ssh tunnel
+ {:details (assoc details
+ :tunnel-enabled true
+ :tunnel-host "localhost"
+ :tunnel-auth-option "password"
+ :tunnel-port ssh-port
+ :tunnel-user username
+ :tunnel-pass ssh-password)}))
+ (testing "Can perform implicit actions on ssh-enabled database"
+ (let [response (try (actions/perform-action! :bulk/update
+ {:database (mt/id)
+ :table-id (mt/id :categories)
+ :arg
+ (let [id (format-field-name :id)
+ name (format-field-name :name)]
+ [{id 1, name "Seed Bowl"}
+ {id 2, name "Millet Treat"}])})
+ (catch Exception e e))]
+ (if correct-password?
+ (is (= {:rows-updated 2} response))
+ (do
+ (is (instance? Exception response) "Did not get an error with wrong password")
+ (is (some (partial instance? org.apache.sshd.common.SshException)
+ (u/full-exception-chain response))
+ "None of the errors are from ssh")))))
+ (testing "Can perform custom actions on ssh-enabled database"
+ (let [query (update (mt/native-query
+ {:query "update categories set name = 'foo' where id = {{id}}"
+ :template-tags {:id {:id "id"
+ :name "id"
+ :type "number"
+ :display-name "Id"}}})
+ :type name)]
+ (mt/with-actions [{card-id :id} {:dataset true
+ :dataset_query (mt/mbql-query categories)}
+ {action-id :action-id} {:name "Query example"
+ :type :query
+ :model_id card-id
+ :dataset_query query
+ :database_id (mt/id)
+ :parameters [{:id "id"
+ :type :number/=
+ :target [:variable
+ [:template-tag "id"]]
+ :name "Id"
+ :slug "id"
+ :hasVariableTemplateTagTarget true}]
+ :visualization_settings {:position "top"}}]
+ (let [action (action/select-action :id action-id)]
+ (if correct-password?
+ (is (= {:rows-affected 1}
+ (actions.execution/execute-action! action {"id" 1})))
+ (let [response (try (actions.execution/execute-action! action {"id" 1})
+ (catch Exception e e))]
+ (is (instance? Exception response) "Did not get an error with wrong password")
+ (is (some (partial instance? org.apache.sshd.common.SshException)
+ (u/full-exception-chain response))
+ "None of the errors are from ssh"))))))))))))))