From 08ae2bc73224cf2566142ee344657c34536a98d9 Mon Sep 17 00:00:00 2001 From: aman Date: Sun, 25 Oct 2026 12:07:56 +0530 Subject: [PATCH] fix(deleter): user delete removes every policy the user holds DeleteUser deletes every live policy of the user after the org removals and before the PATs and the user row. Policies on disabled projects and groups, on projects in orgs where the user has no org-level policy, and on resources in disabled projects stayed live before. --- core/deleter/mocks/membership_service.go | 48 ++++++++ core/deleter/service.go | 4 + core/deleter/service_test.go | 41 ++++++- test/e2e/regression/api_test.go | 135 +++++++++++++++++++++++ 4 files changed, 224 insertions(+), 4 deletions(-) diff --git a/core/deleter/mocks/membership_service.go b/core/deleter/mocks/membership_service.go index 0ff75d628..c99693ae9 100644 --- a/core/deleter/mocks/membership_service.go +++ b/core/deleter/mocks/membership_service.go @@ -242,6 +242,54 @@ func (_c *MembershipService_OnGroupDeleted_Call) RunAndReturn(run func(context.C 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) + + if len(ret) == 0 { + panic("no return value specified for RemovePrincipalPolicies") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string, string) error); ok { + r0 = rf(ctx, principalID, principalType) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// MembershipService_RemovePrincipalPolicies_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'RemovePrincipalPolicies' +type MembershipService_RemovePrincipalPolicies_Call struct { + *mock.Call +} + +// RemovePrincipalPolicies is a helper method to define mock.On call +// - ctx context.Context +// - principalID string +// - principalType string +func (_e *MembershipService_Expecter) RemovePrincipalPolicies(ctx interface{}, principalID interface{}, principalType interface{}) *MembershipService_RemovePrincipalPolicies_Call { + return &MembershipService_RemovePrincipalPolicies_Call{Call: _e.mock.On("RemovePrincipalPolicies", ctx, principalID, principalType)} +} + +func (_c *MembershipService_RemovePrincipalPolicies_Call) Run(run func(ctx context.Context, principalID string, principalType string)) *MembershipService_RemovePrincipalPolicies_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(string), args[2].(string)) + }) + return _c +} + +func (_c *MembershipService_RemovePrincipalPolicies_Call) Return(_a0 error) *MembershipService_RemovePrincipalPolicies_Call { + _c.Call.Return(_a0) + return _c +} + +func (_c *MembershipService_RemovePrincipalPolicies_Call) RunAndReturn(run func(context.Context, string, string) error) *MembershipService_RemovePrincipalPolicies_Call { + _c.Call.Return(run) + return _c +} + // NewMembershipService creates a new instance of MembershipService. 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 NewMembershipService(t interface { diff --git a/core/deleter/service.go b/core/deleter/service.go index d3a5b6b76..4068f4c82 100644 --- a/core/deleter/service.go +++ b/core/deleter/service.go @@ -84,6 +84,7 @@ type MembershipService interface { 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 + RemovePrincipalPolicies(ctx context.Context, principalID, principalType string) error } type InvitationService interface { @@ -568,6 +569,9 @@ func (d Service) DeleteUser(ctx context.Context, userID string) error { return fmt.Errorf("failed to delete user from org[%s]: %w", orgID, err) } } + if err := d.membershipService.RemovePrincipalPolicies(ctx, userID, schema.UserPrincipal); err != nil { + return fmt.Errorf("failed to delete user policies: %w", err) + } if err := d.userPATService.DeleteAllByUser(ctx, userID); err != nil { return fmt.Errorf("failed to delete user PATs: %w", err) } diff --git a/core/deleter/service_test.go b/core/deleter/service_test.go index 8bac6b744..0c84d8540 100644 --- a/core/deleter/service_test.go +++ b/core/deleter/service_test.go @@ -1828,16 +1828,48 @@ func TestRemoveUsersFromOrg(t *testing.T) { } func TestDeleteUser(t *testing.T) { - t.Run("removes user from all orgs, cleans PATs, then deletes user", func(t *testing.T) { + t.Run("removes the user from each org, then every policy it still holds, then its PATs, then the user", func(t *testing.T) { m := newMocks(t) + var steps []string m.mbrSvc.EXPECT().ListResourcesByPrincipal(mock.Anything, mock.Anything, schema.OrganizationNamespace, mock.Anything). - Return(nil, nil) - m.patSvc.EXPECT().DeleteAllByUser(mock.Anything, "user-1").Return(nil) - m.usrSvc.EXPECT().Delete(mock.Anything, "user-1").Return(nil) + Return([]string{"org-1", "org-2"}, nil) + m.mbrSvc.EXPECT().ForceRemoveOrganizationMember(mock.Anything, mock.Anything, "user-1", schema.UserPrincipal). + Run(func(_ context.Context, orgID, _, _ string) { steps = append(steps, "org:"+orgID) }).Return(nil) + m.mbrSvc.EXPECT().RemovePrincipalPolicies(mock.Anything, "user-1", schema.UserPrincipal). + Run(func(context.Context, string, string) { steps = append(steps, "policies") }).Return(nil) + m.patSvc.EXPECT().DeleteAllByUser(mock.Anything, "user-1"). + Run(func(context.Context, string) { steps = append(steps, "pats") }).Return(nil) + m.usrSvc.EXPECT().Delete(mock.Anything, "user-1"). + Run(func(context.Context, string) { steps = append(steps, "user") }).Return(nil) err := m.build().DeleteUser(context.Background(), "user-1") assert.NoError(t, err) + assert.Equal(t, []string{"org:org-1", "org:org-2", "policies", "pats", "user"}, steps) + }) + + t.Run("an org removal failure stops the delete before the remaining policies", func(t *testing.T) { + m := newMocks(t) + + m.mbrSvc.EXPECT().ListResourcesByPrincipal(mock.Anything, mock.Anything, schema.OrganizationNamespace, mock.Anything). + Return([]string{"org-1"}, nil) + m.mbrSvc.EXPECT().ForceRemoveOrganizationMember(mock.Anything, "org-1", "user-1", schema.UserPrincipal). + Return(errors.New("cascade boom")) + + err := m.build().DeleteUser(context.Background(), "user-1") + assert.ErrorContains(t, err, "cascade boom") + }) + + t.Run("a policy removal failure stops the delete before the PATs and the user", func(t *testing.T) { + m := newMocks(t) + + m.mbrSvc.EXPECT().ListResourcesByPrincipal(mock.Anything, mock.Anything, schema.OrganizationNamespace, mock.Anything). + Return(nil, nil) + m.mbrSvc.EXPECT().RemovePrincipalPolicies(mock.Anything, "user-1", schema.UserPrincipal). + Return(errors.New("spicedb unavailable")) + + err := m.build().DeleteUser(context.Background(), "user-1") + assert.ErrorContains(t, err, "spicedb unavailable") }) t.Run("aborts before userService.Delete when PAT cleanup fails", func(t *testing.T) { @@ -1845,6 +1877,7 @@ func TestDeleteUser(t *testing.T) { m.mbrSvc.EXPECT().ListResourcesByPrincipal(mock.Anything, mock.Anything, schema.OrganizationNamespace, mock.Anything). Return(nil, nil) + m.mbrSvc.EXPECT().RemovePrincipalPolicies(mock.Anything, "user-1", schema.UserPrincipal).Return(nil) m.patSvc.EXPECT().DeleteAllByUser(mock.Anything, "user-1").Return(errors.New("pat cleanup boom")) // usrSvc.Delete must NOT be called — strict mock fails on unexpected call. diff --git a/test/e2e/regression/api_test.go b/test/e2e/regression/api_test.go index 62be22653..6fed753af 100644 --- a/test/e2e/regression/api_test.go +++ b/test/e2e/regression/api_test.go @@ -1727,6 +1727,141 @@ func (s *APIRegressionTestSuite) TestUserAPI() { }) } +func (s *APIRegressionTestSuite) TestUserDeleteLeavesNoAccessAndNoLivePolicy() { + ctxOrgAdminAuth := testbench.ContextWithAuth(context.Background(), s.adminCookie) + + createOrg := func(name string) string { + resp, err := s.testBench.Client.CreateOrganization(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateOrganizationRequest{ + Body: &frontierv1beta1.OrganizationRequestBody{Title: name, Name: name}, + })) + s.Require().NoError(err) + return resp.Msg.GetOrganization().GetId() + } + createProject := func(orgID, name string) string { + resp, err := s.testBench.Client.CreateProject(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateProjectRequest{ + Body: &frontierv1beta1.ProjectRequestBody{Name: name, OrgId: orgID}, + })) + s.Require().NoError(err) + return resp.Msg.GetProject().GetId() + } + + memberOrgID := createOrg("user-delete-member-org") + otherOrgID := createOrg("user-delete-other-org") + disabledProjectID := createProject(memberOrgID, "user-delete-disabled-project") + otherOrgProjectID := createProject(otherOrgID, "user-delete-other-org-project") + + orderResp, err := s.testBench.Client.CreateProjectResource(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateProjectResourceRequest{ + ProjectId: disabledProjectID, + Body: &frontierv1beta1.ResourceRequestBody{Name: "user-delete-order", Namespace: computeOrderNamespace}, + })) + s.Require().NoError(err) + orderID := orderResp.Msg.GetResource().GetId() + + groupResp, err := s.testBench.Client.CreateGroup(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateGroupRequest{ + OrgId: memberOrgID, + Body: &frontierv1beta1.GroupRequestBody{Name: "user-delete-disabled-group"}, + })) + s.Require().NoError(err) + disabledGroupID := groupResp.Msg.GetGroup().GetId() + + userResp, err := s.testBench.Client.CreateUser(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateUserRequest{ + Body: &frontierv1beta1.UserRequestBody{Email: "user-delete-target@raystack.org", Name: "user-delete-target"}, + })) + s.Require().NoError(err) + userID := userResp.Msg.GetUser().GetId() + userSubject := schema.JoinNamespaceAndResourceID(schema.UserPrincipal, userID) + + addMembersResp, err := s.testBench.AdminClient.AddOrganizationMembers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationMembersRequest{ + OrgId: memberOrgID, + Members: []*frontierv1beta1.OrgMemberEntry{{UserId: userID, RoleId: s.orgViewerRole}}, + })) + requireAddOrgMembersSuccess(s.T(), addMembersResp, err) + _, err = s.testBench.Client.SetGroupMemberRole(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.SetGroupMemberRoleRequest{ + GroupId: disabledGroupID, + PrincipalId: userID, + PrincipalType: schema.UserPrincipal, + RoleId: s.groupMemberRole, + })) + s.Require().NoError(err) + + disabledProject := schema.JoinNamespaceAndResourceID(schema.ProjectNamespace, disabledProjectID) + otherOrgProject := schema.JoinNamespaceAndResourceID(schema.ProjectNamespace, otherOrgProjectID) + order := schema.JoinNamespaceAndResourceID(computeOrderNamespace, orderID) + for _, grant := range []struct{ role, resource string }{ + {schema.RoleProjectViewer, disabledProject}, + {schema.RoleProjectViewer, otherOrgProject}, + {computeViewerRoleName, order}, + } { + _, err := s.testBench.Client.CreatePolicy(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreatePolicyRequest{ + Body: &frontierv1beta1.PolicyRequestBody{RoleId: grant.role, Resource: grant.resource, Principal: userSubject}, + })) + s.Require().NoError(err) + } + + _, err = s.testBench.Client.DisableProject(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DisableProjectRequest{Id: disabledProjectID})) + s.Require().NoError(err) + _, err = s.testBench.Client.DisableGroup(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DisableGroupRequest{Id: disabledGroupID})) + s.Require().NoError(err) + + resources := []struct { + name string + id string + resource string + }{ + {"the org it is a member of", memberOrgID, schema.JoinNamespaceAndResourceID(schema.OrganizationNamespace, memberOrgID)}, + {"a disabled project", disabledProjectID, disabledProject}, + {"a disabled group", disabledGroupID, schema.JoinNamespaceAndResourceID(schema.GroupNamespace, disabledGroupID)}, + {"a project in an org it is not a member of", otherOrgProjectID, otherOrgProject}, + {"an order in a disabled project", orderID, order}, + } + livePolicies := func(resourceID string) int { + var count int + s.Require().NoError(s.dbc.GetContext(context.Background(), &count, + "SELECT count(*) FROM policies WHERE principal_id = $1 AND resource_id = $2 AND deleted_at IS NULL", userID, resourceID)) + return count + } + canGet := func(resource string) bool { + resp, err := s.testBench.AdminClient.CheckFederatedResourcePermission(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CheckFederatedResourcePermissionRequest{ + Resource: resource, + Permission: schema.GetPermission, + Subject: userSubject, + })) + s.Require().NoError(err) + return resp.Msg.GetStatus() + } + for _, r := range resources { + s.Require().Equalf(1, livePolicies(r.id), "live policy on %s before the delete", r.name) + s.Require().Truef(canGet(r.resource), "access to %s before the delete", r.name) + } + + _, err = s.testBench.Client.DeleteUser(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteUserRequest{Id: userID})) + s.Require().NoError(err) + + s.Run("no live policy names the deleted user", func() { + for _, r := range resources { + s.Assert().Zerof(livePolicies(r.id), "live policy on %s", r.name) + } + s.Assert().Empty(livePoliciesMatching(s.T(), ctxOrgAdminAuth, s.testBench.Client, &frontierv1beta1.ListPoliciesRequest{UserId: userID})) + }) + + s.Run("the deleted user can no longer get anything it had access to", func() { + for _, r := range resources { + s.Assert().Falsef(canGet(r.resource), "access to %s", r.name) + } + }) + + s.Run("org users names the deleted user in neither users nor role pairs", func() { + resp, err := s.testBench.Client.ListOrganizationUsers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.ListOrganizationUsersRequest{Id: memberOrgID})) + s.Require().NoError(err) + for _, u := range resp.Msg.GetUsers() { + s.Assert().NotEqual(userID, u.GetId()) + } + for _, pair := range resp.Msg.GetRolePairs() { + s.Assert().NotEqual(userID, pair.GetUserId()) + } + }) +} + func (s *APIRegressionTestSuite) TestRelationAPI() { ctxOrgAdminAuth := testbench.ContextWithAuth(context.Background(), s.adminCookie)