Isolate more the mfa service (#16925)

* Isolate more the mfa service

* Simplifing the mfa service

* Removing channels and adding waitgroup

* Migrating mfa service to regular errors

* Fixing tests

* i18n extract

* Addressing PR review comments

* Removing unneeded struct

app/authentication.go

@@ -188,10 +188,13 @@ func (a *App) CheckUserMfa(user *model.User, token string) *model.AppError {
 		return nil
 	}
 
-	mfaService := mfa.New(a, a.Srv().Store)
+	if !*a.Config().ServiceSettings.EnableMultifactorAuthentication {
-	ok, err := mfaService.ValidateToken(user.MfaSecret, token)
+		return model.NewAppError("CheckUserMfa", "mfa.mfa_disabled.app_error", nil, "", http.StatusNotImplemented)
+	}
+
+	ok, err := mfa.New(a.Srv().Store.User()).ValidateToken(user.MfaSecret, token)
 	if err != nil {
-		return err
+		return model.NewAppError("CheckUserMfa", "mfa.validate_token.authenticate.app_error", nil, err.Error(), http.StatusBadRequest)
 	}
 
 	if !ok {

app/user.go

@@ -721,15 +721,18 @@ func (a *App) sanitizeProfiles(users []*model.User, asAdmin bool) []*model.User
 }
 
 func (a *App) GenerateMfaSecret(userID string) (*model.MfaSecret, *model.AppError) {
-	user, err := a.GetUser(userID)
+	user, appErr := a.GetUser(userID)
-	if err != nil {
+	if appErr != nil {
-		return nil, err
+		return nil, appErr
 	}
 
-	mfaService := mfa.New(a, a.Srv().Store)
+	if !*a.Config().ServiceSettings.EnableMultifactorAuthentication {
-	secret, img, err := mfaService.GenerateSecret(user)
+		return nil, model.NewAppError("GenerateMfaSecret", "mfa.mfa_disabled.app_error", nil, "", http.StatusNotImplemented)
+	}
+
+	secret, img, err := mfa.New(a.Srv().Store.User()).GenerateSecret(*a.Config().ServiceSettings.SiteURL, user.Email, user.Id)
 	if err != nil {
-		return nil, err
+		return nil, model.NewAppError("GenerateMfaSecret", "mfa.generate_qr_code.create_code.app_error", nil, err.Error(), http.StatusInternalServerError)
 	}
 
 	// Make sure the old secret is not cached on any cluster nodes.
@@ -755,9 +758,17 @@ func (a *App) ActivateMfa(userID, token string) *model.AppError {
 		return model.NewAppError("ActivateMfa", "api.user.activate_mfa.email_and_ldap_only.app_error", nil, "", http.StatusBadRequest)
 	}
 
-	mfaService := mfa.New(a, a.Srv().Store)
+	if !*a.Config().ServiceSettings.EnableMultifactorAuthentication {
-	if err := mfaService.Activate(user, token); err != nil {
+		return model.NewAppError("ActivateMfa", "mfa.mfa_disabled.app_error", nil, "", http.StatusNotImplemented)
-		return err
+	}
+
+	if err := mfa.New(a.Srv().Store.User()).Activate(user.MfaSecret, user.Id, token); err != nil {
+		switch {
+		case errors.Is(err, mfa.InvalidToken):
+			return model.NewAppError("ActivateMfa", "mfa.activate.bad_token.app_error", nil, "", http.StatusUnauthorized)
+		default:
+			return model.NewAppError("ActivateMfa", "mfa.activate.app_error", nil, err.Error(), http.StatusInternalServerError)
+		}
 	}
 
 	// Make sure old MFA status is not cached locally or in cluster nodes.
@@ -767,9 +778,12 @@ func (a *App) ActivateMfa(userID, token string) *model.AppError {
 }
 
 func (a *App) DeactivateMfa(userID string) *model.AppError {
-	mfaService := mfa.New(a, a.Srv().Store)
+	if !*a.Config().ServiceSettings.EnableMultifactorAuthentication {
-	if err := mfaService.Deactivate(userID); err != nil {
+		return model.NewAppError("DeactivateMfa", "mfa.mfa_disabled.app_error", nil, "", http.StatusNotImplemented)
-		return err
+	}
+
+	if err := mfa.New(a.Srv().Store.User()).Deactivate(userID); err != nil {
+		return model.NewAppError("DeactivateMfa", "mfa.deactivate.app_error", nil, err.Error(), http.StatusInternalServerError)
 	}
 
 	// Make sure old MFA status is not cached locally or in cluster nodes.

i18n/en.json

@@ -6935,33 +6935,21 @@
     "translation": "Mattermost Security Bulletin"
   },
   {
-    "id": "mfa.activate.authenticate.app_error",
+    "id": "mfa.activate.app_error",
-    "translation": "Error attempting to authenticate MFA token."
+    "translation": "Unable to update MFA active status for the user."
   },
   {
     "id": "mfa.activate.bad_token.app_error",
     "translation": "Invalid MFA token."
   },
   {
-    "id": "mfa.activate.save_active.app_error",
+    "id": "mfa.deactivate.app_error",
     "translation": "Unable to update MFA active status for the user."
   },
-  {
-    "id": "mfa.deactivate.save_active.app_error",
-    "translation": "Unable to update MFA active status for the user."
-  },
-  {
-    "id": "mfa.deactivate.save_secret.app_error",
-    "translation": "Error clearing the MFA secret."
-  },
   {
     "id": "mfa.generate_qr_code.create_code.app_error",
     "translation": "Error generating QR code."
   },
-  {
-    "id": "mfa.generate_qr_code.save_secret.app_error",
-    "translation": "Error saving the MFA secret."
-  },
   {
     "id": "mfa.mfa_disabled.app_error",
     "translation": "Multi-factor authentication has been disabled on this server."

model/utils.go

@@ -185,14 +185,6 @@ func NewRandomString(length int) string {
 	return encoding.EncodeToString(data)[:length]
 }
 
-// NewRandomBase32String returns a base32 encoded string of a random slice
-// of bytes of the given size. The resulting entropy will be (8 * size) bits.
-func NewRandomBase32String(size int) string {
-	data := make([]byte, size)
-	rand.Read(data)
-	return base32.StdEncoding.EncodeToString(data)
-}
-
 // GetMillis is a convenience method to get milliseconds since epoch.
 func GetMillis() int64 {
 	return time.Now().UnixNano() / int64(time.Millisecond)

model/utils_test.go

@@ -5,7 +5,6 @@ package model
 
 import (
 	"bytes"
-	"encoding/base32"
 	"fmt"
 	"net/http"
 	"reflect"
@@ -32,13 +31,6 @@ func TestRandomString(t *testing.T) {
 	}
 }
 
-func TestRandomBase32String(t *testing.T) {
-	for i := 0; i < 1000; i++ {
-		str := NewRandomBase32String(i)
-		require.Len(t, str, base32.StdEncoding.EncodedLen(i))
-	}
-}
-
 func TestGetMillisForTime(t *testing.T) {
 	thisTimeMillis := int64(1471219200000)
 	thisTime := time.Date(2016, time.August, 15, 0, 0, 0, 0, time.UTC)

services/mfa/mfa.go

@@ -4,39 +4,44 @@
 package mfa
 
 import (
+	"crypto/rand"
+	"encoding/base32"
 	"fmt"
-	"net/http"
 	"net/url"
 	"strings"
 
 	"github.com/dgryski/dgoogauth"
 	"github.com/mattermost/rsc/qr"
-
+	"github.com/pkg/errors"
-	"github.com/mattermost/mattermost-server/v5/model"
-	"github.com/mattermost/mattermost-server/v5/services/configservice"
-	"github.com/mattermost/mattermost-server/v5/store"
 )
 
+// InvalidToken indicates the case where the token validation has failed.
+var InvalidToken = errors.New("invalid mfa token")
+
 const (
 	// This will result in 160 bits of entropy (base32 encoded), as recommended by rfc4226.
-	MFASecretSize = 20
+	mfaSecretSize = 20
 )
 
-type Mfa struct {
+type Store interface {
-	ConfigService configservice.ConfigService
+	UpdateMfaActive(userId string, active bool) error
-	Store         store.Store
+	UpdateMfaSecret(userId, secret string) error
 }
 
-func New(configService configservice.ConfigService, store store.Store) Mfa {
+type MFA struct {
-	return Mfa{configService, store}
+	store Store
 }
 
-func (m *Mfa) checkConfig() *model.AppError {
+func New(store Store) *MFA {
-	if !*m.ConfigService.Config().ServiceSettings.EnableMultifactorAuthentication {
+	return &MFA{store}
-		return model.NewAppError("checkConfig", "mfa.mfa_disabled.app_error", nil, "", http.StatusNotImplemented)
+}
-	}
 
-	return nil
+// newRandomBase32String returns a base32 encoded string of a random slice
+// of bytes of the given size. The resulting entropy will be (8 * size) bits.
+func newRandomBase32String(size int) string {
+	data := make([]byte, size)
+	rand.Read(data)
+	return base32.StdEncoding.EncodeToString(data)
 }
 
 func getIssuerFromUrl(uri string) string {
@@ -52,39 +57,33 @@ func getIssuerFromUrl(uri string) string {
 	return url.QueryEscape(issuer)
 }
 
-func (m *Mfa) GenerateSecret(user *model.User) (string, []byte, *model.AppError) {
+// GenerateSecret generates a new user mfa secret and store it with the StoreSecret function provided
-	if err := m.checkConfig(); err != nil {
+func (m *MFA) GenerateSecret(siteURL, userEmail, userID string) (string, []byte, error) {
-		return "", nil, err
+	issuer := getIssuerFromUrl(siteURL)
-	}
 
-	issuer := getIssuerFromUrl(*m.ConfigService.Config().ServiceSettings.SiteURL)
+	secret := newRandomBase32String(mfaSecretSize)
 
-	secret := model.NewRandomBase32String(MFASecretSize)
+	authLink := fmt.Sprintf("otpauth://totp/%s:%s?secret=%s&issuer=%s", issuer, userEmail, secret, issuer)
-
-	authLink := fmt.Sprintf("otpauth://totp/%s:%s?secret=%s&issuer=%s", issuer, user.Email, secret, issuer)
 
 	code, err := qr.Encode(authLink, qr.H)
 
 	if err != nil {
-		return "", nil, model.NewAppError("GenerateQrCode", "mfa.generate_qr_code.create_code.app_error", nil, err.Error(), http.StatusInternalServerError)
+		return "", nil, errors.Wrap(err, "unable to generate qr code")
 	}
 
 	img := code.PNG()
 
-	if err := m.Store.User().UpdateMfaSecret(user.Id, secret); err != nil {
+	if err := m.store.UpdateMfaSecret(userID, secret); err != nil {
-		return "", nil, model.NewAppError("GenerateQrCode", "mfa.generate_qr_code.save_secret.app_error", nil, err.Error(), http.StatusInternalServerError)
+		return "", nil, errors.Wrap(err, "unable to store mfa secret")
 	}
 
 	return secret, img, nil
 }
 
-func (m *Mfa) Activate(user *model.User, token string) *model.AppError {
+// Activate set the mfa as active and store it with the StoreActive function provided
-	if err := m.checkConfig(); err != nil {
+func (m *MFA) Activate(userMfaSecret, userID string, token string) error {
-		return err
-	}
-
 	otpConfig := &dgoogauth.OTPConfig{
-		Secret:      user.MfaSecret,
+		Secret:      userMfaSecret,
 		WindowSize:  3,
 		HotpCounter: 0,
 	}
@@ -93,47 +92,35 @@ func (m *Mfa) Activate(user *model.User, token string) *model.AppError {
 
 	ok, err := otpConfig.Authenticate(trimmedToken)
 	if err != nil {
-		return model.NewAppError("Activate", "mfa.activate.authenticate.app_error", nil, err.Error(), http.StatusInternalServerError)
+		return errors.Wrap(err, "unable to parse the token")
 	}
 
 	if !ok {
-		return model.NewAppError("Activate", "mfa.activate.bad_token.app_error", nil, "", http.StatusUnauthorized)
+		return InvalidToken
 	}
 
-	if appErr := m.Store.User().UpdateMfaActive(user.Id, true); appErr != nil {
+	if err := m.store.UpdateMfaActive(userID, true); err != nil {
-		return model.NewAppError("Activate", "mfa.activate.save_active.app_error", nil, appErr.Error(), http.StatusInternalServerError)
+		return errors.Wrap(err, "unable to store mfa active")
 	}
 
 	return nil
 }
 
-func (m *Mfa) Deactivate(userId string) *model.AppError {
+// Deactivate set the mfa as deactive, remove the mfa secret, store it with the StoreActive and StoreSecret functions provided
-	if err := m.checkConfig(); err != nil {
+func (m *MFA) Deactivate(userId string) error {
-		return err
+	if err := m.store.UpdateMfaActive(userId, false); err != nil {
+		return errors.Wrap(err, "unable to store mfa active")
 	}
 
-	schan := make(chan error, 1)
+	if err := m.store.UpdateMfaSecret(userId, ""); err != nil {
-	go func() {
+		return errors.Wrap(err, "unable to store mfa secret")
-		schan <- m.Store.User().UpdateMfaSecret(userId, "")
-		close(schan)
-	}()
-
-	if err := m.Store.User().UpdateMfaActive(userId, false); err != nil {
-		return model.NewAppError("Deactivate", "mfa.deactivate.save_active.app_error", nil, err.Error(), http.StatusInternalServerError)
-	}
-
-	if err := <-schan; err != nil {
-		return model.NewAppError("Deactivate", "mfa.deactivate.save_secret.app_error", nil, err.Error(), http.StatusInternalServerError)
 	}
 
 	return nil
 }
 
-func (m *Mfa) ValidateToken(secret, token string) (bool, *model.AppError) {
+// Validate the provide token using the secret provided
-	if err := m.checkConfig(); err != nil {
+func (m *MFA) ValidateToken(secret, token string) (bool, error) {
-		return false, err
-	}
-
 	otpConfig := &dgoogauth.OTPConfig{
 		Secret:      secret,
 		WindowSize:  3,
@@ -143,7 +130,7 @@ func (m *Mfa) ValidateToken(secret, token string) (bool, *model.AppError) {
 	trimmedToken := strings.TrimSpace(token)
 	ok, err := otpConfig.Authenticate(trimmedToken)
 	if err != nil {
-		return false, model.NewAppError("ValidateToken", "mfa.validate_token.authenticate.app_error", nil, err.Error(), http.StatusBadRequest)
+		return false, errors.Wrap(err, "unable to parse the token")
 	}
 
 	return ok, nil

services/mfa/mfa_test.go

@@ -4,6 +4,7 @@
 package mfa
 
 import (
+	"encoding/base32"
 	"errors"
 	"fmt"
 	"net/url"
@@ -11,60 +12,36 @@ import (
 	"time"
 
 	"github.com/dgryski/dgoogauth"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-
-	"github.com/mattermost/mattermost-server/v5/model"
 	"github.com/mattermost/mattermost-server/v5/plugin/plugintest/mock"
 	"github.com/mattermost/mattermost-server/v5/store/storetest/mocks"
-	"github.com/mattermost/mattermost-server/v5/utils/testutils"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestGenerateSecret(t *testing.T) {
-	user := &model.User{Id: model.NewId(), Roles: "system_user"}
+	userID := "user-id"
-
+	userEmail := "sample@sample.com"
-	config := model.Config{}
+	siteURL := "http://localhost:8065"
-	config.SetDefaults()
-	config.ServiceSettings.EnableMultifactorAuthentication = model.NewBool(true)
-	configService := testutils.StaticConfigService{Cfg: &config}
-
-	t.Run("fail on disabled mfa", func(t *testing.T) {
-		wrongConfig := model.Config{}
-		wrongConfig.SetDefaults()
-		wrongConfig.ServiceSettings.EnableMultifactorAuthentication = model.NewBool(false)
-		wrongConfigService := testutils.StaticConfigService{Cfg: &wrongConfig}
-		mfa := New(wrongConfigService, nil)
-		_, _, err := mfa.GenerateSecret(user)
-		require.NotNil(t, err)
-		require.Equal(t, "mfa.mfa_disabled.app_error", err.Id)
-	})
 
 	t.Run("fail on store action fail", func(t *testing.T) {
-		storeMock := mocks.Store{}
+		storeMock := mocks.UserStore{}
-		userStoreMock := mocks.UserStore{}
+		storeMock.On("UpdateMfaSecret", userID, mock.AnythingOfType("string")).Return(func(userId string, secret string) error {
-		userStoreMock.On("UpdateMfaSecret", user.Id, mock.AnythingOfType("string")).Return(func(userId string, secret string) error {
 			return errors.New("failed to update mfa secret")
 		})
-		storeMock.On("User").Return(&userStoreMock)
 
-		mfa := New(configService, &storeMock)
+		_, _, err := New(&storeMock).GenerateSecret(siteURL, userEmail, userID)
-		_, _, err := mfa.GenerateSecret(user)
+		require.Error(t, err)
-		require.NotNil(t, err)
+		require.Contains(t, err.Error(), "unable to store mfa secret")
-		require.Equal(t, "mfa.generate_qr_code.save_secret.app_error", err.Id)
 	})
 
 	t.Run("Successful generate secret", func(t *testing.T) {
-		storeMock := mocks.Store{}
+		storeMock := mocks.UserStore{}
-		userStoreMock := mocks.UserStore{}
+		storeMock.On("UpdateMfaSecret", userID, mock.AnythingOfType("string")).Return(func(userId string, secret string) error {
-		userStoreMock.On("UpdateMfaSecret", user.Id, mock.AnythingOfType("string")).Return(func(userId string, secret string) error {
 			return nil
 		})
-		storeMock.On("User").Return(&userStoreMock)
 
-		mfa := New(configService, &storeMock)
+		secret, img, err := New(&storeMock).GenerateSecret(siteURL, userEmail, userID)
-
+		require.NoError(t, err)
-		secret, img, err := mfa.GenerateSecret(user)
-		require.Nil(t, err)
 		assert.Len(t, secret, 32)
 		require.NotEmpty(t, img, "no image set")
 	})
@@ -92,179 +69,117 @@ func TestGetIssuerFromUrl(t *testing.T) {
 }
 
 func TestActivate(t *testing.T) {
-	user := &model.User{Id: model.NewId(), Roles: "system_user"}
+	userID := "user-id"
-	user.MfaSecret = model.NewRandomBase32String(MFASecretSize)
+	userMfaSecret := newRandomBase32String(mfaSecretSize)
 
-	token := dgoogauth.ComputeCode(user.MfaSecret, time.Now().UTC().Unix()/30)
+	token := dgoogauth.ComputeCode(userMfaSecret, time.Now().UTC().Unix()/30)
-
-	config := model.Config{}
-	config.SetDefaults()
-	config.ServiceSettings.EnableMultifactorAuthentication = model.NewBool(true)
-	configService := testutils.StaticConfigService{Cfg: &config}
-
-	t.Run("fail on disabled mfa", func(t *testing.T) {
-		wrongConfig := model.Config{}
-		wrongConfig.SetDefaults()
-		wrongConfig.ServiceSettings.EnableMultifactorAuthentication = model.NewBool(false)
-		wrongConfigService := testutils.StaticConfigService{Cfg: &wrongConfig}
-		mfa := New(wrongConfigService, nil)
-		err := mfa.Activate(user, "not-important")
-		require.NotNil(t, err)
-		require.Equal(t, "mfa.mfa_disabled.app_error", err.Id)
-	})
 
 	t.Run("fail on wrongly formatted token", func(t *testing.T) {
-		mfa := New(configService, nil)
+		err := New(nil).Activate(userMfaSecret, userID, "invalid-token")
-		err := mfa.Activate(user, "invalid-token")
+		require.Error(t, err)
-		require.NotNil(t, err)
+		require.Contains(t, err.Error(), "unable to parse the token")
-		require.Equal(t, "mfa.activate.authenticate.app_error", err.Id)
 	})
 
 	t.Run("fail on invalid token", func(t *testing.T) {
-		mfa := New(configService, nil)
+		err := New(nil).Activate(userMfaSecret, userID, "000000")
-		err := mfa.Activate(user, "000000")
+		require.Error(t, err)
-		require.NotNil(t, err)
+		require.Contains(t, err.Error(), "invalid mfa token")
-		require.Equal(t, "mfa.activate.bad_token.app_error", err.Id)
 	})
 
 	t.Run("fail on store action fail", func(t *testing.T) {
-		storeMock := mocks.Store{}
+		storeMock := mocks.UserStore{}
-		userStoreMock := mocks.UserStore{}
+		storeMock.On("UpdateMfaActive", userID, true).Return(func(userId string, active bool) error {
-		userStoreMock.On("UpdateMfaActive", user.Id, true).Return(func(userId string, active bool) error {
 			return errors.New("failed to update mfa active")
 		})
-		storeMock.On("User").Return(&userStoreMock)
 
-		mfa := New(configService, &storeMock)
+		err := New(&storeMock).Activate(userMfaSecret, userID, fmt.Sprintf("%06d", token))
-		err := mfa.Activate(user, fmt.Sprintf("%06d", token))
+		require.Error(t, err)
-		require.NotNil(t, err)
+		require.Contains(t, err.Error(), "unable to store mfa active")
-		require.Equal(t, "mfa.activate.save_active.app_error", err.Id)
 	})
 
 	t.Run("Successful activate", func(t *testing.T) {
-		storeMock := mocks.Store{}
+		storeMock := mocks.UserStore{}
-		userStoreMock := mocks.UserStore{}
+		storeMock.On("UpdateMfaActive", userID, true).Return(func(userId string, active bool) error {
-		userStoreMock.On("UpdateMfaActive", user.Id, true).Return(func(userId string, active bool) error {
 			return nil
 		})
-		storeMock.On("User").Return(&userStoreMock)
-		mfa := New(configService, &storeMock)
 
-		err := mfa.Activate(user, fmt.Sprintf("%06d", token))
+		err := New(&storeMock).Activate(userMfaSecret, userID, fmt.Sprintf("%06d", token))
-		require.Nil(t, err)
+		require.NoError(t, err)
 	})
 }
 
 func TestDeactivate(t *testing.T) {
-	user := &model.User{Id: model.NewId(), Roles: "system_user"}
+	userID := "user-id"
-
-	config := model.Config{}
-	config.SetDefaults()
-	config.ServiceSettings.EnableMultifactorAuthentication = model.NewBool(true)
-	configService := testutils.StaticConfigService{Cfg: &config}
-
-	t.Run("fail on disabled mfa", func(t *testing.T) {
-		wrongConfig := model.Config{}
-		wrongConfig.SetDefaults()
-		wrongConfig.ServiceSettings.EnableMultifactorAuthentication = model.NewBool(false)
-		wrongConfigService := testutils.StaticConfigService{Cfg: &wrongConfig}
-		mfa := New(wrongConfigService, nil)
-		err := mfa.Deactivate(user.Id)
-		require.NotNil(t, err)
-		require.Equal(t, "mfa.mfa_disabled.app_error", err.Id)
-	})
 
 	t.Run("fail on store UpdateMfaActive action fail", func(t *testing.T) {
-		storeMock := mocks.Store{}
+		storeMock := mocks.UserStore{}
-		userStoreMock := mocks.UserStore{}
+		storeMock.On("UpdateMfaActive", userID, false).Return(func(userId string, active bool) error {
-		userStoreMock.On("UpdateMfaActive", user.Id, false).Return(func(userId string, active bool) error {
 			return errors.New("failed to update mfa active")
 		})
-		userStoreMock.On("UpdateMfaSecret", user.Id, "").Return(func(userId string, secret string) error {
+		storeMock.On("UpdateMfaSecret", userID, "").Return(func(userId string, secret string) error {
 			return errors.New("failed to update mfa secret")
 		})
-		storeMock.On("User").Return(&userStoreMock)
 
-		mfa := New(configService, &storeMock)
+		err := New(&storeMock).Deactivate(userID)
-		err := mfa.Deactivate(user.Id)
+		require.Error(t, err)
-		require.NotNil(t, err)
+		require.Contains(t, err.Error(), "unable to store mfa active")
-		require.Equal(t, "mfa.deactivate.save_active.app_error", err.Id)
 	})
 
 	t.Run("fail on store UpdateMfaSecret action fail", func(t *testing.T) {
-		storeMock := mocks.Store{}
+		storeMock := mocks.UserStore{}
-		userStoreMock := mocks.UserStore{}
+		storeMock.On("UpdateMfaActive", userID, false).Return(func(userId string, active bool) error {
-		userStoreMock.On("UpdateMfaActive", user.Id, false).Return(func(userId string, active bool) error {
 			return nil
 		})
-		userStoreMock.On("UpdateMfaSecret", user.Id, "").Return(func(userId string, secret string) error {
+		storeMock.On("UpdateMfaSecret", userID, "").Return(func(userId string, secret string) error {
 			return errors.New("failed to update mfa secret")
 		})
-		storeMock.On("User").Return(&userStoreMock)
 
-		mfa := New(configService, &storeMock)
+		err := New(&storeMock).Deactivate(userID)
-		err := mfa.Deactivate(user.Id)
+		require.Error(t, err)
-		require.NotNil(t, err)
+		require.Contains(t, err.Error(), "unable to store mfa secret")
-		require.Equal(t, "mfa.deactivate.save_secret.app_error", err.Id)
 	})
 
 	t.Run("Successful deactivate", func(t *testing.T) {
-		storeMock := mocks.Store{}
+		storeMock := mocks.UserStore{}
-		userStoreMock := mocks.UserStore{}
+		storeMock.On("UpdateMfaActive", userID, false).Return(func(userId string, active bool) error {
-		userStoreMock.On("UpdateMfaActive", user.Id, false).Return(func(userId string, active bool) error {
 			return nil
 		})
-		userStoreMock.On("UpdateMfaSecret", user.Id, "").Return(func(userId string, secret string) error {
+		storeMock.On("UpdateMfaSecret", userID, "").Return(func(userId string, secret string) error {
 			return nil
 		})
-		storeMock.On("User").Return(&userStoreMock)
-		mfa := New(configService, &storeMock)
 
-		err := mfa.Deactivate(user.Id)
+		err := New(&storeMock).Deactivate(userID)
-		require.Nil(t, err)
+		require.NoError(t, err)
 	})
 }
 
 func TestValidateToken(t *testing.T) {
-	secret := model.NewRandomBase32String(MFASecretSize)
+	secret := newRandomBase32String(mfaSecretSize)
 	token := dgoogauth.ComputeCode(secret, time.Now().UTC().Unix()/30)
 
-	config := model.Config{}
-	config.SetDefaults()
-	config.ServiceSettings.EnableMultifactorAuthentication = model.NewBool(true)
-	configService := testutils.StaticConfigService{Cfg: &config}
-
-	t.Run("fail on disabled mfa", func(t *testing.T) {
-		wrongConfig := model.Config{}
-		wrongConfig.SetDefaults()
-		wrongConfig.ServiceSettings.EnableMultifactorAuthentication = model.NewBool(false)
-		wrongConfigService := testutils.StaticConfigService{Cfg: &wrongConfig}
-		mfa := New(wrongConfigService, nil)
-		ok, err := mfa.ValidateToken(secret, fmt.Sprintf("%06d", token))
-		require.NotNil(t, err)
-		require.False(t, ok)
-		require.Equal(t, "mfa.mfa_disabled.app_error", err.Id)
-	})
-
 	t.Run("fail on wrongly formatted token", func(t *testing.T) {
-		mfa := New(configService, nil)
+		ok, err := New(nil).ValidateToken(secret, "invalid-token")
-		ok, err := mfa.ValidateToken(secret, "invalid-token")
+		require.Error(t, err)
-		require.NotNil(t, err)
 		require.False(t, ok)
-		require.Equal(t, "mfa.validate_token.authenticate.app_error", err.Id)
+		require.Contains(t, err.Error(), "unable to parse the token")
 	})
 
 	t.Run("fail on invalid token", func(t *testing.T) {
-		mfa := New(configService, nil)
+		ok, err := New(nil).ValidateToken(secret, "000000")
-		ok, err := mfa.ValidateToken(secret, "000000")
+		require.NoError(t, err)
-		require.Nil(t, err)
 		require.False(t, ok)
 	})
 
 	t.Run("valid token", func(t *testing.T) {
-		mfa := New(configService, nil)
+		ok, err := New(nil).ValidateToken(secret, fmt.Sprintf("%06d", token))
-		ok, err := mfa.ValidateToken(secret, fmt.Sprintf("%06d", token))
+		require.NoError(t, err)
-		require.Nil(t, err)
 		require.True(t, ok)
 	})
 }
+
+func TestRandomBase32String(t *testing.T) {
+	for i := 0; i < 1000; i++ {
+		str := newRandomBase32String(i)
+		require.Len(t, str, base32.StdEncoding.EncodedLen(i))
+	}
+}