Use consistent approach for disabling permission checks in QP to fix...

Use consistent approach for disabling permission checks in QP to fix sandboxing error in downloads (#47481)

enterprise/backend/src/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions.clj

@@ -24,6 +24,7 @@
    ^{:clj-kondo/ignore [:deprecated-namespace]}
    [metabase.query-processor.middleware.fetch-source-query-legacy :as fetch-source-query-legacy]
    [metabase.query-processor.store :as qp.store]
+   [metabase.server.middleware.session :as mw.session]
    [metabase.util :as u]
    [metabase.util.i18n :refer [trs tru]]
    [metabase.util.log :as log]
@@ -123,7 +124,7 @@
     (let [query        {:database (u/the-id (lib.metadata/database (qp.store/metadata-provider)))
                         :type     :query
                         :query    source-query}
-          preprocessed (binding [*current-user-id* nil]
+          preprocessed (mw.session/as-admin
                          ((requiring-resolve 'metabase.query-processor.preprocess/preprocess) query))]
       (select-keys (:query preprocessed) [:source-query :source-metadata]))
     (catch Throwable e
@@ -143,7 +144,7 @@
 
 (mu/defn- mbql-query-metadata :- [:+ :map]
   [inner-query]
-  (binding [*current-user-id* nil]
+  (mw.session/as-admin
     ((requiring-resolve 'metabase.query-processor.preprocess/query->expected-cols)
      {:database (u/the-id (lib.metadata/database (qp.store/metadata-provider)))
       :type     :query
@@ -172,7 +173,7 @@
 
 (mu/defn- native-query-metadata :- [:+ :map]
   [source-query :- [:map [:source-query :any]]]
-  (let [result (binding [*current-user-id* nil]
+  (let [result (mw.session/as-admin
                  ((requiring-resolve 'metabase.query-processor/process-query)
                   {:database (u/the-id (lib.metadata/database (qp.store/metadata-provider)))
                    :type     :query
@@ -347,7 +348,7 @@
         (assoc &match ::gtap? true)))))
 
 (defn- expected-cols [query]
-  (binding [*current-user-id* nil]
+  (mw.session/as-admin
     ((requiring-resolve 'metabase.query-processor.preprocess/query->expected-cols) query)))
 
 (defn- gtapped-query

enterprise/backend/test/metabase_enterprise/sandbox/api/dataset_test.clj

@@ -6,7 +6,7 @@
    [metabase.test :as mt]))
 
 (deftest dataset-parameter-test
-  (testing "POST /api/dataset/parameter/values should follow sandbox rules"
+  (testing "POST /api/dataset/parameter/values should respect sandboxing"
     (met/with-gtaps! {:gtaps {:categories {:query (mt/mbql-query categories {:filter [:<= $id 3]})}}}
       (testing "with values_source_type=card"
         (mt/with-temp

enterprise/backend/test/metabase_enterprise/sandbox/api/download_test.clj

+(ns metabase-enterprise.sandbox.api.download-test
+  (:require
+   [clojure.data.csv :as csv]
+   [clojure.test :refer :all]
+   [metabase-enterprise.test :as met]
+   [metabase.models.data-permissions :as data-perms]
+   [metabase.test :as mt]
+   [metabase.util :as u]))
+
+(deftest card-download-test
+  (testing "POST /api/card/:id/query/csv should respect sandboxing"
+    (mt/with-model-cleanup [:model/Card]
+      (met/with-gtaps! (mt/$ids people
+                         {:gtaps      {:people {:remappings {"state" [:dimension $people.state]}}}
+                          :attributes {"state" "CA"}})
+        (mt/with-temp [:model/Card card {:dataset_query (mt/mbql-query people)}]
+          (data-perms/set-table-permission! &group (mt/id :people) :perms/download-results :one-million-rows)
+
+          ;; Sanity check: admin can download full table
+          (let [res (-> (mt/user-http-request :crowberto :post 200 (format "card/%d/query/csv" (u/the-id card)))
+                        csv/read-csv)]
+            (is (= 2501 (count res))))
+
+          ;; Sandboxed user only downloads a subset (users in CA)
+          (let [res (-> (mt/user-http-request :rasta :post 200 (format "card/%d/query/csv" (u/the-id card)))
+                        csv/read-csv)]
+            (is (= 91 (count res)))))))))

enterprise/backend/test/metabase_enterprise/sandbox/pulse_test.clj

@@ -186,7 +186,7 @@
                            (csv->row-count attachment)))))))))))))
 
 (deftest csv-downloads-test
-  (testing "CSV/XLSX downloads should be sandboxed"
+  (testing "CSV/XLSX downloads attached to an email should be sandboxed"
     (met/with-gtaps! {:gtaps      {:venues {:remappings {:price [:dimension [:field (mt/id :venues :price) nil]]}}}
                       :attributes {"price" "1"}}
       (let [query (mt/mbql-query venues)]
@@ -272,6 +272,6 @@
           (mt/user-http-request :rasta :put 200 (format "pulse/%d" pulse-id)
                                 {:channels [(assoc pc :recipients [{:id (mt/user->id :rasta)}])]})
 
-;; Crowberto should now be removed as a recipient
+          ;; Crowberto should now be removed as a recipient
           (is (= [(mt/user->id :rasta)]
                  (->> (api.alert/email-channel (pulse/retrieve-alert pulse-id)) :recipients (map :id) sort))))))))

src/metabase/models/query/permissions.clj

@@ -18,6 +18,7 @@
    [metabase.query-processor.error-type :as qp.error-type]
    [metabase.query-processor.store :as qp.store]
    [metabase.query-processor.util :as qp.util]
+   [metabase.server.middleware.session :as mw.session]
    [metabase.util :as u]
    [metabase.util.i18n :refer [tru]]
    [metabase.util.log :as log]
@@ -100,7 +101,7 @@
   ;; ignore the current user for the purposes of calculating the permissions required to run the query. Don't want the
   ;; preprocessing to fail because current user doesn't have permissions to run it when we're not trying to run it at
   ;; all
-  (binding [api/*current-user-id* nil]
+  (mw.session/as-admin
     ((requiring-resolve 'metabase.query-processor.preprocess/preprocess) query)))
 
 (defn- referenced-card-ids

src/metabase/query_processor/middleware/add_source_metadata.clj

 (ns metabase.query-processor.middleware.add-source-metadata
   (:require
    [clojure.walk :as walk]
-   [metabase.api.common :as api]
    [metabase.legacy-mbql.schema :as mbql.s]
    [metabase.lib.metadata :as lib.metadata]
    [metabase.lib.util.match :as lib.util.match]
    [metabase.query-processor.interface :as qp.i]
    [metabase.query-processor.store :as qp.store]
+   [metabase.server.middleware.session :as mw.session]
    [metabase.util.log :as log]
    [metabase.util.malli :as mu]))
 
@@ -47,7 +47,7 @@
   "Preprocess a `source-query` so we can determine the result columns."
   [source-query :- mbql.s/MBQLQuery]
   (try
-    (let [cols (binding [api/*current-user-id* nil]
+    (let [cols (mw.session/as-admin
                  ((requiring-resolve 'metabase.query-processor.preprocess/query->expected-cols)
                   {:database (:id (lib.metadata/database (qp.store/metadata-provider)))
                    :type     :query

src/metabase/query_processor/util/nest_query.clj

@@ -86,10 +86,11 @@
   (let [filter-clause (:filter inner-query)
         keep-filter? (nil? (lib.util.match/match-one filter-clause :expression))
         source (as-> (select-keys inner-query [:source-table :source-query :source-metadata :joins :expressions]) source
-                 ;; preprocess this without a current user context so it's not subject to permissions checks. To get
-                 ;; here in the first place we already had to do perms checks to make sure the query we're transforming
-                 ;; is itself ok, so we don't need to run another check
-                 (binding [api/*current-user-id* nil]
+                 ;; preprocess this in a superuser context so it's not subject to permissions checks. To get here in the
+                 ;; first place we already had to do perms checks to make sure the query we're transforming is itself
+                 ;; ok, so we don't need to run another check.
+                 ;; (Not using mw.session/as-admin due to cyclic dependency.)
+                 (binding [api/*is-superuser?* true]
                    ((requiring-resolve 'metabase.query-processor.preprocess/preprocess)
                     {:database (u/the-id (lib.metadata/database (qp.store/metadata-provider)))
                      :type     :query

test/metabase/api/card_test.clj

@@ -2639,7 +2639,7 @@
           (update-card card {:description "a new description"})
           (is (empty? (reviews card)))))
       (testing "Does not add nil moderation reviews when there are reviews but not verified"
-          ;; testing that we aren't just adding a nil moderation each time we update a card
+         ;; testing that we aren't just adding a nil moderation each time we update a card
         (with-card :verified
           (is (verified? card))
           (moderation-review/create-review! {:moderated_item_id   (u/the-id card)