Refactor auth middleware (#36848)

Principles: let the caller decide what it needs, but not let the
framework (middleware) guess what it should do.

Then a lot of hacky code can be removed. And some FIXMEs can be fixed.

This PR introduces a new kind of middleware: "PreMiddleware", it will be
executed before all other middlewares on the same routing level, then a
route can declare its options for other middlewares.

By the way, allow the workflow badge to be accessed by Basic or OAuth2
auth.

Fixes: https://github.com/go-gitea/gitea/pull/36830
Fixes: https://github.com/go-gitea/gitea/issues/36859

modules/web/handler.go

@@ -70,7 +70,8 @@ func preCheckHandler(fn reflect.Value, argsIn []reflect.Value) {
 
 func prepareHandleArgsIn(resp http.ResponseWriter, req *http.Request, fn reflect.Value, fnInfo *routing.FuncInfo) []reflect.Value {
 	defer func() {
-		if err := recover(); err != nil {
+		if recovered := recover(); recovered != nil {
+			err := fmt.Errorf("%v\n%s", recovered, log.Stack(2))
 			log.Error("unable to prepare handler arguments for %s: %v", fnInfo.String(), err)
 			panic(err)
 		}
@@ -117,7 +118,17 @@ func hasResponseBeenWritten(argsIn []reflect.Value) bool {
 	return false
 }
 
-func wrapHandlerProvider[T http.Handler](hp func(next http.Handler) T, funcInfo *routing.FuncInfo) func(next http.Handler) http.Handler {
+type middlewareProvider = func(next http.Handler) http.Handler
+
+func executeMiddlewaresHandler(w http.ResponseWriter, r *http.Request, middlewares []middlewareProvider, endpoint http.HandlerFunc) {
+	handler := endpoint
+	for i := len(middlewares) - 1; i >= 0; i-- {
+		handler = middlewares[i](handler).ServeHTTP
+	}
+	handler(w, r)
+}
+
+func wrapHandlerProvider[T http.Handler](hp func(next http.Handler) T, funcInfo *routing.FuncInfo) middlewareProvider {
 	return func(next http.Handler) http.Handler {
 		h := hp(next) // this handle could be dynamically generated, so we can't use it for debug info
 		return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) {
@@ -129,14 +140,14 @@ func wrapHandlerProvider[T http.Handler](hp func(next http.Handler) T, funcInfo
 
 // toHandlerProvider converts a handler to a handler provider
 // A handler provider is a function that takes a "next" http.Handler, it can be used as a middleware
-func toHandlerProvider(handler any) func(next http.Handler) http.Handler {
+func toHandlerProvider(handler any) middlewareProvider {
 	funcInfo := routing.GetFuncInfo(handler)
 	fn := reflect.ValueOf(handler)
 	if fn.Type().Kind() != reflect.Func {
 		panic(fmt.Sprintf("handler must be a function, but got %s", fn.Type()))
 	}
 
-	if hp, ok := handler.(func(next http.Handler) http.Handler); ok {
+	if hp, ok := handler.(middlewareProvider); ok {
 		return wrapHandlerProvider(hp, funcInfo)
 	} else if hp, ok := handler.(func(http.Handler) http.HandlerFunc); ok {
 		return wrapHandlerProvider(hp, funcInfo)

modules/web/router.go

@@ -18,6 +18,13 @@ import (
 	"github.com/go-chi/chi/v5"
 )
 
+// PreMiddlewareProvider is a special middleware provider which will be executed
+// before other middlewares on the same "routing" level (AfterRouting/Group/Methods/Any, but not BeforeRouting).
+// A route can do something (e.g.: set middleware options) at the place where it is declared,
+// and the code will be executed before other middlewares which are added before the declaration.
+// Use cases: mark a route with some meta info, set some options for middlewares, etc.
+type PreMiddlewareProvider func(next http.Handler) http.Handler
+
 // Bind binding an obj to a handler's context data
 func Bind[T any](_ T) http.HandlerFunc {
 	return func(resp http.ResponseWriter, req *http.Request) {
@@ -41,7 +48,10 @@ func GetForm(dataStore reqctx.RequestDataStore) any {
 
 // Router defines a route based on chi's router
 type Router struct {
-	chiRouter      *chi.Mux
+	chiRouter *chi.Mux
+
+	afterRouting []any
+
 	curGroupPrefix string
 	curMiddlewares []any
 }
@@ -52,8 +62,9 @@ func NewRouter() *Router {
 	return &Router{chiRouter: r}
 }
 
-// Use supports two middlewares
-func (r *Router) Use(middlewares ...any) {
+// BeforeRouting adds middlewares which will be executed before the request path gets routed
+// It should only be used for framework-level global middlewares when it needs to change request method & path.
+func (r *Router) BeforeRouting(middlewares ...any) {
 	for _, m := range middlewares {
 		if !isNilOrFuncNil(m) {
 			r.chiRouter.Use(toHandlerProvider(m))
@@ -61,7 +72,13 @@ func (r *Router) Use(middlewares ...any) {
 	}
 }
 
-// Group mounts a sub-Router along a `pattern` string.
+// AfterRouting adds middlewares which will be executed after the request path gets routed
+// It can see the routed path and resolved path parameters
+func (r *Router) AfterRouting(middlewares ...any) {
+	r.afterRouting = append(r.afterRouting, middlewares...)
+}
+
+// Group mounts a sub-router along a "pattern" string.
 func (r *Router) Group(pattern string, fn func(), middlewares ...any) {
 	previousGroupPrefix := r.curGroupPrefix
 	previousMiddlewares := r.curMiddlewares
@@ -93,36 +110,54 @@ func isNilOrFuncNil(v any) bool {
 	return r.Kind() == reflect.Func && r.IsNil()
 }
 
-func wrapMiddlewareAndHandler(curMiddlewares, h []any) ([]func(http.Handler) http.Handler, http.HandlerFunc) {
-	handlerProviders := make([]func(http.Handler) http.Handler, 0, len(curMiddlewares)+len(h)+1)
-	for _, m := range curMiddlewares {
-		if !isNilOrFuncNil(m) {
-			handlerProviders = append(handlerProviders, toHandlerProvider(m))
+func wrapMiddlewareAppendPre(all []middlewareProvider, middlewares []any) []middlewareProvider {
+	for _, m := range middlewares {
+		if h, ok := m.(PreMiddlewareProvider); ok && h != nil {
+			all = append(all, toHandlerProvider(middlewareProvider(h)))
 		}
 	}
+	return all
+}
+
+func wrapMiddlewareAppendNormal(all []middlewareProvider, middlewares []any) []middlewareProvider {
+	for _, m := range middlewares {
+		if _, ok := m.(PreMiddlewareProvider); !ok && !isNilOrFuncNil(m) {
+			all = append(all, toHandlerProvider(m))
+		}
+	}
+	return all
+}
+
+func wrapMiddlewareAndHandler(useMiddlewares, curMiddlewares, h []any) (_ []middlewareProvider, _ http.HandlerFunc, hasPreMiddlewares bool) {
 	if len(h) == 0 {
 		panic("no endpoint handler provided")
 	}
-	for i, m := range h {
-		if !isNilOrFuncNil(m) {
-			handlerProviders = append(handlerProviders, toHandlerProvider(m))
-		} else if i == len(h)-1 {
-			panic("endpoint handler can't be nil")
-		}
+	if isNilOrFuncNil(h[len(h)-1]) {
+		panic("endpoint handler can't be nil")
 	}
+
+	handlerProviders := make([]middlewareProvider, 0, len(useMiddlewares)+len(curMiddlewares)+len(h)+1)
+	handlerProviders = wrapMiddlewareAppendPre(handlerProviders, useMiddlewares)
+	handlerProviders = wrapMiddlewareAppendPre(handlerProviders, curMiddlewares)
+	handlerProviders = wrapMiddlewareAppendPre(handlerProviders, h)
+	hasPreMiddlewares = len(handlerProviders) > 0
+	handlerProviders = wrapMiddlewareAppendNormal(handlerProviders, useMiddlewares)
+	handlerProviders = wrapMiddlewareAppendNormal(handlerProviders, curMiddlewares)
+	handlerProviders = wrapMiddlewareAppendNormal(handlerProviders, h)
+
 	middlewares := handlerProviders[:len(handlerProviders)-1]
 	handlerFunc := handlerProviders[len(handlerProviders)-1](nil).ServeHTTP
 	mockPoint := RouterMockPoint(MockAfterMiddlewares)
 	if mockPoint != nil {
 		middlewares = append(middlewares, mockPoint)
 	}
-	return middlewares, handlerFunc
+	return middlewares, handlerFunc, hasPreMiddlewares
 }
 
 // Methods adds the same handlers for multiple http "methods" (separated by ",").
 // If any method is invalid, the lower level router will panic.
 func (r *Router) Methods(methods, pattern string, h ...any) {
-	middlewares, handlerFunc := wrapMiddlewareAndHandler(r.curMiddlewares, h)
+	middlewares, handlerFunc, _ := wrapMiddlewareAndHandler(r.afterRouting, r.curMiddlewares, h)
 	fullPattern := r.getPattern(pattern)
 	if strings.Contains(methods, ",") {
 		methods := strings.SplitSeq(methods, ",")
@@ -134,15 +169,19 @@ func (r *Router) Methods(methods, pattern string, h ...any) {
 	}
 }
 
-// Mount attaches another Router along ./pattern/*
+// Mount attaches another Router along "/pattern/*"
 func (r *Router) Mount(pattern string, subRouter *Router) {
-	subRouter.Use(r.curMiddlewares...)
-	r.chiRouter.Mount(r.getPattern(pattern), subRouter.chiRouter)
+	handlerProviders := make([]middlewareProvider, 0, len(r.afterRouting)+len(r.curMiddlewares))
+	handlerProviders = wrapMiddlewareAppendPre(handlerProviders, r.afterRouting)
+	handlerProviders = wrapMiddlewareAppendPre(handlerProviders, r.curMiddlewares)
+	handlerProviders = wrapMiddlewareAppendNormal(handlerProviders, r.afterRouting)
+	handlerProviders = wrapMiddlewareAppendNormal(handlerProviders, r.curMiddlewares)
+	r.chiRouter.With(handlerProviders...).Mount(r.getPattern(pattern), subRouter.chiRouter)
 }
 
 // Any delegate requests for all methods
 func (r *Router) Any(pattern string, h ...any) {
-	middlewares, handlerFunc := wrapMiddlewareAndHandler(r.curMiddlewares, h)
+	middlewares, handlerFunc, _ := wrapMiddlewareAndHandler(r.afterRouting, r.curMiddlewares, h)
 	r.chiRouter.With(middlewares...).HandleFunc(r.getPattern(pattern), handlerFunc)
 }
 
@@ -178,12 +217,16 @@ func (r *Router) Patch(pattern string, h ...any) {
 
 // ServeHTTP implements http.Handler
 func (r *Router) ServeHTTP(w http.ResponseWriter, req *http.Request) {
+	// TODO: need to move it to the top-level common middleware, otherwise each "Mount" will cause it to be executed multiple times, which is inefficient.
 	r.normalizeRequestPath(w, req, r.chiRouter)
 }
 
 // NotFound defines a handler to respond whenever a route could not be found.
 func (r *Router) NotFound(h http.HandlerFunc) {
-	r.chiRouter.NotFound(h)
+	middlewares, handlerFunc, _ := wrapMiddlewareAndHandler(r.afterRouting, r.curMiddlewares, []any{h})
+	r.chiRouter.NotFound(func(w http.ResponseWriter, r *http.Request) {
+		executeMiddlewaresHandler(w, r, middlewares, handlerFunc)
+	})
 }
 
 func (r *Router) normalizeRequestPath(resp http.ResponseWriter, req *http.Request, next http.Handler) {

modules/web/router_path.go

@@ -27,11 +27,7 @@ func (g *RouterPathGroup) ServeHTTP(resp http.ResponseWriter, req *http.Request)
 	for _, m := range g.matchers {
 		if m.matchPath(chiCtx, path) {
 			chiCtx.RoutePatterns = append(chiCtx.RoutePatterns, m.pattern)
-			handler := m.handlerFunc
-			for i := len(m.middlewares) - 1; i >= 0; i-- {
-				handler = m.middlewares[i](handler).ServeHTTP
-			}
-			handler(resp, req)
+			executeMiddlewaresHandler(resp, req, m.middlewares, m.handlerFunc)
 			return
 		}
 	}
@@ -67,7 +63,7 @@ type routerPathMatcher struct {
 	pattern     string
 	re          *regexp.Regexp
 	params      []routerPathParam
-	middlewares []func(http.Handler) http.Handler
+	middlewares []middlewareProvider
 	handlerFunc http.HandlerFunc
 }
 
@@ -111,7 +107,10 @@ func isValidMethod(name string) bool {
 }
 
 func newRouterPathMatcher(methods string, patternRegexp *RouterPathGroupPattern, h ...any) *routerPathMatcher {
-	middlewares, handlerFunc := wrapMiddlewareAndHandler(patternRegexp.middlewares, h)
+	middlewares, handlerFunc, hasPreMiddlewares := wrapMiddlewareAndHandler(nil, patternRegexp.middlewares, h)
+	if hasPreMiddlewares {
+		panic("pre-middlewares are not supported in router path matcher")
+	}
 	p := &routerPathMatcher{methods: make(container.Set[string]), middlewares: middlewares, handlerFunc: handlerFunc}
 	for method := range strings.SplitSeq(methods, ",") {
 		method = strings.TrimSpace(method)

modules/web/router_test.go

@@ -30,6 +30,71 @@ func chiURLParamsToMap(chiCtx *chi.Context) map[string]string {
 	return util.Iif(len(m) == 0, nil, m)
 }
 
+type testResult struct {
+	method          string
+	pathParams      map[string]string
+	handlerMarks    []string
+	chiRoutePattern *string
+}
+
+type testRecorder struct {
+	res testResult
+}
+
+func (r *testRecorder) reset() {
+	r.res = testResult{}
+}
+
+func (r *testRecorder) handle(optMark ...string) func(resp http.ResponseWriter, req *http.Request) {
+	mark := util.OptionalArg(optMark, "")
+	return func(resp http.ResponseWriter, req *http.Request) {
+		chiCtx := chi.RouteContext(req.Context())
+		r.res.method = req.Method
+		r.res.pathParams = chiURLParamsToMap(chiCtx)
+		r.res.chiRoutePattern = new(chiCtx.RoutePattern())
+		if mark != "" {
+			r.res.handlerMarks = append(r.res.handlerMarks, mark)
+		}
+	}
+}
+
+func (r *testRecorder) provider(optMark ...string) func(next http.Handler) http.Handler {
+	return func(next http.Handler) http.Handler {
+		return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) {
+			r.handle(optMark...)(resp, req)
+			next.ServeHTTP(resp, req)
+		})
+	}
+}
+
+func (r *testRecorder) stop(optMark ...string) func(resp http.ResponseWriter, req *http.Request) {
+	mark := util.OptionalArg(optMark, "")
+	return func(resp http.ResponseWriter, req *http.Request) {
+		if stop := req.FormValue("stop"); stop != "" && (mark == "" || mark == stop) {
+			r.handle(stop)(resp, req)
+			resp.WriteHeader(http.StatusOK)
+		} else if mark != "" {
+			r.res.handlerMarks = append(r.res.handlerMarks, mark)
+		}
+	}
+}
+
+func (r *testRecorder) test(t *testing.T, rt *Router, methodPath string, expected testResult) {
+	r.reset()
+	methodPathFields := strings.Fields(methodPath)
+	req, err := http.NewRequest(methodPathFields[0], methodPathFields[1], nil)
+	assert.NoError(t, err)
+
+	buff := &bytes.Buffer{}
+	httpRecorder := httptest.NewRecorder()
+	httpRecorder.Body = buff
+	rt.ServeHTTP(httpRecorder, req)
+	if expected.chiRoutePattern == nil {
+		r.res.chiRoutePattern = nil
+	}
+	assert.Equal(t, expected, r.res)
+}
+
 func TestPathProcessor(t *testing.T) {
 	testProcess := func(pattern, uri string, expectedPathParams map[string]string) {
 		chiCtx := chi.NewRouteContext()
@@ -51,42 +116,10 @@ func TestPathProcessor(t *testing.T) {
 }
 
 func TestRouter(t *testing.T) {
-	buff := &bytes.Buffer{}
-	recorder := httptest.NewRecorder()
-	recorder.Body = buff
-
-	type resultStruct struct {
-		method          string
-		pathParams      map[string]string
-		handlerMarks    []string
-		chiRoutePattern *string
-	}
-
-	var res resultStruct
-	h := func(optMark ...string) func(resp http.ResponseWriter, req *http.Request) {
-		mark := util.OptionalArg(optMark, "")
-		return func(resp http.ResponseWriter, req *http.Request) {
-			chiCtx := chi.RouteContext(req.Context())
-			res.method = req.Method
-			res.pathParams = chiURLParamsToMap(chiCtx)
-			res.chiRoutePattern = new(chiCtx.RoutePattern())
-			if mark != "" {
-				res.handlerMarks = append(res.handlerMarks, mark)
-			}
-		}
-	}
-
-	stopMark := func(optMark ...string) func(resp http.ResponseWriter, req *http.Request) {
-		mark := util.OptionalArg(optMark, "")
-		return func(resp http.ResponseWriter, req *http.Request) {
-			if stop := req.FormValue("stop"); stop != "" && (mark == "" || mark == stop) {
-				h(stop)(resp, req)
-				resp.WriteHeader(http.StatusOK)
-			} else if mark != "" {
-				res.handlerMarks = append(res.handlerMarks, mark)
-			}
-		}
-	}
+	type resultStruct = testResult
+	resRecorder := &testRecorder{}
+	h := resRecorder.handle
+	stopMark := resRecorder.stop
 
 	r := NewRouter()
 	r.NotFound(h("not-found:/"))
@@ -123,15 +156,7 @@ func TestRouter(t *testing.T) {
 
 	testRoute := func(t *testing.T, methodPath string, expected resultStruct) {
 		t.Run(methodPath, func(t *testing.T) {
-			res = resultStruct{}
-			methodPathFields := strings.Fields(methodPath)
-			req, err := http.NewRequest(methodPathFields[0], methodPathFields[1], nil)
-			assert.NoError(t, err)
-			r.ServeHTTP(recorder, req)
-			if expected.chiRoutePattern == nil {
-				res.chiRoutePattern = nil
-			}
-			assert.Equal(t, expected, res)
+			resRecorder.test(t, r, methodPath, expected)
 		})
 	}
 
@@ -273,3 +298,39 @@ func TestRouteNormalizePath(t *testing.T) {
 	testPath("/v2/", paths{EscapedPath: "/v2", RawPath: "/v2", Path: "/v2"})
 	testPath("/v2/%2f", paths{EscapedPath: "/v2/%2f", RawPath: "/v2/%2f", Path: "/v2//"})
 }
+
+func TestPreMiddlewareProvider(t *testing.T) {
+	resRecorder := &testRecorder{}
+	h := resRecorder.handle
+	p := resRecorder.provider
+
+	root := NewRouter()
+	root.BeforeRouting(h("before-root"))
+	root.AfterRouting(h("root"))
+	root.Get("/a/1", h("mid"), PreMiddlewareProvider(p("pre-root")), h("end1"))
+
+	sub := NewRouter()
+	sub.BeforeRouting(h("before-sub"))
+	sub.AfterRouting(h("sub"))
+	sub.Get("/2", h("mid"), PreMiddlewareProvider(p("pre-sub")), h("end2"))
+	sub.NotFound(h("not-found"))
+
+	root.Mount("/a", sub)
+
+	resRecorder.test(t, root, "GET /a/1", testResult{
+		method:       "GET",
+		handlerMarks: []string{"before-root", "pre-root", "root", "mid", "end1"},
+	})
+	resRecorder.test(t, root, "GET /a/2", testResult{
+		method:       "GET",
+		handlerMarks: []string{"before-root", "root", "before-sub", "pre-sub", "sub", "mid", "end2"},
+	})
+	resRecorder.test(t, root, "GET /no-such", testResult{
+		method:       "GET",
+		handlerMarks: []string{"before-root"},
+	})
+	resRecorder.test(t, root, "GET /a/no-such", testResult{
+		method:       "GET",
+		handlerMarks: []string{"before-root", "root", "before-sub", "sub", "not-found"},
+	})
+}

modules/web/routing/context.go

@@ -44,7 +44,7 @@ func MarkLongPolling(resp http.ResponseWriter, req *http.Request) {
 }
 
 // UpdatePanicError updates a context's error info, a panic may be recovered by other middlewares, but we still need to know that.
-func UpdatePanicError(ctx context.Context, err any) {
+func UpdatePanicError(ctx context.Context, err error) {
 	record, ok := ctx.Value(contextKey).(*requestRecord)
 	if !ok {
 		return

modules/web/routing/logger_manager.go

@@ -5,11 +5,13 @@ package routing
 
 import (
 	"context"
+	"fmt"
 	"net/http"
 	"sync"
 	"time"
 
 	"code.gitea.io/gitea/modules/graceful"
+	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/process"
 )
 
@@ -99,7 +101,7 @@ func (manager *requestRecordsManager) handler(next http.Handler) http.Handler {
 			localPanicErr := recover()
 			if localPanicErr != nil {
 				record.lock.Lock()
-				record.panicError = localPanicErr
+				record.panicError = fmt.Errorf("%v\n%s", localPanicErr, log.Stack(2))
 				record.lock.Unlock()
 			}
 

modules/web/routing/requestrecord.go

@@ -24,5 +24,5 @@ type requestRecord struct {
 	// mutable fields
 	isLongPolling bool
 	funcInfo      *FuncInfo
-	panicError    any
+	panicError    error
 }