From 5391b686cf23138642d2db3ca99ba5e2315ace59 Mon Sep 17 00:00:00 2001 From: Victor Lyuboslavsky Date: Mon, 6 Nov 2023 08:06:02 -0600 Subject: [PATCH] Fixing hosts/delete input validation due to QA issue. (#14942) Frontend needs to be able to delete all hosts. However, the API requirement is that host ids or filters must be specified when deleting hosts. The solution is to allow an empty filter to delete all hosts, like: `"filters":{}` REST API updated documentation here: https://github.com/fleetdm/fleet/pull/14952 [x] Tests updated and added. [x] Manual testing done. --- server/fleet/service.go | 2 +- server/service/hosts.go | 47 +++++++++++++++---------- server/service/hosts_test.go | 4 +-- server/service/integration_core_test.go | 47 +++++++++++++++---------- 4 files changed, 60 insertions(+), 40 deletions(-) diff --git a/server/fleet/service.go b/server/fleet/service.go index c6435b4c8e5b..20875548fc74 100644 --- a/server/fleet/service.go +++ b/server/fleet/service.go @@ -339,7 +339,7 @@ type Service interface { // AddHostsToTeamByFilter adds hosts to an existing team, clearing their team settings if teamID is nil. Hosts are // selected by the label and HostListOptions provided. AddHostsToTeamByFilter(ctx context.Context, teamID *uint, opt HostListOptions, lid *uint) error - DeleteHosts(ctx context.Context, ids []uint, opt HostListOptions, lid *uint) error + DeleteHosts(ctx context.Context, ids []uint, opt *HostListOptions, lid *uint) error CountHosts(ctx context.Context, labelID *uint, opts HostListOptions) (int, error) // SearchHosts performs a search on the hosts table using the following criteria: // - matchQuery is the query SQL diff --git a/server/service/hosts.go b/server/service/hosts.go index 8b85b772b013..f02f6a9a7a5d 100644 --- a/server/service/hosts.go +++ b/server/service/hosts.go @@ -167,14 +167,17 @@ var ( deleteHostsSkipAuthorization = false ) +type deleteHostsFilters struct { + MatchQuery string `json:"query"` + Status fleet.HostStatus `json:"status"` + LabelID *uint `json:"label_id"` + TeamID *uint `json:"team_id"` +} + type deleteHostsRequest struct { - IDs []uint `json:"ids"` - Filters struct { - MatchQuery string `json:"query"` - Status fleet.HostStatus `json:"status"` - LabelID *uint `json:"label_id"` - TeamID *uint `json:"team_id"` - } `json:"filters"` + IDs []uint `json:"ids"` + // Using a pointer to help determine whether an empty filter was passed, like: "filters":{} + Filters *deleteHostsFilters `json:"filters"` } type deleteHostsResponse struct { @@ -189,12 +192,17 @@ func (r deleteHostsResponse) Status() int { return r.StatusCode } func deleteHostsEndpoint(ctx context.Context, request interface{}, svc fleet.Service) (errorer, error) { req := request.(*deleteHostsRequest) - listOpt := fleet.HostListOptions{ - ListOptions: fleet.ListOptions{ - MatchQuery: req.Filters.MatchQuery, - }, - StatusFilter: req.Filters.Status, - TeamFilter: req.Filters.TeamID, + var listOpts *fleet.HostListOptions + var labelID *uint + if req.Filters != nil { + listOpts = &fleet.HostListOptions{ + ListOptions: fleet.ListOptions{ + MatchQuery: req.Filters.MatchQuery, + }, + StatusFilter: req.Filters.Status, + TeamFilter: req.Filters.TeamID, + } + labelID = req.Filters.LabelID } // Since bulk deletes can take a long time, after DeleteHostsTimeout, we will return a 202 (Accepted) status code @@ -203,7 +211,7 @@ func deleteHostsEndpoint(ctx context.Context, request interface{}, svc fleet.Ser deleteDone := make(chan bool, 1) ctx = context.WithoutCancel(ctx) // to make sure DB operations don't get killed after we return a 202 go func() { - err = svc.DeleteHosts(ctx, req.IDs, listOpt, req.Filters.LabelID) + err = svc.DeleteHosts(ctx, req.IDs, listOpts, labelID) if err != nil { // logging the error for future debug in case we already sent http.StatusAccepted logging.WithErr(ctx, err) @@ -225,16 +233,16 @@ func deleteHostsEndpoint(ctx context.Context, request interface{}, svc fleet.Ser } } -func (svc *Service) DeleteHosts(ctx context.Context, ids []uint, opts fleet.HostListOptions, lid *uint) error { +func (svc *Service) DeleteHosts(ctx context.Context, ids []uint, opts *fleet.HostListOptions, lid *uint) error { if err := svc.authz.Authorize(ctx, &fleet.Host{}, fleet.ActionList); err != nil { return err } - if len(ids) == 0 && lid == nil && opts.Empty() { + if len(ids) == 0 && lid == nil && opts == nil { return &fleet.BadRequestError{Message: "list of ids or filters must be specified"} } - if len(ids) > 0 && (lid != nil || !opts.Empty()) { + if len(ids) > 0 && (lid != nil || (opts != nil && !opts.Empty())) { return &fleet.BadRequestError{Message: "Cannot specify a list of ids and filters at the same time"} } @@ -246,8 +254,11 @@ func (svc *Service) DeleteHosts(ctx context.Context, ids []uint, opts fleet.Host return svc.ds.DeleteHosts(ctx, ids) } + if opts == nil { + opts = &fleet.HostListOptions{} + } opts.DisableFailingPolicies = true // don't check policies for hosts that are about to be deleted - hostIDs, _, err := svc.hostIDsAndNamesFromFilters(ctx, opts, lid) + hostIDs, _, err := svc.hostIDsAndNamesFromFilters(ctx, *opts, lid) if err != nil { return err } diff --git a/server/service/hosts_test.go b/server/service/hosts_test.go index 178082468a34..b2c173568b27 100644 --- a/server/service/hosts_test.go +++ b/server/service/hosts_test.go @@ -667,10 +667,10 @@ func TestHostAuth(t *testing.T) { err = svc.DeleteHost(ctx, 2) checkAuthErr(t, tt.shouldFailGlobalWrite, err) - err = svc.DeleteHosts(ctx, []uint{1}, fleet.HostListOptions{}, nil) + err = svc.DeleteHosts(ctx, []uint{1}, nil, nil) checkAuthErr(t, tt.shouldFailTeamWrite, err) - err = svc.DeleteHosts(ctx, []uint{2}, fleet.HostListOptions{}, nil) + err = svc.DeleteHosts(ctx, []uint{2}, &fleet.HostListOptions{}, nil) checkAuthErr(t, tt.shouldFailGlobalWrite, err) err = svc.AddHostsToTeam(ctx, ptr.Uint(1), []uint{1}, false) diff --git a/server/service/integration_core_test.go b/server/service/integration_core_test.go index 7d3d9ee343ac..cc0b091937ef 100644 --- a/server/service/integration_core_test.go +++ b/server/service/integration_core_test.go @@ -878,12 +878,7 @@ func (s *integrationTestSuite) TestBulkDeleteHostsFromTeam() { require.NoError(t, s.ds.AddHostsToTeam(context.Background(), &team1.ID, []uint{hosts[0].ID})) req := deleteHostsRequest{ - Filters: struct { - MatchQuery string `json:"query"` - Status fleet.HostStatus `json:"status"` - LabelID *uint `json:"label_id"` - TeamID *uint `json:"team_id"` - }{TeamID: ptr.Uint(team1.ID)}, + Filters: &deleteHostsFilters{TeamID: ptr.Uint(team1.ID)}, } resp := deleteHostsResponse{} s.DoJSON("POST", "/api/latest/fleet/hosts/delete", req, http.StatusOK, &resp) @@ -920,12 +915,7 @@ func (s *integrationTestSuite) TestBulkDeleteHostsInLabel() { require.NoError(t, s.ds.RecordLabelQueryExecutions(context.Background(), hosts[2], map[uint]*bool{label.ID: ptr.Bool(true)}, time.Now(), false)) req := deleteHostsRequest{ - Filters: struct { - MatchQuery string `json:"query"` - Status fleet.HostStatus `json:"status"` - LabelID *uint `json:"label_id"` - TeamID *uint `json:"team_id"` - }{LabelID: ptr.Uint(label.ID)}, + Filters: &deleteHostsFilters{LabelID: ptr.Uint(label.ID)}, } resp := deleteHostsResponse{} s.DoJSON("POST", "/api/latest/fleet/hosts/delete", req, http.StatusOK, &resp) @@ -1001,6 +991,26 @@ func (s *integrationTestSuite) TestBulkDeleteHostByIDsWithTimeout() { } } +func (s *integrationTestSuite) TestBulkDeleteHostsAll() { + t := s.T() + + hosts := s.createHosts(t) + + // All hosts should be deleted when an empty filter is specified + req := deleteHostsRequest{ + Filters: &deleteHostsFilters{}, + } + resp := deleteHostsResponse{} + s.DoJSON("POST", "/api/latest/fleet/hosts/delete", req, http.StatusOK, &resp) + + _, err := s.ds.Host(context.Background(), hosts[0].ID) + require.Error(t, err) + _, err = s.ds.Host(context.Background(), hosts[1].ID) + require.Error(t, err) + _, err = s.ds.Host(context.Background(), hosts[2].ID) + require.Error(t, err) +} + func (s *integrationTestSuite) createHosts(t *testing.T, platforms ...string) []*fleet.Host { var hosts []*fleet.Host if len(platforms) == 0 { @@ -1030,16 +1040,15 @@ func (s *integrationTestSuite) TestBulkDeleteHostsErrors() { hosts := s.createHosts(t) req := deleteHostsRequest{ - IDs: []uint{hosts[0].ID, hosts[1].ID}, - Filters: struct { - MatchQuery string `json:"query"` - Status fleet.HostStatus `json:"status"` - LabelID *uint `json:"label_id"` - TeamID *uint `json:"team_id"` - }{LabelID: ptr.Uint(1)}, + IDs: []uint{hosts[0].ID, hosts[1].ID}, + Filters: &deleteHostsFilters{LabelID: ptr.Uint(1)}, } resp := deleteHostsResponse{} s.DoJSON("POST", "/api/latest/fleet/hosts/delete", req, http.StatusBadRequest, &resp) + + req = deleteHostsRequest{} + // No ids or filter specified + s.DoJSON("POST", "/api/latest/fleet/hosts/delete", req, http.StatusBadRequest, &resp) } func (s *integrationTestSuite) TestHostsCount() {