First of all.. this is a great book! I’ve already used a variety of Chad’s recipes, either as is or (more often) as the basis of something customized.

That said, I was a bit disappointed when I read through Recipe 32 “Authorizing Users With Roles”. Mind you the solution was fine and worked nicely. My issue was with the code itself. Specifically, the unless condition in:

def check_authorization
  user = User.find(session[:user])
  unless user.roles.detect{|role|
    role.rights.detect{|right|
      right.action == action_name && right.controller == self.class.controller_path
      }
    }
    flash[:notice] = "You are not authorized..."
    request.env["HTTP_REFERER"] ? (redirect_to :back) : (redirect_to home_url)
    return false
  end
end

 

What’s wrong? Well, if we look at this code we start with a user, get the users roles, get the roles rights, get the rights action & controller. All from code in a controller. That’s a classic example of function objects & data objects. “But,” you say, “this is Rails.” So what. It’s still Object Oriented Programming. Thankfully, this is easy to clean up.

First, we can extract the condition out into a separate method:

def check_authorization
  user = User.find(session[:user])
  unless has_right?(user)
    flash[:notice] = "You are not authorized..."
    request.env["HTTP_REFERER"] ? (redirect_to :back) : (redirect_to home_url)
    return false
  end
end

def has_right?(user)
  user.roles.detect{|role|
    role.rights.detect{|right|
      right.action == action_name && right.controller == self.class.controller_path
    }
  }
end

 

Now we can push the code that deals with the user into the User class:

class User
  #...
  def has_right_for?(action_name, controller_name)
    roles.detect{|role|
      role.rights.detect{|right|
        right.action == action_name && right.controller == controller_name
      }
    }
  end
end

 

Leaving this behind:

def has_right?(user)
  user.has_right_for?(action_name, self.class.controller_path)
end

 

Most of User#has_right_for? can be pushed farther, into Role:

class User
  #...
  def has_right_for?(action_name, controller_name)
    roles.detect{ |role| role.has_right_for?(action_name, controller_name) }
  end
end

class Role
  #...
  def has_right_for?(action_name, controller_name)
    rights.detect{ |right| right.action == action_name && right.controller == controller_name }
  end
end

 

We can go one step farther:

class Role
  #...
  def has_right_for?(action_name, controller_name)
    rights.detect{ |right| right.has_right_for?(action_name, controller_name) }
  end
end

class Right
  #...
  def has_right_for?(action_name, controller_name)
    action == action_name && controller == controller_name
  end
end

 

Now.. the controller has a user in hand. It asks that user if it has the right to access the requested page. That user in turn, has some roles.. it asks each role if it has the right for the requested page, each role in turn asks it’s rights. Everyone in the chain talks only to what it has directly in hand. This is the way it should be. Now the controller doesn’t know or care what it means to have the right to access an action… only that the user knows, and how to ask that user.