Merge branch 'master' into mark-as-unread

config/database_test.go

@@ -513,11 +513,7 @@ func TestDatabaseStoreSet(t *testing.T) {
 		id, _ := getActualDatabaseConfig(t)
 		assert.NotEqual(t, activeId, id, "new record should have been written")
 
-		select {
-		case <-called:
-		case <-time.After(5 * time.Second):
-			require.Fail(t, "callback should have been called when config written")
-		}
+		require.True(t, wasCalled(called, 5*time.Second), "callback should have been called when config written")
 	})
 }
 
@@ -754,11 +750,7 @@ func TestDatabaseStoreLoad(t *testing.T) {
 		err = ds.Load()
 		require.NoError(t, err)
 
-		select {
-		case <-called:
-		case <-time.After(5 * time.Second):
-			require.Fail(t, "callback should have been called when config loaded")
-		}
+		require.True(t, wasCalled(called, 5*time.Second), "callback should have been called when config loaded")
 	})
 }
 

config/file_test.go

@@ -477,11 +477,7 @@ func TestFileStoreSet(t *testing.T) {
 		require.NoError(t, err)
 		assert.Equal(t, oldCfg, retCfg)
 
-		select {
-		case <-called:
-		case <-time.After(5 * time.Second):
-			require.Fail(t, "callback should have been called when config written")
-		}
+		require.True(t, wasCalled(called, 5*time.Second), "callback should have been called when config written")
 	})
 
 	t.Run("watcher restarted", func(t *testing.T) {
@@ -513,11 +509,7 @@ func TestFileStoreSet(t *testing.T) {
 		require.NoError(t, err)
 
 		ioutil.WriteFile(path, cfgData, 0644)
-		select {
-		case <-called:
-		case <-time.After(5 * time.Second):
-			require.Fail(t, "callback should have been called when config written")
-		}
+		require.True(t, wasCalled(called, 5*time.Second), "callback should have been called when config written")
 	})
 }
 
@@ -749,11 +741,7 @@ func TestFileStoreLoad(t *testing.T) {
 		err = fs.Load()
 		require.NoError(t, err)
 
-		select {
-		case <-called:
-		case <-time.After(5 * time.Second):
-			require.Fail(t, "callback should have been called when config loaded")
-		}
+		require.True(t, wasCalled(called, 5*time.Second), "callback should have been called when config loaded")
 	})
 }
 
@@ -786,11 +774,7 @@ func TestFileStoreWatcherEmitter(t *testing.T) {
 		require.NoError(t, err)
 
 		ioutil.WriteFile(path, cfgData, 0644)
-		select {
-		case <-called:
-			require.Fail(t, "callback should not have been called since watching disabled")
-		case <-time.After(1 * time.Second):
-		}
+		require.False(t, wasCalled(called, 1*time.Second), "callback should not have been called since watching disabled")
 	})
 
 	t.Run("enabled", func(t *testing.T) {
@@ -809,11 +793,7 @@ func TestFileStoreWatcherEmitter(t *testing.T) {
 		require.NoError(t, err)
 
 		ioutil.WriteFile(path, cfgData, 0644)
-		select {
-		case <-called:
-		case <-time.After(5 * time.Second):
-			require.Fail(t, "callback should have been called when config written")
-		}
+		require.True(t, wasCalled(called, 5*time.Second), "callback should have been called when config written")
 	})
 }
 
@@ -1126,3 +1106,14 @@ func TestFileStoreString(t *testing.T) {
 
 	assert.Equal(t, "file://"+path, fs.String())
 }
+
+// wasCalled reports whether a given callback channel was called
+// within the specified time duration or not.
+func wasCalled(c chan bool, duration time.Duration) bool {
+	select {
+	case <-c:
+		return true
+	case <-time.After(duration):
+	}
+	return false
+}

config/memory_test.go

@@ -215,11 +215,7 @@ func TestMemoryStoreSet(t *testing.T) {
 		require.NoError(t, err)
 		assert.Equal(t, oldCfg, retCfg)
 
-		select {
-		case <-called:
-		case <-time.After(5 * time.Second):
-			require.Fail(t, "callback should have been called when config written")
-		}
+		require.True(t, wasCalled(called, 5*time.Second), "callback should have been called when config written")
 	})
 }
 
@@ -268,11 +264,7 @@ func TestMemoryStoreLoad(t *testing.T) {
 		err = ms.Load()
 		require.NoError(t, err)
 
-		select {
-		case <-called:
-		case <-time.After(5 * time.Second):
-			require.Fail(t, "callback should have been called when config loaded")
-		}
+		require.True(t, wasCalled(called, 5*time.Second), "callback should have been called when config loaded")
 	})
 }
 

config/watcher_test.go

@@ -11,8 +11,6 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/require"
-
-	"github.com/mattermost/mattermost-server/utils/testutils"
 )
 
 func TestWatcherInvalidDirectory(t *testing.T) {
@@ -49,9 +47,17 @@ func TestWatcher(t *testing.T) {
 
 	// Write to a different file
 	ioutil.WriteFile(filepath.Join(tempDir, "unrelated"), []byte("data"), 0644)
-	require.False(t, testutils.WasCalled(called, 1*time.Second), "callback should not have been called for unrelated file")
+	select {
+	case <-called:
+		t.Fatal("callback should not have been called for unrelated file")
+	case <-time.After(1 * time.Second):
+	}
 
 	// Write to the watched file
 	ioutil.WriteFile(f.Name(), []byte("data"), 0644)
-	require.True(t, testutils.WasCalled(called, 5*time.Second), "callback should have been called when file written")
+	select {
+	case <-called:
+	case <-time.After(5 * time.Second):
+		t.Fatal("callback should have been called when file written")
+	}
 }

i18n/en.json

@@ -4718,10 +4718,6 @@
     "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."

jobs/schedulers.go

@@ -20,6 +20,7 @@ type Schedulers struct {
 	listenerId           string
 	startOnce            sync.Once
 	jobs                 *JobServer
+	isLeader             bool
 
 	schedulers   []model.Scheduler
 	nextRunTimes []*time.Time
@@ -34,6 +35,7 @@ func (srv *JobServer) InitSchedulers() *Schedulers {
 		configChanged:        make(chan *model.Config),
 		clusterLeaderChanged: make(chan bool),
 		jobs:                 srv,
+		isLeader:             true,
 	}
 
 	if srv.DataRetentionJob != nil {
@@ -103,7 +105,7 @@ func (schedulers *Schedulers) Start() *Schedulers {
 						if time.Now().After(*nextTime) {
 							scheduler := schedulers.schedulers[idx]
 							if scheduler != nil {
-								if scheduler.Enabled(cfg) {
+								if schedulers.isLeader && scheduler.Enabled(cfg) {
 									if _, err := schedulers.scheduleJob(cfg, scheduler); err != nil {
 										mlog.Error("Failed to schedule job", mlog.String("scheduler", scheduler.Name()), mlog.Err(err))
 									} else {
@@ -115,7 +117,7 @@ func (schedulers *Schedulers) Start() *Schedulers {
 					}
 				case newCfg := <-schedulers.configChanged:
 					for idx, scheduler := range schedulers.schedulers {
-						if !scheduler.Enabled(newCfg) {
+						if !schedulers.isLeader || !scheduler.Enabled(newCfg) {
 							schedulers.nextRunTimes[idx] = nil
 						} else {
 							schedulers.setNextRunTime(newCfg, idx, now, false)
@@ -123,6 +125,7 @@ func (schedulers *Schedulers) Start() *Schedulers {
 					}
 				case isLeader := <-schedulers.clusterLeaderChanged:
 					for idx := range schedulers.schedulers {
+						schedulers.isLeader = isLeader
 						if !isLeader {
 							schedulers.nextRunTimes[idx] = nil
 						} else {

jobs/schedulers_test.go

@@ -0,0 +1,105 @@
+// Copyright (c) 2017-present Mattermost, Inc. All Rights Reserved.
+// See License.txt for license information.
+package jobs
+
+import (
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/mattermost/mattermost-server/einterfaces/mocks"
+	"github.com/mattermost/mattermost-server/plugin/plugintest/mock"
+
+	"github.com/mattermost/mattermost-server/model"
+	"github.com/mattermost/mattermost-server/store/storetest"
+	"github.com/mattermost/mattermost-server/utils/testutils"
+)
+
+type MockScheduler struct {
+	mock.Mock
+}
+
+func (scheduler *MockScheduler) Enabled(cfg *model.Config) bool {
+	return true
+}
+
+func (scheduler *MockScheduler) Name() string {
+	return "MockScheduler"
+}
+
+func (scheduler *MockScheduler) JobType() string {
+	return model.JOB_TYPE_DATA_RETENTION
+}
+
+func (scheduler *MockScheduler) NextScheduleTime(cfg *model.Config, now time.Time, pendingJobs bool, lastSuccessfulJob *model.Job) *time.Time {
+	nextTime := time.Now().Add(60 * time.Second)
+	return &nextTime
+}
+
+func (scheduler *MockScheduler) ScheduleJob(cfg *model.Config, pendingJobs bool, lastSuccessfulJob *model.Job) (*model.Job, *model.AppError) {
+	return nil, nil
+}
+
+func TestScheduler(t *testing.T) {
+	mockStore := &storetest.Store{}
+	defer mockStore.AssertExpectations(t)
+
+	job := &model.Job{
+		Id:       model.NewId(),
+		CreateAt: model.GetMillis(),
+		Status:   model.JOB_STATUS_PENDING,
+		Type:     model.JOB_TYPE_MESSAGE_EXPORT,
+	}
+	// mock job store doesn't return a previously successful job, forcing fallback to config
+	mockStore.JobStore.On("GetNewestJobByStatusAndType", mock.AnythingOfType("string"), mock.AnythingOfType("string")).Return(job, nil)
+	mockStore.JobStore.On("GetCountByStatusAndType", mock.AnythingOfType("string"), mock.AnythingOfType("string")).Return(int64(1), nil)
+
+	jobServer := &JobServer{
+		Store: mockStore,
+		ConfigService: &testutils.StaticConfigService{
+			Cfg: &model.Config{
+				// mock config
+				DataRetentionSettings: *&model.DataRetentionSettings{
+					EnableMessageDeletion: model.NewBool(true),
+				},
+				MessageExportSettings: *&model.MessageExportSettings{
+					EnableExport: model.NewBool(true),
+				},
+			},
+		},
+	}
+
+	jobInterface := new(mocks.DataRetentionJobInterface)
+	jobInterface.On("MakeScheduler").Return(new(MockScheduler))
+	jobServer.DataRetentionJob = jobInterface
+
+	exportInterface := new(mocks.MessageExportJobInterface)
+	exportInterface.On("MakeScheduler").Return(new(MockScheduler))
+	jobServer.MessageExportJob = exportInterface
+
+	schedulers := jobServer.InitSchedulers()
+	schedulers.Start()
+	time.Sleep(1 * time.Second)
+
+	// They should be all on here
+	for _, element := range schedulers.nextRunTimes {
+		assert.NotNil(t, element)
+	}
+
+	schedulers.HandleClusterLeaderChange(false)
+	time.Sleep(1 * time.Second)
+	// They should be turned off
+	for _, element := range schedulers.nextRunTimes {
+		assert.Nil(t, element)
+	}
+
+	// After running a config change, they should stay off
+	schedulers.handleConfigChange(nil, nil)
+	for _, element := range schedulers.nextRunTimes {
+		assert.Nil(t, element)
+	}
+
+	schedulers.Stop()
+
+}

model/config.go

@@ -141,14 +141,9 @@ const (
 
 	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
@@ -1911,7 +1906,6 @@ type SamlSettings struct {
 	AssertionConsumerServiceURL *string
 
 	SignatureAlgorithm *string
-	DigestAlgorithm    *string
 	CanonicalAlgorithm *string
 
 	ScopingIDPProviderId *string
@@ -1968,10 +1962,6 @@ func (s *SamlSettings) SetDefaults() {
 		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)
 	}
@@ -2852,12 +2842,9 @@ func (ss *SamlSettings) isValid() *AppError {
 			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) {
+		if !(*ss.SignatureAlgorithm == SAML_SETTINGS_SIGNATURE_ALGORITHM_SHA1 || *ss.SignatureAlgorithm == SAML_SETTINGS_SIGNATURE_ALGORITHM_SHA256 || *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)
 		}

model/config_test.go

@@ -103,9 +103,6 @@ func TestConfigDefaultSignatureAlgorithm(t *testing.T) {
 		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")
 	}
@@ -117,7 +114,6 @@ func TestConfigOverwriteSignatureAlgorithm(t *testing.T) {
 		SamlSettings: SamlSettings{
 			CanonicalAlgorithm: NewString(testAlgorithm),
 			SignatureAlgorithm: NewString(testAlgorithm),
-			DigestAlgorithm:    NewString(testAlgorithm),
 		},
 	}
 
@@ -126,9 +122,6 @@ func TestConfigOverwriteSignatureAlgorithm(t *testing.T) {
 	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")
 	}
@@ -177,15 +170,7 @@ func TestConfigIsValidFakeAlgorithm(t *testing.T) {
 	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 {

utils/testutils/testutils.go

@@ -8,7 +8,6 @@ import (
 	"io"
 	"os"
 	"path/filepath"
-	"time"
 
 	"github.com/mattermost/mattermost-server/utils/fileutils"
 )
@@ -28,15 +27,3 @@ func ReadTestFile(name string) ([]byte, error) {
 		return data.Bytes(), nil
 	}
 }
-
-// WasCalled reports whether a given callback channel was called
-// within the specified time duration or not.
-func WasCalled(c chan bool, duration time.Duration) bool {
-	wasCalled := false
-	select {
-	case <-c:
-		wasCalled = true
-	case <-time.After(duration):
-	}
-	return wasCalled
-}