From d1798f10514814214809e0cc4a5e6461cbdd1290 Mon Sep 17 00:00:00 2001
From: Noah Moss <32746338+noahmoss@users.noreply.github.com>
Date: Fri, 11 Oct 2024 16:52:39 -0400
Subject: [PATCH] Use saved card metadata to filter fields in query metadata
for sandboxed tables (#48577)
---
.../metabase_enterprise/sandbox/api/table.clj | 75 ++++++++++---------
.../sandbox/api/table_test.clj | 34 ++++++++-
src/metabase/models/card/metadata.clj | 4 +-
3 files changed, 75 insertions(+), 38 deletions(-)
diff --git a/enterprise/backend/src/metabase_enterprise/sandbox/api/table.clj b/enterprise/backend/src/metabase_enterprise/sandbox/api/table.clj
index f0c8336784c..b9a9c8a1cf2 100644
--- a/enterprise/backend/src/metabase_enterprise/sandbox/api/table.clj
+++ b/enterprise/backend/src/metabase_enterprise/sandbox/api/table.clj
@@ -4,8 +4,6 @@
[metabase-enterprise.sandbox.api.util :as sandbox.api.util]
[metabase.api.common :as api]
[metabase.api.table :as api.table]
- [metabase.lib.util.match :as lib.util.match]
- [metabase.models.card :refer [Card]]
[metabase.models.data-permissions :as data-perms]
[metabase.models.table :as table :refer [Table]]
[metabase.public-settings.premium-features :refer [defenterprise]]
@@ -14,12 +12,12 @@
[metabase.util.malli.schema :as ms]
[toucan2.core :as t2]))
-(mu/defn- find-gtap-question :- [:maybe (ms/InstanceOf Card)]
- "Find the associated GTAP question (if there is one) for the given `table-or-table-id` and
- `user-or-user-id`. Returns nil if no question was found."
+(mu/defn- find-sandbox-source-card :- [:maybe (ms/InstanceOf :model/Card)]
+ "Find the associated sandboxing card (if there is one) for the given `table-or-table-id` and `user-or-user-id`.
+ Returns nil if no question was found."
[table-or-table-id user-or-user-id]
- (t2/select-one Card
- {:select [:c.id :c.dataset_query]
+ (t2/select-one :model/Card
+ {:select [:c.id :c.dataset_query :c.result_metadata]
:from [[:sandboxes]]
:join [[:permissions_group_membership :pgm] [:= :sandboxes.group_id :pgm.group_id]
[:report_card :c] [:= :c.id :sandboxes.card_id]]
@@ -33,16 +31,26 @@
[table :- (ms/InstanceOf Table)]
(boolean (seq (sandbox.api.util/enforced-sandboxes-for-tables #{(:id table)}))))
-(mu/defn- query->fields-ids :- [:maybe [:sequential :int]]
- [{{{:keys [fields]} :query} :dataset_query} :- [:maybe :map]]
- (lib.util.match/match fields [:field (id :guard integer?) _] id))
+(defn- filter-fields-by-name [names fields]
+ (filter #(contains? names (:name %))
+ fields))
-(defn- maybe-filter-fields [table query-metadata-response]
- ;; If we have sandboxed permissions and the associated GTAP limits the fields returned, we need make sure the
- ;; query_metadata endpoint also excludes any fields the GTAP query would exclude
- (if-let [gtap-field-ids (and (only-sandboxed-perms? table)
- (seq (query->fields-ids (find-gtap-question table api/*current-user-id*))))]
- (update query-metadata-response :fields #(filter (comp (set gtap-field-ids) u/the-id) %))
+(defn- filter-fields-by-id [ids fields]
+ (filter #(contains? ids (:id %))
+ fields))
+
+(defn- filter-fields-for-sandboxing [table query-metadata-response]
+ ;; If we have sandboxed permissions and the associated sandbox limits the fields returned, we need to make sure
+ ;; the query_metadata endpoint also excludes any fields the sandbox query would exclude.
+ (if-let [sandbox-source-card (find-sandbox-source-card table api/*current-user-id*)]
+ (case (get-in sandbox-source-card [:dataset_query :type])
+ :native (update query-metadata-response :fields
+ (partial filter-fields-by-name
+ (->> sandbox-source-card :result_metadata (map :name) set)))
+ :query (update query-metadata-response :fields
+ (partial filter-fields-by-id
+ (->> sandbox-source-card :result_metadata (map u/id) set))))
+ ;; Sandboxed via user attribute, not a source question, so no column-level sandboxing is in place
query-metadata-response))
(defenterprise fetch-table-query-metadata
@@ -50,18 +58,17 @@
`include-hidden-fields?` and `include-editable-data-model?` can be either booleans or boolean strings."
:feature :sandboxes
[id opts]
- (let [table (api/check-404 (t2/select-one Table :id id))
- sandboxed-perms? (only-sandboxed-perms? table)
- thunk (fn []
- (api.table/fetch-query-metadata* table opts))]
- ;; if the user has sandboxed perms, temporarily upgrade their perms to read perms for the Table so they can see the
- ;; metadata
- (if sandboxed-perms?
- (maybe-filter-fields
+ (let [table (api/check-404 (t2/select-one Table :id id))
+ thunk (fn [] (api.table/fetch-query-metadata* table opts))]
+ (if (only-sandboxed-perms? table)
+ (filter-fields-for-sandboxing
table
+ ;; if the user has sandboxed perms, temporarily upgrade their perms to read perms for the Table so they can
+ ;; fetch the metadata
(data-perms/with-additional-table-permission :perms/view-data (:db_id table) (u/the-id table) :unrestricted
(data-perms/with-additional-table-permission :perms/create-queries (:db_id table) (u/the-id table) :query-builder
(thunk))))
+ ;; Not sandboxed, so user can fetch full metadata
(thunk))))
(defenterprise batch-fetch-table-query-metadatas
@@ -69,16 +76,16 @@
:feature :sandboxes
[ids]
(for [table (api.table/batch-fetch-query-metadatas* ids)]
- (let [sandboxed-perms? (only-sandboxed-perms? table)]
- ;; if the user has sandboxed perms, temporarily upgrade their perms to read perms for the Table so they can see
- ;; the metadata
- (if sandboxed-perms?
- (maybe-filter-fields
- table
- (data-perms/with-additional-table-permission :perms/view-data (:db_id table) (u/the-id table) :unrestricted
- (data-perms/with-additional-table-permission :perms/create-queries (:db_id table) (u/the-id table) :query-builder
- table)))
- table))))
+ (if (only-sandboxed-perms? table)
+ (filter-fields-for-sandboxing
+ table
+ ;; if the user has sandboxed perms, temporarily upgrade their perms to read perms for the Table so they can
+ ;; fetch the metadata
+ (data-perms/with-additional-table-permission :perms/view-data (:db_id table) (u/the-id table) :unrestricted
+ (data-perms/with-additional-table-permission :perms/create-queries (:db_id table) (u/the-id table) :query-builder
+ table)))
+ ;; Not sandboxed, so user can fetch full metadata
+ table)))
(api/defendpoint GET "/:id/query_metadata"
"This endpoint essentially acts as a wrapper for the OSS version of this route. When a user has sandboxed permissions
diff --git a/enterprise/backend/test/metabase_enterprise/sandbox/api/table_test.clj b/enterprise/backend/test/metabase_enterprise/sandbox/api/table_test.clj
index 4310e324cef..b883e56a9cc 100644
--- a/enterprise/backend/test/metabase_enterprise/sandbox/api/table_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/sandbox/api/table_test.clj
@@ -4,8 +4,10 @@
[metabase-enterprise.sandbox.api.table :as table]
[metabase-enterprise.sandbox.test-util :as mt.tu]
[metabase-enterprise.test :as met]
+ [metabase.models.card.metadata :as card.metadata]
[metabase.test :as mt]
- [metabase.util :as u]))
+ [metabase.util :as u]
+ [toucan2.core :as t2]))
(def ^:private all-columns
#{"CATEGORY_ID" "ID" "LATITUDE" "LONGITUDE" "NAME" "PRICE"})
@@ -24,7 +26,35 @@
{:remappings {:cat [:variable [:field (mt/id :venues :category_id) nil]]}
:query (mt.tu/restricted-column-query (mt/id))}}
:attributes {:cat 50}}
- (testing "Users with restricted access to the columns of a table should only see columns included in the GTAP question"
+ (testing "Users with restricted access to the columns of a table via an MBQL sandbox should only see columns
+ included in the sandboxing question"
+ (is (= #{"CATEGORY_ID" "ID" "NAME"}
+ (field-names :rasta))))
+
+ (testing "Users with full permissions should not be affected by this field filtering"
+ (is (= all-columns
+ (field-names :crowberto)))))))
+
+(deftest native-query-metadata-test
+ (testing "GET /api/table/:id/query_metadata"
+ (met/with-gtaps! {:gtaps {:venues
+ {:query (mt/native-query {:query "SELECT CATEGORY_ID, ID, NAME from venues;"})
+ :remappings {:cat [:variable [:field (mt/id :venues :category_id) nil]]}}}
+ :attributes {:cat 50}}
+ ;; Fetch the card and manually compute & save the metadata
+ (let [card (t2/select-one :model/Card
+ {:select [:c.id :c.dataset_query]
+ :from [[:sandboxes :s]]
+ :join [[:permissions_group :pg] [:= :s.group_id :pg.id]
+ [:report_card :c] [:= :c.id :s.card_id]]
+ :where [:= :pg.id (u/the-id &group)]})
+ {:keys [metadata metadata-future]} (@#'card.metadata/maybe-async-recomputed-metadata (:dataset_query card))]
+ (if metadata
+ (t2/update! :model/Card :id (u/the-id card) {:result_metadata metadata})
+ (card.metadata/save-metadata-async! metadata-future card)))
+
+ (testing "Users with restricted access to the columns of a table via a native query sandbox should only see
+ columns included in the sandboxing question"
(is (= #{"CATEGORY_ID" "ID" "NAME"}
(field-names :rasta))))
diff --git a/src/metabase/models/card/metadata.clj b/src/metabase/models/card/metadata.clj
index 90705e3504c..23c7be77a71 100644
--- a/src/metabase/models/card/metadata.clj
+++ b/src/metabase/models/card/metadata.clj
@@ -72,8 +72,8 @@ saved later when it is ready."
{:metadata result})))
(mu/defn maybe-async-result-metadata :- ::maybe-async-result-metadata
- "Return return result metadata for the passed in `query`. If metadata needs to be recalculated, waits up
- to [[metadata-sync-wait-ms]] for it to be recalcuated; if not recalculated by then, returns a map with
+ "Return result metadata for the passed in `query`. If metadata needs to be recalculated, waits up to
+ [[metadata-sync-wait-ms]] for it to be recalcuated; if not recalculated by then, returns a map with
`:metadata-future`. Otherwise returns a map with `:metadata`.
Takes the `original-query` so it can determine if existing `metadata` might still be valid. Takes `dataset?` since
--
GitLab