feat: add service user support to membership validatePrincipal#1562
feat: add service user support to membership validatePrincipal#1562whoAbhishekSah merged 1 commit intomainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 25 minutes and 15 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe changes extend the membership service to support service user principals by introducing a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Coverage Report for CI Build 24769902733Coverage increased (+0.03%) to 42.258%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Manual end-to-end test reportTested against branch SDK flow (matches the 1561 test shape)
Regression: user-principal path through
|
| Check | Result |
|---|---|
AdminService/AddOrganizationMembers adds bob as viewer (role UUID) |
✅ 200 success:true |
| Same call again | ✅ 200 with error:"principal is already a member of this resource" — ErrAlreadyMember still surfaces correctly |
ListOrganizationUsers reflects the new member |
✅ |
PR surface area — what's actually reachable
| Aspect | Status |
|---|---|
DI wiring (cmd/serve.go) — server boots with the new serviceUserService arg |
✅ observed live (running debug binary came up on this branch) |
Error message (core/membership/errors.go) — ErrInvalidPrincipal now "unsupported principal type" (was "only user principals are supported") |
✅ confirmed in diff |
validatePrincipal(app/serviceuser) (core/membership/service.go:361-373) — reachable via any public RPC today |
AddOrganizationMember hardcodes schema.UserPrincipal (admin handler organization.go:561, invitation/service.go:316, domain/service.go:161, organization/service.go:468; the principal.Type path in organization/service.go:212 is guarded by ErrUserPrincipalOnly). Pure prerequisite wiring — the new branch starts firing in the follow-up PR that migrates serviceuser.Create to go through membership. Unit tests in core/membership/service_test.go cover the branch directly. |
Delete flow
| Check | Result |
|---|---|
DeleteServiceUser → 200 |
✅ |
serviceusers row hard-deleted |
✅ |
serviceuser_credentials cascaded |
✅ |
| Deleted SU's token rejected on next call | ✅ 401 |
policies row for SU (project policy) |
core/membership migration (#1478) |
Verdict
LGTM. The validatePrincipal change is a well-scoped prerequisite: the SU branch is fully unit-tested, the DI wiring is exercised by every server start, and the user-principal path is regression-clean end-to-end. The error-message rewording is consistent with the new behaviour. The new SU code path remains dormant in RPCs until the follow-up lands, exactly as advertised.
whoAbhishekSah
force-pushed
the
feat/membership-serviceuser-support
branch
from
2f107e7 to
ac2bf4b
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 (1)
core/membership/service.go (1)
591-593:⚠️ Potential issue | 🟠 MajorRecord service users as service-user audit targets.
Now that
validatePrincipalcan returnschema.ServiceUserPrincipal, add/role-change audit records still persistpkgAuditRecord.UserType. The remove path already maps principal type to audit type; do the same here to avoid misclassifying service-user membership changes.Suggested audit target mapping
func (s *Service) auditOrgMemberRoleChanged(ctx context.Context, org organization.Organization, p principalInfo, roleID string) { + targetType := auditRecordTargetType(p) s.auditRecordRepository.Create(ctx, auditrecord.AuditRecord{ @@ Target: &auditrecord.Target{ ID: p.ID, - Type: pkgAuditRecord.UserType, + Type: targetType, Name: p.Name,func (s *Service) auditOrgMemberAdded(ctx context.Context, org organization.Organization, p principalInfo, roleID string) { + targetType := auditRecordTargetType(p) s.auditRecordRepository.Create(ctx, auditrecord.AuditRecord{ @@ Target: &auditrecord.Target{ ID: p.ID, - Type: pkgAuditRecord.UserType, + Type: targetType, Name: p.Name,func auditRecordTargetType(p principalInfo) pkgAuditRecord.EntityType { switch p.Type { case schema.ServiceUserPrincipal: return pkgAuditRecord.ServiceUserType default: return pkgAuditRecord.UserType } }Also applies to: 620-622
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 07d2cd7d-201c-44a2-8430-d80e4d463188
📒 Files selected for processing (5)
cmd/serve.gocore/membership/errors.gocore/membership/mocks/service_user_service.gocore/membership/service.gocore/membership/service_test.go
whoAbhishekSah
force-pushed
the
feat/membership-serviceuser-support
branch
from
ac2bf4b to
4497782
Compare
There was a problem hiding this comment.
Pull request overview
Adds service-user awareness to the core/membership package so membership operations can validate app/serviceuser principals (existence, enabled state, org ownership) as a prerequisite for migrating service-user creation to go through membership.
Changes:
- Introduce
ServiceUserServiceinterface and inject it intomembership.NewService. - Extend
validatePrincipal()to acceptschema.ServiceUserPrincipaland validate org ownership + enabled state. - Update membership unit tests and bootstrap wiring to include the new dependency.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| core/membership/service.go | Adds ServiceUserService dependency and extends principal validation to include service users. |
| core/membership/service_test.go | Updates constructor calls and adds tests covering service-user membership addition validation. |
| core/membership/mocks/service_user_service.go | Adds generated mock for the new ServiceUserService dependency. |
| core/membership/errors.go | Updates ErrInvalidPrincipal message. |
| cmd/serve.go | Wires serviceUserService into membership.NewService during bootstrap. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
whoAbhishekSah
force-pushed
the
feat/membership-serviceuser-support
branch
from
4497782 to
9cb8767
Compare
whoAbhishekSah
force-pushed
the
feat/membership-serviceuser-support
branch
from
9cb8767 to
8479d19
Compare
There was a problem hiding this comment.
Pull request overview
This PR extends the core/membership package to support app/serviceuser principals in validatePrincipal, enabling membership mutations to validate service users (existence + enabled state + org ownership) and wiring the needed dependency through bootstrap and tests as a prerequisite for migrating service user creation to use the membership service.
Changes:
- Add
ServiceUserServiceinterface + dependency injection intomembership.NewService, and wire it incmd/serve.go. - Extend
validatePrincipal()to handleschema.ServiceUserPrincipal(org match + disabled check) and add service-user-specific membership tests. - Update principal-related error messaging to be type-agnostic and adjust audit target typing/metadata to support non-user principals.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| core/membership/service.go | Adds ServiceUserService dependency and service-user-aware principal validation + audit target typing changes. |
| core/membership/errors.go | Updates principal error message(s) and introduces ErrPrincipalNotInOrg. |
| core/membership/service_test.go | Updates constructor usage for new dependency and adds AddOrganizationMember coverage for service users. |
| core/membership/mocks/service_user_service.go | Adds mockery-generated mock for ServiceUserService. |
| cmd/serve.go | Wires serviceUserService into membership.NewService during API dependency construction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
whoAbhishekSah
force-pushed
the
feat/membership-serviceuser-support
branch
from
8479d19 to
e9c7be4
Compare
Extend the membership package to validate service user principals alongside regular users. This is a prerequisite for migrating service user org membership from direct relation creation to the membership package. Changes: - Add orgID param to validatePrincipal for cross-org validation - Implement ServiceUserPrincipal case with org ownership + disabled checks - Add ErrPrincipalNotInOrg for cross-org rejection (distinct from ErrInvalidPrincipal) - Use serviceuser.ErrDisabled instead of user.ErrDisabled - Fix audit helpers to use principalTypeToAuditType and conditional email - Update docstrings for AddOrganizationMember and SetOrganizationMemberRole Ref: #1478 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
whoAbhishekSah
force-pushed
the
feat/membership-serviceuser-support
branch
from
e9c7be4 to
57896c5
Compare
Summary
ServiceUserServiceinterface and dependency to the membership packagevalidatePrincipal()to acceptapp/serviceuserprincipals (validates existence and enabled state)ErrInvalidPrincipalmessage to be type-agnosticserviceUserServiceintomembership.NewServicein bootstrapWhy this is needed
Today, when a service user is created, the backend only creates a bare
org#member@serviceuserrelation — no policy, no role. The SDK confirms this:CreateServiceUser(orgId, title)→ backend creates relation onlyCreatePolicyForProject(projectId, app_project_owner, app/serviceuser:{id})→ project access via policyThe service user has no org-level role — just a raw SpiceDB relation. This means:
The next PR will migrate
serviceuser.Create()to callmembershipService.AddOrganizationMember(), which creates a proper policy with an explicit role (e.g.,app_organization_viewer). This PR is the prerequisite — the membership package needs to know how to validate a service user principal before it can add one.Context
Part of the membership package migration (#1478). Follows #1561 which removes the unused
serviceuser#userrelation.Test plan
app/unknowninstead ofapp/serviceuser🤖 Generated with Claude Code