From 170bc1918370a4788efeeb6afd1e7810fa394aa1 Mon Sep 17 00:00:00 2001 From: Nico <32375741+nulmete@users.noreply.github.com> Date: Tue, 24 Feb 2026 12:15:59 -0300 Subject: [PATCH] Improved validation for host transfers (#40345) ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually --- changes/14489-host-transfer-validations | 1 + server/service/hosts.go | 49 +++- server/service/hosts_test.go | 298 ++++++++++++++++++++++++ 3 files changed, 339 insertions(+), 9 deletions(-) create mode 100644 changes/14489-host-transfer-validations diff --git a/changes/14489-host-transfer-validations b/changes/14489-host-transfer-validations new file mode 100644 index 0000000000..fbaf1f5d8e --- /dev/null +++ b/changes/14489-host-transfer-validations @@ -0,0 +1 @@ +- Improved validation for host transfers. diff --git a/server/service/hosts.go b/server/service/hosts.go index fc7084678f..bbc657ef18 100644 --- a/server/service/hosts.go +++ b/server/service/hosts.go @@ -1169,15 +1169,44 @@ func addHostsToTeamEndpoint(ctx context.Context, request interface{}, svc fleet. return addHostsToTeamResponse{}, err } +// authorizeHostSourceTeams checks that the caller has write access to the +// source teams of the hosts being transferred. +func (svc *Service) authorizeHostSourceTeams(ctx context.Context, hosts []*fleet.Host) error { + seenTeamIDs := make(map[uint]struct{}) + var checkedNoTeam bool + for _, h := range hosts { + if h.TeamID == nil { // "No Team" team / "Unassigned" fleet + if !checkedNoTeam { + checkedNoTeam = true + if err := svc.authz.Authorize(ctx, &fleet.Host{TeamID: nil}, fleet.ActionWrite); err != nil { + return err + } + } + } else if _, ok := seenTeamIDs[*h.TeamID]; !ok { + seenTeamIDs[*h.TeamID] = struct{}{} + if err := svc.authz.Authorize(ctx, &fleet.Host{TeamID: h.TeamID}, fleet.ActionWrite); err != nil { + return err + } + } + } + return nil +} + func (svc *Service) AddHostsToTeam(ctx context.Context, teamID *uint, hostIDs []uint, skipBulkPending bool) 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 - // teams. + // Authorize write access to the destination team. if err := svc.authz.Authorize(ctx, &fleet.Host{TeamID: teamID}, fleet.ActionWrite); err != nil { return err } + // Authorize write access to the source teams of the hosts being transferred. + hosts, err := svc.ds.ListHostsLiteByIDs(ctx, hostIDs) + if err != nil { + return ctxerr.Wrapf(ctx, err, "list hosts by IDs for source team authorization (team_id: %v, host_count: %d)", teamID, len(hostIDs)) + } + if err := svc.authorizeHostSourceTeams(ctx, hosts); err != nil { + return err + } + if err := svc.ds.AddHostsToTeam(ctx, fleet.NewAddHostsToTeamParams(teamID, hostIDs)); err != nil { return err } @@ -1306,10 +1335,7 @@ func addHostsToTeamByFilterEndpoint(ctx context.Context, request interface{}, sv } 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 - // teams. + // Authorize write access to the destination team. if err := svc.authz.Authorize(ctx, &fleet.Host{TeamID: teamID}, fleet.ActionWrite); err != nil { return err } @@ -1323,7 +1349,7 @@ func (svc *Service) AddHostsToTeamByFilter(ctx context.Context, teamID *uint, fi return &fleet.BadRequestError{Message: "filters must be specified"} } - hostIDs, hostNames, _, err := svc.hostIDsAndNamesFromFilters(ctx, *opt, lid) + hostIDs, hostNames, hosts, err := svc.hostIDsAndNamesFromFilters(ctx, *opt, lid) if err != nil { return err } @@ -1331,6 +1357,11 @@ func (svc *Service) AddHostsToTeamByFilter(ctx context.Context, teamID *uint, fi return nil } + // Authorize write access to the source teams of the hosts being transferred. + if err := svc.authorizeHostSourceTeams(ctx, hosts); err != nil { + return err + } + // Apply the team to the selected hosts. if err := svc.ds.AddHostsToTeam(ctx, fleet.NewAddHostsToTeamParams(teamID, hostIDs)); err != nil { return err diff --git a/server/service/hosts_test.go b/server/service/hosts_test.go index 9ad3507098..4671e3c7dc 100644 --- a/server/service/hosts_test.go +++ b/server/service/hosts_test.go @@ -1761,6 +1761,304 @@ func TestAddHostsToTeamByFilterEmptyHosts(t *testing.T) { assert.False(t, ds.AddHostsToTeamFuncInvoked) } +func TestAddHostsToTeamSourceTeamAuth(t *testing.T) { + ds := new(mock.Store) + svc, ctx := newTestService(t, ds, nil, nil) + + ds.AppConfigFunc = func(ctx context.Context) (*fleet.AppConfig, error) { + return &fleet.AppConfig{}, nil + } + ds.AddHostsToTeamFunc = func(ctx context.Context, params *fleet.AddHostsToTeamParams) error { + return nil + } + ds.BulkSetPendingMDMHostProfilesFunc = func(ctx context.Context, hids, tids []uint, puuids, uuids []string, + ) (updates fleet.MDMProfilesUpdates, err error) { + return fleet.MDMProfilesUpdates{}, nil + } + ds.ListMDMAppleDEPSerialsInHostIDsFunc = func(ctx context.Context, hids []uint) ([]string, error) { + return nil, nil + } + ds.NewActivityFunc = func(ctx context.Context, user *fleet.User, activity fleet.ActivityDetails, details []byte, createdAt time.Time) error { + return nil + } + ds.TeamLiteFunc = func(ctx context.Context, id uint) (*fleet.TeamLite, error) { + return &fleet.TeamLite{ID: id}, nil + } + ds.ListMDMAndroidUUIDsToHostIDsFunc = func(ctx context.Context, hostIDs []uint) (map[string]uint, error) { + return map[string]uint{}, nil + } + + t.Run("team maintainer cannot steal host from another team", func(t *testing.T) { + // Host 10 belongs to team 2, team 1 maintainer tries to transfer it to team 1 + ds.ListHostsLiteByIDsFunc = func(ctx context.Context, ids []uint) ([]*fleet.Host, error) { + return []*fleet.Host{ + {ID: 10, TeamID: ptr.Uint(2)}, + }, nil + } + userCtx := test.UserContext(ctx, test.UserTeamMaintainerTeam1) + err := svc.AddHostsToTeam(userCtx, ptr.Uint(1), []uint{10}, false) + require.Error(t, err) + require.Contains(t, err.Error(), "forbidden") + }) + + t.Run("team admin cannot steal host from another team", func(t *testing.T) { + // Host 10 belongs to team 2, team 1 admin tries to transfer it to team 1 + ds.ListHostsLiteByIDsFunc = func(ctx context.Context, ids []uint) ([]*fleet.Host, error) { + return []*fleet.Host{ + {ID: 10, TeamID: ptr.Uint(2)}, + }, nil + } + userCtx := test.UserContext(ctx, test.UserTeamAdminTeam1) + err := svc.AddHostsToTeam(userCtx, ptr.Uint(1), []uint{10}, false) + require.Error(t, err) + require.Contains(t, err.Error(), "forbidden") + }) + + t.Run("team maintainer cannot steal host from no-team", func(t *testing.T) { + // Host 10 has no team (global), team 1 maintainer tries to transfer it to team 1 + ds.ListHostsLiteByIDsFunc = func(ctx context.Context, ids []uint) ([]*fleet.Host, error) { + return []*fleet.Host{ + {ID: 10, TeamID: nil}, + }, nil + } + userCtx := test.UserContext(ctx, test.UserTeamMaintainerTeam1) + err := svc.AddHostsToTeam(userCtx, ptr.Uint(1), []uint{10}, false) + require.Error(t, err) + require.Contains(t, err.Error(), "forbidden") + }) + + t.Run("global admin can transfer host across teams", func(t *testing.T) { + ds.ListHostsLiteByIDsFunc = func(ctx context.Context, ids []uint) ([]*fleet.Host, error) { + return []*fleet.Host{ + {ID: 10, TeamID: ptr.Uint(2)}, + }, nil + } + userCtx := test.UserContext(ctx, test.UserAdmin) + err := svc.AddHostsToTeam(userCtx, ptr.Uint(1), []uint{10}, false) + require.NoError(t, err) + }) + + t.Run("global maintainer can transfer host across teams", func(t *testing.T) { + ds.ListHostsLiteByIDsFunc = func(ctx context.Context, ids []uint) ([]*fleet.Host, error) { + return []*fleet.Host{ + {ID: 10, TeamID: ptr.Uint(2)}, + }, nil + } + userCtx := test.UserContext(ctx, test.UserMaintainer) + err := svc.AddHostsToTeam(userCtx, ptr.Uint(1), []uint{10}, false) + require.NoError(t, err) + }) + + t.Run("team maintainer can transfer host within own team", func(t *testing.T) { + // Host 10 already in team 1, team 1 maintainer moves it to team 1 (no-op effectively) + ds.ListHostsLiteByIDsFunc = func(ctx context.Context, ids []uint) ([]*fleet.Host, error) { + return []*fleet.Host{ + {ID: 10, TeamID: ptr.Uint(1)}, + }, nil + } + userCtx := test.UserContext(ctx, test.UserTeamMaintainerTeam1) + err := svc.AddHostsToTeam(userCtx, ptr.Uint(1), []uint{10}, false) + require.NoError(t, err) + }) + + t.Run("mixed hosts - blocked if any source team is unauthorized", func(t *testing.T) { + // Host 10 in team 1 (ok), host 11 in team 2 (not ok) - team 1 maintainer + ds.ListHostsLiteByIDsFunc = func(ctx context.Context, ids []uint) ([]*fleet.Host, error) { + return []*fleet.Host{ + {ID: 10, TeamID: ptr.Uint(1)}, + {ID: 11, TeamID: ptr.Uint(2)}, + }, nil + } + userCtx := test.UserContext(ctx, test.UserTeamMaintainerTeam1) + err := svc.AddHostsToTeam(userCtx, ptr.Uint(1), []uint{10, 11}, false) + require.Error(t, err) + require.Contains(t, err.Error(), "forbidden") + }) + + t.Run("multi-team admin+maintainer can transfer hosts between their teams", func(t *testing.T) { + multiTeamUser := &fleet.User{ + ID: 99, + Teams: []fleet.UserTeam{ + {Team: fleet.Team{ID: 1}, Role: fleet.RoleAdmin}, + {Team: fleet.Team{ID: 2}, Role: fleet.RoleMaintainer}, + }, + } + // Transfer host from team 2 to team 1 + ds.ListHostsLiteByIDsFunc = func(ctx context.Context, ids []uint) ([]*fleet.Host, error) { + return []*fleet.Host{ + {ID: 10, TeamID: ptr.Uint(2)}, + }, nil + } + userCtx := test.UserContext(ctx, multiTeamUser) + err := svc.AddHostsToTeam(userCtx, ptr.Uint(1), []uint{10}, false) + require.NoError(t, err) + + // Transfer host from team 1 to team 2 + ds.ListHostsLiteByIDsFunc = func(ctx context.Context, ids []uint) ([]*fleet.Host, error) { + return []*fleet.Host{ + {ID: 10, TeamID: ptr.Uint(1)}, + }, nil + } + err = svc.AddHostsToTeam(userCtx, ptr.Uint(2), []uint{10}, false) + require.NoError(t, err) + }) + + t.Run("multi-team admin+observer cannot transfer hosts to or from observed team", func(t *testing.T) { + multiTeamUser := &fleet.User{ + ID: 100, + Teams: []fleet.UserTeam{ + {Team: fleet.Team{ID: 1}, Role: fleet.RoleAdmin}, + {Team: fleet.Team{ID: 2}, Role: fleet.RoleObserver}, + }, + } + // Transfer host from team 2 (observer) to team 1 (admin) — blocked on source + ds.ListHostsLiteByIDsFunc = func(ctx context.Context, ids []uint) ([]*fleet.Host, error) { + return []*fleet.Host{ + {ID: 10, TeamID: ptr.Uint(2)}, + }, nil + } + userCtx := test.UserContext(ctx, multiTeamUser) + err := svc.AddHostsToTeam(userCtx, ptr.Uint(1), []uint{10}, false) + require.Error(t, err) + require.Contains(t, err.Error(), "forbidden") + + // Transfer host from team 1 (admin) to team 2 (observer) — blocked on destination + ds.ListHostsLiteByIDsFunc = func(ctx context.Context, ids []uint) ([]*fleet.Host, error) { + return []*fleet.Host{ + {ID: 10, TeamID: ptr.Uint(1)}, + }, nil + } + err = svc.AddHostsToTeam(userCtx, ptr.Uint(2), []uint{10}, false) + require.Error(t, err) + require.Contains(t, err.Error(), "forbidden") + }) +} + +func TestAddHostsToTeamByFilterSourceTeamAuth(t *testing.T) { + ds := new(mock.Store) + svc, ctx := newTestService(t, ds, nil, nil) + + ds.AppConfigFunc = func(ctx context.Context) (*fleet.AppConfig, error) { + return &fleet.AppConfig{}, nil + } + ds.AddHostsToTeamFunc = func(ctx context.Context, params *fleet.AddHostsToTeamParams) error { + return nil + } + ds.BulkSetPendingMDMHostProfilesFunc = func(ctx context.Context, hids, tids []uint, puuids, uuids []string, + ) (updates fleet.MDMProfilesUpdates, err error) { + return fleet.MDMProfilesUpdates{}, nil + } + ds.ListMDMAppleDEPSerialsInHostIDsFunc = func(ctx context.Context, hids []uint) ([]string, error) { + return nil, nil + } + ds.NewActivityFunc = func(ctx context.Context, user *fleet.User, activity fleet.ActivityDetails, details []byte, createdAt time.Time) error { + return nil + } + ds.TeamLiteFunc = func(ctx context.Context, id uint) (*fleet.TeamLite, error) { + return &fleet.TeamLite{ID: id}, nil + } + + t.Run("team maintainer cannot steal hosts from another team via filter", func(t *testing.T) { + ds.ListHostsFunc = func(ctx context.Context, filter fleet.TeamFilter, opt fleet.HostListOptions) ([]*fleet.Host, error) { + return []*fleet.Host{ + {ID: 10, TeamID: ptr.Uint(2)}, + }, nil + } + + userCtx := test.UserContext(ctx, test.UserTeamMaintainerTeam1) + emptyFilter := &map[string]any{} + err := svc.AddHostsToTeamByFilter(userCtx, ptr.Uint(1), emptyFilter) + require.Error(t, err) + require.Contains(t, err.Error(), "forbidden") + assert.False(t, ds.AddHostsToTeamFuncInvoked) + }) + + t.Run("global admin can transfer hosts across teams via filter", func(t *testing.T) { + ds.AddHostsToTeamFuncInvoked = false + ds.ListHostsFunc = func(ctx context.Context, filter fleet.TeamFilter, opt fleet.HostListOptions) ([]*fleet.Host, error) { + return []*fleet.Host{ + {ID: 10, TeamID: ptr.Uint(2)}, + }, nil + } + + userCtx := test.UserContext(ctx, test.UserAdmin) + emptyFilter := &map[string]any{} + err := svc.AddHostsToTeamByFilter(userCtx, ptr.Uint(1), emptyFilter) + require.NoError(t, err) + assert.True(t, ds.AddHostsToTeamFuncInvoked) + }) + + t.Run("multi-team admin+maintainer can transfer hosts between their teams via filter", func(t *testing.T) { + multiTeamUser := &fleet.User{ + ID: 99, + Teams: []fleet.UserTeam{ + {Team: fleet.Team{ID: 1}, Role: fleet.RoleAdmin}, + {Team: fleet.Team{ID: 2}, Role: fleet.RoleMaintainer}, + }, + } + // Transfer host from team 2 to team 1 + ds.AddHostsToTeamFuncInvoked = false + ds.ListHostsFunc = func(ctx context.Context, filter fleet.TeamFilter, opt fleet.HostListOptions) ([]*fleet.Host, error) { + return []*fleet.Host{ + {ID: 10, TeamID: ptr.Uint(2)}, + }, nil + } + userCtx := test.UserContext(ctx, multiTeamUser) + emptyFilter := &map[string]any{} + err := svc.AddHostsToTeamByFilter(userCtx, ptr.Uint(1), emptyFilter) + require.NoError(t, err) + assert.True(t, ds.AddHostsToTeamFuncInvoked) + + // Transfer host from team 1 to team 2 + ds.AddHostsToTeamFuncInvoked = false + ds.ListHostsFunc = func(ctx context.Context, filter fleet.TeamFilter, opt fleet.HostListOptions) ([]*fleet.Host, error) { + return []*fleet.Host{ + {ID: 10, TeamID: ptr.Uint(1)}, + }, nil + } + emptyFilter = &map[string]any{} + err = svc.AddHostsToTeamByFilter(userCtx, ptr.Uint(2), emptyFilter) + require.NoError(t, err) + assert.True(t, ds.AddHostsToTeamFuncInvoked) + }) + + t.Run("multi-team admin+observer cannot transfer hosts to or from observed team via filter", func(t *testing.T) { + multiTeamUser := &fleet.User{ + ID: 100, + Teams: []fleet.UserTeam{ + {Team: fleet.Team{ID: 1}, Role: fleet.RoleAdmin}, + {Team: fleet.Team{ID: 2}, Role: fleet.RoleObserver}, + }, + } + // Transfer host from team 2 (observer) to team 1 (admin) — blocked on source + ds.AddHostsToTeamFuncInvoked = false + ds.ListHostsFunc = func(ctx context.Context, filter fleet.TeamFilter, opt fleet.HostListOptions) ([]*fleet.Host, error) { + return []*fleet.Host{ + {ID: 10, TeamID: ptr.Uint(2)}, + }, nil + } + userCtx := test.UserContext(ctx, multiTeamUser) + emptyFilter := &map[string]any{} + err := svc.AddHostsToTeamByFilter(userCtx, ptr.Uint(1), emptyFilter) + require.Error(t, err) + require.Contains(t, err.Error(), "forbidden") + assert.False(t, ds.AddHostsToTeamFuncInvoked) + + // Transfer host from team 1 (admin) to team 2 (observer) — blocked on destination + ds.AddHostsToTeamFuncInvoked = false + ds.ListHostsFunc = func(ctx context.Context, filter fleet.TeamFilter, opt fleet.HostListOptions) ([]*fleet.Host, error) { + return []*fleet.Host{ + {ID: 10, TeamID: ptr.Uint(1)}, + }, nil + } + emptyFilter = &map[string]any{} + err = svc.AddHostsToTeamByFilter(userCtx, ptr.Uint(2), emptyFilter) + require.Error(t, err) + require.Contains(t, err.Error(), "forbidden") + assert.False(t, ds.AddHostsToTeamFuncInvoked) + }) +} + func TestRefetchHost(t *testing.T) { ds := new(mock.Store) svc, ctx := newTestService(t, ds, nil, nil)