fix(family-sharing): prevent silent data loss when rehoming or removing users (#1896)

* fix(family-sharing): prevent silent data loss when rehoming or removing users

Fixes #1689.

Two destructive paths could strand a pre-existing user's family and accounts:

1. Invitation#accept_for unconditionally overwrote user.family_id, orphaning
   the prior family + its accounts with no user able to reach them.
2. Settings::ProfilesController#destroy then called @user.destroy when an admin
   removed the rehomed member, destroying the only login path back to the
   now-orphaned data.

Add hard-block guards on both paths. accept_for refuses when the invitee
already belongs to a family with accounts; ProfilesController#destroy refuses
when the member owns accounts in another family (legacy state from the old
flow). InvitationsController#create surfaces a specific, actionable flash so
the admin understands why the auto-accept was refused.

No automatic recovery of already-orphaned data — that needs a separate
one-shot script per dosubot's analysis on the issue.

* fix(family-sharing): scope invite orphan-guard to invitee-owned accounts (#1896 review)

Codex flagged (P1) and the maintainer review independently raised that
would_orphan_existing_family? keyed off user.family.accounts.exists? —
any account in the invitee's current family — which wrongly blocked a
non-owner member from leaving a multi-user household.

Rename to would_orphan_owned_accounts? and key off
user.owned_accounts.where.not(family_id: family_id), making the invite
guard symmetric with the destroy-path guard in
Settings::ProfilesController. A member who owns no accounts now orphans
nothing by moving and is free to accept the invitation; an owner is
still blocked.

Add a regression test for the non-owner case and update the existing
tests to give the invitee explicit account ownership.

* Remove extra comments per project conventions

---------

Co-authored-by: Juan José Mata <jjmata@jjmata.com>

app/controllers/invitations_controller.rb

@@ -17,7 +17,9 @@ class InvitationsController < ApplicationController
     if @invitation.save
       normalized_email = @invitation.email.to_s.strip.downcase
       existing_user = User.find_by(email: normalized_email)
-      if existing_user && @invitation.accept_for(existing_user)
+      if existing_user && @invitation.would_orphan_owned_accounts?(existing_user)
+        flash[:alert] = t(".existing_user_has_family_data")
+      elsif existing_user && @invitation.accept_for(existing_user)
         flash[:notice] = t(".existing_user_added")
       elsif existing_user
         flash[:alert] = t(".failure")

app/controllers/settings/profiles_controller.rb

@@ -26,6 +26,12 @@ class Settings::ProfilesController < ApplicationController
       return
     end
 
+    if @user.owned_accounts.where.not(family_id: Current.family.id).exists?
+      flash[:alert] = t(".member_owns_other_family_data")
+      redirect_to settings_profile_path
+      return
+    end
+
     if @user.destroy
       # Also destroy the invitation associated with this user for this family
       Current.family.invitations.find_by(email: @user.email)&.destroy

app/models/invitation.rb

@@ -32,6 +32,7 @@ class Invitation < ApplicationRecord
     return false if user.blank?
     return false unless pending?
     return false unless emails_match?(user)
+    return false if would_orphan_owned_accounts?(user)
 
     transaction do
       user.update!(family_id: family_id, role: role.to_s)
@@ -41,6 +42,14 @@ class Invitation < ApplicationRecord
     true
   end
 
+  def would_orphan_owned_accounts?(user)
+    return false if user.blank?
+    return false if user.family_id.blank?
+    return false if user.family_id == family_id
+
+    user.owned_accounts.where.not(family_id: family_id).exists?
+  end
+
   private
 
     def emails_match?(user)

config/locales/views/invitations/en.yml

@@ -9,6 +9,7 @@ en:
       title: Join %{family}
     create:
       existing_user_added: User has been added to your household.
+      existing_user_has_family_data: That user already owns a household with accounts. They need to remove or transfer those accounts before joining yours.
       failure: Could not send invitation
       success: Invitation sent successfully
     destroy:

config/locales/views/settings/en.yml

@@ -160,6 +160,7 @@ en:
     profiles:
       destroy:
         cannot_remove_self: You cannot remove yourself from the account.
+        member_owns_other_family_data: This member still owns accounts in another household and cannot be removed. Transfer or remove those accounts first.
         member_removal_failed: There was a problem removing the member.
         member_removed: Member was successfully removed.
         not_authorized: You are not authorized to remove members.

test/controllers/settings/profiles_controller_test.rb

@@ -59,6 +59,25 @@ class Settings::ProfilesControllerTest < ActionDispatch::IntegrationTest
     assert User.find(@admin.id)
   end
 
+  test "admin cannot destroy a member who owns accounts in another family" do
+    other_family = families(:empty)
+    legacy_account = other_family.accounts.create!(
+      name: "Legacy savings", balance: 250, currency: "USD",
+      accountable: Depository.new
+    )
+    legacy_account.update_columns(owner_id: @member.id)
+
+    sign_in @admin
+
+    assert_no_difference("User.count") do
+      delete settings_profile_path(user_id: @member)
+    end
+
+    assert_redirected_to settings_profile_path
+    assert_equal I18n.t("settings.profiles.destroy.member_owns_other_family_data"), flash[:alert]
+    assert User.find(@member.id), "user row must be preserved so historical access can be restored"
+  end
+
   test "admin removing a family member also destroys their invitation" do
     # Create an invitation for the member
     invitation = @admin.family.invitations.create!(

test/models/invitation_test.rb

@@ -122,6 +122,65 @@ class InvitationTest < ActiveSupport::TestCase
     assert_includes invitation.errors[:email], "has already been invited to this family"
   end
 
+  test "accept_for refuses when invitee owns accounts that would be orphaned" do
+    owner = users(:empty)
+    owner_family = families(:empty)
+    owner.update_columns(family_id: owner_family.id, role: "admin")
+    account = owner_family.accounts.create!(
+      name: "Prior savings", balance: 100, currency: "USD",
+      accountable: Depository.new
+    )
+    account.update_columns(owner_id: owner.id)
+
+    invitation = @family.invitations.create!(email: owner.email, role: "member", inviter: @inviter)
+
+    result = invitation.accept_for(owner)
+
+    assert_not result, "accept_for must refuse to rehome a user away from accounts they own"
+    owner.reload
+    assert_equal owner_family.id, owner.family_id, "user.family_id must not be silently overwritten"
+    invitation.reload
+    assert_nil invitation.accepted_at, "invitation must remain pending so a new flow can recover"
+    assert owner_family.accounts.exists?, "original family's accounts must remain intact"
+  end
+
+  test "accept_for allows a member who owns no accounts to join another family" do
+    member = users(:empty)
+    other_owner = users(:sure_support_staff)
+    source_family = families(:empty)
+    member.update_columns(family_id: source_family.id, role: "member")
+    other_owner.update_columns(family_id: source_family.id, role: "admin")
+    account = source_family.accounts.create!(
+      name: "Shared savings", balance: 100, currency: "USD",
+      accountable: Depository.new
+    )
+    account.update_columns(owner_id: other_owner.id)
+
+    invitation = @family.invitations.create!(email: member.email, role: "member", inviter: @inviter)
+
+    result = invitation.accept_for(member)
+
+    assert result, "a non-owner member must be free to join another family"
+    member.reload
+    assert_equal @family.id, member.family_id
+  end
+
+  test "would_orphan_owned_accounts? is false when invitee owns no accounts" do
+    user = users(:empty)
+    user.update_columns(family_id: families(:empty).id, role: "admin")
+    invitation = @family.invitations.create!(email: user.email, role: "member", inviter: @inviter)
+
+    assert_not invitation.would_orphan_owned_accounts?(user)
+  end
+
+  test "would_orphan_owned_accounts? is false when same-family role change" do
+    user = users(:family_member)
+    user.update!(family_id: @family.id, role: "member")
+    invitation = @family.invitations.create!(email: user.email, role: "admin", inviter: @inviter)
+
+    assert_not invitation.would_orphan_owned_accounts?(user)
+  end
+
   test "accept_for applies guest role defaults" do
     user = users(:family_member)
     user.update!(