Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check download perms recursively on referenced card-ids #50307

Merged
merged 4 commits into from
Nov 22, 2024
+100 −37
Merged

Check download perms recursively on referenced card-ids #50307

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2ce4dd6
Check download perms recursively on referenced card-ids
noahmoss Nov 20, 2024
caea2dd
test
noahmoss Nov 22, 2024
719484e
remove unecessary dissoc
noahmoss Nov 22, 2024
790d7d7
deanonymize functions
noahmoss Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions ...d/src/metabase_enterprise/advanced_permissions/query_processor/middleware/permissions.clj
View file
Open in desktop
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 @@ -32,16 +35,23 @@
;; 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))
table-perms (if native?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the comment on 35 reads that this query cannot be native, should it be reworded? to something like:

  ;; Remove the :native key (containing the transpiled MBQL) so that this helper function doesn't mistakenly classify
  ;; the query as a native query. Actual native queries are dispatched to a different method by the :type key.

Actually, I don't see why the dissoc matters since a different defmethod will handle :native. If it's clear to you tho please update the comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, the dissoc is unnecessary now, I've removed it

;; 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]
(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-id]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: naming anon functions makes stack traces easier to follow

Suggested change
(map (fn [card-id]
(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
Open in desktop
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}}}
Comment on lines -83 to -84
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed all these since keywords are used in mt/id everywhere else in the codebase

(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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for updating these, much cleaner with keywords.

: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)))}})))))))