FIX: don't allow username to be changed to same as password

approved
#1

FIX: don’t allow username to be changed to same as password

We were blocking user registrations with same username and password, but allowing usernames to be changed to be same as password later. Also disallow names to be the same as password.

diff --git a/app/models/user.rb b/app/models/user.rb
index 7db7e26..7abdae8 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -106,6 +106,7 @@ class User < ActiveRecord::Base
   validates_presence_of :username
   validate :username_validator, if: :will_save_change_to_username?
   validate :password_validator
+  validate :name_validator, if: :will_save_change_to_name?
   validates :name, user_full_name: true, if: :will_save_change_to_name?, length: { maximum: 255 }
   validates :ip_address, allowed_ip_address: { on: :create, message: :signup_not_allowed }
   validates :primary_email, presence: true
@@ -1333,20 +1334,34 @@ class User < ActiveRecord::Base
 
   def username_validator
     username_format_validator || begin
-      existing = DB.query(
-        USERNAME_EXISTS_SQL,
-        username: self.class.normalize_username(username)
-      )
+      if will_save_change_to_username?
+        existing = DB.query(
+          USERNAME_EXISTS_SQL,
+          username: self.class.normalize_username(username)
+        )
+
+        user_id = existing.select { |u| u.is_user }.first&.id
+        same_user = user_id && user_id == self.id
 
-      user_id = existing.select { |u| u.is_user }.first&.id
-      same_user = user_id && user_id == self.id
+        if existing.present? && !same_user
+          errors.add(:username, I18n.t(:'user.username.unique'))
+        end
 
-      if will_save_change_to_username? && existing.present? && !same_user
-        errors.add(:username, I18n.t(:'user.username.unique'))
+        if confirm_password?(username) || confirm_password?(username.downcase)
+          errors.add(:username, :same_as_password)
+        end
       end
     end
   end
 
+  def name_validator
+    if name.present? &&
+      (confirm_password?(name) || confirm_password?(name&.downcase))
+
+      errors.add(:name, :same_as_password)
+    end
+  end
+
   def set_default_categories_preferences
     return if self.staged?
 
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 05bb8a5..dca7552 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -491,7 +491,12 @@ 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_name: "is the same as your name."
               unique_characters: "has too many repeated characters. Please use a more secure password."
+            username:
+              same_as_password: "is the same as your password."
+            name:
+              same_as_password: "is the same as your password."
             ip_address:
               signup_not_allowed: "Signup is not allowed from this account."
         user_email:
diff --git a/lib/validators/password_validator.rb b/lib/validators/password_validator.rb
index 45a4dab..1b0f937 100644
--- a/lib/validators/password_validator.rb
+++ b/lib/validators/password_validator.rb
@@ -15,6 +15,8 @@ class PasswordValidator < ActiveModel::EachValidator
       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)
diff --git a/spec/components/validators/password_validator_spec.rb b/spec/components/validators/password_validator_spec.rb
index 571a6e1..c4cf642 100644
--- a/spec/components/validators/password_validator_spec.rb
+++ b/spec/components/validators/password_validator_spec.rb
@@ -121,6 +121,13 @@ describe PasswordValidator do
       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
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 19a4bb0..08a736b 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -6,6 +6,10 @@ require_dependency 'user'
 describe User do
   let(:user) { Fabricate(:user) }
 
+  def user_error_message(*keys)
+    I18n.t(:"activerecord.errors.models.user.attributes.#{keys.join('.')}")
+  end
+
   context 'validations' do
     describe '#username' do
       it { is_expected.to validate_presence_of :username }
@@ -32,6 +36,36 @@ describe User do
             .to include(I18n.t(:'user.username.unique'))
         end
       end
+
+      it 'is not valid if username changes to be same as password' do
+        user.username = 'myawesomepassword'
+        expect(user).to_not be_valid
+        expect(user.errors.full_messages.first)
+          .to include(user_error_message(:username, :same_as_password))
+      end
+
+      it 'is not valid if username lowercase changes to be same as password' do
+        user.username = 'MyAwesomePassword'
+        expect(user).to_not be_valid
+        expect(user.errors.full_messages.first)
+          .to include(user_error_message(:username, :same_as_password))
+      end
+    end
+
+    describe 'name' do
+      it 'is not valid if it changes to be the same as the password' do
+        user.name = 'myawesomepassword'
+        expect(user).to_not be_valid
+        expect(user.errors.full_messages.first)
+          .to include(user_error_message(:name, :same_as_password))
+      end
+
+      it 'is not valid if name lowercase changes to be the same as the password' do
+        user.name = 'MyAwesomePassword'
+        expect(user).to_not be_valid
+        expect(user.errors.full_messages.first)
+          .to include(user_error_message(:name, :same_as_password))
+      end
     end
 
     describe 'emails' do

GitHub sha: 6f747c6b

1 Like
Approved #2
#3

This commit has been mentioned on Discourse Meta. There might be relevant details there:

#4

I like that you did this! 1 less query lots of the time

2 Likes