Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fetched records should match the most liberally defined ability #714

Closed
wants to merge 1 commit into from

5 participants

@iamvery

I ran into what appears to be a regression occurring somewhere between 1.6.5 and 1.6.8. Basically, I had defined abilities in my app something like this:

if user.role? :admin
  can :manage, :all
elsif user.role? :other
  #...
end

can :manage, Something, :user_id => user.id

The purpose of this is to say, "Admins can do anything, other users with roles can do 'such and such', and all users can manage their own Somethings". In 1.6.5 this worked great, with the accessible_by method returned "owned" models for general users and all models for admins. In 1.6.8 it would essentially "prefer" the last defined ability and only return "owned" models for every user (including admins).

I believe this has to do with the "mergeable conditions" feature introduced at some point. It doesn't take into account that there is a "super condition" in effect if it determines the conditions are not mergeable. Attached is a pull request with spec that fixes my problem, but it may or may not be the best solution due to my limited understanding / experience w/ this code. Also, I haven't used data mapper or mongoid, so I didn't even attempt to address any issues there.

As always, thanks for the GREAT lib!

(Just realized I didn't do a great job looking through existing pull requests / issues, so I hope I didn't waste my time here... But I AM glad @travisbot likes me! :P

@travisbot

This pull request passes (merged e2783f5 into b4285ae).

@coffeeaddict

Just as a side note (non critique)

what is wrong with:

if user.role? :admin
  # there is no point in eating CPU cycles beyond this point
  return can(:manage, :all)
elseif user.role? :other
 # ...
end
@iamvery

@coffeeaddict, that may be quite a good fix for this particular case!

I'm not sure its a solution to the problem though.Surely there could be a more complicated set of abilities that would need to continue beyond your return. Regardless, this definitely feels like a regression. Perhaps @ryanb hasn't wanted to bother with it because he's so far into v2 at this point.

What about this?

if user.role? :somethingeer
  return can :manage, Something
else if user.role? :widgeteer
  can :manage, Widgets
end

can :manage, Something, :user_id => user.id

Consider the above abilities where a user has both the somethingeer and widgeteer roles. Would that user be able to manage Widgets? That's just off the top of my head, so there may still be a better way to define these abilities.

@felixbuenemann

If I look at the wiki entry for ability precedence I think you should just split it into two if blocks and reverse the order, so the more liberal one comes last. (I'm refering to the first post.)

@jonathanhefner

I just encountered this issue. My use case was essentially:

can :read, [User, Post]

if user
  can :manage, User, id: user.id
  can :manage, Post, user_id: user.id
end

What's intended: anyone can read Users and Posts, but only logged in users can manage themselves and their Posts.

What actually happens: logged in users can only read their own User object and their own Posts.

@felixbuenemann

Yea, just change the order so can :read, [User, Post]comes after the if block.

@jonathanhefner

@fbuenemann That is what I did as the solution. I was more commenting for the principle of least surprise. Thank you for your help though. :)

@iamvery

@fbuenemann @jonathanhefner I was just looking back at this PR. Perhaps in hindsight its all mute. The wiki clearly states the abilities defined "lower" in code order are preferred by those above. Its been a really long time since I've thought about my original problem, but wouldn't just moving the can :manage, Something, :user_id => user.id line above everything fix the problem? It feels like @fbuenemann is spot on.

I'll try to think about this again some time and possibly close this issue out. Thanks for you guys' input :)

@jonathanhefner

@iamvery Agreed, it is easy to fix any problems encountered. And the strategy of "last rule wins" is easy to follow. But, I think in the example I mentioned there is no conflict to win:

# anyone can read these things
can :read, [User, Post]

if user
  # a user can read himself--redundant to the first rule, but not a contradiction
  can :manage, User, id: user.id  
  # a user can read his posts--also redundant, but not a contradiction
  can :manage, Post, user_id: user.id
end

If there actually was a conflict between rules, then it does makes sense for the last rule to win:

can :manage, Wiki
cannot :destroy, Wiki # conflicts the above and below lines
can :destroy, Wiki, user_id: user.id # redundant to 1st line but conflicts 2nd
@iamvery iamvery 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.
View
2  lib/cancan/model_adapters/active_record_adapter.rb
@@ -89,7 +89,7 @@ def database_records
if override_scope
@model_class.scoped.merge(override_scope)
elsif @model_class.respond_to?(:where) && @model_class.respond_to?(:joins)
- mergeable_conditions = @rules.select {|rule| rule.unmergeable? }.blank?
+ mergeable_conditions = conditions == true_sql || @rules.select {|rule| rule.unmergeable? }.blank?
if mergeable_conditions
@model_class.where(conditions).joins(joins)
else
View
8 spec/cancan/model_adapters/active_record_adapter_spec.rb
@@ -109,6 +109,14 @@
Comment.accessible_by(@ability).should == [comment1]
end
+ it "should fetch articles matching the most liberal ability" do
+ @ability.can :read, Article
+ @ability.can :read, Article, :published => true
+ article1 = Article.create!(:published => true)
+ article2 = Article.create!(:published => false)
+ Article.accessible_by(@ability).should == [article1, article2]
+ end
+
it "should allow conditions in SQL and merge with hash conditions" do
@ability.can :read, Article, :published => true
@ability.can :read, Article, ["secret=?", true]
Something went wrong with that request. Please try again.