Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Move `accessible_by` to Ability #235

Closed
ryanb opened this Issue Jan 10, 2011 · 6 comments

Comments

Projects
None yet
2 participants
Owner

ryanb commented Jan 10, 2011

Currently fetching accessible records requires you to call accessible_by directly on the model.

Project.accessible_by(current_ability)

This means CanCan must add this method to every model which supports this (ActiveRecord, Mongoid, etc.). I don't like injecting behavior like this, and since the current_ability is always required, why not use that.

@projects = current_ability.records(Project)

This will also clean up the internals. If you have any suggestions for an alternative name for the method please post a comment.

Owner

ryanb commented Jan 11, 2011

Perhaps the name query would be better.

@projects = current_ability.query(Project)

Agree, query is better. On a side, if accessible_by were to be kept, then passing current_user instead of current_ability would feel more natural.

Also, I guess this need to work with other scopes, e.g. @projects = current_ability.query(Project.my_scope) [which it probably does out of the box]

The downside with this is that using the, already familiar, concept of scopes might be more intuitive. Adding a scope to all models isn't that bad. And with current_ability always being needed, I think scopes can take optional variables so maybe passing it only when explicitly needed (unless not passing it through a controller cause other problems)

Owner

ryanb commented Feb 2, 2011

On a side, if accessible_by were to be kept, then passing current_user instead of current_ability would feel more natural.

I agree the wording would feel more natural, but it would break the elegance of how CanCan works. An ability should only be instantiated in one location (the current_ability method) so it can be easily overridden.

Also, I guess this need to work with other scopes, e.g. @projects = current_ability.query(Project.my_scope) [which it probably does out of the box]

That is the only thing I am unsure about with this approach. I will need to do some testing.

Owner

ryanb commented Mar 25, 2011

I did some experimenting and found out this will not work for scopes/associations in ActiveRecord. For example if I try this.

@tasks = current_ability.query(project.tasks)

It is difficult to get the Task class out of this which is necessary to determine the ability rules. The magic behind having a class method (accessible_by) there is that ActiveRecord handles all the scoping craziness. It automatically passes the Task class to Ability yet still persists the association scope.

Internally Active Record does this by maintaing an array of scope_methods on the current thread. When calling class methods through a scope it internally pushes the current scope to this array and pops it when it's done. This means any find call done in a scoped class method (like accessible_by) will adopt that scope.

This is all too complex to manage in CanCan so we're going to leave it at its current behavior and have Active Record do the heavy lifting here. I don't like having to add a class method but don't know any other way.

@ryanb ryanb closed this Mar 25, 2011

I don't think adding a class method is that much of a problem. Though maybe it should be included instead of added by default. For example:

class MyModel
    include CanCan::ModelSupport
end
Owner

ryanb commented Apr 1, 2011

While I generally agree with enabling functionality by including a module manually, here there's a disconnect because the functionality is triggered indirectly through load_and_authorize_resource. It's not something which is being used directly in the same class as the include statement. Since it's only one simple method with a fairly unique name, I think including it by default is ok.

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