Making generic before_filters a little less ugly?

Question

Hey folks,

I have a few before filters which I am using to control access to resources on a resource-by-resource level. The basic idea is as follows:

1. A user can be a user or admin and can have access to specific resources based on an "accesses" table.
2. Resources/methods can be limited in access to admin, owner, particular users, or everyone.

This is best illustrated by some code examples. We have 4 application-level methods that are added to the call chain with before_filter. Here is the top of an example controller class:

before_filter :require_user
before_filter :get_object, :only=>[:show, :edit, :update, :destroy]
before_filter :require_access, :only=>[:show]
before_filter :require_owner, :only=>[:edit, :update, :destroy]

As you can see, first we require that a user be logged in to access any method in this controller. Here are 3 fo the methods (defined in application.rb) so that you can see what they look like:

 private
 def get_object
   begin
     class_name = controller_name.gsub("Controller","").downcase.singularize
     instance_variable_set "@#{class_name}".to_sym, class_name.capitalize.constantize.find(params[:id])
   rescue
     flash[:error] = "You do not have access to that #{class_name}."
     redirect_to "/" and return
   end
 end

 private
 def require_owner
   class_name = controller_name.gsub("Controller","").downcase.singularize
   accessable = instance_variable_get("@#{class_name.downcase}")
   unless accessable.user == current_user
     flash[:error] = "You do not have access to that #{class_name.downcase}."
     redirect_to "/" and return
   end
 end

 private
 def require_access
   class_name = controller_name.gsub("Controller","").downcase.singularize
   accessable = self.instance_variable_get("@#{class_name.downcase}")
   unless current_user.has_access?(accessable)
     flash[:error] = "You do not have access to that #{class_name.downcase}."
     redirect_to "/" and return
   end
 end

This is all fine, as far as I can tell, from a coding perspective. But it's just so god-damn ugly! In particular the lines:

 class_name = controller_name.gsub("Controller","").downcase.singularize
 obj = instance_variable_get("@#{class_name.downcase}")

OR

 instance_variable_set "@#{class_name}".to_sym, class_name.capitalize.constantize.find(params[:id])

Does anyone know of a bit more elegant way to do what I am doing here?

Answer 1

Well, you can do the 2 following things:

1- First remove the 2 other private statements, the first one is enough. Remember private, protected and public are just other methods defined in Ruby Module class.

2- It's better to refactor the code to set that object creation in its method:

def create_object
  class_name = controller_name.gsub("Controller","").downcase.singularize
  obj = instance_variable_get("@#{class_name.downcase}")
end

def locate_object
 instance_variable_set "@#{class_name}".to_sym class_name.capitalize.constantize.find(params[:id])
end

Answer 2

I don't know if there's a really clean way to do this, but here are a few suggestions:

First, create a controller ResourceController and have all relevant controllers inherit from it. (If this authorization applies to all controllers you can just use ApplicationController.)

Now, implement a private method in the superclass called model_name (like your class_name) so you don't have to derive it every time you need it. And, you should be able to derive it by simply doing this:

def model_name
  controller_name.classify
end

You can also implement a model method in the superclass which returns the actual class:

def model
  model_name.constantize
end

At this point you might as well also add something like this:

def current_object
  model.find(params[:id])
end

def current_object_var_name
  "@#{model_name.underscore}"
end

I don't see a quick way around using instance_variable_get/set except for always using @object or something like it. But if you don't want to do that, those lines are now a little simpler:

instance_variable_set current_object_var_name, current_object
obj = instance_variable_get(current_object_var_name)

At this point your code should be more readable, and a little prettier.

You might also want to look into what some of the recent Rails authorization plugins are doing, in particular cancan and declarative_authorization.

Answer 3

Combining both of your answers I am left with the following fairly clean code, which I really should put into a separate plugin.

 private
 def get_resource
   begin
     instance_variable_set current_object_var_name.to_sym, model_name.constantize.find(params[:id])
   rescue
     flash[:error] = "You do not have access to that #{model_name}."
     redirect_to "/" and return
   end
 end

 def require_owner
   unless resource.user == current_user
     flash[:error] = "You do not have access to that  #{model_name}."
     redirect_to "/" and return
   end
 end

 def require_access
   unless current_user.has_access?(resource)
     flash[:error] = "You do not have access to that #{model_name}."
     redirect_to "/" and return
   end
 end

 def resource
   instance_variable_get(current_object_var_name)
 end

 def model_name
   @model_name ||= controller_name.classify
 end

 def current_object_var_name
   "@#{model_name.underscore}"
 end