From 32403f0dffcc80f24eb5582022d7c0ededb7ddf3 Mon Sep 17 00:00:00 2001
From: dpsutton <dan@dpsutton.com>
Date: Mon, 19 Sep 2022 16:13:11 -0500
Subject: [PATCH] Don't use persisted model tables for segmented users (#25347)
* Don't use persisted model tables for segmented users
This actually isn't a bug, but due to very subtle and arbitrary reasons.
For background about why we need to ensure this never happens, we cannot
use persisted models when sandboxing is at play. A simple example is as
follows: make a model on a products table that does not select the
category. Have a sandbox on category such that someone can only see
products of category "Gizmo". the model lacks the category column but we
insert a where clause that still works. When the model is persisted,
there is no category column in the underlying table so sandboxing cannot
possibly work: the data necessary to filter is no longer associated with
the rest of the data in the model.
The fix for this is quite simple: in
`metabase.query-processor.middleware.fetch-source-query` we only splice
in the persisted query if the user is not a segmented user (product name
for sandboxing).
```clojure
(and persisted-info/*allow-persisted-substitution*
(not (segmented-user?)) ;; <----- new check
(:active card)
(:definition card)
(:query_hash card)
(= (:query_hash card) (persisted-info/query-hash (:dataset_query card)))
(= (:definition card) (persisted-info/metadata->definition (:result_metadata card)
(:table_name card)))
(= (:state card) "persisted"))
```
Technical details about why this bug did not manifest
When swapping out a card__<id> to a source query, if its a model we will
see if it is persisted, and if so, we will use the native sql to select
from the persisted table. It does this by adding the native sql at a key
called `:persisted-info/native` and a middleware
`#'qp.persistence/substitute-persisted-query` walks the query replacing
the query with the native:
```clojure
;; metabase.query-processor.middleware.persistence
(mbql.u/replace query
(x :guard (every-pred map? :persisted-info/native))
{:native (:persisted-info/native x)})
```
There is also a middleware that walks through the query looking for
tables with gtaps on them and replacing them. By change, the sandboxing
middleware runs immediately before the substitute-persisted middleware!
```clojure
;; literally the previous middleware
(resolve 'ee.sandbox.rows/apply-sandboxing)
#'qp.persistence/substitute-persisted-query
```
If you swap the order of these two sandboxing is broken. As is, it
"works" but not by design, just by happenstance. The sandboxing
middleware just did not know that the `:persisted-info/native` key meant
that a native query was to be substituted. In the reverse order, the
native query is already substituted and there is no change for the
sandboxing to occur.
The obvious fix is to ensure that we never even attempt to use the
persisted tables and that is what this PR does.
* Eastwood doesn't like shadowing like this
* Rearrange check order for tests
`segmented-user?` throws if there is no bound user. A test in
`fetch-source-query-test` was failing because there was no user bound,
but it wasn't attempting to swap out a persisted table, it just didn't
expect to need a user.
Moving it lower lets it short circuit on other bits that are bound to
fail (definition, query_hash, etc) requiring persistence before we check
for a bound user
---
.../row_level_restrictions_test.clj | 63 ++++++++++++++++++-
.../middleware/fetch_source_query.clj | 11 +++-
test/metabase/task/persist_refresh_test.clj | 48 +++++++-------
3 files changed, 94 insertions(+), 28 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 d3311b67613..f969265547b 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
@@ -6,21 +6,23 @@
[medley.core :as m]
[metabase-enterprise.sandbox.models.group-table-access-policy :refer [GroupTableAccessPolicy]]
[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 :as driver]
+ [metabase.driver.ddl.interface :as ddl.i]
[metabase.driver.sql.query-processor :as sql.qp]
[metabase.mbql.normalize :as mbql.normalize]
[metabase.mbql.util :as mbql.u]
[metabase.models :refer [Card Collection Field Table]]
[metabase.models.permissions :as perms]
[metabase.models.permissions-group :as perms-group]
+ [metabase.models.persisted-info :as persisted-info]
[metabase.query-processor :as qp]
[metabase.query-processor.middleware.cache-test :as cache-test]
[metabase.query-processor.middleware.permissions :as qp.perms]
[metabase.query-processor.pivot :as qp.pivot]
[metabase.query-processor.util :as qp.util]
[metabase.query-processor.util.add-alias-info :as add]
+ [metabase.task.persist-refresh :as task.persist-refresh]
[metabase.test :as mt]
[metabase.test.data.env :as tx.env]
[metabase.util :as u]
@@ -680,7 +682,7 @@
(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}
+ (mt/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}
@@ -1002,3 +1004,60 @@
;; this should *NOT* be cached because we're generating a nested query with sandboxing in play.
(is (= {:cached? false, :num-rows 5}
(run-query)))))))))
+
+(deftest persistence-disabled-when-sandboxed
+ (mt/test-driver :postgres
+ (mt/dataset sample-dataset
+ ;; with-gtaps creates a new copy of the database. So make sure to do that before anything else. Gets really
+ ;; confusing when `(mt/id)` and friends change value halfway through the test
+ (mt/with-gtaps {:gtaps {:products
+ {:remappings {:category
+ ["dimension"
+ [:field (mt/id :products :category)
+ nil]]}}}
+ :attributes {"category" nil}}
+ (mt/with-temporary-setting-values [:persisted-models-enabled true]
+ (mt/with-temp* [Card [model {:dataset true
+ :dataset_query (mt/mbql-query
+ products
+ ;; note does not include the field we have to filter on. No way
+ ;; to use the sandbox filter on the cached table
+ {:fields [$id $price]})}]]
+ ;; persist model
+ (ddl.i/check-can-persist (mt/db))
+ (persisted-info/ready-database! (mt/id))
+ (#'task.persist-refresh/refresh-tables!
+ (mt/id)
+ (var-get #'task.persist-refresh/dispatching-refresher))
+ (let [persisted-info (db/select-one 'PersistedInfo
+ :database_id (mt/id)
+ :card_id (:id model))]
+ (is (= "persisted" (:state persisted-info))
+ "Model failed to persist")
+ (is (string? (:table_name persisted-info)))
+ (let [query {:type :query
+ ;; just generate a select count(*) from card__<id>
+ :query {:aggregation [:count]
+ :source-table (str "card__" (:id model))}
+ :database (mt/id)}
+ regular-result (mt/with-test-user :crowberto
+ (qp/process-query query))
+ sandboxed-result (mt/with-user-attributes :rasta {"category" "Gizmo"}
+ (mt/with-test-user :rasta
+ (qp/process-query query)))]
+ (testing "Unsandboxed"
+ (testing "Sees full result set"
+ (is (= 200 (-> regular-result mt/rows ffirst))
+ "Expected 200 product results from cached, non-sandboxed results"))
+ (testing "Uses the cache table"
+ (is (str/includes? (-> regular-result :data :native_form :query)
+ (:table_name persisted-info))
+ "Did not use the persisted model cache")))
+ (testing "Sandboxed"
+ (testing "sees partial result"
+ (is (= 51 (-> sandboxed-result mt/rows ffirst))
+ "Sandboxed user got whole results instead of filtered"))
+ (testing "Does not use the cache table"
+ (is (not (str/includes? (-> sandboxed-result :data :native_form :query)
+ (:table_name persisted-info)))
+ "Erroneously used the persisted model cache")))))))))))
diff --git a/src/metabase/query_processor/middleware/fetch_source_query.clj b/src/metabase/query_processor/middleware/fetch_source_query.clj
index 32c741cf9e2..f697cd11b94 100644
--- a/src/metabase/query_processor/middleware/fetch_source_query.clj
+++ b/src/metabase/query_processor/middleware/fetch_source_query.clj
@@ -125,6 +125,12 @@
:table
(:table_name card)))))
+(defn- segmented-user?
+ []
+ (if-let [segmented? (resolve 'metabase-enterprise.sandbox.api.util/segmented-user?)]
+ (segmented?)
+ false))
+
(s/defn card-id->source-query-and-metadata :- SourceQueryAndMetadata
"Return the source query info for Card with `card-id`. Pass true as the optional second arg `log?` to enable
logging. (The circularity check calls this and will print more than desired)"
@@ -142,8 +148,8 @@
(db/do-post-select Card)
(db/do-post-select PersistedInfo)
first)
- (throw (ex-info (tru "Card {0} does not exist." card-id)
- {:card-id card-id})))
+ (throw (ex-info (tru "Card {0} does not exist." card-id)
+ {:card-id card-id})))
{{mbql-query :query
database-id :database
@@ -160,6 +166,7 @@
(= (:query_hash card) (persisted-info/query-hash (:dataset_query card)))
(= (:definition card) (persisted-info/metadata->definition (:result_metadata card)
(:table_name card)))
+ (not (segmented-user?))
(= (:state card) "persisted"))
source-query (cond
diff --git a/test/metabase/task/persist_refresh_test.clj b/test/metabase/task/persist_refresh_test.clj
index f938fff3540..49de4164d2e 100644
--- a/test/metabase/task/persist_refresh_test.clj
+++ b/test/metabase/task/persist_refresh_test.clj
@@ -9,7 +9,7 @@
[metabase.query-processor :as qp]
[metabase.query-processor.interface :as qp.i]
[metabase.query-processor.timezone :as qp.timezone]
- [metabase.task.persist-refresh :as pr]
+ [metabase.task.persist-refresh :as task.persist-refresh]
[metabase.test :as mt]
[metabase.util :as u]
[potemkin.types :as p]
@@ -28,48 +28,48 @@
(deftest cron-schedule-test
(testing "creates schedule per hour when less than 24 hours"
(is (= "0 0 0/8 * * ? *"
- (schedule-string (#'pr/cron-schedule "0 0 0/8 * * ? *"))))
+ (schedule-string (#'task.persist-refresh/cron-schedule "0 0 0/8 * * ? *"))))
(testing "when anchored"
(is (= "0 30 1/8 * * ? *"
- (schedule-string (#'pr/cron-schedule "0 30 1/8 * * ? *"))))))
+ (schedule-string (#'task.persist-refresh/cron-schedule "0 30 1/8 * * ? *"))))))
(testing "creates schedule string per day when 24 hours"
(is (= "0 0 0 * * ? *"
- (schedule-string (#'pr/cron-schedule "0 0 0 * * ? *"))))
+ (schedule-string (#'task.persist-refresh/cron-schedule "0 0 0 * * ? *"))))
(testing "when anchored"
(is (= "0 30 1 * * ? *"
- (schedule-string (#'pr/cron-schedule "0 30 1 * * ? *")))))))
+ (schedule-string (#'task.persist-refresh/cron-schedule "0 30 1 * * ? *")))))))
(deftest trigger-job-info-test
(testing "Database refresh trigger"
- (let [tggr (#'pr/database-trigger {:id 1} "0 0 0/5 * * ? *")]
+ (let [tggr (#'task.persist-refresh/database-trigger {:id 1} "0 0 0/5 * * ? *")]
(is (= {"db-id" 1 "type" "database"}
(qc/from-job-data (.getJobDataMap tggr))))
(is (= "0 0 0/5 * * ? *"
(schedule-string tggr)))
(is (= "metabase.task.PersistenceRefresh.database.trigger.1"
(.. tggr getKey getName))))
- (let [tggr (#'pr/database-trigger {:id 1} "0 0 0 * * ? *")]
+ (let [tggr (#'task.persist-refresh/database-trigger {:id 1} "0 0 0 * * ? *")]
(is (= {"db-id" 1 "type" "database"}
(qc/from-job-data (.getJobDataMap tggr))))
(is (= "0 0 0 * * ? *"
(schedule-string tggr))))
(testing "in report timezone UTC"
(mt/with-temporary-setting-values [report-timezone "UTC"]
- (let [tggr (#'pr/database-trigger {:id 1} "0 0 0/5 * * ? *")]
+ (let [tggr (#'task.persist-refresh/database-trigger {:id 1} "0 0 0/5 * * ? *")]
(is (= "UTC"
(.. tggr getTimeZone getID))))))
(testing "in report timezone LA"
(mt/with-temporary-setting-values [report-timezone "America/Los_Angeles"]
- (let [tggr (#'pr/database-trigger {:id 1} "0 0 0/5 * * ? *")]
+ (let [tggr (#'task.persist-refresh/database-trigger {:id 1} "0 0 0/5 * * ? *")]
(is (= "America/Los_Angeles"
(.. tggr getTimeZone getID))))))
(testing "in system timezone"
(mt/with-temporary-setting-values [report-timezone nil]
- (let [tggr (#'pr/database-trigger {:id 1} "0 0 0/5 * * ? *")]
+ (let [tggr (#'task.persist-refresh/database-trigger {:id 1} "0 0 0/5 * * ? *")]
(is (= (qp.timezone/system-timezone-id)
(.. tggr getTimeZone getID)))))))
(testing "Individual refresh trigger"
- (let [tggr (#'pr/individual-trigger {:card_id 5 :id 1})]
+ (let [tggr (#'task.persist-refresh/individual-trigger {:card_id 5 :id 1})]
(is (= {"persisted-id" 1 "type" "individual"}
(qc/from-job-data (.getJobDataMap tggr))))
(is (= "metabase.task.PersistenceRefresh.individual.trigger.1"
@@ -80,15 +80,15 @@
(let [ids (into #{} (map u/the-id dbs))]
(m/map-vals
#(select-keys % [:data :schedule :key])
- (select-keys (pr/job-info-by-db-id) ids))))
+ (select-keys (task.persist-refresh/job-info-by-db-id) ids))))
(deftest reschedule-refresh-test
(mt/with-temp-scheduler
(mt/with-temp* [Database [db-1 {:options {:persist-models-enabled true}}]
Database [db-2 {:options {:persist-models-enabled true}}]]
- (#'pr/job-init!)
+ (#'task.persist-refresh/job-init!)
(mt/with-temporary-setting-values [persisted-model-refresh-cron-schedule "0 0 0/4 * * ? *"]
- (pr/reschedule-refresh!)
+ (task.persist-refresh/reschedule-refresh!)
(is (= {(u/the-id db-1) {:data {"db-id" (u/the-id db-1) "type" "database"}
:schedule "0 0 0/4 * * ? *"
:key (format "metabase.task.PersistenceRefresh.database.trigger.%d" (u/the-id db-1))}
@@ -97,7 +97,7 @@
:key (format "metabase.task.PersistenceRefresh.database.trigger.%d" (u/the-id db-2))}}
(job-info db-1 db-2))))
(mt/with-temporary-setting-values [persisted-model-refresh-cron-schedule "0 0 0/8 * * ? *"]
- (pr/reschedule-refresh!)
+ (task.persist-refresh/reschedule-refresh!)
(is (= {(u/the-id db-1) {:data {"db-id" (u/the-id db-1) "type" "database"}
:schedule "0 0 0/8 * * ? *"
:key (format "metabase.task.PersistenceRefresh.database.trigger.%d" (u/the-id db-1))}
@@ -106,7 +106,7 @@
:key (format "metabase.task.PersistenceRefresh.database.trigger.%d" (u/the-id db-2))}}
(job-info db-1 db-2))))
(mt/with-temporary-setting-values [persisted-model-refresh-cron-schedule "0 30 1/8 * * ? *"]
- (pr/reschedule-refresh!)
+ (task.persist-refresh/reschedule-refresh!)
(is (= {(u/the-id db-1) {:data {"db-id" (u/the-id db-1) "type" "database"}
:schedule "0 30 1/8 * * ? *"
:key (format "metabase.task.PersistenceRefresh.database.trigger.%d" (u/the-id db-1))}
@@ -129,12 +129,12 @@
PersistedInfo [_punmodeled {:card_id (u/the-id unmodeled) :database_id (u/the-id db)}]]
(testing "Calls refresh on each persisted-info row"
(let [card-ids (atom #{})
- test-refresher (reify pr/Refresher
+ test-refresher (reify task.persist-refresh/Refresher
(refresh! [_ _database _definition card]
(swap! card-ids conj (:id card))
{:state :success})
(unpersist! [_ _database _persisted-info]))]
- (#'pr/refresh-tables! (u/the-id db) test-refresher)
+ (#'task.persist-refresh/refresh-tables! (u/the-id db) test-refresher)
(testing "Does not refresh archived cards or cards no longer models."
(is (= #{(u/the-id model1) (u/the-id model2)} @card-ids)))
(is (partial= {:task "persist-refresh"
@@ -145,7 +145,7 @@
{:order-by [[:id :desc]]})))))
(testing "Handles errors and continues"
(let [call-count (atom 0)
- test-refresher (reify pr/Refresher
+ test-refresher (reify task.persist-refresh/Refresher
(refresh! [_ _database _definition _card]
(swap! call-count inc)
;; throw on first persist
@@ -153,7 +153,7 @@
(throw (ex-info "DBs are risky" {:ka :boom})))
{:state :success})
(unpersist! [_ _database _persisted-info]))]
- (#'pr/refresh-tables! (u/the-id db) test-refresher)
+ (#'task.persist-refresh/refresh-tables! (u/the-id db) test-refresher)
(is (= 2 @call-count))
(is (partial= {:task "persist-refresh"
:task_details {:success 1 :error 1}}
@@ -173,17 +173,17 @@
;; need an "old enough" state change
:state_change_at (t/minus (t/local-date-time) (t/hours 2))}]]
(let [called-on (atom #{})
- test-refresher (reify pr/Refresher
+ test-refresher (reify task.persist-refresh/Refresher
(refresh! [_ _ _ _]
(is false "refresh! called on a model that should not be refreshed"))
(unpersist! [_ _database persisted-info]
(swap! called-on conj (u/the-id persisted-info))))]
(testing "Query finds deletabable, archived, and unmodeled persisted infos"
- (let [queued-for-deletion (into #{} (map :id) (#'pr/deletable-models))]
+ (let [queued-for-deletion (into #{} (map :id) (#'task.persist-refresh/deletable-models))]
(doseq [deletable-persisted [deletable punmodeled parchived]]
(is (contains? queued-for-deletion (u/the-id deletable-persisted))))))
;; we manually pass in the deleteable ones to not catch others in a running instance
- (#'pr/prune-deletables! test-refresher [deletable parchived punmodeled])
+ (#'task.persist-refresh/prune-deletables! test-refresher [deletable parchived punmodeled])
;; don't assert equality if there are any deletable in the app db
(doseq [deletable-persisted [deletable punmodeled parchived]]
(is (contains? @called-on (u/the-id deletable-persisted))))
@@ -217,7 +217,7 @@
[persisted-info-id] (persisted-info/ready-unpersisted-models! (mt/id))]
;; Persist the model
(ddl.i/check-can-persist (db/select-one Database :id (mt/id)))
- (#'pr/refresh-individual! persisted-info-id (var-get #'pr/dispatching-refresher))
+ (#'task.persist-refresh/refresh-individual! persisted-info-id (var-get #'task.persist-refresh/dispatching-refresher))
;; Check the number of rows is the same after persisting
(let [query-on-top {:database (mt/id)
:type :query
--
GitLab