[MM-13610] Fix Login Hooks for SAML (#10288)

* Fix Login Hooks for SAML

* Update unit tests

* Delete extra whitespace

Co-Authored-By: DSchalla <daniel@schalla.me>

app/login.go

@@ -65,27 +65,6 @@ func (a *App) AuthenticateUserForLogin(id, loginId, password, mfaToken string, l
 		return nil, err
 	}
 
-	if pluginsEnvironment := a.GetPluginsEnvironment(); pluginsEnvironment != nil {
-		var rejectionReason string
-		pluginContext := a.PluginContext()
-		pluginsEnvironment.RunMultiPluginHook(func(hooks plugin.Hooks) bool {
-			rejectionReason = hooks.UserWillLogIn(pluginContext, user)
-			return rejectionReason == ""
-		}, plugin.UserWillLogInId)
-
-		if rejectionReason != "" {
-			return nil, model.NewAppError("AuthenticateUserForLogin", "Login rejected by plugin: "+rejectionReason, nil, "", http.StatusBadRequest)
-		}
-
-		a.Srv.Go(func() {
-			pluginContext := a.PluginContext()
-			pluginsEnvironment.RunMultiPluginHook(func(hooks plugin.Hooks) bool {
-				hooks.UserHasLoggedIn(pluginContext, user)
-				return true
-			}, plugin.UserHasLoggedInId)
-		})
-	}
-
 	return user, nil
 }
 
@@ -126,6 +105,19 @@ func (a *App) GetUserForLogin(id, loginId string) (*model.User, *model.AppError)
 }
 
 func (a *App) DoLogin(w http.ResponseWriter, r *http.Request, user *model.User, deviceId string) (*model.Session, *model.AppError) {
+	if pluginsEnvironment := a.GetPluginsEnvironment(); pluginsEnvironment != nil {
+		var rejectionReason string
+		pluginContext := a.PluginContext()
+		pluginsEnvironment.RunMultiPluginHook(func(hooks plugin.Hooks) bool {
+			rejectionReason = hooks.UserWillLogIn(pluginContext, user)
+			return rejectionReason == ""
+		}, plugin.UserWillLogInId)
+
+		if rejectionReason != "" {
+			return nil, model.NewAppError("DoLogin", "Login rejected by plugin: "+rejectionReason, nil, "", http.StatusBadRequest)
+		}
+	}
+
 	session := &model.Session{UserId: user.Id, Roles: user.GetRawRoles(), DeviceId: deviceId, IsOAuth: false}
 	session.GenerateCSRF()
 	maxAge := *a.Config().ServiceSettings.SessionLengthWebInDays * 60 * 60 * 24
@@ -203,6 +195,16 @@ func (a *App) DoLogin(w http.ResponseWriter, r *http.Request, user *model.User,
 	http.SetCookie(w, userCookie)
 	http.SetCookie(w, csrfCookie)
 
+	if pluginsEnvironment := a.GetPluginsEnvironment(); pluginsEnvironment != nil {
+		a.Srv.Go(func() {
+			pluginContext := a.PluginContext()
+			pluginsEnvironment.RunMultiPluginHook(func(hooks plugin.Hooks) bool {
+				hooks.UserHasLoggedIn(pluginContext, user)
+				return true
+			}, plugin.UserHasLoggedInId)
+		})
+	}
+
 	return session, nil
 }
 

app/oauth.go

@@ -18,7 +18,6 @@ import (
 	"github.com/mattermost/mattermost-server/einterfaces"
 	"github.com/mattermost/mattermost-server/mlog"
 	"github.com/mattermost/mattermost-server/model"
-	"github.com/mattermost/mattermost-server/plugin"
 	"github.com/mattermost/mattermost-server/store"
 	"github.com/mattermost/mattermost-server/utils"
 )
@@ -564,27 +563,6 @@ func (a *App) LoginByOAuth(service string, userData io.Reader, teamId string) (*
 		return nil, err
 	}
 
-	if pluginsEnvironment := a.GetPluginsEnvironment(); pluginsEnvironment != nil {
-		var rejectionReason string
-		pluginContext := a.PluginContext()
-		pluginsEnvironment.RunMultiPluginHook(func(hooks plugin.Hooks) bool {
-			rejectionReason = hooks.UserWillLogIn(pluginContext, user)
-			return rejectionReason == ""
-		}, plugin.UserWillLogInId)
-
-		if rejectionReason != "" {
-			return nil, model.NewAppError("LoginByOAuth", "Login rejected by plugin: "+rejectionReason, nil, "", http.StatusBadRequest)
-		}
-
-		a.Srv.Go(func() {
-			pluginContext := a.PluginContext()
-			pluginsEnvironment.RunMultiPluginHook(func(hooks plugin.Hooks) bool {
-				hooks.UserHasLoggedIn(pluginContext, user)
-				return true
-			}, plugin.UserHasLoggedInId)
-		})
-	}
-
 	return user, nil
 }
 

app/plugin_hooks_test.go

@@ -7,9 +7,12 @@ import (
 	"bytes"
 	"io"
 	"io/ioutil"
+	"net/http"
+	"net/http/httptest"
 	"os"
 	"os/exec"
 	"path/filepath"
+	"strings"
 	"testing"
 	"time"
 
@@ -706,14 +709,12 @@ func TestUserWillLogIn_Blocked(t *testing.T) {
 	`}, th.App, th.App.NewPluginAPI)
 	defer tearDown()
 
-	user, err := th.App.AuthenticateUserForLogin("", th.BasicUser.Email, "hunter2", "", false)
+	r := &http.Request{}
+	w := httptest.NewRecorder()
+	_, err = th.App.DoLogin(w, r, th.BasicUser, "")
 
-	if user != nil {
+	if !strings.HasPrefix(err.Id, "Login rejected by plugin") {
-		t.Errorf("Expected nil, got %+v", user)
+		t.Errorf("Expected Login rejected by plugin, got %s", err.Id)
-	}
-
-	if err == nil {
-		t.Errorf("Expected err, got nil")
 	}
 }
 
@@ -751,15 +752,17 @@ func TestUserWillLogInIn_Passed(t *testing.T) {
 	`}, th.App, th.App.NewPluginAPI)
 	defer tearDown()
 
-	user, err := th.App.AuthenticateUserForLogin("", th.BasicUser.Email, "hunter2", "", false)
+	r := &http.Request{}
-
+	w := httptest.NewRecorder()
-	if user == nil {
+	session, err := th.App.DoLogin(w, r, th.BasicUser, "")
-		t.Errorf("Expected user object, got nil")
-	}
 
 	if err != nil {
 		t.Errorf("Expected nil, got %s", err)
 	}
+
+	if session.UserId != th.BasicUser.Id {
+		t.Errorf("Expected %s, got %s", th.BasicUser.Id, session.UserId)
+	}
 }
 
 func TestUserHasLoggedIn(t *testing.T) {
@@ -797,11 +800,9 @@ func TestUserHasLoggedIn(t *testing.T) {
 	`}, th.App, th.App.NewPluginAPI)
 	defer tearDown()
 
-	user, err := th.App.AuthenticateUserForLogin("", th.BasicUser.Email, "hunter2", "", false)
+	r := &http.Request{}
-
+	w := httptest.NewRecorder()
-	if user == nil {
+	_, err = th.App.DoLogin(w, r, th.BasicUser, "")
-		t.Errorf("Expected user object, got nil")
-	}
 
 	if err != nil {
 		t.Errorf("Expected nil, got %s", err)
@@ -809,7 +810,7 @@ func TestUserHasLoggedIn(t *testing.T) {
 
 	time.Sleep(2 * time.Second)
 
-	user, _ = th.App.GetUser(th.BasicUser.Id)
+	user, _ := th.App.GetUser(th.BasicUser.Id)
 
 	if user.FirstName != "plugin-callback-success" {
 		t.Errorf("Expected firstname overwrite, got default")