Conversation
c089c57 to
af4714d
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the auth middleware to follow the principle of "let the caller decide what it needs." It introduces a new PreMiddlewareProvider concept that executes before other middlewares, allowing routes to declare their own auth options (e.g., whether OAuth2 or Basic auth should be enabled) rather than having the middleware guess based on URL path patterns.
Changes:
- Removes path-based auth detection (
authPathDetectorand related regex logic) fromservices/auth, replacing it with explicit per-route flags (CreateSession,AllowOAuth2,AllowBasic) set by callers. - Introduces
PreMiddlewareProviderinmodules/web/router.goand reworkswrapMiddlewareAndHandlerto execute pre-middlewares before normal middlewares. - Updates
routers/web/web.goto build auth groups dynamically per-request based on context flags set by route-level pre-middlewares.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
services/auth/sspi.go |
Replaces path-based session-creation guard with a CreateSession struct field |
services/auth/reverseproxy.go |
Same CreateSession field added, path detection removed |
services/auth/oauth2.go |
Removes path-based gating; OAuth2 is now always attempted when invoked |
services/auth/basic.go |
Removes path-based gating; Basic auth is now always attempted when invoked |
services/auth/auth_test.go |
Deletes tests for the removed authPathDetector |
services/auth/auth.go |
Removes authPathDetector, regex globals, and related helpers |
routers/web/web.go |
Introduces AuthMiddleware with PreMiddlewareProvider-based flags; updates route registrations |
routers/api/v1/api.go |
Removes SSPI from API auth group; adds clarifying comment |
modules/web/router_test.go |
Refactors test helpers into reusable testRecorder; adds TestPreMiddlewareProvider |
modules/web/router_path.go |
Delegates to executeMiddlewaresHandler; panics on pre-middlewares in path matcher |
modules/web/router.go |
Implements PreMiddlewareProvider, wrapMiddlewareAppendPre/Normal, refactors wrapMiddlewareAndHandler |
modules/web/handler.go |
Introduces middlewareProvider type alias and executeMiddlewaresHandler helper |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
af4714d to
dc8bb42
Compare
1535443 to
644a0d8
Compare
61f45ae to
c1d78ed
Compare
c1d78ed to
279efff
Compare
|
Written by Claude. The recovered panic in err := fmt.Errorf("%v\n%s", recovered, log.Stack(2))
RenderPanicErrorPage(respWriter, req, err)Then combinedErr := fmt.Errorf("%w\n%s", err, log.Stack(2))The logged error will contain two stack traces — the useful one from the panic site and a redundant one from the render function. Since it's the same goroutine, the second stack just adds noise. Consider only capturing the stack at the |
A design problem due to history reasons. Ideally the panic stack should be handled in the "defer recover", but not in another function. To avoid unrelated changes in this PR, reverted to the old behavior. |
|
I see this includes a fix for #36859, edited the PR description with both refs. |
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: #36830
Fixes: #36859