inherited_resources and collections

[jtushman]

Hi,

I think I found an issue with IR and cancan. consider the following code:

class AccountsController < InheritedResources::Base
  load_and_authorize_resource

  def collection
    @accounts ||= end_of_association_chain.order_by(:name.asc).paginate(:page => params[:page],:per_page =>10)
  end

end

load_resource does not seem to call the collection method to load the resource. I think it should.

[ryanb]

Good point. I'll mark this to be added.

[jtushman]

Thanks Ryan!

[aq1018]

I recently found this problem as well. Basically, CanCan::InheritedResource#resource_base is calling #end_of_association_chain from IR. Instad, it should be calling #collection.

I monkey patched CanCan with the following:

module CanCan
  class InheritedResource
    def resource_base
      @controller.send :collection
    end
  end
end

[tanordheim]

I added a pull request for this fix now (including a spec) based on aq1018s suggestion. Hope that helps.

[ryanb]

Thanks tanordheim, I'll close this and get that pulled in soon. For the record it's issue #297.

[nestegg]

I don't believe that this is correct. I am using collection and it was being called correctly through inherited_resources. This change broke my app. While collection is still getting called I now always get the full collection rather than the collection scoped by cancan ability.

[tanordheim]

Hm, can you post some code as a gist or something?

I'm running an app here now with cancan (from ryanb's master) and InheritedResources using custom collection methods - and that works just fine.

[ryanb]

I'll open this issue again. Let us know what you figure out @nestegg. I don't use Inherited Resources so I'm leaving it up to other to contribute to this.

[ryanb]

Looks like others have had issues with this too. There is a pull request to revert the change, see issue #309. I'm leaving this issue up for discussion.

[ryanb]

The change is now reverted in the 1.6.1 release. If you submit a pull request regarding Inherited Resources, post here to get others to try it out and provide feedback.

[aq1018]

Please ignore this comment. It was wrong. See my next comment please.

Hmmm... This is actually quite a complicated issue.

For index action:

CanCan::ControllerResource#collection_instance= method actually sets the collection instance variable. Say if you have PostsController, calling #load_resource actually sets @posts as whatever is returned by CanCan::InheritedResource#resource_base, which loads #end_of_association_chain (lazy loaded), but then #accessible_by actually loads the entire association chain without scope or pagination from db into memory.

However, in inherited_resources, @posts is actually set in #collection method, not #end_of_association_chain. Also #collection first checks to see if @posts is already set, and load it if it's not.

Now, if we override #collection in PostsController, like this:

def collection
  @posts ||= end_of_association_chain.paginate(...)
end

Since @posts has already been loaded by load_resource, end_of_association_chain will never be called again. If we take out "||", we are essentially hitting the db twice. Another issue is that when the first time @posts is loaded by CanCan, it loads EVERYTHING, which will cause performance issues.

The solution:

when loading for index, or other collection based actions, #load_resource should use #collection

When loading for show, or other member based actions, #load_resource should use #end_of_association_chain. This is because it needs an association for #build_resource, #find_resource, etc...

I don't know if this made any sense. But I tried. : )

[aq1018]

Ok, I was COMPLETELY wrong! Please ignore what I said earlier.

After tracing the code more carefully, here is what I found:

1). To properly authorize a collection, CanCan::InheritedResource#resource_base MUST return a scope to be used by #accessible_by. See controller_resource.rb#89 ( load_resource method ).

2). #accessible_by returns a scope

If we use #end_of_association_chain in #resource_base, then CanCan assigns @posts for PostsController before #collection gets called, and #collection sees @posts is already there and won't evaluate any further.

If we use #collection in #resource_base, but then we cannot guarantee the return value is a scope due to pagination.

Either way, there is no ideal solution.

If we use #end_of_association_chain, we have to make sure we have to overwrite @posts in collection or else we won't have pagination.

If we use #collection, we have to make sure the value returned by #collection must be a scope or association. ( So pagination will have to be moved into the action? Kinda defeats the purpose of inherited_resource. )

Either way, I can't find a good solution...

Any comments?

AQ

[atwam]

If we use #collection, we have to make sure the value returned by #collection must be a scope or association. ( So pagination will have to be moved into the action? Kinda defeats the purpose of inherited_resource. )

I'm not really sure about that. Using kaminari and not will_paginate, the result from my pagination is a scope, but I still have this bug :

Having simply the following method in my controller :
def collection
@users ||= end_of_association_chain.page(params[:page])
end
I still accidentally the whole collection instead of the permitted one.

However, following your explanation, it looks like simplifying the #collection function to the following may solve the problem (but breaks my test since I'll have to revise the User.stub_chain(:page) I previously had.)

def collection
  end_of_association_chain.page(params[:page])
end

Which mean that we should probably have our load_resource assign the @posts instance variable, rather than have #collection test and assign it.
Any thoughts ?

[aq1018]

If:

1). let #load_resource instead of #collection to assign @posts
2). use #collection inside #resouce_base

There are several drawbacks:

1). we are imposing #collection to not assigning @posts.
2). We are imposing #collection to return scope only. Because CanCan will try to use #accessible_by on the scope. If the it is not a scope, i.e. array, it will skip #accessible_by call, and not loading the colleciton.

I have a different idea:

override #end_of_association_chain to call #accessible_by at the end. This makes more sense to me because I consider accessible_by apply extra scope for authorization purposes, which should be consider really the 'end' of the association chain, ensuring #find to be scoped correctly.

[aq1018]

Here is a monkey patch (again, sorry, it's 3:30 am) based on my idea. It's super ugly though...

module CanCan
  module InheritedResourceMethodsOverride
    def end_of_association_chain
      chain = super
      if chain.respond_to?(:accessible_by)
        chain = chain.accessible_by(current_ability, @cancan_controller_resource.send(:authorization_action))
      end
      chain
    end
  end

  class InheritedResource
    def self.add_before_filter(controller_class, method, *args)
      options = args.extract_options!
      resource_name = args.first
      before_filter_method = options.delete(:prepend) ? :prepend_before_filter : :before_filter
      controller_class.send(before_filter_method, options.slice(:only, :except)) do |controller|
        controller.instance_variable_set(
          "@cancan_controller_resource",
          controller.class.cancan_resource_class.new(controller, resource_name, options.except(:only, :except))
        )

        controller_class.instance_eval do
          # directly overriding #end_of_association_chain didn't work
          # because it is included from another module.
          # However, you can override it with another module.
          include InheritedResourceMethodsOverride
        end

        controller.instance_variable_get("@cancan_controller_resource").send(method)
      end
    end

    def load_collection
      resource_base
    end

    def load_collection?
      !current_ability.has_block?(authorization_action, resource_class)
    end

    def resource_base
      @controller.send :end_of_association_chain
    end
  end
end

[amw]

@aq1018

end_of_association_chain is widely used in inherited_resources, not just for collection. You don't want accessible_by called when loading resource instance.

Also block ability rules don't support accessible_by, and your code will fail for abilities that use them. See load_collection?.

[nestegg]

Sorry to have missed out on most of the discussion. I had notifications turned off from the "gem no-doc +1" fiasco and didn't realize it.

In the default case where you have an InheritedResources controller with load_and_authorize_resource and nothing more, cancan's resource_base must return end_of_association_chain because you need to (potentially) scope it farther.

If you're overriding collection, you're taking responsibility for setting the instance variable, so you don't want to use load_resource in a before filter. I'm not sure that it makes sense for cancan to insert itself in this case. If you're going to bypass load_resource you may as well handle the authorize part in your collection definition.

[aq1018]

@nestegg

If you're going to bypass load_resource you may as well handle the authorize part in your collection definition.

I almost wanted to do so, because how CanCan::InheritedResource inherits from CanCan::ControllerResource, and I feel CanCan and InheritedResources are both trying to do the same thing, and trying really hard not to step on each other's toe. It's almost getting out of control.

A (potentially bad) suggestion:

In order to keep CanCan simple, we split the gem into 3 pieces:

• cancan (Core, authorization and model adapters)
• cancan-action_controller ( Integration with ActionController, basically the CanCan::ControllerResource part)
• cancan-inherited_resource (Integration with InhreitedResources, maybe doing something entirely different from CanCan::ControllerResource since loading can be handled mostly by InhreitedResources already)

What do you say?

[amw]

@aq1018

I don't see a reason for gem splitting.

@nestegg

You're right in that by redefining collection you take some responsibility and that you probably should handle authorization yourself. I don't see a reliable way of handling this in cancan. After all developer isn't even required to use end_of_association_end in his redefined method.

Using load_resource or load_and_authorize_resource still makes sense for those actions that use resource instance. So I'd say developer should :except collection actions and use accessible_by in his collection redefinition if he wants cancan's authorization.

The current implementation is safe, meaning that if the developer doesn't :except the collection actions cancan will load resources with accessible_by and the developer will quickly realize that whatever he used in his collection isn't applied - be it pagination or whatever.

An alternative would be to move cancan's collection loading into cancan's own redefinition of collection. Internally it should check skip? and load_collection? then use accessible_by or super. Then if user redefined collection in his controller, he would instantly see the results he wanted without having to add :except options to load_resource.

[ryanb]

@aq1018, as amw mentioned, I don't see any reason for splitting into separate gems. We can dynamically check for the existence of Inherited Resources and load whatever's necessary in the same gem.

In the worst case scenario we'd have CanCan::InheritedResource not inherit from CanCan::ControllerResource. Since InheritedResources does most of the loading anyway, that may be the best scenario.

@amw, feel free to experiment with your idea in a fork and I'll take a look. I'm fine with moving things around in ControllerResource to make it easy to override the proper things through inheritance.

However if it will require a deep restructuring and make things very abstract I would prefer to first start with making InheritedResource duplicate logic from ControllerResource instead of subclassing, and then we can look into abstracting out the similarities.

It may be worth checking out how I integrated CanCan into rails_admin. Look for @authorization_adapter mentions in the MainController.

This is a good example of integrating authorization into an engine. Even though InheritedResources isn't an engine it is a similar problem because it manages the loading of resources. There I didn't use ControllerResource at all and the code is fairly simple.

[amw]

I could try this out some day, but it'll have to wait. Real busy with work stuff at the moment. I think the current state of cancan's support is quite ok. It works perfectly with single resource actions. It also does it's job for collection actions when not developer doesn't overwrite collection to add extra features. This issue only surfaces when when redefining collection and even then it's quite easy to overcome telling cancan to skip actions that use it.

And just to make this clear - even if we do add our authorization to the default collection implementation user will have to repeat it when reimplementing it anyway.

[aq1018]

@amw

I really like the idea of separating CanCan::InheritedResource from CanCan::ControllerResource. I don't feel there is a need to override the default collection method from cancan as you pointed out the reason already.

I'll try to play around this idea in my own fork as well.

@Rynab

I think this issue was mainly caused by people didn't know how to properly override the collection method. Adding some code examples on how to override it properly in the documentation would probably be sufficient.

[ryanb]

@aq1018, good idea. Could you or others edit the wiki page and provide some documentation on the proper way to do this? Include any other gotchas one may come across as well. Thanks.

[hazah]

If I may add my 2 cents...

This is a classic problem revolving around the concept of "separation of concerns". I have been trying to wrestle down this particular problem space for a while and have determined that fundamentally, we are looking at two separate problems lumped into one. The first being that of resource loading. In essence, this particular aspect deserves to stand on its own (in a way I am supporting splitting the gem up). The second is that of resource authorization. This is especially evident in the fact that there are two distinct methods to perform each action.

The major complication, as I see it has to do with the fact that both CanCan and InheritedResources are supplying overlapping functionality, and that alone forces a user into making a choice of one or the other (an unnecessary consequence imho). I feel that in order to make sense of the responsibility of which lib loads an object, that it must be up to the configuration to select the appropriate route, with a reasonable default selected (which CanCan already provides).

In addition to that, I feel that either approach MUST provide the necessary hooks into authorization processes (much like the current cancan method 'load_resource' isn't forcing an authorization to take place). That said, InheritedResources, while providing such hooks, leaves a lot to be desired when it comes to integrating it with an authorization system.

Though I am not offering any functional solution, I am stating that, ultimately, the loading of a resource, belongs into its own package, in practically every way I am able to slice it.

I don't think it should be up to CanCan to figure this out. CanCan's responsibility for loading an object should end at the exact moment that the user specifies that s/he'll take care of it on their own.

[gvt]

Is this still an issue? I think I may be bumping into this as well. I see there's quite a bit of history here. I am observing that when I override the collection method in my inherited resources controller it does not get called.

[nestegg]

@gvt, no it's been working for me.

[andhapp]

Based on the confirmation by @nestegg and no recent updates, I am going to close this. However, it will be re-opened if someone reports this with their findings.

[vitaly]

What about overwriting 'apply_scopes' with cancan so that you can apply accessible_by(current_ability) on the passed scope. then IR's collection will just work including all the overwrites, pagination and all.

[pironim]

it still not working properly. I'm using cancan (1.6.8) and inherited_resources (1.3.1). Recipe from here https://github.com/ryanb/cancan/wiki/Inherited-Resources help. But like for me there should be at least some warning message.