From cde3ffcbc0f5607b210c49c06cce5942dd28b601 Mon Sep 17 00:00:00 2001 From: Tom Hughes Date: Tue, 28 Apr 2026 22:55:43 +0100 Subject: [PATCH] Redirect users with invalid password to the reset flow --- .rubocop_todo.yml | 2 +- app/controllers/sessions_controller.rb | 18 +++++--- app/models/user.rb | 43 +++++++++---------- config/locales/en.yml | 1 + lib/password_hash.rb | 4 ++ test/controllers/passwords_controller_test.rb | 2 +- test/controllers/sessions_controller_test.rb | 8 ++++ test/integration/user_creation_test.rb | 3 +- test/lib/password_hash_test.rb | 8 ++++ 9 files changed, 57 insertions(+), 32 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index bf88e164f..cbe6a459e 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -56,7 +56,7 @@ Metrics/BlockNesting: # Offense count: 23 # Configuration parameters: CountComments, CountAsOne. Metrics/ClassLength: - Max: 341 + Max: 345 # Offense count: 72 # Configuration parameters: AllowedMethods, AllowedPatterns. diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 6e28093c6..b890b2549 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -51,12 +51,18 @@ class SessionsController < ApplicationController ## # handle password authentication def password_authentication(username, password, referer = nil) - if (user = User.authenticate(:username => username, :password => password)) - successful_login(user, referer) - elsif (user = User.authenticate(:username => username, :password => password, :pending => true)) - unconfirmed_login(user, referer) - elsif User.authenticate(:username => username, :password => password, :suspended => true) - failed_login({ :partial => "sessions/suspended_flash" }, username, referer) + user = User.lookup(username) + + if user&.password_expired? + redirect_to user_forgot_password_path, :warning => t("sessions.new.reset_to_login") + elsif user&.password_matches?(password) + if user.pending? + unconfirmed_login(user, referer) + elsif user.suspended? + failed_login({ :partial => "sessions/suspended_flash" }, username, referer) + else + successful_login(user, referer) + end else failed_login(t("sessions.new.auth failure"), username, referer) end diff --git a/app/models/user.rb b/app/models/user.rb index 4b23c84d3..07764f124 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -156,34 +156,33 @@ class User < ApplicationRecord display_name end - def self.authenticate(options) - if options[:username] && options[:password] - user = find_by("email = ? OR display_name = ?", options[:username].strip, options[:username]) + def self.lookup(username) + user = find_by("email = ? OR display_name = ?", username.strip, username) - if user.nil? - users = where("LOWER(email) = LOWER(?) OR LOWER(NORMALIZE(display_name, NFKC)) = LOWER(NORMALIZE(?, NFKC))", options[:username].strip, options[:username]) + if user.nil? + users = where("LOWER(email) = LOWER(?) OR LOWER(NORMALIZE(display_name, NFKC)) = LOWER(NORMALIZE(?, NFKC))", username.strip, username) - user = users.first if users.one? - end + user = users.first if users.one? + end - if user && PasswordHash.check(user.pass_crypt, user.pass_salt, options[:password]) - if PasswordHash.upgrade?(user.pass_crypt, user.pass_salt) - user.pass_crypt, user.pass_salt = PasswordHash.create(options[:password]) - user.save - end - else - user = nil + user if user && user.status != "deleted" + end + + def password_expired? + !PasswordHash.valid?(pass_crypt, pass_salt) + end + + def password_matches?(password) + if PasswordHash.check(pass_crypt, pass_salt, password) + if PasswordHash.upgrade?(pass_crypt, pass_salt) + self.pass_crypt, self.pass_salt = PasswordHash.create(password) + save end - end - if user && - (user.status == "deleted" || - (user.status == "pending" && !options[:pending]) || - (user.status == "suspended" && !options[:suspended])) - user = nil + true + else + false end - - user end aasm :column => :status, :no_direct_assignment => true do diff --git a/config/locales/en.yml b/config/locales/en.yml index 6ecf7a072..482c1d345 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2228,6 +2228,7 @@ en: login_button: "Log in" with external: "or log in with a third party" or: "or" + reset_to_login: "Sorry, your password has been expired. Please reset your password to login." auth failure: "Sorry, could not log in with those details." destroy: title: "Logout" diff --git a/lib/password_hash.rb b/lib/password_hash.rb index 96ce4f62e..69c47d6b9 100644 --- a/lib/password_hash.rb +++ b/lib/password_hash.rb @@ -39,6 +39,10 @@ module PasswordHash true end + def self.valid?(hash, salt) + Argon2::HashFormat.valid_hash?(hash) || salt&.include?("!") || hash =~ /^[0-9A-Z]{32}$/i + end + def self.pbkdf2(password, salt, iterations, size, algorithm) digest = OpenSSL::Digest.new(algorithm) pbkdf2 = OpenSSL::PKCS5.pbkdf2_hmac(password, salt, iterations, size, digest) diff --git a/test/controllers/passwords_controller_test.rb b/test/controllers/passwords_controller_test.rb index 3adc27624..c44d1b4e5 100644 --- a/test/controllers/passwords_controller_test.rb +++ b/test/controllers/passwords_controller_test.rb @@ -153,6 +153,6 @@ class PasswordsControllerTest < ActionDispatch::IntegrationTest user.reload assert_equal "active", user.status assert user.email_valid - assert_equal user, User.authenticate(:username => user.email, :password => "new_password") + assert user.password_matches?("new_password") end end diff --git a/test/controllers/sessions_controller_test.rb b/test/controllers/sessions_controller_test.rb index 367722ddf..d52d75895 100644 --- a/test/controllers/sessions_controller_test.rb +++ b/test/controllers/sessions_controller_test.rb @@ -97,6 +97,14 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest assert_equal(I18n.t("sessions.new.auth failure"), flash[:error]) end + def test_login_expired_password + user = create(:user, :pass_crypt => "expired password") + + post login_path, :params => { :username => user.display_name, :password => "s3cr3t", :remember_me => "0" } + assert_redirected_to user_forgot_password_path + assert_equal(I18n.t("sessions.new.reset_to_login"), flash[:warning]) + end + def test_logout_without_referer post logout_path assert_redirected_to root_path diff --git a/test/integration/user_creation_test.rb b/test/integration/user_creation_test.rb index 5ec781d3f..c3d60ed8c 100644 --- a/test/integration/user_creation_test.rb +++ b/test/integration/user_creation_test.rb @@ -162,8 +162,7 @@ class UserCreationTest < ActionDispatch::IntegrationTest user.reload assert_predicate user, :active? - - assert_equal user, User.authenticate(:username => new_email, :password => "testtest") + assert user.password_matches?("testtest") end # Check that the user can successfully recover their password diff --git a/test/lib/password_hash_test.rb b/test/lib/password_hash_test.rb index 2939c86c1..1794c8a76 100644 --- a/test/lib/password_hash_test.rb +++ b/test/lib/password_hash_test.rb @@ -7,6 +7,7 @@ class PasswordHashTest < ActiveSupport::TestCase assert PasswordHash.check("5f4dcc3b5aa765d61d8327deb882cf99", nil, "password") assert_not PasswordHash.check("5f4dcc3b5aa765d61d8327deb882cf99", nil, "wrong") assert PasswordHash.upgrade?("5f4dcc3b5aa765d61d8327deb882cf99", nil) + assert PasswordHash.valid?("5f4dcc3b5aa765d61d8327deb882cf99", nil) end def test_md5_with_salt @@ -14,6 +15,7 @@ class PasswordHashTest < ActiveSupport::TestCase assert_not PasswordHash.check("67a1e09bb1f83f5007dc119c14d663aa", "salt", "wrong") assert_not PasswordHash.check("67a1e09bb1f83f5007dc119c14d663aa", "wrong", "password") assert PasswordHash.upgrade?("67a1e09bb1f83f5007dc119c14d663aa", "salt") + assert PasswordHash.valid?("67a1e09bb1f83f5007dc119c14d663aa", "salt") end def test_pbkdf2_1000_32_sha512 @@ -21,6 +23,7 @@ class PasswordHashTest < ActiveSupport::TestCase assert_not PasswordHash.check("ApT/28+FsTBLa/J8paWfgU84SoRiTfeY8HjKWhgHy08=", "sha512!1000!HR4z+hAvKV2ra1gpbRybtoNzm/CNKe4cf7bPKwdUNrk=", "wrong") assert_not PasswordHash.check("ApT/28+FsTBLa/J8paWfgU84SoRiTfeY8HjKWhgHy08=", "sha512!1000!HR4z+hAvKV2ra1gwrongtoNzm/CNKe4cf7bPKwdUNrk=", "password") assert PasswordHash.upgrade?("ApT/28+FsTBLa/J8paWfgU84SoRiTfeY8HjKWhgHy08=", "sha512!1000!HR4z+hAvKV2ra1gpbRybtoNzm/CNKe4cf7bPKwdUNrk=") + assert PasswordHash.valid?("ApT/28+FsTBLa/J8paWfgU84SoRiTfeY8HjKWhgHy08=", "sha512!1000!HR4z+hAvKV2ra1gpbRybtoNzm/CNKe4cf7bPKwdUNrk=") end def test_pbkdf2_10000_32_sha512 @@ -28,6 +31,7 @@ class PasswordHashTest < ActiveSupport::TestCase assert_not PasswordHash.check("3wYbPiOxk/tU0eeIDjUhdvi8aDP3AbFtwYKKxF1IhGg=", "sha512!10000!OUQLgtM7eD8huvanFT5/WtWaCwdOdrir8QOtFwxhO0A=", "wrong") assert_not PasswordHash.check("3wYbPiOxk/tU0eeIDjUhdvi8aDP3AbFtwYKKxF1IhGg=", "sha512!10000!OUQLgtMwronguvanFT5/WtWaCwdOdrir8QOtFwxhO0A=", "password") assert PasswordHash.upgrade?("3wYbPiOxk/tU0eeIDjUhdvi8aDP3AbFtwYKKxF1IhGg=", "sha512!10000!OUQLgtM7eD8huvanFT5/WtWaCwdOdrir8QOtFwxhO0A=") + assert PasswordHash.valid?("3wYbPiOxk/tU0eeIDjUhdvi8aDP3AbFtwYKKxF1IhGg=", "sha512!10000!OUQLgtM7eD8huvanFT5/WtWaCwdOdrir8QOtFwxhO0A=") end def test_argon2_t2_m16_p1 @@ -35,6 +39,7 @@ class PasswordHashTest < ActiveSupport::TestCase assert_not PasswordHash.check("$argon2id$v=19$m=65536,t=2,p=1$b2E7zSvjT6TC5DXrqvfxwg$P4hly807ckgYc+kfvaf3rqmJcmKStzw+kV14oMaz8PQ", nil, "wrong") assert_not PasswordHash.check("$argon2id$v=19$m=65536,t=2,p=1$b2E7zSvwrong5DXrqvfxwg$P4hly807ckgYc+kfvaf3rqmJcmKStzw+kV14oMaz8PQ", nil, "password") assert PasswordHash.upgrade?("$argon2id$v=19$m=65536,t=2,p=1$b2E7zSvjT6TC5DXrqvfxwg$P4hly807ckgYc+kfvaf3rqmJcmKStzw+kV14oMaz8PQ", nil) + assert PasswordHash.valid?("$argon2id$v=19$m=65536,t=2,p=1$b2E7zSvjT6TC5DXrqvfxwg$P4hly807ckgYc+kfvaf3rqmJcmKStzw+kV14oMaz8PQ", nil) end def test_argon2_t3_m16_p4 @@ -42,6 +47,7 @@ class PasswordHashTest < ActiveSupport::TestCase assert_not PasswordHash.check("$argon2id$v=19$m=65536,t=3,p=4$uxzL4aYTEDTRr2+KNA1qNQ$yuNOtH+IsCwWUbE4OGu+hIC0e4iyZ2wGhaCsQY1mJpI", nil, "wrong") assert_not PasswordHash.check("$argon2id$v=19$m=65536,t=3,p=4$uxzL4aYwrongr2+KNA1qNQ$yuNOtH+IsCwWUbE4OGu+hIC0e4iyZ2wGhaCsQY1mJpI", nil, "password") assert_not PasswordHash.upgrade?("$argon2id$v=19$m=65536,t=3,p=4$uxzL4aYTEDTRr2+KNA1qNQ$yuNOtH+IsCwWUbE4OGu+hIC0e4iyZ2wGhaCsQY1mJpI", nil) + assert PasswordHash.valid?("$argon2id$v=19$m=65536,t=3,p=4$uxzL4aYTEDTRr2+KNA1qNQ$yuNOtH+IsCwWUbE4OGu+hIC0e4iyZ2wGhaCsQY1mJpI", nil) end def test_default @@ -56,6 +62,8 @@ class PasswordHashTest < ActiveSupport::TestCase assert_not PasswordHash.check(hash2, salt2, "wrong") assert_not PasswordHash.upgrade?(hash1, salt1) assert_not PasswordHash.upgrade?(hash2, salt2) + assert PasswordHash.valid?(hash1, salt1) + assert PasswordHash.valid?(hash2, salt2) end def test_format -- 2.47.3