From 839d6b36adfbe58475a0d1a3cec13853ca87fbee Mon Sep 17 00:00:00 2001 From: Guillem Arias Date: Fri, 29 May 2026 10:25:05 +0200 Subject: [PATCH] fix(retirement): isolate retirement goals from savings goal routes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Codex P2 on #2044. A Goal::Retirement row lives in Current.family.goals, so the shared GoalsController and GoalPledgesController loaded it through `family.goals.find(...)` — never calling Goal::Retirement#editable_by?. Any preview-enabled family member could therefore open /goals/:id and edit/archive/delete another member's owner-scoped retirement plan, hit its pledge routes, and see it listed in the savings Goals grid. Adds `Goal.savings` (base type only) and scopes both savings controllers to it, so retirement goals are unreachable through the shared routes (RecordNotFound -> goals_path redirect) and absent from the savings index. Owner-only retirement access stays in RetirementController; editable_by? is retained for it. Tests: savings scope excludes retirement; retirement goal absent from goals index; show + pledge routes redirect not-found for retirement. (The Codex schema.rb null:false finding is a false positive — this branch's schema.rb retains null:false on all IBKR payload columns and the diff vs the base branch touches no IBKR lines; Codex compared against main rather than the PR base.) --- app/controllers/goal_pledges_controller.rb | 2 +- app/controllers/goals_controller.rb | 10 ++++---- app/models/goal.rb | 6 +++++ .../goal_pledges_controller_test.rb | 11 +++++++++ test/controllers/goals_controller_test.rb | 24 +++++++++++++++++++ test/models/goal_test.rb | 11 +++++++++ 6 files changed, 58 insertions(+), 6 deletions(-) diff --git a/app/controllers/goal_pledges_controller.rb b/app/controllers/goal_pledges_controller.rb index 45c8fdec0..16e7b0836 100644 --- a/app/controllers/goal_pledges_controller.rb +++ b/app/controllers/goal_pledges_controller.rb @@ -60,7 +60,7 @@ class GoalPledgesController < ApplicationController # Preload linked accounts + their providers so any_connected_account? # and the new-pledge form's per-account helpers don't trigger N+1 # queries on account_providers. - @goal = Current.family.goals + @goal = Current.family.goals.savings .includes(:open_pledges, linked_accounts: :account_providers) .find(params[:goal_id]) end diff --git a/app/controllers/goals_controller.rb b/app/controllers/goals_controller.rb index c54a44371..9418b2505 100644 --- a/app/controllers/goals_controller.rb +++ b/app/controllers/goals_controller.rb @@ -7,12 +7,12 @@ class GoalsController < ApplicationController ACTIVE_STATUS_RANK = { behind: 0, on_track: 1, no_target_date: 2 }.freeze def index - state_counts = Current.family.goals.group(:state).count + state_counts = Current.family.goals.savings.group(:state).count @counts = STATE_FILTERS.each_with_object({}) do |state, h| h[state] = state == "all" ? state_counts.values.sum : (state_counts[state] || 0) end - all_goals = Current.family.goals + all_goals = Current.family.goals.savings .alphabetically .includes(:open_pledges, linked_accounts: :account_providers) .to_a @@ -46,7 +46,7 @@ class GoalsController < ApplicationController end def new - @goal = Current.family.goals.new( + @goal = Current.family.goals.savings.new( color: Goal::COLORS.sample, currency: Current.family.primary_currency_code ) @@ -59,7 +59,7 @@ class GoalsController < ApplicationController end def create - @goal = Current.family.goals.new(goal_params) + @goal = Current.family.goals.savings.new(goal_params) accounts = lookup_accounts(params.dig(:goal, :account_ids)) @goal.currency = accounts.first.currency if accounts.any? && @goal.currency.blank? @@ -148,7 +148,7 @@ class GoalsController < ApplicationController private def set_goal - @goal = Current.family.goals + @goal = Current.family.goals.savings .includes(:open_pledges, linked_accounts: :account_providers) .find(params[:id]) end diff --git a/app/models/goal.rb b/app/models/goal.rb index 6531a7b79..29fda7942 100644 --- a/app/models/goal.rb +++ b/app/models/goal.rb @@ -32,6 +32,12 @@ class Goal < ApplicationRecord scope :active_first, lambda { order(Arel.sql("CASE state WHEN 'active' THEN 0 WHEN 'paused' THEN 1 WHEN 'completed' THEN 2 ELSE 3 END")) } + # Savings goals only — the base Goal type. STI subtypes such as + # Goal::Retirement have their own surfaces (RetirementController) and + # must NOT be reachable through the shared goal/pledge routes, where + # any preview-enabled family member could otherwise view, edit, or + # delete another member's owner-scoped retirement plan. + scope :savings, -> { where(type: "Goal") } def self.advisory_lock_key_for(family_id) Digest::SHA1.hexdigest("goals:family:#{family_id}").to_i(16) % (2**63) diff --git a/test/controllers/goal_pledges_controller_test.rb b/test/controllers/goal_pledges_controller_test.rb index 9395cad32..cb7d9be0d 100644 --- a/test/controllers/goal_pledges_controller_test.rb +++ b/test/controllers/goal_pledges_controller_test.rb @@ -91,4 +91,15 @@ class GoalPledgesControllerTest < ActionDispatch::IntegrationTest get new_goal_pledge_url(other_goal) assert_redirected_to goals_path end + + test "pledge routes are not reachable for retirement goals" do + retirement = Goal::Retirement.create!( + family: @user.family, owner: @user, + name: "Retire", target_amount: 1_000_000, currency: "USD" + ) + + get new_goal_pledge_url(retirement), headers: { "Turbo-Frame" => "modal" } + + assert_redirected_to goals_path + end end diff --git a/test/controllers/goals_controller_test.rb b/test/controllers/goals_controller_test.rb index c20ae7e73..f6147da67 100644 --- a/test/controllers/goals_controller_test.rb +++ b/test/controllers/goals_controller_test.rb @@ -239,4 +239,28 @@ class GoalsControllerTest < ActionDispatch::IntegrationTest assert_redirected_to goals_path assert_equal I18n.t("goals.errors.not_found"), flash[:alert] end + + test "retirement goals are excluded from the savings index" do + Goal::Retirement.create!( + family: @user.family, owner: @user, + name: "My Retirement Plan", target_amount: 1_000_000, currency: "USD" + ) + + get goals_url + + assert_response :success + assert_no_match(/My Retirement Plan/, response.body) + end + + test "retirement goal is not reachable via the savings show route" do + retirement = Goal::Retirement.create!( + family: @user.family, owner: @user, + name: "Retire", target_amount: 1_000_000, currency: "USD" + ) + + get goal_url(retirement) + + assert_redirected_to goals_path + assert_equal I18n.t("goals.errors.not_found"), flash[:alert] + end end diff --git a/test/models/goal_test.rb b/test/models/goal_test.rb index afd88f7a1..a6ae21590 100644 --- a/test/models/goal_test.rb +++ b/test/models/goal_test.rb @@ -261,4 +261,15 @@ class GoalTest < ActiveSupport::TestCase assert_not @goal.editable_by?(foreign_user) assert_not @goal.editable_by?(nil) end + + test "savings scope excludes retirement subtype" do + retirement = Goal::Retirement.create!( + family: @family, owner: users(:family_admin), + name: "Retire", target_amount: 1_000_000, currency: "USD" + ) + + savings_ids = Goal.savings.pluck(:id) + assert_includes savings_ids, @goal.id + assert_not_includes savings_ids, retirement.id + end end