Make sandboxing middleware recursive (#25012)

* minor comment fixes

* fix error

* unskip cypress

* remove debugging code

* retrigger ci

* throw an exception when a recursion limit is hit

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

@@ -22,7 +22,7 @@
             [metabase.query-processor.middleware.permissions :as qp.perms]
             [metabase.query-processor.store :as qp.store]
             [metabase.util :as u]
-            [metabase.util.i18n :refer [tru]]
+            [metabase.util.i18n :refer [trs tru]]
             [metabase.util.schema :as su]
             [schema.core :as s]
             [toucan.db :as db]))
@@ -324,13 +324,26 @@
           (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)))))))))
 
+(def ^:private default-recursion-limit 20)
+(def ^:private ^:dynamic *recursion-limit* default-recursion-limit)
+
 (defn apply-sandboxing
   "Pre-processing middleware. Replaces source tables a User was querying against with source queries that (presumably)
   restrict the rows returned, based on presence of segmented permission GTAPs."
   [query]
   (or (when-let [table-id->gtap (when *current-user-id*
                                   (query->table-id->gtap query))]
-        (gtapped-query query table-id->gtap))
+        (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))
 
 

frontend/test/metabase/scenarios/permissions/sandboxes.cy.spec.js

@@ -941,7 +941,7 @@ describeEE("formatting > sandboxes", () => {
       cy.contains("37.65");
     });
 
-    it.skip("unsaved/dirty query should work on linked table column with multiple dimensions and remapping (metabase#15106)", () => {
+    it("unsaved/dirty query should work on linked table column with multiple dimensions and remapping (metabase#15106)", () => {
       cy.server();
       cy.route("POST", "/api/dataset").as("dataset");
 

src/metabase/models/permissions.clj

@@ -75,7 +75,7 @@
       permissions but no matching GTAP exists.
 
     * Segmented permissions can also be used to enforce column-level permissions -- any column not returned by the
-      underlying GTAP query is not allowed to be references by the parent query thru other means such as filter clauses.
+      underlying GTAP query is not allowed to be referenced by the parent query thru other means such as filter clauses.
       See [[metabase-enterprise.sandbox.query-processor.middleware.column-level-perms-check]].
 
     * GTAPs are not allowed to add columns not present in the original Table, or change their effective type to

src/metabase/query_processor.clj

@@ -105,6 +105,7 @@
    #'resolve-joined-fields/resolve-joined-fields
    #'fix-bad-refs/fix-bad-references
    #'escape-join-aliases/escape-join-aliases
+   ;; yes, this is called a second time, because we need to handle any joins that got added
    (resolve 'ee.sandbox.rows/apply-sandboxing)
    #'qp.cumulative-aggregations/rewrite-cumulative-aggregations
    #'qp.pre-alias-aggregations/pre-alias-aggregations