refactor and add user_password_validator_spec

app/models/user.rb

@@ -925,7 +925,6 @@ class User < ActiveRecord::Base
 
     if user_password
       user_password.password = pw
-      user_password.password_hash_will_change!
     else
       build_user_password(password: pw)
     end

app/models/user_password.rb

@@ -19,6 +19,8 @@ class UserPassword < ActiveRecord::Base
 
   def password=(pw)
     return if pw.blank?
+
+    self.password_hash_will_change!
     @raw_password = pw
   end
 

lib/validators/password_validator.rb

@@ -4,6 +4,6 @@ class PasswordValidator < ActiveModel::EachValidator
   def validate_each(record, attribute, value)
     return unless record.password_validation_required?
 
-    record.errors.add(attribute, :blank) if value.nil?
+    record.errors.add(attribute, :blank) if value.blank?
   end
 end

spec/fabricators/user_password_fabricator.rb

@@ -8,12 +8,14 @@ Fabricator(:user_password) do
   password_algorithm { UserPassword::TARGET_PASSWORD_ALGORITHM }
 
   after_build do |user_password, transients|
-    user_password.password_hash =
-      PasswordHasher.hash_password(
-        password: transients[:password],
-        salt: user_password.password_salt,
-        algorithm: user_password.password_algorithm,
-      )
+    if transients[:password].present?
+      user_password.password_hash =
+        PasswordHasher.hash_password(
+          password: transients[:password],
+          salt: user_password.password_salt,
+          algorithm: user_password.password_algorithm,
+        )
+    end
   end
 end
 

spec/lib/validators/password_validator_spec.rb

@@ -1,145 +1,27 @@
 # 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) { UserPasswordValidator.new(attributes: :password) }
+  let(:validator) { described_class.new(attributes: :password) }
 
   describe "password required" do
     let(:record) do
       u = Fabricate.build(:user, password: @password)
       u.password_required!
-      u.user_password
+      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
-          expect(record.errors[:password]).to be_present
-        end
-
-        it "adds an error when password is nil" do
-          @password = nil
-          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.username = @password
+    it "adds an error when password is blank" do
+      @password = ""
       validate
-      expect(record.errors[:password]).to include(password_error_message(:same_as_username))
+      expect(record.errors[:password]).to be_present
     end
 
-    it "adds an error when password is the same as the name" do
-      @password = "myawesomepassword"
-      record.name = @password
+    it "adds an error when password is nil" do
+      @password = nil
       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))
+      expect(record.errors[:password]).to be_present
     end
 
     it "validation required if password is required" do
@@ -169,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