From 79fb20bc1a70a9d2e1016ad7996b4227375d49f8 Mon Sep 17 00:00:00 2001 From: Daniel Schalla Date: Mon, 10 Jun 2019 23:25:25 +0200 Subject: [PATCH] [MM-15767] Mask errors in login flow only explicitly (#11051) * Explicit list of errors that should be masked for login flow * Fix unit test * fix test #2 * Use of whitelist of passed through errors; Rework error messages --- api4/user.go | 55 ++++++++++++++++++++++++++++---- api4/user_test.go | 81 +++++++++++++++++++++++++++++++++++++++-------- i18n/en.json | 16 ++++++++-- 3 files changed, 131 insertions(+), 21 deletions(-) diff --git a/api4/user.go b/api4/user.go index 0241b983ab..f7ba1a4a54 100644 --- a/api4/user.go +++ b/api4/user.go @@ -1242,13 +1242,56 @@ func sendPasswordReset(c *Context, w http.ResponseWriter, r *http.Request) { func login(c *Context, w http.ResponseWriter, r *http.Request) { // Translate all login errors to generic. MFA error being an exception, since it's required for the login flow itself defer func() { - if c.Err != nil && - c.Err.Id != "mfa.validate_token.authenticate.app_error" && - c.Err.Id != "api.user.login.blank_pwd.app_error" && - c.Err.Id != "api.user.login.bot_login_forbidden.app_error" && - c.Err.Id != "api.user.login.client_side_cert.certificate.app_error" { - c.Err = model.NewAppError("login", "api.user.login.invalid_credentials", nil, "", http.StatusUnauthorized) + if c.Err == nil { + return } + + unmaskedErrors := []string{ + "mfa.validate_token.authenticate.app_error", + "api.user.check_user_mfa.bad_code.app_error", + "api.user.login.blank_pwd.app_error", + "api.user.login.bot_login_forbidden.app_error", + "api.user.login.client_side_cert.certificate.app_error", + "api.user.login.inactive.app_error", + "api.user.login.not_verified.app_error", + } + + maskError := true + + for _, unmaskedError := range unmaskedErrors { + if c.Err.Id == unmaskedError { + maskError = false + } + } + + if !maskError { + return + } + + config := c.App.Config() + enableUsername := *config.EmailSettings.EnableSignInWithUsername + enableEmail := *config.EmailSettings.EnableSignInWithEmail + samlEnabled := *config.SamlSettings.Enable + gitlabEnabled := *config.GetSSOService("gitlab").Enable + googleEnabled := *config.GetSSOService("google").Enable + office365Enabled := *config.GetSSOService("office365").Enable + + if samlEnabled || gitlabEnabled || googleEnabled || office365Enabled { + c.Err = model.NewAppError("login", "api.user.login.invalid_credentials_sso", nil, "", http.StatusUnauthorized) + return + } + + if enableUsername && !enableEmail { + c.Err = model.NewAppError("login", "api.user.login.invalid_credentials_username", nil, "", http.StatusUnauthorized) + return + } + + if !enableUsername && enableEmail { + c.Err = model.NewAppError("login", "api.user.login.invalid_credentials_email", nil, "", http.StatusUnauthorized) + return + } + + c.Err = model.NewAppError("login", "api.user.login.invalid_credentials_email_username", nil, "", http.StatusUnauthorized) }() props := model.MapFromJson(r.Body) diff --git a/api4/user_test.go b/api4/user_test.go index d8869b52de..d6945cf3e9 100644 --- a/api4/user_test.go +++ b/api4/user_test.go @@ -2673,7 +2673,7 @@ func TestLogin(t *testing.T) { t.Run("unknown user", func(t *testing.T) { _, resp := th.Client.Login("unknown", th.BasicUser.Password) - CheckErrorMessage(t, resp, "api.user.login.invalid_credentials") + CheckErrorMessage(t, resp, "api.user.login.invalid_credentials_email_username") }) t.Run("valid login", func(t *testing.T) { @@ -4121,6 +4121,61 @@ func TestGetUserTermsOfService(t *testing.T) { assert.NotEmpty(t, userTermsOfService.CreateAt) } +func TestLoginErrorMessage(t *testing.T) { + th := Setup().InitBasic() + defer th.TearDown() + + _, resp := th.Client.Logout() + CheckNoError(t, resp) + + + // Email and Username enabled + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.EmailSettings.EnableSignInWithEmail = true + *cfg.EmailSettings.EnableSignInWithUsername = true + }) + _, resp = th.Client.Login(th.BasicUser.Email, "wrong") + CheckErrorMessage(t, resp, "api.user.login.invalid_credentials_email_username") + + // Email enabled + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.EmailSettings.EnableSignInWithEmail = true + *cfg.EmailSettings.EnableSignInWithUsername = false + }) + _, resp = th.Client.Login(th.BasicUser.Email, "wrong") + CheckErrorMessage(t, resp, "api.user.login.invalid_credentials_email") + + // Username enabled + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.EmailSettings.EnableSignInWithEmail = false + *cfg.EmailSettings.EnableSignInWithUsername = true + }) + _, resp = th.Client.Login(th.BasicUser.Email, "wrong") + CheckErrorMessage(t, resp, "api.user.login.invalid_credentials_username") + + // SAML/SSO enabled + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.SamlSettings.Enable = true + *cfg.SamlSettings.Verify = false + *cfg.SamlSettings.Encrypt = false + *cfg.SamlSettings.IdpUrl = "https://localhost/adfs/ls" + *cfg.SamlSettings.IdpDescriptorUrl = "https://localhost/adfs/services/trust" + *cfg.SamlSettings.AssertionConsumerServiceURL = "https://localhost/login/sso/saml" + *cfg.SamlSettings.IdpCertificateFile = app.SamlIdpCertificateName + *cfg.SamlSettings.PrivateKeyFile = app.SamlPrivateKeyName + *cfg.SamlSettings.PublicCertificateFile = app.SamlPublicCertificateName + *cfg.SamlSettings.EmailAttribute = "Email" + *cfg.SamlSettings.UsernameAttribute = "Username" + *cfg.SamlSettings.FirstNameAttribute = "FirstName" + *cfg.SamlSettings.LastNameAttribute = "LastName" + *cfg.SamlSettings.NicknameAttribute = "" + *cfg.SamlSettings.PositionAttribute = "" + *cfg.SamlSettings.LocaleAttribute = "" + }) + _, resp = th.Client.Login(th.BasicUser.Email, "wrong") + CheckErrorMessage(t, resp, "api.user.login.invalid_credentials_sso") +} + func TestLoginLockout(t *testing.T) { th := Setup().InitBasic() defer th.TearDown() @@ -4132,34 +4187,34 @@ func TestLoginLockout(t *testing.T) { th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableMultifactorAuthentication = true }) _, resp = th.Client.Login(th.BasicUser.Email, "wrong") - CheckErrorMessage(t, resp, "api.user.login.invalid_credentials") + CheckErrorMessage(t, resp, "api.user.login.invalid_credentials_email_username") _, resp = th.Client.Login(th.BasicUser.Email, "wrong") - CheckErrorMessage(t, resp, "api.user.login.invalid_credentials") + CheckErrorMessage(t, resp, "api.user.login.invalid_credentials_email_username") _, resp = th.Client.Login(th.BasicUser.Email, "wrong") - CheckErrorMessage(t, resp, "api.user.login.invalid_credentials") + CheckErrorMessage(t, resp, "api.user.login.invalid_credentials_email_username") _, resp = th.Client.Login(th.BasicUser.Email, "wrong") - CheckErrorMessage(t, resp, "api.user.login.invalid_credentials") + CheckErrorMessage(t, resp, "api.user.login.invalid_credentials_email_username") _, resp = th.Client.Login(th.BasicUser.Email, "wrong") - CheckErrorMessage(t, resp, "api.user.login.invalid_credentials") + CheckErrorMessage(t, resp, "api.user.login.invalid_credentials_email_username") //Check if lock is active _, resp = th.Client.Login(th.BasicUser.Email, th.BasicUser.Password) - CheckErrorMessage(t, resp, "api.user.login.invalid_credentials") + CheckErrorMessage(t, resp, "api.user.login.invalid_credentials_email_username") // Fake user has MFA enabled if result := <-th.Server.Store.User().UpdateMfaActive(th.BasicUser2.Id, true); result.Err != nil { t.Fatal(result.Err) } _, resp = th.Client.LoginWithMFA(th.BasicUser2.Email, th.BasicUser2.Password, "000000") - CheckErrorMessage(t, resp, "api.user.login.invalid_credentials") + CheckErrorMessage(t, resp, "api.user.check_user_mfa.bad_code.app_error") _, resp = th.Client.LoginWithMFA(th.BasicUser2.Email, th.BasicUser2.Password, "000000") - CheckErrorMessage(t, resp, "api.user.login.invalid_credentials") + CheckErrorMessage(t, resp, "api.user.check_user_mfa.bad_code.app_error") _, resp = th.Client.LoginWithMFA(th.BasicUser2.Email, th.BasicUser2.Password, "000000") - CheckErrorMessage(t, resp, "api.user.login.invalid_credentials") + CheckErrorMessage(t, resp, "api.user.check_user_mfa.bad_code.app_error") _, resp = th.Client.LoginWithMFA(th.BasicUser2.Email, th.BasicUser2.Password, "000000") - CheckErrorMessage(t, resp, "api.user.login.invalid_credentials") + CheckErrorMessage(t, resp, "api.user.login.invalid_credentials_email_username") _, resp = th.Client.LoginWithMFA(th.BasicUser2.Email, th.BasicUser2.Password, "000000") - CheckErrorMessage(t, resp, "api.user.login.invalid_credentials") + CheckErrorMessage(t, resp, "api.user.login.invalid_credentials_email_username") // Fake user has MFA disabled if result := <-th.Server.Store.User().UpdateMfaActive(th.BasicUser2.Id, false); result.Err != nil { @@ -4168,5 +4223,5 @@ func TestLoginLockout(t *testing.T) { //Check if lock is active _, resp = th.Client.Login(th.BasicUser2.Email, th.BasicUser2.Password) - CheckErrorMessage(t, resp, "api.user.login.invalid_credentials") + CheckErrorMessage(t, resp, "api.user.login.invalid_credentials_email_username") } diff --git a/i18n/en.json b/i18n/en.json index ed24c876cf..1e51ad1430 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -2375,8 +2375,20 @@ "translation": "Login failed because your account has been deactivated. Please contact an administrator." }, { - "id": "api.user.login.invalid_credentials", - "translation": "User ID or password incorrect." + "id": "api.user.login.invalid_credentials_email", + "translation": "Enter a valid email and/or password" + }, + { + "id": "api.user.login.invalid_credentials_email_username", + "translation": "Enter a valid email or username and/or password." + }, + { + "id": "api.user.login.invalid_credentials_sso", + "translation": "Enter a valid email or username and/or password, or sign in using another method.\n" + }, + { + "id": "api.user.login.invalid_credentials_username", + "translation": "Enter a valid username and/or password." }, { "id": "api.user.login.not_verified.app_error",