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
Permission System #79
Comments
Also note |
I liked of your suggestion to implement a permission system. Please, could you look this other issue? graphql-python/graphene#385 At this moment I am interested and available to do this, what do you think about we plan this architecture and works together in this? Thank you! |
@pizzapanther perhaps we could use an array of functions or strings. If listed as strings, we can check built in django permission code names and verify context.user against it. Thoughts? |
@valdergallo @romulorosa @rsalmei @nickhudkins @pizzapanther @ekampf @barakcoh In our case, we have over a hundred objects that extends Assuming that context.user has methods called
In this example above, the custom field Important: The not allowed fields for user should not be displayed in introspection query too, which would help |
@nickhudkins Like the strings and functions idea. That makes it simple and very extensible. @fmartins-in-loggi your implementation looks a little complex. I'd rather write a small function than use so many configurations, but that is my bias and we could probably start a flame war about it. Also I think we could start with simple functions and then implement your config example as a layer on top of it. I like to remember "code always wins". I was going to implement this and just wanted to get some feedback. But it looks like others are ready to implement. So who wants to implement? |
Another approach to this is to use the Relay convention of a top level public only has certain models, and a limited number of fields on those models. admin has many more models and all fields on all of them. So if the request.user is not an admin, then 'admin' returns None and that is the end of that. No need to do permission checks on any nodes below. No need to write complicated runtime checks for field level view permissions. Much easier to read and write the code. The other nice thing is that if you write some graphql to query on public and you access a private field then igraphql or your clever graphql compiler can red flag it immediately. |
But wouldn't this mean you have to create two types of nodes for each model you have? One for admin and one for public? |
Yes if the nodes have very different fields, different filters etc. If you want the exact same then you should be able to just include the DjangoObjectType twice. I'm not sure if you can do that because graphene-django has a registry that may (or may not) prevent a fields from being added twice. Maybe you can just subclass to copy it. I will see how it goes. I would think it is much cleaner to have top level fields for roles. eg. public / user / staff. This would let you easily see exactly what is public in igraphql. The code should also be easier to read and write. Of course if you need to do row level permission checks then that is runtime only and the schema won't help you. |
@crucialfelix actually there was a recent pr adding docs showing that you can have two DjangoObjectTypes with the same model class |
For mutations it would be nice if there was an permissions array on the mutation class similar to Django rest This would be an array of functions that looked at context and args and returned true or false Thoughts? |
Can we use inheritance for authorization? (related to #26) when we want resolve class ProfileNode(DjangoObjectType):
class Meta:
only_fields = ()
model = Profile
interfaces = (relay.Node, )
def resolve_type(self, context, info): # or some function like this. i don't know :)
if check_some_perm(self, context.user):
return PrivateProfileNode
return PublicProfileNode
class PublicProfileNode(ProfileNode):
class Meta:
only_fields = ('name', 'age')
class PrivateProfileNode(ProfileNode):
class Meta:
only_fields = ('name', 'age', 'secret_1', 'secret_2') |
Late to the party... and I'm not actually using django (I'm use sqlalchemy), but here are my thoughts on how to approach the problem:
I'd be interested in hearing your guys approach to these points. |
Attached is a version of ACLMiddleware I'm experimenting with. Note that my implementation has:
Those implementation details aren't Django specific, but I think this problem is more general than the Django, and this is where the conversation is happening. As for constraints from previous comments:
class ACLMiddleware:
def resolve(self, next, root, args, context, info):
result = next(root, args, context, info)
graphene_type = getattr(info.return_type, 'graphene_type', None)
if not isinstance(graphene_type, SQLAlchemyObjectTypeMeta):
return result
permission = getattr(graphene_type._meta, 'permission', None)
if not permission:
return result
authenticated_user = context.get('authenticated_user', None)
principals = _User.get_principals(
context['session'],
authenticated_user,
)
return result if permits(result.value, principals, permission) else None |
I would agree that with the way Graphene is currently structured, the middleware is probably the best place to do permissions checking. I am trying to understand your code @dfee , does SQLAlchemyObjectTypeMeta allow you to put permissions on specific nodes and check against them? |
No, but with an PR I made a while back, you can subclass |
I need permission system for graphene-django, similar as in django-rest-framework. In django-rest-framework in api view class we can add: class SomeObjectNode(DjangoObjectType):
class Meta:
model = SomeModelClass
interfaces = (graphene.Node, )
permission_classes = (SomePermissionClass, )
class OurQuery(graphene.AbstractType):
some_object = graphene.Node.Field(SomeObjectNode)
all_some_object = DjangoFilterConnectionField(SomeObjectNode) -Fields for mutations, for example: class DeleteSomeObject(graphene.ClientIDMutation):
permission_classes = (SomePermissionClass, )
class Input:
id = graphene.String()
@classmethod
def mutate_and_get_payload(cls, input, context, info):
SomeObject.objects.get(pk=from_global_id(input.get('id'))[1]).delete()
return DeleteSomeObject()
class OurMutation(graphene.AbstractType):
delete_some_object = DeleteSomeObject.Field() Anyone is implementing this functionality now? If nobody, i can try to do it. Anyone has any advices, comments, etc for me? |
Here's a decorator for adding auth to a mutation: https://gist.github.com/crucialfelix/cb106a008a7a62bdab4a68e1b4ab7a3c It is even easier than your example:
You can do something similar with queries and individual |
Thanks, It could be very usefull for me :) |
Anyone working on this? |
There is a 2.0 version in development: https://github.com/graphql-python/graphene-django/tree/2.0 I have not yet looked at it. There is a new resolver API |
By chance, did you try the top-level viewer field approach to permission handling in the end? Seems conventional, do you think it works out with graphene-django? |
That is what I do. I even have a level below that. query Listings {
viewer {
id
agent {
id
listings(
first: 50
after: $after
visibility: $visibility
category: $category
categoryId: $categoryId
search: $search
section: $section
sr: $sr
aptids: $aptids
) {
edges {
node {
id
pk
// etc
}
}
}
}
}
} Agent (which is a logged in staff member) is checked only once and I know that everything inside of that are fields on A client (a logged in user) or anon has different fields with different security and visibility requirements. I'm a bit divided on whether it was a great way to lay it out. There is a bit more overhead on the client with the extra nesting, but I guess it's always clear what is being accessed. |
@crucialfelix Seems like a sane way to do it, I might actually adopt the extra nested call too since I’m dealing with staff and non-staff roles. Currently figuring out how to implement this approach with Graphene+Django being a bit of a noob, authentication docs aren’t going into much detail on how to add root calls populated based on context like current user. From reading Facebook’s early docs it seemed like the most graphql-y way to go about it though. The decorator you posted above might be my backup plan so thanks for that (I guess I can use it for more than mutations, so I’ll just keep two endpoints and for one of them hide key methods behind |
@jakubste I'm not convinced that trying to build authentication into graphene-django is a good idea. The recommended way of dealing with authentication is to build it into your resolvers (ref: https://graphql.org/learn/authorization/) so I would recommend following that approach. |
First of all: we're talking about authorization, not authentication. But on the topic: I'm currently at place where I have to implement API that is accessible by admins and normal users. Both have access to the same models, but we don't want to show users every field. As an example imagine having user object, which obviously user should have access to, but we don't want to show him, for example, notes about him made by admins. With one field placing it in resolver is ok, but with many models with many fields, it's a lot of copy-paste code, just to check I think that DRF deals with that problem by using different serializers for different users. Unfortunately, I can't see an easy way to copy such behaviour to graphene. I really like the idea presented by @bk-equityzen in #485 and we are probably going to reimplement his proposal in some way. I noticed that it's a common problem and mitigating it by simply saying "build it into your resolvers" doesn't seem like a proper solution to a common problem. |
@jkimbo I think that's quite the opposite actually, unless I'm misunderstanding this:
Then right after:
I found this article that may help on this question: Authentication and Authorization in GraphQL |
I know that GraphQL authors approach is just "give it to the business logic layer". I just can't find any neat solution that works with Django. If we are really going this way, that means that there should be something in between DjangoObjectType and Model that knows about restrictions. Then again |
I don't know django, but I'm pretty sure you would have a way to do it. For Starlette I posted a question about that kind of problem (not having the opportunity to patch how requests are handled). Basically you would intercept calls You could then simply add a decorator to that method so authorization checks are performed before execute. Again, I'm not sure that's the best solution, but at least you can plug/unplug security checks on a single place. edit looking at how django handles this it seems like calls to |
@jakubste sorry you're right we are talking about authorization not authentication. That was a typo. To restrict access at an object level using the Restricting at a field level is harder and currently the only way to do it is by having customer resolvers. I am open to PRs that implement a field level pre-processing API. I just don't know what that looks like or how to implement it efficiently. @jakubste Regarding your uses case of having a single API handle both users and admins: I would suggest splitting the API into 2 separate ones because it makes everything a lot simpler (this is what I did at a previous company). You can share types between them if you like but having separate schemas means you can have fields that only make sense to admins (like the admin notes field) and you can drastically simplify your authorization logic. The user facing API just needs to handle the case where the current user can only view their data and the staff API can bail our early if the current user isn't a staff user. |
Is it possible to restrict access to specific ObjectTypes? Like if I had multiple object types that access If it's not possible, the next best thing in my opinion would be to have the possibility to use django permissions on the fields without having to use a custom resolver. Maybe something like this:
|
@sbernier1 maybe you could try to define a field class for this that contains a custom resolver function in order to centralize the permission checking class MyQueryField(graphene.Field):
def __init__(self, *args, **kwargs):
kwargs["resolver"] = kwargs.get("resolver", self.__class__.resolver)
return super().__init__(MyType, *args, **kwargs)
@staticmethod
@some_permission_decorator
def resolver(parent, info, **kwargs):
# ... or maybe some permission checking code here
return MyModel.objects.get(parent_pk=parent.pk) and then inside the object maybe something like my_field = MyQueryField() I hope this makes sense, works and/or is remotely what you are looking for 😄 |
I ended up overriding the default resolver like this:
It still uses the default |
@sbernier1 I think overriding the default resolver is the way to go and I like your example. I think creating something like an def staff_required(user, info):
if not info.context.user.is_staff:
return False
return True
class MyType(AuthDjangoObjectType):
class Meta:
model = User
fields = ("first_name", "last_name", "email")
class Auth:
fields = {
"email": [staff_required],
} What do you think? Also I'm going to reopen this issue because we should at least have an official answer to this question. |
looks great! If it was possible to access the querySet in the auth function (staff_required in this case), I think it would cover the use cases I can think of. I want to access the querySet to check if the user is allowed to access specific rows. It would also be nice if we could declare a default permission function in the auth class. |
@sbernier1 why would you need to access the queryset? Conceptually the ObjectType doesn't have access to how it was resolved so I don't think it would be possible anyway. You could do it on the parent type though? +1 on being able to declare a default permission function. |
maybe I used the wrong expression, sorry. In the resolvers, be it the default resolver or specific resolvers, we have access to the data. I want this because, for example, if I have a db containing books and book drafts, and I want only the author of the book to access the draft. With resolvers I can access the data to check that info.context.user.id == book.author.id . |
Oh yeah thats the first parameter to the permission function. The def staff_required(user, info):
if not info.context.user.is_staff:
return False
return True |
I suggest everyone in this thread see this video after 38:30 until 41:15 Authenticationwho you are? It should be done in the transport layer (HTTP). and Django context has user: def resolve_items(parent, info):
info.context.user # it can be anonymous or authenticated user Authorizationdo you have permission? it should be done in business logic. example: def resolve_items(parent, info):
return get_all_items(info.context.user) what is In my opinion, In Django, interactors are allowed to know about models and query sets. interactors are defined in a separate file. graphql, Django view/templates, rest, ... can use them as core logic. def get_all_items(user):
if not user.is_authenticated:
return Item.objects.none()
return item.objects.filter(owner=user) graphql layer mission is about translating graphql request data into interactor call interactor:
graphql:
|
If I do not have legacy stuff and only access data through graphql, is it really that bad to put authorization in the resolvers? if I have to put it in the business layer, the only thing it changes for me is that I have to another layer of functions. Thanks |
Any update on this |
I quite like the way rails does it. https://graphql-ruby.org/schema/limiting_visibility.html Need to look at an implementation in graphene for a similar approach. |
Any way to reengage the discussion? |
Is it possible to check for permissions on a generic query that uses DjangoFilterConnectionField to list all records of a certain Type. For example I want only super admin to be able to use list-all query that uses DjangoFilterConnectionField. |
@molt-coder you could implement that currently if you follow the pattern at: https://docs.graphene-python.org/projects/django/en/latest/authorization/#user-based-queryset-filtering Something like: def resolve_my_posts(self, info):
# context will reference to the Django request
if not info.context.user.is_authenticated:
return Post.objects.none()
elif info.context.user.is_superuser:
return Post.objects.all()
else:
return Post.objects.filter(owner=info.context.user) |
@jkimbo Is there any update on adding the |
I would like to add a permission system but want to some feedback on the API before I implement.
You would have two options and I'm proposing to add both:
Option 1: Custom queryset method
This option would let you overwrite how a queryset is filtered.
Option 2: Permissions List
This option would setup a Meta API to use to define permissions
If these look like good APIs then I'll implement.
The text was updated successfully, but these errors were encountered: