From dd707ec91beffdbd2aa46f9d2d4c3e849dd664b7 Mon Sep 17 00:00:00 2001 From: minion1227 Date: Tue, 7 Jul 2026 23:59:54 -0700 Subject: [PATCH] fix(invitations): allow re-inviting after an unaccepted invite expires (#2543) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(invitations): allow re-inviting after an unaccepted invite expires An expired, never-accepted invitation still occupies the partial unique index `index_invitations_on_email_and_family_id_pending` (predicate `accepted_at IS NULL`), but the `pending` scope excludes it via the `expires_at > now` clause. The duplicate-guard validation therefore passes and the INSERT collides with the index, raising ActiveRecord::RecordNotUnique — surfaced to the admin as a 500 with no way to re-invite that address through the UI. Remove the stale expired row inside the create transaction (before_create) so the unique slot is freed and the re-invite succeeds. It runs only after validations pass, so a still-pending (non-expired) invitation can never be removed here. Add a controller-level rescue of ActiveRecord::RecordNotUnique as a safety net against a concurrent double-submit race. Closes #2535 * refactor(invitations): scope RecordNotUnique rescue to the save The method-level `rescue ActiveRecord::RecordNotUnique` wrapped the whole create action, so any future write after @invitation.save (e.g. accept_for) would silently be reported as a generic invite failure. Extract save_invitation to scope the rescue to the save alone, and add a regression test that drives the raced unique-index path directly. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- app/controllers/invitations_controller.rb | 13 ++++++- app/models/invitation.rb | 24 ++++++++++++ .../invitations_controller_test.rb | 39 +++++++++++++++++++ test/models/invitation_test.rb | 26 +++++++++++++ 4 files changed, 101 insertions(+), 1 deletion(-) diff --git a/app/controllers/invitations_controller.rb b/app/controllers/invitations_controller.rb index f11965a0a..70f672581 100644 --- a/app/controllers/invitations_controller.rb +++ b/app/controllers/invitations_controller.rb @@ -14,7 +14,7 @@ class InvitationsController < ApplicationController @invitation = Current.family.invitations.build(invitation_params) @invitation.inviter = Current.user - if @invitation.save + if save_invitation normalized_email = @invitation.email.to_s.strip.downcase existing_user = User.find_by(email: normalized_email) if existing_user && @invitation.would_orphan_owned_accounts?(existing_user) @@ -67,4 +67,15 @@ class InvitationsController < ApplicationController def invitation_params params.require(:invitation).permit(:email, :role) end + + # Persist the invitation, treating a raced partial-unique-index violation as + # an ordinary save failure. A concurrent double-submit can still slip between + # the `no_duplicate_pending_invitation_in_family` validation and the INSERT. + # Scoping the rescue to the save alone keeps later writes in #create (e.g. + # accept_for) from silently swallowing a genuine RecordNotUnique. + def save_invitation + @invitation.save + rescue ActiveRecord::RecordNotUnique + false + end end diff --git a/app/models/invitation.rb b/app/models/invitation.rb index a7b391288..35e59d15a 100644 --- a/app/models/invitation.rb +++ b/app/models/invitation.rb @@ -19,6 +19,7 @@ class Invitation < ApplicationRecord before_validation :normalize_email before_validation :generate_token, on: :create + before_create :remove_expired_duplicates_in_family before_create :set_expiration scope :pending, -> { where(accepted_at: nil).where("expires_at > ?", Time.current) } @@ -69,6 +70,29 @@ class Invitation < ApplicationRecord self.expires_at = 3.days.from_now end + # An expired, never-accepted invitation still occupies the partial unique + # index `index_invitations_on_email_and_family_id_pending` (predicate + # `accepted_at IS NULL`), but the `pending` scope treats it as gone because + # of the `expires_at > now` clause. The duplicate-guard validation therefore + # passes, and the INSERT collides with the index, raising RecordNotUnique. + # Removing the stale row inside the create transaction frees the slot so the + # re-invite can proceed. It runs only after validations pass, so a still + # pending (non-expired) invitation can never be removed here. + def remove_expired_duplicates_in_family + return if email.blank? + + scope = self.class.where(family_id: family_id, accepted_at: nil) + .where("expires_at <= ?", Time.current) + + scope = if self.class.encryption_ready? + scope.where(email: email) + else + scope.where("LOWER(email) = ?", email.to_s.strip.downcase) + end + + scope.delete_all + end + def normalize_email self.email = email.to_s.strip.downcase if email.present? end diff --git a/test/controllers/invitations_controller_test.rb b/test/controllers/invitations_controller_test.rb index 90a8e7102..a0f872d3f 100644 --- a/test/controllers/invitations_controller_test.rb +++ b/test/controllers/invitations_controller_test.rb @@ -134,6 +134,45 @@ class InvitationsControllerTest < ActionDispatch::IntegrationTest assert existing_user.ai_enabled? end + test "re-inviting an email whose prior invitation expired succeeds instead of 500ing" do + email = "expired-reinvite-ctrl@example.com" + expired = @admin.family.invitations.create!(email: email, role: "member", inviter: @admin) + expired.update_column(:expires_at, 1.day.ago) + + post invitations_url, params: { + invitation: { + email: email, + role: "member" + } + } + + assert_redirected_to settings_profile_path + assert_equal I18n.t("invitations.create.success"), flash[:notice] + assert_not Invitation.exists?(expired.id), "stale expired invitation should be replaced" + + new_invitation = @admin.family.invitations.find_by(email: email) + assert new_invitation.present? + assert new_invitation.pending? + end + + test "raced unique-index violation on save is handled as a failure, not a 500" do + # Simulate a concurrent double-submit that slips past the pending-invite + # validation and hits the partial unique index at INSERT time. + Invitation.any_instance.stubs(:save).raises(ActiveRecord::RecordNotUnique) + + assert_no_difference("Invitation.count") do + post invitations_url, params: { + invitation: { + email: "raced@example.com", + role: "member" + } + } + end + + assert_redirected_to settings_profile_path + assert_equal I18n.t("invitations.create.failure"), flash[:alert] + end + test "should handle invalid invitation creation" do assert_no_difference("Invitation.count") do post invitations_url, params: { diff --git a/test/models/invitation_test.rb b/test/models/invitation_test.rb index f54e63d4d..0e451fd7a 100644 --- a/test/models/invitation_test.rb +++ b/test/models/invitation_test.rb @@ -110,6 +110,32 @@ class InvitationTest < ActiveSupport::TestCase assert invitation.valid? end + test "re-inviting an email with an expired unaccepted invitation replaces it without raising" do + email = "expired-reinvite@example.com" + + expired = @family.invitations.create!(email: email, role: "member", inviter: @inviter) + expired.update_column(:expires_at, 1.day.ago) + + invitation = nil + assert_nothing_raised do + invitation = @family.invitations.create!(email: email, role: "admin", inviter: @inviter) + end + + assert invitation.pending? + assert_not Invitation.exists?(expired.id), "stale expired invitation should be removed" + assert_equal 1, @family.invitations.where(email: email).count + end + + test "re-invite does not remove a still-pending duplicate (validation blocks first)" do + email = "still-pending@example.com" + pending = @family.invitations.create!(email: email, role: "member", inviter: @inviter) + + duplicate = @family.invitations.build(email: email, role: "admin", inviter: @inviter) + assert_not duplicate.valid? + + assert Invitation.exists?(pending.id), "an unexpired pending invitation must be preserved" + end + test "can create invitation in same family (uniqueness scoped to family)" do email = "same-family-test@example.com"