From 422f377c964086f689e57cc02ecb0e56a3fd4973 Mon Sep 17 00:00:00 2001 From: Claudio Costa Date: Thu, 31 Oct 2019 12:50:43 +0100 Subject: [PATCH] [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 | 4 +- app/login.go | 19 +++++---- app/plugin_hooks_test.go | 8 ++-- go.mod | 2 + web/oauth.go | 6 +-- web/saml.go | 91 ++++++++++++++++++++-------------------- 6 files changed, 65 insertions(+), 65 deletions(-) diff --git a/api4/user.go b/api4/user.go index 81fda670f5..06c44975fc 100644 --- a/api4/user.go +++ b/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) diff --git a/app/login.go b/app/login.go index 31f1126fdd..80584e6b51 100644 --- a/app/login.go +++ b/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, diff --git a/app/plugin_hooks_test.go b/app/plugin_hooks_test.go index 2feabc26de..d591a8d136 100644 --- a/app/plugin_hooks_test.go +++ b/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) diff --git a/go.mod b/go.mod index 9f2699c127..2c1e6b3078 100644 --- a/go.mod +++ b/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 diff --git a/web/oauth.go b/web/oauth.go index 5e971c9f5a..e99524c429 100644 --- a/web/oauth.go +++ b/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.Session = *session + c.App.AttachSessionCookies(w, r) if _, ok := props["redirect_to"]; ok { redirectUrl = props["redirect_to"] diff --git a/web/saml.go b/web/saml.go index 38185b8254..efdd6fa4f9 100644 --- a/web/saml.go +++ b/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 { - case model.OAUTH_ACTION_SIGNUP: - teamId := relayProps["team_id"] - if len(teamId) > 0 { - c.App.Srv.Go(func() { - 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: - if err := c.App.RevokeAllSessions(user.Id); err != nil { - c.Err = err - return - } - c.LogAuditWithUserId(user.Id, "Revoked all sessions for user") + if err = c.App.CheckUserAllAuthenticationCriteria(user, ""); err != nil { + c.Err = err + c.Err.StatusCode = http.StatusFound + return + } + + switch action { + case model.OAUTH_ACTION_SIGNUP: + teamId := relayProps["team_id"] + if len(teamId) > 0 { 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) } }) } - - c.LogAuditWithUserId(user.Id, "obtained user") - - session, err := c.App.DoLogin(w, r, user, "") - if err != nil { + 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 { + 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 { - http.Redirect(w, r, c.GetSiteURLHeader()+val, http.StatusFound) - return - } + c.App.AttachSessionCookies(w, r) - switch action { - 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) - } + if val, ok := relayProps["redirect_to"]; ok { + http.Redirect(w, r, c.GetSiteURLHeader()+val, http.StatusFound) + return + } + + switch action { + 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) } }