Merge branch 'master' into mark-as-unread

2019-09-30 16:39:24 -04:00
--- a/app/diagnostics.go
+++ b/app/diagnostics.go
@@ -75,7 +75,7 @@ func (a *App) sendDailyDiagnostics(override bool) {
 }

 func (a *App) SendDiagnostic(event string, properties map[string]interface{}) {
-	a.Srv.diagnosticClient.Enqueue(&analytics.Track{
+	a.Srv.diagnosticClient.Enqueue(analytics.Track{
 		Event:      event,
 		UserId:     a.DiagnosticId(),
 		Properties: properties,
--- a/app/diagnostics_test.go
+++ b/app/diagnostics_test.go
@@ -4,6 +4,7 @@
 package app

 import (
+	"encoding/json"
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
@@ -50,12 +51,34 @@ func TestDiagnostics(t *testing.T) {
 	th := Setup(t).InitBasic()
 	defer th.TearDown()

-	data := make(chan string, 100)
+	type payload struct {
+		MessageId string
+		SentAt    time.Time
+		Batch     []struct {
+			MessageId  string
+			UserId     string
+			Event      string
+			Timestamp  time.Time
+			Properties map[string]interface{}
+		}
+		Context struct {
+			Library struct {
+				Name    string
+				Version string
+			}
+		}
+	}
+
+	data := make(chan payload, 100)
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		body, err := ioutil.ReadAll(r.Body)
 		require.NoError(t, err)

-		data <- string(body)
+		var p payload
+		err = json.Unmarshal(body, &p)
+		require.NoError(t, err)
+
+		data <- p
 	}))
 	defer server.Close()

@@ -63,12 +86,30 @@ func TestDiagnostics(t *testing.T) {
 	th.App.SetDiagnosticId(diagnosticID)
 	th.Server.initDiagnostics(server.URL)

+	assertPayload := func(t *testing.T, actual payload, event string, properties map[string]interface{}) {
+		assert.NotEmpty(t, actual.MessageId)
+		assert.False(t, actual.SentAt.IsZero())
+		if assert.Len(t, actual.Batch, 1) {
+			assert.NotEmpty(t, actual.Batch[0].MessageId, "message id should not be empty")
+			assert.Equal(t, diagnosticID, actual.Batch[0].UserId)
+			if event != "" {
+				assert.Equal(t, event, actual.Batch[0].Event)
+			}
+			assert.False(t, actual.Batch[0].Timestamp.IsZero(), "batch timestamp should not be the zero value")
+			if properties != nil {
+				assert.Equal(t, properties, actual.Batch[0].Properties)
+			}
+		}
+		assert.Equal(t, "analytics-go", actual.Context.Library.Name)
+		assert.Equal(t, "3.0.0", actual.Context.Library.Version)
+	}
+
 	// Should send a client identify message
 	select {
 	case identifyMessage := <-data:
-		require.Contains(t, identifyMessage, diagnosticID)
+		assertPayload(t, identifyMessage, "", nil)
 	case <-time.After(time.Second * 1):
-		require.Fail(t,"Did not receive ID message")
+		require.Fail(t, "Did not receive ID message")
 	}

 	t.Run("Send", func(t *testing.T) {
@@ -78,30 +119,31 @@ func TestDiagnostics(t *testing.T) {
 		})
 		select {
 		case result := <-data:
-			require.Contains(t, result, testValue)
+			assertPayload(t, result, "Testing Diagnostic", map[string]interface{}{
+				"hey": testValue,
+			})
 		case <-time.After(time.Second * 1):
-			require.Fail(t,"Did not receive diagnostic")
+			require.Fail(t, "Did not receive diagnostic")
 		}
 	})

 	t.Run("SendDailyDiagnostics", func(t *testing.T) {
 		th.App.sendDailyDiagnostics(true)

-		var info string
+		var info []string
 		// Collect the info sent.
 	Loop:
 		for {
 			select {
 			case result := <-data:
-				info += result
+				assertPayload(t, result, "", nil)
+				info = append(info, result.Batch[0].Event)
 			case <-time.After(time.Second * 1):
 				break Loop
 			}
 		}

 		for _, item := range []string{
-			TRACK_CONFIG_SERVICE,
-			TRACK_CONFIG_TEAM,
 			TRACK_CONFIG_SERVICE,
 			TRACK_CONFIG_TEAM,
 			TRACK_CONFIG_SQL,
@@ -137,7 +179,7 @@ func TestDiagnostics(t *testing.T) {

 		select {
 		case <-data:
-			require.Fail(t,"Should not send diagnostics when the segment key is not set")
+			require.Fail(t, "Should not send diagnostics when the segment key is not set")
 		case <-time.After(time.Second * 1):
 			// Did not receive diagnostics
 		}
@@ -150,7 +192,7 @@ func TestDiagnostics(t *testing.T) {

 		select {
 		case <-data:
-			require.Fail(t,"Should not send diagnostics when they are disabled")
+			require.Fail(t, "Should not send diagnostics when they are disabled")
 		case <-time.After(time.Second * 1):
 			// Did not receive diagnostics
 		}
--- a/app/oauth.go
+++ b/app/oauth.go
@@ -67,7 +67,7 @@ func (a *App) DeleteOAuthApp(appId string) *model.AppError {
 	}

 	if err := a.InvalidateAllCaches(); err != nil {
-		mlog.Error(err.Error())
+		mlog.Error("error in invalidating cache", mlog.Err(err))
 	}

 	return nil
@@ -146,7 +146,7 @@ func (a *App) AllowOAuthAppAccessToUser(userId string, authRequest *model.Author
 	}

 	if err != nil {
-		mlog.Error(err.Error())
+		mlog.Error("error getting oauth redirect uri", mlog.Err(err))
 		return authRequest.RedirectUri + "?error=server_error&state=" + authRequest.State, nil
 	}

@@ -159,7 +159,7 @@ func (a *App) AllowOAuthAppAccessToUser(userId string, authRequest *model.Author
 	}

 	if err = a.Srv.Store.Preference().Save(&model.Preferences{authorizedApp}); err != nil {
-		mlog.Error(err.Error())
+		mlog.Error("error saving store prefrence", mlog.Err(err))
 		return authRequest.RedirectUri + "?error=server_error&state=" + authRequest.State, nil
 	}

@@ -189,7 +189,7 @@ func (a *App) GetOAuthAccessTokenForImplicitFlow(userId string, authRequest *mod
 	accessData := &model.AccessData{ClientId: authRequest.ClientId, UserId: user.Id, Token: session.Token, RefreshToken: "", RedirectUri: authRequest.RedirectUri, ExpiresAt: session.ExpiresAt, Scope: authRequest.Scope}

 	if _, err := a.Srv.Store.OAuth().SaveAccessData(accessData); err != nil {
-		mlog.Error(fmt.Sprint(err))
+		mlog.Error("error saving oauth access data in implicit flow", mlog.Err(err))
 		return nil, model.NewAppError("GetOAuthAccessToken", "api.oauth.get_access_token.internal_saving.app_error", nil, "", http.StatusInternalServerError)
 	}

@@ -267,7 +267,7 @@ func (a *App) GetOAuthAccessTokenForCodeFlow(clientId, grantType, redirectUri, c
 			accessData = &model.AccessData{ClientId: clientId, UserId: user.Id, Token: session.Token, RefreshToken: model.NewId(), RedirectUri: redirectUri, ExpiresAt: session.ExpiresAt, Scope: authData.Scope}

 			if _, err = a.Srv.Store.OAuth().SaveAccessData(accessData); err != nil {
-				mlog.Error(fmt.Sprint(err))
+				mlog.Error("error saving oauth access data in token for code flow", mlog.Err(err))
 				return nil, model.NewAppError("GetOAuthAccessToken", "api.oauth.get_access_token.internal_saving.app_error", nil, "", http.StatusInternalServerError)
 			}

@@ -324,7 +324,7 @@ func (a *App) newSession(appName string, user *model.User) (*model.Session, *mod
 func (a *App) newSessionUpdateToken(appName string, accessData *model.AccessData, user *model.User) (*model.AccessResponse, *model.AppError) {
 	// Remove the previous session
 	if err := a.Srv.Store.Session().Remove(accessData.Token); err != nil {
-		mlog.Error(fmt.Sprint(err))
+		mlog.Error("error removing access data token from session", mlog.Err(err))
 	}

 	session, err := a.newSession(appName, user)
@@ -337,7 +337,7 @@ func (a *App) newSessionUpdateToken(appName string, accessData *model.AccessData
 	accessData.ExpiresAt = session.ExpiresAt

 	if _, err := a.Srv.Store.OAuth().UpdateAccessData(accessData); err != nil {
-		mlog.Error(fmt.Sprint(err))
+		mlog.Error("error updating oauth access data", mlog.Err(err))
 		return nil, model.NewAppError("newSessionUpdateToken", "web.get_access_token.internal_saving.app_error", nil, "", http.StatusInternalServerError)
 	}
 	accessRsp := &model.AccessResponse{
@@ -583,7 +583,7 @@ func (a *App) CompleteSwitchWithOAuth(service string, userData io.Reader, email

 	a.Srv.Go(func() {
 		if err = a.SendSignInChangeEmail(user.Email, strings.Title(service)+" SSO", user.Locale, a.GetSiteURL()); err != nil {
-			mlog.Error(err.Error())
+			mlog.Error("error sending signin change email", mlog.Err(err))
 		}
 	})

@@ -711,7 +711,7 @@ func (a *App) AuthorizeOAuthUser(w http.ResponseWriter, r *http.Request, service

 	appErr = a.DeleteToken(expectedToken)
 	if appErr != nil {
-		mlog.Error(appErr.Error())
+		mlog.Error("error deleting token", mlog.Err(appErr))
 	}

 	subpath, _ := utils.GetSubpathFromConfig(a.Config())
@@ -786,7 +786,7 @@ func (a *App) AuthorizeOAuthUser(w http.ResponseWriter, r *http.Request, service
 		bodyBytes, _ := ioutil.ReadAll(resp.Body)
 		bodyString := string(bodyBytes)

-		mlog.Error("Error getting OAuth user: " + bodyString)
+		mlog.Error("Error getting OAuth user", mlog.String("body_string", bodyString))

 		if service == model.SERVICE_GITLAB && resp.StatusCode == http.StatusForbidden && strings.Contains(bodyString, "Terms of Service") {
 			// Return a nicer error when the user hasn't accepted GitLab's terms of service
@@ -852,7 +852,7 @@ func (a *App) SwitchOAuthToEmail(email, password, requesterId string) (string, *

 	a.Srv.Go(func() {
 		if err := a.SendSignInChangeEmail(user.Email, T("api.templates.signin_change_email.body.method_email"), user.Locale, a.GetSiteURL()); err != nil {
-			mlog.Error(err.Error())
+			mlog.Error("error sending signin change email", mlog.Err(err))
 		}
 	})

--- a/app/post_metadata_test.go
+++ b/app/post_metadata_test.go
@@ -58,12 +58,54 @@ func TestPreparePostListForClient(t *testing.T) {
 }

 func TestPreparePostForClient(t *testing.T) {
+	var serverURL string
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/":
+			w.Header().Set("Content-Type", "text/html")
+			w.Write([]byte(`
+			<html>
+			<head>
+			<meta property="og:image" content="` + serverURL + `/test-image3.png" />
+			<meta property="og:site_name" content="GitHub" />
+			<meta property="og:type" content="object" />
+			<meta property="og:title" content="hmhealey/test-files" />
+			<meta property="og:url" content="https://github.com/hmhealey/test-files" />
+			<meta property="og:description" content="Contribute to hmhealey/test-files development by creating an account on GitHub." />
+			</head>
+			</html>`))
+		case "/test-image1.png":
+			file, err := testutils.ReadTestFile("test.png")
+			require.Nil(t, err)
+
+			w.Header().Set("Content-Type", "image/png")
+			w.Write(file)
+		case "/test-image2.png":
+			file, err := testutils.ReadTestFile("test-data-graph.png")
+			require.Nil(t, err)
+
+			w.Header().Set("Content-Type", "image/png")
+			w.Write(file)
+		case "/test-image3.png":
+			file, err := testutils.ReadTestFile("qa-data-graph.png")
+			require.Nil(t, err)
+
+			w.Header().Set("Content-Type", "image/png")
+			w.Write(file)
+		default:
+			require.Fail(t, "Invalid path", r.URL.Path)
+		}
+	}))
+	serverURL = server.URL
+	defer server.Close()
+
 	setup := func() *TestHelper {
 		th := Setup(t).InitBasic()

 		th.App.UpdateConfig(func(cfg *model.Config) {
 			*cfg.ServiceSettings.EnableLinkPreviews = true
 			*cfg.ImageProxySettings.Enable = false
+			*cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost,127.0.0.1"
 		})

 		return th
@@ -289,7 +331,7 @@ func TestPreparePostForClient(t *testing.T) {
 		post, err := th.App.CreatePost(&model.Post{
 			UserId:    th.BasicUser.Id,
 			ChannelId: th.BasicChannel.Id,
-			Message:   "This is ![our logo](https://github.com/hmhealey/test-files/raw/master/logoVertical.png) and ![our icon](https://github.com/hmhealey/test-files/raw/master/icon.png)",
+			Message:   fmt.Sprintf("This is ![our logo](%s/test-image2.png) and ![our icon](%s/test-image1.png)", server.URL, server.URL),
 		}, th.BasicChannel, false)
 		require.Nil(t, err)

@@ -300,14 +342,14 @@ func TestPreparePostForClient(t *testing.T) {
 			require.Len(t, imageDimensions, 2)
 			assert.Equal(t, &model.PostImage{
 				Format: "png",
-				Width:  1068,
-				Height: 552,
-			}, imageDimensions["https://github.com/hmhealey/test-files/raw/master/logoVertical.png"])
+				Width:  1280,
+				Height: 1780,
+			}, imageDimensions[server.URL+"/test-image2.png"])
 			assert.Equal(t, &model.PostImage{
 				Format: "png",
-				Width:  501,
-				Height: 501,
-			}, imageDimensions["https://github.com/hmhealey/test-files/raw/master/icon.png"])
+				Width:  408,
+				Height: 336,
+			}, imageDimensions[server.URL+"/test-image1.png"])
 		})
 	})

@@ -332,8 +374,8 @@ func TestPreparePostForClient(t *testing.T) {
 		post, err := th.App.CreatePost(&model.Post{
 			UserId:    th.BasicUser.Id,
 			ChannelId: th.BasicChannel.Id,
-			Message: `This is our logo: https://github.com/hmhealey/test-files/raw/master/logoVertical.png
-	And this is our icon: https://github.com/hmhealey/test-files/raw/master/icon.png`,
+			Message: `This is our logo: ` + server.URL + `/test-image2.png
+	And this is our icon: ` + server.URL + `/test-image1.png`,
 		}, th.BasicChannel, false)
 		require.Nil(t, err)

@@ -345,7 +387,7 @@ func TestPreparePostForClient(t *testing.T) {
 			assert.ElementsMatch(t, []*model.PostEmbed{
 				{
 					Type: model.POST_EMBED_IMAGE,
-					URL:  "https://github.com/hmhealey/test-files/raw/master/logoVertical.png",
+					URL:  server.URL + "/test-image2.png",
 				},
 			}, clientPost.Metadata.Embeds)
 		})
@@ -355,9 +397,9 @@ func TestPreparePostForClient(t *testing.T) {
 			require.Len(t, imageDimensions, 1)
 			assert.Equal(t, &model.PostImage{
 				Format: "png",
-				Width:  1068,
-				Height: 552,
-			}, imageDimensions["https://github.com/hmhealey/test-files/raw/master/logoVertical.png"])
+				Width:  1280,
+				Height: 1780,
+			}, imageDimensions[server.URL+"/test-image2.png"])
 		})
 	})

@@ -368,7 +410,7 @@ func TestPreparePostForClient(t *testing.T) {
 		post, err := th.App.CreatePost(&model.Post{
 			UserId:    th.BasicUser.Id,
 			ChannelId: th.BasicChannel.Id,
-			Message:   `This is our web page: https://github.com/hmhealey/test-files`,
+			Message:   `This is our web page: ` + server.URL,
 		}, th.BasicChannel, false)
 		require.Nil(t, err)

@@ -378,13 +420,13 @@ func TestPreparePostForClient(t *testing.T) {

 		t.Run("populates embeds", func(t *testing.T) {
 			assert.Equal(t, firstEmbed.Type, model.POST_EMBED_OPENGRAPH)
-			assert.Equal(t, firstEmbed.URL, "https://github.com/hmhealey/test-files")
+			assert.Equal(t, firstEmbed.URL, server.URL)
 			assert.Equal(t, ogData.Description, "Contribute to hmhealey/test-files development by creating an account on GitHub.")
 			assert.Equal(t, ogData.SiteName, "GitHub")
 			assert.Equal(t, ogData.Title, "hmhealey/test-files")
 			assert.Equal(t, ogData.Type, "object")
-			assert.Equal(t, ogData.URL, "https://github.com/hmhealey/test-files")
-			assert.Equal(t, ogData.Images[0].URL, "https://avatars1.githubusercontent.com/u/3277310?s=400&v=4")
+			assert.Equal(t, ogData.URL, server.URL)
+			assert.Equal(t, ogData.Images[0].URL, server.URL+"/test-image3.png")
 		})

 		t.Run("populates image dimensions", func(t *testing.T) {
@@ -392,9 +434,9 @@ func TestPreparePostForClient(t *testing.T) {
 			require.Len(t, imageDimensions, 1)
 			assert.Equal(t, &model.PostImage{
 				Format: "png",
-				Width:  420,
-				Height: 420,
-			}, imageDimensions["https://avatars1.githubusercontent.com/u/3277310?s=400&v=4"])
+				Width:  1790,
+				Height: 1340,
+			}, imageDimensions[server.URL+"/test-image3.png"])
 		})
 	})

@@ -408,7 +450,7 @@ func TestPreparePostForClient(t *testing.T) {
 			Props: map[string]interface{}{
 				"attachments": []interface{}{
 					map[string]interface{}{
-						"text": "![icon](https://github.com/hmhealey/test-files/raw/master/icon.png)",
+						"text": "![icon](" + server.URL + "/test-image1.png)",
 					},
 				},
 			},
@@ -430,9 +472,9 @@ func TestPreparePostForClient(t *testing.T) {
 			require.Len(t, imageDimensions, 1)
 			assert.Equal(t, &model.PostImage{
 				Format: "png",
-				Width:  501,
-				Height: 501,
-			}, imageDimensions["https://github.com/hmhealey/test-files/raw/master/icon.png"])
+				Width:  408,
+				Height: 336,
+			}, imageDimensions[server.URL+"/test-image1.png"])
 		})
 	})
 }
@@ -444,6 +486,7 @@ func TestPreparePostForClientWithImageProxy(t *testing.T) {
 		th.App.UpdateConfig(func(cfg *model.Config) {
 			*cfg.ServiceSettings.EnableLinkPreviews = true
 			*cfg.ServiceSettings.SiteURL = "http://mymattermost.com"
+			*cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost,127.0.0.1"
 			*cfg.ImageProxySettings.Enable = true
 			*cfg.ImageProxySettings.ImageProxyType = "atmos/camo"
 			*cfg.ImageProxySettings.RemoteImageProxyURL = "https://127.0.0.1"
@@ -490,10 +533,39 @@ func testProxyLinkedImage(t *testing.T, th *TestHelper, shouldProxy bool) {
 }

 func testProxyOpenGraphImage(t *testing.T, th *TestHelper, shouldProxy bool) {
+	var serverURL string
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/":
+			w.Header().Set("Content-Type", "text/html")
+			w.Write([]byte(`
+			<html>
+			<head>
+			<meta property="og:image" content="` + serverURL + `/test-image3.png" />
+			<meta property="og:site_name" content="GitHub" />
+			<meta property="og:type" content="object" />
+			<meta property="og:title" content="hmhealey/test-files" />
+			<meta property="og:url" content="https://github.com/hmhealey/test-files" />
+			<meta property="og:description" content="Contribute to hmhealey/test-files development by creating an account on GitHub." />
+			</head>
+			</html>`))
+		case "/test-image3.png":
+			file, err := testutils.ReadTestFile("qa-data-graph.png")
+			require.Nil(t, err)
+
+			w.Header().Set("Content-Type", "image/png")
+			w.Write(file)
+		default:
+			require.Fail(t, "Invalid path", r.URL.Path)
+		}
+	}))
+	serverURL = server.URL
+	defer server.Close()
+
 	post, err := th.App.CreatePost(&model.Post{
 		UserId:    th.BasicUser.Id,
 		ChannelId: th.BasicChannel.Id,
-		Message:   `This is our web page: https://github.com/hmhealey/test-files`,
+		Message:   `This is our web page: ` + server.URL,
 	}, th.BasicChannel, false)
 	require.Nil(t, err)

@@ -502,10 +574,11 @@ func testProxyOpenGraphImage(t *testing.T, th *TestHelper, shouldProxy bool) {

 	embed := embeds[0]
 	assert.Equal(t, model.POST_EMBED_OPENGRAPH, embed.Type, "embed type should be OpenGraph")
-	assert.Equal(t, "https://github.com/hmhealey/test-files", embed.URL, "embed URL should be correct")
+	assert.Equal(t, server.URL, embed.URL, "embed URL should be correct")

 	og, ok := embed.Data.(*opengraph.OpenGraph)
-	assert.Equal(t, true, ok, "data should be non-nil OpenGraph data")
+	assert.True(t, ok, "data should be non-nil OpenGraph data")
+	assert.NotNil(t, og, "data should be non-nil OpenGraph data")
 	assert.Equal(t, "GitHub", og.SiteName, "OpenGraph data should be correctly populated")

 	require.Len(t, og.Images, 1, "OpenGraph data should have one image")
@@ -513,9 +586,9 @@ func testProxyOpenGraphImage(t *testing.T, th *TestHelper, shouldProxy bool) {
 	image := og.Images[0]
 	if shouldProxy {
 		assert.Equal(t, "", image.URL, "image URL should not be set with proxy")
-		assert.Equal(t, "http://mymattermost.com/api/v4/image?url=https%3A%2F%2Favatars1.githubusercontent.com%2Fu%2F3277310%3Fs%3D400%26v%3D4", image.SecureURL, "secure image URL should be sent through proxy")
+		assert.Equal(t, "http://mymattermost.com/api/v4/image?url="+url.QueryEscape(server.URL+"/test-image3.png"), image.SecureURL, "secure image URL should be sent through proxy")
 	} else {
-		assert.Equal(t, "https://avatars1.githubusercontent.com/u/3277310?s=400&v=4", image.URL, "image URL should be set")
+		assert.Equal(t, server.URL+"/test-image3.png", image.URL, "image URL should be set")
 		assert.Equal(t, "", image.SecureURL, "secure image URL should not be set")
 	}
 }
--- a/app/server.go
+++ b/app/server.go
@@ -760,7 +760,7 @@ func (s *Server) initDiagnostics(endpoint string) {
 			config.BatchSize = 1
 		}
 		client, _ := analytics.NewWithConfig(SEGMENT_KEY, config)
-		client.Enqueue(&analytics.Identify{
+		client.Enqueue(analytics.Identify{
 			UserId: s.diagnosticId,
 		})

--- a/app/team.go
+++ b/app/team.go
@@ -643,12 +643,7 @@ func (a *App) GetTeam(teamId string) (*model.Team, *model.AppError) {
 }

 func (a *App) GetTeamByName(name string) (*model.Team, *model.AppError) {
-	team, err := a.Srv.Store.Team().GetByName(name)
-	if err != nil {
-		err.StatusCode = http.StatusNotFound
-		return nil, err
-	}
-	return team, nil
+	return a.Srv.Store.Team().GetByName(name)
 }

 func (a *App) GetTeamByInviteId(inviteId string) (*model.Team, *model.AppError) {