From 3096b3b4abc557b503d8725d9d39ac79b2e62a11 Mon Sep 17 00:00:00 2001 From: Cam Saul Date: Tue, 12 Jan 2021 18:46:00 -0800 Subject: [PATCH 1/5] Add new test --- .dir-locals.el | 1 + .../row_level_restrictions_test.clj | 40 +++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/.dir-locals.el b/.dir-locals.el index 59d9bd66e2f1e..fec35b27192d1 100644 --- a/.dir-locals.el +++ b/.dir-locals.el @@ -22,6 +22,7 @@ (merge-with 1) (l/matche '(1 (:defn))) (l/matcha '(1 (:defn))) + (p/defprotocol+ '(1 (:defn))) (p.types/defprotocol+ '(1 (:defn))) (p.types/def-abstract-type '(1 (:defn))) (p.types/deftype+ '(2 nil nil (:defn))) diff --git a/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj b/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj index 457b3477ec7b7..4a1101be1968e 100644 --- a/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj +++ b/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj @@ -2,6 +2,7 @@ (:require [clojure [string :as str] [test :refer :all]] + [clojure.core.async :as a] [honeysql.core :as hsql] [metabase [driver :as driver] @@ -11,6 +12,7 @@ [util :as u]] [metabase-enterprise.sandbox.query-processor.middleware.row-level-restrictions :as row-level-restrictions] [metabase-enterprise.sandbox.test-util :as mt.tu] + [metabase.api.common :as api] [metabase.driver.sql.query-processor :as sql.qp] [metabase.mbql [normalize :as normalize] @@ -18,6 +20,7 @@ [metabase.models [permissions :as perms] [permissions-group :as perms-group]] + [metabase.query-processor.middleware.cache-test :as cache-test] [metabase.query-processor.util :as qputil] [metabase.test.data.env :as tx.env] [metabase.test.util :as tu] @@ -551,3 +554,40 @@ :alias "Venue"}] :order-by [[:asc $id]] :limit 3}))))))) + +(deftest dont-cache-sandboxes-test + (cache-test/with-mock-cache [save-chan] + (mt/with-gtaps {:gtaps {:venues (venues-category-mbql-gtap-def)} + :attributes {"cat" 50}} + (letfn [(run-query [] + (qp/process-query (assoc (mt/mbql-query venues {:aggregation [[:count]]}) + :cache-ttl 100)))] + (testing "Run the query, should not be cached" + (let [result (run-query)] + (is (= nil + (:cached result))) + (is (= [[10]] + (mt/rows result))))) + (testing "Cache entry should be saved within 5 seconds" + (let [[_ chan] (a/alts!! [save-chan (a/timeout 5000)])] + (is (= save-chan + chan)))) + + (testing "Run it again, should be cached" + (let [result (run-query)] + (println "result:" result) ; NOCOMMIT + (is (= true + (:cached result))) + (is (= [[10]] + (mt/rows result))))) + (testing "Run the query with different User attributes, should not get the cached result" + (mt.tu/with-user-attributes :rasta {"cat" 40} + ;; re-bind current user so updated attributes come in to effect + (mt/with-test-user :rasta + (is (= {"cat" 40} + (:login_attributes @api/*current-user*))) + (let [result (run-query)] + (is (= nil + (:cached result))) + (is (= [[9]] + (mt/rows result))))))))))) From 85433c463ef2ed3aee32a4b42dfc605a9ee45b3a Mon Sep 17 00:00:00 2001 From: Cam Saul Date: Tue, 12 Jan 2021 18:49:04 -0800 Subject: [PATCH 2/5] Move the point cached results get returned --- .../query_processor/middleware/row_level_restrictions_test.clj | 1 - src/metabase/query_processor.clj | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj b/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj index 4a1101be1968e..cc21b06afe89f 100644 --- a/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj +++ b/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj @@ -575,7 +575,6 @@ (testing "Run it again, should be cached" (let [result (run-query)] - (println "result:" result) ; NOCOMMIT (is (= true (:cached result))) (is (= [[10]] diff --git a/src/metabase/query_processor.clj b/src/metabase/query_processor.clj index 52105c675cf58..9b2e3c1636867 100644 --- a/src/metabase/query_processor.clj +++ b/src/metabase/query_processor.clj @@ -76,6 +76,7 @@ "The default set of middleware applied to queries ran via `process-query`." [#'mbql-to-native/mbql->native #'check-features/check-features + #'cache/maybe-return-cached-results #'optimize-datetime-filters/optimize-datetime-filters #'auto-parse-filter-values/auto-parse-filter-values #'wrap-value-literals/wrap-value-literals @@ -110,7 +111,6 @@ #'resolve-database-and-driver/resolve-database-and-driver #'fetch-source-query/resolve-card-id-source-tables #'store/initialize-store - #'cache/maybe-return-cached-results #'validate/validate-query #'normalize/normalize #'add-rows-truncated/add-rows-truncated From 40b783b743f4839c70a16aeb3365d6561a407f63 Mon Sep 17 00:00:00 2001 From: Cam Saul Date: Tue, 12 Jan 2021 18:54:50 -0800 Subject: [PATCH 3/5] Test fix :wrench: --- test/metabase/query_processor/middleware/cache_test.clj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/metabase/query_processor/middleware/cache_test.clj b/test/metabase/query_processor/middleware/cache_test.clj index f04f391e381b6..5ae02adab52eb 100644 --- a/test/metabase/query_processor/middleware/cache_test.clj +++ b/test/metabase/query_processor/middleware/cache_test.clj @@ -76,7 +76,7 @@ (log/tracef "Purge old entries --> store: %s" this) (a/>!! purge-chan ::purge))))) -(defn- do-with-mock-cache [f] +(defn do-with-mock-cache [f] (mt/with-open-channels [save-chan (a/chan 1) purge-chan (a/chan 1)] (mt/with-temporary-setting-values [enable-query-caching true From 3662cfe24b27df68737b24a9debda7afabd7e693 Mon Sep 17 00:00:00 2001 From: Cam Saul Date: Wed, 13 Jan 2021 11:13:59 -0800 Subject: [PATCH 4/5] Now it's working --- src/metabase/query_processor.clj | 2 +- src/metabase/query_processor/context/default.clj | 5 +++-- .../query_processor/middleware/cache.clj | 16 ++++++++-------- .../query_processor/middleware/cache/impl.clj | 2 +- .../query_processor/middleware/limit.clj | 2 +- 5 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/metabase/query_processor.clj b/src/metabase/query_processor.clj index 9b2e3c1636867..95da6d6225a3c 100644 --- a/src/metabase/query_processor.clj +++ b/src/metabase/query_processor.clj @@ -76,6 +76,7 @@ "The default set of middleware applied to queries ran via `process-query`." [#'mbql-to-native/mbql->native #'check-features/check-features + #'limit/limit #'cache/maybe-return-cached-results #'optimize-datetime-filters/optimize-datetime-filters #'auto-parse-filter-values/auto-parse-filter-values @@ -90,7 +91,6 @@ #'resolve-joins/resolve-joins #'add-implicit-joins/add-implicit-joins #'large-int-id/convert-id-to-string - #'limit/limit #'format-rows/format-rows #'desugar/desugar #'binning/update-binning-strategy diff --git a/src/metabase/query_processor/context/default.clj b/src/metabase/query_processor/context/default.clj index 1149f916b7945..084a7170d913c 100644 --- a/src/metabase/query_processor/context/default.clj +++ b/src/metabase/query_processor/context/default.clj @@ -32,8 +32,9 @@ {:data metadata}) ([result] - {:pre [(map? result)]} - (-> result + {:pre [(map? (unreduced result))]} + ;; if the result is a clojure.lang.Reduced, unwrap it so we always get back the standard-format map + (-> (unreduced result) (assoc :row_count @row-count :status :completed) (assoc-in [:data :rows] @rows))) diff --git a/src/metabase/query_processor/middleware/cache.clj b/src/metabase/query_processor/middleware/cache.clj index 246ab476ac0a1..d09cf17c3b59b 100644 --- a/src/metabase/query_processor/middleware/cache.clj +++ b/src/metabase/query_processor/middleware/cache.clj @@ -130,15 +130,15 @@ ([] (rf)) - ([acc] - ;; if results are in the 'normal format' then use the final metadata from the cache rather than - ;; whatever `acc` is right now since we don't run the entire post-processing pipeline for cached results - (let [normal-format? (and (map? acc) (seq (get-in acc [:data :cols]))) - acc* (-> (if normal-format? - @final-metadata - acc) + ([result] + (let [normal-format? (and (map? (unreduced result)) + (seq (get-in (unreduced result) [:data :cols]))) + result* (-> (if normal-format? + (merge-with merge @final-metadata (unreduced result)) + (unreduced result)) (assoc :cached true, :updated_at last-ran))] - (rf acc*))) + (rf (cond-> result* + (reduced? result) reduced)))) ([acc row] (if (map? row) diff --git a/src/metabase/query_processor/middleware/cache/impl.clj b/src/metabase/query_processor/middleware/cache/impl.clj index 1144a7807993a..8ce37c9c1adc3 100644 --- a/src/metabase/query_processor/middleware/cache/impl.clj +++ b/src/metabase/query_processor/middleware/cache/impl.clj @@ -134,7 +134,7 @@ (reduce [_ rf init] (loop [acc init] (if (reduced? acc) - (reduced acc) + acc (let [row (thaw! is)] (if (= ::eof row) acc diff --git a/src/metabase/query_processor/middleware/limit.clj b/src/metabase/query_processor/middleware/limit.clj index 69b5a046b0eaf..2102368e68c68 100644 --- a/src/metabase/query_processor/middleware/limit.clj +++ b/src/metabase/query_processor/middleware/limit.clj @@ -25,7 +25,7 @@ (let [result' (rf result row) new-row-count (vswap! row-count inc)] (if (>= new-row-count max-rows) - (reduced result') + (ensure-reduced result') result')))))) (defn limit From 875c22745d6ffb70ed46a85fce4eec00dc0ff3c8 Mon Sep 17 00:00:00 2001 From: Cam Saul Date: Wed, 13 Jan 2021 16:13:20 -0800 Subject: [PATCH 5/5] Fixes :wrench: --- src/metabase/async/util.clj | 6 +- src/metabase/models/card.clj | 16 +++++- src/metabase/models/query/permissions.clj | 9 ++- test/metabase/api/card_test.clj | 17 ++++-- .../middleware/cache/impl_test.clj | 2 +- .../query_processor/middleware/cache_test.clj | 55 +++++++++++++++++-- test/metabase/test/data/impl.clj | 17 +++--- 7 files changed, 95 insertions(+), 27 deletions(-) diff --git a/src/metabase/async/util.clj b/src/metabase/async/util.clj index 9e69c023983ad..fa02c01dae97a 100644 --- a/src/metabase/async/util.clj +++ b/src/metabase/async/util.clj @@ -46,7 +46,9 @@ ;; * `result-chan` will get the result of `(f)`, *after* `done-chan` is closed (let [done-chan (a/promise-chan) result-chan (a/promise-chan) - f* (bound-fn [] + binds (clojure.lang.Var/getThreadBindingFrame) + f* (fn [] + (clojure.lang.Var/resetThreadBindingFrame binds) (let [result (try (f) (catch Throwable e @@ -56,7 +58,7 @@ (when (some? result) (a/>!! result-chan result))) (a/close! result-chan)) - futur (.submit ^ThreadPoolExecutor (var-get (resolve 'clojure.core.async/thread-macro-executor)) ^Runnable f*)] + futur (.submit ^ThreadPoolExecutor @#'a/thread-macro-executor ^Runnable f*)] ;; if `result-chan` gets a result/closed *before* `done-chan`, it means it was closed by the caller, so we should ;; cancel the thread running `f*` (a/go diff --git a/src/metabase/models/card.clj b/src/metabase/models/card.clj index e2dabaa7d6db4..e0f4d22ba5a22 100644 --- a/src/metabase/models/card.clj +++ b/src/metabase/models/card.clj @@ -115,8 +115,20 @@ ;; Cards with queries they wouldn't be allowed to run! (when *current-user-id* (when-not (query-perms/can-run-query? query) - (throw (Exception. (tru "You do not have permissions to run ad-hoc native queries against Database {0}." - (:database query)))))) + (let [required-perms (try + (query-perms/perms-set query :throw-exceptions? true) + (catch Throwable e + e))] + (throw (ex-info (tru "You do not have permissions to run ad-hoc native queries against Database {0}." + (:database query)) + {:status-code 403 + :query query + :required-perms (if (instance? Throwable required-perms) + :error + required-perms) + :actual-perms @api/*current-user-permissions-set*} + (when (instance? Throwable required-perms) + required-perms)))))) ;; make sure this Card doesn't have circular source query references (check-for-circular-source-query-references card) (collection/check-collection-namespace Card (:collection_id card)))) diff --git a/src/metabase/models/query/permissions.clj b/src/metabase/models/query/permissions.clj index 3b95a344ad267..f632b18263c3f 100644 --- a/src/metabase/models/query/permissions.clj +++ b/src/metabase/models/query/permissions.clj @@ -126,11 +126,10 @@ ;; if for some reason we can't expand the Card (i.e. it's an invalid legacy card) just return a set of permissions ;; that means no one will ever get to see it (except for superusers who get to see everything) (catch Throwable e - (when throw-exceptions? - (throw e)) - (log/error (tru "Error calculating permissions for query: {0}" (.getMessage e)) - "\n" - (u/pprint-to-str (u/filtered-stacktrace e))) + (let [e (ex-info "Error calculating permissions for query" {:query query} e)] + (when throw-exceptions? + (throw e)) + (log/error e)) #{"/db/0/"}))) ; DB 0 will never exist (s/defn ^:private perms-set* :- #{perms/ObjectPath} diff --git a/test/metabase/api/card_test.clj b/test/metabase/api/card_test.clj index dab25f0d413e4..22c5107605864 100644 --- a/test/metabase/api/card_test.clj +++ b/test/metabase/api/card_test.clj @@ -461,12 +461,17 @@ (mt/with-temp Collection [collection] (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection) (mt/with-model-cleanup [Card] - (mt/user-http-request :rasta :post 202 "card" - (assoc (card-with-name-and-query card-name) - :collection_id (u/get-id collection), :collection_position 1)) - (is (= #metabase.models.card.CardInstance{:collection_id true, :collection_position 1} - (some-> (db/select-one [Card :collection_id :collection_position] :name card-name) - (update :collection_id (partial = (u/get-id collection)))))))))))) + (is (schema= {:collection_id (s/eq (u/get-id collection)) + :collection_position (s/eq 1) + :name (s/eq card-name) + s/Keyword s/Any} + (mt/user-http-request :rasta :post 202 "card" + (assoc (card-with-name-and-query card-name) + :collection_id (u/get-id collection), :collection_position 1)))) + (is (schema= {:collection_id (s/eq (u/get-id collection)) + :collection_position (s/eq 1) + s/Keyword s/Any} + (db/select-one Card :name card-name))))))))) (deftest need-permission-for-collection (testing "You need to have Collection permissions to create a Card in a Collection" diff --git a/test/metabase/query_processor/middleware/cache/impl_test.clj b/test/metabase/query_processor/middleware/cache/impl_test.clj index 562b054a2842e..988291e671ab7 100644 --- a/test/metabase/query_processor/middleware/cache/impl_test.clj +++ b/test/metabase/query_processor/middleware/cache/impl_test.clj @@ -10,7 +10,7 @@ (def ^:private objects [{:metadata? true} -200.0 3 "HELLO!" {:x 100, :y #t "2020-02-02", :z #{:a :b :c}} (Z. 100)]) -(defn- deserialize +(defn deserialize "Deserialize objects serialized with `serialize-async` using reducing function `rf`." ([^bytes bytea] (deserialize bytea (fn [metadata] diff --git a/test/metabase/query_processor/middleware/cache_test.clj b/test/metabase/query_processor/middleware/cache_test.clj index 5ae02adab52eb..6f5f87aa155fa 100644 --- a/test/metabase/query_processor/middleware/cache_test.clj +++ b/test/metabase/query_processor/middleware/cache_test.clj @@ -13,16 +13,23 @@ [test :as mt] [util :as u]] [metabase.driver.sql-jdbc.execute :as sql-jdbc.execute] + [metabase.middleware.session :as session] + [metabase.models + [permissions :as perms] + [permissions-group :as group]] [metabase.query-processor [streaming :as qp.streaming] [util :as qputil]] [metabase.query-processor.middleware.cache :as cache] + [metabase.query-processor.middleware.cache + [impl :as impl] + [impl-test :as impl-test]] [metabase.query-processor.middleware.cache-backend.interface :as i] - [metabase.query-processor.middleware.cache.impl :as impl] [metabase.test [fixtures :as fixtures] [util :as tu]] - [pretty.core :as pretty])) + [pretty.core :as pretty] + [schema.core :as s])) (use-fixtures :once (fixtures/initialize :db)) @@ -35,6 +42,9 @@ "Gets a message whenever old entries are purged from the test backend." nil) +(defprotocol ^:private CacheContents + (^:private contents [cache-backend])) + (defn- test-backend "In in-memory cache backend implementation." [save-chan purge-chan] @@ -45,7 +55,12 @@ (str "\n" (metabase.util/pprint-to-str 'blue (for [[hash {:keys [created]}] @store] - [hash (metabase.util/format-nanoseconds (.getNano (t/duration created (t/instant))))])))) + [hash (u/format-nanoseconds (.getNano (t/duration created (t/instant))))])))) + + CacheContents + (contents [_] + (into {} (for [[k v] store] + [k (impl-test/deserialize v)]))) i/CacheBackend (cached-results [this query-hash max-age-seconds respond] @@ -62,7 +77,7 @@ (respond nil)))) (save-results! [this query-hash results] - (let [hex-hash (buddy.core.codecs/bytes->hex query-hash)] + (let [hex-hash (codecs/bytes->hex query-hash)] (swap! store assoc hex-hash {:results results :created (t/instant)}) (log/tracef "Save results for %s --> store: %s" hex-hash this)) @@ -401,3 +416,35 @@ (is (= max-bytes (count (transduce identity (#'cache/save-results-xform 0 {} (byte 0) conj) (repeat max-bytes [1])))))))) + +(deftest perms-checks-should-still-apply-test + (testing "Double-check that perms checks still happen even for cached results" + (mt/with-temp-copy-of-db + (perms/revoke-permissions! (group/all-users) (mt/db)) + (mt/with-test-user :rasta + (with-mock-cache [save-chan] + (letfn [(run-forbidden-query [] + (qp/process-query (assoc (mt/mbql-query checkins {:aggregation [[:count]]}) + :cache-ttl 100)))] + (testing "Shouldn't be allowed to run a query if we don't have perms for it" + (is (thrown-with-msg? + clojure.lang.ExceptionInfo + #"You do not have permissions to run this query" + (run-forbidden-query)))) + (testing "Run forbidden query as superuser to populate the cache" + (session/with-current-user (mt/user->id :crowberto) + (is (= [[1000]] + (mt/rows (run-forbidden-query)))))) + (testing "Cache entry should be saved within 5 seconds" + (let [[_ chan] (a/alts!! [save-chan (a/timeout 5000)])] + (is (= save-chan + chan)))) + (testing "Run forbidden query again as superuser again, should be cached" + (session/with-current-user (mt/user->id :crowberto) + (is (schema= {:cached (s/eq true), s/Keyword s/Any} + (run-forbidden-query))))) + (testing "Run query as regular user, should get perms Exception even though result is cached" + (is (thrown-with-msg? + clojure.lang.ExceptionInfo + #"You do not have permissions to run this query" + (run-forbidden-query)))))))))) diff --git a/test/metabase/test/data/impl.clj b/test/metabase/test/data/impl.clj index 320e4d2f99f26..08d432c3330db 100644 --- a/test/metabase/test/data/impl.clj +++ b/test/metabase/test/data/impl.clj @@ -112,12 +112,14 @@ ;; make sure we're returing an up-to-date copy of the DB (Database (u/get-id db)) (catch Throwable e - (db/delete! Database :id (u/get-id db)) - (throw (ex-info "Failed to create test database" - {:driver driver - :database-name database-name - :connection-details connection-details} - e))))) + (let [e (ex-info "Failed to create test database" + {:driver driver + :database-name database-name + :connection-details connection-details} + e)] + (println (u/pprint-to-str 'red (Throwable->map e))) + (db/delete! Database :id (u/get-id db)) + (throw e))))) (catch Throwable e (let [message (format "Failed to create %s '%s' test database" driver database-name)] (println message "\n" e) @@ -261,7 +263,8 @@ (try (copy-db-tables-and-fields! old-db-id new-db-id) (do-with-db new-db f) - (finally (db/delete! Database :id new-db-id)))))) + (finally + (db/delete! Database :id new-db-id)))))) ;;; +----------------------------------------------------------------------------------------------------------------+