palkan / action_policy-graphql

Action Policy integration for GraphQL
MIT License
152 stars 9 forks source link

Exposed authorization rules can't be accessed without user context #29

Closed neiljohari closed 4 years ago

neiljohari commented 4 years ago

Say I had a CourseType with a way to query it from QueryType with no authorization, through a field called courses. Then the following query works as expected, returning a list of Course ids:

{     
  courses {
    id
  }
}

The problem is, when context[:current_user] is null, attempting to access any of the exposed rules raises an exception Missing policy authorization context: user.

For instance, lets say that the CourseType has a way to query for a list of LessonType with no authorization, through a field called lessons. Additionally, consider that we expose the ability to create lessons on CourseType with the following: expose_authorization_rules :create?, with: LessonPolicy, field_name: :can_create_lesson.

Now, if we ask the value of canCreateLesson, an exception will be raised:

{     
  courses {
    id
    canCreateLesson {
      value
    }
  }
}

Here is the graphql response with error backtrace:

{
  "errors": [
    {
      "message": "Missing policy authorization context: user",
      "backtrace": [
        "/Users/neiljohari/.rvm/gems/ruby-2.7.1/gems/action_policy-0.4.3/lib/action_policy/policy/authorization.rb:57:in `block in initialize'",
        "/Users/neiljohari/.rvm/gems/ruby-2.7.1/gems/action_policy-0.4.3/lib/action_policy/policy/authorization.rb:51:in `each'",
        "/Users/neiljohari/.rvm/gems/ruby-2.7.1/gems/action_policy-0.4.3/lib/action_policy/policy/authorization.rb:51:in `initialize'",
        "/Users/neiljohari/.rvm/gems/ruby-2.7.1/gems/action_policy-0.4.3/lib/action_policy/rails/policy/instrumentation.rb:16:in `block in initialize'",
        "/Users/neiljohari/.rvm/gems/ruby-2.7.1/gems/activesupport-6.0.3/lib/active_support/notifications.rb:182:in `instrument'",
        "/Users/neiljohari/.rvm/gems/ruby-2.7.1/gems/action_policy-0.4.3/lib/action_policy/rails/policy/instrumentation.rb:16:in `initialize'",
        "/Users/neiljohari/.rvm/gems/ruby-2.7.1/gems/action_policy-0.4.3/lib/action_policy/behaviours/policy_for.rb:13:in `new'",
        "/Users/neiljohari/.rvm/gems/ruby-2.7.1/gems/action_policy-0.4.3/lib/action_policy/behaviours/policy_for.rb:13:in `policy_for'",
        "/Users/neiljohari/.rvm/gems/ruby-2.7.1/gems/action_policy-0.4.3/lib/action_policy/behaviours/thread_memoized.rb:50:in `block in policy_for'",
        "/Users/neiljohari/.rvm/gems/ruby-2.7.1/gems/action_policy-0.4.3/lib/action_policy/behaviours/thread_memoized.rb:57:in `block in __policy_thread_memoize__'",
(more lines that are within the graphql gem)

I believe this is a bug? The expected behavior (I think) is to simply return false values with reasons (maybe something like user? as the reason for I18n integration).

Looking through the action_policy-graphql code, it seems like having user in the context is a requirement? However, when I try to set the user context to Current.user || {} to avoid nil values, then I get a new error altogether:

{
  "errors": [
    {
      "message": "Couldn't find policy class for <LessonPolicy#create?: false (reasons: {:lesson=>[:owner?]}) (ActionPolicy::Base::APR)",
      "backtrace": [
        "/Users/neiljohari/.rvm/gems/ruby-2.7.1/gems/action_policy-0.4.3/lib/action_policy.rb:35:in `lookup'",
(only the lookup seems relevant)
...
neiljohari commented 4 years ago

It also seems like this case of nil user is not tested: https://github.com/palkan/action_policy-graphql/blob/796bc6fcc3dd859df3e31aa2dbb04ec491ba587e/spec/support/schema.rb#L99

The current_user is always set to something by default in that spec (:user).

I am able to somewhat solve my issue via the Null Object pattern (creating a null user). It's not ideal since I'd rather be able to just query for permissions with a missing context defaulting to false, but here's my current solution:

class GraphqlController < ApplicationController
  def execute
     ...
     context = {
       current_user: Current.user || ::NullUser.new
      }
      ...
  end
end
module Types
  class QueryType < Types::BaseObject
...
    field :viewer, UserType, null: true

    def viewer
      return context[:current_user].is_a?(::NullUser) ? nil : context[:current_user]
    end
...
end

This way from the front-end view, viewer genuinely appears as null, but in Ruby (and thus all places Action Policy checks), the NullUser is used to bypass the missing context errors.

The biggest downside I see is some changes in convention of how the client asks what it's allowed to do. Whereas before I was able to ask if the viewer was able to do something, and differentiate it clearly from whether another user is allowed to:

query {
  viewer {
    canCreateCourses {
        value
    }
  }
}

I now must conventionally define viewer permissions at the root level:

query {
  canCreateCourses {
      value
  }
}

Not the worst trade off in the world. Thoughts? Is this just me working around an action policy bug, or is this how we're expected to handle missing context?

palkan commented 4 years ago

If you do not require user for all your policy checks, you need to configure your policy classes to allow this behaviour:

class ApplicationPolicy < ActionPolicy::Base
  authorize :user, allow_nil: true
  # or
  authorize :user, optional: true
end

And then in your rules you can check for user.nil?.

neiljohari commented 4 years ago

@palkan Ah okay, that makes a lot of sense actually—thank you!

I think the documentation could use some clarification (and the specs probably should have a test for nil user with allow_nil/optional as true).

Would you be interested if I started a PR for these documentation/testing enhancements?

palkan commented 4 years ago

the specs probably should have a test for nil user with allow_nil/optional as true

We have such tests in the action_policy itself: https://github.com/palkan/action_policy/blob/863624adefa4a55a4cfc8eb801e873b40d7790e3/test/action_policy/policy/authorization_test.rb#L54-L68

if I started a PR for these documentation/testing enhancements?

Yep, it would be great to enhance allow_nil/optional documentation. Currently, we have just a single note about it without additional information: https://actionpolicy.evilmartians.io/#/authorization_context.

It definitely deserves a sub-section and some examples.

palkan commented 4 years ago

Closed by https://github.com/palkan/action_policy/commit/20d68de05ed7a9c36164c0ac41bbda14277aa8b0