3
\$\begingroup\$

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?

\$\endgroup\$
3
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ I am actually using Pundit for other things, but I haven't had the time to dive deep into it. Thanks! \$\endgroup\$ – Jeff Jan 17 '15 at 14:22
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Oh your understanding it. I just was trying to fix my conditionals so much I didn't think of that (doh). \$\endgroup\$ – Jeff Jan 17 '15 at 13:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.