exAspArk / graphql-guard

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

allow introspection field types to skip authentication #4

Closed stanishev closed 7 years ago

stanishev commented 7 years ago

Hello,

Would you be open to making it configurable for users of graphql-guard to indicate that introspection fields should not be checked for authentication?

We've worked around this by using a policy_object, and checking for type.introspection? in the guard method of the policy_object itself, but it does feel like this should be offered out of the box.

If yes, I would even suggest it should be the default option, so that introspection fields would by default be "guard free", and if someone wants to hide their schema they would explicitly have to flip that option (but perhaps that's just me).

Happy to submit a PR if you think this is a sensible idea.

Cheers, Vlad

exAspArk commented 7 years ago

Hey Vlad!

Thanks for opening the issue!

Do I understand correctly that you don't want to perform authentication to the fields like the following?

query {
  __schema {
    types {
      name
    }
  }
}

By default graphql-guard is disabled. So, it basically performs an authentication only if you specified guard on your field:

field :posts, !types[!PostType] do
  guard ->(obj, args, ctx) { false }
end

Or your policy object with the guard method doesn't return nil:

class GraphqlPolicy
  def self.guard(type, field)
    ->(obj, args, ctx) { false }
  end
end

If you use '*' with the policy objects, then you can either whitelist the fields:

class GraphqlPolicy
  RULES = {
    QueryType => {
      '*': ->(obj, args, ctx) { false },
      __schema: ->(obj, args, ctx) { true } # always accessible
    }
  }

  def self.guard(type, field)
    RULES.dig(type, field)
  end
end

Or don't use '*' and blacklist the fields:

class GraphqlPolicy
  RULES = {
    QueryType => {
      posts: ->(obj, args, ctx) { false }
    }
  }
end

Please let me know whether the solutions work for you.

stanishev commented 7 years ago

Hi exAspArk,

yes that's exactly what I was referring to, thank you for the thorough walkthrough.

We opted for a whitelist policy, using a policy object very similar to the one in your example.

The reason I thought it might make sense to bake an option into the the gem to allow someone to whitelist all introspection types in one go, is that following the sample code in the usage guidelines we started enumerating all of them in the RULES hash and it quickly started feeling very hacky. Listing GraphQL(-ruby) types in the whitelist rules felt very out of place. Also as new types are added to the graphql-ruby gem in the future it implies things will break at some point.

We worked around this by using: type.introspection? in the guard method, which is simple enough and catches all introspection types (Schema, Field, _InputType, etc).

if type.introspection? || WHITELIST.dig(type,field)

So that works fine and doesn't feel too hacky. It should be future proof too.

Also I realize from reading your response above that if someone opts for the WHITELIST approach then they should be prepared to whitelist everything - including having to deal with introspection fields - even if they come as a surprise :). After all that's the definition of a whitelist.

It may still make sense to give introspection fields some special treatment, only because I can't imagine anyone wanting to whitelist just a few of them. They'll either be whitelisted as a group, or not at all.

But I am just thinking out loud at this point. :)

exAspArk commented 7 years ago

Hey, @stanishev!

It may still make sense to give introspection fields some special treatment, only because I can't imagine anyone wanting to whitelist just a few of them.

Your point makes perfect sense 👍

However, seems like I was wrong, the following code doesn't affect introspection queries (at least in my case):

QueryType => {
  __schema: ->(obj, args, ctx) { false }
}

I'm still able to send an introspection query succesfully. Seems like graphql-ruby itself treats them differently:

query IntrospectionQuery {
  __schema {
    queryType { name }
    mutationType { name }
    subscriptionType { name }
    ...
  }
}

We worked around this by using: type.introspection? in the guard method, which is simple enough and catches all introspection types (Schema, Field, _InputType, etc).

Could you please describe what was the problem in your case? What do you have in the schema and which queries do you send? Which versions of graphql and graphql-guard do you use? If you remove type.introspection?, how does it affect the response?

Happy to submit a PR if you think this is a sensible idea.

Will be glad to accept your PR if we're able to identify and reproduce the problem 🙌

stanishev commented 7 years ago

hi @exAspArk

However, seems like I was wrong, the following code doesn't affect introspection queries (at least in my case): QueryType => { __schema: ->(obj, args, ctx) { false } }

yes, having that snippet in the whitelist rules did not work for us either as written. For this to work we'd have to use fully qualified graphql-ruby types (some pseudo code below, won't compile):

class GraphqlWhitelistPolicy
  @no_login_required = ->(_, _, _) { true }
  WHITELIST = {
      GraphQL::Introspection::FieldType => Set[:description, etc],
      GraphQL::Introspection::TypeType => Set[:kind, etc.]
      # etc
  }.freeze

  def self.guard(type, field)
    return @no_login_required if(WHITELIST[type] && WHITELIST[type].include?(field))
    ->(_, _, ctx) { // some login logic }
  end
end

but in any case, we moved away from enumerating these in the WHITELIST hash, and instead used the type.introspection? method as mentioned.

so the first line in the self.guard method above becomes: return @no_login_required if type.introspection? || (WHITELIST[type] && WHITELIST[type].include?(field))

Could you please describe what was the problem in your case?

I would no longer call it a problem because the graphql-guard gem's whitelist policy does work as advertised and because handling the introspection fields turned out to be so simple. But it did feel as a bit of a surprise to have to look into. Again, in any case, we ran into it because:

I suspect that even teams that don't use the graphiql app would run into a similar situation if they attempted a whitelist policy with graphql-guard, because most teams using graphql seem to be making their schema publicly available (github's schema for example).

As far as addressing the "problem" :) I would have suggested adding a new method to guard.rb

def whitelist_introspection?(type)
  type.introspection? && introspection_whitelisted 
  #introspection_whitelisted is a private field on the Guard class, true by default, 
  # but settable to false by the user on calling GraphQL::Guard.new
end

and a call to this method from the guard_proc method in guard.rb

or alternatively, just mentioning the type.introspection? call somewhere in the examples you provide in the readme file would be sufficient to point people in the right direction.

exAspArk commented 7 years ago

@stanishev thanks a lot for explaining!!

I was confused by the fact that guard wasn't triggered for __Schema but it works for __Schema.queryType.

I think that for now, we can mention your type.introspection? solution in the README file 👍 Don't want to make the gem complicated with multiple configuration options unless it's being used very often :)

If you want, feel free to open a PR to mention it. Perhaps, we could add a separate section for it, something like Introspection query authorization after the Error handling with a simple example of the policy object?

stanishev commented 7 years ago

If you want, feel free to open a PR to mention it. Perhaps, we could add a separate section for it, something like Introspection query authorization after the Error handling with a simple example of the policy object?

👍 will do

ghost commented 6 years ago

Can you release this in 1.0.1 please?

ghost commented 6 years ago

I tried with the master branch in my Gemfile but it still doesn't work: #7