Timeoutable timeouts causing Rails to flash[:timedout] as "true"

[nbibler]

I know this has been mentioned in other tickets, but it appears to still be an issue, as of 2.0.4. From the current, master's failure_app.rb:

def redirect_url
  if warden_message == :timeout
    flash[:timedout] = true
    attempted_path || scope_path
  else
    scope_path
  end
end

The third line there is causing the Rails Flash::FlashHash object instance to carry a flash[:timedout] whose value is of TrueClass. For generalized flash implementations, such as the one generated and recommended by this gem:

<%- flash.each do |name, msg| -%>
  <%= content_tag :div, msg, :id => "flash_#{name}" %>
<%- end -%>

The following HTML is being generated and displayed to visitors:

<div id="flash_timedout">true</div>

Obviously, this is non-ideal. :) Is this :timedout flash message actually used for anything? I see that the failure_app is explicitly keeping the :timedout flash message, even though it's simply a true statement:

def redirect
  store_location!
  if flash[:timedout] && flash[:alert]
    flash.keep(:timedout)
    flash.keep(:alert)
  else
    flash[:alert] = i18n_message
  end
  redirect_to redirect_url
end

Based on a cursory reading of this code, it seems to me that a simple instance variable, which is set to true/false and nilified with each Rack call request, would provide the same functionality and work significantly better.

And, yes, I did see @josevalim's comment on the use of Flash... I just wonder if either the README and example application layout should be updated to reflect the check for String-iness or the surprising usage of Flash for non-user messages should be removed.

[josevalim]

Yes, (unfortunately?) we are using such values. The problem with using instance variables in the Rack stack is that they are not threadsafe (the stack is shared between all threads). So I think our best option is to indeed improve the docs and let people know that Devise uses flash to keep other state besides flash messages.

[nbibler]

That certainly makes sense. Another alternative, perhaps, is to use the Rack env to store a devise.timedout value, and look to that. The env is threadsafe and less surprising than Rails's flash.

[josevalim]

Yeah, I tried it out with the env after I replied to you and I realized it won't work because redirects are involved. So we really need to store it in the session. :S

[Burgestrand]

Given that it’s quite common to print flash messages just as-is, at the very least this unexpected behavior should be mentioned somewhere. Got bitten by this issue today as well, but I could not find any mention of it in the README even though this issue has been closed.

I understand it’s convenient to use flash to have the message expire automatically from the session, but sprinkling respond_to? everywhere is not very clean either (where’d that have us all end up?). Could thread-local variables be used as an alternative to instance variables and the env? It’s just an idea, no idea if it’s doable.

Thanks!

[josevalim]

Nope, thread variables are not an alternative because we need to know this information in another request, which may happen in a different process, different machine. If someone want to sprinkle up the readme or add a note to the README we show when you call devise:install, patches are welcome.

[jesseclark]

Would it not be possible to create a separate hash that has the same life cycle as the flash but is specifically for storing Devise specific state?

I guess I would need to look more closely at how this is being used to understand the design needs but it seems like using the flash, which is supposed to be for displaying messages to a user from a controller, to store authentication system state, is a bit of a code smell.

Anyone who was accessing the flash for display like:

- flash.each do |key, msg|

is going to get bit by the :timedout key when they upgrade to Devise 2.0, and flash messages on timeouts are not something that most devs will be testing if they assume that Devise should handle that automagically.

[garrytan]

At best the flash notice is a nuisance, but at worst it is confusing to users.

[ris321]

Why can't this be put into the session hash? session[:timeout]

[deivid-rodriguez]

@josevalim Would you be willing to accept a PR that moves this to the session hash? It seems like a more appropriate place than using flash messages, right?

[josevalim]

@deivid-rodriguez no because it is a fine use of flash. :) Flash is not only about messages but temporary storage.

[deivid-rodriguez]

@josevalim Alright, this is all very subjective.

I just happened to read the code handling this and it seemed like a code smell (https://github.com/plataformatec/devise/blob/master/lib/devise/failure_app.rb#L58-63). It's not clear what it does, it's not commented, it's not tested, there's quite a lot of issues linking here, and that use of keep doesn't intuitively suggest temporary storage...

[josevalim]

We don't have unit tests but we do have integration tests. The flash is an implementation detail to how the feature works. Also, notice we also keep the alert, that's more of a bug of how flash behaves in Rails because it doesn't survive redirects...

[deivid-rodriguez]

Really? I thought they survived a single action... regardless of it being a redirect or not.

[ghost]

Excuse me, how to exclude ":timedout" flash messages from this code:

"flash.each do |msg_type, message|"

that I have in an helper?

[tonycchung]

@ginolon a bit late but perhaps something like this in your block:

unless msg_type == :timedout

or do a check before your block:

flash.except(:timedout).each do |msg_type, message|

[ghost]

@tonycchung If I use the except way, I got this:

undefined method 'except' for #<ActionDispatch::Flash::FlashHash:0x0000000d3df308>

def flash_messages(opts = {})
    flash.except(:timedout).each do |msg_type, message|
      concat(content_tag(:div, message, class: "alert #{bootstrap_class_for(msg_type)} alert-dismissible text-center fade in", role: 'alert') do
        concat(content_tag(:button, class: 'close', data: { dismiss: 'alert' }) do
          concat content_tag(:span, '×'.html_safe, 'aria-hidden' => true)
          concat content_tag(:span, 'Close', class: 'sr-only')
        end)
        concat message
      end)
    end
    nil
  end

[thtonon]

@ginolon Something like this inside your each block should do the trick

next unless value.is_a? String

[tanelsuurhans]

At least in Rails 4.2.4 ActionDispatch::Flash::FlashHash does not extend from Hash and does not have an #except method. So it's not really a viable option.

While I agree this is a good use of the flash hash, I think its a little stubborn to keep that as a justification when clearly its causing heartburn for people. I mean the intended use of the flash has is clearly messaging, with default methods for :notice and :alert keys and so forth.