From b2e858ea8cce779eca93cd44f829bde066e1d1af Mon Sep 17 00:00:00 2001 From: aman Date: Fri, 29 May 2026 13:01:35 +0530 Subject: [PATCH 1/2] fix(group): resolve PAT to user before owner wiring on group create CreateGroup writes the Postgres row before calling membership.OnGroupCreated. For PAT-authenticated principals the membership service rejects the app/pat principal type when assigning the group owner role, returning ErrInvalidPrincipalType. The error propagates back to the caller but the Postgres row is not rolled back, leaving an orphan with no SpiceDB hierarchy and no owner policy. Resolve the PAT to its underlying user via principal.ResolveSubject() before invoking OnGroupCreated, mirroring core/project/service.go. Co-Authored-By: Claude Opus 4.7 (1M context) --- core/group/service.go | 4 +++- core/group/service_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/core/group/service.go b/core/group/service.go index 98e149675..bb8dd7175 100644 --- a/core/group/service.go +++ b/core/group/service.go @@ -74,7 +74,9 @@ func (s Service) Create(ctx context.Context, grp Group) (Group, error) { return Group{}, err } - if err = s.membershipService.OnGroupCreated(ctx, newGroup.ID, newGroup.OrganizationID, principal.ID, principal.Type); err != nil { + // PAT → resolve to underlying user so ownership is on the user, not the token + subjectID, subjectType := principal.ResolveSubject() + if err = s.membershipService.OnGroupCreated(ctx, newGroup.ID, newGroup.OrganizationID, subjectID, subjectType); err != nil { return Group{}, err } diff --git a/core/group/service_test.go b/core/group/service_test.go index 1a8c0efd7..e55edd48a 100644 --- a/core/group/service_test.go +++ b/core/group/service_test.go @@ -68,6 +68,34 @@ func TestService_Create(t *testing.T) { assert.Equal(t, strings.Contains(err.Error(), authenticate.ErrInvalidID.Error()), true) }) + t.Run("PAT creator resolves to underlying user before OnGroupCreated", func(t *testing.T) { + mockRepo := mocks.NewRepository(t) + mockAuthnSvc := mocks.NewAuthnService(t) + mockRelationSvc := mocks.NewRelationService(t) + mockPolicySvc := mocks.NewPolicyService(t) + mockMembershipSvc := mocks.NewMembershipService(t) + + svc := group.NewService(mockRepo, mockRelationSvc, mockAuthnSvc, mockPolicySvc) + svc.SetMembershipService(mockMembershipSvc) + + userID := uuid.New().String() + patID := uuid.New().String() + mockAuthnSvc.On("GetPrincipal", mock.Anything).Return(authenticate.Principal{ + ID: patID, + Type: schema.PATPrincipal, + PAT: &pat.PAT{ID: patID, UserID: userID}, + }, nil) + + groupParam := group.Group{Name: "g", OrganizationID: uuid.New().String()} + groupInRepo := groupParam + groupInRepo.ID = uuid.New().String() + mockRepo.On("Create", mock.Anything, groupParam).Return(groupInRepo, nil) + mockMembershipSvc.EXPECT().OnGroupCreated(mock.Anything, groupInRepo.ID, groupInRepo.OrganizationID, userID, schema.UserPrincipal).Return(nil) + + _, err := svc.Create(context.Background(), groupParam) + assert.NoError(t, err) + }) + t.Run("should propagate error from membership.OnGroupCreated", func(t *testing.T) { mockRepo := mocks.NewRepository(t) mockAuthnSvc := mocks.NewAuthnService(t) From 65b0f735aba5cf206b84f33c1942671c11eb115f Mon Sep 17 00:00:00 2001 From: aman Date: Fri, 29 May 2026 13:17:29 +0530 Subject: [PATCH 2/2] fix(group): roll back Postgres row when OnGroupCreated fails Closes the orphan window for all OnGroupCreated failure modes, not just PAT. Errors are joined so the original membership failure is preserved when rollback itself fails. Co-Authored-By: Claude Opus 4.7 (1M context) --- core/group/service.go | 3 +++ core/group/service_test.go | 33 ++++++++++++++++++++++++++++++++- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/core/group/service.go b/core/group/service.go index bb8dd7175..d2bc189e5 100644 --- a/core/group/service.go +++ b/core/group/service.go @@ -77,6 +77,9 @@ func (s Service) Create(ctx context.Context, grp Group) (Group, error) { // PAT → resolve to underlying user so ownership is on the user, not the token subjectID, subjectType := principal.ResolveSubject() if err = s.membershipService.OnGroupCreated(ctx, newGroup.ID, newGroup.OrganizationID, subjectID, subjectType); err != nil { + if cleanupErr := s.repository.Delete(ctx, newGroup.ID); cleanupErr != nil { + return Group{}, errors.Join(err, fmt.Errorf("rollback group create: %w", cleanupErr)) + } return Group{}, err } diff --git a/core/group/service_test.go b/core/group/service_test.go index e55edd48a..f5a7da59f 100644 --- a/core/group/service_test.go +++ b/core/group/service_test.go @@ -96,7 +96,7 @@ func TestService_Create(t *testing.T) { assert.NoError(t, err) }) - t.Run("should propagate error from membership.OnGroupCreated", func(t *testing.T) { + t.Run("OnGroupCreated failure rolls back the Postgres group row", func(t *testing.T) { mockRepo := mocks.NewRepository(t) mockAuthnSvc := mocks.NewAuthnService(t) mockRelationSvc := mocks.NewRelationService(t) @@ -118,10 +118,41 @@ func TestService_Create(t *testing.T) { groupInRepo.ID = uuid.New().String() mockRepo.On("Create", mock.Anything, groupParam).Return(groupInRepo, nil) mockMembershipSvc.EXPECT().OnGroupCreated(mock.Anything, groupInRepo.ID, groupInRepo.OrganizationID, mockUserID, schema.UserPrincipal).Return(errors.New("spicedb down")) + mockRepo.On("Delete", mock.Anything, groupInRepo.ID).Return(nil).Once() _, err := svc.Create(context.Background(), groupParam) assert.ErrorContains(t, err, "spicedb down") }) + + t.Run("rollback failure surfaces both errors", func(t *testing.T) { + mockRepo := mocks.NewRepository(t) + mockAuthnSvc := mocks.NewAuthnService(t) + mockRelationSvc := mocks.NewRelationService(t) + mockPolicySvc := mocks.NewPolicyService(t) + mockMembershipSvc := mocks.NewMembershipService(t) + + svc := group.NewService(mockRepo, mockRelationSvc, mockAuthnSvc, mockPolicySvc) + svc.SetMembershipService(mockMembershipSvc) + + mockUserID := uuid.New().String() + mockAuthnSvc.On("GetPrincipal", mock.Anything).Return(authenticate.Principal{ + ID: mockUserID, + Type: schema.UserPrincipal, + User: &user.User{ID: mockUserID}, + }, nil) + + groupParam := group.Group{Name: "g", OrganizationID: uuid.New().String()} + groupInRepo := groupParam + groupInRepo.ID = uuid.New().String() + mockRepo.On("Create", mock.Anything, groupParam).Return(groupInRepo, nil) + mockMembershipSvc.EXPECT().OnGroupCreated(mock.Anything, groupInRepo.ID, groupInRepo.OrganizationID, mockUserID, schema.UserPrincipal).Return(errors.New("spicedb down")) + mockRepo.On("Delete", mock.Anything, groupInRepo.ID).Return(errors.New("pg gone")).Once() + + _, err := svc.Create(context.Background(), groupParam) + assert.ErrorContains(t, err, "spicedb down") + assert.ErrorContains(t, err, "rollback group create") + assert.ErrorContains(t, err, "pg gone") + }) } func TestService_Get(t *testing.T) {