diff --git a/.dir-locals.el b/.dir-locals.el
index 59d9bd66e2f1e4f7920cbad3e43e48f904216205..fec35b27192d192a582801b582f95da56f6736ef 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 457b3477ec7b7b1c47b07435e6eb5219f57d551a..cc21b06afe89f3412372674b0cf170478379ea10 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,39 @@
: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)]
+ (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)))))))))))
diff --git a/src/metabase/async/util.clj b/src/metabase/async/util.clj
index 9e69c023983ad54ad941df0b3e751c7c24c98897..fa02c01dae97a526b22b7951830ca253a697f580 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 e2dabaa7d6db48b9503269d80bf4460720a3ea4c..e0f4d22ba5a22f2b61cf0ba75e6bcb9521e1e643 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 3b95a344ad267a6733a054765f3f885c73601d4d..f632b18263c3fe69ea5be6b67d04afa92176c039 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/src/metabase/query_processor.clj b/src/metabase/query_processor.clj
index 52105c675cf586fbdf5d5198f92684dcf2ef1fae..95da6d6225a3c0bb91ddb001799066666fd8300b 100644
--- a/src/metabase/query_processor.clj
+++ b/src/metabase/query_processor.clj
@@ -76,6 +76,8 @@
"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
#'wrap-value-literals/wrap-value-literals
@@ -89,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
@@ -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
diff --git a/src/metabase/query_processor/context/default.clj b/src/metabase/query_processor/context/default.clj
index 1149f916b7945f40e3cc7660c05b8313a057ce19..084a7170d913c255d5b02e225036e8cf089549bf 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 246ab476ac0a1182df442d425d6470017027d03d..d09cf17c3b59bc4dbc65518afcb4d52bac336375 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 1144a7807993a2043914b5deae605119aab8d58d..8ce37c9c1adc3aa5dc10d2a076ac552443543147 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 69b5a046b0eaf0706ede00a8036b7f0ac22a1ee0..2102368e68c68293268dea2088526752cd981fd1 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
diff --git a/test/metabase/api/card_test.clj b/test/metabase/api/card_test.clj
index dab25f0d413e4cc4ea00eae5a1e88b5b81f75c8f..22c51076058647b4407a121bbdbea04942a296b4 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 562b054a2842e6d34d52f8b8b757052b0d2e4f56..988291e671ab716c972b9886a0607b97e77f7908 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 f04f391e381b617c4c20713ac61d3415d80e5894..6f5f87aa155fafdcd1e7a79d526932c74e4f71ca 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))
@@ -76,7 +91,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
@@ -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 320e4d2f99f26ae253a150bd645ad6a7ea2cc071..08d432c3330dbbc981bec68f7d7ccf599a16e619 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))))))
;;; +----------------------------------------------------------------------------------------------------------------+