From 72668928a0f4efc2385f94ce4432d9c3129bd78b Mon Sep 17 00:00:00 2001 From: aman Date: Wed, 11 Nov 2026 12:31:30 +0530 Subject: [PATCH 1/3] fix(membership): revoke a removed member's PATs in the org RemoveOrganizationMember now revokes the PATs the removed user holds in that org, each with reason member_removed in its audit record. User delete is unchanged and still revokes all of the user's PATs at the end. --- core/membership/mocks/user_pat_service.go | 51 +++++++++- core/membership/org.go | 24 ++++- core/membership/org_test.go | 64 +++++++++++++ core/membership/service.go | 1 + core/userpat/service.go | 36 +++++++- core/userpat/service_test.go | 107 +++++++++++++++++++++ test/e2e/regression/pat_test.go | 108 ++++++++++++++++++++++ 7 files changed, 384 insertions(+), 7 deletions(-) diff --git a/core/membership/mocks/user_pat_service.go b/core/membership/mocks/user_pat_service.go index 0987c7e574..8b2a6eae8f 100644 --- a/core/membership/mocks/user_pat_service.go +++ b/core/membership/mocks/user_pat_service.go @@ -5,9 +5,8 @@ package mocks import ( context "context" - mock "github.com/stretchr/testify/mock" - models "github.com/raystack/frontier/core/userpat/models" + mock "github.com/stretchr/testify/mock" ) // UserPATService is an autogenerated mock type for the UserPATService type @@ -80,6 +79,54 @@ func (_c *UserPATService_GetByID_Call) RunAndReturn(run func(context.Context, st return _c } +// OnOrgMemberRemoved provides a mock function with given fields: ctx, orgID, userID +func (_m *UserPATService) OnOrgMemberRemoved(ctx context.Context, orgID string, userID string) error { + ret := _m.Called(ctx, orgID, userID) + + if len(ret) == 0 { + panic("no return value specified for OnOrgMemberRemoved") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string, string) error); ok { + r0 = rf(ctx, orgID, userID) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// UserPATService_OnOrgMemberRemoved_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'OnOrgMemberRemoved' +type UserPATService_OnOrgMemberRemoved_Call struct { + *mock.Call +} + +// OnOrgMemberRemoved is a helper method to define mock.On call +// - ctx context.Context +// - orgID string +// - userID string +func (_e *UserPATService_Expecter) OnOrgMemberRemoved(ctx interface{}, orgID interface{}, userID interface{}) *UserPATService_OnOrgMemberRemoved_Call { + return &UserPATService_OnOrgMemberRemoved_Call{Call: _e.mock.On("OnOrgMemberRemoved", ctx, orgID, userID)} +} + +func (_c *UserPATService_OnOrgMemberRemoved_Call) Run(run func(ctx context.Context, orgID string, userID string)) *UserPATService_OnOrgMemberRemoved_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(string), args[2].(string)) + }) + return _c +} + +func (_c *UserPATService_OnOrgMemberRemoved_Call) Return(_a0 error) *UserPATService_OnOrgMemberRemoved_Call { + _c.Call.Return(_a0) + return _c +} + +func (_c *UserPATService_OnOrgMemberRemoved_Call) RunAndReturn(run func(context.Context, string, string) error) *UserPATService_OnOrgMemberRemoved_Call { + _c.Call.Return(run) + return _c +} + // NewUserPATService creates a new instance of UserPATService. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. // The first argument is typically a *testing.T value. func NewUserPATService(t interface { diff --git a/core/membership/org.go b/core/membership/org.go index 220a024e76..f6a4f7119f 100644 --- a/core/membership/org.go +++ b/core/membership/org.go @@ -205,12 +205,15 @@ 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) + if err := s.removeOrganizationMember(ctx, orgID, principalID, principalType, false); err != nil { + return err + } + return s.revokeMemberPATs(ctx, orgID, principalID, principalType) } // 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. func (s *Service) RemoveOrganizationMemberOnDelete(ctx context.Context, orgID, principalID, principalType string) error { return s.removeOrganizationMember(ctx, orgID, principalID, principalType, true) } @@ -392,6 +395,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) { diff --git a/core/membership/org_test.go b/core/membership/org_test.go index 9de68b50d7..48014dcfec 100644 --- a/core/membership/org_test.go +++ b/core/membership/org_test.go @@ -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 @@ -827,6 +829,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, }, @@ -851,6 +854,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, }, @@ -872,6 +876,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, }, @@ -925,6 +930,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, }, @@ -962,6 +968,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 { @@ -974,6 +1033,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 { @@ -982,6 +1042,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 == "" { @@ -1096,6 +1159,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 { diff --git a/core/membership/service.go b/core/membership/service.go index fefef3373e..4ad232fe85 100644 --- a/core/membership/service.go +++ b/core/membership/service.go @@ -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 { diff --git a/core/userpat/service.go b/core/userpat/service.go index 6517236072..6643fab582 100644 --- a/core/userpat/service.go +++ b/core/userpat/service.go @@ -34,6 +34,8 @@ var supportedPATResourceTypes = []string{ schema.ProjectNamespace, } +const patRemovalPageSize = 1000 + type OrganizationService interface { GetRaw(ctx context.Context, id string) (organization.Organization, error) } @@ -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) @@ -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) } } diff --git a/core/userpat/service_test.go b/core/userpat/service_test.go index ba0a1efd97..3d463a4eaa 100644 --- a/core/userpat/service_test.go +++ b/core/userpat/service_test.go @@ -6,6 +6,7 @@ import ( "encoding/base64" "encoding/hex" "errors" + "fmt" "strings" "testing" "time" @@ -24,6 +25,8 @@ import ( "github.com/raystack/frontier/core/userpat/mocks" "github.com/raystack/frontier/core/userpat/models" "github.com/raystack/frontier/internal/bootstrap/schema" + pkgAuditRecord "github.com/raystack/frontier/pkg/auditrecord" + "github.com/raystack/salt/rql" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" ) @@ -2040,6 +2043,7 @@ func TestService_DeleteAllByOrg(t *testing.T) { auditRepo.EXPECT().Create(mock.Anything, mock.Anything). Run(func(_ context.Context, record auditmodels.AuditRecord) { calls = append(calls, "audit:"+record.Target.ID) + assert.NotContains(t, record.Target.Metadata, "reason") }). Return(auditmodels.AuditRecord{}, nil). Times(2) @@ -2174,6 +2178,109 @@ func TestService_DeleteAllByOrg(t *testing.T) { }) } +func TestService_OnOrgMemberRemoved(t *testing.T) { + newService := func(t *testing.T) (*userpat.Service, *mocks.Repository, *mocks.MembershipService, *mocks.AuditRecordRepository) { + t.Helper() + repo := mocks.NewRepository(t) + membershipSvc := mocks.NewMembershipService(t) + orgSvc := mocks.NewOrganizationService(t) + orgSvc.On("GetRaw", mock.Anything, mock.Anything). + Return(organization.Organization{ID: "org-1", Title: "Test Org"}, nil).Maybe() + auditRepo := mocks.NewAuditRecordRepository(t) + svc := userpat.NewService(slog.New(slog.NewTextHandler(io.Discard, nil)), repo, defaultConfig, + orgSvc, nil, membershipSvc, nil, auditRepo) + return svc, repo, membershipSvc, auditRepo + } + pageAt := func(offset int) any { + return mock.MatchedBy(func(q *rql.Query) bool { + return q.Offset == offset && q.Limit == 1000 + }) + } + livePAT := func(id string) models.PAT { + return models.PAT{ID: id, UserID: "user-1", OrgID: "org-1", Title: id, ExpiresAt: time.Now().Add(time.Hour)} + } + + t.Run("no-op when the user has no PATs in the org", func(t *testing.T) { + svc, repo, _, _ := newService(t) + repo.EXPECT().List(mock.Anything, "user-1", "org-1", pageAt(0)).Return(models.PATList{}, nil) + assert.NoError(t, svc.OnOrgMemberRemoved(context.Background(), "org-1", "user-1")) + }) + + t.Run("returns error when the list fails", func(t *testing.T) { + svc, repo, _, _ := newService(t) + repo.EXPECT().List(mock.Anything, "user-1", "org-1", pageAt(0)).Return(models.PATList{}, errors.New("db down")) + err := svc.OnOrgMemberRemoved(context.Background(), "org-1", "user-1") + assert.ErrorContains(t, err, "listing PATs for user in org") + assert.ErrorContains(t, err, "db down") + }) + + t.Run("revokes each PAT and records member_removed as the reason", func(t *testing.T) { + svc, repo, membershipSvc, auditRepo := newService(t) + pats := []models.PAT{livePAT("pat-1"), livePAT("pat-2")} + repo.EXPECT().List(mock.Anything, "user-1", "org-1", pageAt(0)).Return(models.PATList{PATs: pats}, nil) + for _, pat := range pats { + repo.EXPECT().Delete(mock.Anything, pat.ID).Return(nil).Once() + membershipSvc.EXPECT().RemoveAllPATPolicies(mock.Anything, pat.ID).Return(nil).Once() + } + var records []auditmodels.AuditRecord + auditRepo.EXPECT().Create(mock.Anything, mock.Anything). + Run(func(_ context.Context, record auditmodels.AuditRecord) { records = append(records, record) }). + Return(auditmodels.AuditRecord{}, nil). + Times(2) + + assert.NoError(t, svc.OnOrgMemberRemoved(context.Background(), "org-1", "user-1")) + assert.Len(t, records, 2) + for i, record := range records { + assert.Equal(t, pkgAuditRecord.PATRevokedEvent, record.Event) + assert.Equal(t, pats[i].ID, record.Target.ID) + assert.Equal(t, "member_removed", record.Target.Metadata["reason"]) + assert.Equal(t, "user-1", record.Target.Metadata["user_id"]) + } + }) + + t.Run("reads every page before revoking", func(t *testing.T) { + svc, repo, membershipSvc, auditRepo := newService(t) + firstPage := make([]models.PAT, 1000) + for i := range firstPage { + firstPage[i] = livePAT(fmt.Sprintf("pat-%d", i)) + } + lastPAT := livePAT("pat-last") + + var calls []string + recordList := func(_ context.Context, _, _ string, q *rql.Query) { + calls = append(calls, fmt.Sprintf("list:%d", q.Offset)) + } + repo.EXPECT().List(mock.Anything, "user-1", "org-1", pageAt(0)). + Run(recordList).Return(models.PATList{PATs: firstPage}, nil).Once() + repo.EXPECT().List(mock.Anything, "user-1", "org-1", pageAt(1000)). + Run(recordList).Return(models.PATList{PATs: []models.PAT{lastPAT}}, nil).Once() + repo.EXPECT().Delete(mock.Anything, mock.Anything). + Run(func(_ context.Context, id string) { calls = append(calls, "row:"+id) }). + Return(nil).Times(1001) + membershipSvc.EXPECT().RemoveAllPATPolicies(mock.Anything, mock.Anything).Return(nil).Times(1001) + auditRepo.EXPECT().Create(mock.Anything, mock.Anything).Return(auditmodels.AuditRecord{}, nil).Times(1001) + + assert.NoError(t, svc.OnOrgMemberRemoved(context.Background(), "org-1", "user-1")) + want := []string{"list:0", "list:1000"} + for _, pat := range firstPage { + want = append(want, "row:"+pat.ID) + } + want = append(want, "row:"+lastPAT.ID) + assert.Equal(t, want, calls) + }) + + t.Run("stops at the first PAT it cannot revoke", func(t *testing.T) { + svc, repo, _, _ := newService(t) + pats := []models.PAT{livePAT("pat-bad"), livePAT("pat-2")} + repo.EXPECT().List(mock.Anything, "user-1", "org-1", pageAt(0)).Return(models.PATList{PATs: pats}, nil) + repo.EXPECT().Delete(mock.Anything, "pat-bad").Return(errors.New("db down")) + + err := svc.OnOrgMemberRemoved(context.Background(), "org-1", "user-1") + assert.ErrorContains(t, err, "deleting PAT[pat-bad]") + assert.ErrorContains(t, err, "db down") + }) +} + func TestService_Update(t *testing.T) { testPAT := models.PAT{ ID: "pat-1", diff --git a/test/e2e/regression/pat_test.go b/test/e2e/regression/pat_test.go index 07aaf5be72..af5c0ad826 100644 --- a/test/e2e/regression/pat_test.go +++ b/test/e2e/regression/pat_test.go @@ -12,6 +12,7 @@ import ( "github.com/raystack/frontier/core/userpat" "github.com/raystack/frontier/internal/bootstrap/schema" "github.com/raystack/frontier/pkg/server" + "github.com/raystack/frontier/pkg/utils" "github.com/raystack/frontier/config" "github.com/raystack/frontier/pkg/logger" @@ -861,6 +862,113 @@ func (s *PATRegressionTestSuite) TestPATOfDeletedOrgStopsAuthenticating() { }) } +func (s *PATRegressionTestSuite) TestPATOfRemovedOrgMemberIsRevoked() { + ctxAdmin := testbench.ContextWithAuth(context.Background(), s.adminCookie) + ownerID, ctxOwner := signUpUser(s.T(), s.testBench.Client, "pat-removed-member-owner@raystack.org") + orgResp, err := s.testBench.Client.CreateOrganization(ctxOwner, connect.NewRequest(&frontierv1beta1.CreateOrganizationRequest{ + Body: &frontierv1beta1.OrganizationRequestBody{Name: "org-pat-removed-member"}, + })) + s.Require().NoError(err) + orgID := orgResp.Msg.GetOrganization().GetId() + memberID, ctxMember := signUpUser(s.T(), s.testBench.Client, "pat-removed-member@raystack.org") + + addMember := func() { + resp, err := s.testBench.AdminClient.AddOrganizationMembers(ctxAdmin, connect.NewRequest(&frontierv1beta1.AddOrganizationMembersRequest{ + OrgId: orgID, + Members: []*frontierv1beta1.OrgMemberEntry{{UserId: memberID, RoleId: s.roleID(schema.RoleOrganizationViewer)}}, + })) + requireAddOrgMembersSuccess(s.T(), resp, err) + } + orgPATIDs := func() []string { + resp, err := s.testBench.AdminClient.SearchOrganizationPATs(ctxAdmin, connect.NewRequest(&frontierv1beta1.SearchOrganizationPATsRequest{OrgId: orgID})) + s.Require().NoError(err) + return utils.Map(resp.Msg.GetOrganizationPats(), func(p *frontierv1beta1.SearchOrganizationPATsResponse_OrganizationPAT) string { + return p.GetId() + }) + } + + addMember() + patID, patToken := s.createPAT(ctxMember, orgID, "pat-of-removed-member", []*frontierv1beta1.PATScope{ + {RoleId: s.roleID(schema.RoleOrganizationViewer), ResourceType: schema.OrganizationNamespace}, + }) + patCtx := getPATCtx(patToken) + s.Require().True(s.checkPermission(patCtx, schema.OrganizationNamespace, orgID, schema.GetPermission)) + s.Require().True(s.patHasPermissionInSpiceDB(ctxAdmin, patID, schema.OrganizationNamespace, orgID, schema.GetPermission)) + s.Require().Contains(orgPATIDs(), patID) + + policiesResp, err := s.testBench.Client.ListPolicies(ctxAdmin, connect.NewRequest(&frontierv1beta1.ListPoliciesRequest{UserId: patID})) + s.Require().NoError(err) + patPoliciesBeforeRemoval := policiesResp.Msg.GetPolicies() + s.Require().Len(patPoliciesBeforeRemoval, 1) + + _, err = s.testBench.Client.RemoveOrganizationMember(ctxOwner, connect.NewRequest(&frontierv1beta1.RemoveOrganizationMemberRequest{ + OrgId: orgID, + PrincipalId: memberID, + PrincipalType: schema.UserPrincipal, + })) + s.Require().NoError(err) + + s.Run("the token is rejected", func() { + _, err := s.testBench.Client.GetCurrentUser(patCtx, connect.NewRequest(&frontierv1beta1.GetCurrentUserRequest{})) + s.Require().Error(err) + s.Assert().Equal(connect.CodeUnauthenticated, connect.CodeOf(err)) + }) + + s.Run("the token is hidden from its owner", func() { + _, err := s.testBench.Client.GetCurrentUserPAT(ctxMember, connect.NewRequest(&frontierv1beta1.GetCurrentUserPATRequest{Id: patID})) + s.Require().Error(err) + s.Assert().Equal(connect.CodeNotFound, connect.CodeOf(err)) + }) + + s.Run("no live policy is held by the token", func() { + policiesResp, err := s.testBench.Client.ListPolicies(ctxAdmin, connect.NewRequest(&frontierv1beta1.ListPoliciesRequest{UserId: patID})) + s.Require().NoError(err) + s.Assert().Empty(policiesResp.Msg.GetPolicies()) + for _, policyBeforeRemoval := range patPoliciesBeforeRemoval { + _, err := s.testBench.Client.GetPolicy(ctxAdmin, connect.NewRequest(&frontierv1beta1.GetPolicyRequest{Id: policyBeforeRemoval.GetId()})) + s.Require().Error(err) + s.Assert().Equal(connect.CodeNotFound, connect.CodeOf(err)) + } + }) + + s.Run("the org token list no longer shows it", func() { + s.Assert().NotContains(orgPATIDs(), patID) + }) + + s.Run("the audit record names the remover and the reason", func() { + recordsResp, err := s.testBench.AdminClient.ListAuditRecords(ctxAdmin, connect.NewRequest(&frontierv1beta1.ListAuditRecordsRequest{ + Query: &frontierv1beta1.RQLRequest{ + Filters: []*frontierv1beta1.RQLFilter{{ + Name: "target_id", + Operator: "eq", + Value: &frontierv1beta1.RQLFilter_StringValue{StringValue: patID}, + }}, + }, + })) + s.Require().NoError(err) + var revokedRecords []*frontierv1beta1.AuditRecord + for _, record := range recordsResp.Msg.GetAuditRecords() { + if record.GetEvent() == "pat.revoked" { + revokedRecords = append(revokedRecords, record) + } + } + s.Require().Len(revokedRecords, 1) + s.Assert().Equal(ownerID, revokedRecords[0].GetActor().GetId()) + s.Assert().Equal(orgID, revokedRecords[0].GetResource().GetId()) + targetMetadata := revokedRecords[0].GetTarget().GetMetadata().AsMap() + s.Assert().Equal("member_removed", targetMetadata["reason"]) + s.Assert().Equal(memberID, targetMetadata["user_id"]) + }) + + s.Run("adding the member back does not bring the token back", func() { + addMember() + _, err := s.testBench.Client.GetCurrentUser(patCtx, connect.NewRequest(&frontierv1beta1.GetCurrentUserRequest{})) + s.Require().Error(err) + s.Assert().Equal(connect.CodeUnauthenticated, connect.CodeOf(err)) + s.Assert().False(s.patHasPermissionInSpiceDB(ctxAdmin, patID, schema.OrganizationNamespace, orgID, schema.GetPermission)) + }) +} + func (s *PATRegressionTestSuite) TestPATCannotCheckOrDeleteItsOwnersAccount() { _, userCtx := signUpUser(s.T(), s.testBench.Client, "pat-account-delete@raystack.org") orgResp, err := s.testBench.Client.CreateOrganization(userCtx, connect.NewRequest(&frontierv1beta1.CreateOrganizationRequest{ From c0aec7be00a6e6c61df6325c57c6c98a3c706d2a Mon Sep 17 00:00:00 2001 From: aman Date: Wed, 11 Nov 2026 20:34:13 +0530 Subject: [PATCH 2/3] fix(membership): revoke leftover PATs when the member is already removed RemoveOrganizationMember now runs the PAT step on ErrNotMember too and still returns ErrNotMember, so removing the user again finishes a revoke that failed. --- core/membership/org.go | 10 +++++++--- core/membership/org_test.go | 12 +++++++++++- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/core/membership/org.go b/core/membership/org.go index f6a4f7119f..90b0ac5d92 100644 --- a/core/membership/org.go +++ b/core/membership/org.go @@ -205,10 +205,14 @@ 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 { - if err := s.removeOrganizationMember(ctx, orgID, principalID, principalType, false); err != nil { - return err + 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 { + return patErr } - return s.revokeMemberPATs(ctx, orgID, principalID, principalType) + return removeErr } // RemoveOrganizationMemberOnDelete is RemoveOrganizationMember for deleting a diff --git a/core/membership/org_test.go b/core/membership/org_test.go index 48014dcfec..c38de21e86 100644 --- a/core/membership/org_test.go +++ b/core/membership/org_test.go @@ -777,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) { From ddfe0f438081e83b4bad4a9a5d735919af7da49c Mon Sep 17 00:00:00 2001 From: aman Date: Wed, 11 Nov 2026 20:34:13 +0530 Subject: [PATCH 3/3] test(e2e): cover a project-scoped PAT on member removal The removed member now holds an org-scoped and a project-scoped PAT, and every check runs for both. --- test/e2e/regression/pat_test.go | 146 ++++++++++++++++++++------------ 1 file changed, 92 insertions(+), 54 deletions(-) diff --git a/test/e2e/regression/pat_test.go b/test/e2e/regression/pat_test.go index af5c0ad826..f9a9406953 100644 --- a/test/e2e/regression/pat_test.go +++ b/test/e2e/regression/pat_test.go @@ -870,6 +870,11 @@ func (s *PATRegressionTestSuite) TestPATOfRemovedOrgMemberIsRevoked() { })) s.Require().NoError(err) orgID := orgResp.Msg.GetOrganization().GetId() + projectResp, err := s.testBench.Client.CreateProject(ctxOwner, connect.NewRequest(&frontierv1beta1.CreateProjectRequest{ + Body: &frontierv1beta1.ProjectRequestBody{Name: "pat-removed-member-project", OrgId: orgID}, + })) + s.Require().NoError(err) + projectID := projectResp.Msg.GetProject().GetId() memberID, ctxMember := signUpUser(s.T(), s.testBench.Client, "pat-removed-member@raystack.org") addMember := func() { @@ -878,6 +883,14 @@ func (s *PATRegressionTestSuite) TestPATOfRemovedOrgMemberIsRevoked() { Members: []*frontierv1beta1.OrgMemberEntry{{UserId: memberID, RoleId: s.roleID(schema.RoleOrganizationViewer)}}, })) requireAddOrgMembersSuccess(s.T(), resp, err) + _, err = s.testBench.Client.CreatePolicy(ctxAdmin, connect.NewRequest(&frontierv1beta1.CreatePolicyRequest{ + Body: &frontierv1beta1.PolicyRequestBody{ + RoleId: schema.RoleProjectViewer, + Resource: schema.JoinNamespaceAndResourceID(schema.ProjectNamespace, projectID), + Principal: schema.JoinNamespaceAndResourceID(schema.UserPrincipal, memberID), + }, + })) + s.Require().NoError(err) } orgPATIDs := func() []string { resp, err := s.testBench.AdminClient.SearchOrganizationPATs(ctxAdmin, connect.NewRequest(&frontierv1beta1.SearchOrganizationPATsRequest{OrgId: orgID})) @@ -888,18 +901,32 @@ func (s *PATRegressionTestSuite) TestPATOfRemovedOrgMemberIsRevoked() { } addMember() - patID, patToken := s.createPAT(ctxMember, orgID, "pat-of-removed-member", []*frontierv1beta1.PATScope{ + orgPATID, orgPATToken := s.createPAT(ctxMember, orgID, "pat-of-removed-member", []*frontierv1beta1.PATScope{ {RoleId: s.roleID(schema.RoleOrganizationViewer), ResourceType: schema.OrganizationNamespace}, }) - patCtx := getPATCtx(patToken) - s.Require().True(s.checkPermission(patCtx, schema.OrganizationNamespace, orgID, schema.GetPermission)) - s.Require().True(s.patHasPermissionInSpiceDB(ctxAdmin, patID, schema.OrganizationNamespace, orgID, schema.GetPermission)) - s.Require().Contains(orgPATIDs(), patID) - - policiesResp, err := s.testBench.Client.ListPolicies(ctxAdmin, connect.NewRequest(&frontierv1beta1.ListPoliciesRequest{UserId: patID})) - s.Require().NoError(err) - patPoliciesBeforeRemoval := policiesResp.Msg.GetPolicies() - s.Require().Len(patPoliciesBeforeRemoval, 1) + projectPATID, projectPATToken := s.createPAT(ctxMember, orgID, "pat-of-removed-member-project", []*frontierv1beta1.PATScope{ + {RoleId: s.roleID(schema.RoleProjectViewer), ResourceType: schema.ProjectNamespace, ResourceIds: []string{projectID}}, + }) + pats := []struct { + name string + id string + ctx context.Context + namespace string + resourceID string + policies []*frontierv1beta1.Policy + }{ + {name: "org PAT", id: orgPATID, ctx: getPATCtx(orgPATToken), namespace: schema.OrganizationNamespace, resourceID: orgID}, + {name: "project PAT", id: projectPATID, ctx: getPATCtx(projectPATToken), namespace: schema.ProjectNamespace, resourceID: projectID}, + } + for i, pat := range pats { + s.Require().Truef(s.checkPermission(pat.ctx, pat.namespace, pat.resourceID, schema.GetPermission), "%s works before the removal", pat.name) + s.Require().Truef(s.patHasPermissionInSpiceDB(ctxAdmin, pat.id, pat.namespace, pat.resourceID, schema.GetPermission), "%s has access in SpiceDB before the removal", pat.name) + s.Require().Contains(orgPATIDs(), pat.id) + policiesResp, err := s.testBench.Client.ListPolicies(ctxAdmin, connect.NewRequest(&frontierv1beta1.ListPoliciesRequest{UserId: pat.id})) + s.Require().NoError(err) + s.Require().Lenf(policiesResp.Msg.GetPolicies(), 1, "%s policies before the removal", pat.name) + pats[i].policies = policiesResp.Msg.GetPolicies() + } _, err = s.testBench.Client.RemoveOrganizationMember(ctxOwner, connect.NewRequest(&frontierv1beta1.RemoveOrganizationMemberRequest{ OrgId: orgID, @@ -908,64 +935,75 @@ func (s *PATRegressionTestSuite) TestPATOfRemovedOrgMemberIsRevoked() { })) s.Require().NoError(err) - s.Run("the token is rejected", func() { - _, err := s.testBench.Client.GetCurrentUser(patCtx, connect.NewRequest(&frontierv1beta1.GetCurrentUserRequest{})) - s.Require().Error(err) - s.Assert().Equal(connect.CodeUnauthenticated, connect.CodeOf(err)) + s.Run("the tokens are rejected", func() { + for _, pat := range pats { + _, err := s.testBench.Client.GetCurrentUser(pat.ctx, connect.NewRequest(&frontierv1beta1.GetCurrentUserRequest{})) + s.Assert().Equal(connect.CodeUnauthenticated, connect.CodeOf(err), pat.name) + } }) - s.Run("the token is hidden from its owner", func() { - _, err := s.testBench.Client.GetCurrentUserPAT(ctxMember, connect.NewRequest(&frontierv1beta1.GetCurrentUserPATRequest{Id: patID})) - s.Require().Error(err) - s.Assert().Equal(connect.CodeNotFound, connect.CodeOf(err)) + s.Run("the tokens are hidden from their owner", func() { + for _, pat := range pats { + _, err := s.testBench.Client.GetCurrentUserPAT(ctxMember, connect.NewRequest(&frontierv1beta1.GetCurrentUserPATRequest{Id: pat.id})) + s.Assert().Equal(connect.CodeNotFound, connect.CodeOf(err), pat.name) + } }) - s.Run("no live policy is held by the token", func() { - policiesResp, err := s.testBench.Client.ListPolicies(ctxAdmin, connect.NewRequest(&frontierv1beta1.ListPoliciesRequest{UserId: patID})) - s.Require().NoError(err) - s.Assert().Empty(policiesResp.Msg.GetPolicies()) - for _, policyBeforeRemoval := range patPoliciesBeforeRemoval { - _, err := s.testBench.Client.GetPolicy(ctxAdmin, connect.NewRequest(&frontierv1beta1.GetPolicyRequest{Id: policyBeforeRemoval.GetId()})) - s.Require().Error(err) - s.Assert().Equal(connect.CodeNotFound, connect.CodeOf(err)) + s.Run("no live policy is held by the tokens", func() { + for _, pat := range pats { + policiesResp, err := s.testBench.Client.ListPolicies(ctxAdmin, connect.NewRequest(&frontierv1beta1.ListPoliciesRequest{UserId: pat.id})) + s.Require().NoError(err) + s.Assert().Empty(policiesResp.Msg.GetPolicies(), pat.name) + for _, policyBeforeRemoval := range pat.policies { + _, err := s.testBench.Client.GetPolicy(ctxAdmin, connect.NewRequest(&frontierv1beta1.GetPolicyRequest{Id: policyBeforeRemoval.GetId()})) + s.Assert().Equal(connect.CodeNotFound, connect.CodeOf(err), pat.name) + } } }) - s.Run("the org token list no longer shows it", func() { - s.Assert().NotContains(orgPATIDs(), patID) + s.Run("the org token list no longer shows them", func() { + ids := orgPATIDs() + for _, pat := range pats { + s.Assert().NotContains(ids, pat.id, pat.name) + } }) - s.Run("the audit record names the remover and the reason", func() { - recordsResp, err := s.testBench.AdminClient.ListAuditRecords(ctxAdmin, connect.NewRequest(&frontierv1beta1.ListAuditRecordsRequest{ - Query: &frontierv1beta1.RQLRequest{ - Filters: []*frontierv1beta1.RQLFilter{{ - Name: "target_id", - Operator: "eq", - Value: &frontierv1beta1.RQLFilter_StringValue{StringValue: patID}, - }}, - }, - })) - s.Require().NoError(err) - var revokedRecords []*frontierv1beta1.AuditRecord - for _, record := range recordsResp.Msg.GetAuditRecords() { - if record.GetEvent() == "pat.revoked" { - revokedRecords = append(revokedRecords, record) + s.Run("each audit record names the remover and the reason", func() { + for _, pat := range pats { + recordsResp, err := s.testBench.AdminClient.ListAuditRecords(ctxAdmin, connect.NewRequest(&frontierv1beta1.ListAuditRecordsRequest{ + Query: &frontierv1beta1.RQLRequest{ + Filters: []*frontierv1beta1.RQLFilter{{ + Name: "target_id", + Operator: "eq", + Value: &frontierv1beta1.RQLFilter_StringValue{StringValue: pat.id}, + }}, + }, + })) + s.Require().NoError(err) + var revokedRecords []*frontierv1beta1.AuditRecord + for _, record := range recordsResp.Msg.GetAuditRecords() { + if record.GetEvent() == "pat.revoked" { + revokedRecords = append(revokedRecords, record) + } + } + if !s.Assert().Len(revokedRecords, 1, pat.name) { + continue } + s.Assert().Equal(ownerID, revokedRecords[0].GetActor().GetId(), pat.name) + s.Assert().Equal(orgID, revokedRecords[0].GetResource().GetId(), pat.name) + targetMetadata := revokedRecords[0].GetTarget().GetMetadata().AsMap() + s.Assert().Equal("member_removed", targetMetadata["reason"], pat.name) + s.Assert().Equal(memberID, targetMetadata["user_id"], pat.name) } - s.Require().Len(revokedRecords, 1) - s.Assert().Equal(ownerID, revokedRecords[0].GetActor().GetId()) - s.Assert().Equal(orgID, revokedRecords[0].GetResource().GetId()) - targetMetadata := revokedRecords[0].GetTarget().GetMetadata().AsMap() - s.Assert().Equal("member_removed", targetMetadata["reason"]) - s.Assert().Equal(memberID, targetMetadata["user_id"]) }) - s.Run("adding the member back does not bring the token back", func() { + s.Run("adding the member back does not bring the tokens back", func() { addMember() - _, err := s.testBench.Client.GetCurrentUser(patCtx, connect.NewRequest(&frontierv1beta1.GetCurrentUserRequest{})) - s.Require().Error(err) - s.Assert().Equal(connect.CodeUnauthenticated, connect.CodeOf(err)) - s.Assert().False(s.patHasPermissionInSpiceDB(ctxAdmin, patID, schema.OrganizationNamespace, orgID, schema.GetPermission)) + for _, pat := range pats { + _, err := s.testBench.Client.GetCurrentUser(pat.ctx, connect.NewRequest(&frontierv1beta1.GetCurrentUserRequest{})) + s.Assert().Equal(connect.CodeUnauthenticated, connect.CodeOf(err), pat.name) + s.Assert().False(s.patHasPermissionInSpiceDB(ctxAdmin, pat.id, pat.namespace, pat.resourceID, schema.GetPermission), pat.name) + } }) }