Possible regression with draper when updating from 1.12

[dgilperez]

Hi!

I'm using devise + cancancan along with draper gem, and until 1.12 the behavior was that calling can? on a decorated model used the decorated model for checking authorization instead of the decorator class. Maybe I can better explain myself with an example:

 # in my ability.rb
 can :read, User do |user|
   true
 end

 # in a view
 @user = User.last.decorate # instance of UserDecorator
 can? :show, @user 
 #=> true, since the ability for User was used

After updating to 1.13, this is no longer the case, and calling can? :show, @user on a decorated user will not use User ability at all.

Is this intended behavior or is it a regression? If this is intended, how can I get that bahaviour back, maybe with an extension or configuration in my decorators / ability.rb?

Thanks!

[Senjai]

@dgilperez Definitely not intended. Could you reproduce this using a SimpleDelegator from the ruby standard library? That should behave similarly to draper, if not it might be draper specific. It would be helpful if we had a regression for this with a delegator so this wouldn't happen in the future.

[Senjai]

I would expect this might be related to https://github.com/drapergem/draper/blob/master/lib/draper/decorator.rb#L191-L204, not being checked with the new performance rule indexing stuff.

@dgilperez can't look at this now, but will be able to after work. Any chance you could get a regression test up, or a sample github project where I can reproduce this?

As a workaround, you could can? :show, @user_decorator.object for the time being (If I remember draper correctly)

[dgilperez]

Yeah, let me play around and see if I can add a failing test to the suite.

[dgilperez]

BTW, yeah, that'd be the workaround, though that's really inconvenient in a large app (like mine :().

[dgilperez]

Test added (or so I think), please let me know.

[jaredbeck]

I believe I've run into the same issue, using draper 2.1.0, cancancan 1.13.1, rails 4.2.4.

@redacted #=> #<RedactedDecorator:0x00..a0 @object=#<Redacted:0x00..18>>
can?(:update, @redacted) #=> false
can?(:update, @redacted.object) #=> true

Downgrading to cancancan 1.12.0 fixes the issue.

[Senjai]

@jaredbeck @dgilperez I don't think this problem falls within CanCanCan's domain. The performance fix works with the ancestors of an object, which is a valid thing to check when asserting that something is something else.

If something like Draper is merely pretending to be something else, but doesn't satisfy this requirement completely, it isn't fair to add additional logic at the cost of performance to ensure we cover that edge case.

For example, after releasing 1.13, we thought after seeing this regression we'd have a problem with rspecs' mock_model method. E.g order = mock_model(Spree::Order, number: 'A Number'), we can still use cancancan with this method. This is because mock_model defines the .class method, so cancan correctly checks against the Spree::Order rules. This can be done on Drapers' side if this an importance concern for them.

What I can do is possibly look at adding an extension point for custom rule checking logic in cancancan. That way a plugin can be developed specifically for use with draper, something like cancancan_draper, which would require more work, but would prevent adding things into cancancan to cover an edge case for a single gem.

Does this sound fair to you? Am I perhaps leaving something out?

[jaredbeck]

If you're dropping support for draper, you're going to break a lot of apps. Draper has two million downloads (https://rubygems.org/gems/draper). So, if you're dropping support for draper, maybe add a boot-time warning?

if defined?(::Draper)
  warn "cancancan 1.13 dropped support for Draper. " +
    "Checking abilities against decorated objects will no longer work. " + 
    "Please make sure to check against un-decorated objects."
end

Alternatively, a runtime warning should also be possible.

[Senjai]

@jaredbeck We never explicitly supported draper in the first place. The only thing I can find in the Wiki is a reference to https://github.com/anlek/draper-cancan. I took over the project a couple months ago, did cancan explicitly support draper before it was cancancan?

I may be wrong, if you can identify where it was stated there was explicit support for Draper I'll definitely eat my words. Again, this is still in discussion and I want to hear other peoples' opinion on the matter as well.

Do you have any comments on the extension point I made earlier?

[jaredbeck]

We never explicitly supported draper in the first place.

That's fine. My question is about what you want your user's experience to be like when they upgrade to cancancan 1.13. I'm just suggesting that you give them a happier experience by providing a warning.

Do you have any comments on the extension point I made earlier?

I don't know enough about draper to comment on that. You should probably ask Jeff (@jcasimir).

[McRip]

I got the same problem after upgrading to 1.13. I would really like to see support for draper in cancancan. It currently breaks a lot of things within our application.

[dgilperez]

@Senjai while I understand your thinking - supporting draper not being part of cancan domain - and agree with everything you state, I also think that CanCan + Draper for sure will be a pretty common combination to be found in applications out there.

Thus, I think that a solution for that should be provided, regardless if it comes from an cancan_draper gem or from cancan itself. Unfortunately, I don't really have the time right now to do it myself :(

At the very least, I'd add a visible notice in the Changelog and upgrading instructions, maybe even in the bundle install output, and I'd probably consider this change introduced by 1.13 as a breaking change. It'll save some headaches.

[Senjai]

@dgilperez I'll look into tackling this over the weekend and see if there's an amenable solution.

[Skulli]

Stumbled over the same problem when upgrading to 1.13. Some warning (while bundle install) would be enough for now.

@dgilperez a workaround would be to use some wrapper method or monkey-patch like

def can_do?(action, object)
  if object.decorated?
     object = object.model
  end
  can? action, object
end

Then just replace every usage of can? with can_do?

[dgilperez]

@Skulli yeah, that seems like a feasible workaround, thanks! ... but I think I'll wait for a gem or patch, since changing can? with can_do? would be kind of a big hit in my (large) codebase.

[jaredbeck]

I'll look into tackling this over the weekend and see if there's an amenable solution.

Any news, Richard?

I think I'll wait for a gem or patch [instead of workaround]

David, my team has also decided not to upgrade to 1.13 yet.

[dgynn]

@dgilperez Similar to the suggestion from @Skulli, could you add a single rule like this?

can do |action, object_class, object|
  if object_class < Draper::Decorator
    can? action, object.model
  end
end

[dgilperez]

Pretty clever workaround, @dgynn. It made my test suite fail though, but much less than a plain upgrade ... so it looks very promising. Not much time to look deeper into it until next week, I'll let you know.

[robmathews]

And here I am in the same place .. which class exactly are we monkey-patching?

[Senjai]

@jaredbeck Apologies, I was not able to get to it this past weekend. However, I'm on Vacation after tomorrow!

I'd like to propose two separate solutions here and get feedback on them both. First I want to state that:

This doesn't work because, while draper overrides kind_of?, the ancestors of a Draper::Decorator do not include the ancestors of MyDecoratedObject. We use ancestors to help with lookup performance, after that change was added, only the filtered objects would receive kind_of? calls. The fact being, a Draper::Decorator is not a kind_of? or is_a? of its decorated object, and this is why I'm hesitant to add specific code to address this.

Any methods added onto Draper::Decorator will actually interfere with any other methods you define on your own class. For example, object (which is a terrible name nonetheless) would be intercepted by Draper, you could not define this method on the class you were decorating.

That said, I understand the concern that people may want to use delgators (whether it be SimpleDelegator or Draper) in place of the objects they decorate and have everything magically work.

First suggestion:

Have decorators that wish to work with cancan implement a common interface. A method could be defined on any class wanting to operate this way with cancan, say cancan_delegated_class.

If the subject responds to this method, the result of that method would be used in place of the decorator. This would require a project like Draper to be willing to add this method, or someone to develop a plugin that extends Draper::Decorator, or do it in their own code base.

Second suggestion:

We add a configuration class to cancan. We would use some sort of SubjectResolution class to determine how exactly we resolve the the subject class. This could then be extended by something like:

CanCanCan.configure do |c|
  c.subject_resolution_class = DraperSubjectResolution
end

Which would allow for a composition based approach and would allow for a plugin, or functionality in your own application to configure what class is being used to resolve the subject.

All of these names are pending, and are just for example.

I personally prefer option #1, as it's the least invasive, and I believe that cancancan operates correctly. The issue lies with decorators that want to be 100% substitutes for their decorated class failing to cover all the basis required to do so.

With option 2, there are several other extension points within the system that a configuration class would be handy for. I think that adding a configuration will eventually be necessary at some point anyway.

[Senjai]

@robmathews Also makes an excellent point in #257

You should have an @Article, and a @decorated_article in your controllers. You should be using the real object when using it for real things. (e.g. not view decoration)

[dgynn]

@Senjai Those two approaches seem like they would work. The first approach though would require all Draper users (or the Draper gem) to make a change and then CanCanCan needs to constantly check for subject.respond_to?(:cancan_delegated_class). The second approach of having a Draper (or other) adapter is good but I don't think that should be a global configuration.

What if Ability had a resolve_subject_class method. And a Draper user could include a DraperAdapter in their ability class that would override it. Something like...

class Ability
  include CanCan::Ability
  include CanCan::DraperSubjectResolution
  # rules...
end

This basically does what you have in the second suggestion but makes the configuration ability-specific. Also the DraperSubjectResolution could more easily call super for non-Decorator objects and get the default CanCan::Ability logic.

And now that I look at it, we already have a alternative_subjects method so we don't need a new resolve_subject_class method. Maybe it could work something like this (I haven't tried this at all)...

module CanCan
  module DraperSubjectResolution
    def alternate_subjects(subject)
      if subject.class < Draper::Decorator
        super(subject.object_class)  # draper method for getting true class I believe
      else
        super(subject)
      end
    end
  end
end

[Senjai]

@dgynn Yes, writing a module and prepending it to the ability class to override can? and cannot? would be my goto.

We dont need to override the alternate subjects method. We need to override can? and cannot?.

def can?
  if thing.instance_of?(Draper::Decorator)
    super(thing.object, rest_of_args)
  else
    super
  end
end

[robmathews]

Yes, I'll summarize:

Draper documention itself recommends that the decorator objects only be decorated in the views, not the controllers, and provides a helper method decorates_assigned for this purpose. They are very explicit about this, and really they are in agreement with @Senjai. Doesn't sound like they would be sympathetic to a PR that changed the "kind_of?" implementation.

However, that doesn't really help all of us poor souls who are maintaining large applications that reference @object everywhere. We still want it to just work, and we really don't want to wander through all of our views changing @object => object.

One change you can do I've noticed is not decorate your objects on load, but decorate them on way out of the controller action.

Another possibility would be examine the performance problem that prompt this change in the first place. I've never had a performance issue with CanCan checking abilities, but maybe that's just me, and I don't really all of the changes anyway.

[Senjai]

@robmathews Most apps dont have many rules. Some apps have thousands of rules. The performance change took lookup from O(n) to O(1) which is -major-. See #178. Also the last two commens by @dgynn and I.

[robmathews]

I'll give that a shot, and if it works, maybe create a gem called
'draper-cancancan' or something that mixes it in automatically.

On Thu, Nov 5, 2015 at 1:24 PM, Richard Wilson notifications@github.com
wrote:

@dgynn https://github.com/dgynn Yes, writing a module and prepending it
to the ability class to override can? and cannot? would be my goto.

We dont need to override the alternate subjects method. We need to
override can? and cannot?.

def can?
if thing.instance_of?(Draper::Decorator)
subject = thing.object
else
super
endend

—
Reply to this email directly or view it on GitHub
#255 (comment)
.

[robmathews]

@Senjai That PR convinces me, thanks.

[Senjai]

@robmathews If you do come up with something that works for you as a gem, I'd be happy to fork it from you as part of the organization as well.

[Fire-Dragon-DoL]

May I suggest to perform a Major Version change to better follow SemVer? "Dropping" Draper, even if it was a bug or whatever, is going to break a lot of apps, SemVer itself suggests that if a bug that a lot of apps relies on is fixed, it should be marked as a major change.

Considering draper has a lot of downloads and activeadmin too. And cancancan is used in activeadmin, this is definitely a major version change.

(I just faced this bug)

[kreintjes]

Works like a charm @robmathews. Thanks!

[dgilperez]

I think this issue can be closed @Senjai

At least for me, it was beautifuly solved with @robmathews's draper-cancancan.

Thanks a bunch and 🍻 for you!

[Senjai]

@dgilperez 🍻