exAspArk / graphql-guard

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

[Bug] Guarding or masking a field on an Object Type hides the entire object and not just the individual field #47

Open joeyparis opened 4 years ago

joeyparis commented 4 years ago

Versions
graphql 1.11.2 graphql-guard 2.0.0

Example Code

module Types
  class PostType < Types::BaseObject
    field :id, ID, null: false
    field :name, String, null: true
    field :masked_or_guarded_field, String, null: true, mask: -> (ctx) { !ctx[:current_user].is_admin?} # or, guard: -> (obj, args, ctx) { !ctx[:current_user].is_admin?}
  end
end

Expected Response

{
  "id": "1",
  "name": "This is a name",
  "masked_or_guarded_field": null, // or just not there at all
}

Actual Response

null

I've also tried it using this syntax with the same results:

field :masked_or_guarded_field, String, null: true do
  mask: -> (ctx) { !ctx[:current_user].is_admin?} # or, guard: -> (obj, args, ctx) { !ctx[:current_user].is_admin?}
end

Let me know if I can provide any more information!

sshaw commented 4 years ago

Hi, just upgraded to 2.0 and have found a couple of issues (see issues) so was curious about this one.

It would be good to include what you have in your GraphQL::Schema subclass.

In my case I have something like:

use GraphQL::Guard.new(
  policy_object: MyPolicy,
  not_authorized: ->(*) { handler(*) }
)

If a user is authorized and I add guard: ->(*) { false } to a field it results the not_authorized handler being called. Without the not_authorized handler it raises a GraphQL::Guard::NotAuthorizedError. So maybe these things (or lack there of) are why you're getting your response?

sshaw commented 4 years ago

GQL Ruby adds this stuff (I remove it as it find it useless/inconsistent). Maybe this factors in to why you get the response you do?

joeyparis commented 4 years ago

That's what I thought at first too, but I added this and it's what I currently have in my GraphQL::Schema:

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

Which I believe should only log an error message, but not nullify the entire object.

joeyparis commented 4 years ago

@sshaw Just tried removing the prepapre_variables method from my graphql_controller but unfortunately, I'm experiencing the same problem

joeyparis commented 4 years ago

Okay! I've figured something out and got it "working", but I feel there may still be improvements that can be made to the gem. It's all based around this section of the docs: https://github.com/exAspArk/graphql-guard#error-handling.

# By default it raises an error
not_authorized: ->(type, field) do
  raise GraphQL::Guard::NotAuthorizedError.new("#{type}.#{field}")
end

This behaves as I would expect, it raises an error and the query fails.

# Returns an error in the response
not_authorized: ->(type, field) do
  GraphQL::ExecutionError.new("Not authorized to access #{type}.#{field}")
end

I would expect this to return either 'null' or the error for the individual field, but not null the whole object type. Could this possibly be because my actual field was an integer value and it just couldn't cast the error to an integer? I didn't see any error that indicated that being the reason, but I wouldn't rule it out.

not_authorized: ->(type, field) do
end

This works and only nulls out the object level field I want. Interestingly enough returning nil (compared to actually nothing) actually returned 0 (I'm guessing because the field was of an Integer type).

--

Upon further review, after I started this comment, the following technically "works" if I change my field type to String instead of Integer. (Note adding .to_s at the end of the error)

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

But of course, it returns Not authorized to access #{type}.#{field} for the field which I don't believe is an ideal response.

--

Now that I've gone through these few different scenarios, I think the real bug is that if the not_authorized lambda returns a value that can't be cast to the requested field it just nullifies the entire object but logs/raises no error. Ideally, it would raise/log an error, or at least only nullify the field instead of the whole object.

Hopefully, that helps! At the very least I'm now able to use the gem as I intended with the below, but let me know if there's anything else I can assist with!

not_authorized: ->(type, field) do
end
sshaw commented 4 years ago

Thanks for the info. I also see with the latest version of the Ruby GraphQL library one cannot return a String from rescue_from any more.