views:

502

answers:

6

I am trying to lock-down a few controllers based on role and the 'posts' controller by whether or not they ANY permissions assigned. This appears to be working, but I'm wondering if there is a clean way to handle this. This is what I have in the application controller, which I'm calling as a before filter...

if controller_name == 'users' || 'accounts'
  unless @current_user.master? || @current_user.power?
    render :template => "layouts/no_content"
  end
elsif controller_name == 'posts'
  unless @current_user.permissions.count > 0
    render :template => "layouts/no_content"
  end
end

Thanks in advance.

+7  A: 

You shouldn't make a code snippet that checks for a controller name to take a specific action in application.rb. You should define that before filters only in the controllers that need them

Make 2 methods in ApplicationController:

private
def require_master_or_power_user
  unless @current_user.master? || @current_user.power?
    render :template => "layouts/no_content"
  end
end

def require_some_permisions
  unless @current_user.permissions.count > 0
    render :template => "layouts/no_content"
  end
end

Now add this as a before filter where you need it:

class UsersController < ApplicationController
  before_filter :require_master_or_power_user
  ...
end

class AccountsController < ApplicationController
  before_filter :require_master_or_power_user
  ...
end

class PostsController < ApplicationController
  before_filter :require_some_permisions
  ...
end

So the ApplicationController defines the filters, but its up to your other controllers whether or not to actually use those filters. A superclass like the ApplicationController should never conditionally branch its execution based on its subclasses. Choosing when to use the provided behaviours are one of the reasons why you want to subclass in the first place.

It's also much clearer from a code readability standpoint. When looking at the UsersController, its immediately obvious there is some permission stuff happening when you see a before filter with the name like "require_something". With your approach, you can't tell that from looking at the users controller code itself at all.

Squeegy
Thanks, Squeegy. You clarified things quite a bit; does it make a difference that the code snippet I provided is wrapped in a method and called from a before_filter. Should I still break them down and chain them together like: before_filter filter1, filter2, etc...
Depends on what that code does I suppose. But if each controller needs a different set filters, then break them apart and set them up as individual filters to be called as needed. If a filter is called for all controllers (i.e. popuating @current_user), put the before_filter call in applciation.rb
Squeegy
+1  A: 

I would strongly suggest you adhere to MVC and OOP and move as much of the user related logic back into the User model like this:

class User < ActiveRecord::Base

def has_permission?
  true if self.master? || self.power? || (self.permissions.count > 1)
end

then you could just use one filter in application.rb:

protected

def check_template 
  render :template => "layouts/no_content" if current_user.has_permission? == true
end

and call that with a before_filter as suggested by Squeegy, either in the respective controllers, or site wide in application_controller.rb

before_filter :check_template

This approach is obviously a little cleaner and a lot less brittle if you ever decide to change the scope of what gives people permission, you only have to make one change application wide.

Scott Miller
A: 

I would advise that you use an ACL system for this: http://github.com/ezmobius/acl_system2

Ryan Bigg
A: 

A short little handwritten DSL. Haven't even checked the code for syntax errors, but you'll get the picture. In your application controller:

before_filter :handle_requirements

def self.requirement(*controllers, &block)
  @_requirements ||= {}
  @_requirements[controllers] = block
end

def handle_requirements
  return unless @_requirements
  @_requirements.each do |controllers, proc|
    if controllers.include?(controller.controller_name)
      restrict_access unless instance_eval(&block)
    end
  end
end

def restrict_access
  render :template => "layouts/no_content"
end

Usage (also in your application controller)

requirement('users', 'accounts') do
  @current_user.master? || @current_user.power?
end

Or, just use the ACL system Radar mentions.

August Lilleaas
A: 

Another plugin worth a look is role requirement, which I've been using. I think they can both do roughly the same things.

Jeremy
A: 

Here is a plug for RESTful_ACL; an ACL plugin/gem I've developed, and is being pretty widely used. It give you freedom to design your roles as you see fit, and it very transparent.

Matt Darby