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

Authorization for Node queries #385

Closed
barakcoh opened this issue Dec 8, 2016 · 24 comments
Closed

Authorization for Node queries #385

barakcoh opened this issue Dec 8, 2016 · 24 comments
Labels

Comments

@barakcoh
Copy link

barakcoh commented Dec 8, 2016

Hi

It would be helpful to have a standard way to do authorization in graphene, specifically for Node Relay queries. In non-Node queries it's easy enough to enforce authorization in resolve_XXX (mentioned in #94) but for Node we need a hook to inject type-specific and argument-specific checks.

We're most using @ekampf 's graphene-gae so in theory we could inherit and override NdbObjectType.get_node but that would require doing a similar override in for non-ndb types.

Any plans to support authorization directly in graphene?

@nickhudkins
Copy link

Hey @barakcoh I was concerned about this as well. Personally, I am not sure that it actually impacts me as I protect data at the resolver level, since if I have defined it as a relayNode, I have explicitly asked to be able to resolve it through the node resolver.

Can you elaborate, or provide an example of when you would want a node resolver to be protected, and not sub fields of said node? Is this mainly a concern of leaking data by being able to guess types / ids and construct node queries to find data?

@fmartins-zz
Copy link

@barakcoh @nickhudkins

Hi, I am interested in this topic. Anyone of you already did some draft about this?

In first hand, I had the impression that a model structured based in @annotations is great. What do you think about this?

Please, check this another issue about this some area of knowledge - graphql-python/graphene-django#79

Thank you!

@ekampf
Copy link
Contributor

ekampf commented Jan 13, 2017

I think adding a permission array of strings to the model's Meta is a good idea.
We can then write a middleware that verifies current user (from context) against the current model's permissions...

@nickhudkins
Copy link

@ekampf That sounds fantastic, This seems django specific to me, so I think we can probably close this and track in graphql-python/graphene-django#79

@ekampf
Copy link
Contributor

ekampf commented Jan 13, 2017

Might be too high-level though...
For example, it will allow you to seperate Admin models\fields from user models\fields but wont allow you differentiate between users (say there's a user resource but a user can only access his own resources)

Maybe its best to add a class method to the model:

class Person(graphene.ObjectType)
    ...

    @classmethod
    def is_authorized(cls, user):
         return True

and then call that from a middleware...

@nickhudkins
Copy link

This would certainly keep it from getting overly specific. Are you picturing user getting passed to is_authorized from context?

@ekampf
Copy link
Contributor

ekampf commented Jan 13, 2017

yes

@nickhudkins
Copy link

This sounds terrific then. Do you want to start on this, or I can and submit a PR? Either is fine with me 👍

@ekampf
Copy link
Contributor

ekampf commented Jan 13, 2017

Out graphql handler does the authentication and sets context['current_user'] before passing the query to the GraphQL execution.
So the current user is accessible via context.
We can do very specific authorization code inside the resolvers themselves but a nicer way would be to generalize it via a middleware.

@ekampf
Copy link
Contributor

ekampf commented Jan 13, 2017

@nickhudkins I think this is very implementation specific, what would you want such a PR to contain?

@ekampf
Copy link
Contributor

ekampf commented Jan 13, 2017

maybe we can make the middleware generic...

@nickhudkins
Copy link

I am thinking that there would be two pieces: 1.) Provide a "UserMiddleware" that provides a hook to simply do whatever you need to get a user. The middleware would place that user on context, and the rest would be ensuring that object resolvers (if is_authorized is defined) and get_node would be respected, and otherwise return None.

@ekampf
Copy link
Contributor

ekampf commented Jan 13, 2017

  1. The middleware cant put the user in context, it supposed to get it as input. There are 2 ways to do it:
    1.1. Set it in the handler before executing the query
    1.2. Instead of using the context set it in the middleware's constructor when you execute the query:
class AuthorizationMiddleware(object):
  def __init__(self, user):
    self.user = user

  def resolve(self, next_step, root, args, context, info):
    ... do authorization logic ...


...

        result = schema.execute(query,
                                operation_name=operation_name,
                                variable_values=variables,
                                context_value=dict(),
                                middleware=[AuthorizationMiddleware(current_user)])

@ekampf
Copy link
Contributor

ekampf commented Jan 13, 2017

as for the actual logic for the middleware I think there are 2 levels of authorization.
First level is permission token based - can the current user even access this model\field?
For this its worth looking at what Reindex and Scaphold doing:
https://www.reindex.io/docs/security/authorization/
https://scaphold.io/docs/?__hstc=85828166.8dd772556de03c0650938130c7a38a29.1484336017404.1484336017404.1484336017404.1&__hssc=85828166.1.1484336017405&__hsfp=1917043275#permissions-authorization

Second level of authorization is - given a resolved value, check if user is allowed to get it.
Something like:

def resolve(self, next_step, root, args, context, info):
    ... do authorization logic ...
            result = next_step(root, args, context, info)
        return result.then(
            lambda resolved: self.__check_authorization(resolved, root, args, context, info),
            lambda error: self.__handle_error(metrics, error)
        )

def __check_authorization(resolved, root, args, context, info):
   resolved_type = type
   if hasattr(resolved_type, "check_authorization") and callable(resolve_type.check_authorization):
      resolve_type.check_authorization(self.user, resolved)

@dfee
Copy link
Member

dfee commented Jun 6, 2017

(I shared an implementation on graphql-python/graphene-django#79 (comment), but it might make more sense here…)

Attached is a version of ACLMiddleware I'm experimenting with.

Note that my implementation has:

  • get_principals returns principals (i.e. ['group:Admin', 'user:1234'])
  • permits is a function that returns True or False with arguments (resource typically an instance of the model, principals as above, permission such as 'edit').

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:

  1. this doesn't support permissions per field (though Meta could pretty easily be extended to support that),
  2. when querying node, we don't (can't?) get access to the ObjectType so we have to re-implement the get_node method on ObjectType classes.
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

@dfee
Copy link
Member

dfee commented Jun 6, 2017

One consequence of the approach I took above is that edge nodes that a user doesn't have access to will be returned as None.

I.e.

{
  "data": {
    "node": {
      "id": "VXNlcjp5QnJWTXd6bVV0SFRVM245MmRuN3M1",
      "memberships": {
        "totalCount": 2,
        "edges": [
          {
            "node": null
          },
          {
            "node": null
          }
        ]
      }
    }
  }
}

@ProjectCheshire
Copy link
Member

ProjectCheshire commented Dec 29, 2017

I was working on something similar, and thought someone could refine/make use of, My use case is that I don't even the queries/mutations to appear, (I'm using directives for field level)

BaseQuery(graphene.ObjectType):
    pass

Query_001(BaseQuery):
scopes = ['admin']
queryThing = List(SomeType, thing=SomeTypesInput(), resolver=thisthingsresolver)

Query_002(BaseQuery):
scopes = ['user']
queryanother = (same routine as above, as standard)

USER_SCOPE = 'user' (Get this from the logged in user, which means generating the schema post authentication)
all_queries = [Query_001, Query_002]
filered_queries = [i for i in all_queries if USER_SCOPE in i.scopes]
ROOT_QUERIES = type('Query', tuple(filtered_queries), {})

Then

MYSCHEMA = graphene.Schema(query=ROOT_QUERIES)

I haven't extensively tested it, however, or tested performance. I have no idea if this is remotely legit, but it looks like schema level decorators to filter out mutations/queries like we can do with fields with directives is nowhere to be found (yet - though some people have been making proposals for it)

@dfee
Copy link
Member

dfee commented Dec 30, 2017

There is continued activity here, but after working heavily with graphene over the last few months, it seems like the right way to do this is to use middleware.

If you want permissions on your models (ObjectTypes) then why not just subclass ObjectTypeOptions and ObjectType (specifically __init_subclass_with_meta__) and add that as an attribute? Then you'll just have to watch for those objects to come by in middleware (usually watching on info.return_type.graphene_type.

Or for a more complex authorization architecture, don't define permissions as strings, but add a callable (class, function, or instance – that defines __call__) that receives root and info (which contains your context) and make determinations there.

For an example, see this: https://github.com/graphql-python/graphene/blob/master/graphene/tests/issues/test_425.py

@ProjectCheshire
Copy link
Member

@dfee For permissions on models, that is great :) I hadn't seen that before.

What I was referring to was a dynamic generation of the schema to filter the queries / mutations / etc.

Would it work the same way?

More complex oyvey. Actually, I'm trying to get away from the super complex, which I was gravitating to via neomodel and doing scopes on structuredrels and the like and defining roles as nodes(in the neo4j sense) unto themselves and checking for the rel vs the string.

@dfee
Copy link
Member

dfee commented Dec 31, 2017

@ProjectCheshire I was actually responding to the top and hadn't read all the way down. Anyways, yeah, you could definitely generate a dynamic schema per user. That's a creative idea.

But no, that'd be entirely different. You're going to have a bit more code overhead on your part, but it's definitely doable. You'd likely want to cache the Query, Mutation, Subscription (root types) per user (perhaps using pickle), as it'd become pretty expensive to re-generate it on each request.

Funny enough, the codebase I've been writing tests on to release actually does something very similar to generating dynamic schemas. Take a look at the abstract-factory pattern for software engineering. You'll ultimately create Factory classes that generate the graphene.ObjectTypes / InputObjectTypes that you'll need and supply those to a schema on demand.

edit: just checked, and indeed schemas are not pickleable. It errors out because the actual __Schema uses lambdas which are not pickleable.

@ProjectCheshire
Copy link
Member

@dfee bollocks. I have dynamic schema generation working (I want to run some tests on performance) using the method I outlined. I have a base mutation/query/subscription class I inherit the rest from, so I can actually access them all through Base.subclasses()

Given that list, I then filter them (using the primitive scopes method I described with strings, heh - each base has default scopes) and then the filtered list becomes what the schema is built from using type()

Currently I have it under the RootValue(GraphqlView) class so when I resolve the get root value, I can generate the schema. I ensure that I have a user since I'm using flask_classful decorators and login_required. At load time, I use a default schema with dummy operations (eg returning a random cat from https://placekitten.com/)

What Id love to see is schema level decorators as mentioned in https://github.com/apollographql/graphql-decorators and some other places.

There is a long (but really interesting) discussion of filters on introspection / schema at graphql/graphql-js#113 which I think? resolved to facebook sorts saying 'don't do it'

Hm, I wonder if caching the schema in the session or flask.g would work (Note: I'm not advanced with web whatnot so pardon my naivete. I mostly work on embedded and found graphql to be pretty amazing for some of the edge/cloud/fog work I'm doing. I'm working on a proto port of using graphql as the api layer inside EdgeXFoundry)

@ProjectCheshire
Copy link
Member

So.. I think I'm going back to my original what-feels-heavy implementation, but using some concepts from your implementation above with Permits / resources (at least for the models portion. I'm still waffling on schema filtering)
My stack is Neo4j -> neomodel -> graphene, so I'm going to set up my roles as nodes so i can use the neomodel relationship search capability by associating users to them, and set up permits on the role nodes, either the class or a specific instance. Similarly with the user, so the get_permits for a given user will be the combination of the permits on their roles and their own permits.

Still sketching it out, and I need to verify how neomodel/neo4j will store it. The permit may become simply a structuredrel between a user node and specific instance nodes that are outside their role set of permits (eg, exceptions, or a user cannot edit all nodes of type x but can edit this specific node of type x

So far, graphql plays really nicely with neomodel, so I'm pretty confident I can stuff this then into the AuthorizationMiddleware as suggested above.

Thanks for the commentary and pointers!

@stale
Copy link

stale bot commented Jul 29, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jul 29, 2019
@stale stale bot closed this as completed Aug 5, 2019
@cansin
Copy link

cansin commented Jun 8, 2020

@dfee thank you for the suggestion. That helped me a lot. For reference for people that might end up here via search:

I was relying on https://github.com/taoufik07/django-graphene-permissions to handle permissions mostly. But their has_object_permission was not firing on list of objects. Introducing below middleware helped filter out any such objects from the end result:

from django_graphene_permissions import PermissionDenied, check_object_permissions


class AuthorizationMiddleware(object):
    """
    Make sure to filter out objects where the info.context (e.g. the authenticated user)
    does not have the authorization to query.
    """

    def resolve(self, next, root, info, **args):
        result = next(root, info, **args)

        try:
            if check_object_permissions(
                info.return_type.graphene_type.permission_classes(),
                info.context,
                result.value,
            ):
                return result
            else:
                raise PermissionDenied()
        except AttributeError:
            return result

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants