From 895d3677ce69eeea3f55ca05b751a5a79728b8d0 Mon Sep 17 00:00:00 2001 From: aman Date: Tue, 27 Oct 2026 15:01:57 +0530 Subject: [PATCH 1/3] feat(api): users can check and delete their own account CheckCurrentUserDelete reports what deleting the logged-in user's account would do and what blocks it, without changing anything. DeleteCurrentUser runs the same check, refuses with every blocker, and otherwise deletes the account like the admin DeleteUser, then clears the session cookie. Both refuse service users and PATs. A user delete now leaves each org with the guarded member removal, which also works on a disabled org, so it can no longer remove an org's last owner, and force removal is removed. Anything removed by someone else during the delete counts as done. --- core/deleter/deleter.go | 5 +- core/deleter/mocks/membership_service.go | 98 ++++---- core/deleter/service.go | 53 +++-- core/deleter/service_test.go | 84 +++---- core/membership/org.go | 46 ++-- core/membership/org_test.go | 68 +++--- core/resource/service.go | 2 +- core/resource/service_test.go | 14 ++ internal/api/v1beta1connect/authenticate.go | 13 ++ internal/api/v1beta1connect/interfaces.go | 1 + .../v1beta1connect/mocks/cascade_deleter.go | 57 +++++ internal/api/v1beta1connect/user.go | 78 ++++++- internal/api/v1beta1connect/user_pat.go | 6 +- internal/api/v1beta1connect/user_test.go | 191 ++++++++++++++++ .../connect_interceptors/authorization.go | 4 + test/e2e/regression/api_test.go | 213 ++++++++++++++++++ test/e2e/regression/pat_test.go | 37 +++ 17 files changed, 779 insertions(+), 191 deletions(-) diff --git a/core/deleter/deleter.go b/core/deleter/deleter.go index e63abf68e1..e004570781 100644 --- a/core/deleter/deleter.go +++ b/core/deleter/deleter.go @@ -89,9 +89,8 @@ func labelWithOrg(orgName string, b Blocker) Blocker { return b } -// BlockedError carries every blocker the up-front check found, so the -// caller gets one checklist instead of discovering blockers one retry at -// a time. +// BlockedError carries the blockers that stop a delete, so the caller gets +// one checklist instead of discovering blockers one retry at a time. type BlockedError struct { OrgID string ProjectID string diff --git a/core/deleter/mocks/membership_service.go b/core/deleter/mocks/membership_service.go index c99693ae96..406890b0ca 100644 --- a/core/deleter/mocks/membership_service.go +++ b/core/deleter/mocks/membership_service.go @@ -24,55 +24,6 @@ func (_m *MembershipService) EXPECT() *MembershipService_Expecter { return &MembershipService_Expecter{mock: &_m.Mock} } -// ForceRemoveOrganizationMember provides a mock function with given fields: ctx, orgID, principalID, principalType -func (_m *MembershipService) ForceRemoveOrganizationMember(ctx context.Context, orgID string, principalID string, principalType string) error { - ret := _m.Called(ctx, orgID, principalID, principalType) - - if len(ret) == 0 { - panic("no return value specified for ForceRemoveOrganizationMember") - } - - var r0 error - if rf, ok := ret.Get(0).(func(context.Context, string, string, string) error); ok { - r0 = rf(ctx, orgID, principalID, principalType) - } else { - r0 = ret.Error(0) - } - - return r0 -} - -// MembershipService_ForceRemoveOrganizationMember_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'ForceRemoveOrganizationMember' -type MembershipService_ForceRemoveOrganizationMember_Call struct { - *mock.Call -} - -// ForceRemoveOrganizationMember is a helper method to define mock.On call -// - ctx context.Context -// - orgID string -// - principalID string -// - principalType string -func (_e *MembershipService_Expecter) ForceRemoveOrganizationMember(ctx interface{}, orgID interface{}, principalID interface{}, principalType interface{}) *MembershipService_ForceRemoveOrganizationMember_Call { - return &MembershipService_ForceRemoveOrganizationMember_Call{Call: _e.mock.On("ForceRemoveOrganizationMember", ctx, orgID, principalID, principalType)} -} - -func (_c *MembershipService_ForceRemoveOrganizationMember_Call) Run(run func(ctx context.Context, orgID string, principalID string, principalType string)) *MembershipService_ForceRemoveOrganizationMember_Call { - _c.Call.Run(func(args mock.Arguments) { - run(args[0].(context.Context), args[1].(string), args[2].(string), args[3].(string)) - }) - return _c -} - -func (_c *MembershipService_ForceRemoveOrganizationMember_Call) Return(_a0 error) *MembershipService_ForceRemoveOrganizationMember_Call { - _c.Call.Return(_a0) - return _c -} - -func (_c *MembershipService_ForceRemoveOrganizationMember_Call) RunAndReturn(run func(context.Context, string, string, string) error) *MembershipService_ForceRemoveOrganizationMember_Call { - _c.Call.Return(run) - return _c -} - // ListPrincipalsByResource provides a mock function with given fields: ctx, resourceID, resourceType, filter func (_m *MembershipService) ListPrincipalsByResource(ctx context.Context, resourceID string, resourceType string, filter membership.MemberFilter) ([]membership.Member, error) { ret := _m.Called(ctx, resourceID, resourceType, filter) @@ -242,6 +193,55 @@ func (_c *MembershipService_OnGroupDeleted_Call) RunAndReturn(run func(context.C return _c } +// RemoveOrganizationMemberOnDelete provides a mock function with given fields: ctx, orgID, principalID, principalType +func (_m *MembershipService) RemoveOrganizationMemberOnDelete(ctx context.Context, orgID string, principalID string, principalType string) error { + ret := _m.Called(ctx, orgID, principalID, principalType) + + if len(ret) == 0 { + panic("no return value specified for RemoveOrganizationMemberOnDelete") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string, string, string) error); ok { + r0 = rf(ctx, orgID, principalID, principalType) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// MembershipService_RemoveOrganizationMemberOnDelete_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'RemoveOrganizationMemberOnDelete' +type MembershipService_RemoveOrganizationMemberOnDelete_Call struct { + *mock.Call +} + +// RemoveOrganizationMemberOnDelete is a helper method to define mock.On call +// - ctx context.Context +// - orgID string +// - principalID string +// - principalType string +func (_e *MembershipService_Expecter) RemoveOrganizationMemberOnDelete(ctx interface{}, orgID interface{}, principalID interface{}, principalType interface{}) *MembershipService_RemoveOrganizationMemberOnDelete_Call { + return &MembershipService_RemoveOrganizationMemberOnDelete_Call{Call: _e.mock.On("RemoveOrganizationMemberOnDelete", ctx, orgID, principalID, principalType)} +} + +func (_c *MembershipService_RemoveOrganizationMemberOnDelete_Call) Run(run func(ctx context.Context, orgID string, principalID string, principalType string)) *MembershipService_RemoveOrganizationMemberOnDelete_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(string), args[2].(string), args[3].(string)) + }) + return _c +} + +func (_c *MembershipService_RemoveOrganizationMemberOnDelete_Call) Return(_a0 error) *MembershipService_RemoveOrganizationMemberOnDelete_Call { + _c.Call.Return(_a0) + return _c +} + +func (_c *MembershipService_RemoveOrganizationMemberOnDelete_Call) RunAndReturn(run func(context.Context, string, string, string) error) *MembershipService_RemoveOrganizationMemberOnDelete_Call { + _c.Call.Return(run) + return _c +} + // RemovePrincipalPolicies provides a mock function with given fields: ctx, principalID, principalType func (_m *MembershipService) RemovePrincipalPolicies(ctx context.Context, principalID string, principalType string) error { ret := _m.Called(ctx, principalID, principalType) diff --git a/core/deleter/service.go b/core/deleter/service.go index dd35825cca..0fbdfab22c 100644 --- a/core/deleter/service.go +++ b/core/deleter/service.go @@ -84,7 +84,7 @@ type MembershipService interface { OnGroupDeleted(ctx context.Context, groupID string) error ListResourcesByPrincipal(ctx context.Context, principal authenticate.Principal, resourceType string, filter membership.ResourceFilter) ([]string, error) ListPrincipalsByResource(ctx context.Context, resourceID, resourceType string, filter membership.MemberFilter) ([]membership.Member, error) - ForceRemoveOrganizationMember(ctx context.Context, orgID, principalID, principalType string) error + RemoveOrganizationMemberOnDelete(ctx context.Context, orgID, principalID, principalType string) error RemovePrincipalPolicies(ctx context.Context, principalID, principalType string) error } @@ -542,21 +542,6 @@ func (d Service) deleteCustomers(ctx context.Context, id string, customers []cus return nil } -// RemoveUsersFromOrg removes users from an organization as members. The policy -// and relation cleanup — org, project, group, and custom resource — is -// delegated to membership.ForceRemoveOrganizationMember. It uses the force -// variant because a deletion cascade must succeed even when the user is the -// org's last owner. -func (d Service) RemoveUsersFromOrg(ctx context.Context, orgID string, userIDs []string) error { - var errs error - for _, userID := range userIDs { - if memberErr := d.membershipService.ForceRemoveOrganizationMember(ctx, orgID, userID, schema.UserPrincipal); memberErr != nil { - errs = errors.Join(errs, memberErr) - } - } - return errs -} - func (d Service) CheckUserDelete(ctx context.Context, userID string) (UserDeleteCheck, error) { check, err := d.checkUser(ctx, userID, false) if err != nil { @@ -626,18 +611,22 @@ func (d Service) checkUserOrg(ctx context.Context, userID, orgID string, fromPro toLeave.RoleTitle = self.Roles[0].Title } if isOrgOwner(*self) && otherOwners == 0 { - toLeave.Blockers = []Blocker{{ - Type: BlockerSoleOwnerWithMembers, - Subject: org.ID, - SubjectType: SubjectOrganization, - Message: fmt.Sprintf("organization[%s] has other members and this user is its only owner: make another member an owner or remove the other members, then retry the delete", org.Name), - }} + toLeave.Blockers = []Blocker{soleOwnerBlocker(org)} } check.OrganizationsToLeave = append(check.OrganizationsToLeave, toLeave) } return nil } +func soleOwnerBlocker(org organization.Organization) Blocker { + return Blocker{ + Type: BlockerSoleOwnerWithMembers, + Subject: org.ID, + SubjectType: SubjectOrganization, + Message: fmt.Sprintf("organization[%s] has other members and this user is its only owner: make another member an owner or remove the other members, then retry the delete", org.Name), + } +} + func (d Service) existingOrgUsers(ctx context.Context, orgID string) ([]membership.Member, error) { members, err := d.membershipService.ListPrincipalsByResource(ctx, orgID, schema.OrganizationNamespace, membership.MemberFilter{ PrincipalType: schema.UserPrincipal, @@ -660,8 +649,8 @@ func isOrgOwner(m membership.Member) bool { return slices.ContainsFunc(m.Roles, func(r role.Role) bool { return r.Name == schema.RoleOrganizationOwner }) } -// DeleteUser visits every org the user has a policy on (disabled orgs too), -// otherwise userService.Delete would leave orphan policy rows behind. +// DeleteUser stops on any blocker the check finds. Otherwise it deletes the orgs +// where the user is the only member and leaves the others, disabled orgs too. func (d Service) DeleteUser(ctx context.Context, userID string) error { check, err := d.checkUser(ctx, userID, true) if err != nil { @@ -676,7 +665,7 @@ func (d Service) DeleteUser(ctx context.Context, userID string) error { } } for _, o := range check.OrganizationsToLeave { - if err := d.RemoveUsersFromOrg(ctx, o.Organization.ID, []string{userID}); err != nil { + if err := d.leaveOrg(ctx, userID, o.Organization); err != nil { return fmt.Errorf("failed to delete user from org[%s]: %w", o.Organization.ID, err) } } @@ -689,6 +678,20 @@ func (d Service) DeleteUser(ctx context.Context, userID string) error { return d.userService.Delete(ctx, userID) } +// leaveOrg runs after the check, and the org can change in between. A user who +// is no longer a member, or an org that is gone, counts as done, and a user who +// is now the last owner blocks the delete. +func (d Service) leaveOrg(ctx context.Context, userID string, org organization.Organization) error { + err := d.membershipService.RemoveOrganizationMemberOnDelete(ctx, org.ID, userID, schema.UserPrincipal) + switch { + case errors.Is(err, membership.ErrNotMember), errors.Is(err, organization.ErrNotExist): + return nil + case errors.Is(err, membership.ErrLastOwnerRole): + return &BlockedError{UserID: userID, Blockers: []Blocker{soleOwnerBlocker(org)}} + } + return err +} + func (d Service) listOrgProjects(ctx context.Context, orgID string) ([]project.Project, error) { return d.projService.List(ctx, project.Filter{OrgID: orgID, IncludeDisabled: true}) } diff --git a/core/deleter/service_test.go b/core/deleter/service_test.go index eba56fe423..39cb83dd64 100644 --- a/core/deleter/service_test.go +++ b/core/deleter/service_test.go @@ -1790,43 +1790,6 @@ func TestDeleteCustomers(t *testing.T) { }) } -func TestRemoveUsersFromOrg(t *testing.T) { - const orgID = "org-1" - const userID = "user-1" - - t.Run("delegates the whole removal to the membership cascade", func(t *testing.T) { - m := newMocks(t) - - // org, project, group, and custom-resource cleanup all happen inside the - // cascade — strict mocks fail if the deleter touches them itself - m.mbrSvc.EXPECT().ForceRemoveOrganizationMember(mock.Anything, orgID, userID, schema.UserPrincipal).Return(nil) - - err := m.build().RemoveUsersFromOrg(context.Background(), orgID, []string{userID}) - assert.NoError(t, err) - }) - - t.Run("propagates membership cascade failure", func(t *testing.T) { - m := newMocks(t) - - m.mbrSvc.EXPECT().ForceRemoveOrganizationMember(mock.Anything, orgID, userID, schema.UserPrincipal). - Return(errors.New("cascade boom")) - - err := m.build().RemoveUsersFromOrg(context.Background(), orgID, []string{userID}) - assert.ErrorContains(t, err, "cascade boom") - }) - - t.Run("keeps going to the next user after one fails", func(t *testing.T) { - m := newMocks(t) - - m.mbrSvc.EXPECT().ForceRemoveOrganizationMember(mock.Anything, orgID, userID, schema.UserPrincipal). - Return(errors.New("cascade boom")) - m.mbrSvc.EXPECT().ForceRemoveOrganizationMember(mock.Anything, orgID, "user-2", schema.UserPrincipal).Return(nil) - - err := m.build().RemoveUsersFromOrg(context.Background(), orgID, []string{userID, "user-2"}) - assert.ErrorContains(t, err, "cascade boom") - }) -} - var ( orgOwner = role.Role{Name: schema.RoleOrganizationOwner, Title: "Owner"} orgViewer = role.Role{Name: schema.RoleOrganizationViewer, Title: "Member"} @@ -1891,7 +1854,7 @@ func TestDeleteUser(t *testing.T) { expectNoBillingAccounts(m, solo.ID) expectUserOrg(m, shared, []membership.Member{orgMember(userID, orgViewer), orgMember("user-2", orgOwner)}, userID, "user-2") expectEmptyOrgDelete(m, &steps) - m.mbrSvc.EXPECT().ForceRemoveOrganizationMember(mock.Anything, shared.ID, userID, schema.UserPrincipal). + m.mbrSvc.EXPECT().RemoveOrganizationMemberOnDelete(mock.Anything, shared.ID, userID, schema.UserPrincipal). Run(func(_ context.Context, orgID, _, _ string) { steps = append(steps, "leave:"+orgID) }).Return(nil) m.mbrSvc.EXPECT().RemovePrincipalPolicies(mock.Anything, userID, schema.UserPrincipal). Run(func(context.Context, string, string) { steps = append(steps, "policies") }).Return(nil) @@ -1996,12 +1959,55 @@ func TestDeleteUser(t *testing.T) { assert.Equal(t, []string{"policies", "pats", "user"}, steps) }) + t.Run("a user removed from an org since the check does not stop the delete", func(t *testing.T) { + m := newMocks(t) + + expectUserOrgs(m, shared.ID) + expectUserOrg(m, shared, []membership.Member{orgMember(userID, orgViewer), orgMember("user-2", orgOwner)}, userID, "user-2") + m.mbrSvc.EXPECT().RemoveOrganizationMemberOnDelete(mock.Anything, shared.ID, userID, schema.UserPrincipal). + Return(membership.ErrNotMember) + m.mbrSvc.EXPECT().RemovePrincipalPolicies(mock.Anything, userID, schema.UserPrincipal).Return(nil) + m.patSvc.EXPECT().DeleteAllByUser(mock.Anything, userID).Return(nil) + m.usrSvc.EXPECT().Delete(mock.Anything, userID).Return(nil) + + assert.NoError(t, m.build().DeleteUser(context.Background(), userID)) + }) + + t.Run("an org the user leaves that was deleted since the check does not stop the delete", func(t *testing.T) { + m := newMocks(t) + + expectUserOrgs(m, shared.ID) + expectUserOrg(m, shared, []membership.Member{orgMember(userID, orgViewer), orgMember("user-2", orgOwner)}, userID, "user-2") + m.mbrSvc.EXPECT().RemoveOrganizationMemberOnDelete(mock.Anything, shared.ID, userID, schema.UserPrincipal). + Return(organization.ErrNotExist) + m.mbrSvc.EXPECT().RemovePrincipalPolicies(mock.Anything, userID, schema.UserPrincipal).Return(nil) + m.patSvc.EXPECT().DeleteAllByUser(mock.Anything, userID).Return(nil) + m.usrSvc.EXPECT().Delete(mock.Anything, userID).Return(nil) + + assert.NoError(t, m.build().DeleteUser(context.Background(), userID)) + }) + + t.Run("an org that lost its other owner since the check refuses the delete with the blocker", func(t *testing.T) { + m := newMocks(t) + + expectUserOrgs(m, shared.ID) + expectUserOrg(m, shared, []membership.Member{orgMember(userID, orgOwner), orgMember("user-2", orgOwner)}, userID, "user-2") + m.mbrSvc.EXPECT().RemoveOrganizationMemberOnDelete(mock.Anything, shared.ID, userID, schema.UserPrincipal). + Return(membership.ErrLastOwnerRole) + // RemovePrincipalPolicies, PAT cleanup and usrSvc.Delete must NOT be called — strict mocks fail on unexpected calls. + + err := m.build().DeleteUser(context.Background(), userID) + var blocked *deleter.BlockedError + assert.ErrorAs(t, err, &blocked) + assert.Equal(t, &deleter.BlockedError{UserID: userID, Blockers: []deleter.Blocker{soleOwnerBlocker(shared)}}, blocked) + }) + t.Run("an org removal failure stops the delete before the remaining policies", func(t *testing.T) { m := newMocks(t) expectUserOrgs(m, shared.ID) expectUserOrg(m, shared, []membership.Member{orgMember(userID, orgViewer), orgMember("user-2", orgOwner)}, userID, "user-2") - m.mbrSvc.EXPECT().ForceRemoveOrganizationMember(mock.Anything, shared.ID, userID, schema.UserPrincipal). + m.mbrSvc.EXPECT().RemoveOrganizationMemberOnDelete(mock.Anything, shared.ID, userID, schema.UserPrincipal). Return(errors.New("cascade boom")) err := m.build().DeleteUser(context.Background(), userID) diff --git a/core/membership/org.go b/core/membership/org.go index 71bfc030f5..220a024e76 100644 --- a/core/membership/org.go +++ b/core/membership/org.go @@ -205,20 +205,17 @@ 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, true) + return s.removeOrganizationMember(ctx, orgID, principalID, principalType, false) } -// ForceRemoveOrganizationMember is RemoveOrganizationMember without the -// member-and-owner guards: it does not fail when the principal has no -// org-level policies and it removes the org's last owner without complaint. -// It exists for deletion cascades (see core/deleter), where the principal is -// leaving the system entirely and the org may legitimately be left ownerless. -// Anything user-initiated should go through RemoveOrganizationMember instead. -func (s *Service) ForceRemoveOrganizationMember(ctx context.Context, orgID, principalID, principalType string) error { - return s.removeOrganizationMember(ctx, orgID, principalID, principalType, false) +// 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. +func (s *Service) RemoveOrganizationMemberOnDelete(ctx context.Context, orgID, principalID, principalType string) error { + return s.removeOrganizationMember(ctx, orgID, principalID, principalType, true) } -func (s *Service) removeOrganizationMember(ctx context.Context, orgID, principalID, principalType string, guarded bool) error { +func (s *Service) removeOrganizationMember(ctx context.Context, orgID, principalID, principalType string, allowDisabledOrg bool) error { targetAuditType, err := principalTypeToAuditType(principalType) if err != nil { return err @@ -226,11 +223,9 @@ func (s *Service) removeOrganizationMember(ctx context.Context, orgID, principal org, err := s.orgService.Get(ctx, orgID) if err != nil { - // deletion cascades must keep working on disabled orgs — the deleter - // visits them deliberately so user deletion doesn't leave orphan - // policies behind. Get discards the org payload on ErrDisabled, so - // reconstruct the minimal org the cascade needs. - if guarded || !errors.Is(err, organization.ErrDisabled) { + // a user delete leaves disabled orgs too. Get discards the org payload + // on ErrDisabled, so reconstruct the minimal org the cascade needs. + if !allowDisabledOrg || !errors.Is(err, organization.ErrDisabled) { return err } org = organization.Organization{ID: orgID} @@ -245,19 +240,17 @@ func (s *Service) removeOrganizationMember(ctx context.Context, orgID, principal if err != nil { return fmt.Errorf("list existing policies: %w", err) } + if len(orgPolicies) == 0 { + return ErrNotMember + } // only humans can be the last owner — skip for service users and PATs. // an empty ownerRoleID makes the cascade delete owner policies unguarded. var ownerRoleID string - if guarded { - if len(orgPolicies) == 0 { - return ErrNotMember - } - if principalType == schema.UserPrincipal { - ownerRoleID, err = s.validateMinOwnerConstraint(ctx, orgID, "", orgPolicies) - if err != nil { - return err - } + if principalType == schema.UserPrincipal { + ownerRoleID, err = s.validateMinOwnerConstraint(ctx, orgID, "", orgPolicies) + if err != nil { + return err } } @@ -336,6 +329,9 @@ func (s *Service) cascadeRemovePrincipal(ctx context.Context, org organization.O if errors.Is(err, policy.ErrLastRoleGuard) { return ErrLastOwnerRole } + if errors.Is(err, policy.ErrNotExist) { + continue + } return fmt.Errorf("delete org policy %s: %w", pol.ID, err) } } @@ -343,7 +339,7 @@ func (s *Service) cascadeRemovePrincipal(ctx context.Context, org organization.O // guard passed — delete sub-resource policies var errs error for _, pol := range subResourcePolicies { - if err := s.policyService.Delete(ctx, pol.ID); err != nil { + if err := s.policyService.Delete(ctx, pol.ID); err != nil && !errors.Is(err, policy.ErrNotExist) { errs = errors.Join(errs, fmt.Errorf("delete sub-resource policy %s: %w", pol.ID, err)) } } diff --git a/core/membership/org_test.go b/core/membership/org_test.go index d7788ed03e..9de68b50d7 100644 --- a/core/membership/org_test.go +++ b/core/membership/org_test.go @@ -1000,17 +1000,14 @@ func TestService_RemoveOrganizationMember(t *testing.T) { } } -func TestService_ForceRemoveOrganizationMember(t *testing.T) { +func TestService_RemoveOrganizationMemberOnDelete(t *testing.T) { ctx := context.Background() orgID := uuid.New().String() userID := uuid.New().String() ownerRoleID := uuid.New().String() - enabledOrg := organization.Organization{ID: orgID, Title: "Test Org"} - type testDeps struct { policySvc *mocks.PolicyService - relSvc *mocks.RelationService roleSvc *mocks.RoleService orgSvc *mocks.OrgService projSvc *mocks.ProjectService @@ -1018,6 +1015,12 @@ func TestService_ForceRemoveOrganizationMember(t *testing.T) { resSvc *mocks.ResourceService auditRepo *mocks.AuditRecordRepository } + expectDisabledOrgOwner := func(d testDeps, owners []policy.Policy) { + d.orgSvc.EXPECT().Get(ctx, orgID).Return(organization.Organization{}, organization.ErrDisabled) + d.policySvc.EXPECT().List(ctx, policy.Filter{OrgID: orgID, PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{{ID: "org-p1", RoleID: ownerRoleID}}, nil) + d.roleSvc.EXPECT().Get(ctx, schema.RoleOrganizationOwner).Return(role.Role{ID: ownerRoleID, Name: schema.RoleOrganizationOwner}, nil) + d.policySvc.EXPECT().List(ctx, policy.Filter{OrgID: orgID, RoleID: ownerRoleID}).Return(owners, nil) + } tests := []struct { name string @@ -1025,51 +1028,49 @@ func TestService_ForceRemoveOrganizationMember(t *testing.T) { wantErr error }{ { - name: "removes the org's last owner without the guard", + name: "removes an owner from a disabled org with the owner guard", 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: ownerRoleID}}, nil) - // no roleSvc.Get / owner-count query: the guard is skipped entirely + expectDisabledOrgOwner(d, []policy.Policy{{ID: "org-p1"}, {ID: "other-owner-p"}}) 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, RoleID: ownerRoleID}, }, nil) - // plain delete, not DeleteWithMinRoleGuard - d.policySvc.EXPECT().Delete(ctx, "org-p1").Return(nil) + d.policySvc.EXPECT().DeleteWithMinRoleGuard(ctx, "org-p1", ownerRoleID).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) }, }, { - name: "does not return ErrNotMember when principal has no org policies and still cascades", + name: "skips policies that are already gone", 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.projSvc.EXPECT().List(ctx, project.Filter{OrgID: orgID}).Return([]project.Project{}, nil) + expectDisabledOrgOwner(d, []policy.Policy{{ID: "org-p1"}, {ID: "other-owner-p"}}) + d.projSvc.EXPECT().List(ctx, project.Filter{OrgID: orgID}).Return([]project.Project{{ID: "proj-1"}}, 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{}, nil) - d.resSvc.EXPECT().RemovePrincipalAccess(ctx, userID, schema.UserPrincipal, orgID, []string{}).Return(nil) + d.policySvc.EXPECT().List(ctx, policy.Filter{PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{ + {ID: "org-p1", ResourceType: schema.OrganizationNamespace, ResourceID: orgID, RoleID: ownerRoleID}, + {ID: "proj-p1", ResourceType: schema.ProjectNamespace, ResourceID: "proj-1"}, + }, nil) + d.policySvc.EXPECT().DeleteWithMinRoleGuard(ctx, "org-p1", ownerRoleID).Return(policy.ErrNotExist) + d.policySvc.EXPECT().Delete(ctx, "proj-p1").Return(policy.ErrNotExist) + d.resSvc.EXPECT().RemovePrincipalAccess(ctx, userID, schema.UserPrincipal, orgID, []string{"proj-1"}).Return(nil) d.auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil) }, }, { - name: "proceeds with the cascade when the org is disabled", + name: "refuses the last owner of a disabled org", + setup: func(d testDeps) { + expectDisabledOrgOwner(d, []policy.Policy{{ID: "org-p1"}}) + }, + wantErr: membership.ErrLastOwnerRole, + }, + { + name: "returns ErrNotMember when the principal has no org policies", setup: func(d testDeps) { - // Get discards the org payload on ErrDisabled; the force path - // must reconstruct the org from the input ID and continue — - // the deleter visits disabled orgs deliberately d.orgSvc.EXPECT().Get(ctx, orgID).Return(organization.Organization{}, organization.ErrDisabled) - d.policySvc.EXPECT().List(ctx, policy.Filter{OrgID: orgID, PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{{ID: "org-p1", RoleID: ownerRoleID}}, 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, RoleID: ownerRoleID}, - }, 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.policySvc.EXPECT().List(ctx, policy.Filter{OrgID: orgID, PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{}, nil) }, + wantErr: membership.ErrNotMember, }, { name: "propagates org lookup failure", @@ -1084,7 +1085,6 @@ func TestService_ForceRemoveOrganizationMember(t *testing.T) { t.Run(tt.name, func(t *testing.T) { d := testDeps{ policySvc: mocks.NewPolicyService(t), - relSvc: mocks.NewRelationService(t), roleSvc: mocks.NewRoleService(t), orgSvc: mocks.NewOrgService(t), projSvc: mocks.NewProjectService(t), @@ -1092,14 +1092,12 @@ func TestService_ForceRemoveOrganizationMember(t *testing.T) { resSvc: mocks.NewResourceService(t), auditRepo: mocks.NewAuditRecordRepository(t), } - if tt.setup != nil { - tt.setup(d) - } + tt.setup(d) - 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 := 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) - err := svc.ForceRemoveOrganizationMember(ctx, orgID, userID, schema.UserPrincipal) + err := svc.RemoveOrganizationMemberOnDelete(ctx, orgID, userID, schema.UserPrincipal) if tt.wantErr != nil { assert.ErrorIs(t, err, tt.wantErr) } else { diff --git a/core/resource/service.go b/core/resource/service.go index 997c8ac4b2..0df69f2fba 100644 --- a/core/resource/service.go +++ b/core/resource/service.go @@ -574,7 +574,7 @@ func (s Service) RemovePrincipalAccess(ctx context.Context, principalID, princip var errs error for _, pol := range policies { if _, ok := resourceIDs[pol.ResourceID]; ok { - if err := s.policyService.Delete(ctx, pol.ID); err != nil { + if err := s.policyService.Delete(ctx, pol.ID); err != nil && !errors.Is(err, policy.ErrNotExist) { errs = errors.Join(errs, fmt.Errorf("delete resource policy %s: %w", pol.ID, err)) } } diff --git a/core/resource/service_test.go b/core/resource/service_test.go index 97b5994391..a151ff254e 100644 --- a/core/resource/service_test.go +++ b/core/resource/service_test.go @@ -946,6 +946,20 @@ func TestRemovePrincipalAccess(t *testing.T) { assert.NoError(t, err) }) + t.Run("a policy that is already gone counts as deleted", func(t *testing.T) { + repo, _, _, _, _, _, _, policySvc, svc := newTestService(t) + + policySvc.EXPECT().List(mock.Anything, mock.Anything).Return([]policy.Policy{ + {ID: "store-p1", ResourceType: "resource/store", ResourceID: "store-1"}, + }, nil) + repo.EXPECT().List(mock.Anything, resource.Filter{OrgID: "org-1"}). + Return([]resource.Resource{{ID: "store-1"}}, nil) + policySvc.EXPECT().Delete(mock.Anything, "store-p1").Return(policy.ErrNotExist) + + err := svc.RemovePrincipalAccess(context.Background(), userID, schema.UserPrincipal, "org-1", nil) + assert.NoError(t, err) + }) + t.Run("aggregates delete errors and keeps going", func(t *testing.T) { repo, _, _, _, _, _, _, policySvc, svc := newTestService(t) diff --git a/internal/api/v1beta1connect/authenticate.go b/internal/api/v1beta1connect/authenticate.go index 8315d20a25..5608152190 100644 --- a/internal/api/v1beta1connect/authenticate.go +++ b/internal/api/v1beta1connect/authenticate.go @@ -400,6 +400,19 @@ func (h *ConnectHandler) GetLoggedInPrincipal(ctx context.Context, via ...authen return principal, nil } +// getLoggedInUser returns the authenticated principal, rejecting service users +// and PATs. Use this for endpoints that act on the user's own account. +func (h *ConnectHandler) getLoggedInUser(ctx context.Context) (authenticate.Principal, error) { + principal, err := h.GetLoggedInPrincipal(ctx) + if err != nil { + return principal, err + } + if principal.Type != schema.UserPrincipal { + return principal, connect.NewError(connect.CodePermissionDenied, ErrUnauthenticated) + } + return principal, nil +} + func (h *ConnectHandler) ListAuthStrategies(ctx context.Context, request *connect.Request[frontierv1beta1.ListAuthStrategiesRequest]) (*connect.Response[frontierv1beta1.ListAuthStrategiesResponse], error) { var pbstrategy []*frontierv1beta1.AuthStrategy for _, strategy := range h.authnService.SupportedStrategies() { diff --git a/internal/api/v1beta1connect/interfaces.go b/internal/api/v1beta1connect/interfaces.go index 1b8ef71916..50288490df 100644 --- a/internal/api/v1beta1connect/interfaces.go +++ b/internal/api/v1beta1connect/interfaces.go @@ -400,6 +400,7 @@ type CascadeDeleter interface { CheckOrganizationDelete(ctx context.Context, id string) ([]deleter.Blocker, error) DeleteGroup(ctx context.Context, id string) error DeleteUser(ctx context.Context, userID string) error + CheckUserDelete(ctx context.Context, userID string) (deleter.UserDeleteCheck, error) } type AuditRecordService interface { diff --git a/internal/api/v1beta1connect/mocks/cascade_deleter.go b/internal/api/v1beta1connect/mocks/cascade_deleter.go index f4e8716574..ab6648bcdb 100644 --- a/internal/api/v1beta1connect/mocks/cascade_deleter.go +++ b/internal/api/v1beta1connect/mocks/cascade_deleter.go @@ -140,6 +140,63 @@ func (_c *CascadeDeleter_CheckProjectDelete_Call) RunAndReturn(run func(context. return _c } +// CheckUserDelete provides a mock function with given fields: ctx, userID +func (_m *CascadeDeleter) CheckUserDelete(ctx context.Context, userID string) (deleter.UserDeleteCheck, error) { + ret := _m.Called(ctx, userID) + + if len(ret) == 0 { + panic("no return value specified for CheckUserDelete") + } + + var r0 deleter.UserDeleteCheck + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, string) (deleter.UserDeleteCheck, error)); ok { + return rf(ctx, userID) + } + if rf, ok := ret.Get(0).(func(context.Context, string) deleter.UserDeleteCheck); ok { + r0 = rf(ctx, userID) + } else { + r0 = ret.Get(0).(deleter.UserDeleteCheck) + } + + if rf, ok := ret.Get(1).(func(context.Context, string) error); ok { + r1 = rf(ctx, userID) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// CascadeDeleter_CheckUserDelete_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'CheckUserDelete' +type CascadeDeleter_CheckUserDelete_Call struct { + *mock.Call +} + +// CheckUserDelete is a helper method to define mock.On call +// - ctx context.Context +// - userID string +func (_e *CascadeDeleter_Expecter) CheckUserDelete(ctx interface{}, userID interface{}) *CascadeDeleter_CheckUserDelete_Call { + return &CascadeDeleter_CheckUserDelete_Call{Call: _e.mock.On("CheckUserDelete", ctx, userID)} +} + +func (_c *CascadeDeleter_CheckUserDelete_Call) Run(run func(ctx context.Context, userID string)) *CascadeDeleter_CheckUserDelete_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(string)) + }) + return _c +} + +func (_c *CascadeDeleter_CheckUserDelete_Call) Return(_a0 deleter.UserDeleteCheck, _a1 error) *CascadeDeleter_CheckUserDelete_Call { + _c.Call.Return(_a0, _a1) + return _c +} + +func (_c *CascadeDeleter_CheckUserDelete_Call) RunAndReturn(run func(context.Context, string) (deleter.UserDeleteCheck, error)) *CascadeDeleter_CheckUserDelete_Call { + _c.Call.Return(run) + return _c +} + // DeleteGroup provides a mock function with given fields: ctx, id func (_m *CascadeDeleter) DeleteGroup(ctx context.Context, id string) error { ret := _m.Called(ctx, id) diff --git a/internal/api/v1beta1connect/user.go b/internal/api/v1beta1connect/user.go index b9eeafb214..b1e834d3be 100644 --- a/internal/api/v1beta1connect/user.go +++ b/internal/api/v1beta1connect/user.go @@ -22,6 +22,7 @@ import ( "github.com/raystack/frontier/internal/store/postgres" "github.com/raystack/frontier/pkg/metadata" "github.com/raystack/frontier/pkg/pagination" + "github.com/raystack/frontier/pkg/server/consts" "github.com/raystack/frontier/pkg/str" "github.com/raystack/frontier/pkg/utils" frontierv1beta1 "github.com/raystack/frontier/proto/v1beta1" @@ -409,32 +410,91 @@ func (h *ConnectHandler) DisableUser(ctx context.Context, request *connect.Reque } func (h *ConnectHandler) DeleteUser(ctx context.Context, request *connect.Request[frontierv1beta1.DeleteUserRequest]) (*connect.Response[frontierv1beta1.DeleteUserResponse], error) { + if err := h.deleteUser(ctx, request, "DeleteUser", request.Msg.GetId()); err != nil { + return nil, err + } + return connect.NewResponse(&frontierv1beta1.DeleteUserResponse{}), nil +} + +func (h *ConnectHandler) CheckCurrentUserDelete(ctx context.Context, request *connect.Request[frontierv1beta1.CheckCurrentUserDeleteRequest]) (*connect.Response[frontierv1beta1.CheckCurrentUserDeleteResponse], error) { + principal, err := h.getLoggedInUser(ctx) + if err != nil { + return nil, err + } + check, err := h.deleterService.CheckUserDelete(ctx, principal.ID) + if err != nil { + return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("CheckCurrentUserDelete: user_id=%s: %w", principal.ID, err)) + } + return connect.NewResponse(transformUserDeleteCheckToPB(check)), nil +} + +func (h *ConnectHandler) DeleteCurrentUser(ctx context.Context, request *connect.Request[frontierv1beta1.DeleteCurrentUserRequest]) (*connect.Response[frontierv1beta1.DeleteCurrentUserResponse], error) { + principal, err := h.getLoggedInUser(ctx) + if err != nil { + return nil, err + } + if err := h.deleteUser(ctx, request, "DeleteCurrentUser", principal.ID); err != nil { + return nil, err + } + resp := connect.NewResponse(&frontierv1beta1.DeleteCurrentUserResponse{}) + // instruct interceptor to invalidate cookie + resp.Header().Set(consts.SessionDeleteGatewayKey, "true") + return resp, nil +} + +func (h *ConnectHandler) deleteUser(ctx context.Context, request connect.AnyRequest, operation, userID string) error { errorLogger := NewErrorLogger() - userID := request.Msg.GetId() if err := h.deleterService.DeleteUser(ctx, userID); err != nil { var blocked *deleter.BlockedError if errors.As(err, &blocked) { - return nil, deleteBlockedError(ctx, blocked) + return deleteBlockedError(ctx, blocked) } - errorLogger.LogServiceError(ctx, request, "DeleteUser", err, + errorLogger.LogServiceError(ctx, request, operation, err, "user_id", userID) switch { case errors.Is(err, user.ErrNotExist): - return nil, connect.NewError(connect.CodeNotFound, ErrUserNotExist) + return connect.NewError(connect.CodeNotFound, ErrUserNotExist) case errors.Is(err, user.ErrInvalidID): - return nil, connect.NewError(connect.CodeInvalidArgument, ErrBadRequest) + return connect.NewError(connect.CodeInvalidArgument, ErrBadRequest) default: - return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("DeleteUser: user_id=%s: %w", userID, err)) + return connect.NewError(connect.CodeInternal, fmt.Errorf("%s: user_id=%s: %w", operation, userID, err)) } } - if err := audit.GetAuditor(ctx, schema.PlatformOrgID.String()).Log(audit.UserDeletedEvent, audit.UserTarget(request.Msg.GetId())); err != nil { - errorLogger.LogServiceError(ctx, request, "DeleteUser.AuditLog", err, + if err := audit.GetAuditor(ctx, schema.PlatformOrgID.String()).Log(audit.UserDeletedEvent, audit.UserTarget(userID)); err != nil { + errorLogger.LogServiceError(ctx, request, operation+".AuditLog", err, "user_id", userID) } - return connect.NewResponse(&frontierv1beta1.DeleteUserResponse{}), nil + return nil +} + +func transformUserDeleteCheckToPB(check deleter.UserDeleteCheck) *frontierv1beta1.CheckCurrentUserDeleteResponse { + resp := &frontierv1beta1.CheckCurrentUserDeleteResponse{ + CanDelete: check.CanDelete(), + Blockers: transformBlockersToPB(check.Blockers), + PatCount: int32(check.PATCount), + } + for _, o := range check.OrganizationsToDelete { + resp.OrganizationsToDelete = append(resp.OrganizationsToDelete, &frontierv1beta1.CheckCurrentUserDeleteResponse_OrganizationToDelete{ + Id: o.Organization.ID, + Name: o.Organization.Name, + Title: o.Organization.Title, + CanDelete: len(o.Blockers) == 0, + Blockers: transformBlockersToPB(o.Blockers), + }) + } + for _, o := range check.OrganizationsToLeave { + resp.OrganizationsToLeave = append(resp.OrganizationsToLeave, &frontierv1beta1.CheckCurrentUserDeleteResponse_OrganizationToLeave{ + Id: o.Organization.ID, + Name: o.Organization.Name, + Title: o.Organization.Title, + Role: o.RoleTitle, + Blockers: transformBlockersToPB(o.Blockers), + }) + } + return resp } func (h *ConnectHandler) ListUserGroups(ctx context.Context, request *connect.Request[frontierv1beta1.ListUserGroupsRequest]) (*connect.Response[frontierv1beta1.ListUserGroupsResponse], error) { diff --git a/internal/api/v1beta1connect/user_pat.go b/internal/api/v1beta1connect/user_pat.go index 8ee157ebfb..4c5ba8d3bf 100644 --- a/internal/api/v1beta1connect/user_pat.go +++ b/internal/api/v1beta1connect/user_pat.go @@ -10,7 +10,6 @@ import ( "github.com/raystack/frontier/core/userpat" paterrors "github.com/raystack/frontier/core/userpat/errors" "github.com/raystack/frontier/core/userpat/models" - "github.com/raystack/frontier/internal/bootstrap/schema" "github.com/raystack/frontier/pkg/metadata" "github.com/raystack/frontier/pkg/utils" frontierv1beta1 "github.com/raystack/frontier/proto/v1beta1" @@ -61,13 +60,10 @@ func mapPATError(err error) *connect.Error { func (h *ConnectHandler) CreateCurrentUserPAT(ctx context.Context, request *connect.Request[frontierv1beta1.CreateCurrentUserPATRequest]) (*connect.Response[frontierv1beta1.CreateCurrentUserPATResponse], error) { errorLogger := NewErrorLogger() - principal, err := h.GetLoggedInPrincipal(ctx) + principal, err := h.getLoggedInUser(ctx) if err != nil { return nil, err } - if principal.Type != schema.UserPrincipal { - return nil, connect.NewError(connect.CodePermissionDenied, ErrUnauthenticated) - } if err := h.userPATService.ValidateExpiry(request.Msg.GetExpiresAt().AsTime()); err != nil { return nil, connect.NewError(connect.CodeInvalidArgument, err) diff --git a/internal/api/v1beta1connect/user_test.go b/internal/api/v1beta1connect/user_test.go index 407d7a276f..9e47504cfc 100644 --- a/internal/api/v1beta1connect/user_test.go +++ b/internal/api/v1beta1connect/user_test.go @@ -22,6 +22,7 @@ import ( "github.com/raystack/frontier/internal/bootstrap/schema" "github.com/raystack/frontier/pkg/errors" "github.com/raystack/frontier/pkg/metadata" + "github.com/raystack/frontier/pkg/server/consts" "github.com/raystack/frontier/pkg/utils" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -910,6 +911,8 @@ func TestConnectHandler_DeleteUser(t *testing.T) { if tt.err == connect.Code(0) { assert.NoError(t, err) assert.EqualValues(t, tt.want, resp.Msg) + // the admin's own session cookie stays + assert.Empty(t, resp.Header().Get(consts.SessionDeleteGatewayKey)) } else { assert.Nil(t, resp) assert.Equal(t, tt.err, connect.CodeOf(err)) @@ -955,6 +958,194 @@ func TestConnectHandler_DeleteUser(t *testing.T) { }) } +func TestConnectHandler_CheckCurrentUserDelete(t *testing.T) { + userID := uuid.New().String() + currentUser := authenticate.Principal{ID: userID, Type: schema.UserPrincipal, User: &user.User{ID: userID}} + solo := organization.Organization{ID: uuid.New().String(), Name: "solo", Title: "Solo"} + debt := organization.Organization{ID: uuid.New().String(), Name: "debt", Title: "Debt"} + shared := organization.Organization{ID: uuid.New().String(), Name: "shared", Title: "Shared"} + unpaid := deleter.Blocker{Type: deleter.BlockerUnpaidInvoice, Subject: "inv-1", SubjectType: deleter.SubjectInvoice, Message: "invoice[inv-1] is unpaid"} + soleOwner := deleter.Blocker{Type: deleter.BlockerSoleOwnerWithMembers, Subject: shared.ID, SubjectType: deleter.SubjectOrganization, Message: "organization[shared] has other members and this user is its only owner"} + + tests := []struct { + title string + setup func(*mocks.AuthnService, *mocks.CascadeDeleter) + want *frontierv1beta1.CheckCurrentUserDeleteResponse + err connect.Code + }{ + { + title: "should list the orgs deleted with the user and the orgs it leaves, each with its blockers", + setup: func(as *mocks.AuthnService, cd *mocks.CascadeDeleter) { + as.EXPECT().GetPrincipal(mock.Anything).Return(currentUser, nil) + cd.EXPECT().CheckUserDelete(mock.Anything, userID).Return(deleter.UserDeleteCheck{ + OrganizationsToDelete: []deleter.OrganizationToDelete{ + {Organization: solo}, + {Organization: debt, Blockers: []deleter.Blocker{unpaid}}, + }, + OrganizationsToLeave: []deleter.OrganizationToLeave{ + {Organization: shared, RoleTitle: "Owner", Blockers: []deleter.Blocker{soleOwner}}, + }, + PATCount: 2, + }, nil) + }, + want: &frontierv1beta1.CheckCurrentUserDeleteResponse{ + CanDelete: false, + OrganizationsToDelete: []*frontierv1beta1.CheckCurrentUserDeleteResponse_OrganizationToDelete{ + {Id: solo.ID, Name: "solo", Title: "Solo", CanDelete: true}, + {Id: debt.ID, Name: "debt", Title: "Debt", CanDelete: false, Blockers: []*frontierv1beta1.Blocker{ + {Type: deleter.BlockerUnpaidInvoice, Subject: "inv-1", SubjectType: deleter.SubjectInvoice, Message: "invoice[inv-1] is unpaid"}, + }}, + }, + OrganizationsToLeave: []*frontierv1beta1.CheckCurrentUserDeleteResponse_OrganizationToLeave{ + {Id: shared.ID, Name: "shared", Title: "Shared", Role: "Owner", Blockers: []*frontierv1beta1.Blocker{ + {Type: deleter.BlockerSoleOwnerWithMembers, Subject: shared.ID, SubjectType: deleter.SubjectOrganization, Message: "organization[shared] has other members and this user is its only owner"}, + }}, + }, + PatCount: 2, + }, + }, + { + title: "should allow the delete when nothing blocks it", + setup: func(as *mocks.AuthnService, cd *mocks.CascadeDeleter) { + as.EXPECT().GetPrincipal(mock.Anything).Return(currentUser, nil) + cd.EXPECT().CheckUserDelete(mock.Anything, userID).Return(deleter.UserDeleteCheck{ + OrganizationsToLeave: []deleter.OrganizationToLeave{{Organization: shared, RoleTitle: "Member"}}, + }, nil) + }, + want: &frontierv1beta1.CheckCurrentUserDeleteResponse{ + CanDelete: true, + OrganizationsToLeave: []*frontierv1beta1.CheckCurrentUserDeleteResponse_OrganizationToLeave{ + {Id: shared.ID, Name: "shared", Title: "Shared", Role: "Member"}, + }, + }, + }, + { + title: "should return internal error when the check fails", + setup: func(as *mocks.AuthnService, cd *mocks.CascadeDeleter) { + as.EXPECT().GetPrincipal(mock.Anything).Return(currentUser, nil) + cd.EXPECT().CheckUserDelete(mock.Anything, userID).Return(deleter.UserDeleteCheck{}, errors.New("read failed")) + }, + err: connect.CodeInternal, + }, + { + title: "should return unauthenticated when nobody is logged in", + setup: func(as *mocks.AuthnService, cd *mocks.CascadeDeleter) { + as.EXPECT().GetPrincipal(mock.Anything).Return(authenticate.Principal{}, errors.ErrUnauthenticated) + }, + err: connect.CodeUnauthenticated, + }, + { + title: "should return permission denied for a service user", + setup: func(as *mocks.AuthnService, cd *mocks.CascadeDeleter) { + as.EXPECT().GetPrincipal(mock.Anything).Return(authenticate.Principal{ID: "sv-1", Type: schema.ServiceUserPrincipal}, nil) + }, + err: connect.CodePermissionDenied, + }, + { + title: "should return permission denied for a PAT", + setup: func(as *mocks.AuthnService, cd *mocks.CascadeDeleter) { + as.EXPECT().GetPrincipal(mock.Anything).Return(authenticate.Principal{ID: "pat-1", Type: schema.PATPrincipal, User: &user.User{ID: userID}}, nil) + }, + err: connect.CodePermissionDenied, + }, + } + + for _, tt := range tests { + t.Run(tt.title, func(t *testing.T) { + as := mocks.NewAuthnService(t) + cd := mocks.NewCascadeDeleter(t) + tt.setup(as, cd) + handler := &ConnectHandler{authnService: as, deleterService: cd} + + resp, err := handler.CheckCurrentUserDelete(context.Background(), connect.NewRequest(&frontierv1beta1.CheckCurrentUserDeleteRequest{})) + if tt.err != connect.Code(0) { + assert.Nil(t, resp) + assert.Equal(t, tt.err, connect.CodeOf(err)) + return + } + assert.NoError(t, err) + assert.Equal(t, tt.want, resp.Msg) + }) + } +} + +func TestConnectHandler_DeleteCurrentUser(t *testing.T) { + userID := uuid.New().String() + currentUser := authenticate.Principal{ID: userID, Type: schema.UserPrincipal, User: &user.User{ID: userID}} + + tests := []struct { + title string + setup func(*mocks.AuthnService, *mocks.CascadeDeleter) + err connect.Code + }{ + { + title: "should delete the logged-in user and clear the session cookie", + setup: func(as *mocks.AuthnService, cd *mocks.CascadeDeleter) { + as.EXPECT().GetPrincipal(mock.Anything).Return(currentUser, nil) + cd.EXPECT().DeleteUser(mock.Anything, userID).Return(nil) + }, + }, + { + title: "should return failed precondition when the delete is blocked", + setup: func(as *mocks.AuthnService, cd *mocks.CascadeDeleter) { + as.EXPECT().GetPrincipal(mock.Anything).Return(currentUser, nil) + cd.EXPECT().DeleteUser(mock.Anything, userID).Return(&deleter.BlockedError{ + UserID: userID, + Blockers: []deleter.Blocker{{Type: deleter.BlockerSoleOwnerWithMembers, Subject: "org-1"}}, + }) + }, + err: connect.CodeFailedPrecondition, + }, + { + title: "should return internal error when the delete fails", + setup: func(as *mocks.AuthnService, cd *mocks.CascadeDeleter) { + as.EXPECT().GetPrincipal(mock.Anything).Return(currentUser, nil) + cd.EXPECT().DeleteUser(mock.Anything, userID).Return(errors.New("spicedb unavailable")) + }, + err: connect.CodeInternal, + }, + { + title: "should return unauthenticated when nobody is logged in", + setup: func(as *mocks.AuthnService, cd *mocks.CascadeDeleter) { + as.EXPECT().GetPrincipal(mock.Anything).Return(authenticate.Principal{}, errors.ErrUnauthenticated) + }, + err: connect.CodeUnauthenticated, + }, + { + title: "should return permission denied for a service user", + setup: func(as *mocks.AuthnService, cd *mocks.CascadeDeleter) { + as.EXPECT().GetPrincipal(mock.Anything).Return(authenticate.Principal{ID: "sv-1", Type: schema.ServiceUserPrincipal}, nil) + }, + err: connect.CodePermissionDenied, + }, + { + title: "should return permission denied for a PAT", + setup: func(as *mocks.AuthnService, cd *mocks.CascadeDeleter) { + as.EXPECT().GetPrincipal(mock.Anything).Return(authenticate.Principal{ID: "pat-1", Type: schema.PATPrincipal, User: &user.User{ID: userID}}, nil) + }, + err: connect.CodePermissionDenied, + }, + } + + for _, tt := range tests { + t.Run(tt.title, func(t *testing.T) { + as := mocks.NewAuthnService(t) + cd := mocks.NewCascadeDeleter(t) + tt.setup(as, cd) + handler := &ConnectHandler{authnService: as, deleterService: cd} + + resp, err := handler.DeleteCurrentUser(context.Background(), connect.NewRequest(&frontierv1beta1.DeleteCurrentUserRequest{})) + if tt.err != connect.Code(0) { + assert.Nil(t, resp) + assert.Equal(t, tt.err, connect.CodeOf(err)) + return + } + assert.NoError(t, err) + assert.Equal(t, "true", resp.Header().Get(consts.SessionDeleteGatewayKey)) + }) + } +} + func TestConnectHandler_ListUserGroups(t *testing.T) { userID := uuid.New().String() orgID := uuid.New().String() diff --git a/pkg/server/connect_interceptors/authorization.go b/pkg/server/connect_interceptors/authorization.go index 6c9b41e5bf..9b520ae6c3 100644 --- a/pkg/server/connect_interceptors/authorization.go +++ b/pkg/server/connect_interceptors/authorization.go @@ -118,6 +118,8 @@ var authorizationSkipEndpoints = map[string]bool{ "/raystack.frontier.v1beta1.FrontierService/ListOrganizationsByCurrentUser": true, "/raystack.frontier.v1beta1.FrontierService/ListProjectsByCurrentUser": true, "/raystack.frontier.v1beta1.FrontierService/ListCurrentUserInvitations": true, + "/raystack.frontier.v1beta1.FrontierService/CheckCurrentUserDelete": true, + "/raystack.frontier.v1beta1.FrontierService/DeleteCurrentUser": true, "/raystack.frontier.v1beta1.FrontierService/JoinOrganization": true, "/raystack.frontier.v1beta1.FrontierService/CreateOrganization": true, @@ -157,6 +159,8 @@ var patDeniedEndpoints = map[string]bool{ "/raystack.frontier.v1beta1.FrontierService/PingUserSession": true, "/raystack.frontier.v1beta1.FrontierService/RevokeSession": true, "/raystack.frontier.v1beta1.FrontierService/CreateCurrentUserPAT": true, + "/raystack.frontier.v1beta1.FrontierService/CheckCurrentUserDelete": true, + "/raystack.frontier.v1beta1.FrontierService/DeleteCurrentUser": true, } // authorizationValidationMap stores path to validation function diff --git a/test/e2e/regression/api_test.go b/test/e2e/regression/api_test.go index a4af23d3f5..29cda5a01a 100644 --- a/test/e2e/regression/api_test.go +++ b/test/e2e/regression/api_test.go @@ -2,6 +2,7 @@ package e2e_test import ( "context" + "encoding/base64" "encoding/json" "io" "net/http" @@ -2222,6 +2223,218 @@ CREATE TRIGGER trg_e2e_fail_user_delete_retry BEFORE DELETE OR UPDATE OF deleted }) } +func (s *APIRegressionTestSuite) TestDeleteCurrentUser() { + ctxOrgAdminAuth := testbench.ContextWithAuth(context.Background(), s.adminCookie) + + createOrg := func(ctx context.Context, name string) string { + resp, err := s.testBench.Client.CreateOrganization(ctx, connect.NewRequest(&frontierv1beta1.CreateOrganizationRequest{ + Body: &frontierv1beta1.OrganizationRequestBody{Title: name, Name: name}, + })) + s.Require().NoError(err) + return resp.Msg.GetOrganization().GetId() + } + createProject := func(ctx context.Context, orgID, name string) string { + resp, err := s.testBench.Client.CreateProject(ctx, connect.NewRequest(&frontierv1beta1.CreateProjectRequest{ + Body: &frontierv1beta1.ProjectRequestBody{Name: name, OrgId: orgID}, + })) + s.Require().NoError(err) + return resp.Msg.GetProject().GetId() + } + addMember := func(orgID, userID, roleID string) { + resp, err := s.testBench.AdminClient.AddOrganizationMembers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationMembersRequest{ + OrgId: orgID, + Members: []*frontierv1beta1.OrgMemberEntry{{UserId: userID, RoleId: roleID}}, + })) + requireAddOrgMembersSuccess(s.T(), resp, err) + } + canGet := func(subject, resource string) bool { + resp, err := s.testBench.AdminClient.CheckFederatedResourcePermission(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CheckFederatedResourcePermissionRequest{ + Resource: resource, + Permission: schema.GetPermission, + Subject: subject, + })) + s.Require().NoError(err) + return resp.Msg.GetStatus() + } + orgIsLive := func(id string) bool { + var count int + s.Require().NoError(s.dbc.GetContext(context.Background(), &count, + "SELECT count(*) FROM organizations WHERE id = $1 AND deleted_at IS NULL", id)) + return count == 1 + } + listedInOrgUsers := func(orgID, userID string) (bool, bool) { + resp, err := s.testBench.Client.ListOrganizationUsers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.ListOrganizationUsersRequest{Id: orgID})) + s.Require().NoError(err) + inUsers := slices.ContainsFunc(resp.Msg.GetUsers(), func(u *frontierv1beta1.User) bool { return u.GetId() == userID }) + inRolePairs := slices.ContainsFunc(resp.Msg.GetRolePairs(), func(p *frontierv1beta1.ListOrganizationUsersResponse_RolePair) bool { + return p.GetUserId() == userID + }) + return inUsers, inRolePairs + } + + userID, userCtx := signUpUser(s.T(), s.testBench.Client, "delete-current-user@raystack.org") + userSubject := schema.JoinNamespaceAndResourceID(schema.UserPrincipal, userID) + otherResp, err := s.testBench.Client.CreateUser(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateUserRequest{ + Body: &frontierv1beta1.UserRequestBody{Email: "delete-current-user-other@raystack.org", Name: "delete-current-user-other"}, + })) + s.Require().NoError(err) + otherID := otherResp.Msg.GetUser().GetId() + + aloneOrgID := createOrg(userCtx, "delete-current-user-alone") + aloneProjectID := createProject(userCtx, aloneOrgID, "delete-current-user-alone-project") + sharedOrgID := createOrg(userCtx, "delete-current-user-shared") + sharedProjectID := createProject(userCtx, sharedOrgID, "delete-current-user-shared-project") + addMember(sharedOrgID, otherID, s.orgViewerRole) + memberOrgID := createOrg(ctxOrgAdminAuth, "delete-current-user-member") + memberProjectID := createProject(ctxOrgAdminAuth, memberOrgID, "delete-current-user-member-project") + addMember(memberOrgID, userID, s.orgViewerRole) + _, err = s.testBench.Client.CreatePolicy(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreatePolicyRequest{ + Body: &frontierv1beta1.PolicyRequestBody{ + RoleId: schema.RoleProjectViewer, + Resource: schema.JoinNamespaceAndResourceID(schema.ProjectNamespace, memberProjectID), + Principal: userSubject, + }, + })) + s.Require().NoError(err) + + resources := []struct{ name, resource string }{ + {"the org that goes with the user", schema.JoinNamespaceAndResourceID(schema.OrganizationNamespace, aloneOrgID)}, + {"a project in the org that goes with the user", schema.JoinNamespaceAndResourceID(schema.ProjectNamespace, aloneProjectID)}, + {"the org it owns with another member", schema.JoinNamespaceAndResourceID(schema.OrganizationNamespace, sharedOrgID)}, + {"a project in the org it owns with another member", schema.JoinNamespaceAndResourceID(schema.ProjectNamespace, sharedProjectID)}, + {"the org it is a member of", schema.JoinNamespaceAndResourceID(schema.OrganizationNamespace, memberOrgID)}, + {"a project it was given in that org", schema.JoinNamespaceAndResourceID(schema.ProjectNamespace, memberProjectID)}, + } + for _, r := range resources { + s.Require().Truef(canGet(userSubject, r.resource), "access to %s before the delete", r.name) + } + for _, orgID := range []string{sharedOrgID, memberOrgID} { + inUsers, inRolePairs := listedInOrgUsers(orgID, userID) + s.Require().Truef(inUsers && inRolePairs, "listed in the users and role pairs of org %s before the delete", orgID) + } + + s.Run("the check lists the org that goes with the user and the orgs it leaves, and the only owner blocks the delete", func() { + resp, err := s.testBench.Client.CheckCurrentUserDelete(userCtx, connect.NewRequest(&frontierv1beta1.CheckCurrentUserDeleteRequest{})) + s.Require().NoError(err) + s.Assert().False(resp.Msg.GetCanDelete()) + s.Assert().Empty(resp.Msg.GetBlockers()) + s.Assert().Zero(resp.Msg.GetPatCount()) + + s.Require().Len(resp.Msg.GetOrganizationsToDelete(), 1) + toDelete := resp.Msg.GetOrganizationsToDelete()[0] + s.Assert().Equal(aloneOrgID, toDelete.GetId()) + s.Assert().Equal("delete-current-user-alone", toDelete.GetName()) + s.Assert().Equal("delete-current-user-alone", toDelete.GetTitle()) + s.Assert().True(toDelete.GetCanDelete()) + + toLeave := map[string]*frontierv1beta1.CheckCurrentUserDeleteResponse_OrganizationToLeave{} + for _, o := range resp.Msg.GetOrganizationsToLeave() { + toLeave[o.GetId()] = o + } + s.Require().Len(toLeave, 2) + s.Assert().Equal("Owner", toLeave[sharedOrgID].GetRole()) + s.Require().Len(toLeave[sharedOrgID].GetBlockers(), 1) + s.Assert().Equal(deleter.BlockerSoleOwnerWithMembers, toLeave[sharedOrgID].GetBlockers()[0].GetType()) + s.Assert().Equal(sharedOrgID, toLeave[sharedOrgID].GetBlockers()[0].GetSubject()) + s.Assert().Equal("Member", toLeave[memberOrgID].GetRole()) + s.Assert().Empty(toLeave[memberOrgID].GetBlockers()) + }) + + s.Run("a blocked delete fails with the blocker and changes nothing", func() { + _, err := s.testBench.Client.DeleteCurrentUser(userCtx, connect.NewRequest(&frontierv1beta1.DeleteCurrentUserRequest{})) + message, violations := requireDeleteRefused(s.T(), err) + s.Assert().True(strings.HasPrefix(message, "user cannot be deleted yet"), message) + s.Require().Len(violations, 1) + s.Assert().Equal(deleter.BlockerSoleOwnerWithMembers, violations[0].GetType()) + s.Assert().Equal(sharedOrgID, violations[0].GetSubject()) + + _, err = s.testBench.Client.GetCurrentUser(userCtx, connect.NewRequest(&frontierv1beta1.GetCurrentUserRequest{})) + s.Require().NoError(err) + s.Assert().True(orgIsLive(aloneOrgID)) + for _, r := range resources { + s.Assert().Truef(canGet(userSubject, r.resource), "access to %s", r.name) + } + }) + + s.Run("making the other member an owner clears the blocker", func() { + _, err := s.testBench.Client.SetOrganizationMemberRole(userCtx, connect.NewRequest(&frontierv1beta1.SetOrganizationMemberRoleRequest{ + OrgId: sharedOrgID, + UserId: otherID, + RoleId: s.orgOwnerRole, + })) + s.Require().NoError(err) + + resp, err := s.testBench.Client.CheckCurrentUserDelete(userCtx, connect.NewRequest(&frontierv1beta1.CheckCurrentUserDeleteRequest{})) + s.Require().NoError(err) + s.Assert().True(resp.Msg.GetCanDelete()) + }) + + s.Run("the delete goes through and clears the session cookie", func() { + resp, err := s.testBench.Client.DeleteCurrentUser(userCtx, connect.NewRequest(&frontierv1beta1.DeleteCurrentUserRequest{})) + s.Require().NoError(err) + setCookie := resp.Header().Get("Set-Cookie") + s.Assert().True(strings.HasPrefix(setCookie, consts.SessionRequestKey+"=;"), setCookie) + s.Assert().Contains(setCookie, "Max-Age=0") + }) + + s.Run("the old session cookie is rejected as unauthenticated", func() { + _, err := s.testBench.Client.GetCurrentUser(userCtx, connect.NewRequest(&frontierv1beta1.GetCurrentUserRequest{})) + s.Require().Error(err) + s.Assert().Equal(connect.CodeUnauthenticated, connect.CodeOf(err)) + }) + + s.Run("the deleted user has no access left in any org or project it belonged to", func() { + for _, r := range resources { + s.Assert().Falsef(canGet(userSubject, r.resource), "access to %s", r.name) + } + }) + + s.Run("no live policy names the deleted user", func() { + s.Assert().Empty(livePoliciesMatching(s.T(), ctxOrgAdminAuth, s.testBench.Client, &frontierv1beta1.ListPoliciesRequest{UserId: userID})) + var count int + s.Require().NoError(s.dbc.GetContext(context.Background(), &count, + "SELECT count(*) FROM policies WHERE principal_id = $1 AND deleted_at IS NULL", userID)) + s.Assert().Zero(count) + }) + + s.Run("org users names the deleted user in neither users nor role pairs", func() { + for _, orgID := range []string{sharedOrgID, memberOrgID} { + inUsers, inRolePairs := listedInOrgUsers(orgID, userID) + s.Assert().Falsef(inUsers, "listed in the users of org %s", orgID) + s.Assert().Falsef(inRolePairs, "listed in the role pairs of org %s", orgID) + } + }) + + s.Run("the org that went with the user is deleted, and the orgs it left stay", func() { + s.Assert().False(orgIsLive(aloneOrgID)) + s.Assert().True(orgIsLive(sharedOrgID)) + s.Assert().True(orgIsLive(memberOrgID)) + _, err := s.testBench.Client.GetUser(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.GetUserRequest{Id: userID})) + s.Assert().Equal(connect.CodeNotFound, connect.CodeOf(err)) + }) + + s.Run("a service user can neither check nor delete an account", func() { + serviceUserResp, err := s.testBench.Client.CreateServiceUser(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateServiceUserRequest{OrgId: memberOrgID})) + s.Require().NoError(err) + credentialResp, err := s.testBench.Client.CreateServiceUserCredential(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateServiceUserCredentialRequest{ + Id: serviceUserResp.Msg.GetServiceuser().GetId(), + OrgId: memberOrgID, + })) + s.Require().NoError(err) + secret := credentialResp.Msg.GetSecret() + serviceUserCtx := testbench.ContextWithHeaders(context.Background(), map[string]string{ + "Authorization": "Basic " + base64.StdEncoding.EncodeToString([]byte(secret.GetId()+":"+secret.GetSecret())), + }) + + _, err = s.testBench.Client.CheckCurrentUserDelete(serviceUserCtx, connect.NewRequest(&frontierv1beta1.CheckCurrentUserDeleteRequest{})) + s.Assert().Equal(connect.CodePermissionDenied, connect.CodeOf(err)) + _, err = s.testBench.Client.DeleteCurrentUser(serviceUserCtx, connect.NewRequest(&frontierv1beta1.DeleteCurrentUserRequest{})) + s.Assert().Equal(connect.CodePermissionDenied, connect.CodeOf(err)) + _, err = s.testBench.Client.GetCurrentUser(serviceUserCtx, connect.NewRequest(&frontierv1beta1.GetCurrentUserRequest{})) + s.Assert().NoError(err) + }) +} + func (s *APIRegressionTestSuite) TestRelationAPI() { ctxOrgAdminAuth := testbench.ContextWithAuth(context.Background(), s.adminCookie) diff --git a/test/e2e/regression/pat_test.go b/test/e2e/regression/pat_test.go index 5c5ec683e4..07aaf5be72 100644 --- a/test/e2e/regression/pat_test.go +++ b/test/e2e/regression/pat_test.go @@ -861,6 +861,43 @@ func (s *PATRegressionTestSuite) TestPATOfDeletedOrgStopsAuthenticating() { }) } +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{ + Body: &frontierv1beta1.OrganizationRequestBody{Name: "org-pat-account-delete"}, + })) + s.Require().NoError(err) + _, patToken := s.createPAT(userCtx, orgResp.Msg.GetOrganization().GetId(), "pat-account-delete", []*frontierv1beta1.PATScope{ + {RoleId: s.roleID(schema.RoleOrganizationViewer), ResourceType: schema.OrganizationNamespace}, + }) + patCtx := getPATCtx(patToken) + _, err = s.testBench.Client.GetCurrentUser(patCtx, connect.NewRequest(&frontierv1beta1.GetCurrentUserRequest{})) + s.Require().NoError(err) + + s.Run("the token is refused for both calls", func() { + _, err := s.testBench.Client.CheckCurrentUserDelete(patCtx, connect.NewRequest(&frontierv1beta1.CheckCurrentUserDeleteRequest{})) + s.Assert().Equal(connect.CodePermissionDenied, connect.CodeOf(err)) + s.Assert().ErrorContains(err, "personal access token") + _, err = s.testBench.Client.DeleteCurrentUser(patCtx, connect.NewRequest(&frontierv1beta1.DeleteCurrentUserRequest{})) + s.Assert().Equal(connect.CodePermissionDenied, connect.CodeOf(err)) + s.Assert().ErrorContains(err, "personal access token") + }) + + s.Run("the check counts the token", func() { + resp, err := s.testBench.Client.CheckCurrentUserDelete(userCtx, connect.NewRequest(&frontierv1beta1.CheckCurrentUserDeleteRequest{})) + s.Require().NoError(err) + s.Assert().Equal(int32(1), resp.Msg.GetPatCount()) + }) + + s.Run("the token stops working once its owner deletes the account", func() { + _, err := s.testBench.Client.DeleteCurrentUser(userCtx, connect.NewRequest(&frontierv1beta1.DeleteCurrentUserRequest{})) + s.Require().NoError(err) + _, err = s.testBench.Client.GetCurrentUser(patCtx, connect.NewRequest(&frontierv1beta1.GetCurrentUserRequest{})) + s.Require().Error(err) + s.Assert().Equal(connect.CodeUnauthenticated, connect.CodeOf(err)) + }) +} + func TestEndToEndPATRegressionTestSuite(t *testing.T) { suite.Run(t, new(PATRegressionTestSuite)) } From bfa2f79f5b1d7c7496ddbdd9352f74e982c6e974 Mon Sep 17 00:00:00 2001 From: aman Date: Wed, 28 Oct 2026 21:45:50 +0530 Subject: [PATCH 2/3] refactor(api): log a failed delete check and share the session cookie clearing --- core/deleter/service.go | 4 +--- internal/api/v1beta1connect/authenticate.go | 8 ++++---- internal/api/v1beta1connect/user.go | 7 ++++--- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/core/deleter/service.go b/core/deleter/service.go index 0fbdfab22c..f0e267cc3e 100644 --- a/core/deleter/service.go +++ b/core/deleter/service.go @@ -678,9 +678,7 @@ func (d Service) DeleteUser(ctx context.Context, userID string) error { return d.userService.Delete(ctx, userID) } -// leaveOrg runs after the check, and the org can change in between. A user who -// is no longer a member, or an org that is gone, counts as done, and a user who -// is now the last owner blocks the delete. +// leaveOrg runs after the check, and the org can change in between. func (d Service) leaveOrg(ctx context.Context, userID string, org organization.Organization) error { err := d.membershipService.RemoveOrganizationMemberOnDelete(ctx, org.ID, userID, schema.UserPrincipal) switch { diff --git a/internal/api/v1beta1connect/authenticate.go b/internal/api/v1beta1connect/authenticate.go index 5608152190..2a3ce2ee4f 100644 --- a/internal/api/v1beta1connect/authenticate.go +++ b/internal/api/v1beta1connect/authenticate.go @@ -364,10 +364,12 @@ func (h *ConnectHandler) AuthLogout(ctx context.Context, request *connect.Reques } resp := connect.NewResponse(&frontierv1beta1.AuthLogoutResponse{}) + clearSessionCookie(resp) + return resp, nil +} - // instruct interceptor to invalidate cookie +func clearSessionCookie(resp connect.AnyResponse) { resp.Header().Set(consts.SessionDeleteGatewayKey, "true") - return resp, nil } func (h *ConnectHandler) getLoggedInSessionID(ctx context.Context) (uuid.UUID, error) { @@ -400,8 +402,6 @@ func (h *ConnectHandler) GetLoggedInPrincipal(ctx context.Context, via ...authen return principal, nil } -// getLoggedInUser returns the authenticated principal, rejecting service users -// and PATs. Use this for endpoints that act on the user's own account. func (h *ConnectHandler) getLoggedInUser(ctx context.Context) (authenticate.Principal, error) { principal, err := h.GetLoggedInPrincipal(ctx) if err != nil { diff --git a/internal/api/v1beta1connect/user.go b/internal/api/v1beta1connect/user.go index b1e834d3be..c20d4ea307 100644 --- a/internal/api/v1beta1connect/user.go +++ b/internal/api/v1beta1connect/user.go @@ -22,7 +22,6 @@ import ( "github.com/raystack/frontier/internal/store/postgres" "github.com/raystack/frontier/pkg/metadata" "github.com/raystack/frontier/pkg/pagination" - "github.com/raystack/frontier/pkg/server/consts" "github.com/raystack/frontier/pkg/str" "github.com/raystack/frontier/pkg/utils" frontierv1beta1 "github.com/raystack/frontier/proto/v1beta1" @@ -417,12 +416,15 @@ func (h *ConnectHandler) DeleteUser(ctx context.Context, request *connect.Reques } func (h *ConnectHandler) CheckCurrentUserDelete(ctx context.Context, request *connect.Request[frontierv1beta1.CheckCurrentUserDeleteRequest]) (*connect.Response[frontierv1beta1.CheckCurrentUserDeleteResponse], error) { + errorLogger := NewErrorLogger() + principal, err := h.getLoggedInUser(ctx) if err != nil { return nil, err } check, err := h.deleterService.CheckUserDelete(ctx, principal.ID) if err != nil { + errorLogger.LogServiceError(ctx, request, "CheckCurrentUserDelete", err, "user_id", principal.ID) return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("CheckCurrentUserDelete: user_id=%s: %w", principal.ID, err)) } return connect.NewResponse(transformUserDeleteCheckToPB(check)), nil @@ -437,8 +439,7 @@ func (h *ConnectHandler) DeleteCurrentUser(ctx context.Context, request *connect return nil, err } resp := connect.NewResponse(&frontierv1beta1.DeleteCurrentUserResponse{}) - // instruct interceptor to invalidate cookie - resp.Header().Set(consts.SessionDeleteGatewayKey, "true") + clearSessionCookie(resp) return resp, nil } From c6135acb1285e63823e001660169e5c8f487e913 Mon Sep 17 00:00:00 2001 From: aman Date: Wed, 28 Oct 2026 21:45:50 +0530 Subject: [PATCH 3/3] test(e2e): a deleted account's email signs up again, and the delete writes its audit entries --- core/deleter/service_test.go | 1 - internal/api/v1beta1connect/user_test.go | 3 +-- test/e2e/regression/api_test.go | 23 +++++++++++++++++++++++ 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/core/deleter/service_test.go b/core/deleter/service_test.go index 39cb83dd64..fb42d29883 100644 --- a/core/deleter/service_test.go +++ b/core/deleter/service_test.go @@ -1994,7 +1994,6 @@ func TestDeleteUser(t *testing.T) { expectUserOrg(m, shared, []membership.Member{orgMember(userID, orgOwner), orgMember("user-2", orgOwner)}, userID, "user-2") m.mbrSvc.EXPECT().RemoveOrganizationMemberOnDelete(mock.Anything, shared.ID, userID, schema.UserPrincipal). Return(membership.ErrLastOwnerRole) - // RemovePrincipalPolicies, PAT cleanup and usrSvc.Delete must NOT be called — strict mocks fail on unexpected calls. err := m.build().DeleteUser(context.Background(), userID) var blocked *deleter.BlockedError diff --git a/internal/api/v1beta1connect/user_test.go b/internal/api/v1beta1connect/user_test.go index 9e47504cfc..635648cd6a 100644 --- a/internal/api/v1beta1connect/user_test.go +++ b/internal/api/v1beta1connect/user_test.go @@ -911,8 +911,7 @@ func TestConnectHandler_DeleteUser(t *testing.T) { if tt.err == connect.Code(0) { assert.NoError(t, err) assert.EqualValues(t, tt.want, resp.Msg) - // the admin's own session cookie stays - assert.Empty(t, resp.Header().Get(consts.SessionDeleteGatewayKey)) + assert.Empty(t, resp.Header().Get(consts.SessionDeleteGatewayKey), "the admin's own session cookie stays") } else { assert.Nil(t, resp) assert.Equal(t, tt.err, connect.CodeOf(err)) diff --git a/test/e2e/regression/api_test.go b/test/e2e/regression/api_test.go index 29cda5a01a..d2463a0114 100644 --- a/test/e2e/regression/api_test.go +++ b/test/e2e/regression/api_test.go @@ -2413,6 +2413,29 @@ func (s *APIRegressionTestSuite) TestDeleteCurrentUser() { s.Assert().Equal(connect.CodeNotFound, connect.CodeOf(err)) }) + s.Run("the delete writes one app.user.deleted log and one user.deleted record", func() { + var logs, records int + s.Require().NoError(s.dbc.GetContext(context.Background(), &logs, + "SELECT count(*) FROM auditlogs WHERE action = $1 AND org_id = $2 AND target->>'ID' = $3", + "app.user.deleted", schema.PlatformOrgID.String(), userID)) + s.Require().NoError(s.dbc.GetContext(context.Background(), &records, + "SELECT count(*) FROM audit_records WHERE event = $1 AND target_id = $2", "user.deleted", userID)) + s.Assert().Equal(1, logs) + s.Assert().Equal(1, records) + }) + + s.Run("the same email signs up again as a new user", func() { + newID, newCtx := signUpUser(s.T(), s.testBench.Client, "delete-current-user@raystack.org") + s.Assert().NotEqual(userID, newID) + _, err := s.testBench.Client.GetUser(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.GetUserRequest{Id: userID})) + s.Assert().Equal(connect.CodeNotFound, connect.CodeOf(err)) + + resp, err := s.testBench.Client.CheckCurrentUserDelete(newCtx, connect.NewRequest(&frontierv1beta1.CheckCurrentUserDeleteRequest{})) + s.Require().NoError(err) + s.Assert().Empty(resp.Msg.GetOrganizationsToDelete()) + s.Assert().Empty(resp.Msg.GetOrganizationsToLeave()) + }) + s.Run("a service user can neither check nor delete an account", func() { serviceUserResp, err := s.testBench.Client.CreateServiceUser(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateServiceUserRequest{OrgId: memberOrgID})) s.Require().NoError(err)