Avoid resetting device id when removed from push proxy (#28269)

* Avoid resetting device id when removed from push proxy

* Fix test and minor improvements

* Add MySQL support

server/channels/app/notification_push.go

@@ -530,7 +530,9 @@ func (a *App) sendToPushProxy(msg *model.PushNotification, session *model.Sessio
 
 	switch pushResponse[model.PushStatus] {
 	case model.PushStatusRemove:
-		a.AttachDeviceId(session.Id, "", session.ExpiresAt)
+		a.SetExtraSessionProps(session, map[string]string{
+			model.SessionPropLastRemovedDeviceId: session.DeviceId,
+		})
 		a.ClearSessionCacheForUser(session.UserId)
 		return errors.New(notificationErrorRemoveDevice)
 	case model.PushStatusFail:

server/channels/app/notification_push_test.go

@@ -1240,7 +1240,7 @@ func TestClearPushNotificationSync(t *testing.T) {
 
 	mockSessionStore := mocks.SessionStore{}
 	mockSessionStore.On("GetSessionsWithActiveDeviceIds", mock.AnythingOfType("string")).Return([]*model.Session{sess1, sess2}, nil)
-	mockSessionStore.On("UpdateDeviceId", mock.AnythingOfType("string"), mock.AnythingOfType("string"), mock.AnythingOfType("int64")).Return("testdeviceID", nil)
+	mockSessionStore.On("UpdateProps", mock.Anything).Return(nil)
 	mockStore.On("User").Return(&mockUserStore)
 	mockStore.On("Post").Return(&mockPostStore)
 	mockStore.On("System").Return(&mockSystemStore)
@@ -1316,7 +1316,7 @@ func TestUpdateMobileAppBadgeSync(t *testing.T) {
 
 	mockSessionStore := mocks.SessionStore{}
 	mockSessionStore.On("GetSessionsWithActiveDeviceIds", mock.AnythingOfType("string")).Return([]*model.Session{sess1, sess2}, nil)
-	mockSessionStore.On("UpdateDeviceId", mock.AnythingOfType("string"), mock.AnythingOfType("string"), mock.AnythingOfType("int64")).Return("testdeviceID", nil)
+	mockSessionStore.On("UpdateProps", mock.Anything).Return(nil)
 	mockStore.On("User").Return(&mockUserStore)
 	mockStore.On("Post").Return(&mockPostStore)
 	mockStore.On("System").Return(&mockSystemStore)
@@ -1670,7 +1670,7 @@ func BenchmarkPushNotificationThroughput(b *testing.B) {
 			ExpiresAt: model.GetMillis() + 100000,
 		}
 		mockSessionStore.On("GetSessionsWithActiveDeviceIds", u.Id).Return([]*model.Session{sess1, sess2}, nil)
-		mockSessionStore.On("UpdateDeviceId", sess1.Id, "deviceID"+u.Id, mock.AnythingOfType("int64")).Return("deviceID"+u.Id, nil)
+		mockSessionStore.On("UpdateProps", mock.Anything).Return(nil)
 
 		testData = append(testData, userSession{
 			user:    u,

server/channels/store/sqlstore/session_store.go

@@ -146,6 +146,10 @@ func (me SqlSessionStore) GetLRUSessions(c request.CTX, userId string, limit uin
 }
 
 func (me SqlSessionStore) GetSessionsWithActiveDeviceIds(userId string) ([]*model.Session, error) {
+	lastRemovedQuery := `DeviceId != COALESCE(Props->>'last_removed_device_id', '')`
+	if me.DriverName() == model.DatabaseDriverMysql {
+		lastRemovedQuery = `DeviceId != COALESCE(Props->>'$.last_removed_device_id', '')`
+	}
 	query :=
 		`SELECT *
 		FROM
@@ -154,7 +158,8 @@ func (me SqlSessionStore) GetSessionsWithActiveDeviceIds(userId string) ([]*mode
 			UserId = ? AND
 			ExpiresAt != 0 AND
 			? <= ExpiresAt AND
-			DeviceId != ''`
+			DeviceId != '' AND
+			` + lastRemovedQuery
 
 	sessions := []*model.Session{}
 

server/channels/store/storetest/session_store.go

@@ -91,6 +91,7 @@ func testSessionGetWithDeviceId(t *testing.T, rctx request.CTX, ss store.Store)
 	s1 := &model.Session{}
 	s1.UserId = model.NewId()
 	s1.ExpiresAt = model.GetMillis() + 10000
+	s1.Props = model.StringMap{}
 
 	s1, err := ss.Session().Save(rctx, s1)
 	require.NoError(t, err)
@@ -99,6 +100,7 @@ func testSessionGetWithDeviceId(t *testing.T, rctx request.CTX, ss store.Store)
 	s2.UserId = s1.UserId
 	s2.DeviceId = model.NewId()
 	s2.ExpiresAt = model.GetMillis() + 10000
+	s2.Props = model.StringMap{}
 
 	_, err = ss.Session().Save(rctx, s2)
 	require.NoError(t, err)
@@ -107,10 +109,22 @@ func testSessionGetWithDeviceId(t *testing.T, rctx request.CTX, ss store.Store)
 	s3.UserId = s1.UserId
 	s3.ExpiresAt = 1
 	s3.DeviceId = model.NewId()
+	s3.Props = model.StringMap{}
 
 	_, err = ss.Session().Save(rctx, s3)
 	require.NoError(t, err)
 
+	s4 := &model.Session{}
+	s4.UserId = s1.UserId
+	s4.DeviceId = model.NewId()
+	s4.ExpiresAt = model.GetMillis() + 10000
+	s4.Props = model.StringMap{
+		model.SessionPropLastRemovedDeviceId: s4.DeviceId,
+	}
+
+	_, err = ss.Session().Save(rctx, s4)
+	require.NoError(t, err)
+
 	data, err := ss.Session().GetSessionsWithActiveDeviceIds(s1.UserId)
 	require.NoError(t, err)
 	require.Len(t, data, 1, "should match len")

server/public/model/session.go

@@ -26,6 +26,7 @@ const (
 	SessionPropIsBotValue                 = "true"
 	SessionPropOAuthAppID                 = "oauth_app_id"
 	SessionPropMattermostAppID            = "mattermost_app_id"
+	SessionPropLastRemovedDeviceId        = "last_removed_device_id"
 	SessionPropDeviceNotificationDisabled = "device_notification_disabled"
 	SessionPropMobileVersion              = "mobile_version"
 	SessionTypeUserAccessToken            = "UserAccessToken"