I've been doing a fair bit of training recently — both while bringing Mat up to speed on the latest and greatest best practices, and going and speaking to clients' teams. A few concepts keep coming up, so I'm going to try to blog about them as they do. Here's the first one.
I see this all over client projects, and admittedly, some of my older ones:
<%- if @post.creator == current_user -%>
<%= link_to "Edit", edit_post_path(@post) %>
<%- end -%>
Seems innocuous — the post can only be be edited by its creator. But, that is a business logic rule, so it belongs in your model (and your tests).
context "Posts" do
setup do
@creator = create_user
@post = @creator.posts.create!(hash_for_post)
@another_user = create_user
end
should "be editable by their creator" do
assert @post.can_be_edited_by?(@user)
end
should "not be editable by another random user" do
assert !@post.can_be_edited_by?(@another_user)
end
end
class Post < ActiveRecord::Base
def can_be_edited_by?(user)
user == creator
end
end
Why? A few reasons. First, because your model classes should represent a complete picture of your data's structure, and business logic rules. Second, a rule like that deserves testing, even if it's as simple as the one in this example. Finally, because later on, you might want admins to be able to edit posts, too.
context "Posts" do
setup do
@creator = create_user
@post = @creator.posts.create!(hash_for_post)
@another_user = create_user
@admin = create_user :admin => true
end
should "be editable by their creator" do
assert @post.can_be_edited_by?(@user)
end
should "be editable by an admin" do
assert @post.can_be_edited_by?(@admin)
end
should "not be editable by another random user" do
assert !@post.can_be_edited_by?(@another_user)
end
end
class Post < ActiveRecord::Base
def can_be_edited_by?(user)
user == creator || user.admin?
end
end
If we're using the can_be_edited_by? method all over our controllers and views, a change to the access policy shouldn't entail anything other than editing a couple of lines of code in our models and unit tests.
Subtler
Here's another one I see all the time (especially in my code). This one tends to be harder to spot.
class MessagesController
before_filter :login_required
protected
def project
@project ||= current_user.projects.find(params[:project_id])
end
def messages
project.messages.find(:all)
end
end
The idea here is that we're using the association as an implicit access control mechanism. If the user row is associated with the project row, the user has access to that project. I know that there are several r_c users who have used this pattern, since it's so easy to implement with r_c. I've even recommended it. Ouch.
The problem with this pattern is lack of encapsulation. There are a good handful of situation in which you might want the access rule to change such that you'd have to go and change every call to current_user.projects. Which is ugly.
What if you decided to make ProjectMembership a join model, for example? Rather than actually deleting membership rows, you decide you'd like, for record keeping purposes, to have a revoked_at field which denotes a former membership made invalid. You might think — no problem, I can just change the conditions of the association.
class User
has_many :projects, :through => :project_memberships, :conditions => "project_memberships.revoked_at IS NULL"
end
Aside from ambiguous naming, this remains an incomplete solution. At some point, you're going to decide that admins have access to all projects. You could add that condition to your SQL, too, but that approach is problematic for the same reason we're talking about, here: the definition of an admin may change. So, we need a different strategy.
class Project
has_many :project_memberships
has_many :users, :through => :project_memberships
named_scope :with_active_membership_for, lambda { |user| { :include => :project_memberships, :conditions => ['project_memberships.user_id = ? AND project_memberships.revoked_at IS NULL', user.id] } }
def self.for(user)
user.admin? ? self : self.with_active_membership_for(user)
end
end
This isn't a perfect solution, since the definition of a revoked membership is living in the SQL; ideally that would be defined in ProjectMembership. But, with a join model, I'm not sure of any other way. So, at least in this instance, we can use the Project.for method in our controllers, and views, and not have to worry about a change in the project access rules causing a need for major refactoring later on.
class MessagesController
before_filter :login_required
protected
def project
@project ||= Project.for(current_user).find(params[:project_id])
end
def messages
project.messages.find(:all)
end
end
But, Not Always
One could look at these examples and decide to engage in reductio ad absurdum. Why not encapsulate the messages collection, they'd ask?
The rule I tend to stick to is that if I know that there's any kind of access control policy being enforced on an object or collection, it gets encapsulated, and tested in the model.
In the fictional examples above, the assumption was that provided access to the project, a user would be allowed to access all of messages within. In that case, I wouldn't have encapsulated, because there would have been no rule to encapsulate.
As soon as there is a policy, though, it belongs in tested methods in your models.
The controller's responsibility in all of this is to control access to a resource in the sense that it actually performs the check, and redirects the user to a login screen, or shows an access denied message if they are not entitled to perform said action on said resource. The controller is not, however, responsible for deciding who it is that gets access; that's the model's job.
Like a bouncer at the door of a nightclub, your controller shouldn't make the rules, it should only enforce them. Luckily for you, cute 17 year old girls won't have the same effect on your controller that they might on a bouncer.
