From f21aad446edb52393deb21c0f4ae8acb6d076859 Mon Sep 17 00:00:00 2001
From: Martin Angers <martin.n.angers@gmail.com>
Date: Mon, 22 Apr 2024 16:51:50 -0400
Subject: [PATCH] Fix creation of manual label that allowed overriding existing
 one (#18474)

---
 server/service/integration_core_test.go | 10 ++++++++++
 server/service/labels.go                | 25 ++++++++++---------------
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/server/service/integration_core_test.go b/server/service/integration_core_test.go
index 92ca5b8959..0d5f371e44 100644
--- a/server/service/integration_core_test.go
+++ b/server/service/integration_core_test.go
@@ -3780,6 +3780,11 @@ func (s *integrationTestSuite) TestLabels() {
 	assert.Empty(t, createResp.Label.HostIDs)
 	lbl1 := createResp.Label.Label
 
+	// try to create a manual label with the same name
+	s.DoJSON("POST", "/api/latest/fleet/labels", &fleet.LabelPayload{Name: lbl1.Name, Hosts: []string{manualHosts[0].UUID}}, http.StatusConflict, &createResp)
+	// try to create a dynamic label with the same name
+	s.DoJSON("POST", "/api/latest/fleet/labels", &fleet.LabelPayload{Name: lbl1.Name, Query: "select 2"}, http.StatusConflict, &createResp)
+
 	// get the label
 	var getResp getLabelResponse
 	s.DoJSON("GET", fmt.Sprintf("/api/latest/fleet/labels/%d", lbl1.ID), nil, http.StatusOK, &getResp)
@@ -3814,6 +3819,11 @@ func (s *integrationTestSuite) TestLabels() {
 	assert.Empty(t, createResp.Label.HostIDs)
 	manualLbl2 := createResp.Label.Label
 
+	// try to create a manual label with the same name
+	s.DoJSON("POST", "/api/latest/fleet/labels", &fleet.LabelPayload{Name: manualLbl2.Name, Hosts: []string{manualHosts[0].UUID}}, http.StatusConflict, &createResp)
+	// try to create a dynamic label with the same name
+	s.DoJSON("POST", "/api/latest/fleet/labels", &fleet.LabelPayload{Name: manualLbl2.Name, Query: "select 2"}, http.StatusConflict, &createResp)
+
 	// get the label
 	getResp = getLabelResponse{}
 	s.DoJSON("GET", fmt.Sprintf("/api/latest/fleet/labels/%d", manualLbl2.ID), nil, http.StatusOK, &getResp)
diff --git a/server/service/labels.go b/server/service/labels.go
index 63243a76db..6b5892b79d 100644
--- a/server/service/labels.go
+++ b/server/service/labels.go
@@ -73,9 +73,14 @@ func (svc *Service) NewLabel(ctx context.Context, p fleet.LabelPayload) (*fleet.
 		}
 	}
 
-	// if membership type is manual, must use ApplyLabelSpecs (as NewLabel does
-	// not create label memberships), otherwise NewLabel works for dynamic
-	// membership. Must resolve the host identifiers to hostname so that
+	// first create the new label, which will fail if the name is not unique
+	newLbl, err := svc.ds.NewLabel(ctx, label)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	// Next, if membership type is manual, use ApplyLabelSpecs to create label
+	// memberships. Must resolve the host identifiers to hostname so that
 	// ApplySpecs can be used.
 	var hostIDs []uint
 	if label.LabelMembershipType == fleet.LabelMembershipTypeManual {
@@ -96,21 +101,11 @@ func (svc *Service) NewLabel(ctx context.Context, p fleet.LabelPayload) (*fleet.
 			return nil, nil, err
 		}
 
-		// must reload it to get the id, and the host IDs
-		lblIDsByName, err := svc.ds.LabelIDsByName(ctx, []string{label.Name})
+		// must reload it to get the host IDs
+		label, hostIDs, err = svc.ds.Label(ctx, newLbl.ID)
 		if err != nil {
 			return nil, nil, err
 		}
-		label, hostIDs, err = svc.ds.Label(ctx, lblIDsByName[label.Name])
-		if err != nil {
-			return nil, nil, err
-		}
-	} else {
-		newLbl, err := svc.ds.NewLabel(ctx, label)
-		if err != nil {
-			return nil, nil, err
-		}
-		label = newLbl
 	}
 	return label, hostIDs, nil
 }