Don't sandbox embedded queries (#30628)

* initial fix

* unskip repro

* use as-admin in more places

* macro tweak and unit tests

* tweak macro again and add a sandboxing test

* fix test

e2e/test/scenarios/embedding/reproductions/30535-session-sandboxing.cy.spec.js

@@ -13,8 +13,6 @@ const questionDetails = {
 
 describeEE("issue 30535", () => {
   beforeEach(() => {
-    // TODO Remove the following line once the underlying issue is resolved!
-    cy.skipOn(true);
     restore();
     cy.signInAsAdmin();
 

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

@@ -341,20 +341,22 @@
   "Pre-processing middleware. Replaces source tables a User was querying against with source queries that (presumably)
   restrict the rows returned, based on presence of sandboxes."
   [query]
-  (or (when-let [table-id->gtap (when *current-user-id*
-                                  (query->table-id->gtap query))]
-        (let [gtapped-query (gtapped-query query table-id->gtap)]
-          (if (not= query gtapped-query)
-            ;; Applying GTAPs to the query may have introduced references to tables that are also sandboxed,
-            ;; so we need to recursively appby the middleware until new queries are not returned.
-            (if (= *recursion-limit* 0)
-              (throw (ex-info (trs "Reached recursion limit of {0} in \"apply-sandboxing\" middleware"
-                                   default-recursion-limit)
-                              query))
-              (binding [*recursion-limit* (dec *recursion-limit*)]
-                (apply-sandboxing gtapped-query)))
-            gtapped-query)))
-      query))
+  (if-not api/*is-superuser?*
+    (or (when-let [table-id->gtap (when *current-user-id*
+                                    (query->table-id->gtap query))]
+          (let [gtapped-query (gtapped-query query table-id->gtap)]
+            (if (not= query gtapped-query)
+              ;; Applying GTAPs to the query may have introduced references to tables that are also sandboxed,
+              ;; so we need to recursively appby the middleware until new queries are not returned.
+              (if (= *recursion-limit* 0)
+                (throw (ex-info (trs "Reached recursion limit of {0} in \"apply-sandboxing\" middleware"
+                                     default-recursion-limit)
+                                query))
+                (binding [*recursion-limit* (dec *recursion-limit*)]
+                  (apply-sandboxing gtapped-query)))
+              gtapped-query)))
+        query)
+    query))
 
 
 ;;;; Post-processing

enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj

@@ -23,6 +23,7 @@
    [metabase.query-processor.pivot :as qp.pivot]
    [metabase.query-processor.util :as qp.util]
    [metabase.query-processor.util.add-alias-info :as add]
+   [metabase.server.middleware.session :as mw.session]
    [metabase.test :as mt]
    [metabase.test.data.env :as tx.env]
    [metabase.util :as u]
@@ -340,6 +341,14 @@
         (is (= [[100]]
                (run-venues-count-query)))))
 
+    (testing "A non-admin impersonating an admin (i.e. when running a public or embedded question) should always bypass sandboxes (#30535)"
+      (met/with-gtaps-for-user :rasta {:gtaps      {:venues (venues-category-mbql-gtap-def)}
+                                       :attributes {"cat" 50}}
+        (mt/with-test-user :rasta
+         (mw.session/as-admin
+          (is (= [[100]]
+               (run-venues-count-query)))))))
+
     (testing "Users with view access to the related collection should bypass segmented permissions"
       (mt/with-temp-copy-of-db
         (mt/with-temp* [Collection [collection]

src/metabase/api/public.clj

@@ -31,6 +31,7 @@
    [metabase.query-processor.middleware.constraints :as qp.constraints]
    [metabase.query-processor.pivot :as qp.pivot]
    [metabase.query-processor.streaming :as qp.streaming]
+   [metabase.server.middleware.session :as mw.session]
    [metabase.util :as u]
    [metabase.util.embed :as embed]
    [metabase.util.i18n :refer [tru]]
@@ -137,13 +138,13 @@
   [qp-runner export-format]
   (fn [query info]
     (qp.streaming/streaming-response
-        [{:keys [reducedf], :as context} export-format (u/slugify (:card-name info))]
-        (let [context  (assoc context :reducedf (public-reducedf reducedf))
-              in-chan  (binding [api/*current-user-permissions-set* (atom #{"/"})]
-                         (qp-runner query info context))
-              out-chan (a/promise-chan (map transform-results))]
-          (async.u/promise-pipe in-chan out-chan)
-          out-chan))))
+     [{:keys [reducedf], :as context} export-format (u/slugify (:card-name info))]
+     (let [context  (assoc context :reducedf (public-reducedf reducedf))
+           in-chan  (mw.session/as-admin
+                     (qp-runner query info context))
+           out-chan (a/promise-chan (map transform-results))]
+       (async.u/promise-pipe in-chan out-chan)
+       out-chan))))
 
 (defn run-query-for-card-with-id-async
   "Run the query belonging to Card with `card-id` with `parameters` and other query options (e.g. `:constraints`).
@@ -157,12 +158,12 @@
   ;; we actually need to bind the current user perms here twice, once so `card-api` will have the full perms when it
   ;; tries to do the `read-check`, and a second time for when the query is ran (async) so the QP middleware will have
   ;; the correct perms
-  (binding [api/*current-user-permissions-set* (atom #{"/"})]
-    (m/mapply qp.card/run-query-for-card-async card-id export-format
-              :parameters parameters
-              :context    :public-question
-              :run        (run-query-for-card-with-id-async-run-fn qp-runner export-format)
-              options)))
+  (mw.session/as-admin
+   (m/mapply qp.card/run-query-for-card-async card-id export-format
+             :parameters parameters
+             :context    :public-question
+             :run        (run-query-for-card-with-id-async-run-fn qp-runner export-format)
+             options)))
 
 (s/defn ^:private run-query-for-card-with-public-uuid-async
   "Run query for a *public* Card with UUID. If public sharing is not enabled, this throws an exception. Returns a
@@ -253,8 +254,8 @@
     ;; Run this query with full superuser perms. We don't want the various perms checks failing because there are no
     ;; current user perms; if this Dashcard is public you're by definition allowed to run it without a perms check
     ;; anyway
-    (binding [api/*current-user-permissions-set* (atom #{"/"})]
-      (m/mapply qp.dashboard/run-query-for-dashcard-async options))))
+    (mw.session/as-admin
+     (m/mapply qp.dashboard/run-query-for-dashcard-async options))))
 
 #_{:clj-kondo/ignore [:deprecated-var]}
 (api/defendpoint-schema GET "/dashboard/:uuid/dashcard/:dashcard-id/card/:card-id"
@@ -525,8 +526,8 @@
   [uuid param-key]
   (validation/check-public-sharing-enabled)
   (let [card (t2/select-one Card :public_uuid uuid, :archived false)]
-    (binding [api/*current-user-permissions-set* (atom #{"/"})]
-      (api.card/param-values card param-key))))
+    (mw.session/as-admin
+     (api.card/param-values card param-key))))
 
 #_{:clj-kondo/ignore [:deprecated-var]}
 (api/defendpoint-schema GET "/card/:uuid/params/:param-key/search/:query"
@@ -534,24 +535,24 @@
   [uuid param-key query]
   (validation/check-public-sharing-enabled)
   (let [card (t2/select-one Card :public_uuid uuid, :archived false)]
-    (binding [api/*current-user-permissions-set* (atom #{"/"})]
-      (api.card/param-values card param-key query))))
+    (mw.session/as-admin
+     (api.card/param-values card param-key query))))
 
 #_{:clj-kondo/ignore [:deprecated-var]}
 (api/defendpoint-schema GET "/dashboard/:uuid/params/:param-key/values"
   "Fetch filter values for dashboard parameter `param-key`."
   [uuid param-key :as {:keys [query-params]}]
   (let [dashboard (dashboard-with-uuid uuid)]
-    (binding [api/*current-user-permissions-set* (atom #{"/"})]
-      (api.dashboard/param-values dashboard param-key query-params))))
+    (mw.session/as-admin
+     (api.dashboard/param-values dashboard param-key query-params))))
 
 #_{:clj-kondo/ignore [:deprecated-var]}
 (api/defendpoint-schema GET "/dashboard/:uuid/params/:param-key/search/:query"
   "Fetch filter values for dashboard parameter `param-key`, containing specified `query`."
   [uuid param-key query :as {:keys [query-params]}]
   (let [dashboard (dashboard-with-uuid uuid)]
-    (binding [api/*current-user-permissions-set* (atom #{"/"})]
-      (api.dashboard/param-values dashboard param-key query-params query))))
+    (mw.session/as-admin
+     (api.dashboard/param-values dashboard param-key query-params query))))
 
 ;;; ----------------------------------------------------- Pivot Tables -----------------------------------------------
 

src/metabase/server/middleware/session.clj

@@ -303,13 +303,13 @@
 
 (defn do-with-current-user
   "Impl for `with-current-user`."
-  [{:keys [metabase-user-id is-superuser? user-locale settings is-group-manager?]} thunk]
+  [{:keys [metabase-user-id is-superuser? permissions-set user-locale settings is-group-manager?]} thunk]
   (binding [*current-user-id*              metabase-user-id
             i18n/*user-locale*             user-locale
             *is-group-manager?*            (boolean is-group-manager?)
             *is-superuser?*                (boolean is-superuser?)
             *current-user*                 (delay (find-user metabase-user-id))
-            *current-user-permissions-set* (delay (some-> metabase-user-id user/permissions-set))
+            *current-user-permissions-set* (delay (or permissions-set (some-> metabase-user-id user/permissions-set)))
             *user-local-values*            (delay (atom (or settings
                                                             (user-local-settings metabase-user-id))))]
     (thunk)))
@@ -328,7 +328,7 @@
                                         Overrides `site-locale` if set.
   *  `*is-superuser?*`                  Boolean stating whether current user is a superuser.
   *  `*is-group-manager?*`              Boolean stating whether current user is a group manager of at least one group.
-  *  `current-user-permissions-set*`    delay that returns the set of permissions granted to the current user from DB
+  *  `*current-user-permissions-set*`   delay that returns the set of permissions granted to the current user from DB
   *  `*user-local-values*`              atom containing a map of user-local settings and values for the current user"
   [handler]
   (fn [request respond raise]
@@ -342,8 +342,19 @@
     (t2/select-one [User [:id :metabase-user-id] [:is_superuser :is-superuser?] [:locale :user-locale] :settings]
       :id current-user-id)))
 
+(defmacro as-admin
+  "Execude code in body as an admin user."
+  {:style/indent :defn}
+  [& body]
+  `(do-with-current-user
+    (merge
+      (with-current-user-fetch-user-for-id ~'api/*current-user-id*)
+      {:is-superuser? true
+       :permissions-set #{"/"}})
+    (fn [] ~@body)))
+
 (defmacro with-current-user
-  "Execute code in body with User with `current-user-id` bound as the current user. (This is not used in the middleware
+  "Execute code in body with `current-user-id` bound as the current user. (This is not used in the middleware
   itself but elsewhere where we want to simulate a User context, such as when rendering Pulses or in tests.) "
   {:style/indent :defn}
   [current-user-id & body]

test/metabase/server/middleware/session_test.clj

@@ -5,12 +5,15 @@
    [clojure.test :refer :all]
    [environ.core :as env]
    [java-time :as t]
-   [metabase.api.common :refer [*current-user* *current-user-id*]]
+   [metabase.api.common :as api :refer [*current-user* *current-user-id*]]
    [metabase.config :as config]
    [metabase.core.initialization-status :as init-status]
    [metabase.db :as mdb]
    [metabase.driver.sql.query-processor :as sql.qp]
    [metabase.models :refer [PermissionsGroupMembership Session User]]
+   [metabase.models.setting :as setting]
+   [metabase.models.setting-test :as setting-test]
+   [metabase.models.user :as user]
    [metabase.public-settings :as public-settings]
    [metabase.public-settings.premium-features :as premium-features]
    [metabase.public-settings.premium-features-test
@@ -358,6 +361,33 @@
             (request-with-user-id 0))))))
 
 
+;;; ----------------------------------------------   with-current-user -------------------------------------------------
+
+(deftest with-current-user-test
+  (testing "with-current-user correctly binds the appropriate vars for the provided user ID"
+    (mw.session/with-current-user (mt/user->id :rasta)
+      ;; Set a user-local value for rasta so that we can make sure that the user-local settings map is correctly bound
+      (setting-test/test-user-local-only-setting! "XYZ")
+
+      (is (= (mt/user->id :rasta) *current-user-id*))
+      (is (= "rasta@metabase.com" (:email @*current-user*)))
+      (is (false? api/*is-superuser?*))
+      (is (= nil i18n/*user-locale*))
+      (is (false? api/*is-group-manager?*))
+      (is (= (user/permissions-set (mt/user->id :rasta)) @api/*current-user-permissions-set*))
+      (is (partial= {:test-user-local-only-setting "XYZ"} @@setting/*user-local-values*)))))
+
+(deftest as-admin-test
+  (testing "as-admin overrides *is-superuser?* and *current-user-permissions-set*"
+    (mw.session/with-current-user (mt/user->id :rasta)
+      (mw.session/as-admin
+       ;; Current user ID remains the same
+       (is (= (mt/user->id :rasta) *current-user-id*))
+       ;; *is-superuser?* and permissions set are overrided
+       (is (true? api/*is-superuser?*))
+       (is (= #{"/"} @api/*current-user-permissions-set*))))))
+
+
 ;;; ----------------------------------------------------- Locale -----------------------------------------------------
 
 (deftest bind-locale-test