MM-10417 Improve HTTPService for use in image proxy (#9966)

* Replaced httpservice with proper http.Client

* Added HTTPService.MakeTransport

* Expose timeouts used by HTTPServiceImpl

* Add additional documentation to HTTPService

* Remove MockedHTTPService

* Fix missing license

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"
 }

app/http_service_test.go

@@ -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)
-	})
-}

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())

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
 }

services/httpservice/client.go

@@ -15,8 +15,8 @@ import (
 )
 
 const (
-	connectTimeout = 3 * time.Second
+	ConnectTimeout = 3 * time.Second
-	requestTimeout = 30 * 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 {
+func NewTransport(enableInsecureConnections bool, allowHost func(host string) bool, allowIP func(ip net.IP) bool) http.RoundTripper {
-	*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 {
 	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{
+	return &MattermostTransport{
-		Transport: &http.Transport{
+		&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,
+	}
 }

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)

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
+	// MakeClient returns an http client constructed with a RoundTripper as returned by MakeTransport.
-	Close()
+	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)
+	return NewTransport(insecure, allowHost, allowIP)
-}
-
-func (h *HTTPServiceImpl) Close() {
-	// Does nothing, but allows this to be overridden when mocking the service
 }

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)
+}

utils/testutils/mocked_http_service.go

@@ -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()
-}