From 4c50c7c59b6c1b0c75a5aaa2661d4c77144c9067 Mon Sep 17 00:00:00 2001 From: Hossein Ahmadian-Yazdi Date: Tue, 30 Jun 2020 10:34:05 -0400 Subject: [PATCH] [MM-25780] Fix incorrect session length when logging in through mobile using SSO (#14874) * Pass device ID * dont use device id as way of detecting * fix spelling mistake * update layers * fix test * fix linting * save schema * put columns in correct place * fix linting * update * upgrade go change * use props * fix stuff * update session tests * address PR comments * address PR comments --- api4/user.go | 2 +- app/app_iface.go | 4 ++-- app/login.go | 12 ++++++++++-- app/oauth.go | 4 +++- app/opentracing_layer.go | 8 ++++---- app/plugin_hooks_test.go | 6 +++--- app/session.go | 2 +- app/session_test.go | 42 ++++++++++++++++++++++++++++++++++++++++ model/saml.go | 2 ++ model/session.go | 35 ++++++++++++++++++++++++++++++++- web/oauth.go | 11 ++++++++--- web/saml.go | 10 +++++++++- 12 files changed, 119 insertions(+), 19 deletions(-) diff --git a/api4/user.go b/api4/user.go index d3fea45ded..33084d53b4 100644 --- a/api4/user.go +++ b/api4/user.go @@ -1564,7 +1564,7 @@ func login(c *Context, w http.ResponseWriter, r *http.Request) { c.LogAuditWithUserId(user.Id, "authenticated") - err = c.App.DoLogin(w, r, user, deviceId) + err = c.App.DoLogin(w, r, user, deviceId, false, false, false) if err != nil { c.Err = err return diff --git a/app/app_iface.go b/app/app_iface.go index 421ba8c156..03214e602a 100644 --- a/app/app_iface.go +++ b/app/app_iface.go @@ -457,7 +457,7 @@ type AppIface interface { DoEmojisPermissionsMigration() DoGuestRolesCreationMigration() DoLocalRequest(rawURL string, body []byte) (*http.Response, *model.AppError) - DoLogin(w http.ResponseWriter, r *http.Request, user *model.User, deviceId string) *model.AppError + DoLogin(w http.ResponseWriter, r *http.Request, user *model.User, deviceId string, isMobile, isOAuth, isSaml bool) *model.AppError DoPostAction(postId, actionId, userId, selectedOption string) (string, *model.AppError) DoPostActionWithCookie(postId, actionId, userId, selectedOption string, cookie *model.PostActionCookie) (string, *model.AppError) DoUploadFile(now time.Time, rawTeamId string, rawChannelId string, rawUserId string, rawFilename string, data []byte) (*model.FileInfo, *model.AppError) @@ -579,7 +579,7 @@ type AppIface interface { GetOAuthAppsByCreator(userId string, page, perPage int) ([]*model.OAuthApp, *model.AppError) GetOAuthCodeRedirect(userId string, authRequest *model.AuthorizeRequest) (string, *model.AppError) GetOAuthImplicitRedirect(userId string, authRequest *model.AuthorizeRequest) (string, *model.AppError) - GetOAuthLoginEndpoint(w http.ResponseWriter, r *http.Request, service, teamId, action, redirectTo, loginHint string) (string, *model.AppError) + GetOAuthLoginEndpoint(w http.ResponseWriter, r *http.Request, service, teamId, action, redirectTo, loginHint string, isMobile bool) (string, *model.AppError) GetOAuthSignupEndpoint(w http.ResponseWriter, r *http.Request, service, teamId string) (string, *model.AppError) GetOAuthStateToken(token string) (*model.Token, *model.AppError) GetOpenGraphMetadata(requestURL string) *opengraph.OpenGraph diff --git a/app/login.go b/app/login.go index 0a4b9dc032..135fb29d47 100644 --- a/app/login.go +++ b/app/login.go @@ -6,6 +6,7 @@ package app import ( "fmt" "net/http" + "strconv" "strings" "time" @@ -110,7 +111,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.AppError { +func (a *App) DoLogin(w http.ResponseWriter, r *http.Request, user *model.User, deviceId string, isMobile, isOAuth, isSaml bool) *model.AppError { if pluginsEnvironment := a.GetPluginsEnvironment(); pluginsEnvironment != nil { var rejectionReason string pluginContext := a.PluginContext() @@ -124,7 +125,10 @@ func (a *App) DoLogin(w http.ResponseWriter, r *http.Request, user *model.User, } } - session := &model.Session{UserId: user.Id, Roles: user.GetRawRoles(), DeviceId: deviceId, IsOAuth: false} + session := &model.Session{UserId: user.Id, Roles: user.GetRawRoles(), DeviceId: deviceId, IsOAuth: isOAuth, Props: map[string]string{ + model.USER_AUTH_SERVICE_IS_MOBILE: strconv.FormatBool(isMobile), + model.USER_AUTH_SERVICE_IS_SAML: strconv.FormatBool(isSaml), + }} session.GenerateCSRF() if len(deviceId) > 0 { @@ -135,6 +139,10 @@ func (a *App) DoLogin(w http.ResponseWriter, r *http.Request, user *model.User, err.StatusCode = http.StatusInternalServerError return err } + } else if isMobile { + session.SetExpireInDays(*a.Config().ServiceSettings.SessionLengthMobileInDays) + } else if isOAuth || isSaml { + session.SetExpireInDays(*a.Config().ServiceSettings.SessionLengthSSOInDays) } else { session.SetExpireInDays(*a.Config().ServiceSettings.SessionLengthWebInDays) } diff --git a/app/oauth.go b/app/oauth.go index e00be718e0..dfd1f35e11 100644 --- a/app/oauth.go +++ b/app/oauth.go @@ -350,7 +350,7 @@ func (a *App) newSessionUpdateToken(appName string, accessData *model.AccessData return accessRsp, nil } -func (a *App) GetOAuthLoginEndpoint(w http.ResponseWriter, r *http.Request, service, teamId, action, redirectTo, loginHint string) (string, *model.AppError) { +func (a *App) GetOAuthLoginEndpoint(w http.ResponseWriter, r *http.Request, service, teamId, action, redirectTo, loginHint string, isMobile bool) (string, *model.AppError) { stateProps := map[string]string{} stateProps["action"] = action if len(teamId) != 0 { @@ -361,6 +361,8 @@ func (a *App) GetOAuthLoginEndpoint(w http.ResponseWriter, r *http.Request, serv stateProps["redirect_to"] = redirectTo } + stateProps[model.USER_AUTH_SERVICE_IS_MOBILE] = strconv.FormatBool(isMobile) + authUrl, err := a.GetAuthorizationCode(w, r, service, stateProps, loginHint) if err != nil { return "", err diff --git a/app/opentracing_layer.go b/app/opentracing_layer.go index 56c31bd17f..5c53dc904d 100644 --- a/app/opentracing_layer.go +++ b/app/opentracing_layer.go @@ -3088,7 +3088,7 @@ func (a *OpenTracingAppLayer) DoLocalRequest(rawURL string, body []byte) (*http. return resultVar0, resultVar1 } -func (a *OpenTracingAppLayer) DoLogin(w http.ResponseWriter, r *http.Request, user *model.User, deviceId string) *model.AppError { +func (a *OpenTracingAppLayer) DoLogin(w http.ResponseWriter, r *http.Request, user *model.User, deviceId string, isMobile bool, isOAuth bool, isSaml bool) *model.AppError { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.DoLogin") @@ -3100,7 +3100,7 @@ func (a *OpenTracingAppLayer) DoLogin(w http.ResponseWriter, r *http.Request, us }() defer span.Finish() - resultVar0 := a.app.DoLogin(w, r, user, deviceId) + resultVar0 := a.app.DoLogin(w, r, user, deviceId, isMobile, isOAuth, isSaml) if resultVar0 != nil { span.LogFields(spanlog.Error(resultVar0)) @@ -6215,7 +6215,7 @@ func (a *OpenTracingAppLayer) GetOAuthImplicitRedirect(userId string, authReques return resultVar0, resultVar1 } -func (a *OpenTracingAppLayer) GetOAuthLoginEndpoint(w http.ResponseWriter, r *http.Request, service string, teamId string, action string, redirectTo string, loginHint string) (string, *model.AppError) { +func (a *OpenTracingAppLayer) GetOAuthLoginEndpoint(w http.ResponseWriter, r *http.Request, service string, teamId string, action string, redirectTo string, loginHint string, isMobile bool) (string, *model.AppError) { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.GetOAuthLoginEndpoint") @@ -6227,7 +6227,7 @@ func (a *OpenTracingAppLayer) GetOAuthLoginEndpoint(w http.ResponseWriter, r *ht }() defer span.Finish() - resultVar0, resultVar1 := a.app.GetOAuthLoginEndpoint(w, r, service, teamId, action, redirectTo, loginHint) + resultVar0, resultVar1 := a.app.GetOAuthLoginEndpoint(w, r, service, teamId, action, redirectTo, loginHint, isMobile) if resultVar1 != nil { span.LogFields(spanlog.Error(resultVar1)) diff --git a/app/plugin_hooks_test.go b/app/plugin_hooks_test.go index 21e5eb124c..355459bbcc 100644 --- a/app/plugin_hooks_test.go +++ b/app/plugin_hooks_test.go @@ -696,7 +696,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, "", false, false, false) assert.Contains(t, err.Id, "Login rejected by plugin", "Expected Login rejected by plugin, got %s", err.Id) } @@ -735,7 +735,7 @@ func TestUserWillLogInIn_Passed(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, "", false, false, false) assert.Nil(t, err, "Expected nil, got %s", err) assert.Equal(t, th.App.Session().UserId, th.BasicUser.Id) @@ -776,7 +776,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, "", false, false, false) assert.Nil(t, err, "Expected nil, got %s", err) diff --git a/app/session.go b/app/session.go index a9c8b028f3..a37acdee7d 100644 --- a/app/session.go +++ b/app/session.go @@ -348,7 +348,7 @@ func (a *App) GetSessionLengthInMillis(session *model.Session) int64 { var days int if session.IsMobileApp() { days = *a.Config().ServiceSettings.SessionLengthMobileInDays - } else if session.IsOAuth { + } else if session.IsSSOLogin() { days = *a.Config().ServiceSettings.SessionLengthSSOInDays } else { days = *a.Config().ServiceSettings.SessionLengthWebInDays diff --git a/app/session_test.go b/app/session_test.go index ea6ed8f45c..d671f2b8c6 100644 --- a/app/session_test.go +++ b/app/session_test.go @@ -206,6 +206,35 @@ func TestApp_GetSessionLengthInMillis(t *testing.T) { require.Equal(t, dayMillis*3, sessionLength) }) + t.Run("get session length mobile when isMobile in props is set", func(t *testing.T) { + session := &model.Session{ + UserId: model.NewId(), + Props: map[string]string{ + model.USER_AUTH_SERVICE_IS_MOBILE: "true", + }, + } + session, err := th.App.CreateSession(session) + require.Nil(t, err) + + sessionLength := th.App.GetSessionLengthInMillis(session) + require.Equal(t, dayMillis*3, sessionLength) + }) + + t.Run("get session length mobile when isMobile in props is set and takes priority over saml", func(t *testing.T) { + session := &model.Session{ + UserId: model.NewId(), + Props: map[string]string{ + model.USER_AUTH_SERVICE_IS_MOBILE: "true", + model.USER_AUTH_SERVICE_IS_SAML: "true", + }, + } + session, err := th.App.CreateSession(session) + require.Nil(t, err) + + sessionLength := th.App.GetSessionLengthInMillis(session) + require.Equal(t, dayMillis*3, sessionLength) + }) + t.Run("get session length SSO", func(t *testing.T) { session := &model.Session{ UserId: model.NewId(), @@ -218,6 +247,19 @@ func TestApp_GetSessionLengthInMillis(t *testing.T) { require.Equal(t, dayMillis*2, sessionLength) }) + t.Run("get session length SSO using props", func(t *testing.T) { + session := &model.Session{ + UserId: model.NewId(), + Props: map[string]string{ + model.USER_AUTH_SERVICE_IS_SAML: "true", + }} + session, err := th.App.CreateSession(session) + require.Nil(t, err) + + sessionLength := th.App.GetSessionLengthInMillis(session) + require.Equal(t, dayMillis*2, sessionLength) + }) + t.Run("get session length web/LDAP", func(t *testing.T) { session := &model.Session{ UserId: model.NewId(), diff --git a/model/saml.go b/model/saml.go index 2f289ffd51..59ac2acc34 100644 --- a/model/saml.go +++ b/model/saml.go @@ -13,6 +13,8 @@ import ( const ( USER_AUTH_SERVICE_SAML = "saml" USER_AUTH_SERVICE_SAML_TEXT = "SAML" + USER_AUTH_SERVICE_IS_SAML = "isSaml" + USER_AUTH_SERVICE_IS_MOBILE = "isMobile" ) type SamlAuthRequest struct { diff --git a/model/session.go b/model/session.go index fa53e04829..1cbb276062 100644 --- a/model/session.go +++ b/model/session.go @@ -6,7 +6,10 @@ package model import ( "encoding/json" "io" + "strconv" "strings" + + "github.com/mattermost/mattermost-server/v5/mlog" ) const ( @@ -140,7 +143,37 @@ func (me *Session) GetTeamByTeamId(teamId string) *TeamMember { } func (me *Session) IsMobileApp() bool { - return len(me.DeviceId) > 0 + return len(me.DeviceId) > 0 || me.IsMobile() +} + +func (me *Session) IsMobile() bool { + val, ok := me.Props[USER_AUTH_SERVICE_IS_MOBILE] + if !ok { + return false + } + isMobile, err := strconv.ParseBool(val) + if err != nil { + mlog.Error("Error parsing boolean property from Session", mlog.Err(err)) + return false + } + return isMobile +} + +func (me *Session) IsSaml() bool { + val, ok := me.Props[USER_AUTH_SERVICE_IS_SAML] + if !ok { + return false + } + isSaml, err := strconv.ParseBool(val) + if err != nil { + mlog.Error("Error parsing boolean property from Session", mlog.Err(err)) + return false + } + return isSaml +} + +func (me *Session) IsSSOLogin() bool { + return me.IsOAuth || me.IsSaml() } func (me *Session) GetUserRoles() []string { diff --git a/web/oauth.go b/web/oauth.go index f842d5b571..87bb83cc33 100644 --- a/web/oauth.go +++ b/web/oauth.go @@ -7,6 +7,7 @@ import ( "net/http" "net/url" "path/filepath" + "strconv" "strings" "github.com/mattermost/mattermost-server/v5/app" @@ -301,7 +302,11 @@ 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 { - err = c.App.DoLogin(w, r, user, "") + isMobile, parseErr := strconv.ParseBool(props[model.USER_AUTH_SERVICE_IS_MOBILE]) + if parseErr != nil { + mlog.Error("Error parsing boolean property from props", mlog.Err(parseErr)) + } + err = c.App.DoLogin(w, r, user, "", isMobile, true, false) if err != nil { err.Translate(c.App.T) c.Err = err @@ -343,7 +348,7 @@ func loginWithOAuth(c *Context, w http.ResponseWriter, r *http.Request) { return } - authUrl, err := c.App.GetOAuthLoginEndpoint(w, r, c.Params.Service, teamId, model.OAUTH_ACTION_LOGIN, redirectTo, loginHint) + authUrl, err := c.App.GetOAuthLoginEndpoint(w, r, c.Params.Service, teamId, model.OAUTH_ACTION_LOGIN, redirectTo, loginHint, false) if err != nil { c.Err = err return @@ -364,7 +369,7 @@ func mobileLoginWithOAuth(c *Context, w http.ResponseWriter, r *http.Request) { return } - authUrl, err := c.App.GetOAuthLoginEndpoint(w, r, c.Params.Service, teamId, model.OAUTH_ACTION_MOBILE, "", "") + authUrl, err := c.App.GetOAuthLoginEndpoint(w, r, c.Params.Service, teamId, model.OAUTH_ACTION_MOBILE, "", "", true) if err != nil { c.Err = err return diff --git a/web/saml.go b/web/saml.go index bf917dd021..823858e016 100644 --- a/web/saml.go +++ b/web/saml.go @@ -6,6 +6,7 @@ package web import ( b64 "encoding/base64" "net/http" + "strconv" "strings" "github.com/mattermost/mattermost-server/v5/audit" @@ -32,6 +33,7 @@ func loginWithSaml(c *Context, w http.ResponseWriter, r *http.Request) { return } action := r.URL.Query().Get("action") + isMobile := action == model.OAUTH_ACTION_MOBILE redirectTo := r.URL.Query().Get("redirect_to") relayProps := map[string]string{} relayState := "" @@ -48,6 +50,8 @@ func loginWithSaml(c *Context, w http.ResponseWriter, r *http.Request) { relayProps["redirect_to"] = redirectTo } + relayProps[model.USER_AUTH_SERVICE_IS_MOBILE] = strconv.FormatBool(isMobile) + if len(relayProps) > 0 { relayState = b64.StdEncoding.EncodeToString([]byte(model.MapToJson(relayProps))) } @@ -142,7 +146,11 @@ func completeSaml(c *Context, w http.ResponseWriter, r *http.Request) { auditRec.AddMeta("obtained_user_id", user.Id) c.LogAuditWithUserId(user.Id, "obtained user") - err = c.App.DoLogin(w, r, user, "") + isMobile, parseErr := strconv.ParseBool(relayProps[model.USER_AUTH_SERVICE_IS_MOBILE]) + if parseErr != nil { + mlog.Error("Error parsing boolean property from relay props", mlog.Err(parseErr)) + } + err = c.App.DoLogin(w, r, user, "", isMobile, false, true) if err != nil { c.Err = err return