---
title: Second Refactoring of Security Roles
teaser: Refactoring role-based security code in Ruby.
tags: web,rails
author: Tammer Saleh
published_on: 2007-01-12
---

## A little refresher on what not to do

The wrapper class approach is just fine for a single resource that needs
security applied to it.  For a refresher, here's the model...

```ruby
class Article < ActiveRecord::Base
  # Columns include submitted and accepted
end
```

And here's the wrapper that you would use in its place when in an Admin
controller...

```ruby
class AdminArticle
  def self.method_missing(method, *args)
    scope = { :find => { :conditions => ["submitted = ?", true] }}

    Article.with_scope(scope) do
      # we've limited the scope, so just pass the method call on...
      Article.send(method, *args)
    end
  end

  def self.new(*args)
    # need to explicitly pass this on
    self.method_missing(:new, *args)
  end
end
```

And here's the wrapper that you would use in its place when in a Member
controller...

```ruby
class MemberArticle
  def self.method_missing(method, *args)
    # We get current_user via a class-level accessor I've left out for sanity
    scope = {
      :find => {
        :conditions => ["approved = ? or user_id = ?", true, current_user]
      }
    }
    Article.with_scope(scope) do
      # we've limited the scope, so just pass the method call on...
      Article.send(method, *args)
    end
  end

  def self.new(*args)
    # need to explicitly pass this on
    self.method_missing(:new, *args)
  end
end
```

You can already see how this isn't gonna scale as you add more AR classes that
need security?  Not to mention the fact that it violates the newly private
nature of `ActiveRecord.with\_scope`.  Finally, it's not nearly as purty as I'd
like.  We all like purty, right?  Lemme see some hands!

## That's much better

First, the library...

```ruby
module Filterable
  def self.included(clazz)
    clazz.class_eval do
      extend ClassMethods
    end
  end

  module ClassMethods
    def scope_to_role(role, user = nil)
      case role
      when :public
        scope = { :find => { :conditions => ["approved = true"] } }
      when :admin
        scope = { :find => { :conditions => ["submitted = true"] } }
      when :member
        raise RuntimeError,
          "#{self.name}.scope_to_role(:member, nil) called,
           which doesn't make sense" unless user
        scope = {
          :find => { :conditions => ["approved = true OR user_id = ?", user.id]}
        }
      else
        raise RuntimeError,
          "#{self.name}.scope_to_user called with unrecognized role #{role}"
      end

      with_scope(scope) do
        yield
      end
    end
  end
end
```

Then the models...

```ruby
class Article < ActiveRecord::Base
  include Filterable
  # Columns include submitted and accepted
end
```

And using it in the controllers...

```ruby
class ApplicationController < ActionController::Base
  around_filter :filter_results_for_public

  def filter_results_for_public
    People.scope_to_role(:public) do
      Article.scope_to_role(:public) do
        Photo.scope_to_role(:public) do
          yield
        end
      end
    end
  end

  def filter_results_for_admin
    People.scope_to_role(:admin) do
      Article.scope_to_role(:admin) do
        Photo.scope_to_role(:admin) do
          yield
        end
      end
    end
  end

  def filter_results_for_member
    People.scope_to_role(:member, current_user) do
      Article.scope_to_role(:member, current_user) do
        Photo.scope_to_role(:member, current_user) do
          yield
        end
      end
    end
  end
end

class Admin::BaseController < ApplicationController
  skip_filter :filter_results_for_public
  around_filter :filter_results_for_admin
end

class Member::BaseController < ApplicationController
  skip_filter :filter_results_for_public
  around_filter :filter_results_for_member
end
```

## There's a downside

The biggest downside is that you're yielding about six times for each and every
request.  You could trim that down by excluding certain actions, but it's still
not the most efficient design.

Also, we could probably get all clever and combine those filter\_results\_for\*
methods into one which introspects on the url or current controller.  Do we want
to do that?  I didn't think so.