Fix Issue with Invalid Token Message | Simplify OTP Credentials Contr…

…oller (#95) * add support for :now option to "otp_set_flash_message" method (follows devise "set_flash_message"); * update flash message for invalid token to render immediately within "update" action; * update test specs to ensure correct display of blank/invalid token flash messages, and to ensure that they do not display after a successful authentication; * update both blank and invalid OTP authentication attempts to render :show action for consistency; simplify condition for setting flash message;
wmlele · Sep 21, 2024 · a75f920 · a75f920
1 parent 33b3b77
commit a75f920
Show file tree

Hide file tree

Showing 4 changed files with 48 additions and 7 deletions.
diff --git a/app/controllers/devise_otp/devise/otp_credentials_controller.rb b/app/controllers/devise_otp/devise/otp_credentials_controller.rb
@@ -26,17 +26,15 @@ def show
       # signs the resource in, if the OTP token is valid and the user has a valid challenge
       #
       def update
-        if @token.blank?
-          otp_set_flash_message(:alert, :token_blank)
-          redirect_to otp_credential_path_for(resource_name, challenge: @challenge, recovery: @recovery)
-        elsif resource.otp_challenge_valid? && resource.validate_otp_token(@token, @recovery)
+        if resource.otp_challenge_valid? && resource.validate_otp_token(@token, @recovery)
           sign_in(resource_name, resource)
 
           otp_set_trusted_device_for(resource) if params[:enable_persistence] == "true"
           otp_refresh_credentials_for(resource)
           respond_with resource, location: after_sign_in_path_for(resource)
         else
-          otp_set_flash_message :alert, :token_invalid
+          kind = (@token.blank? ? :token_blank : :token_invalid)
+          otp_set_flash_message :alert, kind, :now => true
           render :show
         end
       end

diff --git a/lib/devise_otp_authenticatable/controllers/helpers.rb b/lib/devise_otp_authenticatable/controllers/helpers.rb
@@ -16,7 +16,14 @@ def otp_set_flash_message(key, kind, options = {})
         options[:resource_name] = resource_name
         options = devise_i18n_options(options) if respond_to?(:devise_i18n_options, true)
         message = I18n.t("#{options[:resource_name]}.#{kind}", **options)
-        flash[key] = message if message.present?
+
+        if message.present?
+          if options[:now]
+            flash.now[key] = message
+          else
+            flash[key] = message
+          end
+        end
       end
 
       def otp_t

diff --git a/test/dummy/app/views/layouts/application.html.erb b/test/dummy/app/views/layouts/application.html.erb
@@ -8,7 +8,13 @@
 </head>
 <body>
 
-<%= yield %>
+  <% if flash[:alert].present? %>
+    <div id="alert">
+      <%= flash[:alert] %>
+    </div>
+  <% end %>
+
+  <%= yield %>
 
 </body>
 </html>
diff --git a/test/integration/sign_in_test.rb b/test/integration/sign_in_test.rb
@@ -43,6 +43,7 @@ def teardown
     click_button "Submit Token"
 
     assert_equal user_otp_credential_path, current_path
+    assert page.has_content? "The token you provided was invalid."
   end
 
   test "fail blank token authentication" do
@@ -53,6 +54,7 @@ def teardown
     click_button "Submit Token"
 
     assert_equal user_otp_credential_path, current_path
+    assert page.has_content? "You need to type in the token you generated with your device."
   end
 
   test "successful token authentication" do
@@ -78,4 +80,32 @@ def teardown
     User.otp_authentication_timeout = old_timeout
     assert_equal new_user_session_path, current_path
   end
+
+  test "blank token flash message does not persist to successful authentication redirect." do
+    user = enable_otp_and_sign_in
+
+    fill_in "token", with: "123456"
+    click_button "Submit Token"
+
+    assert page.has_content?("The token you provided was invalid.")
+
+    fill_in "token", with: ROTP::TOTP.new(user.otp_auth_secret).at(Time.now)
+    click_button "Submit Token"
+
+    assert !page.has_content?("The token you provided was invalid.")
+  end
+
+  test "invalid token flash message does not persist to successful authentication redirect." do
+    user = enable_otp_and_sign_in
+
+    fill_in "token", with: ""
+    click_button "Submit Token"
+
+    assert page.has_content?("You need to type in the token you generated with your device.")
+
+    fill_in "token", with: ROTP::TOTP.new(user.otp_auth_secret).at(Time.now)
+    click_button "Submit Token"
+
+    assert !page.has_content?("You need to type in the token you generated with your device.")
+  end
 end