Skip to content

fix(agent): authorize associate-related on the foreign collection and scope the many-to-many link - #356

Merged
PMerlet merged 4 commits into
mainfrom
feature/prd-927-agent-ruby-associate-related-authorizes-edit-on-the-parent
Aug 13, 2026
Merged

fix(agent): authorize associate-related on the foreign collection and scope the many-to-many link#356
PMerlet merged 4 commits into
mainfrom
feature/prd-927-agent-ruby-associate-related-authorizes-edit-on-the-parent

Conversation

@PMerlet

@PMerlet PMerlet commented Aug 13, 2026

Copy link
Copy Markdown
Member

Fixes PRD-927. Ruby counterpart of PRD-922, fixed in agent-nodejs #1820.

The bug

POST /forest/:collection/:id/relationships/:relation asserted edit on the parent collection, then mutated the child one — the OneToMany branches update the child's origin key, the ManyToMany branch creates a through row.

A user with edit on companies but none on users (relation companies.users, origin key users.company_id) could reassign a users row they cannot edit. dissociate_related already authorizes the child collection since #350, so associating was the cheaper inverse of a write that required more.

The ManyToMany branch also resolved its target through Collection.get_value, which short-circuits on a primary key and never queries, so the through row was created with no scope check at all. #350 excluded that branch explicitly.

The fix, one commit per defect

  1. Authorize the mutated collection. can?(:edit, context.child_collection), above unpack_id so a denied caller still gets a 403 rather than an id-validation error.
  2. Scope the ManyToMany link. The target is resolved through list with match_ids(target) ∩ get_scope(child_collection), and the through row is skipped when it comes back empty.
  3. Write the relation's key targets, not the primary keys. associate_many_to_many built the through record from Schema.primary_keys(...)[0] on both sides. ActiveRecord derives foreign_key_target / origin_key_target from an association's primary_key: option, so has_and_belongs_to_many :addresses, primary_key: :reference was enough to write a primary key into a column holding another key — and make_through_filter, which dissociate_related builds on, already read the targets, so associate wrote rows that dissociate could not match back. No-op wherever the targets default to the primary key.
  4. Match every column of a composite key. The two OneToMany branches identified their target with a leaf on the first primary key, so on a composite-key child the update reassigned every row sharing that column, and a sibling row in scope was enough to let an out-of-scope target through. All three branches now build their filter through one target_in_scope_filter, which also stops them from drifting apart on the scope again.

Contract when the scope excludes the target

204, nothing written — as in agent-nodejs #1820 and as in the OneToMany branches, whose intersected filter simply matches no row. Pinned by a test.

Notes for the reviewer

  • match_ids requires Equal/In on the child's primary key, which the leaf did not. Every datasource in the tree declares them (ActiveRecord, Mongoid, BASE_OPERATORS in hasura, STRING_OPS/NUMBER_OPS in zendesk, ID_OPS in mambu's reconcile_filter_operators!). A hand-written datasource declaring filter_operators: [] on a primary key would 500 here, but it is already broken on show and dissociate, which both go through match_ids.
  • origin_value is still read on the parent without scope, as on Node — out of scope for this PRD.

Tests

edit asserted on the child and never on the parent in all three branches, 403 raised before the target id is parsed, the scope-intersected filter, no link when the target is out of scope, and two new fixtures: a ManyToMany whose keys target code / reference, and a composite-key note collection reached both through a OneToMany and through a note_user through collection.

Every assertion was checked against the faulty implementation it guards — permission back on the parent, scope dropped, guard removed, key targets and the shared filter reverted — and each mutation is caught in a couple of seconds.

Permission and scope assertions compare collection names inside a have_received do |...| block instead of passing the collection to having_attributes: rspec formats the decorator chain on failure, and the example then takes minutes and reports no failure at all. #353 hit the same wall.

849 examples, 0 failures on forest_admin_agent; bundle exec rubocop clean over the package.

🤖 Generated with Claude Code

… scope the many-to-many link

The associate-related route asserted `edit` on the parent collection, then
mutated the child one: the OneToMany branches update the child's origin key
and the ManyToMany branch creates a through row. A user with `edit` on
`companies` but none on `users` could reassign a `users` row through
`POST /forest/companies/:id/relationships/users`. Dissociate already
authorizes the child collection (#350), so associating was the cheaper
inverse of a write that required more.

The ManyToMany branch also resolved its target through `get_value`, which
short-circuits on a primary key and never queries, so the link ignored the
child scope entirely. It now resolves the target through a scope-intersected
`list` and skips the through row when the target is out of scope, as
agent-nodejs does.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@linear-code

linear-code Bot commented Aug 13, 2026

Copy link
Copy Markdown

PRD-927

@qltysh

qltysh Bot commented Aug 13, 2026

Copy link
Copy Markdown

Qlty


Coverage Impact

This PR will not change total coverage.

Modified Files with Diff Coverage (1)

RatingFile% DiffUncovered Line #s
Coverage rating: A Coverage rating: A
...rest_admin_agent/routes/resources/related/associate_related.rb100.0%
Total100.0%
🚦 See full report on Qlty Cloud »

🛟 Help
  • Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.

  • Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.

  • File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)

    • Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

PMerlet and others added 3 commits August 13, 2026 15:13
…h record

associate_many_to_many resolved both sides of the through record through
`Schema.primary_keys(...)[0]` instead of the relation's declared key targets,
so a ManyToMany whose keys point at another column wrote primary-key values
into columns holding a different key. ActiveRecord derives those targets from
`association_primary_key` and `join_foreign_key`, both driven by the
`primary_key:` option, so `has_and_belongs_to_many :addresses,
primary_key: :reference` is enough to reach it.

The read path already used the targets: make_through_filter, which
dissociate-related builds on, reads `origin_key_target` on the parent and
projects `foreign_key_target` on the target. So associate wrote rows that
dissociate and list could not match back.

The target is now projected on `foreign_key_target` in the scope-intersected
list, and the origin value read on `origin_key_target`, as agent-nodejs does.
Identifying the target still matches on the primary key, since the packed id
in the route comes from there.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
associate_one_to_many and associate_polymorphic_one_to_many identified their
target with a leaf on the first primary key column only, so on a child with a
composite primary key the update reassigned every row sharing that column, and
a sibling row in scope was enough to let an out-of-scope target through.

All three branches now build the target filter through one `target_in_scope_filter`
helper: `match_ids(target) ∩ get_scope(child_collection)`. For a single-column
primary key `match_ids` yields the same Equal leaf, so nothing changes there --
only the composite case is fixed, and the three branches can no longer drift
apart on the scope, which is what this series of tickets keeps running into.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ite key

The shared target filter was only pinned on the OneToMany branch, so the
composite-key mutation was caught by one example out of the three branches
that share it. A `note_user` through collection linking a composite-key
`note` now pins the ManyToMany branch too: the scoped list carries every key
column plus the scope, projects `foreign_key_target`, and the through record
is built from the value it returns.

A through foreign key can only carry one column, so the relation targets a
single column of the composite key -- which is exactly why the link still has
to be matched on all of them.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@PMerlet
PMerlet force-pushed the feature/prd-927-agent-ruby-associate-related-authorizes-edit-on-the-parent branch from 1c477a0 to 9b3b37e Compare August 13, 2026 13:14
@PMerlet
PMerlet merged commit 2f618b9 into main Aug 13, 2026
56 checks passed
@PMerlet
PMerlet deleted the feature/prd-927-agent-ruby-associate-related-authorizes-edit-on-the-parent branch August 13, 2026 13:45
forest-bot added a commit that referenced this pull request Aug 13, 2026
## [1.38.2](v1.38.1...v1.38.2) (2026-08-13)

### Bug Fixes

* **agent:** authorize associate-related on the foreign collection and scope the many-to-many link ([#356](#356)) ([2f618b9](2f618b9)), closes [#350](#350)
@forest-bot

Copy link
Copy Markdown
Member

🎉 This PR is included in version 1.38.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants