feat: implement SearchOrganizationPATs admin RPC#1482
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds organization-level PAT search: new orgpats service (with scope enrichment), PostgreSQL repository with RQL support, Connect RPC handler and mocks/tests, DI wiring and authorization rule, and a Makefile/go.mod dependency update. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 |
Pull Request Test Coverage Report for Build 23638162435Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
internal/api/v1beta1connect/organization_pats_test.go (2)
165-204: Missing mock expectation assertion in standalone test.The standalone test
"should transform scopes correctly"creates a new mock but doesn't callAssertExpectations(t)at the end. While this may pass, it won't verify that the expected mock calls were made.Proposed fix
assert.True(t, scopeTypes[schema.OrganizationNamespace]) assert.True(t, scopeTypes[schema.ProjectNamespace]) + + mockOrgPATsSrv.AssertExpectations(t) })
206-224: Missing mock expectation assertion in pagination test.Similar to the scope transformation test, this test should call
AssertExpectations(t)for consistency with the table-driven tests.Proposed fix
assert.Equal(t, uint32(10), resp.Msg.GetPagination().GetOffset()) assert.Equal(t, uint32(30), resp.Msg.GetPagination().GetLimit()) assert.Equal(t, uint32(100), resp.Msg.GetPagination().GetTotalCount()) + + mockOrgPATsSrv.AssertExpectations(t) })core/aggregates/orgpats/service_test.go (1)
93-103: Weak assertion in "all-projects" resolution test.The combination of
.Maybe()on the mock expectation and the conditionalif len(result.PATs[0].Scopes[0].ResourceIDs) > 0means this test will pass even ifListByUseris never called and resolution doesn't happen. This undermines the test's purpose of verifying SpiceDB resolution.Consider making the assertion unconditional and removing
.Maybe()to ensure the resolution behavior is actually tested:Proposed fix
repo.EXPECT().Search(mock.Anything, orgID, query).Return(repoResult, nil) - projSvc.EXPECT().ListByUser(mock.Anything, mock.Anything, mock.Anything). - Return([]project.Project{{ID: "proj-1"}, {ID: "proj-2"}}, nil).Maybe() + projSvc.EXPECT().ListByUser(mock.Anything, mock.Anything, mock.Anything). + Return([]project.Project{{ID: "proj-1"}, {ID: "proj-2"}}, nil) svc := orgpats.NewService(repo, projSvc) result, err := svc.Search(ctx, orgID, query) assert.NoError(t, err) assert.Len(t, result.PATs, 1) // 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") - } + assert.ElementsMatch(t, []string{"proj-1", "proj-2"}, result.PATs[0].Scopes[0].ResourceIDs)internal/api/v1beta1connect/organization_pats.go (1)
60-64: Potential integer truncation when castingTotalCount.
result.Pagination.TotalCountisint64but is cast touint32for the protobuf response. For organizations with more than ~4.2 billion PATs (uint32 max), this would silently truncate. While unlikely in practice, this is a latent issue.Consider documenting this limitation or using a larger type in the proto if needed in the future.
internal/store/postgres/org_pats_repository.go (2)
319-329: Edge case: rows with unexpected ResourceType/GrantRelation combinations are silently skipped.The switch statement (lines 319-329) has a
default: continuethat silently skips rows that don't match expected patterns. While this is defensive, it could mask data inconsistencies in the policies table.Consider logging a warning for unexpected policy row patterns to aid debugging:
29-57: Filter fields include "id" but sort fields do not.
orgPATFilterFieldsincludes"id": "p.id"butorgPATSortFieldsdoes not include "id". Users can filter by ID but cannot sort by ID, which may be unexpected.Proposed fix if sorting by ID should be supported
var orgPATSortFields = map[string]string{ + "id": "p.id", "title": "p.title", "created_by_title": "u.title",
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 15e7d3be-73e6-4481-b1f6-73e689411f71
⛔ Files ignored due to path filters (4)
go.sumis excluded by!**/*.sumproto/v1beta1/admin.pb.gois excluded by!**/*.pb.go,!proto/**proto/v1beta1/admin.pb.validate.gois excluded by!proto/**proto/v1beta1/frontierv1beta1connect/admin.connect.gois excluded by!proto/**
📒 Files selected for processing (16)
Makefilecmd/serve.gocore/aggregates/orgpats/mocks/project_service.gocore/aggregates/orgpats/mocks/repository.gocore/aggregates/orgpats/service.gocore/aggregates/orgpats/service_test.gogo.modinternal/api/api.gointernal/api/v1beta1connect/interfaces.gointernal/api/v1beta1connect/mocks/org_pa_ts_service.gointernal/api/v1beta1connect/organization_pats.gointernal/api/v1beta1connect/organization_pats_test.gointernal/api/v1beta1connect/v1beta1connect.gointernal/store/postgres/org_pats_repository.gointernal/store/postgres/org_pats_repository_test.gopkg/server/connect_interceptors/authorization.go
3 participants
Summary
SearchOrganizationPATsadmin RPC to list all PATs in an org with enriched dataIsSuperUser)How it works
SQL (2 queries)
Uses subquery pagination — inner subquery selects paginated PATs with RQL applied, outer query joins users + policies for enrichment:
Returns multiple rows per PAT (one per policy). Go code groups rows by PAT ID to build nested scopes.
Scope resolution
RQL support
Supported filter operators: eq, neq, gt, gte, lt, lte, like, notlike, ilike, notilike, empty, notempty, in, notin
Default and max limit: 30 (reduced from standard 50 due to SpiceDB enrichment cost).
Sample request/response
Request
Response
Filter example