From a4f7df6f6e3c9702f2e22dd1035086dde087cdc1 Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Mon, 24 May 2021 10:24:51 +0530 Subject: [PATCH] Remove all remaining occurences of fakeApp (#17661) * Remove all remaining occurences of fakeApp Migrated any remaining methods under server. And for everything else, renamed fakeApp to app. The word fakeApp is confusing and we should just call it for what it is - an app. https://focalboard-community.octo.mattermost.com/workspace/zyoahc9uapdn3xdptac6jb69ic?id=285b80a3-257d-41f6-8cf4-ed80ca9d92e5&v=495cdb4d-c13a-4992-8eb9-80cfee2819a4&c=639a0bc1-4401-43d5-81ec-0dd54e796d9a ```release-note NONE ``` * fix tests --- app/app_iface.go | 4 ---- app/integration_action.go | 2 +- app/notification_push.go | 6 +----- app/opentracing/opentracing_layer.go | 30 ---------------------------- app/plugin_requests.go | 28 +++++++++++++------------- app/plugin_requests_test.go | 2 +- app/plugin_test.go | 6 +++--- app/server.go | 23 ++++++++++----------- 8 files changed, 31 insertions(+), 70 deletions(-) diff --git a/app/app_iface.go b/app/app_iface.go index 1e1877d196..054a90f058 100644 --- a/app/app_iface.go +++ b/app/app_iface.go @@ -294,9 +294,6 @@ type AppIface interface { SendAdminUpgradeRequestEmail(username string, subscription *model.Subscription, action string) *model.AppError // SendNoCardPaymentFailedEmail SendNoCardPaymentFailedEmail() *model.AppError - // ServePluginPublicRequest serves public plugin files - // at the URL http(s)://$SITE_URL/plugins/$PLUGIN_ID/public/{anything} - ServePluginPublicRequest(w http.ResponseWriter, r *http.Request) // SessionHasPermissionToManageBot returns nil if the session has access to manage the given bot. // This function deviates from other authorization checks in returning an error instead of just // a boolean, allowing the permission failure to be exposed with more granularity. @@ -973,7 +970,6 @@ type AppIface interface { SendPasswordReset(email string, siteURL string) (bool, *model.AppError) SendPaymentFailedEmail(failedPayment *model.FailedPayment) *model.AppError ServeInterPluginRequest(w http.ResponseWriter, r *http.Request, sourcePluginId, destinationPluginId string) - ServePluginRequest(w http.ResponseWriter, r *http.Request) SessionCacheLength() int SessionHasPermissionTo(session model.Session, permission *model.Permission) bool SessionHasPermissionToAny(session model.Session, permissions []*model.Permission) bool diff --git a/app/integration_action.go b/app/integration_action.go index 814bb6ce8d..bdd78bc3a7 100644 --- a/app/integration_action.go +++ b/app/integration_action.go @@ -412,7 +412,7 @@ func (a *App) doPluginRequest(c *request.Context, method, rawURL string, values params["plugin_id"] = pluginID r = mux.SetURLVars(r, params) - a.ServePluginRequest(w, r) + a.srv.ServePluginRequest(w, r) resp := &http.Response{ StatusCode: w.status, diff --git a/app/notification_push.go b/app/notification_push.go index 24ed7400c3..45d608baac 100644 --- a/app/notification_push.go +++ b/app/notification_push.go @@ -262,13 +262,9 @@ func (a *App) UpdateMobileAppBadge(userID string) { func (s *Server) createPushNotificationsHub() { buffer := *s.Config().EmailSettings.PushNotificationBuffer - // XXX: This can be _almost_ removed except that there is a dependency with - // a.ClearSessionCacheForUser(session.UserId) which invalidates caches, - // which then takes to web_hub code. It's a bit complicated, so leaving as is for now. - fakeApp := New(ServerConnector(s)) hub := PushNotificationsHub{ notificationsChan: make(chan PushNotification, buffer), - app: fakeApp, + app: New(ServerConnector(s)), wg: new(sync.WaitGroup), semaWg: new(sync.WaitGroup), sema: make(chan struct{}, runtime.NumCPU()*8), // numCPU * 8 is a good amount of concurrency. diff --git a/app/opentracing/opentracing_layer.go b/app/opentracing/opentracing_layer.go index 42d3a60d5b..2f1d45946f 100644 --- a/app/opentracing/opentracing_layer.go +++ b/app/opentracing/opentracing_layer.go @@ -14272,36 +14272,6 @@ func (a *OpenTracingAppLayer) ServeInterPluginRequest(w http.ResponseWriter, r * a.app.ServeInterPluginRequest(w, r, sourcePluginId, destinationPluginId) } -func (a *OpenTracingAppLayer) ServePluginPublicRequest(w http.ResponseWriter, r *http.Request) { - origCtx := a.ctx - span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.ServePluginPublicRequest") - - a.ctx = newCtx - a.app.Srv().Store.SetContext(newCtx) - defer func() { - a.app.Srv().Store.SetContext(origCtx) - a.ctx = origCtx - }() - - defer span.Finish() - a.app.ServePluginPublicRequest(w, r) -} - -func (a *OpenTracingAppLayer) ServePluginRequest(w http.ResponseWriter, r *http.Request) { - origCtx := a.ctx - span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.ServePluginRequest") - - a.ctx = newCtx - a.app.Srv().Store.SetContext(newCtx) - defer func() { - a.app.Srv().Store.SetContext(origCtx) - a.ctx = origCtx - }() - - defer span.Finish() - a.app.ServePluginRequest(w, r) -} - func (a *OpenTracingAppLayer) SessionCacheLength() int { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.SessionCacheLength") diff --git a/app/plugin_requests.go b/app/plugin_requests.go index d0132fcb72..d49221233a 100644 --- a/app/plugin_requests.go +++ b/app/plugin_requests.go @@ -20,11 +20,11 @@ import ( "github.com/mattermost/mattermost-server/v5/utils" ) -func (a *App) ServePluginRequest(w http.ResponseWriter, r *http.Request) { - pluginsEnvironment := a.GetPluginsEnvironment() +func (s *Server) ServePluginRequest(w http.ResponseWriter, r *http.Request) { + pluginsEnvironment := s.GetPluginsEnvironment() if pluginsEnvironment == nil { err := model.NewAppError("ServePluginRequest", "app.plugin.disabled.app_error", nil, "Enable plugins to serve plugin requests", http.StatusNotImplemented) - a.Log().Error(err.Error()) + s.Log.Error(err.Error()) w.WriteHeader(err.StatusCode) w.Header().Set("Content-Type", "application/json") w.Write([]byte(err.ToJson())) @@ -34,7 +34,7 @@ func (a *App) ServePluginRequest(w http.ResponseWriter, r *http.Request) { params := mux.Vars(r) hooks, err := pluginsEnvironment.HooksForPlugin(params["plugin_id"]) if err != nil { - a.Log().Error("Access to route for non-existent plugin", + s.Log.Error("Access to route for non-existent plugin", mlog.String("missing_plugin_id", params["plugin_id"]), mlog.String("url", r.URL.String()), mlog.Err(err)) @@ -42,7 +42,7 @@ func (a *App) ServePluginRequest(w http.ResponseWriter, r *http.Request) { return } - a.servePluginRequest(w, r, hooks.ServeHTTP) + s.servePluginRequest(w, r, hooks.ServeHTTP) } func (a *App) ServeInterPluginRequest(w http.ResponseWriter, r *http.Request, sourcePluginId, destinationPluginId string) { @@ -81,7 +81,7 @@ func (a *App) ServeInterPluginRequest(w http.ResponseWriter, r *http.Request, so // ServePluginPublicRequest serves public plugin files // at the URL http(s)://$SITE_URL/plugins/$PLUGIN_ID/public/{anything} -func (a *App) ServePluginPublicRequest(w http.ResponseWriter, r *http.Request) { +func (s *Server) ServePluginPublicRequest(w http.ResponseWriter, r *http.Request) { if strings.HasSuffix(r.URL.Path, "/") { http.NotFound(w, r) return @@ -91,7 +91,7 @@ func (a *App) ServePluginPublicRequest(w http.ResponseWriter, r *http.Request) { vars := mux.Vars(r) pluginID := vars["plugin_id"] - pluginsEnv := a.GetPluginsEnvironment() + pluginsEnv := s.GetPluginsEnvironment() // Check if someone has nullified the pluginsEnv in the meantime if pluginsEnv == nil { @@ -115,11 +115,11 @@ func (a *App) ServePluginPublicRequest(w http.ResponseWriter, r *http.Request) { http.ServeFile(w, r, publicFile) } -func (a *App) servePluginRequest(w http.ResponseWriter, r *http.Request, handler func(*plugin.Context, http.ResponseWriter, *http.Request)) { +func (s *Server) servePluginRequest(w http.ResponseWriter, r *http.Request, handler func(*plugin.Context, http.ResponseWriter, *http.Request)) { token := "" context := &plugin.Context{ RequestId: model.NewId(), - IpAddress: utils.GetIPAddress(r, a.Config().ServiceSettings.TrustedProxyIPHeader), + IpAddress: utils.GetIPAddress(r, s.Config().ServiceSettings.TrustedProxyIPHeader), AcceptLanguage: r.Header.Get("Accept-Language"), UserAgent: r.UserAgent(), } @@ -142,7 +142,7 @@ func (a *App) servePluginRequest(w http.ResponseWriter, r *http.Request, handler r.Header.Del("Mattermost-User-Id") if token != "" { - session, err := a.GetSession(token) + session, err := New(ServerConnector(s)).GetSession(token) defer ReturnSessionToPool(session) csrfCheckPassed := false @@ -184,10 +184,10 @@ func (a *App) servePluginRequest(w http.ResponseWriter, r *http.Request, handler mlog.String("user_id", userID), } - if *a.Config().ServiceSettings.ExperimentalStrictCSRFEnforcement { - a.Log().Warn(csrfErrorMessage, fields...) + if *s.Config().ServiceSettings.ExperimentalStrictCSRFEnforcement { + s.Log.Warn(csrfErrorMessage, fields...) } else { - a.Log().Debug(csrfErrorMessage, fields...) + s.Log.Debug(csrfErrorMessage, fields...) csrfCheckPassed = true } } @@ -213,7 +213,7 @@ func (a *App) servePluginRequest(w http.ResponseWriter, r *http.Request, handler params := mux.Vars(r) - subpath, _ := utils.GetSubpathFromConfig(a.Config()) + subpath, _ := utils.GetSubpathFromConfig(s.Config()) newQuery := r.URL.Query() newQuery.Del("access_token") diff --git a/app/plugin_requests_test.go b/app/plugin_requests_test.go index a99dec176c..bb1f293299 100644 --- a/app/plugin_requests_test.go +++ b/app/plugin_requests_test.go @@ -40,7 +40,7 @@ func TestServePluginPublicRequest(t *testing.T) { require.NoError(t, err) rr := httptest.NewRecorder() - handler := http.HandlerFunc(app.ServePluginPublicRequest) + handler := http.HandlerFunc(srv.ServePluginPublicRequest) handler.ServeHTTP(rr, req) assert.Equal(t, http.StatusNotFound, rr.Code) diff --git a/app/plugin_test.go b/app/plugin_test.go index 15e6f7d5a9..169406ea4b 100644 --- a/app/plugin_test.go +++ b/app/plugin_test.go @@ -342,7 +342,7 @@ func TestServePluginRequest(t *testing.T) { w := httptest.NewRecorder() r := httptest.NewRequest("GET", "/plugins/foo/bar", nil) - th.App.ServePluginRequest(w, r) + th.App.srv.ServePluginRequest(w, r) assert.Equal(t, http.StatusNotImplemented, w.Result().StatusCode) } @@ -386,7 +386,7 @@ func TestPrivateServePluginRequest(t *testing.T) { request = mux.SetURLVars(request, map[string]string{"plugin_id": "id"}) - th.App.servePluginRequest(recorder, request, handler) + th.App.srv.servePluginRequest(recorder, request, handler) }) } @@ -409,7 +409,7 @@ func TestHandlePluginRequest(t *testing.T) { var assertions func(*http.Request) router := mux.NewRouter() router.HandleFunc("/plugins/{plugin_id:[A-Za-z0-9\\_\\-\\.]+}/{anything:.*}", func(_ http.ResponseWriter, r *http.Request) { - th.App.servePluginRequest(nil, r, func(_ *plugin.Context, _ http.ResponseWriter, r *http.Request) { + th.App.srv.servePluginRequest(nil, r, func(_ *plugin.Context, _ http.ResponseWriter, r *http.Request) { assertions(r) }) }) diff --git a/app/server.go b/app/server.go index 549659bb39..55f7b22f29 100644 --- a/app/server.go +++ b/app/server.go @@ -248,8 +248,8 @@ func NewServer(options ...Option) (*Server, error) { // It is important to initialize the hub only after the global logger is set // to avoid race conditions while logging from inside the hub. - fakeApp := New(ServerConnector(s)) - fakeApp.HubStart() + app := New(ServerConnector(s)) + app.HubStart() if *s.Config().LogSettings.EnableDiagnostics && *s.Config().LogSettings.EnableSentry { if strings.Contains(SentryDSN, "placeholder") { @@ -489,11 +489,10 @@ func NewServer(options ...Option) (*Server, error) { } s.Router = s.RootRouter.PathPrefix(subpath).Subrouter() - // FakeApp: remove this when we have the ServePluginRequest and ServePluginPublicRequest migrated in the server pluginsRoute := s.Router.PathPrefix("/plugins/{plugin_id:[A-Za-z0-9\\_\\-\\.]+}").Subrouter() - pluginsRoute.HandleFunc("", fakeApp.ServePluginRequest) - pluginsRoute.HandleFunc("/public/{public_file:.*}", fakeApp.ServePluginPublicRequest) - pluginsRoute.HandleFunc("/{anything:.*}", fakeApp.ServePluginRequest) + pluginsRoute.HandleFunc("", s.ServePluginRequest) + pluginsRoute.HandleFunc("/public/{public_file:.*}", s.ServePluginPublicRequest) + pluginsRoute.HandleFunc("/{anything:.*}", s.ServePluginRequest) // If configured with a subpath, redirect 404s at the root back into the subpath. if subpath != "/" { @@ -505,7 +504,7 @@ func NewServer(options ...Option) (*Server, error) { s.WebSocketRouter = &WebSocketRouter{ handlers: make(map[string]webSocketHandler), - app: fakeApp, + app: app, } mailConfig := s.MailServiceConfig() @@ -634,7 +633,7 @@ func NewServer(options ...Option) (*Server, error) { // if enabled - perform initial product notices fetch if *s.Config().AnnouncementSettings.AdminNoticesEnabled || *s.Config().AnnouncementSettings.UserNoticesEnabled { go func() { - if err := fakeApp.UpdateProductNotices(); err != nil { + if err := app.UpdateProductNotices(); err != nil { mlog.Warn("Failied to perform initial product notices fetch", mlog.Err(err)) } }() @@ -647,7 +646,7 @@ func NewServer(options ...Option) (*Server, error) { c := request.EmptyContext() s.AddConfigListener(func(oldConfig *model.Config, newConfig *model.Config) { if *oldConfig.GuestAccountsSettings.Enable && !*newConfig.GuestAccountsSettings.Enable { - if appErr := fakeApp.DeactivateGuests(c); appErr != nil { + if appErr := app.DeactivateGuests(c); appErr != nil { mlog.Error("Unable to deactivate guest accounts", mlog.Err(appErr)) } } @@ -655,7 +654,7 @@ func NewServer(options ...Option) (*Server, error) { // Disable active guest accounts on first run if guest accounts are disabled if !*s.Config().GuestAccountsSettings.Enable { - if appErr := fakeApp.DeactivateGuests(c); appErr != nil { + if appErr := app.DeactivateGuests(c); appErr != nil { mlog.Error("Unable to deactivate guest accounts", mlog.Err(appErr)) } } @@ -663,8 +662,8 @@ func NewServer(options ...Option) (*Server, error) { if s.runEssentialJobs { s.Go(func() { s.runLicenseExpirationCheckJob() - runCheckAdminSupportStatusJob(fakeApp, c) - runCheckWarnMetricStatusJob(fakeApp, c) + runCheckAdminSupportStatusJob(app, c) + runCheckWarnMetricStatusJob(app, c) }) s.runJobs() }