From 2bcaa42dc0e029e2596fcabdc3ec24d141feede3 Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Tue, 4 Jun 2024 14:06:37 -0400 Subject: [PATCH] MM-58275 Ensure image proxy site URL is updated when that changes (#27214) * MM-58275 Ensure image proxy site URL is updated when that changes * Check if proxy settings changed using reflection --- .../utils/testutils/static_config_service.go | 32 +++++++-- .../services/imageproxy/atmos_camo.go | 28 ++++---- .../services/imageproxy/atmos_camo_test.go | 9 ++- .../services/imageproxy/imageproxy.go | 20 +++--- .../services/imageproxy/imageproxy_test.go | 70 +++++++++++++++++++ server/platform/services/imageproxy/local.go | 9 +-- 6 files changed, 127 insertions(+), 41 deletions(-) diff --git a/server/channels/utils/testutils/static_config_service.go b/server/channels/utils/testutils/static_config_service.go index 72e41459a3..66be06c942 100644 --- a/server/channels/utils/testutils/static_config_service.go +++ b/server/channels/utils/testutils/static_config_service.go @@ -11,23 +11,41 @@ import ( type StaticConfigService struct { Cfg *model.Config + + listeners map[string]func(old, current *model.Config) } -func (s StaticConfigService) Config() *model.Config { +func (s *StaticConfigService) Config() *model.Config { return s.Cfg } -func (StaticConfigService) AddConfigListener(func(old, current *model.Config)) string { - return "" +func (s *StaticConfigService) AddConfigListener(listener func(old, current *model.Config)) string { + if s.listeners == nil { + s.listeners = make(map[string]func(old, current *model.Config)) + } + + listenerID := model.NewId() + s.listeners[listenerID] = listener + return listenerID } -func (StaticConfigService) RemoveConfigListener(string) { - +func (s *StaticConfigService) RemoveConfigListener(listenerID string) { + delete(s.listeners, listenerID) } -func (StaticConfigService) AsymmetricSigningKey() *ecdsa.PrivateKey { +func (s *StaticConfigService) AsymmetricSigningKey() *ecdsa.PrivateKey { return &ecdsa.PrivateKey{} } -func (StaticConfigService) PostActionCookieSecret() []byte { + +func (s *StaticConfigService) PostActionCookieSecret() []byte { return make([]byte, 32) } + +func (s *StaticConfigService) UpdateConfig(newConfig *model.Config) { + oldConfig := s.Config() + s.Cfg = newConfig + + for _, listener := range s.listeners { + listener(oldConfig, newConfig) + } +} diff --git a/server/platform/services/imageproxy/atmos_camo.go b/server/platform/services/imageproxy/atmos_camo.go index b8157852cf..9e9d1f89c9 100644 --- a/server/platform/services/imageproxy/atmos_camo.go +++ b/server/platform/services/imageproxy/atmos_camo.go @@ -10,26 +10,27 @@ import ( "io" "net/http" "net/url" + + "github.com/mattermost/mattermost/server/public/model" ) type AtmosCamoBackend struct { - proxy *ImageProxy - siteURL *url.URL - remoteURL *url.URL - client *http.Client + siteURL *url.URL + remoteOptions string + remoteURL *url.URL + client *http.Client } -func makeAtmosCamoBackend(proxy *ImageProxy) *AtmosCamoBackend { +func makeAtmosCamoBackend(proxy *ImageProxy, proxySettings model.ImageProxySettings) *AtmosCamoBackend { // We deliberately ignore the error because it's from config.json. // The function returns a nil pointer in case of error, and we handle it when it's used. - siteURL, _ := url.Parse(*proxy.ConfigService.Config().ServiceSettings.SiteURL) - remoteURL, _ := url.Parse(*proxy.ConfigService.Config().ImageProxySettings.RemoteImageProxyURL) + remoteURL, _ := url.Parse(*proxySettings.RemoteImageProxyURL) return &AtmosCamoBackend{ - proxy: proxy, - siteURL: siteURL, - remoteURL: remoteURL, - client: proxy.HTTPService.MakeClient(false), + siteURL: proxy.siteURL, + remoteURL: remoteURL, + remoteOptions: *proxySettings.RemoteImageProxyOptions, + client: proxy.HTTPService.MakeClient(false), } } @@ -53,9 +54,6 @@ func (backend *AtmosCamoBackend) GetImageDirect(imageURL string) (io.ReadCloser, } func (backend *AtmosCamoBackend) getAtmosCamoImageURL(imageURL string) string { - cfg := *backend.proxy.ConfigService.Config() - options := *cfg.ImageProxySettings.RemoteImageProxyOptions - if imageURL == "" || backend.siteURL == nil { return imageURL } @@ -84,7 +82,7 @@ func (backend *AtmosCamoBackend) getAtmosCamoImageURL(imageURL string) string { } urlBytes := []byte(parsedURL.String()) - mac := hmac.New(sha1.New, []byte(options)) + mac := hmac.New(sha1.New, []byte(backend.remoteOptions)) mac.Write(urlBytes) digest := hex.EncodeToString(mac.Sum(nil)) diff --git a/server/platform/services/imageproxy/atmos_camo_test.go b/server/platform/services/imageproxy/atmos_camo_test.go index f1c1d20fc4..45f5b0f6d8 100644 --- a/server/platform/services/imageproxy/atmos_camo_test.go +++ b/server/platform/services/imageproxy/atmos_camo_test.go @@ -66,14 +66,13 @@ func TestAtmosCamoBackend_GetImageDirect(t *testing.T) { defer mock.Close() proxy := makeTestAtmosCamoProxy() - parsedURL, err := url.Parse(*proxy.ConfigService.Config().ServiceSettings.SiteURL) + parsedURL, err := url.Parse("https://mattermost.example.com") require.NoError(t, err) remoteURL, err := url.Parse(mock.URL) require.NoError(t, err) backend := &AtmosCamoBackend{ - proxy: proxy, siteURL: parsedURL, remoteURL: remoteURL, client: proxy.HTTPService.MakeClient(false), @@ -177,9 +176,9 @@ func TestGetAtmosCamoImageURL(t *testing.T) { require.NoError(t, err) backend := &AtmosCamoBackend{ - proxy: makeTestAtmosCamoProxy(), - siteURL: parsedURL, - remoteURL: remoteURL, + siteURL: parsedURL, + remoteURL: remoteURL, + remoteOptions: *makeTestAtmosCamoProxy().ConfigService.Config().ImageProxySettings.RemoteImageProxyOptions, } assert.Equal(t, test.Expected, backend.getAtmosCamoImageURL(test.Input)) diff --git a/server/platform/services/imageproxy/imageproxy.go b/server/platform/services/imageproxy/imageproxy.go index 2737691e68..13a9309b0d 100644 --- a/server/platform/services/imageproxy/imageproxy.go +++ b/server/platform/services/imageproxy/imageproxy.go @@ -8,6 +8,7 @@ import ( "io" "net/http" "net/url" + "reflect" "strings" "sync" @@ -59,21 +60,21 @@ func MakeImageProxy(configService configservice.ConfigService, httpService https proxy.configListenerID = proxy.ConfigService.AddConfigListener(proxy.OnConfigChange) config := proxy.ConfigService.Config() - proxy.backend = proxy.makeBackend(*config.ImageProxySettings.Enable, *config.ImageProxySettings.ImageProxyType) + proxy.backend = proxy.makeBackend(config.ImageProxySettings) return proxy } -func (proxy *ImageProxy) makeBackend(enable bool, proxyType string) ImageProxyBackend { - if !enable { +func (proxy *ImageProxy) makeBackend(proxySettings model.ImageProxySettings) ImageProxyBackend { + if !*proxySettings.Enable { return nil } - switch proxyType { + switch *proxySettings.ImageProxyType { case model.ImageProxyTypeLocal: return makeLocalBackend(proxy) case model.ImageProxyTypeAtmosCamo: - return makeAtmosCamoBackend(proxy) + return makeAtmosCamoBackend(proxy, proxySettings) default: return nil } @@ -87,12 +88,15 @@ func (proxy *ImageProxy) Close() { } func (proxy *ImageProxy) OnConfigChange(oldConfig, newConfig *model.Config) { - if *oldConfig.ImageProxySettings.Enable != *newConfig.ImageProxySettings.Enable || - *oldConfig.ImageProxySettings.ImageProxyType != *newConfig.ImageProxySettings.ImageProxyType { + if *oldConfig.ServiceSettings.SiteURL != *newConfig.ServiceSettings.SiteURL || + !reflect.DeepEqual(oldConfig.ImageProxySettings, newConfig.ImageProxySettings) { proxy.lock.Lock() defer proxy.lock.Unlock() - proxy.backend = proxy.makeBackend(*newConfig.ImageProxySettings.Enable, *newConfig.ImageProxySettings.ImageProxyType) + siteURL, _ := url.Parse(*newConfig.ServiceSettings.SiteURL) + proxy.siteURL = siteURL + + proxy.backend = proxy.makeBackend(newConfig.ImageProxySettings) } } diff --git a/server/platform/services/imageproxy/imageproxy_test.go b/server/platform/services/imageproxy/imageproxy_test.go index dcd21b0060..537d52cc2c 100644 --- a/server/platform/services/imageproxy/imageproxy_test.go +++ b/server/platform/services/imageproxy/imageproxy_test.go @@ -7,6 +7,8 @@ import ( "net/url" "testing" + "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/v8/channels/utils/testutils" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -115,3 +117,71 @@ func TestGetUnproxiedImageURL(t *testing.T) { }) } } + +func TestOnConfigChange(t *testing.T) { + t.Run("should switch between backends", func(t *testing.T) { + proxy := makeTestAtmosCamoProxy() + + require.Equal(t, "https://mattermost.example.com", proxy.backend.(*AtmosCamoBackend).siteURL.String()) + + newConfig := proxy.ConfigService.Config().Clone() + newConfig.ImageProxySettings.ImageProxyType = model.NewString(model.ImageProxyTypeLocal) + + proxy.ConfigService.(*testutils.StaticConfigService).UpdateConfig(newConfig) + + require.Equal(t, "https://mattermost.example.com", proxy.backend.(*LocalBackend).baseURL.String()) + + newConfig = proxy.ConfigService.Config().Clone() + newConfig.ImageProxySettings.ImageProxyType = model.NewString(model.ImageProxyTypeAtmosCamo) + + proxy.ConfigService.(*testutils.StaticConfigService).UpdateConfig(newConfig) + + require.Equal(t, "https://mattermost.example.com", proxy.backend.(*AtmosCamoBackend).siteURL.String()) + }) + + t.Run("for local proxy, should update site URL when that changes", func(t *testing.T) { + proxy := makeTestLocalProxy() + + require.Equal(t, "https://mattermost.example.com", proxy.siteURL.String()) + require.Equal(t, "https://mattermost.example.com", proxy.backend.(*LocalBackend).baseURL.String()) + + newConfig := proxy.ConfigService.Config().Clone() + newConfig.ServiceSettings.SiteURL = model.NewString("https://new.example.com") + + proxy.ConfigService.(*testutils.StaticConfigService).UpdateConfig(newConfig) + + require.Equal(t, "https://new.example.com", proxy.siteURL.String()) + require.Equal(t, "https://new.example.com", proxy.backend.(*LocalBackend).baseURL.String()) + }) + + t.Run("for atmos/camo proxy, should update site URL when that changes", func(t *testing.T) { + proxy := makeTestAtmosCamoProxy() + + require.Equal(t, "https://mattermost.example.com", proxy.siteURL.String()) + require.Equal(t, "https://mattermost.example.com", proxy.backend.(*AtmosCamoBackend).siteURL.String()) + + newConfig := proxy.ConfigService.Config().Clone() + newConfig.ServiceSettings.SiteURL = model.NewString("https://new.example.com") + + proxy.ConfigService.(*testutils.StaticConfigService).UpdateConfig(newConfig) + + require.Equal(t, "https://new.example.com", proxy.siteURL.String()) + require.Equal(t, "https://new.example.com", proxy.backend.(*AtmosCamoBackend).siteURL.String()) + }) + + t.Run("for atmos/camo proxy, should update additional options when those change", func(t *testing.T) { + proxy := makeTestAtmosCamoProxy() + + require.Equal(t, "http://images.example.com", proxy.backend.(*AtmosCamoBackend).remoteURL.String()) + // require.Equal(t, "7e5f3fab20b94782b43cdb022a66985ef28ba355df2c5d5da3c9a05e4b697bac", proxy.backend.(*AtmosCamoBackend).remoteOptions) + + newConfig := proxy.ConfigService.Config().Clone() + newConfig.ImageProxySettings.RemoteImageProxyURL = model.NewString("https://new.example.com") + newConfig.ImageProxySettings.RemoteImageProxyOptions = model.NewString("some other random hash") + + proxy.ConfigService.(*testutils.StaticConfigService).UpdateConfig(newConfig) + + require.Equal(t, "https://new.example.com", proxy.backend.(*AtmosCamoBackend).remoteURL.String()) + // require.Equal(t, "some other random hash", proxy.backend.(*AtmosCamoBackend).remoteOptions) + }) +} diff --git a/server/platform/services/imageproxy/local.go b/server/platform/services/imageproxy/local.go index a01c154973..33b8169e2e 100644 --- a/server/platform/services/imageproxy/local.go +++ b/server/platform/services/imageproxy/local.go @@ -40,8 +40,6 @@ var msgNotAllowed = "requested URL is not allowed" var ErrLocalRequestFailed = Error{errors.New("imageproxy.LocalBackend: failed to request proxied image")} type LocalBackend struct { - proxy *ImageProxy - client *http.Client baseURL *url.URL } @@ -57,15 +55,14 @@ func (e URLError) Error() string { } func makeLocalBackend(proxy *ImageProxy) *LocalBackend { - baseURL, err := url.Parse(*proxy.ConfigService.Config().ServiceSettings.SiteURL) - if err != nil { - mlog.Warn("Failed to set base URL for image proxy. Relative image links may not work.", mlog.Err(err)) + baseURL := proxy.siteURL + if baseURL == nil { + mlog.Warn("Failed to set base URL for image proxy. Relative image links may not work.") } client := proxy.HTTPService.MakeClient(false) return &LocalBackend{ - proxy: proxy, client: client, baseURL: baseURL, }