fix(auth): make resource-scoped policies actually authorize - #268
Open
blake-regalia wants to merge 2 commits into
Open
fix(auth): make resource-scoped policies actually authorize#268blake-regalia wants to merge 2 commits into
blake-regalia wants to merge 2 commits into
Conversation
The permission BGP required every candidate policy scope to be typed in m-graph:Cluster, but only orgs, repos, and collections are typed there: branches, locks, scratches, diffs, commits, and artifacts are typed in their repo's Metadata/Artifacts graph, and agents/policies in the AccessControl graphs. Any policy scoped directly to one of those resources — including every auto-created owner policy — could therefore never satisfy a permit() check, making per-resource grants silently inert. Union the scope-class lookup with the appropriate graph per scope kind. The repo graph is matched by IRI suffix through a graph variable rather than the mor-graph: prefix, because the BGP is also emitted for requests that carry no repo context (policy writes pass explicit scope URIs); the candidate scope VALUES are repeated inside that union branch so it always evaluates with a bounded subject. Also adds the missing branch/lock/scratch/artifact/commit role constants to MMS_OBJECT.ROLE, matching the roles already present in the access control definitions. Regression tests land with the follow-up commit that corrects the call-site scope constants. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Several call sites checked permissions at a broader scope than the resource being operated on, defeating per-resource grants: - single-org GET/HEAD used Scope.CLUSTER, so an org-scoped ReadOrg policy could list the org via /orgs but not read it directly - scratch replace used Scope.REPO, so the auto-created AdminScratch policy could never authorize updating one's own scratch - single-group GET/HEAD used Scope.CLUSTER with dead find/replace arguments (the regex could never match the cluster scope value) Use the resource-level scope for single-resource operations; ancestor scopes still authorize through the scope value chain. List-all endpoints keep their existing cluster-scope behavior. Adds regression tests covering branch-scoped, scratch-scoped, and org-scoped policies, plus a no-policy negative control. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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
1 participant
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.
Problem
The permission BGP in
permittedActionSparqlBgprequires every candidate policy scope IRI to be typed inm-graph:Cluster. Only orgs, repos, and collections are typed there — branches, locks, scratches, diffs, commits, and artifacts are typed in their repo'sMetadata/Artifactsgraph, and agents/policies in the AccessControl graphs.Consequently, any policy scoped directly to one of those resources could never authorize anything, including every auto-created owner policy (
AutoBranchOwner,AutoLockOwner,AdminScratch, …). Fine-grained delegation ("give this user access to branch B only") silently failed with 403/404, and several call sites had visible workarounds: single-org reads checkedScope.CLUSTER, scratch replacement checkedScope.REPOagainst its own comment, and single-group reads carried dead find/replace arguments.Fix
59194d0— union the scope-class lookup with the appropriate graph per scope kind (repoMetadata/Artifactsgraphs for repo-nested resources, AccessControl graphs for agents/policies). The repo graph is matched by IRI suffix through a graph variable rather than themor-graph:prefix because the BGP is also emitted for requests with no repo context (policy writes pass explicit scope URIs). The candidate scope VALUES are repeated inside the union branch so it always evaluates with a bounded subject — without this, the branch scans everyrdf:typetriple and stalls large updates (this bit was caught by the squash tests during development).2486f65— correct the call-site scope constants the mechanism fix unblocks: single-org reads →Scope.ORG, scratch replace →Scope.SCRATCH, single-group reads →Scope.GROUP. Ancestor scopes still authorize through the scope value chain; list-all endpoints keep their cluster-scope behavior.Tests
New
LeafScopePolicyTest: a branch-scopedAdminBranchgrant authorizes PATCH on that branch and not another; a scratch-scopedAdminScratchgrant authorizes replacing that scratch; an org-scopedReadOrggrant authorizes direct GET of the org; a user with no policy gets 403. All three positive tests fail without the fix; full suite green (368/368).Dependencies
None (based on
develop). Recommended to land the review-findings PR alongside: it fixes a per-request HTTP client leak that makes long test-suite runs progressively slower, which this PR's suite runtime also benefits from.🤖 Generated with Claude Code