exAspArk / graphql-guard

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

Guarding for the entire root of an object #5

Closed dzucconi closed 6 years ago

dzucconi commented 6 years ago
QueryType = GraphQL::ObjectType.define do
  name 'Root'

  field :widget do
    type WidgetType
    argument :id, !types.ID

    # Can't guard here because we haven't queried for the object yet

    resolve ->(_obj, args, _ctx) do
      Widget.find(args['id'])
    end
  end
end

WidgetType = GraphQL::ObjectType.define do
  name 'Widget'

  guard ->(widget, _args, ctx) do
    ctx[:current_user].can?(:read, widget)
  end

  field :id, type: types.String # => Not authorized to access Widget.id
  field :title, TruncatedStringField.new # Returns the title because we haven't explicitly guarded on this path
end

How would one go about guarding the entire WidgetType and every field under it? It seems to me that TruncatedStringField should not be returning, but that is in fact what happens. The errors return for every field that's queried, which also seems non-ideal.

exAspArk commented 6 years ago

Hey, @dzucconi!

I wonder why TruncatedStringField is being returned? 🤔 When you specify guard on the type level (WidgetType in your case), it should be applied for every field in the type.

What is TruncatedStringField? Didn't know that graphql gem allows specifying instance objects for fields. What GraphQL type the title has?

dzucconi commented 6 years ago

Ah, so it seems to no longer be referenced in the docs and this field implementation is likely just outdated.

Originally:

include ActionView::Helpers::TextHelper

class TruncatedStringField < GraphQL::Field
  def initialize
    self.type = GraphQL::STRING_TYPE
    self.description = "String field"
    self.arguments = {
      "truncate" => GraphQL::Argument.define do
        name "truncate"
        type types.Int
        description "Max-length for string"
        default_value nil
      end
    }
    self
  end

  def resolve(obj, args, ctx)
    if args[:truncate]
      return truncate(obj[ctx.ast_node.name], length: args[:truncate])
    end

    obj[ctx.ast_node.name]
  end
end

Replacing that with the substantially less gangly GraphQL::Function does the correct thing and respects the guard.

include ActionView::Helpers::TextHelper

class Truncate < GraphQL::Function
  type types.String

  argument :truncate, types.Int

  def call(obj, args, ctx)
    if args[:truncate]
      return truncate(obj[ctx.ast_node.name], length: args[:truncate])
    end

    obj[ctx.ast_node.name]
  end
end

Some more food for thought here though:

Re: https://github.com/rmosolgo/graphql-ruby/blob/master/lib/graphql/field.rb

The documentation there does state that you can create a field using:

name_field = GraphQL::Field.define do
  name("Name")
  type(!types.String)
  description("The name of this thing")
  resolve ->(object, arguments, context) { object.name }
end

Implementing title using this syntax respects guard, however the errors for any field requested under the title stops returning. Meaning if my query is

{
  widget(id: 1) {
    foo
    title
    bar
  }
}

I'll see path errors raised for foo and title but not bar (widget is obviously null at least in this case however)

dzucconi commented 6 years ago

So, modified version of the initial question here: how does one go about actually returning null for the entire object you want to guard against?

{
  widget(id: 1) {
    foo
    title
    bar
  }
}

Returns

{
  "widget": {
    "foo": null,
    "title": null,
    "bar": null,
  }
}

Which seems problematic, instead of doing an existential check against widget you'd have to then do it against every single field you've queried on.

exAspArk commented 6 years ago

Hey, @dzucconi!

Thanks a lot for showing the GraphQL::Field. I wonder why guard wasn't triggered out of the box 🤔 Probably, it's a bug with introspection in graphql gem or it requires some additional setup.

actually returning null for the entire object you want to guard against

That's a good question. I usually just returned nulls on the widget which meant for the clients that the widget exists, but this specific user doesn't have an access to read it. Since "widget": null can mean semantically mean that it doesn't exist.

One of the easiest solutions for your problem is to check the value in the resolver:

resolve ->(_obj, args, ctx) do
  widget = Widget.find(args['id'])
  widget if ctx[:current_user].can?(:read, widget)
end

Perhaps, it's possible to add a feature with something like post_guard lambda. But I'm not sure that it's worth it.

Please let me know what do you think.

dzucconi commented 6 years ago

@exAspArk thanks for your work/thoughts.

which meant for the clients that the widget exists, but this specific user doesn't have an access to read it

Yeah, this is a good point. Perhaps it just comes down to personal preference then.

I'm currently doing authorization using a function to wrap the resolvers, but I do like the idea of having that information tied to the type, so that every time someone implements a field that returns the type you don't have to wrap the resolver again.

Thanks for bearing with!