From fc3f54d2da875b05ffbd0a1c8ad24b0e0f50f17d Mon Sep 17 00:00:00 2001 From: Tom Coupland Date: Thu, 23 Aug 2018 12:24:56 +0100 Subject: [PATCH] Insist trigger sources are prefixed (#1184) * Insist trigger sources are prefixed All trigger sources must have a '/' prefix to be allowed into the datastore. * Adding condition to novelValue for gen tests NovelValue was failing to detect same Config values correctly. This adds a specific check for Config, like the one for Annotation, to ensure a novel value is indeed generated. --- api/datastore/datastoretest/test.go | 10 +++++----- api/models/app_test.go | 8 +++++++- api/models/trigger.go | 9 +++++++++ api/models/trigger_test.go | 4 ++-- api/server/trigger_test.go | 16 +++++++++------- 5 files changed, 32 insertions(+), 15 deletions(-) diff --git a/api/datastore/datastoretest/test.go b/api/datastore/datastoretest/test.go index 27c17499c..f865bd085 100644 --- a/api/datastore/datastoretest/test.go +++ b/api/datastore/datastoretest/test.go @@ -1286,7 +1286,7 @@ func RunTriggersTest(t *testing.T, dsf DataStoreFunc, rp ResourceProvider) { for i := 0; i < 10; i++ { trigger := rp.ValidTrigger(testApp.ID, testFn.ID) - trigger.Source = fmt.Sprintf("src_%v", i) + trigger.Source = fmt.Sprintf("/src_%v", i) storedTriggers = append(storedTriggers, h.GivenTriggerInDb(trigger)) } @@ -1397,12 +1397,12 @@ func RunTriggersTest(t *testing.T, dsf DataStoreFunc, rp ResourceProvider) { for i := 0; i < 10; i++ { trigger := rp.ValidTrigger(testApp.ID, testFn.ID) - trigger.Source = fmt.Sprintf("src_%v", i) + trigger.Source = fmt.Sprintf("/src_%v", i) storedTriggers = append(storedTriggers, h.GivenTriggerInDb(trigger)) } trigger := rp.ValidTrigger(testApp.ID, testFn2.ID) - trigger.Source = fmt.Sprintf("src_%v", 11) + trigger.Source = fmt.Sprintf("/src_%v", 11) trigger = h.GivenTriggerInDb(trigger) storedTriggers = append(storedTriggers, trigger) @@ -1458,7 +1458,7 @@ func RunTriggersTest(t *testing.T, dsf DataStoreFunc, rp ResourceProvider) { testTrigger := h.GivenTriggerInDb(rp.ValidTrigger(testApp.ID, testFn.ID)) testTrigger.Name = "newName" - testTrigger.Source = "newSource" + testTrigger.Source = "/newSource" time.Sleep(10 * time.Millisecond) gotTrigger, err := ds.UpdateTrigger(ctx, testTrigger) @@ -1557,7 +1557,7 @@ func RunTriggerBySourceTests(t *testing.T, dsf DataStoreFunc, rp ResourceProvide ds := dsf(t) ctx := rp.DefaultCtx() t.Run("get_non_existant_trigger", func(t *testing.T) { - _, err := ds.GetTriggerBySource(ctx, "none", "http", "source") + _, err := ds.GetTriggerBySource(ctx, "none", "http", "/source") if err != models.ErrTriggerNotFound { t.Fatalf("Expecting trigger not found, got %s", err) } diff --git a/api/models/app_test.go b/api/models/app_test.go index e4ea53cf2..182cf4277 100644 --- a/api/models/app_test.go +++ b/api/models/app_test.go @@ -20,7 +20,9 @@ func appReflectType() reflect.Type { } func configGenerator() gopter.Gen { - return gen.MapOf(gen.AlphaString(), gen.AlphaString()) + return gen.MapOf(gen.AlphaString(), gen.AlphaString()).Map(func(m map[string]string) Config { + return Config(m) + }) } func annotationGenerator() gopter.Gen { @@ -75,6 +77,10 @@ func novelValue(t *testing.T, originalInstance reflect.Value, fieldName string, if !newValue.(Annotations).Equals(currentValue.(Annotations)) { break } + } else if fieldName == "Config" { + if !newValue.(Config).Equals(currentValue.(Config)) { + break + } } else { if newValue != currentValue { break diff --git a/api/models/trigger.go b/api/models/trigger.go index c1f64e68a..7f346e23e 100644 --- a/api/models/trigger.go +++ b/api/models/trigger.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "net/http" + "strings" "time" "unicode" @@ -119,6 +120,10 @@ var ( ErrTriggerMissingSource = err{ code: http.StatusBadRequest, error: errors.New("Missing Trigger Source")} + //ErrTriggerMissingSourcePrefix - source does not have a / prefix + ErrTriggerMissingSourcePrefix = err{ + code: http.StatusBadRequest, + error: errors.New("Missing Trigger Source Prefix '/'")} //ErrTriggerNotFound - trigger not found ErrTriggerNotFound = err{ code: http.StatusNotFound, @@ -164,6 +169,10 @@ func (t *Trigger) Validate() error { return ErrTriggerMissingSource } + if !strings.HasPrefix(t.Source, "/") { + return ErrTriggerMissingSourcePrefix + } + err := t.Annotations.Validate() if err != nil { return err diff --git a/api/models/trigger_test.go b/api/models/trigger_test.go index 8fc3cf1ce..9291d09d8 100644 --- a/api/models/trigger_test.go +++ b/api/models/trigger_test.go @@ -44,8 +44,8 @@ func TestTriggerListJsonMarshalling(t *testing.T) { } } -var httpTrigger = &Trigger{Name: "name", AppID: "foo", FnID: "bar", Type: "http", Source: "baz"} -var invalidTrigger = &Trigger{Name: "name", AppID: "foo", FnID: "bar", Type: "error", Source: "baz"} +var httpTrigger = &Trigger{Name: "name", AppID: "foo", FnID: "bar", Type: "http", Source: "/baz"} +var invalidTrigger = &Trigger{Name: "name", AppID: "foo", FnID: "bar", Type: "error", Source: "/baz"} var triggerValidateCases = []struct { val *Trigger diff --git a/api/server/trigger_test.go b/api/server/trigger_test.go index 36e2be92f..43baf24cb 100644 --- a/api/server/trigger_test.go +++ b/api/server/trigger_test.go @@ -51,20 +51,21 @@ func TestTriggerCreate(t *testing.T) { {commonDS, logs.NewMock(), BaseRoute, `{"app_id":"appid", "fn_id":"fnid", "name": "Test" }`, http.StatusBadRequest, models.ErrTriggerTypeUnknown}, {commonDS, logs.NewMock(), BaseRoute, `{ "name": "Test", "app_id": "appid", "fn_id": "fnid", "type":"http"}`, http.StatusBadRequest, models.ErrTriggerMissingSource}, + {commonDS, logs.NewMock(), BaseRoute, `{ "name": "Test", "app_id": "appid", "fn_id": "fnid", "type":"http", "source":"src"}`, http.StatusBadRequest, models.ErrTriggerMissingSourcePrefix}, {commonDS, logs.NewMock(), BaseRoute, `{ "name": "1234567890123456789012345678901", "app_id": "appid", "fn_id": "fnid", "type":"http"}`, http.StatusBadRequest, models.ErrTriggerTooLongName}, {commonDS, logs.NewMock(), BaseRoute, `{ "name": "&&%@!#$#@$","app_id": "appid", "fn_id": "fnid", "type":"http" }`, http.StatusBadRequest, models.ErrTriggerInvalidName}, - {commonDS, logs.NewMock(), BaseRoute, `{ "name": "trigger", "app_id": "appid", "fn_id": "fnid", "type": "http", "source": "src", "annotations" : { "":"val" }}`, http.StatusBadRequest, models.ErrInvalidAnnotationKey}, - {commonDS, logs.NewMock(), BaseRoute, `{ "id": "asdasca", "name": "trigger", "app_id": "appid", "fn_id": "fnid", "type": "http", "source": "src"}`, http.StatusBadRequest, models.ErrTriggerIDProvided}, - {commonDS, logs.NewMock(), BaseRoute, `{ "name": "trigger", "app_id": "appid", "fn_id": "fnid", "type": "unsupported", "source": "src"}`, http.StatusBadRequest, models.ErrTriggerTypeUnknown}, + {commonDS, logs.NewMock(), BaseRoute, `{ "name": "trigger", "app_id": "appid", "fn_id": "fnid", "type": "http", "source": "/src", "annotations" : { "":"val" }}`, http.StatusBadRequest, models.ErrInvalidAnnotationKey}, + {commonDS, logs.NewMock(), BaseRoute, `{ "id": "asdasca", "name": "trigger", "app_id": "appid", "fn_id": "fnid", "type": "http", "source": "/src"}`, http.StatusBadRequest, models.ErrTriggerIDProvided}, + {commonDS, logs.NewMock(), BaseRoute, `{ "name": "trigger", "app_id": "appid", "fn_id": "fnid", "type": "unsupported", "source": "/src"}`, http.StatusBadRequest, models.ErrTriggerTypeUnknown}, - {commonDS, logs.NewMock(), BaseRoute, `{ "name": "trigger", "app_id": "appid2", "fn_id": "fnid", "type": "http", "source": "src"}`, http.StatusBadRequest, models.ErrTriggerFnIDNotSameApp}, + {commonDS, logs.NewMock(), BaseRoute, `{ "name": "trigger", "app_id": "appid2", "fn_id": "fnid", "type": "http", "source": "/src"}`, http.StatusBadRequest, models.ErrTriggerFnIDNotSameApp}, // // success - {commonDS, logs.NewMock(), BaseRoute, `{ "name": "trigger", "app_id": "appid", "fn_id": "fnid", "type": "http", "source": "src"}`, http.StatusOK, nil}, + {commonDS, logs.NewMock(), BaseRoute, `{ "name": "trigger", "app_id": "appid", "fn_id": "fnid", "type": "http", "source": "/src"}`, http.StatusOK, nil}, //repeated name - {commonDS, logs.NewMock(), BaseRoute, `{ "name": "trigger", "app_id": "appid", "fn_id": "fnid", "type": "http", "source": "src"}`, http.StatusConflict, nil}, + {commonDS, logs.NewMock(), BaseRoute, `{ "name": "trigger", "app_id": "appid", "fn_id": "fnid", "type": "http", "source": "/src"}`, http.StatusConflict, nil}, } { rnr, cancel := testRunner(t) @@ -402,7 +403,7 @@ func TestTriggerUpdate(t *testing.T) { AppID: "appid", FnID: "fnid", Type: "http", - Source: "source"} + Source: "/source"} commonDS := datastore.NewMockInit([]*models.App{a}, []*models.Fn{fn}, []*models.Trigger{trig}) @@ -419,6 +420,7 @@ func TestTriggerUpdate(t *testing.T) { {commonDS, logs.NewMock(), BaseRoute + "/notexist", `{"id": "notexist", "name":"changed"}`, "", http.StatusNotFound, nil}, {commonDS, logs.NewMock(), BaseRoute + "/triggerid", `{"id": "nonmatching", "name":"changed}`, "", http.StatusBadRequest, models.ErrTriggerIDMismatch}, {commonDS, logs.NewMock(), BaseRoute + "/triggerid", `{"id": "triggerid", "name":"changed"}`, "changed", http.StatusOK, nil}, + {commonDS, logs.NewMock(), BaseRoute + "/triggerid", `{"id": "triggerid", "source":"noprefix"}`, "changed", http.StatusBadRequest, nil}, {commonDS, logs.NewMock(), BaseRoute + "/triggerid", `{"name":"again"}`, "again", http.StatusOK, nil}, } { rnr, cancel := testRunner(t)