From 5ef5419e2cfca69a028ec19047975a712c8b8269 Mon Sep 17 00:00:00 2001 From: Jonathan Boulle Date: Thu, 4 Sep 2014 01:31:04 -0700 Subject: [PATCH] api: check unit name on set operations --- api/units.go | 30 +++++++++++++++++---- api/units_test.go | 69 ++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 85 insertions(+), 14 deletions(-) diff --git a/api/units.go b/api/units.go index fa49cbb12..1e7f490ad 100644 --- a/api/units.go +++ b/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 diff --git a/api/units_test.go b/api/units_test.go index 26637ed7c..fefc16295 100644 --- a/api/units_test.go +++ b/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) + } + } +}