Graphql-shield: GraphQL Shield 2.0

[maticzav]

Where is graphql-shield headed

First, thanks for all the great feedback you've all given me so far. While listening to you and thinking about what I want and how I would like to use graphql-shield in my projects, I came to some conclusions, which will lead the second version of this package.

Whitelisting

The most requested feature and I, in my opinion, the most important one is whitelisting. Whitelisting, in comparison to blacklisting, requires all the exposed queries to be explicitly allowed in order to be used. Adoption of whitelisting also brings a lot easier stage separation as queries/mutations can be allowed in the development phase and restricted in production.

interface Options {
   debug: true // -> makes all the queries "available"
   cache: true
}

Nesting permissions

Currently, graphql-shield only supports nesting permissions using type-specific permissions, which get evaluated during execution chain.

const permissions = {
   Query: {
      me: authenticated
   },
   Me: {
      id: isMe,
      name: isMe,
      secret: isMe
   }
}

I think that the best approach to tackle this problem is by making possible lists of permissions.

const permissions = {
   Query: {
      me: [authenticated, isMe]
   },
}

As things get more complicated you might as well want to make some permissions optional (OR), which would require helper functions like oneOf and all to make it all possible.

const permissions = {
   Query: {
      posts: oneOf(isAdmin, isEditor, isOwner),
      oneTimeLatter: all(isAuthenticated, latterNotRead)
   }
}

This is also the solution I am aiming for.

Allow entire type permissions

Right now, you can only restrict certain fields and the entire types altogether. By introducing whitelisting instead of blacklisting this feature becomes essential as having to explicitly allow each field for each type is all too much work. In my opinion, making Type general permissions available would perfectly solve this problem.

const permissions = {
   Query: {
      me: auth
   },
   Me: allowAllFields
}

Debugging

I think that most of the functionality should be directed to developers during the development stage. By introducing the right tools to make permission handling as easy as possible, we can make an actual difference with graphql-shield.

__Few ideas:__

• Summary of all permissions in debug mode.
• Stage separation (as mentioned above) which would allow access to non-whitelisted queries to test and develop new permissions.

[a-type]

Looks like a good roadmap to me.

For what it's worth, it's not very hard to build your own permission composition on top of what you already have. I don't think it's really necessary to include a new resolver format like an array. Here's what I'm working with so far (very WIP, just whipped this up in the last couple hours as I adopt this library):

const { get } = require('lodash');

const hasScope = (scopeName) => 
  (_parent, _args, ctx, _info) => get(ctx, 'user.scopes', []).includes(scopeName);

const belongsToMe = (typeName) => (_parent, args, ctx, _info) => {
  const id = args.id;
  const userId = ctx.user.id;
  return ctx.db.exists[typeName]({ where: { id, user: { id: userId } } });
};

const all = (...verifiers) => (parent, args, ctx, info) => {
  for (verifier of verifiers) {
    if (!verifier(parent, args, ctx, info)) {
      return false;
    }
  }
  return true;
};

const any = (...verifiers) => (parent, args, ctx, info) => {
  for (verifiers of verifiers) {
    if (verifier(parent, args, ctx, info)) {
      return true;
    }
  }
  return false;
};

module.exports = {
  hasScope,
  belongsToMe,
  all,
  any,
};

Usage has been a breeze:

const { hasScope, belongsToMe, all } = require('./generic');

module.exports = {
  Query: {
    user: hasScope('readUser'),
    users: hasScope('readUsers'),
    timeline: hasScope('readTimeline'),
    timelines: hasScope('readTimeline'),
    event: hasScope('readEvent'),
    events: hasScope('readEvent'),
    file: hasScope('readFile'),
    me: hasScope('me'),
  },

  Mutation: {
    updateMe: hasScope('updateMe'),
    createTimeline: hasScope('createTimeline'),
    updateTimeline: all(
      hasScope('updateMyTimeline'),
      belongsToMe('Timeline'), 
    ),
    deleteTimeline: all(
      hasScope('deleteMyTimeline'),
      belongsToMe('Timeline'), 
    ),
    createEvent: hasScope('createEvent'),
    updateEvent: all(
      hasScope('updateMyEvent'),
      belongsToMe('Event'),
    ),
    deleteEvent: all(
      hasScope('deleteMyEvent'),
      belongsToMe('Event'), 
    ),
    deleteFile: all(
      hasScope('deleteMyFile'),
      belongsToMe('File'),
    ),
    updateFile: all(
      hasScope('updateMyFile'),
      belongsToMe('File'),
    ),
    login: () => true,
    signup: () => true,
  },
};

all and any are powerful enough for my needs. If I wanted to create a new Admin role with a updateAnyTimeline scope, for instance, I could just nest the existing scopes in an any and write in the logic for that scope as another all block.

When my user authenticates, I load their scopes into the JWT token. That makes things super simple so far.

[maticzav]

@a-type I love what you've achieved using the current library! 🙈

I agree that both all and any are not required, but they could greatly benefit from cache functionality - that's why I thought they might be super useful. Atop of that I just had an idea of adding something like withPermission(permission, resolversObject) where resolversObject could work like a spread operator to help you add the same permissions to multiple resolvers.

// previously 
const permissions = {
  Query: {
    feed: auth,
    explore: auth,
    posts: auth,
  }
}

// now
const homeScreen = {
  feed,
  explore,
  posts
} // might be imported from another file

const permissions = {
  Query: {
    ...withPermission(auth, homeScreen)
  }
}

All in all, as I said, I just love what you've built with graphql-shield. I would love to see you put up a more advanced example if you will have any spare time! 🙂

[a-type]

Caching might be good, yeah. It wouldn't be too hard to add to my stuff really. I'm thinking about caching hasScope based on the context's user ID, for instance. As I continue to build that out, though, it seems like it will end up pretty specific to my app's implementation of user / scope context for authorization. Which is perfectly fine, since this library is very simple to build on top of with more app-specific tooling. I think that's a really positive quality in any library... that it can be extended easily according to a user's needs without requiring additional tooling within the library itself.

I'm not sure I'd personally use the withPermission helper you describe, since all my permissions are very specific to a particular query/mutation. Perhaps I may find a user for it in the future, though.

There's not much more to my real-life implementation than what I posted before... pretty simple stuff. The only missing piece is how to assign scopes to a user. In my case, I'm loading scopes into the JWT token provided to the user on login. That means no extra queries are required, and no latency is added to each request. When I create the GraphQL context for my resolvers at the start of a request, I read the JWT token if it's present and add the scopes to context.user.scopes. Then my hasScope authorizers do the rest of the work and block access to queries / mutations the user doesn't have access to. If no JWT token is present, I go ahead and load in some 'default' anonymous scopes.

I'd say the biggest thing on the roadmap which would be nice is whitelisting / type-level auth for me.

[jhnferraris]

@maticzav My thought on the custom error is something like:

Currently we have this:

{
  "data": null,
  "errors": [
    {
      "message": "Insufficient Permissions.",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "getSurveys"
      ]
    }
  ]
}

Maybe we can find a way to set data object in the error object. In that way we can set different fields like what permission is the graphql client lacks, what fields they lack, etc.

I'm thinking of along these set of codes:

const { setError } = require("graphql-shield");

userAccess = (obj, args, ctx) => {
     // Assuming that it fails
    // Some method in graphql-shield named `setError`
    if (fails) {
       const errorObj = new CustomClientError(message, extraData)
       setError(errorObj);
       return false;
    } else {
       return true;
    }


}

const permissions = {
   Query: {
     users: (_, args, ctx) => userAccess(ctx)
   }
}

then the response object will be like

{
  "data": null,
  "errors": [
    {
      "message": "Some error message from CustomError object"
      "data": {
             // Some data parsed from the CustomError object
      },
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "getSurveys"
      ]
    }
  ]
}

by default setError will use PermissionError unless specified by the client.

What do you think?

Cheers!

[a-type]

Being able to provide more detail would be great for testing and debugging. I think setError as described, though, represents a side-effect in an otherwise functional pattern.

Another suggestion: allow users to return an Error instance instead of false, or allow them to return a more detailed object with a field to indicate permission status, and a description of the problem to pass to the user ({ failed: true, message: 'Please contact your account administrator to change your payment details', data: { /* etc */ } })

[jhnferraris]

@a-type Appreciate the inputs. I agree, allowing users to return Error instance would be easier.

[maticzav]

@a-type @jhnferraris I love both of your suggestions!

I agree with @a-type that setError would represent a side-effect and is not the best option. Furthermore, GraphQL by default tells the path to the field which caused the error. Currently, we always throw insufficient permissions error, as I believe that production shouldn't expose too much info about the backend. Debug mode, on the other hand, prints out the thrown error (if any is thrown) - this part is badly documented.

I love the idea of { error: Error } return type. Maybe we could, as you mentioned, make something like { allow: boolean, error?: error } - depends on the complexity...

[a-type]

Agreed that you wouldn't want to encourage users to provide more information without thought. It's important not to give the user anything more than they need to solve the problem... more information gives attackers more ideas.

Perhaps there's good use cases for providing at least a custom message, though. For those using a UI, we could always map permissions errors to customized messages on the frontend, but perhaps someone wants to provide a particular message to API users besides "insufficient permissions". From working with UX designers, I've learned that an error message should always include an actionable item for the user to try, so I could see myself wanting to customize the message if I exposed my API publicly.

[jhnferraris]

For those using a UI, we could always map permissions errors to customized messages on the frontend, but perhaps someone wants to provide a particular message to API users besides "insufficient permissions".

Our current use case is that our graphql methods will be limited to some user role. That's why additional fields/messages on the error part is important for our GraphQL server so the actionable item can be easily done. heh