From 0d32f7507c82cb656de8b42909399f0d2037082a Mon Sep 17 00:00:00 2001 From: Guillem Arias Fauste Date: Thu, 4 Jun 2026 11:52:28 +0200 Subject: [PATCH] fix(goals): scope funding-account picker to the current user's accessible accounts (#2172) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(goals): scope funding-account picker to the current user's accessible accounts The new/edit goal funding picker and the linkable-account count queried `Current.family.accounts`, so it listed (and would link/fund from) every depository account in the family — including accounts owned by other members that aren't shared with the current user. Switch the three queries (index count, lookup, picker list) to `Current.user.accessible_accounts`, matching the access boundary used elsewhere. Adds controller tests covering the new-form picker and the create path rejecting a non-accessible same-family account. Fixes #2168 * fix(goals): preserve inaccessible linked accounts on goal edit The funding picker only renders Current.user.accessible_accounts, so a family goal linked to another member's private account renders no checkbox for it. On update, sync_linked_accounts! treated that omission as an intentional removal and destroyed the link the editor could not see. Restrict unlinking to the editor's accessible accounts so links outside their access are preserved. Adds a regression test. --- app/controllers/goals_controller.rb | 14 +++-- test/controllers/goals_controller_test.rb | 63 +++++++++++++++++++++++ 2 files changed, 73 insertions(+), 4 deletions(-) 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