Repro for #12354 (#12525)

* Repro for #12354

* Remove the qbnewb check because that's closed by default now

* Fix native -> source query perms handling

* Test fix

Co-authored-by: Cam Saul <github@camsaul.com>

frontend/test/metabase/scenarios/question/native_subquery.cy.spec.js

+import { signInAsAdmin, restore, signIn } from "__support__/cypress";
+
+describe("scenarios > question > native subquery", () => {
+  before(restore);
+  beforeEach(signInAsAdmin);
+
+  it("should allow a user with no data access to execute a native subquery", () => {
+    // Create the initial SQL question and followup nested question
+    cy.request("POST", "/api/card", {
+      name: "People in WA",
+      dataset_query: {
+        type: "native",
+        native: {
+          query: "select * from PEOPLE where STATE = 'WA'",
+        },
+        database: 1,
+      },
+      display: "table",
+      description: null,
+      visualization_settings: {},
+      collection_id: null,
+      result_metadata: null,
+      metadata_checksum: null,
+    })
+      .then(response => {
+        cy.wrap(response.body.id).as("nestedQuestionId");
+        const tagID = `#${response.body.id}`;
+
+        cy.request("POST", "/api/card", {
+          name: "Count of People in WA",
+          dataset_query: {
+            type: "native",
+            native: {
+              query: `select COUNT(*) from {{#${response.body.id}}}`,
+              "template-tags": {
+                [tagID]: {
+                  id: "10422a0f-292d-10a3-fd90-407cc9e3e20e",
+                  name: tagID,
+                  "display-name": tagID,
+                  type: "card",
+                  "card-id": response.body.id,
+                },
+              },
+            },
+            database: 1,
+          },
+          display: "table",
+          description: null,
+          visualization_settings: {},
+          collection_id: null,
+          result_metadata: null,
+          metadata_checksum: null,
+        });
+      })
+      .then(response => {
+        cy.wrap(response.body.id).as("toplevelQuestionId");
+
+        cy.visit(`/question/${response.body.id}`);
+        cy.contains("41");
+      });
+
+    // Now sign in as a user w/no data access
+    signIn("nodata");
+
+    // They should be able to access both questions
+    cy.get("@nestedQuestionId").then(nestedQuestionId => {
+      cy.visit(`/question/${nestedQuestionId}`);
+      cy.contains("Showing 41 rows");
+    });
+
+    cy.get("@toplevelQuestionId").then(toplevelQuestionId => {
+      cy.visit(`/question/${toplevelQuestionId}`);
+      cy.contains("41");
+    });
+  });
+});

src/metabase/api/dataset.clj

@@ -16,7 +16,9 @@
             [metabase.query-processor
              [streaming :as qp.streaming]
              [util :as qputil]]
-            [metabase.query-processor.middleware.constraints :as constraints]
+            [metabase.query-processor.middleware
+             [constraints :as qp.constraints]
+             [permissions :as qp.perms]]
             [metabase.util
              [i18n :refer [trs]]
              [schema :as su]]
@@ -104,12 +106,13 @@
   {:average (or
              (some (comp query/average-execution-time-ms qputil/query-hash)
                    [query
-                    (assoc query :constraints constraints/default-query-constraints)])
+                    (assoc query :constraints qp.constraints/default-query-constraints)])
              0)})
 
 (api/defendpoint POST "/native"
   "Fetch a native version of an MBQL query."
   [:as {query :body}]
+  (qp.perms/check-current-user-has-adhoc-native-query-perms query)
   (qp/query->native-with-spliced-params query))
 
 

src/metabase/driver/common/parameters/values.clj

@@ -134,17 +134,17 @@
   (when-let [card-id (:card-id tag)]
     (when-let [query (db/select-one-field :dataset_query Card :id card-id)]
       (try
-       (i/map->ReferencedCardQuery
-        {:card-id card-id
-         :query   (:query (qp/query->native (assoc query :parameters params)))})
-       (catch ExceptionInfo e
-         (throw (ex-info
-                 (tru "The sub-query from referenced question #{0} failed with the following error: {1}"
-                      (str card-id) (.getMessage e))
-                 (merge (ex-data e)
-                        {:card-query-error? true
-                         :card-id           card-id
-                         :tag               tag}))))))))
+        (i/map->ReferencedCardQuery
+         {:card-id card-id
+          :query   (:query (qp/query->native (assoc query :parameters params, :info {:card-id card-id})))})
+        (catch ExceptionInfo e
+          (throw (ex-info
+                  (tru "The sub-query from referenced question #{0} failed with the following error: {1}"
+                       (str card-id) (.getMessage e))
+                  {:card-query-error? true
+                   :card-id           card-id
+                   :tag               tag}
+                  e)))))))
 
 (defn- validate-tag-snippet-db
   [{database-id :database, :as tag} snippet]

src/metabase/query_processor.clj

@@ -164,11 +164,8 @@
 
 (defn query->native
   "Return the native form for `query` (e.g. for a MBQL query on Postgres this would return a map containing the compiled
-  SQL form). (Like `preprocess`, this function will throw an Exception if preprocessing was not successful.)
-  (Currently, this function is mostly used by tests and in the REPL; `mbql-to-native/mbql->native` middleware handles
-  simliar functionality for queries that are actually executed.)"
+  SQL form). Like `preprocess`, this function will throw an Exception if preprocessing was not successful."
   [query]
-  (perms/check-current-user-has-adhoc-native-query-perms query)
   (preprocess-query query {:nativef
                            (fn [query context]
                              (context/raisef (qp.reducible/quit query) context))}))

test/metabase/driver/common/parameters/values_test.clj

@@ -2,12 +2,16 @@
   (:require [clojure.test :refer :all]
             [metabase
              [driver :as driver]
-             [test :as mt]]
+             [models :refer [Card Collection]]
+             [query-processor :as qp]
+             [test :as mt]
+             [util :as u]]
             [metabase.driver.common.parameters :as i]
             [metabase.driver.common.parameters.values :as values]
             [metabase.models
-             [card :refer [Card]]
-             [field :refer [map->FieldInstance]]]
+             [field :refer [map->FieldInstance]]
+             [permissions :as perms]
+             [permissions-group :as group]]
             [metabase.test.data :as data]
             [toucan.util.test :as tt])
   (:import clojure.lang.ExceptionInfo))
@@ -233,3 +237,28 @@
             (testing "tag"
               (is (= tag
                      (:tag exc-data))))))))))
+
+(deftest card-query-permissions-test
+  (testing "We should be able to run a query referenced via a template tag if we have perms for the Card in question (#12354)"
+    (mt/with-non-admin-groups-no-root-collection-perms
+      (mt/with-temp-copy-of-db
+        (perms/revoke-permissions! (group/all-users) (mt/id))
+        (mt/with-temp* [Collection [collection]
+                        Card       [{card-1-id :id, :as card-1} {:collection_id (u/get-id collection)
+                                                                 :dataset_query (mt/mbql-query venues
+                                                                                  {:order-by [[:asc $id]], :limit 2})}]
+                        Card       [card-2 {:collection_id (u/get-id collection)
+                                            :dataset_query (mt/native-query
+                                                             {:query         "SELECT * FROM {{card}}"
+                                                              :template-tags {"card" {:name         "card"
+                                                                                      :display-name "card"
+                                                                                      :type         :card
+                                                                                      :card-id      card-1-id}}})}]]
+          (perms/grant-collection-read-permissions! (group/all-users) collection)
+          (mt/with-test-user :rasta
+            (is (= [[1 "Red Medicine"           4 10.0646 -165.374 3]
+                    [2 "Stout Burgers & Beers" 11 34.0996 -118.329 2]]
+                   (mt/rows
+                     (qp/process-userland-query (assoc (:dataset_query card-2)
+                                                       :info {:executed-by (mt/user->id :rasta)
+                                                              :card-id     (u/get-id card-2)})))))))))))

test/metabase/query_processor/middleware/permissions_test.clj

@@ -224,44 +224,62 @@
 (deftest e2e-nested-source-card-test
   (testing "Make sure permissions are calculated for Card -> Card -> Source Query (#12354)"
     (mt/with-non-admin-groups-no-root-collection-perms
-      (mt/with-temp* [Collection [collection]
-                      Card       [card-1 {:collection_id (u/get-id collection)
-                                          :dataset_query (mt/mbql-query venues {:order-by [[:asc $id]], :limit 2})}]
-                      Card       [card-2 {:collection_id (u/get-id collection)
-                                          :dataset_query (mt/mbql-query nil
-                                                           {:source-table (format "card__%d" (u/get-id card-1))})}]]
-        (testing "\nshould be able to read nested-nested Card if we have Collection permissions\n"
+      (mt/with-temp-copy-of-db
+        (perms/revoke-permissions! (perms-group/all-users) (mt/id))
+        (mt/with-temp Collection [collection]
           (perms/grant-collection-read-permissions! (perms-group/all-users) collection)
-          (mt/with-test-user :rasta
-            (let [expected [[1 "Red Medicine"           4 10.0646 -165.374 3]
-                            [2 "Stout Burgers & Beers" 11 34.0996 -118.329 2]]]
-              (testing "Should be able to run Card 1 [Card -> Source Query]"
-                (is (= expected
-                       (mt/rows
-                         (qp/process-userland-query (assoc (:dataset_query card-1)
-                                                           :info {:executed-by (mt/user->id :rasta)
-                                                                  :card-id     (u/get-id card-1)}))))))
+          (doseq [[card-1-query-type card-1-query] {"MBQL"   (mt/mbql-query venues
+                                                               {:order-by [[:asc $id]], :limit 2})
+                                                    "native" (mt/native-query
+                                                               {:query (str "SELECT id, name, category_id, latitude, longitude, price "
+                                                                            "FROM venues "
+                                                                            "ORDER BY id ASC "
+                                                                            "LIMIT 2")})}]
+            (testing (format "\nCard 1 is a %s query" card-1-query-type)
+              (mt/with-temp Card [{card-1-id :id, :as card-1} {:collection_id (u/get-id collection)
+                                                               :dataset_query card-1-query}]
+                (doseq [[card-2-query-type card-2-query] {"MBQL"   (mt/mbql-query nil
+                                                                     {:source-table (format "card__%d" card-1-id)})
+                                                          "native" (mt/native-query
+                                                                     {:query         "SELECT * FROM {{card}}"
+                                                                      :template-tags {"card" {:name         "card"
+                                                                                              :display-name "card"
+                                                                                              :type         :card
+                                                                                              :card-id      card-1-id}}})}]
+                  (testing (format "\nCard 2 is a %s query" card-2-query-type)
+                    (mt/with-temp Card [card-2 {:collection_id (u/get-id collection)
+                                                :dataset_query card-2-query}]
+                      (testing "\nshould be able to read nested-nested Card if we have Collection permissions\n"
+                        (mt/with-test-user :rasta
+                          (let [expected [[1 "Red Medicine"           4 10.0646 -165.374 3]
+                                          [2 "Stout Burgers & Beers" 11 34.0996 -118.329 2]]]
+                            (testing "Should be able to run Card 1 directly"
+                              (is (= expected
+                                     (mt/rows
+                                       (qp/process-userland-query (assoc (:dataset_query card-1)
+                                                                         :info {:executed-by (mt/user->id :rasta)
+                                                                                :card-id     card-1-id}))))))
 
-              (testing "Should be able to run Card 2 [Card -> Card -> Source Query]"
-                (is (= expected
-                       (mt/rows
-                         (qp/process-userland-query (assoc (:dataset_query card-2)
-                                                           :info {:executed-by (mt/user->id :rasta)
-                                                                  :card-id     (u/get-id card-2)}))))))
+                            (testing "Should be able to run Card 2 directly [Card 2 -> Card 1 -> Source Query]"
+                              (is (= expected
+                                     (mt/rows
+                                       (qp/process-userland-query (assoc (:dataset_query card-2)
+                                                                         :info {:executed-by (mt/user->id :rasta)
+                                                                                :card-id     (u/get-id card-2)}))))))
 
-              (testing "Should be able to run query with Card 1 as source query [Ad-hoc -> Card -> Source Query]"
-                (is (= expected
-                       (mt/rows
-                         (qp/process-userland-query (assoc (mt/mbql-query nil
-                                                             {:source-table (format "card__%d" (u/get-id card-1))})
-                                                           :info {:executed-by (mt/user->id :rasta)}))))))
+                            (testing "Should be able to run ad-hoc query with Card 1 as source query [Ad-hoc -> Card -> Source Query]"
+                              (is (= expected
+                                     (mt/rows
+                                       (qp/process-userland-query (assoc (mt/mbql-query nil
+                                                                           {:source-table (format "card__%d" card-1-id)})
+                                                                         :info {:executed-by (mt/user->id :rasta)}))))))
 
-              (testing "Should be able to run query with Card 2 as source query [Ad-hoc -> Card -> Card -> Source Query]"
-                (is (= expected
-                       (mt/rows
-                         (qp/process-userland-query (assoc (mt/mbql-query nil
-                                                             {:source-table (format "card__%d" (u/get-id card-2))})
-                                                           :info {:executed-by (mt/user->id :rasta)})))))))))))))
+                            (testing "Should be able to run ad-hoc query with Card 2 as source query [Ad-hoc -> Card -> Card -> Source Query]"
+                              (is (= expected
+                                     (mt/rows
+                                       (qp/process-userland-query (assoc (mt/mbql-query nil
+                                                                           {:source-table (format "card__%d" (u/get-id card-2))})
+                                                                         :info {:executed-by (mt/user->id :rasta)}))))))))))))))))))))
 
 (deftest e2e-ignore-user-supplied-card-ids-test
   (testing "You shouldn't be able to bypass security restrictions by passing `[:info :card-id]` in the query."

test/metabase/query_processor_test/query_to_native_test.clj

@@ -60,16 +60,7 @@
                     s/Any  s/Any}
                    (query->native-with-user-perms
                     (mt/mbql-query venues)
-                    {:object-perms? false, :native-perms? true}))))
-    (testing "If you don't have have native query execution permissions for the DB it should throw an error"
-      (is (= {:error                "You do not have permissions to run this query."
-              :required-permissions (perms/adhoc-native-query-path (mt/id))
-              :actual-permissions   #{(perms/object-path (mt/id) "PUBLIC" (mt/id :venues))}
-              :permissions-error?   true
-              :type                 :missing-required-permissions}
-             (query->native-with-user-perms
-              (mt/mbql-query venues)
-              {:object-perms? true, :native-perms? false}))))))
+                    {:object-perms? false, :native-perms? true}))))))
 
 (deftest error-test
   (testing "If the query is bad in some way it should return a relevant error (?)"