From 7f64199a37b8ddbe722ba611cd6df4f1baba7fd3 Mon Sep 17 00:00:00 2001 From: Doug Lauder Date: Tue, 4 Aug 2020 16:10:37 -0400 Subject: [PATCH] MM-27184 deprecate model.SetExpireInDays (#15165) Mobile users were having their sessions unexpectedly expired, despite having ServiceSettings.ExtendSessionLengthWithActivity enabled. Every time a mobile app is opened it called `/api/v4/sessions/device` which calls attachDeviceId which calls `(*Session)SetExpireInDays`. This code above assumed the expiry should be relative to CreateAt which is incorrect when ExtendSessionLengthWithActivity is enabled. Therefore, every time the mobile app was opened, the maximum expiry was set in memory to CreateAt + session_length, even if the session was extended. (*Session)SetExpireInDays is now deprecated and replaced with (*App)SetSessionExpireInDays which takes into account the ExtendSessionLengthWithActivity setting. --- .gitignore | 5 ++- api4/user.go | 2 +- app/app_iface.go | 4 +++ app/login.go | 8 ++--- app/oauth.go | 2 +- app/oauth_test.go | 4 +-- app/opentracing/opentracing_layer.go | 15 ++++++++ app/session.go | 16 ++++++++- app/session_test.go | 52 ++++++++++++++++++++++++++++ model/session.go | 3 ++ web/handlers_test.go | 2 +- 11 files changed, 102 insertions(+), 11 deletions(-) diff --git a/.gitignore b/.gitignore index 022b1dcfa7..04d6a57ff3 100644 --- a/.gitignore +++ b/.gitignore @@ -36,7 +36,6 @@ imports/imports.go # Folders _obj _test -.vscode testfiles # Architecture specific extensions/prefixes @@ -73,6 +72,10 @@ Session.vim .netrwhist *~ +# VSCode project files +.vscode +*.code-workspace + # Gogland project files mattermost-server.iml diff --git a/api4/user.go b/api4/user.go index 3326115551..b2d14d1b67 100644 --- a/api4/user.go +++ b/api4/user.go @@ -1863,7 +1863,7 @@ func attachDeviceId(c *Context, w http.ResponseWriter, r *http.Request) { } c.App.ClearSessionCacheForUser(c.App.Session().UserId) - c.App.Session().SetExpireInDays(*c.App.Config().ServiceSettings.SessionLengthMobileInDays) + c.App.SetSessionExpireInDays(c.App.Session(), *c.App.Config().ServiceSettings.SessionLengthMobileInDays) maxAge := *c.App.Config().ServiceSettings.SessionLengthMobileInDays * 60 * 60 * 24 diff --git a/app/app_iface.go b/app/app_iface.go index 9a403e5133..da6ddecafe 100644 --- a/app/app_iface.go +++ b/app/app_iface.go @@ -275,6 +275,10 @@ type AppIface interface { SetBotIconImage(botUserId string, file io.ReadSeeker) *model.AppError // SetBotIconImageFromMultiPartFile sets LHS icon for a bot. SetBotIconImageFromMultiPartFile(botUserId string, imageData *multipart.FileHeader) *model.AppError + // SetSessionExpireInDays sets the session's expiry the specified number of days + // relative to either the session creation date or the current time, depending + // on the `ExtendSessionOnActivity` config setting. + SetSessionExpireInDays(session *model.Session, days int) // SetStatusLastActivityAt sets the last activity at for a user on the local app server and updates // status to away if needed. Used by the WS to set status to away if an 'online' device disconnects // while an 'away' device is still connected diff --git a/app/login.go b/app/login.go index 135fb29d47..2795f95777 100644 --- a/app/login.go +++ b/app/login.go @@ -132,7 +132,7 @@ func (a *App) DoLogin(w http.ResponseWriter, r *http.Request, user *model.User, session.GenerateCSRF() if len(deviceId) > 0 { - session.SetExpireInDays(*a.Config().ServiceSettings.SessionLengthMobileInDays) + a.SetSessionExpireInDays(session, *a.Config().ServiceSettings.SessionLengthMobileInDays) // A special case where we logout of all other sessions with the same Id if err := a.RevokeSessionsForDeviceId(user.Id, deviceId, ""); err != nil { @@ -140,11 +140,11 @@ func (a *App) DoLogin(w http.ResponseWriter, r *http.Request, user *model.User, return err } } else if isMobile { - session.SetExpireInDays(*a.Config().ServiceSettings.SessionLengthMobileInDays) + a.SetSessionExpireInDays(session, *a.Config().ServiceSettings.SessionLengthMobileInDays) } else if isOAuth || isSaml { - session.SetExpireInDays(*a.Config().ServiceSettings.SessionLengthSSOInDays) + a.SetSessionExpireInDays(session, *a.Config().ServiceSettings.SessionLengthSSOInDays) } else { - session.SetExpireInDays(*a.Config().ServiceSettings.SessionLengthWebInDays) + a.SetSessionExpireInDays(session, *a.Config().ServiceSettings.SessionLengthWebInDays) } ua := uasurfer.Parse(r.UserAgent()) diff --git a/app/oauth.go b/app/oauth.go index 25d6196cc5..1514ffe9ac 100644 --- a/app/oauth.go +++ b/app/oauth.go @@ -366,7 +366,7 @@ func (a *App) newSession(appName string, user *model.User) (*model.Session, *mod // Set new token an session session := &model.Session{UserId: user.Id, Roles: user.Roles, IsOAuth: true} session.GenerateCSRF() - session.SetExpireInDays(*a.Config().ServiceSettings.SessionLengthSSOInDays) + a.SetSessionExpireInDays(session, *a.Config().ServiceSettings.SessionLengthSSOInDays) session.AddProp(model.SESSION_PROP_PLATFORM, appName) session.AddProp(model.SESSION_PROP_OS, "OAuth2") session.AddProp(model.SESSION_PROP_BROWSER, "OAuth2") diff --git a/app/oauth_test.go b/app/oauth_test.go index f69a01aad9..60e28db747 100644 --- a/app/oauth_test.go +++ b/app/oauth_test.go @@ -77,7 +77,7 @@ func TestOAuthRevokeAccessToken(t *testing.T) { session.UserId = model.NewId() session.Token = model.NewId() session.Roles = model.SYSTEM_USER_ROLE_ID - session.SetExpireInDays(1) + th.App.SetSessionExpireInDays(session, 1) session, _ = th.App.CreateSession(session) err = th.App.RevokeAccessToken(session.Token) @@ -119,7 +119,7 @@ func TestOAuthDeleteApp(t *testing.T) { session.Token = model.NewId() session.Roles = model.SYSTEM_USER_ROLE_ID session.IsOAuth = true - session.SetExpireInDays(1) + th.App.SetSessionExpireInDays(session, 1) session, _ = th.App.CreateSession(session) diff --git a/app/opentracing/opentracing_layer.go b/app/opentracing/opentracing_layer.go index f83cfa08fb..34e4252a47 100644 --- a/app/opentracing/opentracing_layer.go +++ b/app/opentracing/opentracing_layer.go @@ -13218,6 +13218,21 @@ func (a *OpenTracingAppLayer) SetSearchEngine(se *searchengine.Broker) { a.app.SetSearchEngine(se) } +func (a *OpenTracingAppLayer) SetSessionExpireInDays(session *model.Session, days int) { + origCtx := a.ctx + span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.SetSessionExpireInDays") + + a.ctx = newCtx + a.app.Srv().Store.SetContext(newCtx) + defer func() { + a.app.Srv().Store.SetContext(origCtx) + a.ctx = origCtx + }() + + defer span.Finish() + a.app.SetSessionExpireInDays(session, days) +} + func (a *OpenTracingAppLayer) SetStatusAwayIfNeeded(userId string, manual bool) { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.SetStatusAwayIfNeeded") diff --git a/app/session.go b/app/session.go index 54a7fca821..6da7c61589 100644 --- a/app/session.go +++ b/app/session.go @@ -346,6 +346,9 @@ func (a *App) ExtendSessionExpiryIfNeeded(session *model.Session) bool { session.ExpiresAt = newExpiry a.AddSessionToCache(session) + mlog.Debug("Session extended", mlog.String("user_id", session.UserId), mlog.String("session_id", session.Id), + mlog.Int64("newExpiry", newExpiry), mlog.Int64("session_length", sessionLength)) + auditRec.Success() auditRec.AddMeta("extended_session", session) return true @@ -369,6 +372,17 @@ func (a *App) GetSessionLengthInMillis(session *model.Session) int64 { return int64(days * 24 * 60 * 60 * 1000) } +// SetSessionExpireInDays sets the session's expiry the specified number of days +// relative to either the session creation date or the current time, depending +// on the `ExtendSessionOnActivity` config setting. +func (a *App) SetSessionExpireInDays(session *model.Session, days int) { + if session.CreateAt == 0 || *a.Config().ServiceSettings.ExtendSessionLengthWithActivity { + session.ExpiresAt = model.GetMillis() + (1000 * 60 * 60 * 24 * int64(days)) + } else { + session.ExpiresAt = session.CreateAt + (1000 * 60 * 60 * 24 * int64(days)) + } +} + func (a *App) CreateUserAccessToken(token *model.UserAccessToken) (*model.UserAccessToken, *model.AppError) { user, err := a.Srv().Store.User().Get(token.UserId) @@ -444,7 +458,7 @@ func (a *App) createSessionForUserAccessToken(tokenString string) (*model.Sessio } else { session.AddProp(model.SESSION_PROP_IS_GUEST, "false") } - session.SetExpireInDays(model.SESSION_USER_ACCESS_TOKEN_EXPIRY) + a.SetSessionExpireInDays(session, model.SESSION_USER_ACCESS_TOKEN_EXPIRY) session, nErr = a.Srv().Store.Session().Save(session) if nErr != nil { diff --git a/app/session_test.go b/app/session_test.go index 801eeb0e54..7aee85d249 100644 --- a/app/session_test.go +++ b/app/session_test.go @@ -350,3 +350,55 @@ func TestApp_ExtendExpiryIfNeeded(t *testing.T) { } } + +const ( + dayInMillis = 86400000 + grace = 5 * 1000 + thirtyDays = dayInMillis * 30 +) + +func TestApp_SetSessionExpireInDays(t *testing.T) { + th := Setup(t) + defer th.TearDown() + + now := model.GetMillis() + createAt := now - (dayInMillis * 20) + + tests := []struct { + name string + extend bool + create bool + days int + want int64 + }{ + {name: "zero days, extend", extend: true, create: true, days: 0, want: now}, + {name: "zero days, extend", extend: true, create: false, days: 0, want: now}, + {name: "zero days, no extend", extend: false, create: true, days: 0, want: createAt}, + {name: "zero days, no extend", extend: false, create: false, days: 0, want: now}, + {name: "thirty days, extend", extend: true, create: true, days: 30, want: now + thirtyDays}, + {name: "thirty days, extend", extend: true, create: false, days: 30, want: now + thirtyDays}, + {name: "thirty days, no extend", extend: false, create: true, days: 30, want: createAt + thirtyDays}, + {name: "thirty days, no extend", extend: false, create: false, days: 30, want: now + thirtyDays}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.ExtendSessionLengthWithActivity = tt.extend + }) + var create int64 + if tt.create { + create = createAt + } + + session := &model.Session{ + CreateAt: create, + ExpiresAt: model.GetMillis() + dayInMillis, + } + th.App.SetSessionExpireInDays(session, tt.days) + + // must be within 5 seconds of expected time. + require.GreaterOrEqual(t, session.ExpiresAt, tt.want-grace) + require.LessOrEqual(t, session.ExpiresAt, tt.want+grace) + }) + } +} diff --git a/model/session.go b/model/session.go index 1cbb276062..06f3e003b5 100644 --- a/model/session.go +++ b/model/session.go @@ -115,6 +115,9 @@ func (me *Session) IsExpired() bool { return false } +// Deprecated: SetExpireInDays is deprecated and should not be used. +// Use (*App).SetSessionExpireInDays instead which handles the +// cases where the new ExpiresAt is not relative to CreateAt. func (me *Session) SetExpireInDays(days int) { if me.CreateAt == 0 { me.ExpiresAt = GetMillis() + (1000 * 60 * 60 * 24 * int64(days)) diff --git a/web/handlers_test.go b/web/handlers_test.go index e569d14389..bbf8a0f3e1 100644 --- a/web/handlers_test.go +++ b/web/handlers_test.go @@ -128,7 +128,7 @@ func TestHandlerServeCSRFToken(t *testing.T) { IsOAuth: false, } session.GenerateCSRF() - session.SetExpireInDays(1) + th.App.SetSessionExpireInDays(session, 1) session, err := th.App.CreateSession(session) if err != nil { t.Errorf("Expected nil, got %s", err)