api: check unit name on set operations

api/units.go

@@ -63,10 +63,21 @@ func (ur *unitsResource) set(rw http.ResponseWriter, req *http.Request, item str
 		sendError(rw, http.StatusBadRequest, fmt.Errorf("unable to decode body: %v", err))
 		return
 	}
+	if su.Name == "" {
+		su.Name = item
+	}
+	if item != su.Name {
+		sendError(rw, http.StatusBadRequest, fmt.Errorf("name in URL %q differs from unit name in request body %q", item, su.Name))
+		return
+	}
+	if err := validateName(su.Name); err != nil {
+		sendError(rw, http.StatusBadRequest, err)
+		return
+	}
 
-	eu, err := ur.cAPI.Unit(item)
+	eu, err := ur.cAPI.Unit(su.Name)
 	if err != nil {
-		log.Errorf("Failed fetching Unit(%s) from Registry: %v", item, err)
+		log.Errorf("Failed fetching Unit(%s) from Registry: %v", su.Name, err)
 		sendError(rw, http.StatusInternalServerError, nil)
 		return
 	}
@@ -76,9 +87,9 @@ func (ur *unitsResource) set(rw http.ResponseWriter, req *http.Request, item str
 			err := errors.New("unit does not exist and options field empty")
 			sendError(rw, http.StatusConflict, err)
 		} else if err := ValidateOptions(su.Options); err != nil {
-			sendError(rw, http.StatusConflict, err)
+			sendError(rw, http.StatusBadRequest, err)
 		} else {
-			ur.create(rw, item, &su)
+			ur.create(rw, su.Name, &su)
 		}
 		return
 	}
@@ -89,7 +100,16 @@ func (ur *unitsResource) set(rw http.ResponseWriter, req *http.Request, item str
 		return
 	}
 
-	ur.update(rw, item, su.DesiredState)
+	ur.update(rw, su.Name, su.DesiredState)
+}
+
+// validateName ensures that a given unit name is valid; if not, an error is
+// returned detailing the issue encountered.
+func validateName(name string) error {
+	if len(name) == 0 {
+		return errors.New("unit name cannot be empty")
+	}
+	return nil
 }
 
 // ValidateOptions ensures that a set of UnitOptions is valid; if not, an error

api/units_test.go

@@ -270,7 +270,9 @@ func TestUnitsSetDesiredState(t *testing.T) {
 		// initial state of Registry
 		initJobs   []job.Job
 		initStates map[string]job.JobState
-		// which Job to attempt to delete
+		// item path (name) of the Unit
+		item string
+		// Unit to attempt to set
 		arg schema.Unit
 		// expected HTTP status code
 		code int
@@ -281,6 +283,7 @@ func TestUnitsSetDesiredState(t *testing.T) {
 		{
 			initJobs:    []job.Job{job.Job{Name: "XXX", Unit: newUnit(t, "[Service]\nFoo=Bar")}},
 			initStates:  map[string]job.JobState{"XXX": "inactive"},
+			item:        "XXX",
 			arg:         schema.Unit{Name: "XXX", DesiredState: "launched"},
 			code:        http.StatusNoContent,
 			finalStates: map[string]job.JobState{"XXX": "launched"},
@@ -289,6 +292,7 @@ func TestUnitsSetDesiredState(t *testing.T) {
 		{
 			initJobs:   []job.Job{},
 			initStates: map[string]job.JobState{},
+			item:       "YYY",
 			arg: schema.Unit{
 				Name:         "YYY",
 				DesiredState: "loaded",
@@ -303,6 +307,7 @@ func TestUnitsSetDesiredState(t *testing.T) {
 		{
 			initJobs:   []job.Job{},
 			initStates: map[string]job.JobState{},
+			item:       "YYY",
 			arg: schema.Unit{
 				Name:         "YYY",
 				DesiredState: "loaded",
@@ -311,13 +316,46 @@ func TestUnitsSetDesiredState(t *testing.T) {
 			code:        http.StatusConflict,
 			finalStates: map[string]job.JobState{},
 		},
-		// Modifying a nonexistent Unit should fail
+		// Referencing a Unit where the name is inconsistent with the path should fail
 		{
-			initJobs:    []job.Job{},
-			initStates:  map[string]job.JobState{},
-			arg:         schema.Unit{Name: "YYY", DesiredState: "loaded"},
-			code:        http.StatusConflict,
-			finalStates: map[string]job.JobState{},
+			initJobs: []job.Job{
+				job.Job{Name: "XXX", Unit: newUnit(t, "[Service]\nFoo=Bar")},
+				job.Job{Name: "YYY", Unit: newUnit(t, "[Service]\nFoo=Baz")},
+			},
+			initStates: map[string]job.JobState{
+				"XXX": "inactive",
+				"YYY": "inactive",
+			},
+			item: "XXX",
+			arg: schema.Unit{
+				Name:         "YYY",
+				DesiredState: "loaded",
+			},
+			code: http.StatusBadRequest,
+			finalStates: map[string]job.JobState{
+				"XXX": "inactive",
+				"YYY": "inactive",
+			},
+		},
+		// Referencing a Unit where the name is omitted should substitute the name from the path
+		{
+			initJobs: []job.Job{
+				job.Job{Name: "XXX", Unit: newUnit(t, "[Service]\nFoo=Bar")},
+				job.Job{Name: "YYY", Unit: newUnit(t, "[Service]\nFoo=Baz")},
+			},
+			initStates: map[string]job.JobState{
+				"XXX": "inactive",
+				"YYY": "inactive",
+			},
+			item: "XXX",
+			arg: schema.Unit{
+				DesiredState: "loaded",
+			},
+			code: http.StatusNoContent,
+			finalStates: map[string]job.JobState{
+				"XXX": "loaded",
+				"YYY": "inactive",
+			},
 		},
 	}
 
@@ -331,7 +369,7 @@ func TestUnitsSetDesiredState(t *testing.T) {
 			}
 		}
 
-		req, err := http.NewRequest("PUT", fmt.Sprintf("http://example.com/units/%s", tt.arg.Name), nil)
+		req, err := http.NewRequest("PUT", fmt.Sprintf("http://example.com/units/%s", tt.item), nil)
 		if err != nil {
 			t.Errorf("case %d: failed creating http.Request: %v", i, err)
 			continue
@@ -348,7 +386,7 @@ func TestUnitsSetDesiredState(t *testing.T) {
 		fAPI := &client.RegistryClient{fr}
 		resource := &unitsResource{fAPI, "/units"}
 		rw := httptest.NewRecorder()
-		resource.set(rw, req, tt.arg.Name)
+		resource.set(rw, req, tt.item)
 
 		if tt.code/100 == 2 {
 			if tt.code != rw.Code {
@@ -588,3 +626,16 @@ func TestValidateOptions(t *testing.T) {
 		}
 	}
 }
+
+func TestValidateName(t *testing.T) {
+	testCases := map[string]bool{
+		"asdf": true,
+		"":     false,
+	}
+	for name, want := range testCases {
+		err := validateName(name)
+		if (err == nil) != want {
+			t.Errorf("name %q: bad validation: want %t, got err=%v", name, want, err)
+		}
+	}
+}