Improving mfa tests coverage and format (#13978)

* Improving mfa tests coverage and format

* Adding assertions for the specific error types

Co-authored-by: mattermod <mattermod@users.noreply.github.com>

services/mfa/mfa_test.go

@@ -6,6 +6,7 @@ package mfa
 import (
 	b32 "encoding/base32"
 	"fmt"
+	"net/http"
 	"net/url"
 	"testing"
 	"time"
@@ -27,26 +28,47 @@ func TestGenerateSecret(t *testing.T) {
 	config.SetDefaults()
 	config.ServiceSettings.EnableMultifactorAuthentication = model.NewBool(true)
 	configService := testutils.StaticConfigService{Cfg: &config}
-	storeMock := mocks.Store{}
+
-	userStoreMock := mocks.UserStore{}
+	t.Run("fail on disabled mfa", func(t *testing.T) {
-	userStoreMock.On("UpdateMfaSecret", user.Id, mock.AnythingOfType("string")).Return(func(userId string, secret string) *model.AppError {
+		wrongConfig := model.Config{}
-		return nil
+		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, err.Id, "mfa.mfa_disabled.app_error")
 	})
-	storeMock.On("User").Return(&userStoreMock)
 
-	mfa := Mfa{configService, &storeMock}
+	t.Run("fail on store action fail", func(t *testing.T) {
+		storeMock := mocks.Store{}
+		userStoreMock := mocks.UserStore{}
+		userStoreMock.On("UpdateMfaSecret", user.Id, mock.AnythingOfType("string")).Return(func(userId string, secret string) *model.AppError {
+			return model.NewAppError("GenerateQrCode", "mfa.generate_qr_code.save_secret.app_error", nil, "", http.StatusInternalServerError)
+		})
+		storeMock.On("User").Return(&userStoreMock)
 
-	secret, img, err := mfa.GenerateSecret(user)
+		mfa := New(configService, &storeMock)
-	require.Nil(t, err)
+		_, _, err := mfa.GenerateSecret(user)
+		require.NotNil(t, err)
+		require.Equal(t, err.Id, "mfa.generate_qr_code.save_secret.app_error")
+	})
 
-	assert.Len(t, secret, 32)
+	t.Run("Successful generate secret", func(t *testing.T) {
+		storeMock := mocks.Store{}
+		userStoreMock := mocks.UserStore{}
+		userStoreMock.On("UpdateMfaSecret", user.Id, mock.AnythingOfType("string")).Return(func(userId string, secret string) *model.AppError {
+			return nil
+		})
+		storeMock.On("User").Return(&userStoreMock)
 
-	require.NotEmpty(t, img, "no image set")
+		mfa := New(configService, &storeMock)
 
-	config.ServiceSettings.EnableMultifactorAuthentication = model.NewBool(false)
+		secret, img, err := mfa.GenerateSecret(user)
-
+		require.Nil(t, err)
-	_, _, err = mfa.GenerateSecret(user)
+		assert.Len(t, secret, 32)
-	require.NotNil(t, err)
+		require.NotEmpty(t, img, "no image set")
+	})
 }
 
 func TestGetIssuerFromUrl(t *testing.T) {
@@ -72,24 +94,177 @@ func TestGetIssuerFromUrl(t *testing.T) {
 
 func TestActivate(t *testing.T) {
 	user := &model.User{Id: model.NewId(), Roles: "system_user"}
+	user.MfaSecret = b32.StdEncoding.EncodeToString([]byte(model.NewRandomString(MFA_SECRET_SIZE)))
+	token := dgoogauth.ComputeCode(user.MfaSecret, time.Now().UTC().Unix()/30)
 
 	config := model.Config{}
 	config.SetDefaults()
 	config.ServiceSettings.EnableMultifactorAuthentication = model.NewBool(true)
 	configService := testutils.StaticConfigService{Cfg: &config}
-	storeMock := mocks.Store{}
+
-	userStoreMock := mocks.UserStore{}
+	t.Run("fail on disabled mfa", func(t *testing.T) {
-	userStoreMock.On("UpdateMfaActive", user.Id, mock.AnythingOfType("bool")).Return(func(userId string, active bool) *model.AppError {
+		wrongConfig := model.Config{}
-		return nil
+		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, err.Id, "mfa.mfa_disabled.app_error")
 	})
-	storeMock.On("User").Return(&userStoreMock)
 
-	mfa := Mfa{configService, &storeMock}
+	t.Run("fail on wrongly formatted token", func(t *testing.T) {
+		mfa := New(configService, nil)
+		err := mfa.Activate(user, "invalid-token")
+		require.NotNil(t, err)
+		require.Equal(t, err.Id, "mfa.activate.authenticate.app_error")
+	})
 
-	user.MfaSecret = b32.StdEncoding.EncodeToString([]byte(model.NewRandomString(MFA_SECRET_SIZE)))
+	t.Run("fail on invalid token", func(t *testing.T) {
+		mfa := New(configService, nil)
+		err := mfa.Activate(user, "000000")
+		require.NotNil(t, err)
+		require.Equal(t, err.Id, "mfa.activate.bad_token.app_error")
+	})
 
-	token := dgoogauth.ComputeCode(user.MfaSecret, time.Now().UTC().Unix()/30)
+	t.Run("fail on store action fail", func(t *testing.T) {
+		storeMock := mocks.Store{}
+		userStoreMock := mocks.UserStore{}
+		userStoreMock.On("UpdateMfaActive", user.Id, true).Return(func(userId string, active bool) *model.AppError {
+			return model.NewAppError("Activate", "mfa.activate.save_active.app_error", nil, "", http.StatusInternalServerError)
+		})
+		storeMock.On("User").Return(&userStoreMock)
 
-	err := mfa.Activate(user, fmt.Sprintf("%06d", token))
+		mfa := New(configService, &storeMock)
-	require.Nil(t, err)
+		err := mfa.Activate(user, fmt.Sprintf("%06d", token))
+		require.NotNil(t, err)
+		require.Equal(t, err.Id, "mfa.activate.save_active.app_error")
+	})
+
+	t.Run("Successful activate", func(t *testing.T) {
+		storeMock := mocks.Store{}
+		userStoreMock := mocks.UserStore{}
+		userStoreMock.On("UpdateMfaActive", user.Id, true).Return(func(userId string, active bool) *model.AppError {
+			return nil
+		})
+		storeMock.On("User").Return(&userStoreMock)
+		mfa := New(configService, &storeMock)
+
+		err := mfa.Activate(user, fmt.Sprintf("%06d", token))
+		require.Nil(t, err)
+	})
+}
+
+func TestDeactivate(t *testing.T) {
+	user := &model.User{Id: model.NewId(), Roles: "system_user"}
+
+	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, err.Id, "mfa.mfa_disabled.app_error")
+	})
+
+	t.Run("fail on store UpdateMfaActive action fail", func(t *testing.T) {
+		storeMock := mocks.Store{}
+		userStoreMock := mocks.UserStore{}
+		userStoreMock.On("UpdateMfaActive", user.Id, false).Return(func(userId string, active bool) *model.AppError {
+			return model.NewAppError("Deactivate", "mfa.deactivate.save_active.app_error", nil, "", http.StatusInternalServerError)
+		})
+		userStoreMock.On("UpdateMfaSecret", user.Id, "").Return(func(userId string, secret string) *model.AppError {
+			return model.NewAppError("Deactivate", "mfa.deactivate.save_secret.app_error", nil, "", http.StatusInternalServerError)
+		})
+		storeMock.On("User").Return(&userStoreMock)
+
+		mfa := New(configService, &storeMock)
+		err := mfa.Deactivate(user.Id)
+		require.NotNil(t, err)
+		require.Equal(t, err.Id, "mfa.deactivate.save_active.app_error")
+	})
+
+	t.Run("fail on store UpdateMfaSecret action fail", func(t *testing.T) {
+		storeMock := mocks.Store{}
+		userStoreMock := mocks.UserStore{}
+		userStoreMock.On("UpdateMfaActive", user.Id, false).Return(func(userId string, active bool) *model.AppError {
+			return nil
+		})
+		userStoreMock.On("UpdateMfaSecret", user.Id, "").Return(func(userId string, secret string) *model.AppError {
+			return model.NewAppError("Deactivate", "mfa.deactivate.save_secret.app_error", nil, "", http.StatusInternalServerError)
+		})
+		storeMock.On("User").Return(&userStoreMock)
+
+		mfa := New(configService, &storeMock)
+		err := mfa.Deactivate(user.Id)
+		require.NotNil(t, err)
+		require.Equal(t, err.Id, "mfa.deactivate.save_secret.app_error")
+	})
+
+	t.Run("Successful deactivate", func(t *testing.T) {
+		storeMock := mocks.Store{}
+		userStoreMock := mocks.UserStore{}
+		userStoreMock.On("UpdateMfaActive", user.Id, false).Return(func(userId string, active bool) *model.AppError {
+			return nil
+		})
+		userStoreMock.On("UpdateMfaSecret", user.Id, "").Return(func(userId string, secret string) *model.AppError {
+			return nil
+		})
+		storeMock.On("User").Return(&userStoreMock)
+		mfa := New(configService, &storeMock)
+
+		err := mfa.Deactivate(user.Id)
+		require.Nil(t, err)
+	})
+}
+
+func TestValidateToken(t *testing.T) {
+	secret := b32.StdEncoding.EncodeToString([]byte(model.NewRandomString(MFA_SECRET_SIZE)))
+	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, err.Id, "mfa.mfa_disabled.app_error")
+	})
+
+	t.Run("fail on wrongly formatted token", func(t *testing.T) {
+		mfa := New(configService, nil)
+		ok, err := mfa.ValidateToken(secret, "invalid-token")
+		require.NotNil(t, err)
+		require.False(t, ok)
+		require.Equal(t, err.Id, "mfa.validate_token.authenticate.app_error")
+	})
+
+	t.Run("fail on invalid token", func(t *testing.T) {
+		mfa := New(configService, nil)
+		ok, err := mfa.ValidateToken(secret, "000000")
+		require.Nil(t, err)
+		require.False(t, ok)
+	})
+
+	t.Run("valid token", func(t *testing.T) {
+		mfa := New(configService, nil)
+		ok, err := mfa.ValidateToken(secret, fmt.Sprintf("%06d", token))
+		require.Nil(t, err)
+		require.True(t, ok)
+	})
 }