Return types from permission classes, instead of raising exceptions.

[nrbnlulu]

trafficstars

Return graphql types from permission classes, instead of raising exceptions.

Feature Request Type

• [ ] Core functionality
• [ ] Alteration (enhancement/optimization) of existing feature(s)
• [x] New behavior

Description

Currently this is what you'll do for permissions:

# copied from the docs. 

class IsAuthenticated(BasePermission):
    message = "User is not authenticated"

    # This method can also be async!
    def has_permission(self, source: typing.Any, info: Info, **kwargs) -> bool:
        return False

@strawberry.type
class Query:
    user: str = strawberry.field(permission_classes=[IsAuthenticated])

The problem is that it is not considered best practice in GraphQL to raise GraphQL exceptions for stuff that you really except.

Therefore a proposal:

@strawberry.type
class AuthOutput:
    success: bool
    code: int
    error: str = None

class IsAuthenticated(BasePermission):
    message = "User is not authenticated"

    def has_permission(self, source: typing.Any, info: Info, **kwargs) -> AuthOutput:
        return AuthOutput(success=True, code=0)

@strawberry.type
class Query:
    user: str = strawberry.field(permission_classes=[IsAuthenticated])

There are 4 ways I can think of to make this work:

1. migrate the return type of the field to Union[Value[str], AuthOutput]

Pros	Cons
No need to override the original return type	Issues when the field returns a scalar
Easier to combine multiple permissions

2. Create a new type with the fields from both the field and the permission class return type.

Pros	Cons
can handle scalars seamlessly	Quite messy and determines stuff to the user upfront

3. Let the user handle it, It is up to the user to determine the return type for both the field and the permission class.

Pros	Cons
Easier on the strawberry side	More code for the user

4. Let users extend a base Permission interface that we will create.

# this would be something that strawberry provides.
@strawberry.interface
class PermissionInterface:
    success: bool

# user code:
class IsAuthenticated(BasePermission):
    @strawberry.type
    class IsAuthenticatedPayload(PermissionInterface):
        success: bool

    def has_permission(self, source: typing.Any, info: Info, **kwargs) -> IsAuthenticatedPayload:
        return self.IsAuthenticatedPayload(success=False, message="Not Authorized")

This would generate an Union of the field that is wrapped with the permission and the IsAuthenticatedPayload type.

Pros	Cons
More elegant	might be unwanted by some users,
We can easily tell if has_permission was successful with the success field.	the success field might be useless in the schema

[nrbnlulu]

@patrick91 what do you prefer from above? or you have another idea?

I tend to go with 4.

[patrick91]

hi @nrbnlulu, I haven't had the chance of taking a proper look at this. I just wanted to ask you if you have seen this proposal from @jkimbo https://github.com/strawberry-graphql/strawberry/issues/920 ?

My worry is that using something like this:

@strawberry.type
class AuthOutput:
    success: bool
    code: int
    error: str = None

class IsAuthenticated(BasePermission):
    message = "User is not authenticated"

    def has_permission(self, source: typing.Any, info: Info, **kwargs) -> AuthOutput:
        return AuthOutput(success=True, code=0)

@strawberry.type
class Query:
    user: str = strawberry.field(permission_classes=[IsAuthenticated])

will make it more difficult to understand what the schema will be, since the permission classes will change the output type

[nrbnlulu]

Yeah #920 would indeed make things easier.

[jkimbo]

FYI my intention with #920 was that it would eventually replace the permission_classes argument with a custom field.

[nrbnlulu]

@jkimbo Your drafted PR is something worth to work on (since you stated it is very old), or should I create a new one?

[jkimbo]

@nrbnlulu I would probably create a new one because it's very out of date.

Most of the changes in that PR are just internal refactoring changes (type -> resolved_type so that it doesn't clash with dataclasses) which could probably just be extracted into a separate PR.

Oh and the bit that I was least sure about (in terms of implementation) was how to handle stacking of custom fields. It's possible that stacking is a bad idea but I think it's important to have some way of composing multiple fields together. It could just be a utility function like @composeFields(fields=[]) rather than trying to detect if the field if stacked on another one.

[nrbnlulu]

partially solved by #2567