diff --git a/api4/user.go b/api4/user.go index 2e0778bb7d..2216f1f13e 100644 --- a/api4/user.go +++ b/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"] diff --git a/api4/user_test.go b/api4/user_test.go index 683737b137..a116f2c00f 100644 --- a/api4/user_test.go +++ b/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) { diff --git a/app/authentication.go b/app/authentication.go index ebc016ea21..7278813d3e 100644 --- a/app/authentication.go +++ b/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 } diff --git a/config/default.json b/config/default.json index a36c29f066..0dbee3e4f8 100644 --- a/config/default.json +++ b/config/default.json @@ -73,6 +73,7 @@ "ExperimentalChannelOrganization": false, "EnableAPITeamDeletion": false, "ExperimentalEnableHardenedMode": false, + "DisableLegacyMFA": true, "EnableEmailInvitations": false, "ExperimentalLdapGroupSync": false, "ExperimentalStrictCSRFEnforcement": false diff --git a/model/client4.go b/model/client4.go index 1cdf50abd9..14d5524cbf 100644 --- a/model/client4.go +++ b/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 diff --git a/model/config.go b/model/config.go index ffe5cbbd33..1ae3c1d4f7 100644 --- a/model/config.go +++ b/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) }