[Feature branch] Permissions schema + API change #37473
Merged
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
This stack of pull requests is managed by Graphite. Learn more about stacking. |
This was referenced Jan 9, 2024
noahmoss
added
no-backport
noahmoss
force-pushed
the
perms-refactor-feature-branch
branch
from
4106a6d
to
8d2084f
Compare
noahmoss
force-pushed
the
perms-refactor-feature-branch
branch
from
8d2084f
to
cfc3be1
Compare
escherize
force-pushed
the
perms-refactor-feature-branch
branch
from
c1a657f
to
6a1f6ba
Compare
Closed
|
noahmoss
force-pushed
the
perms-refactor-feature-branch
branch
from
6a1f6ba
to
7310d65
Compare
noahmoss
force-pushed
the
perms-refactor-feature-branch
branch
8 times, most recently
from
eb38571
to
f67bb9f
Compare
First, I added a new function to `models.data-permissions`, `full-database-permission-for-user`, that is equivalent to `full-schema-permission-for-user` but for an entire database rather than a table. For each group, it gets the *least* permissive permission in the database, then coalesces the result like normal. If permissions look like this: ``` id | object | group_id -----+---------------+---------- 156 | /db/1/native/ | 1 155 | /db/1/schema/ | 2 154 | /db/1/ | 3 158 | /db/1/schema/ | 4 159 | /db/1/native/ | 4 ``` DataPermissions end up looking like this: ``` id | group_id | perm_type | db_id | schema_name | table_id | perm_value -----+----------+-----------------------------+-------+-------------+----------+----------------- 809 | 1 | perms/data-access | 1 | | | no-self-service 816 | 1 | perms/download-results | 1 | | | no 825 | 1 | perms/manage-database | 1 | | | no 832 | 1 | perms/manage-table-metadata | 1 | | | no 841 | 1 | perms/native-query-editing | 1 | | | yes 813 | 2 | perms/data-access | 1 | | | unrestricted 818 | 2 | perms/download-results | 1 | | | no 829 | 2 | perms/manage-database | 1 | | | no 834 | 2 | perms/manage-table-metadata | 1 | | | no 845 | 2 | perms/native-query-editing | 1 | | | no 811 | 3 | perms/data-access | 1 | | | unrestricted 817 | 3 | perms/download-results | 1 | | | no 827 | 3 | perms/manage-database | 1 | | | no 833 | 3 | perms/manage-table-metadata | 1 | | | no 843 | 3 | perms/native-query-editing | 1 | | | yes 815 | 4 | perms/data-access | 1 | | | unrestricted 819 | 4 | perms/download-results | 1 | | | no 831 | 4 | perms/manage-database | 1 | | | no 835 | 4 | perms/manage-table-metadata | 1 | | | no 847 | 4 | perms/native-query-editing | 1 | | | yes ``` So to match the existing logic (matching permissions to `/db/:id/` OR BOTH OF `/db/:id/native/` and `/db/:id/schema`) we can just check that the user has `yes` for `native-query-editing`, plus unrestricted `data-access`, for *every* table in the database. Note that we are now filtering tables after selecting them from the database. There is an existing comment in `metabase.api.search` saying: > We get to do this slicing and dicing with the result data because the > pagination of search is for UI improvement, not for performance. We > intend for the cardinality of the search results to be below the default > max before this slicing occurs So I think this ok. Also, the `api.search-test` namespace was the only user of `mt/with-all-users-data-perms-graph`. I swapped out the implementation of that macro with what we've been using in our advanced permissions tests, and deleted the existing macro from our advanced permissions tests, to avoid duplication.
Co-authored-by: Noah Moss <noahbmoss@gmail.com>
* Prohibit creating new data permissions And stop trying to create them. Co-authored-by: Noah Moss <32746338+noahmoss@users.noreply.github.com>
…rating sandboxes (#39111)
noahmoss
force-pushed
the
perms-refactor-feature-branch
branch
from
2664e55
to
e237a66
Compare
|
@noahmoss Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
This was referenced Mar 8, 2024
This was referenced Mar 19, 2024
This was referenced Mar 22, 2024
This was referenced Apr 1, 2024
Merged
This was referenced Apr 9, 2024
Closed
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.
Feature branch for #26918
DO NOT MERGE until all changes are ready and merged into this PR.
This should only contain changes that have already been code reviewed and fully passed CI.