metabase icon indicating copy to clipboard operation
metabase copied to clipboard

Migration search API to DataPermissions

Open johnswanson opened this issue 1 year ago • 1 comments

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.

johnswanson avatar Feb 16 '24 19:02 johnswanson

Status Complete ↗︎
Commit cde831ddb277881cd27498373171fc98b5c33f26
Results
⚠️ 2 Flaky
2313 Passed

replay-io[bot] avatar Feb 16 '24 19:02 replay-io[bot]