Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix #494: Make sure to actually call block with nil when subject is a Class #540

Closed
wants to merge 1 commit into from

3 participants

@ninkendo

Since there seemed to be two conflicting specs for can? being called with a Class, one simply returning true and one broken spec for, calling a block, I think I resolved the conflict and fixed #494. Check the bug and see if this is a fix you agree with.

Thanks!

Ken

@Sija Sija referenced this pull request from a commit in Sija/cancan
Sijawusz Pur Rahnama Merged PR #540 b75b0ba
@jeremyf
Collaborator

[Verified] The specs all pass. And it closes an issue. :+1: :+1:

@ryanb
Owner

This isn't a bug but by design. CanCan used to actually work this way (passing nil into the block) but I changed it to skip the block. You can see my reasoning in issue #116. Also see Checking Abilities wiki page for more information.

I realize this is one of the most confusing parts of CanCan and worst of all can cause security holes. This behavior is fixed in CanCan 2.0 but requires a significant structure change to bring that fix back to 1.

@ryanb ryanb closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 18 additions and 6 deletions.
  1. +6 −2 lib/cancan/rule.rb
  2. +12 −4 spec/cancan/ability_spec.rb
View
8 lib/cancan/rule.rb
@@ -30,8 +30,12 @@ def relevant?(action, subject)
def matches_conditions?(action, subject, extra_args)
if @match_all
call_block_with_all(action, subject, extra_args)
- elsif @block && !subject_class?(subject)
- @block.call(subject, *extra_args)
+ elsif @block
+ if subject_class?(subject)
+ @block.call(nil)
+ else
+ @block.call(subject, *extra_args)
+ end
elsif @conditions.kind_of?(Hash) && subject.class == Hash
nested_subject_matches_conditions?(subject)
elsif @conditions.kind_of?(Hash) && !subject_class?(subject)
View
16 spec/cancan/ability_spec.rb
@@ -55,13 +55,16 @@
@block_called.should be_true
end
- it "should not call block when only class is passed, only return true" do
+ it "should call block with nil when only class is passed" do
@block_called = false
+ @passed_object = Object.new
@ability.can :preview, :all do |object|
@block_called = true
+ @passed_object = object
end
- @ability.can?(:preview, Hash).should be_true
- @block_called.should be_false
+ @ability.can?(:preview, Hash).should be_false
+ @block_called.should be_true
+ @passed_object.should be_nil
end
it "should pass only object for global manage actions" do
@@ -283,12 +286,17 @@ class A; include B; end
it "should pass nil to a block for ability on Module when no instance is passed" do
module B; end
class A; include B; end
+ @block_called = false
+ @passed_arg = Object.new
@ability.can :read, B do |sym|
- sym.should be_nil
+ @block_called = true
+ @passed_arg = sym
true
end
@ability.can?(:read, B).should be_true
@ability.can?(:read, A).should be_true
+ @passed_arg.should be_nil
+ @block_called.should be_true
end
it "passing a hash of subjects should check permissions through association" do
Something went wrong with that request. Please try again.