exAspArk / graphql-guard

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

PORO Policy receiving incorrect (unexpected) type #23

Closed kalzoo closed 5 years ago

kalzoo commented 5 years ago

Hey, first of all, thanks for making this! Its easy integration with Pundit and its good documentation are pretty great.

Running into an issue here - I'm looking to provide authorization and not_authorized handling on the field level. So, I copy-pasted your example, swapped out constants ... and found that the procs in the GraphqlPolicy weren't being executed.

This may be a misunderstanding of graphql-ruby itself.

Here are snippets:

the schema (verbatim from documentation, except for Types:: prefix):

  use GraphQL::Guard.new(
    policy_object: Types::GraphqlPolicy,
    not_authorized: ->(type, field) {
      handler = Types::GraphqlPolicy.not_authorized_handler(type, field)
      handler.call(type, field)
    }
  )

graphql/types/graphql_policy.rb (named and placed that way to put it in the same load path as UserType etc, definitely open to suggestions) (pretty much copy-pasted)

module Types
  class GraphqlPolicy
    RULES = {
      UserType => {
        '*': {
          guard: ->(obj, args, ctx) { byebug; nil; UserPolicy.new(ctx[:current_user], obj.object).show? },
        },
        summary: {
          guard: ->(obj, args, ctx) { byebug; nil; UserPolicy.new(ctx[:current_user], obj.object).show_progress? },
          not_authorized: ->(type, field) { nil } # simply return nil if not authorized, no errors
        }
      }
    }

    def self.guard(type, field)
      byebug # This is the only byebug that is evaluated, and produces the output in next snippet
      RULES.dig(type, field, :guard)
    end

    def self.not_authorized_handler(type, field)
      Rails.logger.error("Calling not_authorized_handler for #{type.class}: #{type}, #{field}")
      RULES.dig(type, field, :not_authorized) || RULES.dig(type, :'*', :not_authorized)
    end
  end

The output (one of many) from that byebug statement, showing it's not matching the rule:

(byebug)
1: type = User
2: field = :*
3: RULES.dig(type, field, :guard) = nil

The output from the logger:

Calling not_authorized_handler for GraphQL::ObjectType: User, summary

Attempted workarounds:

  1. UserType to User -> No change

  2. UserType to GraphQL::ObjectType::User -> .../rails-root/app/graphql/types/graphql_policy.rb:4: warning: toplevel constant User referenced by GraphQL::ObjectType::User

  3. RULES.dig("#{type.to_s}Type", field, :not_authorized) || RULES.dig("#{type.to_s}Type", :'*', :not_authorized), and using a string as the key in RULES -> it works, but is hacky and adds unnecessary overhead in string conversions

So, the problem seems to be that the parameters sent to guard don't line up with that object's shape.

What's weird, to me, is that the type is User, which isn't the graphql type name at all:

module Types
  class UserType < Types::BaseObject

User is the model's class name.

The entire point of using the PORO was to be able to deny access only to that field, returning nil (but still returning an error, ideally, and definitely errors for other fields in other queries). Unfortunately, the best I can do with this is block the whole query:

{
  "data": {
    "user": null
  },
  "errors": [
    {
      "message": "Not authorized to access summary on this User.",
      "locations": [
        {
          "line": 8,
          "column": 5
        }
      ],
      "path": [
        "user",
        "summary"
      ]
    }
  ]
}

IMO it would intuitive if we could just define not_authorized right there in the type definition:

field :summary, UserSummaryType, null: false do not_authorized ->(type, field) { nil } guard ->(obj, args, ctx) { UserPolicy.new(ctx[:current_user], obj.object).show_progress? } end

but unfortunately

undefined method `not_authorized' for #<GraphQL::Schema::Field:>

PS, in the example for not_authorized_handler, there's a good chance of that returning nil, which throws a NoMethodError on call(). Shouldn't there also be a fallback to a default handler in the schema?

not_authorized: ->(type, field) {
      handler = Types::GraphqlPolicy.not_authorized_handler(type, field)
      handler ? handler.call(type, field) : GraphQL::ExecutionError.new("Not authorized to access #{field} on this #{type}.")
    }
kalzoo commented 5 years ago

As an aside, found that using type.graphql_name in .dig avoids the string conversion, but that's still fairly hacky:

module Types
  class GraphqlPolicy
    RULES = {
      'User' => {
        '*': {
          guard: ->(obj, args, ctx) { UserPolicy.new(ctx[:current_user], obj.object).show? },
        },
        summary: {
          not_authorized: ->(type, field) { nil } # simply return nil if not authorized, no errors
        }
      }
    }

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

    def self.not_authorized_handler(type, field)
      RULES.dig(type.graphql_name, field, :not_authorized) || RULES.dig(type.graphql_name, :'*', :not_authorized)
    end
  end
end
exAspArk commented 5 years ago

Hey @kalzoo,

Is the issue that you have User instead of UserType, so it can't find the correct RULES.dig(type, field, :not_authorized)?

From your code sample, it looks like you use class-based definitions:

module Types
  class UserType < Types::BaseObject

I'll try to convert these types in the project's tests, which historically use DSL, and see if it fails as well

https://github.com/exAspArk/graphql-guard/blob/a749f25a589f7b793a6d9b9f78c8b3dab75140bf/spec/fixtures/policy_object_schema.rb#L4-L16

exAspArk commented 5 years ago

I was able to reproduce it with graphql-ruby gem version 1.8 and class-based definitions:

module PolicyObject
  class PostType < GraphQL::Schema::Object
    field :id, ID, null: false
    field :title, String, null: true
  end

  class QueryType < GraphQL::Schema::Object
    field :posts, [PostType], null: false do
      argument :user_id, ID, required: true
    end

    def posts(user_id:)
      Post.where(user_id: user_id)
    end
  end

  class GraphqlPolicy
    RULES = {
      QueryType => {
        posts: ->(_obj, args, ctx) { args[:userId] == ctx[:current_user].id }
      },
      PostType => {
        '*': ->(_post, args, ctx) { ctx[:current_user].admin? }
      }
    }

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

  class Schema < GraphQL::Schema
    query QueryType
    use GraphQL::Guard.new(policy_object: GraphqlPolicy)
  end
end
[14] pry(PolicyObject::GraphqlPolicy)> @

graphql-guard/spec/fixtures/policy_object_schema.rb @ line 30 PolicyObject::GraphqlPolicy.guard:

    29: def self.guard(type, field)
 => 30:   require 'pry'; binding.pry
    31:   RULES.dig(type, field)
    32: end

[15] pry(PolicyObject::GraphqlPolicy)> type
=> Post

[16] pry(PolicyObject::GraphqlPolicy)> type.class
=> GraphQL::ObjectType

[17] pry(PolicyObject::GraphqlPolicy)> type.class.ancestors
=> [GraphQL::ObjectType,
 GraphQL::BaseType,
 GraphQL::Relay::TypeExtensions,
 GraphQL::Define::InstanceDefinable,
 GraphQL::Define::NonNullWithBang,
 Object,
 PP::ObjectMixin,
 JSON::Ext::Generator::GeneratorMethods::Object,
 Kernel,
 BasicObject]

It is related to this issue https://github.com/rmosolgo/graphql-ruby/issues/1429. A workaround for now is to use type.metadata[:type_class], for example:

def self.guard(type, field)
  RULES.dig(type.metadata[:type_class], field)
end

I'm going to add a note to the readme and add tests to cover this case.

exAspArk commented 5 years ago

Added tests and some notes to the README in https://github.com/exAspArk/graphql-guard/pull/24

kalzoo commented 5 years ago

Wow, good eye for the underlying graphql-ruby issue! That's definitely it. That workaround is super easy and will make for a quick fix once he works out the kinks in the class-based schema. Thanks!