Clean up deprecated data permission types #40545

noahmoss · 2024-03-24T19:52:49Z

This PR removes the old data-access and native-query-editing permissions entirely: from the DB and from the code. This ensures that everything is migrated over to view-data and create-queries.

I've also updated a couple small spots that were still using the old permissions which slipped through the cracks, and updated a lot of tests that were still relying on the old permission types.

replay-io · 2024-03-24T20:05:58Z

Status	In Progress ↗︎ 60 / 68
Commit	`1784393`
Results	⚠️ 7 Flaky ✅ 2382 Passed

noahmoss · 2024-03-26T14:06:16Z

Clean up deprecated data permission types #40545 👈
[Feature branch] Split data access and query builder access #39209 : 6 other dependent PRs (#39967 , #40534 , #40825 and 3 others)
[Feature branch] Permissions schema + API change #37473 : 26 other dependent PRs (#37191 , #37377 , #37379 and 23 others)
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @noahmoss and the rest of your teammates on Graphite

noahmoss · 2024-04-01T20:56:24Z

enterprise/backend/src/metabase_enterprise/sandbox/models/group_table_access_policy.clj

+    ;; Remove native query access to the DB when saving a sandbox
+    (when (= (data-perms/table-permission-for-group group_id :perms/create-queries db-id table_id) :query-builder-and-native)
+      (data-perms/set-database-permission! group_id db-id :perms/create-queries :query-builder)))


Here's a spot that still needed to be migrated — we need to ensure that when you save a sandbox, we correctly revoke native access

Nice find, whoops.

noahmoss · 2024-04-01T20:58:15Z

resources/migrations/001_update_migrations.yaml

+      changes:
+        - sql:
+            sql: >
+              DELETE FROM data_permissions where perm_type = 'perms/data-access' OR perm_type = 'perms/native-query-editing';


Here's the new migration

noahmoss · 2024-04-01T20:58:50Z

src/metabase/models/data_permissions.clj

-  For permissions which can be set at the table-level or the database-level, this function will return the database-level
-  permission if the user has it."


Removed this as it is inaccurate

noahmoss · 2024-04-01T21:00:12Z

src/metabase/models/data_permissions/graph.clj

@@ -383,12 +312,34 @@
    (when new-db-perms
      (data-perms/set-database-permission! group-id db-id :perms/create-queries new-db-perms))))

+(defn- update-table-level-view-data-permissions!


These are needed to support setting legacy-no-self-service at the table-level. It shouldn't be set in practice (and should be disallowed by the FE) but the BE should allow it for testing purposes.

johnswanson

🎉 nice!

johnswanson · 2024-04-02T19:28:27Z

e2e/support/commands/permissions/sandboxTable.js

@@ -25,7 +25,7 @@ Cypress.Commands.add(
          [db_id]: {
            "view-data": {
              [schema]: {
-                [table_id]: "segmented",


Oops, good catch.

johnswanson · 2024-04-02T19:31:39Z

enterprise/backend/src/metabase_enterprise/sandbox/api/util.clj

-       (throw (ex-info (str (tru "No current user found"))
-                       {:status-code 403}))))))
+    (if *current-user-id*
+      (let [enforced-sandboxes (enforced-sandboxes-for *current-user-id*)]


Is this functionally different than before?

nope — I think I changed this while trying to fix a different issue and didn't revert it. I'll switch it back.

johnswanson · 2024-04-02T19:32:35Z

enterprise/backend/src/metabase_enterprise/sandbox/models/group_table_access_policy.clj

+    ;; Remove native query access to the DB when saving a sandbox
+    (when (= (data-perms/table-permission-for-group group_id :perms/create-queries db-id table_id) :query-builder-and-native)
+      (data-perms/set-database-permission! group_id db-id :perms/create-queries :query-builder)))


Nice find, whoops.

github-actions · 2024-04-03T13:01:25Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 28b97c8...3c15764.

Notify	File(s)
@alxnddr	frontend/src/metabase/visualizations/components/TableInteractive/TableInteractive.jsx frontend/src/metabase/visualizations/components/TableInteractive/TableInteractive.module.css frontend/src/metabase/visualizations/visualizations/Gauge/Gauge.jsx frontend/src/metabase/visualizations/visualizations/PieChart/PieChart.jsx frontend/src/metabase/visualizations/visualizations/PieChart/PieChart.unit.spec.js
@kdoh	frontend/src/metabase/ui/components/overlays/Popover/index.tsx frontend/src/metabase/ui/theme.ts
@ranquild	frontend/src/metabase/home/components/HomeXraySection/HomeXraySection.tsx

noahmoss changed the base branch from master to enforce-new-permissions March 24, 2024 19:52

noahmoss marked this pull request as ready for review March 24, 2024 19:53

metabase-bot bot assigned noahmoss Mar 24, 2024

metabase-bot bot added the .Team/AdminWebapp Admin and Webapp team label Mar 24, 2024

johnswanson force-pushed the enforce-new-permissions branch from 43db7b5 to d5544d4 Compare March 25, 2024 19:18

noahmoss requested a review from camsaul as a code owner March 25, 2024 19:18

Base automatically changed from enforce-new-permissions to nm-split-data-access-migration March 26, 2024 12:22

noahmoss force-pushed the nm-split-data-access-migration branch from feb5f5a to d32026d Compare March 26, 2024 14:05

noahmoss force-pushed the nm-clean-up-old-data-permission-types branch from 6976adf to ec4295c Compare March 26, 2024 14:06

This was referenced Mar 26, 2024

[Feature branch] Split data access and query builder access #39209

Merged

Allow new view-data and create-queries permissions to be read and written via permission graph #39967

Merged

noahmoss mentioned this pull request Mar 26, 2024

Email to alert admins about legacy-no-self-service permissions #40534

Merged

noahmoss force-pushed the nm-split-data-access-migration branch 2 times, most recently from 950a0b9 to 6e3bda6 Compare March 27, 2024 21:09

noahmoss force-pushed the nm-clean-up-old-data-permission-types branch from ec4295c to 6cd0d75 Compare March 27, 2024 21:09

noahmoss force-pushed the nm-split-data-access-migration branch from 6e3bda6 to f030141 Compare March 28, 2024 13:52

noahmoss force-pushed the nm-clean-up-old-data-permission-types branch from 6cd0d75 to f3a6c7f Compare March 28, 2024 13:52

noahmoss force-pushed the nm-split-data-access-migration branch from f030141 to 7b4db54 Compare March 28, 2024 20:33

noahmoss force-pushed the nm-clean-up-old-data-permission-types branch from 4536e3f to a7e1a7d Compare March 28, 2024 20:33

noahmoss force-pushed the nm-split-data-access-migration branch from 7b4db54 to 438e735 Compare March 29, 2024 13:37

noahmoss force-pushed the nm-clean-up-old-data-permission-types branch from 6509eaa to 3d42ab1 Compare March 29, 2024 15:35

noahmoss mentioned this pull request Mar 29, 2024

fix permissions check when filtering for enforced sandboxes #40825

Merged

noahmoss force-pushed the nm-split-data-access-migration branch from e66246e to 42c32c7 Compare March 29, 2024 17:28

noahmoss force-pushed the nm-clean-up-old-data-permission-types branch from bfe1635 to a352a2d Compare March 29, 2024 17:28

noahmoss mentioned this pull request Apr 1, 2024

New default DB permissions #40869

Merged

noahmoss force-pushed the nm-clean-up-old-data-permission-types branch from a352a2d to 8fac4cd Compare April 1, 2024 20:29

noahmoss changed the base branch from nm-split-data-access-migration to nm-split-data-perms-in-graph-ui April 1, 2024 20:30

noahmoss commented Apr 1, 2024

View reviewed changes

noahmoss requested a review from johnswanson April 1, 2024 21:01

johnswanson approved these changes Apr 2, 2024

View reviewed changes

noahmoss force-pushed the nm-clean-up-old-data-permission-types branch from 8fac4cd to 3c15764 Compare April 3, 2024 13:00

metabase-bot bot added the visual Run Percy visual testing label Apr 3, 2024

noahmoss force-pushed the nm-clean-up-old-data-permission-types branch from 3c15764 to e100732 Compare April 3, 2024 13:20

noahmoss removed the visual Run Percy visual testing label Apr 3, 2024

noahmoss added 16 commits April 4, 2024 09:34

new migration to remove data-access and native-query-editing perms

ab26849

a bunch of cleanup

a60c178

fix up data perm tests

99b5a5c

fix more tests

1e8c3e6

fix more tests

04d1f06

fix cypress tests

fcd2d18

fix sandbox tests

1c99139

fix more tests

93fe0b2

fix an e2e test

2d5ffa8

fix a cypress command to use sandboxed in graph instead of segmented

4ff3420

likely fix last of the BE tests

ffb26eb

revert small unneeded refactor

ed29569

bump ci

f3f541f

fix a sandboxing e2e test

ddf3926

fix newly failing unit test

a15301f

bump ci

1784393

noahmoss force-pushed the nm-clean-up-old-data-permission-types branch from 0df3b50 to 1784393 Compare April 4, 2024 13:34

noahmoss merged commit 49f4a71 into nm-split-data-perms-in-graph-ui Apr 4, 2024
104 checks passed

noahmoss deleted the nm-clean-up-old-data-permission-types branch April 4, 2024 14:54

This was referenced Apr 5, 2024

Perms bugfixes #41104

Merged

Updated Permissions UI - view-data UI E2E tests #41031

Merged

[NOCOMMIT] comment out new migration tests to see if postgres driver tests don't timeout #41153

Closed

New default permissions for groups #41323

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up deprecated data permission types #40545

Clean up deprecated data permission types #40545

noahmoss commented Mar 24, 2024 •

edited

replay-io bot commented Mar 24, 2024 •

edited

noahmoss commented Mar 26, 2024 •

edited

noahmoss Apr 1, 2024

johnswanson Apr 2, 2024

noahmoss Apr 1, 2024

noahmoss Apr 1, 2024

noahmoss Apr 1, 2024

johnswanson left a comment

johnswanson Apr 2, 2024

johnswanson Apr 2, 2024

noahmoss Apr 3, 2024

johnswanson Apr 2, 2024

github-actions bot commented Apr 3, 2024

		For permissions which can be set at the table-level or the database-level, this function will return the database-level
		permission if the user has it."

Clean up deprecated data permission types #40545

Clean up deprecated data permission types #40545

Conversation

noahmoss commented Mar 24, 2024 • edited

replay-io bot commented Mar 24, 2024 • edited

noahmoss commented Mar 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnswanson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 3, 2024

noahmoss commented Mar 24, 2024 •

edited

replay-io bot commented Mar 24, 2024 •

edited

noahmoss commented Mar 26, 2024 •

edited