diff --git a/bin/build/src/build/uberjar.clj b/bin/build/src/build/uberjar.clj index b8174bd97652d..560a81d41516c 100644 --- a/bin/build/src/build/uberjar.clj +++ b/bin/build/src/build/uberjar.clj @@ -1,6 +1,7 @@ (ns build.uberjar (:require [clojure.java.io :as io] + [clojure.set :as set] [clojure.tools.build.api :as b] [clojure.tools.build.util.zip :as build.zip] [clojure.tools.namespace.dependency :as ns.deps] @@ -67,11 +68,14 @@ (let [ns-decls (mapcat (comp ns.find/find-ns-decls-in-dir io/file) (all-paths basis)) - ns-symbols (set (map ns.parse/name-from-ns-decl ns-decls))] - (->> (dependencies-graph ns-decls) - ns.deps/topo-sort - (filter ns-symbols) - (cons 'metabase.bootstrap)))) + ns-symbols (set (map ns.parse/name-from-ns-decl ns-decls)) + sorted (->> (dependencies-graph ns-decls) + ns.deps/topo-sort + (filter ns-symbols)) + orphans (set/difference (set ns-symbols) (set sorted))] + (concat + orphans + sorted))) (defn compile-sources! [basis] (u/step "Compile Clojure source files" @@ -88,17 +92,18 @@ (defn copy-resources! [edition basis] (u/step "Copy resources" - ;; technically we don't NEED to copy the Clojure source files but it doesn't really hurt anything IMO. (doseq [path (all-paths basis)] - (u/step (format "Copy %s" path) - (b/copy-dir {:target-dir class-dir, :src-dirs [path]}))))) + (when (not (#{"src" "shared/src" "enterprise/backend/src"} path)) + (u/step (format "Copy %s" path) + (b/copy-dir {:target-dir class-dir, :src-dirs [path]})))))) (defn create-uberjar! [basis] (u/step "Create uberjar" (with-duration-ms [duration-ms] (depstar/uber {:class-dir class-dir :uber-file uberjar-filename - :basis basis}) + :basis basis + :exclude [".*metabase.*.clj[c|s]?$"]}) (u/announce "Created uberjar in %.1f seconds." (/ duration-ms 1000.0))))) (def manifest-entries diff --git a/e2e/runner/cypress-runner-backend.js b/e2e/runner/cypress-runner-backend.js old mode 100644 new mode 100755 index 4528fc07cc3c9..10c2a68dda5bf --- a/e2e/runner/cypress-runner-backend.js +++ b/e2e/runner/cypress-runner-backend.js @@ -36,6 +36,7 @@ const CypressBackend = { MB_JETTY_HOST: "0.0.0.0", MB_JETTY_PORT: server.port, MB_ENABLE_TEST_ENDPOINTS: "true", + MB_DANGEROUS_UNSAFE_ENABLE_TESTING_H2_CONNECTIONS_DO_NOT_ENABLE: "true", MB_PREMIUM_EMBEDDING_TOKEN: (process.env["MB_EDITION"] === "ee" && process.env["MB_PREMIUM_EMBEDDING_TOKEN"]) || diff --git a/e2e/runner/cypress-runner-generate-snapshots.js b/e2e/runner/cypress-runner-generate-snapshots.js index 9c9eddcb50077..6696ef0488175 100644 --- a/e2e/runner/cypress-runner-generate-snapshots.js +++ b/e2e/runner/cypress-runner-generate-snapshots.js @@ -4,7 +4,6 @@ const { parseArguments, args } = require("./cypress-runner-utils"); const getConfig = baseUrl => { return { - browser: "chrome", configFile: "e2e/support/cypress-snapshots.config.js", config: { baseUrl, diff --git a/e2e/runner/cypress-runner-run-tests.js b/e2e/runner/cypress-runner-run-tests.js index dc1e9c8773d73..50b53548137bb 100644 --- a/e2e/runner/cypress-runner-run-tests.js +++ b/e2e/runner/cypress-runner-run-tests.js @@ -22,7 +22,6 @@ const runCypress = async (baseUrl, exitFunction) => { }); const defaultConfig = { - browser: "chrome", configFile: "e2e/support/cypress.config.js", config: { baseUrl, diff --git a/e2e/test/scenarios/admin/databases/add-new-database.cy.spec.js b/e2e/test/scenarios/admin/databases/add-new-database.cy.spec.js index a9d75305d5fc6..7c2f03fe201c0 100644 --- a/e2e/test/scenarios/admin/databases/add-new-database.cy.spec.js +++ b/e2e/test/scenarios/admin/databases/add-new-database.cy.spec.js @@ -24,7 +24,8 @@ describe("admin > database > add", () => { cy.findByLabelText("Database type").click(); }); - it("should add a new database", () => { + // Skipping as we are removing H2 + it.skip("should add a new database", () => { popover().within(() => { if (isEE) { // EE should ship with Oracle and Vertica as options @@ -56,6 +57,14 @@ describe("admin > database > add", () => { describe("external databases", { tags: "@external" }, () => { it("should add Postgres database and redirect to listing (metabase#12972, metabase#14334, metabase#17450)", () => { + popover().within(() => { + if (isEE) { + // EE should ship with Oracle and Vertica as options + cy.findByText("Oracle"); + cy.findByText("Vertica"); + } + }); + cy.contains("PostgreSQL").click({ force: true }); cy.findByText("Show advanced options").click(); diff --git a/e2e/test/scenarios/filters/view.cy.spec.js b/e2e/test/scenarios/filters/view.cy.spec.js index a0919c545149f..834471767769e 100644 --- a/e2e/test/scenarios/filters/view.cy.spec.js +++ b/e2e/test/scenarios/filters/view.cy.spec.js @@ -18,7 +18,11 @@ describe("scenarios > question > view", () => { beforeEach(() => { // All users upgraded to collection view access cy.visit("/admin/permissions/collections/root"); - cy.icon("close").first().click(); + cy.findByText("All Users") + .closest("tr") + .within(() => { + cy.icon("close").click(); + }); cy.findAllByRole("option").contains("View").click(); cy.findByText("Save changes").click(); cy.findByText("Yes").click(); diff --git a/e2e/test/scenarios/onboarding/setup/setup.cy.spec.js b/e2e/test/scenarios/onboarding/setup/setup.cy.spec.js index ecf00bbf5f1b3..aaa47a29b6c08 100644 --- a/e2e/test/scenarios/onboarding/setup/setup.cy.spec.js +++ b/e2e/test/scenarios/onboarding/setup/setup.cy.spec.js @@ -114,20 +114,9 @@ describe("scenarios > setup", () => { cy.findByText("SQLite").click(); cy.findByText("Need help connecting?"); - // add h2 database + // remove database cy.findByLabelText("Remove database").click(); - cy.findByText("Show more options").click(); - cy.findByText("H2").click(); - cy.findByLabelText("Display name").type("Metabase H2"); - cy.findByText("Connect database").closest("button").should("be.disabled"); - - const dbFilename = "e2e/runner/empty.db"; - const dbPath = Cypress.config("fileServerFolder") + "/" + dbFilename; - cy.findByLabelText("Connection String").type(`file:${dbPath}`); - cy.findByText("Connect database") - .closest("button") - .should("not.be.disabled") - .click(); + cy.findByText("I'll add my data later").click(); // test database setup help card is hidden on the next step cy.findByText("Need help connecting?").should("not.be.visible"); @@ -185,16 +174,7 @@ describe("scenarios > setup", () => { // Database cy.findByText("Add your data"); - cy.findByText("I'll add my data later"); - - cy.findByText("Show more options").click(); - cy.findByText("H2").click(); - cy.findByLabelText("Display name").type("Metabase H2"); - - const dbFilename = "e2e/runner/empty.db"; - const dbPath = Cypress.config("fileServerFolder") + "/" + dbFilename; - cy.findByLabelText("Connection String").type(`file:${dbPath}`); - cy.button("Connect database").click(); + cy.findByText("I'll add my data later").click(); // Turns off anonymous data collection cy.findByLabelText( diff --git a/e2e/test/scenarios/visualizations/reproductions/11435-time-tooltip-native.cy.spec.js b/e2e/test/scenarios/visualizations/reproductions/11435-time-tooltip-native.cy.spec.js index da8c0cc08f9d1..8e37f88f7a143 100644 --- a/e2e/test/scenarios/visualizations/reproductions/11435-time-tooltip-native.cy.spec.js +++ b/e2e/test/scenarios/visualizations/reproductions/11435-time-tooltip-native.cy.spec.js @@ -30,11 +30,11 @@ describe("issue 11435", () => { it("should use time formatting settings in tooltips for native questions (metabase#11435)", () => { cy.createNativeQuestion(questionDetails, { visitQuestion: true }); - clickLineDot({ index: 1 }); + hoverLineDot({ index: 1 }); popover().findByTextEnsureVisible("March 11, 2019, 8:45:17.010 PM"); }); }); -const clickLineDot = ({ index } = {}) => { - cy.get(".Visualization .dot").eq(index).click({ force: true }); +const hoverLineDot = ({ index } = {}) => { + cy.get(".Visualization .dot").eq(index).realHover(); }; diff --git a/enterprise/backend/test/metabase_enterprise/advanced_config/file/databases_test.clj b/enterprise/backend/test/metabase_enterprise/advanced_config/file/databases_test.clj index 0aad0264c2a36..f4a70cc943f4c 100644 --- a/enterprise/backend/test/metabase_enterprise/advanced_config/file/databases_test.clj +++ b/enterprise/backend/test/metabase_enterprise/advanced_config/file/databases_test.clj @@ -3,6 +3,7 @@ [clojure.test :refer :all] [metabase-enterprise.advanced-config.file :as advanced-config.file] [metabase.db.connection :as mdb.connection] + [metabase.driver.h2 :as h2] [metabase.models :refer [Database Table]] [metabase.public-settings.premium-features-test :as premium-features-test] [metabase.test :as mt] @@ -10,7 +11,8 @@ [toucan.db :as db])) (use-fixtures :each (fn [thunk] - (binding [advanced-config.file/*supported-versions* {:min 1, :max 1}] + (binding [advanced-config.file/*supported-versions* {:min 1, :max 1} + h2/*allow-testing-h2-connections* true] (premium-features-test/with-premium-features #{:advanced-config} (thunk))))) diff --git a/frontend/src/metabase/admin/app/selectors.ts b/frontend/src/metabase/admin/app/selectors.ts index fa96f4ddfb6f3..3a53c153babc1 100644 --- a/frontend/src/metabase/admin/app/selectors.ts +++ b/frontend/src/metabase/admin/app/selectors.ts @@ -16,7 +16,10 @@ export const isNoticeEnabled = (state: State): boolean => { }; export const hasDeprecatedDatabase = (state: State, props: Props): boolean => { - return props.databases?.some(d => isDeprecatedEngine(d.engine)) ?? false; + return ( + props.databases?.some(d => !d.is_sample && isDeprecatedEngine(d.engine)) ?? + false + ); }; export const getAdminPaths = (state: State) => { diff --git a/src/metabase/api/database.clj b/src/metabase/api/database.clj index 973928b3f36a5..eba008d7a4395 100644 --- a/src/metabase/api/database.clj +++ b/src/metabase/api/database.clj @@ -878,13 +878,16 @@ settings (s/maybe su/Map)} ;; TODO - ensure that custom schedules and let-user-control-scheduling go in lockstep (let [existing-database (api/write-check (db/select-one Database :id id)) - details (driver.u/db-details-client->server engine details) - details (upsert-sensitive-fields existing-database details) - conn-error (when (some? details) - (assert (some? engine)) - (test-database-connection engine details)) - full-sync? (when-not (nil? is_full_sync) - (boolean is_full_sync))] + details (some->> details + (driver.u/db-details-client->server (or engine (:engine existing-database))) + (upsert-sensitive-fields existing-database)) + ;; verify that we can connect to the database if `:details` OR `:engine` have changed. + details-changed? (some-> details (not= (:details existing-database))) + engine-changed? (some-> engine (not= (:engine existing-database))) + conn-error (when (or details-changed? engine-changed?) + (test-database-connection (or engine (:engine existing-database)) + (or details (:details existing-database)))) + full-sync? (some-> is_full_sync boolean)] (if conn-error ;; failed to connect, return error {:status 400 diff --git a/src/metabase/api/setup.clj b/src/metabase/api/setup.clj index a20aee9c527b9..3d16ae27bb144 100644 --- a/src/metabase/api/setup.clj +++ b/src/metabase/api/setup.clj @@ -93,6 +93,8 @@ (when-not (some-> (u/ignore-exceptions (driver/the-driver driver)) driver/available?) (let [msg (tru "Cannot create Database: cannot find driver {0}." driver)] (throw (ex-info msg {:errors {:database {:engine msg}}, :status-code 400})))) + (when-let [error (api.database/test-database-connection driver details)] + (throw (ex-info (:message error (tru "Cannot connect to Database")) (assoc error :status-code 400)))) (db/insert! Database (merge {:name name, :engine driver, :details details, :creator_id creator-id} @@ -180,6 +182,9 @@ [:as {{{:keys [engine details]} :details, token :token} :body}] {token SetupToken engine DBEngineString} + (when (setup/has-user-setup) + (throw (ex-info (tru "Instance already initialized") + {:status-code 400}))) (let [engine (keyword engine) error-or-nil (api.database/test-database-connection engine details)] (when error-or-nil diff --git a/src/metabase/api/table.clj b/src/metabase/api/table.clj index 77d5bbc814727..5ab4c5737cd0c 100644 --- a/src/metabase/api/table.clj +++ b/src/metabase/api/table.clj @@ -6,6 +6,7 @@ [metabase.api.common :as api] [metabase.db.query :as mdb.query] [metabase.driver :as driver] + [metabase.driver.h2 :as h2] [metabase.driver.util :as driver.u] [metabase.models.card :refer [Card]] [metabase.models.field :refer [Field]] @@ -15,7 +16,7 @@ [metabase.related :as related] [metabase.sync :as sync] [metabase.sync.concurrent :as sync.concurrent] - #_:clj-kondo/ignore + #_{:clj-kondo/ignore [:consistent-alias]} [metabase.sync.field-values :as sync.field-values] [metabase.types :as types] [metabase.util :as u] @@ -75,7 +76,10 @@ (sync.concurrent/submit-task (fn [] (let [database (table/database (first newly-unhidden))] - (if (driver.u/can-connect-with-details? (:engine database) (:details database)) + ;; it's okay to allow testing H2 connections during sync. We only want to disallow you from testing them for the + ;; purposes of creating a new H2 database. + (if (binding [h2/*allow-testing-h2-connections* true] + (driver.u/can-connect-with-details? (:engine database) (:details database))) (doseq [table newly-unhidden] (log/info (u/format-color 'green (trs "Table ''{0}'' is now visible. Resyncing." (:name table)))) (sync/sync-table! table)) diff --git a/src/metabase/cmd/copy.clj b/src/metabase/cmd/copy.clj index d390015ad06f0..f4fa95dbef116 100644 --- a/src/metabase/cmd/copy.clj +++ b/src/metabase/cmd/copy.clj @@ -163,18 +163,33 @@ (log/error (with-out-str (jdbc/print-sql-exception-chain e))) (throw e)))) -(def ^:private table-select-fragments - {;; ensure ID order to ensure that parent fields are inserted before children - "metabase_field" "ORDER BY id ASC"}) +(def ^:dynamic *allow-loading-h2-databases* + "Whether `load-from-h2` should allow loading H2 databases. Normally disabled for security reasons. This is only here + so we can disable this check for tests." + false) + +(defn- table-select-fragment [table-name] + (cond + ;; ensure ID order to ensure that parent fields are inserted before children + (= table-name (name (t2/table-name Field))) + "ORDER BY id ASC" + + ;; don't copy H2 Databases, since we don't allow people to create them for security reasons. + (when-not *allow-loading-h2-databases* + (= table-name (name (t2/table-name Database)))) + "WHERE engine <> 'h2'")) + +(defn- sql-for-selecting-instances-from-source-db [table-name] + (let [fragment (table-select-fragment (u/lower-case-en (name table-name)))] + (str "SELECT * FROM " + (name table-name) + (when fragment (str " " fragment))))) (defn- copy-data! [^javax.sql.DataSource source-data-source target-db-type target-db-conn-spec] (with-open [source-conn (.getConnection source-data-source)] (doseq [entity entities :let [table-name (t2/table-name entity) - fragment (table-select-fragments (u/lower-case-en (name table-name))) - sql (str "SELECT * FROM " - (name table-name) - (when fragment (str " " fragment))) + sql (sql-for-selecting-instances-from-source-db table-name) results (jdbc/reducible-query {:connection source-conn} sql)]] (transduce (partition-all chunk-size) diff --git a/src/metabase/driver.clj b/src/metabase/driver.clj index b69d08e337541..4396bd14062c9 100644 --- a/src/metabase/driver.clj +++ b/src/metabase/driver.clj @@ -7,11 +7,13 @@ these drivers define additional multimethods that child drivers should implement; see [[metabase.driver.sql]] and [[metabase.driver.sql-jdbc]] for more details." (:require + [clojure.set :as set] [clojure.string :as str] [java-time :as t] [metabase.driver.impl :as driver.impl] [metabase.models.setting :as setting :refer [defsetting]] [metabase.plugins.classloader :as classloader] + [metabase.util :as u] [metabase.util.i18n :refer [deferred-tru trs tru]] [metabase.util.log :as log] [potemkin :as p] @@ -258,6 +260,20 @@ [_] nil) +(defn dispatch-on-initialized-driver-safe-keys + "Dispatch on initialized driver, except checks for `classname`, + `subprotocol`, `connection-uri` in the details map in order to + prevent a mismatch in spec type vs driver." + [driver details-map] + (let [invalid-keys #{"classname" "subprotocol" "connection-uri"} + ks (->> details-map keys + (map name) + (map u/lower-case-en) set)] + (when (seq (set/intersection ks invalid-keys)) + (throw (ex-info "Cannot specify subname, protocol, or connection-uri in details map" + {:invalid-keys (set/intersection ks invalid-keys)}))) + (dispatch-on-initialized-driver driver))) + (defmulti can-connect? "Check whether we can connect to a `Database` with `details-map` and perform a simple query. For example, a SQL database might try running a query like `SELECT 1;`. This function should return truthy if a connection to the DB @@ -265,7 +281,7 @@ connection cannot be made. Throw an `ex-info` containing a truthy `::can-connect-message?` in `ex-data` in order to suppress logging expected driver validation messages during setup." {:arglists '([driver details])} - dispatch-on-initialized-driver + dispatch-on-initialized-driver-safe-keys :hierarchy #'hierarchy) (defmulti dbms-version diff --git a/src/metabase/driver/h2.clj b/src/metabase/driver/h2.clj index a3fbadb8083ec..edc6e59b6f306 100644 --- a/src/metabase/driver/h2.clj +++ b/src/metabase/driver/h2.clj @@ -3,6 +3,7 @@ [clojure.math.combinatorics :as math.combo] [clojure.string :as str] [java-time :as t] + [metabase.config :as config] [metabase.db.jdbc-protocols :as mdb.jdbc-protocols] [metabase.db.spec :as mdb.spec] [metabase.driver :as driver] @@ -34,10 +35,36 @@ (driver/register! :h2, :parent :sql-jdbc) +(def ^:dynamic *allow-testing-h2-connections* + "Whether to allow testing new H2 connections. Normally this is disabled, which effectively means you cannot create new + H2 databases from the API, but this flag is here to disable that behavior for syncing existing databases, or when + needed for tests." + ;; you can disable this flag with the env var below, please do not use it under any circumstances, it is only here so + ;; existing e2e tests will run without us having to update a million tests. We should get rid of this and rework those + ;; e2e tests to use SQLite ASAP. + (or (config/config-bool :mb-dangerous-unsafe-enable-testing-h2-connections-do-not-enable) + false)) + +;;; this will prevent the H2 driver from showing up in the list of options when adding a new Database. +(defmethod driver/superseded-by :h2 [_driver] :deprecated) + (defmethod sql.qp/honey-sql-version :h2 [_driver] 2) +(defn- get-field + "Returns value of private field. This function is used to bypass field protection to instantiate + a low-level H2 Parser object in order to detect DDL statements in queries." + ([obj field] + (.get (doto (.getDeclaredField (class obj) field) + (.setAccessible true)) + obj)) + ([obj field or-else] + (try (get-field obj field) + (catch java.lang.NoSuchFieldException _e + ;; when there are no fields: return or-else + or-else)))) + ;;; +----------------------------------------------------------------------------------------------------------------+ ;;; | metabase.driver impls | ;;; +----------------------------------------------------------------------------------------------------------------+ @@ -72,6 +99,43 @@ (map u/one-or-many) (apply concat))) +(defn- malicious-property-value + "Checks an h2 connection string for connection properties that could be malicious. Markers of this include semi-colons + which allow for sql injection in org.h2.engine.Engine/openSession. The others are markers for languages like + javascript and ruby that we want to suppress." + [s] + ;; list of strings it looks for to compile scripts: + ;; https://github.com/h2database/h2database/blob/master/h2/src/main/org/h2/util/SourceCompiler.java#L178-L187 we + ;; can't use the static methods themselves since they expect to check the beginning of the string + (let [bad-markers [";" + "//javascript" + "#ruby" + "//groovy" + "@groovy"] + pred (apply some-fn (map (fn [marker] (fn [s] (str/includes? s marker))) + bad-markers))] + (pred s))) + +(defmethod driver/can-connect? :h2 + [driver {:keys [db] :as details}] + (when-not *allow-testing-h2-connections* + (throw (ex-info (tru "H2 is not supported as a data warehouse") {:status-code 400}))) + (when (string? db) + (let [connection-str (cond-> db + (not (str/includes? db "h2:")) (str/replace-first #"^" "h2:") + (not (str/includes? db "jdbc:")) (str/replace-first #"^" "jdbc:")) + connection-info (org.h2.engine.ConnectionInfo. connection-str nil nil nil) + properties (get-field connection-info "prop") + bad-props (into {} (keep (fn [[k v]] (when (malicious-property-value v) [k v]))) + properties)] + (when (seq bad-props) + (throw (ex-info "Malicious keys detected" {:keys (keys bad-props)}))) + ;; keys are uppercased by h2 when parsed: + ;; https://github.com/h2database/h2database/blob/master/h2/src/main/org/h2/engine/ConnectionInfo.java#L298 + (when (contains? properties "INIT") + (throw (ex-info "INIT not allowed" {:keys ["INIT"]}))))) + (sql-jdbc.conn/can-connect? driver details)) + (defmethod driver/db-start-of-week :h2 [_] :monday) @@ -109,18 +173,6 @@ (ex-info (tru "Running SQL queries against H2 databases using the default (admin) database user is forbidden.") {:type qp.error-type/db}))))))) -(defn- get-field - "Returns value of private field. This function is used to bypass field protection to instantiate - a low-level H2 Parser object in order to detect DDL statements in queries." - ([obj field] - (.get (doto (.getDeclaredField (class obj) field) - (.setAccessible true)) - obj)) - ([obj field or-else] - (try (get-field obj field) - (catch java.lang.NoSuchFieldException _e - ;; when there are no fields: return or-else - or-else)))) (defn- make-h2-parser "Returns an H2 Parser object for the given (H2) database ID" @@ -452,7 +504,7 @@ (defmethod sql-jdbc.conn/connection-details->spec :h2 [_ details] - {:pre [(map? details)]} + {:pre [(map? details) (:db details)]} (mdb.spec/spec :h2 (update details :db connection-string-set-safe-options))) (defmethod sql-jdbc.sync/active-tables :h2 diff --git a/src/metabase/driver/sql_jdbc/connection.clj b/src/metabase/driver/sql_jdbc/connection.clj index 985c30fced7fe..1530e53250184 100644 --- a/src/metabase/driver/sql_jdbc/connection.clj +++ b/src/metabase/driver/sql_jdbc/connection.clj @@ -35,7 +35,7 @@ WANT A CONNECTION SPEC FOR RUNNING QUERIES USE [[db->pooled-connection-spec]] INSTEAD WHICH WILL RETURN A *POOLED* CONNECTION SPEC." {:arglists '([driver details-map])} - driver/dispatch-on-initialized-driver + driver/dispatch-on-initialized-driver-safe-keys :hierarchy #'driver/hierarchy) diff --git a/src/metabase/driver/util.clj b/src/metabase/driver/util.clj index eda377b8665ec..fd979f82b34a2 100644 --- a/src/metabase/driver/util.clj +++ b/src/metabase/driver/util.clj @@ -145,6 +145,7 @@ ;; actually if we are going to `throw-exceptions` we'll rethrow the original but attempt to humanize the message ;; first (catch Throwable e + (log/errorf e "Failed to connect to Database") (throw (if-let [humanized-message (some->> (.getMessage e) (driver/humanize-connection-error-message driver))] (let [error-data (cond diff --git a/src/metabase/setup.clj b/src/metabase/setup.clj index f7bbac8686c19..023bd87386944 100644 --- a/src/metabase/setup.clj +++ b/src/metabase/setup.clj @@ -1,9 +1,11 @@ (ns metabase.setup (:require [environ.core :as env] + [metabase.config :as config] [metabase.db.connection :as mdb.connection] [metabase.models.setting :as setting :refer [defsetting Setting]] [metabase.models.user :refer [User]] + [metabase.util.i18n :refer [deferred-tru tru]] [toucan.db :as db]) (:import (java.util UUID))) @@ -37,10 +39,14 @@ (setting/set-value-of-type! :string :setup-token (str (UUID/randomUUID))))) (defsetting has-user-setup - "A value that is true iff the metabase instance has one or more users registered." + (deferred-tru "A value that is true iff the metabase instance has one or more users registered.") :visibility :public :type :boolean - :setter :none + :setter (fn [value] + (if (or config/is-dev? config/is-test?) + (setting/set-value-of-type! :boolean :has-user-setup value) + (throw (ex-info (tru "Cannot set `has-user-setup`.") + {:value value})))) ;; Once a User is created it's impossible for this to ever become falsey -- deleting the last User is disallowed. ;; After this returns true once the result is cached and it will continue to return true forever without any ;; additional DB hits. @@ -49,8 +55,14 @@ ;; it out in the REPL :getter (let [app-db-id->user-exists? (atom {})] (fn [] - (or (get @app-db-id->user-exists? (mdb.connection/unique-identifier)) - (let [exists? (db/exists? User)] - (swap! app-db-id->user-exists? assoc (mdb.connection/unique-identifier) exists?) - exists?)))) + (let [possible-override (when (or config/is-dev? config/is-test?) + ;; allow for overriding in dev and test + (setting/get-value-of-type :boolean :has-user-setup))] + ;; override could be false so have to check non-nil + (if (some? possible-override) + possible-override + (or (get @app-db-id->user-exists? (mdb.connection/unique-identifier)) + (let [exists? (db/exists? User)] + (swap! app-db-id->user-exists? assoc (mdb.connection/unique-identifier) exists?) + exists?)))))) :doc false) diff --git a/src/metabase/sync.clj b/src/metabase/sync.clj index 8811cd1d425c0..894a8f564a9ea 100644 --- a/src/metabase/sync.clj +++ b/src/metabase/sync.clj @@ -9,6 +9,7 @@ In the near future these steps will be scheduled individually, meaning those functions will be called directly instead of calling the `sync-database!` function to do all three at once." (:require + [metabase.driver.h2 :as h2] [metabase.driver.util :as driver.u] [metabase.models.field :as field] [metabase.models.table :as table] @@ -70,7 +71,10 @@ [field :- i/FieldInstance] (let [table (field/table field) database (table/database table)] - (if (driver.u/can-connect-with-details? (:engine database) (:details database)) + ;; it's okay to allow testing H2 connections during sync. We only want to disallow you from testing them for the + ;; purposes of creating a new H2 database. + (if (binding [h2/*allow-testing-h2-connections* true] + (driver.u/can-connect-with-details? (:engine database) (:details database))) (sync-util/with-error-handling (format "Error refingerprinting field %s" (sync-util/name-for-logging field)) (fingerprint/refingerprint-field field)) diff --git a/test/metabase/api/database_test.clj b/test/metabase/api/database_test.clj index 9527dd454d165..1192cd20845c6 100644 --- a/test/metabase/api/database_test.clj +++ b/test/metabase/api/database_test.clj @@ -7,6 +7,7 @@ [metabase.api.database :as api.database] [metabase.api.table :as api.table] [metabase.driver :as driver] + [metabase.driver.h2 :as h2] [metabase.driver.util :as driver.u] [metabase.mbql.schema :as mbql.s] [metabase.models @@ -28,7 +29,9 @@ [ring.util.codec :as codec] [schema.core :as s] [toucan.db :as db] - [toucan.hydrate :as hydrate])) + [toucan.hydrate :as hydrate] + [toucan2.core :as t2] + [toucan2.tools.with-temp :as t2.with-temp])) (use-fixtures :once (fixtures/initialize :db :plugins :test-drivers)) @@ -281,6 +284,15 @@ :engine (u/qualified-name ::test-driver) :details {:db "my_db"}})))))))) +(deftest disallow-creating-h2-database-test + (testing "POST /api/database/:id" + (mt/with-model-cleanup [Database] + (let [db-name (mt/random-name) + details (:details (mt/db))] + (is (= {:message "H2 is not supported as a data warehouse"} + (mt/user-http-request :crowberto :post 400 "database" {:engine :h2, :name db-name, :details details}))) + (is (not (t2/exists? Database :name db-name))))))) + (deftest delete-database-test (testing "DELETE /api/database/:id" (testing "Check that a superuser can delete a Database" @@ -292,6 +304,11 @@ (mt/with-temp Database [db] (mt/user-http-request :rasta :delete 403 (format "database/%d" (:id db))))))) +(defn- api-update-database! [expected-status-code db-or-id changes] + (with-redefs [h2/*allow-testing-h2-connections* true] + (mt/user-http-request :crowberto :put expected-status-code (format "database/%d" (u/the-id db-or-id)) + changes))) + (deftest update-database-test (testing "PUT /api/database/:id" (testing "Check that we can update fields in a Database" @@ -302,9 +319,10 @@ :cache_ttl 1337 :details {:host "localhost", :port 5432, :dbname "fakedb", :user "rastacan"}} update! (fn [expected-status-code] - (mt/user-http-request :crowberto :put expected-status-code (format "database/%d" db-id) updates))] + (api-update-database! expected-status-code db-id updates))] (testing "Should check that connection details are valid on save" - (is (string? (:message (update! 400))))) + (is (= {:message "Assert failed: (:db details)"} + (update! 400)))) (testing "If connection details are valid, we should be able to update the Database" (with-redefs [driver/can-connect? (constantly true)] (is (= nil @@ -342,6 +360,27 @@ (let [curr-db (db/select-one [Database :cache_ttl], :id db-id)] (is (= nil (:cache_ttl curr-db))))))))) +(deftest disallow-updating-h2-database-details-test + (testing "PUT /api/database/:id" + (letfn [(update! [db request-body] + (mt/user-http-request :crowberto :put 400 (str "database/" (u/the-id db)) request-body))] + (t2.with-temp/with-temp [Database db {:name (mt/random-name) + :details (:details (mt/db)) + :engine :postgres}] + (testing "Don't allow changing engine to H2" + (is (= {:message "H2 is not supported as a data warehouse"} + (update! db {:engine :h2}))) + (is (= :postgres + (t2/select-one-fn :engine Database (u/the-id db)))))) + (t2.with-temp/with-temp [Database db {:name (mt/random-name) + :details (:details (mt/db)) + :engine :h2}] + (testing "Don't allow editing H2 connection details" + (is (= {:message "H2 is not supported as a data warehouse"} + (update! db {:details {:db "mem:test-data;USER=GUEST;PASSWORD=guest;WHATEVER=true"}}))) + (is (= (:details db) + (t2/select-one-fn :details Database (u/the-id db))))))))) + (deftest enable-model-actions-with-user-controlled-scheduling-test (testing "Should be able to enable/disable actions for a database with user-controlled scheduling (metabase#30699)" (mt/with-temp Database [{db-id :id} {:details {:let-user-control-scheduling true} @@ -797,14 +836,13 @@ (testing "We cannot if we don't mark `:let-user-control-scheduling`" (mt/with-temp Database [db {:engine "h2", :details (:details (mt/db))}] (is (=? (select-keys (mt/db) [:cache_field_values_schedule :metadata_sync_schedule]) - (mt/user-http-request :crowberto :put 200 (format "database/%d" (u/the-id db)) - (assoc db :schedules attempted)))) + (api-update-database! 200 db (assoc db :schedules attempted)))) (is (not= expected (into {} (db/select-one [Database :cache_field_values_schedule :metadata_sync_schedule] :id (u/the-id db))))))) (testing "We can if we mark `:let-user-control-scheduling`" (mt/with-temp Database [db {:engine "h2", :details (:details (mt/db))}] (is (=? expected - (mt/user-http-request :crowberto :put 200 (format "database/%d" (u/the-id db)) + (api-update-database! 200 db (-> (into {} db) (assoc :schedules attempted) (assoc-in [:details :let-user-control-scheduling] true))))) @@ -816,7 +854,7 @@ :let-user-control-scheduling true)} original-custom-schedules)] (is (=? {:id (u/the-id db)} - (mt/user-http-request :crowberto :put 200 (format "database/%d" (u/the-id db)) + (api-update-database! 200 db (assoc-in db [:details :let-user-control-scheduling] false)))) (let [schedules (into {} (db/select-one [Database :cache_field_values_schedule :metadata_sync_schedule] :id (u/the-id db)))] (is (not= original-custom-schedules schedules)) @@ -915,33 +953,46 @@ (is (= "You don't have permissions to do that." (mt/user-http-request :rasta :post 403 (format "database/%d/discard_values" (mt/id))))))) +(defn- api-validate-database + ([request-body] + (api-validate-database nil request-body)) + + ([{:keys [expected-status-code user] + :or {expected-status-code 200 + user :crowberto}} + request-body] + (with-redefs [h2/*allow-testing-h2-connections* true] + (mt/user-http-request user :post expected-status-code "database/validate" request-body)))) + +(defn- test-connection-details [engine details] + (with-redefs [h2/*allow-testing-h2-connections* true] + (#'api.database/test-connection-details engine details))) + (deftest validate-database-test (testing "POST /api/database/validate" (testing "Should require superuser permissions" (is (= "You don't have permissions to do that." - (mt/user-http-request :rasta :post 403 "database/validate" - {:details {:engine :h2, :details (:details (mt/db))}})))) + (api-validate-database {:user :rasta, :expected-status-code 403} + {:details {:engine :h2, :details (:details (mt/db))}})))) (testing "Underlying `test-connection-details` function should work" (is (= (:details (mt/db)) - (#'api.database/test-connection-details "h2" (:details (mt/db)))))) + (test-connection-details "h2" (:details (mt/db)))))) (testing "Valid database connection details" (is (= {:valid true} - (mt/user-http-request :crowberto :post 200 "database/validate" - {:details {:engine :h2, :details (:details (mt/db))}})))) + (api-validate-database {:details {:engine :h2, :details (:details (mt/db))}})))) (testing "invalid database connection details" (testing "calling test-connection-details directly" - (is (= {:errors {:db "check your connection string"} + (is (= {:errors {:db "check your connection string"} :message "Implicitly relative file paths are not allowed." :valid false} - (#'api.database/test-connection-details "h2" {:db "ABC"})))) + (test-connection-details "h2" {:db "ABC"})))) (testing "via the API endpoint" (is (= {:valid false} - (mt/user-http-request :crowberto :post 200 "database/validate" - {:details {:engine :h2, :details {:db "ABC"}}}))))) + (api-validate-database {:details {:engine :h2, :details {:db "ABC"}}}))))) (let [call-count (atom 0) ssl-values (atom []) @@ -1464,8 +1515,11 @@ (letfn [(settings [] (db/select-one-field :settings Database :id (mt/id))) (set-settings! [m] - (mt/user-http-request :crowberto :put 200 (format "database/%d" (mt/id)) - {:settings m}))] + (with-redefs [h2/*allow-testing-h2-connections* true] + (u/prog1 (mt/user-http-request :crowberto :put 200 (format "database/%d" (mt/id)) + {:settings m}) + (is (=? {:id (mt/id)} + <>)))))] (testing "Should initially be nil" (is (nil? (settings)))) (testing "Set initial value" diff --git a/test/metabase/api/setting_test.clj b/test/metabase/api/setting_test.clj index 0727ccbd3e15a..c726bca1be8e7 100644 --- a/test/metabase/api/setting_test.clj +++ b/test/metabase/api/setting_test.clj @@ -2,6 +2,7 @@ (:require [clojure.test :refer :all] [metabase.api.common.validation :as validation] + [metabase.driver.h2 :as h2] [metabase.models.setting :as setting :refer [defsetting]] [metabase.models.setting-test :as models.setting-test] [metabase.public-settings.premium-features-test @@ -11,6 +12,8 @@ [metabase.util.i18n :refer [deferred-tru]] [schema.core :as s])) +(comment h2/keep-me) + (set! *warn-on-reflection* true) (use-fixtures :once (fixtures/initialize :db)) @@ -125,6 +128,13 @@ (test-api-setting-integer! 42) (is (= 42 (fetch-setting :test-api-setting-integer 200)))))) +(deftest ^:parallel engines-mark-h2-superseded-test + (testing "GET /api/setting/:key" + (testing "H2 should have :superseded-by set so it doesn't show up in the list of available drivers in the UI DB edit forms" + (is (=? {:driver-name "H2" + :superseded-by "deprecated"} + (:h2 (fetch-setting :engines 200))))))) + (deftest fetch-calculated-settings-test (testing "GET /api/setting" (testing "Should return the correct `:value` for Settings with no underlying DB/env var value" diff --git a/test/metabase/api/setup_test.clj b/test/metabase/api/setup_test.clj index ead2a968cba3d..a232888db1389 100644 --- a/test/metabase/api/setup_test.clj +++ b/test/metabase/api/setup_test.clj @@ -7,6 +7,7 @@ [medley.core :as m] [metabase.analytics.snowplow-test :as snowplow-test] [metabase.api.setup :as api.setup] + [metabase.driver.h2 :as h2] [metabase.events :as events] [metabase.http-client :as client] [metabase.models :refer [Activity Database Table User]] @@ -19,7 +20,8 @@ [metabase.util :as u] [metabase.util.schema :as su] [schema.core :as schema] - [toucan.db :as db])) + [toucan.db :as db] + [toucan2.core :as t2])) (set! *warn-on-reflection* true) @@ -65,10 +67,11 @@ (do-with-setup* request-body (fn [] - (with-redefs [api.setup/*allow-api-setup-after-first-user-is-created* true] + (with-redefs [api.setup/*allow-api-setup-after-first-user-is-created* true + h2/*allow-testing-h2-connections* true] (testing "API response should return a Session UUID" - (is (schema= {:id (schema/pred mt/is-uuid-string? "UUID string")} - (client/client :post 200 "setup" request-body)))) + (is (=? {:id mt/is-uuid-string?} + (client/client :post 200 "setup" request-body)))) ;; reset our setup token (setup/create-token!) (thunk)))))) @@ -164,23 +167,25 @@ (deftest create-database-test (testing "POST /api/setup" (testing "Check that we can Create a Database when we set up MB (#10135)" - (doseq [[k {:keys [default]}] {:is_on_demand {:default false} + (doseq [:let [details (:details (mt/db))] + [k {:keys [default]}] {:is_on_demand {:default false} :is_full_sync {:default true} :auto_run_queries {:default true}} v [true false nil]] (let [db-name (mt/random-name)] (with-setup {:database {:engine "h2" :name db-name - :details {:db "file:/home/hansen/Downloads/Metabase/longnames.db", - :ssl true} + :details details k v}} (testing "Database should be created" (is (= true (db/exists? Database :name db-name)))) (testing (format "should be able to set %s to %s (default: %s) during creation" k (pr-str v) default) (is (= (if (some? v) v default) - (db/select-one-field k Database :name db-name)))))))) + (db/select-one-field k Database :name db-name)))))))))) +(deftest create-database-trigger-sync-test + (testing "POST /api/setup" (testing "Setup should trigger sync right away for the newly created Database (#12826)" (let [db-name (mt/random-name)] (mt/with-open-channels [chan (a/chan)] @@ -202,17 +207,36 @@ (wait-for-result (fn [] (let [cnt (db/count Table :db_id (u/the-id db))] (when (= cnt 4) - cnt)))))))))))) + cnt)))))))))))))) +(deftest create-database-test-error-conditions-test + (testing "POST /api/setup" (testing "error conditions" (testing "should throw Exception if driver is invalid" (is (= {:errors {:database {:engine "Cannot create Database: cannot find driver my-fake-driver."}}} - (with-redefs [api.setup/*allow-api-setup-after-first-user-is-created* true] + (with-redefs [api.setup/*allow-api-setup-after-first-user-is-created* true + h2/*allow-testing-h2-connections* true] (client/client :post 400 "setup" (assoc (default-setup-input) :database {:engine "my-fake-driver" :name (mt/random-name) :details {}}))))))))) +(deftest disallow-h2-setup-test + (testing "POST /api/setup" + (mt/with-temporary-setting-values [has-user-setup false] + (let [details (:details (mt/db)) + db-name (mt/random-name) + request (merge (default-setup-input) + {:database {:engine :h2 + :details details + :name db-name}})] + (do-with-setup* + request + (fn [] + (is (=? {:message "H2 is not supported as a data warehouse"} + (mt/user-http-request :crowberto :post 400 "setup" request))) + (is (not (t2/exists? Database :name db-name))))))))) + (s/def ::setup!-args (s/cat :expected-status (s/? integer?) :f any? @@ -293,7 +317,7 @@ (setting.cache-test/reset-last-update-check!) (setting.cache-test/clear-cache!) (let [db-name (mt/random-name)] - (with-setup {:database {:engine "h2", :name db-name}} + (with-setup {:database {:details (:details (mt/db)), :engine "h2", :name db-name}} (is (db/exists? Database :name db-name))))))) (deftest has-user-setup-setting-test @@ -338,8 +362,9 @@ body {:token setup-token :prefs {:site_locale "es_MX" :site_name site-name} - :database {:engine "h2" - :name db-name} + :database {:engine "h2" + :details (:details (mt/db)) + :name db-name} :user {:first_name (mt/random-name) :last_name (mt/random-name) :email user-email @@ -348,6 +373,7 @@ body (fn [] (with-redefs [api.setup/*allow-api-setup-after-first-user-is-created* true + h2/*allow-testing-h2-connections* true api.setup/setup-set-settings! (let [orig @#'api.setup/setup-set-settings!] (fn [& args] (apply orig args) @@ -376,30 +402,47 @@ ;;; | POST /api/setup/validate | ;;; +----------------------------------------------------------------------------------------------------------------+ +(defn- api-validate [expected-status-code request-body] + (with-redefs [h2/*allow-testing-h2-connections* true] + (client/client :post expected-status-code "setup/validate" request-body))) + (deftest validate-setup-test (testing "POST /api/setup/validate" (testing "Should validate token" - (is (= {:errors {:token "Token does not match the setup token."}} - (client/client :post 400 "setup/validate" {}))) - (is (= {:errors {:token "Token does not match the setup token."}} - (client/client :post 400 "setup/validate" {:token "foobar"}))) + (mt/with-temporary-setting-values [has-user-setup false] + (is (= {:errors {:token "Token does not match the setup token."}} + (api-validate 400 {}))) + (is (= {:errors {:token "Token does not match the setup token."}} + (api-validate 400 {:token "foobar"})))) ;; make sure we have a valid setup token (setup/create-token!) (is (= {:errors {:engine "value must be a valid database engine."}} - (client/client :post 400 "setup/validate" {:token (setup/setup-token)})))) - - (testing "should validate that database connection works" - (is (= {:errors {:db "check your connection string"}, - :message "Database cannot be found."} - (client/client :post 400 "setup/validate" {:token (setup/setup-token) - :details {:engine "h2" - :details {:db "file:///tmp/fake.db"}}})))) - - (testing "should return 204 no content if everything is valid" - (is (= nil - (client/client :post 204 "setup/validate" {:token (setup/setup-token) - :details {:engine "h2" - :details (:details (mt/db))}})))))) + (api-validate 400 {:token (setup/setup-token)})))) + + (mt/with-temporary-setting-values [has-user-setup false] + (testing "should validate that database connection works" + (is (= {:errors {:db "check your connection string"}, + :message "Database cannot be found."} + (api-validate 400 {:token (setup/setup-token) + :details {:engine "h2" + :details {:db "file:///tmp/fake.db"}}})))) + + (testing "should return 204 no content if everything is valid" + (is (= nil + (api-validate 204 {:token (setup/setup-token) + :details {:engine "h2" + :details (:details (mt/db))}}))))))) + +(deftest disallow-h2-validation-test + (testing "POST /api/setup/validate" + (mt/with-temporary-setting-values [has-user-setup false] + (setup/create-token!) + (let [details (:details (mt/db)) + request {:details {:engine :h2 + :details details} + :token (setup/setup-token)}] + (is (= {:message "H2 is not supported as a data warehouse"} + (mt/user-http-request :crowberto :post 400 "setup/validate" request))))))) ;;; +----------------------------------------------------------------------------------------------------------------+ diff --git a/test/metabase/cmd/copy_test.clj b/test/metabase/cmd/copy_test.clj index f2cd1113cd4be..21fd6773031a5 100644 --- a/test/metabase/cmd/copy_test.clj +++ b/test/metabase/cmd/copy_test.clj @@ -2,11 +2,32 @@ (:require [clojure.test :refer :all] [metabase.cmd.copy :as copy] + [metabase.models :refer [Database]] [metabase.plugins.classloader :as classloader] + [metabase.test :as mt] [metabase.util :as u] - [toucan.models :as models])) + [toucan.models :as models] + [toucan2.core :as t2])) -(deftest all-models-accounted-for-test +(deftest ^:parallel sql-for-selecting-instances-from-source-db-test + (are [table-name] (re-find #"(?i)SELECT \* FROM METABASE_DATABASE WHERE engine <> 'h2'" + (#'copy/sql-for-selecting-instances-from-source-db table-name)) + (t2/table-name Database) + :METABASE_DATABASE + "metabase_database") + ;; make sure the H2 `test-data` DB is loaded + (mt/db) + (let [sql (#'copy/sql-for-selecting-instances-from-source-db (t2/table-name Database)) + selected-drivers (t2/select-fn-set :engine Database sql)] + (is (not (contains? selected-drivers :h2))))) + +(deftest ^:parallel allow-loading-h2-databases-test + (testing `copy/*allow-loading-h2-databases* + (binding [copy/*allow-loading-h2-databases* true] + (is (= "SELECT * FROM metabase_database" + (#'copy/sql-for-selecting-instances-from-source-db (t2/table-name Database))))))) + +(deftest ^:paralell all-models-accounted-for-test ;; This fetches the `metabase.cmd.load-from-h2/entities` and compares it all existing entities (let [migrated-model-names (set (map :name copy/entities)) ;; Models that should *not* be migrated in `load-from-h2`. diff --git a/test/metabase/cmd/dump_to_h2_test.clj b/test/metabase/cmd/dump_to_h2_test.clj index f897ebf6ea973..13ac9a82675e0 100644 --- a/test/metabase/cmd/dump_to_h2_test.clj +++ b/test/metabase/cmd/dump_to_h2_test.clj @@ -78,13 +78,14 @@ (persistent-data-source driver/*driver* db-name))] (when-not (= driver/*driver* :h2) (tx/create-db! driver/*driver* {:database-name db-name})) - (load-from-h2/load-from-h2! h2-fixture-db-file) - (encryption-test/with-secret-key "89ulvIGoiYw6mNELuOoEZphQafnF/zYe+3vT+v70D1A=" - (db/insert! Setting {:key "my-site-admin", :value "baz"}) - (db/update! Database 1 {:details "{\"db\":\"/tmp/test.db\"}"}) - (dump-to-h2/dump-to-h2! h2-file-plaintext {:dump-plaintext? true}) - (dump-to-h2/dump-to-h2! h2-file-enc {:dump-plaintext? false}) - (dump-to-h2/dump-to-h2! h2-file-default-enc)) + (binding [copy/*allow-loading-h2-databases* true] + (load-from-h2/load-from-h2! h2-fixture-db-file) + (encryption-test/with-secret-key "89ulvIGoiYw6mNELuOoEZphQafnF/zYe+3vT+v70D1A=" + (db/insert! Setting {:key "my-site-admin", :value "baz"}) + (db/update! Database 1 {:details "{\"db\":\"/tmp/test.db\"}"}) + (dump-to-h2/dump-to-h2! h2-file-plaintext {:dump-plaintext? true}) + (dump-to-h2/dump-to-h2! h2-file-enc {:dump-plaintext? false}) + (dump-to-h2/dump-to-h2! h2-file-default-enc))) (testing "decodes settings and dashboard.details" (with-open [target-conn (.getConnection (copy.h2/h2-data-source h2-file-plaintext))] diff --git a/test/metabase/cmd/load_and_dump_test.clj b/test/metabase/cmd/load_and_dump_test.clj index 6570a2c9df72d..aa63ac30717fb 100644 --- a/test/metabase/cmd/load_and_dump_test.clj +++ b/test/metabase/cmd/load_and_dump_test.clj @@ -3,6 +3,7 @@ [clojure.java.io :as io] [clojure.test :refer :all] [metabase.cmd.compare-h2-dbs :as compare-h2-dbs] + [metabase.cmd.copy :as copy] [metabase.cmd.copy.h2 :as copy.h2] [metabase.cmd.dump-to-h2 :as dump-to-h2] [metabase.cmd.load-from-h2 :as load-from-h2] @@ -41,8 +42,9 @@ (with-redefs [i18n.impl/site-locale-from-setting-fn (atom (constantly false))] (when-not (= driver/*driver* :h2) (tx/create-db! driver/*driver* {:database-name db-name})) - (load-from-h2/load-from-h2! h2-fixture-db-file) - (dump-to-h2/dump-to-h2! h2-file) + (binding [copy/*allow-loading-h2-databases* true] + (load-from-h2/load-from-h2! h2-fixture-db-file) + (dump-to-h2/dump-to-h2! h2-file)) (is (not (compare-h2-dbs/different-contents? h2-file h2-fixture-db-file)))))))))) diff --git a/test/metabase/cmd/rotate_encryption_key_test.clj b/test/metabase/cmd/rotate_encryption_key_test.clj index 98e01a0e584e5..bfeade2f38e81 100644 --- a/test/metabase/cmd/rotate_encryption_key_test.clj +++ b/test/metabase/cmd/rotate_encryption_key_test.clj @@ -3,6 +3,7 @@ [clojure.java.jdbc :as jdbc] [clojure.test :refer :all] [metabase.cmd :as cmd] + [metabase.cmd.copy :as copy] [metabase.cmd.dump-to-h2-test :as dump-to-h2-test] [metabase.cmd.load-from-h2 :as load-from-h2] [metabase.cmd.rotate-encryption-key :refer [rotate-encryption-key!]] @@ -95,7 +96,8 @@ mdb.connection/*application-db* (mdb.connection/application-db driver/*driver* data-source)] (when-not (= driver/*driver* :h2) (tx/create-db! driver/*driver* {:database-name db-name})) - (load-from-h2/load-from-h2! h2-fixture-db-file) + (binding [copy/*allow-loading-h2-databases* true] + (load-from-h2/load-from-h2! h2-fixture-db-file)) (db/insert! Setting {:key "nocrypt", :value "unencrypted value"}) (db/insert! Setting {:key "settings-last-updated", :value original-timestamp}) (let [u (db/insert! User {:email "nobody@nowhere.com" diff --git a/test/metabase/driver/h2_test.clj b/test/metabase/driver/h2_test.clj index cb87b9a1e3381..5aba84ef29a61 100644 --- a/test/metabase/driver/h2_test.clj +++ b/test/metabase/driver/h2_test.clj @@ -59,11 +59,47 @@ (deftest only-connect-to-existing-dbs-test (testing "Make sure we *cannot* connect to a non-existent database by default" - (is (= ::exception-thrown - (try (driver/can-connect? :h2 {:db (str (System/getProperty "user.dir") "/toucan_sightings")}) - (catch org.h2.jdbc.JdbcSQLNonTransientConnectionException e - (and (re-matches #"Database .+ not found, .+" (.getMessage e)) - ::exception-thrown))))))) + (binding [h2/*allow-testing-h2-connections* true] + (is (thrown-with-msg? + org.h2.jdbc.JdbcSQLNonTransientConnectionException + #"Database .+ not found, .+" + (driver/can-connect? :h2 {:db (str (System/getProperty "user.dir") "/toucan_sightings")})))))) + +(deftest ^:parallel only-connect-when-non-malicious-properties + (testing "Reject connection strings with malicious properties" + (let [conn-str (str "jdbc:h2:file:" + (System/getProperty "user.dir") + "/toucan_sightings.db" + ";TRACE_LEVEL_SYSTEM_OUT=1\\;CREATE TRIGGER IAMPWNED BEFORE SELECT ON INFORMATION_SCHEMA.TABLES AS $$//javascript\nnew java.net.URL('http://localhost:3000/api/health').openConnection().getContentLength()\n$$--=x\\;") + result (try (binding [h2/*allow-testing-h2-connections* true] + (driver/can-connect? :h2 {:db conn-str})) + ::did-not-throw + (catch Exception e e))] + (is (instance? clojure.lang.ExceptionInfo result)) + (is (partial= {:cause "Malicious keys detected" + :data {:keys ["TRACE_LEVEL_SYSTEM_OUT"]}} + (Throwable->map result))))) + (testing "Reject connection details which lie about their driver" + (let [conn "mem:fake-h2-db" + f (fn f [details] + (try (driver/can-connect? :postgres details) + ::did-not-throw + (catch Exception e e)))] + (testing "connection-uri" + (let [result (f {:connection-uri conn})] + (is (= "Cannot specify subname, protocol, or connection-uri in details map" + (ex-message result))) + (is (= {:invalid-keys #{"connection-uri"}} (ex-data result))))) + (testing "subprotocol" + (let [result (f {:db conn, :subprotocol "h2"})] + (is (= "Cannot specify subname, protocol, or connection-uri in details map" + (ex-message result))) + (is (= {:invalid-keys #{"subprotocol"}} (ex-data result))))) + (testing "subprotocol" + (let [result (f {:db conn, :classname "org.h2.Driver"})] + (is (= "Cannot specify subname, protocol, or connection-uri in details map" + (ex-message result))) + (is (= {:invalid-keys #{"classname"}} (ex-data result)))))))) (deftest db-default-timezone-test (mt/test-driver :h2 diff --git a/test/metabase/driver/sql_jdbc/connection_test.clj b/test/metabase/driver/sql_jdbc/connection_test.clj index 485b7dba85187..7e6b41d82dec4 100644 --- a/test/metabase/driver/sql_jdbc/connection_test.clj +++ b/test/metabase/driver/sql_jdbc/connection_test.clj @@ -4,6 +4,7 @@ [clojure.test :refer :all] [metabase.db.spec :as mdb.spec] [metabase.driver :as driver] + [metabase.driver.h2 :as h2] [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn] [metabase.driver.sql-jdbc.test-util :as sql-jdbc.tu] [metabase.driver.util :as driver.u] @@ -20,20 +21,19 @@ (use-fixtures :once (fixtures/initialize :db)) (deftest can-connect-with-details?-test - (is (= true - (driver.u/can-connect-with-details? :h2 (:details (data/db))))) - (testing "Lie and say Test DB is Postgres. `can-connect?` should fail" - (is (= false - (driver.u/can-connect-with-details? :postgres (:details (data/db)))))) - (testing "Random made-up DBs should fail" - (is (= false - (driver.u/can-connect-with-details? :postgres {:host "localhost" - :port 5432 - :dbname "ABCDEFGHIJKLMNOP" - :user "rasta"})))) - (testing "Things that you can connect to, but are not DBs, should fail" - (is (= false - (driver.u/can-connect-with-details? :postgres {:host "google.com", :port 80}))))) + (testing "Should not be able to connect without setting h2/*allow-testing-h2-connections*" + (is (not (driver.u/can-connect-with-details? :h2 (:details (data/db)))))) + (binding [h2/*allow-testing-h2-connections* true] + (is (driver.u/can-connect-with-details? :h2 (:details (data/db)))) + (testing "Lie and say Test DB is Postgres. `can-connect?` should fail" + (is (not (driver.u/can-connect-with-details? :postgres (:details (data/db)))))) + (testing "Random made-up DBs should fail" + (is (not (driver.u/can-connect-with-details? :postgres {:host "localhost" + :port 5432 + :dbname "ABCDEFGHIJKLMNOP" + :user "rasta"})))) + (testing "Things that you can connect to, but are not DBs, should fail" + (is (not (driver.u/can-connect-with-details? :postgres {:host "google.com", :port 80})))))) (deftest db->pooled-connection-spec-test (mt/test-driver :h2 diff --git a/test/metabase/driver/util_test.clj b/test/metabase/driver/util_test.clj index 7039927ce888e..bf4e9edfdeb12 100644 --- a/test/metabase/driver/util_test.clj +++ b/test/metabase/driver/util_test.clj @@ -3,6 +3,7 @@ [clojure.java.io :as io] [clojure.string :as str] [clojure.test :refer :all] + [metabase.driver.h2 :as h2] [metabase.driver.util :as driver.u] [metabase.public-settings.premium-features :as premium-features] [metabase.test :as mt] @@ -12,6 +13,8 @@ (java.util Base64) (javax.net.ssl SSLSocketFactory))) +(comment h2/keep-me) + (set! *warn-on-reflection* true) (use-fixtures :once (fixtures/initialize :plugins :test-drivers)) @@ -260,3 +263,8 @@ (is (false? (driver.u/semantic-version-gte [4 0 1] [4 1]))) (is (false? (driver.u/semantic-version-gte [3 9] [4 0]))) (is (false? (driver.u/semantic-version-gte [3 1] [4]))))) + +(deftest ^:parallel mark-h2-superseded-test + (testing "H2 should have :superseded-by set so it doesn't show up in the list of available drivers in the UI DB edit forms" + (is (=? {:driver-name "H2", :superseded-by :deprecated} + (:h2 (driver.u/available-drivers-info)))))) diff --git a/test/metabase/server/request/util_test.clj b/test/metabase/server/request/util_test.clj index e84a27c02baed..c487eb2bac80d 100644 --- a/test/metabase/server/request/util_test.clj +++ b/test/metabase/server/request/util_test.clj @@ -85,8 +85,8 @@ (are [ip-addresses expected] (schema= (s/conditional some? expected nil? (s/eq nil)) (request.u/geocode-ip-addresses ip-addresses)) ["8.8.8.8"] - {(s/required-key "8.8.8.8") {:description (s/eq "Los Angeles, California, United States") - :timezone (s/eq (t/zone-id "America/Los_Angeles"))}} + {(s/required-key "8.8.8.8") {:description #"United States" + :timezone (s/eq (t/zone-id "America/Chicago"))}} ;; this is from the MaxMind sample high-risk IP address list https://www.maxmind.com/en/high-risk-ip-sample-list ["185.233.100.23"] diff --git a/test/metabase/setup_test.clj b/test/metabase/setup_test.clj index e1892e01060e8..8f792f7c31665 100644 --- a/test/metabase/setup_test.clj +++ b/test/metabase/setup_test.clj @@ -30,8 +30,8 @@ (is (= false (setup/has-user-setup)))) (testing "Should continue doing new DB calls as long as there is no User" - (is (= 5 - (call-count))))))) + (is (<= (call-count) + 10)))))) ;; in dev/test we check settings for an override (testing "Switch back to the 'normal' app DB; value should still be cached for it" (db/with-call-counting [call-count] (is (= true