diff --git a/app/models/user.rb b/app/models/user.rb index 88393c76a58..4b930f1ead3 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -83,7 +83,7 @@ class User < ActiveRecord::Base dependent: :destroy has_one :invited_user, dependent: :destroy has_one :user_notification_schedule, dependent: :destroy - has_many :passwords, class_name: "UserPassword", dependent: :destroy + has_one :user_password, class_name: "UserPassword", dependent: :destroy, autosave: true # delete all is faster but bypasses callbacks has_many :bookmarks, dependent: :delete_all @@ -954,12 +954,9 @@ class User < ActiveRecord::Base end def password_expired?(password) - passwords - .where("password_expired_at IS NOT NULL AND password_expired_at < ?", Time.zone.now) - .any? do |user_password| - user_password.password_hash == - hash_password(password, user_password.password_salt, user_password.password_algorithm) - end + return false if user_password.nil? || user_password.password_expired_at.nil? + user_password.password_hash == + hash_password(password, user_password.password_salt, user_password.password_algorithm) end def confirm_password?(password) diff --git a/app/models/user_password.rb b/app/models/user_password.rb index 80baa199e32..3efd007ac12 100644 --- a/app/models/user_password.rb +++ b/app/models/user_password.rb @@ -3,12 +3,7 @@ class UserPassword < ActiveRecord::Base validates :user_id, presence: true - validates :user_id, - uniqueness: { - scope: :password_expired_at, - }, - if: -> { password_expired_at.nil? } - + 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 } @@ -31,7 +26,5 @@ end # # Indexes # -# idx_user_passwords_on_user_id_and_expired_at_and_hash (user_id,password_expired_at,password_hash) -# index_user_passwords_on_user_id (user_id) UNIQUE WHERE (password_expired_at IS NULL) -# index_user_passwords_on_user_id_and_password_hash (user_id,password_hash) UNIQUE +# index_user_passwords_on_user_id (user_id) UNIQUE # diff --git a/app/services/user_password_expirer.rb b/app/services/user_password_expirer.rb index 4a1944932ce..7c3d7a2c812 100644 --- a/app/services/user_password_expirer.rb +++ b/app/services/user_password_expirer.rb @@ -3,13 +3,13 @@ class UserPasswordExpirer def self.expire_user_password(user) UserPassword - .where( - user:, + .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, ) - .first_or_initialize - .update!(password_expired_at: Time.zone.now) end end diff --git a/db/migrate/20240818113758_remove_all_but_most_recent_user_password.rb b/db/migrate/20240818113758_remove_all_but_most_recent_user_password.rb new file mode 100644 index 00000000000..f218891bcb2 --- /dev/null +++ b/db/migrate/20240818113758_remove_all_but_most_recent_user_password.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true +class RemoveAllButMostRecentUserPassword < ActiveRecord::Migration[7.1] + def up + execute <<~SQL.squish + DELETE FROM user_passwords + WHERE id NOT IN ( + SELECT DISTINCT ON (user_id) id + FROM user_passwords + ORDER BY user_id, password_expired_at DESC NULLS FIRST + ); + SQL + end + + def down + end +end diff --git a/db/migrate/20240819130737_update_unique_index_on_user_passwords.rb b/db/migrate/20240819130737_update_unique_index_on_user_passwords.rb new file mode 100644 index 00000000000..57f1c1ee6ae --- /dev/null +++ b/db/migrate/20240819130737_update_unique_index_on_user_passwords.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true +class UpdateUniqueIndexOnUserPasswords < ActiveRecord::Migration[7.1] + disable_ddl_transaction! + + def change + remove_index :user_passwords, + %i[user_id], + unique: true, + where: "password_expired_at IS NULL", + algorithm: :concurrently, + if_exists: true + + add_index :user_passwords, + %i[user_id], + unique: true, + algorithm: :concurrently, + if_not_exists: true + end +end diff --git a/db/migrate/20240903024157_remove_user_passwords_indexes.rb b/db/migrate/20240903024157_remove_user_passwords_indexes.rb new file mode 100644 index 00000000000..a2b460f7c24 --- /dev/null +++ b/db/migrate/20240903024157_remove_user_passwords_indexes.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true +class RemoveUserPasswordsIndexes < ActiveRecord::Migration[7.1] + def change + remove_index :user_passwords, %i[user_id password_hash], unique: true + + remove_index :user_passwords, + %i[user_id password_expired_at password_hash], + name: "idx_user_passwords_on_user_id_and_expired_at_and_hash" + end +end diff --git a/spec/services/user_password_expirer_spec.rb b/spec/services/user_password_expirer_spec.rb index 9498c853aac..d5fb3e36610 100644 --- a/spec/services/user_password_expirer_spec.rb +++ b/spec/services/user_password_expirer_spec.rb @@ -8,11 +8,9 @@ RSpec.describe UserPasswordExpirer do it "should create a new UserPassword record with the user's current password information" do freeze_time - described_class.expire_user_password(user) + expect { described_class.expire_user_password(user) }.to change(UserPassword, :count).by 1 - expect(user.passwords.count).to eq(1) - - user_password = user.passwords.first + 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) @@ -24,18 +22,39 @@ RSpec.describe UserPasswordExpirer do freeze_time(1.hour.ago) do described_class.expire_user_password(user) - user_password = user.passwords.first - - expect(user_password.password_expired_at).to eq_time(Time.zone.now) + expect(user.reload.user_password.password_expired_at).to eq_time(Time.zone.now) end freeze_time do + expect { described_class.expire_user_password(user) }.not_to change(UserPassword, :count) + + user_password = user.user_password.reload + + 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 + end + + it "updates UserPassword attributes if user already has an existing UserPassword record which has a different password_hash" do + new_password = password + "_new" + old_password_hash = user.password_hash + + freeze_time(1.hour.ago) do described_class.expire_user_password(user) - expect(user.passwords.count).to eq(1) + expect(user.user_password.password_hash).to eq(old_password_hash) + expect(user.user_password.password_expired_at).to eq_time(Time.zone.now) + end - user_password = user.passwords.first + freeze_time do + user.update!(password: new_password) + expect { described_class.expire_user_password(user) }.not_to change(UserPassword, :count) + user_password = user.user_password.reload + + expect(user_password.password_hash).not_to eq(old_password_hash) 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)