Rails: Tracking DRY User Spoofing Errors
In a fit of unoriginality, I am writing a blogging application using Ruby on Rails. Mine PostsController
contains some code to ensure that the logged in user can edit or delete their own posts.
I tried to decompose this code into a private method with only one argument to display the flash message, but when I did and tested it by editing another post by the author, I got ActionController::DoubleRenderError
- "Can only display or redirect once per action."
How can I store these DRY checks ? The obvious approach is to use a before filter, but the method destroy
should display a different flash.
Here's the relevant controller code:
before_filter :find_post_by_slug!, :only => [:edit, :show]
def edit
# FIXME Refactor this into a separate method
if @post.user != current_user
flash[:notice] = "You cannot edit another author’s posts."
redirect_to root_path and return
end
...
end
def update
@post = Post.find(params[:id])
# FIXME Refactor this into a separate method
if @post.user != current_user
flash[:notice] = "You cannot edit another author’s posts."
redirect_to root_path and return
end
...
end
def destroy
@post = Post.find_by_slug(params[:slug])
# FIXME Refactor this into a separate method
if @post.user != current_user
flash[:notice] = "You cannot delete another author’s posts."
redirect_to root_path and return
end
...
end
private
def find_post_by_slug!
slug = params[:slug]
@post = Post.find_by_slug(slug) if slug
raise ActiveRecord::RecordNotFound if @post.nil?
end
a source to share
The filter approach is still the ok option. You can access what action was requested using the controller method action_name
.
before_filter :check_authorization
...
protected
def check_authorization
@post = Post.find_by_slug(params[:slug])
if @post.user != current_user
flash[:notice] = (action_name == "destroy") ?
"You cannot delete another author’s posts." :
"You cannot edit another author’s posts."
redirect_to root_path and return false
end
end
Sorry for that ternary operator in the middle. :) Naturally, you can do whatever logic you like.
You can also use the method if you like and avoid double rendering by explicitly returning if it fails. The key here is return so you don't double render.
def destroy
@post = Post.find_by_slug(params[:slug])
return unless authorized_to('delete')
...
end
protected
def authorized_to(mess_with)
if @post.user != current_user
flash[:notice] = "You cannot #{mess_with} another author’s posts."
redirect_to root_path and return false
end
return true
end
You could simplify it (in my opinion) by separating the different parts of the behavior (authorization, handling incorrect authorization) like this:
def destroy
@post = Post.find_by_slug(params[:slug])
punt("You cannot mess with another author post") and return unless author_of(@post)
...
end
protected
def author_of(post)
post.user == current_user
end
def punt(message)
flash[:notice] = message
redirect_to root_path
end
Personally, I prefer dumping all this chore into a plugin. My personal favorite plugin is Authorization . I have used it with great success over the past few years.
This will refactor your controller to use variants:
permit "author of :post"
a source to share
If you don't like the ugly * return in this latest solution, you can use the filter around and conditionally give only if the user is logged in.
around_filter :check_authorization, :only => [:destroy, :update]
private
def check_authorization
@post = Post.find_by_slug(params[:slug])
if @post.user == current_user
yield
else
flash[:notice] = case action_name
when "destroy"
"You cannot delete another author posts."
when "update"
"You cannot edit another author posts."
end
redirect_to root_path
end
end
* - which is my preference, albeit by code, is perfectly fine. I just find it in the style, it doesn't fit.
I must also add that I have not tested this and I am not 100% sure that it will work, although it should be easy enough to try.
a source to share