From 220ed6c7914f5977d36b7617e9c38999317f57e5 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Thu, 16 Jul 2026 19:45:42 +0200 Subject: Implement transparent password hash migration Add has_secure_password and bcrypt while retaining compatibility with legacy SHA-1 password hashes. Existing users are upgraded to password_digest on their next successful login. Add regression tests covering both legacy and modern authentication paths. --- Gemfile | 1 + Gemfile.lock | 3 +++ app/models/user.rb | 31 ++++++++++++++++++++++++--- lib/authentication.rb | 10 +++++---- test/models/user_test.rb | 56 ++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 94 insertions(+), 7 deletions(-) diff --git a/Gemfile b/Gemfile index 97257a9..cad34df 100644 --- a/Gemfile +++ b/Gemfile @@ -38,6 +38,7 @@ gem 'rails_icons' gem 'globalize', '~> 7.0' # translated model attributes (Page title/abstract/body) gem 'acts_as_list' # page revision ordering gem 'will_paginate', '~> 3.0' +gem 'bcrypt', '~> 3.1' # Pinned to git until a release widens the activerecord < 8.1 ceiling. # Both gems work correctly on Rails 8.1; the gemspec constraint is overly conservative. diff --git a/Gemfile.lock b/Gemfile.lock index e113726..8e4d5af 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -95,6 +95,7 @@ GEM activerecord (>= 6.1) activesupport (>= 6.1) base64 (0.3.0) + bcrypt (3.1.22) bigdecimal (4.1.2) builder (3.3.0) coffee-rails (4.2.2) @@ -334,6 +335,7 @@ PLATFORMS DEPENDENCIES acts-as-taggable-on! acts_as_list + bcrypt (~> 3.1) chaos_calendar! coffee-rails (~> 4.0) concurrent-ruby (~> 1.3) @@ -375,6 +377,7 @@ CHECKSUMS acts-as-taggable-on (13.0.0) acts_as_list (1.2.6) sha256=8345380900b7bee620c07ad00991ccee59af3d8c9e8574f426e321da2865fdc8 base64 (0.3.0) sha256=27337aeabad6ffae05c265c450490628ef3ebd4b67be58257393227588f5a97b + bcrypt (3.1.22) sha256=1f0072e88c2d705d94aff7f2c5cb02eb3f1ec4b8368671e19112527489f29032 bigdecimal (4.1.2) sha256=53d217666027eab4280346fba98e7d5b66baaae1b9c3c1c0ffe89d48188a3fbd builder (3.3.0) sha256=497918d2f9dca528fdca4b88d84e4ef4387256d984b8154e9d5d3fe5a9c8835f bundler (4.0.15) sha256=a4ceb882fe94a0e0ac63cd0813932bbfd631a14e5ac0b7975189b19a4d28d9e7 diff --git a/app/models/user.rb b/app/models/user.rb index 92ac33a..5e47ae7 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1,6 +1,10 @@ require 'digest/sha1' class User < ApplicationRecord + has_secure_password(validations: false) + + alias_method :authenticate_bcrypt, :authenticate + # Mixins and Plugins include Authentication include Authentication::ByPassword @@ -23,9 +27,30 @@ class User < ApplicationRecord # Authenticates a user by their login name and unencrypted password. Returns the user or nil. def self.authenticate(login, password) - return nil if login.blank? || password.blank? - u = find_by_login(login) # need to get the salt - u && u.authenticated?(password) ? u : nil + return if login.blank? || password.blank? + + user = find_by(login: login) + return unless user + + user&.authenticate(password) + end + + def authenticate(password) + if password_digest.present? + return self if authenticate_bcrypt(password) + return nil + end + + return nil unless authenticate_legacy(password) + + transaction do + self.password = password + self.crypted_password = nil + self.salt = nil + save!(validate: false) + end + + self end # TODO: Do we really want to have downcase logins only? diff --git a/lib/authentication.rb b/lib/authentication.rb index 4a3ad46..56df391 100644 --- a/lib/authentication.rb +++ b/lib/authentication.rb @@ -46,7 +46,6 @@ module Authentication include ModelInstanceMethods # Virtual attribute for the unencrypted password - attr_accessor :password validates_presence_of :password, :if => :password_required? validates_presence_of :password_confirmation, :if => :password_required? validates_confirmation_of :password, :if => :password_required? @@ -85,16 +84,19 @@ module Authentication self.class.password_digest(password, salt) end - def authenticated?(password) + def authenticate_legacy(password) crypted_password == encrypt(password) end - + # before filter def encrypt_password return if password.blank? - self.salt = self.class.make_token if new_record? + return if password_digest.present? + + self.salt ||= self.class.make_token self.crypted_password = encrypt(password) end + def password_required? crypted_password.blank? || !password.blank? end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 049892e..514bdea 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -62,6 +62,62 @@ class UserTest < ActiveSupport::TestCase def test_should_not_authenticate_unknown_user assert_nil User.authenticate("nosuchuser", "monkey") end + + def test_user_with_crypted_password_is_migrated_on_login + user = users(:quentin) + + assert_nil user.password_digest + + assert User.authenticate("quentin", "monkey") + + user.reload + + assert_not_nil user.password_digest + assert_nil user.crypted_password + assert_nil user.salt + end + + def test_new_user_uses_password_digest + user = create_user + + assert_not_nil user.password_digest + assert_nil user.crypted_password + assert_nil user.salt + + assert_equal user, User.authenticate("quire", "quire69") + end + + def test_legacy_user_is_migrated_on_login + user = users(:quentin) + + assert_nil user.password_digest + assert_not_nil user.crypted_password + assert_not_nil user.salt + + assert_equal user, User.authenticate("quentin", "monkey") + + user.reload + + assert_not_nil user.password_digest + assert_nil user.crypted_password + assert_nil user.salt + end + + def test_migrated_user_authenticates_using_password_digest + user = users(:quentin) + + # Trigger automatic migration. + assert_equal user, User.authenticate("quentin", "monkey") + + user.reload + + assert_not_nil user.password_digest + assert_nil user.crypted_password + assert_nil user.salt + + # Second login should now use password_digest. + assert_equal user, User.authenticate("quentin", "monkey") + end protected def create_user(options = {}) -- cgit v1.3