From d44e862a0c947bd16bf5c3e84ba58c9b8aa5243b Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Tue, 16 Jun 2026 12:33:05 +0200 Subject: [PATCH] [web] throw early in UserCreator.createNewUser with missing analyticsId (#34664) * [web] throw early in UserCreator.createNewUser with missing analyticsId * [migrations] rerun back-fill of db.users.analyticsId GitOrigin-RevId: eabf16326192fca4dbea00ad643cee4315c426c6 --- .../web/app/src/Features/User/UserCreator.mjs | 3 + .../Features/User/UserRegistrationHandler.mjs | 1 + .../launchpad/app/src/LaunchpadController.mjs | 3 +- .../unit/src/LaunchpadController.test.mjs | 37 +++++++++-- services/web/scripts/devcontainer_setup.mjs | 2 + services/web/scripts/e2e_test_setup.mjs | 2 + .../src/ProjectStructureMongoLockTest.mjs | 2 + .../acceptance/src/helpers/UserHelper.mjs | 5 ++ .../test/unit/src/User/UserCreator.test.mjs | 65 +++++++++++-------- .../src/User/UserRegistrationHandler.test.mjs | 7 +- ...0616070000_back_fill_users_analyticsId.mjs | 24 +++++++ 11 files changed, 117 insertions(+), 34 deletions(-) create mode 100644 tools/migrations/20260616070000_back_fill_users_analyticsId.mjs diff --git a/services/web/app/src/Features/User/UserCreator.mjs b/services/web/app/src/Features/User/UserCreator.mjs index d6e5aa8f57..1171678252 100644 --- a/services/web/app/src/Features/User/UserCreator.mjs +++ b/services/web/app/src/Features/User/UserCreator.mjs @@ -54,6 +54,9 @@ async function recordRegistrationEvent(user) { } async function createNewUser(attributes, options = {}) { + if (!attributes.analyticsId) { + throw new Error('bug: attributes.analyticsId is missing') + } let user = new User() if (attributes.first_name == null || attributes.first_name === '') { diff --git a/services/web/app/src/Features/User/UserRegistrationHandler.mjs b/services/web/app/src/Features/User/UserRegistrationHandler.mjs index 0011774c70..e4c474c675 100644 --- a/services/web/app/src/Features/User/UserRegistrationHandler.mjs +++ b/services/web/app/src/Features/User/UserRegistrationHandler.mjs @@ -100,6 +100,7 @@ const UserRegistrationHandler = { user = await UserRegistrationHandler.registerNewUser({ email, password: crypto.randomBytes(32).toString('hex'), + analyticsId: crypto.randomUUID(), }) } catch (error) { if (error.message === 'EmailAlreadyRegistered') { diff --git a/services/web/modules/launchpad/app/src/LaunchpadController.mjs b/services/web/modules/launchpad/app/src/LaunchpadController.mjs index 691c918e80..f392c4692b 100644 --- a/services/web/modules/launchpad/app/src/LaunchpadController.mjs +++ b/services/web/modules/launchpad/app/src/LaunchpadController.mjs @@ -128,6 +128,7 @@ function registerExternalAuthAdmin(authMethod) { password: crypto.randomBytes(32).toString('hex'), first_name: email, last_name: '', + analyticsId: crypto.randomUUID(), } logger.debug( { email, authMethod }, @@ -212,7 +213,7 @@ async function registerAdmin(req, res) { .json({ message: { type: 'error', text: invalidPassword.message } }) } - const body = { email, password } + const body = { email, password, analyticsId: crypto.randomUUID() } const user = await UserRegistrationHandler.promises.registerNewUser(body) diff --git a/services/web/modules/launchpad/test/unit/src/LaunchpadController.test.mjs b/services/web/modules/launchpad/test/unit/src/LaunchpadController.test.mjs index 7475e522f6..35aebeac77 100644 --- a/services/web/modules/launchpad/test/unit/src/LaunchpadController.test.mjs +++ b/services/web/modules/launchpad/test/unit/src/LaunchpadController.test.mjs @@ -71,6 +71,19 @@ describe('LaunchpadController', function () { }) ) + vi.doMock('crypto', () => ({ + default: (ctx.crypto = { + randomUUID: sinon + .stub() + .returns('8055c676-bcc7-4e64-a66f-8069f9a0bd92'), + randomBytes: sinon.stub().returns({ + toString: () => { + return (ctx.password = 'mock-password123') + }, + }), + }), + })) + ctx.LaunchpadController = (await import(modulePath)).default ctx.email = 'bob@smith.com' @@ -344,7 +357,11 @@ describe('LaunchpadController', function () { 1 ) ctx.UserRegistrationHandler.promises.registerNewUser - .calledWith({ email: ctx.email, password: ctx.password }) + .calledWith({ + email: ctx.email, + password: ctx.password, + analyticsId: '8055c676-bcc7-4e64-a66f-8069f9a0bd92', + }) .should.equal(true) }) @@ -603,7 +620,11 @@ describe('LaunchpadController', function () { 1 ) ctx.UserRegistrationHandler.promises.registerNewUser - .calledWith({ email: ctx.email, password: ctx.password }) + .calledWith({ + email: ctx.email, + password: ctx.password, + analyticsId: '8055c676-bcc7-4e64-a66f-8069f9a0bd92', + }) .should.equal(true) }) @@ -649,7 +670,11 @@ describe('LaunchpadController', function () { 1 ) ctx.UserRegistrationHandler.promises.registerNewUser - .calledWith({ email: ctx.email, password: ctx.password }) + .calledWith({ + email: ctx.email, + password: ctx.password, + analyticsId: '8055c676-bcc7-4e64-a66f-8069f9a0bd92', + }) .should.equal(true) }) }) @@ -693,7 +718,11 @@ describe('LaunchpadController', function () { 1 ) ctx.UserRegistrationHandler.promises.registerNewUser - .calledWith({ email: ctx.email, password: ctx.password }) + .calledWith({ + email: ctx.email, + password: ctx.password, + analyticsId: '8055c676-bcc7-4e64-a66f-8069f9a0bd92', + }) .should.equal(true) }) diff --git a/services/web/scripts/devcontainer_setup.mjs b/services/web/scripts/devcontainer_setup.mjs index f57260f329..612a085dd8 100644 --- a/services/web/scripts/devcontainer_setup.mjs +++ b/services/web/scripts/devcontainer_setup.mjs @@ -14,6 +14,7 @@ import OError from '@overleaf/o-error' import fs from 'node:fs' import Path from 'node:path' import { fileURLToPath } from 'node:url' +import crypto from 'node:crypto' const MONOREPO = Path.dirname( Path.dirname(Path.dirname(Path.dirname(fileURLToPath(import.meta.url)))) @@ -42,6 +43,7 @@ async function createUser( const user = await UserRegistrationHandler.promises.registerNewUser({ email, password: PASSWORD, + analyticsId: crypto.randomUUID(), }) userId = user._id } catch (err) { diff --git a/services/web/scripts/e2e_test_setup.mjs b/services/web/scripts/e2e_test_setup.mjs index a0f918db7e..46f9f50f76 100644 --- a/services/web/scripts/e2e_test_setup.mjs +++ b/services/web/scripts/e2e_test_setup.mjs @@ -11,6 +11,7 @@ import UserDeleter from '../app/src/Features/User/UserDeleter.mjs' import UserRegistrationHandler from '../app/src/Features/User/UserRegistrationHandler.mjs' import HistoryManager from '../app/src/Features/History/HistoryManager.mjs' import ProjectCreationHandler from '../app/src/Features/Project/ProjectCreationHandler.mjs' +import crypto from 'node:crypto' const MONOREPO = Path.dirname( Path.dirname(Path.dirname(Path.dirname(fileURLToPath(import.meta.url)))) @@ -24,6 +25,7 @@ async function createUser(email) { const user = await UserRegistrationHandler.promises.registerNewUser({ email, password: process.env.CYPRESS_DEFAULT_PASSWORD, + analyticsId: crypto.randomUUID(), }) const features = email.startsWith('free+') ? Settings.defaultFeatures diff --git a/services/web/test/acceptance/src/ProjectStructureMongoLockTest.mjs b/services/web/test/acceptance/src/ProjectStructureMongoLockTest.mjs index 6c21681e65..32c47c2f63 100644 --- a/services/web/test/acceptance/src/ProjectStructureMongoLockTest.mjs +++ b/services/web/test/acceptance/src/ProjectStructureMongoLockTest.mjs @@ -20,6 +20,7 @@ import ProjectEntityMongoUpdateHandler from '../../../app/src/Features/Project/P import UserCreator from '../../../app/src/Features/User/UserCreator.mjs' import { expect } from 'chai' import _ from 'lodash' +import crypto from 'node:crypto' // These tests are neither acceptance tests nor unit tests. It's difficult to // test/verify that our locking is doing what we hope. @@ -46,6 +47,7 @@ describe('ProjectStructureMongoLock', function () { const userDetails = { holdingAccount: false, email: 'test@example.com', + analyticsId: crypto.randomUUID(), } UserCreator.createNewUser(userDetails, {}, (err, user) => { this.user = user diff --git a/services/web/test/acceptance/src/helpers/UserHelper.mjs b/services/web/test/acceptance/src/helpers/UserHelper.mjs index b7332f5c3a..ac98e9cac5 100644 --- a/services/web/test/acceptance/src/helpers/UserHelper.mjs +++ b/services/web/test/acceptance/src/helpers/UserHelper.mjs @@ -8,6 +8,7 @@ import UserUpdater from '../../../../app/src/Features/User/UserUpdater.mjs' import moment from 'moment' import fetch from 'node-fetch' import mongodb from 'mongodb-legacy' +import Crypto from 'node:crypto' import { UserAuditLogEntry } from '../../../../app/src/models/UserAuditLogEntry.mjs' @@ -248,6 +249,10 @@ class UserHelper { delete attributes.password } + if (!attributes.analyticsId) { + attributes.analyticsId = Crypto.randomUUID() + } + userHelper.user = await UserCreator.promises.createNewUser(attributes) return userHelper diff --git a/services/web/test/unit/src/User/UserCreator.test.mjs b/services/web/test/unit/src/User/UserCreator.test.mjs index 594c284740..a0fdddd0cf 100644 --- a/services/web/test/unit/src/User/UserCreator.test.mjs +++ b/services/web/test/unit/src/User/UserCreator.test.mjs @@ -102,13 +102,17 @@ describe('UserCreator', function () { ctx.UserCreator = (await import(modulePath)).default ctx.email = 'bob.oswald@gmail.com' + ctx.attributes = { + email: ctx.email, + analyticsId: '8055c676-bcc7-4e64-a66f-8069f9a0bd92', + } }) describe('createNewUser', function () { describe('with callbacks', function () { it('should take the opts and put them in the model', async function (ctx) { const user = await ctx.UserCreator.promises.createNewUser({ - email: ctx.email, + ...ctx.attributes, holdingAccount: true, }) assert.equal(user.email, ctx.email) @@ -118,7 +122,7 @@ describe('UserCreator', function () { it('should use the start of the email if the first name is empty string', async function (ctx) { const user = await ctx.UserCreator.promises.createNewUser({ - email: ctx.email, + ...ctx.attributes, holdingAccount: true, first_name: '', }) @@ -129,7 +133,7 @@ describe('UserCreator', function () { it('should use the first name if passed', async function (ctx) { const user = await ctx.UserCreator.promises.createNewUser({ - email: ctx.email, + ...ctx.attributes, holdingAccount: true, first_name: 'fiiirstname', }) @@ -140,7 +144,7 @@ describe('UserCreator', function () { it('should use the last name if passed', async function (ctx) { const user = await ctx.UserCreator.promises.createNewUser({ - email: ctx.email, + ...ctx.attributes, holdingAccount: true, last_name: 'lastNammmmeee', }) @@ -150,9 +154,9 @@ describe('UserCreator', function () { }) it('should set emails attribute', async function (ctx) { - const user = await ctx.UserCreator.promises.createNewUser({ - email: ctx.email, - }) + const user = await ctx.UserCreator.promises.createNewUser( + ctx.attributes + ) user.email.should.equal(ctx.email) user.emails.length.should.equal(1) user.emails[0].email.should.equal(ctx.email) @@ -161,9 +165,8 @@ describe('UserCreator', function () { }) describe('with affiliations feature', function () { - let attributes, user + let user beforeEach(function (ctx) { - attributes = { email: ctx.email } ctx.Features.hasFeature = sinon .stub() .withArgs('affiliations') @@ -172,7 +175,7 @@ describe('UserCreator', function () { describe('when v1 affiliations API does not return an error', function () { beforeEach(async function (ctx) { - user = await ctx.UserCreator.promises.createNewUser(attributes) + user = await ctx.UserCreator.promises.createNewUser(ctx.attributes) }) it('should flag that affiliation is unchecked', function () { @@ -198,7 +201,7 @@ describe('UserCreator', function () { describe('when v1 affiliations API does return an error', function () { beforeEach(async function (ctx) { ctx.UserUpdater.promises.addAffiliationForNewUser.rejects() - user = await ctx.UserCreator.promises.createNewUser(attributes) + user = await ctx.UserCreator.promises.createNewUser(ctx.attributes) }) it('should flag that affiliation is unchecked', function () { @@ -228,7 +231,7 @@ describe('UserCreator', function () { describe('when v1 affiliations API returns an error and requireAffiliation=true', function () { beforeEach(async function (ctx) { ctx.UserUpdater.promises.addAffiliationForNewUser.rejects() - user = await ctx.UserCreator.promises.createNewUser(attributes) + user = await ctx.UserCreator.promises.createNewUser(ctx.attributes) }) it('should flag that affiliation is unchecked', function () { @@ -257,8 +260,7 @@ describe('UserCreator', function () { }) it('should not add affiliation when without affiliation feature', async function (ctx) { - const attributes = { email: ctx.email } - await ctx.UserCreator.promises.createNewUser(attributes) + await ctx.UserCreator.promises.createNewUser(ctx.attributes) sinon.assert.notCalled( ctx.UserUpdater.promises.addAffiliationForNewUser ) @@ -268,7 +270,7 @@ describe('UserCreator', function () { describe('with promises', function () { it('should take the opts and put them in the model', async function (ctx) { const opts = { - email: ctx.email, + ...ctx.attributes, holdingAccount: true, } const user = await ctx.UserCreator.promises.createNewUser(opts) @@ -282,8 +284,9 @@ describe('UserCreator', function () { .stub() .withArgs('affiliations') .returns(true) - const attributes = { email: ctx.email } - const user = await ctx.UserCreator.promises.createNewUser(attributes) + const user = await ctx.UserCreator.promises.createNewUser( + ctx.attributes + ) sinon.assert.calledOnce( ctx.UserUpdater.promises.addAffiliationForNewUser ) @@ -296,8 +299,7 @@ describe('UserCreator', function () { it('should not add affiliation when without affiliation feature', async function (ctx) { ctx.Features.hasFeature = sinon.stub().returns(false) - const attributes = { email: ctx.email } - await ctx.UserCreator.promises.createNewUser(attributes) + await ctx.UserCreator.promises.createNewUser(ctx.attributes) sinon.assert.notCalled( ctx.UserUpdater.promises.addAffiliationForNewUser ) @@ -305,7 +307,7 @@ describe('UserCreator', function () { it('should include SAML provider ID with email', async function (ctx) { const attributes = { - email: ctx.email, + ...ctx.attributes, samlIdentifiers: [{ email: ctx.email, providerId: '1' }], } const user = await ctx.UserCreator.promises.createNewUser(attributes) @@ -313,9 +315,9 @@ describe('UserCreator', function () { }) it('should fire an analytics event and user property on registration', async function (ctx) { - const user = await ctx.UserCreator.promises.createNewUser({ - email: ctx.email, - }) + const user = await ctx.UserCreator.promises.createNewUser( + ctx.attributes + ) assert.equal(user.email, ctx.email) sinon.assert.calledWith( ctx.Analytics.recordEventForUserInBackground, @@ -331,9 +333,9 @@ describe('UserCreator', function () { it('should schedule post registration jobs on registration with saas feature', async function (ctx) { ctx.Features.hasFeature = sinon.stub().withArgs('saas').returns(true) - const user = await ctx.UserCreator.promises.createNewUser({ - email: ctx.email, - }) + const user = await ctx.UserCreator.promises.createNewUser( + ctx.attributes + ) assert.equal(user.email, ctx.email) sinon.assert.calledWith( ctx.UserOnboardingEmailManager.scheduleOnboardingEmail, @@ -347,8 +349,7 @@ describe('UserCreator', function () { }) it('should not schedule post registration checks when without saas feature', async function (ctx) { - const attributes = { email: ctx.email } - await ctx.UserCreator.promises.createNewUser(attributes) + await ctx.UserCreator.promises.createNewUser(ctx.attributes) sinon.assert.notCalled( ctx.UserOnboardingEmailManager.scheduleOnboardingEmail ) @@ -357,6 +358,14 @@ describe('UserCreator', function () { .schedulePostRegistrationAnalytics ) }) + + it('should throw without analyticsId', async function (ctx) { + await expect( + ctx.UserCreator.promises.createNewUser({ email: ctx.email }) + ).to.eventually.be.rejectedWith( + /bug: attributes.analyticsId is missing/ + ) + }) }) }) }) diff --git a/services/web/test/unit/src/User/UserRegistrationHandler.test.mjs b/services/web/test/unit/src/User/UserRegistrationHandler.test.mjs index 563caa2eb6..59ca9888dc 100644 --- a/services/web/test/unit/src/User/UserRegistrationHandler.test.mjs +++ b/services/web/test/unit/src/User/UserRegistrationHandler.test.mjs @@ -68,7 +68,11 @@ describe('UserRegistrationHandler', function () { })) vi.doMock('crypto', () => ({ - default: (ctx.crypto = {}), + default: (ctx.crypto = { + randomUUID: sinon + .stub() + .returns('8055c676-bcc7-4e64-a66f-8069f9a0bd92'), + }), })) vi.doMock('../../../../app/src/Features/Email/EmailHandler', () => ({ @@ -287,6 +291,7 @@ describe('UserRegistrationHandler', function () { sinon.assert.calledWith(ctx.handler.promises.registerNewUser, { email: ctx.email, password: ctx.password, + analyticsId: '8055c676-bcc7-4e64-a66f-8069f9a0bd92', }) }) diff --git a/tools/migrations/20260616070000_back_fill_users_analyticsId.mjs b/tools/migrations/20260616070000_back_fill_users_analyticsId.mjs new file mode 100644 index 0000000000..c7327a55da --- /dev/null +++ b/tools/migrations/20260616070000_back_fill_users_analyticsId.mjs @@ -0,0 +1,24 @@ +import { db } from './lib/mongodb.mjs' +import { batchedUpdate } from '@overleaf/mongo-utils/batchedUpdate.js' + +const tags = ['saas', 'server-ce', 'server-pro', 'nonblocking'] + +const migrate = async () => { + await batchedUpdate(db.users, { analyticsId: { $exists: false } }, [ + { $set: { analyticsId: { $toString: '$_id' } } }, + ]) +} + +const rollback = async () => { + await batchedUpdate( + db.users, + { $expr: { $eq: [{ $strLenCP: '$analyticsId' }, 24] } }, + { $unset: { analyticsId: 1 } } + ) +} + +export default { + tags, + migrate, + rollback, +}