From 502fb0c5ccd0f3623656ea237a0a2a1e96a2c85a Mon Sep 17 00:00:00 2001 From: Lucas Manuel Rodriguez Date: Mon, 7 Jul 2025 16:03:53 -0300 Subject: [PATCH] Support host vital labels with department without groups (#30598) Fix for #23899. I found this while preparing the demo (I had a user with department and no groups). Assigning to @getvictor because Scott is OOO. ## Summary by CodeRabbit * **Bug Fixes** * Improved host filtering for Identity Provider (IdP) group labels to include hosts without corresponding group entries. * **Tests** * Enhanced label tests to cover IdP department labels, including scenarios where users have no groups. * Added new subtests to verify correct host inclusion and label counts for department-based labels. --- server/fleet/hosts.go | 2 +- server/service/integration_core_test.go | 130 +++++++++++++++++++++--- server/service/labels_test.go | 2 +- 3 files changed, 117 insertions(+), 17 deletions(-) diff --git a/server/fleet/hosts.go b/server/fleet/hosts.go index ad5b3cbbe1..c1c7cea9c4 100644 --- a/server/fleet/hosts.go +++ b/server/fleet/hosts.go @@ -411,7 +411,7 @@ 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) JOIN scim_user_group ON (host_scim_user.scim_user_id = scim_user_group.scim_user_id) JOIN scim_groups ON (scim_user_group.group_id = scim_groups.id)`, + 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)`, }, } diff --git a/server/service/integration_core_test.go b/server/service/integration_core_test.go index dcf9fcfd20..ba40e73fe6 100644 --- a/server/service/integration_core_test.go +++ b/server/service/integration_core_test.go @@ -4246,9 +4246,9 @@ func (s *integrationTestSuite) TestLabels() { t := s.T() // create some hosts to use for manual labels - hosts := s.createHosts(t, "debian", "linux", "fedora", "darwin", "darwin", "darwin") + hosts := s.createHosts(t, "debian", "linux", "fedora", "darwin", "darwin", "darwin", "darwin") manualHosts := hosts[:3] - lbl2Hosts := hosts[3:] + lbl2Hosts := hosts[3:6] t.Run("Manual and Dynamic Labels", func(t *testing.T) { // list labels, has the built-in ones @@ -4626,15 +4626,22 @@ func (s *integrationTestSuite) TestLabels() { t, s.ds, func(db sqlx.ExtContext) error { _, err := db.ExecContext( context.Background(), - "INSERT INTO scim_users (id, user_name) VALUES (?, ?), (?, ?), (?, ?), (?, ?)", + "INSERT INTO scim_users (id, user_name, department) VALUES (?, ?, ?), (?, ?, ?), (?, ?, ?), (?, ?, ?), (?, ?, ?)", 1, - "no_groups", + "no_groups_no_department", + nil, 2, - "one_group", + "one_group_good_department", + "department_good", 3, - "all_the_groups", + "all_the_groups_good_department", + "department_good", 4, - "wrong_groups", + "wrong_groups_wrong_department", + "department_other", + 5, + "no_groups_with_department", + "department_other_2", ) return err }, @@ -4675,13 +4682,13 @@ func (s *integrationTestSuite) TestLabels() { t, s.ds, func(db sqlx.ExtContext) error { _, err := db.ExecContext( context.Background(), - "INSERT INTO host_scim_user (host_id, scim_user_id) VALUES (?, ?), (?, ?), (?, ?), (?, ?), (?, ?)", - hosts[0].ID, 1, // host 1 shouldn't be returned because its scim user has no groups - hosts[1].ID, 2, // host 2 should be returned because its scim user has the "group_good" group - hosts[2].ID, 3, // host 3 should be returned because it has a scim user with the "group_good" group - hosts[3].ID, 2, // host 4 should be returned because it has a scim user with the "group_good" group - hosts[4].ID, 4, // host 5 shouldn't be returned because its scim user only has the "group_bad" group - // host 6 shouldn't be returned because it has no scim user + "INSERT INTO host_scim_user (host_id, scim_user_id) VALUES (?, ?), (?, ?), (?, ?), (?, ?), (?, ?), (?, ?)", + hosts[0].ID, 1, + hosts[1].ID, 2, + hosts[2].ID, 3, + hosts[3].ID, 2, + hosts[4].ID, 4, + hosts[6].ID, 5, ) return err }, @@ -4716,7 +4723,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) JOIN scim_user_group ON (host_scim_user.scim_user_id = scim_user_group.scim_user_id) 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 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, `["group_good"]`, string(queryValuesJson)) // Update label membership. @@ -4725,6 +4732,14 @@ func (s *integrationTestSuite) TestLabels() { // Verify that the label has the correct hosts. // Check that the label has the correct hosts + // + // host 1 shouldn't be returned because its scim user has no groups + // host 2 should be returned because its scim user has the "group_good" group + // host 3 should be returned because it has a scim user with the "group_good" group + // host 4 should be returned because it has a scim user with the "group_good" group + // host 5 shouldn't be returned because its scim user only has the "group_bad" group + // host 6 shouldn't be returned because it has no scim user + // hostsInLabel, err := s.ds.ListHostsInLabel(context.Background(), filter, label.ID, fleet.HostListOptions{}) require.NoError(t, err) require.Len(t, hostsInLabel, 3) @@ -4735,6 +4750,91 @@ func (s *integrationTestSuite) TestLabels() { require.NoError(t, err) assert.Equal(t, 3, label.HostCount) }) + + t.Run("IdP Department Label", func(t *testing.T) { + // Create a label for an IdP department + criteria := &fleet.HostVitalCriteria{ + Vital: ptr.String("end_user_idp_department"), + Value: ptr.String("department_good"), + } + + labelParams := createLabelRequest{ + fleet.LabelPayload{ + Name: "Test IdP Department Label", + Criteria: criteria, + }, + } + + labelResp := createLabelResponse{} + s.DoJSON("POST", "/api/latest/fleet/labels", labelParams, http.StatusOK, &labelResp) + require.NotNil(t, labelResp.Label) + + filter := fleet.TeamFilter{User: test.UserAdmin} + label, _, err := s.ds.Label(context.Background(), labelResp.Label.Label.ID, filter) + require.NoError(t, err) + + // Verify that the query and values are correct. + // Test parsing the criteria + query, queryValues, err := label.CalculateHostVitalsQuery() + 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_users.department = ? GROUP BY hosts.id", query) + assert.Equal(t, `["department_good"]`, string(queryValuesJson)) + + // Update label membership. + _, err = s.ds.UpdateLabelMembershipByHostCriteria(context.Background(), label) + require.NoError(t, err) + + // Verify that the label has the correct hosts. + hostsInLabel, err := s.ds.ListHostsInLabel(context.Background(), filter, label.ID, fleet.HostListOptions{}) + require.NoError(t, err) + require.Len(t, hostsInLabel, 3) + require.ElementsMatch(t, []uint{hosts[1].ID, hosts[2].ID, hosts[3].ID}, []uint{hostsInLabel[0].ID, hostsInLabel[1].ID, hostsInLabel[2].ID}) + + // Check that the label has the correct host count + label, _, err = s.ds.Label(context.Background(), labelResp.Label.Label.ID, filter) + require.NoError(t, err) + assert.Equal(t, 3, label.HostCount) + + t.Run("No Groups", func(t *testing.T) { + // Create a label for IdP department to test users with department but no groups + criteria := &fleet.HostVitalCriteria{ + Vital: ptr.String("end_user_idp_department"), + Value: ptr.String("department_other_2"), + } + labelParams := createLabelRequest{ + fleet.LabelPayload{ + Name: "Test IdP Department Label 2", + Criteria: criteria, + }, + } + + labelResp := createLabelResponse{} + s.DoJSON("POST", "/api/latest/fleet/labels", labelParams, http.StatusOK, &labelResp) + require.NotNil(t, labelResp.Label) + + label, _, err = s.ds.Label(context.Background(), labelResp.Label.Label.ID, filter) + require.NoError(t, err) + + // Update label membership. + _, err = s.ds.UpdateLabelMembershipByHostCriteria(context.Background(), label) + require.NoError(t, err) + + // Verify that the label has the correct hosts. + hostsInLabel, err := s.ds.ListHostsInLabel(context.Background(), filter, label.ID, fleet.HostListOptions{}) + require.NoError(t, err) + require.Len(t, hostsInLabel, 1) + // host 7 is mapped to user 5 which matches the department but has no groups. + require.ElementsMatch(t, []uint{hosts[6].ID}, []uint{hostsInLabel[0].ID}) + + // Check that the label has the correct host count + label, _, err = s.ds.Label(context.Background(), label.ID, filter) + require.NoError(t, err) + assert.Equal(t, 1, label.HostCount) + }) + }) }) } diff --git a/server/service/labels_test.go b/server/service/labels_test.go index f51c3bb09d..d90a1763fe 100644 --- a/server/service/labels_test.go +++ b/server/service/labels_test.go @@ -543,7 +543,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) JOIN scim_user_group ON (host_scim_user.scim_user_id = scim_user_group.scim_user_id) 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 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, `["admin"]`, string(queryValuesJson)) }) }