diff --git a/changes/46869-idp-team-vitals-labels b/changes/46869-idp-team-vitals-labels new file mode 100644 index 0000000000..047754d6f2 --- /dev/null +++ b/changes/46869-idp-team-vitals-labels @@ -0,0 +1 @@ +- Fixed a bug where host vitals labels (e.g. IdP group/department labels) scoped to a fleet/team never got any hosts. The membership cron only looked at global labels, and team-scoped IdP labels also failed to populate due to an incorrect SQL join. diff --git a/cmd/fleet/cron.go b/cmd/fleet/cron.go index 5696c3b45a..b00891a0a7 100644 --- a/cmd/fleet/cron.go +++ b/cmd/fleet/cron.go @@ -2068,7 +2068,12 @@ func cronHostVitalsLabelMembership( ) error { // Get all labels. We don't have a function for labels by membership type // so we'll filter them later. - labels, err := ds.ListLabels(ctx, fleet.TeamFilter{}, fleet.ListOptions{ + // + // We use a global admin filter so that fleet/team-scoped labels are included. + // An empty TeamFilter (nil User) falls back to the "global-only" filter + // (l.team_id IS NULL), which would silently exclude every fleet-scoped host + // vitals label and leave them unpopulated. See #46869. + labels, err := ds.ListLabels(ctx, fleet.TeamFilter{User: &fleet.User{GlobalRole: new(fleet.RoleAdmin)}}, fleet.ListOptions{ PerPage: 0, // No limit. }, false) if err != nil { diff --git a/cmd/fleet/cron_test.go b/cmd/fleet/cron_test.go index a7a3cd2504..b43dccb065 100644 --- a/cmd/fleet/cron_test.go +++ b/cmd/fleet/cron_test.go @@ -11,6 +11,7 @@ import ( "testing" "time" + "github.com/jmoiron/sqlx" "github.com/stretchr/testify/require" "github.com/fleetdm/fleet/v4/server/datastore/mysql/mysqltest" @@ -345,3 +346,94 @@ func TestBuildChartScopeResolver(t *testing.T) { require.Nil(t, disabled) }) } + +func TestHostVitalsLabelMembershipCronIDP(t *testing.T) { + ctx := context.Background() + ds := mysqltest.CreateMySQLDS(t) + + team1, err := ds.NewTeam(ctx, &fleet.Team{Name: "idp-cron-team1"}) + require.NoError(t, err) + team2, err := ds.NewTeam(ctx, &fleet.Team{Name: "idp-cron-team2"}) + require.NoError(t, err) + + // host0 -> team1, host1 -> team2, host2 -> global (no team). + hosts := make([]*fleet.Host, 3) + teamIDs := []*uint{&team1.ID, &team2.ID, nil} + for i := range 3 { + h, err := ds.NewHost(ctx, &fleet.Host{ + OsqueryHostID: new(fmt.Sprintf("idp-cron-%d", i)), + NodeKey: new(fmt.Sprintf("idp-cron-%d", i)), + UUID: fmt.Sprintf("idp-cron-uuid%d", i), + Hostname: fmt.Sprintf("idp-cron-host%d.local", i), + HardwareSerial: fmt.Sprintf("idp-cron-hwd%d", i), + Platform: "darwin", + TeamID: teamIDs[i], + }) + require.NoError(t, err) + hosts[i] = h + } + + // All three SCIM users are in the same "Engineering" IdP group. + scimUserIDs := make([]uint, 3) + for i := range 3 { + id, err := ds.CreateScimUser(ctx, &fleet.ScimUser{ + UserName: fmt.Sprintf("idp-cron-user%d", i), + Active: new(true), + }) + require.NoError(t, err) + scimUserIDs[i] = id + hostID, scimUserID := hosts[i].ID, id + mysqltest.ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + _, err := q.ExecContext(ctx, + "INSERT INTO host_scim_user (host_id, scim_user_id) VALUES (?, ?)", + hostID, scimUserID) + return err + }) + } + _, err = ds.CreateScimGroup(ctx, &fleet.ScimGroup{DisplayName: "Engineering", ScimUsers: scimUserIDs}) + require.NoError(t, err) + + criteria, err := json.Marshal(&fleet.HostVitalCriteria{ + Vital: new("end_user_idp_group"), + Value: new("Engineering"), + }) + require.NoError(t, err) + + // Create a global and a team1-scoped IdP host vitals label. + globalLabel, err := ds.NewLabel(ctx, &fleet.Label{ + Name: "idp-cron-global", + LabelType: fleet.LabelTypeRegular, + LabelMembershipType: fleet.LabelMembershipTypeHostVitals, + HostVitalsCriteria: new(json.RawMessage(criteria)), + }) + require.NoError(t, err) + team1Label, err := ds.NewLabel(ctx, &fleet.Label{ + Name: "idp-cron-team1", + TeamID: &team1.ID, + LabelType: fleet.LabelTypeRegular, + LabelMembershipType: fleet.LabelMembershipTypeHostVitals, + HostVitalsCriteria: new(json.RawMessage(criteria)), + }) + require.NoError(t, err) + + // Run the actual cron. + require.NoError(t, cronHostVitalsLabelMembership(ctx, ds)) + + filter := fleet.TeamFilter{User: test.UserAdmin} + + globalHosts, err := ds.ListHostsInLabel(ctx, filter, globalLabel.ID, fleet.HostListOptions{}) + require.NoError(t, err) + gotGlobal := make([]uint, 0, len(globalHosts)) + for _, h := range globalHosts { + gotGlobal = append(gotGlobal, h.ID) + } + require.ElementsMatch(t, []uint{hosts[0].ID, hosts[1].ID, hosts[2].ID}, gotGlobal) + + team1Hosts, err := ds.ListHostsInLabel(ctx, filter, team1Label.ID, fleet.HostListOptions{}) + require.NoError(t, err) + gotTeam1 := make([]uint, 0, len(team1Hosts)) + for _, h := range team1Hosts { + gotTeam1 = append(gotTeam1, h.ID) + } + require.ElementsMatch(t, []uint{hosts[0].ID}, gotTeam1) +} diff --git a/cmd/fleet/serve_test.go b/cmd/fleet/serve_test.go index c9475cd83b..bcc62de6c3 100644 --- a/cmd/fleet/serve_test.go +++ b/cmd/fleet/serve_test.go @@ -1315,6 +1315,12 @@ func TestHostVitalsLabelMembershipJob(t *testing.T) { } ds.ListLabelsFunc = func(ctx context.Context, filter fleet.TeamFilter, opt fleet.ListOptions, includeHostCounts bool) ([]*fleet.Label, error) { + // The cron must pass a filter that includes fleet/team-scoped labels, + // otherwise host vitals labels on a team never get populated (#46869). An + // empty TeamFilter (nil User) falls back to global-only labels in + // applyLabelTeamFilter, so require a global-admin user here. + require.NotNil(t, filter.User, "cron must pass a user-scoped filter so team labels are included") + require.True(t, filter.User.HasAnyGlobalRole(), "cron must use a global-admin filter to see all team labels") return labels, nil } diff --git a/server/datastore/mysql/labels_test.go b/server/datastore/mysql/labels_test.go index d493d21cbc..133a78d8d7 100644 --- a/server/datastore/mysql/labels_test.go +++ b/server/datastore/mysql/labels_test.go @@ -101,6 +101,7 @@ func TestLabels(t *testing.T) { {"ApplyLabelSpecSerialUUID", testApplyLabelSpecsForSerialUUID}, {"ApplyLabelSpecsWithPlatformChange", testApplyLabelSpecsWithPlatformChange}, {"UpdateLabelMembershipByHostCriteria", testUpdateLabelMembershipByHostCriteria}, + {"UpdateLabelMembershipByHostCriteriaIDP", testUpdateLabelMembershipByHostCriteriaIDP}, {"TeamLabels", testTeamLabels}, {"UpdateLabelMembershipForTransferredHost", testUpdateLabelMembershipForTransferredHost}, {"SetAsideLabels", testSetAsideLabels}, @@ -2917,6 +2918,108 @@ func testUpdateLabelMembershipByHostCriteria(t *testing.T, ds *Datastore) { } } +// testUpdateLabelMembershipByHostCriteriaIDP exercises the real IdP foreign +// vital query (end_user_idp_group) for both global and fleet/team-scoped host +// vitals labels. This is the path that broke in #46869: fleet-scoped IdP labels +// never got any hosts. +func testUpdateLabelMembershipByHostCriteriaIDP(t *testing.T, ds *Datastore) { + ctx := t.Context() + + team1, err := ds.NewTeam(ctx, &fleet.Team{Name: "idp-team1"}) + require.NoError(t, err) + team2, err := ds.NewTeam(ctx, &fleet.Team{Name: "idp-team2"}) + require.NoError(t, err) + + // host1 -> team1, host2 -> team2, host3 -> no team (global). + hosts := make([]*fleet.Host, 3) + teamIDs := []*uint{&team1.ID, &team2.ID, nil} + for i := range 3 { + host, err := ds.NewHost(ctx, &fleet.Host{ + OsqueryHostID: new(fmt.Sprintf("idp-%d", i)), + NodeKey: new(fmt.Sprintf("idp-%d", i)), + UUID: fmt.Sprintf("idp-uuid%d", i), + Hostname: fmt.Sprintf("idp-host%d.local", i), + HardwareSerial: fmt.Sprintf("idp-hwd%d", i), + Platform: "darwin", + TeamID: teamIDs[i], + }) + require.NoError(t, err) + hosts[i] = host + } + + // Create a SCIM user per host, all in the "Engineering" IdP group. + scimUserIDs := make([]uint, 3) + for i := range 3 { + id, err := ds.CreateScimUser(ctx, &fleet.ScimUser{ + UserName: fmt.Sprintf("idp-user%d", i), + Active: new(true), + }) + require.NoError(t, err) + scimUserIDs[i] = id + ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + _, err := q.ExecContext(ctx, + "INSERT INTO host_scim_user (host_id, scim_user_id) VALUES (?, ?)", + hosts[i].ID, id) + return err + }) + } + _, err = ds.CreateScimGroup(ctx, &fleet.ScimGroup{ + DisplayName: "Engineering", + ScimUsers: scimUserIDs, + }) + require.NoError(t, err) + + criteria, err := json.Marshal(&fleet.HostVitalCriteria{ + Vital: new("end_user_idp_group"), + Value: new("Engineering"), + }) + require.NoError(t, err) + + newIDPLabel := func(name string, teamID *uint) *fleet.Label { + lbl, err := ds.NewLabel(ctx, &fleet.Label{ + Name: name, + TeamID: teamID, + LabelType: fleet.LabelTypeRegular, + LabelMembershipType: fleet.LabelMembershipTypeHostVitals, + HostVitalsCriteria: new(json.RawMessage(criteria)), + }) + require.NoError(t, err) + return lbl + } + + globalLabel := newIDPLabel("idp-global", nil) + team1Label := newIDPLabel("idp-team1-label", &team1.ID) + + filter := fleet.TeamFilter{User: test.UserAdmin} + + // Global label: all three hosts (all SCIM users are in "Engineering"). + updated, err := ds.UpdateLabelMembershipByHostCriteria(ctx, globalLabel) + require.NoError(t, err) + require.Equal(t, 3, updated.HostCount) + globalHosts, err := ds.ListHostsInLabel(ctx, filter, globalLabel.ID, fleet.HostListOptions{}) + require.NoError(t, err) + require.ElementsMatch(t, []uint{hosts[0].ID, hosts[1].ID, hosts[2].ID}, hostIDs(globalHosts)) + + // Team1 label: only host1 (in team1) despite host2/host3 also being in the + // "Engineering" IdP group. Before the fix this returned an error / zero hosts. + updated, err = ds.UpdateLabelMembershipByHostCriteria(ctx, team1Label) + require.NoError(t, err) + require.Equal(t, 1, updated.HostCount) + team1Hosts, err := ds.ListHostsInLabel(ctx, filter, team1Label.ID, fleet.HostListOptions{}) + require.NoError(t, err) + require.ElementsMatch(t, []uint{hosts[0].ID}, hostIDs(team1Hosts)) + + _ = team2 // team2 is used only to give host2 an out-of-team membership. +} + +func hostIDs(hosts []*fleet.Host) []uint { + ids := make([]uint, 0, len(hosts)) + for _, h := range hosts { + ids = append(ids, h.ID) + } + return ids +} + func testTeamLabels(t *testing.T, ds *Datastore) { ctx := t.Context() diff --git a/server/fleet/hosts.go b/server/fleet/hosts.go index a672df229d..ed433b0ce6 100644 --- a/server/fleet/hosts.go +++ b/server/fleet/hosts.go @@ -497,8 +497,16 @@ type HostVital struct { var hostForeignVitalGroups = map[string]HostForeignVitalGroup{ "idp": { - Name: "Identity Provider", - Query: `RIGHT JOIN host_scim_user ON (hosts.id = host_scim_user.host_id) JOIN scim_users ON (host_scim_user.scim_user_id = scim_users.id) LEFT JOIN scim_user_group ON (host_scim_user.scim_user_id = scim_user_group.scim_user_id) LEFT JOIN scim_groups ON (scim_user_group.group_id = scim_groups.id)`, + Name: "Identity Provider", + // NOTE: This must be an INNER JOIN (not RIGHT JOIN) on host_scim_user. A + // RIGHT JOIN keeps all host_scim_user rows even when the host side has been + // filtered out -- e.g. for fleet/team-scoped labels, where the hosts table + // is pre-filtered to the label's team. An out-of-team scim user that + // matches the criteria would then survive the join with hosts.id = NULL, + // which both leaks cross-team membership and breaks the INSERT into + // label_membership (NULL host_id, which is NOT NULL), rolling back the whole + // update so the fleet label gets zero hosts. See #46869. + Query: `JOIN host_scim_user ON (hosts.id = host_scim_user.host_id) JOIN scim_users ON (host_scim_user.scim_user_id = scim_users.id) LEFT JOIN scim_user_group ON (host_scim_user.scim_user_id = scim_user_group.scim_user_id) LEFT JOIN scim_groups ON (scim_user_group.group_id = scim_groups.id)`, }, } diff --git a/server/service/integration_core_test.go b/server/service/integration_core_test.go index cc5e3c310d..56f6ca9005 100644 --- a/server/service/integration_core_test.go +++ b/server/service/integration_core_test.go @@ -5700,7 +5700,7 @@ func (s *integrationTestSuite) TestLabels() { queryValuesJson, err := json.Marshal(queryValues) require.NoError(t, err) - assert.Equal(t, "SELECT %s FROM %s RIGHT JOIN host_scim_user ON (hosts.id = host_scim_user.host_id) JOIN scim_users ON (host_scim_user.scim_user_id = scim_users.id) LEFT JOIN scim_user_group ON (host_scim_user.scim_user_id = scim_user_group.scim_user_id) LEFT JOIN scim_groups ON (scim_user_group.group_id = scim_groups.id) WHERE scim_groups.display_name = ? GROUP BY hosts.id", query) + assert.Equal(t, "SELECT %s FROM %s JOIN host_scim_user ON (hosts.id = host_scim_user.host_id) JOIN scim_users ON (host_scim_user.scim_user_id = scim_users.id) LEFT JOIN scim_user_group ON (host_scim_user.scim_user_id = scim_user_group.scim_user_id) LEFT JOIN scim_groups ON (scim_user_group.group_id = scim_groups.id) WHERE scim_groups.display_name = ? GROUP BY hosts.id", query) assert.Equal(t, `["group_good"]`, string(queryValuesJson)) // Update label membership. @@ -5757,7 +5757,7 @@ func (s *integrationTestSuite) TestLabels() { queryValuesJson, err := json.Marshal(queryValues) require.NoError(t, err) - assert.Equal(t, "SELECT %s FROM %s RIGHT JOIN host_scim_user ON (hosts.id = host_scim_user.host_id) JOIN scim_users ON (host_scim_user.scim_user_id = scim_users.id) LEFT JOIN scim_user_group ON (host_scim_user.scim_user_id = scim_user_group.scim_user_id) LEFT JOIN scim_groups ON (scim_user_group.group_id = scim_groups.id) WHERE scim_users.department = ? GROUP BY hosts.id", query) + assert.Equal(t, "SELECT %s FROM %s JOIN host_scim_user ON (hosts.id = host_scim_user.host_id) JOIN scim_users ON (host_scim_user.scim_user_id = scim_users.id) LEFT JOIN scim_user_group ON (host_scim_user.scim_user_id = scim_user_group.scim_user_id) LEFT JOIN scim_groups ON (scim_user_group.group_id = scim_groups.id) WHERE scim_users.department = ? GROUP BY hosts.id", query) assert.Equal(t, `["department_good"]`, string(queryValuesJson)) // Update label membership. diff --git a/server/service/labels_test.go b/server/service/labels_test.go index 899f6ef54a..0df3c70665 100644 --- a/server/service/labels_test.go +++ b/server/service/labels_test.go @@ -987,7 +987,7 @@ func TestNewHostVitalsLabel(t *testing.T) { require.NoError(t, err) queryValuesJson, err := json.Marshal(queryValues) require.NoError(t, err) - assert.Equal(t, "SELECT %s FROM %s RIGHT JOIN host_scim_user ON (hosts.id = host_scim_user.host_id) JOIN scim_users ON (host_scim_user.scim_user_id = scim_users.id) LEFT JOIN scim_user_group ON (host_scim_user.scim_user_id = scim_user_group.scim_user_id) LEFT JOIN scim_groups ON (scim_user_group.group_id = scim_groups.id) WHERE scim_groups.display_name = ? GROUP BY hosts.id", query) + assert.Equal(t, "SELECT %s FROM %s JOIN host_scim_user ON (hosts.id = host_scim_user.host_id) JOIN scim_users ON (host_scim_user.scim_user_id = scim_users.id) LEFT JOIN scim_user_group ON (host_scim_user.scim_user_id = scim_user_group.scim_user_id) LEFT JOIN scim_groups ON (scim_user_group.group_id = scim_groups.id) WHERE scim_groups.display_name = ? GROUP BY hosts.id", query) assert.Equal(t, `["admin"]`, string(queryValuesJson)) }) }