From 1603ee0ea8358ad6cf143bca3158a082f47d7fc2 Mon Sep 17 00:00:00 2001 From: Lucas Manuel Rodriguez Date: Wed, 19 Apr 2023 18:43:15 -0300 Subject: [PATCH] `/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 | 1 + server/fleet/hostresponse.go | 4 ++-- server/service/hosts.go | 11 ++--------- server/service/integration_core_test.go | 9 +++++---- server/service/labels.go | 6 +----- server/service/transport.go | 3 ++- 6 files changed, 13 insertions(+), 21 deletions(-) create mode 100644 changes/10880-api-fleet-hosts-cleanup diff --git a/changes/10880-api-fleet-hosts-cleanup b/changes/10880-api-fleet-hosts-cleanup new file mode 100644 index 000000000..66cef5ce8 --- /dev/null +++ b/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. diff --git a/server/fleet/hostresponse.go b/server/fleet/hostresponse.go index cc5aa163d..8042190bb 100644 --- a/server/fleet/hostresponse.go +++ b/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. diff --git a/server/service/hosts.go b/server/service/hosts.go index 97a5f49f8..b2ca3afa7 100644 --- a/server/service/hosts.go +++ b/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 diff --git a/server/service/integration_core_test.go b/server/service/integration_core_test.go index 7fdfbf3ae..ea36e7a3b 100644 --- a/server/service/integration_core_test.go +++ b/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"})) diff --git a/server/service/labels.go b/server/service/labels.go index f39aadbed..799b17fae 100644 --- a/server/service/labels.go +++ b/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 diff --git a/server/service/transport.go b/server/service/transport.go index 09d90d4af..5d7296ddb 100644 --- a/server/service/transport.go +++ b/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")