From b84e26671e94ea4633b875ac5e2821a9273c3c7e Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Wed, 24 Jun 2026 22:26:42 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=95=92=20feat:=20Track=20Terms=20Acceptan?= =?UTF-8?q?ce=20Timestamp=20(#10810)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: add terms acceptance timestamp tracking and migration script * feat: update migration script to use countUsers method for user count * Update config/migrate-terms-timestamp.js Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * feat: enhance terms acceptance response to include acceptance timestamp * fix: make terms acceptance idempotent and fail migration on partial errors Preserve the original termsAcceptedAt on repeat accepts within a terms cycle so retried or duplicate requests no longer overwrite the first acceptance time. Exit the migration script with a non-zero status when any per-user update fails so partial failures are not reported as successful. * style: fix import ordering in data-provider mutations * refactor: record terms acceptance atomically to preserve first-accept time Replace the read-then-write in acceptTermsController with a single atomic acceptTerms method that conditionally stamps termsAcceptedAt via an $ifNull aggregation update. This removes the TOCTOU window where two concurrent first-time accepts could overwrite the earlier acceptance timestamp, while still preserving an existing timestamp and backfilling legacy accepted users. * fix: run terms timestamp migration under system tenant context Wrap the count, cursor scan, and per-user updates in runAsSystem so the tenant isolation plugin does not throw under TENANT_ISOLATION_STRICT or scope the cross-tenant migration to a non-existent tenant, matching the other maintenance migrations. * fix: guard terms backfill against concurrent acceptances Add the missing-timestamp predicate to the per-user updateOne filter so a user who accepts through the API between the cursor read and the write keeps their real acceptance time instead of being overwritten with createdAt. Track modified vs skipped so the summary reflects skips. * fix: scope terms backfill to still-accepted users Add termsAccepted: true to the per-user updateOne filter so a reset that clears acceptance between the cursor read and the write is not re-stamped with createdAt, which would otherwise poison the next acceptance cycle through the $ifNull preserve in acceptTerms. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- api/server/controllers/UserController.js | 14 +- api/server/controllers/UserController.spec.js | 48 ++++++- client/src/data-provider/mutations.ts | 1 + config/list-users.js | 9 +- config/migrate-terms-timestamp.js | 136 ++++++++++++++++++ config/reset-terms.js | 5 +- package.json | 1 + packages/api/src/utils/env.ts | 1 + packages/data-provider/src/types.ts | 4 +- .../src/methods/user.methods.spec.ts | 59 +++++++- packages/data-schemas/src/methods/user.ts | 24 ++++ packages/data-schemas/src/schema/user.ts | 4 + packages/data-schemas/src/types/user.ts | 2 + 13 files changed, 299 insertions(+), 9 deletions(-) create mode 100644 config/migrate-terms-timestamp.js diff --git a/api/server/controllers/UserController.js b/api/server/controllers/UserController.js index 5fd43b66d1..fe3a206458 100644 --- a/api/server/controllers/UserController.js +++ b/api/server/controllers/UserController.js @@ -86,11 +86,14 @@ const getUserController = async (req, res) => { const getTermsStatusController = async (req, res) => { try { - const user = await db.getUserById(req.user.id, 'termsAccepted'); + const user = await db.getUserById(req.user.id, 'termsAccepted termsAcceptedAt'); if (!user) { return res.status(404).json({ message: 'User not found' }); } - res.status(200).json({ termsAccepted: !!user.termsAccepted }); + res.status(200).json({ + termsAccepted: !!user.termsAccepted, + termsAcceptedAt: user.termsAcceptedAt || null, + }); } catch (error) { logger.error('Error fetching terms acceptance status:', error); res.status(500).json({ message: 'Error fetching terms acceptance status' }); @@ -99,11 +102,14 @@ const getTermsStatusController = async (req, res) => { const acceptTermsController = async (req, res) => { try { - const user = await db.updateUser(req.user.id, { termsAccepted: true }); + const user = await db.acceptTerms(req.user.id); if (!user) { return res.status(404).json({ message: 'User not found' }); } - res.status(200).json({ message: 'Terms accepted successfully' }); + res.status(200).json({ + message: 'Terms accepted successfully', + termsAcceptedAt: user.termsAcceptedAt, + }); } catch (error) { logger.error('Error accepting terms:', error); res.status(500).json({ message: 'Error accepting terms' }); diff --git a/api/server/controllers/UserController.spec.js b/api/server/controllers/UserController.spec.js index 6a165fe718..1466dce291 100644 --- a/api/server/controllers/UserController.spec.js +++ b/api/server/controllers/UserController.spec.js @@ -40,6 +40,7 @@ jest.mock('~/models', () => { deleteConvos: jest.fn().mockResolvedValue(undefined), deleteFiles: jest.fn().mockResolvedValue(undefined), updateUser: jest.fn(), + acceptTerms: jest.fn(), getUserById: jest.fn().mockResolvedValue(null), findToken: jest.fn(), getFiles: jest.fn().mockResolvedValue([]), @@ -111,11 +112,12 @@ afterEach(async () => { const { deleteUserController, getUserController, + acceptTermsController, resendVerificationController, verifyEmailController, } = require('./UserController'); const { Group } = require('~/db/models'); -const { deleteConvos } = require('~/models'); +const { deleteConvos, acceptTerms } = require('~/models'); const { verifyEmail, resendVerificationEmail } = require('~/server/services/AuthService'); describe('verifyEmailController', () => { @@ -256,6 +258,50 @@ describe('getUserController', () => { }); }); +describe('acceptTermsController', () => { + const mockRes = { + status: jest.fn().mockReturnThis(), + json: jest.fn().mockReturnThis(), + }; + + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('returns 404 when the user does not exist', async () => { + acceptTerms.mockResolvedValueOnce(null); + + await acceptTermsController({ user: { id: 'missing-user' } }, mockRes); + + expect(acceptTerms).toHaveBeenCalledWith('missing-user'); + expect(mockRes.status).toHaveBeenCalledWith(404); + expect(mockRes.json).toHaveBeenCalledWith({ message: 'User not found' }); + }); + + it('returns the recorded acceptance timestamp on success', async () => { + const acceptedAt = new Date('2026-06-14T10:00:00.000Z'); + acceptTerms.mockResolvedValueOnce({ termsAccepted: true, termsAcceptedAt: acceptedAt }); + + await acceptTermsController({ user: { id: 'user-id' } }, mockRes); + + expect(acceptTerms).toHaveBeenCalledWith('user-id'); + expect(mockRes.status).toHaveBeenCalledWith(200); + expect(mockRes.json).toHaveBeenCalledWith({ + message: 'Terms accepted successfully', + termsAcceptedAt: acceptedAt, + }); + }); + + it('returns 500 when the update throws', async () => { + acceptTerms.mockRejectedValueOnce(new Error('db down')); + + await acceptTermsController({ user: { id: 'user-id' } }, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(500); + expect(mockRes.json).toHaveBeenCalledWith({ message: 'Error accepting terms' }); + }); +}); + describe('deleteUserController', () => { const mockRes = { status: jest.fn().mockReturnThis(), diff --git a/client/src/data-provider/mutations.ts b/client/src/data-provider/mutations.ts index f6aede97c8..c919e0eaf5 100644 --- a/client/src/data-provider/mutations.ts +++ b/client/src/data-provider/mutations.ts @@ -1126,6 +1126,7 @@ export const useAcceptTermsMutation = ( onSuccess: (data, variables, context) => { queryClient.setQueryData([QueryKeys.userTerms], { termsAccepted: true, + termsAcceptedAt: data.termsAcceptedAt, }); options?.onSuccess?.(data, variables, context); }, diff --git a/config/list-users.js b/config/list-users.js index 7315ff7251..1c6f7445c2 100644 --- a/config/list-users.js +++ b/config/list-users.js @@ -7,7 +7,10 @@ const connect = require('./connect'); const listUsers = async () => { try { await connect(); - const users = await User.find({}, 'email provider avatar username name createdAt'); + const users = await User.find( + {}, + 'email provider avatar username name createdAt termsAccepted termsAcceptedAt', + ); console.log('\nUser List:'); console.log('----------------------------------------'); @@ -18,6 +21,10 @@ const listUsers = async () => { console.log(`Name: ${user.name || 'N/A'}`); console.log(`Provider: ${user.provider || 'email'}`); console.log(`Created: ${user.createdAt}`); + console.log(`Terms Accepted: ${user.termsAccepted ? 'Yes' : 'No'}`); + console.log( + `Terms Accepted At: ${user.termsAcceptedAt ? user.termsAcceptedAt.toISOString() : 'N/A'}`, + ); console.log('----------------------------------------'); }); diff --git a/config/migrate-terms-timestamp.js b/config/migrate-terms-timestamp.js new file mode 100644 index 0000000000..5d67249f3f --- /dev/null +++ b/config/migrate-terms-timestamp.js @@ -0,0 +1,136 @@ +const path = require('path'); +const mongoose = require('mongoose'); +const { runAsSystem } = require('@librechat/data-schemas'); +const { User } = require('@librechat/data-schemas').createModels(mongoose); +const { countUsers } = require('@librechat/data-schemas').createMethods(mongoose); +require('module-alias')({ base: path.resolve(__dirname, '..', 'api') }); +const { askQuestion, silentExit } = require('./helpers'); +const connect = require('./connect'); + +/** + * Migration script for Terms Acceptance Timestamp Tracking + * + * This script migrates existing users who have termsAccepted: true but no termsAcceptedAt timestamp. + * For these users, it sets termsAcceptedAt to their account creation date (createdAt) as a fallback. + * + * Usage: npm run migrate:terms-timestamp + */ +(async () => { + await connect(); + + console.purple('--------------------------'); + console.purple('Migrate Terms Acceptance Timestamps'); + console.purple('--------------------------'); + + // Count users that need migration. This script spans every tenant, so run + // it under system context or the tenant isolation plugin throws under + // TENANT_ISOLATION_STRICT=true and scopes to a non-existent tenant otherwise. + const usersToMigrate = await runAsSystem(() => + countUsers({ + termsAccepted: true, + $or: [{ termsAcceptedAt: null }, { termsAcceptedAt: { $exists: false } }], + }), + ); + + if (usersToMigrate === 0) { + console.green( + 'No users need migration. All users with termsAccepted: true already have a termsAcceptedAt timestamp.', + ); + silentExit(0); + } + + console.yellow( + `Found ${usersToMigrate} user(s) with termsAccepted: true but no termsAcceptedAt timestamp.`, + ); + console.yellow( + 'These users will have their termsAcceptedAt set to their account creation date (createdAt).', + ); + + const confirm = await askQuestion('Are you sure you want to proceed? (y/n): '); + + if (confirm.toLowerCase() !== 'y') { + console.yellow('Operation cancelled.'); + silentExit(0); + } + + try { + // Scan and update across every tenant under system context, matching the + // other cross-tenant migrations, so the tenant isolation plugin does not + // throw or scope queries to a non-existent tenant. + await runAsSystem(async () => { + const cursor = User.find({ + termsAccepted: true, + $or: [{ termsAcceptedAt: null }, { termsAcceptedAt: { $exists: false } }], + }).cursor(); + + let migratedCount = 0; + let skippedCount = 0; + let errorCount = 0; + + for await (const user of cursor) { + try { + // Use createdAt as fallback for termsAcceptedAt + const termsAcceptedAt = user.createdAt || new Date(); + if (!user.createdAt) { + console.yellow( + `Warning: User ${user._id} has no createdAt, using current date for termsAcceptedAt`, + ); + } + // Only backfill users who are still accepted and have no timestamp. + // If they accept through the API or get reset between the cursor read + // and this write, the filter no longer matches and their state is kept. + const result = await User.updateOne( + { + _id: user._id, + termsAccepted: true, + $or: [{ termsAcceptedAt: null }, { termsAcceptedAt: { $exists: false } }], + }, + { $set: { termsAcceptedAt } }, + ); + + if (result.modifiedCount > 0) { + migratedCount++; + if (migratedCount % 100 === 0) { + console.yellow(`Migrated ${migratedCount} users...`); + } + } else { + skippedCount++; + } + } catch (error) { + console.red(`Error migrating user ${user._id}: ${error.message}`); + errorCount++; + } + } + + console.green(`Migration complete!`); + console.green(`Successfully migrated: ${migratedCount} user(s)`); + if (skippedCount > 0) { + console.yellow( + `Skipped ${skippedCount} user(s) whose terms state changed during migration.`, + ); + } + if (errorCount > 0) { + console.red(`Errors encountered: ${errorCount}`); + silentExit(1); + } + }); + } catch (error) { + console.red('Error during migration:', error); + silentExit(1); + } + + silentExit(0); +})(); + +process.on('uncaughtException', (err) => { + if (!err.message.includes('fetch failed')) { + console.error('There was an uncaught error:'); + console.error(err); + } + + if (err.message.includes('fetch failed')) { + return; + } else { + process.exit(1); + } +}); diff --git a/config/reset-terms.js b/config/reset-terms.js index 0b3be80661..f6d2ea152d 100644 --- a/config/reset-terms.js +++ b/config/reset-terms.js @@ -21,7 +21,10 @@ const connect = require('./connect'); } try { - const result = await User.updateMany({}, { $set: { termsAccepted: false } }); + const result = await User.updateMany( + {}, + { $set: { termsAccepted: false, termsAcceptedAt: null } }, + ); console.green(`Updated ${result.modifiedCount} user(s).`); } catch (error) { console.red('Error resetting terms acceptance:', error); diff --git a/package.json b/package.json index a22677bb8f..967b229bab 100644 --- a/package.json +++ b/package.json @@ -96,6 +96,7 @@ "b:balance": "bun config/add-balance.js", "b:list-balances": "bun config/list-balances.js", "reset-terms": "node config/reset-terms.js", + "migrate:terms-timestamp": "node config/migrate-terms-timestamp.js", "flush-cache": "node config/flush-cache.js", "migrate:agent-permissions:dry-run": "node config/migrate-agent-permissions.js --dry-run", "migrate:agent-permissions": "node config/migrate-agent-permissions.js", diff --git a/packages/api/src/utils/env.ts b/packages/api/src/utils/env.ts index 7d331ada95..3349a66ecc 100644 --- a/packages/api/src/utils/env.ts +++ b/packages/api/src/utils/env.ts @@ -26,6 +26,7 @@ const ALLOWED_USER_FIELDS = [ 'emailVerified', 'twoFactorEnabled', 'termsAccepted', + 'termsAcceptedAt', ] as const; type AllowedUserField = (typeof ALLOWED_USER_FIELDS)[number]; diff --git a/packages/data-provider/src/types.ts b/packages/data-provider/src/types.ts index f3d40b139b..013d9f2943 100644 --- a/packages/data-provider/src/types.ts +++ b/packages/data-provider/src/types.ts @@ -740,10 +740,12 @@ export type TCustomConfigSpeechResponse = { [key: string]: string }; export type TUserTermsResponse = { termsAccepted: boolean; + termsAcceptedAt: Date | string | null; }; export type TAcceptTermsResponse = { - success: boolean; + message: string; + termsAcceptedAt: Date | string; }; export type TBannerResponse = TBanner | null; diff --git a/packages/data-schemas/src/methods/user.methods.spec.ts b/packages/data-schemas/src/methods/user.methods.spec.ts index 8869b2006e..a2d3ea90cd 100644 --- a/packages/data-schemas/src/methods/user.methods.spec.ts +++ b/packages/data-schemas/src/methods/user.methods.spec.ts @@ -1,9 +1,9 @@ import mongoose from 'mongoose'; import { MongoMemoryServer } from 'mongodb-memory-server'; import type * as t from '~/types'; +import balanceSchema from '~/schema/balance'; import { createUserMethods } from './user'; import userSchema from '~/schema/user'; -import balanceSchema from '~/schema/balance'; /** Mocking crypto for generateToken */ jest.mock('~/crypto', () => ({ @@ -369,6 +369,63 @@ describe('User Methods - Database Tests', () => { }); }); + describe('acceptTerms', () => { + test('sets termsAccepted and stamps termsAcceptedAt on first acceptance', async () => { + const user = await User.create({ + name: 'Terms User', + email: 'terms@example.com', + provider: 'local', + }); + + const before = Date.now(); + const updated = await methods.acceptTerms(user._id?.toString() ?? ''); + const after = Date.now(); + + expect(updated?.termsAccepted).toBe(true); + expect(updated?.termsAcceptedAt).toBeInstanceOf(Date); + const stamped = (updated?.termsAcceptedAt as Date).getTime(); + expect(stamped).toBeGreaterThanOrEqual(before - 1000); + expect(stamped).toBeLessThanOrEqual(after + 1000); + }); + + test('preserves the original termsAcceptedAt on repeat acceptance', async () => { + const originalAcceptedAt = new Date('2026-01-01T00:00:00.000Z'); + const user = await User.create({ + name: 'Repeat User', + email: 'repeat@example.com', + provider: 'local', + termsAccepted: true, + termsAcceptedAt: originalAcceptedAt, + }); + + const updated = await methods.acceptTerms(user._id?.toString() ?? ''); + + expect(updated?.termsAccepted).toBe(true); + expect((updated?.termsAcceptedAt as Date).getTime()).toBe(originalAcceptedAt.getTime()); + }); + + test('backfills termsAcceptedAt for a legacy accepted user without a timestamp', async () => { + const user = await User.create({ + name: 'Legacy User', + email: 'legacy@example.com', + provider: 'local', + termsAccepted: true, + }); + + const updated = await methods.acceptTerms(user._id?.toString() ?? ''); + + expect(updated?.termsAccepted).toBe(true); + expect(updated?.termsAcceptedAt).toBeInstanceOf(Date); + }); + + test('returns null for non-existent user', async () => { + const fakeId = new mongoose.Types.ObjectId(); + const result = await methods.acceptTerms(fakeId.toString()); + + expect(result).toBeNull(); + }); + }); + describe('deleteUserById', () => { test('should delete user by ID', async () => { const user = await User.create({ diff --git a/packages/data-schemas/src/methods/user.ts b/packages/data-schemas/src/methods/user.ts index 56ba50fae4..4376fc07a0 100644 --- a/packages/data-schemas/src/methods/user.ts +++ b/packages/data-schemas/src/methods/user.ts @@ -26,6 +26,7 @@ export function createUserMethods(mongoose: typeof import('mongoose')): { returnUser?: boolean, ) => Promise>; updateUser: (userId: string, updateData: Partial) => Promise; + acceptTerms: (userId: string) => Promise; searchUsers: ({ searchPattern, limit, @@ -253,6 +254,28 @@ export function createUserMethods(mongoose: typeof import('mongoose')): { }).lean(); } + /** + * Atomically records terms acceptance for a user. + * Sets termsAccepted and, only when no timestamp is already stored, stamps + * termsAcceptedAt with the server time so the first acceptance within a terms + * cycle is preserved across concurrent or repeated requests. + */ + async function acceptTerms(userId: string): Promise { + const User = mongoose.models.User; + return await User.findByIdAndUpdate( + userId, + [ + { + $set: { + termsAccepted: true, + termsAcceptedAt: { $ifNull: ['$termsAcceptedAt', '$$NOW'] }, + }, + }, + ], + { new: true, runValidators: true }, + ).lean(); + } + /** * Retrieve a user by ID and convert the found user document to a plain object. */ @@ -511,6 +534,7 @@ export function createUserMethods(mongoose: typeof import('mongoose')): { countUsers, createUser, updateUser, + acceptTerms, searchUsers, getUserById, generateToken, diff --git a/packages/data-schemas/src/schema/user.ts b/packages/data-schemas/src/schema/user.ts index 8c407e3d36..706c73403b 100644 --- a/packages/data-schemas/src/schema/user.ts +++ b/packages/data-schemas/src/schema/user.ts @@ -127,6 +127,10 @@ const userSchema: Schema = new Schema( type: Boolean, default: false, }, + termsAcceptedAt: { + type: Date, + default: null, + }, personalization: { type: { memories: { diff --git a/packages/data-schemas/src/types/user.ts b/packages/data-schemas/src/types/user.ts index b9296a4be5..5b302ad275 100644 --- a/packages/data-schemas/src/types/user.ts +++ b/packages/data-schemas/src/types/user.ts @@ -48,6 +48,7 @@ export interface IUser extends Document { }>; expiresAt?: Date; termsAccepted?: boolean; + termsAcceptedAt?: Date | null; personalization?: { memories?: boolean; }; @@ -93,6 +94,7 @@ export interface UpdateUserRequest { plugins?: string[]; twoFactorEnabled?: boolean; termsAccepted?: boolean; + termsAcceptedAt?: Date | null; personalization?: { memories?: boolean; };