6
\$\begingroup\$

I created a simple Authorization module with Rails. I found that there are other authorization systems, such as CanCanCan, but they grant permissions at Model level and, for this particular website I am developing, it is more convenient to authorize at Controller level.

The idea is simple: the permissions are stored in a YAML file in the config directory and a function checks if the combination of admin_role, controller and action exist in the config file. There is also a wildcard :all.

# app/controllers/admins/base_controller.rb
class Admins::BaseController < ApplicationController

  include Admins::AuthorizationHelper

  before_action :authorize_admin

  ...

end
# app/helpers/admin/authorization_helper.rb
module Admins::AuthorizationHelper

  private

  def authorize_admin
    unless is_authorized? params[:controller], params[:action]
      refuse_access_to_admin_site
    end
  end

  def is_authorized?(controller, action)
    permissions = Rails.application.config_for :admin_auth
    authority = current_admin.authority

    if permissions[authority].nil?
      false
    elsif permissions[authority] == ['all']
      true
    elsif permissions[authority][controller].nil?
      false
    elsif permissions[authority][controller] == ['all']
      true
    elsif permissions[authority][controller].include? action
      true
    else
      false
    end
  end

  def refuse_access_to_admin_site
    flash[:error] = 'Permission denied'

    if request.referer.present?
      redirect_to :back
    else
      redirect_to admins_products_path
    end
  end

end
# config/admin_auth.yml   
defaults: &defaults
    super_admins:
        - all
    admins:
        admins/shops:
            - index
            - show_used_history
        admins/products:
            - index
        admins/prescriptions:
            - all
        admins/products:
            - index
    operators:
        admins/products:
            - index
        admins/prescriptions:
            - all
        admins/shipments:
            - index
            - show
test:
    <<: *defaults
development:
    <<: *defaults
staging:
    <<: *defaults
production:
    <<: *defaults
\$\endgroup\$
  • 1
    \$\begingroup\$ I don't know much about rails, what's the point of this? if permissions[authority].nil?;false. Why not just deny permission if they no permission? \$\endgroup\$ – 13aal Dec 13 '15 at 18:07
  • \$\begingroup\$ I was trying to avoid getting and exception while checking for permissions[authority][controller] if authority was nil. But this is because I still didn't know the method fetch( , []) for arrays. ;) \$\endgroup\$ – Daniel Dec 14 '15 at 1:11
  • \$\begingroup\$ Awh I didn't know that either, makes sense. \$\endgroup\$ – 13aal Dec 14 '15 at 1:40
3
\$\begingroup\$

There's a lot of repeated code in that long if..else. Here's an idea (code not tested) for shortening it up:

def is_authorized?(controller, action)
  permissions = Rails.application.config_for :admin_auth
  authority = current_admin.authority

  global_auth = permissions.fetch(authority, [])
  return true if global_auth.include? 'all'

  # Note the implicit assumption in this code, and the original code,
  # that global_auth is a Hash if we have not already returned

  ctrl_auth = global_auth.fetch(controller, [])
  ctrl_auth.include? 'all' || ctrl_auth.include? action
end

Please see the inline comment for an assumption your making about the config data.

At a higher level, the deeper reason this code has to be overly complicated and do nil checks and checks for different kinds of data (array or hash) is because the config data structure has no regularity.

The real fix here is probably to wrap that returned data structure in an object that regularizes it, essentially using something similar to the NullObject pattern. So that your code could then read like this:

permissions = Rails.application.config_for :admin_auth
permissions = ConfiguredPermissions.new permissions

permissions.global_access? || permissions.for?(controller, action)

You can also avoid the if..else in your redirect method:

  def refuse_access_to_admin_site
    flash[:error] = 'Permission denied'

    location = request.referer.present? ? :back : admins_products_path
    redirect_to location
  end
\$\endgroup\$
  • \$\begingroup\$ Thanks for the corrections! I really didn't like the long if...else, and your solution is quite more elegant (even if I had to check global_auth for nil). I've been checking for the NullObject pattern and I don't know if it would make things more complex. At the end, would be to create a new class in order to get rid of 5 lines of code, isn't it? \$\endgroup\$ – Daniel Dec 15 '15 at 14:08
  • \$\begingroup\$ Indeed, it would probably be overkill in this situation. I just wanted to point it out to give deeper insight into what is going on. and if you added more security levels or other logic into the authorization, it might make sense at that point \$\endgroup\$ – Jonah Dec 15 '15 at 15:05

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.