[rspec-users] Best Practice for Controllers

Phillip Koebbe phillipkoebbe at gmail.com
Sat Jan 22 07:06:49 EST 2011



On 2011-01-21 10:25 PM, David Kahn wrote:
>
>
> On Mon, Jan 17, 2011 at 1:43 PM, David Chelimsky <dchelimsky at gmail.com 
> <mailto:dchelimsky at gmail.com>> wrote:
>
>     On Jan 17, 2011, at 10:16 AM, David Kahn wrote:
>
>>     On Mon, Jan 17, 2011 at 9:48 AM, Ants Pants
>>     <antsmailinglist at gmail.com <mailto:antsmailinglist at gmail.com>> wrote:
>>
>>         Hello all,
>>
>>         From what I've seen, this type of question doesn't really
>>         seem to get an answer on this list as most of the replies
>>         relate to failures of RSpec. If this is the case, where is
>>         the best place to go to get advice about best practices etc?
>>
>>         I have a question about best practice. In some of my
>>         controllers only an admin user can perform edit, update, show
>>         etc. So I have a before filter in those controllers;
>>         ApplicationController#authorise_is_admin
>>
>>         The ApplicationController#authorise_is_admin throws an
>>         AccessDenied exception and that is caught
>>         in ApplicationController#access_denied
>>
>>         My question is, in the spec for the calling controller, let's
>>         say ProductGroups, what should I spec?
>>
>>         I have a context "user is admin" and that's easy to spec, but
>>         the context "user is not admin" is where I'm stuck as no
>>         actions are performed in that controller but I would just
>>         like to cover that failure somehow.
>>
>>     Interesting question. I had the same dilemma and decided that it
>>     took too much effort and test code to test this at the controller
>>     level. What I do (and this may or may not work for you depending
>>     on your apps security needs), is to have an authorize method in
>>     the User model. It returns success or failure based on the
>>     controller and action passed. The model looks something like this:
>>
>>       def authorize(controller_name, action_name)
>>         if self.role
>>           current_role = self.role.name <http://self.role.name/>
>>         else
>>           # guest user is empty user
>>           current_role = 'guest'
>>         end
>>
>>
>>         case controller_name
>>         when 'activations'
>>           if current_role != 'guest'
>>             return set_autorize_failure_value("You are already logged
>>     in to the system. If you are activating a new user please log out
>>     first and try again.")
>>           end
>>           return authorize_success_message
>>
>>         when 'feedback_supports'
>>           if current_role == 'guest' || current_role == 'sysadmin'
>>             return set_autorize_failure_value(LOGIN_NOTICE)
>>           end
>>           return authorize_success_message
>>     ...
>>
>>     end
>>
>>     Then in the spec it is real easy:
>>
>>       describe "user authorization - guest role" do
>>         it "is authorized to access certain pages only" do
>>           user = User.new
>>           user.authorize('activations', 'create')[:success].should ==
>>     true
>>           user.authorize('home', 'index')[:success].should == false
>>
>>         ....
>>
>>         end
>>       end
>>
>>     This might not be everyone's cup of tea and I am sure I can
>>     refactor and make this less verbose, but what I like is having
>>     the 'dna' of all my access rights app wide in one place.
>
>     Definitely agree with the idea of keeping decisions in one place.
>     I don't really like the idea of 'controllers' living inside a
>     model, but change 'controller_name' to 'resource_collection_name'
>     and that solves that problem.
>
>     I would still want to specify that the controller asks the user
>     for authorization. WDYT?
>
>
> If I am following the thread right, I should clarify that I call 
> 'authorize' from the application controller -- should have included 
> this in my first response. It is from there that the User model is 
> invoked:
>
> class ApplicationController < ActionController::Base
>
>     private
>
>     def authorize
>       if current_user
>         return_value = current_user.authorize(controller_name, 
> action_name)
>       else
>         return_value = User.new.authorize(controller_name, action_name)
>       end
>
>       if !return_value[:success]
>         flash[:notice] = return_value[:failure_message]
>         if current_user
>           redirect_to home_path
>         else
>           redirect_to login_path
>         end
>       end
>     end
>
> end
>
>
> I would not venture to say what I have is the best but just what came 
> up from refactoring in search of simplifying my code and tests. I also 
> do not like the idea of anything dependent like a session or params or 
> something else from a controller getting coupled to a model, but in 
> this case it feels to me that the model is just a switchboard in 
> decision-making, which for me I would consider business logic.
>
>

Keeping the "cup of tea" idea in mind, I can understand how you could 
arrive at this point. I did something similar a couple of years ago. I 
was passing something into the model that could come only from a 
controller, which was fine and dandy until I needed to use the model 
method in some way that proved difficult to get that information. It may 
have been a rake task, I can't remember the exact details. But from that 
point, I started thinking about my models as separate from the 
particular application I was working on at the moment. When creating 
methods and passing in arguments, I started thinking about what would 
happen if I used this model outside of the Rails application. Would I 
still be able to pass that information in easily and would it make sense?

The other thought that I had with this particular approach (passing 
controller name and action into the model) is that it seems the model 
specs would feel sort of fake. In order to unit test the authorize 
method, you have to test with real values. I may be overlooking 
something (just got up, so it's very possible), but that doesn't seem 
like it's testing behavior so much as how the method handles specific 
values. If you add new controllers and/or actions, you must then go add 
them to your unit test to make sure the authorize method responds 
correctly. That doesn't seem like the kind of test that I'd feel 
comfortable trusting.

Now that I think about it, I actually had this same situation come up a 
couple of years ago. Before I started splitting role-based functionality 
into separate namespaced controllers, I was trying to control access to 
various actions via before_filters. Using the same concept of 
controller/action combination, I created a hash of those pairs and which 
roles could access them. The role names then turned into model methods 
that returned true/false. Something like:

access_rules = {
   'products' => {
     'show' => [:clerk, :manager],
     'new' => [:manager],
     'edit' => [:manager]
   },
   'stores' => {
     'show' => [:clerk, :manager],
     'new' => [:manager],
     'edit' => [:manager]
   }
}

roles = access_rules[controller_name][action_name]

return true if roles.any? { |role| current_user.send("#{role}?") }
return false

Something like that anyway. I even went so far as to write a controller 
plugin so I could simplify it to

allow_access(:clerk, :only => [:show])
allow_access(:manager, :only => [:show, :new, :edit])
verify_access

This is obviously a contrived example, but it should suffice to get the 
point across. Bringing this back to RSpec, specs make more sense this 
way as you can check to make sure your role methods on the user model 
return true or false based on a database setting and you can easily 
mock/stub those methods in the controller specs to make sure that you 
don't get to actions that you shouldn't.

By the way, I prefer English breakfast tea and Indian spiced chai. :)

Peace.
Phillip



More information about the rspec-users mailing list