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
Branch
rmreadonly
Lint
Lint Passed
Unit
No Test Coverage

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