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

incosistent subject used for CanCan integration #4291

Closed
christian-toscano opened this issue Jan 13, 2023 · 4 comments
Closed

incosistent subject used for CanCan integration #4291

christian-toscano opened this issue Jan 13, 2023 · 4 comments

Comments

@christian-toscano
Copy link

christian-toscano commented Jan 13, 2023

Describe the bug

Using the CanCan integration with a Resolver that returns a custom type with multiple fields gives inconsistent behaviour while choosing the right Ability rule to use.

can :read, Search do |search|
      # search is a Hash instead of a Search type
end

with a bit of debugging I found out why this happens.

module CanCan
  module Relevant
    # Matches both the action, subject, and attribute, not necessarily the conditions
    def relevant?(action, subject)
      subject = subject.values.first if subject.class == Hash
      @match_all || (matches_action?(action) && matches_subject?(subject))
    end

the line subject = subject.values.first if subject.class == Hash changes the subject to Search type but the rest of the code does not mirror this behaviour so it passes a Hash to the authorization block instead of using the subject.values.first value.

At the same time, if I define a block like this

can :read, Hash do |hash|
      ...
end

it is ignored

Versions

graphql (2.0.14)
graphql-pro (1.23.1)

GraphQL schema

my Resolver

class FindSearch < Resolvers::Base
	  ...
      type Types::Objects::Searches::SearchWithConfiguration, null: true
      ...
end

the type I want to return and authorize

module Types
  module Objects
    module Searches
      class SearchWithConfiguration < Types::Objects::BaseObject
        field :search, Types::Interfaces::SearchInterface, null: true
        field :configuration, GraphQL::Types::JSON, null: true
      end
    end
  end
end

the Ability file

class Ability
  include CanCan::Ability

  attr_accessor :user

  def initialize(user)
    return false if user.nil?
    ...
    can :read, Search do |search|
      # search is a Hash instead of a Search type
    end
  end
end

GraphQL query

Just calling the resover

Expected behavior

I expected the block to be called with a Search object

Actual behavior

The block is being called with a Hash

@rmosolgo
Copy link
Owner

Hey, sorry for the trouble with this! Thanks for reporting it, I definitely want to get to the bottom of it.

As I try to understand what's going wrong, I'm wondering, what Hash is passed to the authorization block? Do you mind sharing pp search in that case? (I'd like to know what keys it has, and what values, since that would give me a clue where the Hash might be coming from...)

Also, would you mind sharing an example query that demonstrates this bug? It's not totally clear to me whether you expect can :read, Search ... to be called on the SearchWithConfiguration value or on the field :search, Types::Interfaces::SearchInterface value that belongs to it, and an example query would help me understand the intent. Thanks!

@christian-toscano
Copy link
Author

christian-toscano commented Jan 18, 2023

I expect can :read, Search to be called with a Search type insistead of a Hash, the problem is that CanCan select this rule using subject.values.first but then it passes a Hash, the original subject, instead of the subject.values.first.

I know this is related to the gem cancancan and I do not know if this is the expected behaviour but it is leading to several issues with our application. I'm asking if you can do something with your CanCanIntegration now that we figured out this weird behaviour from cancancan

search is an ActiveRecord Model, configuration is just a Hash, here is the pp search run inside the ability block:

{:search=>
  #<Search:0x000000012b07e280
   id: 1,
   type: "Search",
   name: "Test search 1_380">,
 :configuration=>{:my_field=>"hello"}}

You can replicate the issue with this code that emulates the data CanCanCan receives from the CanCanIntegration:

class Search < ActiveRecord::Base
end

class Ability
  include CanCan::Ability

  attr_accessor :user

  def initialize
    can :read, Search do |search|
      puts search.class
      true
    end
  end
end

Ability.new.can?(:read, { search: Search.new, configuration: {field: 'hello' }})

it will print Hash.

or using a GraphQL query, it happens by just calling a resolver:

query($id: ID!) {
  findSearch(id: $id) {
		search {
			name
		}
		configuration
  }
}

@rmosolgo
Copy link
Owner

Thanks for sharing those details. Does your FindSearch resolver return a hash with those keys ({ search: ..., configuration: ...}?) If so, how about adding a new Ruby class for search-with-configuration values? That way, CanCanCan could tell the difference between a "real" Search and a search-with-configuration. For example:

class SearchWithConfiguration 
  def initialize(search:, configuration:) 
    @search = search 
    @configuration = configuration 
  end 
  
  attr_reader :search, :configuration
end 

Then, return that from your resolver instead:

# ...
SearchWithConfiguration.new(search: search, configuration: configuration) 

Then, when the SearchWithConfiguration reaches CanCanCan, it won't get confused by finding a Hash.

What do you think of that approach?

@christian-toscano
Copy link
Author

Thanks for your support, this solution seems to be working, it's not perfect but I understand this is due to CanCan behaviour so you can do little to change it. I think we can close the issue, thank you

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