pundit: Conversation about authorization, business logic, and custom error messages

Have been having an interesting conversation with some colleagues today and was wondering where you folks stand on it.

The question is: where do you draw your line between authorization and business logic/context? Do you use Pundit purely do define who can do what based on their role, or do you bring your deeper contexts into the picture?

Here’s a plain english version of the scenario I’m dealing with:

Our system has a User model, a Post model, and an Event model. Users have roles (“admin” and “user”).

Post belongs_to Event, but can also stand alone.
Any user of any role can create Posts not associated with an Event.
Admin users can create Posts for any Event.
Regular users can only create Posts for Events that haven’t passed (if they try to create an Event for a past Post, they should get a custom error message).

Option 1 Put everything in a Pundit Policy

If I were to do this all in Pundit, the PostPolicy and controller would look something like this:

class PostPolicy

  def create?
    user.admin? || record.event_id.nil? || record.event.ends_at > Time.current
  end

end

class PostsController < ApplicationController

  before_action :set_post

  def create
    authorize @post
    if @post.save...
  end

  private

  def set_post
    @post = current_user.posts.build(post_params)
  end

end

This works… with caveats:

Pro: the policy could be used within a view to fully represent the rules around creating a Post.
Grey area: We’re getting into tertiary data by looking at the record’s Event association. This is a slippery slope.
Con: We don’t have the ability to return a custom error message in the case of an expired Event (do we?). If authorization fails for whatever reason, you can only generate a singular error message based off the exception on PostPolicy#create.

Option 2 Create multiple policies

class PostPolicy

def create?
  record.event_id.nil? || record.event.ends_at > Time.current
end

class AdminPostPolicy

def create?
  user.admin?
end

Assuming we could split policies cleanly along role lines, we might be able to do something like this, which opens up slightly more messaging options. Uncertain at this time, so this is the least fleshed out example.

Option 3 Use Pundit purely for role checking (signed in, can create Posts) and put business logic in controllers

class PostPolicy < ApplicationPolicy
  # ApplicationPolicy ensures user is logged in
end

class PostsController < ApplicationController

  before_action :set_post
  before_action :set_event

  def create
    authorize @post
    if @post.save...
  end

  private

  def set_post
    @post = current_user.posts.build(post_params)
  end

  def set_event
    return unless params[:post][:event_id].present?
    @post.event = Event.find(params[:post][:event_id])
    render_errors("custom message") unless current_user.admin? || @post.event.ends_at > Time.current
  end

It gets the job done, and can be different if we have a PostsController under /admin, but can end up fairly wet. Also, we lose the singular authority on when a user can create a post if we need the same total logic in the view.

Option 4 Use Pundit purely for role checking (signed in, can create Posts) and put business logic in service objects

class PostPolicy < ApplicationPolicy
  # ApplicationPolicy ensures user is logged in
end

class PostsController < ApplicationController

  def create
    post_creator = PostCreator.new(post_params, current_user)
    authorize post_creator.post, :create?
    if post_creator.save
      render post_creator.post
    else
      render_errors(post_creator.errors)
    end
  end

class PostCreator

  attr_accessor :post, :errors, :current_user

  def initialize(params, current_user)
    self.current_user = current_user
    self.post = current_user.posts.build(params)
  end

  def save
    if current_user.admin? || post.event_id.nil? || post.event.ends_at > Time.current
      post.save
    else
      self.errors = "custom message"
      false
    end
  end
end

Cleaner than controllers, but again you’ve split out your authority into two places. Also, I originally had the pundit authorize call within the service object but don’t actually know if that’s possible.

Another option would be to pass in the current_user context to the model and run the event check as a validation, but that is exceedingly gross, and I find my models are getting thinner and thinner these days (basically just wrappers around the table with rudimentary validation).

Feedback appreciated. Cheers.

About this issue

Original URL
State: closed
Created 10 years ago
Comments: 23

Most upvoted comments

@uberllama I had monkeypatched pundit to another project to allow guest users to authorized like that:

module AuthorizeWithReturn
  def authorize(record, query=nil)
    query ||= params[:action].to_s + '?'
    @_pundit_policy_authorized = true

    policy = policy(record)
    return true if policy.public_send(query)
  end
end

module Pundit
  prepend AuthorizeWithReturn
end

and checking true/false in the controller.

But now I also built an API for a non trivial project so I am at the same position. My current approach is the following. Given this simple controller action:

  def show
    user = User.find(params[:id])
    authorize user

    render json: user, serializer: Api::V1::UserSerializer
  end

when I do authorize user I want to know 3 things:

if user is authorized to access the resource in specific action, 2) if there are any authorization errors, 3) what permissions has the user in the resource attributes

If I don’t know any of these then this means that I have to duplicate the pundit’s logic on authorization somewhere else. So essentially I need to get an object from authorization that will tell me if there are any errors to show or the attributes that the serializer will serialize. So my controller action would look like:

  def show
    user = User.find(params[:id])
    authorized_user = authorize user
    return api_error(status: 422, authorized_user.errors) unless authorized_user.errors.nil?

    render(
      json: Api::V1::UserSerializer.new(
        authorized_user.record,
        only: authorized_user.attributes
      ).to_json
    ) 
  end

Where attributes includes all the attributes that the serializer will serialize (with the association attributes)

Given that, I am considering the following approach: First monkey patch pundit to return the actual value of the method call based on the policy when you call authorize resource

module AuthorizeWithReturn
  def authorize(record, query=nil)
    query ||= params[:action].to_s
    @_pundit_policy_authorized = true

    policy = policy(record)
    policy.public_send(query)
  end
end

module Pundit
  prepend AuthorizeWithReturn
end

Then inside the pundit policy, I would do something like:

class UserPolicy < ApplicationPolicy
  def show
    #you can add errors to the object, if you have a non trivial authorization condition
    return Permissions::Show::Admin.new(record) if user.super_admin?
    return Permissions::Show::Owner.new(record) if record.association1.eql?(user)
    return Permissions::Show::Regular.new(record) if record.association2.include?(user)
    return Permissions::Show::Regular.new(record) if record.association3.users.include?(user)
    return Permissions::Show::Guest.new(record)
  end

And define Permissions object for Users like:

  class Permissions
    attr_accessor :attributes, :errors
    attr_reader :record

    def initialize(record)
      @record = record
      @errors = []
    end

    def attributes
      [
        :id, :email, :password, :password_confirmation, :first_name,
        :last_name, :image_public_url, :user_type, :created_at, :updated_at, :association1,
        :association2
      ]
    end

    class Show < self

      class Admin < self
      end

      class Owner < self
        def attributes
          super - [:association2]
        end
      end

      class Regular < self
        def attributes
          super - [:created_at, :updated_at, :association2]
        end
      end

      class Guest < self
        def attributes
          [:first_name, :image_public_url]
        end
      end
    end
  end

You can use activemodel errors but it won’t be POROs.

Unfortunately with OO it’s getting veery verbose, but you can get rid of on level of inheritance if you have the same attributes for all actions (index/show/update/destroy) for each level of authorization. So it could be like:

class UserPolicy < ApplicationPolicy
  def show
    #you can add errors to the object, if you have a non trivial authorization condition
    return Permissions::Admin.new(record) if user.super_admin?
    return Permissions::Owner.new(record) if record.association1.eql?(user)
    return Permissions::Regular.new(record) if record.association2.include?(user)
    return Permissions::Regular.new(record) if record.association3.users.include?(user)
    return Permissions::Guest.new(record)
  end

and:

  class Permissions
    attr_accessor :attributes, :errors
    attr_reader :record

    def initialize(record)
      @record = record
      @errors = []
    end

    def attributes
      [
        :id, :email, :password, :password_confirmation, :first_name,
        :last_name, :image_public_url, :user_type, :created_at, :updated_at, :association1,
        :association2
      ]
    end

    class Admin < self
    end

    class Owner < self
      def attributes
        super - [:association2]
      end
    end

    class Regular < self
      def attributes
        super - [:created_at, :updated_at, :association2]
      end
    end

    class Guest < self
      def attributes
        [:first_name, :image_public_url]
      end
    end
  end

I can’t find anyother way in my case where I have 4-5 levels of authorization. @jnicklas any suggestion?

vasilakisfil on Nov 21, 2014