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 #23

Closed
wants to merge 1 commit into from
Closed

Authorization #23

wants to merge 1 commit into from

Conversation

tgriesser
Copy link
Member

This PR is the beginning of an authorization strategy for GraphQL Nexus. Feedback welcome!

I added a field-level authorize method, but I'm not 100% convinced this should be the final solution here. Mainly wondering if there are things we can learn/adapt from graphql-ruby's approaches to authorization? Their concepts of "scoping", "authorization", "accessibility", and "visibility" are pretty interesting.

Goals:

  • Covers all of the major use cases
  • Type safety, as always
  • Simplicity (as much as we can), the fewer concepts/codepaths the better

Questions:

  • What are the major paths that folks would expect Nexus to reasonably cover? I don't mind building out a more robust layer around this, since it's a major pain-point when developing schemas.
  • Should we be opinionated here and require an authorize property by default on "root type" fields (Query, Mutation, Subscription), with the ability to disable with forceRootAuthorize: false on the makeSchema config? Or maybe the other way around... opt in?

@tgriesser tgriesser mentioned this pull request Feb 11, 2019
@jakelowen
Copy link

Good stuff. I like this approach. When documentation is written for authorization I think it would be a great idea to show an example of reusable and(), or(), not() like graphql-shield has. These could just be simple helper functions that thinly wrap lodash's every() and some() methods

export const or = (authChecks) => _.any(Promise.all(authChecks))

import { Context } from "./Context";

 export class AuthSource {
  constructor(protected ctx: Context) {}

   async postIsPublished(id: string): Promise<boolean> {
    const post = await this.ctx.post.byId(id);
    if (post.status === "published") {
      return true;
    }
    return false;
  }

   async isSelf(id: string): Promise<boolean> {
    return id === req.session!.userId
  }

  async someComposedPermCheck = (postId:string, userId:string) => 
       or([
                postIsPublished(postId),
                isSelf(userId)
            ])
}

And then later
authorize: (root, args, ctx) => ctx.auth.someComposedPermCheck(args.id, '1234')

I really like composing authChecks out of lots of smaller atomic functions and I think documentation examples which illustrate that would be powerful.

@avocadowastaken
Copy link

I added a field-level authorize method, but I'm not 100% convinced this should be the final solution here.

We get used to resolvers, so another resolver to check user permission fits great.

What are the major paths that folks would expect Nexus to reasonably cover? I don't mind building out a more robust layer around this, since it's a major pain-point when developing schemas.

{
  // Mostly I use higher order resolver
  addPost: createResolver({
    roles: ['admin', 'manager'],
    resolve: (root, args, ctx) => {  }
  }),
  // But also checking inside resolver
  editPostPost: createResolver({
    roles: ['admin', 'manager'],
    resolve: (root, args, ctx) => {
        const post = ctx.repos.post.get(args.id);
        
        if (ctx.user.role === 'manager' && post.authorId != ctx.user.id) {
          throw new Error('Forbidden');
        }
    }
  }),
}

Should we be opinionated here and require an authorize property by default on "root type" fields (Query, Mutation, Subscription), with the ability to disable with forceRootAuthorize: false on the makeSchema config? Or maybe the other way around... opt in?

Require authorize by default and opt out with forceRootAuthorize sounds fine.

@otrebu
Copy link

otrebu commented Feb 11, 2019

As it stands at the moment can we use graphql-shield?

@tgriesser
Copy link
Member Author

As it stands at the moment can we use graphql-shield?

Yes

@SachaG
Copy link

SachaG commented Feb 11, 2019

Just a few thoughts (feel free to ignore them if not applicable):

(edit: after a better look I guess this is more about authorization than authentication, sorry! I'll still leave the links here just in case they're useful to anyone :)

@swolidity
Copy link

@tgriesser in regards to graphql-shield there is no real integration with nexus yet right? You have to define your permissions and rules in a separate file and then add them to the schema with graphql-middleware correct? Or did I miss something? Some sort of integration would be amazing so we don't have to redundantly describe the schema in another file. I was naively thinking of a "plugin" like nexus-prisma but I am really just at the trying to grok it stage to all this so I don't know if that idea has any merit.

@colinhacks
Copy link

I thought everyone was on the "directive permissions" train until everyone ditched SDL for code-first solutions that don't support directives 😑

Anyway - I think the big problem with the authorize method is that you'll fetching the same data twice from the database. For instance in the ghost example, both canViewPost and the resolver make separate calls to ctx.post.byId().

Way back in 2017 someone proposed passing a executeResolver function as a parameter of authorize. Basically it lets you call the resolver, get back the data, and do whatever authorization logic you want without requiring duplicate database calls. I always thought it was a neat idea.

@tgriesser
Copy link
Member Author

Hey @SachaG, thanks for the links! Yeah, this is more around authorization - determining which mutations can be executed, which fields can be fetched/returned etc. The general stance by GraphQL is to push that to the business layer, though I think there's room for something slightly more opinionated/declarative. In terms of general authentication, I'm not sure if Nexus should take any sort of stance here since (at least as far as I've seen) that's typically something handled outside of the scope of GraphQL. Accounts.js looks really neat though, I'm going to look into it more!

@andyk91 There is no real integration, you can currently just layer it over your schema as though you were using it with any other GraphQL schema. The reason I'm leaning against integrating with graphql-shield via a plugin or otherwise, is you'd lose some of the type-safety benefits nexus provides without re-inventing a lot of the same concepts that might be simpler expressed with something more integrated. Also I think the concepts of "caching" can be eliminated by pushing it to a more robust context object/data-fetching layer.

@tgriesser
Copy link
Member Author

Anyway - I think the big problem with the authorize method is that you'll fetching the same data twice from the database. For instance in the ghost example, both canViewPost and the resolver make separate calls to ctx.post.byId().

@colinmcd94 that's not the case with the way it's setup in the example, the post.byId uses a DataLoader so multiple requests to the same primary key across the request will only result in a single batched query, each subsequent request will return the cached value without hitting the network.

Way back in 2017 someone proposed passing a executeResolver function as a parameter of authorize. Basically it lets you call the resolver, get back the data, and do whatever authorization logic you want without requiring duplicate database calls. I always thought it was a neat idea.

I think a lot of these ideas are mirrored very nicely (albeit differently) in graphql-ruby with the concept of scoping and authorization both at the field-level and the type-level. I'm going to explore these concepts a bit more to see how they might look in an implementation here.

@colinhacks
Copy link

colinhacks commented Feb 12, 2019

[update: didn't see your response before submitting this]:

I think considering the power of directives is a good way to consider potential scenarios you wouldn't have anticipated. You can attach a directive to an object type, object field, input type, input field, argument, interface, union, enum, and enum value. It's interesting to think about how a directive permission could be used on each of these data types.

One scenario that isn't accounted for with the current approach: the ability to apply an authorization rule to entire model. This is possible in graphql-shield.

Also: you mention here wanting to do validation/authorization of nested mutation input. Please do this! It would be a total game changer. Or at the very least, you should be able to mark a particular relation as "off limits" to nested mutations. (Sorry this is probably more relevant to the nexus-prisma plugin repo).

@colinhacks
Copy link

@colinmcd94 that's not the case with the way it's setup in the example, the post.byId uses a DataLoader so multiple requests to the same primary key across the request will only result in a single batched query, each subsequent request will return the cached value without hitting the network.

Ah great! I hadn't looked into the implementation deeply enough.

@SachaG
Copy link

SachaG commented Feb 12, 2019

The general stance by GraphQL is to push that to the business layer, though I think there's room for something slightly more opinionated/declarative.

Yeah that's what we do in Vulcan.js. It ends up looking pretty similar to what you're proposing actually, except as a shorthand each field can also take an array of user group names that are allowed to perform an operation instead of the explicit function. Here's the relevant documentation if you're curious, although I'll admit the current implementation still feels a little clunky. It's just had to find one consistent API to handle things like reading documents, mutating them, and then individual field permissions as well.

In terms of general authentication, I'm not sure if Nexus should take any sort of stance here since (at least as far as I've seen) that's typically something handled outside of the scope of GraphQL.

I've also noticed this, but I wonder if the reason for that isn't simply the lack of good tools/boilerplates for GraphQL authentication, and if maybe that trend isn't going to change?

@tgriesser
Copy link
Member Author

One scenario that isn't accounted for with the current approach: the ability to apply an authorization rule to entire model. This is possible in graphql-shield.

This is just the beginning of the approach. This ticket is to discuss and implement these scenarios.

Or at the very least, you should be able to mark a particular relation as "off limits" to nested mutations. (Sorry this is probably more relvant to the nexus-prisma plugin repo).

Yeah, might be better for discussion/feature requests over there - though possibly related, in the next major GraphQL release it looks like there will (hopefully) be validation in input types that will help with this.

@tgriesser
Copy link
Member Author

Here's the relevant documentation if you're curious, although I'll admit the current implementation still feels a little clunky. It's just had to find one consistent API to handle things like reading documents, mutating them, and then individual field permissions as well.

Thanks! I'll take a look / will take a look more into how Vulcan is doing schemas in general since it looks like you've gone with the code-first approach as well. Would love to hear any other thoughts on where you've found rough-edges with that approach, and whether nexus or a wrapper around it might ever be a good fit with what you're trying to do!

I've also noticed this, but I wonder if the reason for that isn't simply the lack of good tools/boilerplates for GraphQL authentication, and if maybe that trend isn't going to change?

I don't know if it's good tools vs. just not the best tool for the job. I've found with authentication you have a lot more server redirects, errors, headers, things that you actually want non-200 http status codes for that feel a bit awkward to try to fit into the GraphQL model.

@SachaG
Copy link

SachaG commented Feb 12, 2019

Thanks! I'll take a look / will take a look more into how Vulcan is doing schemas in general since it looks like you've gone with the code-first approach as well. Would love to hear any other thoughts on where you've found rough-edges with that approach, and whether nexus or a wrapper around it might ever be a good fit with what you're trying to do!

Honestly that approach seems to work pretty well, but Vulcan is a bit special as its goal is to abstract away the GraphQL layer, at least when you're getting started. Probably the main downside is that code-first can be a bit more verbose compared to just adding one line to a GraphQL schema document. But I think the trade-off is worth it in the end.

I don't know if it's good tools vs. just not the best tool for the job. I've found with authentication you have a lot more server redirects, errors, headers, things that you actually want non-200 http status codes for that feel a bit awkward to try to fit into the GraphQL model.

That's a great point, I hadn't really thought about it like that.

@jangerhofer
Copy link

jangerhofer commented Feb 25, 2019

One struggle I have faced with GraphQL authorization in general is deciding when to delegate authorization to the appropriate resolver and when to utilize the authorization layer (be it the authorize function from #32 or GraphQL shield). Primarily, I have often struggled with list authorization.

To illustrate one potential tricky use-case with lists, suppose I want to include a products field in my root Query type, while allowing the user to provide the Prisma-esque where filter on the products. Additionally, presume a user can only see his/her own products. In this case, the authorize function does not (please correct me if I have missed a pattern here!) cover all the authorization functionality I need.

Briefly illustrated in SDL:

type Query {
    ...
    products(where: ProductsWhere): [Product]
}

type Product {
    ...
    owner: User!
}

As a first step, I can include an authorize function on the Query.products field which checks that the user is in fact authenticated (i.e. logged in). But the conundrum now is that I need to make sure that the user is only accessing products he/she owns. I see two ways forward:

  1. Inside of the authorize function, I can pick off that where argument and then execute the query that would eventually run in the resolver. With the results, I can check each of the resulting products to make sure each one belongs to the user. If any product which matches the requested query does not belong to the user, then I return false from the authorize function and the user gets an authorization error because they (perhaps unwittingly) requested products that were not their own. Pretty poor design, if you ask me.

  2. Inside of the resolve function, I can filter out the results of the where query such that only the requester's products are returned. In the past, my quick fix has been simply to force a condition in the where map by injecting the requester's identity into the query before executing it. This methodology avoids the pitfall of misalignment of offsets/first/last/size constraints, etc. which would result from filtering the list after the query. Regardless of how the filter takes place, the main benefit of the authorize field is lost on me in this model, because in performing that filter, I have now mixed concerns: authorization with retrieval.

I have not given the problem much thought, but I am curious as to (a) whether others have taken a different or better approach to similar impasses and (b) whether Nexus might be able to address this use-case to keep authorization nicely encapsulated!

@Weakky
Copy link
Member

Weakky commented Mar 9, 2019

Coming to the discussion a bit late! I think the concept of visibility is super useful.

I started trying to look for ways to hide fields from the introspection query, found that issue ardatan/graphql-tools#323 (which has never been solved), which lead me to ruby's visibility concept, which reminded me that you've mentioned it here.

As stated in the ruby documentation, removing fields from your introspection schema based on auth has two main use-cases:

  • Shipping experimental features to a set of users (just like Github is doing)
  • Hiding admin related stuff from regular users (the most common use-case)

Most e-commerce related app would probably want that to hide all the CRUD operations to manage products/inventory/orders etc etc!

@Nayni
Copy link
Contributor

Nayni commented Mar 13, 2019

Like a lot of people in the GraphQL community, me and my team have also faced some of the challenges that aren't clearly identified in the spec. The main points of this being the classic use cases that would fall under the express-style-middlewares.

Authorization is one of the most important concepts when it comes to overall API design and therefor is a high priority detail in a GraphQL API as well. I personally have seen two general approaches here:

  • Push the concept of authorization to a lower lever service and deal with it later down the execution path. This usually is the path that teams take when GraphQL is a layer on an existing group of APIs and the schema is more used as a sort of gateway into a microservices architecture.
  • Handle authorization at field/resolver level within the schema execution. This is usually the solution teams tend to use when the API is being built without any prior existence of another API and the GraphQL endpoint is the source of truth.

For situations where the first approach is more suitable I think there isn't much more GraphQL or any library or framework has to offer because the way authorization will be taken care of is determined by a down-stream service and the most common approach is to just find a way to bubble up the error.

The second approach is where I fee like libraries and frameworks like Nexus could offer a great first-class API and make this experience better to solve. I've not yet used the ruby implementation but from reading their docs I very much like the approach they've taken and I think concepts like authorization, scoping and visibility are very common use-cases.

When it comes to plain authorization and basically checking if a user has permissions to execute a certain field or mutation we've mostly been using an approach that closely aligns with what @umidbekkarimov has explained. It mostly involves writing higher order helper functions that wrap multiple resolvers into one. I personally like this approach as it is fairly easy to grasp and explain to new team members and it follows the basic building blocks of a GraphQL implementation.

I also think that with this approach for authorization we can safely assume that it's become best practice to put any auth/token/cookie reference onto your context by now and use this data to drive any authorization check so I think there are some opportunities here to create conventions and allow for easy access to these things via something like an authorize option (which has already landed).

Should we be opinionated here and require an authorize property by default on "root type" fields (Query, Mutation, Subscription), with the ability to disable with forceRootAuthorize: false on the makeSchema config? Or maybe the other way around... opt in?

I would argue to advice against this. Mostly because root types are not always the barrier into something that becomes authorized-only. I've seen and built plenty of schema's where the Query root type exposes a lot of what could be considered "public" data and only a limited portion of it was truly authorize-locked. I would prefer a solution that gives me quick and easy ways to add an authorization level on a field or mutation rather than forcing this onto the root types. Also it would get in the way of situations where I am just using GraphQL as a gateway into my underlying architecture.

Scoping is something that I've personally had limited use cases for and this is usually solved by just having more verbose resolver implementation that do these checks by their implementation needs. However I wouldn't mind having an API that suggests an implementation like the ruby one, my main concern here are things like:

  • if I needed to scope a list down to a certain number of items I would've preferred this decision to also determine the actual data fetching or query my data layer was making because otherwise I'm doing a potential database query which is grabbing way too much to begin with.

Visibility is possibly the most interesting to me and also one that I've been missing when it comes to implementing GraphQL in JavaScript. The only way I've been able to deal with visibility is by just setting up a second schema that doesn't expose the "secret" things. However I am very interested in an approach that would allow us to do this by just having it baked into the schema definition/code.

An approach I've been exploring for this is to use schema transitions which are supported by graphql-tools (https://www.apollographql.com/docs/graphql-tools/schema-transforms#Transform). The idea mainly resolves around re-interpreting the schema based on the user context and use this to fulfill the query. The main problem here is that this transformation can only be done after the entire schema is created/compiled and also re-creates/compiles the schema again, making it tricky on what to do with for example Apollo Server as it would potentially have to "restart" (?)

Anyway, very much looking forward to what Nexus comes up with. I'm loving the focus on type safety and the code first approach as it bring a lot of clarity is larger code bases. To sum up:

  • I think most middleware use case needs can be solved by leveraging the resolver concepts and higher order functions. It's what I've seen as a widely adopted solution and I think it works great and cleanly.
  • Scoping and Visibility seem like some harder problems to solve because they combine the idea of declaring something but they need the run-time behavior to alter what is declared. I'm not sure if the underlying tools like graphql-js and apollo are ready to do this yet.

@soosap
Copy link

soosap commented Apr 9, 2019

@tgriesser Have you considered using the casbin library to manage access control? One could configure casbin and pass it into context. Subsequently, it would be accessible in the authorize hook to use like in the attached pseudo code. I think to make this work we need a Prisma ORM Adapter (see available adapters).

Unlike graphql-shield, this solution offers object-level authorization like django-guardian.

nexus-authorize-casbin

@Nayni
Copy link
Contributor

Nayni commented Apr 19, 2019

Wanted to share an existing solution we came up with for now. As we started using nexus we refactored and fine-tuned some of our existing helper functions to aid in this process.

This solution mainly focuses on authorization and validation of input before executing the real resolver. Our solution looks like this:

export const MutationDoStuff = mutationField("doStuff", {
    type: "Stuff",
    args: {
        input: arg({
            type: "DoStuffInput",
            required: true
        })
    },
    resolve: mutationResolver({
        authorize: [
            isAuthenticated,
            // These are just type hints for TypeScript, still looking to improve this.
            hasAccessToNode<"Mutation", "doStuff">(
                ({ input }) => input.id
            )
        ],
        validate: [validateDoStuff],
        resolve: (root, { input }, { providers }) => {
            const stuffID = fromGlobalId(input.id).id;
            // ... resolve
        }
    })
});

The mutationResolver is our own helper function which is basically a glorified resolver wrapper. The authorize and validate fields are just arrays of resolvers that will be called in before the real resolve runs. The implementation looks somewhat like this (we do a lot more in our actual implementation, like automatically setting up a transaction and logging the resolve etc)

type OrAny<T> = T | any;

export type GetFieldName<TypeName extends keyof NexusGenFieldTypes> = Extract<
    keyof NexusGenFieldTypes[TypeName],
    string
>;

export type MiddlewareResolver<
    TypeName extends keyof NexusGenFieldTypes,
    FieldName extends GetFieldName<TypeName>
> = (
    root: RootValue<TypeName>,
    args: ArgsValue<TypeName, FieldName>,
    context: GetGen<"context">,
    info: GraphQLResolveInfo
) => Promise<void> | void; // We chose to throw errors if something fails, so these middlewares are just void functions.

type MutationResolverOptions<
    TypeName extends string,
    FieldName extends string
> = Readonly<{
    authorize?: Array<MiddlewareResolver<OrAny<TypeName>, OrAny<TypeName>>>;
    validate?: Array<MiddlewareResolver<OrAny<TypeName>, OrAny<TypeName>>>;
    resolve: FieldResolver<TypeName, FieldName>;
}>;

const mutationResolver= <TypeName extends string, FieldName extends string>(
    options: MutationResolverOptions<TypeName, FieldName>
): FieldResolver<TypeName, FieldName> => {
    return async (root, args, context, info) => {
        await Promise.all(
            (options.authorize || []).map(authorize =>
                authorize(root, args, context, info)
            )
        );

        await Promise.all(
            (options.validate || []).map(validate =>
                validate(root, args, context, info)
            )
        );

        return options.resolve(root, args, context, info) as any;
    };
};

The isAuthenticated authorizer is a simple implemntation of a middleware resolver which looks like this (make middleware is just a simple wrapper):

export const isAuthenticated = makeMiddleware<any, any>(
    (root, args, { auth }) => {
        if (_.isNil(auth)) {
            throw new AuthenticationError("You are not authenticated");
        }
    }
);

Also we use the global ID and Node specifications from Relay and have a general guard to check if a user has access to a given node (as seen in the example), the hasAccessToNode looks something like this:

export const hasAccessToNode = <
    TypeName extends keyof NexusGenFieldTypes,
    FieldName extends GetFieldName<TypeName>
>(
    nodeIdSelector: (args: ArgsValue<TypeName, FieldName>) => string
) =>
    makeMiddleware<TypeName, FieldName>(async (root, args, context) => {
        const nodeID = nodeIdSelector(args);

        // This will throw if access is denied.
        await checkAccessToNode(nodeID, context);
    });

Also validate works the same way. It relies on the same makeMiddleware helper to create a mini-resolver that throws when something is not valid. By throwing an error we stop the promise chain and the final resolver never executes.

Even though we don't need it, but the concept of scoping could be solved in the same way, which would actually be a resolver that runs after the core resolve is complete (to filter out the results).

I think ideally something similar would be available on the actual definition of the nexus definition so we don't need our helper anymore.

@Bjoernstjerne
Copy link

Hi
I played a little bit with authorize and found something which I would like to be solved better:

//t.list.field("conversations", {type: "Conversation", nullable: true, authorize: (root, args, context, info) => {return true}})
        t.model("Profile").conversations({})

It would be nice if authorize is also available when using model not only for field. If I can use this on model I do not have to know something about conversations for t.field I have to know is this a list, can this field be null...

@jasonkuhrt jasonkuhrt added this to In Progress in Labs Team via automation Sep 24, 2019
@jasonkuhrt jasonkuhrt moved this from In Progress to Backlog in Labs Team Sep 24, 2019
@jasonkuhrt jasonkuhrt added the type/feat Add a new capability or enhance an existing one label Oct 3, 2019
@hsluoyz
Copy link

hsluoyz commented Oct 3, 2019

We should use Casbin. It has a lot of built-in helper functions to help guard the API: https://casbin.org/docs/en/function

@jhalborg
Copy link

Not sure if given/obvious or already discussed between the lines somewhere, but I'd like for the authorize resolver function to be able to be async, in case the function would need to do an I/O operation or similar to determine authorization.

@tre-dev
Copy link

tre-dev commented Jan 9, 2020

@Nayni are you still using your posted solution? Or has something changed over time?

@Nayni
Copy link
Contributor

Nayni commented Jan 10, 2020

@Nayni are you still using your posted solution? Or has something changed over time?

I am sadly not working on the project that I mentioned anymore. However nothing much changed between the time I wrote that comment and before I moved on. I remember we cleaned up the typings a bit more and moved from a resolver-middleware to a simple boolean for checking logged in status (which basically just calls the middleware under the hood then).

For new projects I still end up using approaches that are very much inspired with the one I suggested so the main ideas stay the same:

  • Resolve is wrapped in a helper function
  • It's based on a middleware like approach where throwing will cause the chain to stop.
  • It includes opens up for both Authorization and Validation.
  • Having a Node / Global ID is something I still prefer for easy object lookup.

I've been thinking about trying to nail this down into a common utils package but I haven't gotten around to this yet neither have I fully figured all the edge cases out.

@jasonkuhrt
Copy link
Member

@tgriesser can we close this now that we have a authorization plugin?

@tgriesser
Copy link
Member Author

Yep, I think we can close this for now. Might revisit object-level validation / field visibility at some point, but that can be a separate discussion.

@tgriesser tgriesser closed this Oct 26, 2020
@tgriesser tgriesser deleted the authorization branch October 26, 2020 15:37
@Sytten
Copy link
Collaborator

Sytten commented Nov 6, 2020

If people need something more complex I created https://github.com/Sytten/nexus-shield for that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope/auth type/feat Add a new capability or enhance an existing one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet