Skip to content

fix(deleter): cascade PAT cleanup on user delete #1672

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
AmanGIT07 merged 3 commits into from
Jun 4, 2026
Merged

fix(deleter): cascade PAT cleanup on user delete #1672

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
452fbe6
fix(deleter): cascade PAT cleanup on user delete
AmanGIT07 Jun 3, 2026
1c06840
fix(userpat): allow Delete and DeleteAllByUser when feature is disabled
AmanGIT07 Jun 3, 2026
1f68903
chore(userpat): restore Delete TOCTOU comment
AmanGIT07 Jun 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cmd/serve.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -577,8 +577,8 @@ func buildAPIDependencies(
)

cascadeDeleter := deleter.NewCascadeDeleter(organizationService, projectService, resourceService,
groupService, membershipService, policyService, roleService, invitationService, userService, serviceUserService,
customerService, subscriptionService, invoiceService,
groupService, membershipService, policyService, roleService, invitationService, userService, userPATService,
serviceUserService, customerService, subscriptionService, invoiceService,
)

// we should default it with a stdout logger repository as postgres can start to bloat really fast
Expand Down
83 changes: 83 additions & 0 deletions core/deleter/mocks/user_pat_service.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions core/deleter/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@ type UserService interface {
Delete(ctx context.Context, id string) error
}

type UserPATService interface {
DeleteAllByUser(ctx context.Context, userID string) error
}

type ServiceUserService interface {
List(ctx context.Context, flt serviceuser.Filter) ([]serviceuser.ServiceUser, error)
Delete(ctx context.Context, id string) error
Expand Down Expand Up @@ -112,6 +116,7 @@ type Service struct {
roleService RoleService
invitationService InvitationService
userService UserService
userPATService UserPATService
serviceUserService ServiceUserService
customerService CustomerService
subService SubscriptionService
Expand All @@ -123,6 +128,7 @@ func NewCascadeDeleter(orgService OrganizationService, projService ProjectServic
membershipService MembershipService,
policyService PolicyService, roleService RoleService,
invitationService InvitationService, userService UserService,
userPATService UserPATService,
serviceUserService ServiceUserService,
customerService CustomerService, subService SubscriptionService,
invoiceService InvoiceService) *Service {
Expand All @@ -136,6 +142,7 @@ func NewCascadeDeleter(orgService OrganizationService, projService ProjectServic
roleService: roleService,
invitationService: invitationService,
userService: userService,
userPATService: userPATService,
serviceUserService: serviceUserService,
customerService: customerService,
subService: subService,
Expand Down Expand Up @@ -394,6 +401,9 @@ func (d Service) DeleteUser(ctx context.Context, userID string) error {
return fmt.Errorf("failed to delete user from org[%s]: %w", orgID, err)
}
}
if err := d.userPATService.DeleteAllByUser(ctx, userID); err != nil {
return fmt.Errorf("failed to delete user PATs: %w", err)
}
return d.userService.Delete(ctx, userID)
}

Expand Down
62 changes: 39 additions & 23 deletions core/deleter/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ func newMocks(t *testing.T) (
*mocks.RoleService,
*mocks.InvitationService,
*mocks.UserService,
*mocks.UserPATService,
*mocks.ServiceUserService,
*mocks.CustomerService,
*mocks.SubscriptionService,
Expand All @@ -47,6 +48,7 @@ func newMocks(t *testing.T) (
mocks.NewRoleService(t),
mocks.NewInvitationService(t),
mocks.NewUserService(t),
mocks.NewUserPATService(t),
mocks.NewServiceUserService(t),
mocks.NewCustomerService(t),
mocks.NewSubscriptionService(t),
Expand All @@ -55,7 +57,7 @@ func newMocks(t *testing.T) (

func TestDeleteProject(t *testing.T) {
t.Run("deletes policies, resources, then project model", func(t *testing.T) {
orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t)
orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t)

polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: "proj-1"}).
Return([]policy.Policy{{ID: "pol-1"}, {ID: "pol-2"}}, nil)
Expand All @@ -68,54 +70,54 @@ func TestDeleteProject(t *testing.T) {

projSvc.EXPECT().DeleteModel(mock.Anything, "proj-1").Return(nil)

svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc)
svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc)
err := svc.DeleteProject(context.Background(), "proj-1")
assert.NoError(t, err)
})

t.Run("returns error when policy list fails", func(t *testing.T) {
_, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t)
_, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t)
orgSvc := mocks.NewOrganizationService(t)

polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: "proj-1"}).
Return(nil, errors.New("db error"))

svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc)
svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc)
err := svc.DeleteProject(context.Background(), "proj-1")
assert.ErrorContains(t, err, "db error")
})

t.Run("returns error when policy delete fails", func(t *testing.T) {
_, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t)
_, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t)
orgSvc := mocks.NewOrganizationService(t)

polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: "proj-1"}).
Return([]policy.Policy{{ID: "pol-fail"}}, nil)
polSvc.EXPECT().Delete(mock.Anything, "pol-fail").Return(errors.New("delete error"))

svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc)
svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc)
err := svc.DeleteProject(context.Background(), "proj-1")
assert.ErrorContains(t, err, "pol-fail")
})

t.Run("no policies — still deletes resources and project", func(t *testing.T) {
orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t)
orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t)

polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: "proj-1"}).
Return([]policy.Policy{}, nil)
resSvc.EXPECT().List(mock.Anything, resource.Filter{ProjectID: "proj-1"}).
Return([]resource.Resource{}, nil)
projSvc.EXPECT().DeleteModel(mock.Anything, "proj-1").Return(nil)

svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc)
svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc)
err := svc.DeleteProject(context.Background(), "proj-1")
assert.NoError(t, err)
})
}

func TestDeleteOrganization(t *testing.T) {
t.Run("full cascade delete", func(t *testing.T) {
orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t)
orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t)

// canDelete: no customers
custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}).
Expand Down Expand Up @@ -165,27 +167,27 @@ func TestDeleteOrganization(t *testing.T) {
// finally delete org model
orgSvc.EXPECT().DeleteModel(mock.Anything, "org-1").Return(nil)

svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc)
svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc)
err := svc.DeleteOrganization(context.Background(), "org-1")
assert.NoError(t, err)
})

t.Run("blocked when billed customer has invoices", func(t *testing.T) {
_, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t)
_, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t)
orgSvc := mocks.NewOrganizationService(t)

custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}).
Return([]customer.Customer{{ID: "cust-1", ProviderID: "stripe-1"}}, nil)
invocSvc.EXPECT().List(mock.Anything, invoice.Filter{CustomerID: "cust-1"}).
Return([]invoice.Invoice{{ID: "inv-1"}}, nil)

svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc)
svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc)
err := svc.DeleteOrganization(context.Background(), "org-1")
assert.ErrorIs(t, err, deleter.ErrDeleteNotAllowed)
})

t.Run("propagates error when service user list fails", func(t *testing.T) {
orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t)
orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t)

custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}).
Return([]customer.Customer{}, nil)
Expand All @@ -198,13 +200,13 @@ func TestDeleteOrganization(t *testing.T) {
suSvc.EXPECT().List(mock.Anything, serviceuser.Filter{OrgID: "org-1"}).
Return(nil, errors.New("su list failed"))

svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc)
svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc)
err := svc.DeleteOrganization(context.Background(), "org-1")
assert.ErrorContains(t, err, "su list failed")
})

t.Run("propagates error when service user delete fails", func(t *testing.T) {
orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t)
orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t)

custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}).
Return([]customer.Customer{}, nil)
Expand All @@ -218,7 +220,7 @@ func TestDeleteOrganization(t *testing.T) {
Return([]serviceuser.ServiceUser{{ID: "su-1"}}, nil)
suSvc.EXPECT().Delete(mock.Anything, "su-1").Return(errors.New("su delete failed"))

svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc)
svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc)
err := svc.DeleteOrganization(context.Background(), "org-1")
assert.ErrorContains(t, err, "su delete failed")
assert.ErrorContains(t, err, "su-1")
Expand All @@ -227,7 +229,7 @@ func TestDeleteOrganization(t *testing.T) {

func TestDeleteCustomers(t *testing.T) {
t.Run("deletes subscriptions invoices and customer", func(t *testing.T) {
orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t)
orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t)

c := customer.Customer{ID: "cust-1", ProviderID: "stripe-1"}
custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}).
Expand All @@ -236,36 +238,50 @@ func TestDeleteCustomers(t *testing.T) {
invocSvc.EXPECT().DeleteByCustomer(mock.Anything, c).Return(nil)
custSvc.EXPECT().Delete(mock.Anything, "cust-1").Return(nil)

svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc)
svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc)
err := svc.DeleteCustomers(context.Background(), "org-1")
assert.NoError(t, err)
})

t.Run("skips subscription and invoice delete when no provider", func(t *testing.T) {
orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t)
orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t)

c := customer.Customer{ID: "cust-no-provider", ProviderID: ""}
custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}).
Return([]customer.Customer{c}, nil)
// no sub or invoice delete expected
custSvc.EXPECT().Delete(mock.Anything, "cust-no-provider").Return(nil)

svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc)
svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc)
err := svc.DeleteCustomers(context.Background(), "org-1")
assert.NoError(t, err)
})
}

func TestDeleteUser(t *testing.T) {
t.Run("removes user from all orgs then deletes", func(t *testing.T) {
orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t)
t.Run("removes user from all orgs, cleans PATs, then deletes user", func(t *testing.T) {
orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t)

mbrSvc.EXPECT().ListResourcesByPrincipal(mock.Anything, mock.Anything, schema.OrganizationNamespace, mock.Anything).
Return(nil, nil)
patSvc.EXPECT().DeleteAllByUser(mock.Anything, "user-1").Return(nil)
usrSvc.EXPECT().Delete(mock.Anything, "user-1").Return(nil)

svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, suSvc, custSvc, subSvc, invocSvc)
svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc)
err := svc.DeleteUser(context.Background(), "user-1")
assert.NoError(t, err)
})

t.Run("aborts before userService.Delete when PAT cleanup fails", func(t *testing.T) {
orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t)

mbrSvc.EXPECT().ListResourcesByPrincipal(mock.Anything, mock.Anything, schema.OrganizationNamespace, mock.Anything).
Return(nil, nil)
patSvc.EXPECT().DeleteAllByUser(mock.Anything, "user-1").Return(errors.New("pat cleanup boom"))
// usrSvc.Delete must NOT be called — strict mock fails on unexpected call.

svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc)
err := svc.DeleteUser(context.Background(), "user-1")
assert.ErrorContains(t, err, "pat cleanup boom")
})
}
Loading
Loading