Wrong ability precedence for PostgreSQL #788

Open
LukasSkywalker opened this Issue Dec 3, 2012 · 8 comments

Comments

Projects
None yet
4 participants
@LukasSkywalker

I have my abilities defined as follows:

can :read, :all

cannot :read, Topic, :private => true
cannot :read, Topic, :status => 'pending'

can :read, Topic, :private => true, :user_id => user.id
can :read, Topic, :private => false

This returns all Topics that are not private (see last line). Removing the last line returns all Topics that are private AND belong to the user. Shouldn't can's be ORed together?
Btw: for both situations, also 'pending' topics are returned.

@ansel1

This comment has been minimized.

Show comment Hide comment
@ansel1

ansel1 Dec 4, 2012

I have something similiar:

if user.super?
  can :manage, :all
end
# all users
can :manage, Profile, user_id: user.id

should mean super users can manage everything, regular users can manage their own profile. You'd expect the accessible_by query for super users to return all Profiles, but it doesn't. The query ends up AND'ing any conditions on any rules relevant to Profiles.

"SELECT "profiles".* FROM "profiles" INNER JOIN "users" ON "users"."id" = "profiles"."user_id" WHERE "users"."id" = 242 AND (NULL)"

ansel1 commented Dec 4, 2012

I have something similiar:

if user.super?
  can :manage, :all
end
# all users
can :manage, Profile, user_id: user.id

should mean super users can manage everything, regular users can manage their own profile. You'd expect the accessible_by query for super users to return all Profiles, but it doesn't. The query ends up AND'ing any conditions on any rules relevant to Profiles.

"SELECT "profiles".* FROM "profiles" INNER JOIN "users" ON "users"."id" = "profiles"."user_id" WHERE "users"."id" = 242 AND (NULL)"
@ansel1

This comment has been minimized.

Show comment Hide comment
@ansel1

ansel1 Dec 4, 2012

Sorry, mine must be a different issue. It seems my case is explicitly spelled out in the specs (https://github.com/ryanb/cancan/blob/master/spec/cancan/model_adapters/active_record_adapter_spec.rb#L248) and should work. back to the debugger...

ansel1 commented Dec 4, 2012

Sorry, mine must be a different issue. It seems my case is explicitly spelled out in the specs (https://github.com/ryanb/cancan/blob/master/spec/cancan/model_adapters/active_record_adapter_spec.rb#L248) and should work. back to the debugger...

@ansel1

This comment has been minimized.

Show comment Hide comment
@ansel1

ansel1 Dec 4, 2012

mine is #765

ansel1 commented Dec 4, 2012

mine is #765

@runemadsen

This comment has been minimized.

Show comment Hide comment
@runemadsen

runemadsen Dec 4, 2012

I'm having the same problem with Postgresql. These two simple lines of code allows to index books only when there's a permission with user_id of 1 AND a package with a type of "free". I believe this is wrong. The documentation states that this should clearly result in an AND. Any tips?

can :index, Book, :permissions => { :user_id => 1}
can :index, Book, :packages => { :type => "free"}

I'm having the same problem with Postgresql. These two simple lines of code allows to index books only when there's a permission with user_id of 1 AND a package with a type of "free". I believe this is wrong. The documentation states that this should clearly result in an AND. Any tips?

can :index, Book, :permissions => { :user_id => 1}
can :index, Book, :packages => { :type => "free"}
@runemadsen

This comment has been minimized.

Show comment Hide comment
@runemadsen

runemadsen Dec 4, 2012

Investigating further, it seems like it's doing the correct call. But I'm still ending up with an incorrect number of books. Any ideas?

SELECT "books".* FROM "books" INNER JOIN "packages" ON "packages"."book_id" = "books"."id" INNER JOIN "permissions" ON "permissions"."book_id" = "books"."id" WHERE (("packages"."type" = 'free') OR ("permissions"."user_id" = 1))

Investigating further, it seems like it's doing the correct call. But I'm still ending up with an incorrect number of books. Any ideas?

SELECT "books".* FROM "books" INNER JOIN "packages" ON "packages"."book_id" = "books"."id" INNER JOIN "permissions" ON "permissions"."book_id" = "books"."id" WHERE (("packages"."type" = 'free') OR ("permissions"."user_id" = 1))
@runemadsen

This comment has been minimized.

Show comment Hide comment
@runemadsen

runemadsen Dec 4, 2012

After investigating for an hour, I recreated this scenario with a failing test:

it "should merge multiple nested conditions" do
      @ability.can :read, Article, :comments => {:spam => true}
      @ability.can :read, Article, :tags => {:tag => "awesome"}
      article1 = Article.create!(:published => true, :secret => false)
      article2 = Article.create!(:published => true, :secret => true)
      article3 = Article.create!(:published => false, :secret => true)
      article4 = Article.create!(:published => false, :secret => false)
      comment1 = Comment.create!(:article => article1, :spam => true)
      comment2 = Comment.create!(:article => article2, :spam => false)
      tag1 = Tag.create!(:article => article3, :tag => "awesome")
      tag2 = Tag.create!(:article => article4, :tag => "terrible")
      Article.accessible_by(@ability).should == [article1, article3]
end

As you can see, when passing nested conditions on separate associations to the ability, it will not work. If you comment out either of the can calls, that line will work.

You can see the full commit here in my fork:
runemadsen/cancan@98446bb

I'm not an expert of SQL, but it looks fine to me. Does anyone have an idea on what could be wrong?

After investigating for an hour, I recreated this scenario with a failing test:

it "should merge multiple nested conditions" do
      @ability.can :read, Article, :comments => {:spam => true}
      @ability.can :read, Article, :tags => {:tag => "awesome"}
      article1 = Article.create!(:published => true, :secret => false)
      article2 = Article.create!(:published => true, :secret => true)
      article3 = Article.create!(:published => false, :secret => true)
      article4 = Article.create!(:published => false, :secret => false)
      comment1 = Comment.create!(:article => article1, :spam => true)
      comment2 = Comment.create!(:article => article2, :spam => false)
      tag1 = Tag.create!(:article => article3, :tag => "awesome")
      tag2 = Tag.create!(:article => article4, :tag => "terrible")
      Article.accessible_by(@ability).should == [article1, article3]
end

As you can see, when passing nested conditions on separate associations to the ability, it will not work. If you comment out either of the can calls, that line will work.

You can see the full commit here in my fork:
runemadsen/cancan@98446bb

I'm not an expert of SQL, but it looks fine to me. Does anyone have an idea on what could be wrong?

@runemadsen

This comment has been minimized.

Show comment Hide comment
@runemadsen

runemadsen Feb 11, 2013

After looking into this, the failed test actually makes sense. If you define abilites like this, where the Comment and Tag class both belongs_to Article...

@ability.can :read, Article, :comments => {:body => "hello"}
@ability.can :read, Article, :tags => {:tag => "awesome"}

... CanCan will automatically gather these into a single INNER JOIN query like this:

Article.where(""(\"tags\".\"tag\" = 'awesome') OR (\"comments\".\"body\" = 'hello')").joins([:tags, :comments])

But it really doesn't matter that the query OR's, as it's an INNER JOIN, and it will only fetch articles that have at least a tag and a comment. The problem here is that this is in no way visible to the developer when defining abilities.

I wanted to ask you (cc @ryanb @natebird) before implementing this, but I can see two solutions:

  1. If the .joins array is a flat array of symbols, automatically convert the array to a LEFT JOIN string, to get this query:
Article.where(""(\"tags\".\"tag\" = 'awesome') OR (\"comments\".\"body\" = 'hello')").joins(LEFT JOIN tags ON tags.article_id = articles.id LEFT JOIN comments ON comments.article_id = articles.id")

That would solve the problem, although it would get a little hairy when we have nested hashes as joins.

  1. Not care about it and let people solve it with blocks. However, this would require some more documentation. The first thing I did was to add 2 abilities like above, run Rails, and see everything fail. It's not super intuitive, as it's associations from different abilities.

Let me know.

  • Rune

After looking into this, the failed test actually makes sense. If you define abilites like this, where the Comment and Tag class both belongs_to Article...

@ability.can :read, Article, :comments => {:body => "hello"}
@ability.can :read, Article, :tags => {:tag => "awesome"}

... CanCan will automatically gather these into a single INNER JOIN query like this:

Article.where(""(\"tags\".\"tag\" = 'awesome') OR (\"comments\".\"body\" = 'hello')").joins([:tags, :comments])

But it really doesn't matter that the query OR's, as it's an INNER JOIN, and it will only fetch articles that have at least a tag and a comment. The problem here is that this is in no way visible to the developer when defining abilities.

I wanted to ask you (cc @ryanb @natebird) before implementing this, but I can see two solutions:

  1. If the .joins array is a flat array of symbols, automatically convert the array to a LEFT JOIN string, to get this query:
Article.where(""(\"tags\".\"tag\" = 'awesome') OR (\"comments\".\"body\" = 'hello')").joins(LEFT JOIN tags ON tags.article_id = articles.id LEFT JOIN comments ON comments.article_id = articles.id")

That would solve the problem, although it would get a little hairy when we have nested hashes as joins.

  1. Not care about it and let people solve it with blocks. However, this would require some more documentation. The first thing I did was to add 2 abilities like above, run Rails, and see everything fail. It's not super intuitive, as it's associations from different abilities.

Let me know.

  • Rune
@xhoy

This comment has been minimized.

Show comment Hide comment
@xhoy

xhoy 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!

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