From 3827a2b13c6aab900e65a8abe6a083ea5aa40e0a Mon Sep 17 00:00:00 2001 From: Jerry Huang <34140255+qwef@users.noreply.github.com> Date: Fri, 17 Nov 2023 13:29:11 -0600 Subject: [PATCH] Add properties to query_execution (#35508) --- .../middleware/row_level_restrictions.clj | 12 +++++++----- .../metabase_enterprise/sandbox/api/card_test.clj | 15 +++++++++++++-- .../middleware/row_level_restrictions_test.clj | 12 ++++++++++++ resources/migrations/001_update_migrations.yaml | 15 +++++++++++++++ .../middleware/process_userland_query.clj | 11 +++++++---- test/metabase/api/dataset_test.clj | 1 + .../middleware/process_userland_query_test.clj | 5 +++-- 7 files changed, 58 insertions(+), 13 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 5d5534cf4c94d..240aa428c03d0 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 @@ -377,8 +377,10 @@ (defenterprise merge-sandboxing-metadata "Post-processing middleware. Merges in column metadata from the original, unsandboxed version of the query." :feature :sandboxes - [{::keys [original-metadata]} rff] - (if original-metadata - (fn merge-sandboxing-metadata-rff* [metadata] - (rff (merge-metadata original-metadata metadata))) - rff)) + [{::keys [original-metadata] :as query} rff] + (fn merge-sandboxing-metadata-rff* [metadata] + (let [metadata (assoc metadata :is_sandboxed (some? (get-in query [::qp.perms/perms :gtaps]))) + metadata (if original-metadata + (merge-metadata original-metadata metadata) + metadata)] + (rff metadata)))) diff --git a/enterprise/backend/test/metabase_enterprise/sandbox/api/card_test.clj b/enterprise/backend/test/metabase_enterprise/sandbox/api/card_test.clj index 3f802a35e719f..f33f932718440 100644 --- a/enterprise/backend/test/metabase_enterprise/sandbox/api/card_test.clj +++ b/enterprise/backend/test/metabase_enterprise/sandbox/api/card_test.clj @@ -3,11 +3,14 @@ [clojure.test :refer :all] [metabase-enterprise.test :as met] [metabase.api.card-test :as api.card-test] - [metabase.models :refer [Card Collection Database PermissionsGroup PermissionsGroupMembership Table]] + [metabase.models :refer [Card Collection Database PermissionsGroup + PermissionsGroupMembership Table]] [metabase.models.permissions :as perms] [metabase.models.permissions-group :as perms-group] + [metabase.query-processor :as qp] [metabase.test :as mt] - [metabase.util :as u])) + [metabase.util :as u] + [toucan2.tools.with-temp :as t2.with-temp])) (deftest users-with-segmented-perms-test (testing "Users with segmented permissions should be able to save cards" @@ -83,3 +86,11 @@ (is (=? {:values [] :has_more_values false} (search :rasta))))))))) + +(deftest is-sandboxed-test + (testing "Adding a GTAP to the all users group to a table makes it such that is_sandboxed returns true." + (met/with-gtaps {:gtaps {:categories {:query (mt/mbql-query categories {:filter [:<= $id 3]})}}} + (t2.with-temp/with-temp [Card card {:database_id (mt/id) + :table_id (mt/id :categories) + :dataset_query (mt/mbql-query categories)}] + (is (get-in (qp/process-userland-query (:dataset_query card)) [:data :is_sandboxed])))))) 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 9ff175caa3fd4..961a9fe2877c9 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 @@ -20,6 +20,7 @@ [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.middleware.process-userland-query-test :as process-userland-query-test] [metabase.query-processor.pivot :as qp.pivot] [metabase.query-processor.store :as qp.store] [metabase.query-processor.util :as qp.util] @@ -1113,3 +1114,14 @@ (is (not (str/includes? (-> sandboxed-result :data :native_form :query) (:table_name persisted-info))) "Erroneously used the persisted model cache"))))))))))) + +(deftest is-sandboxed-success-test + (testing "Integration test that checks that is_sandboxed is recorded in query_execution correctly for a sandboxed query" + (met/with-gtaps {:gtaps {:categories {:query (mt/mbql-query categories {:filter [:<= $id 3]})}}} + (t2.with-temp/with-temp [Card card {:database_id (mt/id) + :table_id (mt/id :categories) + :dataset_query (mt/mbql-query categories)}] + (let [query (:dataset_query card)] + (process-userland-query-test/with-query-execution [qe query] + (qp/process-userland-query query) + (is (:is_sandboxed (qe))))))))) diff --git a/resources/migrations/001_update_migrations.yaml b/resources/migrations/001_update_migrations.yaml index b78a17342ad34..cf5384021ce72 100644 --- a/resources/migrations/001_update_migrations.yaml +++ b/resources/migrations/001_update_migrations.yaml @@ -3901,6 +3901,21 @@ databaseChangeLog: - sql: sql: drop view if exists v_view_log; + - changeSet: + id: v48.00-045 + author: qwef + comment: Added 0.48.0 - add is_sandboxed to query_execution + changes: + - addColumn: + tableName: query_execution + columns: + - column: + name: is_sandboxed + type: boolean + remarks: "Is query from a sandboxed user" + constraints: + nullable: true + - changeSet: id: v48.00-046 author: noahmoss diff --git a/src/metabase/query_processor/middleware/process_userland_query.clj b/src/metabase/query_processor/middleware/process_userland_query.clj index 482f39fe9341f..42fed7659908d 100644 --- a/src/metabase/query_processor/middleware/process_userland_query.clj +++ b/src/metabase/query_processor/middleware/process_userland_query.clj @@ -59,8 +59,11 @@ (catch Throwable e (log/error e (trs "Error saving query execution info")))))))) -(defn- save-successful-query-execution! [cached? query-execution result-rows] - (let [qe-map (assoc query-execution :cache_hit (boolean cached?) :result_rows result-rows)] +(defn- save-successful-query-execution! [cached? is_sandboxed? query-execution result-rows] + (let [qe-map (assoc query-execution + :cache_hit (boolean cached?) + :result_rows result-rows + :is_sandboxed (boolean is_sandboxed?))] (save-query-execution! qe-map))) (defn- save-failed-query-execution! [query-execution message] @@ -75,7 +78,7 @@ (merge (-> query-execution add-running-time - (dissoc :error :hash :executor_id :action_id :card_id :dashboard_id :pulse_id :result_rows :native)) + (dissoc :error :hash :executor_id :action_id :is_sandboxed :card_id :dashboard_id :pulse_id :result_rows :native)) result {:status :completed :average_execution_time (when cached? @@ -94,7 +97,7 @@ (events/publish-event! :event/card-query {:user-id (:executor_id execution-info) :card-id (:card_id execution-info) :context (:context execution-info)})) - (save-successful-query-execution! (:cached acc) execution-info @row-count) + (save-successful-query-execution! (:cached acc) (get-in acc [:data :is_sandboxed]) execution-info @row-count) (rf (if (map? acc) (success-response execution-info acc) acc))) diff --git a/test/metabase/api/dataset_test.clj b/test/metabase/api/dataset_test.clj index 78165dac00ffa..7a418eec527b5 100644 --- a/test/metabase/api/dataset_test.clj +++ b/test/metabase/api/dataset_test.clj @@ -103,6 +103,7 @@ :native false :pulse_id nil :card_id nil + :is_sandboxed false :dashboard_id nil :error nil :id true diff --git a/test/metabase/query_processor/middleware/process_userland_query_test.clj b/test/metabase/query_processor/middleware/process_userland_query_test.clj index 9ebddbdbcf0c1..c3b5b5ac9545d 100644 --- a/test/metabase/query_processor/middleware/process_userland_query_test.clj +++ b/test/metabase/query_processor/middleware/process_userland_query_test.clj @@ -15,7 +15,7 @@ (set! *warn-on-reflection* true) -(defn- do-with-query-execution [query run] +(defn do-with-query-execution [query run] (mt/with-clock #t "2020-02-04T12:22-08:00[US/Pacific]" (let [original-hash (qp.util/query-hash query) result (promise)] @@ -31,7 +31,7 @@ (:hash qe) (update :hash (fn [^bytes a-hash] (some-> a-hash codecs/bytes->hex))))))))))) -(defmacro ^:private with-query-execution {:style/indent 1} [[qe-result-binding query] & body] +(defmacro with-query-execution {:style/indent 1} [[qe-result-binding query] & body] `(do-with-query-execution ~query (fn [~qe-result-binding] ~@body))) (defn- process-userland-query @@ -71,6 +71,7 @@ :pulse_id nil :card_id nil :action_id nil + :is_sandboxed false :context nil :running_time true :cache_hit false