Simple DRY Rails - check if a model belongs to a user

Question

I can clearly see I'm repeating myself, but I don't know how to refractor this without making it really ugly. What I have:

class ChildModelController < ApplicationController

    def new
        @model = ParentModel.find(params[:id])
        # Does the current user have permissions on the parent model    
        if signed_in?
          unless current_user.admin? or current_user.id == @model.user_id
            flash[:error] = t('errors.messages.not_authorized')
            flash.keep
            redirect_to root_path
          end
        else
          unless session and session[:session_id] == @model.author_session_token
            flash[:error] = t('errors.messages.not_authorized')
            flash.keep
            redirect_to root_path
          end
        end
        #do stuff to the model
    end

end

The only way I can think to make it smaller is to make it ugly:

class ChildModelController < ApplicationController

    def new
        @model = ParentModel.find(params[:id])
        # Does the current user have permissions on the parent model    
        if (!signed_in? and session and session[:session_id] == @model.author_session_token ) || (signed_in and current_user.admin? or current_user.id == @model.user_id)
            flash[:error] = t('errors.messages.not_authorized')
            flash.keep
            redirect_to root_path
          end

        #do stuff to the model
    end

end

Am I missing an even easier way?

Answer 1

I suggest you extract two methods: authorized? to check if the user is authorized, and not_authorized to handle behaviour. You would then end up with this:

def new
  @model = ParentModel.find(params[:id])
  not_authorized unless authorized?
end

private

def authorized?
  if signed_in?
    # If you have your associations setup correctly you don't have 
    # to check equality on id. You can just compare the objects
    current_user.admin? || current_user == @model.user
  else
    # See aside number 2 for why I removed session
    # session[:session_id] will return nil if it's not present
    # which is false in ruby, so it's safe to do this
    session[:session_id] == @model.author_session_token
  end
end

def not_authorized
  flash[:error] = t('errors.messages.not_authorized')
  flash.keep
  redirect_to root_path
end

But I suggest you think about using an authorisation library, or extracting authorisation logic into a policy object. This way you gain the benefit of DRY, and an extensible way to add and reuse permissions.

def new
  @model = ParentModel.find(params[:id])
  not_authorized unless authorized?(current_user, @model)
end

In a separate policy object you could:

# Choose a better name for this
class ModelPolicy
  attr_reader :user, :model

  def initialize(user, model)
    @user, @model = user, model
  end

  def authorized?
    user.admin? || user.id == model.user_id
  end
end

This example illustrates my point but it is not complete. You must wire the policy to the controllers, maybe include it in a concern. But it's not that hard.

There's a great gem called Pundit that gives you similar interface.

Asides

1. Try not to use unless with || and && conditions because it becomes confusing. It's clearer to use negation: !current_user. Some people are boolean impaired, including myself.
2. I'm not sure under what circumstances the session hash does not exist. So checking for its presence may be redundant.

Answer 2

Unless I'm not understanding something about your code, can't you just move the duplicated code into a method?

class ChildModelController < ApplicationController
  def new
    @model = ParentModel.find(params[:id])
    # Does the current user have permissions on the parent model
    if signed_in?
      unless current_user.admin? or current_user.id == @model.user_id
        not_authorized
      end
    else
      unless session and session[:session_id] == @model.author_session_token
        not_authorized
      end
    end
    #do stuff to the model
  end

  def not_authorized
    flash[:error] = t('errors.messages.not_authorized')
    flash.keep
    redirect_to root_path
  end
end

The conditions are fine as they are, maybe you should move them into a separate check_auth method when new gets longer, or you need the same checks in other places.