Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add boolean permission support #3451

Closed
wants to merge 33 commits into from
Closed
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
ee8d93c
First pass looking at boolean operators on permissions
Mar 14, 2024
9a8ce6c
Boolean permissions tests
Mar 14, 2024
ea4040b
Update docs
Mar 14, 2024
ecedc67
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Mar 14, 2024
f442f06
Formatting fix
Mar 14, 2024
7f02182
Merge remote-tracking branch 'origin/boolean-expression-permissions' …
Mar 14, 2024
8b08575
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Mar 14, 2024
6300052
Move try catch out of loop!
Mar 14, 2024
796dc99
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Mar 14, 2024
9c9eedc
Few fixes from AI review
Mar 14, 2024
c682525
Merge remote-tracking branch 'origin/boolean-expression-permissions' …
Mar 14, 2024
9694236
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Mar 14, 2024
c38d868
Revert some breaking changes. Move from boolean to composite permiss…
Mar 22, 2024
d455926
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Mar 22, 2024
af0c1d4
Absolutely hadn't finished my changes!
Mar 22, 2024
340f4a0
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Mar 22, 2024
f00a5a6
Merge branch 'strawberry-graphql:main' into boolean-expression-permis…
vethan Mar 22, 2024
7823b38
Fix to undefined Info type
Mar 22, 2024
70e695e
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Mar 22, 2024
ed67c14
Test the async versions of composite permissions, and some linting fixes
Apr 2, 2024
7064f74
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Apr 2, 2024
62ec845
Final linting fixes
Apr 2, 2024
cf53546
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Apr 2, 2024
496cb36
Merge branch 'strawberry-graphql:main' into boolean-expression-permis…
vethan Apr 2, 2024
6226b39
Make sure to test asyncs
Apr 4, 2024
d8c9cce
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Apr 4, 2024
425d739
Of course I missed a single _async
Apr 4, 2024
4a2805a
Merge remote-tracking branch 'origin/boolean-expression-permissions' …
Apr 4, 2024
e1c2b68
refactor: use default has permission syntax for boolean permissions
erikwrede Apr 14, 2024
d338665
chore: adjust type annotation for kwargs
erikwrede Apr 14, 2024
49f9d81
chore: raise error cleanly
erikwrede Apr 14, 2024
9ed6a54
fix: use correct type annotation for kwargs
erikwrede May 3, 2024
29fe77f
chore: adjust type hints
erikwrede May 3, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 23 additions & 0 deletions RELEASE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
Release type: patch

Adds the ability to use the `&` and `|` operators on permissions to form boolean logic. For example, if you want
a field to be accessible with either the `IsAdmin` or `IsOwner` permission you
could define the field as follows:

```python
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"
```
25 changes: 25 additions & 0 deletions docs/guides/permissions.md
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,31 @@ consider if it is possible to use alternative solutions like the `@skip` or
without permission. Check the GraphQL documentation for more information on
[directives](https://graphql.org/learn/queries/#directives).

## Boolean Operations

When using the `PermissionExtension`, it is possible to combine permissions
using the `&` and `|` operators to form boolean logic. For example, if you want
a field to be accessible with either the `IsAdmin` or `IsOwner` permission you
could define the field as follows:

```python
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"
```

## Customizable Error Handling

To customize the error handling, the `on_unauthorized` method on the
Expand Down
161 changes: 140 additions & 21 deletions strawberry/permission.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
Type,
Union,
)
from typing_extensions import deprecated

from strawberry.exceptions import StrawberryGraphQLError
from strawberry.exceptions.permission_fail_silently_requires_optional import (
Expand Down Expand Up @@ -51,17 +52,35 @@
@abc.abstractmethod
def has_permission(
self, source: Any, info: Info, **kwargs: Any
) -> Union[bool, Awaitable[bool]]:
) -> Union[bool, Awaitable[bool], (False, dict), Awaitable[(False, dict)]]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (code_refinement): Consider simplifying the return type for better maintainability.

The method now has multiple return types, which could complicate the handling of its output. Simplifying to a consistent return type, possibly encapsulated within a class or structure, might improve maintainability and readability.

Suggested change
) -> Union[bool, Awaitable[bool], (False, dict), Awaitable[(False, dict)]]:
) -> Union[bool, Awaitable[bool]]:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, this looks like a mistake (a, b) is not a valid type 🤔 Perhaps you need Tuple[Literal[False], dict]?

"""
This method is a required override in the permission class. It checks if the user has the necessary permissions to access a specific field.

The method should return a boolean value:
- True: The user has the necessary permissions.
- False: The user does not have the necessary permissions. In this case, the `on_unauthorized` method will be invoked.

Avoid raising exceptions in this method. Instead, use the `on_unauthorized` method to handle errors and customize the error response.

If there's a need to pass additional information to the `on_unauthorized` method, return a tuple. The first element should be False, and the second element should be a dictionary containing the additional information.

Args:
source (Any): The source field that the permission check is being performed on.
info (Info): The GraphQL resolve info associated with the field.
**kwargs (Any): Additional arguments that are typically passed to the field resolver.

Returns:
bool or tuple: Returns True if the user has the necessary permissions. Returns False or a tuple (False, additional_info) if the user does not have the necessary permissions. In the latter case, the `on_unauthorized` method will be invoked.
"""
raise NotImplementedError(
"Permission classes should override has_permission method"
)

def on_unauthorized(self) -> None:
def on_unauthorized(self, **kwargs: dict) -> None:
"""
Default error raising for permissions.
This can be overridden to customize the behavior.
"""

# Instantiate error class
error = self.error_class(self.message or "")

Expand All @@ -74,6 +93,9 @@
raise error

@property
@deprecated(
"@schema_directive is deprecated and will be disabled by default on 31.12.2024 with future removal planned. Use the new @permissions directive instead."
)
def schema_directive(self) -> object:
if not self._schema_directive:

Expand All @@ -89,6 +111,85 @@

return self._schema_directive

@cached_property
def is_async(self) -> bool:
return iscoroutinefunction(self.has_permission)

def __and__(self, other: BasePermission):
return AndPermission([self, other])

def __or__(self, other: BasePermission):
return OrPermission([self, other])


class CompositePermission(BasePermission, abc.ABC):
def __init__(self, child_permissions: List[BasePermission]):
self.child_permissions = child_permissions

def on_unauthorized(self, **kwargs: dict) -> Any:
failed_permissions = kwargs.get("failed_permissions", [])
for permission in failed_permissions:
permission.on_unauthorized()

@cached_property
def is_async(self) -> bool:
return any(x.is_async for x in self.child_permissions)


class AndPermission(CompositePermission):
def has_permission(
self, source: Any, info: Info, **kwargs: dict
) -> Union[bool, Awaitable[bool], (False, dict), Awaitable[(False, dict)]]:
if self.is_async:
return self._has_permission_async(source, info, **kwargs)

for permission in self.child_permissions:
if not permission.has_permission(source, info, **kwargs):
return False, {"failed_permissions": [permission]}
return True
Comment on lines +157 to +160
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (code-quality): Use the built-in function next instead of a for-loop (use-next)

Suggested change
for permission in self.child_permissions:
if not permission.has_permission(source, info, **kwargs):
return False, {"failed_permissions": [permission]}
return True
return next(
(
(False, {"failed_permissions": [permission]})
for permission in self.child_permissions
if not permission.has_permission(source, info, **kwargs)
),
True,
)


async def _has_permission_async(
self, source: Any, info: Info, **kwargs: dict
) -> Union[bool, (False, dict)]:
for permission in self.child_permissions:
if not await await_maybe(permission.has_permission(source, info, **kwargs)):
return False, {"failed_permissions": [permission]}
return True

def __and__(self, other: BasePermission):
self.child_permissions.append(other)
erikwrede marked this conversation as resolved.
Show resolved Hide resolved
return self

Check warning on line 161 in strawberry/permission.py

View check run for this annotation

Codecov / codecov/patch

strawberry/permission.py#L160-L161

Added lines #L160 - L161 were not covered by tests


class OrPermission(CompositePermission):
def has_permission(
self, source: Any, info: Info, **kwargs: dict
) -> Union[bool, Awaitable[bool], (False, dict), Awaitable[(False, dict)]]:
if self.is_async:
return self._has_permission_async(source, info, **kwargs)
failed_permissions = []
for permission in self.child_permissions:
if permission.has_permission(source, info, **kwargs):
return True
failed_permissions.append(permission)

return False, {"failed_permissions": failed_permissions}

Check warning on line 176 in strawberry/permission.py

View check run for this annotation

Codecov / codecov/patch

strawberry/permission.py#L176

Added line #L176 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably get rid of dictionaries here, maybe make a dataclass instead 🤔

Copy link
Member Author

@erikwrede erikwrede Apr 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea here was to remain as flexible as possible, allowing users to pass context in the future as well. With a dataclass we would be again very static about handling additional permission error context. Maybe a TypedDict can help improve our defaults for this particular case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I'm not sure how context would be used here, is it in the release noted (that I didn't read)? 😅

Copy link
Member Author

@erikwrede erikwrede May 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's internal for now, but it allows us to pass information to on_unauthorized from has_permission. We opted to separate exception raising from permission checking back when implementing this extension due to the fail silently feature.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So basically the context contains "this permission failed out of the list of and / or permissions"


async def _has_permission_async(
self, source: Any, info: Info, **kwargs: dict
) -> Union[bool, (False, dict)]:
failed_permissions = []
for permission in self.child_permissions:
if await await_maybe(permission.has_permission(source, info, **kwargs)):
return True
failed_permissions.append(permission)

return False, {"failed_permissions": failed_permissions}

Check warning on line 187 in strawberry/permission.py

View check run for this annotation

Codecov / codecov/patch

strawberry/permission.py#L187

Added line #L187 was not covered by tests

def __or__(self, other: BasePermission):
self.child_permissions.append(other)
return self

Check warning on line 191 in strawberry/permission.py

View check run for this annotation

Codecov / codecov/patch

strawberry/permission.py#L190-L191

Added lines #L190 - L191 were not covered by tests


class PermissionExtension(FieldExtension):
"""
Expand All @@ -100,8 +201,8 @@

NOTE:
Currently, this is automatically added to the field, when using
field.permission_classes
This is deprecated behavior, please manually add the extension to field.extensions
field.permission_classes. You are free to use whichever method you prefer.
Use PermissionExtension if you want additional customization.
"""

def __init__(
Expand All @@ -117,12 +218,16 @@

def apply(self, field: StrawberryField) -> None:
"""
Applies all of the permission directives to the schema
Applies all the permission directives to the schema
and sets up silent permissions
"""
if self.use_directives:
field.directives.extend(
p.schema_directive for p in self.permissions if p.schema_directive
[
p.schema_directive
for p in self.permissions
if not isinstance(p, CompositePermission)
]
)
# We can only fail silently if the field is optional or a list
if self.fail_silently:
Expand All @@ -132,13 +237,16 @@
elif isinstance(field.type, StrawberryList):
self.return_empty_list = True
else:
errror = PermissionFailSilentlyRequiresOptionalError(field)
raise errror
error = PermissionFailSilentlyRequiresOptionalError(field)
raise error
erikwrede marked this conversation as resolved.
Show resolved Hide resolved

def _on_unauthorized(self, permission: BasePermission) -> Any:
def _on_unauthorized(self, permission: BasePermission, **kwargs: dict) -> Any:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably a mistake - with kwargs you annotate value type (e.g. int instead of dict[str, int], not sure what was intended here 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are no particular operations performed on kwargs values you could annotate them as object, as it's the most narrow type, Any being worse for that use case

if self.fail_silently:
return [] if self.return_empty_list else None
return permission.on_unauthorized()

if kwargs in (None, {}):
return permission.on_unauthorized()
return permission.on_unauthorized(**kwargs)

def resolve(
self,
Expand All @@ -151,9 +259,19 @@
Checks if the permission should be accepted and
raises an exception if not
"""

for permission in self.permissions:
if not permission.has_permission(source, info, **kwargs):
return self._on_unauthorized(permission)
permission_response = permission.has_permission(source, info, **kwargs)

context = {}
if isinstance(permission_response, tuple):
has_permission, context = permission_response
else:
has_permission = permission_response

if not has_permission:
return self._on_unauthorized(permission, **context)

return next_(source, info, **kwargs)

async def resolve_async(
Expand All @@ -164,12 +282,18 @@
**kwargs: Dict[str, Any],
) -> Any:
for permission in self.permissions:
has_permission = await await_maybe(
permission_response = await await_maybe(
permission.has_permission(source, info, **kwargs)
)

context = {}
if isinstance(permission_response, tuple):
has_permission, context = permission_response
else:
has_permission = permission_response

if not has_permission:
return self._on_unauthorized(permission)
return self._on_unauthorized(permission, **context)
next = next_(source, info, **kwargs)
if inspect.isasyncgen(next):
return next
Expand All @@ -179,9 +303,4 @@
def supports_sync(self) -> bool:
"""The Permission extension always supports async checking using await_maybe,
but only supports sync checking if there are no async permissions"""
async_permissions = [
True
for permission in self.permissions
if iscoroutinefunction(permission.has_permission)
]
return len(async_permissions) == 0
return all(not permission.is_async for permission in self.permissions)