can :manage behavior #114

Closed
noahhendrix opened this Issue Aug 7, 2010 · 11 comments

Comments

Projects
None yet
2 participants

If I pass an attribute to can :manage, it causes the controller to throw an expection when I access the new action, presumably because the resource doesn't match the criteria. Is the normal, or should load_and_authorize_resource build the resource with the attributes pre-set (similar to scopes in rails 3).

Here is the code:
ability.rb
can :manage, Activity, :user_id => user.id

activity_controller.rb
load_and_authorize_resource
...
def new
end

development.log
You are not authorized to access this page.

Owner

ryanb commented Aug 10, 2010

The load_and_authorize_resource will do this in a before filter for the new action.

@activity = Activity.new
authorize! :new, @activity

Since an instance is passed in, the activity's user_id will be nil and therefore will not match the user.id. There may be a better solution to this, but in the meantime you can use the new :through feature in CanCan 1.3. This currently relies on an instance variable so you'll need to set the current user to that in a before filter.

before_filter :load_current_user
load_and_authorize_resource :through => :current_user

private

def load_current_user
  @current_user = current_user
end

This way all activities will be built/fetched through the @current_user.activites association. I could have the :through option check for a method name if an instance variable doesn't exist so you don't have to have a separate before_filter.

An alternative is to add another can call to allow an activity to be created without any user_id requirement.

can :manage, Activity, :user_id => user.id
can :create, Activity

The second option is what I am doing now, I just wonder if since CanCan has all the attributes that are req'd could it not internally call build setting up those attributes as quasi-defaults?

Owner

ryanb commented Aug 10, 2010

Are you wanting it to do @activity.user_id = current_user automatically in the before filter? The problem is this would make the user_id attribute magical, and I don't want CanCan assigning attributes without specifically being told. There may also be times one wants a user_id attribute to not automatically be set to current_user.

However, I agree it is a common pattern and should be made easier. It would be nice if it was as simple as this.

load_and_authorize_resource :through => :current_user

But I will need to think about how best to do that since it's not an instance variable. This would make all find/build calls go through the current_user for every action which I think is common behavior. Is that what you are wanting to do here?

What I was thinking was something like this:

can :manage, Activity, :user_id => user.id, :activity_type => "bacon"....

Then CanCan might try to intelligently build resources like this...

@activity = Activity.build(:user_id => ??, :activity_type => ??)... etc.

Couldn't you fill in ?? with values from the ability definitions? This is of course coming from someone who hasn't dug in to the source of CanCan so I could be completely off base on what is "possible".

I imagine, on the front-end, it working similar to scopes

scope :bacon_activities, where("activity_type = ?", "bacon")
Activity.bacon_activities.build

Owner

ryanb commented Aug 10, 2010

Currently the can definitions are intended to be used only for checking permissions and not building through them. However what you are asking here should be technically possible.

If it's not the default it could be an option. I'll have to think about this a bit more and consider various scenarios. In the meantime I'd like to get more input, are there others who would find this feature useful? Please post here.

Owner

ryanb commented Aug 10, 2010

After thinking about this for a bit, it is making more sense to me. Currently if you have a hash of conditions on the new action it makes the load_and_authorize_resource almost useless since those conditions will never be met on a new, blank item.

It also makes sense that if there are requirements on an item, it should attempt to meet those requirements when building the item. I'll play around with this idea more in a branch and hope to have something in version 1.4.

The implementation will get pretty complex if there are multiple can definitions on the same item, or if there are cannot conditions. But even if it just works for the simple cases I think that would be okay.

Haha glad to see the idea has captured you. I'd love to help test this, so just let me know when/if you need help.

The basics are great, CanCan is great because you keep to the basics, if developers have unique constraints they can handle that by building the resource on their own in the new action., right? Maybe even a :simple_build => true flag on load_and_authorize_resource which doesn't attempt to build with req'd attributes.

Owner

ryanb commented Aug 12, 2010

Yeah, I think an option to fall back to the old behavior is necessary for backwards compatibility, but I look forward to experimenting with it.

Owner

ryanb commented Sep 3, 2010

the new and create actions will now build the resource with attributes based on ability conditions hash - closed by a744377

Thanks!

This issue was closed.

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