DRY concepts in rails controllers

Question

I'm working on my first rails api server.

I've got a controller for my User model that looks as such:

class UsersController < ApplicationController
  def index
    if current_user.admin?
      @users = User.all
      render json: @users
    else
      render json: { message: 'You do not have the appropriate permissions to access this resource' }, status: 401
    end
  end

  def show
    if User.exists?(@id)
      @id = params[:id]
      if current_user.id.to_s == @id || current_user.admin?
        @user = User.find(@id)
        render json: @user
      else
        render json: { message: 'You do not have the appropriate permissions to access this resource' }, status: 401
      end
    else
      render json: { message: 'Requested resource not found' }, status: 404
    end
  end
end

What I want and currently have for these two controller methods is:

• /users fetch all users only if the authenticated user making the request is of role admin
• /users/:id fetch a user by id only if the authenticated user making the request has a matching id or is of role admin

The current implementation breaks the DRY philosophy. The reasoning is that the logic for handling whether or not the requesting user has the permissions to access the requested resource(s) is repeated across both controller methods. Furthermore, any model's controller method for show will repeat the logic for checking whether or not the requested resource exists. I also feel like this kind of implementation makes for fat controllers, where I'd rather them be skinny.

What I want to know from the community and from those that have solved this problem before; what is the best way to go about this in order to conform to the DRY philosophy and to keep controllers skinny.

Good to know: I'm using devise and devise-token-auth for authentication.

Answer 1

You need to use some kind of Authorization gem like cancancan. It is exactly what you need. Also it's else not elsif. elsif is followed by condition.

Answer 2

You can use github.com/varvet/pundit instead, for authorization.

It matches with the controller, instead of putting the authorization in the controller, you can use this to move out the authorization to another class.

I have used this across multiple Rails/Rails-API projects and didn't encounter a problem so far.

Instead of writing the code above. You can do this instead.

Also, prioritize early returns over nested ifs for readability.

In your controller.

class UsersController < ApplicationController
  def index
    authorize User # This will call the policy that matches this controller since this is UsersController it will call `UserPolicy`

    @users = User.all

    render :json => @users
  end

  def show
    @user = User.find_by :id => params[:id] # Instead of using exists which query the data from db then finding it again, you can use find_by which will return nil if no records found.

    if @user.blank?
      return render :json => {:message => 'User not found.'}, :status => 404
    end

    authorize @user # This will call the policy that matches this controller since this is UsersController it will call `UserPolicy`

    render :json => @user
  end
end

In your Policy

class UserPolicy < ApplicationPolicy
  def index?
    @user.admin? # The policy is called in controller then this will check if the user is admin if not it will raise Pundit::NotAuthorizedError
  end

  def show?
    @user.admin? || @record == @user # The policy is called in controller then this will check if the user is admin or the user is the same as the record he is accessing if not it will raise Pundit::NotAuthorizedError
  end
end

In your ApplicationController

class ApplicationController < ActionController::API
  include Pundit

  rescue_from Pundit::NotAuthorizedError, :with => :show_forbidden



  private

  def show_forbidden exception
    return render :json => {
      :message => 'You are not authorized to perform this action.'
    }, :status => 403
  end
end