Skip to content

fix(agent): authorize create with a linked one-to-one on the foreign collection - #353

Merged
PMerlet merged 3 commits into
mainfrom
feature/prd-921-agent-ruby-create-with-a-linked-one-to-one-writes-the
Aug 13, 2026
Merged

fix(agent): authorize create with a linked one-to-one on the foreign collection#353
PMerlet merged 3 commits into
mainfrom
feature/prd-921-agent-ruby-create-with-a-linked-one-to-one-writes-the

Conversation

@PMerlet

@PMerlet PMerlet commented Aug 12, 2026

Copy link
Copy Markdown
Member

Fixes PRD-921. Ruby counterpart of PRD-916, fixed in agent-nodejs #1819.

The bug

POST /forest/:collection with a linked OneToOne / PolymorphicOneToOne relation asserted only add on the parent collection, then updated the foreign collection with no authorization and no scope:

condition_tree = ConditionTreeFactory.match_records(foreign_collection, [primary_key_values])
foreign_collection.update(context.caller, Filter.new(condition_tree: condition_tree), patch)

A user with add on books but no edit on passports could rewrite the foreign key of an arbitrary passport — stealing it from its owner — including a passport outside their scope. Worse than the Node instance on two counts: Node at least asserted edit (on the wrong side) and kept the writes scope-bounded.

The fix

  • Assert edit on the foreign collection, once per linked one-to-one relation, and before the parent record is created — a denied link writes nothing at all, and a denial on one relation no longer lets writes on the others through. The parent-side add check is unchanged.
  • Intersect get_scope(foreign_collection) into the update filter, mirroring create_new_one_to_one_relationship in update_related.rb (PRD-908, fix(agent): scope and authorize related write routes on the mutated collection #350).
  • Resolve everything the linking pass needs — the edit assertion, the scope, and the unpacked linked id — in a single pre-create pass, and carry it in the relation hash. get_scope reads the forest.rendering cache that nothing earlier in this route warms, and unlike can? it has no permission_system? bypass; leaving it below create meant a cold cache, a Forest API 5xx or an unresolvable team/user raised after the parent row was committed, so the 500 the caller retried duplicated the parent. Same shape for a malformed packed id. The "a rejected link writes nothing at all" invariant now holds for failed lookups, not only for denied permissions.
  • Single linked_one_to_one_relations helper feeding both the permission pass and the linking pass, so widening the guard on one side can no longer silently drop the check on the other.

Observable contract when the scope excludes the target

A caller linking a foreign record outside their scope gets a 201 with the parent created and the relation absent — the intersected filter matches no row and entity&.update! is a no-op, indistinguishable from linking an id that does not exist. This is deliberate: it is what agent-nodejs #1819 and create_new_one_to_one_relationship in update_related.rb both do, and the PRD prescribes the mechanism without asking for a 403. Counting first to raise ForbiddenError would diverge from Node and cost an extra query; the contract is instead pinned by a test and stated at the call site.

Two defects on the rewritten lines, fixed along the way and each pinned by a test — flagging them explicitly as they sit outside the strict scope of the PRD:

  • the linked id was unpacked against context.collection (the parent) while the match_records that follows targets the foreign collection, so the filter carried the wrong primary key names as soon as the two sides disagreed;
  • a one-to-one relationship carrying data: null raised a NoMethodError (value['data']['id']); it is now ignored, matching Node's linked !== null.

Deliberate divergence from Node

Node runs two updates (null the old foreign-key owner, then claim the new one); this route keeps its single update. On a create, origin_value is the id of a record that has just been born, so no foreign record can already carry it — Node's first update is a no-op costing one query. The PRD notes the same, and it has no bearing on the authorization gap.

Tests

store_spec.rb: scope intersected (one-to-one and polymorphic), a denied edit creating neither parent nor link, 403 raised before the linked id is parsed (the invariant PRD-908 established on update_related.rb), a failed scope lookup and a malformed linked id each creating neither parent nor link, the silent no-op when the scope excludes the target, data: null ignored, and the parent-side add check pinned so removing it would not go unnoticed.

The polymorphic fixture carries origin_type_value: 'Person::Legacy' on a collection named person, so the addressable_type assertion pins context.collection.name as the source of the type column instead of passing under either candidate.

Permission assertions go through a collected [action, collection_name] list rather than having_attributes on the collection instances: on failure, rspec inspects the decorator chain and the run hangs instead of reporting.

Every new assertion was checked against the faulty implementation it guards — the pre-fix route, unpack_id hoisted back above the permission check, the parent collection restored as the unpack target, get_scope and unpack_id dropped back below create, and schema.origin_type_value swapped in for the collection name — and each mutation is caught.

842 examples, 0 failures on forest_admin_agent; rubocop clean.

🤖 Generated with Claude Code

Note

Authorize edit on linked one-to-one foreign collections before creating a record

  • The Store#handle_request action now resolves linked one-to-one (and polymorphic one-to-one) relations from the request payload before creating the main record, and calls can?(:edit) on each foreign collection, aborting if any permission is denied.
  • link_one_to_one_relations is refactored to accept a precomputed relations array and now unpacks the foreign record's primary key against the foreign collection (not the current collection), fixing incorrect record matching.
  • Scope from permissions.get_scope is intersected into the update filter for each foreign collection, ensuring scoped access controls are respected during relation linking.
  • One-to-one relationships with nil data ids are skipped rather than processed.

Changes since #353 opened

  • Added authorization checks for one-to-one linked relations during record creation [7a1a75c]
  • Removed inline comments from test files [7a1a75c]

Macroscope summarized 222e53d.

@linear-code

linear-code Bot commented Aug 12, 2026

Copy link
Copy Markdown

PRD-921

@qltysh

qltysh Bot commented Aug 12, 2026

Copy link
Copy Markdown

All good ✅

Comment thread packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/store.rb Outdated
…collection

The create route only asserted `add` on the parent collection, then updated
the foreign collection with no authorization and no scope: a caller with
`add` on books but no `edit` on passports could rewrite the foreign key of
any passport, including one outside their scope.

Assert `edit` on the foreign collection, once per linked one-to-one relation
and before the parent record is created, so a denied link writes nothing at
all instead of leaving an orphan parent behind. Intersect the foreign
collection's scope into the update filter, as update_related.rb already does
since PRD-908 (#350). The parent-side `add` check is unchanged.

Two defects on the rewritten lines, fixed along the way:
- the linked id was unpacked against the parent collection instead of the
  foreign one, producing a filter on the wrong primary key names;
- a one-to-one relationship carrying `data: null` raised a NoMethodError.

Fixes PRD-921.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@PMerlet
PMerlet force-pushed the feature/prd-921-agent-ruby-create-with-a-linked-one-to-one-writes-the branch from cab50b8 to 222e53d Compare August 12, 2026 14:42
@qltysh

qltysh Bot commented Aug 12, 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
...t_admin_agent/lib/forest_admin_agent/routes/resources/store.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.

Comment thread packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/store.rb Outdated
PMerlet and others added 2 commits August 13, 2026 11:46
…g the record

Brian's review on #353: the pre-create pass asserted `edit` on the foreign
collection but left `get_scope` and `unpack_id` below `create`. `get_scope`
reads the `forest.rendering` cache, which nothing earlier in the route warms,
and has no `permission_system?` bypass — so a cold cache, a 5xx from the Forest
API or an unresolvable team/user raised after the parent row was committed,
returning a 500 whose retry duplicated the parent. A malformed packed id had
the same shape.

Both are now resolved in the same pass as the `edit` assertion and carried in
the relation hash, so the "a rejected link writes nothing at all" invariant
holds for failed lookups too, not just denied permissions.

Also from the review:
- pin the silent no-op contract when the caller's scope excludes the linked
  record (a created parent and no relation, matching update_related.rb and
  agent-nodejs), which no test covered;
- give the polymorphic fixture an `origin_type_value` that differs from the
  collection name, so the test pins `context.collection.name` as the source of
  the type column instead of passing under either candidate.

Splitting the relation lookup in two also clears the qlty complexity flag on
`linked_one_to_one_relations`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The intent belongs in the names and the call order: the method is
`authorized_linked_one_to_one_relations`, and it sits above
`context.collection.create` in `handle_request`. The silent drop when the scope
excludes the linked record is stated by the test that pins it and by the PR
description, not by a comment on the linking method.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@PMerlet
PMerlet merged commit 9577d80 into main Aug 13, 2026
56 checks passed
@PMerlet
PMerlet deleted the feature/prd-921-agent-ruby-create-with-a-linked-one-to-one-writes-the branch August 13, 2026 10:16
forest-bot added a commit that referenced this pull request Aug 13, 2026
## [1.38.1](v1.38.0...v1.38.1) (2026-08-13)

### Bug Fixes

* **agent:** authorize create with a linked one-to-one on the foreign collection ([#353](#353)) ([9577d80](9577d80)), closes [#350](#350)
@forest-bot

Copy link
Copy Markdown
Member

🎉 This PR is included in version 1.38.1 🎉

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