Skip to content

Commit

Permalink
Check download perms recursively on referenced card-ids (metabase#50307)
Browse files Browse the repository at this point in the history
  • Loading branch information
@noahmoss
noahmoss authored Nov 22, 2024
1 parent 50585b3 commit e9f6c50
Show file tree
Hide file tree
Showing 2 changed files with 100 additions and 37 deletions.
26 changes: 17 additions & 9 deletions ...d/src/metabase_enterprise/advanced_permissions/query_processor/middleware/permissions.clj
View file
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
(ns metabase-enterprise.advanced-permissions.query-processor.middleware.permissions
(:require
[clojure.set :as set]
[clojure.string :as str]
[metabase.api.common :as api]
[metabase.lib.metadata.protocols :as lib.metadata.protocols]
[metabase.models.data-permissions :as data-perms]
[metabase.models.query.permissions :as query-perms]
[metabase.public-settings.premium-features
:as premium-features
:refer [defenterprise]]
[metabase.query-processor.error-type :as qp.error-type]
[metabase.query-processor.store :as qp.store]
[metabase.util.i18n :refer [tru]]))

(def ^:private max-rows-in-limited-downloads 10000)
Expand All @@ -29,19 +32,24 @@

(defmethod current-user-download-perms-level :query
[{db-id :database, :as query}]
;; Remove the :native key (containing the transpiled MBQL) so that this helper function doesn't think the query is
;; a native query. Actual native queries are dispatched to a different method by the :type key.
(let [{:keys [table-ids card-ids native?]} (query-perms/query->source-ids (dissoc query :native))
table-perms (if (or native? (seq card-ids))
(let [{:keys [table-ids card-ids native?]} (query-perms/query->source-ids query)
table-perms (if native?
;; If we detect any native subqueries/joins, even with source-card IDs, require full native
;; download perms
#{(data-perms/native-download-permission-for-user api/*current-user-id* db-id)}
(set (map (fn [table-id]
(set (map (fn table-perms-lookup [table-id]
(data-perms/table-permission-for-user api/*current-user-id* :perms/download-results db-id table-id))
table-ids)))]
;; The download perm level for a query should be equal to the lowest perm level of any table referenced by the query.
(or (table-perms :no)
(table-perms :ten-thousand-rows)
table-ids)))
card-perms (set
;; If we have any card references in the query, check perms recursively
(map (fn card-perms-lookup [card-id]
(let [{query :dataset-query} (lib.metadata.protocols/card (qp.store/metadata-provider) card-id)]
(current-user-download-perms-level query)))
card-ids))
perms (set/union table-perms card-perms)]
;; The download perm level for a query should be equal to the lowest perm level of any table referenced by the query.
(or (perms :no)
(perms :ten-thousand-rows)
:one-million-rows)))

(defenterprise apply-download-limit
Expand Down
111 changes: 83 additions & 28 deletions .../metabase_enterprise/advanced_permissions/query_processor/middleware/permissions_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,8 @@
(testing "Native queries are unmodified"
(is (= (native-download-query) (ee.qp.perms/apply-download-limit (native-download-query))))))

(with-download-perms! (mt/id) {:schemas {"PUBLIC" {(mt/id 'venues) :limited
(mt/id 'checkins) :full}}}
(with-download-perms! (mt/id) {:schemas {"PUBLIC" {(mt/id :venues) :limited
(mt/id :checkins) :full}}}
(mt/with-current-user (mt/user->id :rasta)
(testing "A limit is added to MBQL queries if the user has limited download permissions for a table which
the query references"
Expand Down Expand Up @@ -163,7 +163,7 @@
"A user with limited download perms for a DB has their query results limited"
{:query {:database (mt/id)
:type :query
:query {:source-table (mt/id 'venues)
:query {:source-table (mt/id :venues)
:limit 10}}
:endpoints [:card :dataset]
:assertions {:csv (fn [results] (is (= 3 (csv-row-count results))))}})
Expand All @@ -172,7 +172,7 @@
"An admin has full download permissions, even if downloads for All Users are limited"
{:query {:database (mt/id)
:type :query
:query {:source-table (mt/id 'venues)
:query {:source-table (mt/id :venues)
:limit 10}}
:user :crowberto
:endpoints [:card :dataset]
Expand All @@ -183,24 +183,24 @@
"A user with limited download perms for a schema has their query results limited for queries on that schema"
{:query {:database (mt/id)
:type :query
:query {:source-table (mt/id 'venues)
:query {:source-table (mt/id :venues)
:limit 10}}
:endpoints [:card :dataset]
:assertions {:csv (fn [results] (is (= 3 (csv-row-count results))))}}))

(with-download-perms! (mt/id) {:schemas {"PUBLIC" {(mt/id 'users) :full
(mt/id 'categories) :full
(mt/id 'venues) :limited
(mt/id 'checkins) :full
(mt/id 'products) :limited
(mt/id 'people) :limited
(mt/id 'reviews) :limited
(mt/id 'orders) :limited}}}
(with-download-perms! (mt/id) {:schemas {"PUBLIC" {(mt/id :users) :full
(mt/id :categories) :full
(mt/id :venues) :limited
(mt/id :checkins) :full
(mt/id :products) :limited
(mt/id :people) :limited
(mt/id :reviews) :limited
(mt/id :orders) :limited}}}
(streaming-test/do-test!
"A user with limited download perms for a table has their query results limited for queries on that table"
{:query {:database (mt/id)
:type :query
:query {:source-table (mt/id 'venues)
:query {:source-table (mt/id :venues)
:limit 10}}
:endpoints [:card :dataset]
:assertions {:csv (fn [results] (is (= 3 (csv-row-count results))))}})
Expand All @@ -209,7 +209,7 @@
"A user with limited download perms for a table still has full download perms for MBQL queries on other tables"
{:query {:database (mt/id)
:type :query
:query {:source-table (mt/id 'users)
:query {:source-table (mt/id :users)
:limit 10}}
:endpoints [:card :dataset]
:assertions {:csv (fn [results] (is (= 10 (csv-row-count results))))}})
Expand All @@ -228,7 +228,7 @@
"A user with no download perms for a DB receives an error response"
{:query {:database (mt/id)
:type :query
:query {:source-table (mt/id 'venues)
:query {:source-table (mt/id :venues)
:limit 10}}
:endpoints [:card :dataset]
:assertions {:csv (fn [results]
Expand All @@ -240,7 +240,7 @@
"An admin can always run download queries, even if the All Users group has no download permissions "
{:query {:database (mt/id)
:type :query
:query {:source-table (mt/id 'venues)
:query {:source-table (mt/id :venues)
:limit 10}}
:user :crowberto
:endpoints [:card :dataset]
Expand All @@ -251,23 +251,23 @@
"A user with no download perms for a schema receives an error response for download queries on that schema"
{:query {:database (mt/id)
:type :query
:query {:source-table (mt/id 'venues)
:query {:source-table (mt/id :venues)
:limit 10}}
:endpoints [:card :dataset]
:assertions {:csv (fn [results]
(is (partial=
{:error "You do not have permissions to download the results of this query."}
results)))}}))

(with-download-perms! (mt/id) {:schemas {"PUBLIC" {(mt/id 'venues) :none
(mt/id 'checkins) :full
(mt/id 'users) :full
(mt/id 'categories) :full}}}
(with-download-perms! (mt/id) {:schemas {"PUBLIC" {(mt/id :venues) :none
(mt/id :checkins) :full
(mt/id :users) :full
(mt/id :categories) :full}}}
(streaming-test/do-test!
"A user with no download perms for a table receives an error response for download queries on that table"
{:query {:database (mt/id)
:type :query
:query {:source-table (mt/id 'venues)
:query {:source-table (mt/id :venues)
:limit 10}}
:endpoints [:card :dataset]
:assertions {:csv (fn [results]
Expand All @@ -279,7 +279,7 @@
"A user with no download perms for a table still has full download perms for MBQL queries on other tables"
{:query {:database (mt/id)
:type :query
:query {:source-table (mt/id 'users)
:query {:source-table (mt/id :users)
:limit 10}}
:endpoints [:card :dataset]
:assertions {:csv (fn [results] (is (= 10 (csv-row-count results))))}})
Expand All @@ -295,10 +295,10 @@

(deftest joins-test
(mt/with-full-data-perms-for-all-users!
(with-download-perms! (mt/id) {:schemas {"PUBLIC" {(mt/id 'venues) :full
(mt/id 'checkins) :full
(mt/id 'users) :limited
(mt/id 'categories) :none}}}
(with-download-perms! (mt/id) {:schemas {"PUBLIC" {(mt/id :venues) :full
(mt/id :checkins) :full
(mt/id :users) :limited
(mt/id :categories) :none}}}
(with-redefs [ee.qp.perms/max-rows-in-limited-downloads 3]
(streaming-test/do-test!
"A user can't download the results of a query with a join if they have no permissions for one of the tables"
Expand All @@ -320,3 +320,58 @@
:limit 10})
:endpoints [:card :dataset]
:assertions {:csv (fn [results] (is (= 3 (csv-row-count results))))}})))))

(deftest joined-card-test
(testing "Do we correctly check download perms for queries that involve a join between a table and a card? (#50304)"
(mt/with-full-data-perms-for-all-users!
(with-redefs [ee.qp.perms/max-rows-in-limited-downloads 3]
(mt/with-temp [:model/Card {card-id :id} {:dataset_query {:database (mt/id)
:type :query
:query {:source-table (mt/id :venues)}}}]
(with-download-perms! (mt/id) {:schemas {"PUBLIC" {(mt/id :venues) :full
(mt/id :checkins) :full}}}
(streaming-test/do-test!
"A table joined to a card, both with full download perms"
{:query (mt/mbql-query checkins
{:joins [{:fields [$id]
:source-table (format "card__%d" card-id)
:alias "card"
:condition [:= $checkins.venue_id &card.venues.id]
:strategy :left-join}]
:order-by [[:asc $id]]
:limit 10})
:endpoints [:card :dataset]
:assertions {:csv (fn [results] (is (= 10 (csv-row-count results))))}}))

(with-download-perms! (mt/id) {:schemas {"PUBLIC" {(mt/id :venues) :limited
(mt/id :checkins) :full}}}
(streaming-test/do-test!
"A table joined to a card, with limited download perms for the card, results in a limited download"
{:query (mt/mbql-query checkins
{:joins [{:fields [$id]
:source-table (format "card__%d" card-id)
:alias "card"
:condition [:= $checkins.venue_id &card.venues.id]
:strategy :left-join}]
:order-by [[:asc $id]]
:limit 10})
:endpoints [:card :dataset]
:assertions {:csv (fn [results] (is (= 3 (csv-row-count results))))}}))

(with-download-perms! (mt/id) {:schemas {"PUBLIC" {(mt/id :venues) :none
(mt/id :checkins) :full}}}
(streaming-test/do-test!
"A table joined to a card, with no download perms for the card, results in blocked download"
{:query (mt/mbql-query checkins
{:joins [{:fields [$id]
:source-table (format "card__%d" card-id)
:alias "card"
:condition [:= $checkins.venue_id &card.venues.id]
:strategy :left-join}]
:order-by [[:asc $id]]
:limit 10})
:endpoints [:card :dataset]
:assertions {:csv (fn [results]
(is (partial=
{:error "You do not have permissions to download the results of this query."}
results)))}})))))))

0 comments on commit e9f6c50

Please sign in to comment.