Page MenuHomeElementl

Replace check_read_only with specific permissions checks
ClosedPublic

Authored by dgibson on Jul 7 2021, 9:59 PM.
Tags
None
Referenced Files
F2423410: D8750.diff
Wed, Aug 10, 10:40 PM
Unknown Object (File)
Tue, Aug 9, 11:03 PM
Unknown Object (File)
Fri, Jul 15, 3:11 AM
Unknown Object (File)
Fri, Jul 15, 3:11 AM
Unknown Object (File)
Fri, Jul 15, 3:11 AM
Unknown Object (File)
Fri, Jul 15, 3:10 AM
Unknown Object (File)
Fri, Jul 15, 3:10 AM
Unknown Object (File)
Fri, Jul 15, 3:10 AM
Subscribers
None

Details

Summary

Making these checks more granular will help in a future where read only is not a single flag.

Test Plan

BK, existing read-only tests

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 8 2021, 12:04 AM
Harbormaster failed remote builds in B33385: Diff 41176!

This is a really good change - we should more explicitly test it and be a little bit more defensive w/r/t passing strings around.

python_modules/dagster-graphql/dagster_graphql/implementation/utils.py
9–12

I think this util would benefit from a few explicit unit test cases:

- test_check_permission_has_permission()
- test_check_permission_does_not_have_permission()
- test_check_permission_permission_does_not_exist()

Instead of implicitly testing it via our read-only tests. At that point, we could probably even remove some of the read-only tests (unless there's specific behavior they test beyond just whether or not read-only permissions are applied).

10

Smart to generalize this 🤓

15

I think we want to raise a different, more general error class. Perhaps we need a function that maps error class to permission? Or a more general PermissionDeniedError?

python_modules/dagster-graphql/dagster_graphql/schema/roots/mutation.py
148

Given that we're just passing strings in, do we want to add an additional check inside of check_permission to ensure that the permission actually exists? That you didn't typo the permission name.

Alternatively, we could pass in an enum or a constant to make it less prone to typos.

This revision now requires changes to proceed.Jul 8 2021, 1:41 PM
python_modules/dagster-graphql/dagster_graphql/implementation/utils.py
15

yeah my next diff is going to change this to GrapheneAuthorizationError

add test - handling the new error in a separate followup

This revision is now accepted and ready to land.Jul 8 2021, 2:50 PM