conditions are ignored? #615

Closed
Silex opened this Issue May 7, 2012 · 7 comments

Comments

Projects
None yet
3 participants
@Silex

Silex commented May 7, 2012

Maybe it's me misusing CanCan, but see this simple paste (http://pastie.org/3872605)

p = Person.last
ability = Ability.new(p)
puts ability.can? :create, Task
ability.can :create, Task, :person_id => 123
puts ability.can? :create, Task

This prints false then true. Shouldn't it print false both times?

This is with 1.6.7.

@Silex

This comment has been minimized.

Show comment Hide comment
@Silex

Silex May 7, 2012

Ok, according to https://github.com/ryanb/cancan/wiki/Nested-Resources I'm actually supposed to check against p.tasks.build instead of Task, but I think this is still somewhat of a bug.

The issue is in Rule#matches_conditions, when you do ability.can? :create, Task, it falls back in the last case of the when statement which is @conditions.empty? ? true : @base_behavior

There the conditions are never checked and it yields (IMHO) a false positive.

Silex commented May 7, 2012

Ok, according to https://github.com/ryanb/cancan/wiki/Nested-Resources I'm actually supposed to check against p.tasks.build instead of Task, but I think this is still somewhat of a bug.

The issue is in Rule#matches_conditions, when you do ability.can? :create, Task, it falls back in the last case of the when statement which is @conditions.empty? ? true : @base_behavior

There the conditions are never checked and it yields (IMHO) a false positive.

@the8472

This comment has been minimized.

Show comment Hide comment
@the8472

the8472 May 10, 2012

The semantic of checking against a class is that you verify that for some instance of that class this action might succeed. This is useful to not offer some kind of stuff in the first place (think: not linking to the create-task action). Whether some specific task can be created has to be checked against an instance of course.

the8472 commented May 10, 2012

The semantic of checking against a class is that you verify that for some instance of that class this action might succeed. This is useful to not offer some kind of stuff in the first place (think: not linking to the create-task action). Whether some specific task can be created has to be checked against an instance of course.

@Silex

This comment has been minimized.

Show comment Hide comment
@Silex

Silex May 10, 2012

Okay... but I still think that if you have:

can :create, Task, :person_id => 123

Then the following should happen:

  1. can? :create, Person.find(123).tasks.build should be true (ok)
  2. can? :create, Person.find(456).tasks.build should be false (ok)
  3. can? :create, Task should be false (not ok)

Point 3 is clearly giving more rights than it should have done when you defined the rule, even if it's somewhat a misuse of CanCan.

Silex commented May 10, 2012

Okay... but I still think that if you have:

can :create, Task, :person_id => 123

Then the following should happen:

  1. can? :create, Person.find(123).tasks.build should be true (ok)
  2. can? :create, Person.find(456).tasks.build should be false (ok)
  3. can? :create, Task should be false (not ok)

Point 3 is clearly giving more rights than it should have done when you defined the rule, even if it's somewhat a misuse of CanCan.

@Silex

This comment has been minimized.

Show comment Hide comment
@Silex

Silex May 10, 2012

Hum, how do you check that someone has the right to read a task for any person if you don't have a person instance at hand?

E.g for admins, you give them the following right:

can :read, Task

And for normal users you give them the right to read their own tasks only:

can :read, Task, :person_id => person.id

Now in some admin form you naturally do:

if can? :read, Task
  puts 'Hello my admin'
end

Can you see the problem I have? How am I supposed to do?

Silex commented May 10, 2012

Hum, how do you check that someone has the right to read a task for any person if you don't have a person instance at hand?

E.g for admins, you give them the following right:

can :read, Task

And for normal users you give them the right to read their own tasks only:

can :read, Task, :person_id => person.id

Now in some admin form you naturally do:

if can? :read, Task
  puts 'Hello my admin'
end

Can you see the problem I have? How am I supposed to do?

@the8472

This comment has been minimized.

Show comment Hide comment
@the8472

the8472 May 10, 2012

Now in some admin form you naturally do
Cancan does not define roles, only abilities. So there are two types of check available:

a) Can a user do something with this specific instance?
b) Might a user be able to do something with some (but not necessarily all) instances of this class?

What you cannot ask is

c) Does the user have a role X

Roles are orthogonal to Abilities. E.g. you could define them as modules to add to your abilities and then check with current_ability.is_a? Admin (where Admin is a module).

An a possibly simpler solution is to provide an instance when you want field-specific checks.

if can? :read, Task.new(:person => nil)
  ...
end

the8472 commented May 10, 2012

Now in some admin form you naturally do
Cancan does not define roles, only abilities. So there are two types of check available:

a) Can a user do something with this specific instance?
b) Might a user be able to do something with some (but not necessarily all) instances of this class?

What you cannot ask is

c) Does the user have a role X

Roles are orthogonal to Abilities. E.g. you could define them as modules to add to your abilities and then check with current_ability.is_a? Admin (where Admin is a module).

An a possibly simpler solution is to provide an instance when you want field-specific checks.

if can? :read, Task.new(:person => nil)
  ...
end
@leeatchison

This comment has been minimized.

Show comment Hide comment
@leeatchison

leeatchison May 11, 2012

I know I have several places in several projects where I depend on the fact that:

can? :create, Task

when I have rules such as:

can :create, Task, :person_id => 123

Changing this would have major backwards compatibility issues, in my opinion.

I know I have several places in several projects where I depend on the fact that:

can? :create, Task

when I have rules such as:

can :create, Task, :person_id => 123

Changing this would have major backwards compatibility issues, in my opinion.

@Silex

This comment has been minimized.

Show comment Hide comment
@Silex

Silex May 11, 2012

Oh... ok that makes sense. Thanks for clearing it up.

Silex commented May 11, 2012

Oh... ok that makes sense. Thanks for clearing it up.

@Silex Silex closed this May 11, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment