Action policy scoping unable to deal with promises returned from graphql fragment cache library

[linstantnoodles]

hey @palkan ! i've got a interesting one for you involving graphql fragment cache gem when using the automatic auth scoping (authorized_scope: true). it's somewhat related to DmitryTsepelev/graphql-ruby-fragment_cache#68, but instead of unrecognized raw cached values we're now dealing with promises that fragment cache gem is returning since >= 1.12.x

Tell us about your environment

Ruby Version:

3.0.6

here's an executable test script with all the versions of dependencies (action policy, graphql fragment cache, ...). I've also included a potential workaround in comments inside the executable test. At the end i've also included an alternative approach

you can copy it to some file like some-test.rb and run it

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"
  gem "rails"
  gem "sqlite3", "~> 1.4"
  gem "graphql", "~> 2.3.4"
  gem "action_policy-graphql", "~> 0.5.4"
  gem "graphql-fragment_cache", "~> 1.12.0" # 1.12.0 has the lazy resolver
end

require "active_record"
require "active_support"
require "minitest/autorun"
require "logger"

# setup app configs
class TestApp < Rails::Application
  config.load_defaults Rails::VERSION::STRING.to_f
  config.root = __dir__
  config.hosts << "example.org"
  config.eager_load = false
  config.session_store :cookie_store, key: "cookie_store_key"
  config.secret_key_base = "secret_key_base"
  config.graphql_fragment_cache.store = :file_store, "cache"
end
Rails.application.initialize!

# setup database & models
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)
ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
  end

  create_table :comments, force: true do |t|
    t.integer :post_id
  end
end

class Post < ActiveRecord::Base
end

class PostPolicy < ActionPolicy::Base
  relation_scope do |relation|
    relation
  end

  # ========== POTENTIAL WORKAROUND ==========
  # Warning: i think .. this bypasses all other existing matchers for `Path` 
  # scope_matcher :lazy_resolver, GraphQL::FragmentCache::Schema::LazyCacheResolver 
  # scope_for(:lazy_resolver) do |cached_collection|
  #   cached_collection.resolve
  # end
  
   def show?
    true
   end
end

  
# graphql 
module Types
  class PostType < GraphQL::Schema::Object
    description "A blog post"
    field :id, ID, null: false
  end
end

class QueryType < GraphQL::Schema::Object
  include ActionPolicy::GraphQL::Behaviour
  include GraphQL::FragmentCache::Object

  description "The query root of this schema"

  field :post, Types::PostType, authorize: true do 
    argument :id, ID, required: true
  end

  field :posts, [Types::PostType], authorized_scope: { with: PostPolicy }

  def posts
    cache_fragment { Post.all }
  end
end

class Schema < GraphQL::Schema
  use GraphQL::FragmentCache
  query QueryType
end

class BugTest < Minitest::Test
  def test_multiple_posts
    post1 = Post.create!
    post2 = Post.create!
    query_string = "
      {
        posts {
          id
        }
      }
    "
    user = Struct.new(:username)

    assert_raises ActionPolicy::UnrecognizedScopeTarget do
      Schema.execute(query_string, context: { current_user: user.new("bob") })
    end
  end
end

What did you do?

we use both action policy and graphql fragment cache, it looks like >= 1.12 of fragment cache raises ActionPolicy::UnrecognizedScopeTarget because the cache call cache_fragment returns a promise-like object (GraphQL::FragmentCache::Schema::LazyCacheResolver).

What did you expect to happen?

I expect the promises to resolve before reaching scoping rules in action policy, so that we don't have to write custom scope matchers to force promise resolution. i know that the default behavior of action policy for unrecognized scope objects (pretty much anything other than ActiveRecord::Relation) is to throw ActionPolicy::UnrecognizedScopeTarget if it can't find scope matchers. That behavior makes sense - but i wonder if we should make an exception for promise-like objects?

the fragment cache's lazy resolver object represents an eventual value - i think it would be nice for action policy to ensure that it's always dealing with promise-resolved values!

What actually happened?

ActionPolicy::UnrecognizedScopeTarget is raised because there is no built-in scope matcher for GraphQL::FragmentCache::Schema::LazyCacheResolver

issues with the workaround

one issue with the "workaround" i added above is that it's basically a complete bypass. For example, cache_fragment { Posts.all } will return a lazy resolver and in this case match the lazy_resolver matcher, but if i also had a relation scope, it will fail to run unless i call it explicitly

for example:

class PostPolicy < ActionPolicy::Base
  # this will not match even if the `cached_collection.resolve` returns a relation
  relation_scope do |relation|
    relation.where(id: 1)
  end

  scope_matcher :lazy_resolver, GraphQL::FragmentCache::Schema::LazyCacheResolver
  scope_for(:lazy_resolver) do |cached_collection|
    cached_collection.resolve
  end

   def show?
    true
   end
end

a fix for this is to invoke authorized_scope inside the lazy_resolver scope matcher call. we check if it resolved to a cached value - if so, return it since we've already previously authorized the response. If not, scope it again.

scope_for(:lazy_resolver) do |cached_collection|
  result = cached_collection.resolve
  if result.is_a?(GraphQL::Execution::Interpreter::RawValue)
    result
  else
    authorized_scope(result)
  end
end

alternative approach / hooking into graphql-ruby resolver

an alternative approach is to add an extension to underlying graphql-ruby and ensure all promises are resolved before hitting any policy code (similar to your extensions in https://github.com/palkan/action_policy-graphql/blob/master/lib/action_policy/graphql/authorized_field.rb) . so if it resolves to a relation / list of Path active record instances, the correct corresponding policy code will run

class BaseField < GraphQL::Schema::Field
    include ApolloFederation::Field

    class FragmentCacheResolver < GraphQL::Schema::FieldExtension
      def resolve(object:, arguments:, **rest)
        value = yield(object, arguments)
        if value.kind_of?(GraphQL::FragmentCache::Schema::LazyCacheResolver)
          return value.resolve
        end
        value
      end
    end
   end

while this ensures more correct policy behavior, it's not obvious why this extension exists at a glance ... so there's a bit of an implicit dependency on action policy and gql fragment behavior

would love to hear your thoughts on how to address this. i'm wondering if there's a nice way of baking this into existing action policy gql hooks so that if we're dealing with a promise-like object that defines resolve, those values are resolved before any calls to authorized_scope

we still need to write custom matcher to deal with the raw values returned re: DmitryTsepelev/graphql-ruby-fragment_cache#68, but i don't feel as weird about doing that

[palkan]

Thanks for such a detailed report!

I need a bit more time to thoroughly think through it, but there is a quick question: wouldn't the following be a solution?

def posts
  cache_fragment { authorized_scope(Post.all) }
end

I think, scoping must happen during the value resolution and only if we do not return cache (i.e., the authorized field value is cached, not something before the authorization).

[linstantnoodles]

@palkan wow thanks for your prompt response! I hadn't thought of inlining that directly into the block. i think that can definitely work if we turn off authorized_scope: true in our current field definitions and opt to manually scope instead.

like right now the authorized_scope option kicks off the field resolver hook inside this gem

 class ScopeExtension < Extension
   def resolve(context:, object:, arguments:, **_rest)
     value = yield(object, arguments)
     return value if value.nil?

     object.authorized_scope(value, **options)
   end
 end

so if we keep that option as true it'll still attempt to scope the response from cache_fragment.

i'm gonna take a look and see if we can switch to manual calls for our current fields. i don't think turning off the auto-scoping is too much effort

it feels like the big problem with automatic hook behavior is that it's surprising when dealing with non-typical (not plain AR instances) values from gql field resolvers

[linstantnoodles]

Update: @palkan so i audited our code and turned off authorized_scope: which was getting applied everywhere and replaced them with your inlined suggestion and was able to ditch some of our custom fragment cache object scope matchers. I think getting away from automatic hooks is a fine solution!

[palkan]

I think getting away from automatic hooks is a fine solution!

Yeah, I think that's the best solution for now.

I think, we can explore adding a custom option to do this automatically, smth like: cached_authorized_scope: true and cached_authorized_scope: {with: SomePolicy, cache_options: {}}. But I don't have enough capacity to dig into this right now.