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.
This commit is contained in:
Neil Lalonde 2019-05-13 16:43:19 -04:00
parent 13e54bca3d
commit 6f747c6b71
5 changed files with 71 additions and 8 deletions

View File

@ -106,6 +106,7 @@ class User < ActiveRecord::Base
validates_presence_of :username validates_presence_of :username
validate :username_validator, if: :will_save_change_to_username? validate :username_validator, if: :will_save_change_to_username?
validate :password_validator 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 :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 :ip_address, allowed_ip_address: { on: :create, message: :signup_not_allowed }
validates :primary_email, presence: true validates :primary_email, presence: true
@ -1333,20 +1334,34 @@ class User < ActiveRecord::Base
def username_validator def username_validator
username_format_validator || begin username_format_validator || begin
existing = DB.query( if will_save_change_to_username?
USERNAME_EXISTS_SQL, existing = DB.query(
username: self.class.normalize_username(username) USERNAME_EXISTS_SQL,
) username: self.class.normalize_username(username)
)
user_id = existing.select { |u| u.is_user }.first&.id user_id = existing.select { |u| u.is_user }.first&.id
same_user = user_id && user_id == self.id same_user = user_id && user_id == self.id
if will_save_change_to_username? && existing.present? && !same_user if existing.present? && !same_user
errors.add(:username, I18n.t(:'user.username.unique')) errors.add(:username, I18n.t(:'user.username.unique'))
end
if confirm_password?(username) || confirm_password?(username.downcase)
errors.add(:username, :same_as_password)
end
end end
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 def set_default_categories_preferences
return if self.staged? return if self.staged?

View File

@ -491,7 +491,12 @@ en:
same_as_username: "is the same as your username. 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_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_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." 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: ip_address:
signup_not_allowed: "Signup is not allowed from this account." signup_not_allowed: "Signup is not allowed from this account."
user_email: user_email:

View File

@ -15,6 +15,8 @@ class PasswordValidator < ActiveModel::EachValidator
record.errors.add(attribute, :too_short, count: SiteSetting.min_password_length) record.errors.add(attribute, :too_short, count: SiteSetting.min_password_length)
elsif record.username.present? && value == record.username elsif record.username.present? && value == record.username
record.errors.add(attribute, :same_as_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 elsif record.email.present? && value == record.email
record.errors.add(attribute, :same_as_email) record.errors.add(attribute, :same_as_email)
elsif record.confirm_password?(value) elsif record.confirm_password?(value)

View File

@ -121,6 +121,13 @@ describe PasswordValidator do
expect(record.errors[:password]).to include(password_error_message(:same_as_username)) expect(record.errors[:password]).to include(password_error_message(:same_as_username))
end 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 it "adds an error when password is the same as the email" do
@password = "pork@chops.com" @password = "pork@chops.com"
record.email = @password record.email = @password

View File

@ -6,6 +6,10 @@ require_dependency 'user'
describe User do describe User do
let(:user) { Fabricate(:user) } let(:user) { Fabricate(:user) }
def user_error_message(*keys)
I18n.t(:"activerecord.errors.models.user.attributes.#{keys.join('.')}")
end
context 'validations' do context 'validations' do
describe '#username' do describe '#username' do
it { is_expected.to validate_presence_of :username } it { is_expected.to validate_presence_of :username }
@ -32,6 +36,36 @@ describe User do
.to include(I18n.t(:'user.username.unique')) .to include(I18n.t(:'user.username.unique'))
end end
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 end
describe 'emails' do describe 'emails' do