Skip to content

fix(membership): revoke a removed member's PATs in the org #2048

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

Open
AmanGIT07 wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix(membership): revoke a removed member's PATs in the org #2048

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
7266892
fix(membership): revoke a removed member's PATs in the org
AmanGIT07 Nov 11, 2026
c0aec7b
fix(membership): revoke leftover PATs when the member is already removed
AmanGIT07 Nov 11, 2026
ddfe0f4
test(e2e): cover a project-scoped PAT on member removal
AmanGIT07 Nov 11, 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
51 changes: 49 additions & 2 deletions core/membership/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.

28 changes: 26 additions & 2 deletions core/membership/org.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,12 +205,19 @@ func (s *Service) SetPATAllProjectsRole(ctx context.Context, orgID, patID, roleI
// relations. Returns ErrNotMember if the principal has no policies on this org,
// and ErrLastOwnerRole when removing the org's last owner.
func (s *Service) RemoveOrganizationMember(ctx context.Context, orgID, principalID, principalType string) error {
return s.removeOrganizationMember(ctx, orgID, principalID, principalType, false)
removeErr := s.removeOrganizationMember(ctx, orgID, principalID, principalType, false)
if removeErr != nil && !errors.Is(removeErr, ErrNotMember) {
return removeErr
}
if patErr := s.revokeMemberPATs(ctx, orgID, principalID, principalType); patErr != nil {

@coderabbitai coderabbitai Bot Nov 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor · Data Integrity & Integration · Quick win

Supported by static analysis

Script executed:

# Locate PAT repository implementations and inspect deletion filters.
fd -i 'pat.*\.go$' . |
while IFS= read -r file; do
  rg -n -C 6 'func .*List\(|deleted_at|DeletedAt|IncludeDeleted|include_deleted' "$file"
done

# Inspect the policy-cleanup entry point.
rg -n -C 12 --glob '*.go' 'func .*RemoveAllPATPolicies\(' .

Repository: raystack/frontier

Length of output: 19075

Script executed:

git diff f38ac119be14d4fda11f874ecfad9b2c4d9b65e8 ddfe0f438081e83b4bad4a9a5d735919af7da49c -- core/membership/org.go core/userpat/service.go
rg -n -F -e 'listAllByUserInOrg' -e 'OnOrgMemberRemoved' -e 'RemoveAllPATPolicies' -e 'ListAllByOrg' -- core/userpat core/membership internal | grep -v '/mocks/'
rg -n -F 'type UserPATRepository struct' -- .
cat -n core/membership/org.go | sed -n '175,260p'
cat -n core/userpat/service.go | sed -n '1,180p'
cat -n core/userpat/service.go | sed -n '200,420p'

Repository: raystack/frontier

Length of output: 32815

Script executed:

cat -n internal/store/postgres/userpat_repository.go | sed -n '25,45p;123,216p;414,460p;483,515p'
cat -n core/membership/service.go | sed -n '130,205p'
cat -n core/membership/org.go | sed -n '399,418p'
rg -n -F -e 'NewUserPATRepository(' -e 'userpat.NewService(' -e 'func (s *Service) removePoliciesByFilter' -- internal core
rg -n -F 'func (s *Service) Delete(' -- core/policy

Repository: raystack/frontier

Length of output: 26172

Script executed:

rg -n -F -e 'NewUserPATRepository' -e 'userpat.NewService' -e 'SetUserPATService' --glob '!**/*test.go' --glob '!**/mocks/**' .

Repository: raystack/frontier

Length of output: 795

Allow retries to finish PAT policy cleanup.

If policy cleanup fails after revokePAT soft-deletes a PAT, the next member-removal attempt skips that PAT because UserPATRepository.List excludes deleted rows. Its policies can remain until a separate cleanup occurs.

Make pending cleanup reachable on retry without changing normal PAT listings.

return patErr
}
return removeErr
}

// RemoveOrganizationMemberOnDelete is RemoveOrganizationMember for deleting a
// user. It also works on a disabled org, because the user has to leave every
// org before the account can go.
// org before the account can go. It leaves PATs to the user delete.

@rohilsurana rohilsurana Nov 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small one: this adds a new sentence to a code comment. We are trying not to add new code comments. Could the "leaves PATs to the user delete" note move to the PR description instead?

@AmanGIT07 AmanGIT07 Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping this line. The code here can't show it: user delete revokes all of the user's PATs itself, in core/deleter (DeleteAllByUser). Without the line, the missing PAT step next to RemoveOrganizationMember reads like a bug.

func (s *Service) RemoveOrganizationMemberOnDelete(ctx context.Context, orgID, principalID, principalType string) error {
return s.removeOrganizationMember(ctx, orgID, principalID, principalType, true)
}
Expand Down Expand Up @@ -392,6 +399,23 @@ func (s *Service) removeCustomResourceAccess(ctx context.Context, principalID, p
return nil
}

func (s *Service) revokeMemberPATs(ctx context.Context, orgID, principalID, principalType string) error {
if principalType != schema.UserPrincipal {
return nil
}
if s.userPATService == nil {
s.log.ErrorContext(ctx, "user PAT service not set: PATs left behind",
"org_id", orgID,
"principal_id", principalID,
)
return nil
}
if err := s.userPATService.OnOrgMemberRemoved(ctx, orgID, principalID); err != nil {
return fmt.Errorf("revoke member PATs: %w", err)
}
return nil
}

// validateMinOwnerConstraint ensures the org always has at least one owner after a role change.
// Returns the resolved owner role ID for reuse by callers.
func (s *Service) validateMinOwnerConstraint(ctx context.Context, orgID, newRoleID string, existing []policy.Policy) (string, error) {
Expand Down
76 changes: 75 additions & 1 deletion core/membership/org_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -744,12 +744,14 @@ func TestService_RemoveOrganizationMember(t *testing.T) {
projSvc *mocks.ProjectService
grpSvc *mocks.GroupService
resSvc *mocks.ResourceService
patSvc *mocks.UserPATService
auditRepo *mocks.AuditRecordRepository
}

tests := []struct {
name string
principalType string
noPATService bool
setup func(d testDeps)
wantErr error
wantErrContain string
Expand All @@ -775,13 +777,23 @@ func TestService_RemoveOrganizationMember(t *testing.T) {
wantErr: organization.ErrDisabled,
},
{
name: "should return ErrNotMember if principal has no org policies",
name: "should revoke leftover PATs and return ErrNotMember if principal has no org policies",
setup: func(d testDeps) {
d.orgSvc.EXPECT().Get(ctx, orgID).Return(enabledOrg, nil)
d.policySvc.EXPECT().List(ctx, policy.Filter{OrgID: orgID, PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{}, nil)
d.patSvc.EXPECT().OnOrgMemberRemoved(ctx, orgID, userID).Return(nil)
},
wantErr: membership.ErrNotMember,
},
{
name: "should return error if revoking leftover PATs fails for a principal with no org policies",
setup: func(d testDeps) {
d.orgSvc.EXPECT().Get(ctx, orgID).Return(enabledOrg, nil)
d.policySvc.EXPECT().List(ctx, policy.Filter{OrgID: orgID, PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{}, nil)
d.patSvc.EXPECT().OnOrgMemberRemoved(ctx, orgID, userID).Return(errors.New("pat boom"))
},
wantErrContain: "revoke member PATs",
},
{
name: "should return ErrLastOwnerRole when removing the last owner",
setup: func(d testDeps) {
Expand Down Expand Up @@ -827,6 +839,7 @@ func TestService_RemoveOrganizationMember(t *testing.T) {
d.policySvc.EXPECT().Delete(ctx, "org-p1").Return(nil)
d.resSvc.EXPECT().RemovePrincipalAccess(ctx, userID, schema.UserPrincipal, orgID, []string{}).Return(nil)
d.auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil)
d.patSvc.EXPECT().OnOrgMemberRemoved(ctx, orgID, userID).Return(nil)
},
wantErr: nil,
},
Expand All @@ -851,6 +864,7 @@ func TestService_RemoveOrganizationMember(t *testing.T) {
d.resSvc.EXPECT().RemovePrincipalAccess(ctx, userID, schema.UserPrincipal, orgID, []string{projectID}).Return(nil)
d.relSvc.EXPECT().Delete(ctx, relation.Relation{Object: grpObj, Subject: userSub, RelationName: schema.MemberRelationName}).Return(relation.ErrNotExist)
d.auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil)
d.patSvc.EXPECT().OnOrgMemberRemoved(ctx, orgID, userID).Return(nil)
},
wantErr: nil,
},
Expand All @@ -872,6 +886,7 @@ func TestService_RemoveOrganizationMember(t *testing.T) {
d.policySvc.EXPECT().Delete(ctx, "org-p1").Return(nil)
d.resSvc.EXPECT().RemovePrincipalAccess(ctx, userID, schema.UserPrincipal, orgID, []string{}).Return(nil)
d.auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil)
d.patSvc.EXPECT().OnOrgMemberRemoved(ctx, orgID, userID).Return(nil)
},
wantErr: nil,
},
Expand Down Expand Up @@ -925,6 +940,7 @@ func TestService_RemoveOrganizationMember(t *testing.T) {
d.policySvc.EXPECT().Delete(ctx, "org-p1").Return(nil)
d.resSvc.EXPECT().RemovePrincipalAccess(ctx, userID, schema.UserPrincipal, orgID, []string{projectID, secondProjectID}).Return(nil)
d.auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil)
d.patSvc.EXPECT().OnOrgMemberRemoved(ctx, orgID, userID).Return(nil)
},
wantErr: nil,
},
Expand Down Expand Up @@ -962,6 +978,59 @@ func TestService_RemoveOrganizationMember(t *testing.T) {
},
wantErr: membership.ErrLastOwnerRole,
},
{
name: "should return error if PAT revocation fails after the member is removed",
setup: func(d testDeps) {
d.orgSvc.EXPECT().Get(ctx, orgID).Return(enabledOrg, nil)
d.policySvc.EXPECT().List(ctx, policy.Filter{OrgID: orgID, PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{{ID: "org-p1", RoleID: viewerRoleID}}, nil)
d.roleSvc.EXPECT().Get(ctx, schema.RoleOrganizationOwner).Return(role.Role{ID: ownerRoleID, Name: schema.RoleOrganizationOwner}, nil)
d.projSvc.EXPECT().List(ctx, project.Filter{OrgID: orgID}).Return([]project.Project{}, nil)
d.grpSvc.EXPECT().List(ctx, group.Filter{OrganizationID: orgID}).Return([]group.Group{}, nil)
d.policySvc.EXPECT().List(ctx, policy.Filter{PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{
{ID: "org-p1", ResourceType: schema.OrganizationNamespace, ResourceID: orgID},
}, nil)
d.policySvc.EXPECT().Delete(ctx, "org-p1").Return(nil)
d.resSvc.EXPECT().RemovePrincipalAccess(ctx, userID, schema.UserPrincipal, orgID, []string{}).Return(nil)
d.auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil)
d.patSvc.EXPECT().OnOrgMemberRemoved(ctx, orgID, userID).Return(errors.New("pat boom"))
},
wantErrContain: "revoke member PATs",
},
{
name: "should not revoke PATs when the removed principal is not a user",
principalType: schema.PATPrincipal,
setup: func(d testDeps) {
d.orgSvc.EXPECT().Get(ctx, orgID).Return(enabledOrg, nil)
d.policySvc.EXPECT().List(ctx, policy.Filter{OrgID: orgID, PrincipalID: userID, PrincipalType: schema.PATPrincipal}).Return([]policy.Policy{{ID: "org-p1", RoleID: viewerRoleID}}, nil)
d.projSvc.EXPECT().List(ctx, project.Filter{OrgID: orgID}).Return([]project.Project{}, nil)
d.grpSvc.EXPECT().List(ctx, group.Filter{OrganizationID: orgID}).Return([]group.Group{}, nil)
d.policySvc.EXPECT().List(ctx, policy.Filter{PrincipalID: userID, PrincipalType: schema.PATPrincipal}).Return([]policy.Policy{
{ID: "org-p1", ResourceType: schema.OrganizationNamespace, ResourceID: orgID},
}, nil)
d.policySvc.EXPECT().Delete(ctx, "org-p1").Return(nil)
d.resSvc.EXPECT().RemovePrincipalAccess(ctx, userID, schema.PATPrincipal, orgID, []string{}).Return(nil)
d.auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil)
},
wantErr: nil,
},
{
name: "should still remove the member when the PAT service is not set",
noPATService: true,
setup: func(d testDeps) {
d.orgSvc.EXPECT().Get(ctx, orgID).Return(enabledOrg, nil)
d.policySvc.EXPECT().List(ctx, policy.Filter{OrgID: orgID, PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{{ID: "org-p1", RoleID: viewerRoleID}}, nil)
d.roleSvc.EXPECT().Get(ctx, schema.RoleOrganizationOwner).Return(role.Role{ID: ownerRoleID, Name: schema.RoleOrganizationOwner}, nil)
d.projSvc.EXPECT().List(ctx, project.Filter{OrgID: orgID}).Return([]project.Project{}, nil)
d.grpSvc.EXPECT().List(ctx, group.Filter{OrganizationID: orgID}).Return([]group.Group{}, nil)
d.policySvc.EXPECT().List(ctx, policy.Filter{PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{
{ID: "org-p1", ResourceType: schema.OrganizationNamespace, ResourceID: orgID},
}, nil)
d.policySvc.EXPECT().Delete(ctx, "org-p1").Return(nil)
d.resSvc.EXPECT().RemovePrincipalAccess(ctx, userID, schema.UserPrincipal, orgID, []string{}).Return(nil)
d.auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil)
},
wantErr: nil,
},
}

for _, tt := range tests {
Expand All @@ -974,6 +1043,7 @@ func TestService_RemoveOrganizationMember(t *testing.T) {
projSvc: mocks.NewProjectService(t),
grpSvc: mocks.NewGroupService(t),
resSvc: mocks.NewResourceService(t),
patSvc: mocks.NewUserPATService(t),
auditRepo: mocks.NewAuditRecordRepository(t),
}
if tt.setup != nil {
Expand All @@ -982,6 +1052,9 @@ func TestService_RemoveOrganizationMember(t *testing.T) {

svc := membership.NewService(slog.New(slog.NewTextHandler(io.Discard, nil)), d.policySvc, d.relSvc, d.roleSvc, d.orgSvc, mocks.NewUserService(t), d.projSvc, d.grpSvc, mocks.NewServiceuserService(t), d.auditRepo)
svc.SetResourceService(d.resSvc)
if !tt.noPATService {
svc.SetUserPATService(d.patSvc)
}

principalType := tt.principalType
if principalType == "" {
Expand Down Expand Up @@ -1096,6 +1169,7 @@ func TestService_RemoveOrganizationMemberOnDelete(t *testing.T) {

svc := membership.NewService(slog.New(slog.NewTextHandler(io.Discard, nil)), d.policySvc, mocks.NewRelationService(t), d.roleSvc, d.orgSvc, mocks.NewUserService(t), d.projSvc, d.grpSvc, mocks.NewServiceuserService(t), d.auditRepo)
svc.SetResourceService(d.resSvc)
svc.SetUserPATService(mocks.NewUserPATService(t))

err := svc.RemoveOrganizationMemberOnDelete(ctx, orgID, userID, schema.UserPrincipal)
if tt.wantErr != nil {
Expand Down
1 change: 1 addition & 0 deletions core/membership/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ type ServiceuserService interface {

type UserPATService interface {
GetByID(ctx context.Context, id string) (patmodels.PAT, error)
OnOrgMemberRemoved(ctx context.Context, orgID, userID string) error
}

type ResourceService interface {
Expand Down
36 changes: 33 additions & 3 deletions core/userpat/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ var supportedPATResourceTypes = []string{
schema.ProjectNamespace,
}

const patRemovalPageSize = 1000

type OrganizationService interface {
GetRaw(ctx context.Context, id string) (organization.Organization, error)
}
Expand Down Expand Up @@ -198,14 +200,42 @@ func (s *Service) DeleteAllByOrg(ctx context.Context, orgID string) error {
return fmt.Errorf("listing PATs for org: %w", err)
}
for _, pat := range pats {
if err := s.revokeOrgPAT(ctx, pat); err != nil {
if err := s.revokePAT(ctx, pat, nil); err != nil {
return fmt.Errorf("deleting PAT[%s]: %w", pat.ID, err)
}
}
return nil
}

func (s *Service) OnOrgMemberRemoved(ctx context.Context, orgID, userID string) error {
pats, err := s.listAllByUserInOrg(ctx, userID, orgID)
if err != nil {
return fmt.Errorf("listing PATs for user in org: %w", err)
}
for _, pat := range pats {
if err := s.revokePAT(ctx, pat, map[string]any{"reason": "member_removed"}); err != nil {
return fmt.Errorf("deleting PAT[%s]: %w", pat.ID, err)
}
}
return nil
}

func (s *Service) revokeOrgPAT(ctx context.Context, pat patmodels.PAT) error {
func (s *Service) listAllByUserInOrg(ctx context.Context, userID, orgID string) ([]patmodels.PAT, error) {
var pats []patmodels.PAT
for offset := 0; ; offset += patRemovalPageSize {
query := pkgUtils.NewRQLQuery("", offset, patRemovalPageSize, []rql.Filter{}, []rql.Sort{}, []string{})
page, err := s.repo.List(ctx, userID, orgID, query)
if err != nil {
return nil, err
}
pats = append(pats, page.PATs...)
if len(page.PATs) < patRemovalPageSize {
return pats, nil
}
}
}

func (s *Service) revokePAT(ctx context.Context, pat patmodels.PAT, targetMetadata map[string]any) error {
softDeleted := false
if pat.DeletedAt == nil {
err := s.repo.Delete(ctx, pat.ID)
Expand All @@ -223,7 +253,7 @@ func (s *Service) revokeOrgPAT(ctx context.Context, pat patmodels.PAT) error {
}

if softDeleted {
if err := s.createAuditRecord(ctx, pkgAuditRecord.PATRevokedEvent, pat, time.Now().UTC(), nil); err != nil {
if err := s.createAuditRecord(ctx, pkgAuditRecord.PATRevokedEvent, pat, time.Now().UTC(), targetMetadata); err != nil {
s.logger.ErrorContext(ctx, "failed to create audit record for PAT revocation", "pat_id", pat.ID, "error", err)
}
}
Expand Down
Loading
Loading