From 9fada844a77987a679ba82a976132e324d81b7e5 Mon Sep 17 00:00:00 2001
From: Noah Moss <32746338+noahmoss@users.noreply.github.com>
Date: Tue, 28 Mar 2023 12:59:22 -0400
Subject: [PATCH] Allow sandboxing based on FK even if user doesn't have
self-service access to the linked table (#29449)
* bypass perm checks for linked tables in a sandbox
* clean ns
* fix test
* add test
* uncomment test
---
.../middleware/row_level_restrictions.clj | 41 ++++++++++++-------
.../sandbox/api/field_test.clj | 2 +-
.../row_level_restrictions_test.clj | 15 ++++++-
.../metabase_enterprise/sandbox/test_util.clj | 4 +-
4 files changed, 43 insertions(+), 19 deletions(-)
diff --git a/enterprise/backend/src/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions.clj b/enterprise/backend/src/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions.clj
index a7b70976d80..a576b14a843 100644
--- a/enterprise/backend/src/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions.clj
+++ b/enterprise/backend/src/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions.clj
@@ -19,7 +19,6 @@
[metabase.models.permissions-group-membership
:refer [PermissionsGroupMembership]]
[metabase.models.query.permissions :as query-perms]
- [metabase.models.table :refer [Table]]
[metabase.plugins.classloader :as classloader]
[metabase.query-processor.error-type :as qp.error-type]
[metabase.query-processor.middleware.fetch-source-query
@@ -245,24 +244,38 @@
preprocess-source-query
(source-query-form-ensure-metadata table-id card-id)))
-(s/defn ^:private gtap->perms-set :- #{perms/PathSchema}
- "Calculate the set of permissions needed to run the query associated with a GTAP; this set of permissions is excluded
+(defn- sandbox->table-ids
+ "Returns the set of table IDs which are used by the given sandbox. These are the sandboxed table itself, as well as
+ any linked tables referenced via fields in the attribute remappings. This is the set of tables which need to be
+ excluded from subsequent permission checks in order to run the sandboxed query."
+ [{table-id :table_id, attribute-remappings :attribute_remappings}]
+ (->>
+ (for [target-field-clause (vals attribute-remappings)]
+ (mbql.u/match-one target-field-clause
+ [:field (field-id :guard integer?) _]
+ (t2/select-one-fn :table_id Field :id field-id)))
+ (cons table-id)
+ (remove nil?)
+ set))
+
+(s/defn ^:private sandbox->perms-set :- #{perms/PathSchema}
+ "Calculate the set of permissions needed to run the query associated with a sandbox; this set of permissions is excluded
during the normal QP perms check.
- Background: when applying GTAPs, we don't want the QP perms check middleware to throw an Exception if the Current
- User doesn't have permissions to run the underlying GTAP query, which will likely be greater than what they actually
- have. (For example, a User might have segmented query perms for Table 15, which is why we're applying a GTAP in the
- first place; the actual perms required to normally run the underlying GTAP query is more likely something like
- *full* query perms for Table 15.) The QP perms check middleware subtracts this set from the set of required
- permissions, allowing the user to run their GTAPped query."
- [{card-id :card_id, table-id :table_id}]
+ Background: when applying sandboxing, we don't want the QP perms check middleware to throw an Exception if the Current
+ User doesn't have permissions to run the underlying sandboxed query, which will likely be greater than what they
+ actually have. (For example, a User might have sandboxed query perms for Table 15, which is why we're applying a
+ sandbox in the first place; the actual perms required to normally run the underlying sandbox query is more likely
+ something like *full* query perms for Table 15.) The QP perms check middleware subtracts this set from the set of
+ required permissions, allowing the user to run their sandboxed query."
+ [{card-id :card_id :as sandbox}]
(if card-id
(qp.store/cached card-id
(query-perms/perms-set (t2/select-one-fn :dataset_query Card :id card-id), :throw-exceptions? true))
- #{(perms/table-query-path (t2/select-one Table :id table-id))}))
+ (set (map perms/table-query-path (sandbox->table-ids sandbox)))))
-(defn- gtaps->perms-set [gtaps]
- (set (mapcat gtap->perms-set gtaps)))
+(defn- sandboxes->perms-set [sandboxes]
+ (set (mapcat sandbox->perms-set sandboxes)))
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -319,7 +332,7 @@
original-query
(-> sandboxed-query
(assoc ::original-metadata (expected-cols original-query))
- (update-in [::qp.perms/perms :gtaps] (fn [perms] (into (set perms) (gtaps->perms-set (vals table-id->gtap)))))))))
+ (update-in [::qp.perms/perms :gtaps] (fn [perms] (into (set perms) (sandboxes->perms-set (vals table-id->gtap)))))))))
(def ^:private default-recursion-limit 20)
(def ^:private ^:dynamic *recursion-limit* default-recursion-limit)
diff --git a/enterprise/backend/test/metabase_enterprise/sandbox/api/field_test.clj b/enterprise/backend/test/metabase_enterprise/sandbox/api/field_test.clj
index 409ee4f0900..3f71d432cee 100644
--- a/enterprise/backend/test/metabase_enterprise/sandbox/api/field_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/sandbox/api/field_test.clj
@@ -47,7 +47,7 @@
(letfn [(fetch-values [user field]
(-> (mt/user-http-request user :get 200 (format "field/%d/values" (mt/id :venues field)))
(update :values (partial take 3))))]
- ;; Rasta Toucan is only allowed to see Venues that are in the "Mexican" category [category_id = 50]. n
+ ;; Rasta Toucan is only allowed to see Venues that are in the "Mexican" category [category_id = 50]. When
;; fetching FieldValues for `venue.name` should do an ad-hoc fetch and only return the names of venues in
;; that category.
(is (= {:field_id (mt/id :venues :name)
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 666f528343f..22f4bc00a33 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
@@ -718,8 +718,6 @@
(is (= [[9]]
(mt/rows result)))))))))))
-
-
(deftest remapped-fks-test
(testing "Sandboxing should work with remapped FK columns (#14629)"
(mt/dataset sample-dataset
@@ -768,6 +766,19 @@
"Rustic Paper Wallet"] ; <- Includes the remapped column
(first (mt/rows result))))))))))))
+(deftest sandboxing-linked-table-perms
+ (testing "Sandboxing based on a column in a linked table should work even if the user doesn't have self-service query
+ permissions for the linked table (#15105)"
+ (mt/dataset sample-dataset
+ (met/with-gtaps (mt/$ids orders
+ {:gtaps {:orders {:remappings {"user_id" [:dimension $user_id->people.id]}}}
+ :attributes {"user_id" 1}})
+ (mt/with-test-user :rasta
+ (is (= [11]
+ (-> (mt/run-mbql-query orders {:aggregation [[:count]]})
+ mt/rows
+ first))))))))
+
(deftest drill-thru-on-joins-test
(testing "should work on questions with joins, with sandboxed target table, where target fields cannot be filtered (#13642)"
;; Sandbox ORDERS and PRODUCTS
diff --git a/enterprise/backend/test/metabase_enterprise/sandbox/test_util.clj b/enterprise/backend/test/metabase_enterprise/sandbox/test_util.clj
index 7405a8970ec..df093769da2 100644
--- a/enterprise/backend/test/metabase_enterprise/sandbox/test_util.clj
+++ b/enterprise/backend/test/metabase_enterprise/sandbox/test_util.clj
@@ -95,8 +95,8 @@
`(do-with-gtaps-for-user (fn [] ~gtaps-and-attributes-map) ~test-user-name-or-user-id (fn [~'&group] ~@body)))
(defmacro with-gtaps
- "Execute `body` with `gtaps` and optionally user `attributes` in effect. All underlying objects and permissions are
- created automatically.
+ "Execute `body` with `gtaps` and optionally user `attributes` in effect, for the :rasta test user. All underlying
+ objects and permissions are created automatically.
`gtaps-and-attributes-map` is a map containing `:gtaps` and optionally `:attributes`; see the `WithGTAPsArgs` schema
in this namespace.
--
GitLab