ABC-179: check email verification last (#8172)

* ABC-179: check email verification last

This change changes the authentication checks to be:
* "preflight checks"
** mfa
** not disabled
** login attempts
* password
* "postflight checks"
** email verified

Checking whether the email is verified or not last avoids the weird edge
case where entering any bogus password for an account with an unverified
email shows a message about verifying the email and offering to resend.

* fix invalid unit test assertion

Client.CreateUser returns a user whose password has been sanitized.
Adopt the pattern in the previous assertions to use a new variable name
and test the password on the original model.User object. This didn't
expose any underlying broken behaviour, but the test wouldn't have
caught it if it had regressed.

Also fix a minor typo.

3 files changed, 67 insertions, 10 deletions

diff --git a/api/user.go b/api/user.go
index ae8a6cab6..a2e06c013 100644
--- a/api/user.go
+++ b/api/user.go
@@ -1159,7 +1159,7 @@ func completeSaml(c *Context, w http.ResponseWriter, r *http.Request) {
 		}
 		return
 	} else {
-		if err := c.App.CheckUserAdditionalAuthenticationCriteria(user, ""); err != nil {
+		if err := c.App.CheckUserAllAuthenticationCriteria(user, ""); err != nil {
 			c.Err = err
 			c.Err.StatusCode = http.StatusFound
 			return
diff --git a/api/user_test.go b/api/user_test.go
index b74039ce9..8d6aad22b 100644
--- a/api/user_test.go
+++ b/api/user_test.go
@@ -16,6 +16,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/assert"
+
 	"github.com/mattermost/mattermost-server/app"
 	"github.com/mattermost/mattermost-server/model"
 	"github.com/mattermost/mattermost-server/store"
@@ -187,7 +189,7 @@ func TestLogin(t *testing.T) {
 	}
 
 	if _, err := Client.Login(ruser2.Data.(*model.User).Email, user2.Password); err != nil {
-		t.Fatal("From verfied hash")
+		t.Fatal("From verified hash")
 	}
 
 	Client.AuthToken = authToken
@@ -199,14 +201,49 @@ func TestLogin(t *testing.T) {
 		Password:    "passwd1",
 		AuthService: model.USER_AUTH_SERVICE_LDAP,
 	}
-	user3 = Client.Must(Client.CreateUser(user3, "")).Data.(*model.User)
-	store.Must(th.App.Srv.Store.User().VerifyEmail(user3.Id))
+	ruser3 := Client.Must(Client.CreateUser(user3, "")).Data.(*model.User)
+	store.Must(th.App.Srv.Store.User().VerifyEmail(ruser3.Id))
 
-	if _, err := Client.Login(user3.Id, user3.Password); err == nil {
+	if _, err := Client.Login(ruser3.Id, user3.Password); err == nil {
 		t.Fatal("AD/LDAP user should not be able to log in with AD/LDAP disabled")
 	}
 }
 
+func TestLoginUnverifiedEmail(t *testing.T) {
+	assert := assert.New(t)
+
+	th := Setup().InitBasic()
+	defer th.TearDown()
+
+	Client := th.BasicClient
+
+	th.App.UpdateConfig(func(cfg *model.Config) {
+		*cfg.EmailSettings.EnableSignInWithEmail = true
+		cfg.EmailSettings.RequireEmailVerification = true
+	})
+
+	Client.Logout()
+
+	user := &model.User{
+		Email:         strings.ToLower(model.NewId()) + "success+test@simulator.amazonses.com",
+		Nickname:      "Corey Hulen",
+		Username:      "corey" + model.NewId(),
+		Password:      "passwd1",
+		EmailVerified: false,
+	}
+	user = Client.Must(Client.CreateUser(user, "")).Data.(*model.User)
+
+	_, err := Client.Login(user.Email, user.Password+"invalid")
+	if assert.NotNil(err) {
+		assert.Equal("api.user.check_user_password.invalid.app_error", err.Id)
+	}
+
+	_, err = Client.Login(user.Email, "passwd1")
+	if assert.NotNil(err) {
+		assert.Equal("api.user.login.not_verified.app_error", err.Id)
+	}
+}
+
 func TestLoginByLdap(t *testing.T) {
 	th := Setup().InitBasic()
 	defer th.TearDown()
diff --git a/app/authentication.go b/app/authentication.go
index 140bffd5a..0b3659449 100644
--- a/app/authentication.go
+++ b/app/authentication.go
@@ -43,7 +43,7 @@ func (a *App) IsPasswordValid(password string) *model.AppError {
 }
 
 func (a *App) CheckPasswordAndAllCriteria(user *model.User, password string, mfaToken string) *model.AppError {
-	if err := a.CheckUserAdditionalAuthenticationCriteria(user, mfaToken); err != nil {
+	if err := a.CheckUserPreflightAuthenticationCriteria(user, mfaToken); err != nil {
 		return err
 	}
 
@@ -51,6 +51,10 @@ func (a *App) CheckPasswordAndAllCriteria(user *model.User, password string, mfa
 		return err
 	}
 
+	if err := a.CheckUserPostflightAuthenticationCriteria(user); err != nil {
+		return err
+	}
+
 	return nil
 }
 
@@ -109,13 +113,21 @@ func (a *App) checkLdapUserPasswordAndAllCriteria(ldapId *string, password strin
 	return user, nil
 }
 
-func (a *App) CheckUserAdditionalAuthenticationCriteria(user *model.User, mfaToken string) *model.AppError {
-	if err := a.CheckUserMfa(user, mfaToken); err != nil {
+func (a *App) CheckUserAllAuthenticationCriteria(user *model.User, mfaToken string) *model.AppError {
+	if err := a.CheckUserPreflightAuthenticationCriteria(user, mfaToken); err != nil {
 		return err
 	}
 
-	if !user.EmailVerified && a.Config().EmailSettings.RequireEmailVerification {
-		return model.NewAppError("Login", "api.user.login.not_verified.app_error", nil, "user_id="+user.Id, http.StatusUnauthorized)
+	if err := a.CheckUserPostflightAuthenticationCriteria(user); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func (a *App) CheckUserPreflightAuthenticationCriteria(user *model.User, mfaToken string) *model.AppError {
+	if err := a.CheckUserMfa(user, mfaToken); err != nil {
+		return err
 	}
 
 	if err := checkUserNotDisabled(user); err != nil {
@@ -129,6 +141,14 @@ func (a *App) CheckUserAdditionalAuthenticationCriteria(user *model.User, mfaTok
 	return nil
 }
 
+func (a *App) CheckUserPostflightAuthenticationCriteria(user *model.User) *model.AppError {
+	if !user.EmailVerified && a.Config().EmailSettings.RequireEmailVerification {
+		return model.NewAppError("Login", "api.user.login.not_verified.app_error", nil, "user_id="+user.Id, http.StatusUnauthorized)
+	}
+
+	return nil
+}
+
 func (a *App) CheckUserMfa(user *model.User, token string) *model.AppError {
 	if !user.MfaActive || !utils.IsLicensed() || !*utils.License().Features.MFA || !*a.Config().ServiceSettings.EnableMultifactorAuthentication {
 		return nil