Allow users to run ad-hoc query starting with Card if they have perms for it (#12505)

* Allow users to run ad-hoc query starting with Card if they have perms for it

* Convert metabase.query-processor.middleware.fetch-source-query-test to modern style

* When resolving source query from card_id, use Card ID for perms

* Add test to make sure we don't overwrite existing card-id

* Add missing require

* Test fix 

* Sort namespaces

* Tweak codecov.yaml

codecov.yml

@@ -3,5 +3,10 @@ codecov:
     require_ci_to_pass: no
 
 coverage:
-    round: down
-    range: 69..100
+  status:
+    project:
+      default:
+        # New code must have minimum 80% test coverage
+        target: 75%
+        # Whole-project test coverage is allowed to drop up to 5%. (For situtations where we delete code with full coverage)
+        threshold: 5%

src/metabase/query_processor.clj

 (ns metabase.query-processor
-  "Preprocessor that does simple transformations to all incoming queries, simplifing the driver-specific
-  implementations."
+  "The main entrypoint to running queries."
   (:require [clojure.tools.logging :as log]
             [metabase
              [config :as config]
              [driver :as driver]]
             [metabase.driver.util :as driver.u]
-            [metabase.mbql.schema :as mbql.s]
             [metabase.query-processor
              [context :as context]
              [error-type :as error-type]
@@ -219,18 +217,15 @@
          query
          args))
 
-(s/defn ^:private add-info [query info :- mbql.s/Info]
-  (update query :info merge info))
-
 (s/defn process-query-and-save-execution!
   "Process and run a 'userland' MBQL query (e.g. one ran as the result of an API call, scheduled Pulse, MetaBot query,
   etc.). Returns results in a format appropriate for consumption by FE client. Saves QueryExecution row in application
   DB."
   ([query info]
-   (process-userland-query (add-info query info)))
+   (process-userland-query (assoc query :info info)))
 
   ([query info context]
-   (process-userland-query (add-info query info) context)))
+   (process-userland-query (assoc query :info info) context)))
 
 (defn- add-default-constraints [query]
   (assoc-in query [:middleware :add-default-userland-constraints?] true))

src/metabase/query_processor/middleware/fetch_source_query.clj

@@ -22,6 +22,7 @@
   TODO - consider renaming this namespace to `metabase.query-processor.middleware.resolve-card-id-source-tables`"
   (:require [clojure.string :as str]
             [clojure.tools.logging :as log]
+            [medley.core :as m]
             [metabase.mbql
              [normalize :as normalize]
              [schema :as mbql.s]
@@ -142,9 +143,12 @@
 
 (s/defn ^:private resolve-one :- MapWithResolvedSourceQuery
   [{:keys [source-table], :as m} :- {:source-table mbql.s/source-table-card-id-regex, s/Keyword s/Any}]
-  (let [source-query-and-metadata (-> source-table source-table-str->card-id card-id->source-query-and-metadata)]
+  (let [card-id                   (-> source-table source-table-str->card-id)
+        source-query-and-metadata (-> card-id card-id->source-query-and-metadata)]
     (merge
      (dissoc m :source-table)
+     ;; record the `::card-id` we've resolved here. We'll include it in `:info` for permissions purposes later
+     {::card-id card-id}
      source-query-and-metadata)))
 
 (defn- resolve-all*
@@ -215,15 +219,29 @@
     (&match :guard (every-pred map? :database (comp integer? :database)))
     (recur (dissoc &match :database))))
 
+(defn- add-card-id-to-info
+  "If the ID of the Card we've resolved (`::card-id`) was added by a previous step, add it to `:info` so it can be used
+  for permissions purposes; remove any `::card-id`s in the query."
+  [query]
+  (let [card-id (get-in query [:query ::card-id])
+        query   (mbql.u/replace-in query [:query]
+                  (&match :guard (every-pred map? ::card-id))
+                  (recur (dissoc &match ::card-id)))]
+    (cond-> query
+      card-id (update-in [:info :card-id] #(or % card-id)))))
+
 (s/defn ^:private resolve-all :- su/Map
   "Recursively replace all Card ID source tables in `query` with resolved `:source-query` and `:source-metadata`. Since
   the `:database` is only useful for top-level source queries, we'll remove it from all other levels."
   [query :- su/Map]
-  (-> query
+  ;; if a `::card-id` is already in the query, remove it, so we don't pull user-supplied input up into `:info`
+  ;; allowing someone to bypass permissions
+  (-> (m/dissoc-in query [:query ::card-id])
       check-for-circular-references
       resolve-all*
       copy-source-query-database-ids
-      remove-unneeded-database-ids))
+      remove-unneeded-database-ids
+      add-card-id-to-info))
 
 (s/defn ^:private resolve-card-id-source-tables* :- FullyResolvedQuery
   "Resolve `card__n`-style `:source-tables` in `query`."

test/metabase/query_processor/middleware/permissions_test.clj

@@ -2,22 +2,17 @@
   "Tests for the middleware that checks whether the current user has permissions to run a given query."
   (:require [clojure.test :refer :all]
             [metabase
+             [models :refer [Card Collection Database Table]]
              [query-processor :as qp]
              [test :as mt]
              [util :as u]]
             [metabase.api.common :as api]
             [metabase.models
-             [card :refer [Card]]
-             [database :refer [Database]]
              [permissions :as perms]
-             [permissions-group :as perms-group]
-             [table :refer [Table]]]
+             [permissions-group :as perms-group]]
             [metabase.query-processor.error-type :as error-type]
             [metabase.query-processor.middleware.permissions :refer [check-query-permissions]]
-            [metabase.test.data :as data]
-            [metabase.test.data.users :as users]
-            [schema.core :as s]
-            [toucan.util.test :as tt])
+            [schema.core :as s])
   (:import clojure.lang.ExceptionInfo))
 
 (defn- check-perms [query]
@@ -46,7 +41,7 @@
             :native   {:query "SELECT * FROM VENUES"}}))))
 
   (testing "...but it should work if user has perms"
-    (tt/with-temp Database [db]
+    (mt/with-temp Database [db]
       ;; query should be returned by middleware unchanged
       (is (= {:database (u/get-id db)
               :type     :native
@@ -59,7 +54,7 @@
 (deftest mbql-query-perms-test
   (testing "Make sure the MBQL query fails to run if current user doesn't have perms"
     (is (thrown-with-msg? ExceptionInfo perms-error-msg
-          (tt/with-temp* [Database [db]
+          (mt/with-temp* [Database [db]
                           Table    [table {:db_id (u/get-id db)}]]
             ;; All users get perms for all new DBs by default
             (perms/revoke-permissions! (perms-group/all-users) (u/get-id db))
@@ -69,7 +64,7 @@
               :query    {:source-table (u/get-id table)}})))))
 
   (testing "...but it should work if user has perms [MBQL]"
-    (tt/with-temp* [Database [db]
+    (mt/with-temp* [Database [db]
                     Table    [table {:db_id (u/get-id db)}]]
       ;; query should be returned by middleware unchanged
       (= {:database (u/get-id db)
@@ -89,7 +84,7 @@
             :query   {:source-query {:native "SELECT * FROM VENUES"}}}))))
 
   (testing "...but it should work if user has perms [nested native queries]"
-    (tt/with-temp Database [db]
+    (mt/with-temp Database [db]
       ;; query should be returned by middleware unchanged
       (= {:database (u/get-id db)
           :type     :query
@@ -102,7 +97,7 @@
 (deftest nested-mbql-query-test
   (testing "Make sure nested MBQL query fails to run if current user doesn't have perms"
     (is (thrown-with-msg? ExceptionInfo perms-error-msg
-          (tt/with-temp* [Database [db]
+          (mt/with-temp* [Database [db]
                           Table    [table {:db_id (u/get-id db)}]]
             ;; All users get perms for all new DBs by default
             (perms/revoke-permissions! (perms-group/all-users) (u/get-id db))
@@ -112,7 +107,7 @@
               :query    {:source-query {:source-table (u/get-id table)}}})))))
 
   (testing "...but it should work if user has perms [nested MBQL queries]"
-    (tt/with-temp* [Database [db]
+    (mt/with-temp* [Database [db]
                     Table    [table {:db_id (u/get-id db)}]]
       (= {:database (u/get-id db)
           :type     :query
@@ -125,7 +120,7 @@
 (deftest template-tags-referenced-queries-test
   (testing "Fails for MBQL query referenced in template tag, when user has no perms to referenced query"
     (is (thrown-with-msg? ExceptionInfo perms-error-msg
-          (tt/with-temp* [Database [db]
+          (mt/with-temp* [Database [db]
                           Table    [table-1 {:db_id (u/get-id db)}]
                           Table    [table-2 {:db_id (u/get-id db)}]
                           Card     [card    {:dataset_query {:database (u/get-id db), :type :query,
@@ -143,7 +138,7 @@
                                             :type "card", :card card-id}}}}))))))
 
   (testing "...but it should work if user has perms [template tag referenced query]"
-    (tt/with-temp* [Database [db]
+    (mt/with-temp* [Database [db]
                     Table    [table-1 {:db_id (u/get-id db)}]
                     Table    [table-2 {:db_id (u/get-id db)}]
                     Card     [card    {:dataset_query {:database (u/get-id db), :type :query,
@@ -167,7 +162,7 @@
 
   (testing "Fails for native query referenced in template tag, when user has no perms to referenced query"
     (is (thrown-with-msg? ExceptionInfo perms-error-msg
-          (tt/with-temp* [Database [db]
+          (mt/with-temp* [Database [db]
                           Card     [card {:dataset_query
                                           {:database (u/get-id db), :type :native,
                                            :native {:query "SELECT 1 AS \"foo\", 2 AS \"bar\", 3 AS \"baz\""}}}]]
@@ -184,7 +179,7 @@
                                             :type "card", :card card-id}}}}))))))
 
   (testing "...but it should work if user has perms [template tag referenced query]"
-    (tt/with-temp* [Database [db]
+    (mt/with-temp* [Database [db]
                     Card     [card {:dataset_query
                                     {:database (u/get-id db), :type :native,
                                      :native {:query "SELECT 1 AS \"foo\", 2 AS \"bar\", 3 AS \"baz\""}}}]]
@@ -208,19 +203,75 @@
 (deftest end-to-end-test
   (testing (str "Make sure it works end-to-end: make sure bound `*current-user-id*` and `*current-user-permissions-set*` "
                 "are used to permissions check queries")
-    (binding [api/*current-user-id*              (users/user->id :rasta)
+    (binding [api/*current-user-id*              (mt/user->id :rasta)
               api/*current-user-permissions-set* (delay #{})]
       (is (schema= {:status   (s/eq :failed)
                     :class    (s/eq clojure.lang.ExceptionInfo)
                     :error    (s/eq "You do not have permissions to run this query.")
-                    :ex-data  {:required-permissions (s/eq #{(perms/table-query-path (data/id) "PUBLIC" (data/id :venues))})
+                    :ex-data  {:required-permissions (s/eq #{(perms/table-query-path (mt/id) "PUBLIC" (mt/id :venues))})
                                :actual-permissions   (s/eq #{})
                                :permissions-error?   (s/eq true)
                                :type                 (s/eq error-type/missing-required-permissions)
                                s/Keyword             s/Any}
                     s/Keyword s/Any}
                    (qp/process-userland-query
-                    {:database (data/id)
+                    {:database (mt/id)
                      :type     :query
-                     :query    {:source-table (data/id :venues)
+                     :query    {:source-table (mt/id :venues)
                                 :limit        1}}))))))
+
+(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"
+          (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)}))))))
+
+              (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 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 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."
+    (mt/with-temp-copy-of-db
+      (perms/revoke-permissions! (perms-group/all-users) (mt/id))
+      (mt/with-temp* [Collection [collection]
+                      Card       [card {:collection_id (u/get-id collection)
+                                        :dataset_query (mt/mbql-query venues {:fields [$id], :order-by [[:asc $id]], :limit 2})}]]
+        ;; Since the collection derives from the root collection this grant shouldn't really be needed, but better to
+        ;; be extra-sure in this case that the user is getting rejected for data perms and not card/collection perms
+        (perms/grant-collection-read-permissions! (perms-group/all-users) collection)
+        (is (= "You don't have permissions to do that."
+               ((mt/user->client :rasta) :post "dataset" (assoc (mt/mbql-query venues {:limit 1})
+                                                                :info {:card-id (u/get-id card)}))))))))