feat(resolve): add opt-in pre-fetch field authorization mode - #1561
Conversation
Adds an opt-in mode that authorizes protected fields (@requiresScopes / @authenticated) in a single up-front batch before any subgraph fetch, instead of filtering them from the response after the fetch. This avoids unnecessary subgraph work for unauthorized selections and emits a consistent UNAUTHORIZED_FIELD_OR_TYPE error even when a protected field resolves to an empty list / no objects. - New BatchAuthorizer interface + AuthorizationDecision (value, explicit allow/deny) - Context.SetPreFetchFieldAuthorizer enables the mode by presence (no bool flag) - Plan-time coordinate collection via Plan.CollectAuthorizationCoordinates - Up-front batch in ResolveGraphQLResponse seeds the decision cache; the loader and resolvable become pure readers, with per-operation-type enforcement (query: prune/partial; mutation & subscription: whole-fetch reject) - Empty/null-parent emission closes the no-error-on-empty-list gap for all operation types - Authorization code extracted into resolvable_authorization.go (100% covered) Default off; existing post-fetch authorization behavior is byte-for-byte unchanged. Refs ENG-9828. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds opt-in pre-fetch field authorization across plan processing, resolver execution, and loader fetch decisions. It also collects protected field coordinates during postprocessing, emits authorization errors for unreached data, and expands test coverage for the new flow. ChangesPre-fetch field authorization
Estimated code review effort: 4 (Complex) | ~75 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
v2/pkg/engine/resolve/context.go (1)
195-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCarry the contract invariants from PLAN.md into the GoDoc.
PLAN.md §3.2 specifies important invariants for this new public interface — decisions must be index-aligned with
coordinates, contain exactly one entry per coordinate, and be a pure function of(coordinate, request scopes)with no subgraph input/data — but none of that made it into the shipped comments. SinceBatchAuthorizeris meant to be implemented by external code (e.g. a future routerCosmoAuthorizer), documenting these constraints directly on the interface/struct reduces the risk of a misaligned or impure implementation causing silent authorization mismatches.📝 Proposed doc enrichment
-// AuthorizationDecision is an explicit allow/deny decision for a single field coordinate. +// AuthorizationDecision is an explicit allow/deny decision for a single field coordinate. type AuthorizationDecision struct { - Allowed bool - Reason string + // Allowed reports whether the coordinate is authorized for the request. + Allowed bool + // Reason optionally explains a denial. Only meaningful when Allowed is false. + Reason string } -// BatchAuthorizer authorizes field coordinates in one call before execution. +// BatchAuthorizer authorizes a set of field coordinates in a single call, before execution. +// decisions must be index-aligned with coordinates and contain exactly one decision per +// coordinate. Decisions must be a pure function of the coordinate and the request context +// (e.g. token scopes); no subgraph input or response data is provided, by design. type BatchAuthorizer interface { AuthorizeFields(ctx *Context, coordinates []GraphCoordinate) (decisions []AuthorizationDecision, err error) }🤖 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 `@v2/pkg/engine/resolve/context.go` around lines 195 - 204, Update the GoDoc on AuthorizationDecision and BatchAuthorizer to include the PLAN.md §3.2 contract invariants: AuthorizeFields must return exactly one decision per input coordinate, in the same order/index alignment as coordinates, and each decision must be derived only from the coordinate and request scopes with no subgraph input or execution data. Add this guidance directly on the AuthorizationDecision struct and BatchAuthorizer.AuthorizeFields method so external implementers like CosmoAuthorizer can follow the required behavior.v2/pkg/engine/plan/plan.go (1)
14-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider an optional interface instead of extending the public
Planinterface.Adding
CollectAuthorizationCoordinates()toPlanforces every external implementer of this exported interface to add the method, which is a breaking API change for library consumers with customPlanimplementations. A type-assertion at the single call site inplanner.go(if c, ok := plan.(interface{ CollectAuthorizationCoordinates() }); ok { c.CollectAuthorizationCoordinates() }) would achieve the same behavior without widening the required interface surface.♻️ Optional interface pattern
type Plan interface { PlanKind() Kind SetFlushInterval(interval int64) GetCostCalculator() *CostCalculator SetCostCalculator(calc *CostCalculator) - // CollectAuthorizationCoordinates populates the plan's response with the field coordinates that - // require an authorization decision, so pre-fetch field authorization can resolve them up front. - CollectAuthorizationCoordinates() } + +// authorizationCoordinateCollector is implemented by plans that can populate +// pre-fetch authorization coordinates. +type authorizationCoordinateCollector interface { + CollectAuthorizationCoordinates() +}🤖 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 `@v2/pkg/engine/plan/plan.go` around lines 14 - 22, Remove CollectAuthorizationCoordinates() from the exported Plan interface and switch the planner call site to an optional interface type assertion. Keep the behavior in planner.go by checking whether the concrete plan implements a local interface with CollectAuthorizationCoordinates() before invoking it, so existing external Plan implementations are not forced to change. Use the Plan interface and the planner.go call site as the main symbols to update.
🤖 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 `@v2/pkg/engine/resolve/authorization.go`:
- Around line 66-82: The coordinate collection in
collectNodeAuthorizationCoordinates only seeds authorization for the first
source in field.Info.Source.IDs, so merged protected fields with multiple
resolving sources are missed. Update the Object branch to iterate over every ID
in field.Info.Source.IDs and call addAuthorizationCoordinate for each one, while
keeping the existing GraphCoordinate built from field.Info.ExactParentTypeName
and field.Info.Name.
In `@v2/pkg/engine/resolve/resolvable_authorization.go`:
- Around line 81-83: The authorizationDecisionID helper builds cache keys by
concatenating dataSourceID, coordinate.TypeName, and coordinate.FieldName
without separators, which can make different coordinates hash the same input.
Update authorizationDecisionID to include unambiguous delimiters or another
structured encoding before calling xxhash.Sum64String, so each unique
GraphCoordinate/dataSourceID tuple maps to a distinct cache key.
In `@v2/pkg/engine/resolve/resolve.go`:
- Around line 416-420: Scope the inbound single-flight to the authorization
policy used by authorizeFieldsPreFetch in resolve.go, because GetOrCreate
currently dedupes only by request ID, variables hash, and header hash and
followers can reuse a response authorized under a different policy. Update the
keying or inflight selection in the resolve flow around
r.inboundRequestSingleFlight.GetOrCreate and the authorizeFieldsPreFetch call so
requests with different auth policies do not share the same inflight result.
---
Nitpick comments:
In `@v2/pkg/engine/plan/plan.go`:
- Around line 14-22: Remove CollectAuthorizationCoordinates() from the exported
Plan interface and switch the planner call site to an optional interface type
assertion. Keep the behavior in planner.go by checking whether the concrete plan
implements a local interface with CollectAuthorizationCoordinates() before
invoking it, so existing external Plan implementations are not forced to change.
Use the Plan interface and the planner.go call site as the main symbols to
update.
In `@v2/pkg/engine/resolve/context.go`:
- Around line 195-204: Update the GoDoc on AuthorizationDecision and
BatchAuthorizer to include the PLAN.md §3.2 contract invariants: AuthorizeFields
must return exactly one decision per input coordinate, in the same order/index
alignment as coordinates, and each decision must be derived only from the
coordinate and request scopes with no subgraph input or execution data. Add this
guidance directly on the AuthorizationDecision struct and
BatchAuthorizer.AuthorizeFields method so external implementers like
CosmoAuthorizer can follow the required behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a90344dc-91b5-4258-9109-133f460b334e
📒 Files selected for processing (13)
PLAN.mdv2/pkg/engine/datasourcetesting/datasourcetesting.gov2/pkg/engine/plan/plan.gov2/pkg/engine/plan/planner.gov2/pkg/engine/resolve/authorization.gov2/pkg/engine/resolve/authorization_prefetch_test.gov2/pkg/engine/resolve/context.gov2/pkg/engine/resolve/loader.gov2/pkg/engine/resolve/resolvable.gov2/pkg/engine/resolve/resolvable_authorization.gov2/pkg/engine/resolve/resolvable_authorization_test.gov2/pkg/engine/resolve/resolve.gov2/pkg/engine/resolve/response.go
- Guard pre-fetch authorization behind !SkipLoader so query-plan-only responses do not invoke the authorizer (Codex) - Collect authorization coordinates from RawFetches as well, since the fetch tree is only built by the post-processor after planning (Codex) - Seed a coordinate for every Source.ID on merged/@Shareable protected fields (CodeRabbit) - Delimit authorization cache-key components with NUL to avoid hash collisions (CodeRabbit) - Make coordinate collection nil-safe; add regression tests for the RawFetches and SkipLoader paths Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pre-fetch field authorization for a subscription now evaluates the protected root field before starting/adding the trigger (in ResolveGraphQLSubscription and AsyncResolveGraphQLSubscription), so an unauthorized subscription never opens or holds an upstream subgraph subscription. Previously the batch check only ran per update in executeSubscriptionUpdate, after the trigger was already started. (Codex) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@v2/pkg/engine/resolve/resolve.go`:
- Around line 524-529: The subscription root authorization check in resolveField
currently treats any decision count other than exactly one as allowed, so update
the preFetchFieldAuthorizer.AuthorizeFields handling to fail closed when the
batch size is wrong. In the resolveField path, mirror the pre-fetch query
validation by checking that decisions has exactly one entry before inspecting
decisions[0].Allowed, and return an error if the authorizer returns zero or
multiple decisions instead of proceeding.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3d55b9f7-bef6-4ada-936b-68a7d765662b
📒 Files selected for processing (2)
v2/pkg/engine/resolve/authorization_prefetch_test.gov2/pkg/engine/resolve/resolve.go
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/engine/resolve/authorization_prefetch_test.go
authorizeSubscriptionPreFetch now returns an error when the batch authorizer returns a decision count other than 1 for the single subscription root coordinate, instead of treating it as authorized. Matches the parity check on the pre-fetch query path. (CodeRabbit) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tion Run the unreached-data authorization error sweep before the resolvable walk instead of after it. The walk nulls a parent when a denied non-null child propagates up; running the sweep afterwards treated that just-nulled parent as unreached and re-emitted the same UNAUTHORIZED_FIELD_OR_TYPE error. Running the sweep first makes "unreached" reflect the origin response (empty list / null parent), complementary to what the walk reaches, so each denial is reported once. (Codex) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…uble-auth - Add a doc comment to CollectAuthorizationCoordinates describing what it collects, when it runs, its sources, and the empty=no-op behavior. - Document on Resolvable.authorize how the seeded allow/deny cache short-circuits the post-fetch AuthorizeObjectField call, so a field is never authorized twice under pre-fetch field authorization. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a plan-package test that runs Planner.Plan and asserts how the planned response's Info.AuthorizationCoordinates is populated by CollectAuthorizationCoordinates: empty with no authorization rules, collected from the fetch for a protected root field, collected from the data tree for a protected nested field, and deduplicated + sorted for multiple protected fields. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-pre-fetch-scope-authorization
…-pre-fetch-scope-authorization # Conflicts: # v2/pkg/engine/plan/plan.go # v2/pkg/engine/resolve/resolve.go
…-pre-fetch-scope-authorization
🤖 I have created a release *beep* *boop* --- ## [2.10.0](v2.9.2...v2.10.0) (2026-07-09) ### Features * **resolve:** add opt-in pre-fetch field authorization mode ([#1561](#1561)) ([eb3b142](eb3b142)) ### Bug Fixes * do not charge actual cost for denied fields ([#1582](#1582)) ([00d9f66](00d9f66)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: wundergraph-bot[bot] <285992168+wundergraph-bot[bot]@users.noreply.github.com>
What
Adds an opt-in mode that authorizes fields protected by
@requiresScopes/@authenticatedin a single up-front batch, before any subgraph fetch, instead of filtering them out of the response after the fetch.Motivation (Monday.com, migrating Apollo Router → Cosmo Router): today these directives are enforced post-fetch while walking returned data, which means (1) unauthorized selections still trigger subgraph fetches, and (2) when a protected field resolves to an empty list / no objects, no
UNAUTHORIZED_FIELD_OR_TYPEerror is emitted. This mode matches Apollo-style pre-execution behavior.Refs ENG-9828. The Cosmo Router wiring lands in a follow-up PR referencing these primitives.
How
BatchAuthorizerinterface +AuthorizationDecision(explicit allow/deny value type), separate fromAuthorizer(non-breaking).Context.SetPreFetchFieldAuthorizer(BatchAuthorizer)— presence enables the mode (no boolean flag).Plan.CollectAuthorizationCoordinates()(stored onGraphQLResponseInfo; no-op when the operation selects no protected fields).ResolveGraphQLResponse(and subscription paths) seeds the resolvable decision cache; the loader and resolvable become pure readers. Enforcement stays per-operation-type: queries prune (partial allowed), mutations & subscriptions whole-fetch reject.resolvable_authorization.go(100% unit-test coverage).Backward compatibility
Default off — with no
preFetchFieldAuthorizerset, behavior is byte-for-byte unchanged. No existing test or golden file was modified.Testing
v2suite green,-racegreen,golangci-lint(v2.10.1) clean,gofmt/vetclean.🤖 Generated with Claude Code