From f1a5b88aa9cd7b3c579d4c32c1911dccbdde51b3 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Mon, 13 Jun 2022 14:56:07 +0100 Subject: [PATCH 1/6] make session id recycle after 24 hours --- src/__tests__/sessionid.js | 96 ++++++++++++++++++++++++++++++-------- src/sessionid.js | 41 +++++++++++----- 2 files changed, 106 insertions(+), 31 deletions(-) diff --git a/src/__tests__/sessionid.js b/src/__tests__/sessionid.js index dab10ff7e..652d8e80e 100644 --- a/src/__tests__/sessionid.js +++ b/src/__tests__/sessionid.js @@ -23,10 +23,12 @@ describe('Session ID manager', () => { given('timestamp', () => 1603107479471) + given('now', () => given.timestamp + 1000) + beforeEach(() => { _.UUID.mockReturnValue('newUUID') sessionStore.is_supported.mockReturnValue(true) - const mockDate = new Date(1603107460000) + const mockDate = new Date(given.now) jest.spyOn(global, 'Date').mockImplementation(() => mockDate) }) @@ -36,7 +38,9 @@ describe('Session ID manager', () => { windowId: 'newUUID', sessionId: 'newUUID', }) - expect(given.persistence.register).toHaveBeenCalledWith({ [SESSION_ID]: [given.timestamp, 'newUUID'] }) + expect(given.persistence.register).toHaveBeenCalledWith({ + [SESSION_ID]: [given.timestamp, given.timestamp, 'newUUID'], + }) expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newUUID') }) @@ -46,13 +50,17 @@ describe('Session ID manager', () => { windowId: 'newUUID', sessionId: 'newUUID', }) - expect(given.persistence.register).toHaveBeenCalledWith({ [SESSION_ID]: [given.timestamp, 'newUUID'] }) + expect(given.persistence.register).toHaveBeenCalledWith({ + [SESSION_ID]: [given.timestamp, given.timestamp, 'newUUID'], + }) expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newUUID') }) }) describe('stored session data', () => { - given('storedSessionIdData', () => [1603107460000, 'oldSessionID']) + given('timestamp_of_session_start', () => given.now - 3600) + + given('storedSessionIdData', () => [given.timestamp_of_session_start, given.now, 'oldSessionID']) beforeEach(() => { sessionStore.parse.mockReturnValue('oldWindowID') }) @@ -62,19 +70,26 @@ describe('Session ID manager', () => { windowId: 'oldWindowID', sessionId: 'oldSessionID', }) - expect(given.persistence.register).toHaveBeenCalledWith({ [SESSION_ID]: [given.timestamp, 'oldSessionID'] }) + expect(given.persistence.register).toHaveBeenCalledWith({ + [SESSION_ID]: [given.timestamp_of_session_start, given.timestamp, 'oldSessionID'], + }) }) it('reuses old ids and does not update the session timestamp if > 30m pass and readOnly is true', () => { - const old_timestamp = 1602107460000 - given('storedSessionIdData', () => [old_timestamp, 'oldSessionID']) + let thirtyMinutesAndOneSecond = 60 * 60 * 30 + 1 + const old_timestamp = given.now - thirtyMinutesAndOneSecond + const sessionStart = old_timestamp - 1000 + + given('storedSessionIdData', () => [sessionStart, old_timestamp, 'oldSessionID']) given('readOnly', () => true) expect(given.subject).toEqual({ windowId: 'oldWindowID', sessionId: 'oldSessionID', }) - expect(given.persistence.register).toHaveBeenCalledWith({ [SESSION_ID]: [old_timestamp, 'oldSessionID'] }) + expect(given.persistence.register).toHaveBeenCalledWith({ + [SESSION_ID]: [sessionStart, old_timestamp, 'oldSessionID'], + }) }) it('generates only a new window id, and saves it when there is no previous window id set', () => { @@ -83,31 +98,72 @@ describe('Session ID manager', () => { windowId: 'newUUID', sessionId: 'oldSessionID', }) - expect(given.persistence.register).toHaveBeenCalledWith({ [SESSION_ID]: [given.timestamp, 'oldSessionID'] }) + expect(given.persistence.register).toHaveBeenCalledWith({ + [SESSION_ID]: [given.timestamp_of_session_start, given.timestamp, 'oldSessionID'], + }) expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newUUID') }) it('generates a new session id and window id, and saves it when >30m since last event', () => { const old_timestamp = 1602107460000 - given('storedSessionIdData', () => [old_timestamp, 'oldSessionID']) + given('storedSessionIdData', () => [given.timestamp_of_session_start, old_timestamp, 'oldSessionID']) expect(given.subject).toEqual({ windowId: 'newUUID', sessionId: 'newUUID', }) - expect(given.persistence.register).toHaveBeenCalledWith({ [SESSION_ID]: [given.timestamp, 'newUUID'] }) + expect(given.persistence.register).toHaveBeenCalledWith({ + [SESSION_ID]: [given.timestamp, given.timestamp, 'newUUID'], + }) + expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newUUID') + }) + + it('generates a new session id and window id, and saves it when >24 hours since start timestamp', () => { + const old_timestamp = 1602107460000 + const twentyFourHours = 3600 * 24 + given('storedSessionIdData', () => [given.timestamp_of_session_start, old_timestamp, 'oldSessionID']) + given('timestamp', () => given.timestamp_of_session_start + twentyFourHours) + + expect(given.subject).toEqual({ + windowId: 'newUUID', + sessionId: 'newUUID', + }) + + expect(given.persistence.register).toHaveBeenCalledWith({ + [SESSION_ID]: [given.timestamp, given.timestamp, 'newUUID'], + }) + expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newUUID') + }) + + it('generates a new session id and window id, and saves it when >24 hours since start timestamp even when readonly is true', () => { + const old_timestamp = 1602107460000 + const twentyFourHoursAndOneSecond = (3600 * 24 + 1) * 1000 + given('storedSessionIdData', () => [given.timestamp_of_session_start, old_timestamp, 'oldSessionID']) + given('timestamp', () => given.timestamp_of_session_start + twentyFourHoursAndOneSecond) + given('readOnly', () => true) + + expect(given.subject).toEqual({ + windowId: 'newUUID', + sessionId: 'newUUID', + }) + + expect(given.persistence.register).toHaveBeenCalledWith({ + [SESSION_ID]: [given.timestamp, given.timestamp, 'newUUID'], + }) expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newUUID') }) it('uses the current time if no timestamp is provided', () => { const old_timestamp = 1601107460000 - given('storedSessionIdData', () => [old_timestamp, 'oldSessionID']) + given('storedSessionIdData', () => [given.timestamp_of_session_start, old_timestamp, 'oldSessionID']) given('timestamp', () => undefined) expect(given.subject).toEqual({ windowId: 'newUUID', sessionId: 'newUUID', }) - expect(given.persistence.register).toHaveBeenCalledWith({ [SESSION_ID]: [1603107460000, 'newUUID'] }) + expect(given.persistence.register).toHaveBeenCalledWith({ + [SESSION_ID]: [given.now, given.now, 'newUUID'], + }) }) }) @@ -133,20 +189,22 @@ describe('Session ID manager', () => { describe('session id storage', () => { it('stores and retrieves a session id and timestamp', () => { - given.sessionIdManager._setSessionId('newSessionId', 1603107460000) - expect(given.sessionIdManager._getSessionId()).toEqual([1603107460000, 'newSessionId']) - expect(given.persistence.register).toHaveBeenCalledWith({ [SESSION_ID]: [1603107460000, 'newSessionId'] }) + given.sessionIdManager._setSessionId('newSessionId', 1603107460000, 1603107460000) + expect(given.sessionIdManager._getSessionId()).toEqual([1603107460000, 1603107460000, 'newSessionId']) + expect(given.persistence.register).toHaveBeenCalledWith({ + [SESSION_ID]: [1603107460000, 1603107460000, 'newSessionId'], + }) }) }) describe('reset session id', () => { it('clears the existing session id', () => { given.sessionIdManager.resetSessionId() - expect(given.persistence.register).toHaveBeenCalledWith({ [SESSION_ID]: [null, null] }) + expect(given.persistence.register).toHaveBeenCalledWith({ [SESSION_ID]: [null, null, null] }) }) it('a new session id is generated when called', () => { - given('storedSessionIdData', () => [null, null]) - expect(given.sessionIdManager._getSessionId()).toEqual([null, null]) + given('storedSessionIdData', () => [null, null, null]) + expect(given.sessionIdManager._getSessionId()).toEqual([null, null, null]) expect(given.subject).toMatchObject({ windowId: 'newUUID', sessionId: 'newUUID', diff --git a/src/sessionid.js b/src/sessionid.js index b5eaaa94a..85592183f 100644 --- a/src/sessionid.js +++ b/src/sessionid.js @@ -3,6 +3,7 @@ import { sessionStore } from './storage' import { _ } from './utils' const SESSION_CHANGE_THRESHOLD = 30 * 60 * 1000 // 30 mins +const SESSION_LENGTH_LIMIT = 24 * 3600 * 1000 // 24 hours export class SessionIdManager { constructor(config, persistence) { @@ -40,25 +41,32 @@ export class SessionIdManager { // Note: 'this.persistence.register' can be disabled in the config. // In that case, this works by storing sessionId and the timestamp in memory. - _setSessionId(sessionId, timestamp) { - if (sessionId !== this._sessionId || timestamp !== this._timestamp) { - this._timestamp = timestamp + _setSessionId(sessionId, sessionActivityTimestamp, sessionStartTimestamp) { + if ( + sessionId !== this._sessionId || + sessionActivityTimestamp !== this._sessionActivityTimestamp || + sessionStartTimestamp !== this._sessionStartTimestamp + ) { + this._sessionStartTimestamp = sessionStartTimestamp + this._sessionActivityTimestamp = sessionActivityTimestamp this._sessionId = sessionId - this.persistence.register({ [SESSION_ID]: [timestamp, sessionId] }) + this.persistence.register({ + [SESSION_ID]: [sessionStartTimestamp, sessionActivityTimestamp, sessionId], + }) } } _getSessionId() { - if (this._sessionId && this._timestamp) { - return [this._timestamp, this._sessionId] + if (this._sessionId && this._sessionActivityTimestamp && this._sessionStartTimestamp) { + return [this._sessionStartTimestamp, this._sessionActivityTimestamp, this._sessionId] } - return this.persistence['props'][SESSION_ID] || [0, null] + return this.persistence['props'][SESSION_ID] || [0, 0, null] } // Resets the session id by setting it to null. On the subsequent call to checkAndGetSessionAndWindowId, // new ids will be generated. resetSessionId() { - this._setSessionId(null, null) + this._setSessionId(null, null, null) } /* @@ -80,20 +88,29 @@ export class SessionIdManager { checkAndGetSessionAndWindowId(readOnly = false, timestamp = null) { timestamp = timestamp || new Date().getTime() - let [lastTimestamp, sessionId] = this._getSessionId() + let [startTimestamp, lastTimestamp, sessionId] = this._getSessionId() let windowId = this._getWindowId() - if (!sessionId || (!readOnly && Math.abs(timestamp - lastTimestamp) > SESSION_CHANGE_THRESHOLD)) { + const sessionPastMaximumLength = + startTimestamp && startTimestamp > 0 && Math.abs(timestamp - startTimestamp) > SESSION_LENGTH_LIMIT + + if ( + !sessionId || + (!readOnly && Math.abs(timestamp - lastTimestamp) > SESSION_CHANGE_THRESHOLD) || + sessionPastMaximumLength + ) { sessionId = _.UUID() windowId = _.UUID() + startTimestamp = timestamp } else if (!windowId) { windowId = _.UUID() } - const newTimestamp = lastTimestamp === 0 || !readOnly ? timestamp : lastTimestamp + const newTimestamp = lastTimestamp === 0 || !readOnly || sessionPastMaximumLength ? timestamp : lastTimestamp + const sessionStartTimestamp = startTimestamp === 0 ? new Date().getTime() : startTimestamp this._setWindowId(windowId) - this._setSessionId(sessionId, newTimestamp) + this._setSessionId(sessionId, newTimestamp, sessionStartTimestamp) return { sessionId: sessionId, From d5a80373a111f3aa38592bfcbcc0d8ffb264e9ec Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Mon, 13 Jun 2022 14:59:21 +0100 Subject: [PATCH 2/6] less pythony variables in test --- src/__tests__/sessionid.js | 42 +++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/src/__tests__/sessionid.js b/src/__tests__/sessionid.js index 652d8e80e..8fa805223 100644 --- a/src/__tests__/sessionid.js +++ b/src/__tests__/sessionid.js @@ -13,9 +13,9 @@ describe('Session ID manager', () => { given('persistence', () => ({ props: { [SESSION_ID]: given.storedSessionIdData }, register: jest.fn(), - disabled: given.disable_persistence, + disabled: given.disablePersistence, })) - given('disable_persistence', () => false) + given('disablePersistence', () => false) given('config', () => ({ persistence_name: 'persistance-name', @@ -58,9 +58,9 @@ describe('Session ID manager', () => { }) describe('stored session data', () => { - given('timestamp_of_session_start', () => given.now - 3600) + given('timestampOfSessionStart', () => given.now - 3600) - given('storedSessionIdData', () => [given.timestamp_of_session_start, given.now, 'oldSessionID']) + given('storedSessionIdData', () => [given.timestampOfSessionStart, given.now, 'oldSessionID']) beforeEach(() => { sessionStore.parse.mockReturnValue('oldWindowID') }) @@ -71,16 +71,16 @@ describe('Session ID manager', () => { sessionId: 'oldSessionID', }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestamp_of_session_start, given.timestamp, 'oldSessionID'], + [SESSION_ID]: [given.timestampOfSessionStart, given.timestamp, 'oldSessionID'], }) }) it('reuses old ids and does not update the session timestamp if > 30m pass and readOnly is true', () => { let thirtyMinutesAndOneSecond = 60 * 60 * 30 + 1 - const old_timestamp = given.now - thirtyMinutesAndOneSecond - const sessionStart = old_timestamp - 1000 + const oldTimestamp = given.now - thirtyMinutesAndOneSecond + const sessionStart = oldTimestamp - 1000 - given('storedSessionIdData', () => [sessionStart, old_timestamp, 'oldSessionID']) + given('storedSessionIdData', () => [sessionStart, oldTimestamp, 'oldSessionID']) given('readOnly', () => true) expect(given.subject).toEqual({ @@ -88,7 +88,7 @@ describe('Session ID manager', () => { sessionId: 'oldSessionID', }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [sessionStart, old_timestamp, 'oldSessionID'], + [SESSION_ID]: [sessionStart, oldTimestamp, 'oldSessionID'], }) }) @@ -99,14 +99,14 @@ describe('Session ID manager', () => { sessionId: 'oldSessionID', }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestamp_of_session_start, given.timestamp, 'oldSessionID'], + [SESSION_ID]: [given.timestampOfSessionStart, given.timestamp, 'oldSessionID'], }) expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newUUID') }) it('generates a new session id and window id, and saves it when >30m since last event', () => { - const old_timestamp = 1602107460000 - given('storedSessionIdData', () => [given.timestamp_of_session_start, old_timestamp, 'oldSessionID']) + const oldTimestamp = 1602107460000 + given('storedSessionIdData', () => [given.timestampOfSessionStart, oldTimestamp, 'oldSessionID']) expect(given.subject).toEqual({ windowId: 'newUUID', @@ -119,10 +119,10 @@ describe('Session ID manager', () => { }) it('generates a new session id and window id, and saves it when >24 hours since start timestamp', () => { - const old_timestamp = 1602107460000 + const oldTimestamp = 1602107460000 const twentyFourHours = 3600 * 24 - given('storedSessionIdData', () => [given.timestamp_of_session_start, old_timestamp, 'oldSessionID']) - given('timestamp', () => given.timestamp_of_session_start + twentyFourHours) + given('storedSessionIdData', () => [given.timestampOfSessionStart, oldTimestamp, 'oldSessionID']) + given('timestamp', () => given.timestampOfSessionStart + twentyFourHours) expect(given.subject).toEqual({ windowId: 'newUUID', @@ -136,10 +136,10 @@ describe('Session ID manager', () => { }) it('generates a new session id and window id, and saves it when >24 hours since start timestamp even when readonly is true', () => { - const old_timestamp = 1602107460000 + const oldTimestamp = 1602107460000 const twentyFourHoursAndOneSecond = (3600 * 24 + 1) * 1000 - given('storedSessionIdData', () => [given.timestamp_of_session_start, old_timestamp, 'oldSessionID']) - given('timestamp', () => given.timestamp_of_session_start + twentyFourHoursAndOneSecond) + given('storedSessionIdData', () => [given.timestampOfSessionStart, oldTimestamp, 'oldSessionID']) + given('timestamp', () => given.timestampOfSessionStart + twentyFourHoursAndOneSecond) given('readOnly', () => true) expect(given.subject).toEqual({ @@ -154,8 +154,8 @@ describe('Session ID manager', () => { }) it('uses the current time if no timestamp is provided', () => { - const old_timestamp = 1601107460000 - given('storedSessionIdData', () => [given.timestamp_of_session_start, old_timestamp, 'oldSessionID']) + const oldTimestamp = 1601107460000 + given('storedSessionIdData', () => [given.timestampOfSessionStart, oldTimestamp, 'oldSessionID']) given('timestamp', () => undefined) expect(given.subject).toEqual({ windowId: 'newUUID', @@ -174,7 +174,7 @@ describe('Session ID manager', () => { expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newWindowId') }) it('stores and retrieves a window_id if persistance is disabled and storage is not used', () => { - given('disable_persistence', () => true) + given('disablePersistence', () => true) given.sessionIdManager._setWindowId('newWindowId') expect(given.sessionIdManager._getWindowId()).toEqual('newWindowId') expect(sessionStore.set).not.toHaveBeenCalled() From 65fb81dc198bb0239abf9f15fa1b5161d6f8d3cb Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Mon, 13 Jun 2022 18:12:57 +0100 Subject: [PATCH 3/6] add the start time if the client doesn't already have it --- src/__tests__/sessionid.js | 13 ++++++++++++- src/sessionid.js | 9 ++++++++- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/src/__tests__/sessionid.js b/src/__tests__/sessionid.js index 8fa805223..ad412bfbf 100644 --- a/src/__tests__/sessionid.js +++ b/src/__tests__/sessionid.js @@ -165,6 +165,17 @@ describe('Session ID manager', () => { [SESSION_ID]: [given.now, given.now, 'newUUID'], }) }) + + it('populates the session start timestamp for a browser with no start time stored', () => { + given('storedSessionIdData', () => [given.timestamp, 'oldSessionID']) + expect(given.subject).toEqual({ + windowId: 'oldWindowID', + sessionId: 'oldSessionID', + }) + expect(given.persistence.register).toHaveBeenCalledWith({ + [SESSION_ID]: [given.timestamp, given.timestamp, 'oldSessionID'], + }) + }) }) describe('window id storage', () => { @@ -190,10 +201,10 @@ describe('Session ID manager', () => { describe('session id storage', () => { it('stores and retrieves a session id and timestamp', () => { given.sessionIdManager._setSessionId('newSessionId', 1603107460000, 1603107460000) - expect(given.sessionIdManager._getSessionId()).toEqual([1603107460000, 1603107460000, 'newSessionId']) expect(given.persistence.register).toHaveBeenCalledWith({ [SESSION_ID]: [1603107460000, 1603107460000, 'newSessionId'], }) + expect(given.sessionIdManager._getSessionId()).toEqual([1603107460000, 1603107460000, 'newSessionId']) }) }) diff --git a/src/sessionid.js b/src/sessionid.js index 85592183f..d8d214654 100644 --- a/src/sessionid.js +++ b/src/sessionid.js @@ -60,7 +60,14 @@ export class SessionIdManager { if (this._sessionId && this._sessionActivityTimestamp && this._sessionStartTimestamp) { return [this._sessionStartTimestamp, this._sessionActivityTimestamp, this._sessionId] } - return this.persistence['props'][SESSION_ID] || [0, 0, null] + const sessionId = this.persistence['props'][SESSION_ID] + + if (Array.isArray(sessionId) && sessionId.length === 2) { + // Storage does not yet have a session start time. Add the last activity timestamp as the start time + sessionId.unshift(sessionId[0]) + } + + return sessionId || [0, 0, null] } // Resets the session id by setting it to null. On the subsequent call to checkAndGetSessionAndWindowId, From 70dbeceb0f69b48518d1327182ab089c75a50771 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Mon, 13 Jun 2022 18:24:56 +0100 Subject: [PATCH 4/6] put session start time at end of storage so you can rollback if needed --- src/__tests__/sessionid.js | 36 ++++++++++++++++++------------------ src/sessionid.js | 10 +++++----- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/src/__tests__/sessionid.js b/src/__tests__/sessionid.js index ad412bfbf..3b286305a 100644 --- a/src/__tests__/sessionid.js +++ b/src/__tests__/sessionid.js @@ -39,7 +39,7 @@ describe('Session ID manager', () => { sessionId: 'newUUID', }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestamp, given.timestamp, 'newUUID'], + [SESSION_ID]: [given.timestamp, 'newUUID', given.timestamp], }) expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newUUID') }) @@ -51,7 +51,7 @@ describe('Session ID manager', () => { sessionId: 'newUUID', }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestamp, given.timestamp, 'newUUID'], + [SESSION_ID]: [given.timestamp, 'newUUID', given.timestamp], }) expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newUUID') }) @@ -60,7 +60,7 @@ describe('Session ID manager', () => { describe('stored session data', () => { given('timestampOfSessionStart', () => given.now - 3600) - given('storedSessionIdData', () => [given.timestampOfSessionStart, given.now, 'oldSessionID']) + given('storedSessionIdData', () => [given.now, 'oldSessionID', given.timestampOfSessionStart]) beforeEach(() => { sessionStore.parse.mockReturnValue('oldWindowID') }) @@ -71,7 +71,7 @@ describe('Session ID manager', () => { sessionId: 'oldSessionID', }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestampOfSessionStart, given.timestamp, 'oldSessionID'], + [SESSION_ID]: [given.timestamp, 'oldSessionID', given.timestampOfSessionStart], }) }) @@ -80,7 +80,7 @@ describe('Session ID manager', () => { const oldTimestamp = given.now - thirtyMinutesAndOneSecond const sessionStart = oldTimestamp - 1000 - given('storedSessionIdData', () => [sessionStart, oldTimestamp, 'oldSessionID']) + given('storedSessionIdData', () => [oldTimestamp, 'oldSessionID', sessionStart]) given('readOnly', () => true) expect(given.subject).toEqual({ @@ -88,7 +88,7 @@ describe('Session ID manager', () => { sessionId: 'oldSessionID', }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [sessionStart, oldTimestamp, 'oldSessionID'], + [SESSION_ID]: [oldTimestamp, 'oldSessionID', sessionStart], }) }) @@ -99,21 +99,21 @@ describe('Session ID manager', () => { sessionId: 'oldSessionID', }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestampOfSessionStart, given.timestamp, 'oldSessionID'], + [SESSION_ID]: [given.timestamp, 'oldSessionID', given.timestampOfSessionStart], }) expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newUUID') }) it('generates a new session id and window id, and saves it when >30m since last event', () => { const oldTimestamp = 1602107460000 - given('storedSessionIdData', () => [given.timestampOfSessionStart, oldTimestamp, 'oldSessionID']) + given('storedSessionIdData', () => [oldTimestamp, 'oldSessionID', given.timestampOfSessionStart]) expect(given.subject).toEqual({ windowId: 'newUUID', sessionId: 'newUUID', }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestamp, given.timestamp, 'newUUID'], + [SESSION_ID]: [given.timestamp, 'newUUID', given.timestamp], }) expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newUUID') }) @@ -121,7 +121,7 @@ describe('Session ID manager', () => { it('generates a new session id and window id, and saves it when >24 hours since start timestamp', () => { const oldTimestamp = 1602107460000 const twentyFourHours = 3600 * 24 - given('storedSessionIdData', () => [given.timestampOfSessionStart, oldTimestamp, 'oldSessionID']) + given('storedSessionIdData', () => [oldTimestamp, 'oldSessionID', given.timestampOfSessionStart]) given('timestamp', () => given.timestampOfSessionStart + twentyFourHours) expect(given.subject).toEqual({ @@ -130,7 +130,7 @@ describe('Session ID manager', () => { }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestamp, given.timestamp, 'newUUID'], + [SESSION_ID]: [given.timestamp, 'newUUID', given.timestamp], }) expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newUUID') }) @@ -138,7 +138,7 @@ describe('Session ID manager', () => { it('generates a new session id and window id, and saves it when >24 hours since start timestamp even when readonly is true', () => { const oldTimestamp = 1602107460000 const twentyFourHoursAndOneSecond = (3600 * 24 + 1) * 1000 - given('storedSessionIdData', () => [given.timestampOfSessionStart, oldTimestamp, 'oldSessionID']) + given('storedSessionIdData', () => [oldTimestamp, 'oldSessionID', given.timestampOfSessionStart]) given('timestamp', () => given.timestampOfSessionStart + twentyFourHoursAndOneSecond) given('readOnly', () => true) @@ -148,21 +148,21 @@ describe('Session ID manager', () => { }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestamp, given.timestamp, 'newUUID'], + [SESSION_ID]: [given.timestamp, 'newUUID', given.timestamp], }) expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newUUID') }) it('uses the current time if no timestamp is provided', () => { const oldTimestamp = 1601107460000 - given('storedSessionIdData', () => [given.timestampOfSessionStart, oldTimestamp, 'oldSessionID']) + given('storedSessionIdData', () => [oldTimestamp, 'oldSessionID', given.timestampOfSessionStart]) given('timestamp', () => undefined) expect(given.subject).toEqual({ windowId: 'newUUID', sessionId: 'newUUID', }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.now, given.now, 'newUUID'], + [SESSION_ID]: [given.now, 'newUUID', given.now], }) }) @@ -173,7 +173,7 @@ describe('Session ID manager', () => { sessionId: 'oldSessionID', }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestamp, given.timestamp, 'oldSessionID'], + [SESSION_ID]: [given.timestamp, 'oldSessionID', given.timestamp], }) }) }) @@ -202,9 +202,9 @@ describe('Session ID manager', () => { it('stores and retrieves a session id and timestamp', () => { given.sessionIdManager._setSessionId('newSessionId', 1603107460000, 1603107460000) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [1603107460000, 1603107460000, 'newSessionId'], + [SESSION_ID]: [1603107460000, 'newSessionId', 1603107460000], }) - expect(given.sessionIdManager._getSessionId()).toEqual([1603107460000, 1603107460000, 'newSessionId']) + expect(given.sessionIdManager._getSessionId()).toEqual([1603107460000, 'newSessionId', 1603107460000]) }) }) diff --git a/src/sessionid.js b/src/sessionid.js index d8d214654..d79f33ea8 100644 --- a/src/sessionid.js +++ b/src/sessionid.js @@ -51,23 +51,23 @@ export class SessionIdManager { this._sessionActivityTimestamp = sessionActivityTimestamp this._sessionId = sessionId this.persistence.register({ - [SESSION_ID]: [sessionStartTimestamp, sessionActivityTimestamp, sessionId], + [SESSION_ID]: [sessionActivityTimestamp, sessionId, sessionStartTimestamp], }) } } _getSessionId() { if (this._sessionId && this._sessionActivityTimestamp && this._sessionStartTimestamp) { - return [this._sessionStartTimestamp, this._sessionActivityTimestamp, this._sessionId] + return [this._sessionActivityTimestamp, this._sessionId, this._sessionStartTimestamp] } const sessionId = this.persistence['props'][SESSION_ID] if (Array.isArray(sessionId) && sessionId.length === 2) { // Storage does not yet have a session start time. Add the last activity timestamp as the start time - sessionId.unshift(sessionId[0]) + sessionId.push(sessionId[0]) } - return sessionId || [0, 0, null] + return sessionId || [0, null, 0] } // Resets the session id by setting it to null. On the subsequent call to checkAndGetSessionAndWindowId, @@ -95,7 +95,7 @@ export class SessionIdManager { checkAndGetSessionAndWindowId(readOnly = false, timestamp = null) { timestamp = timestamp || new Date().getTime() - let [startTimestamp, lastTimestamp, sessionId] = this._getSessionId() + let [lastTimestamp, sessionId, startTimestamp] = this._getSessionId() let windowId = this._getWindowId() const sessionPastMaximumLength = From 24b4282f16ccde1c7fc3e3eae528f118a7fcfada Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Thu, 16 Jun 2022 15:02:05 +0100 Subject: [PATCH 5/6] add tests that show that clock moving forward by 24 hours resets session id --- src/__tests__/extensions/sessionrecording.js | 105 ++++++++++++++++++- 1 file changed, 101 insertions(+), 4 deletions(-) diff --git a/src/__tests__/extensions/sessionrecording.js b/src/__tests__/extensions/sessionrecording.js index 83d11a5bc..e477754f6 100644 --- a/src/__tests__/extensions/sessionrecording.js +++ b/src/__tests__/extensions/sessionrecording.js @@ -5,7 +5,8 @@ import { MUTATION_SOURCE_TYPE, SessionRecording, } from '../../extensions/sessionrecording' -import { SESSION_RECORDING_ENABLED_SERVER_SIDE } from '../../posthog-persistence' +import { PostHogPersistence, SESSION_RECORDING_ENABLED_SERVER_SIDE } from '../../posthog-persistence' +import { SessionIdManager } from '../../sessionid' // Type and source defined here designate a non-user-generated recording event const NON_USER_GENERATED_EVENT = { type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, data: { source: MUTATION_SOURCE_TYPE } } @@ -19,15 +20,16 @@ describe('SessionRecording', () => { given('sessionRecording', () => new SessionRecording(given.posthog)) given('incomingSessionAndWindowId', () => ({ sessionId: 'sessionId', windowId: 'windowId' })) + given('sessionManager', () => ({ + checkAndGetSessionAndWindowId: jest.fn().mockImplementation(() => given.incomingSessionAndWindowId), + })) given('posthog', () => ({ get_property: () => given.$session_recording_enabled_server_side, get_config: jest.fn().mockImplementation((key) => given.config[key]), capture: jest.fn(), persistence: { register: jest.fn() }, _captureMetrics: { incr: jest.fn() }, - sessionManager: { - checkAndGetSessionAndWindowId: jest.fn().mockImplementation(() => given.incomingSessionAndWindowId), - }, + sessionManager: given.sessionManager, _addCaptureHook: jest.fn(), })) @@ -41,6 +43,7 @@ describe('SessionRecording', () => { recordCanvas: true, someUnregisteredProp: 'abc', }, + persistence: 'memory', })) given('$session_recording_enabled_server_side', () => true) given('disabled', () => false) @@ -338,5 +341,99 @@ describe('SessionRecording', () => { ) }) }) + + describe('with a real session id manager', () => { + const startingDate = new Date() + + beforeEach(() => { + given('sessionManager', () => new SessionIdManager(given.config, new PostHogPersistence(given.config))) + given.sessionRecording.startRecordingIfEnabled() + given.sessionRecording.startCaptureAndTrySendingQueuedSnapshots() + }) + + it('takes a full snapshot for the first _emit', () => { + _emit({ event: 123, type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, timestamp: startingDate.getTime() }) + expect(window.rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1) + }) + + it('does not take a full snapshot for the second _emit', () => { + _emit({ event: 123, type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, timestamp: startingDate.getTime() }) + _emit({ + event: 123, + type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, + timestamp: new Date( + startingDate.getFullYear(), + startingDate.getMonth(), + startingDate.getDate(), + startingDate.getHours(), + startingDate.getMinutes() + 1 + ).getTime(), + }) + expect(window.rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1) + }) + + it('does not change session id for a second _emit', () => { + const startingSessionId = given.sessionManager._getSessionId()[1] + + _emit({ event: 123, type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, timestamp: startingDate.getTime() }) + _emit({ event: 123, type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, timestamp: startingDate.getTime() }) + + expect(given.sessionManager._getSessionId()[1]).toEqual(startingSessionId) + }) + + it('does not take a full snapshot for the third _emit', () => { + _emit({ event: 123, type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, timestamp: startingDate.getTime() }) + _emit({ + event: 123, + type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, + timestamp: new Date( + startingDate.getFullYear(), + startingDate.getMonth(), + startingDate.getDate(), + startingDate.getHours(), + startingDate.getMinutes() + 1 + ).getTime(), + }) + _emit({ + event: 123, + type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, + timestamp: new Date( + startingDate.getFullYear(), + startingDate.getMonth(), + startingDate.getDate(), + startingDate.getHours(), + startingDate.getMinutes() + 2 + ).getTime(), + }) + expect(window.rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1) + }) + + it('sends a full snapshot if the session is rotated because too much time has passed', () => { + const startingSessionId = given.sessionManager._getSessionId()[1] + _emit({ event: 123, type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, timestamp: startingDate.getTime() }) + _emit({ + event: 123, + type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, + timestamp: new Date( + startingDate.getFullYear(), + startingDate.getMonth(), + startingDate.getDate(), + startingDate.getHours(), + startingDate.getMinutes() + 1 + ).getTime(), + }) + + const moreThanADayLater = new Date( + startingDate.getFullYear(), + startingDate.getMonth(), + startingDate.getDate() + 1, + startingDate.getHours() + 1 + ) + _emit({ event: 123, type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, timestamp: moreThanADayLater.getTime() }) + + expect(given.sessionManager._getSessionId()[1]).not.toEqual(startingSessionId) + expect(window.rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(2) + }) + }) }) }) From 4af9048270c2ab172bc34243b8004687569d3b81 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Thu, 16 Jun 2022 15:08:08 +0100 Subject: [PATCH 6/6] and a test for inactivity threshold --- src/__tests__/extensions/sessionrecording.js | 95 +++++++++++++------- 1 file changed, 63 insertions(+), 32 deletions(-) diff --git a/src/__tests__/extensions/sessionrecording.js b/src/__tests__/extensions/sessionrecording.js index e477754f6..7979e6d10 100644 --- a/src/__tests__/extensions/sessionrecording.js +++ b/src/__tests__/extensions/sessionrecording.js @@ -351,77 +351,108 @@ describe('SessionRecording', () => { given.sessionRecording.startCaptureAndTrySendingQueuedSnapshots() }) + const emitAtDateTime = (date) => + _emit({ event: 123, type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, timestamp: date.getTime() }) + it('takes a full snapshot for the first _emit', () => { - _emit({ event: 123, type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, timestamp: startingDate.getTime() }) + emitAtDateTime(startingDate) expect(window.rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1) }) it('does not take a full snapshot for the second _emit', () => { - _emit({ event: 123, type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, timestamp: startingDate.getTime() }) - _emit({ - event: 123, - type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, - timestamp: new Date( + emitAtDateTime(startingDate) + emitAtDateTime( + new Date( startingDate.getFullYear(), startingDate.getMonth(), startingDate.getDate(), startingDate.getHours(), startingDate.getMinutes() + 1 - ).getTime(), - }) + ) + ) expect(window.rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1) }) it('does not change session id for a second _emit', () => { const startingSessionId = given.sessionManager._getSessionId()[1] - _emit({ event: 123, type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, timestamp: startingDate.getTime() }) - _emit({ event: 123, type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, timestamp: startingDate.getTime() }) + emitAtDateTime(startingDate) + emitAtDateTime( + new Date( + startingDate.getFullYear(), + startingDate.getMonth(), + startingDate.getDate(), + startingDate.getHours(), + startingDate.getMinutes() + 1 + ) + ) expect(given.sessionManager._getSessionId()[1]).toEqual(startingSessionId) }) it('does not take a full snapshot for the third _emit', () => { - _emit({ event: 123, type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, timestamp: startingDate.getTime() }) - _emit({ - event: 123, - type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, - timestamp: new Date( + emitAtDateTime(startingDate) + + emitAtDateTime( + new Date( startingDate.getFullYear(), startingDate.getMonth(), startingDate.getDate(), startingDate.getHours(), startingDate.getMinutes() + 1 - ).getTime(), - }) - _emit({ - event: 123, - type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, - timestamp: new Date( + ) + ) + + emitAtDateTime( + new Date( startingDate.getFullYear(), startingDate.getMonth(), startingDate.getDate(), startingDate.getHours(), startingDate.getMinutes() + 2 - ).getTime(), - }) + ) + ) expect(window.rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1) }) - it('sends a full snapshot if the session is rotated because too much time has passed', () => { + it('sends a full snapshot if the session is rotated because session has been inactive for 30 minutess', () => { const startingSessionId = given.sessionManager._getSessionId()[1] - _emit({ event: 123, type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, timestamp: startingDate.getTime() }) - _emit({ - event: 123, - type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, - timestamp: new Date( + emitAtDateTime(startingDate) + emitAtDateTime( + new Date( startingDate.getFullYear(), startingDate.getMonth(), startingDate.getDate(), startingDate.getHours(), startingDate.getMinutes() + 1 - ).getTime(), - }) + ) + ) + + const inactivityThresholdLater = new Date( + startingDate.getFullYear(), + startingDate.getMonth(), + startingDate.getDate(), + startingDate.getHours(), + startingDate.getMinutes() + 32 + ) + emitAtDateTime(inactivityThresholdLater) + + expect(given.sessionManager._getSessionId()[1]).not.toEqual(startingSessionId) + expect(window.rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(2) + }) + + it('sends a full snapshot if the session is rotated because max time has passed', () => { + const startingSessionId = given.sessionManager._getSessionId()[1] + emitAtDateTime(startingDate) + emitAtDateTime( + new Date( + startingDate.getFullYear(), + startingDate.getMonth(), + startingDate.getDate(), + startingDate.getHours(), + startingDate.getMinutes() + 1 + ) + ) const moreThanADayLater = new Date( startingDate.getFullYear(), @@ -429,7 +460,7 @@ describe('SessionRecording', () => { startingDate.getDate() + 1, startingDate.getHours() + 1 ) - _emit({ event: 123, type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, timestamp: moreThanADayLater.getTime() }) + emitAtDateTime(moreThanADayLater) expect(given.sessionManager._getSessionId()[1]).not.toEqual(startingSessionId) expect(window.rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(2)