fix: resolve PAT to user for resource/project ownership and block PAT superuser access#1551
fix: resolve PAT to user for resource/project ownership and block PAT superuser access#1551
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughAdded audit record creation to the resource service with PAT principal resolution. Updated resource service constructor to accept an audit record repository, integrated audit record creation after resource creation, modified PAT principal handling to resolve to underlying users, adjusted project service to use resolved principal subjects, and updated authorization checks for PAT principals. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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.
🧹 Nitpick comments (5)
core/resource/service_test.go (3)
23-35: Consider a struct-based test harness to tame the 9-value tuple.The
newTestServicereturn signature now returns 9 values, and every test site repeats the_, _, …, svc := newTestService(t)dance. A smalltestDepsstruct (or returning*testDeps+*resource.Service) would make call sites far less error-prone and future-proof against further dependencies. Not blocking.♻️ Example refactor
-func newTestService(t *testing.T) (*mocks.Repository, *mocks.ConfigRepository, *mocks.RelationService, *mocks.AuthnService, *mocks.ProjectService, *mocks.OrgService, *mocks.PATService, *mocks.AuditRepository, *resource.Service) { - t.Helper() - repo := mocks.NewRepository(t) - configRepo := mocks.NewConfigRepository(t) - relationSvc := mocks.NewRelationService(t) - authnSvc := mocks.NewAuthnService(t) - projectSvc := mocks.NewProjectService(t) - orgSvc := mocks.NewOrgService(t) - patSvc := mocks.NewPATService(t) - auditRepo := mocks.NewAuditRepository(t) - svc := resource.NewService(repo, configRepo, relationSvc, authnSvc, projectSvc, orgSvc, patSvc, auditRepo) - return repo, configRepo, relationSvc, authnSvc, projectSvc, orgSvc, patSvc, auditRepo, svc -} +type testDeps struct { + repo *mocks.Repository + configRepo *mocks.ConfigRepository + relationSvc *mocks.RelationService + authnSvc *mocks.AuthnService + projectSvc *mocks.ProjectService + orgSvc *mocks.OrgService + patSvc *mocks.PATService + auditRepo *mocks.AuditRepository + svc *resource.Service +} + +func newTestService(t *testing.T) *testDeps { + t.Helper() + d := &testDeps{ + repo: mocks.NewRepository(t), + configRepo: mocks.NewConfigRepository(t), + relationSvc: mocks.NewRelationService(t), + authnSvc: mocks.NewAuthnService(t), + projectSvc: mocks.NewProjectService(t), + orgSvc: mocks.NewOrgService(t), + patSvc: mocks.NewPATService(t), + auditRepo: mocks.NewAuditRepository(t), + } + d.svc = resource.NewService(d.repo, d.configRepo, d.relationSvc, d.authnSvc, d.projectSvc, d.orgSvc, d.patSvc, d.auditRepo) + return d +}
383-420: Consider asserting the audit record payload, not justAnythingOfType.
TestCreateis the primary guard for the new audit-emission behavior. Matching only the parameter type means regressions like a missingOrgID, wrongEvent, or an incorrectTarget.Typewouldn't be caught. Considermock.MatchedBy(or a captured arg) to assertEvent == pkgauditrecord.ResourceCreatedEvent,Resource.ID == project.ID,Target.ID == newResource.ID, andOrgID == project.Organization.ID.
383-397: Nit:patSvc.GetByIDshouldn't actually be reachable in this test.With the authenticated principal carrying a matching
PAT.ID == patID,resolvePATUsersatisfies via the context fast path and never callspatService.GetByID. The.Maybe()masks this; consider removing thepatSvcexpectation here and adding a separate test that specifically covers the DB fallback (explicit-principal path withPATunset in context). That way each path is genuinely exercised.core/resource/service.go (2)
84-105: Minor:GetPrincipalis invoked twice on the common PAT path.When
principalIDis empty and the caller is a PAT,GetPrincipalis called on line 90 and then again insideresolvePATUser(line 296). It's harmless but an unnecessary round-trip on the hot path. You can piggyback off the already-fetched principal:♻️ Suggested tweak
- principalID := res.PrincipalID - principalType := res.PrincipalType - if strings.TrimSpace(principalID) == "" { - principal, err := s.authnService.GetPrincipal(ctx) - if err != nil { - return Resource{}, err - } - principalID = principal.ID - principalType = principal.Type - } - // PAT → resolve to underlying user - if principalType == schema.PATPrincipal { - sub, err := s.resolvePATUser(ctx, principalID) - if err != nil { - return Resource{}, fmt.Errorf("resolving PAT principal: %w", err) - } - principalID = sub.ID - principalType = sub.Namespace - } + principalID := res.PrincipalID + principalType := res.PrincipalType + if strings.TrimSpace(principalID) == "" { + principal, err := s.authnService.GetPrincipal(ctx) + if err != nil { + return Resource{}, err + } + if principal.PAT != nil { + principalID, principalType = principal.PAT.UserID, schema.UserPrincipal + } else { + principalID, principalType = principal.ID, principal.Type + } + } else if principalType == schema.PATPrincipal { + // explicit PAT subject (e.g., admin federated call) — resolve via DB + sub, err := s.resolvePATUser(ctx, principalID) + if err != nil { + return Resource{}, fmt.Errorf("resolving PAT principal: %w", err) + } + principalID, principalType = sub.ID, sub.Namespace + }
57-59: Interface naming nit: considerAuditRecordRepositoryfor consistency.The struct field is
auditRepo, but the concrete type incmd/serve.goisauditRecordRepository(postgres.NewAuditRecordRepository(...)) and the package isauditrecord. Naming the local interfaceAuditRecordRepository(matchingcore/organization,core/membership, anduserpatusages that also depend on the same repo) keeps the mental model consistent across services.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e525389-e548-4abd-a686-8f525019f8b7
📒 Files selected for processing (8)
cmd/serve.gocore/project/service.gocore/resource/mocks/audit_repository.gocore/resource/service.gocore/resource/service_test.gointernal/api/v1beta1connect/authorize.gointernal/api/v1beta1connect/mocks/membership_service.gopkg/auditrecord/consts.go
Coverage Report for CI Build 24607736829Coverage increased (+0.3%) to 42.071%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/resource/service_test.go (1)
373-374: Tighten audit record assertions to lock in content contract.All three
TestCreatesubtests match the audit record withmock.AnythingOfType("models.AuditRecord"), which will accept a completely empty record. This is why the missingActorfield increateAuditRecord(flagged incore/resource/service.go) slips through. Usingmock.MatchedByon at least one subtest to assertEvent,Target.ID == createdRes.ID,OrgID, and — once fixed —Actor.ID/Actor.Typewould prevent regressions on the audit payload.♻️ Example tightening for the PAT subtest
- auditRepo.EXPECT().Create(mock.Anything, mock.AnythingOfType("models.AuditRecord")). - Return(auditmodels.AuditRecord{}, nil) + auditRepo.EXPECT().Create(mock.Anything, mock.MatchedBy(func(ar auditmodels.AuditRecord) bool { + return ar.Event == pkgauditrecord.ResourceCreatedEvent && + ar.Target != nil && ar.Target.Name == "res-pat" && + ar.OrgID == testProject.Organization.ID && + ar.Actor.ID == patID // once Actor is populated + })).Return(auditmodels.AuditRecord{}, nil)Also applies to: 411-412, 438-439
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c4cb959a-9859-4762-af52-2bb10561fc2e
📒 Files selected for processing (3)
core/resource/mocks/audit_record_repository.gocore/resource/service.gocore/resource/service_test.go
2 participants
Summary
app/pat) was used directly to create ownership policies, relations forresources, where onlyapp/userorapp/serviceuserare allowedResolveSubject()before creating ownership artifactsresource.createdaudit event with actor from auth context (PAT ID + user metadata preserved in actor)IsSuperUser— scoped tokens should not bypass authzFixes
core/resource/service.goCreatecore/project/service.goCreateResolveSubject()before policy creationauthorize.goIsSuperUserPermissionDeniedauthorize.goIntrospectPolicyprincipal.IDuser_idAudit record
resource.createdevent constantTests
go build ./...passesmake lint-fix— 0 issues