fix(deleter): user delete removes every policy the user holds

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)
 	}

core/deleter/service_test.go

@@ -1828,23 +1828,56 @@ 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) {
 		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(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.
 

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)