feat(membership): add ListResourcesByPrincipal with schema-derived inheritance#1618
feat(membership): add ListResourcesByPrincipal with schema-derived inheritance#1618AmanGIT07 wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR implements a principal-scoped resource listing API for membership that filters organization, group, and project visibility by resolving principals to effective subjects, applying role-permission gates, expanding group and org-level policies, and intersecting user-scoped and PAT-scoped visibility constraints. Supporting changes add schema-derived role-permission constants, multi-organization project filtering, and comprehensive tests. ChangesPrincipal Resource Listing Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
AmanGIT07
force-pushed
the
feat/membership-list-resources-by-principal
branch
from
0ea655d to
7030621
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/bootstrap/service.go (1)
200-209:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCompute inheritance before mutating SpiceDB.
WriteSchemaruns beforepopulateInheritanceproves the finalized schema is compatible with the extractor. If extraction fails, bootstrap returns an error after the engine has already advanced, leaving migration non-atomic and startup stuck on a partially applied state. Move the compile/extract step ahead of the external write so this fails before any persistent mutation.Suggested change
- // apply azSchema to engine - if err = s.authzEngine.WriteSchema(ctx, authzedSchemaSource); err != nil { - return fmt.Errorf("%w: %s", schema.ErrMigration, err.Error()) - } - // derive the inheritance map from the finalized schema so membership // listing can't drift from the canonical SpiceDB chains. if err = s.populateInheritance(authzedSchemaSource); err != nil { return fmt.Errorf("populateInheritance: %w", err) } + + // apply azSchema to engine + if err = s.authzEngine.WriteSchema(ctx, authzedSchemaSource); err != nil { + return fmt.Errorf("%w: %s", schema.ErrMigration, err.Error()) + }core/membership/service.go (1)
93-111: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDon’t silently substitute an empty inheritance map.
ListResourcesByPrincipalunder-returns projects when this stays at the zero value, so allocating a private&schema.Inheritance{}hides wiring mistakes instead of surfacing them. Since the new API depends on sharing bootstrap’s extracted data, require callers to pass the shared pointer explicitly; tests that do not exercise this path can pass&schema.Inheritance{}themselves.
🧹 Nitpick comments (1)
core/membership/service_test.go (1)
2153-2155: ⚡ Quick winMake the role lookup mandatory in the PAT-narrowing case.
This case already expects an empty intersection, so the
.Maybe()means it still passes if the implementation stops consulting roles and just returns empty from both passes. Use explicit expectations for the user and PAT role lookups so the test actually guards the permission-gating path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d24053c9-b299-41ed-b4da-d987a9b0e106
📒 Files selected for processing (8)
cmd/serve.gocore/membership/service.gocore/membership/service_test.gocore/project/filter.gointernal/bootstrap/schema/inheritance.gointernal/bootstrap/schema/inheritance_test.gointernal/bootstrap/service.gointernal/store/postgres/project_repository.go
Coverage Report for CI Build 26031121262Coverage increased (+0.2%) to 42.515%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
AmanGIT07
force-pushed
the
feat/membership-list-resources-by-principal
branch
from
7030621 to
e2ab44d
Compare
AmanGIT07
force-pushed
the
feat/membership-list-resources-by-principal
branch
from
e2ab44d to
3d052ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
internal/bootstrap/service.go (2)
136-140: 💤 Low valuePrefer returning an error over
panicfor DI invariants.A nil
inheritancehere is a wiring bug at startup, but every other constructor in this package surfaces such failures by returning an error tocmd/serverather than crashing the process from a constructor. Returning(*Service, error)keeps the failure observable in logs/exit codes and makes this constructor consistent with the rest of the bootstrap surface. If you want to keep the contract strict, anerrors.New("bootstrap: inheritance pointer must be non-nil")returned alongside is equivalent and easier to test.
246-263: ⚡ Quick winEliminate duplicate schema compilation by threading the compiled schema from
ValidatePreparedAZSchemathrough topopulateInheritance.
ValidatePreparedAZSchema(line 18, generator.go) compilesauthzedSchemaSourcebut returns onlyerror, forcingpopulateInheritance(line 246, service.go) to recompile the same source. The current code rationalizes this with a comment ("so input matches what SpiceDB itself ingests"), but since both compile calls use identical parameters on identical input, the compiled artifact from the first call can be reused. Refactor to return the compiled schema fromValidatePreparedAZSchemaand pass it topopulateInheritanceto avoid the redundant parse.Additionally, verify that the consistent use of
compiler.ObjectTypePrefix("frontier")across all compile calls aligns with the base schema namespace definitions, which are prefixed withapp/(e.g.,definition app/project,definition app/organization). While the codebase currently works, confirm that the prefix parameter matches the intended namespace organization to avoid future fragility if compile behavior or schema structure changes.core/membership/service_test.go (3)
264-264: 💤 Low valueConsider a small constructor helper to absorb the new dependency.
Eighteen call sites now end in the same boilerplate (
..., mockAuditRepo, &schema.Inheritance{})), and every futuremembership.Servicedependency will require an identical sweep across this file. A tiny test helper such asnewTestService(t, opts...)that takes a struct of overrides and fills the rest with freshmocks.New*(t)instances would localize the constructor signature to one place and make the table-driven tests easier to scan. Not blocking — purely a maintainability nudge while the signature is still in flux.Also applies to: 307-307, 319-319, 331-331, 526-526, 574-574, 586-586, 598-598, 811-811, 952-952, 1033-1033, 1225-1225, 1399-1399, 1469-1469, 1481-1481, 1496-1496, 1524-1524, 1650-1650
2136-2160: 💤 Low valueLoose role mock makes this case pass for slightly the wrong reason.
The "PAT narrows … empty intersection" case is the most behaviorally important assertion in this file (it proves PAT genuinely narrows), but the role expectation here is
m.role.EXPECT().List(ctx, mock.Anything).Return([]role.Role{projectViewerRole, orgViewerRole}, nil)— a single any-args expectation that satisfies everyRoleService.Listinvocation across both the user-pass and the PAT-pass. If the service ever stops callingListfor one of the passes (e.g., because direct-project gating is short-circuited), the intersection still ends up empty and the test will continue to pass without exercising the filter.Consider tightening this to two
mock.MatchedByexpectations keyed on the role IDs each pass is expected to look up (one for{roleProjectViewerID, roleOrgViewerID}from the user pass, one for{roleProjectViewerID}from the PAT pass), so a regression infilterByRolePermissionsinvocations would surface as an unmet expectation rather than a coincidentally-correct result.
1675-2195: 💤 Low valueSolid coverage on the new listing path.
The table covers the behaviors that matter for this contract: resource-type rejection, dedup, no-inheritance for groups, direct-vs-inherited project gating via the shared
Inheritancefixture, viewer-on-org not expanding, custom-role expansion when the role permission lands inOrganizationToProjectInherit, group expansion underNonInherited=true,OrgIDnarrowing throughproject.Filter, service-user principal type, the PAT-nil short-circuit, and both PAT outcomes (intersection-equal and intersection-empty).A couple of small things to consider when this evolves:
- The inheritance fixture is hand-rolled rather than derived from the real
base_schema.zedviaExtractInheritance. That's fine for a unit test, but if the canonical permission names ever drift (e.g.,app_organization_administeris renamed) this test stays green while production breaks. A single integration-style test that compiles the real schema and asserts non-emptyProjectDirectVisibility/OrganizationToProjectInheritwould catch that drift cheaply.- The "PAT all-projects scope" case asserts
Times-less expectations onproject.List(project.Filter{OrgIDs: []string{orgA}})for both passes, which works because testify treats unconstrainedEXPECT()asTimes(0..n). If you want the test to also prove the algorithm runs exactly twice (once per pass), an explicit.Times(2)on that expectation would lock the contract.No changes required for this PR; flagging for follow-up.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0fee9fea-40f5-4c1e-94d3-cc50180939b9
📒 Files selected for processing (8)
cmd/serve.gocore/membership/service.gocore/membership/service_test.gocore/project/filter.gointernal/bootstrap/schema/inheritance.gointernal/bootstrap/schema/inheritance_test.gointernal/bootstrap/service.gointernal/store/postgres/project_repository.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/store/postgres/project_repository.go
- cmd/serve.go
- core/project/filter.go
- internal/bootstrap/schema/inheritance.go
- internal/bootstrap/schema/inheritance_test.go
- core/membership/service.go
…heritance Adds membership.Service.ListResourcesByPrincipal(principal, resourceType, filter) — a policy-driven replacement for org/project/group.ListByUser that reads from Postgres policies instead of SpiceDB relations. Why: today's ListByUser methods call relation.LookupResources, which reads the SpiceDB membership permission (member + owner). When a user's role on an org/group is demoted, the policy is updated but the direct SpiceDB owner/member relation lingers — so demoted users keep appearing in listings. Policy-driven listing makes Postgres policies the single source of truth. Highlights: - internal/bootstrap/schema/inheritance.go (new) — Inheritance struct with ProjectDirectVisibility and OrganizationToProjectInherit lists extracted from base_schema.zed at MigrateSchema time. Walks both granted-> and pat_granted-> arrows; errors loudly on non-Union rewrites. - internal/bootstrap/service.go — populateInheritance runs after the effective schema is finalized. inheritance pointer is threaded through DI so membership reads the canonical lists without drift. - core/membership/service.go — ListResourcesByPrincipal (top-level, PAT-aware) plus listResourcesForPrincipal (per-principal core). Three project branches: direct project policies gated by ProjectDirectVisibility, group expansion, org inheritance gated by OrganizationToProjectInherit and batched via project.Filter.OrgIDs. - core/project/filter.go — adds OrgIDs []string for the batched cross-org expansion (avoids N+1 for users in many orgs). - core/membership/service_test.go — 16 table-driven cases including stale-relation regression, NonInherited, group expansion, OrgID narrowing, PAT all-projects scope, no-PAT short-circuit, and PAT-narrows-user-access. No callers migrate in this commit — purely additive. Org/group/project listing migrations follow in subsequent commits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the runtime SpiceDB-AST-walker-extracted inheritance map with
two small hardcoded constants in internal/bootstrap/schema/schema.go and
a regex-based drift test that scans base_schema.zed at build time.
Why the simpler approach is the right one:
- The two permission lists never change in normal feature work. If they
ever do, it's a major authz rewrite — at that point a hardcoded list
is the least of the maintainer's worries.
- The AST walker required importing SpiceDB compiler internals into app
code. Vendor-internal imports make upgrades harder and tie us to a
specific SpiceDB version.
- ~150 lines of recursive proto-tree traversal in inheritance.go were a
maintenance liability for anyone debugging future schema issues.
- The regex drift guard is ~80 lines, fails loudly when arrows change,
and rejects non-Union expressions (intersection/exclusion) since those
would silently break filterByRolePermissions's any-of semantics.
Changes:
- internal/bootstrap/schema/schema.go — new ProjectDirectVisibilityPerms
and OrganizationProjectInheritPerms vars.
- internal/bootstrap/schema/inheritance_perms_test.go — drift guard
(renamed from inheritance_test.go).
- internal/bootstrap/schema/inheritance.go — deleted.
- internal/bootstrap/service.go — populateInheritance, inheritance
pointer field, panic guard, compiler import all removed.
- core/membership/service.go — inheritance field and constructor param
removed; helpers reference the new schema constants directly.
- core/membership/service_test.go — inheritance arg dropped from all
NewService call sites.
- cmd/serve.go — shared &schema.Inheritance{} allocation and the args
to both constructors removed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
AmanGIT07
force-pushed
the
feat/membership-list-resources-by-principal
branch
from
3d052ad to
737326f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
core/membership/service_test.go (1)
1790-1797: ⚡ Quick winAdd a negative project-visibility case.
Every project-path case here uses roles that are supposed to pass the allowlist, so dropping
filterByRolePermissionson the direct or group-expanded branches would still leave most of this table green. Add one policy whose role lacksschema.ProjectDirectVisibilityPermsand assert it is excluded.Also applies to: 1912-2086
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f983746-0726-45e9-8d39-6dd4ac849839
📒 Files selected for processing (6)
core/membership/service.gocore/membership/service_test.gocore/project/filter.gointernal/bootstrap/schema/inheritance_perms_test.gointernal/bootstrap/schema/schema.gointernal/store/postgres/project_repository.go
🚧 Files skipped from review as they are similar to previous changes (2)
- core/project/filter.go
- internal/store/postgres/project_repository.go
| inheritingOrgIDs, err := s.filterByRolePermissions(ctx, policies, schema.OrganizationProjectInheritPerms) | ||
| if err != nil { |
There was a problem hiding this comment.
Don’t collapse granted and pat_granted into one inheritance gate.
OrganizationProjectInheritPerms is the union of both arrow kinds, but this helper ignores policy.GrantRelation. If those branches ever diverge, the PAT pass will treat a PAT-bound org role as inheriting projects that only the normal granted path should see, so the PAT no longer narrows visibility correctly. Split the allowlists by relation type or branch on pol.GrantRelation here.
Also applies to: 1806-1814
| var ProjectDirectVisibilityPerms = []string{ | ||
| "app_project_administer", | ||
| "app_project_get", | ||
| "app_project_update", | ||
| } | ||
|
|
||
| // OrganizationProjectInheritPerms — role permissions that, on an org-level | ||
| // policy, grant the principal visibility into every project in that org. | ||
| // Mirrors the granted-> and pat_granted-> arrows of | ||
| // app/organization.project_get in base_schema.zed. | ||
| var OrganizationProjectInheritPerms = []string{ | ||
| "app_organization_administer", | ||
| "app_project_get", | ||
| "app_project_administer", | ||
| } |
There was a problem hiding this comment.
Freeze these authz allowlists before exporting them.
These are exported []string vars, so any package in this module can mutate project visibility rules at runtime. For authorization gates, keep the backing storage unexported and return a cloned slice (or expose helper predicates) so callers can't widen or narrow access accidentally.
2 participants
Summary
Adds
membership.Service.ListResourcesByPrincipal(principal, resourceType, filter)— a policy-driven replacement for today'sorg.ListByUser,project.ListByUser,group.ListByUsermethods that read SpiceDB relations. Purely additive: no callers migrated in this PR.Why
SpiceDB is built for permission checks, not listing. Postgres already has the policies and roles needed to answer "what can this principal see," so the listing path moves off
relationService.LookupResourcesand onto direct policy reads. Making Postgres policies the single source of truth for listing simplifies the model and decouples a heavily-used UI path from SpiceDB.Companion to #1616 (legacy service-user policy backfill) — both need to land before any caller is migrated.
Algorithm
ListResourcesByPrincipalruns the underlying algorithm twice when a PAT is present (once for the user, once for the PAT-as-principal) and intersects, so the PAT can narrow but never widen the user's visibility.For each resource type:
org.membershipcheck.filter.OrgID:schema.ProjectDirectVisibilityPerms(mirrors today'sLookupResources(project, ..., project.get)).NonInherited=true.NonInherited) — every project in any org whose policy role grantsschema.OrganizationProjectInheritPerms. Batched viaproject.Filter.OrgIDsto avoid N+1.Schema-derived permission lists
Two constants live in
internal/bootstrap/schema/schema.go:They mirror the
granted->/pat_granted->arrows ofapp/project.getandapp/organization.project_getinbase_schema.zed.inheritance_perms_test.gois a regex-only drift guard: it scans the embedded schema source and asserts the constants match what's in the schema. It also rejects non-Union expressions (intersection/exclusion would silently break the any-of semantics infilterByRolePermissions). No SpiceDB compiler import, no AST walking.Files changed
internal/bootstrap/schema/schema.go—ProjectDirectVisibilityPermsandOrganizationProjectInheritPermsconstantsinternal/bootstrap/schema/inheritance_perms_test.go— regex drift guard for both constantscore/membership/service.go—ResourceFilter,ListResourcesByPrincipal+ per-type helpers +filterByRolePermissionscore/membership/service_test.go— 16 new table-driven cases (rejection of unsupported type, direct-only listing per type, inheritance for owner/manager/viewer/custom roles, NonInherited, group expansion, OrgID narrowing, ServiceUser principal, deduplication, PAT all-projects, no-PAT short-circuit, stale-relation regression, PAT narrowing)core/project/filter.go+internal/store/postgres/project_repository.go—OrgIDs []stringplural filter for batched org-inheritance expansionTest plan
make buildpassesmake lint(0 issues)make test -raceon touched packages greenlistOrgInheritedProjectIDsproduces correct results for actual users on the dev DB:Not in this PR
org.ListByUser,project.ListByUser,group.ListByUserstill in production; new method is unreachable from the API surface today. Migration of handlers (ListOrganizationsByUser,ListUserGroups,ListProjectsByUser, etc.) and the deletion of oldListByUsermethods come in follow-up PRs.🤖 Generated with Claude Code