Why does a Pundit policy for one controller is affected by another?

Question

I am perhaps misunderstanding Pundit policies but I am facing an issue where the UserPolicy is clashing with the SongPolicy.

What happens if that a statement in UserPolicy is being asserted ignoring what's written in SongPolicy:

Pundit::NotAuthorizedError in SongsController#edit 

not allowed to edit? this User

  def authorization
    authorize Current.user
  end

The issue emerged after introducing a new role for users but I believe that I probably haven't configured it right and for some reason only UserPolicy is looked at for asserting authorization in the SongsController?

I have two controllers that check for the user to be signed in (require_user_logged_in) and another to check on Pundit's policies (authorization):

class UsersController < ApplicationController
  before_action :require_user_logged_in!, :authorization, :turbo_frame_check
  # Actions were removed for brevity.
end

class SongsController < ApplicationController
  before_action :require_user_logged_in!, :authorization, except: [:index, :show]
  # Actions were removed for brevity.
end

The authorization methods looks like this:

  def authorization
    authorize Current.user
  end

There's an application-level policy class, ApplicationPolicy:

# frozen_string_literal: true

class ApplicationPolicy
  attr_reader :user, :params, :record

  # Allows params to be part of policies.
  def initialize(context, record)
    if context.is_a?(Hash)
      @user = context[:user]
      @params = context[:params]
    else
      @user = context
      @params = {}
    end
    @record = record
  end

  def index?
    false
  end

  def show?
    false
  end

  def create?
    false
  end

  def new?
    create?
  end

  def update?
    false
  end

  def edit?
    update?
  end

  def destroy?
    false
  end

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

    def resolve
      raise NotImplementedError, "You must define #resolve in #{self.class}"
    end

    private

    attr_reader :user, :scope
  end
end

The UserPolicy to protect user views:

class UserPolicy < ApplicationPolicy
  class Scope < Scope
  end

  def index?
    user.has_role?(:admin)
  end

  def show?
    # Access if admin or the same user only.
    user.has_role?(:admin) || is_same_user?
  end

  def create?
    index?
  end

  def new?
    create?
  end

  def update?
    index? || is_same_user?
  end

  def edit?
    update? # This is called when accessing a view for `SongsController`.
  end

  def destroy?
    index? || is_same_user? 
  end

  def delete?
    destroy?
  end

  private

  # Used to keep a user from editing another.
  # Admins should be allowed to edit all users.
  def is_same_user?
    # Check if user being accessed is the one being logged in.
    params[:id].to_s == Current.user.username.to_s
  end
end

And the SongPolicy:

class SongPolicy < ApplicationPolicy
  class Scope < Scope
  end

  def index?
  end

  def show?
  end

  def create?
    user.has_role?(:admin) || user.has_role?(:collaborator) # This is ignored.
  end

  def new?
    create?
  end

  def update?
    create?
  end

  def edit?
    create?
  end

  def destroy?
    user.has_role?(:admin)
  end

  def delete?
    destroy?
  end
end

Not sure what else to try here, I'm sure I'm missing something, if someone with more knowledge of Pundit could let me know their thoughts on why a statement for one policy can leak into another, it would be really helpful.

Answer 1

You're calling authorize on the current user, which is a User, so Pundit is going to infer the UserPolicy policy. It won't automatically infer the SongPolicy policy unless you provide a Song record, even if you're in the SongController controller.

If you want to use a different policy, you'll need to provide it via authorize(policy_class:).

authorize Current.user, policy_class: SongPolicy

Implicit authorization like this is generally a code smell. Ideally, you should be explicitly authorizing the current Song record(s) against the current user context.

Answer 2

My opinion is that you have a misconception on Pundit's approach. Particularly you're twisting the subject of the authorization with the object of the authorization. Let's try to explain.

You have an actor who wants to apply an action on an object. The object may be authorized to receive the action.

By default Pundit's always consider the actor to be the current_user.

The action is a method on a Policy.

The object is the resource you're working on; in the most trivial scenario it could be an ActiveRecord object - but it doesn't have to.

Pundit's authorize methods is intended, in plain english, as "authorize the the resource bar to receive the action foo from the current user". What you're trying to do is instead "authorize the current user to apply the action foo on the resource bar.

What's the difference? The subject and the object of the authorization are swapped. IMO, while doing the authorization process, you should respond to the question: "Is this object authorized to receive this action by the actor?"

          object        action
          ------------  ------
authorize Current.user, :edit?

NOTE: the actor implicitly is current_user

NOTE: if action is not declared, then it will implicitly be action_name

which resolves to the question "is this specific user authorized to receive :edit? from the current user?"

Following the reasoning, this is what I'd consider the right approach for your example scenario:

class SongsController < ApplicationController
  before_action :require_user_logged_in!, :authorization, except: [:index, :show]

  private

  def authorization
    authorize Song
  end
end

I do not advise to rely on callbacks and I'd rather write more explicit code

def edit
  @song = Song.find(params[:id])
  authorize @song, :edit?
end

This code resolves to the question "is this specific song authorized to receive :edit? from the current user?"

A word of warning about using a custom policy_class

like in

authorize Current.user, policy_class: SongPolicy

With this code the authorization will be made by calling SongPolicy#edit? but the record will regularly be set to Current.user's value ; let's suppose to have

class SongPolicy
  def edit?
    record.in_my_playlist?
  end
end

where in_my_playlist? is Song's instance method: you'll end having

undefined method `in_my_playlist?` for #<User>

Probably you're not doing the thing you intended to do there.

A word of warning about the use of Current.user into your logic

If Current.user is using http://api.rubyonrails.org/classes/ActiveSupport/CurrentAttributes.html and your entire application is relying on that singleton, then you probably want to redefine Pundit's default user as documented here

class ApplicationController < ActionController::Base
  def pundit_user
    Current.user
  end
end

otherwise you'll end up having your business logic and your authorization logic relying on two - potentially - different sources of truth.