From e1e385b318f4c944769e9c7013ff683e52fbec33 Mon Sep 17 00:00:00 2001 From: Serge Zaitsev Date: Mon, 13 Sep 2021 16:41:03 +0300 Subject: [PATCH] Chore: Remove untyped data map from macaron context (#39077) --- pkg/macaron/context.go | 1 - pkg/macaron/macaron.go | 1 - pkg/middleware/auth.go | 3 +- pkg/middleware/logger.go | 20 +++++------ pkg/middleware/org_redirect.go | 5 +-- pkg/middleware/recovery.go | 33 ++++++++++--------- pkg/models/context.go | 19 +++++++---- .../contexthandler/auth_proxy_test.go | 7 ++-- pkg/services/contexthandler/contexthandler.go | 3 +- 9 files changed, 44 insertions(+), 48 deletions(-) diff --git a/pkg/macaron/context.go b/pkg/macaron/context.go index 75ed5669e43..1c1164e58d6 100644 --- a/pkg/macaron/context.go +++ b/pkg/macaron/context.go @@ -45,7 +45,6 @@ type Context struct { Resp ResponseWriter params Params template *template.Template - Data map[string]interface{} } func (ctx *Context) handler() Handler { diff --git a/pkg/macaron/macaron.go b/pkg/macaron/macaron.go index 2f0d5188dcc..714445e5720 100644 --- a/pkg/macaron/macaron.go +++ b/pkg/macaron/macaron.go @@ -157,7 +157,6 @@ func (m *Macaron) createContext(rw http.ResponseWriter, req *http.Request) *Cont index: 0, Router: m.Router, Resp: NewResponseWriter(req.Method, rw), - Data: make(map[string]interface{}), } req = req.WithContext(context.WithValue(req.Context(), macaronContextKey{}, c)) c.Map(c) diff --git a/pkg/middleware/auth.go b/pkg/middleware/auth.go index 267a69c87a6..ff9b66433d9 100644 --- a/pkg/middleware/auth.go +++ b/pkg/middleware/auth.go @@ -111,9 +111,8 @@ func Auth(options *AuthOptions) macaron.Handler { requireLogin := !c.AllowAnonymous || forceLogin || options.ReqNoAnonynmous if !c.IsSignedIn && options.ReqSignedIn && requireLogin { - lookupTokenErr, hasTokenErr := c.Data["lookupTokenErr"].(error) var revokedErr *models.TokenRevokedError - if hasTokenErr && errors.As(lookupTokenErr, &revokedErr) { + if errors.As(c.LookupTokenErr, &revokedErr) { tokenRevoked(c, revokedErr) return } diff --git a/pkg/middleware/logger.go b/pkg/middleware/logger.go index d9e1daa411a..9732ea2c7c2 100644 --- a/pkg/middleware/logger.go +++ b/pkg/middleware/logger.go @@ -19,9 +19,8 @@ import ( "net/http" "time" - "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/contexthandler" "github.com/grafana/grafana/pkg/setting" - "github.com/prometheus/client_golang/prometheus" cw "github.com/weaveworks/common/middleware" "gopkg.in/macaron.v1" ) @@ -29,16 +28,15 @@ import ( func Logger(cfg *setting.Cfg) macaron.Handler { return func(res http.ResponseWriter, req *http.Request, c *macaron.Context) { start := time.Now() - c.Data["perfmon.start"] = start rw := res.(macaron.ResponseWriter) c.Next() timeTaken := time.Since(start) / time.Millisecond - if timer, ok := c.Data["perfmon.timer"]; ok { - timerTyped := timer.(prometheus.Summary) - timerTyped.Observe(float64(timeTaken)) + ctx := contexthandler.FromContext(c.Req.Context()) + if ctx != nil && ctx.PerfmonTimer != nil { + ctx.PerfmonTimer.Observe(float64(timeTaken)) } status := rw.Status() @@ -48,9 +46,7 @@ func Logger(cfg *setting.Cfg) macaron.Handler { } } - if ctx, ok := c.Data["ctx"]; ok { - ctxTyped := ctx.(*models.ReqContext) - + if ctx != nil { logParams := []interface{}{ "method", req.Method, "path", req.URL.Path, @@ -61,15 +57,15 @@ func Logger(cfg *setting.Cfg) macaron.Handler { "referer", req.Referer(), } - traceID, exist := cw.ExtractTraceID(ctxTyped.Req.Context()) + traceID, exist := cw.ExtractTraceID(ctx.Req.Context()) if exist { logParams = append(logParams, "traceID", traceID) } if status >= 500 { - ctxTyped.Logger.Error("Request Completed", logParams...) + ctx.Logger.Error("Request Completed", logParams...) } else { - ctxTyped.Logger.Info("Request Completed", logParams...) + ctx.Logger.Info("Request Completed", logParams...) } } } diff --git a/pkg/middleware/org_redirect.go b/pkg/middleware/org_redirect.go index 59cbddf58e9..8dfc87a7edf 100644 --- a/pkg/middleware/org_redirect.go +++ b/pkg/middleware/org_redirect.go @@ -8,6 +8,7 @@ import ( "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/contexthandler" "github.com/grafana/grafana/pkg/setting" "gopkg.in/macaron.v1" ) @@ -23,8 +24,8 @@ func OrgRedirect(cfg *setting.Cfg) macaron.Handler { return } - ctx, ok := c.Data["ctx"].(*models.ReqContext) - if !ok || !ctx.IsSignedIn { + ctx := contexthandler.FromContext(req.Context()) + if !ctx.IsSignedIn { return } diff --git a/pkg/middleware/recovery.go b/pkg/middleware/recovery.go index 364ac2ac865..5fbad323a92 100644 --- a/pkg/middleware/recovery.go +++ b/pkg/middleware/recovery.go @@ -26,7 +26,7 @@ import ( "gopkg.in/macaron.v1" "github.com/grafana/grafana/pkg/infra/log" - "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/contexthandler" "github.com/grafana/grafana/pkg/setting" ) @@ -109,9 +109,9 @@ func Recovery(cfg *setting.Cfg) macaron.Handler { if r := recover(); r != nil { panicLogger := log.Root // try to get request logger - if ctx, ok := c.Data["ctx"]; ok { - ctxTyped := ctx.(*models.ReqContext) - panicLogger = ctxTyped.Logger + ctx := contexthandler.FromContext(c.Req.Context()) + if ctx != nil { + panicLogger = ctx.Logger } if err, ok := r.(error); ok { @@ -132,33 +132,34 @@ func Recovery(cfg *setting.Cfg) macaron.Handler { return } - c.Data["Title"] = "Server Error" - c.Data["AppSubUrl"] = cfg.AppSubURL - c.Data["Theme"] = cfg.DefaultTheme + data := struct { + Title string + AppSubUrl string + Theme string + ErrorMsg string + }{"Server Error", cfg.AppSubURL, cfg.DefaultTheme, ""} if setting.Env == setting.Dev { if err, ok := r.(error); ok { - c.Data["Title"] = err.Error() + data.Title = err.Error() } - c.Data["ErrorMsg"] = string(stack) + data.ErrorMsg = string(stack) } - ctx, ok := c.Data["ctx"].(*models.ReqContext) - - if ok && ctx.IsApiRequest() { + if ctx != nil && ctx.IsApiRequest() { resp := make(map[string]interface{}) resp["message"] = "Internal Server Error - Check the Grafana server logs for the detailed error message." - if c.Data["ErrorMsg"] != nil { - resp["error"] = fmt.Sprintf("%v - %v", c.Data["Title"], c.Data["ErrorMsg"]) + if data.ErrorMsg != "" { + resp["error"] = fmt.Sprintf("%v - %v", data.Title, data.ErrorMsg) } else { - resp["error"] = c.Data["Title"] + resp["error"] = data.Title } c.JSON(500, resp) } else { - c.HTML(500, cfg.ErrTemplateName, c.Data) + c.HTML(500, cfg.ErrTemplateName, data) } } }() diff --git a/pkg/models/context.go b/pkg/models/context.go index a3bb6838985..b631f80bc22 100644 --- a/pkg/models/context.go +++ b/pkg/models/context.go @@ -21,22 +21,27 @@ type ReqContext struct { Logger log.Logger // RequestNonce is a cryptographic request identifier for use with Content Security Policy. RequestNonce string + + PerfmonTimer prometheus.Summary + LookupTokenErr error } // Handle handles and logs error by given status. func (ctx *ReqContext) Handle(cfg *setting.Cfg, status int, title string, err error) { + data := struct { + Title string + AppSubUrl string + Theme string + ErrorMsg error + }{title, cfg.AppSubURL, "dark", nil} if err != nil { ctx.Logger.Error(title, "error", err) if setting.Env != setting.Prod { - ctx.Data["ErrorMsg"] = err + data.ErrorMsg = err } } - ctx.Data["Title"] = title - ctx.Data["AppSubUrl"] = cfg.AppSubURL - ctx.Data["Theme"] = "dark" - - ctx.HTML(status, cfg.ErrTemplateName, ctx.Data) + ctx.HTML(status, cfg.ErrTemplateName, data) } func (ctx *ReqContext) IsApiRequest() bool { @@ -76,7 +81,7 @@ func (ctx *ReqContext) HasHelpFlag(flag HelpFlags1) bool { } func (ctx *ReqContext) TimeRequest(timer prometheus.Summary) { - ctx.Data["perfmon.timer"] = timer + ctx.PerfmonTimer = timer } // QueryBoolWithDefault extracts a value from the request query params and applies a bool default if not present. diff --git a/pkg/services/contexthandler/auth_proxy_test.go b/pkg/services/contexthandler/auth_proxy_test.go index 072a0004283..1d4b7ce82b2 100644 --- a/pkg/services/contexthandler/auth_proxy_test.go +++ b/pkg/services/contexthandler/auth_proxy_test.go @@ -58,11 +58,8 @@ func TestInitContextWithAuthProxy_CachedInvalidUserID(t *testing.T) { req, err := http.NewRequest("POST", "http://example.com", nil) require.NoError(t, err) ctx := &models.ReqContext{ - Context: &macaron.Context{ - Req: req, - Data: map[string]interface{}{}, - }, - Logger: log.New("Test"), + Context: &macaron.Context{Req: req}, + Logger: log.New("Test"), } req.Header.Set(svc.Cfg.AuthProxyHeaderName, name) h, err := authproxy.HashCacheKey(name) diff --git a/pkg/services/contexthandler/contexthandler.go b/pkg/services/contexthandler/contexthandler.go index a593d673f2f..f8253df52cf 100644 --- a/pkg/services/contexthandler/contexthandler.go +++ b/pkg/services/contexthandler/contexthandler.go @@ -121,7 +121,6 @@ func (h *ContextHandler) Middleware(mContext *macaron.Context) { } reqContext.Logger = log.New("context", "userId", reqContext.UserId, "orgId", reqContext.OrgId, "uname", reqContext.Login) - reqContext.Data["ctx"] = reqContext span.LogFields( ol.String("uname", reqContext.Login), @@ -299,7 +298,7 @@ func (h *ContextHandler) initContextWithToken(reqContext *models.ReqContext, org token, err := h.AuthTokenService.LookupToken(ctx, rawToken) if err != nil { reqContext.Logger.Error("Failed to look up user based on cookie", "error", err) - reqContext.Data["lookupTokenErr"] = err + reqContext.LookupTokenErr = err return false }