Fixing bad practices with authorization and pundit-CodePudding

I'm using pundit in a rails app to do authorization and I'm not sure what is the appropriate way to go about this with the setup I have.

I have a namepsaced controller

class Rentals::OffersController < ApplicationController
  before_action :set_rental
  before_action :set_offer, only: [:accept, :reject]

  def index
    @offers = @rental.offers
    authorize [:rental @rental]
  end
end

and also a namespaced policy

class Rentals::OfferPolicy < ApplicationPolicy
    def index?
    end
end

and what I would like to achieve is to authorize with the @rental instance to see if it's created by the user. However this authorize [:rental @rental] does not work as it's not looking for the Rentals::OfferPolicy.

I've got this to work by using a RentalPolicy's edit? method as

class RentalPolicy < ApplicationPolicy

    def edit?
        user.present? && user.id == record.id
    end
end

and then in Rentals::OffersController I have

class Rentals::OffersController < ApplicationController
  before_action :set_rental
  before_action :set_offer, only: [:accept, :reject]

  def index
    @offers = @rental.offers
    authorize @rental, :edit?, policy_class: RentalPolicy
  end
end

but I feel like I'm going against the practices of pundit. How should I achieve this with the way it should be done?

CodePudding user response：

Besides the fact that authorize [:rental @rental] causes a syntax error a big issue with this code is that you're not actually namespacing your code correctly. class Rentals::OffersController does not reopen the Rentals module and set the module nesting so when you reference OfferPolicy Ruby will just look in Rentals::OffersController and the root namespace.

# bad
class Rentals::OffersController < ::ApplicationController
  puts Module.nesting # [ Rentals::OffersController]
  puts OfferPolicy.name # uninitialized constant Rentals::OffersController::OfferPolicy (NameError) 
end

# Good
module Rentals
  class OffersController < ::ApplicationController
    puts Module.nesting # [Rentals, Rentals::OffersController]
    puts OfferPolicy.name # it can correctly find other constants in the Rentals module
  end
end

Using the scope resolution operator (::) is not as many people have been missled to believe just an alternative style of writing nested classes/modules and unfortunately the Rails generators and guides just make the confusion worse. It is not recommended by the Ruby Style Guide.

Instead you should explicitly nest modules/classes.

module Rentals
  class OffersController < ::ApplicationController
    before_action :set_rental
    before_action :set_offer, only: [:accept, :reject]

    def index
      @offers = @rental.offers
      authorize @rental
    end
  end
end

module Rentals
  class OfferPolicy < ::ApplicationPolicy
    class Scope < Scope
      def resolve
        if user
          # this ensures that we are scoped to current_user, without relying on other policies
          scope.joins(:rental).where(rental: { user_id: user.id })
        else
          # no user, no offers
          scope.none
        end
      end
    end
  end
  # ...
end

Since this actually sets the module nesting to [Rentals, Rentals::OffersController ] it will look for the constant RentalPolicy in Rentals before looking in the root namespace (main).

Maybe someday Ruby will actually have a separate namespace operator that lets you define nested classes without the excess indentation but until then you should ensure that your code doesn't do suprising and buggy constant lookups.

CodePudding user response：

If you don't absolutely need namespaced models, I'd suggest don't use them; it'll make things easier. Rental and RentalOffer or Offer is just fine.

Use policy scope to narrow the offers to current user https://github.com/varvet/pundit#scopes

# controller

# GET /rentals/1/offers
# based on the url, we should authorize rental#1 and scope the offers
def index
  # since "get /rentals/1" maps to show action
  authorize @rental, :show?
  
  # if user should have access to everything rental related, then authorizing @rental is enough.
  # @offers = @rental.offers

  # this is a must if url is not nested (get /offers)
  @offers = policy_scope([:rentals, Offer]).where(rental_id: @rental.id)
end

The depth of authorization, really, depends on how granular you want/need to be. Per role, per resource, per action, per attribute, per attribute value etc.

# rental policy

class RentalPolicy < ApplicationPolicy
  def show?
    user && user.id == record.user_id
  end
end

# offer policy 

class Rentals::OfferPolicy < ApplicationPolicy
  class Scope < Scope
    def resolve
      if user
        # this ensures that we are scoped to current_user, without relying on other policies
        scope.joins(:rental).where(rental: { user_id: user.id })
      else
        # no user, no offers
        scope.none
      end
    end
  end

  # ...
end