Only apply persistence substitution when there's no sandboxing (#22501)

* Only apply persistence substitution when there's no sandboxing

* Added test for sandboxed persistence

* Recursively apply persisted-info/native queries

* Move test into permissions test that has ee code

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

 (ns metabase-enterprise.sandbox.api.permissions-test
-  (:require [clojure.test :refer :all]
+  (:require [cheshire.core :as json]
+            [clojure.string :as str]
+            [clojure.test :refer :all]
             [metabase-enterprise.sandbox.models.group-table-access-policy :refer [GroupTableAccessPolicy]]
-            [metabase.models :refer [Database PermissionsGroup Table]]
+            [metabase.models :refer [Card Database PermissionsGroup
+                                     PersistedInfo Table]]
             [metabase.models.permissions-group :as perms-group]
+            [metabase.models.persisted-info :as persisted-info]
+            [metabase.query-processor :as qp]
             [metabase.test :as mt]
             [metabase.util :as u]
             [metabase.util.schema :as su]
@@ -128,3 +133,48 @@
             (mt/user-http-request :crowberto :put 200 "permissions/graph" graph')
             (testing "GTAP should not have been deleted"
               (is (db/exists? GroupTableAccessPolicy :group_id (u/the-id (perms-group/all-users)), :table_id (mt/id :venues))))))))))
+
+(defn- fake-persist-card! [card]
+  (let [persisted-info (persisted-info/make-ready! (mt/user->id :rasta) card)]
+    (db/update-where! PersistedInfo {:card_id (u/the-id card)}
+                      :definition (json/encode
+                                    (persisted-info/metadata->definition
+                                      (:result_metadata card)
+                                      (:table_name persisted-info)))
+                      :active true
+                      :state "persisted"
+                      :query_hash (persisted-info/query-hash (:dataset_query card)))))
+
+(deftest persistence-and-permissions
+  (mt/with-model-cleanup [PersistedInfo]
+    (testing "Queries from cache if not sandboxed"
+      (mt/with-current-user
+        (mt/user->id :rasta)
+        (mt/with-temp*
+          [Card [card {:dataset_query (mt/mbql-query venues)
+                       :dataset true
+                       :database_id (mt/id)}]]
+          (fake-persist-card! card)
+          (is (str/includes?
+                (:query (qp/compile
+
+                          {:database (mt/id)
+                           :query {:source-table (str "card__" (u/the-id card))}
+                           :type :query}))
+                "metabase_cache")))))
+    (testing "Queries from source if sandboxed"
+      (mt/with-gtaps
+        {:gtaps {:venues {:query (mt/mbql-query venues)
+                          :remappings {:cat ["variable" [:field (mt/id :venues :category_id) nil]]}}}
+         :attributes {"cat" 50}}
+        (mt/with-temp*
+          [Card [card {:dataset_query (mt/mbql-query venues)
+                       :dataset true
+                       :database_id (mt/id)}]]
+          (fake-persist-card! card)
+          (is (not (str/includes?
+                     (:query (qp/compile
+                               {:database (mt/id)
+                                :query {:source-table (str "card__" (u/the-id card))}
+                                :type :query}))
+                     "metabase_cache"))))))))

src/metabase/query_processor.clj

@@ -41,6 +41,7 @@
             [metabase.query-processor.middleware.optimize-temporal-filters :as optimize-temporal-filters]
             [metabase.query-processor.middleware.parameters :as parameters]
             [metabase.query-processor.middleware.permissions :as qp.perms]
+            [metabase.query-processor.middleware.persistence :as qp.persistence]
             [metabase.query-processor.middleware.pre-alias-aggregations :as qp.pre-alias-aggregations]
             [metabase.query-processor.middleware.prevent-infinite-recursive-preprocesses :as prevent-infinite-recursive-preprocesses]
             [metabase.query-processor.middleware.process-userland-query :as process-userland-query]
@@ -92,6 +93,7 @@
    #'qp.add-source-metadata/add-source-metadata-for-source-queries
    #'upgrade-field-literals/upgrade-field-literals
    (resolve 'ee.sandbox.rows/apply-sandboxing)
+   #'qp.persistence/substitute-persisted-query
    #'qp.add-implicit-clauses/add-implicit-clauses
    #'qp.add-dimension-projections/add-remapped-columns
    #'qp.resolve-fields/resolve-fields

src/metabase/query_processor/middleware/fetch_source_query.clj

@@ -47,7 +47,8 @@
    :database        mbql.s/DatabaseID
    :source-metadata [mbql.s/SourceQueryMetadata]
 
-   (s/optional-key :source-query/dataset?) s/Bool})
+   (s/optional-key :source-query/dataset?) s/Bool
+   (s/optional-key :persisted-info/native) s/Str})
 
 (def ^:private MapWithResolvedSourceQuery
   (s/constrained
@@ -121,8 +122,9 @@
                   (db/do-post-select Card)
                   (db/do-post-select PersistedInfo)
                   first)
-             (throw (ex-info (tru "Card {0} does not exist." card-id)
-                             {:card-id card-id})))
+           (throw (ex-info (tru "Card {0} does not exist." card-id)
+                           {:card-id card-id})))
+
          {{mbql-query                   :query
            database-id                  :database
            {template-tags :template-tags
@@ -131,42 +133,47 @@
           dataset?                               :dataset}
          card
 
-         [persisted? source-query]
-         (or (when (and persisted-info/*allow-persisted-substitution*
-                        (:active card)
-                        (:definition card)
-                        (:query_hash card)
-                        (= (:query_hash card) (persisted-info/query-hash (:dataset_query card)))
-                        (= (:definition card) (persisted-info/metadata->definition (:result_metadata card)
-                                                                                   (:table_name card)))
-                        (= (:state card) "persisted"))
-               (when log?
-                 (log/info (trs "Substituting cached query for card {0} from {1}.{2}"
-                                card-id
-                                (ddl.i/schema-name {:id database-id} (public-settings/site-uuid))
-                                (:table_name card))))
-               [true
-                {:native (format "select %s from %s.%s"
-                                 (str/join ", " (map :field-name (get-in card [:definition :field-definitions])))
-                                 (ddl.i/schema-name {:id database-id} (public-settings/site-uuid))
-                                 (:table_name card))}])
-             (when mbql-query
-               [false mbql-query])
-             (when native-query
-               ;; rename `:query` to `:native` because source queries have a slightly different shape
-               (let [native-query (set/rename-keys native-query {:query :native})]
-                 [false
-                  (cond-> native-query
-                    ;; trim trailing comments from SQL, but not other types of native queries
-                    (string? (:native native-query)) (update :native (partial trim-sql-query card-id))
-                    (empty? template-tags)           (dissoc :template-tags))]))
-             (throw (ex-info (tru "Missing source query in Card {0}" card-id)
-                             {:card card})))]
+         persisted? (and persisted-info/*allow-persisted-substitution*
+                         (:active card)
+                         (:definition card)
+                         (:query_hash card)
+                         (= (:query_hash card) (persisted-info/query-hash (:dataset_query card)))
+                         (= (:definition card) (persisted-info/metadata->definition (:result_metadata card)
+                                                                                    (:table_name card)))
+                         (= (:state card) "persisted"))
+
+         source-query (cond
+                        mbql-query
+                        mbql-query
+
+                        native-query
+                        ;; rename `:query` to `:native` because source queries have a slightly different shape
+                        (let [native-query (set/rename-keys native-query {:query :native})]
+                          (cond-> native-query
+                            ;; trim trailing comments from SQL, but not other types of native queries
+                            (string? (:native native-query)) (update :native (partial trim-sql-query card-id))
+                            (empty? template-tags)           (dissoc :template-tags)))
+
+                        :else
+                        (throw (ex-info (tru "Missing source query in Card {0}" card-id)
+                                        {:card card})))]
+     (when (and persisted? log?)
+       (log/info (trs "Found substitute cached query for card {0} from {1}.{2}"
+                      card-id
+                      (ddl.i/schema-name {:id database-id} (public-settings/site-uuid))
+                      (:table_name card))))
+
      ;; log the query at this point, it's useful for some purposes
      (log/debug (trs "Fetched source query from Card {0}:" card-id)
                 "\n"
                 (u/pprint-to-str 'yellow source-query))
-     (cond-> {:source-query    source-query
+
+     (cond-> {:source-query    (cond-> source-query
+                                 ;; This will be applied, if still appropriate, by the peristence middleware
+                                 persisted? (assoc :persisted-info/native (format "select %s from %s.%s"
+                                                                                  (str/join ", " (map :field-name (get-in card [:definition :field-definitions])))
+                                                                                  (ddl.i/schema-name {:id database-id} (public-settings/site-uuid))
+                                                                                  (:table_name card))))
               :database        database-id
               :source-metadata (cond-> (seq (map mbql.normalize/normalize-source-metadata result-metadata))
                                  persisted? sub-cached-field-refs)}

src/metabase/query_processor/middleware/persistence.clj

+(ns metabase.query-processor.middleware.persistence
+  (:require [metabase.mbql.util :as mbql.u]
+            [metabase.query-processor.middleware.permissions :as qp.perms]))
+
+(defn substitute-persisted-query
+  "Replaces source-query with the native query to the cache table.
+   `:persisted-info/native` is set in `fetch-source-query`.
+
+   If permissions are applied to the query (sandboxing) then do not use the cached query.
+   It may be be possible to use the persistence cache with sandboxing at a later date with further work."
+  [{::qp.perms/keys [perms] :as  query}]
+  (if perms
+    query
+    (mbql.u/replace query
+      (x :guard (every-pred map? :persisted-info/native))
+      {:native (:persisted-info/native x)})))