From d4ba30cfa180be389df5fa318c5cd966c8df360e Mon Sep 17 00:00:00 2001 From: Tim Lee Date: Mon, 11 Mar 2024 10:02:51 -0600 Subject: [PATCH] 17257 validate bulk ops request filters (#17514) --- changes/17257-host-ops-validation | 1 + server/fleet/hosts.go | 9 ++ server/fleet/hosts_test.go | 20 +++ server/fleet/service.go | 4 +- server/service/client_hosts.go | 22 +-- server/service/hosts.go | 113 +++++++++------ server/service/hosts_test.go | 179 ++++++++++++++++++++++-- server/service/integration_core_test.go | 15 +- 8 files changed, 300 insertions(+), 63 deletions(-) create mode 100644 changes/17257-host-ops-validation diff --git a/changes/17257-host-ops-validation b/changes/17257-host-ops-validation new file mode 100644 index 0000000000..a0e039791c --- /dev/null +++ b/changes/17257-host-ops-validation @@ -0,0 +1 @@ +- added validation to the json request for bulk host operations for transfer and delete \ No newline at end of file diff --git a/server/fleet/hosts.go b/server/fleet/hosts.go index f9815f7e4a..f81cfbb5b4 100644 --- a/server/fleet/hosts.go +++ b/server/fleet/hosts.go @@ -39,6 +39,15 @@ const ( OnlineIntervalBuffer = 60 ) +func (s HostStatus) IsValid() bool { + switch s { + case StatusOnline, StatusOffline, StatusNew, StatusMissing, StatusMIA: + return true + default: + return false + } +} + // MDMEnrollStatus defines the possible MDM enrollment statuses. type MDMEnrollStatus string diff --git a/server/fleet/hosts_test.go b/server/fleet/hosts_test.go index 2bba30a7d7..18c4fc42b6 100644 --- a/server/fleet/hosts_test.go +++ b/server/fleet/hosts_test.go @@ -52,6 +52,26 @@ func TestHostStatus(t *testing.T) { } } +func TestHostStatusIsValid(t *testing.T) { + for _, tt := range []struct { + name string + status HostStatus + expected bool + }{ + {"online", StatusOnline, true}, + {"offline", StatusOffline, true}, + {"new", StatusNew, true}, + {"missing", StatusMissing, true}, + {"mia", StatusMIA, true}, // As of Fleet 4.15, StatusMIA is deprecated in favor of StatusOffline + {"empty", HostStatus(""), false}, + {"invalid", HostStatus("invalid"), false}, + } { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, tt.status.IsValid()) + }) + } +} + func TestHostIsNew(t *testing.T) { mockClock := clock.NewMockClock() diff --git a/server/fleet/service.go b/server/fleet/service.go index 0088594bb9..ec360e3d2d 100644 --- a/server/fleet/service.go +++ b/server/fleet/service.go @@ -350,8 +350,8 @@ type Service interface { AddHostsToTeam(ctx context.Context, teamID *uint, hostIDs []uint, skipBulkPending bool) error // 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 + AddHostsToTeamByFilter(ctx context.Context, teamID *uint, filter *map[string]interface{}) error + DeleteHosts(ctx context.Context, ids []uint, filters *map[string]interface{}) 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/client_hosts.go b/server/service/client_hosts.go index 029916a3cb..cc2e7ab506 100644 --- a/server/service/client_hosts.go +++ b/server/service/client_hosts.go @@ -114,21 +114,27 @@ func (c *Client) TransferHosts(hosts []string, label string, status, searchQuery return c.authenticatedRequest(params, verb, path, &responseBody) } - var labelIDPtr *uint + filter := make(map[string]interface{}) + if label != "" { - labelIDPtr = &labelID + filter["label_id"] = labelID + } + + if status != "" { + filter["status"] = fleet.HostStatus(status) + } + + if searchQuery != "" { + filter["query"] = searchQuery } verb, path := "POST", "/api/latest/fleet/hosts/transfer/filter" var responseBody addHostsToTeamByFilterResponse params := addHostsToTeamByFilterRequest{ - TeamID: teamIDPtr, Filters: struct { - MatchQuery string `json:"query"` - Status fleet.HostStatus `json:"status"` - LabelID *uint `json:"label_id"` - TeamID *uint `json:"team_id"` - }{MatchQuery: searchQuery, Status: fleet.HostStatus(status), LabelID: labelIDPtr}, + TeamID: teamIDPtr, + Filters: &filter, } + return c.authenticatedRequest(params, verb, path, &responseBody) } diff --git a/server/service/hosts.go b/server/service/hosts.go index 7d030d0d00..c01d1047f9 100644 --- a/server/service/hosts.go +++ b/server/service/hosts.go @@ -217,17 +217,10 @@ 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"` // Using a pointer to help determine whether an empty filter was passed, like: "filters":{} - Filters *deleteHostsFilters `json:"filters"` + Filters *map[string]interface{} `json:"filters"` } type deleteHostsResponse struct { @@ -242,18 +235,6 @@ func (r deleteHostsResponse) Status() int { return r.StatusCode } func deleteHostsEndpoint(ctx context.Context, request interface{}, svc fleet.Service) (errorer, error) { req := request.(*deleteHostsRequest) - 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 // and allow the delete operation to proceed. @@ -261,7 +242,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, listOpts, labelID) + err = svc.DeleteHosts(ctx, req.IDs, req.Filters) if err != nil { // logging the error for future debug in case we already sent http.StatusAccepted logging.WithErr(ctx, err) @@ -283,11 +264,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, filter *map[string]interface{}) error { if err := svc.authz.Authorize(ctx, &fleet.Host{}, fleet.ActionList); err != nil { return err } + opts, lid, err := hostListOptionsFromFilters(filter) + if err != nil { + return err + } + if len(ids) == 0 && lid == nil && opts == nil { return &fleet.BadRequestError{Message: "list of ids or filters must be specified"} } @@ -862,13 +848,8 @@ func (svc *Service) createTransferredHostsActivity(ctx context.Context, teamID * //////////////////////////////////////////////////////////////////////////////// type addHostsToTeamByFilterRequest struct { - TeamID *uint `json:"team_id"` - Filters struct { - MatchQuery string `json:"query"` - Status fleet.HostStatus `json:"status"` - LabelID *uint `json:"label_id"` - TeamID *uint `json:"team_id"` - } `json:"filters"` + TeamID *uint `json:"team_id"` + Filters *map[string]interface{} `json:"filters"` } type addHostsToTeamByFilterResponse struct { @@ -879,14 +860,7 @@ func (r addHostsToTeamByFilterResponse) error() error { return r.Err } func addHostsToTeamByFilterEndpoint(ctx context.Context, request interface{}, svc fleet.Service) (errorer, error) { req := request.(*addHostsToTeamByFilterRequest) - listOpt := fleet.HostListOptions{ - ListOptions: fleet.ListOptions{ - MatchQuery: req.Filters.MatchQuery, - }, - StatusFilter: req.Filters.Status, - TeamFilter: req.Filters.TeamID, - } - err := svc.AddHostsToTeamByFilter(ctx, req.TeamID, listOpt, req.Filters.LabelID) + err := svc.AddHostsToTeamByFilter(ctx, req.TeamID, req.Filters) if err != nil { return addHostsToTeamByFilterResponse{Err: err}, nil } @@ -894,7 +868,7 @@ func addHostsToTeamByFilterEndpoint(ctx context.Context, request interface{}, sv return addHostsToTeamByFilterResponse{}, err } -func (svc *Service) AddHostsToTeamByFilter(ctx context.Context, teamID *uint, opt fleet.HostListOptions, lid *uint) error { +func (svc *Service) AddHostsToTeamByFilter(ctx context.Context, teamID *uint, filters *map[string]interface{}) error { // This is currently treated as a "team write". If we ever give users // besides global admins permissions to modify team hosts, we will need to // check that the user has permissions for both the source and destination @@ -903,7 +877,16 @@ func (svc *Service) AddHostsToTeamByFilter(ctx context.Context, teamID *uint, op return err } - hostIDs, hostNames, err := svc.hostIDsAndNamesFromFilters(ctx, opt, lid) + opt, lid, err := hostListOptionsFromFilters(filters) + if err != nil { + return err + } + + if opt == nil { + return &fleet.BadRequestError{Message: "filters must be specified"} + } + + hostIDs, hostNames, err := svc.hostIDsAndNamesFromFilters(ctx, *opt, lid) if err != nil { return err } @@ -2160,3 +2143,55 @@ func (svc *Service) HostLiteByID(ctx context.Context, id uint) (*fleet.HostLite, return host, nil } + +func hostListOptionsFromFilters(filter *map[string]interface{}) (*fleet.HostListOptions, *uint, error) { + var labelID *uint + + if filter == nil { + return nil, nil, nil + } + + opt := fleet.HostListOptions{} + + for k, v := range *filter { + switch k { + case "label_id": + if l, ok := v.(float64); ok { // json unmarshals numbers as float64 + lid := uint(l) + labelID = &lid + } else { + return nil, nil, badRequest("label_id must be a number") + } + case "team_id": + if teamID, ok := v.(float64); ok { // json unmarshals numbers as float64 + teamID := uint(teamID) + opt.TeamFilter = &teamID + } else { + return nil, nil, badRequest("team_id must be a number") + } + case "status": + status, ok := v.(string) + if !ok { + return nil, nil, badRequest("status must be a string") + } + if !fleet.HostStatus(status).IsValid() { + return nil, nil, badRequest("status must be one of: new, online, offline, missing") + } + opt.StatusFilter = fleet.HostStatus(status) + case "query": + query, ok := v.(string) + if !ok { + return nil, nil, badRequest("query must be a string") + } + if query == "" { + return nil, nil, badRequest("query must not be empty") + } + opt.MatchQuery = query + + default: + return nil, nil, badRequest(fmt.Sprintf("unknown filter key: %s", k)) + } + } + + return &opt, labelID, nil +} diff --git a/server/service/hosts_test.go b/server/service/hosts_test.go index 1d3a5ac4c5..621b9c7bfd 100644 --- a/server/service/hosts_test.go +++ b/server/service/hosts_test.go @@ -723,16 +723,17 @@ func TestHostAuth(t *testing.T) { err = svc.DeleteHost(ctx, 2) checkAuthErr(t, tt.shouldFailGlobalWrite, err) - err = svc.DeleteHosts(ctx, []uint{1}, nil, nil) + err = svc.DeleteHosts(ctx, []uint{1}, nil) checkAuthErr(t, tt.shouldFailTeamWrite, err) - err = svc.DeleteHosts(ctx, []uint{2}, &fleet.HostListOptions{}, nil) + err = svc.DeleteHosts(ctx, []uint{2}, nil) checkAuthErr(t, tt.shouldFailGlobalWrite, err) err = svc.AddHostsToTeam(ctx, ptr.Uint(1), []uint{1}, false) checkAuthErr(t, tt.shouldFailTeamWrite, err) - err = svc.AddHostsToTeamByFilter(ctx, ptr.Uint(1), fleet.HostListOptions{}, nil) + emptyFilter := make(map[string]interface{}) + err = svc.AddHostsToTeamByFilter(ctx, ptr.Uint(1), &emptyFilter) checkAuthErr(t, tt.shouldFailTeamWrite, err) err = svc.RefetchHost(ctx, 1) @@ -855,7 +856,9 @@ func TestAddHostsToTeamByFilter(t *testing.T) { return nil } - require.NoError(t, svc.AddHostsToTeamByFilter(test.UserContext(ctx, test.UserAdmin), expectedTeam, fleet.HostListOptions{}, nil)) + emptyRequest := &map[string]interface{}{} + + require.NoError(t, svc.AddHostsToTeamByFilter(test.UserContext(ctx, test.UserAdmin), expectedTeam, emptyRequest)) assert.True(t, ds.ListHostsFuncInvoked) assert.True(t, ds.AddHostsToTeamFuncInvoked) } @@ -866,10 +869,10 @@ func TestAddHostsToTeamByFilterLabel(t *testing.T) { expectedHostIDs := []uint{6} expectedTeam := ptr.Uint(1) - expectedLabel := ptr.Uint(2) + expectedLabel := float64(2) ds.ListHostsInLabelFunc = func(ctx context.Context, filter fleet.TeamFilter, lid uint, opt fleet.HostListOptions) ([]*fleet.Host, error) { - assert.Equal(t, *expectedLabel, lid) + assert.Equal(t, uint(expectedLabel), lid) var hosts []*fleet.Host for _, id := range expectedHostIDs { hosts = append(hosts, &fleet.Host{ID: id}) @@ -893,7 +896,9 @@ func TestAddHostsToTeamByFilterLabel(t *testing.T) { return nil } - require.NoError(t, svc.AddHostsToTeamByFilter(test.UserContext(ctx, test.UserAdmin), expectedTeam, fleet.HostListOptions{}, expectedLabel)) + filter := &map[string]interface{}{"label_id": expectedLabel} + + require.NoError(t, svc.AddHostsToTeamByFilter(test.UserContext(ctx, test.UserAdmin), expectedTeam, filter)) assert.True(t, ds.ListHostsInLabelFuncInvoked) assert.True(t, ds.AddHostsToTeamFuncInvoked) } @@ -912,7 +917,9 @@ func TestAddHostsToTeamByFilterEmptyHosts(t *testing.T) { return nil } - require.NoError(t, svc.AddHostsToTeamByFilter(test.UserContext(ctx, test.UserAdmin), nil, fleet.HostListOptions{}, nil)) + emptyFilter := &map[string]interface{}{} + + require.NoError(t, svc.AddHostsToTeamByFilter(test.UserContext(ctx, test.UserAdmin), nil, emptyFilter)) assert.True(t, ds.ListHostsFuncInvoked) assert.False(t, ds.AddHostsToTeamFuncInvoked) } @@ -1628,3 +1635,159 @@ func TestLockUnlockWipeHostAuth(t *testing.T) { }) } } + +func TestBulkOperationFilterValidation(t *testing.T) { + ds := new(mock.Store) + svc, ctx := newTestService(t, ds, nil, nil) + viewerCtx := test.UserContext(ctx, test.UserAdmin) + + ds.ListHostsFunc = func(ctx context.Context, filter fleet.TeamFilter, opt fleet.HostListOptions) ([]*fleet.Host, error) { + return []*fleet.Host{}, nil + } + + ds.ListHostsInLabelFunc = func(ctx context.Context, filter fleet.TeamFilter, lid uint, opt fleet.HostListOptions) ([]*fleet.Host, error) { + return []*fleet.Host{}, nil + } + + tc := []struct { + name string + filters *map[string]interface{} + has400Err bool + }{ + { + name: "valid status filter", + filters: &map[string]interface{}{ + "status": "new", + }, + }, + { + name: "invalid status", + filters: &map[string]interface{}{ + "status": "invalid", + }, + has400Err: true, + }, + { + name: "empty status is invalid", + filters: &map[string]interface{}{ + "status": "", + }, + has400Err: true, + }, + + { + name: "valid team filter", + filters: &map[string]interface{}{ + "team_id": float64(1), // json unmarshals to float64 + }, + }, + { + name: "invalid team_id type", + filters: &map[string]interface{}{ + "team_id": "invalid", + }, + has400Err: true, + }, + { + name: "valid label_id filter", + filters: &map[string]interface{}{ + "label_id": float64(1), + }, + }, + { + name: "invalid label_id type", + filters: &map[string]interface{}{ + "label_id": "invalid", + }, + has400Err: true, + }, + + { + name: "invalid status type", + filters: &map[string]interface{}{ + "status": float64(1), + }, + has400Err: true, + }, + { + name: "empty filter", + filters: &map[string]interface{}{}, + }, + { + name: "valid query filter", + filters: &map[string]interface{}{ + "query": "test", + }, + }, + { + name: "invalid query type", + filters: &map[string]interface{}{ + "query": float64(1), + }, + has400Err: true, + }, + { + name: "empty query is invalid", + filters: &map[string]interface{}{ + "query": "", + }, + has400Err: true, + }, + { + name: "multiple valid filters", + filters: &map[string]interface{}{ + "status": "new", + "team_id": float64(1), + "query": "test", + }, + }, + { + name: "mixed valid and invalid filters", + filters: &map[string]interface{}{ + "status": "new", + "team_id": "invalid", + }, + has400Err: true, + }, + { + name: "mixed invalid filters and valid filters (different order)", + filters: &map[string]interface{}{ + "status": "invalid", + "team_id": 1, + }, + has400Err: true, + }, + { + name: "mixed valid and unknown filters", + filters: &map[string]interface{}{ + "status": "new", + "unknown": "filter", + }, + has400Err: true, + }, + { + name: "unknown filter", + filters: &map[string]interface{}{ + "unknown": "filter", + }, + has400Err: true, + }, + } + + checkErr := func(t *testing.T, err error, has400Err bool) { + if has400Err { + require.Error(t, err) + var be *fleet.BadRequestError + require.ErrorAs(t, err, &be) + } else { + require.NoError(t, err) + } + } + + for _, tt := range tc { + t.Run(tt.name, func(t *testing.T) { + checkErr(t, svc.AddHostsToTeamByFilter(viewerCtx, nil, tt.filters), tt.has400Err) + checkErr(t, svc.DeleteHosts(viewerCtx, nil, tt.filters), tt.has400Err) + }) + } +} diff --git a/server/service/integration_core_test.go b/server/service/integration_core_test.go index 7a90586386..5cbea73c36 100644 --- a/server/service/integration_core_test.go +++ b/server/service/integration_core_test.go @@ -1000,7 +1000,7 @@ func (s *integrationTestSuite) TestBulkDeleteHostsFromTeam() { require.NoError(t, s.ds.AddHostsToTeam(context.Background(), &team1.ID, []uint{hosts[0].ID})) req := deleteHostsRequest{ - Filters: &deleteHostsFilters{TeamID: ptr.Uint(team1.ID)}, + Filters: &map[string]interface{}{"team_id": float64(team1.ID)}, } resp := deleteHostsResponse{} s.DoJSON("POST", "/api/latest/fleet/hosts/delete", req, http.StatusOK, &resp) @@ -1037,7 +1037,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: &deleteHostsFilters{LabelID: ptr.Uint(label.ID)}, + Filters: &map[string]interface{}{"label_id": float64(label.ID)}, } resp := deleteHostsResponse{} s.DoJSON("POST", "/api/latest/fleet/hosts/delete", req, http.StatusOK, &resp) @@ -1120,7 +1120,7 @@ func (s *integrationTestSuite) TestBulkDeleteHostsAll() { // All hosts should be deleted when an empty filter is specified req := deleteHostsRequest{ - Filters: &deleteHostsFilters{}, + Filters: &map[string]interface{}{}, } resp := deleteHostsResponse{} s.DoJSON("POST", "/api/latest/fleet/hosts/delete", req, http.StatusOK, &resp) @@ -1163,7 +1163,7 @@ func (s *integrationTestSuite) TestBulkDeleteHostsErrors() { req := deleteHostsRequest{ IDs: []uint{hosts[0].ID, hosts[1].ID}, - Filters: &deleteHostsFilters{LabelID: ptr.Uint(1)}, + Filters: &map[string]interface{}{"label_id": float64(1)}, } resp := deleteHostsResponse{} s.DoJSON("POST", "/api/latest/fleet/hosts/delete", req, http.StatusBadRequest, &resp) @@ -2814,8 +2814,11 @@ func (s *integrationTestSuite) TestHostsAddToTeam() { // assign host to team 2 with filter var addfResp addHostsToTeamByFilterResponse - req := addHostsToTeamByFilterRequest{TeamID: &tm2.ID} - req.Filters.MatchQuery = hosts[2].Hostname + req := addHostsToTeamByFilterRequest{ + TeamID: &tm2.ID, + Filters: &map[string]interface{}{"query": hosts[2].Hostname}, + } + s.DoJSON("POST", "/api/latest/fleet/hosts/transfer/filter", req, http.StatusOK, &addfResp) s.lastActivityOfTypeMatches( fleet.ActivityTypeTransferredHostsToTeam{}.ActivityName(),