Conversation about authorization, business logic, and custom error messages

[uberllama]

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:

1. Pro: the policy could be used within a view to fully represent the rules around creating a Post.
2. Grey area: We're getting into tertiary data by looking at the record's Event association. This is a slippery slope.
3. 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.

[uberllama]

Examples had some missing code. Updated.

[jnicklas]

I would personally always go for option 1. I think reaching into the secondary object is not so bad. As for custom error messages, IMO you should be preventing users from creating incorrect posts in the first place, that's usually better UX. So hide the "new post" form, and replace it with "you cannot post to past events" or something. The one place where this doesn't work is if what you're designing is a pure API. In that case I don't really have a very good answer.

I don't like splitting policies along roles. Policies are already split on domain models and this makes them even more fragmented.

Service objects are good sometimes, but IMO not in this case. The whole point of Pundit is to encapsulate this kind of logic in objects. If you're introducing another kind of object to do this, you're back to square one, plus you're violating SRP.

[uberllama]

Thanks for your response and thoughts, Jonas. Option 1 definitely seemed to cover most bases and do the best encapsulation, except the handling of multiple error messages depending on which facet of the policy method failed. Any thoughts on passing through specific messages to the NotAuthorized exception from within a policy method?

Using this example, we'd raise a standard NotAuthorized if the user wasn't logged in, a "You don't have permission" message if some generic condition failed, or a "This event has passed" message if the event has passed.

[jnicklas]

@uberllama as I said, IMO you shouldn't present this error to the user in the first place. Prevent the user from performing an action which could lead to authorization failure before they do it.

If that's not possible, you can use the information in the error raised by Pundit: https://github.com/elabs/pundit#creating-custom-error-messages

[uberllama]

My use case is actually an API, so I don't have full control of requests. Even if I could, there are always race-type conditions (event passed while looking at the page). I did read through the custom error messages section but its still based on a single message per policy/query. In this case, my requirement is for the create query to return multiple errors depending on which aspect of authorization within a single query failed.

[vasilakisfil]

@uberllama I would override pundit to return an object instead of true/false and check if the object has any errors like you do in AR objects. I have many problem's with pundit's true/false authorization philosophy.. not everything is black and white, the user might have access but a restricted one. Eventually I monkeypatched pundit and I am happy with it.

[uberllama]

@vasilakisfil Nice! Would you be willing to publish a gist, or a fork?

[vasilakisfil]

@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:

1. 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?

[foxyblocks]

What about being able to override the exception from within the policy methods themselves and still returning a boolean result:

class ApplicationPolicy
  attr_accessor :error
end

class PostPolicy < ApplicationPolicy
  def create?
    return false unless user
    if record.event.ends_at.past? && !user.admin?
      # custom error
      error = Pundit::NotAuthorizedError.new("can't post to an event in the past")
      return false
    end
    return true
  end
end

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

    policy = policy(record)
    unless policy.public_send(query)
      error = policy.error || NotAuthorizedError.new("not allowed to #{query} this #{record}")
      error.query, error.record, error.policy = query, record, policy

      raise error
    end

    true
  end
end

module Pundit
  prepend AuthorizeWithCustomException
end

This would also allow you to create custom exception classes and react to them differently in your controller (e.g. handle redirection differently).

Plus it maintains current behavior.

[vasilakisfil]

Personally I avoid handling logic through exceptions since exceptions in ruby are slow. Btw I have revised and expanded my previous comment here.

[uberllama]

Interesting solutions, thanks guys.

[waiting-for-dev]

I'm also in the middle of a pure API application and I would like to also render custom error messages depending on the unauthorization reason. I think I'm going to try to handle it just adding error messages to models from policies and adapting the API to work with them. Surely this is a bit of overuse of Rails model error intention (they are not validation errors, another option it is just to add another hash to the objects) but I don't feel too bad relying on Pundit to add the errors. One could say that it is the same responsibility to authorize and if not to inform why.

Something like:

class ApplicationPolicy
  def unauth(message)
    record.errors.add(:base, message)
    return false
  end
end

class PostPolicy < ApplicationPolicy
  def create?
    return unauth('User not authenticated') unless user
    if record.event.ends_at.past? && !user.admin?
      return unauth("Can't post to an event in the past")
    end
    return true
  end
end

[damien]

Great discussion! I think this is a common need a lot of people using authentication systems run into; the team I'm on certainly is. Any chance of some of this thread making it into the Pundit wiki or README?

[vasilakisfil]

hmm not sure, we haven't even reached a consensus here! But @damien if you have any suggestion let us know, personally I mainly build APIs and I am running to the same problem all the time!

[damien]

A developer on my team kindly made a PR that implements this within our own codebase. He was thinking about making a gem that did this, I'll see if I can have him or I make an excerpt of the solution so we can get feedback from you guys.

[damien]

Update: My good friend @luisdaher published a gemified version of an initial solution to this problem: https://github.com/luisdaher/pundit_custom_errors

[jnicklas]

@damien @luisdaher: this actually seems like a pretty nice solution. I'm a bit worried about the statefulness of it, since policies are cached, but maybe that's just being overly cautious. This is how ActiveRecord does error conditions as well, so it's not without precedent (although I think it's a bit of a bad idea in AR as well, to be honest).

If you want to provide a PR to integrate this functionality into Pundit I think I'd be in favour of merging that. @thomasklemm what do you think?

[damien]

@jnicklas That would be an attractive proposal for the org I work at; maintaining such a library takes a lot of work and getting it into the pundit mainline would ensure support/compatibility for this functionality doesn't go away in future versions. I may be able to get the go ahead to allocate engineering time to this, but failing that I wouldn't mind drafting a PR for this over a few odd weekends. @luisdaher: thoughts?

[luisdaher]

@damien @jnicklas I agree. @damien , I don't mind using some of my free time to draft a PR as well :)

[jnicklas]

Closing this for now. Please send a PR if you're still interested in this.

[damien]

Should I take this to mean that the proposed changes for custom error messages are acceptable to pundit core? If so, I'll see what I can do about drafting a PR for this.

[openface]

@damien I'd be interested in this PR. I'm currently having to create custom query methods to accommodate needing to have specific error messages. Ie: #update_foo? #update_bar? etc.

[vraravam]

@jnicklas - the closing comment is ambiguous to me. Is this functionality present in pundit mainline as of now?