fix(query): sound caveat resolution for recursive relations in the query planner#3220
Draft
josephschorr wants to merge 3 commits into
Draft
fix(query): sound caveat resolution for recursive relations in the query planner#3220josephschorr wants to merge 3 commits into
josephschorr wants to merge 3 commits into
Conversation
The recursive self-edge existence probe (SubjectExistsAsRelationship) and the forward MatchingResourcesForSubject lookup did not specify a query shape. The former panics under the validating datastore; the latter fails index checking on SQL datastores because the forward query-shape validator had no case for it. Specify queryshape.Varying on the existence probe and add the missing forward validator case.
…default The query-plan consistency suite ran zero assertions: the Check gate called t.Skip on the parent test node, whose runtime.Goexit prevented the LookupResources and LookupSubjects subtests from executing at all. Remove the Check and LookupResources gates so both run on every test config. LookupSubjects stays gated behind TEST_QUERY_PLAN_SUBJECTS pending a separate wildcard-with-exclusion fix.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…lations Recursive relations tracked reachable objects with a global visited set and kept the caveat of the first path by which each object was reached. An object first reached via a caveated edge kept that caveat on every descendant even when later reached unconditionally, reporting conditional access where access was in fact unconditional (a caveated diamond is enough to trigger it; no cycle required). Replace the visited set with a semi-naive fixpoint over a canonical caveat condition (a DNF with idempotent AND and an absorbing unconditional case), re-expanding an object only when a new path weakens its condition and buffering results until the traversal converges so a caveat is never emitted before it is final. When no recursive edge is caveated this reduces to the previous BFS. Also surface MaxRecursionDepthError on depth exhaustion instead of silently returning a truncated result.
59de9de to
ddad6b9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a set of correctness bugs in the experimental query planner's recursive relation handling (
pkg/query), plus the test-infrastructure gaps that were hiding them. Scoped to the query planner; the legacy dispatch engine is untouched.All changes are opt-in-path only: the query planner is gated behind
--experimental-query-plan(default off), so production behavior is unchanged unless that flag is set.What was broken
Caveat soundness on recursive relations (the important one). For a schema like
relation member: user | group#member | group#member with somecaveat, the planner tracked reachable objects with a global visited set and kept the caveat of the first path by which each object was reached. When the same object was later reached by a weaker (less-caveated) path, its descendants kept the stale, over-restrictive caveat. Result: a subject reachable by an unconditional path could be reported as only conditionally reachable — a wrong authorization decision. No cycle is required; a caveated diamond triggers it.Silent truncation at max recursion depth. When a recursive traversal exceeded
MaxRecursionDepth, the planner fell off the end of its loop and returned a partial result (orNOT_MEMBER) with no error — a "deny" that actually means "I gave up." The legacy engine errors here; the planner now does too.Missing query shapes. The existence-probe query used for recursive self-edges specified no query shape, which panics under the validating datastore — the reason the caveat bug was never caught in tests. A second query-shape descriptor (
MatchingResourcesForSubject) was missing from the forward validator, breaking index checking on SQL datastores.Inert consistency suite. The query-plan consistency test skipped every assertion because a gate on the parent test node called
runtime.Goexit. Check and LookupResources consistency now run by default on every test config.How it's fixed
internal/caveats/canonical.go) represents a caveat expression as a DNF with idempotentAND, absorbing "unconditional", and subsumption. This makes it possible to detect when a newly-discovered path weakens the condition under which an object is reachable, and guarantees the traversal terminates.IterSubjects/Check andIterResources) now run a semi-naive fixpoint: an object is re-expanded when its reaching condition weakens, and results are buffered and flushed at convergence so a caveat is never emitted before it is final. When no recursive edge is caveated, this reduces to exactly the previous BFS — no change to the common path.MaxRecursionDepthError.Testing
go test ./pkg/query/... ./internal/caveats/...and the query-plan consistency integration suite pass;-raceclean.Follow-ups (not in this PR)
TEST_QUERY_PLAN_SUBJECTSwith a pointer to the issue.