Check download perms recursively on referenced card-ids #50307
Merged
+100
−37
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
noahmoss
added
the
backport
|
|
noahmoss
force-pushed
the
nm-download-perms-check-cards-recursively
branch
from
1301102 to
caea2dd
Compare
noahmoss
commented
Nov 22, 2024
Comment on lines
-83
to
-84
| (with-download-perms! (mt/id) {:schemas {"PUBLIC" {(mt/id 'venues) :limited | ||
| (mt/id 'checkins) :full}}} |
There was a problem hiding this comment.
Changed all these since keywords are used in mt/id everywhere else in the codebase
escherize
approved these changes
Nov 22, 2024
| (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) |
There was a problem hiding this comment.
thanks for updating these, much cleaner with keywords.
| @@ -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? | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good point, the dissoc is unnecessary now, I've removed it
| table-ids))) | ||
| card-perms (set | ||
| ;; If we have any card references in the query, check perms recursively | ||
| (map (fn [card-id] |
There was a problem hiding this comment.
nit: naming anon functions makes stack traces easier to follow
Suggested change
| (map (fn [card-id] | |
| (map (fn card-perms-lookup [card-id] |
github-automation-metabase
pushed a commit
that referenced
this pull request
Nov 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #50304
In #49364 I changed
query->source-table-idsto return sets oftable-idsandcard-ids. In that PR, I adjusted download perm enforcement to require full native perms if anycard-idswere returned from that function, since we previously hadn't been considering referenced cards at all for download perms. (i.e. we were being overly permissive, allowing downloads even for queries joined to cards without download perms).This was too restrictive, though, and ended up blocking downloads for some queries that should be allowed. The more correct behavior is to check download perms recursively for any referenced card IDs in the query, and go with the least-permissive value from that entire set.
I've fixed the logic and added a test to catch this case.