Skip to content

Commit

Permalink
Fixing hosts/delete input validation due to QA issue. (#14942)
Browse files Browse the repository at this point in the history
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:
#14952

[x] Tests updated and added.
[x] Manual testing done.
  • Loading branch information
getvictor authored Nov 6, 2023
1 parent 15dbc1b commit 5391b68
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 40 deletions.
2 changes: 1 addition & 1 deletion server/fleet/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
47 changes: 29 additions & 18 deletions server/service/hosts.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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"}
}

Expand All @@ -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
}
Expand Down
4 changes: 2 additions & 2 deletions server/service/hosts_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
47 changes: 28 additions & 19 deletions server/service/integration_core_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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() {
Expand Down

0 comments on commit 5391b68

Please sign in to comment.