Incorrect error message when entering valid token after entering invalid token

[pkarjala]

devise 4.9.4
devise-otp 0.7.1

Steps to reproduce:

1. Log into site using username/password.
2. Enter incorrect token value and press "Submit Token".
3. Receive an error message The token you provided was invalid. and be redirected back to token entry page.
4. Enter a valid token value and press "Submit Token".
5. Be successfully logged in, but still get error message The token you provided was invalid.

This occurs each time after entering an invalid token, then subsequently entering a valid token. Once you have logged into the application, the error message displays. Navigating to another page in the application without logging out causes the error message to disappear, so it is only happening after the initial failed and subsequent successful token login.

[strzibny]

Could you try to track this down and submit a PR?

[pkarjala]

I will try to find some time to do so!

[pkarjala]

OK, I tracked this down to

devise-otp/lib/devise_otp_authenticatable/controllers/helpers.rb

Line 19 in 33b3b77

flash[key] = message if message.present?

The issue may be that the messages are being set using flash instead of flash.now. When flash is used by itself, the messages will persist through redirects. For flash.now they will only show for the currently loaded page.

See https://api.rubyonrails.org/classes/ActionDispatch/Flash/FlashHash.html#method-i-now, https://stackoverflow.com/questions/18748072/rails-4-flash-message-persists-for-the-next-page-view, and https://medium.com/@tdonovan79/flash-in-rails-apps-df5ef389139f for additional info.

I'm not sure if there needs to be differentiation between some flash messages persisting through redirects and others only for the current page. So I haven't created a PR yet as there is probably some nuance there that may be affected by creating all messages with flash.new.

[pkarjala]

Can confirm that this does impact other places by altering the above call to flash, resulting in messages not showing up when they should be. So there needs to be some further nuance on when to call flash and when to call flash.now for alerts and messages.

[strouptl]

Hi @pkarjala, thank you for bringing this to our attention. And yes, you are correct that some flash messages need to called with "flash.new."

@strzibny, a further complication here is that the OtpCredentialsController#update action redirects for blank tokens (which works fine, since the redirect itself counts as the next action), but renders "show" for invalid tokens (which displays the flash on the next controller action, i.e. after logging in).

The devise gem itself handles this in the equivalent "set_flash_message" by toggling whether to display the message immediately (i.e. flash.now) or on the next action (i.e. just flash) via an additional "now" option, so this is probably a good design pattern for us too (see below).

def set_flash_message(key, kind, options = {})     
  message = find_message(kind, options)
  if options[:now]
    flash.now[key] = message if message.present?
  else
    flash[key] = message if message.present?
  end 
end

https://github.com/heartcombo/devise/blob/72884642f5700439cc96ac560ee19a44af5a2d45/app/controllers/devise_controller.rb#L168

I will push up a PR with a fix for the immediate issue (i.e. the "invalid_token" message), a modification to the otp_set_flash_message method, and the underlying consistency between the way we handle blank and invalid tokens.

[strzibny]

Thanks for the investigation, I agree

[strouptl]

FYI, I found a couple of additional occurrences of the flash message issue. See PR #96 for resolution.

[strzibny]

Okay, I think we can close this and I'll likely release 1.0 soon.