refactor(project): migrate ListByUser callers to membership, delete legacy method#1648
Merged
Merged
Conversation
…egacy method Switches every reader of "projects a principal sees" off `project.Service.ListByUser` (SpiceDB `LookupResources` + repo enrichment + intersectPATScope) onto Postgres policy reads via `membership.Service.ListProjectsByPrincipal`. Adds `project.Filter.Principal` so the composition lives inside `Service.List` instead of the handler. Three handler call sites and two internal callers migrated; old `ListByUser`, `listNonInheritedProjectIDs`, `intersectPATScope`, and the project service's `RelationService.LookupResources` dependency are deleted. Hardens `policy.Service.List` to refuse principal-narrowed filters with empty PrincipalID, closing a mass-disclosure path exposed by the move from SpiceDB to policy reads. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
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 (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactors project listing to pass principals via project.Filter.Principal and delegates visibility resolution to membership.Service.ListProjectsByPrincipal. ProjectService.List behavior updated to use the membership shim; mocks, handlers, tests, and cmd/serve dependency wiring updated to the new contract. ChangesProject Listing Principal-Based Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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. 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 |
Coverage Report for CI Build 26435250899Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage decreased (-0.04%) to 42.808%Details
Uncovered Changes
Coverage Regressions2 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/aggregates/orgpats/service.go (1)
83-85:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale comment about SpiceDB usage.
The comment says this path calls SpiceDB, but Line 111 now goes through
projectService.Listwith principal filter (membership/policy-backed path). Please update wording to avoid misleading future maintenance.
🧹 Nitpick comments (3)
core/aggregates/orgpats/service_test.go (1)
93-103: ⚡ Quick winMake the all-projects resolution test deterministic.
This test can still pass when resolution is skipped because
Listis optional (Maybe) and the assertion is conditional. Tighten it so the migration path is actually verified.🔧 Suggested test hardening
- projSvc.EXPECT().List(mock.Anything, mock.Anything). - Return([]project.Project{{ID: "proj-1"}, {ID: "proj-2"}}, nil).Maybe() + projSvc.EXPECT().List(mock.Anything, mock.Anything). + Return([]project.Project{{ID: "proj-1"}, {ID: "proj-2"}}, nil).Once() @@ - // After resolution, the all-projects scope should have project IDs - if len(result.PATs[0].Scopes[0].ResourceIDs) > 0 { - assert.Contains(t, result.PATs[0].Scopes[0].ResourceIDs, "proj-1") - } + // After resolution, the all-projects scope should have project IDs + assert.ElementsMatch(t, []string{"proj-1", "proj-2"}, result.PATs[0].Scopes[0].ResourceIDs)core/userpat/service_test.go (1)
649-651: ⚡ Quick winAssert principal-scoped filter shape in
ProjectService.Listmocks.These expectations currently accept any
project.Filter, so wiring regressions inPrincipal/OrgIDwon’t fail tests.🔧 Suggested matcher pattern
- projSvc.On("List", mock.Anything, mock.Anything).Return([]project.Project{ + projSvc.On("List", mock.Anything, mock.MatchedBy(func(f project.Filter) bool { + return f.OrgID == "org-1" && + f.Principal != nil && + f.Principal.ID == "user-1" && + f.Principal.Type == schema.UserPrincipal + })).Return([]project.Project{ {ID: "proj-a"}, {ID: "proj-b"}, }, nil).Maybe()Also applies to: 1173-1175, 2562-2564, 2600-2602
internal/api/v1beta1connect/serviceuser_test.go (1)
1341-1342: ⚡ Quick winAdd one org-scoped filter assertion for
ListServiceUserProjects.These updates verify
Principal, but notOrgIDforwarding. Add at least one case withOrgIdset and assertproject.Filter.OrgIDto lock tenant scoping behavior.Also applies to: 1357-1358, 1402-1403
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db4fc029-bf0d-4089-be24-6d0d0c2c00d5
📒 Files selected for processing (27)
cmd/serve.gocore/aggregates/orgpats/mocks/project_service.gocore/aggregates/orgpats/service.gocore/aggregates/orgpats/service_test.gocore/membership/service.gocore/membership/service_test.gocore/policy/errors.gocore/policy/service.gocore/policy/service_test.gocore/project/filter.gocore/project/mocks/group_service.gocore/project/mocks/membership_service.gocore/project/mocks/relation_service.gocore/project/service.gocore/project/service_test.gocore/userpat/mocks/project_service.gocore/userpat/service.gocore/userpat/service_test.gointernal/api/v1beta1connect/interfaces.gointernal/api/v1beta1connect/mocks/project_service.gointernal/api/v1beta1connect/serviceuser.gointernal/api/v1beta1connect/serviceuser_test.gointernal/api/v1beta1connect/user.gointernal/api/v1beta1connect/user_test.gointernal/store/postgres/project_repository.gointernal/store/postgres/user_projects_repository.gotest/e2e/regression/api_test.go
💤 Files with no reviewable changes (3)
- internal/api/v1beta1connect/interfaces.go
- core/project/mocks/group_service.go
- core/project/mocks/relation_service.go
The previous commit added validation to refuse `policy.Filter{PrincipalType: X}`
without a matching `PrincipalID` or `PrincipalIDs`. That over-restricted the
legitimate `ListPrincipalsByResource` shape (`{OrgID/ProjectID/GroupID, PrincipalType}`),
breaking 14 regression tests across `ListGroupUsers`, `ListOrganizationUsers`,
and `ListProjectUsers`.
The behaviour the validation was meant to gate — `ListProjectsByUser(id: "")`
returning every project — is consistent with the codebase convention that
listing RPCs return all rows when called without a narrowing filter, and the
RPC is already super-admin gated by the interceptor. Not a bug.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
RPC test resultsTested the three direct call sites + two indirect call sites, across cookie / PAT / service-account auth. Seed: org with three projects (
|
| # | Case | Result |
|---|---|---|
| H1 | default, no filter | 3 (direct+group+inherit) ✓ |
| H2 | nonInherited=true |
2 (direct+group) ✓ |
| H3 | orgId=<test> |
3 ✓ |
| H4 | withMemberCount=true |
mc populated (direct=2, group=1, inherit=1), count=3 ✓ |
| E1 | no cookie | unauthenticated ✓ |
| E2 | bogus cookie | unauthenticated ✓ |
| E3 | orgId=<foreign-org> |
{} ✓ |
| E4 | orgId=00000000-... |
{} ✓ |
| Q1 | pageSize=2,pageNum=1 |
2, count=3 ✓ |
| Q2 | pageSize=2,pageNum=2 |
1, count=3 ✓ |
| Q3 | pageSize=0,pageNum=0 (IGNORE_IF_ZERO) |
3, count=3 ✓ |
| Q4 | pageSize=-1 |
invalid_argument ✓ |
| Q5 | pageNum=5 (past last) |
0, count=3 ✓ |
| Q6 | pageSize=1+nonInherited+withMemberCount |
1, count=2 (count matches post-NonInherited total) ✓ |
| Q7 | withPermissions=["get","delete"] |
dup access_pairs (see #1647) ⚠ |
| Q8 | withPermissions=["bogus_perm"] |
internal (see #1647) ⚠ |
| Q9 | withMemberCount + withPermissions combined |
both populated correctly ✓ |
ListProjectsByCurrentUser (PAT bearer)
| # | PAT scope | nonInherited | Result |
|---|---|---|---|
| P1 | [direct] |
false | 1 (direct) ✓ |
| P2 | [direct] |
true | 1 (direct) ✓ |
| P3 | [direct], no orgId |
false | 1 ✓ |
| P4 | [direct, group] |
false | 2 ✓ |
| P5 | [] (org-wide) |
false | 3 (all PAT owner can see) ✓ |
| P6 | [inherit] |
false | 1 (inherit) ✓ |
| P7 | [inherit] |
true | 0 (correct — intersection empty) ✓ |
| P8 | [direct, group] + pageSize=1 |
– | 1, count=2 ✓ |
| P9 | [direct, group] + withMemberCount + withPermissions=["get"] |
– | matches ✓ |
ListProjectsByCurrentUser (service-account Basic auth)
| # | Case | Result |
|---|---|---|
| SA1 | GetCurrentUser as svcuser |
serviceuser block returned ✓ |
| SA2 | List with orgId |
1 (direct-proj) ✓ |
| SA3 | List without orgId |
1 (direct-proj) ✓ |
FrontierService/ListProjectsByUser (superadmin)
| # | Case | Result |
|---|---|---|
| H1 | admin → user's projects | 3 ✓ |
| E1 | non-existent UUID | {} ✓ |
| E2 | garbage id |
internal (see #1647) ⚠ |
| E3 | non-admin caller | unavailable (interceptor) ✓ |
| E4 | regular user, own id | unavailable (admin-only) ✓ |
FrontierService/ListServiceUserProjects (superadmin)
| # | Case | Result |
|---|---|---|
| H1 | admin → svcuser's projects | 1 (direct-proj) ✓ |
| E1 | omit orgId |
400 validation (min_len=3) ✓ |
| E2 | non-existent svcuser | not_found ✓ |
| E3 | empty id, valid orgId |
internal (see #1647) ⚠ |
| E4 | empty body | 400 validation ✓ |
| Q1 | withPermissions=["get","delete"] |
dup access_pairs (see #1647) ⚠ |
| Q2 | withPermissions=["bogus"] |
not_found (see #1647) ⚠ |
| SA1 | svcuser → own listing via Basic | permission_denied (no manage perm) ✓ |
FrontierService/CreateCurrentUserPAT — indirect (userpat.validateProjectAccess)
| # | Scope | Result |
|---|---|---|
| C1 | [direct] |
created ✓ |
| C2 | [direct, group] |
created ✓ |
| C3 | [] (org-wide) |
created ✓ |
| C4 | [inherit] (org-inherited project) |
created ✓ |
| C5 | [foreign-proj] (different org) |
invalid_argument: user does not have access to one or more specified projects ✓ |
| C6 | [direct, foreign-proj] |
rejected ✓ |
| C7 | [00000000-...] |
rejected ✓ |
AdminService/SearchOrganizationPATs — indirect (orgpats.resolveAllProjectsScope)
| # | PAT scope at create | resource_ids in Search response |
|---|---|---|
| S1 | [inherit] (literal) |
1 (preserved) ✓ |
| S2 | [] (org-wide) |
3 (resolved from membership) ✓ |
| S3 | [direct, group] (literal) |
2 (preserved) ✓ |
| S4 | [direct] (literal) |
1 (preserved) ✓ |
Bugs surfaced
Three pre-existing issues showed up while testing — all filed under #1647. None are caused by this refactor; they live in the same handlers and are independent of the ListByUser → List(Filter.Principal) migration.
| ID | Where | Symptom |
|---|---|---|
| B2 | ListProjectsByUser, ListServiceUserProjects |
malformed / empty id → Internal instead of InvalidArgument |
| B3 | ListProjectsByCurrentUser, ListServiceUserProjects |
with_permissions=[a,b] emits each project once per permission instead of grouping (N×M entries) |
| B4 | same | unknown permission name → Internal / NotFound instead of empty access_pairs |
B1 retest (empty id leak)
After the handler guard + service-level Filter.Principal.ID/Type check:
| # | Case | Result |
|---|---|---|
| B1.1 | ListProjectsByUser id="" |
invalid_argument ✓ (was leaking 430 projects before) |
| B1.2 | ListProjectsByUser valid id |
3 ✓ |
| B1.3 | ListServiceUserProjects id="" + valid orgId |
internal (leak path closed; remaining 500 is the interceptor's GetServiceUser("") shape — same class as B2 in #1647) |
| B1.4 | ListServiceUserProjects valid id |
1 ✓ |
Review feedback addressed:
- core/project/service.go: refuse Filter.Principal pointers with empty ID
or Type. Catches programmer error without affecting the admin path,
which now passes Filter{} (no Principal) instead of an empty struct.
- internal/api/v1beta1connect/user.go::ListProjectsByUser: build the
Filter.Principal only when the request id is non-empty; empty id keeps
the listing-returns-all convention.
- internal/api/v1beta1connect/serviceuser.go::ListServiceUserProjects:
same conditional-Principal pattern.
- core/aggregates/orgpats: update the stale SpiceDB comment now that
resolution goes through membership/policy; tighten test to assert the
Principal+OrgID filter shape and call count.
- core/userpat/service_test.go: switch four List mocks to MatchedBy on
the Principal+OrgID filter so wiring regressions fail loudly.
- internal/api/v1beta1connect/serviceuser_test.go: add a case that
exercises the OrgId forwarding path explicitly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
core/userpat/service_test.go (2)
649-651: ⚡ Quick winMake
ProjectService.Listmandatory in the specific-project scope test.For this scenario, access validation depends on calling
ProjectService.List; using.Maybe()can let a missed call slip through.Suggested test tightening
- projSvc.On("List", mock.Anything, mock.MatchedBy(func(f project.Filter) bool { + projSvc.On("List", mock.Anything, mock.MatchedBy(func(f project.Filter) bool { return f.OrgID == "org-1" && f.Principal != nil && f.Principal.ID == "user-1" && f.Principal.Type == schema.UserPrincipal - })).Return([]project.Project{ + })).Return([]project.Project{ {ID: "proj-a"}, {ID: "proj-b"}, - }, nil).Maybe() + }, nil).Once()
2566-2568: ⚡ Quick winUse strict call expectations in both
ValidateProjectAccesssubtests.These subtests are asserting project-access validation behavior;
.Maybe()weakens that contract and can miss regressions where visibility checks are skipped.Suggested test tightening
- projSvc.On("List", mock.Anything, mock.MatchedBy(func(f project.Filter) bool { + projSvc.On("List", mock.Anything, mock.MatchedBy(func(f project.Filter) bool { return f.OrgID == "org-1" && f.Principal != nil && f.Principal.ID == "user-1" && f.Principal.Type == schema.UserPrincipal - })).Return([]project.Project{ + })).Return([]project.Project{ {ID: "proj-in-org"}, - }, nil) + }, nil).Once()Also applies to: 2606-2608
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5fe65e9b-7676-48c7-8109-a44c68f3d013
📒 Files selected for processing (8)
core/aggregates/orgpats/service.gocore/aggregates/orgpats/service_test.gocore/project/service.gocore/project/service_test.gocore/userpat/service_test.gointernal/api/v1beta1connect/serviceuser.gointernal/api/v1beta1connect/serviceuser_test.gointernal/api/v1beta1connect/user.go
…ojects
Both handlers previously built a project.Filter without a Principal when the
request id was empty, falling through to a plain repository.List that returned
every project in the system. ListProjectsByUser is parameterized by a specific
user id, not a generic filtered listing — empty id is malformed input, not "no
filter". AdminService/ListProjects is the dedicated all-projects endpoint.
Reject empty id upfront with InvalidArgument. ListServiceUserProjects had the
same shape; currently masked by the auth interceptor erroring on
GetServiceUser(""), but the unsafe pattern is removed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
whoAbhishekSah
approved these changes
May 26, 2026
- project_repository.go: the Postgres bind-parameter ceiling (32767) is
real, but ~10 other call sites use the same goqu.Op{"in"} pattern
without per-site caps. A targeted cap here would be asymmetric; the
fix is structural (switch to = ANY($1::uuid[]) globally) if the limit
ever bites.
- user_projects_repository.go: the divergence with
project.Service.List(Filter{Principal}) is a tracked product/UX
question and belongs on the issue tracker, not as a source TODO.
Co-Authored-By: Claude Opus 4.7 (1M context) <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
3 participants
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
Switches every reader of "projects a principal sees" off
project.Service.ListByUser(SpiceDBLookupResources) onto Postgres policy reads viamembership.Service.ListProjectsByPrincipal. Addsproject.Filter.Principalso the composition lives insideService.List. Three handler call sites and two internal callers migrated. Parent: #1478.Changes
project.Filter.Principalandmembership.Service.ListProjectsByPrincipal(ctx, principal, orgID, nonInherited).Service.Listbranches onFilter.Principal: shim → intersect withProjectIDs→ short-circuit empty → fall through to repo.ListProjectsByUser,ListProjectsByCurrentUser,ListServiceUserProjects.orgpats.resolveAllProjectsScope,userpat.validateProjectAccess.project.Service.ListByUser,listNonInheritedProjectIDs,intersectPATScope, andRelationService.LookupResourcesfrom the project package (LookupSubjectsstays — still used byserviceuser.Service.ListByOrg).ListByUserfromorgpats.ProjectService,userpat.ProjectService, andv1beta1connect.ProjectServiceinterfaces.project.GroupServiceinterface toGetonly (still consumed byvalidatePrincipal).projectService.SetMembershipService(membershipService)incmd/serve.go.Test Plan