Merge pull request #7026 from metabase/use-source-card-read-perms-for-saving-cards

When saving Card with source Card use its read perms

src/metabase/api/card.clj

@@ -265,11 +265,11 @@
    metadata_checksum      (s/maybe su/NonBlankString)}
   ;; check that we have permissions to run the query that we're trying to save
   (api/check-403 (perms/set-has-full-permissions-for-set? @api/*current-user-permissions-set*
-                                                          (card/query-perms-set dataset_query :write)))
+                   (card/query-perms-set dataset_query :write)))
   ;; check that we have permissions for the collection we're trying to save this card to, if applicable
   (when collection_id
     (api/check-403 (perms/set-has-full-permissions? @api/*current-user-permissions-set*
-                                                    (perms/collection-readwrite-path collection_id))))
+                     (perms/collection-readwrite-path collection_id))))
   ;; everything is g2g, now save the card
   (let [card (db/insert! Card
                :creator_id             api/*current-user-id*

src/metabase/api/dataset.clj

@@ -40,15 +40,15 @@
 ;;; -------------------------------------------- Running a Query Normally --------------------------------------------
 
 (defn- query->source-card-id
-  "Return the ID of the Card used as the \"source\" query of this query, if applicable; otherwise return `nil`.
-   Used so `:card-id` context can be passed along with the query so Collections perms checking is done if appropriate."
+  "Return the ID of the Card used as the \"source\" query of this query, if applicable; otherwise return `nil`. Used so
+  `:card-id` context can be passed along with the query so Collections perms checking is done if appropriate. This fn
+  is a wrapper for the function of the same name in the QP util namespace; it adds additional permissions checking as
+  well."
   [outer-query]
-  (let [source-table (qputil/get-in-normalized outer-query [:query :source-table])]
-    (when (string? source-table)
-      (when-let [[_ card-id-str] (re-matches #"^card__(\d+$)" source-table)]
-        (log/info (str "Source query for this query is Card " card-id-str))
-        (u/prog1 (Integer/parseInt card-id-str)
-          (api/read-check Card <>))))))
+  (when-let [source-card-id (qputil/query->source-card-id outer-query)]
+    (log/info (str "Source query for this query is Card " source-card-id))
+    (api/read-check Card source-card-id)
+    source-card-id))
 
 (api/defendpoint POST "/"
   "Execute a query and retrieve the results in the usual format."

src/metabase/models/card.clj

@@ -101,18 +101,33 @@
                               (:schema table)
                               (or (:id table) (:table-id table)))))))
 
+(declare perms-objects-set)
+
+(defn- source-card-perms
+  "If `outer-query` is based on a source Card (if `:source-table` uses a psuedo-source-table like `card__<id>`) then
+  return the permissions needed to *read* that Card. Running or saving a Card that uses another Card as a source query
+  simply requires read permissions for that Card; e.g. if you are allowed to view a query you can save a new query
+  that uses it as a source. Thus the distinction between read and write permissions in not important here.
+
+  See issue #6845 for further discussion."
+  [outer-query]
+  (when-let [source-card-id (qputil/query->source-card-id outer-query)]
+    (perms-objects-set (Card source-card-id) :read)))
+
 (defn- mbql-permissions-path-set
   "Return the set of required permissions needed to run QUERY."
   [read-or-write query]
   {:pre [(map? query) (map? (:query query))]}
-  (try (let [{:keys [query database]} (qp/expand query)]
-         (tables->permissions-path-set read-or-write database (query->source-and-join-tables query)))
-       ;; if for some reason we can't expand the Card (i.e. it's an invalid legacy card)
-       ;; just return a set of permissions that means no one will ever get to see it
-       (catch Throwable e
-         (log/warn "Error getting permissions for card:" (.getMessage e) "\n"
-                   (u/pprint-to-str (u/filtered-stacktrace e)))
-         #{"/db/0/"})))                        ; DB 0 will never exist
+  (try
+    (or (source-card-perms query)
+        (let [{:keys [query database]} (qp/expand query)]
+          (tables->permissions-path-set read-or-write database (query->source-and-join-tables query))))
+    ;; if for some reason we can't expand the Card (i.e. it's an invalid legacy card)
+    ;; just return a set of permissions that means no one will ever get to see it
+    (catch Throwable e
+      (log/warn "Error getting permissions for card:" (.getMessage e) "\n"
+                (u/pprint-to-str (u/filtered-stacktrace e)))
+      #{"/db/0/"})))                    ; DB 0 will never exist
 
 ;; it takes a lot of DB calls and function calls to expand/resolve a query, and since they're pure functions we can
 ;; save ourselves some a lot of DB calls by caching the results. Cache the permissions reqquired to run a given query
@@ -154,12 +169,14 @@
 ;;; -------------------------------------------------- Dependencies --------------------------------------------------
 
 (defn card-dependencies
-  "Calculate any dependent objects for a given `Card`."
-  [this id {:keys [dataset_query]}]
-  (when (and dataset_query
-             (= :query (keyword (:type dataset_query))))
-    {:Metric  (q/extract-metric-ids (:query dataset_query))
-     :Segment (q/extract-segment-ids (:query dataset_query))}))
+  "Calculate any dependent objects for a given `card`."
+  ([_ _ card]
+   (card-dependencies card))
+  ([{:keys [dataset_query]}]
+   (when (and dataset_query
+              (= :query (keyword (:type dataset_query))))
+     {:Metric  (q/extract-metric-ids (:query dataset_query))
+      :Segment (q/extract-segment-ids (:query dataset_query))})))
 
 
 ;;; -------------------------------------------------- Revisions --------------------------------------------------

src/metabase/models/permissions.clj

@@ -145,17 +145,20 @@
 
 (defn set-has-full-permissions?
   "Does PERMISSIONS-SET grant *full* access to object with PATH?"
+  {:style/indent 1}
   ^Boolean [permissions-set path]
   (boolean (some (u/rpartial is-permissions-for-object? path) permissions-set)))
 
 (defn set-has-partial-permissions?
   "Does PERMISSIONS-SET grant access full access to object with PATH *or* to a descendant of it?"
+  {:style/indent 1}
   ^Boolean [permissions-set path]
   (boolean (some (u/rpartial is-partial-permissions-for-object? path) permissions-set)))
 
 
 (defn ^Boolean set-has-full-permissions-for-set?
   "Do the permissions paths in PERMISSIONS-SET grant *full* access to all the object paths in OBJECT-PATHS-SET?"
+  {:style/indent 1}
   [permissions-set object-paths-set]
   {:pre [(is-permissions-set? permissions-set) (is-permissions-set? object-paths-set)]}
   (every? (partial set-has-full-permissions? permissions-set)
@@ -164,6 +167,7 @@
 (defn ^Boolean set-has-partial-permissions-for-set?
   "Do the permissions paths in PERMISSIONS-SET grant *partial* access to all the object paths in OBJECT-PATHS-SET?
    (PERMISSIONS-SET must grant partial access to *every* object in OBJECT-PATH-SETS set)."
+  {:style/indent 1}
   [permissions-set object-paths-set]
   {:pre [(is-permissions-set? permissions-set) (is-permissions-set? object-paths-set)]}
   (every? (partial set-has-partial-permissions? permissions-set)

src/metabase/query_processor/util.clj

@@ -127,3 +127,14 @@
   "Return a 256-bit SHA3 hash of QUERY as a key for the cache. (This is returned as a byte array.)"
   [query]
   (hash/sha3-256 (json/generate-string (select-keys-for-hashing query))))
+
+
+;;; --------------------------------------------- Query Source Card IDs ----------------------------------------------
+
+(defn query->source-card-id
+  "Return the ID of the Card used as the \"source\" query of this query, if applicable; otherwise return `nil`."
+  ^Integer [outer-query]
+  (let [source-table (get-in-normalized outer-query [:query :source-table])]
+    (when (string? source-table)
+      (when-let [[_ card-id-str] (re-matches #"^card__(\d+$)" source-table)]
+        (Integer/parseInt card-id-str)))))

test/metabase/models/card_test.clj

@@ -38,26 +38,29 @@
 (expect
   {:Segment #{2 3}
    :Metric  nil}
-  (card-dependencies Card 12 {:dataset_query {:type :query
-                                              :query {:aggregation ["rows"]
-                                                      :filter      ["AND" [">" 4 "2014-10-19"] ["=" 5 "yes"] ["SEGMENT" 2] ["SEGMENT" 3]]}}}))
+  (card-dependencies
+   {:dataset_query {:type :query
+                    :query {:aggregation ["rows"]
+                            :filter      ["AND" [">" 4 "2014-10-19"] ["=" 5 "yes"] ["SEGMENT" 2] ["SEGMENT" 3]]}}}))
 
 (expect
   {:Segment #{1}
    :Metric #{7}}
-  (card-dependencies Card 12 {:dataset_query {:type :query
-                                              :query {:aggregation ["METRIC" 7]
-                                                      :filter      ["AND" [">" 4 "2014-10-19"] ["=" 5 "yes"] ["OR" ["SEGMENT" 1] ["!=" 5 "5"]]]}}}))
+  (card-dependencies
+   {:dataset_query {:type :query
+                    :query {:aggregation ["METRIC" 7]
+                            :filter      ["AND" [">" 4 "2014-10-19"] ["=" 5 "yes"] ["OR" ["SEGMENT" 1] ["!=" 5 "5"]]]}}}))
 
 (expect
   {:Segment nil
    :Metric  nil}
-  (card-dependencies Card 12 {:dataset_query {:type :query
-                                              :query {:aggregation nil
-                                                      :filter      nil}}}))
+  (card-dependencies
+   {:dataset_query {:type :query
+                    :query {:aggregation nil
+                            :filter      nil}}}))
 
 
-;;; ------------------------------------------------------------ Permissions Checking ------------------------------------------------------------
+;;; ---------------------------------------------- Permissions Checking ----------------------------------------------
 
 (expect
   false
@@ -150,13 +153,24 @@
                                             :native   {:query "SELECT * FROM CHECKINS"}}}]
     (query-perms-set (query-with-source-card card) :read)))
 
+;; You should still only need native READ permissions if you want to save a Card based on another Card you can already
+;; READ.
 (expect
-  #{(perms/native-readwrite-path (data/id))}
+  #{(perms/native-read-path (data/id))}
   (tt/with-temp Card [card {:dataset_query {:database (data/id)
                                             :type     :native
                                             :native   {:query "SELECT * FROM CHECKINS"}}}]
     (query-perms-set (query-with-source-card card) :write)))
 
+;; However if you just pass in the same query directly as a `:source-query` you will still require READWRITE
+;; permissions to save the query since we can't verify that it belongs to a Card that you can view.
+(expect
+  #{(perms/native-readwrite-path (data/id))}
+  (query-perms-set {:database (data/id)
+                    :type     :query
+                    :query    {:source-query {:native "SELECT * FROM CHECKINS"}}}
+                   :write))
+
 ;; invalid/legacy card should return perms for something that doesn't exist so no one gets to see it
 (expect
   #{"/db/0/"}

test/metabase/query_processor_test/nested_queries_test.clj

@@ -9,13 +9,19 @@
              [util :as u]]
             [metabase.driver.generic-sql :as generic-sql]
             [metabase.models
-             [card :refer [Card]]
-             [database :as database]
+             [card :as card :refer [Card]]
+             [database :as database :refer [Database]]
              [field :refer [Field]]
+             [permissions :as perms]
+             [permissions-group :as perms-group]
              [segment :refer [Segment]]
              [table :refer [Table]]]
-            [metabase.test.data :as data]
-            [metabase.test.data.datasets :as datasets]
+            [metabase.test
+             [data :as data]
+             [util :as tu]]
+            [metabase.test.data
+             [datasets :as datasets]
+             [users :refer [user->client]]]
             [toucan.db :as db]
             [toucan.util.test :as tt]))
 
@@ -458,3 +464,62 @@
           :aggregation [:count])
         qp/process-query
         rows)))
+
+
+;; Make suer you're allowed to save a query that uses a SQL-based source query even if you don't have SQL *write*
+;; permissions (#6845)
+
+;; Check that write perms for a Card with a source query require that you are able to *read* (i.e., view) the source
+;; query rather than be able to write (i.e., save) it. For example you should be able to save a query that uses a
+;; native query as its source query if you have permissions to view that query, even if you aren't allowed to create
+;; new ad-hoc SQL queries yourself.
+(expect
+ #{(perms/native-read-path (data/id))}
+ (tt/with-temp Card [card {:dataset_query {:database (data/id)
+                                           :type     :native
+                                           :native   {:query "SELECT * FROM VENUES"}}}]
+   (card/query-perms-set (query-with-source-card card :aggregation [:count]) :write)))
+
+;; try this in an end-to-end fashion using the API and make sure we can save a Card if we have appropriate read
+;; permissions for the source query
+(defn- do-with-temp-copy-of-test-db
+  "Run `f` with a temporary Database that copies the details from the standard test database. `f` is invoked as `(f
+  db)`."
+  [f]
+  (tt/with-temp Database [db {:details (:details (data/db)), :engine "h2"}]
+    (f db)))
+
+(defn- save-card-via-API-with-native-source-query!
+  "Attempt to save a Card that uses a native source query for Database with `db-id` via the API using Rasta. Use this to
+  test how the API endpoint behaves based on certain permissions grants for the `All Users` group."
+  [expected-status-code db-id]
+  (tt/with-temp Card [card {:dataset_query {:database db-id
+                                            :type     :native
+                                            :native   {:query "SELECT * FROM VENUES"}}}]
+    ((user->client :rasta) :post "card"
+     {:name                   (tu/random-name)
+      :display                "scalar"
+      :visualization_settings {}
+      :dataset_query          (query-with-source-card card
+                                :aggregation [:count])})))
+
+;; ok... grant native *read* permissions which means we should be able to view our source query generated with the
+;; function above. API should allow use to save here because write permissions for a query require read permissions
+;; for any source queries
+(expect
+  :ok
+  (do-with-temp-copy-of-test-db
+   (fn [db]
+     (perms/revoke-permissions! (perms-group/all-users) (u/get-id db))
+     (perms/grant-permissions! (perms-group/all-users) (perms/native-read-path (u/get-id db)))
+     (save-card-via-API-with-native-source-query! 200 (u/get-id db))
+     :ok)))
+
+;; however, if we do *not* have read permissions for the source query, we shouldn't be allowed to save the query. This
+;; API call should fail
+(expect
+  "You don't have permissions to do that."
+  (do-with-temp-copy-of-test-db
+   (fn [db]
+     (perms/revoke-permissions! (perms-group/all-users) (u/get-id db))
+     (save-card-via-API-with-native-source-query! 403 (u/get-id db)))))