Skip to content

feat(membership): add ListResourcesByPrincipal with inheritance#1618

Merged
AmanGIT07 merged 4 commits into
mainfrom
feat/membership-list-resources-by-principal
May 19, 2026
Merged

feat(membership): add ListResourcesByPrincipal with inheritance#1618
AmanGIT07 merged 4 commits into
mainfrom
feat/membership-list-resources-by-principal

Conversation

@AmanGIT07
Copy link
Copy Markdown
Contributor

@AmanGIT07 AmanGIT07 commented May 15, 2026

Summary

Adds membership.Service.ListResourcesByPrincipal(principal, resourceType, filter) — a policy-driven replacement for today's org.ListByUser, project.ListByUser, group.ListByUser methods 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.LookupResources and 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

ListResourcesByPrincipal runs 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:

  • Organization: list policies, take resource IDs. Not role-permission-gated — matches today's relation-based org.membership check.
  • Group: same as org. No inheritance.
  • Project: unions three sources, deduped, then narrowed by filter.OrgID:
    1. Direct project policies filtered by schema.ProjectDirectVisibilityPerms (mirrors today's LookupResources(project, ..., project.get)).
    2. Group expansion — project policies on the principal's groups, gated the same way. Runs even with NonInherited=true.
    3. Org inheritance (skipped if NonInherited) — every project in any org whose policy role grants schema.OrganizationProjectInheritPerms. Batched via project.Filter.OrgIDs to avoid N+1.

Schema-derived permission lists

Two constants live in internal/bootstrap/schema/schema.go:

ProjectDirectVisibilityPerms = []string{
    "app_project_administer", "app_project_get", "app_project_update",
}
OrganizationProjectInheritPerms = []string{
    "app_organization_administer", "app_project_get", "app_project_administer",
}

They mirror the granted-> / pat_granted-> arrows of app/project.get and app/organization.project_get in base_schema.zed. inheritance_perms_test.go is 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 in filterByRolePermissions). No SpiceDB compiler import, no AST walking.

Files changed

  • internal/bootstrap/schema/schema.goProjectDirectVisibilityPerms and OrganizationProjectInheritPerms constants
  • internal/bootstrap/schema/inheritance_perms_test.go — regex drift guard for both constants
  • core/membership/service.goResourceFilter, ListResourcesByPrincipal + per-type helpers + filterByRolePermissions
  • core/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.goOrgIDs []string plural filter for batched org-inheritance expansion

Test plan

  • make build passes
  • make lint (0 issues)
  • make test -race on touched packages green
  • Algorithm trace against real local DB: verified listOrgInheritedProjectIDs produces correct results for actual users on the dev DB:
    • Org Owner on a real org (PixxelSpace, 175 projects) → 175 inherited project IDs
    • Org Viewer on the same org → 0 projects (correctly excluded by role-permission filter)
    • Org Owner on an empty org → 0 projects (early-return fast path)

Not in this PR

  • No callers migrated. org.ListByUser, project.ListByUser, group.ListByUser still in production; new method is unreachable from the API surface today. Migration of handlers (ListOrganizationsByUser, ListUserGroups, ListProjectsByUser, etc.) and the deletion of old ListByUser methods come in follow-up PRs.
  • No end-to-end runtime test possible yet. End-to-end coverage lands with the handler-migration PRs.

🤖 Generated with Claude Code

@vercel
Copy link
Copy Markdown

vercel Bot commented May 15, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
frontier Ready Ready Preview, Comment May 19, 2026 10:19am

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 15, 2026

Review Change Stack

Caution

Review failed

Pull request was closed or merged during review

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • Role-aware resource listing for organizations, groups, and projects with permission-gated visibility
    • Optional org filtering plus a toggle to disable org→project inheritance
    • Personal Access Token (PAT) scope enforced: PATs can only narrow listed resources
    • Project queries support narrowing to multiple org IDs
  • Tests

    • Comprehensive tests covering listing behavior, inheritance, group expansion, and PAT interactions
  • Schema/Validation

    • Added checks to keep project visibility and org→project inheritance permission definitions in sync

Walkthrough

Adds principal-scoped resource listing: new Service.ListResourcesByPrincipal with ResourceFilter, role-permission gating and Postgres policy filtering, group/org expansion for projects, PAT-scoped intersection, schema permission constants, multi-org project filtering, and comprehensive tests.

Changes

Principal Resource Listing Feature

Layer / File(s) Summary
Schema role-permission constants and validation
internal/bootstrap/schema/schema.go, internal/bootstrap/schema/inheritance_perms_test.go
Defines ProjectDirectVisibilityPerms and OrganizationProjectInheritPerms and a Zed-based drift-guard test that extracts granted relations and asserts parity with the Go lists.
Multi-organization project filtering
core/project/filter.go, internal/store/postgres/project_repository.go
Adds OrgIDs []string to core/project.Filter and applies org_id IN (flt.OrgIDs) filtering in ProjectRepository.List when provided.
Policy role-permissions filtering (Postgres)
core/policy/filter.go, internal/store/postgres/policy_repository.go
Adds RolePermissions []string to core/policy.Filter and updates Postgres policy repository to join roles and filter policies whose roles.permissions JSONB contains any listed permission names.
ListResourcesByPrincipal implementation
core/membership/service.go
Adds ResourceFilter and ListResourcesByPrincipal that resolves principals, dispatches listing by type (orgs/groups/projects), composes project visibility from direct, group-expanded, and org-inherited policies, narrows candidates via ProjectService.List/GroupService.List when OrgID is set, and intersects user and PAT results.
ListResourcesByPrincipal tests
core/membership/service_test.go
Table-driven tests exercising unsupported types, deduplication, group and org expansion, project narrowing via ProjectService.List, and PAT intersection/nil-PAT branching.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers

  • rohilsurana
  • whoAbhishekSah
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@AmanGIT07 AmanGIT07 force-pushed the feat/membership-list-resources-by-principal branch from 0ea655d to 7030621 Compare May 15, 2026 06:28
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Compute inheritance before mutating SpiceDB.

WriteSchema runs before populateInheritance proves 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 win

Don’t silently substitute an empty inheritance map.

ListResourcesByPrincipal under-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 win

Make 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

📥 Commits

Reviewing files that changed from the base of the PR and between 74c44fa and 0ea655d.

📒 Files selected for processing (8)
  • cmd/serve.go
  • core/membership/service.go
  • core/membership/service_test.go
  • core/project/filter.go
  • internal/bootstrap/schema/inheritance.go
  • internal/bootstrap/schema/inheritance_test.go
  • internal/bootstrap/service.go
  • internal/store/postgres/project_repository.go

Comment thread core/project/filter.go
Comment thread internal/store/postgres/project_repository.go
@coveralls
Copy link
Copy Markdown

coveralls commented May 15, 2026

Coverage Report for CI Build 26091024408

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Coverage increased (+0.1%) to 42.472%

Details

  • Coverage increased (+0.1%) from the base build.
  • Patch coverage: 60 uncovered changes across 3 files (131 of 191 lines covered, 68.59%).
  • No coverage regressions found.

Uncovered Changes

File Changed Covered %
core/membership/service.go 172 129 75.0%
internal/store/postgres/policy_repository.go 14 1 7.14%
internal/store/postgres/project_repository.go 5 1 20.0%

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 37907
Covered Lines: 16100
Line Coverage: 42.47%
Coverage Strength: 11.91 hits per line

💛 - Coveralls

@AmanGIT07 AmanGIT07 force-pushed the feat/membership-list-resources-by-principal branch from 7030621 to e2ab44d Compare May 15, 2026 07:11
@AmanGIT07 AmanGIT07 force-pushed the feat/membership-list-resources-by-principal branch from e2ab44d to 3d052ad Compare May 15, 2026 07:39
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (5)
internal/bootstrap/service.go (2)

136-140: 💤 Low value

Prefer returning an error over panic for DI invariants.

A nil inheritance here is a wiring bug at startup, but every other constructor in this package surfaces such failures by returning an error to cmd/serve rather 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, an errors.New("bootstrap: inheritance pointer must be non-nil") returned alongside is equivalent and easier to test.


246-263: ⚡ Quick win

Eliminate duplicate schema compilation by threading the compiled schema from ValidatePreparedAZSchema through to populateInheritance.

ValidatePreparedAZSchema (line 18, generator.go) compiles authzedSchemaSource but returns only error, forcing populateInheritance (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 from ValidatePreparedAZSchema and pass it to populateInheritance to 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 with app/ (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 value

Consider a small constructor helper to absorb the new dependency.

Eighteen call sites now end in the same boilerplate (..., mockAuditRepo, &schema.Inheritance{})), and every future membership.Service dependency will require an identical sweep across this file. A tiny test helper such as newTestService(t, opts...) that takes a struct of overrides and fills the rest with fresh mocks.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 value

Loose 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 every RoleService.List invocation across both the user-pass and the PAT-pass. If the service ever stops calling List for 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.MatchedBy expectations 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 in filterByRolePermissions invocations would surface as an unmet expectation rather than a coincidentally-correct result.


1675-2195: 💤 Low value

Solid 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 Inheritance fixture, viewer-on-org not expanding, custom-role expansion when the role permission lands in OrganizationToProjectInherit, group expansion under NonInherited=true, OrgID narrowing through project.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.zed via ExtractInheritance. That's fine for a unit test, but if the canonical permission names ever drift (e.g., app_organization_administer is renamed) this test stays green while production breaks. A single integration-style test that compiles the real schema and asserts non-empty ProjectDirectVisibility / OrganizationToProjectInherit would catch that drift cheaply.
  • The "PAT all-projects scope" case asserts Times-less expectations on project.List(project.Filter{OrgIDs: []string{orgA}}) for both passes, which works because testify treats unconstrained EXPECT() as Times(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

📥 Commits

Reviewing files that changed from the base of the PR and between 0ea655d and 3d052ad.

📒 Files selected for processing (8)
  • cmd/serve.go
  • core/membership/service.go
  • core/membership/service_test.go
  • core/project/filter.go
  • internal/bootstrap/schema/inheritance.go
  • internal/bootstrap/schema/inheritance_test.go
  • internal/bootstrap/service.go
  • internal/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

Comment thread internal/bootstrap/service.go Outdated
AmanGIT07 and others added 2 commits May 18, 2026 17:05
…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 AmanGIT07 force-pushed the feat/membership-list-resources-by-principal branch from 3d052ad to 737326f Compare May 18, 2026 11:38
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
core/membership/service_test.go (1)

1790-1797: ⚡ Quick win

Add a negative project-visibility case.

Every project-path case here uses roles that are supposed to pass the allowlist, so dropping filterByRolePermissions on the direct or group-expanded branches would still leave most of this table green. Add one policy whose role lacks schema.ProjectDirectVisibilityPerms and 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

📥 Commits

Reviewing files that changed from the base of the PR and between 3d052ad and 737326f.

📒 Files selected for processing (6)
  • core/membership/service.go
  • core/membership/service_test.go
  • core/project/filter.go
  • internal/bootstrap/schema/inheritance_perms_test.go
  • internal/bootstrap/schema/schema.go
  • internal/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

Comment thread core/membership/service.go Outdated
Comment thread internal/bootstrap/schema/schema.go
Comment thread core/membership/service.go Outdated
Comment thread core/membership/service.go Outdated
Comment thread core/membership/service.go
Removes SpiceDB-internal jargon ("role-permission-gated", "*.membership
check (member + owner)", "no org-> chain", "granted-> arrows") in favor
of language that doesn't require schema/SpiceDB context to read.

No behavior change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@AmanGIT07 AmanGIT07 changed the title feat(membership): add ListResourcesByPrincipal with schema-derived inheritance feat(membership): add ListResourcesByPrincipal with inheritance May 19, 2026
…sting (#1623)

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@AmanGIT07 AmanGIT07 enabled auto-merge (squash) May 19, 2026 10:19
@AmanGIT07 AmanGIT07 merged commit 326d7b2 into main May 19, 2026
7 of 8 checks passed
@AmanGIT07 AmanGIT07 deleted the feat/membership-list-resources-by-principal branch May 19, 2026 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants