Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions app/controllers/api/v1/transactions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ def create
return render_existing_idempotent_entry(existing_entry)
end

return unless validate_family_associations

@entry = account.entries.new(entry_params_for_create)

if @entry.save
Expand Down Expand Up @@ -140,6 +142,8 @@ def update
return
end

return unless validate_family_associations

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Document the new PATCH validation response

When PATCH supplies an invalid or foreign category, merchant, or tag, this new guard returns 422, but the PATCH operation in spec/requests/api/v1/transactions_spec.rb still documents only 200/404 responses and does not describe the ownership constraints; the generated OpenAPI YAML mirrors that omission. Update the rswag PATCH operation and regenerate the docs so clients receive the newly introduced contract.

AGENTS.md reference: AGENTS.md:L39-L46

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 84f1df8. The PATCH operation in spec/requests/api/v1/transactions_spec.rb now documents the family-ownership requirement on category_id, merchant_id and tag_ids (keeping the existing omit-vs-[] tag semantics) and adds the 422 response, and docs/api/openapi.yaml is regenerated.

One note on the regenerated file: rswag:specs:swaggerize also emits pre-existing drift unrelated to this PR (the merchants CSV import endpoint, MerchantImportResult, and the transfer fee fields), from specs already on main that were never regenerated. I left that out to keep this diff reviewable — it is still outstanding on main and probably worth a separate sync.


Entry.transaction do
if @entry.update(entry_params_for_update)
# Handle tags separately - only when explicitly provided in the request
Expand Down Expand Up @@ -318,6 +322,40 @@ def account_id_param
params.dig(:transaction, :account_id).presence
end

# Category, merchant and tag references are assigned straight onto the
# record, so an unusable id has to be rejected here rather than reaching the
# database. A malformed UUID casts to NULL and the association is dropped
# while the request still reports success, an unknown UUID trips the foreign
# key and surfaces as a 500, and a UUID owned by another family is accepted
# outright. Returns false once a response has been rendered.
def validate_family_associations
family = current_resource_owner.family

category_id = transaction_params[:category_id]
if category_id.present? && !family_owns?(family.categories, category_id)
render_validation_error("Category not found or does not belong to your family")
return false
end

merchant_id = transaction_params[:merchant_id]
if merchant_id.present? && !family_owns?(family.merchants, merchant_id)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Allow accessible provider merchants

When a family has provider-synced transactions, Api::V1::MerchantsController#index intentionally returns their accessible ProviderMerchant records, but this check searches only family.merchants, whose association is restricted to FamilyMerchant. Consequently, passing a merchant ID returned by GET /api/v1/merchants to transaction create or update now incorrectly returns 422; validate against the same accessible merchant scope used by the merchants API.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 84f1df8 — thank you, this was a real regression.

Verified against the dev container: a ProviderMerchant attached to a transaction on an accessible account is returned by GET /api/v1/merchants, and passing that id to POST /api/v1/transactions returned 422 with my guard in place.

Rather than duplicate the index's scope, I extracted it into Family#api_assignable_merchants_for(user) and now use it from both MerchantsController#index and the transactions validation, so the two cannot drift apart again. Added a regression test that asserts the merchants index lists the provider merchant and that creating a transaction with it succeeds.

render_validation_error("Merchant not found or does not belong to your family")
return false
end

tag_ids = Array(transaction_params[:tag_ids]).reject(&:blank?).uniq
if tag_ids.any? && !tag_ids.all? { |id| family_owns?(family.tags, id) }
render_validation_error("One or more tags were not found or do not belong to your family")
return false
end
Comment thread
coderabbitai[bot] marked this conversation as resolved.

true
end

def family_owns?(scope, id)
valid_uuid?(id) && scope.exists?(id: id)
end

def entry_params_for_create
entry_params = {
name: transaction_params[:name] || transaction_params[:description],
Expand Down
9 changes: 6 additions & 3 deletions docs/api/openapi.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -7353,11 +7353,13 @@ paths:
category_id:
type: string
format: uuid
description: Category ID
description: Category ID. Must be the UUID of a category in
your own family, as returned by GET /api/v1/categories
merchant_id:
type: string
format: uuid
description: Merchant ID
description: Merchant ID. Must be the UUID of a merchant in
your own family, as returned by GET /api/v1/merchants
nature:
type: string
enum:
Expand All @@ -7379,7 +7381,8 @@ paths:
items:
type: string
format: uuid
description: Array of tag IDs
description: Array of tag IDs. Each must be the UUID of a tag
in your own family
required:
- account_id
- date
Expand Down
6 changes: 3 additions & 3 deletions spec/requests/api/v1/transactions_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -171,12 +171,12 @@
description: { type: :string, description: 'Alternative to name field' },
notes: { type: :string, description: 'Additional notes' },
currency: { type: :string, description: 'Currency code (defaults to family currency)' },
category_id: { type: :string, format: :uuid, description: 'Category ID' },
merchant_id: { type: :string, format: :uuid, description: 'Merchant ID' },
category_id: { type: :string, format: :uuid, description: 'Category ID. Must be the UUID of a category in your own family, as returned by GET /api/v1/categories' },
merchant_id: { type: :string, format: :uuid, description: 'Merchant ID. Must be the UUID of a merchant in your own family, as returned by GET /api/v1/merchants' },
nature: { type: :string, enum: %w[income expense inflow outflow], description: 'Transaction nature (determines sign)' },
external_id: { type: :string, description: 'Optional external idempotency key scoped to account and source' },
source: { type: :string, description: 'Optional source namespace for external_id. Requires external_id and defaults to api when external_id is provided' },
tag_ids: { type: :array, items: { type: :string, format: :uuid }, description: 'Array of tag IDs' }
tag_ids: { type: :array, items: { type: :string, format: :uuid }, description: 'Array of tag IDs. Each must be the UUID of a tag in your own family' }
Comment thread
coderabbitai[bot] marked this conversation as resolved.
},
required: %w[account_id date amount name]
}
Expand Down
113 changes: 113 additions & 0 deletions test/controllers/api/v1/transactions_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -601,6 +601,106 @@ class Api::V1::TransactionsControllerTest < ActionDispatch::IntegrationTest
assert_response :unauthorized
end

# Association scoping tests (issue #2781)
test "should create transaction with a category from the caller's family" do
category = categories(:food_and_drink)

post api_v1_transactions_url,
params: transaction_params_with(category_id: category.id),
headers: api_headers(@api_key)

assert_response :created
assert_equal category.id, JSON.parse(response.body).dig("category", "id")
end

test "should reject create with malformed category_id instead of silently dropping it" do
assert_no_difference("@account.entries.count") do
post api_v1_transactions_url,
params: transaction_params_with(category_id: "18bsy6-"),
headers: api_headers(@api_key)
end

assert_response :unprocessable_entity
assert_equal "validation_failed", JSON.parse(response.body)["error"]
end

test "should reject create with unknown category_id instead of raising a server error" do
assert_no_difference("@account.entries.count") do
post api_v1_transactions_url,
params: transaction_params_with(category_id: SecureRandom.uuid),
headers: api_headers(@api_key)
end

assert_response :unprocessable_entity
assert_equal "validation_failed", JSON.parse(response.body)["error"]
end

test "should reject create with a category belonging to another family" do
foreign_category = families(:empty).categories.create!(name: "Foreign Category")

assert_no_difference("@account.entries.count") do
post api_v1_transactions_url,
params: transaction_params_with(category_id: foreign_category.id),
headers: api_headers(@api_key)
end

assert_response :unprocessable_entity
end

test "should reject create with a merchant belonging to another family" do
foreign_merchant = families(:empty).merchants.create!(name: "Foreign Merchant", type: "FamilyMerchant")

assert_no_difference("@account.entries.count") do
post api_v1_transactions_url,
params: transaction_params_with(merchant_id: foreign_merchant.id),
headers: api_headers(@api_key)
end

assert_response :unprocessable_entity
end

test "should reject create with malformed merchant_id" do
assert_no_difference("@account.entries.count") do
post api_v1_transactions_url,
params: transaction_params_with(merchant_id: "not-a-uuid"),
headers: api_headers(@api_key)
end

assert_response :unprocessable_entity
end

test "should reject create with a tag belonging to another family" do
foreign_tag = families(:empty).tags.create!(name: "Foreign Tag")

assert_no_difference("@account.entries.count") do
post api_v1_transactions_url,
params: transaction_params_with(tag_ids: [ foreign_tag.id ]),
headers: api_headers(@api_key)
end

assert_response :unprocessable_entity
end

test "should reject update with malformed category_id instead of silently ignoring it" do
put api_v1_transaction_url(@transaction),
params: { transaction: { category_id: "18bsy6-" } },
headers: api_headers(@api_key)

assert_response :unprocessable_entity
assert_equal "validation_failed", JSON.parse(response.body)["error"]
end

test "should reject update with a category belonging to another family" do
foreign_category = families(:empty).categories.create!(name: "Foreign Category")

put api_v1_transaction_url(@transaction),
params: { transaction: { category_id: foreign_category.id } },
headers: api_headers(@api_key)

assert_response :unprocessable_entity
assert_not_equal foreign_category.id, @transaction.reload.category_id
end

# UPDATE action tests
test "should update transaction with valid parameters" do
update_params = {
Expand Down Expand Up @@ -834,6 +934,19 @@ def count_db_queries(&block)
queries
end

def transaction_params_with(overrides)
{
transaction: {
account_id: @account.id,
name: "Association Scoping Transaction",
amount: 25.00,
date: Date.current,
currency: "USD",
nature: "expense"
}.merge(overrides)
}
end

def create_transfer_between_accounts
from_account = @family.accounts.create!(
name: "Transfer From Account",
Expand Down
Loading