Allow actions to run on H2 and sample database (#28212)

* Decouple checking ddl from classifying h2 stmts

- should enable followup for easily blocking more kinds of queries
- check all statements to make sure they aren't "ddl".

* fix classify-query

* linter fixes + get-field refactor

* return the CommandInterface values as ints

Reach into the CommandList when needed

* docstring wording

* catch invalid queries -- they can't be classified

* Remove action subtypes from inlined-models

* Add ddl check for `execute-write-query!`

* Remove ACCESS_MODE_DATA

* Check queries are single select statements

* Add test for sample database privileges

* Fix single-select check

* Add single-select test

* Rename and add more tests for checking read only commands

* commands -> statements

* Fix check-disallow-ddl-commands

* new line

* Add more read-only statements to the tests

* Update error text

* Use are

* Add integration test for executing actions with disallowed commands

* Add test before inserting row

* Run GRANT ALL ON SCHEMA "PUBLIC" TO GUEST

* Restore classify-query

* whitespace

---------

Co-authored-by: Bryan Maass <bryan.maass@gmail.com>

src/metabase/driver/h2.clj

@@ -185,12 +185,33 @@
         (throw (ex-info "IllegalArgument: DDL commands are not allowed to be used with h2."
                         {:classification query-classification}))))))
 
+(defn- read-only-statements? [{:keys [command-types remaining-sql]}]
+  (let [cmd-type-nums command-types]
+    (boolean
+     (and (every? #{CommandInterface/SELECT ; includes SHOW, TABLE, VALUES
+                    CommandInterface/EXPLAIN
+                    CommandInterface/CALL} cmd-type-nums)
+          (nil? remaining-sql)))))
+
+(defn- check-read-only-statements [{:keys [database] {:keys [query]} :native}]
+  (when query
+    (let [query-classification (classify-query database query)]
+      (when-not (read-only-statements? query-classification)
+        (throw (ex-info "Only SELECT statements are allowed in a native query."
+                        {:classification query-classification}))))))
+
 (defmethod driver/execute-reducible-query :h2
   [driver query chans respond]
   (check-native-query-not-using-default-user query)
-  (check-disallow-ddl-commands query)
+  (check-read-only-statements query)
   ((get-method driver/execute-reducible-query :sql-jdbc) driver query chans respond))
 
+(defmethod driver/execute-write-query! :h2
+  [driver query]
+  (check-native-query-not-using-default-user query)
+  (check-disallow-ddl-commands query)
+  ((get-method driver/execute-write-query! :sql-jdbc) driver query))
+
 (defmethod sql.qp/add-interval-honeysql-form :h2
   [driver hsql-form amount unit]
   (cond
@@ -406,8 +427,7 @@
   (db-type->base-type database-type))
 
 ;; These functions for exploding / imploding the options in the connection strings are here so we can override shady
-;; options users might try to put in their connection string. e.g. if someone sets `ACCESS_MODE_DATA` to `rws` we can
-;; replace that and make the connection read-only.
+;; options users might try to put in their connection string, like INIT=...
 
 (defn- file+options->connection-string
   "Implode the results of `connection-string->file+options` back into a connection string."
@@ -426,8 +446,7 @@
                                                 ;; http://h2database.com/html/features.html#execute_sql_on_connection
                                                 (remove (fn [[k _]] (= (u/lower-case-en k) "init")))
                                                 (into {}))
-                                           {"IFEXISTS"         "TRUE"
-                                            "ACCESS_MODE_DATA" "r"}))))
+                                           {"IFEXISTS" "TRUE"}))))
 
 (defmethod sql-jdbc.conn/connection-details->spec :h2
   [_ details]

test/metabase/cmd/compare_h2_dbs.clj

@@ -16,7 +16,6 @@
    :subprotocol       "h2"
    :subname           (str "file:" db-file)
    "IFEXISTS"         "TRUE"
-   "ACCESS_MODE_DATA" "r"
    ;; close DB right away when done
    "DB_CLOSE_DELAY"   "0"})
 

test/metabase/driver/h2_test.clj

@@ -33,15 +33,15 @@
 
 (deftest set-safe-options-test
   (testing "Check that we add safe connection options to connection strings"
-    (is (= "file:my-file;LOOK_I_INCLUDED_AN_EXTRA_SEMICOLON=NICE_TRY;IFEXISTS=TRUE;ACCESS_MODE_DATA=r"
+    (is (= "file:my-file;LOOK_I_INCLUDED_AN_EXTRA_SEMICOLON=NICE_TRY;IFEXISTS=TRUE"
            (#'h2/connection-string-set-safe-options "file:my-file;;LOOK_I_INCLUDED_AN_EXTRA_SEMICOLON=NICE_TRY"))))
 
   (testing "Check that we override shady connection string options set by shady admins with safe ones"
-    (is (= "file:my-file;LOOK_I_INCLUDED_AN_EXTRA_SEMICOLON=NICE_TRY;IFEXISTS=TRUE;ACCESS_MODE_DATA=r"
-           (#'h2/connection-string-set-safe-options "file:my-file;;LOOK_I_INCLUDED_AN_EXTRA_SEMICOLON=NICE_TRY;IFEXISTS=FALSE;ACCESS_MODE_DATA=rws"))))
+    (is (= "file:my-file;LOOK_I_INCLUDED_AN_EXTRA_SEMICOLON=NICE_TRY;IFEXISTS=TRUE"
+           (#'h2/connection-string-set-safe-options "file:my-file;;LOOK_I_INCLUDED_AN_EXTRA_SEMICOLON=NICE_TRY;IFEXISTS=FALSE;"))))
 
   (testing "Check that we override the INIT connection string option"
-    (is (= "file:my-file;IFEXISTS=TRUE;ACCESS_MODE_DATA=r"
+    (is (= "file:my-file;IFEXISTS=TRUE"
            (#'h2/connection-string-set-safe-options "file:my-file;INIT=ANYTHING_HERE_WILL_BE_IGNORED")))))
 
 (deftest db-details->user-test
@@ -207,3 +207,61 @@
                     {:database (u/the-id (mt/db))
                      :engine :h2
                      :native {:query trigger-creation-attempt}}))))))
+
+(deftest check-read-only-test
+  (testing "read only statements should pass"
+    (are [query] (nil?
+                  (#'h2/check-read-only-statements
+                   {:database (u/the-id (mt/db))
+                    :engine :h2
+                    :native {:query query}}))
+      "select * from orders"
+      "select 1; select 2;"
+      "explain select * from orders"
+      "values (1, 'Hello'), (2, 'World');"
+      "show tables"
+      "table orders"
+      "call 1 + 1"
+      ;; Note this passes the check, but will fail on execution
+      "update venues set name = 'bill'; some query that can't be parsed;"))
+  (testing "not read only statements should fail"
+    (are [query] (thrown?
+                  clojure.lang.ExceptionInfo
+                  #"Only SELECT statements are allowed in a native query."
+                  (#'h2/check-read-only-statements
+                   {:database (u/the-id (mt/db))
+                    :engine :h2
+                    :native {:query query}}))
+      "update venues set name = 'bill'"
+      "insert into venues (name) values ('bill')"
+      "delete venues"
+      "select 1; update venues set name = 'bill'; delete venues;"
+      (str/join "\n" ["DROP TRIGGER IF EXISTS MY_SPECIAL_TRIG;"
+                        "CREATE OR REPLACE TRIGGER MY_SPECIAL_TRIG BEFORE SELECT ON INFORMATION_SCHEMA.Users AS '';"
+                        "SELECT * FROM INFORMATION_SCHEMA.Users;"]))))
+
+(deftest disallowed-commands-in-action-test
+  (mt/test-driver :h2
+    (mt/with-actions-test-data-and-actions-enabled
+      (testing "Should not be able to execute query actions with disallowed commands"
+        (let [sql "select * from categories; update categories set name = 'stomp';
+                 CREATE ALIAS EXEC AS 'String shellexec(String cmd) throws java.io.IOException {Runtime.getRuntime().exec(cmd);return \"y4tacker\";}';
+                 EXEC ('open -a Calculator.app')"]
+          (mt/with-actions [{:keys [action-id]} {:type :query
+                                                 :dataset_query {:database (mt/id)
+                                                                 :type     "native"
+                                                                 :native   {:query sql}}}]
+            (is (=? {:message "Error executing Action: IllegalArgument: DDL commands are not allowed to be used with h2."}
+                    (mt/user-http-request :crowberto
+                                          :post 500
+                                          (format "action/%s/execute" action-id)))))))
+      (testing "Should be able to execute query actions with allowed commands"
+        (let [sql "update categories set name = 'stomp' where id = 1; update categories set name = 'stomp' where id = 2;"]
+          (mt/with-actions [{:keys [action-id]} {:type :query
+                                                 :dataset_query {:database (mt/id)
+                                                                 :type     "native"
+                                                                 :native   {:query sql}}}]
+            (is (=? {:rows-affected 1}
+                    (mt/user-http-request :crowberto
+                                          :post 200
+                                          (format "action/%s/execute" action-id))))))))))

test/metabase/sample_data_test.clj

@@ -2,8 +2,10 @@
   "Tests to make sure the Sample Database syncs the way we would expect."
   (:require
    [clojure.core.memoize :as memoize]
+   [clojure.java.jdbc :as jdbc]
    [clojure.string :as str]
    [clojure.test :refer :all]
+   [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
    [metabase.models :refer [Database Field Table]]
    [metabase.plugins :as plugins]
    [metabase.sample-data :as sample-data]
@@ -99,3 +101,45 @@
                  (hydrate :has_field_values)
                  (select-keys [:name :description :database_type :semantic_type :has_field_values :active :visibility_type
                                :preview_display :display_name :fingerprint :base_type])))))))
+
+(deftest write-rows-sample-database-test
+  (testing "should be able to execute INSERT, UPDATE, and DELETE statements on the Sample Database"
+    (with-temp-sample-database-db [db]
+      (mt/with-db db
+        (let [conn-spec (sql-jdbc.conn/db->pooled-connection-spec (mt/db))]
+          (testing "update row"
+            (let [quantity (fn []
+                             (->> (jdbc/query conn-spec "SELECT QUANTITY FROM ORDERS WHERE ID = 1;")
+                                  (map :quantity)))]
+              (testing "before"
+                (is (= [2]
+                       (quantity))))
+              (is (= [1]
+                     (jdbc/execute! conn-spec "UPDATE ORDERS SET QUANTITY = 1 WHERE ID = 1;")))
+              (testing "after"
+                (is (= [1]
+                       (quantity))))
+              ;; TODO: this shouldn't be necessary, since we're modifying a temp sample database.
+              (testing "restore"
+                (is (= [1]
+                       (jdbc/execute! conn-spec "UPDATE ORDERS SET QUANTITY = 2 WHERE ID = 1;"))))))
+          (let [rating (fn []
+                         (->> (jdbc/query conn-spec "SELECT RATING FROM PRODUCTS WHERE PRICE = 12.345;")
+                              (map :rating)))]
+            (testing "before"
+              (is (= []
+                     (rating))))
+            (testing "insert row"
+              (is (= [1]
+                     (jdbc/execute! conn-spec "INSERT INTO PRODUCTS (price, rating) VALUES (12.345, 6.789);")))
+              (is (= [6.789]
+                     (rating))))
+            (testing "delete row"
+              (testing "before"
+                (is (= [6.789]
+                       (rating))))
+              (is (= [1]
+                     (jdbc/execute! conn-spec "DELETE FROM PRODUCTS WHERE PRICE = 12.345;")))
+              (testing "after"
+                (is (= []
+                       (rating)))))))))))