Skip to content
This repository

load_and_authorize_resource exception when resource not found #43

Closed
flatrocks opened this Issue March 02, 2010 · 13 comments

8 participants

Tom Wilson Ryan Bates Alexander S. Matt Slay Gordon McNaughton alanheppenstall David Gil Nathan Lilienthal
Tom Wilson

In an application where it is possible to delete resources, one user may destroy an item while a second user has a link to that resource still up on the browser. When the second user clicks the link, load_and_authorize_resource will cause an exception when trying to create the page. Typical message is "Couldn't find Item with ID=47".

Normally, I'd just intercept the exception and apologize/redirect. But I am thinking that it would be cool if load_and_authorize_resource handled the "not found" case in a more friendly way since this situation can occur naturally, without any hacking or really anything resembling a genuine error, etc.

The "CanCan::AccessDenied" exception type makes handling that case easy enough, so perhaps throwing a specific exception type ("ResourceAuthorization::NotFound" ??) from ResourceAuthorization load_resource would be helpful. That would make it easy to handle the exception in an appropriate way at the application level, or by controller.

Any opinions on this? Am I just being a weenie and should just handle this with a general application-level rescue?

Ryan Bates
Owner
ryanb commented March 02, 2010

I don't think this should play a part in authorization. Imagine you had no authorization and every user was able to delete records, you would still run into the problem where one user deleted a record and the "delete" link was still visible on another user's browser.

Either way, I'm not sure why the 404 (item not found) response is not sufficient? Since the record has already been deleted it makes sense that it says it could not find the item. You can rescue from ActiveRecord::RecordNotFound, but I suggest leaving it as is and returning the 404 response. If this is a common situation then perhaps say in the 404 message the item may have been already removed.

I'll leave this open for discussion, but I don't think this is part of authorization.

Tom Wilson

Thanks for the quick reply on this issue. I agree... doesn't have anything to do with authorization and that is a good argument for leaving it out.

Alexander S.

I don't think this should play a part in authorization. Imagine you had no authorization and every user was able to delete records, you would still run into the problem where one user deleted a record and the "delete" link was still visible on another user's browser.

I like to do something like:

def edit
  begin
    @session = @project.sessions.find(params[:id])
  rescue ActiveRecord::RecordNotFound
    flash[:error] = "Project session cannot be found"
    redirect_to admin_project_sessions_path(@project)
  end
end

But this doesn't work because of the issue described above, so I can't catch the "record not found" error in my controller.

Alexander S.

Please reopen the issue.

Alexander S.

Adding this patch to lib/cancan/controller_resource.rb solves the problem.

module CanCan
  class ControllerResource
    def find_resource
      if @options[:singleton] && parent_resource.respond_to?(name)
        parent_resource.send(name)
      else
        if @options[:find_by]
          if resource_base.respond_to? "find_by_#{@options[:find_by]}!"
            resource_base.send("find_by_#{@options[:find_by]}!", id_param)
          else
            resource_base.send(@options[:find_by], id_param)
          end
        else
          adapter.find(resource_base, id_param) rescue nil
        end
      end
    end
  end
end
Matt Slay

Ryan - you said: "I'm not sure why the 404 (item not found) response is not sufficient?"

But, this situation does not issue a 404, it issues an "ActiveRecord::RecordNotFound in UsersController#edit" exception, and there is no way to catch this when using load_and_authorize_resource at the top of your controllers.

Any suggestions?

Alexander S.
heaven commented May 01, 2012

@mattslay I think he meant that you can handle this is application controller and respond with 404 page in case when record not found.

But actually rendering 404 is not suitable in all cases. Some employers want their pages to be more user friendly and often this requires individual approaches for individual actions. We, of course, can handle all that in application controller, but I don't see too many sence in building single big method that will handle the whole app.

Anyway, load_and_authorize_resource :raise_on_record_not_found => false looks like a solution.

Gordon McNaughton

This may not be approved usage, but it looks like you can work around it by explicitly specifying :find_by => find_by_id. This will cause the finder to use find_by_id (which doesn't throw an exception) instead of find or find_by_id! which do. The result is no exception, and the member variable is nil.

alanheppenstall

The suggested workaround doesn't seem to resolve the issue.

If I use: load_and_authorize_resource in my controller and then:

@object = Object.find_by_id(params[:id])

    if @object then
        #do stuff
    else
        redirect_to wherever_path
    end

This results in an error:
ActiveRecord::RecordNotFound ObjectsController#show

Whereby removing load_and_authorize_resource from the controller results in the expected behaviour of redirecting to the wherever_path.

Gordon McNaughton

@alanheppenstall I think the difference is in the parameters you pass to load_and_authorize_resource.

This will cause an exception to be thrown if no match is found: load_and_authorize_resource :object, :find_by => :id

But this will NOT trigger an exception: load_and_authorize_resource :object, :find_by => :find_by_id

In the sample you posted, you could use the latter and omit the @object = Object.find_by_id(params[:id]) line entirely.

Hope that helps!

alanheppenstall

Thanks @gmcnaughton, that makes sense :)

Nathan Lilienthal

This is another example of cancan behaving in ways that might be misleading. The default behavior of loading a resource should be what users would expect, mirroring Model.find. Raising the exception would be the most intuitive.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.