diff --git a/services/web/app/src/Features/Analytics/AnalyticsManager.mjs b/services/web/app/src/Features/Analytics/AnalyticsManager.mjs index 915799d02c..7d25613693 100644 --- a/services/web/app/src/Features/Analytics/AnalyticsManager.mjs +++ b/services/web/app/src/Features/Analytics/AnalyticsManager.mjs @@ -453,11 +453,16 @@ async function analyticsIdMiddleware(req, res, next) { const sessionUser = SessionManager.getSessionUser(session) if (sessionUser) { - session.analyticsId = await UserAnalyticsIdCache.getWithMetrics( - sessionUser._id, - // Do not drill down further, this middleware is on all endpoints. - 'analyticsIdMiddleware' - ) + // For old sessions, session.analyticsId is the anon id immediately after login. Do not use it! + session.analyticsId = sessionUser.analyticsId + if (!session.analyticsId) { + session.analyticsId = sessionUser.analyticsId = + await UserAnalyticsIdCache.getWithMetrics( + sessionUser._id, + // Do not drill down further, this middleware is on all endpoints. + 'analyticsIdMiddleware' + ) + } } else if (!session.analyticsId) { // generate an `analyticsId` if needed session.analyticsId = crypto.randomUUID() diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.mjs b/services/web/app/src/Features/Authentication/AuthenticationController.mjs index 16acb68605..e32468b5bc 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.mjs +++ b/services/web/app/src/Features/Authentication/AuthenticationController.mjs @@ -614,6 +614,10 @@ function _afterLoginSessionSetup(req, user, callback) { } delete req.session.__tmp delete req.session.csrfSecret + + // Populate the analyticsId cache in the session AFTER switching it into logged-in mode. + req.session.analyticsId = user.analyticsId + req.session.save(function (err) { if (err) { OError.tag(err, 'error saving regenerated session after login', { diff --git a/services/web/test/acceptance/src/SessionTests.mjs b/services/web/test/acceptance/src/SessionTests.mjs index 17a7d82e89..c2506b0fc3 100644 --- a/services/web/test/acceptance/src/SessionTests.mjs +++ b/services/web/test/acceptance/src/SessionTests.mjs @@ -2,7 +2,10 @@ import { expect } from 'chai' import { setTimeout } from 'node:timers/promises' import UserHelper from './helpers/User.mjs' import redis from './helpers/redis.mjs' +import Metrics from './helpers/metrics.mjs' import UserSessionsRedis from '../../../app/src/Features/User/UserSessionsRedis.mjs' +import UserAnalyticsIdCache from '../../../app/src/Features/Analytics/UserAnalyticsIdCache.mjs' +import Features from '../../../app/src/infrastructure/Features.mjs' const rclient = UserSessionsRedis.client() @@ -356,4 +359,73 @@ describe('Sessions', function () { await checkSessionIsValid(user) }) }) + + describe('analyticsIdMiddleware', function () { + async function getCacheMetric() { + const hit = await Metrics.promises.getMetric( + s => s.includes('analyticsIdMiddleware') && s.includes('hit') + ) + const miss = await Metrics.promises.getMetric( + s => s.includes('analyticsIdMiddleware') && s.includes('miss') + ) + return { hit, miss } + } + let usersAnalyticsId + beforeEach(function () { + if (!Features.hasFeature('saas')) { + this.skip() + } + usersAnalyticsId = this.user1.analyticsId + expect(usersAnalyticsId).to.match(/^[0-9a-f-]{36}$/) + }) + + it('should resolve to the users analyticsId', async function () { + await this.user1.login() + const session = await this.user1.getSession() + expect(session.analyticsId).to.equal(usersAnalyticsId) + }) + + it('should not lookup the users analyticsId after login', async function () { + const prev = await getCacheMetric() + await this.user1.login() + for (let i = 0; i < 5; i++) { + await this.user1.doRequest('GET', '/project') + } + const current = await getCacheMetric() + expect(current).to.deep.equal(prev) + const session = await this.user1.getSession() + expect(session.analyticsId).to.equal(usersAnalyticsId) + }) + + it('should lookup the users analyticsId for old session', async function () { + usersAnalyticsId = this.user1._id + await this.user1.mongoUpdate({ + $set: { analyticsId: usersAnalyticsId }, + }) + const prev = await getCacheMetric() + await this.user1.login() + let session = await this.user1.getSession() + await this.user1.setInSession({ + analyticsId: null, + passport: { + ...session.passport, + user: { + ...session.passport.user, + analyticsId: null, + }, + }, + }) + await UserAnalyticsIdCache.reset() + for (let i = 0; i < 5; i++) { + await this.user1.doRequest('GET', '/project') + } + const current = await getCacheMetric() + expect(current).to.deep.equal({ + hit: prev.hit, + miss: prev.miss + 1, + }) + session = await this.user1.getSession() + expect(session.analyticsId).to.equal(usersAnalyticsId) + }) + }) }) diff --git a/services/web/test/acceptance/src/helpers/User.mjs b/services/web/test/acceptance/src/helpers/User.mjs index 5d592beeca..09e6e01936 100644 --- a/services/web/test/acceptance/src/helpers/User.mjs +++ b/services/web/test/acceptance/src/helpers/User.mjs @@ -1,3 +1,4 @@ +import Crypto from 'node:crypto' import OError from '@overleaf/o-error' import request from './request.js' import settings from '@overleaf/settings' @@ -37,6 +38,7 @@ class User { }) this.signUpDate = options.signUpDate ?? new Date() this.labsProgram = options.labsProgram || false + this.analyticsId = options.analyticsId || Crypto.randomUUID() } getSession(options, callback) { @@ -249,6 +251,7 @@ class User { this.first_name = user.first_name this.referal_id = user.referal_id this.enrollment = user.enrollment + this.analyticsId = user.analyticsId } get(callback) { @@ -444,6 +447,7 @@ class User { emails: this.emails, signUpDate: this.signUpDate, labsProgram: this.labsProgram, + analyticsId: this.analyticsId, }, }, options