[MM-14081] Disable checkMFA Endpoint by default and add tests for MFA login (#10356)

api4/user.go

@@ -915,7 +915,15 @@ func updateUserAuth(c *Context, w http.ResponseWriter, r *http.Request) {
 	w.Write([]byte(user.ToJson()))
 }
 
+// Deprecated: checkUserMfa is deprecated and should not be used anymore, starting with version 6.0 it will be disabled.
+//			   Clients should attempt a login without MFA and will receive a MFA error when it's required.
 func checkUserMfa(c *Context, w http.ResponseWriter, r *http.Request) {
+
+	if *c.App.Config().ServiceSettings.DisableLegacyMFA {
+		http.NotFound(w, r)
+		return
+	}
+
 	props := model.MapFromJson(r.Body)
 
 	loginId := props["login_id"]

api4/user_test.go

@@ -4,6 +4,7 @@
 package api4
 
 import (
+	"github.com/dgryski/dgoogauth"
 	"net/http"
 	"strconv"
 	"strings"
@@ -1804,6 +1805,7 @@ func TestUpdateUserMfa(t *testing.T) {
 	CheckForbiddenStatus(t, resp)
 }
 
+// CheckUserMfa is deprecated and should not be used anymore, it will be disabled by default in version 6.0
 func TestCheckUserMfa(t *testing.T) {
 	th := Setup().InitBasic()
 	defer th.TearDown()
@@ -1847,6 +1849,68 @@ func TestCheckUserMfa(t *testing.T) {
 	if required {
 		t.Fatal("should be false - mfa not active")
 	}
+
+	th.App.UpdateConfig(func (c *model.Config){
+		*c.ServiceSettings.DisableLegacyMFA = true
+	})
+
+	_, resp = th.Client.CheckUserMfa(th.BasicUser.Email)
+	CheckNotFoundStatus(t, resp)
+}
+
+func TestUserLoginMFAFlow(t *testing.T) {
+	th := Setup().InitBasic()
+	defer th.TearDown()
+
+	th.App.UpdateConfig(func (c *model.Config){
+		*c.ServiceSettings.DisableLegacyMFA = true
+		*c.ServiceSettings.EnableMultifactorAuthentication = true
+	})
+
+	secret, err := th.App.GenerateMfaSecret(th.BasicUser.Id)
+	assert.Nil(t, err)
+
+	t.Run("WithoutMFA", func (t *testing.T){
+		_, resp := th.Client.Login(th.BasicUser.Email, th.BasicUser.Password)
+		CheckNoError(t, resp)
+	})
+
+	// Fake user has MFA enabled
+	if result := <-th.Server.Store.User().UpdateMfaActive(th.BasicUser.Id, true); result.Err != nil {
+		t.Fatal(result.Err)
+	}
+
+	if result := <-th.Server.Store.User().UpdateMfaSecret(th.BasicUser.Id, secret.Secret); result.Err != nil {
+		t.Fatal(result.Err)
+	}
+
+	t.Run("WithInvalidMFA", func (t *testing.T){
+		user, resp := th.Client.Login(th.BasicUser.Email, th.BasicUser.Password)
+		CheckErrorMessage(t, resp, "mfa.validate_token.authenticate.app_error")
+		assert.Nil(t, user)
+
+		user, resp = th.Client.LoginWithMFA(th.BasicUser.Email, th.BasicUser.Password, "")
+		CheckErrorMessage(t, resp, "mfa.validate_token.authenticate.app_error")
+		assert.Nil(t, user)
+
+		user, resp = th.Client.LoginWithMFA(th.BasicUser.Email, th.BasicUser.Password, "abcdefgh")
+		CheckErrorMessage(t, resp, "mfa.validate_token.authenticate.app_error")
+		assert.Nil(t, user)
+
+		secret2, err := th.App.GenerateMfaSecret(th.BasicUser2.Id)
+		assert.Nil(t, err)
+		user, resp = th.Client.LoginWithMFA(th.BasicUser.Email, th.BasicUser.Password, secret2.Secret)
+		CheckErrorMessage(t, resp, "mfa.validate_token.authenticate.app_error")
+		assert.Nil(t, user)
+	})
+
+	t.Run("WithCorrectMFA", func (t *testing.T){
+		code := dgoogauth.ComputeCode(secret.Secret, time.Now().UTC().Unix() / 30)
+
+		user, resp := th.Client.LoginWithMFA(th.BasicUser.Email, th.BasicUser.Password, strconv.Itoa(code))
+		CheckNoError(t, resp)
+		assert.NotNil(t, user)
+	})
 }
 
 func TestGenerateMfaSecret(t *testing.T) {

app/authentication.go

@@ -53,8 +53,12 @@ func (a *App) CheckPasswordAndAllCriteria(user *model.User, password string, mfa
 	}
 
 	if err := a.CheckUserMfa(user, mfaToken); err != nil {
-		if result := <-a.Srv.Store.User().UpdateFailedPasswordAttempts(user.Id, user.FailedAttempts+1); result.Err != nil {
-			return result.Err
+		// If the mfaToken is not set, we assume the client used this as a pre-flight request to query the server
+		// about the MFA state of the user in question
+		if mfaToken != "" {
+			if result := <-a.Srv.Store.User().UpdateFailedPasswordAttempts(user.Id, user.FailedAttempts+1); result.Err != nil {
+				return result.Err
+			}
 		}
 		return err
 	}

config/default.json

@@ -73,6 +73,7 @@
         "ExperimentalChannelOrganization": false,
         "EnableAPITeamDeletion": false,
         "ExperimentalEnableHardenedMode": false,
+        "DisableLegacyMFA": true,
         "EnableEmailInvitations": false,
         "ExperimentalLdapGroupSync": false,
         "ExperimentalStrictCSRFEnforcement": false

model/client4.go

@@ -991,6 +991,7 @@ func (c *Client4) UpdateUserMfa(userId, code string, activate bool) (bool, *Resp
 
 // CheckUserMfa checks whether a user has MFA active on their account or not based on the
 // provided login id.
+// Deprecated: Clients should use Login method and check for MFA Error
 func (c *Client4) CheckUserMfa(loginId string) (bool, *Response) {
 	requestBody := make(map[string]interface{})
 	requestBody["login_id"] = loginId

model/config.go

@@ -283,6 +283,7 @@ type ServiceSettings struct {
 	DEPRECATED_DO_NOT_USE_ImageProxyOptions           *string `json:"ImageProxyOptions" mapstructure:"ImageProxyOptions"` // This field is deprecated and must not be used.
 	EnableAPITeamDeletion                             *bool
 	ExperimentalEnableHardenedMode                    *bool
+	DisableLegacyMFA                                  *bool
 	ExperimentalStrictCSRFEnforcement                 *bool
 	EnableEmailInvitations                            *bool
 	ExperimentalLdapGroupSync                         *bool
@@ -609,6 +610,10 @@ func (s *ServiceSettings) SetDefaults() {
 		s.ExperimentalEnableHardenedMode = NewBool(false)
 	}
 
+	if s.DisableLegacyMFA == nil {
+		s.DisableLegacyMFA = NewBool(false)
+	}
+
 	if s.ExperimentalLdapGroupSync == nil {
 		s.ExperimentalLdapGroupSync = NewBool(false)
 	}