Skip to content

Commit

Permalink
[MM-15767] Mask errors in login flow only explicitly (#11051)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@DSchalla
DSchalla authored Jun 10, 2019
1 parent 41774b6 commit 79fb20b
Show file tree
Hide file tree
Showing 3 changed files with 131 additions and 21 deletions.
55 changes: 49 additions & 6 deletions api4/user.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
81 changes: 68 additions & 13 deletions api4/user_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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()
Expand All @@ -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 {
Expand All @@ -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")
}
16 changes: 14 additions & 2 deletions i18n/en.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down

0 comments on commit 79fb20b

Please sign in to comment.