diff --git a/src/metabase/api/card.clj b/src/metabase/api/card.clj
index 6e45e8600d51c1aa2237d52725d4db2661275f53..181ab8214394d21ce490a12785c82fc48369cae6 100644
--- a/src/metabase/api/card.clj
+++ b/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*
diff --git a/src/metabase/api/dataset.clj b/src/metabase/api/dataset.clj
index 6392480c6a3b4a5004400a9702924368f01c99f0..be2c68d17e734f5ed42d2cbe6935e1cd6ff20a5d 100644
--- a/src/metabase/api/dataset.clj
+++ b/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."
diff --git a/src/metabase/models/card.clj b/src/metabase/models/card.clj
index dbe47eec12ab2da9d1c567282e568e824c1ce8c9..e840cb7cfc09e68831cfba31fb35ef009f0a83c0 100644
--- a/src/metabase/models/card.clj
+++ b/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 --------------------------------------------------
diff --git a/src/metabase/models/permissions.clj b/src/metabase/models/permissions.clj
index af5bfc5ef55c1c2de27752eaf6260a6b3588cd87..1bb3ac67e9f999875dea08cc0a6a8a2765b23201 100644
--- a/src/metabase/models/permissions.clj
+++ b/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)
diff --git a/src/metabase/query_processor/util.clj b/src/metabase/query_processor/util.clj
index 45aeed87ef43ab42611faef191d490c090b21d88..5441fa30c01096ff1fed8fcd3795fafcc819d0a0 100644
--- a/src/metabase/query_processor/util.clj
+++ b/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)))))
diff --git a/test/metabase/models/card_test.clj b/test/metabase/models/card_test.clj
index 37769bdceaf889bbc075a02fefcab77748e5e1bd..26fd2374dfb40afcba5e36f1ad1584234bd7e15f 100644
--- a/test/metabase/models/card_test.clj
+++ b/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/"}
diff --git a/test/metabase/query_processor_test/nested_queries_test.clj b/test/metabase/query_processor_test/nested_queries_test.clj
index adfd312d32a3ef354a2bb5a73c82d538c7528f53..fcd817f818b4bc5d112c02b814fcd63d19606f42 100644
--- a/test/metabase/query_processor_test/nested_queries_test.clj
+++ b/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)))))