diff --git a/core/membership/mocks/user_pat_service.go b/core/membership/mocks/user_pat_service.go index 0987c7e57..8b2a6eae8 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 220a024e7..90b0ac5d9 100644 --- a/core/membership/org.go +++ b/core/membership/org.go @@ -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 { + 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. func (s *Service) RemoveOrganizationMemberOnDelete(ctx context.Context, orgID, principalID, principalType string) error { return s.removeOrganizationMember(ctx, orgID, principalID, principalType, true) } @@ -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) { diff --git a/core/membership/org_test.go b/core/membership/org_test.go index 9de68b50d..c38de21e8 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 @@ -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) { @@ -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, }, @@ -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, }, @@ -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, }, @@ -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, }, @@ -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 { @@ -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 { @@ -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 == "" { @@ -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 { diff --git a/core/membership/service.go b/core/membership/service.go index fefef3373..4ad232fe8 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 651723607..6643fab58 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 ba0a1efd9..3d463a4ea 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 07aaf5be7..f9a940695 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,151 @@ 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() + 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() { + 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) + _, 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})) + s.Require().NoError(err) + return utils.Map(resp.Msg.GetOrganizationPats(), func(p *frontierv1beta1.SearchOrganizationPATsResponse_OrganizationPAT) string { + return p.GetId() + }) + } + + addMember() + orgPATID, orgPATToken := s.createPAT(ctxMember, orgID, "pat-of-removed-member", []*frontierv1beta1.PATScope{ + {RoleId: s.roleID(schema.RoleOrganizationViewer), ResourceType: schema.OrganizationNamespace}, + }) + 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, + PrincipalId: memberID, + PrincipalType: schema.UserPrincipal, + })) + s.Require().NoError(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 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 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 them", func() { + ids := orgPATIDs() + for _, pat := range pats { + s.Assert().NotContains(ids, pat.id, pat.name) + } + }) + + 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.Run("adding the member back does not bring the tokens back", func() { + addMember() + 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) + } + }) +} + 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{