fix(api): scope valuations create/show/update by account-level access

[galuis116]

Summary

Api::V1::ValuationsController#create resolved the target account through current_resource_owner.family.accounts.find(...), and #set_valuation (used by #show + #update) resolved the entry through the bare family scope. Both ignored Account.accessible_by / Account.writable_by, so a family member with a read_write API key could write reconciliation valuations against unshared accounts and could read/mutate valuations on accounts shared with them as read_only. The #index action already scopes via accessible_by (line 15), and the sister Api::V1::TransactionsController already applies the same pattern in create (line 93) and set_transaction (line 209) — this is the missing alignment on the valuations resource.

Fixes #1972.

Fix

• create resolves the account through family.accounts.writable_by(current_resource_owner).find(...). Unshared or read-only accounts surface as ActiveRecord::RecordNotFound and use the existing 404 rescue, avoiding existence-leak.
• set_valuation constrains entries to account_id IN family.accounts.accessible_by(user).select(:id). Valuations on unshared accounts return 404.
• update gets a new private ensure_writable_entry registered as before_action ... only: :update. It returns 403 when the loaded entry's account is not in writable_by(user) — so read-only-shared accounts remain readable via show but not mutable via update.

Tests

Adds 7 Minitest cases at the end of test/controllers/api/v1/valuations_controller_test.rb:

• create on an unshared account → 404, no row written
• create on a read-only-shared account → 404, no row written
• create on a full-control-shared account → 201, row written (happy path)
• show of a valuation on an unshared account → 404
• update of a valuation on a read-only-shared account → 403, amount unchanged
• show of a valuation on a full-control-shared account → 200 (happy path)

A small member_api_key(scopes:) helper builds a per-test read_write key for family_member (Jakob) and clears its Redis rate-limit slot. The existing account_shares fixtures (depository_shared_with_member full_control, credit_card_shared_with_member read_only) provide the share matrix without needing new fixtures.

Files changed

• app/controllers/api/v1/valuations_controller.rb — add writable_by / accessible_by scopes to create + set_valuation; add ensure_writable_entry before_action for update.
• test/controllers/api/v1/valuations_controller_test.rb — 7 new family-sharing scope tests + private member_api_key helper.

CI / pre-PR checks

The standard CI gate from CLAUDE.md (bin/rails test, bin/rubocop -f github -a, bin/brakeman --no-pager) is required to pass before merge; GitHub Actions will run these on push. The fix does not change any *.erb so erb_lint has no impact, and no OpenAPI schema changes are introduced (only adds a 403/404 error response shape that mirrors TransactionsController).

[Allow edits from maintainers] is enabled on this PR.

Summary by CodeRabbit

Release Notes

• Bug Fixes
  • Enforced write-access authorization for valuation updates to prevent unauthorized modifications
  • Tightened account-scoping to restrict users to only accounts they have write permissions for
  • Added 403 Forbidden response for unauthorized account access attempts

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR fixes a security bug in the Valuations API controller where create, show, and update actions bypassed per-account share permissions. Account lookups are now scoped to writable accounts in create, entry lookups filter by accessible accounts in set_valuation, and update is guarded by a writable-access check returning 403 when access is denied.

Changes

Valuations API Account Access Enforcement

Layer / File(s)	Summary
Update action authorization guard app/controllers/api/v1/valuations_controller.rb	Introduces before_action :ensure_writable_entry, only: :update and implements the ensure_writable_entry private method that verifies the current resource owner has writable access to the entry's account, returning 403 forbidden JSON when not.
Account and entry lookup scoping app/controllers/api/v1/valuations_controller.rb	create now resolves the target account through writable_by(current_resource_owner) instead of any family account. set_valuation now filters entries by account_id in the current resource owner's accessible accounts instead of only by entryable_type.
Authorization test coverage test/controllers/api/v1/valuations_controller_test.rb	Added family-sharing scope tests covering create rejection on unshared and read-only accounts, create permission on full-control accounts, show rejection on unshared accounts, update rejection on read-only accounts (with unchanged amount assertion), and show permission on shared accounts. Introduced member_api_key(scopes:) helper to create fresh API keys with cleared Redis rate-limit state.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• we-promise/sure#745: Both PRs modify app/controllers/api/v1/valuations_controller.rb around valuation scoping/authorization—main PR tightens account/entry write access for update/lookup, while the retrieved PR introduces the controller's initial ensure_*_scope and set_valuation flows.

Suggested labels

codex, pr:verified

Suggested reviewers

• jjmata

Poem

🐰 A rabbit hops through API gates,
where shares and scopes align just right—
no member now can twist the weights
of accounts not shared in sight.
The locks are checked, the tests run true,
a safer family for me and you! 🔐

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: scoping valuations API actions by account-level access control.
Linked Issues check	✅ Passed	All objectives from issue #1972 are met: create now scopes by writable_by, show/update scope by accessible_by via set_valuation, and update verifies writable_by before mutation with 403 response.
Out of Scope Changes check	✅ Passed	All changes directly address the authorization scoping requirements in issue #1972; no unrelated modifications detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

test/controllers/api/v1/valuations_controller_test.rb (1)

471-485: 💤 Low value

Consider adding explicit test for show on read-only shared account.

The update test for read-only accounts (line 451) implicitly proves that show works on read-only shares (since set_valuation succeeds before ensure_writable_entry returns 403). However, an explicit positive test for show on a read-only account would make the test suite more self-documenting and ensure accessible_by behavior is directly verified for the read case.

🧪 Optional test to add

test "should allow show of valuation on a read-only-shared family account" do
  member_key = member_api_key(scopes: [ "read_write" ])
  read_only_account = accounts(:credit_card) # shared :read_only with member
  entry = read_only_account.entries.valuations.first ||
          read_only_account.entries.valuations.create!(
            date: 5.days.ago.to_date,
            amount: 500,
            currency: "USD",
            name: "Read-only valuation",
            entryable: Valuation.new(kind: "reconciliation")
          )

  get api_v1_valuation_url(entry), headers: api_headers(member_key)
  assert_response :success
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/valuations_controller_test.rb` around lines 471 -
485, Add a new test that mirrors the existing "should allow show of valuation on
a shared family account" but targets the read-only shared account fixture (use
accounts(:credit_card) or the read-only fixture), create or reuse a valuations
entry via read_only_account.entries.valuations.create! (date, amount, currency,
name, entryable: Valuation.new(kind: "reconciliation")), call get
api_v1_valuation_url(entry) with headers: api_headers(member_api_key(scopes:
["read_write"])), and assert_response :success to explicitly verify that
Valuation#show is accessible for read-only shared accounts.

🤖 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.

Nitpick comments:
In `@test/controllers/api/v1/valuations_controller_test.rb`:
- Around line 471-485: Add a new test that mirrors the existing "should allow
show of valuation on a shared family account" but targets the read-only shared
account fixture (use accounts(:credit_card) or the read-only fixture), create or
reuse a valuations entry via read_only_account.entries.valuations.create! (date,
amount, currency, name, entryable: Valuation.new(kind: "reconciliation")), call
get api_v1_valuation_url(entry) with headers: api_headers(member_api_key(scopes:
["read_write"])), and assert_response :success to explicitly verify that
Valuation#show is accessible for read-only shared accounts.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8a235643-0a4e-4cd3-a0c2-7da57151e184

📥 Commits

Reviewing files that changed from the base of the PR and between 89f4249 and a8295f8.

📒 Files selected for processing (2)

• app/controllers/api/v1/valuations_controller.rb
• test/controllers/api/v1/valuations_controller_test.rb