Browse Source

[web] fix caching of session.analyticsId (#33300)

* [web] fix caching of session.analyticsId

* [web] disable analyticsIdMiddleware tests for Server Pro/CE

GitOrigin-RevId: 2acf76f937adabd62b1e9f876a858211ef7a13c6
Jakob Ackermann 3 months ago
parent
commit
d61e3f5521

+ 10 - 5
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()

+ 4 - 0
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', {

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

+ 4 - 0
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