feat: implement RegenerateCurrentUserPAT RPC#1468
feat: implement RegenerateCurrentUserPAT RPC#1468AmanGIT07 wants to merge 1 commit intofeat/update-current-user-patfrom
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 ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 Coding Plan
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 23337397879Details
💛 - 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: 3
🧹 Nitpick comments (3)
core/userpat/service_test.go (1)
2148-2371: Strengthen regenerate tests with return-value and boundary assertions.
Current success cases only verifyerr == nil; please also assert returned PAT fields/token and add a case where existing expiry is exactlynowto lock the active-limit boundary behavior.internal/api/v1beta1connect/user_pat_test.go (1)
1169-1252: Add explicit coverage forErrExpiryExceededmapping.
RegenerateCurrentUserPATmaps both expiry errors toCodeInvalidArgument; this suite currently asserts onlyErrExpiryInPast.core/userpat/service.go (1)
167-183: Consider documenting or hardening the count-then-regenerate race window.
CountActive+Regenerateis non-atomic, so concurrent revivals can exceedMaxPerUserPerOrg.Createalready documents a similar TOCTOU risk; this path should get equivalent treatment (comment or atomic DB guard strategy).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dfed0060-a853-4392-ac7f-108e4a9ee92b
⛔ Files ignored due to path filters (1)
proto/v1beta1/frontier.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (14)
Makefilecore/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/store/postgres/userpat_repository.gopkg/auditrecord/consts.gopkg/server/connect_interceptors/authorization.goproto/v1beta1/frontier.pb.validate.goproto/v1beta1/frontierv1beta1connect/frontier.connect.go
| // If PAT is expired, regenerating revives it — check active count limit. | ||
| if pat.ExpiresAt.Before(time.Now()) { | ||
| count, err := s.repo.CountActive(ctx, pat.UserID, pat.OrgID) | ||
| if err != nil { | ||
| return patmodels.PAT{}, "", fmt.Errorf("counting active PATs: %w", err) | ||
| } | ||
| if count >= s.config.MaxPerUserPerOrg { | ||
| return patmodels.PAT{}, "", paterrors.ErrLimitExceeded | ||
| } | ||
| } |
There was a problem hiding this comment.
Treat expiry-at-now as expired when enforcing active-PAT limit.
CountActive considers active as expires_at > now, but Line 166 uses Before(now). A PAT expiring exactly now bypasses the limit check incorrectly.
🐛 Proposed fix
- // If PAT is expired, regenerating revives it — check active count limit.
- if pat.ExpiresAt.Before(time.Now()) {
+ // If PAT is expired (or expires exactly now), regenerating revives it — check active count limit.
+ now := time.Now()
+ if !pat.ExpiresAt.After(now) {
count, err := s.repo.CountActive(ctx, pat.UserID, pat.OrgID)
if err != nil {
return patmodels.PAT{}, "", fmt.Errorf("counting active PATs: %w", err)
}| return nil, connect.NewError(connect.CodeInvalidArgument, err) | ||
| } | ||
|
|
||
| regenerated, patValue, err := h.userPATService.Regenerate(ctx, principal.User.ID, request.Msg.GetId(), request.Msg.GetExpiresAt().AsTime()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for RegenerateCurrentUserPATRequest proto definition and validation rules
# Find the proto definition for RegenerateCurrentUserPATRequest
rg -n -A 10 'message RegenerateCurrentUserPATRequest' --type proto
# Also check in .pb.go or .pb.validate.go files
rg -n -A 15 'RegenerateCurrentUserPATRequest' --glob '*.pb.go' --glob '*.pb.validate.go' | head -80Repository: raystack/frontier
Length of output: 6643
🏁 Script executed:
# Find .proto source files
fd -e proto . | head -20Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Look for the proto definition of RegenerateCurrentUserPATRequest with validation
rg 'RegenerateCurrentUserPATRequest' --type-add 'proto:*.proto' -lRepository: raystack/frontier
Length of output: 269
🏁 Script executed:
# Check the actual code at line 241 and surrounding context
cat -n internal/api/v1beta1connect/user_pat.go | sed -n '230,250p'Repository: raystack/frontier
Length of output: 974
🏁 Script executed:
# Search for validation patterns in the codebase
rg 'Validate\(\)' internal/api/v1beta1connect/user_pat.goRepository: raystack/frontier
Length of output: 348
🏁 Script executed:
# Check the validation implementation for RegenerateCurrentUserPATRequest
rg -A 20 'func.*RegenerateCurrentUserPATRequest.*Validate' proto/v1beta1/frontier.pb.validate.goRepository: raystack/frontier
Length of output: 903
🏁 Script executed:
# Also search for any validation rules on expires_at field
rg -B 5 -A 5 'expires_at' proto/v1beta1/frontier.pb.validate.go | grep -A 10 -B 10 'RegenerateCurrentUserPATRequest'Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Get more context of the validate function to see expires_at handling
rg -A 50 'func.*RegenerateCurrentUserPATRequest.*validate\(all bool\)' proto/v1beta1/frontier.pb.validate.goRepository: raystack/frontier
Length of output: 1402
🏁 Script executed:
# Find the .proto source file
fd -e proto -name 'frontier.proto' -o -name '*.proto' | head -10Repository: raystack/frontier
Length of output: 291
🏁 Script executed:
# Search for the proto source definition of RegenerateCurrentUserPATRequest
rg -A 5 'message RegenerateCurrentUserPATRequest' 2>/dev/null || echo "Searching in proto directory..."
find . -name '*.proto' -exec grep -l 'RegenerateCurrentUserPATRequest' {} \;Repository: raystack/frontier
Length of output: 92
🏁 Script executed:
# Search for proto files more broadly
find . -type f -name '*.proto' | head -20Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Also check if there's a proto directory
ls -la proto/ 2>/dev/null | head -20Repository: raystack/frontier
Length of output: 228
🏁 Script executed:
# Look for the source proto definition using grep
grep -r 'message RegenerateCurrentUserPATRequest' . --include='*.proto' 2>/dev/nullRepository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Check what's in proto/v1beta1
ls -la proto/v1beta1/Repository: raystack/frontier
Length of output: 674
🏁 Script executed:
# Search for proto validation rules more broadly
rg 'message RegenerateCurrentUserPATRequest' --type-add 'proto:*.proto' -A 10Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Check if there's any reference to proto definition location in git or build files
find . -name 'buf.yaml' -o -name 'buf.lock' -o -name '*.md' | grep -i buf | head -5Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Check the service implementation to see how Regenerate handles nil expires_at
rg -A 20 'func.*Regenerate' internal/api/v1beta1connect/user_pat.go | head -50Repository: raystack/frontier
Length of output: 961
🏁 Script executed:
# Look at the PAT service implementation
find . -path '*/service/*' -name '*pat*' -type fRepository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Search for the userPATService.Regenerate implementation
rg -l 'Regenerate' --type go | grep -i service | head -10Repository: raystack/frontier
Length of output: 167
🏁 Script executed:
# Check the Regenerate method signature in the service
rg -A 30 'func.*Regenerate' core/userpat/service.go | head -80Repository: raystack/frontier
Length of output: 973
🏁 Script executed:
# Also check if there are any nil checks on expires_at in the service
rg 'expiresAt\|expires_at' core/userpat/service.go -A 2 -B 2 | head -50Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Look for the .proto source files that might be in a different location
find . -name '*.proto' 2>/dev/null | head -20Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Check ValidateExpiry to see what it expects
rg -B 5 -A 15 'ValidateExpiry' core/userpat/service.goRepository: raystack/frontier
Length of output: 1386
🏁 Script executed:
# Look for any nil checks on expires_at in the handler or service layer
rg 'GetExpiresAt' internal/api/v1beta1connect/user_pat.go core/userpat/service.go -B 2 -A 2Repository: raystack/frontier
Length of output: 1275
🏁 Script executed:
# Search for proto source - might be in a vendor or external location
git ls-files | grep -i 'frontier.*proto' | head -20Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Check if there's actually a nil check for expires_at anywhere
rg 'GetExpiresAt.*nil\|expires_at.*nil' --type goRepository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Look for the proto validation rules in more detail - search for field constraints
rg '(required|rules)' proto/v1beta1/frontier.pb.validate.go | grep -i 'expires\|regenerate' -B 2 -A 2Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Check the proto source format by looking at generated code metadata
head -50 proto/v1beta1/frontier.pb.go | grep -i 'source\|proto'Repository: raystack/frontier
Length of output: 1754
🏁 Script executed:
# Search for the actual proto source file
find . -name 'frontier.proto' 2>/dev/nullRepository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Check git to see if the proto file is tracked
git ls-files '*.proto'Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Look in a typical location for proto sources
find . -path '*/proto/**' -name '*.proto' 2>/dev/null | head -10Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Check if there's any nil handling for expires_at in other similar methods
rg -B 5 -A 5 'GetExpiresAt.*AsTime\|expires_at' internal/api/v1beta1connect/user_pat.goRepository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Check all methods in user_pat.go to see the pattern
rg 'func.*ConnectHandler.*\(' internal/api/v1beta1connect/user_pat.go -A 30 | grep -E 'func|GetExpiresAt|Validate' | head -50Repository: raystack/frontier
Length of output: 2176
🏁 Script executed:
# Get the complete RegenerateCurrentUserPAT method to check all calls
sed -n '227,260p' internal/api/v1beta1connect/user_pat.goRepository: raystack/frontier
Length of output: 1413
🏁 Script executed:
# Compare with CreateCurrentUserPAT to see if there's a difference
rg -A 25 'func.*CreateCurrentUserPAT' internal/api/v1beta1connect/user_pat.go | head -40Repository: raystack/frontier
Length of output: 1124
🏁 Script executed:
# Check the service.Regenerate signature again and how it handles zero time
rg -B 2 -A 40 'func.*Regenerate' core/userpat/service.go | head -50Repository: raystack/frontier
Length of output: 1551
🏁 Script executed:
# Double-check: Does ValidateExpiry handle zero/nil time.Time?
rg -B 2 -A 10 'func.*ValidateExpiry' core/userpat/service.goRepository: raystack/frontier
Length of output: 513
🏁 Script executed:
# Check what time.Time(AsTime()) returns for nil Timestamp
python3 << 'EOF'
import time
from google.protobuf import timestamp_pb2
# Simulate what happens when GetExpiresAt returns nil
# In Go, nil.AsTime() would panic, but let's check zero time behavior
ts = timestamp_pb2.Timestamp() # default/empty
print(f"Empty timestamp: {ts}")
print(f"AsTime() would return: 1970-01-01 (zero time)")
EOFRepository: raystack/frontier
Length of output: 124
🏁 Script executed:
# Check if there's any wrapper or helper for safe AsTime conversion
rg 'AsTime\|GetExpiresAt' internal/api/v1beta1connect/user_pat.go -B 1 -A 1Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Check if expires_at is treated as required anywhere else
rg 'expires_at.*required\|expires_at.*\(required\)' --type goRepository: raystack/frontier
Length of output: 43
🏁 Script executed:
# See if the proto source might be in vendor or generated from somewhere else
find . -name 'buf.lock' -exec cat {} \;Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Check how other services handle optional timestamp fields
rg 'GetExpiresAt\|expires_at' --type go -B 2 -A 2 | grep -E 'nil|AsTime|==|!=' | head -20Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Final check: Look at the exact line again and what happens after validation
sed -n '235,245p' internal/api/v1beta1connect/user_pat.goRepository: raystack/frontier
Length of output: 507
🏁 Script executed:
# Check if proto field is optional or required by looking at field definition again
grep -A 5 'type RegenerateCurrentUserPATRequest struct' proto/v1beta1/frontier.pb.goRepository: raystack/frontier
Length of output: 313
Add nil check before calling .AsTime() on expires_at.
The proto validation at line 237 does not enforce expires_at as a required field. Since RegenerateCurrentUserPATRequest.expires_at is optional, GetExpiresAt() can return nil, causing request.Msg.GetExpiresAt().AsTime() at line 241 to panic. Either add a nil check before calling .AsTime(), or update the proto definition to mark expires_at as required.
| err = checkPostgresError(err) | ||
| if errors.Is(err, sql.ErrNoRows) { | ||
| return models.PAT{}, paterrors.ErrNotFound | ||
| } | ||
| return models.PAT{}, fmt.Errorf("%w: %w", dbErr, err) |
There was a problem hiding this comment.
Handle duplicate-key conflicts consistently in Regenerate.
secret_hash is unique; on collision this path currently returns a generic DB error instead of paterrors.ErrConflict, unlike Create/Update.
♻️ Proposed fix
if err = r.dbc.WithTimeout(ctx, TABLE_USER_PATS, "Regenerate", func(ctx context.Context) error {
return r.dbc.QueryRowxContext(ctx, query, params...).StructScan(&model)
}); err != nil {
err = checkPostgresError(err)
if errors.Is(err, sql.ErrNoRows) {
return models.PAT{}, paterrors.ErrNotFound
}
+ if errors.Is(err, ErrDuplicateKey) {
+ return models.PAT{}, paterrors.ErrConflict
+ }
return models.PAT{}, fmt.Errorf("%w: %w", dbErr, err)
}📝 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.
| err = checkPostgresError(err) | |
| if errors.Is(err, sql.ErrNoRows) { | |
| return models.PAT{}, paterrors.ErrNotFound | |
| } | |
| return models.PAT{}, fmt.Errorf("%w: %w", dbErr, err) | |
| err = checkPostgresError(err) | |
| if errors.Is(err, sql.ErrNoRows) { | |
| return models.PAT{}, paterrors.ErrNotFound | |
| } | |
| if errors.Is(err, ErrDuplicateKey) { | |
| return models.PAT{}, paterrors.ErrConflict | |
| } | |
| return models.PAT{}, fmt.Errorf("%w: %w", dbErr, err) |
2 participants
Description:
Summary
RegenerateCurrentUserPATRPC that creates a new secret and updates expiry for an existing PATChanges
RegenerateCurrentUserPATRPC, request (UUID-validated id, required expires_at), response with PAT (includes new plaintext token)Regeneratemethod — updates secret_hash + expires_at viaRETURNING, handles ErrNotFoundRegeneratemethod — ownership check, expiry validation, limit check for expired PATs, new secret generation, audit recordPATRegeneratedEventwith expires_at and old_expires_at in metadataDesign decisions
GetByIDdoesn't filter by expiry (onlydeleted_at), so expired PATs can be regenerated.Manual test verification
pat.regeneratedevent, including old and new expiry date