refactored UserPassword#confirm_password? and changes required to accommodate hashing the password after validations

app/models/user.rb

@@ -3,9 +3,9 @@
 class User < ActiveRecord::Base
   self.ignored_columns = [
     :old_seen_notification_id, # TODO: Remove when column is dropped. At this point, the migration to drop the column has not been written.
-    :salt, # TODO: Remove once DropPasswordColumnsFromUsers has been promoted to pre-deploy
-    :password_hash, # TODO: Remove once DropPasswordColumnsFromUsers has been promoted to pre-deploy
-    :password_algorithm, # TODO: Remove once  DropPasswordColumnsFromUsers has been promoted to pre-deploy
+    :salt, # TODO: Remove when column is dropped. At this point, the migration to drop the column has not been written.
+    :password_hash, # TODO: Remove when column is dropped. At this point, the migration to drop the column has not been written.
+    :password_algorithm, # TODO: Remove when column is dropped. At this point, the migration to drop the column has not been written.
   ]
 
   include Searchable
@@ -925,6 +925,7 @@ class User < ActiveRecord::Base
 
     if user_password
       user_password.password = pw
+      user_password.password_hash_will_change!
     else
       build_user_password(password: pw)
     end
@@ -936,14 +937,29 @@ class User < ActiveRecord::Base
   end
 
   def password_hash
+    Discourse.deprecate(
+      "User#password_hash is deprecated, use UserPassword#password_hash instead.",
+      drop_from: "3.3",
+      raise_error: false,
+    )
     user_password&.password_hash
   end
 
   def password_algorithm
+    Discourse.deprecate(
+      "User#password_algorithm is deprecated, use UserPassword#password_algorithm instead.",
+      drop_from: "3.3",
+      raise_error: false,
+    )
     user_password&.password_algorithm
   end
 
   def salt
+    Discourse.deprecate(
+      "User#password_salt is deprecated, use UserPassword#password_salt instead.",
+      drop_from: "3.3",
+      raise_error: false,
+    )
     user_password&.password_salt
   end
 

app/models/user_password.rb

@@ -6,43 +6,59 @@ class UserPassword < ActiveRecord::Base
     "$pbkdf2-#{Rails.configuration.pbkdf2_algorithm}$i=#{Rails.configuration.pbkdf2_iterations},l=32$"
   PASSWORD_SALT_LENGTH = 16
 
-  attr_reader :password # required to build an instance of this model with password attribute without backing column
-
   belongs_to :user, required: true
 
   validates :user_id, uniqueness: true
-  validates :password_hash, presence: true, length: { is: 64 }, uniqueness: { scope: :user_id }
-  validates :password_salt, presence: true, length: { is: 32 }
-  validates :password_algorithm, presence: true, length: { maximum: 64 }
   validate :password_validator
+  before_save :ensure_password_is_hashed
+  after_save :clear_raw_password
+
+  def password
+    nil
+  end
 
   def password=(pw)
     return if pw.blank?
-    @password = pw
-
-    # ensure password is hashed before validations, this should not be in a before_validation hook as that's skipped during .save(validate: false) (e.g. Seedfu)
-    self.password_salt = SecureRandom.hex(PASSWORD_SALT_LENGTH)
-    self.password_algorithm = TARGET_PASSWORD_ALGORITHM
-    self.password_hash = hash_password(password, password_salt, password_algorithm)
+    @raw_password = pw
   end
 
   def password_validation_required?
-    password.present?
+    @raw_password.present?
   end
 
   def confirm_password?(pw)
     # nothing to confirm if this record has not been persisted yet
     return false if !persisted?
-
-    if password_hash_changed? &&
-         password_hash_was != hash_password(pw, password_salt_was, password_algorithm_was)
-      return false
-    end
-
     return false if password_hash != hash_password(pw, password_salt, password_algorithm)
+    regen_password!(pw) if password_algorithm != TARGET_PASSWORD_ALGORITHM
 
-    return true if password_algorithm == TARGET_PASSWORD_ALGORITHM
+    true
+  end
 
+  private
+
+  def clear_raw_password
+    @raw_password = nil
+  end
+
+  def password_validator
+    UserPasswordValidator.new(attributes: :password).validate_each(self, :password, @raw_password)
+  end
+
+  def hash_password(pw, salt, algorithm)
+    raise StandardError.new("password is too long") if pw.size > MAX_PASSWORD_LENGTH
+    PasswordHasher.hash_password(password: pw, salt: salt, algorithm: algorithm)
+  end
+
+  def ensure_password_is_hashed
+    return if @raw_password.blank?
+
+    self.password_salt = SecureRandom.hex(PASSWORD_SALT_LENGTH)
+    self.password_algorithm = TARGET_PASSWORD_ALGORITHM
+    self.password_hash = hash_password(@raw_password, password_salt, password_algorithm)
+  end
+
+  def regen_password!(pw)
     # Regenerate password_hash with new algorithm and persist, we skip validation here since it has already run once when the hash was persisted the first time
     salt = SecureRandom.hex(PASSWORD_SALT_LENGTH)
     update_columns(
@@ -51,17 +67,6 @@ class UserPassword < ActiveRecord::Base
       password_hash: hash_password(pw, salt, TARGET_PASSWORD_ALGORITHM),
     )
   end
-
-  private
-
-  def password_validator
-    UserPasswordValidator.new(attributes: :password).validate_each(self, :password, password)
-  end
-
-  def hash_password(password, salt, algorithm)
-    raise StandardError.new("password is too long") if password.size > MAX_PASSWORD_LENGTH
-    PasswordHasher.hash_password(password: password, salt: salt, algorithm: algorithm)
-  end
 end
 
 # == Schema Information

lib/validators/password_validator.rb

@@ -4,30 +4,6 @@ class PasswordValidator < ActiveModel::EachValidator
   def validate_each(record, attribute, value)
     return unless record.password_validation_required?
 
-    if value.nil?
-      record.errors.add(attribute, :blank)
-    elsif value.length < SiteSetting.min_admin_password_length &&
-          (record.admin? || is_developer?(record.email))
-      record.errors.add(attribute, :too_short, count: SiteSetting.min_admin_password_length)
-    elsif value.length < SiteSetting.min_password_length
-      record.errors.add(attribute, :too_short, count: SiteSetting.min_password_length)
-    elsif record.username.present? && value == record.username
-      record.errors.add(attribute, :same_as_username)
-    elsif record.name.present? && value == record.name
-      record.errors.add(attribute, :same_as_name)
-    elsif record.email.present? && value == record.email
-      record.errors.add(attribute, :same_as_email)
-    elsif record.confirm_password?(value)
-      record.errors.add(attribute, :same_as_current)
-    elsif SiteSetting.block_common_passwords && CommonPasswords.common_password?(value)
-      record.errors.add(attribute, :common)
-    elsif value.chars.uniq.length < SiteSetting.password_unique_characters
-      record.errors.add(attribute, :unique_characters)
-    end
-  end
-
-  def is_developer?(value)
-    Rails.configuration.respond_to?(:developer_emails) &&
-      Rails.configuration.developer_emails.include?(value)
+    record.errors.add(attribute, :blank) if value.nil?
   end
 end

spec/lib/validators/password_validator_spec.rb

@@ -7,13 +7,13 @@ RSpec.describe PasswordValidator do
 
   subject(:validate) { validator.validate_each(record, :password, @password) }
 
-  let(:validator) { described_class.new(attributes: :password) }
+  let(:validator) { UserPasswordValidator.new(attributes: :password) }
 
   describe "password required" do
     let(:record) do
       u = Fabricate.build(:user, password: @password)
       u.password_required!
-      u
+      u.user_password
     end
 
     context "when password is not common" do
@@ -50,7 +50,7 @@ RSpec.describe PasswordValidator do
           SiteSetting.min_admin_password_length = 15
 
           @password = "12345678912"
-          record.admin = true
+          record.user.admin = true
           validate
           expect(record.errors[:password]).to be_present
         end

spec/models/user_password_spec.rb

@@ -1,6 +1,22 @@
 # frozen_string_literal: true
 
 RSpec.describe UserPassword do
+  context "when saving passwords through User" do
+    it "should save when first time password" do
+      pw = "helloworldakjdlakjfalkjfd"
+      u = Fabricate(:user, password: nil)
+      u.password = pw
+      u.save
+      expect(u.confirm_password?(pw)).to eq true
+    end
+
+    it "should save when updated existing password" do
+      pw = SecureRandom.hex
+      u = Fabricate(:user, password: "lajdlaksjfalkfjaelkfj")
+      u.update!(password: pw)
+      expect(u.confirm_password?(pw)).to eq true
+    end
+  end
   context "for validations" do
     it "should validate presence of user_id" do
       user_password = Fabricate.build(:user_password, user: nil)
@@ -8,79 +24,5 @@ RSpec.describe UserPassword do
       expect(user_password).not_to be_valid
       expect(user_password.errors[:user]).to include("must exist")
     end
-
-    it "should validate presence of password_hash" do
-      user_password = Fabricate.build(:user_password)
-      user_password.password_hash = nil
-
-      expect(user_password).not_to be_valid
-      expect(user_password.errors[:password_hash]).to include("can't be blank")
-    end
-
-    it "should validate that password_hash is 64 characters long" do
-      user_password = Fabricate.build(:user_password)
-      user_password.password_hash = "a" * 65
-
-      expect(user_password).not_to be_valid
-
-      expect(user_password.errors[:password_hash]).to include(
-        "is the wrong length (should be 64 characters)",
-      )
-    end
-
-    it "should validate uniqueness of password_hash scoped to user_id" do
-      password = "password"
-      user_password_1 = Fabricate(:user_password, password:)
-      user = user_password_1.user
-
-      user_password_2 =
-        Fabricate.build(
-          :user_password,
-          user:,
-          password:,
-          password_salt: user_password_1.password_salt,
-          password_algorithm: user_password_1.password_algorithm,
-        )
-
-      expect(user_password_2).not_to be_valid
-      expect(user_password_2.errors[:password_hash]).to include("has already been taken")
-    end
-
-    it "should validate presence of password_salt" do
-      user_password = Fabricate.build(:user_password)
-      user_password.password_salt = nil
-
-      expect(user_password).not_to be_valid
-      expect(user_password.errors[:password_salt]).to include("can't be blank")
-    end
-
-    it "should validate that password_salt is 32 characters long" do
-      user_password = Fabricate.build(:user_password)
-      user_password.password_salt = "a" * 33
-
-      expect(user_password).not_to be_valid
-
-      expect(user_password.errors[:password_salt]).to include(
-        "is the wrong length (should be 32 characters)",
-      )
-    end
-
-    it "should validate presence of password_algorithm" do
-      user_password = Fabricate.build(:user_password)
-      user_password.password_algorithm = nil
-
-      expect(user_password).not_to be_valid
-      expect(user_password.errors[:password_algorithm]).to include("can't be blank")
-    end
-
-    it "should validate that password_algorithm is at most 64 characters long" do
-      user_password = Fabricate.build(:user_password)
-      user_password.password_algorithm = "a" * 65
-
-      expect(user_password).not_to be_valid
-      expect(user_password.errors[:password_algorithm]).to include(
-        "is too long (maximum is 64 characters)",
-      )
-    end
   end
 end

spec/requests/session_controller_spec.rb

@@ -2038,7 +2038,7 @@ RSpec.describe SessionController do
         use_redis_snapshotting
 
         it "should return an error response code with the right error message" do
-          UserPasswordExpirer.expire_user_password(user.reload)
+          UserPasswordExpirer.expire_user_password(user)
           post "/session.json", params: { login: user.username, password: "myawesomepassword" }
 
           expect(response.status).to eq(200)

spec/services/user_password_expirer_spec.rb

@@ -37,10 +37,7 @@ RSpec.describe UserPasswordExpirer do
 
       freeze_time do
         user.update!(password: new_password)
-        expect { described_class.expire_user_password(user.reload) }.not_to change(
-          UserPassword,
-          :count,
-        )
+        expect { described_class.expire_user_password(user) }.not_to change(UserPassword, :count)
 
         user_password = user.user_password.reload