diff --git a/pkg/services/ngalert/api/hooks.go b/pkg/services/ngalert/api/hooks.go index e9341f47784..663b886a0d0 100644 --- a/pkg/services/ngalert/api/hooks.go +++ b/pkg/services/ngalert/api/hooks.go @@ -1,8 +1,10 @@ package api import ( + "net/http" "net/url" + "github.com/gorilla/mux" "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/infra/log" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" @@ -11,31 +13,40 @@ import ( type RequestHandlerFunc func(*contextmodel.ReqContext) response.Response type Hooks struct { - logger log.Logger - hooks map[string]RequestHandlerFunc + logger log.Logger + router *mux.Router + routeHooks map[*mux.Route]RequestHandlerFunc } // NewHooks creates an empty set of request handler hooks. Hooks can be used // to replace handlers for specific paths. func NewHooks(logger log.Logger) *Hooks { return &Hooks{ - logger: logger, - hooks: make(map[string]RequestHandlerFunc), + logger: logger, + router: mux.NewRouter(), + routeHooks: make(map[*mux.Route]RequestHandlerFunc), } } // Add creates a new request hook for a path, causing requests to the path to // be handled by the hook function, and not the original handler. -func (h *Hooks) Set(path string, hook RequestHandlerFunc) { +func (h *Hooks) Set(method string, path string, hook RequestHandlerFunc) { h.logger.Info("Setting hook override for the specified route", "path", path) - h.hooks[path] = hook + route := h.router.NewRoute().Path(path).Methods(method) + h.routeHooks[route] = hook } // Get returns a hook if one is defined for the matching URL. // Get also returns a bool indicating whether or not a matching hook exists. -func (h *Hooks) Get(url *url.URL) (RequestHandlerFunc, bool) { - hook, ok := h.hooks[url.Path] - return hook, ok +func (h *Hooks) Get(method string, url *url.URL) (RequestHandlerFunc, bool) { + req := http.Request{Method: method, URL: url} + + match := mux.RouteMatch{} + if ok := h.router.Match(&req, &match); ok { + return h.routeHooks[match.Route], ok + } + + return nil, false } // Wrap returns a new handler which will intercept paths with hooks configured, @@ -43,7 +54,7 @@ func (h *Hooks) Get(url *url.URL) (RequestHandlerFunc, bool) { // then the given handler is invoked. func (h *Hooks) Wrap(next RequestHandlerFunc) RequestHandlerFunc { return func(req *contextmodel.ReqContext) response.Response { - if hook, ok := h.Get(req.Context.Req.URL); ok { + if hook, ok := h.Get(req.Context.Req.Method, req.Context.Req.URL); ok { h.logger.Debug("Hook defined - invoking new handler", "path", req.Context.Req.URL.Path) return hook(req) } diff --git a/pkg/services/ngalert/api/hooks_test.go b/pkg/services/ngalert/api/hooks_test.go index fc70b410276..6e9b04b10e1 100644 --- a/pkg/services/ngalert/api/hooks_test.go +++ b/pkg/services/ngalert/api/hooks_test.go @@ -19,9 +19,9 @@ func TestHooks(t *testing.T) { invoked := false hook := func(*contextmodel.ReqContext) response.Response { invoked = true; return nil } - hooks.Set("/some/path", hook) + hooks.Set("GET", "/some/path", hook) reqURL, _ := url.Parse("http://domain.test/some/path") - handler, ok := hooks.Get(reqURL) + handler, ok := hooks.Get("GET", reqURL) require.True(t, ok, "hooks did not contain a matching hook for path") require.False(t, invoked, "hook was invoked earlier than expected") @@ -33,9 +33,9 @@ func TestHooks(t *testing.T) { hooks := NewHooks(log.NewNopLogger()) hook := func(*contextmodel.ReqContext) response.Response { return nil } - hooks.Set("/some/path", hook) + hooks.Set("GET", "/some/path", hook) reqURL, _ := url.Parse("http://domain.test/does/not/match") - handler, ok := hooks.Get(reqURL) + handler, ok := hooks.Get("GET", reqURL) require.False(t, ok, "hooks returned a hook when we expected it not to") require.Nil(t, handler) @@ -45,9 +45,21 @@ func TestHooks(t *testing.T) { hooks := NewHooks(log.NewNopLogger()) hook := func(*contextmodel.ReqContext) response.Response { return nil } - hooks.Set("/some/path", hook) + hooks.Set("GET", "/some/path", hook) reqURL, _ := url.Parse("http://domain.test/some/path/with/more") - handler, ok := hooks.Get(reqURL) + handler, ok := hooks.Get("GET", reqURL) + + require.False(t, ok, "hooks returned a hook when we expected it not to") + require.Nil(t, handler) + }) + + t.Run("hooks do not match requests with the wrong HTTP method", func(t *testing.T) { + hooks := NewHooks(log.NewNopLogger()) + hook := func(*contextmodel.ReqContext) response.Response { return nil } + + hooks.Set("POST", "/some/path", hook) + reqURL, _ := url.Parse("http://domain.test/some/path") + handler, ok := hooks.Get("GET", reqURL) require.False(t, ok, "hooks returned a hook when we expected it not to") require.Nil(t, handler) @@ -58,9 +70,9 @@ func TestHooks(t *testing.T) { invoked := false hook := func(*contextmodel.ReqContext) response.Response { invoked = true; return nil } - hooks.Set("/some/path", hook) + hooks.Set("GET", "/some/path", hook) reqURL, _ := url.Parse("http://domain.test/some/path?query=param") - handler, ok := hooks.Get(reqURL) + handler, ok := hooks.Get("GET", reqURL) require.True(t, ok, "hooks did not contain a matching hook for path") require.False(t, invoked, "hook was invoked earlier than expected") @@ -68,16 +80,19 @@ func TestHooks(t *testing.T) { require.True(t, invoked, "the hook returned by get() was not invoked as expected") }) - t.Run("hooks do not match routes with path variables", func(t *testing.T) { + t.Run("hooks match routes with path variables", func(t *testing.T) { hooks := NewHooks(log.NewNopLogger()) - hook := func(*contextmodel.ReqContext) response.Response { return nil } + invoked := false + hook := func(*contextmodel.ReqContext) response.Response { invoked = true; return nil } - hooks.Set("/some/{value}", hook) + hooks.Set("GET", "/some/{value}", hook) reqURL, _ := url.Parse("http://domain.test/some/123") - handler, ok := hooks.Get(reqURL) + handler, ok := hooks.Get("GET", reqURL) - require.False(t, ok, "hooks returned a hook when we expected it not to") - require.Nil(t, handler) + require.True(t, ok, "hooks did not contain a matching hook for path") + require.False(t, invoked, "hook was invoked earlier than expected") + handler(nil) + require.True(t, invoked, "the hook returned by get() was not invoked as expected") }) }) @@ -88,10 +103,10 @@ func TestHooks(t *testing.T) { hookInvoked := false hookHandler := func(*contextmodel.ReqContext) response.Response { hookInvoked = true; return nil } hooks := NewHooks(log.NewNopLogger()) - hooks.Set("/some/path", hookHandler) + hooks.Set("GET", "/some/path", hookHandler) composed := hooks.Wrap(defaultHandler) - req := createReqWithURL("http://domain.test/some/path") + req := createReqForTests("GET", "http://domain.test/some/path") composed(req) require.True(t, hookInvoked, "hook was expected to be invoked, but it was not") @@ -104,10 +119,10 @@ func TestHooks(t *testing.T) { hookInvoked := false hookHandler := func(*contextmodel.ReqContext) response.Response { hookInvoked = true; return nil } hooks := NewHooks(log.NewNopLogger()) - hooks.Set("/some/path", hookHandler) + hooks.Set("GET", "/some/path", hookHandler) composed := hooks.Wrap(defaultHandler) - req := createReqWithURL("http://domain.test/does/not/match") + req := createReqForTests("GET", "http://domain.test/does/not/match") composed(req) require.False(t, hookInvoked, "hook was invoked, but it should not have been") @@ -116,12 +131,13 @@ func TestHooks(t *testing.T) { }) } -func createReqWithURL(setupURL string) *contextmodel.ReqContext { +func createReqForTests(method, setupURL string) *contextmodel.ReqContext { reqURL, _ := url.Parse(setupURL) return &contextmodel.ReqContext{ Context: &web.Context{ Req: &http.Request{ - URL: reqURL, + Method: method, + URL: reqURL, }, }, }