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

@@ -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
 

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
 

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

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())

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")

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)
 

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")

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 {

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)
+		})
+	}
+}

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))

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)