DEV: Migrate user passwords data to UserPassword table (#28746)

* Add migrations to ensure password hash is synced across users & user_passwords

* Persist password-related data in user_passwords instead of users

* Merge User#expire_old_email_tokens with User#expire_tokens_if_password_changed

* Add post deploy migration to mark password-related columns from users table as read-only

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

app/models/user.rb

@@ -3,6 +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 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
@@ -13,11 +16,6 @@ class User < ActiveRecord::Base
   include HasDeprecatedColumns
 
   DEFAULT_FEATURED_BADGE_COUNT = 3
-
-  PASSWORD_SALT_LENGTH = 16
-  TARGET_PASSWORD_ALGORITHM =
-    "$pbkdf2-#{Rails.configuration.pbkdf2_algorithm}$i=#{Rails.configuration.pbkdf2_iterations},l=32$"
-
   MAX_SIMILAR_USERS = 10
 
   deprecate_column :flag_level, drop_from: "3.2"
@@ -190,7 +188,6 @@ class User < ActiveRecord::Base
   after_update :change_display_name, if: :saved_change_to_name?
 
   before_save :update_usernames
-  before_save :ensure_password_is_hashed
   before_save :match_primary_group_changes
   before_save :check_if_title_is_badged_granted
   before_save :apply_watched_words, unless: :should_skip_user_fields_validation?
@@ -201,7 +198,6 @@ class User < ActiveRecord::Base
   after_save :clear_global_notice_if_needed
   after_save :refresh_avatar
   after_save :badge_grant
-  after_save :expire_old_email_tokens
   after_save :index_search
   after_save :check_site_contact_username
   after_save :add_to_user_directory,
@@ -410,7 +406,7 @@ class User < ActiveRecord::Base
   end
 
   def self.max_password_length
-    200
+    UserPassword::MAX_PASSWORD_LENGTH
   end
 
   def self.username_length
@@ -923,15 +919,49 @@ class User < ActiveRecord::Base
     )
   end
 
-  def password=(password)
+  def password=(pw)
     # special case for passwordless accounts
-    @raw_password = password if password.present?
+    return if pw.blank?
+
+    if user_password
+      user_password.password = pw
+    else
+      build_user_password(password: pw)
+    end
+    @raw_password = pw # still required to maintain compatibility with usage of password-related User interface
   end
 
   def password
     "" # so that validator doesn't complain that a password attribute doesn't exist
   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
+
   # Indicate that this is NOT a passwordless account for the purposes of validation
   def password_required!
     @password_required = true
@@ -946,7 +976,7 @@ class User < ActiveRecord::Base
   end
 
   def has_password?
-    password_hash.present?
+    user_password ? true : false
   end
 
   def password_validator
@@ -960,20 +990,8 @@ class User < ActiveRecord::Base
   end
 
   def confirm_password?(password)
-    return false unless password_hash && salt && password_algorithm
+    return false if !user_password
-    confirmed = self.password_hash == hash_password(password, salt, password_algorithm)
+    user_password.confirm_password?(password)
-
-    if confirmed && persisted? && password_algorithm != TARGET_PASSWORD_ALGORITHM
-      # Regenerate password_hash with new algorithm and persist
-      salt = SecureRandom.hex(PASSWORD_SALT_LENGTH)
-      update_columns(
-        password_algorithm: TARGET_PASSWORD_ALGORITHM,
-        salt: salt,
-        password_hash: hash_password(password, salt, TARGET_PASSWORD_ALGORITHM),
-      )
-    end
-
-    confirmed
   end
 
   def new_user_posting_on_first_day?
@@ -1903,12 +1921,6 @@ class User < ActiveRecord::Base
     BadgeGranter.queue_badge_grant(Badge::Trigger::UserChange, user: self)
   end
 
-  def expire_old_email_tokens
-    if saved_change_to_password_hash? && !saved_change_to_id?
-      email_tokens.where("not expired").update_all(expired: true)
-    end
-  end
-
   def index_search
     # force is needed as user custom fields are updated using SQL and after_save callback is not triggered
     SearchIndexer.index(self, force: true)
@@ -1939,20 +1951,15 @@ class User < ActiveRecord::Base
     email_tokens.create!(email: email, scope: EmailToken.scopes[:signup])
   end
 
-  def ensure_password_is_hashed
-    if @raw_password
-      self.salt = SecureRandom.hex(PASSWORD_SALT_LENGTH)
-      self.password_algorithm = TARGET_PASSWORD_ALGORITHM
-      self.password_hash = hash_password(@raw_password, salt, password_algorithm)
-    end
-  end
-
   def expire_tokens_if_password_changed
     # NOTE: setting raw password is the only valid way of changing a password
     # the password field in the DB is actually hashed, nobody should be amending direct
     if @raw_password
       # Association in model may be out-of-sync
       UserAuthToken.where(user_id: id).destroy_all
+
+      email_tokens.where("not expired").update_all(expired: true) if !saved_change_to_id?
+
       # We should not carry this around after save
       @raw_password = nil
       @password_required = false
@@ -2253,8 +2260,6 @@ end
 #  updated_at                :datetime         not null
 #  name                      :string
 #  last_posted_at            :datetime
-#  password_hash             :string(64)
-#  salt                      :string(32)
 #  active                    :boolean          default(FALSE), not null
 #  username_lower            :string(60)       not null
 #  last_seen_at              :datetime
@@ -2285,7 +2290,6 @@ end
 #  secure_identifier         :string
 #  flair_group_id            :integer
 #  last_seen_reviewable_id   :integer
-#  password_algorithm        :string(64)
 #  required_fields_version   :integer
 #  seen_notification_id      :bigint           default(0), not null
 #

app/models/user_password.rb

@@ -1,14 +1,75 @@
 # frozen_string_literal: true
 
 class UserPassword < ActiveRecord::Base
-  validates :user_id, presence: true
+  MAX_PASSWORD_LENGTH = 200
+  TARGET_PASSWORD_ALGORITHM =
+    "$pbkdf2-#{Rails.configuration.pbkdf2_algorithm}$i=#{Rails.configuration.pbkdf2_iterations},l=32$"
+  PASSWORD_SALT_LENGTH = 16
+
+  belongs_to :user, required: true
 
   validates :user_id, uniqueness: true
-  validates :password_hash, presence: true, length: { is: 64 }, uniqueness: { scope: :user_id }
+  validate :password_validator
-  validates :password_salt, presence: true, length: { is: 32 }
+  before_save :ensure_password_is_hashed
-  validates :password_algorithm, presence: true, length: { maximum: 64 }
+  after_save :clear_raw_password
 
-  belongs_to :user
+  def password
+    # this getter method is still required, but we store the set password in @raw_password instead of making it easily accessible from the getter
+    nil
+  end
+
+  def password=(pw)
+    return if pw.blank?
+
+    self.password_hash_will_change!
+    @raw_password = pw
+  end
+
+  def password_validation_required?
+    @raw_password.present?
+  end
+
+  def confirm_password?(pw)
+    # nothing to confirm if this record has not been persisted yet
+    return false if !persisted?
+    return false if password_hash != hash_password(pw, password_salt, password_algorithm)
+    regen_password!(pw) 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(
+      password_algorithm: TARGET_PASSWORD_ALGORITHM,
+      password_salt: salt,
+      password_hash: hash_password(pw, salt, TARGET_PASSWORD_ALGORITHM),
+    )
+  end
 end
 
 # == Schema Information

app/services/user_password_expirer.rb

@@ -2,14 +2,6 @@
 
 class UserPasswordExpirer
   def self.expire_user_password(user)
-    UserPassword
+    user.user_password&.update!(password_expired_at: Time.zone.now)
-      .where(user:)
-      .first_or_initialize
-      .update!(
-        password_hash: user.password_hash,
-        password_salt: user.salt,
-        password_algorithm: user.password_algorithm,
-        password_expired_at: Time.zone.now,
-      )
   end
 end

config/locales/server.en.yml

@@ -770,7 +770,6 @@ en:
               same_as_username: "is the same as your username. Please use a more secure password."
               same_as_email: "is the same as your email. Please use a more secure password."
               same_as_current: "is the same as your current password."
-              same_as_previous: "is the same as your previous password."
               same_as_name: "is the same as your name."
               unique_characters: "has too many repeated characters. Please use a more secure password."
             username:
@@ -780,6 +779,15 @@ en:
             ip_address:
               blocked: "New registrations are not allowed from your IP address."
               max_new_accounts_per_registration_ip: "New registrations are not allowed from your IP address (maximum limit reached). Contact a staff member."
+        user_password:
+          attributes:
+            password:
+              common: "is one of the 10000 most common passwords. Please use a more secure password."
+              same_as_username: "is the same as your username. Please use a more secure password."
+              same_as_email: "is the same as your email. Please use a more secure password."
+              same_as_current: "is the same as your current password."
+              same_as_name: "is the same as your name."
+              unique_characters: "has too many repeated characters. Please use a more secure password."
         user_profile:
           attributes:
             featured_topic_id:

db/migrate/20240903024211_add_trigger_to_users_to_sync_user_passwords.rb

@@ -0,0 +1,67 @@
+# frozen_string_literal: true
+class AddTriggerToUsersToSyncUserPasswords < ActiveRecord::Migration[7.1]
+  def up
+    # necessary for postgres < v14 which does not have CREATE OR REPLACE TRIGGER
+    execute <<~SQL.squish
+      DROP TRIGGER IF EXISTS
+      users_password_sync ON users;
+    SQL
+
+    execute <<~SQL.squish
+    CREATE OR REPLACE FUNCTION mirror_user_password_data() RETURNS TRIGGER AS $$
+    BEGIN
+      INSERT INTO user_passwords (user_id, password_hash, password_salt, password_algorithm, password_expired_at, created_at, updated_at)
+      VALUES (NEW.id, NEW.password_hash, NEW.salt, NEW.password_algorithm, NULL, now(), now())
+      ON CONFLICT(user_id)
+      DO UPDATE SET
+        password_hash = EXCLUDED.password_hash,
+        password_salt = EXCLUDED.password_salt,
+        password_algorithm = EXCLUDED.password_algorithm,
+        password_expired_at = EXCLUDED.password_expired_at,
+        updated_at = now()
+        WHERE user_passwords.password_hash <> EXCLUDED.password_hash;
+      RETURN NEW;
+    END;
+    $$ LANGUAGE plpgsql;
+    SQL
+
+    execute <<~SQL.squish
+      CREATE TRIGGER users_password_sync
+      AFTER INSERT OR UPDATE OF password_hash ON users
+      FOR EACH ROW
+      WHEN (NEW.password_hash IS NOT NULL)
+      EXECUTE PROCEDURE mirror_user_password_data();
+    SQL
+
+    execute <<~SQL.squish
+      DROP TRIGGER IF EXISTS
+      users_password_sync_on_delete_password ON users;
+    SQL
+
+    execute <<~SQL.squish
+    CREATE OR REPLACE FUNCTION delete_user_password() RETURNS TRIGGER AS $$
+    BEGIN
+      DELETE FROM user_passwords WHERE user_id = NEW.id;
+      RETURN NEW;
+    END;
+    $$ LANGUAGE plpgsql;
+    SQL
+
+    execute <<~SQL.squish
+      CREATE TRIGGER users_password_sync_on_delete_password
+      AFTER UPDATE OF password_hash ON users
+      FOR EACH ROW
+      WHEN (NEW.password_hash IS NULL)
+      EXECUTE PROCEDURE delete_user_password();
+    SQL
+  end
+
+  def down
+    execute <<~SQL.squish
+      DROP TRIGGER IF EXISTS users_password_sync_on_delete_password ON users;
+      DROP FUNCTION IF EXISTS delete_user_password;
+      DROP TRIGGER IF EXISTS users_password_sync ON users;
+      DROP FUNCTION IF EXISTS mirror_user_password_data;
+    SQL
+  end
+end

db/migrate/20240903024311_backfill_user_passwords_from_users.rb

@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+class BackfillUserPasswordsFromUsers < ActiveRecord::Migration[7.1]
+  disable_ddl_transaction!
+  BATCH_SIZE = 50_000
+
+  def up
+    min_id, max_id = DB.query_single(<<~SQL.squish)
+      SELECT MIN(id), MAX(id)
+      FROM users
+      WHERE password_hash IS NOT NULL AND salt IS NOT NULL AND password_algorithm IS NOT NULL;
+    SQL
+
+    return if max_id.nil?
+
+    (min_id..max_id).step(BATCH_SIZE) { |start_id| execute <<~SQL }
+      INSERT INTO user_passwords (user_id, password_hash, password_salt, password_algorithm, password_expired_at, created_at, updated_at)
+      SELECT id, password_hash, salt, password_algorithm, NULL, now(), now()
+      FROM users
+      WHERE id >= #{start_id} AND id < #{start_id + BATCH_SIZE} AND password_hash IS NOT NULL AND salt IS NOT NULL AND password_algorithm IS NOT NULL
+      ON CONFLICT (user_id) DO UPDATE SET
+        password_hash = EXCLUDED.password_hash,
+        password_salt = EXCLUDED.password_salt,
+        password_algorithm = EXCLUDED.password_algorithm,
+        password_expired_at = EXCLUDED.password_expired_at,
+        updated_at = now()
+      WHERE user_passwords.password_hash <> EXCLUDED.password_hash
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

db/post_migrate/20240910090759_make_password_columns_from_users_read_only.rb

@@ -0,0 +1,20 @@
+# frozen_string_literal: true
+class MakePasswordColumnsFromUsersReadOnly < ActiveRecord::Migration[7.1]
+  def up
+    # remove invalid triggers/functions dependent on the columns to be dropped
+    execute <<~SQL.squish
+      DROP TRIGGER IF EXISTS users_password_sync_on_delete_password ON users;
+      DROP FUNCTION IF EXISTS delete_user_password;
+      DROP TRIGGER IF EXISTS users_password_sync ON users;
+      DROP FUNCTION IF EXISTS mirror_user_password_data;
+    SQL
+
+    %i[password_hash salt password_algorithm].each do |column|
+      Migration::ColumnDropper.mark_readonly(:users, column)
+    end
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

lib/validators/password_validator.rb

@@ -4,32 +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) if value.blank?
-      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 record.password_expired?(value)
-      record.errors.add(attribute, :same_as_previous)
-    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)
   end
 end

lib/validators/user_password_validator.rb

@@ -0,0 +1,36 @@
+# frozen_string_literal: true
+
+class UserPasswordValidator < ActiveModel::EachValidator
+  def validate_each(record, attribute, value)
+    return unless record.password_validation_required?
+    user = record.user
+
+    if value.nil?
+      record.errors.add(attribute, :blank)
+    elsif value.length < SiteSetting.min_admin_password_length &&
+          (user.admin? || is_developer?(user.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 user.username.present? && value == user.username
+      record.errors.add(attribute, :same_as_username)
+    elsif user.name.present? && value == user.name
+      record.errors.add(attribute, :same_as_name)
+    elsif user.email.present? && value == user.email
+      record.errors.add(attribute, :same_as_email)
+    elsif record.password_hash_changed? && 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
+
+  private
+
+  def is_developer?(value)
+    Rails.configuration.respond_to?(:developer_emails) &&
+      Rails.configuration.developer_emails.include?(value)
+  end
+end

spec/fabricators/user_password_fabricator.rb

@@ -3,11 +3,12 @@
 Fabricator(:user_password) do
   transient password: "myawesomefakepassword"
 
-  user { Fabricate(:user) }
+  user { Fabricate(:user, password: nil) }
-  password_salt { SecureRandom.hex(User::PASSWORD_SALT_LENGTH) }
+  password_salt { SecureRandom.hex(UserPassword::PASSWORD_SALT_LENGTH) }
-  password_algorithm { User::TARGET_PASSWORD_ALGORITHM }
+  password_algorithm { UserPassword::TARGET_PASSWORD_ALGORITHM }
 
   after_build do |user_password, transients|
+    if transients[:password].present?
       user_password.password_hash =
         PasswordHasher.hash_password(
           password: transients[:password],
@@ -16,5 +17,6 @@ Fabricator(:user_password) do
         )
     end
   end
+end
 
 Fabricator(:expired_user_password, from: :user_password) { password_expired_at { 1.day.ago } }

spec/lib/validators/password_validator_spec.rb

@@ -1,10 +1,6 @@
 # frozen_string_literal: true
 
 RSpec.describe PasswordValidator do
-  def password_error_message(key)
-    I18n.t("activerecord.errors.models.user.attributes.password.#{key}")
-  end
-
   subject(:validate) { validator.validate_each(record, :password, @password) }
 
   let(:validator) { described_class.new(attributes: :password) }
@@ -16,24 +12,6 @@ RSpec.describe PasswordValidator do
       u
     end
 
-    context "when password is not common" do
-      before { CommonPasswords.stubs(:common_password?).returns(false) }
-
-      context "when min password length is 8" do
-        before { SiteSetting.min_password_length = 8 }
-
-        it "doesn't add an error when password is good" do
-          @password = "weron235alsfn234"
-          validate
-          expect(record.errors[:password]).not_to be_present
-        end
-
-        it "adds an error when password is too short" do
-          @password = "p"
-          validate
-          expect(record.errors[:password]).to be_present
-        end
-
     it "adds an error when password is blank" do
       @password = ""
       validate
@@ -46,124 +24,6 @@ RSpec.describe PasswordValidator do
       expect(record.errors[:password]).to be_present
     end
 
-        it "adds an error when user is admin and password is less than 15 chars" do
-          SiteSetting.min_admin_password_length = 15
-
-          @password = "12345678912"
-          record.admin = true
-          validate
-          expect(record.errors[:password]).to be_present
-        end
-      end
-
-      context "when min password length is 12" do
-        before { SiteSetting.min_password_length = 12 }
-
-        it "adds an error when password length is 11" do
-          @password = "gt38sdt92bv"
-          validate
-          expect(record.errors[:password]).to be_present
-        end
-      end
-    end
-
-    context "when password is commonly used" do
-      before do
-        SiteSetting.min_password_length = 8
-        CommonPasswords.stubs(:common_password?).returns(true)
-      end
-
-      it "adds an error when block_common_passwords is enabled" do
-        SiteSetting.block_common_passwords = true
-        @password = "password"
-        validate
-        expect(record.errors[:password]).to include(password_error_message(:common))
-      end
-
-      it "doesn't add an error when block_common_passwords is disabled" do
-        SiteSetting.block_common_passwords = false
-        @password = "password"
-        validate
-        expect(record.errors[:password]).not_to be_present
-      end
-    end
-
-    context "when password_unique_characters is 5" do
-      before { SiteSetting.password_unique_characters = 5 }
-
-      it "adds an error when there are too few unique characters" do
-        SiteSetting.password_unique_characters = 6
-        @password = "aaaaaa5432"
-        validate
-        expect(record.errors[:password]).to include(password_error_message(:unique_characters))
-      end
-
-      it "doesn't add an error when there are enough unique characters" do
-        @password = "aaaaa12345"
-        validate
-        expect(record.errors[:password]).not_to be_present
-      end
-
-      it "counts capital letters as different" do
-        @password = "aaaAaa1234"
-        validate
-        expect(record.errors[:password]).not_to be_present
-      end
-    end
-
-    it "adds an error when password is the same as the username" do
-      @password = "porkchops1234"
-      record.username = @password
-      validate
-      expect(record.errors[:password]).to include(password_error_message(:same_as_username))
-    end
-
-    it "adds an error when password is the same as the name" do
-      @password = "myawesomepassword"
-      record.name = @password
-      validate
-      expect(record.errors[:password]).to include(password_error_message(:same_as_name))
-    end
-
-    it "adds an error when password is the same as the email" do
-      @password = "pork@chops.com"
-      record.email = @password
-      validate
-      expect(record.errors[:password]).to include(password_error_message(:same_as_email))
-    end
-
-    it "adds an error when new password is same as current password" do
-      @password = "mypetsname"
-      record.save!
-      record.reload
-      record.password = @password
-      validate
-      expect(record.errors[:password]).to include(password_error_message(:same_as_current))
-    end
-
-    it "adds an error when new password is same as a previous password" do
-      @password = "thisisaoldpassword"
-      record.save!
-      record.reload
-
-      new_password = "thisisanewpassword"
-
-      _expired_user_password =
-        Fabricate(
-          :expired_user_password,
-          password: new_password,
-          password_algorithm: record.password_algorithm,
-          password_salt: record.salt,
-          user: record,
-        )
-
-      record.password = new_password
-      @password = new_password
-      validate
-
-      expect(record.errors[:password]).to include(password_error_message(:same_as_previous))
-    end
-
     it "validation required if password is required" do
       expect(record.password_validation_required?).to eq(true)
     end
@@ -191,11 +51,5 @@ RSpec.describe PasswordValidator do
       @password = "mygameshow"
       expect(record.password_validation_required?).to eq(true)
     end
-
-    it "adds an error even password not required" do
-      @password = "p"
-      validate
-      expect(record.errors[:password]).to be_present
-    end
   end
 end

spec/lib/validators/user_password_validator_spec.rb

@@ -0,0 +1,129 @@
+# frozen_string_literal: true
+
+RSpec.describe UserPasswordValidator do
+  def password_error_message(key)
+    I18n.t("activerecord.errors.models.user_password.attributes.password.#{key}")
+  end
+
+  subject(:validate) { validator.validate_each(record, :password, @password) }
+
+  let(:validator) { described_class.new(attributes: :password) }
+
+  # fabrication doesn't work here as it somehow bypasses the password= setter logic
+  let(:record) do
+    UserPassword.build(password: @password, user: Fabricate.build(:user, password: nil))
+  end
+
+  context "when password is not common" do
+    before { CommonPasswords.stubs(:common_password?).returns(false) }
+
+    context "when min password length is 8" do
+      before { SiteSetting.min_password_length = 8 }
+
+      it "doesn't add an error when password is good" do
+        @password = "weron235alsfn234"
+        validate
+        expect(record.errors[:password]).not_to be_present
+      end
+
+      it "adds an error when password is too short" do
+        @password = "p"
+        validate
+        expect(record.errors[:password]).to be_present
+      end
+
+      it "adds an error when user is admin and password is less than 15 chars" do
+        SiteSetting.min_admin_password_length = 15
+
+        @password = "12345678912"
+        record.user.admin = true
+        validate
+        expect(record.errors[:password]).to be_present
+      end
+    end
+
+    context "when min password length is 12" do
+      before { SiteSetting.min_password_length = 12 }
+
+      it "adds an error when password length is 11" do
+        @password = "gt38sdt92bv"
+        validate
+        expect(record.errors[:password]).to be_present
+      end
+    end
+  end
+  context "when password is commonly used" do
+    before do
+      SiteSetting.min_password_length = 8
+      CommonPasswords.stubs(:common_password?).returns(true)
+    end
+
+    it "adds an error when block_common_passwords is enabled" do
+      SiteSetting.block_common_passwords = true
+      @password = "password"
+      validate
+      expect(record.errors[:password]).to include(password_error_message(:common))
+    end
+
+    it "doesn't add an error when block_common_passwords is disabled" do
+      SiteSetting.block_common_passwords = false
+      @password = "password"
+      validate
+      expect(record.errors[:password]).not_to be_present
+    end
+  end
+
+  context "when password_unique_characters is 5" do
+    before { SiteSetting.password_unique_characters = 5 }
+
+    it "adds an error when there are too few unique characters" do
+      SiteSetting.password_unique_characters = 6
+      @password = "aaaaaa5432"
+      validate
+      expect(record.errors[:password]).to include(password_error_message(:unique_characters))
+    end
+
+    it "doesn't add an error when there are enough unique characters" do
+      @password = "aaaaa12345"
+      validate
+      expect(record.errors[:password]).not_to be_present
+    end
+
+    it "counts capital letters as different" do
+      @password = "aaaAaa1234"
+      validate
+      expect(record.errors[:password]).not_to be_present
+    end
+  end
+
+  it "adds an error when password is the same as the username" do
+    @password = "porkchops1234"
+    record.user.username = @password
+    validate
+    expect(record.errors[:password]).to include(password_error_message(:same_as_username))
+  end
+
+  it "adds an error when password is the same as the name" do
+    @password = "myawesomepassword"
+    record.user.name = @password
+    validate
+    expect(record.errors[:password]).to include(password_error_message(:same_as_name))
+  end
+
+  it "adds an error when password is the same as the email" do
+    @password = "pork@chops.com"
+    record.user.email = @password
+    validate
+    expect(record.errors[:password]).to include(password_error_message(:same_as_email))
+  end
+
+  it "adds an error when new password is same as current password" do
+    @password = "mypetsname"
+    record.save!
+    record.reload
+    record.password = @password
+    validate
+
+    expect(record.errors[:password]).to include(password_error_message(:same_as_current))
+  end
+end

spec/models/invite_redeemer_spec.rb

@@ -150,7 +150,7 @@ RSpec.describe InviteRedeemer do
         error = e
       end
       expect(error).to be_present
-      expect(error.record.errors[:password]).to be_present
+      expect(error.record.errors.errors[0].attribute).to eq :"user_password.password"
     end
 
     it "should unstage user" do

spec/models/user_password_spec.rb

@@ -1,97 +1,71 @@
 # frozen_string_literal: true
 
 RSpec.describe UserPassword do
+  describe "#confirm_password?" do
+    context "when input password is same as saved password" do
+      let(:pw) { SecureRandom.hex }
+
+      it "returns true after saving password for the first time" do
+        u = Fabricate(:user, password: nil)
+        u.password = pw
+        u.save!
+        expect(u.confirm_password?(pw)).to eq true
+      end
+
+      it "returns true after updating existing password" do
+        pw = SecureRandom.hex
+        u = Fabricate(:user, password: "initial_password_123")
+        u.update!(password: pw)
+        expect(u.confirm_password?(pw)).to eq true
+      end
+
+      it "updates the algorithm if it's outdated and password_hash, and returns true" do
+        user_password = Fabricate(:user, password: pw).user_password
+        old_algorithm = "$pbkdf2-sha256$i=5,l=32$"
+        old_hash =
+          described_class.new.send(:hash_password, pw, user_password.password_salt, old_algorithm)
+        user_password.update_columns(password_algorithm: old_algorithm, password_hash: old_hash)
+
+        result = nil
+        expect { result = user_password.confirm_password?(pw) }.to change {
+          user_password.password_algorithm
+        }.from(old_algorithm).to(described_class::TARGET_PASSWORD_ALGORITHM)
+        expect(result).to eq true
+
+        new_hash =
+          described_class.new.send(
+            :hash_password,
+            pw,
+            user_password.password_salt,
+            user_password.password_algorithm,
+          )
+        expect(user_password.password_hash).to eq(new_hash)
+      end
+    end
+
+    context "when input password is not the same as saved password" do
+      let(:actual_pw) { SecureRandom.hex }
+
+      it "returns false" do
+        u = Fabricate(:user, password: actual_pw)
+        expect(u.confirm_password?(SecureRandom.hex)).to eq false
+      end
+    end
+
+    context "when used on an unpersisted record" do
+      it "returns false" do
+        user_password = Fabricate.build(:user_password, user: nil)
+        expect(user_password.confirm_password?(user_password.password)).to eq false
+      end
+    end
+  end
+
   context "for validations" do
     it "should validate presence of user_id" do
-      user_password = Fabricate.build(:user_password, user_id: nil)
+      user_password = Fabricate.build(:user_password, user: nil)
 
       expect(user_password).not_to be_valid
-      expect(user_password.errors[:user_id]).to include("can't be blank")
+      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 uniqueness of user_id scoped to password_expired_at" do
-      user = Fabricate(:user)
-      user_password_1 = Fabricate.create(:user_password, user:, password_expired_at: nil)
-
-      user_password_2 =
-        Fabricate.build(:user_password, user: user_password_1.user, password_expired_at: nil)
-
-      expect(user_password_2).not_to be_valid
-      expect(user_password_2.errors[:user_id]).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/models/user_spec.rb

@@ -1909,7 +1909,7 @@ RSpec.describe User do
   describe "hash_passwords" do
     let(:too_long) { "x" * (User.max_password_length + 1) }
 
-    def hash(password, salt, algorithm = User::TARGET_PASSWORD_ALGORITHM)
+    def hash(password, salt, algorithm = UserPassword::TARGET_PASSWORD_ALGORITHM)
       User.new.send(:hash_password, password, salt, algorithm)
     end
 
@@ -1930,17 +1930,17 @@ RSpec.describe User do
     end
 
     it "uses the target algorithm for new users" do
-      expect(user.password_algorithm).to eq(User::TARGET_PASSWORD_ALGORITHM)
+      expect(user.password_algorithm).to eq(UserPassword::TARGET_PASSWORD_ALGORITHM)
     end
 
     it "can use an older algorithm to verify existing passwords, then upgrade" do
       old_algorithm = "$pbkdf2-sha256$i=5,l=32$"
-      expect(old_algorithm).not_to eq(User::TARGET_PASSWORD_ALGORITHM)
+      expect(old_algorithm).not_to eq(UserPassword::TARGET_PASSWORD_ALGORITHM)
 
       password = "poutine"
       old_hash = hash(password, user.salt, old_algorithm)
 
-      user.update!(password_algorithm: old_algorithm, password_hash: old_hash)
+      user.user_password.update_columns(password_algorithm: old_algorithm, password_hash: old_hash)
 
       expect(user.password_algorithm).to eq(old_algorithm)
       expect(user.password_hash).to eq(old_hash)
@@ -1954,13 +1954,13 @@ RSpec.describe User do
       expect(user.confirm_password?(password)).to eq(true)
 
       # Auto-upgrades to new algorithm
-      expected_new_hash = hash(password, user.salt, User::TARGET_PASSWORD_ALGORITHM)
+      expected_new_hash = hash(password, user.salt, UserPassword::TARGET_PASSWORD_ALGORITHM)
-      expect(user.password_algorithm).to eq(User::TARGET_PASSWORD_ALGORITHM)
+      expect(user.password_algorithm).to eq(UserPassword::TARGET_PASSWORD_ALGORITHM)
       expect(user.password_hash).to eq(expected_new_hash)
 
       # And persists to the db
       user.reload
-      expect(user.password_algorithm).to eq(User::TARGET_PASSWORD_ALGORITHM)
+      expect(user.password_algorithm).to eq(UserPassword::TARGET_PASSWORD_ALGORITHM)
       expect(user.password_hash).to eq(expected_new_hash)
 
       # And can still log in

spec/requests/session_controller_spec.rb

@@ -2023,19 +2023,10 @@ RSpec.describe SessionController do
       end
 
       describe "when user's password has been marked as expired" do
-        let!(:expired_user_password) do
-          Fabricate(
-            :expired_user_password,
-            user:,
-            password: "myawesomepassword",
-            password_salt: user.salt,
-            password_algorithm: user.password_algorithm,
-          )
-        end
-
         before { RateLimiter.enable }
 
         it "should return an error response code with the right error message" do
+          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

@@ -5,19 +5,6 @@ RSpec.describe UserPasswordExpirer do
   fab!(:user) { Fabricate(:user, password:) }
 
   describe ".expire_user_password" do
-    it "should create a new UserPassword record with the user's current password information" do
-      freeze_time
-
-      expect { described_class.expire_user_password(user) }.to change(UserPassword, :count).by 1
-
-      user_password = user.reload.user_password
-
-      expect(user_password.password_hash).to eq(user.password_hash)
-      expect(user_password.password_salt).to eq(user.salt)
-      expect(user_password.password_algorithm).to eq(user.password_algorithm)
-      expect(user_password.password_expired_at).to eq_time(Time.zone.now)
-    end
-
     it "should update `UserPassword#password_expired_at` if the user already has an existing UserPassword record with the same password hash, salt and algorithm" do
       freeze_time(1.hour.ago) do
         described_class.expire_user_password(user)

spec/system/login_spec.rb

@@ -81,7 +81,7 @@ shared_examples "login scenarios" do
     it "displays the right message when user's email has been marked as expired" do
       password = "myawesomepassword"
       user.update!(password:)
-      Fabricate(:expired_user_password, user:, password:)
+      UserPasswordExpirer.expire_user_password(user)
 
       login_modal.open
       login_modal.fill(username: user.username, password:)