exAspArk / graphql-guard

Simple authorization gem for GraphQL :lock:
MIT License
471 stars 36 forks source link

Way to `nil` out the whole field rather than each of it's sub-fields if unauthorized to view it. #12

Closed justgage closed 6 years ago

justgage commented 6 years ago

Right now we configure this library this way:

  use GraphQL::Guard.new(
    not_authorized: ->(type, field) { GraphQL::ExecutionError.new("Not authorized to access #{type}.#{field}") }
  )

And we add gaurds on each of the types:

Types::LocationType = GraphQL::ObjectType.define do
  name 'Location'
  guard ->(location, _args, context) { context[:current_ability].can?(:read, location) }
end

(We use cancancan as our ability library)

However this query...

{
  location(id:12) {
    id
    users {name}
  }
}

Has a semi-odd behavior:

{
  "data": {
    "location": {
      "id": null,
      "users": null
    }
  },
  "errors": [
    {
      "message": "Not authorized to access Location.id",
      "locations": [
        {
          "line": 3,
          "column": 5
        }
      ],
      "path": [
        "location",
        "id"
      ]
    },
    {
      "message": "Not authorized to access Location.users",
      "locations": [
        {
          "line": 4,
          "column": 5
        }
      ],
      "path": [
        "location",
        "users"
      ]
    }
  ]
}

Rather than:

{
  "data": {
    "location": null
  },
  "errors": [
    {
      "message": "Not authorized to access Location id=12",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "location"
      ]
    }
  ]
}

Is there an easy way to do that with this library?

we managed to resolve it by doing:

  field :location, Types::LocationType do
    argument :id, !types.ID
    description 'Find a Location by ID'

    resolve(lambda do |_obj, args, context|
      location = Location.find_by(id: args['id'])
      if location.present? &&
         context[:current_ability].cannot?(:read, location)
        return GraphQL::ExecutionError.new("Not authorized to access Location id=#{args['id']}")
      end
      location
    end)
  end

However we have to do that for every field we want to protect. 🤷‍♂️

exAspArk commented 6 years ago

Hi, @justgage. Thanks for opening the issue!

Yeah, that's a common problem. Since you don't have a location object on the top level query before executing the resolve lambda, guard can't be performed. That's why, in my opinion, in most of the cases gems like CanCanCan or Pundit are used explicitly in the controller actions with RESTful APIs.

You can try to extract the authorization logic something like:

field :location, Types::LocationType do
  argument :id, !types.ID
  description 'Find a Location by ID'

  guard ->(_obj, args, context) do
    location = Location.find_by(id: args['id'])
    location && context[:current_ability].can?(:read, location)
  end

  resolve ->(_obj, args, context) do
    Location.find_by(id: args['id'])
  end
end

But that produces 2 Location.find_by queries. You can also try to memoize the location somehow. For example, through the context:

guard ->(_obj, args, context) do
  context[:location] = Location.find_by(id: args['id'])
  context[:location] && context[:current_ability].can?(:read, context[:location])
end

resolve ->(_obj, _args, context) do
  context[:location]
end

I personally wish graphql-ruby fully supports Plain Old Ruby Objects (PORO) for schema definition, so, it'is possible to memoize values with just @instance variables :)

Alternatively, as you suggested, simply inline the authorization logic to the resolve lambda.

Let me know what you think :)

justgage commented 6 years ago

I ended up keeping both the guard clauses to provide a blanket of safety and add inline resolve logic for API niceness. The next version of the library I think will support PORO's for what it's worth. Thanks for your response!