QT/sure
1
0
Fork 0
mirror of https://github.com/we-promise/sure.git synced 2026-05-30 07:49:01 +00:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
* 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>
This commit is contained in:
dripsmvcp
2026-05-28 06:25:46 +09:00
committed by Juan José Mata
parent 21d5bd54e1
commit 416e6c048b
7 changed files with 98 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files

19
test/controllers/settings/profiles_controller_test.rb
View File

@@ -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!(