Implementing scopes in Pundit

Question

I am using the Pundit gem (with Devise and Rolify) to restrict access to information based on logged-in user roles.

At this time I have three roles for my User model defined: Admin, Client Admin, and Customer Admin.

A User belongs_to a Customer. Customer has_many Users.

I have successfully implemented a Pundit policy when indexing the Customer model. Admins and Client Admins can see all Customers. Customer Admin can only see their OWN record.

The problem lies when I am trying to restrict the show method of the Customer controller. Admins and Client Admins can see all Customers. However, the Customer Admin should only be able to see his own record. But as it stands the Customer Admin can input any id in the URL and see any Customer record.

I'm fuzzy on the scoping. It's my understanding that the Policy methods (i.e. index? and show?) are to restrict WHO can perform these actions and the Scoping methods restrict WHICH RECORDS can be obtained. I'm having trouble composing the correct scope for the above scenario.

Here's the Customer controller:

class CustomersController < ApplicationController
  before_action :set_customer, only: [:show, :edit, :update, :destroy]
  after_action :verify_authorized

  # GET /customers
  # GET /customers.json
  def index
    @customers = policy_scope(Customer)
    authorize Customer
  end

  # GET /customers/1
  # GET /customers/1.json
  def show
    authorize @customer
  end

  # GET /customers/new
  def new
    @customer = Customer.new
    authorize @customer
  end

  # GET /customers/1/edit
  def edit
    authorize @customer
  end

  # POST /customers
  # POST /customers.json
  def create
    @customer = Customer.new(customer_params)
    authorize @customer

    respond_to do |format|
      if @customer.save
        format.html { redirect_to @customer, notice: 'Customer was successfully created.' }
        format.json { render :show, status: :created, location: @customer }
      else
        format.html { render :new }
        format.json { render json: @customer.errors, status: :unprocessable_entity }
      end
    end
  end

  # PATCH/PUT /customers/1
  # PATCH/PUT /customers/1.json
  def update
    authorize @customer
    respond_to do |format|
      if @customer.update(customer_params)
        format.html { redirect_to @customer, notice: 'Customer was successfully updated.' }
        format.json { render :show, status: :ok, location: @customer }
      else
        format.html { render :edit }
        format.json { render json: @customer.errors, status: :unprocessable_entity }
      end
    end
  end

  # DELETE /customers/1
  # DELETE /customers/1.json
  def destroy
    authorize @customer
    @customer.destroy
    respond_to do |format|
      format.html { redirect_to customers_url, notice: 'Customer was successfully destroyed.' }
      format.json { head :no_content }
    end
  end

  private
    # Use callbacks to share common setup or constraints between actions.
    def set_customer
      @customer = Customer.find(params[:id])
    end

    # Never trust parameters from the scary internet, only allow the white list through.
    def customer_params
      params.require(:customer).permit(:name, :parent_customer_id, :customer_type, :active, :currency)
    end
end

And here is the Customer policy:

class CustomerPolicy < ApplicationPolicy

  def index?
    # Admins, ClientAdmins, and CustomerAdmins can index customers (see Scope class for filters)
    @user.has_role? :admin or @user.has_role? :client_admin or @user.has_role? :customer_admin
  end

  def show?
    # Admins, ClientAdmins, and CustomerAdmins can see any customer details
    @user.has_role? :admin or @user.has_role? :client_admin or @user.has_role? :customer_admin
  end

  def update?
    # Only Admins and ClientAdmins can update customer details
    @user.has_role? :admin  or @user.has_role? :client_admin
  end

  def destroy?
    @user.has_role? :admin or @user.has_role? :client_admin
  end

  class Scope < Struct.new(:user, :scope)
    def resolve
      if (user.has_role? :admin or user.has_role? :client_admin)
        # Admins and ClientAdmins can see all Customers
        scope.where(:parent_id => nil)
      elsif user.has_role? :customer_admin
        # Customer Admins can only see their own Customer
        scope.where(:id => user.customer) # THIS DOES NOT APPEAR TO GET INVOKED BY THE SHOW METHOD OF THE CONTROLLER
      end
    end    

    def show?
      # NOT SURE WHAT TO PUT IN HERE
    end
  end
end

---

Success!! Thanks to the headstart given to me by railscard, the trick was to modify the show? method in the Customer policy file like the following:

  def show?
    # Admins, ClientAdmins, and CustomerAdmins can see any customer details
    # Students cannot see customer details

    return true if user.has_role?(:admin) || user.has_role?(:client_admin)
    return true if user.customer_id == @record.id && user.has_role?(:customer_admin)
    false
  end

Note that I had to use the @record instance variable, as that's what the Application policy class uses to refer to the record being passed in by the authorize method.

Thanks!!

Mowry

Answer 1

I think you don't need scope to restrict access for show action.

def show?
  return true if user.has_role? :admin || user.has_role? :client_admin
  return true if user.customer_id == customer.id && user.has_role? :customer_admin
  false
end

Pundit scopes usually used to fetch a list of records which user have access to. In case of show method (or any other method in controller, where you call authorize) Pundit instantiates policy class with current user and given customer and then simply calls show? method to check user permissions, i.e. CustomerPolicy.new(current_user, @customer).show?

Weese

Thanks in advance for the help. I understand what you're saying -- scope should be used when I expect to return a collection. But I'm not sure where to put this method you've described. Would that be in the Customer policy? Sorry. This is making me feel completely dumb. – Mowry

To be clear: I've tried inserting the method in the Customer policy file and I'm not seeing any difference in behavior. I guess I'm not sure what would trigger that method. – Mowry

GOT IT! You're my hero thank you SO much. I will post revised code in the question, but this was nearly exactly the right answer. – Mowry

why do you return true if condition returns it? I would write the method body in one string user.has_role?(:admin) || user.has_role?(:client_admin) || (user.customer_id == customer.id && user.has_role?(:customer_admin)) – Cadaverine

@mpugach, I'm not a big fan of complex one-liners, so I think my option looks a little bit more readable – Weese

Answer 2

To get Pundit's scoping working for the show action, Pundit's policy_scope helper (or policy_scope!) could be used, or you could just inherit show? from the generated ApplicationPolicy.

The index action is already using policy_scope correctly, we just need to do something similar for the show action. Here are some options:

Option 1: Modify the show action to

def show
  # Also remove :show from the :only option where
  # before_action :set_customer, only: ... is called.
  @customer = policy_scope(Customer).find(params[:id])
  authorize @customer
end

OR

Option 2: Modify set_customer to

def set_customer
  @customer = policy_scope(Customer).find(params[:id])
end

OR

Option 3: Modify CustomerPolicy#show? to

def show?
  # scope call here will return the 
  # result of CustomerPolicy::Scope#resolve
  # This is the same implementation generated
  # in the default ApplicationPolicy so you could
  # just delete this method here and inherit instead.
  scope.where(:id => record.id).exists?
end

Here's the code that generates the default ApplicationPolicy#show? method.

See Pundit's README section on Scopes for additional details.

I think you can safely delete the empty show? method you have in CustomerPolicy::Scope, I don't believe it will be called.

Scanty

I like Option 3 you have there! However it seems in 2016, there was a pull request merged which changed the way this worked. There was a brief discussion in the linked pull request, but I don't quite follow what their suggested alternative is, if we're to no longer to use scope.where(id: record.id).exists?. Should we just, on a case-by-case basis, put back the scope method in the policy files we need it in? – Catrinacatriona