From 32e261ef73dedcff4a28728e0368f791a6ef4144 Mon Sep 17 00:00:00 2001 From: Kelv Date: Thu, 10 Oct 2024 09:23:06 +0800 Subject: [PATCH] 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 | 88 +++++----- app/models/user_password.rb | 71 +++++++- app/services/user_password_expirer.rb | 10 +- config/locales/server.en.yml | 10 +- ...trigger_to_users_to_sync_user_passwords.rb | 67 ++++++++ ...4311_backfill_user_passwords_from_users.rb | 33 ++++ ...e_password_columns_from_users_read_only.rb | 20 +++ lib/validators/password_validator.rb | 28 +--- lib/validators/user_password_validator.rb | 36 ++++ spec/fabricators/user_password_fabricator.rb | 20 ++- .../lib/validators/password_validator_spec.rb | 158 +----------------- .../user_password_validator_spec.rb | 129 ++++++++++++++ spec/models/invite_redeemer_spec.rb | 2 +- spec/models/user_password_spec.rb | 148 +++++++--------- spec/models/user_spec.rb | 14 +- spec/requests/session_controller_spec.rb | 11 +- spec/services/user_password_expirer_spec.rb | 13 -- spec/system/login_spec.rb | 2 +- 18 files changed, 496 insertions(+), 364 deletions(-) create mode 100644 db/migrate/20240903024211_add_trigger_to_users_to_sync_user_passwords.rb create mode 100644 db/migrate/20240903024311_backfill_user_passwords_from_users.rb create mode 100644 db/post_migrate/20240910090759_make_password_columns_from_users_read_only.rb create mode 100644 lib/validators/user_password_validator.rb create mode 100644 spec/lib/validators/user_password_validator_spec.rb diff --git a/app/models/user.rb b/app/models/user.rb index 29e47a444be..661dd9cf6bb 100644 --- a/app/models/user.rb +++ b/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 - confirmed = self.password_hash == hash_password(password, salt, password_algorithm) - - 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 + return false if !user_password + user_password.confirm_password?(password) 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 # diff --git a/app/models/user_password.rb b/app/models/user_password.rb index 3efd007ac12..bf5b3b5ad7c 100644 --- a/app/models/user_password.rb +++ b/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 } - 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 - 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 diff --git a/app/services/user_password_expirer.rb b/app/services/user_password_expirer.rb index 7c3d7a2c812..f73beae074e 100644 --- a/app/services/user_password_expirer.rb +++ b/app/services/user_password_expirer.rb @@ -2,14 +2,6 @@ class UserPasswordExpirer def self.expire_user_password(user) - UserPassword - .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, - ) + user.user_password&.update!(password_expired_at: Time.zone.now) end end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 2b4c8487639..49e77e75b0b 100644 --- a/config/locales/server.en.yml +++ b/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: diff --git a/db/migrate/20240903024211_add_trigger_to_users_to_sync_user_passwords.rb b/db/migrate/20240903024211_add_trigger_to_users_to_sync_user_passwords.rb new file mode 100644 index 00000000000..917875e31d7 --- /dev/null +++ b/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 diff --git a/db/migrate/20240903024311_backfill_user_passwords_from_users.rb b/db/migrate/20240903024311_backfill_user_passwords_from_users.rb new file mode 100644 index 00000000000..bb6c14a9a03 --- /dev/null +++ b/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 diff --git a/db/post_migrate/20240910090759_make_password_columns_from_users_read_only.rb b/db/post_migrate/20240910090759_make_password_columns_from_users_read_only.rb new file mode 100644 index 00000000000..a3d28de5d98 --- /dev/null +++ b/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 diff --git a/lib/validators/password_validator.rb b/lib/validators/password_validator.rb index 763ca656272..53565816326 100644 --- a/lib/validators/password_validator.rb +++ b/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) - 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) + record.errors.add(attribute, :blank) if value.blank? end end diff --git a/lib/validators/user_password_validator.rb b/lib/validators/user_password_validator.rb new file mode 100644 index 00000000000..f68cba6a88c --- /dev/null +++ b/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 diff --git a/spec/fabricators/user_password_fabricator.rb b/spec/fabricators/user_password_fabricator.rb index 017874f36a6..19083cd464d 100644 --- a/spec/fabricators/user_password_fabricator.rb +++ b/spec/fabricators/user_password_fabricator.rb @@ -3,17 +3,19 @@ Fabricator(:user_password) do transient password: "myawesomefakepassword" - user { Fabricate(:user) } - password_salt { SecureRandom.hex(User::PASSWORD_SALT_LENGTH) } - password_algorithm { User::TARGET_PASSWORD_ALGORITHM } + user { Fabricate(:user, password: nil) } + password_salt { SecureRandom.hex(UserPassword::PASSWORD_SALT_LENGTH) } + 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 diff --git a/spec/lib/validators/password_validator_spec.rb b/spec/lib/validators/password_validator_spec.rb index d220735166e..e569dd8ce44 100644 --- a/spec/lib/validators/password_validator_spec.rb +++ b/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,152 +12,16 @@ 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 - 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.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)) - 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)) + expect(record.errors[:password]).to be_present end it "validation required if password is required" do @@ -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 diff --git a/spec/lib/validators/user_password_validator_spec.rb b/spec/lib/validators/user_password_validator_spec.rb new file mode 100644 index 00000000000..24d0259d5ac --- /dev/null +++ b/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 diff --git a/spec/models/invite_redeemer_spec.rb b/spec/models/invite_redeemer_spec.rb index cc69e857932..4481749c00b 100644 --- a/spec/models/invite_redeemer_spec.rb +++ b/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 diff --git a/spec/models/user_password_spec.rb b/spec/models/user_password_spec.rb index aacebb0caec..908a4fc4d7d 100644 --- a/spec/models/user_password_spec.rb +++ b/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") - 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)", - ) + expect(user_password.errors[:user]).to include("must exist") end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 6e50f172259..18f3f97d45c 100644 --- a/spec/models/user_spec.rb +++ b/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) - expect(user.password_algorithm).to eq(User::TARGET_PASSWORD_ALGORITHM) + expected_new_hash = hash(password, user.salt, UserPassword::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 diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index 33e964f7832..933a4450167 100644 --- a/spec/requests/session_controller_spec.rb +++ b/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) diff --git a/spec/services/user_password_expirer_spec.rb b/spec/services/user_password_expirer_spec.rb index d5fb3e36610..f5c7d7695b7 100644 --- a/spec/services/user_password_expirer_spec.rb +++ b/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) diff --git a/spec/system/login_spec.rb b/spec/system/login_spec.rb index d11ca4f39be..8eb901a2c90 100644 --- a/spec/system/login_spec.rb +++ b/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:)