diff --git a/app/helper_test.go b/app/helper_test.go index e277bc49f1..d8f5218837 100644 --- a/app/helper_test.go +++ b/app/helper_test.go @@ -6,7 +6,6 @@ package app import ( "io" "io/ioutil" - "net/http" "os" "path/filepath" "time" @@ -16,7 +15,6 @@ import ( "github.com/mattermost/mattermost-server/mlog" "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/utils" - "github.com/mattermost/mattermost-server/utils/testutils" ) type TestHelper struct { @@ -32,8 +30,6 @@ type TestHelper struct { tempConfigPath string tempWorkspace string - - MockedHTTPService *testutils.MockedHTTPService } func setupTestHelper(enterprise bool) *TestHelper { @@ -133,13 +129,6 @@ func (me *TestHelper) InitBasic() *TestHelper { return me } -func (me *TestHelper) MockHTTPService(handler http.Handler) *TestHelper { - me.MockedHTTPService = testutils.MakeMockedHTTPService(handler) - me.App.HTTPService = me.MockedHTTPService - - return me -} - func (me *TestHelper) MakeEmail() string { return "success_" + model.NewId() + "@simulator.amazonses.com" } diff --git a/app/http_service_test.go b/app/http_service_test.go deleted file mode 100644 index 396a991b1b..0000000000 --- a/app/http_service_test.go +++ /dev/null @@ -1,65 +0,0 @@ -package app - -import ( - "io/ioutil" - "net/http" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestMockHTTPService(t *testing.T) { - getCalled := false - putCalled := false - - handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == "/get" && r.Method == http.MethodGet { - getCalled = true - - w.WriteHeader(http.StatusOK) - w.Write([]byte("OK")) - } else if r.URL.Path == "/put" && r.Method == http.MethodPut { - putCalled = true - - w.WriteHeader(http.StatusCreated) - w.Write([]byte("CREATED")) - } else { - w.WriteHeader(http.StatusNotFound) - } - }) - - th := Setup().MockHTTPService(handler) - defer th.TearDown() - - url := th.MockedHTTPService.Server.URL - - t.Run("GET", func(t *testing.T) { - client := th.App.HTTPService.MakeClient(false) - - resp, err := client.Get(url + "/get") - defer consumeAndClose(resp) - - bodyContents, _ := ioutil.ReadAll(resp.Body) - - require.Nil(t, err) - assert.Equal(t, http.StatusOK, resp.StatusCode) - assert.Equal(t, "OK", string(bodyContents)) - assert.True(t, getCalled) - }) - - t.Run("PUT", func(t *testing.T) { - client := th.App.HTTPService.MakeClient(false) - - request, _ := http.NewRequest(http.MethodPut, url+"/put", nil) - resp, err := client.Do(request) - defer consumeAndClose(resp) - - bodyContents, _ := ioutil.ReadAll(resp.Body) - - require.Nil(t, err) - assert.Equal(t, http.StatusCreated, resp.StatusCode) - assert.Equal(t, "CREATED", string(bodyContents)) - assert.True(t, putCalled) - }) -} diff --git a/app/integration_action.go b/app/integration_action.go index a8ea700279..9f40a2f808 100644 --- a/app/integration_action.go +++ b/app/integration_action.go @@ -27,7 +27,6 @@ import ( "strings" "github.com/mattermost/mattermost-server/model" - "github.com/mattermost/mattermost-server/services/httpservice" "github.com/mattermost/mattermost-server/utils" ) @@ -132,7 +131,7 @@ func (a *App) DoActionRequest(rawURL string, body []byte) (*http.Response, *mode req.Header.Set("Accept", "application/json") // Allow access to plugin routes for action buttons - var httpClient *httpservice.Client + var httpClient *http.Client url, _ := url.Parse(rawURL) siteURL, _ := url.Parse(*a.Config().ServiceSettings.SiteURL) subpath, _ := utils.GetSubpathFromConfig(a.Config()) diff --git a/app/server.go b/app/server.go index b8d660bdf0..ba817cc1d1 100644 --- a/app/server.go +++ b/app/server.go @@ -425,9 +425,6 @@ func (s *Server) Shutdown() error { s.DisableConfigWatch() - if s.HTTPService != nil { - s.HTTPService.Close() - } mlog.Info("Server stopped") return nil } diff --git a/services/httpservice/client.go b/services/httpservice/client.go index 1e7b7b5f92..51e1391271 100644 --- a/services/httpservice/client.go +++ b/services/httpservice/client.go @@ -15,8 +15,8 @@ import ( ) const ( - connectTimeout = 3 * time.Second - requestTimeout = 30 * time.Second + ConnectTimeout = 3 * time.Second + RequestTimeout = 30 * time.Second ) var reservedIPRanges []*net.IPNet @@ -102,25 +102,9 @@ func dialContextFilter(dial DialContextFunction, allowHost func(host string) boo } } -type Client struct { - *http.Client -} - -func (c *Client) Do(req *http.Request) (*http.Response, error) { - req.Header.Set("User-Agent", defaultUserAgent) - return c.Client.Do(req) -} - -// NewHTTPClient returns a variation the default implementation of Client. -// It uses a Transport with the same settings as the default Transport -// but with the following modifications: -// - shorter timeout for dial and TLS handshake (defined as constant -// "connectTimeout") -// - timeout for the end-to-end request (defined as constant -// "requestTimeout") -func NewHTTPClient(enableInsecureConnections bool, allowHost func(host string) bool, allowIP func(ip net.IP) bool) *Client { +func NewTransport(enableInsecureConnections bool, allowHost func(host string) bool, allowIP func(ip net.IP) bool) http.RoundTripper { dialContext := (&net.Dialer{ - Timeout: connectTimeout, + Timeout: ConnectTimeout, KeepAlive: 30 * time.Second, }).DialContext @@ -128,20 +112,24 @@ func NewHTTPClient(enableInsecureConnections bool, allowHost func(host string) b dialContext = dialContextFilter(dialContext, allowHost, allowIP) } - client := &http.Client{ - Transport: &http.Transport{ + return &MattermostTransport{ + &http.Transport{ Proxy: http.ProxyFromEnvironment, DialContext: dialContext, MaxIdleConns: 100, IdleConnTimeout: 90 * time.Second, - TLSHandshakeTimeout: connectTimeout, + TLSHandshakeTimeout: ConnectTimeout, ExpectContinueTimeout: 1 * time.Second, TLSClientConfig: &tls.Config{ InsecureSkipVerify: enableInsecureConnections, }, }, - Timeout: requestTimeout, } - - return &Client{Client: client} +} + +func NewHTTPClient(transport http.RoundTripper) *http.Client { + return &http.Client{ + Transport: transport, + Timeout: RequestTimeout, + } } diff --git a/services/httpservice/client_test.go b/services/httpservice/client_test.go index 174ddf2de1..9f3cc66446 100644 --- a/services/httpservice/client_test.go +++ b/services/httpservice/client_test.go @@ -16,7 +16,7 @@ import ( func TestHTTPClient(t *testing.T) { for _, allowInternal := range []bool{true, false} { - c := NewHTTPClient(false, func(_ string) bool { return false }, func(ip net.IP) bool { return allowInternal || !IsReservedIP(ip) }) + c := NewHTTPClient(NewTransport(false, func(_ string) bool { return false }, func(ip net.IP) bool { return allowInternal || !IsReservedIP(ip) })) for _, tc := range []struct { URL string IsInternal bool @@ -56,9 +56,9 @@ func TestHTTPClientWithProxy(t *testing.T) { proxy := createProxyServer() defer proxy.Close() - c := NewHTTPClient(true, nil, nil) + c := NewHTTPClient(NewTransport(true, nil, nil)) purl, _ := url.Parse(proxy.URL) - c.Transport.(*http.Transport).Proxy = http.ProxyURL(purl) + c.Transport.(*MattermostTransport).Transport.(*http.Transport).Proxy = http.ProxyURL(purl) resp, err := c.Get("http://acme.com") if err != nil { @@ -132,7 +132,7 @@ func TestUserAgentIsSet(t *testing.T) { } })) defer ts.Close() - client := NewHTTPClient(true, nil, nil) + client := NewHTTPClient(NewTransport(true, nil, nil)) req, err := http.NewRequest("GET", ts.URL, nil) if err != nil { t.Fatal("NewRequest failed", err) diff --git a/services/httpservice/httpservice.go b/services/httpservice/httpservice.go index 6e776890fc..d0eb2cc6e8 100644 --- a/services/httpservice/httpservice.go +++ b/services/httpservice/httpservice.go @@ -5,15 +5,25 @@ package httpservice import ( "net" + "net/http" "strings" "github.com/mattermost/mattermost-server/services/configservice" ) -// Wraps the functionality for creating a new http.Client to encapsulate that and allow it to be mocked when testing +// HTTPService wraps the functionality for making http requests to provide some improvements to the default client +// behaviour. type HTTPService interface { - MakeClient(trustURLs bool) *Client - Close() + // MakeClient returns an http client constructed with a RoundTripper as returned by MakeTransport. + MakeClient(trustURLs bool) *http.Client + + // MakeTransport returns a RoundTripper that is suitable for making requests to external resources. The default + // implementation provides: + // - A shorter timeout for dial and TLS handshake (defined as constant "ConnectTimeout") + // - A timeout for end-to-end requests (defined as constant "RequestTimeout") + // - A Mattermost-specific user agent header + // - Additional security for untrusted and insecure connections + MakeTransport(trustURLs bool) http.RoundTripper } type HTTPServiceImpl struct { @@ -24,11 +34,15 @@ func MakeHTTPService(configService configservice.ConfigService) HTTPService { return &HTTPServiceImpl{configService} } -func (h *HTTPServiceImpl) MakeClient(trustURLs bool) *Client { +func (h *HTTPServiceImpl) MakeClient(trustURLs bool) *http.Client { + return NewHTTPClient(h.MakeTransport(trustURLs)) +} + +func (h *HTTPServiceImpl) MakeTransport(trustURLs bool) http.RoundTripper { insecure := h.configService.Config().ServiceSettings.EnableInsecureOutgoingConnections != nil && *h.configService.Config().ServiceSettings.EnableInsecureOutgoingConnections if trustURLs { - return NewHTTPClient(insecure, nil, nil) + return NewTransport(insecure, nil, nil) } allowHost := func(host string) bool { @@ -58,9 +72,5 @@ func (h *HTTPServiceImpl) MakeClient(trustURLs bool) *Client { return false } - return NewHTTPClient(insecure, allowHost, allowIP) -} - -func (h *HTTPServiceImpl) Close() { - // Does nothing, but allows this to be overridden when mocking the service + return NewTransport(insecure, allowHost, allowIP) } diff --git a/services/httpservice/transport.go b/services/httpservice/transport.go new file mode 100644 index 0000000000..f442d0c8b2 --- /dev/null +++ b/services/httpservice/transport.go @@ -0,0 +1,21 @@ +// Copyright (c) 2017-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package httpservice + +import ( + "net/http" +) + +// MattermostTransport is an implementation of http.RoundTripper that ensures each request contains a custom user agent +// string to indicate that the request is coming from a Mattermost instance. +type MattermostTransport struct { + // Transport is the underlying http.RoundTripper that is actually used to make the request + Transport http.RoundTripper +} + +func (t *MattermostTransport) RoundTrip(req *http.Request) (*http.Response, error) { + req.Header.Set("User-Agent", defaultUserAgent) + + return t.Transport.RoundTrip(req) +} diff --git a/utils/testutils/mocked_http_service.go b/utils/testutils/mocked_http_service.go deleted file mode 100644 index 9b083ef083..0000000000 --- a/utils/testutils/mocked_http_service.go +++ /dev/null @@ -1,30 +0,0 @@ -// Copyright (c) 2016-present Mattermost, Inc. All Rights Reserved. -// See License.txt for license information. - -package testutils - -import ( - "net/http" - "net/http/httptest" - - "github.com/mattermost/mattermost-server/services/httpservice" -) - -type MockedHTTPService struct { - Server *httptest.Server -} - -func MakeMockedHTTPService(handler http.Handler) *MockedHTTPService { - return &MockedHTTPService{ - Server: httptest.NewServer(handler), - } -} - -func (h *MockedHTTPService) MakeClient(trustURLs bool) *httpservice.Client { - return &httpservice.Client{Client: h.Server.Client()} -} - -func (h *MockedHTTPService) Close() { - h.Server.CloseClientConnections() - h.Server.Close() -}