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

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

Open
joeyparis opened this issue Oct 20, 2020 · 6 comments

Comments

@joeyparis
Copy link

joeyparis commented Oct 20, 2020

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
Copy link
Contributor

sshaw commented Oct 20, 2020

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
Copy link
Contributor

sshaw commented Oct 20, 2020

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
Copy link
Author

joeyparis commented Oct 20, 2020

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
Copy link
Author

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

@joeyparis
Copy link
Author

joeyparis commented Oct 20, 2020

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
Copy link
Contributor

sshaw commented Oct 26, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants