From 60848143b78bb82227894561109522a4f792c73e Mon Sep 17 00:00:00 2001 From: Ryan Bates Date: Tue, 20 Jul 2010 11:04:03 -0700 Subject: [PATCH] refactoring can definition matching behavior --- lib/cancan/ability.rb | 37 +++++++----------- lib/cancan/can_definition.rb | 38 ++++++++---------- spec/cancan/ability_spec.rb | 75 ++++++++++++++++++++++-------------- spec/spec_helper.rb | 1 + 4 files changed, 76 insertions(+), 75 deletions(-) diff --git a/lib/cancan/ability.rb b/lib/cancan/ability.rb index afc3d00..c376622 100644 --- a/lib/cancan/ability.rb +++ b/lib/cancan/ability.rb @@ -50,12 +50,10 @@ module CanCan # Also see the RSpec Matchers to aid in testing. def can?(action, subject, *extra_args) raise Error, "Nom nom nom. I eated it." if action == :has && subject == :cheezburger - matching_can_definition(action, subject) do |can_definition| - unless (can = can_definition.can?(action, subject, extra_args)) == :_NOT_MATCHED - return can - end + match = relevant_can_definitions(action, subject).detect do |can_definition| + can_definition.matches_conditions?(action, subject, extra_args) end - false + match ? match.base_behavior : false end # Convenience method which works the same as "can?" but returns the opposite value. @@ -200,12 +198,12 @@ module CanCan # If the ability is defined using a block then this will raise an exception since a hash of conditions cannot be # determined from that. def conditions(action, subject, options = {}) - matched = matching_can_definition(action, subject) - unless matched.empty? - if matched.any?{|can_definition| can_definition.only_block? } + relevant = relevant_can_definitions(action, subject) + unless relevant.empty? + if relevant.any?{|can_definition| can_definition.only_block? } raise Error, "Cannot determine ability conditions from block for #{action.inspect} #{subject.inspect}" end - matched.map{|can_definition| + relevant.map{|can_definition| [can_definition.base_behavior, can_definition.conditions(options)] } else @@ -255,7 +253,7 @@ module CanCan # Returns the associations used in conditions. This is usually used in the :joins option for a search. # See ActiveRecordAdditions#accessible_by for use in Active Record. def association_joins(action, subject) - can_definitions = matching_can_definition(action, subject) + can_definitions = relevant_can_definitions(action, subject) unless can_definitions.empty? if can_definitions.any?{|can_definition| can_definition.only_block? } raise Error, "Cannot determine association joins from block for #{action.inspect} #{subject.inspect}" @@ -281,19 +279,12 @@ module CanCan @can_definitions ||= [] end - def matching_can_definition(action, subject) - if block_given? - can_definitions.reverse.each do |can_definition| - can_definition.expanded_actions = expand_actions(can_definition.actions) - if can_definition.matches? action, subject - yield can_definition - break if can_definition.definitive? - end - end - else - matched = [] - matching_can_definition(action, subject){|can_definition| matched << can_definition} - matched + # Returns an array of CanDefinition instances which match the action and subject + # This does not take into consideration any hash conditions or block statements + def relevant_can_definitions(action, subject) + can_definitions.reverse.select do |can_definition| + can_definition.expanded_actions = expand_actions(can_definition.actions) + can_definition.relevant? action, subject end end diff --git a/lib/cancan/can_definition.rb b/lib/cancan/can_definition.rb index 4c5c8f0..69508d8 100644 --- a/lib/cancan/can_definition.rb +++ b/lib/cancan/can_definition.rb @@ -3,7 +3,7 @@ module CanCan # it holds the information about a "can" call made on Ability and provides # helpful methods to determine permission checking and conditions hash generation. class CanDefinition # :nodoc: - attr_reader :conditions, :block, :base_behavior, :definitive + attr_reader :conditions, :block, :base_behavior include ActiveSupport::Inflector attr_reader :block attr_reader :actions @@ -21,14 +21,20 @@ module CanCan @block = block end - def matches?(action, subject) + # Matches both the subject and action, not necessarily the conditions + def relevant?(action, subject) matches_action?(action) && matches_subject?(subject) end - def can?(action, subject, extra_args) - result = can_without_base_behavior?(action, subject, extra_args) - return :_NOT_MATCHED if result == :_NOT_MATCHED || !result - @base_behavior ? result : !result + # Matches the block or conditions hash + def matches_conditions?(action, subject, extra_args) + if @block + call_block(action, subject, extra_args) + elsif @conditions.kind_of?(Hash) && subject.class != Class + matches_conditions_hash?(subject) + else + true + end end # Returns a hash of conditions. If the ":tableize => true" option is passed @@ -45,10 +51,6 @@ module CanCan end end - def definitive? - conditions_empty? && @block.nil? - end - def only_block? conditions_empty? && !@block.nil? end @@ -83,24 +85,14 @@ module CanCan @subjects.include?(:all) || @subjects.include?(subject) || @subjects.any? { |sub| sub.kind_of?(Class) && subject.kind_of?(sub) } end - def can_without_base_behavior?(action, subject, extra_args) - if @block - call_block(action, subject, extra_args) - elsif @conditions.kind_of?(Hash) && subject.class != Class - matches_conditions?(subject) - else - true - end - end - - def matches_conditions?(subject, conditions = @conditions) + def matches_conditions_hash?(subject, conditions = @conditions) conditions.all? do |name, value| attribute = subject.send(name) if value.kind_of?(Hash) if attribute.kind_of? Array - attribute.any? { |element| matches_conditions? element, value } + attribute.any? { |element| matches_conditions_hash? element, value } else - matches_conditions? attribute, value + matches_conditions_hash? attribute, value end elsif value.kind_of?(Array) || value.kind_of?(Range) value.include? attribute diff --git a/spec/cancan/ability_spec.rb b/spec/cancan/ability_spec.rb index 4c4858e..57b81c5 100644 --- a/spec/cancan/ability_spec.rb +++ b/spec/cancan/ability_spec.rb @@ -16,56 +16,63 @@ describe CanCan::Ability do @ability.can?(:foodfight, String).should be_false end - it "should return what block returns on a can call, except for nil and false" do + it "should pass true to `can?` when non false/nil is returned in block" do @ability.can :read, :all @ability.can :read, Symbol do |sym| - [ sym ] + "foo" # TODO test that sym is nil when no instance is passed end - @ability.can?(:read, Symbol).should == [ nil ] - @ability.can?(:read, :some_symbol).should == [ :some_symbol ] + @ability.can?(:read, :some_symbol).should == true end it "should pass to previous can definition, if block returns false or nil" do - @ability.can :read, :all - @ability.can :read, Symbol do |sym| - sym + @ability.can :read, Symbol + @ability.can :read, Integer do |i| + i < 5 end @ability.can :read, Integer do |i| - i > 0 + i > 10 end @ability.can?(:read, Symbol).should be_true - @ability.can?(:read, :some_symbol).should == :some_symbol + @ability.can?(:read, 11).should be_true @ability.can?(:read, 1).should be_true - @ability.can?(:read, -1).should be_true + @ability.can?(:read, 6).should be_false end it "should pass class with object if :all objects are accepted" do @ability.can :preview, :all do |object_class, object| - [object_class, object] + object_class.should == Fixnum + object.should == 123 + @block_called = true end - @ability.can?(:preview, 123).should == [Fixnum, 123] + @ability.can?(:preview, 123) + @block_called.should be_true end it "should pass class with no object if :all objects are accepted and class is passed directly" do @ability.can :preview, :all do |object_class, object| - [object_class, object] + object_class.should == Hash + object.should be_nil + @block_called = true end - @ability.can?(:preview, Hash).should == [Hash, nil] + @ability.can?(:preview, Hash) + @block_called.should be_true end it "should pass action and object for global manage actions" do @ability.can :manage, Array do |action, object| - [action, object] + action.should == :stuff + object.should == [1, 2] + @block_called = true end - @ability.can?(:stuff, [1, 2]).should == [:stuff, [1, 2]] - @ability.can?(:stuff, Array).should == [:stuff, nil] + @ability.can?(:stuff, [1, 2]).should + @block_called.should be_true end it "should alias update or destroy actions to modify action" do @ability.alias_action :update, :destroy, :to => :modify - @ability.can(:modify, :all) { :modify_called } - @ability.can?(:update, 123).should == :modify_called - @ability.can?(:destroy, 123).should == :modify_called + @ability.can :modify, :all + @ability.can?(:update, 123).should be_true + @ability.can?(:destroy, 123).should be_true end it "should allow deeply nested aliased actions" do @@ -77,10 +84,13 @@ describe CanCan::Ability do it "should return block result for action, object_class, and object for any action" do @ability.can :manage, :all do |action, object_class, object| - [action, object_class, object] + action.should == :foo + object_class.should == Fixnum + object.should == 123 + @block_called = true end - @ability.can?(:foo, 123).should == [:foo, Fixnum, 123] - @ability.can?(:bar, Fixnum).should == [:bar, Fixnum, nil] + @ability.can?(:foo, 123) + @block_called.should be_true end it "should automatically alias index and show into read calls" do @@ -90,10 +100,10 @@ describe CanCan::Ability do end it "should automatically alias new and edit into create and update respectively" do - @ability.can(:create, :all) { :create_called } - @ability.can(:update, :all) { :update_called } - @ability.can?(:new, 123).should == :create_called - @ability.can?(:edit, 123).should == :update_called + @ability.can :create, :all + @ability.can :update, :all + @ability.can?(:new, 123).should be_true + @ability.can?(:edit, 123).should be_true end it "should not respond to prepare (now using initialize)" do @@ -260,7 +270,10 @@ describe CanCan::Ability do @ability.cannot :read, SqlSanitizer @ability.can :read, SqlSanitizer, :blocked => false, :user_id => 1 - @ability.sql_conditions(:read, SqlSanitizer).should == 'blocked=false AND user_id=1' + result = @ability.sql_conditions(:read, SqlSanitizer) + result.should include("blocked=false") + result.should include(" AND ") + result.should include("user_id=1") end it "should return `true condition` for single `can` definition in front of default `can` condition" do @@ -287,7 +300,11 @@ describe CanCan::Ability do @ability.can :read, SqlSanitizer @ability.cannot :read, SqlSanitizer, :blocked => true, :user_id => 1 - @ability.sql_conditions(:read, SqlSanitizer).should == 'not (blocked=true AND user_id=1)' + result = @ability.sql_conditions(:read, SqlSanitizer) + result.should include("not ") + result.should include("blocked=true") + result.should include(" AND ") + result.should include("user_id=1") end it "should return appropriate sql conditions in complex case" do diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 6001ea4..2654c31 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -22,6 +22,7 @@ end class Person end +# Maybe we can use ActiveRecord directly here instead of duplicating the behavior class SqlSanitizer def self.sanitize_sql(hash_cond) case hash_cond