fix(api): scope holdings endpoints to accounts accessible by the token owner#2468
fix(api): scope holdings endpoints to accounts accessible by the token owner#2468cleanjunc wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthrough
ChangesHoldings API access control scoping
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d35b6e074
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| def index | ||
| family = current_resource_owner.family | ||
| holdings_query = family.holdings.joins(:account).where(accounts: { status: [ "draft", "active" ] }) | ||
| holdings_query = accessible_holdings |
There was a problem hiding this comment.
This changes the /api/v1/holdings list/show behavior to scope by accessible accounts, but the corresponding rswag spec and generated docs/api/openapi.yaml were not updated. The repository's AGENTS.md OpenAPI Documentation section marks spec/requests/api/v1/{resource}_spec.rb and regenerated OpenAPI docs as mandatory for changes under app/controllers/api/v1/, so the public API contract will not document the new accessible-account behavior for account_id/account_ids filtering or show responses until those docs are refreshed.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/controllers/api/v1/holdings_controller.rb (1)
39-41: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUnify not-found payload with shared API v1 contract.
Line 40 returns a custom 404 shape (
error: "not_found"), which can drift from the shared APIrecord_not_foundcontract and create client-side branching for the same status. Prefer lettingActiveRecord::RecordNotFoundbubble to the base handler (or match its payload exactly).♻️ Proposed fix
def set_holding `@holding` = accessible_holdings.find(params[:id]) -rescue ActiveRecord::RecordNotFound - render json: { error: "not_found", message: "Holding not found" }, status: :not_found end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/api/v1/holdings_controller.rb` around lines 39 - 41, The rescue block for ActiveRecord::RecordNotFound in the holdings_controller returns a custom error payload that diverges from the shared API v1 contract for record_not_found responses. Remove this rescue block entirely (the rescue ActiveRecord::RecordNotFound block at lines 39-41) and allow the exception to bubble up to the base/shared API error handler, which already handles this exception with the standardized payload format expected by API clients.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/controllers/api/v1/holdings_controller_test.rb`:
- Around line 50-78: Add two new test cases to the holdings controller test file
to cover missing behavioral scenarios. First, add a test similar to the existing
"account_ids filter cannot reach inaccessible accounts" test but using the
singular account_id parameter instead of account_ids to ensure the same security
restriction applies. Second, add a test that passes an invalid date parameter to
the API endpoint and verifies it returns a 422 Unprocessable Entity response,
ensuring proper validation of date inputs on the endpoint.
---
Nitpick comments:
In `@app/controllers/api/v1/holdings_controller.rb`:
- Around line 39-41: The rescue block for ActiveRecord::RecordNotFound in the
holdings_controller returns a custom error payload that diverges from the shared
API v1 contract for record_not_found responses. Remove this rescue block
entirely (the rescue ActiveRecord::RecordNotFound block at lines 39-41) and
allow the exception to bubble up to the base/shared API error handler, which
already handles this exception with the standardized payload format expected by
API clients.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aa81e0b8-20bf-41f6-a66f-fd7a121a6dd8
📒 Files selected for processing (2)
app/controllers/api/v1/holdings_controller.rbtest/controllers/api/v1/holdings_controller_test.rb
| test "account_ids filter cannot reach inaccessible accounts" do | ||
| get api_v1_holdings_url, | ||
| params: { account_ids: [ @inaccessible_account.id ] }, | ||
| headers: api_headers(@api_key) | ||
|
|
||
| assert_response :success | ||
| holding_ids = JSON.parse(response.body)["holdings"].map { |h| h["id"] } | ||
| assert_not_includes holding_ids, @inaccessible_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(@inaccessible_holding), headers: api_headers(@api_key) | ||
|
|
||
| assert_response :not_found | ||
| end | ||
|
|
||
| test "requires authentication" do | ||
| get api_v1_holdings_url | ||
|
|
||
| assert_response :unauthorized | ||
| end |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add missing API-v1 behavioral coverage for account_id and invalid date.
The suite currently guards account_ids but not account_id, and it misses the invalid-date 422 path. Both are important regression guards for this endpoint’s filter surface.
✅ Suggested test additions
+ test "account_id filter cannot reach an inaccessible account" do
+ get api_v1_holdings_url,
+ params: { account_id: `@inaccessible_account.id` },
+ headers: api_headers(`@api_key`)
+
+ assert_response :success
+ holding_ids = JSON.parse(response.body)["holdings"].map { |h| h["id"] }
+ assert_not_includes holding_ids, `@inaccessible_holding.id`
+ end
+
+ test "returns unprocessable entity for invalid date filter" do
+ get api_v1_holdings_url,
+ params: { date: "not-a-date" },
+ headers: api_headers(`@api_key`)
+
+ assert_response :unprocessable_entity
+ endAs per coding guidelines, test/controllers/api/v1/**/*_controller_test.rb must cover key API behaviors including invalid date (422) scenarios.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test "account_ids filter cannot reach inaccessible accounts" do | |
| get api_v1_holdings_url, | |
| params: { account_ids: [ @inaccessible_account.id ] }, | |
| headers: api_headers(@api_key) | |
| assert_response :success | |
| holding_ids = JSON.parse(response.body)["holdings"].map { |h| h["id"] } | |
| assert_not_includes holding_ids, @inaccessible_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(@inaccessible_holding), headers: api_headers(@api_key) | |
| assert_response :not_found | |
| end | |
| test "requires authentication" do | |
| get api_v1_holdings_url | |
| assert_response :unauthorized | |
| end | |
| test "account_ids filter cannot reach inaccessible accounts" do | |
| get api_v1_holdings_url, | |
| params: { account_ids: [ `@inaccessible_account.id` ] }, | |
| headers: api_headers(`@api_key`) | |
| assert_response :success | |
| holding_ids = JSON.parse(response.body)["holdings"].map { |h| h["id"] } | |
| assert_not_includes holding_ids, `@inaccessible_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(`@inaccessible_holding`), headers: api_headers(`@api_key`) | |
| assert_response :not_found | |
| end | |
| test "requires authentication" do | |
| get api_v1_holdings_url | |
| assert_response :unauthorized | |
| end | |
| test "account_id filter cannot reach an inaccessible account" do | |
| get api_v1_holdings_url, | |
| params: { account_id: `@inaccessible_account.id` }, | |
| headers: api_headers(`@api_key`) | |
| assert_response :success | |
| holding_ids = JSON.parse(response.body)["holdings"].map { |h| h["id"] } | |
| assert_not_includes holding_ids, `@inaccessible_holding.id` | |
| end | |
| test "returns unprocessable entity for invalid date filter" do | |
| get api_v1_holdings_url, | |
| params: { date: "not-a-date" }, | |
| headers: api_headers(`@api_key`) | |
| assert_response :unprocessable_entity | |
| end |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/controllers/api/v1/holdings_controller_test.rb` around lines 50 - 78,
Add two new test cases to the holdings controller test file to cover missing
behavioral scenarios. First, add a test similar to the existing "account_ids
filter cannot reach inaccessible accounts" test but using the singular
account_id parameter instead of account_ids to ensure the same security
restriction applies. Second, add a test that passes an invalid date parameter to
the API endpoint and verifies it returns a 422 Unprocessable Entity response,
ensuring proper validation of date inputs on the endpoint.
Source: Coding guidelines
1 participant
Summary
The
/api/v1/holdingslist and show endpoints scoped results to the whole family instead of the accounts the API token owner can actually access. A read scoped key could therefore read holdings (account name, quantity, price, market value) for accounts owned by other family members that were never shared with the key owner, and theaccount_id/account_idsfilters made targeted enumeration easy.This brings the holdings API in line with the balances API and the web holdings controller, both of which already scope through
accounts.accessible_by.Fixes #2467
What changed
accessible_holdingsscope that restricts holdings to accounts the token owner owns or has been shared, while keeping the existingdraft/activestatus filter.indexandset_holdingthrough that scope, so the list, the show action, and theaccount_id/account_idsfilters all respect access boundaries.Why
Holdings expose financial position data, so the leak is a real privacy gap between members of the same family. Scoping through
accessible_bymatches the established pattern and keeps the access rules consistent across the codebase.Testing
Added
test/controllers/api/v1/holdings_controller_test.rb, covering list scoping, theaccount_idsenumeration vector, show for an accessible holding, a not found response for an inaccessible holding, and the authentication requirement.Summary by CodeRabbit
Bug Fixes
Tests