diff --git a/app/controllers/goals_controller.rb b/app/controllers/goals_controller.rb index c54a44371..7406f8fe4 100644 --- a/app/controllers/goals_controller.rb +++ b/app/controllers/goals_controller.rb @@ -26,7 +26,7 @@ class GoalsController < ApplicationController # entirely (rendered with filterable: false). @grid_goals = @active_goals + @completed_goals - @linkable_account_count = Current.family.accounts.where(accountable_type: "Depository").visible.count + @linkable_account_count = Current.user.accessible_accounts.where(accountable_type: "Depository").visible.count @kpi = kpi_payload(@active_goals) @any_pending_pledge = @active_goals.any? { |g| g.open_pledges.any? } @show_search = @grid_goals.size > 6 @@ -169,18 +169,24 @@ class GoalsController < ApplicationController return [] if ids.blank? ids = Array(ids).reject(&:blank?) - Current.family.accounts.where(accountable_type: "Depository").visible.where(id: ids).to_a + Current.user.accessible_accounts.where(accountable_type: "Depository").visible.where(id: ids).to_a end def linkable_accounts_for_new - Current.family.accounts.where(accountable_type: "Depository").visible.alphabetically.to_a + Current.user.accessible_accounts.where(accountable_type: "Depository").visible.alphabetically.to_a end def sync_linked_accounts!(goal, accounts) desired_ids = accounts.map(&:id).to_set current_ids = goal.goal_accounts.pluck(:account_id).to_set - (current_ids - desired_ids).each do |id| + # Only unlink accounts the current user can actually see in the picker. + # A family goal may be linked to another member's private account, which + # never renders as a checkbox — so its absence from the submitted set is + # not an intentional removal and must not destroy the link. + removable_ids = Current.user.accessible_accounts.where(id: current_ids.to_a).pluck(:id).to_set + + ((current_ids & removable_ids) - desired_ids).each do |id| goal.goal_accounts.where(account_id: id).destroy_all end additions = accounts.reject { |a| current_ids.include?(a.id) } diff --git a/test/controllers/goals_controller_test.rb b/test/controllers/goals_controller_test.rb index c20ae7e73..2fb08236c 100644 --- a/test/controllers/goals_controller_test.rb +++ b/test/controllers/goals_controller_test.rb @@ -90,6 +90,47 @@ class GoalsControllerTest < ActionDispatch::IntegrationTest assert_response :unprocessable_entity end + test "new form excludes same-family accounts not shared with the current user" do + # Regression for #2168: funding-account picker leaked accounts owned by + # other family members that were never shared with the current user. + private_account = Account.create!( + family: @user.family, + owner: users(:family_member), + accountable: Depository.new, + name: "Member Private Checking", + currency: "USD", + balance: 100 + ) + + get new_goal_url + assert_response :success + assert_no_match(/Member Private Checking/, response.body) + assert_no_match(/goal_account_ids_#{private_account.id}/, response.body) + end + + test "create rejects a same-family account not shared with the current user" do + private_account = Account.create!( + family: @user.family, + owner: users(:family_member), + accountable: Depository.new, + name: "Member Private Checking", + currency: "USD", + balance: 100 + ) + + assert_no_difference "Goal.count" do + post goals_url, params: { + goal: { + name: "Sneaky goal", + target_amount: "1000", + color: "#4da568", + account_ids: [ private_account.id ] + } + } + end + assert_response :unprocessable_entity + end + test "update modifies identity fields" do patch goal_url(@goal), params: { goal: { name: "Renamed" } } assert_redirected_to goal_path(@goal) @@ -109,6 +150,28 @@ class GoalsControllerTest < ActionDispatch::IntegrationTest assert_equal [ @connected.id ], @goal.reload.goal_accounts.pluck(:account_id) end + test "update preserves a linked account the current user cannot access" do + # Regression for #2172 review: a family goal can be linked to a private + # account owned by another member. That account is never rendered in the + # picker, so its absence from the submitted set must not unlink it. + private_account = Account.create!( + family: @user.family, + owner: users(:family_member), + accountable: Depository.new, + name: "Member Private Checking", + currency: @goal.currency, + balance: 100 + ) + @goal.goal_accounts.create!(account: private_account) + + patch goal_url(@goal), params: { goal: { account_ids: [ @depository.id ] } } + + assert_redirected_to goal_path(@goal) + linked = @goal.reload.goal_accounts.pluck(:account_id) + assert_includes linked, private_account.id, "inaccessible private link must be preserved" + assert_includes linked, @depository.id + end + test "update with empty account_ids re-renders with error" do patch goal_url(@goal), params: { goal: { account_ids: [ "" ] } } assert_response :unprocessable_entity