Skip to content
This repository has been archived by the owner on Dec 12, 2021. It is now read-only.

:read ability not being logically OR'ed as it says in the docs #613

Open
pshear0 opened this issue May 2, 2012 · 9 comments
Open

:read ability not being logically OR'ed as it says in the docs #613

pshear0 opened this issue May 2, 2012 · 9 comments

Comments

@pshear0
Copy link

pshear0 commented May 2, 2012

I have a Message class which can belong to a Group, a Tournament or a User.
It can also be of type MsgScope = "System"

I want the user to be able to have :read access according to these restrictions logically OR'ed (as it says they should be in the docs):

  can :read, Message, :user_id => user.id
  can :read, Message, :group => { :id => user.group_ids }
  can :read, Message, :tournament => { :id => user.tournament_ids }
  can :read, Message, :msg_scope => { :name => "System" }

However, these lines all work individually when I comment-out the others but as a group they exclude eachother and you never see any messages on the INDEX page. What am I doing wrong?

@pshear0
Copy link
Author

pshear0 commented May 2, 2012

Update!!!

Actually I can use these two together and it works as expected:

can :read, Message, :user_id => user.id
can :read, Message, :msg_scope => { :name => "System" }

It is a problem when group_id and tournament_id are blank......?

@zenom
Copy link

zenom commented May 2, 2012

Just a thought as I don't know all the code, but if its a problem when group_id and tournament_id are blank then maybe you need some sort of if/else to check for that and alter the ability so they can or cannot do what you need it to do if those variables are blank. If they are blank, I don't see this as an issue with CanCan specifically. Just my thoughts. - A

@mikepack
Copy link
Collaborator

I've verified the issue, though I'm not sure if it's intentional.

The following fails:

user = User.create!
group = Group.create!
@ability.can :read, Message, :user => {:id => user.id}
@ability.can :read, Message, :group => {:id => group.id}
message = Message.create!(:user => user)
Message.accessible_by(@ability).should == [message]

I believe this fails due to the nature of the INNER JOIN generated by ActiveRecordAdapter#joins. #joins will generate [:user, :group] and the query will attempt to INNER JOIN on messages.user_id = users.id and messages.group_id = groups.id which will leave out messages records when group_id is nil. Potentially, an OUTER JOIN is necessary for this to behave correctly.

Using IDs explicitly will get the spec to pass because it references message#group_id, circumventing the JOINS:

user = User.create!
group = Group.create!
@ability.can :read, Message, :user_id => user.id
@ability.can :read, Message, :group_id => group.id
message = Message.create!(:user => user)
Message.accessible_by(@ability).should == [message]

In this specific case, you can use a workaround which does not use associations via a nested conditionals hash:

can :read, Message, :user_id => user.id
can :read, Message, :group_id => user.group_ids
can :read, Message, :tournament_id => user.tournament_ids

@andhapp
Copy link
Collaborator

andhapp commented May 12, 2012

@pshear0: Hey, can you please try using the fix suggested by @mikepack.

@pshear0
Copy link
Author

pshear0 commented May 17, 2012

Hi Mikepack,

Thanks for the explanation, that makes more sense now. In the end I moved the tests into a block and created a method in User model to do all the checks I needed.

can :read, Message do |message|
user.is_recipient?(message)
end

However, I have just tested your suggestion and yes it works as originally intended. So now I have a choice to make...

Thanks!

@pshear0 pshear0 closed this as completed May 17, 2012
@mikepack mikepack reopened this May 20, 2012
@mikepack
Copy link
Collaborator

Reopening: issue is still a bug and needs resolution/documentation.

@jachenry
Copy link

I'm facing a similar issue with abilities below

can :manage, Activity, :user_id => user.id
can :read, Activity, :is_public => true
can :read, Activity, :invitations => {:user_id => user.id}

Calling accessible_by on the Activity results in an INNER_JOIN which eliminates activities with zero invitations

Activity.accessible_by(current_ability, :read)

ghost pushed a commit to varvet/cancan that referenced this issue Aug 22, 2012
There are two issues with the current way cancan handles associations:

1) Records are returned multiple times in some circumstances
2) Several defined abilities prevent some records to show up under certain circumstances

This commit includes tests for both cases. It fixes both problems by changing `joins` to `includes` for the AR adapters. This could have performance implications, since `includes` will also select all columns in the associated records. We tried various ways of achieving the same thing using Arel directly, but were unable to make this work due to lack of support for outer joins in Rails 3.1.

This closes issues ryanb#724, ryanb#566 and ryanb#613
@qnm
Copy link

qnm commented Mar 20, 2013

@ryanb @pshear0 this issue was closed by 02841bbc195cdb17c6a4f511da44e0040e80f5f7 - can you close this issue please?

bryanrite pushed a commit to bryanrite/cancan that referenced this issue Jan 27, 2014
There are two issues with the current way cancan handles associations:

1) Records are returned multiple times in some circumstances
2) Several defined abilities prevent some records to show up under certain circumstances

This commit includes tests for both cases. It fixes both problems by changing `joins` to `includes` for the AR adapters. This could have performance implications, since `includes` will also select all columns in the associated records. We tried various ways of achieving the same thing using Arel directly, but were unable to make this work due to lack of support for outer joins in Rails 3.1.

This closes issues ryanb#724, ryanb#566 and ryanb#613
@xhoy
Copy link

xhoy commented Apr 10, 2014

Dear submitter, Since cancan/raynB hasn't been active for more than 6 months and no body else then ryam himself has commit permissions the cancan project is on a stand still.
Since cancan has several issues including missing support for rails 4 cancan is moving forward to cancancan. More details on: #994

If your feel that your pull request or bug is still applicable (and hasn't been merged in to cancan) it would be really appreciated if you would resubmit it to cancancan (https://github.com/cancancommunity/cancancan)

We hope to see you on the other side!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants