password_expirable helper bugfix (#201)

Fixes #186 
Co-authored-by: Troy Rosenberg <tmr08c@gmail.com>

README.md

@@ -1,8 +1,8 @@
 # Devise Security
 
-[![Build
-Status](https://travis-ci.org/devise-security/devise-security.svg?branch=master)](https://travis-ci.org/devise-security/devise-security)[![Coverage
-Status](https://coveralls.io/repos/github/devise-security/devise-security/badge.svg?branch=master)](https://coveralls.io/github/devise-security/devise-security?branch=master)[![Maintainability](https://api.codeclimate.com/v1/badges/ace7cd003a0db8bffa5a/maintainability)](https://codeclimate.com/github/devise-security/devise-security/maintainability)
+[![Build Status](https://travis-ci.org/devise-security/devise-security.svg?branch=master)](https://travis-ci.org/devise-security/devise-security)
+[![Coverage Status](https://coveralls.io/repos/github/devise-security/devise-security/badge.svg?branch=master)](https://coveralls.io/github/devise-security/devise-security?branch=master)
+[![Maintainability](https://api.codeclimate.com/v1/badges/ace7cd003a0db8bffa5a/maintainability)](https://codeclimate.com/github/devise-security/devise-security/maintainability)
 
 A [Devise](https://github.com/plataformatec/devise) extension to add additional
 security features required by modern web applications. Forked from
@@ -84,7 +84,7 @@ Devise.setup do |config|
 
   # Password expires after a configurable time (in seconds).
   # Or expire passwords on demand by setting this configuration to `true`
-  # Use `user.need_password_change!` to expire a password.
+  # Use `user.need_change_password!` to expire a password.
   # Setting the configuration to `false` will completely disable expiration checks.
   # config.expire_password_after = 3.months | true | false
 

lib/devise-security/controllers/helpers.rb

@@ -42,20 +42,29 @@ def valid_security_question_answer?(resource, answer)
 
       private
 
-      # lookup if an password change needed
+      # Called as a `before_action` on all actions on any controller that uses
+      # this helper. If the user's session is marked as having an expired
+      # password we double check in case it has been changed by another process,
+      # then redirect to the password change url.
+      #
+      # @note `Warden::Manager.after_authentication` is run AFTER this method
+      #
+      # @note Once the warden session has `'password_expired'` set to `false`,
+      #    it will **never** be checked again until the user re-logs in.
       def handle_password_change
         return if warden.nil?
 
-        if !devise_controller? && !ignore_password_expire? && !request.format.nil? && request.format.html?
+        if !devise_controller? &&
+           !ignore_password_expire? &&
+           !request.format.nil? &&
+           request.format.html?
           Devise.mappings.keys.flatten.any? do |scope|
-            if signed_in?(scope) && warden.session(scope)['password_expired']
-              # re-check to avoid infinite loop if date changed after login attempt
+            if signed_in?(scope) && warden.session(scope)['password_expired'] == true
               if send(:"current_#{scope}").try(:need_change_password?)
                 store_location_for(scope, request.original_fullpath) if request.get?
-                redirect_for_password_change scope
-                return
+                redirect_for_password_change(scope)
               else
-                warden.session(scope)[:password_expired] = false
+                warden.session(scope)['password_expired'] = false
               end
             end
           end

lib/devise-security/hooks/password_expirable.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+# @note This happens after
+#   {DeviseSecurity::Controller::Helpers#handle_password_change}
 Warden::Manager.after_authentication do |record, warden, options|
   if record.respond_to?(:need_change_password?)
     warden.session(options[:scope])['password_expired'] = record.need_change_password?

test/controllers/test_password_expired_controller.rb

@@ -16,6 +16,8 @@ class Devise::PasswordExpiredControllerTest < ActionController::TestCase
       confirmed_at: 5.months.ago
     )
     assert @user.valid?
+    assert @user.need_change_password?
+
     sign_in(@user)
   end
 

test/dummy/config/routes.rb

@@ -3,11 +3,11 @@
 RailsApp::Application.routes.draw do
   devise_for :users
 
-  devise_for :captcha_users, only: [:sessions], controllers: { sessions: "captcha/sessions" }
-  devise_for :security_question_users, only: [:sessions, :unlocks], controllers: { unlocks: "security_question/unlocks" }
+  devise_for :captcha_users, only: [:sessions], controllers: { sessions: 'captcha/sessions' }
+  devise_for :security_question_users, only: [:sessions, :unlocks], controllers: { unlocks: 'security_question/unlocks' }
 
   resources :foos
   resource :widgets
 
-  root to: 'foos#index'
+  root to: 'widgets#show'
 end

test/integration/test_password_expirable_workflow.rb

@@ -0,0 +1,57 @@
+# frozen_string_literal: true
+
+require 'test_helper'
+
+class TestPasswordExpirableWorkflow < ActionDispatch::IntegrationTest
+  include IntegrationHelpers
+
+  setup do
+    @user = User.create!(password: 'passWord1',
+                         password_confirmation: 'passWord1',
+                         email: 'bob@microsoft.com',
+                         password_changed_at: 4.months.ago) # the default expiration time is 3.months.ago
+    @user.confirm
+
+    assert @user.valid?
+    assert @user.need_change_password?
+  end
+
+  test 'sign in and change expired password' do
+    skip("Does not work in Rails < 5.0") if Rails.gem_version < Gem::Version.new('5.0')
+
+    sign_in(@user)
+    assert_redirected_to(root_path)
+    follow_redirect!
+    assert_redirected_to(user_password_expired_path)
+    # @note This is not the same controller used by Devise for password changes
+    put '/users/password_expired', params: {
+      user: {
+        current_password: 'passWord1',
+        password: 'Password12345!',
+        password_confirmation: 'Password12345!',
+      },
+    }
+    assert_redirected_to(root_path)
+    @user.reload
+    assert_not @user.need_change_password?
+  end
+
+  test 'sign in and password is updated before redirect completes' do
+    skip("Does not work in Rails < 5.0") if Rails.gem_version < Gem::Version.new('5.0')
+
+    sign_in(@user)
+    assert_redirected_to(root_path)
+
+    # simulates an external process updating the password
+    @user.update(password_changed_at: Time.zone.now)
+    assert_not @user.need_change_password?
+
+    follow_redirect!
+    assert_response :success
+
+    # if the password is expired at this point they will be redirected to the
+    # password change controller.
+    get root_path
+    assert_response :success
+  end
+end

test/support/integration_helpers.rb

@@ -4,7 +4,7 @@ module IntegrationHelpers
   # @param session [ActionDispatch::Integration::Session]
   # @return [void]
   # @note accounts for differences in the integration test API between rails versions
-  def sign_in(user, session)
+  def sign_in(user, session = integration_session)
     if Rails.gem_version > Gem::Version.new('5.0')
       session.post new_user_session_path, params: {
         user: {