From 416e6c048b6e357ef62900db6b63c866b2510bfc Mon Sep 17 00:00:00 2001 From: dripsmvcp <138900956+dripsmvcp@users.noreply.github.com> Date: Thu, 28 May 2026 06:25:46 +0900 Subject: [PATCH] fix(family-sharing): prevent silent data loss when rehoming or removing users (#1896) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- app/controllers/invitations_controller.rb | 4 +- .../settings/profiles_controller.rb | 6 ++ app/models/invitation.rb | 9 +++ config/locales/views/invitations/en.yml | 1 + config/locales/views/settings/en.yml | 1 + .../settings/profiles_controller_test.rb | 19 ++++++ test/models/invitation_test.rb | 59 +++++++++++++++++++ 7 files changed, 98 insertions(+), 1 deletion(-) diff --git a/app/controllers/invitations_controller.rb b/app/controllers/invitations_controller.rb index ba98b9e68..f11965a0a 100644 --- a/app/controllers/invitations_controller.rb +++ b/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") diff --git a/app/controllers/settings/profiles_controller.rb b/app/controllers/settings/profiles_controller.rb index e1f3b5db4..c08341eba 100644 --- a/app/controllers/settings/profiles_controller.rb +++ b/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 diff --git a/app/models/invitation.rb b/app/models/invitation.rb index 99ecb0da7..a7b391288 100644 --- a/app/models/invitation.rb +++ b/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) diff --git a/config/locales/views/invitations/en.yml b/config/locales/views/invitations/en.yml index 45c2d51a5..bc4dfded3 100644 --- a/config/locales/views/invitations/en.yml +++ b/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: diff --git a/config/locales/views/settings/en.yml b/config/locales/views/settings/en.yml index e7abc768f..1762c9d37 100644 --- a/config/locales/views/settings/en.yml +++ b/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. diff --git a/test/controllers/settings/profiles_controller_test.rb b/test/controllers/settings/profiles_controller_test.rb index 9228a5ab5..fd0e6fc6f 100644 --- a/test/controllers/settings/profiles_controller_test.rb +++ b/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!( diff --git a/test/models/invitation_test.rb b/test/models/invitation_test.rb index 9895538e8..f54e63d4d 100644 --- a/test/models/invitation_test.rb +++ b/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!(