[MM-19473] Fix data race on user login (#12870)

* Avoid writes to App.Session outside the app layer

* Fix merge

* Remove unneeded else condition

api4/user.go

@@ -1399,7 +1399,7 @@ func login(c *Context, w http.ResponseWriter, r *http.Request) {
 
 	c.LogAuditWithUserId(user.Id, "authenticated")
 
-	session, err := c.App.DoLogin(w, r, user, deviceId)
+	err = c.App.DoLogin(w, r, user, deviceId)
 	if err != nil {
 		c.Err = err
 		return
@@ -1408,7 +1408,7 @@ func login(c *Context, w http.ResponseWriter, r *http.Request) {
 	c.LogAuditWithUserId(user.Id, "success")
 
 	if r.Header.Get(model.HEADER_REQUESTED_WITH) == model.HEADER_REQUESTED_WITH_XML {
-		c.App.AttachSessionCookies(w, r, session)
+		c.App.AttachSessionCookies(w, r)
 	}
 
 	userTermsOfService, err := c.App.GetUserTermsOfService(user.Id)

app/login.go

@@ -110,7 +110,7 @@ func (a *App) GetUserForLogin(id, loginId string) (*model.User, *model.AppError)
 	return nil, model.NewAppError("GetUserForLogin", "store.sql_user.get_for_login.app_error", nil, "", http.StatusBadRequest)
 }
 
-func (a *App) DoLogin(w http.ResponseWriter, r *http.Request, user *model.User, deviceId string) (*model.Session, *model.AppError) {
+func (a *App) DoLogin(w http.ResponseWriter, r *http.Request, user *model.User, deviceId string) *model.AppError {
 	if pluginsEnvironment := a.GetPluginsEnvironment(); pluginsEnvironment != nil {
 		var rejectionReason string
 		pluginContext := a.PluginContext()
@@ -120,7 +120,7 @@ func (a *App) DoLogin(w http.ResponseWriter, r *http.Request, user *model.User,
 		}, plugin.UserWillLogInId)
 
 		if rejectionReason != "" {
-			return nil, model.NewAppError("DoLogin", "Login rejected by plugin: "+rejectionReason, nil, "", http.StatusBadRequest)
+			return model.NewAppError("DoLogin", "Login rejected by plugin: "+rejectionReason, nil, "", http.StatusBadRequest)
 		}
 	}
 
@@ -133,7 +133,7 @@ func (a *App) DoLogin(w http.ResponseWriter, r *http.Request, user *model.User,
 		// A special case where we logout of all other sessions with the same Id
 		if err := a.RevokeSessionsForDeviceId(user.Id, deviceId, ""); err != nil {
 			err.StatusCode = http.StatusInternalServerError
-			return nil, err
+			return err
 		}
 	} else {
 		session.SetExpireInDays(*a.Config().ServiceSettings.SessionLengthWebInDays)
@@ -158,10 +158,11 @@ func (a *App) DoLogin(w http.ResponseWriter, r *http.Request, user *model.User,
 	var err *model.AppError
 	if session, err = a.CreateSession(session); err != nil {
 		err.StatusCode = http.StatusInternalServerError
-		return nil, err
+		return err
 	}
 
 	w.Header().Set(model.HEADER_TOKEN, session.Token)
+
 	a.Session = *session
 
 	if pluginsEnvironment := a.GetPluginsEnvironment(); pluginsEnvironment != nil {
@@ -174,10 +175,10 @@ func (a *App) DoLogin(w http.ResponseWriter, r *http.Request, user *model.User,
 		})
 	}
 
-	return session, nil
+	return nil
 }
 
-func (a *App) AttachSessionCookies(w http.ResponseWriter, r *http.Request, session *model.Session) {
+func (a *App) AttachSessionCookies(w http.ResponseWriter, r *http.Request) {
 	secure := false
 	if GetProtocol(r) == "https" {
 		secure = true
@@ -190,7 +191,7 @@ func (a *App) AttachSessionCookies(w http.ResponseWriter, r *http.Request, sessi
 	expiresAt := time.Unix(model.GetMillis()/1000+int64(maxAge), 0)
 	sessionCookie := &http.Cookie{
 		Name:     model.SESSION_COOKIE_TOKEN,
-		Value:    session.Token,
+		Value:    a.Session.Token,
 		Path:     subpath,
 		MaxAge:   maxAge,
 		Expires:  expiresAt,
@@ -201,7 +202,7 @@ func (a *App) AttachSessionCookies(w http.ResponseWriter, r *http.Request, sessi
 
 	userCookie := &http.Cookie{
 		Name:    model.SESSION_COOKIE_USER,
-		Value:   session.UserId,
+		Value:   a.Session.UserId,
 		Path:    subpath,
 		MaxAge:  maxAge,
 		Expires: expiresAt,
@@ -211,7 +212,7 @@ func (a *App) AttachSessionCookies(w http.ResponseWriter, r *http.Request, sessi
 
 	csrfCookie := &http.Cookie{
 		Name:    model.SESSION_COOKIE_CSRF,
-		Value:   session.GetCSRF(),
+		Value:   a.Session.GetCSRF(),
 		Path:    subpath,
 		MaxAge:  maxAge,
 		Expires: expiresAt,

app/plugin_hooks_test.go

@@ -694,7 +694,7 @@ func TestUserWillLogIn_Blocked(t *testing.T) {
 
 	r := &http.Request{}
 	w := httptest.NewRecorder()
-	_, err = th.App.DoLogin(w, r, th.BasicUser, "")
+	err = th.App.DoLogin(w, r, th.BasicUser, "")
 
 	assert.Contains(t, err.Id, "Login rejected by plugin", "Expected Login rejected by plugin, got %s", err.Id)
 }
@@ -733,10 +733,10 @@ func TestUserWillLogInIn_Passed(t *testing.T) {
 
 	r := &http.Request{}
 	w := httptest.NewRecorder()
-	session, err := th.App.DoLogin(w, r, th.BasicUser, "")
+	err = th.App.DoLogin(w, r, th.BasicUser, "")
 
 	assert.Nil(t, err, "Expected nil, got %s", err)
-	assert.Equal(t, session.UserId, th.BasicUser.Id)
+	assert.Equal(t, th.App.Session.UserId, th.BasicUser.Id)
 }
 
 func TestUserHasLoggedIn(t *testing.T) {
@@ -774,7 +774,7 @@ func TestUserHasLoggedIn(t *testing.T) {
 
 	r := &http.Request{}
 	w := httptest.NewRecorder()
-	_, err = th.App.DoLogin(w, r, th.BasicUser, "")
+	err = th.App.DoLogin(w, r, th.BasicUser, "")
 
 	assert.Nil(t, err, "Expected nil, got %s", err)
 

go.mod

@@ -52,6 +52,8 @@ require (
 	github.com/miekg/dns v1.1.19 // indirect
 	github.com/minio/minio-go/v6 v6.0.38
 	github.com/mitchellh/go-testing-interface v1.0.0 // indirect
+	github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
+	github.com/modern-go/reflect2 v1.0.1 // indirect
 	github.com/muesli/smartcrop v0.3.0 // indirect
 	github.com/olekukonko/tablewriter v0.0.1 // indirect
 	github.com/onsi/ginkgo v1.8.0 // indirect

web/oauth.go

@@ -283,7 +283,7 @@ func completeOAuth(c *Context, w http.ResponseWriter, r *http.Request) {
 	} else if action == model.OAUTH_ACTION_SSO_TO_EMAIL {
 		redirectUrl = app.GetProtocol(r) + "://" + r.Host + "/claim?email=" + url.QueryEscape(props["email"])
 	} else {
-		session, err := c.App.DoLogin(w, r, user, "")
+		err = c.App.DoLogin(w, r, user, "")
 		if err != nil {
 			err.Translate(c.App.T)
 			c.Err = err
@@ -293,9 +293,7 @@ func completeOAuth(c *Context, w http.ResponseWriter, r *http.Request) {
 			return
 		}
 
-		c.App.AttachSessionCookies(w, r, session)
+		c.App.AttachSessionCookies(w, r)
-
-		c.App.Session = *session
 
 		if _, ok := props["redirect_to"]; ok {
 			redirectUrl = props["redirect_to"]

web/saml.go

@@ -87,7 +87,8 @@ func completeSaml(c *Context, w http.ResponseWriter, r *http.Request) {
 	c.LogAudit("attempt")
 
 	action := relayProps["action"]
-	if user, err := samlInterface.DoLogin(encodedXML, relayProps); err != nil {
+	user, err := samlInterface.DoLogin(encodedXML, relayProps)
+	if err != nil {
 		c.LogAudit("fail")
 
 		if action == model.OAUTH_ACTION_MOBILE {
@@ -98,64 +99,62 @@ func completeSaml(c *Context, w http.ResponseWriter, r *http.Request) {
 			c.Err.StatusCode = http.StatusFound
 		}
 		return
-	} else {
+	}
-		if err := c.App.CheckUserAllAuthenticationCriteria(user, ""); err != nil {
-			c.Err = err
-			c.Err.StatusCode = http.StatusFound
-			return
-		}
 
-		switch action {
+	if err = c.App.CheckUserAllAuthenticationCriteria(user, ""); err != nil {
-		case model.OAUTH_ACTION_SIGNUP:
+		c.Err = err
-			teamId := relayProps["team_id"]
+		c.Err.StatusCode = http.StatusFound
-			if len(teamId) > 0 {
+		return
-				c.App.Srv.Go(func() {
+	}
-					if err := c.App.AddUserToTeamByTeamId(teamId, user); err != nil {
+
-						mlog.Error(err.Error())
+	switch action {
-					} else {
+	case model.OAUTH_ACTION_SIGNUP:
-						c.App.AddDirectChannels(teamId, user)
+		teamId := relayProps["team_id"]
-					}
+		if len(teamId) > 0 {
-				})
-			}
-		case model.OAUTH_ACTION_EMAIL_TO_SSO:
-			if err := c.App.RevokeAllSessions(user.Id); err != nil {
-				c.Err = err
-				return
-			}
-			c.LogAuditWithUserId(user.Id, "Revoked all sessions for user")
 			c.App.Srv.Go(func() {
-				if err := c.App.SendSignInChangeEmail(user.Email, strings.Title(model.USER_AUTH_SERVICE_SAML)+" SSO", user.Locale, c.App.GetSiteURL()); err != nil {
+				if err = c.App.AddUserToTeamByTeamId(teamId, user); err != nil {
 					mlog.Error(err.Error())
+				} else {
+					c.App.AddDirectChannels(teamId, user)
 				}
 			})
 		}
-
+	case model.OAUTH_ACTION_EMAIL_TO_SSO:
-		c.LogAuditWithUserId(user.Id, "obtained user")
+		if err = c.App.RevokeAllSessions(user.Id); err != nil {
-
-		session, err := c.App.DoLogin(w, r, user, "")
-		if err != nil {
 			c.Err = err
 			return
 		}
+		c.LogAuditWithUserId(user.Id, "Revoked all sessions for user")
+		c.App.Srv.Go(func() {
+			if err = c.App.SendSignInChangeEmail(user.Email, strings.Title(model.USER_AUTH_SERVICE_SAML)+" SSO", user.Locale, c.App.GetSiteURL()); err != nil {
+				mlog.Error(err.Error())
+			}
+		})
+	}
 
-		c.LogAuditWithUserId(user.Id, "success")
+	c.LogAuditWithUserId(user.Id, "obtained user")
 
-		c.App.AttachSessionCookies(w, r, session)
+	err = c.App.DoLogin(w, r, user, "")
+	if err != nil {
+		c.Err = err
+		return
+	}
 
-		c.App.Session = *session
+	c.LogAuditWithUserId(user.Id, "success")
 
-		if val, ok := relayProps["redirect_to"]; ok {
+	c.App.AttachSessionCookies(w, r)
-			http.Redirect(w, r, c.GetSiteURLHeader()+val, http.StatusFound)
-			return
-		}
 
-		switch action {
+	if val, ok := relayProps["redirect_to"]; ok {
-		case model.OAUTH_ACTION_MOBILE:
+		http.Redirect(w, r, c.GetSiteURLHeader()+val, http.StatusFound)
-			ReturnStatusOK(w)
+		return
-		case model.OAUTH_ACTION_EMAIL_TO_SSO:
+	}
-			http.Redirect(w, r, c.GetSiteURLHeader()+"/login?extra=signin_change", http.StatusFound)
+
-		default:
+	switch action {
-			http.Redirect(w, r, c.GetSiteURLHeader(), http.StatusFound)
+	case model.OAUTH_ACTION_MOBILE:
-		}
+		ReturnStatusOK(w)
+	case model.OAUTH_ACTION_EMAIL_TO_SSO:
+		http.Redirect(w, r, c.GetSiteURLHeader()+"/login?extra=signin_change", http.StatusFound)
+	default:
+		http.Redirect(w, r, c.GetSiteURLHeader(), http.StatusFound)
 	}
 }