Paper trail swallows warden errors

[richmolj]

Our Devise setup has some custom Warden strategies. One of them had an error every so often, which would cause users to get kicked out with no explanation. It turns out this:

def user_for_paper_trail
   current_user rescue nil
end

Will rescue any error that pops up in a warden strategy silently, so you never get notified there's a problem with your code. At the very least, please add extensive logging to this method.

[batter]

I'm unsure as to how you would propose that PaperTrail be responsible for logging this type of thing. Ithink, if you want logging, you should setup your method something like this:

def user_for_paper_trail
  current_user.tap { |user| logger.info("PaperTrail `user_for_paper_trail` invoked and set to #{user.name}") }
rescue => e
  logger.debug "PaperTrail `user_for_paper_trail` was invoked but `current_user` could not be returned: #{e.message}"
  nil
end

I don't believe PaperTrail should be responsible for logging data such as this because it makes it further dependent up on Rails, and we are actually going to try to decouple it from ActiveRecord at some point in the future. Also in your example, it's not PaperTrail that is swallowing the Warden errors, but the rescue statement that is swallowing it (silently).

[richmolj]

PaperTrail implements

current_user rescue nil

which is poor code, which is why I say PaperTrail swallows the Warden errors. I say it is poor because it has a large, unintended consequence that is damaging to applications and irrelevant to PaperTrail's purpose.

Yes, you can override this method. What I am saying is, every user of Paper trail should always override this method. Why? Because it is damaging to applications and irrelevant to PaperTrails's purpose.

Any time every user should always override a method, it's a good idea to just change the gem itself.

Finally, though the Readme does note that you can override this, it does not mention the Warden-related consequences of note doing so.

[batter]

I'm trying to follow what your'e saying here, but I would think that the warden error would be pretty clear outside of PaperTrail's functionality, assuming that current_user is not returning properly, wouldn't you know right away based on other parts of the code that depend on current_user being set?

[richmolj]

This is a good question.

The answer is that if paper trail's before_filter is the first thing to call current_user (not unlikely, since this filter is added on gem load, when the usual #authenticate_user! is in ApplicationController and thus loads after) it will be in charge of firing the warden strategies. From Devise:

def current_#{mapping}
  @current_#{mapping} ||= warden.authenticate(:scope => :#{mapping})
end

So, since PaperTrail is the one (inadvertently) actually firing my auth strategies, I never get to other parts of the code relying on current_user. What happens is I raise, paper trail traps it, Warden reads this as the user is nil after attempting auth and therefore logs me out. So to reiterate, paper trail is accidentally swallowing unrelated authentication errors (which may happen because a custom Warden strategy has an error), causing me a great deal of headache the past few weeks :(

What's the purpose of the rescue? Could this be refactored to:

current_user if defined?(current_user)

[richmolj]

You could also make these before filters some kind of controller macro, and just note they should fire after your auth filters fire in the Readme. I think the code above would incorrectly avoid setting your user if the order was incorrect.

[batter]

Sure we can change the default implementation to current_user if defined?(current_user), although, in most cases it will return nil regardless, but it probably makes just as much, if not more sense. I'll give it a go tomorrow.