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 = {})