Open nevinera opened 3 years ago
I'm a bit confused by the lack of response here, this is a major security problem.
Rails developers will not expect can :read, Thing, :holder
to grant full read-access to all instances of Thing, only to that attribute. In the example you give here, you've just given all Users access to read each other's records, clearly not what you were intending to accomplish.
I expect that nearly every application that has attempted to use attribute rules now has holes in their authorization logic allowing view access to things they didn't intend - worse, if they tried to grant write access to particular fields.
I think you're correct, the documentation is not good for this feature. It should probably include an example with more restrict permissions and point out that you have to combine the Permissions for the attribute and the Objects.
My first guess was that it works like the following because for me it dos not make sence to give the user access to an attribute without access to the object itself:
can :read, Thing, :holder
can :read, Thing, { holder: :foo }
But the rules need to be combined into one: can :read, Thing, :holder, { holder: :foo }
I think giving access to an attribute but not to the object is not possible or did you find a way?
It's not possible, as far as I can tell, but seems like the obvious assumption to make:
can :read, Thing, :foo
cannot :read, Thing
Seems like the right way to say "You cannot read Things in general, but you are allowed to see their foos". A more realistic example:
cannot(:view, User)
can(:view, User) { |u| user == u }
can(:view, User, :username) { |u| user.is_friends_with?(u) }
This would seem at a glance to let you see the usernames of the users you are friends with, but actually it also lets you read the records - if the existing UsersController does a can? :view, @user
, it will succeed if they're the current-user, but also if they are a friend of the current-user.
Try hooking up attribute-authorization into a graphql system, for example - we want to allow users in set X to be able to view certain details about users in set Y, but we've been using cancancan for a long time, so all of our controllers (like in every rails app I've ever worked on) are checking access at the model level. So when we add declarations that User X can see Attribute Y on Model Z, we've actually given them access to Model Z itself. This doesn't seem like an edge case to me - at the very least, the documentation about attribute authorization ought to have in big capital letters "WILL IMPLICITLY GRANT ACCESS TO THE MODEL ITSELF AS WELL"?
For me it seams that if you start using "Accessible attributes" you need to update all views, where the normal can? :view, @user
happens
Because the user can :view
a User from a friend. At least a bit of it so can? :view, @friends_user == false
would not be correct as well.
If you want to seperate the controller and the view checks you could do it like this:
can(:view, User) { |u| user == u }
can(:view_attributes, User) { |u| user == u }
can(:view_attributes, User, :username) { |u| user.is_friends_with?(u) }
Maybe you could update the documentation and add your case as a warning that someone has to be carful with this feature.
I think a change of cancan internals to filter out attribute rules for non attribute lookups would be a bigger change. But I might be wrong because I'm not familiar with the internals of cancan. I was just responding because I was interested in this feature a while ago but did not use it.
Maybe @coorasse can help here I think he wrote the feature and the documentation. So he should at least know what the intentional usecase is.
The main story of concern:
authorize! :read, @thing
calls in controller actions.We're covered now, but this seems like a trap that a large number of rails projects could fall into - most shops don't write comprehensive authorization tests on their integration layer, and testing an Ability constructed with a DSL like CanCan feels redundant, since the tests are pretty much exactly the same as the declarations. Truly thorough tests would have caught it (and eventually did in our case), but the potential for harm here is pretty high.
Steps to reproduce
Expected behavior
I'd expect the first line to print
true
and the second to printfalse
Actual behavior
Instead, both lines print
true
. (same if the cannot/can declarations swap positions)System configuration
Rails version: None required
Ruby version: 2.7.2p137
CanCanCan version 3.3.0
Context
We're using cancancan to manage access to objects and attributes in a graphql type-graph. There are some attributes on some types that should be accessible to all users in a certain subgroup, and we recently found that those users were actually able to access all of the data on the type in question, not just the specified attributes.