Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ 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 (1)
📝 WalkthroughWalkthroughThis PR adds Personal Access Token (PAT) update functionality across the stack: repository interface and PostgreSQL implementation, service layer with validation and policy management, Connect RPC handler with error mapping, proto message validation, interface mocks, and audit event tracking. The Makefile's PROTON_COMMIT hash is updated to enable proto regeneration. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
📝 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 23331104157Details
💛 - 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: 2
🧹 Nitpick comments (3)
internal/api/v1beta1connect/user_pat_test.go (1)
1066-1110: This happy-path test doesn't exercise the fields the RPC is supposed to update.
UserPATService.Updateis matched withmock.Anything, the request omitsProjectIds/Metadata, and the assertions only checkId/Title. A handler regression that dropsUserID,ProjectIds, orMetadatawould still pass. Please match the outboundmodels.PATand assert the response carries the updated scope/metadata as well.core/userpat/service_test.go (1)
2061-2093: The happy path never proves stale policies are revoked.This success case starts from an empty old scope, so a regression that appends new policies without deleting the old ones would still pass. Seed
policySvc.Listwith at least one existing policy and assert the matchingDeletecalls beforeCreate.core/userpat/service.go (1)
233-239: Prefer auditing the effective scope fromupdated.
role_idsandproject_idshere are taken fromtoUpdate, while the effective scope is already computed intoupdatedon Lines 184-186. Usingupdated.RoleIDs/updated.ProjectIDskeeps the audit payload aligned with whatever normalization or scope pruning happened during policy replacement.Suggested change
func (s *Service) auditUpdate(ctx context.Context, updated patmodels.PAT, toUpdate patmodels.PAT, oldTitle string, oldRoleIDs []string, oldProjectIDs []string) { if err := s.createAuditRecord(ctx, pkgAuditRecord.PATUpdatedEvent, updated, time.Now().UTC(), map[string]any{ - "role_ids": toUpdate.RoleIDs, - "project_ids": toUpdate.ProjectIDs, + "role_ids": updated.RoleIDs, + "project_ids": updated.ProjectIDs, "old_title": oldTitle, "old_role_ids": oldRoleIDs, "old_project_ids": oldProjectIDs, }); err != nil {
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e8928912-f18b-4e71-9aaa-6c9f876377b5
⛔ 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
| repo.EXPECT().Update(mock.Anything, mock.Anything). | ||
| Return(updatedPAT, nil) | ||
| // deletePolicies call | ||
| policySvc.EXPECT().List(mock.Anything, policy.Filter{ | ||
| PrincipalID: "pat-1", | ||
| PrincipalType: schema.PATPrincipal, | ||
| }).Return(nil, errors.New("spicedb down")).Once() | ||
| return userpat.NewService(log.NewNoop(), repo, defaultConfig, nil, roleSvc, policySvc, nil) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Update flow and helper order ---'
rg -n -C5 'func \(.*Service\) Update|captureOldScope|replacePolicies|repo\.Update\(' core/userpat
echo
echo '--- Any compensating rollback / fail-closed logic ---'
rg -n -C3 'rollback|restore|reconcile|ErrDisabled' core/userpat/service.goRepository: raystack/frontier
Length of output: 7677
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- replacePolicies and deletePolicies implementation ---'
sed -n '214,260p' core/userpat/service.go
echo
echo '--- deletePolicies method ---'
rg -n -A20 'func \(.*Service\) deletePolicies' core/userpat/service.go
echo
echo '--- Error handling in Update after repo.Update ---'
sed -n '171,195p' core/userpat/service.goRepository: raystack/frontier
Length of output: 3881
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Test at lines 2024-2031 (failure setup context) ---'
sed -n '2010,2045p' core/userpat/service_test.go
echo
echo '--- Test success path at 2061-2093 ---'
sed -n '2050,2100p' core/userpat/service_test.go
echo
echo '--- Full test case name around 2024-2031 ---'
sed -n '1980,2035p' core/userpat/service_test.go | grep -A 50 'func Test'Repository: raystack/frontier
Length of output: 3650
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Test case name for lines 2024-2031 ---'
sed -n '1968,1980p' core/userpat/service_test.go
echo
echo '--- Verify if success test includes Delete assertion ---'
sed -n '2084,2115p' core/userpat/service_test.go
echo
echo '--- Check what the success test is checking ---'
grep -n 'should update successfully' core/userpat/service_test.go -A 50 | head -60Repository: raystack/frontier
Length of output: 4557
repo.Update commits to Postgres before replacePolicies updates SpiceDB, creating a split-brain risk if the latter fails.
If deletePolicies or createPolicies fails (e.g., SpiceDB unavailable), the Postgres PAT row is already updated with new title/metadata while the authz state is stale or partially changed. The error is returned, but there is no rollback or compensating action. This leaves the PAT and its permissions in an inconsistent state.
Additionally, the success test (lines 2061–2093) does not exercise the old-policy revocation path because the old scope is empty. A test with pre-existing policies being replaced would better validate this critical path.
| updated, err := s.repo.Update(ctx, patmodels.PAT{ | ||
| ID: toUpdate.ID, | ||
| Title: toUpdate.Title, | ||
| Metadata: toUpdate.Metadata, | ||
| }) | ||
| if err != nil { | ||
| return patmodels.PAT{}, fmt.Errorf("updating PAT: %w", err) | ||
| } |
There was a problem hiding this comment.
Update can return an error after persisting a partial state.
The read on Line 222 only proves the PAT existed at that instant; Delete can still soft-delete it before createPolicies runs, leaving orphaned policies behind. Also, because Lines 171-175 commit title/metadata before Line 217 removes the old bindings, any later delete/create failure returns an error after the PAT has already been mutated and can leave it with partial or empty scope. Please serialize Update/Delete on the PAT row (or version) and add compensation/swap semantics here instead of a standalone GetByID guard. Based on learnings, writes to the app DB and authz engine are non-atomic in core/userpat, so this path needs an explicit partial-failure safeguard.
Also applies to: 217-227
2 participants
Description:
Summary
UpdateCurrentUserPATRPC to replace a PAT's title, metadata, and scope (roles + projects)Changes
UpdateCurrentUserPATRPC, request (UUID-validated id, title, role_ids, project_ids, metadata), response with updated PATUpdatemethod — updates title + metadata, returns updated row viaRETURNING, handles ErrNotFound/ErrConflictUpdatemethod with helpers:getOwnedPAT,captureOldScope,replacePolicies,auditUpdatePATUpdatedEventwith old_title, old_role_ids, old_project_ids in metadataManual test verification
pat.updatedevent, including old and new title, role_ids, and project_ids