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. 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/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/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)) } }) 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") } } 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, }) 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/cmd/mattermost/commands/server.go b/cmd/mattermost/commands/server.go index b705671f4a..d8553e3167 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) @@ -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 diff --git a/config/database.go b/config/database.go index 4c61375007..c80cf8ef55 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,7 +21,11 @@ import ( _ "github.com/lib/pq" ) -var tcpStripper = regexp.MustCompile(`@tcp\((.*)\)`) +// 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 // DatabaseStore is a config store backed by a database. type DatabaseStore struct { @@ -65,6 +67,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 ( @@ -90,6 +94,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 } @@ -130,6 +148,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) @@ -141,6 +168,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") @@ -236,6 +268,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, @@ -295,16 +332,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/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) { 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 } diff --git a/i18n/en.json b/i18n/en.json index 44e3abd5e4..265785a0ef 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." @@ -6766,6 +6778,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/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 } 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) } diff --git a/model/config.go b/model/config.go index 378933abc7..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/" @@ -174,7 +188,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" @@ -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() 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 } 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 { 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