From 8903feee70ed355af3752d85be7bd7e01d257d8c Mon Sep 17 00:00:00 2001 From: Ryan Bates Date: Fri, 16 Apr 2010 14:54:18 -0700 Subject: [PATCH] removing unauthorized! in favor of authorize! and including more information in AccessDenied exception - closes #40 --- CHANGELOG.rdoc | 4 ++ README.rdoc | 8 ++-- lib/cancan.rb | 10 +---- lib/cancan/controller_additions.rb | 35 ++++++++++++------ lib/cancan/controller_resource.rb | 2 +- lib/cancan/exceptions.rb | 43 ++++++++++++++++++++++ lib/cancan/matchers.rb | 8 ++-- lib/cancan/resource_authorization.rb | 2 +- spec/cancan/controller_additions_spec.rb | 39 +++++++++++++++----- spec/cancan/controller_resource_spec.rb | 2 +- spec/cancan/exceptions_spec.rb | 35 ++++++++++++++++++ spec/cancan/resource_authorization_spec.rb | 13 ++----- 12 files changed, 152 insertions(+), 49 deletions(-) create mode 100644 lib/cancan/exceptions.rb create mode 100644 spec/cancan/exceptions_spec.rb diff --git a/CHANGELOG.rdoc b/CHANGELOG.rdoc index f4180b86..1a7d829f 100644 --- a/CHANGELOG.rdoc +++ b/CHANGELOG.rdoc @@ -1,5 +1,9 @@ 1.1.0 (not released) +* Removing "unauthorized!" method in favor of "authorize!" + +* Adding action, subject and default_message abilities to AccessDenied exception - see issue #40 + * Adding caching to current_ability controller method, if you're overriding this be sure to add caching too. * Adding "accessible_by" method to Active Record for fetching records matching a specific ability diff --git a/README.rdoc b/README.rdoc index b9934648..f20aca8e 100644 --- a/README.rdoc +++ b/README.rdoc @@ -39,17 +39,17 @@ First, define a class called Ability in "models/ability.rb". This is where all permissions will go. See the "Defining Abilities" section below for more information. -You can access the current permissions at any point using the "can?" and "cannot?" methods in the view. +You can access the current permissions at any point using the "can?" and "cannot?" methods in the view and controller. <% if can? :update, @article %> <%= link_to "Edit", edit_article_path(@article) %> <% end %> -You can also use these methods in a controller along with the "unauthorized!" method to restrict access. +The "authorize!" method in the controller will raise CanCan::AccessDenied if the user is not able to perform the given action. def show @article = Article.find(params[:id]) - unauthorized! if cannot? :read, @article + authorize! :read, @article end Setting this for every action can be tedious, therefore the load_and_authorize_resource method is also provided to automatically authorize all actions in a RESTful style resource controller. It will set up a before filter which loads the resource into the instance variable and authorizes it. @@ -71,6 +71,8 @@ If the user authorization fails, a CanCan::AccessDenied exception will be raised end end +See the CanCan::AccessDenied rdoc for more information on exception handling. + == Defining Abilities diff --git a/lib/cancan.rb b/lib/cancan.rb index aed1282a..6b89243d 100644 --- a/lib/cancan.rb +++ b/lib/cancan.rb @@ -1,14 +1,6 @@ -module CanCan - # A general CanCan exception - class Error < StandardError; end - - # This error is raised when a user isn't allowed to access a given - # controller action. See ControllerAdditions#unauthorized! for details. - class AccessDenied < Error; end -end - require 'cancan/ability' require 'cancan/controller_resource' require 'cancan/resource_authorization' require 'cancan/controller_additions' require 'cancan/active_record_additions' +require 'cancan/exceptions' diff --git a/lib/cancan/controller_additions.rb b/lib/cancan/controller_additions.rb index 73e8b112..863780d5 100644 --- a/lib/cancan/controller_additions.rb +++ b/lib/cancan/controller_additions.rb @@ -85,7 +85,7 @@ def load_resource(options = {}) # and ensure the user can perform the current action on it. Under the hood it is doing # something like the following. # - # unauthorized! if cannot?(params[:action].to_sym, @article || Article) + # authorize!(params[:action].to_sym, @article || Article) # # Call this method directly on the controller class. # @@ -116,18 +116,21 @@ def self.included(base) base.helper_method :can?, :cannot? end - # Raises the CanCan::AccessDenied exception. This is often used in a - # controller action to mark a request as unauthorized. + # Raises a CanCan::AccessDenied exception if the current_ability cannot + # perform the given action. This is usually called in a controller action or + # before filter to perform the authorization. # # def show # @article = Article.find(params[:id]) - # unauthorized! if cannot? :read, @article + # authorize! :read, @article # end # - # The unauthorized! method accepts an optional argument which sets the - # message of the exception. + # A :message option can be passed to specify a different message. # - # You can rescue from the exception in the controller to define the behavior. + # authorize! :read, @article, :message => "Not authorized to read #{@article.name}" + # + # You can rescue from the exception in the controller to customize how unauthorized + # access is displayed to the user. # # class ApplicationController < ActionController::Base # rescue_from CanCan::AccessDenied do |exception| @@ -136,10 +139,20 @@ def self.included(base) # end # end # - # See the load_and_authorize_resource method to automatically add - # the "unauthorized!" behavior to a RESTful controller's actions. - def unauthorized!(message = "You are not authorized to access this page.") - raise AccessDenied, message + # See the CanCan::AccessDenied exception for more details on working with the exception. + # + # See the load_and_authorize_resource method to automatically add the authorize! behavior + # to the default RESTful actions. + def authorize!(action, subject, *args) + message = nil + if args.last.kind_of?(Hash) && args.last.has_key?(:message) + message = args.pop[:message] + end + raise AccessDenied.new(message, action, subject) if cannot?(action, subject, *args) + end + + def unauthorized!(message = nil) + raise ImplementationRemoved, "The unauthorized! method has been removed from CanCan, use authorize! instead." end # Creates and returns the current user's ability and caches it. If you diff --git a/lib/cancan/controller_resource.rb b/lib/cancan/controller_resource.rb index 39bd8197..83065e04 100644 --- a/lib/cancan/controller_resource.rb +++ b/lib/cancan/controller_resource.rb @@ -1,7 +1,7 @@ module CanCan class ControllerResource # :nodoc: def initialize(controller, name, parent = nil, options = {}) - raise CanCan::Error, "The :class option has been renamed to :resource for specifying the class in CanCan." if options.has_key? :class + raise ImplementationRemoved, "The :class option has been renamed to :resource for specifying the class in CanCan." if options.has_key? :class @controller = controller @name = name @parent = parent diff --git a/lib/cancan/exceptions.rb b/lib/cancan/exceptions.rb new file mode 100644 index 00000000..e7166d4f --- /dev/null +++ b/lib/cancan/exceptions.rb @@ -0,0 +1,43 @@ +module CanCan + # A general CanCan exception + class Error < StandardError; end + + # Raised when removed code is called, an alternative solution is provided in message. + class ImplementationRemoved < Error; end + + # This error is raised when a user isn't allowed to access a given controller action. + # This usually happens within a call to ControllerAdditions#authorized! but can be + # raised manually. + # + # raise CanCan::AccessDenied.new("Not authorized!", :read, Article) + # + # The passed message, action, and subject are optional and can later be retrieved when + # rescuing from the exception. + # + # exception.message # => "Not authorized!" + # exception.action # => :read + # exception.subject # => Article + # + # If the message is not specified (or is nil) it will default to "You are anot authorized + # to access this page." This default can be overridden by setting default_message. + # + # exception.default_message = "Default error message" + # exception.message # => "Default error message" + # + # See ControllerAdditions#authorized! for more information on rescuing from this exception. + class AccessDenied < Error + attr_reader :action, :subject + attr_writer :default_message + + def initialize(message = nil, action = nil, subject = nil) + @message = message + @action = action + @subject = subject + @default_message = "You are not authorized to access this page." + end + + def to_s + @message || @default_message + end + end +end diff --git a/lib/cancan/matchers.rb b/lib/cancan/matchers.rb index d9628bc5..a71c458f 100644 --- a/lib/cancan/matchers.rb +++ b/lib/cancan/matchers.rb @@ -1,13 +1,13 @@ Spec::Matchers.define :be_able_to do |*args| - match do |model| - model.can?(*args) + match do |ability| + ability.can?(*args) end - failure_message_for_should do |model| + failure_message_for_should do |ability| "expected to be able to #{args.map(&:inspect).join(" ")}" end - failure_message_for_should_not do |model| + failure_message_for_should_not do |ability| "expected not to be able to #{args.map(&:inspect).join(" ")}" end end diff --git a/lib/cancan/resource_authorization.rb b/lib/cancan/resource_authorization.rb index fe8d22df..b11842e9 100644 --- a/lib/cancan/resource_authorization.rb +++ b/lib/cancan/resource_authorization.rb @@ -30,7 +30,7 @@ def load_resource end def authorize_resource - @controller.unauthorized! if @controller.cannot?(params[:action].to_sym, resource.model_instance || resource.model_class) + @controller.authorize!(params[:action].to_sym, resource.model_instance || resource.model_class) end private diff --git a/spec/cancan/controller_additions_spec.rb b/spec/cancan/controller_additions_spec.rb index 1460177f..9239557d 100644 --- a/spec/cancan/controller_additions_spec.rb +++ b/spec/cancan/controller_additions_spec.rb @@ -5,29 +5,48 @@ @controller_class = Class.new @controller = @controller_class.new stub(@controller).params { {} } + stub(@controller).current_user { :current_user } mock(@controller_class).helper_method(:can?, :cannot?) @controller_class.send(:include, CanCan::ControllerAdditions) end - it "should raise access denied with default message when calling unauthorized!" do - lambda { - @controller.unauthorized! - }.should raise_error(CanCan::AccessDenied, "You are not authorized to access this page.") + it "should raise ImplementationRemoved when attempting to call 'unauthorized!' on a controller" do + lambda { @controller.unauthorized! }.should raise_error(CanCan::ImplementationRemoved) + end + + it "should raise access denied exception if ability us unauthorized to perform a certain action" do + begin + @controller.authorize! :read, :foo, 1, 2, 3, :message => "Access denied!" + rescue CanCan::AccessDenied => e + e.message.should == "Access denied!" + e.action.should == :read + e.subject.should == :foo + else + fail "Expected CanCan::AccessDenied exception to be raised" + end end - it "should raise access denied with custom message when calling unauthorized!" do - lambda { - @controller.unauthorized! "Access denied!" - }.should raise_error(CanCan::AccessDenied, "Access denied!") + it "should not raise access denied exception if ability is authorized to perform an action" do + @controller.current_ability.can :read, :foo + lambda { @controller.authorize!(:read, :foo) }.should_not raise_error + end + + it "should raise access denied exception with default message if not specified" do + begin + @controller.authorize! :read, :foo + rescue CanCan::AccessDenied => e + e.default_message = "Access denied!" + e.message.should == "Access denied!" + else + fail "Expected CanCan::AccessDenied exception to be raised" + end end it "should have a current_ability method which generates an ability for the current user" do - stub(@controller).current_user { :current_user } @controller.current_ability.should be_kind_of(Ability) end it "should provide a can? and cannot? methods which go through the current ability" do - stub(@controller).current_user { :current_user } @controller.current_ability.should be_kind_of(Ability) @controller.can?(:foo, :bar).should be_false @controller.cannot?(:foo, :bar).should be_true diff --git a/spec/cancan/controller_resource_spec.rb b/spec/cancan/controller_resource_spec.rb index 64f639ce..c93b055e 100644 --- a/spec/cancan/controller_resource_spec.rb +++ b/spec/cancan/controller_resource_spec.rb @@ -54,6 +54,6 @@ it "should raise an exception when specifying :class option since it is no longer used" do lambda { CanCan::ControllerResource.new(@controller, :ability, nil, :class => Person) - }.should raise_error(CanCan::Error) + }.should raise_error(CanCan::ImplementationRemoved) end end diff --git a/spec/cancan/exceptions_spec.rb b/spec/cancan/exceptions_spec.rb new file mode 100644 index 00000000..6ea6b437 --- /dev/null +++ b/spec/cancan/exceptions_spec.rb @@ -0,0 +1,35 @@ +require "spec_helper" + +describe CanCan::AccessDenied do + describe "with action and subject" do + before(:each) do + @exception = CanCan::AccessDenied.new(nil, :some_action, :some_subject) + end + + it "should have action and subject accessors" do + @exception.action.should == :some_action + @exception.subject.should == :some_subject + end + + it "should have a changable default message" do + @exception.message.should == "You are not authorized to access this page." + @exception.default_message = "Unauthorized!" + @exception.message.should == "Unauthorized!" + end + end + + describe "with only a message" do + before(:each) do + @exception = CanCan::AccessDenied.new("Access denied!") + end + + it "should have nil action and subject" do + @exception.action.should be_nil + @exception.subject.should be_nil + end + + it "should have passed message" do + @exception.message.should == "Access denied!" + end + end +end diff --git a/spec/cancan/resource_authorization_spec.rb b/spec/cancan/resource_authorization_spec.rb index bb21fd62..29f20481 100644 --- a/spec/cancan/resource_authorization_spec.rb +++ b/spec/cancan/resource_authorization_spec.rb @@ -3,7 +3,6 @@ describe CanCan::ResourceAuthorization do before(:each) do @controller = Object.new # simple stub for now - stub(@controller).unauthorized! { raise CanCan::AccessDenied } end it "should load the resource into an instance variable if params[:id] is specified" do @@ -49,19 +48,15 @@ it "should perform authorization using controller action and loaded model" do @controller.instance_variable_set(:@ability, :some_resource) - stub(@controller).cannot?(:show, :some_resource) { true } + stub(@controller).authorize!(:show, :some_resource) { raise CanCan::AccessDenied } authorization = CanCan::ResourceAuthorization.new(@controller, :controller => "abilities", :action => "show") - lambda { - authorization.authorize_resource - }.should raise_error(CanCan::AccessDenied) + lambda { authorization.authorize_resource }.should raise_error(CanCan::AccessDenied) end it "should perform authorization using controller action and non loaded model" do - stub(@controller).cannot?(:show, Ability) { true } + stub(@controller).authorize!(:show, Ability) { raise CanCan::AccessDenied } authorization = CanCan::ResourceAuthorization.new(@controller, :controller => "abilities", :action => "show") - lambda { - authorization.authorize_resource - }.should raise_error(CanCan::AccessDenied) + lambda { authorization.authorize_resource }.should raise_error(CanCan::AccessDenied) end it "should call load_resource and authorize_resource for load_and_authorize_resource" do