fix: Add middleware earlier #1775 #1776
Conversation
abador
changed the title
cmd/daemon/serve.go
Outdated
| n.Use(reqlog.NewMiddlewareFromLogger(l, "public#"+c.SelfPublicURL(nil).String())) | ||
| n.Use(sqa(ctx, cmd, r)) | ||
| n.Use(r.PrometheusManager()) |
There was a problem hiding this comment.
Please move this after the for loop below
cmd/daemon/serve.go
Outdated
| n.Use(reqlog.NewMiddlewareFromLogger(l, "admin#"+c.SelfPublicURL(nil).String())) | ||
| n.Use(sqa(ctx, cmd, r)) | ||
| n.Use(r.PrometheusManager()) |
There was a problem hiding this comment.
Please move this after the for loop below
Yeah this also looked a bit weird to me. This shouldn't make much difference, but it does, since the CSRFHandler is based on the router |
|
Oh I see, that makes sense! :) |
| for _, mw := range modifiers.mwf { | ||
| n.UseFunc(mw) | ||
| } | ||
| n.Use(r.PrometheusManager()) |
There was a problem hiding this comment.
I meant to say to move all n.Use after the modifiers :) The modifiers are used only for testing, and it is important for them to execute before all other middleware. Thanks!
Codecov Report
@@ Coverage Diff @@
## master #1776 +/- ##
==========================================
+ Coverage 74.10% 74.13% +0.02%
==========================================
Files 260 260
Lines 12715 12733 +18
==========================================
+ Hits 9423 9439 +16
- Misses 2667 2668 +1
- Partials 625 626 +1
Continue to review full report at Codecov.
|
|
|
Related issue(s)
#1775
Checklist
contributing code guidelines.
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
Further Comments
No docs and tests, itonly changes the order of middlewares