Allowed method handling file spa (#459)

* move default allowed methods for endpoints to runtime package; pass as defaultAllowedMethods to NewAllowedMethodsHandler() * added test cases current situation for files and spa * changed test case expectations (GET, HEAD, OPTIONS allowed) * use AllowedMethodsHandler to handle allowed methods for for files and spa * added test case CORS preflight request for files/spa * bar regular OPTIONS requests for files/spa (as formerly in file/spa handlers) * no need to return Mux pointer * wrap internal handlers (currently: health) with AllowedMethodsHandler * no need for (always nil) methods param Co-authored-by: Marcel Ludwig <marcel.ludwig@avenga.com>
coupergateway · Apr 4, 2022 · 36cc910 · 36cc910
1 parent 4c6ecad
commit 36cc910
Show file tree

Hide file tree

Showing 8 changed files with 96 additions and 49 deletions.
diff --git a/config/runtime/server.go b/config/runtime/server.go
@@ -88,6 +88,21 @@ func GetHostPort(hostPort string) (string, int, error) {
 	return host, port, nil
 }
 
+var defaultFileSpaAllowedMethods = []string{
+	http.MethodGet,
+	http.MethodHead,
+}
+
+var defaultEndpointAllowedMethods = []string{
+	http.MethodGet,
+	http.MethodHead,
+	http.MethodPost,
+	http.MethodPut,
+	http.MethodPatch,
+	http.MethodDelete,
+	http.MethodOptions,
+}
+
 // NewServerConfiguration sets http handler specific defaults and validates the given gateway configuration.
 // Wire up all endpoints and maps them within the returned Server.
 func NewServerConfiguration(conf *config.Couper, log *logrus.Entry, memStore *cache.MemoryStore) (ServerConfiguration, error) {
@@ -152,11 +167,16 @@ func NewServerConfiguration(conf *config.Couper, log *logrus.Entry, memStore *ca
 				return nil, err
 			}
 
+			epOpts := &handler.EndpointOptions{ErrorTemplate: serverOptions.ServerErrTpl}
+			notAllowedMethodsHandler := epOpts.ErrorTemplate.WithError(errors.MethodNotAllowed)
+			allowedMethodsHandler := middleware.NewAllowedMethodsHandler(nil, defaultFileSpaAllowedMethods, spaHandler, notAllowedMethodsHandler)
+			spaHandler = allowedMethodsHandler
+
 			spaHandler, err = configureProtectedHandler(accessControls, confCtx,
 				config.NewAccessControl(srvConf.AccessControl, srvConf.DisableAccessControl),
 				config.NewAccessControl(srvConf.Spa.AccessControl, srvConf.Spa.DisableAccessControl),
 				&protectedOptions{
-					epOpts:       &handler.EndpointOptions{ErrorTemplate: serverOptions.ServerErrTpl},
+					epOpts:       epOpts,
 					handler:      spaHandler,
 					memStore:     memStore,
 					proxyFromEnv: conf.Settings.NoProxyFromEnv,
@@ -166,7 +186,7 @@ func NewServerConfiguration(conf *config.Couper, log *logrus.Entry, memStore *ca
 				return nil, err
 			}
 
-			corsOptions, cerr := middleware.NewCORSOptions(whichCORS(srvConf, srvConf.Spa), nil)
+			corsOptions, cerr := middleware.NewCORSOptions(whichCORS(srvConf, srvConf.Spa), allowedMethodsHandler.MethodAllowed)
 			if cerr != nil {
 				return nil, cerr
 			}
@@ -196,11 +216,16 @@ func NewServerConfiguration(conf *config.Couper, log *logrus.Entry, memStore *ca
 				return nil, err
 			}
 
+			epOpts := &handler.EndpointOptions{ErrorTemplate: serverOptions.FilesErrTpl}
+			notAllowedMethodsHandler := epOpts.ErrorTemplate.WithError(errors.MethodNotAllowed)
+			allowedMethodsHandler := middleware.NewAllowedMethodsHandler(nil, defaultFileSpaAllowedMethods, fileHandler, notAllowedMethodsHandler)
+			fileHandler = allowedMethodsHandler
+
 			fileHandler, err = configureProtectedHandler(accessControls, confCtx,
 				config.NewAccessControl(srvConf.AccessControl, srvConf.DisableAccessControl),
 				config.NewAccessControl(srvConf.Files.AccessControl, srvConf.Files.DisableAccessControl),
 				&protectedOptions{
-					epOpts:       &handler.EndpointOptions{ErrorTemplate: serverOptions.FilesErrTpl},
+					epOpts:       epOpts,
 					handler:      fileHandler,
 					memStore:     memStore,
 					proxyFromEnv: conf.Settings.NoProxyFromEnv,
@@ -210,7 +235,7 @@ func NewServerConfiguration(conf *config.Couper, log *logrus.Entry, memStore *ca
 				return nil, err
 			}
 
-			corsOptions, cerr := middleware.NewCORSOptions(whichCORS(srvConf, srvConf.Files), nil)
+			corsOptions, cerr := middleware.NewCORSOptions(whichCORS(srvConf, srvConf.Files), allowedMethodsHandler.MethodAllowed)
 			if cerr != nil {
 				return nil, cerr
 			}
@@ -327,7 +352,7 @@ func NewServerConfiguration(conf *config.Couper, log *logrus.Entry, memStore *ca
 				allowedMethods = parentAPI.AllowedMethods
 			}
 			notAllowedMethodsHandler := epOpts.ErrorTemplate.WithError(errors.MethodNotAllowed)
-			allowedMethodsHandler := middleware.NewAllowedMethodsHandler(allowedMethods, protectedHandler, notAllowedMethodsHandler)
+			allowedMethodsHandler := middleware.NewAllowedMethodsHandler(allowedMethods, defaultEndpointAllowedMethods, protectedHandler, notAllowedMethodsHandler)
 			protectedHandler = allowedMethodsHandler
 
 			epHandler, err = configureProtectedHandler(accessControls, confCtx, accessControl,

diff --git a/handler/file.go b/handler/file.go
@@ -64,11 +64,6 @@ func NewFile(docRoot string, srvOpts *server.Options, modifier []hcl.Body) (*Fil
 }
 
 func (f *File) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
-	if req.Method != http.MethodGet && req.Method != http.MethodHead {
-		rw.WriteHeader(http.StatusMethodNotAllowed)
-		return
-	}
-
 	reqPath := f.removeBasePath(req.URL.Path)
 
 	file, info, err := f.openDocRootFile(reqPath)

diff --git a/handler/middleware/allowed_methods.go b/handler/middleware/allowed_methods.go
@@ -5,16 +5,6 @@ import (
 	"strings"
 )
 
-var defaultAllowedMethods = []string{
-	http.MethodGet,
-	http.MethodHead,
-	http.MethodPost,
-	http.MethodPut,
-	http.MethodPatch,
-	http.MethodDelete,
-	http.MethodOptions,
-}
-
 var _ http.Handler = &AllowedMethodsHandler{}
 
 type AllowedMethodsHandler struct {
@@ -25,17 +15,20 @@ type AllowedMethodsHandler struct {
 
 type methodAllowedFunc func(string) bool
 
-func NewAllowedMethodsHandler(allowedMethods []string, allowedHandler, notAllowedHandler http.Handler) *AllowedMethodsHandler {
+func NewAllowedMethodsHandler(allowedMethods, defaultAllowedMethods []string, allowedHandler, notAllowedHandler http.Handler) *AllowedMethodsHandler {
 	amh := &AllowedMethodsHandler{
 		allowedMethods:    make(map[string]struct{}),
 		allowedHandler:    allowedHandler,
 		notAllowedHandler: notAllowedHandler,
 	}
-	if allowedMethods == nil {
+	if allowedMethods == nil && defaultAllowedMethods != nil {
 		allowedMethods = defaultAllowedMethods
 	}
 	for _, method := range allowedMethods {
 		if method == "*" {
+			if defaultAllowedMethods == nil {
+				continue
+			}
 			for _, m := range defaultAllowedMethods {
 				amh.allowedMethods[m] = struct{}{}
 			}

diff --git a/handler/spa.go b/handler/spa.go
@@ -37,11 +37,6 @@ func NewSpa(bootstrapFile string, srvOpts *server.Options, modifier []hcl.Body)
 }
 
 func (s *Spa) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
-	if req.Method != http.MethodGet && req.Method != http.MethodHead {
-		rw.WriteHeader(http.StatusMethodNotAllowed)
-		return
-	}
-
 	file, err := os.Open(s.file)
 	if err != nil {
 		if _, ok := err.(*os.PathError); ok {

diff --git a/server/http.go b/server/http.go
@@ -72,7 +72,7 @@ func New(cmdCtx, evalCtx context.Context, log logrus.FieldLogger, settings *conf
 	muxersList := make(muxers)
 	for host, muxOpts := range hosts {
 		mux := NewMux(muxOpts)
-		mux.mustAddRoute(mux.endpointRoot, []string{http.MethodGet}, settings.HealthPath, handler.NewHealthCheck(settings.HealthPath, shutdownCh), true)
+		mux.registerHandler(mux.endpointRoot, []string{http.MethodGet}, settings.HealthPath, handler.NewHealthCheck(settings.HealthPath, shutdownCh))
 
 		muxersList[host] = mux
 	}

diff --git a/server/http_integration_test.go b/server/http_integration_test.go
@@ -4844,6 +4844,30 @@ func TestAllowedMethods(t *testing.T) {
 		{"restricted by api only, CONNECT", http.MethodConnect, "/api2/restrictedByApiOnly", http.Header{}, http.StatusMethodNotAllowed, "method not allowed error"},
 		{"restricted by api only, TRACE", http.MethodTrace, "/api2/restrictedByApiOnly", http.Header{}, http.StatusMethodNotAllowed, "method not allowed error"},
 		{"restricted by api only, BREW", "BREW", "/api2/restrictedByApiOnly", http.Header{}, http.StatusMethodNotAllowed, "method not allowed error"},
+
+		{"files, GET", http.MethodGet, "/index.html", http.Header{}, http.StatusOK, ""},
+		{"files, HEAD", http.MethodHead, "/index.html", http.Header{}, http.StatusOK, ""},
+		{"files, POST", http.MethodPost, "/index.html", http.Header{}, http.StatusMethodNotAllowed, "method not allowed error"},
+		{"files, PUT", http.MethodPut, "/index.html", http.Header{}, http.StatusMethodNotAllowed, "method not allowed error"},
+		{"files, PATCH", http.MethodPatch, "/index.html", http.Header{}, http.StatusMethodNotAllowed, "method not allowed error"},
+		{"files, DELETE", http.MethodDelete, "/index.html", http.Header{}, http.StatusMethodNotAllowed, "method not allowed error"},
+		{"files, OPTIONS", http.MethodOptions, "/index.html", http.Header{}, http.StatusMethodNotAllowed, "method not allowed error"},
+		{"files, CONNECT", http.MethodConnect, "/index.html", http.Header{}, http.StatusMethodNotAllowed, "method not allowed error"},
+		{"files, TRACE", http.MethodTrace, "/index.html", http.Header{}, http.StatusMethodNotAllowed, "method not allowed error"},
+		{"files, BREW", "BREW", "/index.html", http.Header{}, http.StatusMethodNotAllowed, "method not allowed error"},
+		{"files, CORS preflight", http.MethodOptions, "/index.html", http.Header{"Origin": []string{"https://www.example.com"}, "Access-Control-Request-Method": []string{"POST"}, "Access-Control-Request-Headers": []string{"Authorization"}}, http.StatusNoContent, ""},
+
+		{"spa, GET", http.MethodGet, "/app/foo", http.Header{}, http.StatusOK, ""},
+		{"spa, HEAD", http.MethodHead, "/app/foo", http.Header{}, http.StatusOK, ""},
+		{"spa, POST", http.MethodPost, "/app/foo", http.Header{}, http.StatusMethodNotAllowed, "method not allowed error"},
+		{"spa, PUT", http.MethodPut, "/app/foo", http.Header{}, http.StatusMethodNotAllowed, "method not allowed error"},
+		{"spa, PATCH", http.MethodPatch, "/app/foo", http.Header{}, http.StatusMethodNotAllowed, "method not allowed error"},
+		{"spa, DELETE", http.MethodDelete, "/app/foo", http.Header{}, http.StatusMethodNotAllowed, "method not allowed error"},
+		{"spa, OPTIONS", http.MethodOptions, "/app/foo", http.Header{}, http.StatusMethodNotAllowed, "method not allowed error"},
+		{"spa, CONNECT", http.MethodConnect, "/app/foo", http.Header{}, http.StatusMethodNotAllowed, "method not allowed error"},
+		{"spa, TRACE", http.MethodTrace, "/app/foo", http.Header{}, http.StatusMethodNotAllowed, "method not allowed error"},
+		{"spa, BREW", "BREW", "/app/foo", http.Header{}, http.StatusMethodNotAllowed, "method not allowed error"},
+		{"spa, CORS preflight", http.MethodOptions, "/app/foo", http.Header{"Origin": []string{"https://www.example.com"}, "Access-Control-Request-Method": []string{"POST"}, "Access-Control-Request-Headers": []string{"Authorization"}}, http.StatusNoContent, ""},
 	} {
 		t.Run(tc.name, func(subT *testing.T) {
 			helper := test.New(subT)

diff --git a/server/mux.go b/server/mux.go
@@ -16,6 +16,7 @@ import (
 	"github.com/avenga/couper/config/runtime/server"
 	"github.com/avenga/couper/errors"
 	"github.com/avenga/couper/handler"
+	"github.com/avenga/couper/handler/middleware"
 	"github.com/avenga/couper/utils"
 )
 
@@ -57,41 +58,38 @@ func NewMux(options *runtime.MuxOptions) *Mux {
 
 	for path, h := range opts.EndpointRoutes {
 		// TODO: handle method option per endpoint configuration
-		mux.mustAddRoute(mux.endpointRoot, nil, path, h, true)
+		mux.mustAddRoute(mux.endpointRoot, path, h, true)
 	}
 
 	for path, h := range opts.FileRoutes {
-		mux.mustAddRoute(mux.fileRoot, fileMethods, utils.JoinPath(path, "/**"), h, false)
+		mux.mustAddRoute(mux.fileRoot, utils.JoinPath(path, "/**"), h, false)
 	}
 
 	for path, h := range opts.SPARoutes {
-		mux.mustAddRoute(mux.spaRoot, fileMethods, path, h, false)
+		mux.mustAddRoute(mux.spaRoot, path, h, false)
 	}
 
 	return mux
 }
 
-func (m *Mux) mustAddRoute(root *pathpattern.Node, methods []string, path string, handler http.Handler, forEndpoint bool) *Mux {
-	if forEndpoint && strings.HasSuffix(path, wildcardSearch) {
-		route := mustCreateNode(root, handler, "", path)
-		m.handler[route] = handler
-		return m
-	}
+var noDefaultMethods []string
 
-	if methods == nil {
-		// EndpointRoutes allowed methods are handled by handler
-		route := mustCreateNode(root, handler, "", path)
-		m.handler[route] = handler
-
-		return m
-	}
+func (m *Mux) registerHandler(root *pathpattern.Node, methods []string, path string, handler http.Handler) {
+	notAllowedMethodsHandler := errors.DefaultJSON.WithError(errors.MethodNotAllowed)
+	allowedMethodsHandler := middleware.NewAllowedMethodsHandler(methods, noDefaultMethods, handler, notAllowedMethodsHandler)
+	m.mustAddRoute(root, path, allowedMethodsHandler, true)
+}
 
-	for _, method := range methods {
-		route := mustCreateNode(root, handler, method, path)
+func (m *Mux) mustAddRoute(root *pathpattern.Node, path string, handler http.Handler, forEndpoint bool) {
+	if forEndpoint && strings.HasSuffix(path, wildcardSearch) {
+		route := mustCreateNode(root, handler, "", path)
 		m.handler[route] = handler
+		return
 	}
 
-	return m
+	// EndpointRoutes allowed methods are handled by handler
+	route := mustCreateNode(root, handler, "", path)
+	m.handler[route] = handler
 }
 
 func (m *Mux) FindHandler(req *http.Request) http.Handler {
@@ -114,7 +112,7 @@ func (m *Mux) FindHandler(req *http.Request) http.Handler {
 			return fileHandler
 		}
 
-		node, paramValues = m.match(m.spaRoot, req)
+		node, paramValues = m.matchWithoutMethod(m.spaRoot, req)
 
 		if node == nil {
 			if fileHandler != nil {
@@ -163,7 +161,7 @@ func (m *Mux) matchWithoutMethod(root *pathpattern.Node, req *http.Request) (*pa
 }
 
 func (m *Mux) hasFileResponse(req *http.Request) (http.Handler, bool) {
-	node, _ := m.match(m.fileRoot, req)
+	node, _ := m.matchWithoutMethod(m.fileRoot, req)
 	if node == nil {
 		return nil, false
 	}

diff --git a/server/testdata/integration/config/11_couper.hcl b/server/testdata/integration/config/11_couper.hcl
@@ -1,4 +1,21 @@
 server {
+  files {
+    document_root = "../files/htdocs_a"
+
+    cors {
+      allowed_origins = ["*"]
+    }
+  }
+
+  spa {
+    bootstrap_file = "../files/htdocs_a/index.html"
+    paths = ["/app/**"]
+
+    cors {
+      allowed_origins = ["*"]
+    }
+  }
+
   api {
     base_path = "/api1"