Boolean expression permissions #3408
Conversation
|
Thanks for adding the Here's a preview of the changelog: Adds the ability to use the import strawberry
from strawberry.permission import PermissionExtension, BasePermission
@strawberry.type
class Query:
@strawberry.field(
extensions=[
PermissionExtension(
permissions=[(IsAdmin() | IsOwner())], fail_silently=True
)
]
)
def name(self) -> str:
return "ABC"Here's the tweet text: |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @vethan - I've reviewed your changes and they look great!
General suggestions:
- Consider adding more detailed examples in the documentation to cover complex use cases and best practices for using boolean expressions with permissions.
- Ensure that the performance implications of the new boolean permission checks are well understood and documented, especially for applications with high throughput.
- Review the exception handling strategy, particularly the use of
BaseException, to ensure that it aligns with best practices and does not catch more than intended. - Clarify the behavior and implications of the
fail_silentlyoption in the documentation to prevent any unexpected behavior from client perspectives.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3408 +/- ##
========================================
Coverage 96.51% 96.51%
========================================
Files 511 511
Lines 32965 33209 +244
Branches 5482 5517 +35
========================================
+ Hits 31815 32051 +236
- Misses 917 921 +4
- Partials 233 237 +4 |
…into boolean-expression-permissions # Conflicts: # strawberry/permission.py
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
CodSpeed Performance ReportMerging #3408 will not alter performanceComparing Summary
|
…into boolean-expression-permissions
for more information, see https://pre-commit.ci
|
Do we need to have new public methods |
This may just be my lack of pythonic syntax knowledge creeping up, but I'm not sure if I can return the Union from the parent class without the knowledge of the children's return type. (Aka, does the AND need to await to figure out it's own permission or not) |
strawberry/permission.py
Outdated
| def resolve_permission_sync(self, source: Any, info: Info, **kwargs: Any) -> None: | ||
| if self.has_permission(source, info, **kwargs): | ||
| return None | ||
| else: | ||
| return self.on_unauthorized() | ||
|
|
||
| async def resolve_permission_async( |
There was a problem hiding this comment.
Splitting into sync and async is a breaking change, I'd like to avoid this. We can think about a different solution :)
There was a problem hiding this comment.
Yeah, it seemed a bit gross, very open to a better solution. As mentioned, might be my lack of Python Syntax knowledge here!
There was a problem hiding this comment.
@vethan Sorry for that, but could you probably adapt some of the stuff from my implementation on discord? 🤔
https://discord.com/channels/689806334337482765/741575032257511485/1217820662245101609
If I remember correctly it should work with both async and sync by inspecting whenever any of child permissions are async
There was a problem hiding this comment.
Also if applied operator is the same as the one in PermissionHolder I think you could avoid nesting them too by returning something like
PermissionHolder(operator=..., permissions=[*self.permissions, other])There was a problem hiding this comment.
Your implementation was great for has_permission, but then there was no way to work through on_unauthorised correctly unfortunately :/ Need to split the logic a bit for that
There was a problem hiding this comment.
Yeah, on_unauthorised is a bit hard to implement because we don't know which permission didn't succeed without checking them again (and we can't even do that)
…ons as base class. Branch on composite permissions to handle them differently!
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…into boolean-expression-permissions
strawberry/permission.py
Outdated
|
|
||
|
|
||
| class CompositePermission(BasePermission, abc.ABC): | ||
| child_permissions: List[BasePermission] |
There was a problem hiding this comment.
Remove this annotation, this basically lies to type checker that this property would always be present on a class.
Your __init__ is enough to provide that type information
strawberry/permission.py
Outdated
| for index, permission in enumerate(self.child_permissions): | ||
| if not permission.has_permission(source, info, **kwargs): | ||
| return False, index | ||
| return True, 0 |
There was a problem hiding this comment.
Maybe it would be a better idea to return failed permission and not it's index?
strawberry/permission.py
Outdated
| True | ||
| for permission in self.permissions | ||
| if iscoroutinefunction(permission.has_permission) | ||
| True for permission in self.permissions if permission.is_async | ||
| ] | ||
| return len(async_permissions) == 0 |
There was a problem hiding this comment.
| return len(async_permissions) == 0 | |
| return all(not permission.is_async for permission in self.permissions) |
|
Hey everyone, I worked on streamlining this with @patrick91 over the last days. New version is in #3451. Please comment there! @vethan thanks for the solid ground work. I just reduced the complexity a bit in some places but essentially it's your PR. The only missing thing is some schema directive support. We have decided to deprecate the old way of handling Schema Directives in favor of a new |
Co-authored-by: Doctor <[email protected]>
|
@erikwrede maybe you can push those commits on this PR? do you have permission? 😊 |
…ssion-permissions
|
/pre-release |
Pre-release👋 Pre-release 0.229.2.dev.1715881453 [64bcbad] has been released on PyPi! 🚀 poetry add strawberry-graphql==0.229.2.dev.1715881453 |
…olean-expression-permissions
|
Just pushed some additional fixes from local regarding context passing with multiple nested boolean permissions |
|
/pre-release |
Adding the ability to perform boolean expressions on permissions using the permission extension
Description
Allows permissions to be supplied in the form
IsAuthed() & (ControlsAccount() | IsAdmin()), by overriding the implementation of permissions onBasePermissionTypes of Changes
Issues Fixed or Closed by This PR
Checklist