feat: CreateCurrentUserPAT RPC implementation#1401
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Personal Access Token (PAT) system: config, domain types, service logic, persistence (Postgres + migrations), API endpoints (Connect handlers, protobufs, validation), authorization and bootstrap changes, audit integration, and extensive tests and mocks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
Pull Request Test Coverage Report for Build 22760765153Warning: 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 |
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, ... })
internal/store/postgres/migrations/20260218100000_create_user_tokens.up.sql
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
core/userpat/config.go (1)
14-19:⚠️ Potential issue | 🟡 MinorAvoid silent fallback for invalid
MaxLifetime.Line 15–18 still masks bad duration config by defaulting to 365 days, which can unintentionally alter PAT lifetime policy.
#!/bin/bash # Verify whether PAT config has explicit validation and where MaxExpiry is consumed. rg -nP --type go 'func\s+\(c\s+Config\)\s+Validate\s*\(' rg -nP --type go '\bMaxExpiry\s*\(' rg -nP --type go '\bMaxLifetime\b|\bDefaultLifetime\b'internal/api/v1beta1connect/user_pat.go (1)
44-59:⚠️ Potential issue | 🟡 MinorMove
LogServiceErrorto thedefaultcase — expected errors pollute error logs.
LogServiceErrorfires unconditionally for all errors, soErrDisabled,ErrConflict, andErrLimitExceeded— 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, "CreateCurrentUserPAT", 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, "CreateCurrentUserPAT", err, + zap.String("user_id", principal.User.ID), + zap.String("org_id", request.Msg.GetOrgId())) return nil, connect.NewError(connect.CodeInternal, ErrInternalServerError) }internal/store/postgres/userpat_repository_test.go (1)
10-10:⚠️ Potential issue | 🟡 MinorUpdate import to use dockertest v3.
The import path
github.com/ory/dockertestreferences the old v2 version, but the project uses v3.10.0 (github.com/ory/dockertest/v3). This was flagged in a previous review and should be updated.- "github.com/ory/dockertest" + "github.com/ory/dockertest/v3"
🧹 Nitpick comments (6)
core/userpat/service_test.go (1)
35-36: Make audit expectations strict for successful create paths.Line 35 marks
AuditRecordRepository.Createas optional. That allows success tests to pass even if audit logging is skipped. For success-path helpers, require at least one audit call.🛠️ Proposed fix
auditRepo := mocks.NewAuditRecordRepository(t) auditRepo.On("Create", mock.Anything, mock.Anything). - Return(models.AuditRecord{}, nil).Maybe() + Return(models.AuditRecord{}, nil)internal/api/v1beta1connect/user_pat.go (2)
31-33: Potential nil pointer dereference onGetExpiresAt().If
request.Msg.GetExpiresAt()returnsnil, calling.AsTime()on it will return the zero time (0001-01-01), which then gets passed toValidateExpiry. While this would fail the "expiry in future" check, it's cleaner to validate presence explicitly before conversion to provide a more specific error message.🔧 Optional: explicit nil check for clearer error
+ if request.Msg.GetExpiresAt() == nil { + return nil, connect.NewError(connect.CodeInvalidArgument, errors.New("expires_at is required")) + } + if err := h.userPATService.ValidateExpiry(request.Msg.GetExpiresAt().AsTime()); err != nil { return nil, connect.NewError(connect.CodeInvalidArgument, err) }
82-87: Metadata conversion error is silently swallowed.If
pat.Metadata.ToStructPB()fails, the error is ignored and metadata is simply omitted from the response. Consider logging a warning so operators can diagnose conversion issues.🔧 Optional: log metadata conversion failure
if pat.Metadata != nil { metaPB, err := pat.Metadata.ToStructPB() - if err == nil { + if err != nil { + // Consider logging: metadata conversion failed + } else { pbPAT.Metadata = metaPB } }internal/store/postgres/userpat_repository.go (1)
28-28: Consider using pointer receiver for consistency.The
Createmethod uses a value receiver(r UserPATRepository)while many Go codebases prefer pointer receivers for struct methods. This is a minor stylistic point; the current implementation works correctly since the struct only holds a pointer field.♻️ Optional: use pointer receiver for consistency
-func (r UserPATRepository) Create(ctx context.Context, pat userpat.PAT) (userpat.PAT, error) { +func (r *UserPATRepository) Create(ctx context.Context, pat userpat.PAT) (userpat.PAT, error) {Apply the same change to
CountActiveon line 66.internal/store/postgres/migrations/20260218100000_create_user_pats.up.sql (1)
15-24: Consider a composite index forCountActivequery optimization.The
CountActivequery filters by(user_id, org_id, deleted_at IS NULL, expires_at > now). The currentidx_user_pats_expires_activeindex only coversexpires_at, which may not be optimal.The existing
idx_user_pats_user_org_activeon(user_id, org_id) WHERE deleted_at IS NULLshould help, but addingexpires_atto it could further optimize the count query.💡 Optional: enhanced composite index
-- Replace idx_user_pats_user_org_active with a version that includes expires_at CREATE INDEX idx_user_pats_user_org_expires_active ON user_pats(user_id, org_id, expires_at) WHERE deleted_at IS NULL;This would allow the
CountActivequery to be fully covered by a single index scan.core/userpat/service.go (1)
56-66: Minor: Twotime.Now()calls may cause edge-case inconsistency.The validation at lines 59 and 62 each call
time.Now()separately. In an extremely rare edge case where the clock ticks between calls, a time that's exactly at the boundary could pass one check but fail the other. Consider capturingtime.Now()once.🔧 Optional: single time capture
func (s *Service) ValidateExpiry(expiresAt time.Time) error { - if !expiresAt.After(time.Now()) { + now := time.Now() + if !expiresAt.After(now) { return ErrExpiryInPast } - if expiresAt.After(time.Now().Add(s.config.MaxExpiry())) { + if expiresAt.After(now.Add(s.config.MaxExpiry())) { return ErrExpiryExceeded } return nil }
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
proto/v1beta1/frontier.pb.gois excluded by!**/*.pb.goproto/v1beta1/frontier_grpc.pb.gois excluded by!**/*.pb.goproto/v1beta1/models.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (22)
Makefilecore/userpat/config.gocore/userpat/mocks/repository.gocore/userpat/service.gocore/userpat/service_test.gocore/userpat/userpat.gointernal/api/v1beta1connect/interfaces.gointernal/api/v1beta1connect/mocks/user_pat_service.gointernal/api/v1beta1connect/user_pat.gointernal/api/v1beta1connect/user_pat_test.gointernal/api/v1beta1connect/v1beta1connect.gointernal/store/postgres/migrations/20260218100000_create_user_pats.down.sqlinternal/store/postgres/migrations/20260218100000_create_user_pats.up.sqlinternal/store/postgres/postgres.gointernal/store/postgres/userpat.gointernal/store/postgres/userpat_repository.gointernal/store/postgres/userpat_repository_test.gopkg/server/connect_interceptors/authorization.goproto/apidocs.swagger.yamlproto/v1beta1/frontier.pb.validate.goproto/v1beta1/frontierv1beta1connect/frontier.connect.goproto/v1beta1/models.pb.validate.go
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/api/v1beta1connect/interfaces.go
- pkg/server/connect_interceptors/authorization.go
- Makefile
- internal/api/v1beta1connect/v1beta1connect.go
- internal/store/postgres/postgres.go
- internal/api/v1beta1connect/user_pat_test.go
- core/userpat/mocks/repository.go
AmanGIT07
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
core/userpat/service_test.go (1)
203-204: Hardcoded expiry date will cause test flakiness after June 2026The test uses a hardcoded
time.Date(2026, 6, 1, ...)which will become a past date after June 2026. While a past review flagged this, the code still contains the hardcoded value. Consider using a relative timestamp:stableExpiry := time.Now().UTC().Add(24 * time.Hour).Truncate(time.Second)This applies to lines 203, 228-229, and 238-239.
internal/bootstrap/service.go (1)
270-304: Consider performance for large deployments.The migration iterates all roles and all their permissions on every bootstrap. For deployments with many roles/permissions, this could impact startup time. The SpiceDB Touch semantics ensure correctness (idempotent creates), and Delete handles non-existent tuples gracefully.
This is acceptable for the initial implementation, but consider adding logging or metrics if bootstrap times become a concern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2f2a99b0-2b76-4b4c-891b-73ef6d5d3e7b
📒 Files selected for processing (25)
cmd/serve.gocore/authenticate/mocks/authenticator_func.gocore/policy/policy.gocore/policy/service.gocore/policy/service_test.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.zedinternal/store/postgres/migrations/20260226100000_add_grant_relation_to_policies.down.sqlinternal/store/postgres/migrations/20260226100000_add_grant_relation_to_policies.up.sqlinternal/store/postgres/policy.gointernal/store/postgres/policy_repository.go
# Conflicts: # Makefile # pkg/server/config.go # proto/apidocs.swagger.yaml # proto/v1beta1/frontier.pb.go # proto/v1beta1/frontier_grpc.pb.go # proto/v1beta1/models.pb.go
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
AmanGIT07
force-pushed
the
feat/create-pat-rpc
branch
from
7e05aea to
28ef3aa
Compare
3 participants
Title: feat: add CreateCurrentUserPersonalToken RPC
Summary
CreateCurrentUserPATRPC — the first endpoint for Personal Access Tokens (PATs), allowing users to create org-scoped tokens with configurable prefix, expiry, and metadata.wiring.