From 8b2d792dab0a147ebe57c1f83964ba8b6db1d52a Mon Sep 17 00:00:00 2001 From: pro3958 Date: Fri, 24 Jul 2026 19:44:27 -0700 Subject: [PATCH] fix(api): scope holdings API to accessible accounts (#2706) Api::V1::HoldingsController built both the index and show queries from current_resource_owner.family.holdings, filtering only on account status. A read-scoped API key could therefore read holdings (account id/name, quantity, price, market value) for accounts owned by other family members that were never shared with the key owner, and the account_id / account_ids filters made targeted enumeration of those accounts trivial. Scope the query through accounts.accessible_by(current_resource_owner) so results are limited to accounts the token owner owns or has been granted access to. Route both index and set_holding through the new accessible_holdings scope. This matches Api::V1::BalancesController and the web HoldingsController, which already scope the same way. Fixes #2467 Co-authored-by: agentloop --- app/controllers/api/v1/holdings_controller.rb | 19 +++- .../api/v1/holdings_controller_test.rb | 95 +++++++++++++++++++ 2 files changed, 110 insertions(+), 4 deletions(-) create mode 100644 test/controllers/api/v1/holdings_controller_test.rb diff --git a/app/controllers/api/v1/holdings_controller.rb b/app/controllers/api/v1/holdings_controller.rb index 7f08dfe3b..519c69165 100644 --- a/app/controllers/api/v1/holdings_controller.rb +++ b/app/controllers/api/v1/holdings_controller.rb @@ -7,8 +7,7 @@ class Api::V1::HoldingsController < Api::V1::BaseController before_action :set_holding, only: [ :show ] def index - family = current_resource_owner.family - holdings_query = family.holdings.joins(:account).where(accounts: { status: [ "draft", "active" ] }) + holdings_query = accessible_holdings holdings_query = apply_filters(holdings_query) holdings_query = holdings_query.includes(:account, :security).chronological @@ -36,12 +35,24 @@ class Api::V1::HoldingsController < Api::V1::BaseController private def set_holding - family = current_resource_owner.family - @holding = family.holdings.joins(:account).where(accounts: { status: %w[draft active] }).find(params[:id]) + @holding = accessible_holdings.find(params[:id]) rescue ActiveRecord::RecordNotFound render json: { error: "not_found", message: "Holding not found" }, status: :not_found end + # Holdings restricted to accounts the token owner can access (owned or shared), + # not the whole family. Mirrors Api::V1::BalancesController and the web + # HoldingsController, both of which scope through Account.accessible_by. + def accessible_holdings + Holding + .joins(:account) + .where(accounts: { status: %w[draft active], id: accessible_account_ids }) + end + + def accessible_account_ids + @accessible_account_ids ||= current_resource_owner.family.accounts.accessible_by(current_resource_owner).select(:id) + end + def ensure_read_scope authorize_scope!(:read) end diff --git a/test/controllers/api/v1/holdings_controller_test.rb b/test/controllers/api/v1/holdings_controller_test.rb new file mode 100644 index 000000000..6cf51d966 --- /dev/null +++ b/test/controllers/api/v1/holdings_controller_test.rb @@ -0,0 +1,95 @@ +# frozen_string_literal: true + +require "test_helper" + +class Api::V1::HoldingsControllerTest < ActionDispatch::IntegrationTest + setup do + @user = users(:family_admin) + @family = @user.family + @user.api_keys.active.destroy_all + + @api_key = ApiKey.create!( + user: @user, + name: "Test Read Key", + scopes: [ "read" ], + source: "web", + display_key: "test_read_#{SecureRandom.hex(8)}" + ) + + @account = accounts(:investment) + @holding = holdings(:one) + + # An account owned by another family member, never shared with @user. + @other_member = users(:family_member) + @private_account = @family.accounts.create!( + name: "Member Brokerage", + accountable: Investment.new, + balance: 5000, + currency: "USD", + owner: @other_member + ) + @private_holding = @private_account.holdings.create!( + security: securities(:msft), + date: Date.current, + qty: 5, + price: 100, + amount: 500, + currency: "USD" + ) + end + + test "lists holdings scoped to accessible accounts" do + get api_v1_holdings_url, headers: api_headers(@api_key) + + assert_response :success + holding_ids = JSON.parse(response.body)["holdings"].map { |holding| holding["id"] } + assert_includes holding_ids, @holding.id + assert_not_includes holding_ids, @private_holding.id + end + + test "account_ids filter cannot reach inaccessible accounts" do + get api_v1_holdings_url, + params: { account_ids: [ @private_account.id ] }, + headers: api_headers(@api_key) + + assert_response :success + holding_ids = JSON.parse(response.body)["holdings"].map { |holding| holding["id"] } + assert_not_includes holding_ids, @private_holding.id + end + + test "account_id filter cannot reach inaccessible accounts" do + get api_v1_holdings_url, + params: { account_id: @private_account.id }, + headers: api_headers(@api_key) + + assert_response :success + holding_ids = JSON.parse(response.body)["holdings"].map { |holding| holding["id"] } + assert_not_includes holding_ids, @private_holding.id + end + + test "shows an accessible holding" do + get api_v1_holding_url(@holding), headers: api_headers(@api_key) + + assert_response :success + response_data = JSON.parse(response.body) + assert_equal @holding.id, response_data["id"] + end + + test "returns not found for an inaccessible holding" do + get api_v1_holding_url(@private_holding), headers: api_headers(@api_key) + + assert_response :not_found + end + + test "requires authentication" do + get api_v1_holdings_url + + assert_response :unauthorized + end + + private + + def api_headers(api_key) + { "X-Api-Key" => api_key.display_key } + end +end