From 9a363f559e6608d06e8fb1689d2ba4c7ff62180e Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Thu, 26 Sep 2019 07:17:39 +0200 Subject: [PATCH 01/16] [MM-18625] Make config.DatabaseStore.String() more robust (#12309) --- config/database.go | 15 +----------- config/utils.go | 21 +++++++++++++++++ config/utils_test.go | 55 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 14 deletions(-) diff --git a/config/database.go b/config/database.go index 4c61375007..e6f83c63cb 100644 --- a/config/database.go +++ b/config/database.go @@ -7,8 +7,6 @@ import ( "bytes" "database/sql" "io/ioutil" - "net/url" - "regexp" "strings" "github.com/jmoiron/sqlx" @@ -23,8 +21,6 @@ import ( _ "github.com/lib/pq" ) -var tcpStripper = regexp.MustCompile(`@tcp\((.*)\)`) - // DatabaseStore is a config store backed by a database. type DatabaseStore struct { commonStore @@ -295,16 +291,7 @@ func (ds *DatabaseStore) RemoveFile(name string) error { // String returns the path to the database backing the config, masking the password. func (ds *DatabaseStore) String() string { - // Remove @tcp and the parentheses from the host and parse the rest as a URL - u, err := url.Parse(tcpStripper.ReplaceAllString(ds.originalDsn, `@$1`)) - if err != nil { - return "(omitted due to error parsing the DSN)" - } - - // Strip out the password to avoid leaking in logs. - u.User = url.User(u.User.Username()) - - return u.String() + return stripPassword(ds.originalDsn, ds.driverName) } // Close cleans up resources associated with the store. diff --git a/config/utils.go b/config/utils.go index 457f3721e7..3e934d439b 100644 --- a/config/utils.go +++ b/config/utils.go @@ -140,3 +140,24 @@ func Merge(cfg *model.Config, patch *model.Config, mergeConfig *utils.MergeConfi retCfg := ret.(model.Config) return &retCfg, nil } + +// stripPassword remove the password from a given DSN +func stripPassword(dsn, schema string) string { + prefix := schema + "://" + dsn = strings.TrimPrefix(dsn, prefix) + + i := strings.Index(dsn, ":") + j := strings.LastIndex(dsn, "@") + + // Return error if no @ sign is found + if j < 0 { + return "(omitted due to error parsing the DSN)" + } + + // Return back the input if no password is found + if i < 0 || i > j { + return prefix + dsn + } + + return prefix + dsn[:i+1] + dsn[j:] +} diff --git a/config/utils_test.go b/config/utils_test.go index 4ea04267ae..8bfc2eb33b 100644 --- a/config/utils_test.go +++ b/config/utils_test.go @@ -142,6 +142,61 @@ func TestFixInvalidLocales(t *testing.T) { assert.Contains(t, *cfg.LocalizationSettings.AvailableLocales, *cfg.LocalizationSettings.DefaultClientLocale, "DefaultClientLocale should have been added to AvailableLocales") } +func TestStripPassword(t *testing.T) { + for name, test := range map[string]struct { + DSN string + Schema string + ExpectedOut string + }{ + "mysql": { + DSN: "mysql://mmuser:password@tcp(localhost:3306)/mattermost?charset=utf8mb4,utf8&readTimeout=30s", + Schema: "mysql", + ExpectedOut: "mysql://mmuser:@tcp(localhost:3306)/mattermost?charset=utf8mb4,utf8&readTimeout=30s", + }, + "mysql idempotent": { + DSN: "mysql://mmuser:@tcp(localhost:3306)/mattermost?charset=utf8mb4,utf8&readTimeout=30s", + Schema: "mysql", + ExpectedOut: "mysql://mmuser:@tcp(localhost:3306)/mattermost?charset=utf8mb4,utf8&readTimeout=30s", + }, + "mysql: password with : and @": { + DSN: "mysql://mmuser:p:assw@ord@tcp(localhost:3306)/mattermost?charset=utf8mb4,utf8&readTimeout=30s", + Schema: "mysql", + ExpectedOut: "mysql://mmuser:@tcp(localhost:3306)/mattermost?charset=utf8mb4,utf8&readTimeout=30s", + }, + "mysql: password with @ and :": { + DSN: "mysql://mmuser:pa@sswo:rd@tcp(localhost:3306)/mattermost?charset=utf8mb4,utf8&readTimeout=30s", + Schema: "mysql", + ExpectedOut: "mysql://mmuser:@tcp(localhost:3306)/mattermost?charset=utf8mb4,utf8&readTimeout=30s", + }, + "postgres": { + DSN: "postgres://mmuser:password@localhost:5432/mattermost?sslmode=disable&connect_timeout=10", + Schema: "postgres", + ExpectedOut: "postgres://mmuser:@localhost:5432/mattermost?sslmode=disable&connect_timeout=10", + }, + "pipe": { + DSN: "mysql://user@unix(/path/to/socket)/dbname", + Schema: "mysql", + ExpectedOut: "mysql://user@unix(/path/to/socket)/dbname", + }, + "malformed without :": { + DSN: "postgres://mmuserpassword@localhost:5432/mattermost?sslmode=disable&connect_timeout=10", + Schema: "postgres", + ExpectedOut: "postgres://mmuserpassword@localhost:5432/mattermost?sslmode=disable&connect_timeout=10", + }, + "malformed without @": { + DSN: "postgres://mmuser:passwordlocalhost:5432/mattermost?sslmode=disable&connect_timeout=10", + Schema: "postgres", + ExpectedOut: "(omitted due to error parsing the DSN)", + }, + } { + t.Run(name, func(t *testing.T) { + out := stripPassword(test.DSN, test.Schema) + + assert.Equal(t, test.ExpectedOut, out) + }) + } +} + func sToP(s string) *string { return &s } From effb6d80032245e3948132223622c7a3d5662c97 Mon Sep 17 00:00:00 2001 From: Micah Thompson Date: Thu, 26 Sep 2019 01:19:52 -0400 Subject: [PATCH 02/16] =?UTF-8?q?MM-18253=20Refactor=20"manualtesting/manu?= =?UTF-8?q?al=5Ftesting.go"=20to=20use=20str=E2=80=A6=20(#12363)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- manualtesting/manual_testing.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/manualtesting/manual_testing.go b/manualtesting/manual_testing.go index ae8e65637c..7dbd7aac91 100644 --- a/manualtesting/manual_testing.go +++ b/manualtesting/manual_testing.go @@ -4,7 +4,6 @@ package manualtesting import ( - "fmt" "hash/fnv" "math/rand" "net/http" @@ -162,6 +161,6 @@ func getChannelID(a *app.App, channelname string, teamid string, userid string) return channel.Id, true } } - mlog.Debug(fmt.Sprintf("Could not find channel: %v, %v possibilities searched", channelname, strconv.Itoa(len(*channels)))) + mlog.Debug("Could not find channel", mlog.String("Channel name", channelname), mlog.Int("Possibilities searched", len(*channels))) return "", false } From af3ffeed1a4acc3ee1c790a87d2330c95c0e88ee Mon Sep 17 00:00:00 2001 From: Pavel Biryukov Date: Thu, 26 Sep 2019 14:26:48 +0300 Subject: [PATCH 03/16] Fix wrong error check (#12310) --- store/sqlstore/audit_store.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/store/sqlstore/audit_store.go b/store/sqlstore/audit_store.go index 258b59e26d..98758b8907 100644 --- a/store/sqlstore/audit_store.go +++ b/store/sqlstore/audit_store.go @@ -87,7 +87,7 @@ func (s SqlAuditStore) PermanentDeleteBatch(endTime int64, limit int64) (int64, rowsAffected, err1 := sqlResult.RowsAffected() if err1 != nil { - return 0, model.NewAppError("SqlAuditStore.PermanentDeleteBatch", "store.sql_audit.permanent_delete_batch.app_error", nil, ""+err.Error(), http.StatusInternalServerError) + return 0, model.NewAppError("SqlAuditStore.PermanentDeleteBatch", "store.sql_audit.permanent_delete_batch.app_error", nil, ""+err1.Error(), http.StatusInternalServerError) } return rowsAffected, nil } From 1a4d7869cbdea2f7e6645ff2ba5cd6a92eba7ffb Mon Sep 17 00:00:00 2001 From: Michael Kochell Date: Thu, 26 Sep 2019 11:54:51 -0600 Subject: [PATCH 04/16] [MM-18628] Fix flaky OpenGraph test (#12365) * use local httptest server instead of github.com * use switch statements --- app/post_metadata_test.go | 131 +++++++++++++++++++++++++++++--------- 1 file changed, 102 insertions(+), 29 deletions(-) diff --git a/app/post_metadata_test.go b/app/post_metadata_test.go index a2dcd7efc3..240f18df1d 100644 --- 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(` + + + + + + + + + + `)) + 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(` + + + + + + + + + + `)) + 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") } } From d82584a7839f3190e842da9e9e939c17942f6cb5 Mon Sep 17 00:00:00 2001 From: Martin Kraft Date: Thu, 26 Sep 2019 14:11:55 -0400 Subject: [PATCH 05/16] MM-18668: Fix for scan error selecting null SchemeGuest columns. (#12370) --- store/sqlstore/group_store.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/store/sqlstore/group_store.go b/store/sqlstore/group_store.go index 84f2139845..0895ef0ae9 100644 --- a/store/sqlstore/group_store.go +++ b/store/sqlstore/group_store.go @@ -977,7 +977,7 @@ func (s *SqlGroupStore) teamMembersMinusGroupMembersQuery(teamID string, groupID if isCount { selectStr = "count(DISTINCT Users.Id)" } else { - tmpl := "Users.*, TeamMembers.SchemeGuest, TeamMembers.SchemeAdmin, TeamMembers.SchemeUser, %s AS GroupIDs" + tmpl := "Users.*, coalesce(TeamMembers.SchemeGuest, false), TeamMembers.SchemeAdmin, TeamMembers.SchemeUser, %s AS GroupIDs" if s.DriverName() == model.DATABASE_DRIVER_MYSQL { selectStr = fmt.Sprintf(tmpl, "group_concat(UserGroups.Id)") } else { @@ -1055,7 +1055,7 @@ func (s *SqlGroupStore) channelMembersMinusGroupMembersQuery(channelID string, g if isCount { selectStr = "count(DISTINCT Users.Id)" } else { - tmpl := "Users.*, ChannelMembers.SchemeGuest, ChannelMembers.SchemeAdmin, ChannelMembers.SchemeUser, %s AS GroupIDs" + tmpl := "Users.*, coalesce(ChannelMembers.SchemeGuest, false), ChannelMembers.SchemeAdmin, ChannelMembers.SchemeUser, %s AS GroupIDs" if s.DriverName() == model.DATABASE_DRIVER_MYSQL { selectStr = fmt.Sprintf(tmpl, "group_concat(UserGroups.Id)") } else { From 993947c70a9f230b4dd7752c9e89e515f33b258f Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Thu, 26 Sep 2019 23:47:13 -0300 Subject: [PATCH 06/16] MM-18741: clarify error message (#12357) Clarify the error message that is emitted by the server when failing to parse the configuration. Fixes: MM-18741 --- cmd/mattermost/commands/server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/mattermost/commands/server.go b/cmd/mattermost/commands/server.go index b705671f4a..658d6a0ad4 100644 --- a/cmd/mattermost/commands/server.go +++ b/cmd/mattermost/commands/server.go @@ -47,7 +47,7 @@ func serverCmdF(command *cobra.Command, args []string) error { } configStore, err := config.NewStore(configDSN, !disableConfigWatch) if err != nil { - return err + return errors.Wrap(err, "failed to load configuration") } return runServer(configStore, disableConfigWatch, usedPlatform, interruptChan) From 74533371b291fcd59128cc9e5115d0144387aaff Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Thu, 26 Sep 2019 23:49:43 -0300 Subject: [PATCH 07/16] MM-18115: fix segment v3 usage (#12317) * diagnostics_test.go: fix spacing * diagnostics_test.go: explicitly assert payload This fails, since the package is currently receiving struct pointers and won't set the MessageId or Timestamp on the corresponding Message. * MM-18115: fix segment v3 usage In v5.14, we updated [github.com/segmentio/analytics-go](https://github.com/segmentio/analytics-go) to v3 as part of https://mattermost.atlassian.net/browse/MM-12389. As noted in the [migration guide](https://segment.com/docs/sources/server/go/#migrating-from-v2), the API subtly changed to expect a struct value and not a struct pointer: ```go // in v2, you would call the `Track` method with a `Track` struct. client.Track(&track) // in v3, you would call the `Enqueue` method with a `Track` struct. // Note that a pointer is not used here. client.Enqueue(track) ``` Unfortunately, we kept passing a pointer, and the package didn't complain since it only required an interface -- which the pointer to these structs still implemented. Internally, it only checked for the value types, and failed to annotate our payloads with the requisite metadata. Upstream, segment.io accepted the payload, but then discarded it silently. This has since been reported and fixed in https://github.com/segmentio/analytics-go/pull/146, but isn't yet part of a tagged release of the package. Fix our code to pass struct values instead. Fixes: MM-18115 --- app/diagnostics.go | 2 +- app/diagnostics_test.go | 66 +++++++++++++++++++++++++++++++++-------- app/server.go | 2 +- 3 files changed, 56 insertions(+), 14 deletions(-) diff --git a/app/diagnostics.go b/app/diagnostics.go index d5541b5814..758b25ace4 100644 --- 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, diff --git a/app/diagnostics_test.go b/app/diagnostics_test.go index 0d70beef0e..8d9b51da1c 100644 --- 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 } diff --git a/app/server.go b/app/server.go index 7a832530ed..d8568011f7 100644 --- 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, }) From 975055a7e77c5d7104109f1dfc6a4eefff84bfa6 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Thu, 26 Sep 2019 23:59:44 -0300 Subject: [PATCH 08/16] MM-18894: Update production marketplace URL (#12378) --- model/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/model/config.go b/model/config.go index 378933abc7..3e2a2ab9f5 100644 --- a/model/config.go +++ b/model/config.go @@ -174,7 +174,7 @@ const ( PLUGIN_SETTINGS_DEFAULT_DIRECTORY = "./plugins" PLUGIN_SETTINGS_DEFAULT_CLIENT_DIRECTORY = "./client/plugins" PLUGIN_SETTINGS_DEFAULT_ENABLE_MARKETPLACE = true - PLUGIN_SETTINGS_DEFAULT_MARKETPLACE_URL = "https://marketplace.integrations.mattermost.com" + PLUGIN_SETTINGS_DEFAULT_MARKETPLACE_URL = "https://api.integrations.mattermost.com" COMPLIANCE_EXPORT_TYPE_CSV = "csv" COMPLIANCE_EXPORT_TYPE_ACTIANCE = "actiance" From 340287890a78b69f1659aa161d715636dec19b1f Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Fri, 27 Sep 2019 09:10:38 -0300 Subject: [PATCH 09/16] MM-18636: limit configuration writes to 4Mb (#12266) * MM-18636: limit configuration writes to 4Mb By default, MySQL silently truncates writes that exceed the column type in question. Change the column type from `TEXT` to `MEDIUMTEXT` to allow writes to the `Configurations` and `ConfigurationFiles` table to exceed 65535 bytes. This is a backwards compatible migration, but does require a rewrite of the table. However, MySQL is further constrained by the default `max_allowed_packet` value of 4Mb, so limit writes accordingly. Fixes: https://mattermost.atlassian.net/browse/MM-18636 * simplify unit tests * fix import --- config/database.go | 44 +++++++++++++++++++++++++++++++++++++++++ config/database_test.go | 39 ++++++++++++++++++++++++++++++++---- 2 files changed, 79 insertions(+), 4 deletions(-) diff --git a/config/database.go b/config/database.go index e6f83c63cb..b6bc765941 100644 --- a/config/database.go +++ b/config/database.go @@ -7,6 +7,7 @@ import ( "bytes" "database/sql" "io/ioutil" + "regexp" "strings" "github.com/jmoiron/sqlx" @@ -21,6 +22,14 @@ import ( _ "github.com/lib/pq" ) +// MaxWriteLength defines the maximum length accepted for write to the Configurations or +// ConfigurationFiles table. +// +// It is imposed by MySQL's default max_allowed_packet value of 4Mb. +const MaxWriteLength = 4 * 1024 * 1024 + +var tcpStripper = regexp.MustCompile(`@tcp\((.*)\)`) + // DatabaseStore is a config store backed by a database. type DatabaseStore struct { commonStore @@ -61,6 +70,8 @@ func NewDatabaseStore(dsn string) (ds *DatabaseStore, err error) { } // initializeConfigurationsTable ensures the requisite tables in place to form the backing store. +// +// Uses MEDIUMTEXT on MySQL, and TEXT on sane databases. func initializeConfigurationsTable(db *sqlx.DB) error { _, err := db.Exec(` CREATE TABLE IF NOT EXISTS Configurations ( @@ -86,6 +97,20 @@ func initializeConfigurationsTable(db *sqlx.DB) error { return errors.Wrap(err, "failed to create ConfigurationFiles table") } + // Change from TEXT (65535 limit) to MEDIUM TEXT (16777215) on MySQL. This is a + // backwards-compatible migration for any existing schema. + if db.DriverName() == "mysql" { + _, err = db.Exec(`ALTER TABLE Configurations MODIFY Value MEDIUMTEXT`) + if err != nil { + return errors.Wrap(err, "failed to alter Configurations table") + } + + _, err = db.Exec(`ALTER TABLE ConfigurationFiles MODIFY Data MEDIUMTEXT`) + if err != nil { + return errors.Wrap(err, "failed to alter ConfigurationFiles table") + } + } + return nil } @@ -126,6 +151,15 @@ func (ds *DatabaseStore) Set(newCfg *model.Config) (*model.Config, error) { return ds.commonStore.set(newCfg, true, ds.commonStore.validate, ds.persist) } +// maxLength identifies the maximum length of a configuration or configuration file +func (ds *DatabaseStore) checkLength(length int) error { + if ds.db.DriverName() == "mysql" && length > MaxWriteLength { + return errors.Errorf("value is too long: %d > %d bytes", length, MaxWriteLength) + } + + return nil +} + // persist writes the configuration to the configured database. func (ds *DatabaseStore) persist(cfg *model.Config) error { b, err := marshalConfig(cfg) @@ -137,6 +171,11 @@ func (ds *DatabaseStore) persist(cfg *model.Config) error { value := string(b) createAt := model.GetMillis() + err = ds.checkLength(len(value)) + if err != nil { + return errors.Wrap(err, "marshalled configuration failed length check") + } + tx, err := ds.db.Beginx() if err != nil { return errors.Wrap(err, "failed to begin transaction") @@ -232,6 +271,11 @@ func (ds *DatabaseStore) GetFile(name string) ([]byte, error) { // SetFile sets or replaces the contents of a configuration file. func (ds *DatabaseStore) SetFile(name string, data []byte) error { + err := ds.checkLength(len(data)) + if err != nil { + return errors.Wrap(err, "file data failed length check") + } + params := map[string]interface{}{ "name": name, "data": data, diff --git a/config/database_test.go b/config/database_test.go index 6119fc2305..b63390fd6b 100644 --- a/config/database_test.go +++ b/config/database_test.go @@ -451,7 +451,6 @@ func TestDatabaseStoreSet(t *testing.T) { }) t.Run("persist failed", func(t *testing.T) { - t.Skip("skipping persistence test inside Set") _, tearDown := setupConfigDatabase(t, emptyConfig, nil) defer tearDown() @@ -466,13 +465,29 @@ func TestDatabaseStoreSet(t *testing.T) { newCfg := &model.Config{} _, err = ds.Set(newCfg) - if assert.Error(t, err) { - assert.True(t, strings.HasPrefix(err.Error(), "failed to persist: failed to write to database")) - } + require.Error(t, err) + assert.True(t, strings.HasPrefix(err.Error(), "failed to persist: failed to query active configuration"), "unexpected error: "+err.Error()) assert.Equal(t, "", *ds.Get().ServiceSettings.SiteURL) }) + t.Run("persist failed: too long", func(t *testing.T) { + _, tearDown := setupConfigDatabase(t, emptyConfig, nil) + defer tearDown() + + ds, err := config.NewDatabaseStore(fmt.Sprintf("%s://%s", *sqlSettings.DriverName, *sqlSettings.DataSource)) + require.NoError(t, err) + defer ds.Close() + + longSiteURL := fmt.Sprintf("http://%s", strings.Repeat("a", config.MaxWriteLength)) + newCfg := emptyConfig.Clone() + newCfg.ServiceSettings.SiteURL = sToP(longSiteURL) + + _, err = ds.Set(newCfg) + require.Error(t, err) + assert.True(t, strings.HasPrefix(err.Error(), "failed to persist: marshalled configuration failed length check: value is too long"), "unexpected error: "+err.Error()) + }) + t.Run("listeners notified", func(t *testing.T) { activeId, tearDown := setupConfigDatabase(t, emptyConfig, nil) defer tearDown() @@ -809,6 +824,22 @@ func TestDatabaseSetFile(t *testing.T) { require.NoError(t, err) require.Equal(t, []byte("overwritten file"), data) }) + + t.Run("max length", func(t *testing.T) { + longFile := bytes.Repeat([]byte{0x0}, config.MaxWriteLength) + + err := ds.SetFile("toolong", longFile) + require.NoError(t, err) + }) + + t.Run("too long", func(t *testing.T) { + longFile := bytes.Repeat([]byte{0x0}, config.MaxWriteLength+1) + + err := ds.SetFile("toolong", longFile) + if assert.Error(t, err) { + assert.True(t, strings.HasPrefix(err.Error(), "file data failed length check: value is too long")) + } + }) } func TestDatabaseHasFile(t *testing.T) { From de8e798052397c3faf477f4fc8c40993b90e4368 Mon Sep 17 00:00:00 2001 From: Claudio Costa Date: Fri, 27 Sep 2019 17:40:16 +0200 Subject: [PATCH 10/16] [MM-18638] Send mlog console output to stderr (#12366) * Send server logs to stderr * Update comment --- mlog/default.go | 7 ++++--- mlog/log.go | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/mlog/default.go b/mlog/default.go index 366d22f88a..b30ca16a20 100644 --- a/mlog/default.go +++ b/mlog/default.go @@ -6,9 +6,10 @@ package mlog import ( "encoding/json" "fmt" + "os" ) -// defaultLog manually encodes the log to STDOUT, providing a basic, default logging implementation +// defaultLog manually encodes the log to STDERR, providing a basic, default logging implementation // before mlog is fully configured. func defaultLog(level, msg string, fields ...Field) { log := struct { @@ -22,9 +23,9 @@ func defaultLog(level, msg string, fields ...Field) { } if b, err := json.Marshal(log); err != nil { - fmt.Printf(`{"level":"error","msg":"failed to encode log message"}%s`, "\n") + fmt.Fprintf(os.Stderr, `{"level":"error","msg":"failed to encode log message"}%s`, "\n") } else { - fmt.Printf("%s\n", b) + fmt.Fprintf(os.Stderr, "%s\n", b) } } diff --git a/mlog/log.go b/mlog/log.go index 07d35a32da..59bc91df2d 100644 --- a/mlog/log.go +++ b/mlog/log.go @@ -86,7 +86,7 @@ func NewLogger(config *LoggerConfiguration) *Logger { } if config.EnableConsole { - writer := zapcore.Lock(os.Stdout) + writer := zapcore.Lock(os.Stderr) core := zapcore.NewCore(makeEncoder(config.ConsoleJson), writer, logger.consoleLevel) cores = append(cores, core) } From cee19b0332d66851cd7d9a16ea9984d8275a747d Mon Sep 17 00:00:00 2001 From: Scott Bishel Date: Fri, 27 Sep 2019 12:13:31 -0600 Subject: [PATCH 11/16] MM-18013 Allow configuration of SAML crypto hashing algorithms (#12362) * MM-18013 Add SAML Algorithms to config. * set defaults to current values, add validation for settings * update to use simplier config entry --- i18n/en.json | 12 ++++++ model/config.go | 40 +++++++++++++++++ model/config_test.go | 100 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 152 insertions(+) diff --git a/i18n/en.json b/i18n/en.json index 87fc85f360..3005763f68 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -4706,6 +4706,14 @@ "id": "model.config.is_valid.saml_assertion_consumer_service_url.app_error", "translation": "Service Provider Login URL must be a valid URL and start with http:// or https://." }, + { + "id": "model.config.is_valid.saml_canonical_algorithm.app_error", + "translation": "Invalid Canonical Algorithm." + }, + { + "id": "model.config.is_valid.saml_digest_algorithm.app_error", + "translation": "Invalid Digest Algorithm." + }, { "id": "model.config.is_valid.saml_email_attribute.app_error", "translation": "Invalid Email attribute. Must be set." @@ -4730,6 +4738,10 @@ "id": "model.config.is_valid.saml_public_cert.app_error", "translation": "Service Provider Public Certificate missing. Did you forget to upload it?" }, + { + "id": "model.config.is_valid.saml_signature_algorithm.app_error", + "translation": "Invalid Signature Algorithm." + }, { "id": "model.config.is_valid.saml_username_attribute.app_error", "translation": "Invalid Username attribute. Must be set." diff --git a/model/config.go b/model/config.go index 3e2a2ab9f5..acbd3d6f60 100644 --- a/model/config.go +++ b/model/config.go @@ -138,6 +138,20 @@ const ( SAML_SETTINGS_DEFAULT_LOCALE_ATTRIBUTE = "" SAML_SETTINGS_DEFAULT_POSITION_ATTRIBUTE = "" + SAML_SETTINGS_SIGNATURE_ALGORITHM_SHA1 = "RSAwithSHA1" + SAML_SETTINGS_SIGNATURE_ALGORITHM_SHA256 = "RSAwithSHA256" + SAML_SETTINGS_SIGNATURE_ALGORITHM_SHA384 = "RSAwithSHA384" + SAML_SETTINGS_SIGNATURE_ALGORITHM_SHA512 = "RSAwithSHA512" + SAML_SETTINGS_DEFAULT_SIGNATURE_ALGORITHM = SAML_SETTINGS_SIGNATURE_ALGORITHM_SHA1 + + SAML_SETTINGS_DIGEST_ALGORITHM_SHA1 = "SHA1" + SAML_SETTINGS_DIGEST_ALGORITHM_SHA256 = "SHA256" + SAML_SETTINGS_DEFAULT_DIGEST_ALGORITHM = SAML_SETTINGS_DIGEST_ALGORITHM_SHA1 + + SAML_SETTINGS_CANONICAL_ALGORITHM_C14N = "Canonical1.0" + SAML_SETTINGS_CANONICAL_ALGORITHM_C14N11 = "Canonical1.1" + SAML_SETTINGS_DEFAULT_CANONICAL_ALGORITHM = SAML_SETTINGS_CANONICAL_ALGORITHM_C14N + NATIVEAPP_SETTINGS_DEFAULT_APP_DOWNLOAD_LINK = "https://mattermost.com/download/#mattermostApps" NATIVEAPP_SETTINGS_DEFAULT_ANDROID_APP_DOWNLOAD_LINK = "https://about.mattermost.com/mattermost-android-app/" NATIVEAPP_SETTINGS_DEFAULT_IOS_APP_DOWNLOAD_LINK = "https://about.mattermost.com/mattermost-ios-app/" @@ -1885,6 +1899,10 @@ type SamlSettings struct { IdpDescriptorUrl *string AssertionConsumerServiceURL *string + SignatureAlgorithm *string + DigestAlgorithm *string + CanonicalAlgorithm *string + ScopingIDPProviderId *string ScopingIDPName *string @@ -1934,6 +1952,18 @@ func (s *SamlSettings) SetDefaults() { s.SignRequest = NewBool(false) } + if s.SignatureAlgorithm == nil { + s.SignatureAlgorithm = NewString(SAML_SETTINGS_DEFAULT_SIGNATURE_ALGORITHM) + } + + if s.DigestAlgorithm == nil { + s.DigestAlgorithm = NewString(SAML_SETTINGS_DEFAULT_DIGEST_ALGORITHM) + } + + if s.CanonicalAlgorithm == nil { + s.CanonicalAlgorithm = NewString(SAML_SETTINGS_DEFAULT_CANONICAL_ALGORITHM) + } + if s.IdpUrl == nil { s.IdpUrl = NewString("") } @@ -2800,6 +2830,16 @@ func (ss *SamlSettings) isValid() *AppError { if len(*ss.EmailAttribute) == 0 { return NewAppError("Config.IsValid", "model.config.is_valid.saml_email_attribute.app_error", nil, "", http.StatusBadRequest) } + + if !(*ss.SignatureAlgorithm == SAML_SETTINGS_SIGNATURE_ALGORITHM_SHA1 || *ss.SignatureAlgorithm == SAML_SETTINGS_SIGNATURE_ALGORITHM_SHA256 || *ss.SignatureAlgorithm == SAML_SETTINGS_SIGNATURE_ALGORITHM_SHA384 || *ss.SignatureAlgorithm == SAML_SETTINGS_SIGNATURE_ALGORITHM_SHA512) { + return NewAppError("Config.IsValid", "model.config.is_valid.saml_signature_algorithm.app_error", nil, "", http.StatusBadRequest) + } + if !(*ss.DigestAlgorithm == SAML_SETTINGS_DIGEST_ALGORITHM_SHA1 || *ss.DigestAlgorithm == SAML_SETTINGS_DIGEST_ALGORITHM_SHA256) { + return NewAppError("Config.IsValid", "model.config.is_valid.saml_digest_algorithm.app_error", nil, "", http.StatusBadRequest) + } + if !(*ss.CanonicalAlgorithm == SAML_SETTINGS_CANONICAL_ALGORITHM_C14N || *ss.CanonicalAlgorithm == SAML_SETTINGS_CANONICAL_ALGORITHM_C14N11) { + return NewAppError("Config.IsValid", "model.config.is_valid.saml_canonical_algorithm.app_error", nil, "", http.StatusBadRequest) + } } return nil diff --git a/model/config_test.go b/model/config_test.go index ec6e78de29..a3ed8bc7ff 100644 --- a/model/config_test.go +++ b/model/config_test.go @@ -95,6 +95,106 @@ func TestConfigDefaultFileSettingsS3SSE(t *testing.T) { } } +func TestConfigDefaultSignatureAlgorithm(t *testing.T) { + c1 := Config{} + c1.SetDefaults() + + if *c1.SamlSettings.SignatureAlgorithm != SAML_SETTINGS_DEFAULT_SIGNATURE_ALGORITHM { + t.Fatal("SamlSettings.SignatureAlgorithm default not set") + } + + if *c1.SamlSettings.DigestAlgorithm != SAML_SETTINGS_DEFAULT_DIGEST_ALGORITHM { + t.Fatal("SamlSettings.DigestAlgorithm default not set") + } + if *c1.SamlSettings.CanonicalAlgorithm != SAML_SETTINGS_DEFAULT_CANONICAL_ALGORITHM { + t.Fatal("SamlSettings.CanonicalAlgorithm default not set") + } +} + +func TestConfigOverwriteSignatureAlgorithm(t *testing.T) { + const testAlgorithm = "FakeAlgorithm" + c1 := Config{ + SamlSettings: SamlSettings{ + CanonicalAlgorithm: NewString(testAlgorithm), + SignatureAlgorithm: NewString(testAlgorithm), + DigestAlgorithm: NewString(testAlgorithm), + }, + } + + c1.SetDefaults() + + if *c1.SamlSettings.SignatureAlgorithm != testAlgorithm { + t.Fatal("SamlSettings.SignatureAlgorithm should be overwritten") + } + if *c1.SamlSettings.DigestAlgorithm != testAlgorithm { + t.Fatal("SamlSettings.DigestAlgorithm should be overwritten") + } + if *c1.SamlSettings.CanonicalAlgorithm != testAlgorithm { + t.Fatal("SamlSettings.CanonicalAlgorithm should be overwritten") + } +} + +func TestConfigIsValidDefaultAlgorithms(t *testing.T) { + c1 := Config{} + c1.SetDefaults() + + *c1.SamlSettings.Enable = true + *c1.SamlSettings.Verify = false + *c1.SamlSettings.Encrypt = false + + *c1.SamlSettings.IdpUrl = "http://test.url.com" + *c1.SamlSettings.IdpDescriptorUrl = "http://test.url.com" + *c1.SamlSettings.IdpCertificateFile = "certificatefile" + *c1.SamlSettings.EmailAttribute = "Email" + *c1.SamlSettings.UsernameAttribute = "Username" + + err := c1.SamlSettings.isValid() + if err != nil { + t.Fatal("SAMLSettings validation should pass with default settings") + } +} + +func TestConfigIsValidFakeAlgorithm(t *testing.T) { + c1 := Config{} + c1.SetDefaults() + + *c1.SamlSettings.Enable = true + *c1.SamlSettings.Verify = false + *c1.SamlSettings.Encrypt = false + + *c1.SamlSettings.IdpUrl = "http://test.url.com" + *c1.SamlSettings.IdpDescriptorUrl = "http://test.url.com" + *c1.SamlSettings.IdpCertificateFile = "certificatefile" + *c1.SamlSettings.EmailAttribute = "Email" + *c1.SamlSettings.UsernameAttribute = "Username" + + temp := *c1.SamlSettings.CanonicalAlgorithm + *c1.SamlSettings.CanonicalAlgorithm = "Fake Algorithm" + err := c1.SamlSettings.isValid() + if err == nil { + t.Fatal("SAMLSettings validation should fail with fake Canonical Algorithm") + } + require.Equal(t, "model.config.is_valid.saml_canonical_algorithm.app_error", err.Message) + *c1.SamlSettings.CanonicalAlgorithm = temp + + temp = *c1.SamlSettings.DigestAlgorithm + *c1.SamlSettings.DigestAlgorithm = "Fake Algorithm" + err = c1.SamlSettings.isValid() + if err == nil { + t.Fatal("SAMLSettings validation should pass fake digest Algorithm") + } + require.Equal(t, "model.config.is_valid.saml_digest_algorithm.app_error", err.Message) + *c1.SamlSettings.DigestAlgorithm = temp + + temp = *c1.SamlSettings.SignatureAlgorithm + *c1.SamlSettings.SignatureAlgorithm = "Fake Algorithm" + err = c1.SamlSettings.isValid() + if err == nil { + t.Fatal("SAMLSettings validation should pass with fake signature settings") + } + require.Equal(t, "model.config.is_valid.saml_signature_algorithm.app_error", err.Message) +} + func TestConfigDefaultServiceSettingsExperimentalGroupUnreadChannels(t *testing.T) { c1 := Config{} c1.SetDefaults() From e5ba0a0a1808660d4885ef04888da437c2a5b6a0 Mon Sep 17 00:00:00 2001 From: Nikhil Ranjan Date: Sun, 29 Sep 2019 12:42:53 +0200 Subject: [PATCH 12/16] Converting to structured logging the file app/oauth.go (#12135) --- app/oauth.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/app/oauth.go b/app/oauth.go index 0cfba889ff..1b4a73ce34 100644 --- 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)) } }) From 841099194d3f0bfe51a3114afae99a23f16dbe83 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Mon, 30 Sep 2019 14:23:00 -0300 Subject: [PATCH 13/16] MM-18636: fix wrong merge conflict (#12386) I resolved a merge conflict incorrectly when submitting MM-18636, and want to remove the unused code I left behind. --- config/database.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/config/database.go b/config/database.go index b6bc765941..c80cf8ef55 100644 --- a/config/database.go +++ b/config/database.go @@ -7,7 +7,6 @@ import ( "bytes" "database/sql" "io/ioutil" - "regexp" "strings" "github.com/jmoiron/sqlx" @@ -28,8 +27,6 @@ import ( // It is imposed by MySQL's default max_allowed_packet value of 4Mb. const MaxWriteLength = 4 * 1024 * 1024 -var tcpStripper = regexp.MustCompile(`@tcp\((.*)\)`) - // DatabaseStore is a config store backed by a database. type DatabaseStore struct { commonStore From beef13ae46daa44940b930377eac35efac55a73f Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Mon, 30 Sep 2019 15:44:56 -0300 Subject: [PATCH 14/16] MM-18721: disable SIGPIPE handling (#12394) We recently added support for handling SIGPIPE cleanly, in order to safely shutdown plugins in the event that the STDOUT/STDERR had been closed unexpectedly. Unfortunately, this signal is also emitted when writing to a closed socket connection: an event that occurs frequently for a webserver. Normally, if no signal handler is registered for same, the Go subsystem distinguishes the file descriptor and ignores those from the network. But once a handler is registered, all SIGPIPEs are passed through: and sadly there is no way to distinguish the original file descriptor. We don't strictly need the SIGPIPE handling for development any longer since `make stop-server` no longer shuts down the logrus process which explained the majority of hanging plugin processes. It remains suboptimal that a signal can terminate the server and leave plugin processes hanging, but the current symptoms are worse. --- cmd/mattermost/commands/server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/mattermost/commands/server.go b/cmd/mattermost/commands/server.go index 658d6a0ad4..d8553e3167 100644 --- a/cmd/mattermost/commands/server.go +++ b/cmd/mattermost/commands/server.go @@ -91,7 +91,7 @@ func runServer(configStore config.Store, disableConfigWatch bool, usedPlatform b // wait for kill signal before attempting to gracefully shutdown // the running service - signal.Notify(interruptChan, os.Interrupt, syscall.SIGINT, syscall.SIGTERM, syscall.SIGPIPE) + signal.Notify(interruptChan, os.Interrupt, syscall.SIGINT, syscall.SIGTERM) <-interruptChan return nil From 89b7b2d99bd1f657e3e1fb4fa1af2a6b975eeb63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jes=C3=BAs=20Espino?= Date: Mon, 30 Sep 2019 20:50:56 +0200 Subject: [PATCH 15/16] Adding MM_NO_DOCKER env variable to Makefile (#12384) * Adding MM_NO_DOCKER env variable to Makefile * Making it consisten tiwh the previous IS_CI value --- Makefile | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/Makefile b/Makefile index 4cedf9b666..f4758fd939 100644 --- a/Makefile +++ b/Makefile @@ -3,6 +3,7 @@ ROOT := $(dir $(abspath $(lastword $(MAKEFILE_LIST)))) IS_CI ?= false +MM_NO_DOCKER ?= false # Build Flags BUILD_NUMBER ?= $(BUILD_NUMBER:) BUILD_DATE = $(shell date -u) @@ -117,27 +118,35 @@ all: run ## Alias for 'run'. include build/*.mk start-docker: ## Starts the docker containers for local development. -ifeq ($(IS_CI),false) +ifneq ($(IS_CI),false) + @echo CI Build: skipping docker start +else ifeq ($(MM_NO_DOCKER),true) + @echo No Docker Enabled: skipping docker start +else @echo Starting docker containers docker-compose run --rm start_dependencies cat tests/${LDAP_DATA}-data.ldif | docker-compose exec -T openldap bash -c 'ldapadd -x -D "cn=admin,dc=mm,dc=test,dc=com" -w mostest || true'; - -else - @echo CI Build: skipping docker start endif stop-docker: ## Stops the docker containers for local development. +ifeq ($(MM_NO_DOCKER),true) + @echo No Docker Enabled: skipping docker stop +else @echo Stopping docker containers docker-compose stop - +endif clean-docker: ## Deletes the docker containers for local development. +ifeq ($(MM_NO_DOCKER),true) + @echo No Docker Enabled: skipping docker clean +else @echo Removing docker containers docker-compose down -v docker-compose rm -v +endif govet: ## Runs govet against all packages. From 8cea561ba6170f78864d31bdb0d2576fad6d9c91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jes=C3=BAs=20Espino?= Date: Mon, 30 Sep 2019 21:39:21 +0200 Subject: [PATCH 16/16] More robust team exists api endpoint (#12130) * More robust team exists api endpoint * Making the code more concise * Better handling of errors on GetTeamByName --- api4/team.go | 29 ++++++++--- api4/team_test.go | 99 ++++++++++++++++++++++++++++++------ app/team.go | 7 +-- i18n/en.json | 4 ++ store/sqlstore/team_store.go | 3 ++ 5 files changed, 115 insertions(+), 27 deletions(-) diff --git a/api4/team.go b/api4/team.go index 5c0df4902c..da7a8f2da6 100644 --- a/api4/team.go +++ b/api4/team.go @@ -812,14 +812,31 @@ func teamExists(c *Context, w http.ResponseWriter, r *http.Request) { return } - resp := make(map[string]bool) - - if _, err := c.App.GetTeamByName(c.Params.TeamName); err != nil { - resp["exists"] = false - } else { - resp["exists"] = true + team, err := c.App.GetTeamByName(c.Params.TeamName) + if err != nil && err.StatusCode != http.StatusNotFound { + c.Err = err + return } + exists := false + + if team != nil { + var teamMember *model.TeamMember + teamMember, err = c.App.GetTeamMember(team.Id, c.App.Session.UserId) + if err != nil && err.StatusCode != http.StatusNotFound { + c.Err = err + return + } + + // Verify that the user can see the team (be a member or have the permission to list the team) + if (teamMember != nil && teamMember.DeleteAt == 0) || + (team.AllowOpenInvite && c.App.SessionHasPermissionTo(c.App.Session, model.PERMISSION_LIST_PUBLIC_TEAMS)) || + (!team.AllowOpenInvite && c.App.SessionHasPermissionTo(c.App.Session, model.PERMISSION_LIST_PRIVATE_TEAMS)) { + exists = true + } + } + + resp := map[string]bool{"exists": exists} w.Write([]byte(model.MapBoolToJson(resp))) } diff --git a/api4/team_test.go b/api4/team_test.go index dce8f8b7d0..7dfda587c8 100644 --- a/api4/team_test.go +++ b/api4/team_test.go @@ -2102,25 +2102,94 @@ func TestTeamExists(t *testing.T) { th := Setup().InitBasic() defer th.TearDown() Client := th.Client - team := th.BasicTeam + public_member_team := th.BasicTeam + err := th.App.UpdateTeamPrivacy(public_member_team.Id, model.TEAM_OPEN, true) + require.Nil(t, err) - th.LoginBasic() + public_not_member_team := th.CreateTeamWithClient(th.SystemAdminClient) + err = th.App.UpdateTeamPrivacy(public_not_member_team.Id, model.TEAM_OPEN, true) + require.Nil(t, err) - exists, resp := Client.TeamExists(team.Name, "") - CheckNoError(t, resp) - if !exists { - t.Fatal("team should exist") - } + private_member_team := th.CreateTeamWithClient(th.SystemAdminClient) + th.LinkUserToTeam(th.BasicUser, private_member_team) + err = th.App.UpdateTeamPrivacy(private_member_team.Id, model.TEAM_INVITE, false) + require.Nil(t, err) - exists, resp = Client.TeamExists("testingteam", "") - CheckNoError(t, resp) - if exists { - t.Fatal("team should not exist") - } + private_not_member_team := th.CreateTeamWithClient(th.SystemAdminClient) + err = th.App.UpdateTeamPrivacy(private_not_member_team.Id, model.TEAM_INVITE, false) + require.Nil(t, err) - Client.Logout() - _, resp = Client.TeamExists(team.Name, "") - CheckUnauthorizedStatus(t, resp) + // Check the appropriate permissions are enforced. + defaultRolePermissions := th.SaveDefaultRolePermissions() + defer func() { + th.RestoreDefaultRolePermissions(defaultRolePermissions) + }() + + th.AddPermissionToRole(model.PERMISSION_LIST_PUBLIC_TEAMS.Id, model.SYSTEM_USER_ROLE_ID) + th.AddPermissionToRole(model.PERMISSION_LIST_PRIVATE_TEAMS.Id, model.SYSTEM_USER_ROLE_ID) + + t.Run("Logged user with permissions and valid public team", func(t *testing.T) { + th.LoginBasic() + exists, resp := Client.TeamExists(public_not_member_team.Name, "") + CheckNoError(t, resp) + assert.True(t, exists, "team should exist") + }) + + t.Run("Logged user with permissions and valid private team", func(t *testing.T) { + th.LoginBasic() + exists, resp := Client.TeamExists(private_not_member_team.Name, "") + CheckNoError(t, resp) + assert.True(t, exists, "team should exist") + }) + + t.Run("Logged user and invalid team", func(t *testing.T) { + th.LoginBasic() + exists, resp := Client.TeamExists("testingteam", "") + CheckNoError(t, resp) + assert.False(t, exists, "team should not exist") + }) + + t.Run("Logged out user", func(t *testing.T) { + Client.Logout() + _, resp := Client.TeamExists(public_not_member_team.Name, "") + CheckUnauthorizedStatus(t, resp) + }) + + t.Run("Logged without LIST_PUBLIC_TEAMS permissions and member public team", func(t *testing.T) { + th.LoginBasic() + th.RemovePermissionFromRole(model.PERMISSION_LIST_PUBLIC_TEAMS.Id, model.SYSTEM_USER_ROLE_ID) + + exists, resp := Client.TeamExists(public_member_team.Name, "") + CheckNoError(t, resp) + assert.True(t, exists, "team should be visible") + }) + + t.Run("Logged without LIST_PUBLIC_TEAMS permissions and not member public team", func(t *testing.T) { + th.LoginBasic() + th.RemovePermissionFromRole(model.PERMISSION_LIST_PUBLIC_TEAMS.Id, model.SYSTEM_USER_ROLE_ID) + + exists, resp := Client.TeamExists(public_not_member_team.Name, "") + CheckNoError(t, resp) + assert.False(t, exists, "team should not be visible") + }) + + t.Run("Logged without LIST_PRIVATE_TEAMS permissions and member private team", func(t *testing.T) { + th.LoginBasic() + th.RemovePermissionFromRole(model.PERMISSION_LIST_PRIVATE_TEAMS.Id, model.SYSTEM_USER_ROLE_ID) + + exists, resp := Client.TeamExists(private_member_team.Name, "") + CheckNoError(t, resp) + assert.True(t, exists, "team should be visible") + }) + + t.Run("Logged without LIST_PRIVATE_TEAMS permissions and not member private team", func(t *testing.T) { + th.LoginBasic() + th.RemovePermissionFromRole(model.PERMISSION_LIST_PRIVATE_TEAMS.Id, model.SYSTEM_USER_ROLE_ID) + + exists, resp := Client.TeamExists(private_not_member_team.Name, "") + CheckNoError(t, resp) + assert.False(t, exists, "team should not be visible") + }) } func TestImportTeam(t *testing.T) { diff --git a/app/team.go b/app/team.go index 3bc3e8409e..c8e2546a39 100644 --- 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) { diff --git a/i18n/en.json b/i18n/en.json index 3005763f68..a8a78b1019 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -6770,6 +6770,10 @@ "id": "store.sql_team.get_by_name.app_error", "translation": "Unable to find the existing team" }, + { + "id": "store.sql_team.get_by_name.missing.app_error", + "translation": "Unable to find the existing team" + }, { "id": "store.sql_team.get_by_scheme.app_error", "translation": "Unable to get the channels for the provided scheme" diff --git a/store/sqlstore/team_store.go b/store/sqlstore/team_store.go index 0de45d0f4e..b4784771f2 100644 --- a/store/sqlstore/team_store.go +++ b/store/sqlstore/team_store.go @@ -285,6 +285,9 @@ func (s SqlTeamStore) GetByName(name string) (*model.Team, *model.AppError) { err := s.GetReplica().SelectOne(&team, "SELECT * FROM Teams WHERE Name = :Name", map[string]interface{}{"Name": name}) if err != nil { + if err == sql.ErrNoRows { + return nil, model.NewAppError("SqlTeamStore.GetByName", "store.sql_team.get_by_name.missing.app_error", nil, "name="+name+","+err.Error(), http.StatusNotFound) + } return nil, model.NewAppError("SqlTeamStore.GetByName", "store.sql_team.get_by_name.app_error", nil, "name="+name+", "+err.Error(), http.StatusInternalServerError) } return &team, nil