varvet / pundit

Minimal authorization through OO design and pure Ruby classes
MIT License
8.27k stars 628 forks source link

Is Pundit structurally incompatible with Statesman state machine #440

Closed MincePie closed 7 years ago

MincePie commented 7 years ago

I have just had a session on getting started with pundit scopes with codementor.io. After an hour, we stopped because it appears that it is not possible to use Pundit scopes with my state machine - which is Statesman. Apparently, Pundit can't run an SQL query where one of the query parameters is a statesman state.

The suggestion I have received is to use a different state machine. Before I do that, i just want to check if anyone has managed to find a way to use Pundit (with scopes) and Statesman state machine.

My setup is:

ApplicationPolicy

class ApplicationPolicy
  attr_reader :user, :record

  def initialize(user, record)
    @user = user
    @record = record
  end

  def index?
    true
  end

  def show?
    scope.where(:id => record.id).exists?
  end

  def create?
    false
  end

  def new?
    create?
  end

  def update?
    false
  end

  def edit?
    update?
  end

  def destroy?
    false
  end

  def scope
    Pundit.policy_scope!(user, record.class)
  end

  class Scope
    attr_reader :user, :scope

    def initialize(user, scope)
      @user = user
      @scope = scope
    end

    def resolve
      scope
    end
  end
end

ProposalPolicy

class ProposalPolicy < ApplicationPolicy
  class Scope < Scope
     def resolve
       accum_scope = scope.where(user_id: @user.id)
  #     # Depends on moving off statesman
  #     # accum_scope = accum_scope.or(accum_scope.reviewable) if @user.has_role?(:research_management, Organisation.first)
       accum_scope
     end
  end

  # I think index isnt necessary when I have a Scope
  # def index?
  #   true
  # end

  def new?
   true 
  end

  def create?
    new?
  end

  def show?
    #if its in the index results for that user
    true
  end

  def edit?
    update?
  end

  def update?
    true if record.try(:user) == user
  end

  def destroy?
    true if record.try(:user) == user
  end

  def draft?
    create?
  end

  def under_review?
    true if record.try(:user) == user
  end

  def approved?
    @user.has_role?(:research_management, Organisation.matching) 
  end

  def not_approved?
    approved?
  end

  def publish_openly?
    true if record.try(:user) == user && record.can_transition_to?(:publish_openly)
  end

  def publish_to_invitees?
    true if record.try(:user) == user && record.can_transition_to?(:publish_to_invitees)
  end

  def publish_counterparties_only?
    true if record.try(:user) == user && record.can_transition_to?(:publish_counterparties_only)
  end

  def remove?
    true if record.try(:user) == user #|| @user.has_role? :admin
  end

  private

  def matching
    @user.organisation_id == record.user.organisation_id
  end
end

Proposal.rb

class Proposal < ApplicationRecord
  include Statesman::Adapters::ActiveRecordQueries

has_many :proposal_transitions, class_name: "ProposalTransition", autosave: false
scope :reviewable,  -> { in_state(:under_review) }
  def state_machine
    @state_machine ||= ProposalStateMachine.new(self, transition_class: ProposalTransition,
                                                   association_name: :proposal_transitions)
  end

  delegate :can_transition_to?, :transition_to!, :transition_to, :current_state,
           to: :state_machine

  private

  def self.transition_class
    ProposalTransition
  end

  def self.initial_state
    :draft
  end

When I try to run the query in the console, I get an error that indicates the SQL is incompatible. The current problem is:

ProposalTransition.rb

class ProposalTransition < ActiveRecord::Base
  include Statesman::Adapters::ActiveRecordTransition

  belongs_to :proposal, inverse_of: :proposal_transitions

  after_destroy :update_most_recent, if: :most_recent?

  private

  def update_most_recent
    last_transition = proposal.proposal_transitions.order(:sort_key).last
    return unless last_transition.present?
    last_transition.update_column(:most_recent, true)
  end
end

ProposalPolicy::Scope.new(User.first, Proposal).resolve.all

  User Load (1.0ms)  SELECT  "users".* FROM "users" ORDER BY "users"."id" ASC LIMIT $1  [["LIMIT", 1]]
  Organisation Load (0.7ms)  SELECT  "organisations".* FROM "organisations" ORDER BY "organisations"."id" ASC LIMIT $1  [["LIMIT", 1]]
   (0.9ms)  SELECT COUNT(*) FROM "roles" INNER JOIN "users_roles" ON "roles"."id" = "users_roles"."role_id" WHERE "users_roles"."user_id" = $1 AND "roles"."name" = $2 AND "roles"."resource_type" = $3 AND "roles"."resource_id" = $4  [["user_id", 43], ["name", :research_management], ["resource_type", "Organisation"], ["resource_id", 5]]
  Proposal Load (0.3ms)  SELECT "proposals".* FROM "proposals" WHERE "proposals"."user_id" = $1  [["user_id", 43]]
 => #<ActiveRecord::Relation [#<Proposal id: 17, user_id: 43, title: "asdf", description: "adsf", byline: "asdf", nda_required: true, created_at: "2016-11-16 00:28:31", updated_at: "2016-11-28 01:16:47", trl_id: 1, invitee_id: nil>]> 
2.3.1p112 :012 > Proposal.where(user_id: User.first).or(Proposal.reviewable)
  User Load (2.3ms)  SELECT  "users".* FROM "users" ORDER BY "users"."id" ASC LIMIT $1  [["LIMIT", 1]]
ArgumentError: Relation passed to #or must be structurally compatible. Incompatible values: [:joins]

The insight I've just received is that the pundit scope cannot query the proposal state because statesman has a series of tables behind the scenes that manage the transitions.

Does anyone see a work around that would let me continue using statesman as well as using Pundit scopes?

damien commented 7 years ago

This doesn't seem to be a problem with Pundit, but with whatever the return value your code is giving you for Proposal.reviewable.

Do you know if Proposal.reviewable is an ActiveRecord relation? I'm not familiar with how Statesman works or what else you have going on in your application, but it may be that you're getting an array of results rather than a relation. ActiveRecord won't be able to do any fancy relational algebra unless you feed it a relation.

MincePie commented 7 years ago

Proposal.reviewable works fine on its own.

My proposal model defines a scope called 'reviewable' which checks the current_state of the proposal.

The problem comes when I try to define the resolve method in the proposal policy in the way that i have shown. The logic is that the index should include all of a users own records and if the user is not the record owner (that created the proposal) and the proposal is reviewable, then the current user should see the reviewable proposals.

Reviewable will return an array of results because it is a scope on the proposal model.

The trouble is that the pundit resolve method doesnt seem to accept a SQL query that includes an or statement. See the note about incompatible search structure at the end of my post.

rystraum commented 7 years ago

@MincePie

Pundit scopes doesn't do anything special. When reading Pundit scopes, just mentally replace the scope keyword with the class name and try to type exactly the same thing into your Rails console.

The error isn't specific to Pundit. It's an error related to the or operator. I took a peek at how statesman implements the in_state call, and it does a join with your ProposalTransition class.

ActiveRecord's or operator requires the left side and the right side to be compatible. In your case, your left-side query (Proposal.where(user: User.first)) is incompatible with your right-side query:

Proposal.reviewable which translates to Proposal.in_state(:under_review) which translates to Proposal.joins(most_recent_transition_join).where(states_where(most_recent_transition_alias, states), :under_review) which does a LEFT OUTER JOIN with the transition table (proposal_transitions, I believe) [1]

To make them compatible, you'd have to apply the same join to both sides of the or.

Since you're already making an expensive call anyway, you could do a poor man's or by doing the following:

def resolve
  proposal_ids = current_user.proposal_ids + Proposal.reviewable.pluck(:id)
  Proposal.where(id: proposal_ids)
end

[1] I looked at statesman's source code: https://github.com/gocardless/statesman/blob/77958f1c2ae613ac29c6db5329c67538a5655ddf/lib/statesman/adapters/active_record_queries.rb

damien commented 7 years ago

@rystraum above and beyond on that little investigation. Nicely done!

MincePie commented 7 years ago

@damien @rystraum - thank you very much for this. It's beyond generous of you to look at this in so much detail. Thank you

ce07c3 commented 7 years ago

@MincePie care to close this one? :)