Inconsistent ability on class with defined ability on instances #731

Open
momolog opened this Issue Aug 28, 2012 · 7 comments

Comments

Projects
None yet
4 participants

momolog commented Aug 28, 2012

Assume we have a definition like

can(:message, User){|other_user|  other_user.active? }

a subsequent call

can? :message, User

will always yield true, because there is no object to check against.
(see also here: #392)

Now if we define it the other way around (we do this a lot, especially for admins, who can :manage everything, except in some special cases:

can(:message, User)
cannot(:message, User){|other_user| !other_user.active?}

the same subsequent call to

can? :message, User

will always yield false(!!) for the same reason, even though all instance abilities are exactly the same.

I find this behaviour confusing and inconsequent. The ability on the class effectively depends on the order of the definitions for the instances.

Imho. the right behaviour here would be, that if any positive ability for instances were defined (using a block), then

can? :message, User

should always return true, even if a cannot rule with a block is defined later. That would make the behaviour more consistent and predictable.
Also semantically it makes sense: If we can define cans and cannots for an action for instances, there is in principle an ability to do this action for the class.

mltsy commented Sep 19, 2012

I agree with you to some degree, but I feel like if the condition is based on the object itself, you should put that in the first (can) definition, and not have two. Can you come up with an example where that isn't possible or doesn't make sense?

In any case, I would actually think that if there is a rule with a condition and one without, then the rule without a condition should take precedence when no instance is passed. If they both have conditions... they shouldn't both have conditions... :) So I would fall back on the default which would be to deny access.

The intention of can? :manage, Class is not entirely clear to me. In some cases is could mean "Can I manage any instance I want," or it could mean "Am I ever able to manage an instance of this class?"

+1 for fixing this

momolog commented Feb 19, 2013

@mltsy: can manage: MyClass is saying: "You can call any action on any object of this class."
It's a good start for defining the abilities of admin users. After specifying that you could (if this ticket was fixed) selectively deny certain abilities that even an admin user does not have - based on instance properties.

You get away with a lot less code as opposed to defining the abilities for all actions individually (likely the reason for adding the :manage ability to CanCan in the first place).

This strategy only makes sense, if the amount of positive abilities is higher than the exceptions.

mltsy commented Feb 19, 2013

My question was just about what it means to ask can? :ability, ClassName without passing a specific instance. Is that supposed to return true only if the user can perform ability on all instances of ClassName, or if the user can perform ability on at least one hypothetical instance of ClassName?

In the first case, you would just return false if there are any conditional statements (because there are some instances for which the user can not perform that ability), in the second case, you would return true as long as there are any can directives (conditional or not) for the specified user, ability and ClassName, because that means there is at least one hypothetical instance on which the user could potentially perform that ability.

I would assume the first case to be the more useful and secure. Asking a question about permissions without giving all the necessary information to positively confirm access should generally result in denying it (i.e. can? :message, User where there are some cases access should be granted, and some it shouldn't, depending on the user in question, should deny access). The only case it should grant access is if the current user has permission to perform that ability on all users, and therefore it doesn't matter which user is in question.

Does that make sense? Do you still disagree?

I do see your point about writing concise code, but I just don't understand what kind of case you would need to use can :message, User without specifying a specific user, and expect to get true, when there are actually some users which the current user is not allowed to message... that seems wrong.

I suppose I could see the case... where you would ask that in order to display a form to try to message a user (which user could be selected in the form), and when submitted it would return a permission error if you aren't actually able to message that particular one? But I think we might need a special syntax for that case, since it's actually asking something more like can? :message, :some=>User or ... can_sometimes? :message, User

Is that the kind of situation you're thinking of? What do you think of that solution?

momolog commented Feb 20, 2013

@mltsy: True, you could argue both ways. I have two complaints here:

  1. The answer to can? :message, User (whether the answer be true or false) should not depend on the order in which class and instance abilities are defined.
  2. If class and instance abilities are defined, answering true to the general question is more useful. It allows specifying "you can do this action on this class" and then selectively giving exceptions if necessary. truewould mean "there are cases in which you can do this".
    Answering false will effectively make the class ability useless - it will never even be considered and the information is lost.

mltsy commented Feb 20, 2013

Yeah, I definitely agree with you on point 1 - the order of declaration shouldn't make a difference. I don't necessarily agree on point 2 that it's more useful or correct to return true, but I can definitely see a valid use-case where that would be helpful. That is why I think a different syntax for that use-case would be a good thing.

However, I see in the documentation now that it is documented as working as you describe ;) So, in that case I can't argue that it shouldn't! https://github.com/ryanb/cancan/wiki/Checking-Abilities

I do think, though, that the pitfalls of someone misunderstanding that and thinking it works the other way are much more costly than the pitfalls of the opposite case where it actually returns true only if the current user can manage all users, but someone thinks it will return true if the user can manage some user, hypothetically (which is basically self-correcting, since the application simply wouldn't work until you fixed it).

In any case, this issue is mainly about fixing the order-of-declaration significance problem, which is agreed on 👍

xhoy commented Jul 1, 2014

Thanks for your submission! The ryanb/cancan repository has been inactive since Sep 06, 2013.
Since only Ryan himself has commit permissions, the CanCan project is on a standstill.

CanCan has many open issues, including missing support for Rails 4. To keep CanCan alive, an active fork exists at cancancommunity/cancancan. The new gem is cancancan. More info is available at #994.

If your pull request or issue is still applicable, it would be really appreciated if you resubmit it to CanCanCan.

We hope to see you on the other side!

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