/api/_version_/fleet/hosts to return bad request instead of server error when passing invalid mdm_enrollment_status (#11242)

#10880

I was not able to reproduce other 500s in `/api/_version_/fleet/hosts`
other than the one fixed in the PR.

- [X] Changes file added for user-visible changes in `changes/` or
`orbit/changes/`.
See [Changes
files](https://fleetdm.com/docs/contributing/committing-changes#changes-files)
for more information.
- ~[ ] Documented any API changes (docs/Using-Fleet/REST-API.md or
docs/Contributing/API-for-contributors.md)~
- ~[ ] Documented any permissions changes~
- ~[ ] Input data is properly validated, `SELECT *` is avoided, SQL
injection is prevented (using placeholders for values in statements)~
- ~[ ] Added support on fleet's osquery simulator `cmd/osquery-perf` for
new osquery data ingestion features.~
- [X] Added/updated tests
- [X] Manual QA for all new/changed functionality
  - ~For Orbit and Fleet Desktop changes:~
- ~[ ] Manual QA must be performed in the three main OSs, macOS, Windows
and Linux.~
- ~[ ] Auto-update manual QA, from released version of component to new
version (see [tools/tuf/test](../tools/tuf/test/README.md)).~

changes/10880-api-fleet-hosts-cleanup

@@ -0,0 +1 @@
+* Fix a HTTP 500 on `GET /api/_version_/fleet/hosts` returned when `mdm_enrollment_status` is invalid.

server/fleet/hostresponse.go

@@ -19,10 +19,10 @@ type HostResponse struct {
 }
 
 // HostResponseForHost returns a HostResponse from Host with Geolocation.
-func HostResponseForHost(ctx context.Context, svc Service, host *Host) (*HostResponse, error) {
+func HostResponseForHost(ctx context.Context, svc Service, host *Host) *HostResponse {
 	hr := HostResponseForHostCheap(host)
 	hr.Geolocation = svc.LookupGeoIP(ctx, host.PublicIP)
-	return hr, nil
+	return hr
 }
 
 // HostResponseForHostCheap returns a new HostResponse from a Host without computing Geolocation.

server/service/hosts.go

@@ -106,11 +106,7 @@ func listHostsEndpoint(ctx context.Context, request interface{}, svc fleet.Servi
 
 	hostResponses := make([]fleet.HostResponse, len(hosts))
 	for i, host := range hosts {
-		h, err := fleet.HostResponseForHost(ctx, svc, host)
-		if err != nil {
-			return listHostsResponse{Err: err}, nil
-		}
-
+		h := fleet.HostResponseForHost(ctx, svc, host)
 		hostResponses[i] = *h
 	}
 	return listHostsResponse{
@@ -1337,10 +1333,7 @@ func hostsReportEndpoint(ctx context.Context, request interface{}, svc fleet.Ser
 
 	hostResps := make([]*fleet.HostResponse, len(hosts))
 	for i, h := range hosts {
-		hr, err := fleet.HostResponseForHost(ctx, svc, h)
-		if err != nil {
-			return hostsReportResponse{Err: err}, nil
-		}
+		hr := fleet.HostResponseForHost(ctx, svc, h)
 		hostResps[i] = hr
 	}
 	return hostsReportResponse{Columns: cols, Hosts: hostResps}, nil

server/service/integration_core_test.go

@@ -1244,9 +1244,6 @@ func (s *integrationTestSuite) TestListHosts() {
 	})
 
 	s.DoJSON("GET", "/api/latest/fleet/hosts", nil, http.StatusOK, &resp)
-	for _, h := range resp.Hosts {
-		fmt.Println("host", fmt.Sprintf("%+v", h.Host.HardwareSerial))
-	}
 
 	// set MDM information for another host installed from DEP and pending enrollment to Fleet MDM
 	pendingMDMHost, err := s.ds.NewHost(context.Background(), &fleet.Host{
@@ -1312,7 +1309,11 @@ func (s *integrationTestSuite) TestListHosts() {
 	assert.Equal(t, mdmID, resp.MDMSolution.ID)
 
 	resp = listHostsResponse{}
-	s.DoJSON("GET", "/api/latest/fleet/hosts", nil, http.StatusInternalServerError, &resp, "mdm_enrollment_status", "invalid-status") // TODO: to be addressed by #4406
+	s.DoJSON("GET", "/api/latest/fleet/hosts", nil, http.StatusBadRequest, &resp, "mdm_enrollment_status", "invalid-status")
+
+	// Filter by inexistent software.
+	resp = listHostsResponse{}
+	s.DoJSON("GET", "/api/latest/fleet/hosts", nil, http.StatusNotFound, &resp, "software_id", fmt.Sprint(9999))
 
 	// set munki information on a host
 	require.NoError(t, s.ds.SetOrUpdateMunkiInfo(context.Background(), host.ID, "1.2.3", []string{"err"}, []string{"warn"}))

server/service/labels.go

@@ -272,11 +272,7 @@ func listHostsInLabelEndpoint(ctx context.Context, request interface{}, svc flee
 
 	hostResponses := make([]fleet.HostResponse, len(hosts))
 	for i, host := range hosts {
-		h, err := fleet.HostResponseForHost(ctx, svc, host)
-		if err != nil {
-			return listHostsResponse{Err: err}, nil
-		}
-
+		h := fleet.HostResponseForHost(ctx, svc, host)
 		hostResponses[i] = *h
 	}
 	return listHostsResponse{Hosts: hostResponses, MDMSolution: mdmSolution}, nil

server/service/transport.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	"fmt"
 	"io"
 	"net/http"
 	"net/url"
@@ -313,7 +314,7 @@ func hostListOptionsFromRequest(r *http.Request) (fleet.HostListOptions, error)
 	case "":
 		// No error when unset
 	default:
-		return hopt, ctxerr.Errorf(r.Context(), "invalid mdm enrollment status %s", enrollmentStatus)
+		return hopt, ctxerr.Wrap(r.Context(), badRequest(fmt.Sprintf("invalid mdm enrollment status %s", enrollmentStatus)))
 	}
 
 	macOSSettingsStatus := r.URL.Query().Get("macos_settings")