Support for decent exposure #317

Open
julian7 opened this Issue Mar 22, 2011 · 25 comments

Comments

Projects
None yet
10 participants
Owner

ryanb commented Mar 22, 2011

I haven't given Decent Exposure a good look yet, but when I do I'll try to come up with some ideas on how best to do this.

In the meantime, anyone have suggestions?

Owner

ryanb commented Mar 28, 2011

I'm planning to get this in to CanCan 2.0, not sure how difficult this will be yet or how best to go about it. If you have any suggestions please let me know.

voxdolo commented Mar 28, 2011

Ryan, we've used cancan and decent_exposure on a project at Hashrocket and it worked out of the box.

That said, I recently had a request to support cancan from with decent_exposure and my reply was that it should just work... after the requestor tried it, they confirmed it. If there is anything I can do to provide additional help or interoperation, I'm happy to... just let me know!

Owner

ryanb commented Mar 28, 2011

@voxdolo, true, for the most part decent_exposure will work fine with CanCan and there won't be any obvious incompatibilities. But right now if you do load_and_authorize_resource it does its own loading behavior and creates instance variables which goes against decent_exposure's way of handling resources.

If you want to keep the loading behavior in decent_exposure and just use authorize_resource in CanCan it will still attempt to look for instance variables.

Maybe all we need to do here is check for a method name if an instance variable doesn't exist and use that to handle authorization on a resource. Things get a little trickier when handling initial attributes and authorizing before/after states in the update action, but at least it's a start.

voxdolo commented Mar 28, 2011

@ryanb okay, I see. Off hand, it seems like the easiest interoperation might be for decent_exposure to assign those instance variables as it creates the methods. It'd be easy enough to modify the default_exposure to do such. Making the actual state unavailable directly like that was definitely a conscious design decision though (right now everything is stored in a hash inside @_resources), in hopes of discouraging people from attempting to access the ivars directly.

That said, maybe decent_exposure could provide an interface for getting at the entirety of it's internal content... like a #resources method that returns the @_resources hash. Given that, it'd be straightforward to expose the state as instance variables (the hash is keyed by the symbols you give the expose method).

All of this talk about ivars is making me want a shower ;) Kidding aside though, I'm keen to help if I can.

voxdolo commented Mar 28, 2011

a quick hack that might be a stop-gap is to just do the ivar assignment in a custom default_exposure. here's an example (untested):

https://gist.github.com/891504

The only real change is on line 9, adding the instance_variable_set call. Theoretically, cancan users should be able to drop that in application_controller.rb and manage state in decent_exposure, while handling authorization in cancan.

Owner

ryanb commented Mar 29, 2011

Setting ivars in decent_exposure won't solve the issue here because the method still won't be called to set the ivar initially (unless something happens before the authorization before filter).

Changing CanCan to look for a method I think will be best. It adds flexibility even outside of decent_exposure. I wouldn't change decent_exposure here, unless you have other needs. Thanks for the ideas though.

ccahoon commented Apr 6, 2011

Out of curiosity, is this change still in the plans?

Owner

ryanb commented Apr 6, 2011

Yes, I haven't had time to work on CanCan recently, but I am planning to do this.

We have decided to use CanCan in our present project and I love to see that best compatibility with decent_exposure is already under way.
Besides, I agree with ryanb here. Voxdolo should keep his plugin clean and slick. If he starts overloading it with unneeded features it will become slower and lose its meaning (I hope not to sound too hard here).

Owner

ryanb commented Jun 13, 2011

I added some of this functionality in commit b8ff2db. I'm not yet certain how to handle collection methods because I would need to override the collection method to add accessible_by scope to it. I would like to avoid that. Any ideas?

julian7 commented Jun 16, 2011

Two days, no chance. Maybe today :)

julian7 commented Jun 16, 2011

My first try worked:

class ProjectsController < ApplicationController
  load_and_authorize_resource
  expose(:projects) { Project.accessible_by(current_ability) }
end

Then app/views/projects/index.html.erb picks up projects nicely.

voxdolo commented Jun 16, 2011

I think julian is on the right path here. As of right now, decent_exposure does nothing to automatically provide collections, you must always provide a block. As such, it doesn't seem like any special concessions need be made by cancan.

julian7 commented Jun 17, 2011

Maybe load_and_authorize_resource should accept :decent => :exposure (just kidding, :expose => true would be better) to do it automatically. Then, ProjectsController would look like this:

class ProjectsController < ApplicationController
  load_and_authorize_resource :expose => true
end

Then, load_and_authorize_resource would do

expose(:project)
expose(:projects) { Project.accessible_by(current_ability) }

If this gets implemented as proposed, I would suggest that :expose accepts a block to allow for further refinement:

class ProjectsController < ApplicationController
  expose(:category)
  load_and_authorize_resource :expose => { category.projects.order(:title) }
end

fgro commented Sep 15, 2011

any further progress on this feature? cheers ;-)

Owner

ryanb commented Sep 28, 2011

My current plan is to get 2.0 out as quickly as possible, and I am primarily focusing on the ability/model layer features in the next release. In 2.1 I will be focusing on the controller layer features and plan to address this then.

hoenth commented Sep 13, 2012

Though I haven't tested it thoroughly, I am having some success with the following, placed in an initializer:

module CanCan
  class ControllerResource

    def resource_instance
      if load_instance?
        if use_decent_exposure?
          @controller.send(instance_name) 
        else
          @controller.instance_variable_get("@#{instance_name}")
        end
      end

    end

    def use_decent_exposure?
      @options[:decent_exposure] && @controller.respond_to?(instance_name)
    end

  end
end

Then in my controller:

authorize_resource :decent_exposure => true

So the load portion is skipped, and the instance is obtained from the decent_exposure method with the same name.

If the method has a different name than the name passed in, you can use the instance_name option.

Curious to get your thoughts, even though this discussion seems to have stalled.

Hoenth, your approach works for me, thanks.
I would be happy to see it in CanCan.
Maybe it could transparently check whether use instance variable or decent exposure.

ekampp commented Mar 25, 2013

@hoenth your extension will not allow to pass in stuff like attributes and such, to decent exposure, to allow it to function wth strong parameters?

hoenth commented Mar 25, 2013

@ekamp - My extension doesn't change how decent exposure methods are defined, it just checks whether there is a decent exposure method with the same name as the expected CanCan attribute. Unless I am mis-understanding your point.

ekampp commented Apr 3, 2013

@hoenth you are quite right. I had read some of the names wrong 😄

I have attempted to build on your extension, and this is what I have to far. The problem with that is, that it gives me a CanCan::InsufficientAuthorizationCheck error. @ryanb maybe you can shed some light on what I'm missing when trying to extend the collection?

module CanCan
  class ControllerResource

    def resource_instance
      if load_instance?
        if use_decent_exposure?
          @controller.send(instance_name)
        else
          @controller.instance_variable_get("@#{instance_name}")
        end
      end
    end

    def collection_instance
      if load_collection?
        if use_decent_exposure?
          @controller.send(instance_name.to_s.pluralize)
        else
          @controller.instance_variable_get("@#{instance_name.to_s.pluralize}")
        end
      end
    end

    def use_decent_exposure?
      @options[:decent_exposure] && (@controller.respond_to?(instance_name) ||
        @controller.respond_to?(instance_name.to_s.pluralize))
    end

  end
end

ekampp commented Apr 3, 2013

Never mind. I found out that the current 2.0 branch supports the singular type without the extension. So I just needed to extend the collection like so:

module CanCan
  class ControllerResource

    # Extends CanCan in such a way that it supports the way that decent
    # exposure sets collection variables.
    #
    def collection_instance
      collection_name = instance_name.to_s.pluralize
      if @controller.instance_variable_defined? "@#{collection_name}"
        @controller.instance_variable_get("@#{collection_name}")
      elsif @controller.respond_to?(collection_name, true)
        @controller.send(collection_name)
      end
    end
  end
end

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 join this conversation on GitHub. Already have an account? Sign in to comment