Rails: Keeping user spoofing checks DRY

Question

In a fit of unoriginality, I'm writing a blog application using Ruby on Rails. My PostsController contains some code that ensures that the logged in user can only edit or delete their own posts.

I tried factoring this code out into a private method with a single argument for the flash message to display, but when I did this and tested it by editing another author's post, I got an ActionController::DoubleRenderError - "Can only render or redirect once per action".

How can I keep these checks DRY? The obvious approach is to use a before filter but the destroy method needs to 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

Answer 1

The simple answer is to change the message to something that fits both: "You cannot mess with another author's posts."

Answer 2

The before filter approach is still an ok option. You can gain access to which action was requested using the controller's action_name method.

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 there. :) Naturally you can do whatever logic you like.

You can also use a method if you like, and avoid the double render by explicitly returning if it fails. The key here is to return so that 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 more (in my opinion) by splitting out the different parts of behavior (authorization, handling bad authorization) like this:

def destroy
  @post = Post.find_by_slug(params[:slug])
  punt("You cannot mess with another author's 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 to offload all of this routine work to a plugin. My personal favorite authorization plugin is Authorization. I've used it with great success for the last several years.

That would refactor your controller to use variations on:

permit "author of :post"

Answer 3

If you don't like the ugly* return in that last solution, you can use an around filter and conditionally yield only if the user is authorized.

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's posts."
     when "update"
      "You cannot edit another author's posts."
     end
     redirect_to root_path
    end
end

*-- that's my preference, though code-wise it's perfectly valid. I just find that style-wise, it tends to not fit.

I also should add I haven't tested this and am not 100% certain it would work, though it should be easy enough to try.