Problem
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
Solution
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