From ac2bd92f1f59181a3c6d6f19d8ff35a0e7d46c54 Mon Sep 17 00:00:00 2001
From: Jens Kraemer <jk@jkraemer.net>
Date: Wed, 17 Oct 2018 12:07:14 +0800
Subject: [PATCH] Handles the case when an expired token is in the users
 session

- otherwise a user whose token expired after it was already put into
  their session would be redirected to the login page all the time.
- to fix that, the token is cleared from the session and the user is
  asked to try again
- before this change, the user would have to clear their cookies in this
  case to be able to ever get a new token
---
 app/controllers/account_controller.rb |  8 ++++-
 config/locales/de.yml                 |  1 +
 config/locales/en.yml                 |  1 +
 test/integration/account_test.rb      | 55 +++++++++++++++++++++++++++++++++++
 4 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/app/controllers/account_controller.rb b/app/controllers/account_controller.rb
index 7bb644761..f604540a7 100644
--- a/app/controllers/account_controller.rb
+++ b/app/controllers/account_controller.rb
@@ -62,9 +62,15 @@ class AccountController < ApplicationController
     (redirect_to(home_url); return) unless Setting.lost_password?
     if prt = (params[:token] || session[:password_recovery_token])
       @token = Token.find_token("recovery", prt.to_s)
-      if @token.nil? || @token.expired?
+      if @token.nil?
         redirect_to home_url
         return
+      elsif @token.expired?
+        # remove expired token from session and let user try again
+        session[:password_recovery_token] = nil
+        flash[:error] = l(:error_token_expired)
+        redirect_to lost_password_url
+        return
       end
 
       # redirect to remove the token query parameter from the URL and add it to the session
diff --git a/config/locales/de.yml b/config/locales/de.yml
index 1b166a033..cc04355d7 100644
--- a/config/locales/de.yml
+++ b/config/locales/de.yml
@@ -264,6 +264,7 @@ de:
   error_scm_command_failed: "Beim Zugriff auf das Repository ist ein Fehler aufgetreten: %{value}"
   error_scm_not_found: Eintrag und/oder Revision existiert nicht im Repository.
   error_session_expired: Ihre Sitzung ist abgelaufen. Bitte melden Sie sich erneut an.
+  error_token_expired: "Dieser Link zum Passwort zurücksetzen ist nicht mehr gültig. Bitte versuchen Sie es erneut."
   error_unable_delete_issue_status: "Der Ticket-Status konnte nicht gelöscht werden."
   error_unable_to_connect: Fehler beim Verbinden (%{value})
   error_workflow_copy_source: Bitte wählen Sie einen Quell-Tracker und eine Quell-Rolle.
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 6e2cd75c5..36d2099c8 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -208,6 +208,7 @@ en:
   error_unable_to_connect: "Unable to connect (%{value})"
   error_attachment_too_big: "This file cannot be uploaded because it exceeds the maximum allowed file size (%{max_size})"
   error_session_expired: "Your session has expired. Please login again."
+  error_token_expired: "This password recovery link has expired, please try again."
   warning_attachments_not_saved: "%{count} file(s) could not be saved."
   error_password_expired: "Your password has expired or the administrator requires you to change it."
   error_invalid_file_encoding: "The file is not a valid %{encoding} encoded file"
diff --git a/test/integration/account_test.rb b/test/integration/account_test.rb
index a3a1ecb2e..4f0a45156 100644
--- a/test/integration/account_test.rb
+++ b/test/integration/account_test.rb
@@ -146,6 +146,61 @@ class AccountTest < Redmine::IntegrationTest
     assert_equal false, Token.exists?(token.id), "Password recovery token was not deleted"
   end
 
+  def test_lost_password_expired_token
+    Token.delete_all
+
+    get "/account/lost_password"
+    assert_response :success
+    assert_select 'input[name=mail]'
+
+    post "/account/lost_password", :params => {
+        :mail => 'jSmith@somenet.foo'
+      }
+    assert_redirected_to "/login"
+
+    token = Token.first
+    assert_equal 'recovery', token.action
+    assert_equal 'jsmith@somenet.foo', token.user.mail
+    refute token.expired?
+
+    get "/account/lost_password", :params => {
+        :token => token.value
+      }
+    assert_redirected_to '/account/lost_password'
+
+    follow_redirect!
+    assert_response :success
+
+    # suppose the user forgets to continue the process and the token expires.
+    token.update_column :created_on, 1.week.ago
+    assert token.expired?
+
+    assert_select 'input[type=hidden][name=token][value=?]', token.value
+    assert_select 'input[name=new_password]'
+    assert_select 'input[name=new_password_confirmation]'
+
+    post "/account/lost_password", :params => {
+        :token => token.value, :new_password => 'newpass123',
+        :new_password_confirmation => 'newpass123'
+      }
+
+    assert_redirected_to "/account/lost_password"
+    assert_equal 'This password recovery link has expired, please try again.', flash[:error]
+    follow_redirect!
+    assert_response :success
+
+    post "/account/lost_password", :params => {
+        :mail => 'jSmith@somenet.foo'
+      }
+    assert_redirected_to "/login"
+
+    # should have a new token now
+    token = Token.last
+    assert_equal 'recovery', token.action
+    assert_equal 'jsmith@somenet.foo', token.user.mail
+    refute token.expired?
+  end
+
   def test_user_with_must_change_passwd_should_be_forced_to_change_its_password
     User.find_by_login('jsmith').update_attribute :must_change_passwd, true
 
-- 
2.11.0