feat: CreateCurrentUserPersonalToken RPC implementation#1401
feat: CreateCurrentUserPersonalToken RPC implementation#1401
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds Personal Access Token (PAT) functionality: domain model, service, repository, DB migrations, API RPC and handler, protobuf/schema updates, server wiring and config, authorization rule, mocks, extensive tests, and generated validation code. Also updates Makefile Proton commit. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/bootstrap/schema/schema.go (1)
255-260:⚠️ Potential issue | 🟡 MinorInclude
PATPrincipalinIsSystemNamespace()for consistency with other principals.All other principals—
UserPrincipal,ServiceUserPrincipal,SuperUserPrincipal,GroupPrincipal—are included in the check, butPATPrincipal(defined at line 75) is omitted. SincePATPrincipalis recognized as a valid namespace alias inParseNamespaceAliasIfRequired(), it should be treated the same way as other principals to maintain consistent behavior.
🧹 Nitpick comments (9)
pkg/auditrecord/consts.go (1)
57-58: Consider adding aPATDeletedEvent/PATRevokedEventconstant for completeness.Every other audited resource with a lifecycle (service users, sessions, billing entities) has at minimum a deletion/revocation event alongside the creation event. If PAT revocation or deletion is planned, pre-declaring the constant here keeps the audit vocabulary consistent and avoids a follow-up PR solely to add
"pat.deleted"or"pat.revoked".♻️ Proposed addition
// PAT Events PATCreatedEvent Event = "pat.created" + PATDeletedEvent Event = "pat.deleted"core/userpat/errors.go (1)
1-14: LGTM — clean, idiomatic sentinel error set.The
varblock witherrors.Newis the canonical Go pattern for package-level sentinel errors. All eight values are exported, clearly named with theErrprefix, and carry descriptive messages that unambiguously attribute them to the PAT domain. The distinction betweenErrExpiryInPast(creation-time validation) andErrExpiryExceeded(lifetime cap) is a nice touch.One optional consideration: adding a short package prefix such as
"userpat: personal access token not found"to each message can improve discoverability when these errors are wrapped and appear deep in a multi-layer error chain or log stream. Since "personal access token" already appears in every string, the attribution is already clear—this is purely a style preference.cmd/serve.go (1)
391-436:userPATRepodeclaration is separated fromuserPATServicecreation by ~45 unrelated lines.Every other repo/service pair in this function is declared back-to-back (e.g.,
prospectRepository/prospectServiceat lines 388-389). MovinguserPATRepoimmediately beforeuserPATServicewould keep the locality consistent.♻️ Suggested repositioning
- userPATRepo := postgres.NewUserPATRepository(dbc) - svUserRepo := postgres.NewServiceUserRepository(dbc) // ... (lines 393-435 unchanged) ... organizationService := organization.NewService(...) + + userPATRepo := postgres.NewUserPATRepository(dbc) userPATService := userpat.NewService(userPATRepo, cfg.App.PAT, organizationService, auditRecordRepository)core/userpat/config.go (1)
10-10:DefaultTokenLifetimehas noDefaultExpiry()helper to matchMaxExpiry().Any code that needs the parsed default lifetime must replicate the
time.ParseDuration+ fallback logic inline, risking divergence. Adding a symmetric helper ensures consistent parsing behaviour.♻️ Proposed addition
+func (c Config) DefaultExpiry() time.Duration { + d, err := time.ParseDuration(c.DefaultTokenLifetime) + if err != nil { + return 90 * 24 * time.Hour // matches the "2160h" default + } + return d +}internal/api/v1beta1connect/user_pat.go (1)
84-89: Silently swallowed metadata conversion error may hide data loss.If
ToStructPB()fails (e.g., due to unsupported value types in metadata), the error is ignored and metadata is silently omitted from the response. Consider logging the error so operators can diagnose missing metadata.🔧 Proposed fix
if pat.Metadata != nil { metaPB, err := pat.Metadata.ToStructPB() if err == nil { pbPAT.Metadata = metaPB + } else { + // Log metadata conversion failure so it's diagnosable + zap.L().Warn("failed to convert PAT metadata to protobuf", zap.String("pat_id", pat.ID), zap.Error(err)) } }core/userpat/service.go (2)
88-88:RolesandProjectIDsare silently discardedBoth fields flow through
CreateRequest, are passed into the audit metadata, but theTODOmeans no policies are created. Any caller relying on the returned token having the scoped access they requested will get an over-privileged (or under-privileged) token with no error surfaced.Would you like me to open a tracking issue or scaffold the policy-creation step once the role/project model is available?
107-108:maps.Copysilently overwrites"user_id"iftargetMetadatacontains that key
maps.Copycopies all key/value pairs from src into dst; when a key in src is already present in dst, the value in dst is overwritten. If any future caller ofcreateAuditRecordpasses"user_id"insidetargetMetadata, the explicitly setpat.UserIDbinding will be silently replaced. Build the merged map in the other order to give the explicituser_idprecedence:♻️ Proposed fix
- metadata := map[string]any{"user_id": pat.UserID} - maps.Copy(metadata, targetMetadata) + metadata := maps.Clone(targetMetadata) + metadata["user_id"] = pat.UserID // always authoritativecore/userpat/service_test.go (2)
272-301: "should hash the full token string" test case verifies nothing about the stored hashThe assertion — that
sha256(tokenValue)produces a 64-character hex string — is trivially true for any non-empty input and carries zero coverage value. It does not captureSecretHashfrom the mock call or compare it to the token.TestService_Create_HashVerification(lines 428-458) already does the correct end-to-end hash verification. Consider removing this table case or replacing it with a comment referencing the standalone test.
353-372: Table case "should generate unique tokens on each call" makes only oneCreatecallWithin the table loop body (lines 374-394) each case calls
Createexactly once, so this case cannot demonstrate uniqueness. It reads as an ownership claim that the table runner doesn't fulfil.TestService_Create_UniqueTokens(lines 397-426) already covers uniqueness correctly. Rename this case to something that reflects its actual intent (e.g.,"should succeed when count is 0") or remove it as a duplicate.
| frontierv1beta1connect.FrontierServiceCreateCurrentUserPersonalTokenProcedure: func(ctx context.Context, handler *v1beta1connect.ConnectHandler, req connect.AnyRequest) error { | ||
| pbreq := req.(*connect.Request[frontierv1beta1.CreateCurrentUserPersonalTokenRequest]) | ||
| return handler.IsAuthorized(ctx, relation.Object{Namespace: schema.OrganizationNamespace, ID: pbreq.Msg.GetOrgId()}, schema.GetPermission, req) | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all PAT-related procedure constants and check authorization maps
echo "=== PAT-related procedure constants in codebase ==="
rg -n "PersonalToken" --type go
echo ""
echo "=== PAT procedures in authorization.go ==="
rg -n "PersonalToken" pkg/server/connect_interceptors/authorization.goRepository: raystack/frontier
Length of output: 20840
🏁 Script executed:
#!/bin/bash
# Read the authorization map section to see exact permission for CreateCurrentUserPersonalToken
echo "=== Authorization entry for CreateCurrentUserPersonalToken (lines 700-715) ==="
sed -n '700,715p' pkg/server/connect_interceptors/authorization.go
echo ""
echo "=== Other Create* endpoints and their permissions ==="
rg -n "^[[:space:]]*frontierv1beta1connect\.FrontierServiceCreate" pkg/server/connect_interceptors/authorization.go -A 2
echo ""
echo "=== Schema permission constants ==="
rg -n "^[[:space:]]*(Get|Update|Create|Manage)Permission\s*=" --type go | head -30Repository: raystack/frontier
Length of output: 2490
🏁 Script executed:
#!/bin/bash
# Search for other Create* endpoints to verify the pattern
echo "=== All Create* endpoints in authorization.go with their permissions ==="
rg -n "FrontierServiceCreate|AdminServiceCreate" pkg/server/connect_interceptors/authorization.go -A 2 | head -80Repository: raystack/frontier
Length of output: 891
🏁 Script executed:
#!/bin/bash
# Search more broadly for all Create* procedures to see the full pattern
echo "=== All procedures matching Create pattern ==="
rg -n "case.*ServiceCreate" pkg/server/connect_interceptors/authorization.go -B 1 | head -100
echo ""
echo "=== Broader search: all FrontierServiceCreate procedures with full context ==="
rg -n "FrontierServiceCreate\w+Procedure:" pkg/server/connect_interceptors/authorization.go -A 2 | head -120Repository: raystack/frontier
Length of output: 945
🏁 Script executed:
#!/bin/bash
# Get a comprehensive view of all entries in the authorizationValidationMap
echo "=== Full authorizationValidationMap from authorization.go ==="
# Extract the entire map - assuming it's bounded by "var authorizationValidationMap" and "}"
awk '/var authorizationValidationMap/,/^}/' pkg/server/connect_interceptors/authorization.go | grep -E "(Procedure:|Permission|func\()" | head -100Repository: raystack/frontier
Length of output: 15806
🏁 Script executed:
#!/bin/bash
# Verify the actual permission values and understand the Create* endpoint patterns
echo "=== Schema permission constants definitions ==="
cat internal/bootstrap/schema/schema.go | grep -A 20 "GetPermission\|CreatePermission\|UpdatePermission\|ManagePermission" | head -30
echo ""
echo "=== Summary of Create* endpoints and their permissions ==="
rg "Create\w+Procedure.*Permission" pkg/server/connect_interceptors/authorization.go -o | sort | uniqRepository: raystack/frontier
Length of output: 1254
🏁 Script executed:
#!/bin/bash
# Get a cleaner view of all Create* procedures and their permissions
echo "=== All Create* procedures and their permissions ==="
rg -n "Create\w+Procedure.*func\(ctx" pkg/server/connect_interceptors/authorization.go -A 2 | grep -E "(Procedure:|Permission)" | paste - - | head -40Repository: raystack/frontier
Length of output: 174
🏁 Script executed:
#!/bin/bash
# Get all Create* procedures with their permissions
echo "=== All Create* procedures and their permissions ==="
rg -n "^\s+frontierv1beta1connect\.FrontierServiceCreate" pkg/server/connect_interceptors/authorization.go -A 2 | grep -v "^--$"Repository: raystack/frontier
Length of output: 868
🏁 Script executed:
#!/bin/bash
# Verify that CreateProject and CreateOrganizationRole exist with their permissions
echo "=== Searching for CreateProject and CreateOrganizationRole ==="
rg -n "CreateProject|CreateRole" pkg/server/connect_interceptors/authorization.go | head -20Repository: raystack/frontier
Length of output: 1036
🏁 Script executed:
#!/bin/bash
# Get the full context for CreateProject, CreateRole, and other Create* endpoints
echo "=== CreateProject permission ==="
sed -n '464,467p' pkg/server/connect_interceptors/authorization.go
echo ""
echo "=== CreateRole permission ==="
sed -n '1039,1042p' pkg/server/connect_interceptors/authorization.go
echo ""
echo "=== CreateGroup permission ==="
sed -n '555,558p' pkg/server/connect_interceptors/authorization.go
echo ""
echo "=== CreateOrganizationInvitation permission ==="
sed -n '494,497p' pkg/server/connect_interceptors/authorization.go
echo ""
echo "=== CreateServiceUser permission ==="
sed -n '410,413p' pkg/server/connect_interceptors/authorization.goRepository: raystack/frontier
Length of output: 2144
🏁 Script executed:
#!/bin/bash
# Use rg to find and display the exact lines with context
echo "=== CreateProject ==="
rg -n "CreateProject.*func\(ctx" pkg/server/connect_interceptors/authorization.go -A 2 | head -5
echo ""
echo "=== CreateGroup ==="
rg -n "CreateGroup.*func\(ctx" pkg/server/connect_interceptors/authorization.go -A 2 | head -5
echo ""
echo "=== CreateOrganizationInvitation ==="
rg -n "CreateOrganizationInvitation.*func\(ctx" pkg/server/connect_interceptors/authorization.go -A 2 | head -5
echo ""
echo "=== CreateServiceUser ==="
rg -n "CreateServiceUser.*func\(ctx" pkg/server/connect_interceptors/authorization.go -A 2 | head -5Repository: raystack/frontier
Length of output: 2335
GetPermission (read) gates a token-creation write operation — inconsistent with all other Create* endpoints.
Every other Create* endpoint in authorizationValidationMap uses a write-level or specialized-manage permission as the authorization gate:
| Endpoint | Permission |
|---|---|
CreateServiceUser |
ServiceUserManagePermission |
CreateOrganizationInvitation |
InvitationCreatePermission |
CreateGroup |
GroupCreatePermission |
CreateProject |
ProjectCreatePermission |
CreateCurrentUserPersonalToken uses only schema.GetPermission ("get"), meaning any read-only org member can create a PAT scoped to that org. Since PATs carry Roles and ProjectIds fields that can embed sensitive permissions, this allows a low-privileged member to create tokens with elevated role claims — a privilege escalation risk.
Unless the intent is explicitly to allow any org member to self-issue PATs, the permission should be raised — at minimum to schema.UpdatePermission, or ideally a dedicated PAT*ManagePermission constant.
Suggested fix
- return handler.IsAuthorized(ctx, relation.Object{Namespace: schema.OrganizationNamespace, ID: pbreq.Msg.GetOrgId()}, schema.GetPermission, req)
+ return handler.IsAuthorized(ctx, relation.Object{Namespace: schema.OrganizationNamespace, ID: pbreq.Msg.GetOrgId()}, schema.UpdatePermission, req)📝 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.
| frontierv1beta1connect.FrontierServiceCreateCurrentUserPersonalTokenProcedure: func(ctx context.Context, handler *v1beta1connect.ConnectHandler, req connect.AnyRequest) error { | |
| pbreq := req.(*connect.Request[frontierv1beta1.CreateCurrentUserPersonalTokenRequest]) | |
| return handler.IsAuthorized(ctx, relation.Object{Namespace: schema.OrganizationNamespace, ID: pbreq.Msg.GetOrgId()}, schema.GetPermission, req) | |
| }, | |
| frontierv1beta1connect.FrontierServiceCreateCurrentUserPersonalTokenProcedure: func(ctx context.Context, handler *v1beta1connect.ConnectHandler, req connect.AnyRequest) error { | |
| pbreq := req.(*connect.Request[frontierv1beta1.CreateCurrentUserPersonalTokenRequest]) | |
| return handler.IsAuthorized(ctx, relation.Object{Namespace: schema.OrganizationNamespace, ID: pbreq.Msg.GetOrgId()}, schema.UpdatePermission, req) | |
| }, |
Pull Request Test Coverage Report for Build 22259293803Details
💛 - Coveralls |
| { | ||
| name: "should generate unique tokens on each call", | ||
| req: userpat.CreateRequest{ | ||
| UserID: "user-1", | ||
| OrgID: "org-1", | ||
| Title: "my-token", | ||
| Roles: []string{"role-1"}, | ||
| ExpiresAt: time.Now().Add(24 * time.Hour), | ||
| }, | ||
| wantErr: false, | ||
| setup: func() *userpat.Service { | ||
| repo := mocks.NewRepository(t) | ||
| repo.EXPECT().CountActive(mock.Anything, "user-1", "org-1"). | ||
| Return(int64(0), nil) | ||
| repo.EXPECT().Create(mock.Anything, mock.AnythingOfType("userpat.PersonalAccessToken")). | ||
| Return(userpat.PersonalAccessToken{ID: "pat-1", OrgID: "org-1"}, nil) | ||
| orgSvc, auditRepo := newSuccessMocks(t) | ||
| return userpat.NewService(log.NewNoop(), repo, defaultConfig, orgSvc, auditRepo) | ||
| }, | ||
| }, |
There was a problem hiding this comment.
"should generate unique tokens on each call" makes only one Create call — test name is misleading
The table case sets up the repo mock with a single CountActive + Create expectation and has no validateFunc, so it just verifies that one call succeeds without errors. Uniqueness across two calls is already tested properly in TestService_Create_UniqueTokens (line 398). This case either duplicates existing "happy-path creation" coverage or the test author intended it to assert uniqueness but forgot the second call.
Consider removing this case (redundant with other happy-path cases) or renaming it to reflect what it actually exercises.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/api/v1beta1connect/user_pat.go (2)
87-91: Log metadata serialisation failure intransformPATToPBinstead of silently dropping it.If
ToStructPB()fails, the response silently returns nil metadata. A caller receiving a token with no metadata has no way to distinguish "empty metadata" from "serialisation error". At minimum, log atwarnso it surfaces during debugging.🛠️ Proposed fix
if pat.Metadata != nil { metaPB, err := pat.Metadata.ToStructPB() if err == nil { pbPAT.Metadata = metaPB + } else { + // log and continue: the token is valid, only metadata rendering failed + // h.logger.Warn("failed to convert PAT metadata to protobuf", zap.Error(err)) } }
32-46: ReuseexpiresAtinstead of calling.AsTime()twice.
request.Msg.GetExpiresAt().AsTime()is invoked at line 32 and again at line 46 inside theCreateRequestliteral. Reuse the already-computed value.🛠️ Proposed fix
expiresAt := request.Msg.GetExpiresAt().AsTime() if !expiresAt.After(time.Now()) { return nil, connect.NewError(connect.CodeInvalidArgument, userpat.ErrExpiryInPast) } if expiresAt.After(time.Now().Add(h.patConfig.MaxExpiry())) { return nil, connect.NewError(connect.CodeInvalidArgument, userpat.ErrExpiryExceeded) } created, tokenValue, err := h.userPATService.Create(ctx, userpat.CreateRequest{ ... - ExpiresAt: request.Msg.GetExpiresAt().AsTime(), + ExpiresAt: expiresAt, ... })
| if err != nil { | ||
| errorLogger.LogServiceError(ctx, request, "CreateCurrentUserPersonalToken", err, | ||
| zap.String("user_id", principal.User.ID), | ||
| zap.String("org_id", request.Msg.GetOrgId())) | ||
|
|
||
| switch { | ||
| case errors.Is(err, userpat.ErrDisabled): | ||
| return nil, connect.NewError(connect.CodeFailedPrecondition, err) | ||
| case errors.Is(err, userpat.ErrConflict): | ||
| return nil, connect.NewError(connect.CodeAlreadyExists, err) | ||
| case errors.Is(err, userpat.ErrLimitExceeded): | ||
| return nil, connect.NewError(connect.CodeResourceExhausted, err) | ||
| default: | ||
| return nil, connect.NewError(connect.CodeInternal, ErrInternalServerError) | ||
| } | ||
| } |
There was a problem hiding this comment.
Move LogServiceError to the default case — expected errors pollute error logs.
LogServiceError fires unconditionally for all errors, so ErrDisabled (feature flag off), ErrConflict (duplicate title), and ErrLimitExceeded (quota reached) — all normal user-facing outcomes — generate error-level log entries. This degrades log signal and can trigger false alerts.
🛠️ Proposed fix
- errorLogger.LogServiceError(ctx, request, "CreateCurrentUserPersonalToken", err,
- zap.String("user_id", principal.User.ID),
- zap.String("org_id", request.Msg.GetOrgId()))
-
switch {
case errors.Is(err, userpat.ErrDisabled):
return nil, connect.NewError(connect.CodeFailedPrecondition, err)
case errors.Is(err, userpat.ErrConflict):
return nil, connect.NewError(connect.CodeAlreadyExists, err)
case errors.Is(err, userpat.ErrLimitExceeded):
return nil, connect.NewError(connect.CodeResourceExhausted, err)
default:
+ errorLogger.LogServiceError(ctx, request, "CreateCurrentUserPersonalToken", err,
+ zap.String("user_id", principal.User.ID),
+ zap.String("org_id", request.Msg.GetOrgId()))
return nil, connect.NewError(connect.CodeInternal, ErrInternalServerError)
}| org_id UUID NOT NULL REFERENCES organizations(id) ON DELETE CASCADE, | ||
| title TEXT NOT NULL, | ||
| secret_hash TEXT NOT NULL UNIQUE, | ||
| metadata JSONB DEFAULT '{}', |
There was a problem hiding this comment.
Add NOT NULL to metadata column.
DEFAULT '{}' only applies when the column is omitted from an INSERT. If the Go driver sends an explicit NULL (which it will for a nil map[string]any), the default is bypassed and the row stores NULL. This breaks any downstream code that relies on metadata always being a valid JSONB object (e.g., transformPATToPB in the handler, which guards on pat.Metadata != nil).
🛠️ Proposed fix
- metadata JSONB DEFAULT '{}',
+ metadata JSONB NOT NULL DEFAULT '{}',📝 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.
| metadata JSONB DEFAULT '{}', | |
| metadata JSONB NOT NULL DEFAULT '{}', |
2 participants
Title: feat: add CreateCurrentUserPersonalToken RPC
Summary
CreateCurrentUserPersonalTokenRPC — the first endpoint for Personal Access Tokens (PATs), allowing users to create org-scoped tokens with configurable prefix, expiry, and metadata.wiring.