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)