before_filter that checks whether the controller instance variable belongs to the current_user

Question

Currently I have a CRUD resources set up for restaurants. An Owner has_many restaurants and a restaurant belongs_to an Owner. The restaurants #index and #show views are accesible by any user. However, in order to create a new Restaurant the Owner has to be logged in. I implemented devise and this works fine. My issue is in making sure that the current_owner owns the restaurant before being able to edit, update, or destroy the restaurant.

I'm having trouble creating a before_filter that will check whether the logged in owner (current_owner) is the owner of that restaurant that the logged in owner is trying to view.

Before setting up the before_filter I quickly made a #check_if_owner method and placed it inside the edit action. If the Owner does not own the restaurant they should be redirect to the previous page. However, for some reason I am receiving the following error:

ActiveRecord::RecordNotFound in RestaurantsController#edit

Couldn't find Restaurant with id=4 [WHERE "restaurants"."owner_id" = 1]

I'm not sure why this is happening because when I run the #check_ownership method in the console it returns false, which is exactly what I want the method to do if the current_owner doesn't own the restaurant. If it returns false in the console shouldn't the user be redirected to the previous page instead of receiving the RecordNotFound error? I'm not sure why this is happening.

Posted is the rest of the code...

class RestaurantsController < ApplicationController
  before_filter :authenticate_owner!, except: [:index, :show]
  # before_filter :check_if_owner, only: [:edit, :update, :destroy]
  def index
    @restaurants = Restaurant.all
  end

  def show
    @restaurant = Restaurant.find(params[:id])
  end

  def new
    @restaurant = current_owner.restaurants.new
  end

  def create
    @restaurant = current_owner.restaurants.build(params[:restaurant])
    if @restaurant.save
      redirect_to restaurants_path
    else
      flash[:error] = "<ul>" + @restaurant.errors.full_messages.map{|o| "<li>" + o + "</li>" }.join("") + "</ul>"
      redirect_to new_restaurant_path
    end
  end

  def edit
    check_if_owner(Restaurant.find(params[:id]))
    @restaurant = current_owner.restaurants.find(params[:id])
  end

  def update
    @restaurant = current_owner.restaurants.find(params[:id])
    @restaurant.update_attributes(params[:restaurant])
    redirect_to restaurant_path(@restaurant)
  end

  def destroy
    @restaurant = current_owner.restaurants.find(params[:id])
    @restaurant.destroy
    redirect_to restaurants_path
  end

  private

    def check_if_owner(restaurant)
      debugger
      if current_owner.check_ownership(restaurant)
        return
      else
        redirect_to :back
      end
    end

end


class Owner < ActiveRecord::Base
  # Include default devise modules. Others available are:
  # :token_authenticatable, :confirmable,
  # :lockable, :timeoutable and :omniauthable
  devise :database_authenticatable, :registerable,
         :recoverable, :rememberable, :trackable, :validatable

  # Setup accessible (or protected) attributes for your model
  attr_accessible :email, :password, :password_confirmation, :remember_me, :name
  # attr_accessible :title, :body

  has_many :restaurants

  validates :name, presence: true
  validates :email, presence: true

  def check_ownership(restaurant)
    !self.restaurants.find_by_id(restaurant.id).nil?
  end

end

Answer 1

First, I would put the responsibility on checking ownership on the restaurant, not on the owner, especially since you are implementing this check in the RestaurantsController.

And, further, you seem to be over-engineering the ownership check. Really, you just need to check restaurant.owner == current_owner.

restaurant.rb

def owned_by?(current_owner)
  owner == current_owner
end

This method only needs to go in the model (as opposed to living as a single line in the controller before filter) if you think you'll be reusing it elsewhere.

Or, alternately, if your owner is going to be managing many different sorts of objects, you could leave the permissions check in the owner model and make it more flexible.

owner.rb

def manages?(object)
  object.respond_to?(:owner) && object.owner == self
end

The approach you were using, with find and find_by_id is fragile and non-preferred in ActiveRecord querying. Specifically, find_by_* throws an exception when you get no result. On the other hand, using where(:id => id) would return an empty array or nil if there was no result.

Take a look at these for more insight and best practices.
http://tenmiles.com/blog/2011/07/activerecord-finders-returns-nil-or-throws-exception/ http://guides.rubyonrails.org/active_record_querying.html

Answer 2

Solved it with the following:

class RestaurantsController < ApplicationController
  before_filter :authenticate_owner!, except: [:index, :show]
  before_filter :check_if_owner, only: [:edit, :update, :destroy]

  private

  def check_if_owner
    if current_owner.has_ownership?(Restaurant.find(params[:id]))
      return
    else
      flash[:error]= "You do not have permission to do that." 
      redirect_to :back
    end
  end
end

Owner.rb

  def has_ownership?(restaurant)
    self.restaurants.find_by_id(restaurant.id).present?
  end