feat: add role and policy enforcement for PATs#1419
feat: add role and policy enforcement for PATs#1419AmanGIT07 wants to merge 5 commits intofeat/create-pat-rpcfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/userpat/service.go (1)
113-120:⚠️ Potential issue | 🔴 CriticalPAT creation is non-atomic with policy provisioning, allowing partial state.
If policy creation fails after
repo.Create, the token is already persisted; and if failure occurs mid-loop, some policies may already exist. That breaks all-or-nothing behavior and can leave orphaned/partial authorization state.Also applies to: 205-220, 253-284
🧹 Nitpick comments (3)
core/role/service.go (1)
105-160: Consider extracting the repeated relation-creation pattern into a helper.The same
relationService.Createblock is now repeated three times (user, serviceuser, PAT) for each permission, differing only in theSubject.Namespaceand the denial check. A small helper iterating over a slice of{namespace, skipCheck}tuples would reduce duplication and make adding future principal types trivial.This is purely a readability/maintainability suggestion — the current code is correct.
♻️ Sketch of a possible refactor
func (s Service) createRolePermissionRelation(ctx context.Context, roleID string, permissions []string) error { + type principalDef struct { + namespace string + denyCheck bool // if true, skip when perm is in patDeniedPerms + } + principals := []principalDef{ + {namespace: schema.UserPrincipal}, + {namespace: schema.ServiceUserPrincipal}, + {namespace: schema.PATPrincipal, denyCheck: true}, + } for _, perm := range permissions { - _, err := s.relationService.Create(ctx, relation.Relation{ - ...UserPrincipal... - }) - ... - _, err = s.relationService.Create(ctx, relation.Relation{ - ...ServiceUserPrincipal... - }) - ... - if _, denied := s.patDeniedPerms[perm]; !denied { - _, err = s.relationService.Create(ctx, relation.Relation{ - ...PATPrincipal... - }) - ... - } + for _, p := range principals { + if p.denyCheck { + if _, denied := s.patDeniedPerms[perm]; denied { + continue + } + } + if _, err := s.relationService.Create(ctx, relation.Relation{ + Object: relation.Object{ID: roleID, Namespace: schema.RoleNamespace}, + Subject: relation.Subject{ID: "*", Namespace: p.namespace}, + RelationName: perm, + }); err != nil { + return err + } + } } return nil }internal/api/v1beta1connect/user_pat_test.go (1)
184-225: Add coverage forErrRoleNotFoundinvalid-argument mapping.The table now covers denied and unsupported scope errors, but not
userpat.ErrRoleNotFound, which is also part of the new API error contract.🧪 Suggested test case
+ { + name: "should return invalid argument when role is not found", + setup: func(ps *mocks.UserPATService, as *mocks.AuthnService) { + as.EXPECT().GetPrincipal(mock.Anything).Return(authenticate.Principal{ + ID: testUserID, + Type: schema.UserPrincipal, + User: &user.User{ID: testUserID}, + }, nil) + ps.EXPECT().Create(mock.Anything, mock.AnythingOfType("userpat.CreateRequest")). + Return(userpat.PersonalAccessToken{}, "", fmt.Errorf("resolving roles: %w", userpat.ErrRoleNotFound)) + }, + patConfig: defaultPATConfig, + request: connect.NewRequest(&frontierv1beta1.CreateCurrentUserPersonalTokenRequest{ + Title: "my-token", + OrgId: testOrgID, + RoleIds: []string{testRoleID}, + ExpiresAt: timestamppb.New(testTime), + }), + want: nil, + wantErr: connect.NewError(connect.CodeInvalidArgument, userpat.ErrRoleNotFound), + },internal/bootstrap/service_test.go (1)
54-237: AssertroleSvcexpectations in each subtest.Most subtests configure
roleSvc.On("List", ...)but never callroleSvc.AssertExpectations(t). That allows false positives ifmigratePATRelationsstops listing roles.🧰 Minimal pattern to apply per subtest
roleSvc := new(mockRoleService) relSvc := new(mockRelationService) + t.Cleanup(func() { + roleSvc.AssertExpectations(t) + relSvc.AssertExpectations(t) + })
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
proto/v1beta1/frontier.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (22)
Makefilecmd/serve.gocore/authenticate/mocks/auth_handler.gocore/authenticate/mocks/authenticator_func.gocore/policy/service.gocore/role/service.gocore/role/service_test.gocore/userpat/config.gocore/userpat/errors.gocore/userpat/mocks/policy_service.gocore/userpat/mocks/role_service.gocore/userpat/service.gocore/userpat/service_test.gointernal/api/v1beta1connect/user_pat.gointernal/api/v1beta1connect/user_pat_test.gointernal/bootstrap/generator.gointernal/bootstrap/schema/base_schema.zedinternal/bootstrap/schema/schema.gointernal/bootstrap/service.gointernal/bootstrap/service_test.gointernal/bootstrap/testdata/compiled_schema.zedproto/v1beta1/frontier.pb.validate.go
| grantRelation := schema.RoleGrantRelationName | ||
| if gr, ok := pol.Metadata[schema.GrantRelationMetadataKey].(string); ok && gr != "" { | ||
| grantRelation = gr | ||
| } |
There was a problem hiding this comment.
Validate grant_relation metadata against allowed relations.
Line 120-123 accepts any non-empty string. Invalid values can fail after policy upsert and create partial state; unexpected values can also bind against unintended relation names.
✅ Proposed validation guard
import (
"context"
+ "fmt"
@@
// bind policy to resource
grantRelation := schema.RoleGrantRelationName
- if gr, ok := pol.Metadata[schema.GrantRelationMetadataKey].(string); ok && gr != "" {
- grantRelation = gr
+ if raw, ok := pol.Metadata[schema.GrantRelationMetadataKey]; ok {
+ gr, ok := raw.(string)
+ if !ok {
+ return fmt.Errorf("invalid %q metadata type", schema.GrantRelationMetadataKey)
+ }
+ if gr != "" {
+ if gr != schema.RoleGrantRelationName && gr != schema.PATGrantRelationName {
+ return fmt.Errorf("invalid %q metadata value: %q", schema.GrantRelationMetadataKey, gr)
+ }
+ grantRelation = gr
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| grantRelation := schema.RoleGrantRelationName | |
| if gr, ok := pol.Metadata[schema.GrantRelationMetadataKey].(string); ok && gr != "" { | |
| grantRelation = gr | |
| } | |
| grantRelation := schema.RoleGrantRelationName | |
| if raw, ok := pol.Metadata[schema.GrantRelationMetadataKey]; ok { | |
| gr, ok := raw.(string) | |
| if !ok { | |
| return fmt.Errorf("invalid %q metadata type", schema.GrantRelationMetadataKey) | |
| } | |
| if gr != "" { | |
| if gr != schema.RoleGrantRelationName && gr != schema.PATGrantRelationName { | |
| return fmt.Errorf("invalid %q metadata value: %q", schema.GrantRelationMetadataKey, gr) | |
| } | |
| grantRelation = gr | |
| } | |
| } |
| func newSuccessMocks(t *testing.T) (*mocks.OrganizationService, *mocks.RoleService, *mocks.PolicyService, *mocks.AuditRecordRepository) { | ||
| t.Helper() | ||
| orgSvc := mocks.NewOrganizationService(t) | ||
| orgSvc.On("GetRaw", mock.Anything, mock.Anything). | ||
| Return(organization.Organization{ID: "org-1", Title: "Test Org"}, nil).Maybe() | ||
| roleSvc := mocks.NewRoleService(t) | ||
| roleSvc.On("List", mock.Anything, mock.Anything). | ||
| Return([]role.Role{{ | ||
| ID: "role-1-id", | ||
| Name: "test-role", | ||
| Scopes: []string{schema.OrganizationNamespace}, | ||
| }}, nil).Maybe() |
There was a problem hiding this comment.
Role fixture IDs are inconsistent with requested RoleIDs, so several tests exercise the wrong path.
These mocks return role IDs with -id suffix while requests pass different IDs. With current service validation, that triggers ErrRoleNotFound before the policy assertions run.
✅ Example fix (align returned role IDs with requested RoleIDs)
func newSuccessMocks(t *testing.T) (*mocks.OrganizationService, *mocks.RoleService, *mocks.PolicyService, *mocks.AuditRecordRepository) {
@@
roleSvc := mocks.NewRoleService(t)
roleSvc.On("List", mock.Anything, mock.Anything).
Return([]role.Role{{
- ID: "role-1-id",
+ ID: "role-1",
Name: "test-role",
Scopes: []string{schema.OrganizationNamespace},
}}, nil).Maybe()Apply the same alignment in explicit policy tests (e.g. org-role-1, proj-role-1).
Also applies to: 511-516, 553-558, 598-603
| for _, r := range roles { | ||
| if !slices.Contains(r.Scopes, schema.ProjectNamespace) && !slices.Contains(r.Scopes, schema.OrganizationNamespace) { | ||
| return nil, fmt.Errorf("role %s has scopes %v: %w", r.Name, r.Scopes, ErrUnsupportedScope) | ||
| } |
There was a problem hiding this comment.
Unsupported scopes can pass validation when a role has mixed scopes.
Current logic only ensures at least one supported scope exists. A role with ["app/project", "app/group"] is accepted, but should be rejected due to unsupported scope presence.
🔧 Tighten scope validation to reject any unsupported scope
for _, r := range roles {
- if !slices.Contains(r.Scopes, schema.ProjectNamespace) && !slices.Contains(r.Scopes, schema.OrganizationNamespace) {
- return nil, fmt.Errorf("role %s has scopes %v: %w", r.Name, r.Scopes, ErrUnsupportedScope)
- }
+ if len(r.Scopes) == 0 {
+ return nil, fmt.Errorf("role %s has scopes %v: %w", r.Name, r.Scopes, ErrUnsupportedScope)
+ }
+ for _, scope := range r.Scopes {
+ if scope != schema.ProjectNamespace && scope != schema.OrganizationNamespace {
+ return nil, fmt.Errorf("role %s has scopes %v: %w", r.Name, r.Scopes, ErrUnsupportedScope)
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, r := range roles { | |
| if !slices.Contains(r.Scopes, schema.ProjectNamespace) && !slices.Contains(r.Scopes, schema.OrganizationNamespace) { | |
| return nil, fmt.Errorf("role %s has scopes %v: %w", r.Name, r.Scopes, ErrUnsupportedScope) | |
| } | |
| for _, r := range roles { | |
| if len(r.Scopes) == 0 { | |
| return nil, fmt.Errorf("role %s has scopes %v: %w", r.Name, r.Scopes, ErrUnsupportedScope) | |
| } | |
| for _, scope := range r.Scopes { | |
| if scope != schema.ProjectNamespace && scope != schema.OrganizationNamespace { | |
| return nil, fmt.Errorf("role %s has scopes %v: %w", r.Name, r.Scopes, ErrUnsupportedScope) | |
| } | |
| } | |
| } |
| case errors.Is(err, userpat.ErrRoleNotFound): | ||
| return nil, connect.NewError(connect.CodeInvalidArgument, err) | ||
| case errors.Is(err, userpat.ErrDeniedRole): | ||
| return nil, connect.NewError(connect.CodeInvalidArgument, userpat.ErrDeniedRole) | ||
| case errors.Is(err, userpat.ErrUnsupportedScope): | ||
| return nil, connect.NewError(connect.CodeInvalidArgument, userpat.ErrUnsupportedScope) | ||
| default: |
There was a problem hiding this comment.
Return the sentinel error for ErrRoleNotFound consistently.
Line 62 returns wrapped err, while other role-validation branches return stable sentinel errors. Use userpat.ErrRoleNotFound directly for consistent API error text.
🩹 Proposed consistency fix
case errors.Is(err, userpat.ErrRoleNotFound):
- return nil, connect.NewError(connect.CodeInvalidArgument, err)
+ return nil, connect.NewError(connect.CodeInvalidArgument, userpat.ErrRoleNotFound)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case errors.Is(err, userpat.ErrRoleNotFound): | |
| return nil, connect.NewError(connect.CodeInvalidArgument, err) | |
| case errors.Is(err, userpat.ErrDeniedRole): | |
| return nil, connect.NewError(connect.CodeInvalidArgument, userpat.ErrDeniedRole) | |
| case errors.Is(err, userpat.ErrUnsupportedScope): | |
| return nil, connect.NewError(connect.CodeInvalidArgument, userpat.ErrUnsupportedScope) | |
| default: | |
| case errors.Is(err, userpat.ErrRoleNotFound): | |
| return nil, connect.NewError(connect.CodeInvalidArgument, userpat.ErrRoleNotFound) | |
| case errors.Is(err, userpat.ErrDeniedRole): | |
| return nil, connect.NewError(connect.CodeInvalidArgument, userpat.ErrDeniedRole) | |
| case errors.Is(err, userpat.ErrUnsupportedScope): | |
| return nil, connect.NewError(connect.CodeInvalidArgument, userpat.ErrUnsupportedScope) | |
| default: |
| relation app_group_administer: app/user:* | app/serviceuser:* | app/pat:* | ||
| relation app_group_delete: app/user:* | app/serviceuser:* | app/pat:* | ||
| relation app_group_update: app/user:* | app/serviceuser:* | app/pat:* | ||
| relation app_group_get: app/user:* | app/serviceuser:* | app/pat:* |
There was a problem hiding this comment.
Constrain PAT principals for unsupported group scope permissions.
Line 162-165 still authorizes app/pat:* for group permissions, while PAT group scope is intended to be unsupported. This leaves a permissive schema path if upstream validation is bypassed.
🔒 Proposed schema hardening
- relation app_group_administer: app/user:* | app/serviceuser:* | app/pat:*
- relation app_group_delete: app/user:* | app/serviceuser:* | app/pat:*
- relation app_group_update: app/user:* | app/serviceuser:* | app/pat:*
- relation app_group_get: app/user:* | app/serviceuser:* | app/pat:*
+ relation app_group_administer: app/user:* | app/serviceuser:*
+ relation app_group_delete: app/user:* | app/serviceuser:*
+ relation app_group_update: app/user:* | app/serviceuser:*
+ relation app_group_get: app/user:* | app/serviceuser:*📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| relation app_group_administer: app/user:* | app/serviceuser:* | app/pat:* | |
| relation app_group_delete: app/user:* | app/serviceuser:* | app/pat:* | |
| relation app_group_update: app/user:* | app/serviceuser:* | app/pat:* | |
| relation app_group_get: app/user:* | app/serviceuser:* | app/pat:* | |
| relation app_group_administer: app/user:* | app/serviceuser:* | |
| relation app_group_delete: app/user:* | app/serviceuser:* | |
| relation app_group_update: app/user:* | app/serviceuser:* | |
| relation app_group_get: app/user:* | app/serviceuser:* |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/userpat/service.go (1)
113-120:⚠️ Potential issue | 🟠 MajorPartial failure leaves orphan tokens without policies.
If
createPoliciesfails (e.g., SpiceDB unavailable), the token already exists in the database (line 113) but has no associated policies. This creates an orphan token that grants no permissions but consumes the user's token quota.The Repository interface lacks a Delete method for cleanup, and the Create operation is not wrapped in a transaction. Transaction support exists elsewhere in the codebase (using
WithTxn), so wrapping token creation and policy creation in a transaction is a viable solution.
♻️ Duplicate comments (2)
core/policy/service.go (1)
121-133:⚠️ Potential issue | 🟠 MajorValidate
grant_relationbefore any relation writes.Line 122-Line 133 runs after two
relationService.Createcalls (Line 90-Line 118). If validation fails on Line 125 or Line 129,AssignRoleexits with partially-created bindings.Proposed fix (resolve metadata first, then write relations)
func (s Service) AssignRole(ctx context.Context, pol Policy) error { + grantRelation := schema.RoleGrantRelationName + if raw, ok := pol.Metadata[schema.GrantRelationMetadataKey]; ok { + gr, ok := raw.(string) + if !ok { + return fmt.Errorf("invalid %q metadata type", schema.GrantRelationMetadataKey) + } + if gr != "" { + if gr != schema.RoleGrantRelationName && gr != schema.PATGrantRelationName { + return fmt.Errorf("invalid %q metadata value: %q", schema.GrantRelationMetadataKey, gr) + } + grantRelation = gr + } + } + // bind role with user subjectSubRelation := "" if pol.PrincipalType == schema.GroupPrincipal { subjectSubRelation = schema.MemberRelationName } @@ - grantRelation := schema.RoleGrantRelationName - if raw, ok := pol.Metadata[schema.GrantRelationMetadataKey]; ok { - gr, ok := raw.(string) - if !ok { - return fmt.Errorf("invalid %q metadata type", schema.GrantRelationMetadataKey) - } - if gr != "" { - if gr != schema.RoleGrantRelationName && gr != schema.PATGrantRelationName { - return fmt.Errorf("invalid %q metadata value: %q", schema.GrantRelationMetadataKey, gr) - } - grantRelation = gr - } - } _, err = s.relationService.Create(ctx, relation.Relation{ Object: relation.Object{ ID: pol.ResourceID, Namespace: pol.ResourceType, },internal/bootstrap/schema/base_schema.zed (1)
162-165:⚠️ Potential issue | 🟠 MajorRemove PAT principals from group role relations to match unsupported PAT group scope.
Line 162-Line 165 still authorize
app/pat:*for group permissions, leaving a permissive fallback if request-time scope validation is bypassed.🔒 Proposed schema hardening
- relation app_group_administer: app/user:* | app/serviceuser:* | app/pat:* - relation app_group_delete: app/user:* | app/serviceuser:* | app/pat:* - relation app_group_update: app/user:* | app/serviceuser:* | app/pat:* - relation app_group_get: app/user:* | app/serviceuser:* | app/pat:* + relation app_group_administer: app/user:* | app/serviceuser:* + relation app_group_delete: app/user:* | app/serviceuser:* + relation app_group_update: app/user:* | app/serviceuser:* + relation app_group_get: app/user:* | app/serviceuser:*In SpiceDB, if a relation type includes `app/pat:*` in a role permission relation, can PAT principals satisfy those permissions whenever matching rolebindings exist, even if application-level validation blocks assigning those rolebindings?
🧹 Nitpick comments (1)
core/userpat/service.go (1)
210-226: Consider handling roles with multiple scopes more explicitly.A role could have both
OrganizationNamespaceandProjectNamespacescopes. The currentswitchusesslices.Containswhich matches the first case that applies (ProjectNamespacetakes precedence). This may be intentional, but the behavior for multi-scope roles should be documented or validated upfront.💡 Document or validate multi-scope behavior
Consider adding a comment explaining the precedence, or validating that roles have exactly one supported scope:
// Note: For roles with multiple scopes, project scope takes precedence. // Roles are validated upfront to only contain supported scopes.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
proto/v1beta1/frontier.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (21)
Makefilecmd/serve.gocore/authenticate/mocks/authenticator_func.gocore/policy/service.gocore/role/service.gocore/role/service_test.gocore/userpat/config.gocore/userpat/errors.gocore/userpat/mocks/policy_service.gocore/userpat/mocks/role_service.gocore/userpat/service.gocore/userpat/service_test.gointernal/api/v1beta1connect/user_pat.gointernal/api/v1beta1connect/user_pat_test.gointernal/bootstrap/generator.gointernal/bootstrap/schema/base_schema.zedinternal/bootstrap/schema/schema.gointernal/bootstrap/service.gointernal/bootstrap/service_test.gointernal/bootstrap/testdata/compiled_schema.zedproto/v1beta1/frontier.pb.validate.go
# Conflicts: # Makefile # core/userpat/config.go # core/userpat/service.go # core/userpat/service_test.go # internal/api/v1beta1connect/user_pat.go # internal/api/v1beta1connect/user_pat_test.go # proto/v1beta1/frontier.pb.go # proto/v1beta1/frontier.pb.validate.go
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
AmanGIT07
force-pushed
the
feat/pat-scope-policies
branch
from
bd07dea to
4c1876f
Compare
1 participant
Summary
Implements PAT scope enforcement during token creation. When creating a personal access
token, the requested roles are resolved, validated, and bound to the token as scoped
policies in SpiceDB. Each role is handled by its scope:
grantedrelation on the organizationpat_grantedon the org (all projects) orgrantedoneach specific project
All role validation (existence, denied permissions, scope support) runs before the token
is persisted to avoid orphaned tokens in the database. Non-existent role IDs return
invalid_argumentwith the specific missing IDs. Validation is all-or-nothing — mixedvalid/invalid roles don't create partial state.
Also moves
migratePATRelationsfrom the role service into the bootstrap service whereit belongs as a startup-time reconciliation concern, keeping the role service interface
clean.
Test plan
role IDs, unsupported scope roles, and mixed valid/invalid roles — all return correct
error codes (
invalid_argument, notinternal)all role/scope combinations (org-scoped, project-scoped all, project-scoped specific)