DEV: make UserPassword 1:1 to User (#28528)

* add data migration to keep only unexpired or most recently expired user password
* refactor to 1:1 relationship between User and UserPassword
* add migration to remove redundant indexes on user passwords

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
+    return false if user_password.nil? || user_password.password_expired_at.nil?
-      .where("password_expired_at IS NOT NULL AND password_expired_at < ?", Time.zone.now)
+    user_password.password_hash ==
-      .any? do |user_password|
+      hash_password(password, user_password.password_salt, user_password.password_algorithm)
-        user_password.password_hash ==
-          hash_password(password, user_password.password_salt, user_password.password_algorithm)
-      end
   end
 
   def confirm_password?(password)

app/models/user_password.rb

@@ -3,12 +3,7 @@
 class UserPassword < ActiveRecord::Base
   validates :user_id, presence: true
 
-  validates :user_id,
+  validates :user_id, uniqueness: true
-            uniqueness: {
-              scope: :password_expired_at,
-            },
-            if: -> { password_expired_at.nil? }
-
   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
-#  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
 #

app/services/user_password_expirer.rb

@@ -3,13 +3,13 @@
 class UserPasswordExpirer
   def self.expire_user_password(user)
     UserPassword
-      .where(
+      .where(user:)
-        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

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

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

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

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.reload.user_password
-
-      user_password = user.passwords.first
 
       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.reload.user_password.password_expired_at).to eq_time(Time.zone.now)
-
-        expect(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)