From 9a99e2b601cdefb73b8ab11757c6890593355bdc Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 12:39:34 -0700 Subject: [PATCH 1/2] Serialize administrator mutations across workers Use one Mongo primary-key lock for role, ban and administrative deletion handlers and applying synthetic-account purge. Revalidate the actor and recount under the lock. Exclude administrators from purge candidates. No expiry or takeover: a paused worker and a rejected write may still complete later. Fail closed after callback or uncertain release failure; known pre-write refusals unlock normally. Document operator quiescence, database-operation drain, owner-matched recovery and mixed-version rollout. Controlled old-route probes leave zero administrators in demotion races. An unlock-on-error mutant also leaves zero after a late write; restored. Final concurrency suite 11/11 on real standalone Mongo7; focused regression suites 72/72. Aggregate including global limiter census is77/78: unrelated null-header probes fail in that existing instrument, including an isolated run. Current-head CI remains required; do not report that aggregate green. Source typecheck and targeted lint have no errors. Written by Sprint Impl, a Commonly agent https://commonly.me/v2/pods/6addd4d47f8fc798a6f66bbb#message-85722 Co-authored-by: sprint-impl Agent-Model: codex/gpt-6.1-sol/medium --- .../service/adminPauseLeavesHostedRow.test.js | 7 + .../service/adminRoleConcurrency.test.js | 153 ++++++++++ .../__tests__/unit/routes/admin.users.test.js | 29 +- .../routes/routeRateLimitGuard.baseline.json | 2 - .../syntheticAccountPurgeService.test.js | 22 ++ backend/models/AdminUserMutationLock.ts | 14 + backend/routes/admin/users.ts | 261 ++++++++++-------- backend/services/adminUserMutationService.ts | 55 ++++ .../services/syntheticAccountPurgeService.ts | 22 +- docs/runbooks/admin-user-mutation-lock.md | 66 +++++ 10 files changed, 497 insertions(+), 134 deletions(-) create mode 100644 backend/__tests__/service/adminRoleConcurrency.test.js create mode 100644 backend/models/AdminUserMutationLock.ts create mode 100644 backend/services/adminUserMutationService.ts create mode 100644 docs/runbooks/admin-user-mutation-lock.md diff --git a/backend/__tests__/service/adminPauseLeavesHostedRow.test.js b/backend/__tests__/service/adminPauseLeavesHostedRow.test.js index bde8626c6f..6c9e4e3a3e 100644 --- a/backend/__tests__/service/adminPauseLeavesHostedRow.test.js +++ b/backend/__tests__/service/adminPauseLeavesHostedRow.test.js @@ -1,3 +1,10 @@ +// This fixture stubs User and auth; real lock/authorization behavior is tested +// across Mongo in service/adminRoleConcurrency.test.js. +jest.mock('../../services/adminUserMutationService', () => ({ + withAdminUserMutation: (_actor, operation) => operation(), + AdminUserMutationError: class extends Error {}, +})); + // TASK-172 §10 step 6b: the admin pause/resume lever projects onto the rows an // InstallableInstallation owns, and it selects them by `installationId` // (`routes/admin/installables.ts:108` / `:145`). A hosted-MCP connection is diff --git a/backend/__tests__/service/adminRoleConcurrency.test.js b/backend/__tests__/service/adminRoleConcurrency.test.js new file mode 100644 index 0000000000..ff2c74b046 --- /dev/null +++ b/backend/__tests__/service/adminRoleConcurrency.test.js @@ -0,0 +1,153 @@ +const express = require('express'); +const request = require('supertest'); +const { setupMongoDb, closeMongoDb, clearMongoDb } = require('../utils/testUtils'); +const User = require('../../models/User'); +const { issueSessionToken } = require('../../services/sessionTokenService'); +const routes = require('../../routes/admin/users'); + +const deferred = () => { + let resolve; + const promise = new Promise((done) => { resolve = done; }); + return { promise, resolve }; +}; +const makeAdmin = (name) => User.create({ username: name, email: `${name}@example.test`, + password: 'Role-concurrency-test-123!', role: 'admin' }); + +describe('Administrator role concurrency', () => { + const app = express(); app.use(express.json()); app.use('/api/admin/users', routes); + beforeAll(async () => { process.env.JWT_SECRET = 'admin-role-service-test'; await setupMongoDb(); }); + afterEach(async () => { jest.restoreAllMocks(); await clearMongoDb(); }); + afterAll(closeMongoDb); + + it.each(['demote', 'delete'])('preserves an administrator when demotion races %s', async (operation) => { + const [alice, bob] = await Promise.all([makeAdmin('role-alice'), makeAdmin('role-bob')]); + const firstCount = deferred(); + const releaseFirst = deferred(); + const count = User.countDocuments.bind(User); + let calls = 0; + jest.spyOn(User, 'countDocuments').mockImplementation(async (...args) => { + const value = await count(...args); + if (++calls === 1) { firstCount.resolve(); await releaseFirst.promise; } + return value; + }); + const first = request(app).patch(`/api/admin/users/${bob._id}/role`) + .set('Authorization', `Bearer ${issueSessionToken(alice)}`).send({ role: 'user' }).then((res) => res); + await firstCount.promise; + let second; + try { + const pending = operation === 'demote' + ? request(app).patch(`/api/admin/users/${alice._id}/role`).send({ role: 'user' }) + : request(app).delete(`/api/admin/users/${alice._id}`); + second = await pending.set('Authorization', `Bearer ${issueSessionToken(bob)}`); + } finally { releaseFirst.resolve(); } + const firstResult = await first; + expect(firstResult.status).toBe(200); + expect({ secondStatus: second.status, remainingAdmins: await count({ role: 'admin' }) }) + .toEqual({ secondStatus: 503, remainingAdmins: 1 }); + }); + it.each(['demote', 'delete', 'ban'])('never steals an abandoned lock for %s', async (operation) => { + const [alice, bob] = await Promise.all([makeAdmin('lock-alice'), makeAdmin('lock-bob')]); + const Lock = require('../../models/AdminUserMutationLock'); + await Lock.create({ _id: 'global-admin-user-mutations', owner: 'stopped-worker', acquiredAt: new Date('2000-01-01') }); + const pending = operation === 'demote' + ? request(app).patch(`/api/admin/users/${bob._id}/role`).send({ role: 'user' }) + : operation === 'delete' ? request(app).delete(`/api/admin/users/${bob._id}`) + : request(app).patch(`/api/admin/users/${bob._id}/ban`).send({ banned: true }); + const res = await pending.set('Authorization', `Bearer ${issueSessionToken(alice)}`); + expect(res.status).toBe(503); expect(res.body.code).toBe('ADMIN_MUTATION_BUSY'); + expect((await Lock.findById('global-admin-user-mutations')).owner).toBe('stopped-worker'); + expect(await User.countDocuments({ role: 'admin' })).toBe(2); + }); + + it('rechecks actor authorization and only releases a known pre-write refusal', async () => { + const alice = await makeAdmin('stale-alice'); + const { withAdminUserMutation } = require('../../services/adminUserMutationService'); + const Lock = require('../../models/AdminUserMutationLock'); + const operation = jest.fn(); + await User.updateOne({ _id: alice._id }, { $set: { role: 'user' } }); + await expect(withAdminUserMutation(String(alice._id), operation)).rejects.toMatchObject({ status: 403 }); + expect(operation).not.toHaveBeenCalled(); + expect(await Lock.countDocuments()).toBe(0); + await User.updateOne({ _id: alice._id }, { $set: { role: 'admin' } }); + await expect(withAdminUserMutation(String(alice._id), async () => { throw new Error('write failure'); })) + .rejects.toThrow('write failure'); + expect(await Lock.countDocuments()).toBe(1); + await expect(withAdminUserMutation(String(alice._id), async () => 'next writer')) + .rejects.toMatchObject({ code: 'ADMIN_MUTATION_BUSY' }); + }); + + it('does not send success before releasing the lock, and fails closed if release fails', async () => { + const [alice, bob] = await Promise.all([makeAdmin('release-alice'), makeAdmin('release-bob')]); + const Lock = require('../../models/AdminUserMutationLock'); + jest.spyOn(Lock, 'deleteOne').mockRejectedValueOnce(new Error('release unavailable')); + const res = await request(app).patch(`/api/admin/users/${bob._id}/role`) + .set('Authorization', `Bearer ${issueSessionToken(alice)}`).send({ role: 'user' }); + expect(res.status).toBe(500); + expect(await Lock.countDocuments()).toBe(1); + const retry = await request(app).patch(`/api/admin/users/${bob._id}/role`) + .set('Authorization', `Bearer ${issueSessionToken(alice)}`).send({ role: 'user' }); + expect(retry.status).toBe(503); + expect(await User.countDocuments({ role: 'admin' })).toBe(1); + }); + + it('serializes ban with a concurrent promotion', async () => { + const alice = await makeAdmin('promotion-alice'); + const bob = await User.create({ username: 'promotion-bob', email: 'promotion-bob@example.test', + password: 'Role-concurrency-test-123!', role: 'user' }); + const writing = deferred(); const resume = deferred(); + const save = User.prototype.save; + jest.spyOn(User.prototype, 'save').mockImplementation(async function (...args) { + if (String(this._id) === String(bob._id) && this.role === 'admin') { + writing.resolve(); await resume.promise; + } + return save.apply(this, args); + }); + const first = request(app).patch(`/api/admin/users/${bob._id}/role`) + .set('Authorization', `Bearer ${issueSessionToken(alice)}`).send({ role: 'admin' }).then((res) => res); + await writing.promise; + let second; + try { + second = await request(app).patch(`/api/admin/users/${bob._id}/ban`) + .set('Authorization', `Bearer ${issueSessionToken(alice)}`).send({ banned: true }); + } finally { resume.resolve(); } + expect((await first).status).toBe(200); expect(second.status).toBe(503); + const target = await User.findById(bob._id); + expect(target.role).toBe('admin'); expect(target.banned).not.toBe(true); + }); + + it('retains the lock when a rejected operation can still finish its write later', async () => { + const [alice, bob] = await Promise.all([makeAdmin('uncertain-alice'), makeAdmin('uncertain-bob')]); + const { withAdminUserMutation } = require('../../services/adminUserMutationService'); + const resume = deferred(); let late; + await expect(withAdminUserMutation(String(alice._id), async () => { + late = resume.promise.then(() => User.updateOne({ _id: bob._id }, { $set: { role: 'user' } })); + throw new Error('unknown write outcome'); + })).rejects.toThrow('unknown write outcome'); + let second; + try { + second = await request(app).patch(`/api/admin/users/${alice._id}/role`) + .set('Authorization', `Bearer ${issueSessionToken(bob)}`).send({ role: 'user' }); + } finally { resume.resolve(); await late; } + expect({ secondStatus: second.status, remainingAdmins: await User.countDocuments({ role: 'admin' }) }) + .toEqual({ secondStatus: 503, remainingAdmins: 1 }); + }); + + it('cannot release a replacement owner’s lock', async () => { + const alice = await makeAdmin('owner-alice'); + const Lock = require('../../models/AdminUserMutationLock'); + const { withAdminUserMutation } = require('../../services/adminUserMutationService'); + await expect(withAdminUserMutation(String(alice._id), async () => { + await Lock.updateOne({ _id: 'global-admin-user-mutations' }, { $set: { owner: 'replacement' } }); + })).rejects.toMatchObject({ code: 'ADMIN_MUTATION_LOCK_LOST' }); + expect((await Lock.findById('global-admin-user-mutations')).owner).toBe('replacement'); + }); + + it('rejects a malformed target before any database write without leaving a lock', async () => { + const alice = await makeAdmin('invalid-target-alice'); + const Lock = require('../../models/AdminUserMutationLock'); + const res = await request(app).patch('/api/admin/users/not-an-id/role') + .set('Authorization', `Bearer ${issueSessionToken(alice)}`).send({ role: 'user' }); + expect(res.status).toBe(400); expect(await Lock.countDocuments()).toBe(0); + }); + +}); diff --git a/backend/__tests__/unit/routes/admin.users.test.js b/backend/__tests__/unit/routes/admin.users.test.js index 4f57aa38a4..ba82d1a6c6 100644 --- a/backend/__tests__/unit/routes/admin.users.test.js +++ b/backend/__tests__/unit/routes/admin.users.test.js @@ -1,3 +1,10 @@ +// This fixture stubs User and auth; real lock/authorization behavior is tested +// across Mongo in service/adminRoleConcurrency.test.js. +jest.mock('../../../services/adminUserMutationService', () => ({ + withAdminUserMutation: (_actor, operation) => operation(), + AdminUserMutationError: class extends Error {}, +})); + jest.mock('../../../middleware/auth', () => (req, _res, next) => { req.user = req.user || { id: 'admin-1' }; req.userId = req.user.id; @@ -105,12 +112,12 @@ describe('admin users routes', () => { it('deletes a non-bot user', async () => { const handler = getRouteHandler('/:userId', 'delete'); const req = { - params: { userId: 'u2' }, + params: { userId: '000000000000000000000002' }, user: { id: 'admin-1' }, }; const res = createRes(); const target = { - _id: 'u2', + _id: '000000000000000000000002', role: 'user', isBot: false, deleteOne: jest.fn().mockResolvedValue(true), @@ -126,13 +133,13 @@ describe('admin users routes', () => { it('updates a user role', async () => { const handler = getRouteHandler('/:userId/role', 'patch'); const req = { - params: { userId: 'u2' }, + params: { userId: '000000000000000000000002' }, body: { role: 'admin' }, user: { id: 'admin-1' }, }; const res = createRes(); const target = { - _id: 'u2', + _id: '000000000000000000000002', username: 'bob', email: 'b@example.com', role: 'user', @@ -148,7 +155,7 @@ describe('admin users routes', () => { expect(res.json).toHaveBeenCalledWith( expect.objectContaining({ message: 'User role updated successfully', - user: expect.objectContaining({ id: 'u2', role: 'admin' }), + user: expect.objectContaining({ id: '000000000000000000000002', role: 'admin' }), }), ); }); @@ -354,9 +361,9 @@ describe('admin users routes', () => { it('bans a non-admin user with a reason', async () => { const handler = getRouteHandler('/:userId/ban', 'patch'); const save = jest.fn().mockResolvedValue(undefined); - const target = { _id: 'u2', username: 'mallory', role: 'user', isBot: false, save }; + const target = { _id: '000000000000000000000002', username: 'mallory', role: 'user', isBot: false, save }; User.findById.mockResolvedValue(target); - const req = { params: { userId: 'u2' }, body: { banned: true, reason: 'spam' }, user: { id: 'admin1' } }; + const req = { params: { userId: '000000000000000000000002' }, body: { banned: true, reason: 'spam' }, user: { id: 'admin1' } }; const res = createRes(); await handler(req, res); @@ -373,9 +380,9 @@ describe('admin users routes', () => { it('unbans and clears the reason', async () => { const handler = getRouteHandler('/:userId/ban', 'patch'); const save = jest.fn().mockResolvedValue(undefined); - const target = { _id: 'u2', username: 'mallory', role: 'user', isBot: false, banned: true, banReason: 'spam', save }; + const target = { _id: '000000000000000000000002', username: 'mallory', role: 'user', isBot: false, banned: true, banReason: 'spam', save }; User.findById.mockResolvedValue(target); - const req = { params: { userId: 'u2' }, body: { banned: false }, user: { id: 'admin1' } }; + const req = { params: { userId: '000000000000000000000002' }, body: { banned: false }, user: { id: 'admin1' } }; const res = createRes(); await handler(req, res); @@ -387,8 +394,8 @@ describe('admin users routes', () => { it('refuses to ban an admin account', async () => { const handler = getRouteHandler('/:userId/ban', 'patch'); - User.findById.mockResolvedValue({ _id: 'a2', role: 'admin', isBot: false, save: jest.fn() }); - const req = { params: { userId: 'a2' }, body: { banned: true }, user: { id: 'admin1' } }; + User.findById.mockResolvedValue({ _id: '0000000000000000000000a2', role: 'admin', isBot: false, save: jest.fn() }); + const req = { params: { userId: '0000000000000000000000a2' }, body: { banned: true }, user: { id: 'admin1' } }; const res = createRes(); await handler(req, res); diff --git a/backend/__tests__/unit/routes/routeRateLimitGuard.baseline.json b/backend/__tests__/unit/routes/routeRateLimitGuard.baseline.json index ef7c9c4886..e137c4cfe4 100644 --- a/backend/__tests__/unit/routes/routeRateLimitGuard.baseline.json +++ b/backend/__tests__/unit/routes/routeRateLimitGuard.baseline.json @@ -10,11 +10,9 @@ "routes/admin/globalIntegrations.ts POST /sync [unlimited]", "routes/admin/globalIntegrations.ts POST /x/oauth/start [unlimited]", "routes/admin/globalIntegrations.ts POST /x/test [unlimited]", - "routes/admin/users.ts DELETE /:userId [unlimited]", "routes/admin/users.ts GET / [unlimited]", "routes/admin/users.ts GET /invitations [unlimited]", "routes/admin/users.ts GET /waitlist [unlimited]", - "routes/admin/users.ts PATCH /:userId/role [unlimited]", "routes/admin/users.ts PATCH /waitlist/:requestId [unlimited]", "routes/admin/users.ts POST /invitations [unlimited]", "routes/admin/users.ts POST /invitations/:invitationId/revoke [unlimited]", diff --git a/backend/__tests__/unit/services/syntheticAccountPurgeService.test.js b/backend/__tests__/unit/services/syntheticAccountPurgeService.test.js index 5099cb7a18..10ef49063c 100644 --- a/backend/__tests__/unit/services/syntheticAccountPurgeService.test.js +++ b/backend/__tests__/unit/services/syntheticAccountPurgeService.test.js @@ -75,6 +75,28 @@ describe('syntheticAccountPurgeService', () => { expect(isSyntheticAccount({ username: 'eng-smoke-journey-1015124709', email: 'a@ops.commonly.invalid', isBot: true })).toBe(false); }); + it('preserves a synthetic account promoted to administrator, including its data', async () => { + const { human, pod, scout } = await syntheticAccount(); + await User.updateOne({ _id: human._id }, { $set: { role: 'admin' } }); + const pool = fakePool(); + const report = await purgeSyntheticAccounts({ apply: true, now: NOW, pool }); + expect(report.accounts).toHaveLength(0); + expect(await User.findById(human._id)).not.toBeNull(); + expect(await User.findById(scout._id)).not.toBeNull(); + expect(await Pod.findById(pod._id)).not.toBeNull(); + expect(pool.statements).toHaveLength(0); + }); + + it('uses the administrator mutation lock before an applying purge', async () => { + const { human } = await syntheticAccount(); + const { withAdminUserMutationLock } = require('../../../services/adminUserMutationService'); + await withAdminUserMutationLock(async () => { + await expect(purgeSyntheticAccounts({ apply: true, now: NOW, pool: fakePool() })) + .rejects.toMatchObject({ code: 'ADMIN_MUTATION_BUSY' }); + }); + expect(await User.findById(human._id)).not.toBeNull(); + }); + it('a dry run counts the footprint and deletes nothing', async () => { const { human, pod } = await syntheticAccount(); const report = await purgeSyntheticAccounts({ now: NOW, pool: fakePool() }); diff --git a/backend/models/AdminUserMutationLock.ts b/backend/models/AdminUserMutationLock.ts new file mode 100644 index 0000000000..399c797772 --- /dev/null +++ b/backend/models/AdminUserMutationLock.ts @@ -0,0 +1,14 @@ +import mongoose, { Schema } from 'mongoose'; + +// Intentionally no TTL or takeover deadline: a stopped process can resume and +// still write User rows. Expiring its lock would admit a concurrent writer. +const schema = new Schema({ + _id: { type: String, required: true }, + owner: { type: String, required: true }, + acquiredAt: { type: Date, required: true }, +}, { collection: 'admin_user_mutation_locks' }); + +export default mongoose.model('AdminUserMutationLock', schema); +// CJS compat: let require() return the default export directly. +// eslint-disable-next-line @typescript-eslint/no-require-imports +module.exports = exports["default"]; Object.assign(module.exports, exports); diff --git a/backend/routes/admin/users.ts b/backend/routes/admin/users.ts index 7dfea60f20..91032e04ac 100644 --- a/backend/routes/admin/users.ts +++ b/backend/routes/admin/users.ts @@ -1,3 +1,5 @@ +import { isValidObjectId } from 'mongoose'; +import { AdminUserMutationError, withAdminUserMutation } from '../../services/adminUserMutationService'; export {}; const express = require('express'); const crypto = require('crypto'); @@ -135,39 +137,48 @@ router.get('/', auth, adminAuth, async (req: any, res: any) => { } }); +const mutationResponse = (status: number, body: Record) => ({ status, body }); + // PATCH /api/admin/users/:userId/role -router.patch('/:userId/role', auth, adminAuth, async (req: any, res: any) => { +router.patch('/:userId/role', adminWriteLimiter, auth, adminAuth, async (req: any, res: any) => { try { - const { userId } = req.params; - const { role } = req.body || {}; - const nextRole = String(role || '').trim().toLowerCase(); - if (!['admin', 'user'].includes(nextRole)) { - return res.status(400).json({ error: 'Role must be either "admin" or "user"' }); - } - if (req.user?.id && String(req.user.id) === String(userId)) { - return res.status(400).json({ error: 'You cannot change your own admin role' }); - } + const result = await withAdminUserMutation(String(req.user?.id || ''), async () => { + const { userId } = req.params; + const { role } = req.body || {}; + const nextRole = String(role || '').trim().toLowerCase(); + if (!['admin', 'user'].includes(nextRole)) { + return mutationResponse(400, { error: 'Role must be either "admin" or "user"' }); + } + if (req.user?.id && String(req.user.id) === String(userId)) { + return mutationResponse(400, { error: 'You cannot change your own admin role' }); + } - const target = await User.findById(userId); - if (!target) { - return res.status(404).json({ error: 'User not found' }); - } + if (!isValidObjectId(userId)) return mutationResponse(400, { error: 'Invalid user ID' }); + const target = await User.findById(userId); + if (!target) { + return mutationResponse(404, { error: 'User not found' }); + } - if (target.role === 'admin' && nextRole === 'user') { - const adminCount = await User.countDocuments({ role: 'admin' }); - if (adminCount <= 1) { - return res.status(400).json({ error: 'Cannot remove the last global admin' }); + if (target.role === 'admin' && nextRole === 'user') { + const adminCount = await User.countDocuments({ role: 'admin' }); + if (adminCount <= 1) { + return mutationResponse(400, { error: 'Cannot remove the last global admin' }); + } } - } - target.role = nextRole; - await target.save(); + target.role = nextRole; + await target.save(); - return res.json({ - message: 'User role updated successfully', - user: sanitizeUser(target), + return mutationResponse(200, { + message: 'User role updated successfully', + user: sanitizeUser(target), + }); }); + return res.status(result.status).json(result.body); } catch (error) { + if (error instanceof AdminUserMutationError) { + return res.status(error.status).json({ error: error.message, code: error.code }); + } console.error('Failed to update user role:', error); return res.status(500).json({ error: 'Failed to update user role' }); } @@ -234,33 +245,40 @@ router.patch('/:userId/entitlements', adminWriteLimiter, auth, adminAuth, async // first — prevents an admin-vs-admin lockout war). router.patch('/:userId/ban', adminWriteLimiter, auth, adminAuth, async (req: any, res: any) => { try { - const { userId } = req.params; - const { banned, reason } = req.body || {}; - if (typeof banned !== 'boolean') { - return res.status(400).json({ error: 'banned (boolean) is required' }); - } - if (reason !== undefined && typeof reason !== 'string') { - return res.status(400).json({ error: 'reason must be a string' }); - } - if (req.user?.id && String(req.user.id) === String(userId)) { - return res.status(400).json({ error: 'You cannot ban your own account' }); - } - const target = await User.findById(userId); - if (!target) return res.status(404).json({ error: 'User not found' }); - if (target.isBot) { - return res.status(400).json({ error: 'Bot accounts cannot be banned from this endpoint' }); - } - if (target.role === 'admin' && banned) { - return res.status(400).json({ error: 'Demote the admin role before banning this account' }); - } + const result = await withAdminUserMutation(String(req.user?.id || ''), async () => { + const { userId } = req.params; + const { banned, reason } = req.body || {}; + if (typeof banned !== 'boolean') { + return mutationResponse(400, { error: 'banned (boolean) is required' }); + } + if (reason !== undefined && typeof reason !== 'string') { + return mutationResponse(400, { error: 'reason must be a string' }); + } + if (req.user?.id && String(req.user.id) === String(userId)) { + return mutationResponse(400, { error: 'You cannot ban your own account' }); + } + if (!isValidObjectId(userId)) return mutationResponse(400, { error: 'Invalid user ID' }); + const target = await User.findById(userId); + if (!target) return mutationResponse(404, { error: 'User not found' }); + if (target.isBot) { + return mutationResponse(400, { error: 'Bot accounts cannot be banned from this endpoint' }); + } + if (target.role === 'admin' && banned) { + return mutationResponse(400, { error: 'Demote the admin role before banning this account' }); + } - target.banned = banned; - target.bannedAt = banned ? new Date() : undefined; - target.banReason = banned ? (reason || '').slice(0, 500) : undefined; - await target.save(); + target.banned = banned; + target.bannedAt = banned ? new Date() : undefined; + target.banReason = banned ? (reason || '').slice(0, 500) : undefined; + await target.save(); - return res.json({ ok: true, user: sanitizeUser(target) }); + return mutationResponse(200, { ok: true, user: sanitizeUser(target) }); + }); + return res.status(result.status).json(result.body); } catch (error) { + if (error instanceof AdminUserMutationError) { + return res.status(error.status).json({ error: error.message, code: error.code }); + } console.error('Failed to update ban state:', error); return res.status(500).json({ error: 'Failed to update ban state' }); } @@ -283,83 +301,90 @@ router.post('/:userId/sessions/revoke', adminWriteLimiter, auth, adminAuth, asyn } }); -router.delete('/:userId', auth, adminAuth, async (req: any, res: any) => { +router.delete('/:userId', adminWriteLimiter, auth, adminAuth, async (req: any, res: any) => { try { - const { userId } = req.params; - if (req.user?.id && String(req.user.id) === String(userId)) { - return res.status(400).json({ error: 'You cannot delete your own account from admin tools' }); - } + const result = await withAdminUserMutation(String(req.user?.id || ''), async () => { + const { userId } = req.params; + if (req.user?.id && String(req.user.id) === String(userId)) { + return mutationResponse(400, { error: 'You cannot delete your own account from admin tools' }); + } - const target = await User.findById(userId); - if (!target) { - return res.status(404).json({ error: 'User not found' }); - } - if (target.isBot) { - return res.status(400).json({ error: 'Bot accounts cannot be deleted from this endpoint' }); - } + if (!isValidObjectId(userId)) return mutationResponse(400, { error: 'Invalid user ID' }); + const target = await User.findById(userId); + if (!target) { + return mutationResponse(404, { error: 'User not found' }); + } + if (target.isBot) { + return mutationResponse(400, { error: 'Bot accounts cannot be deleted from this endpoint' }); + } - if (target.role === 'admin') { - const adminCount = await User.countDocuments({ role: 'admin' }); - if (adminCount <= 1) { - return res.status(400).json({ error: 'Cannot delete the last global admin' }); + if (target.role === 'admin') { + const adminCount = await User.countDocuments({ role: 'admin' }); + if (adminCount <= 1) { + return mutationResponse(400, { error: 'Cannot delete the last global admin' }); + } } - } - // Deleting the person leaves their hosted connections behind for whoever - // happens to look. An admin CAN finish the removal — `canDeleteIntegration` - // answers true on `role === 'admin'` before it reads `createdBy` - // (`routes/integrations.ts:173`), and `GET /api/integrations/admin/all` - // still lists the row (it filters on `isActive`, so the orphan comes back - // with `createdBy: null`) — but nothing prompts the look, so until someone - // takes it the grants and the material outlive the person they act for. - // Refusing here does that cleanup at the one moment an admin is already - // acting on this user, and hands over the ids and entries to do it with. - // - // The predicate is the EXISTENCE of the row — not `status`, not the presence - // of material (wren 75857, reverting my material-keyed version). Both - // narrower keys let a live connect through. `status` waves past the row a - // failed provider revoke left `disconnected` with its material still on it - // (`markDisconnected` writes that at step 2, the material does not go until - // step 4). Material waves past the row that is mid-exchange: the callback - // unsets `config.pendingAuth` at `:341`, then makes its upstream calls, and - // `config.credentialRef` is not written until `:427` — and each failure - // branch in between (`issuer_unreachable`, `exchange_refused`, - // `exchange_unreachable`, `exchange_incomplete`) returns a redirect with the - // row left exactly so. A row holding no ref is therefore not a dead stub: - // it is a connect that lands material seconds later, or one that already - // failed while owing a cleanup. - // - // The cost of the coarser predicate is bounded and falls on a caller who can - // pay it: a genuinely dead attempt costs one DELETE from the ids this - // response lists. `createdBy` is the right owner term for the same reason it - // is exact — `routes/grants.ts:266` resolves an owner as `createdBy || - // config.linkedUserId`, and a hosted row never gets the second. - // - // The remedy is the ordinary removal, which admits admins and runs the whole - // sequence (grants first); this endpoint never calls a vendor, the same way - // pod delete does not. - const ownedConnections = await Integration.find({ - type: HOSTED_MCP_TYPE, - createdBy: target._id, - }).select('_id config.entryId').lean() as Array<{ _id: unknown; config?: { entryId?: string } }>; - if (ownedConnections.length > 0) { - return res.status(409).json({ - error: 'This user still owns hosted connections. Remove them first.', - code: 'hosted_mcp_connection_owned', - // The id AND the entry, per the plan's §7 body (plan doc :192): the pair - // is exactly what `DELETE /api/integrations/:id` takes, so an admin - // reading this knows which row to delete for which app without a second - // lookup. A parallel array of entries would couple by index and drift. - connections: ownedConnections.map((connection) => ({ - id: String(connection._id), - entryId: connection.config?.entryId || null, - })), - }); - } + // Deleting the person leaves their hosted connections behind for whoever + // happens to look. An admin CAN finish the removal — `canDeleteIntegration` + // answers true on `role === 'admin'` before it reads `createdBy` + // (`routes/integrations.ts:173`), and `GET /api/integrations/admin/all` + // still lists the row (it filters on `isActive`, so the orphan comes back + // with `createdBy: null`) — but nothing prompts the look, so until someone + // takes it the grants and the material outlive the person they act for. + // Refusing here does that cleanup at the one moment an admin is already + // acting on this user, and hands over the ids and entries to do it with. + // + // The predicate is the EXISTENCE of the row — not `status`, not the presence + // of material (wren 75857, reverting my material-keyed version). Both + // narrower keys let a live connect through. `status` waves past the row a + // failed provider revoke left `disconnected` with its material still on it + // (`markDisconnected` writes that at step 2, the material does not go until + // step 4). Material waves past the row that is mid-exchange: the callback + // unsets `config.pendingAuth` at `:341`, then makes its upstream calls, and + // `config.credentialRef` is not written until `:427` — and each failure + // branch in between (`issuer_unreachable`, `exchange_refused`, + // `exchange_unreachable`, `exchange_incomplete`) returns a redirect with the + // row left exactly so. A row holding no ref is therefore not a dead stub: + // it is a connect that lands material seconds later, or one that already + // failed while owing a cleanup. + // + // The cost of the coarser predicate is bounded and falls on a caller who can + // pay it: a genuinely dead attempt costs one DELETE from the ids this + // response lists. `createdBy` is the right owner term for the same reason it + // is exact — `routes/grants.ts:266` resolves an owner as `createdBy || + // config.linkedUserId`, and a hosted row never gets the second. + // + // The remedy is the ordinary removal, which admits admins and runs the whole + // sequence (grants first); this endpoint never calls a vendor, the same way + // pod delete does not. + const ownedConnections = await Integration.find({ + type: HOSTED_MCP_TYPE, + createdBy: target._id, + }).select('_id config.entryId').lean() as Array<{ _id: unknown; config?: { entryId?: string } }>; + if (ownedConnections.length > 0) { + return mutationResponse(409, { + error: 'This user still owns hosted connections. Remove them first.', + code: 'hosted_mcp_connection_owned', + // The id AND the entry, per the plan's §7 body (plan doc :192): the pair + // is exactly what `DELETE /api/integrations/:id` takes, so an admin + // reading this knows which row to delete for which app without a second + // lookup. A parallel array of entries would couple by index and drift. + connections: ownedConnections.map((connection) => ({ + id: String(connection._id), + entryId: connection.config?.entryId || null, + })), + }); + } - await target.deleteOne(); - return res.json({ message: 'User deleted successfully' }); + await target.deleteOne(); + return mutationResponse(200, { message: 'User deleted successfully' }); + }); + return res.status(result.status).json(result.body); } catch (error) { + if (error instanceof AdminUserMutationError) { + return res.status(error.status).json({ error: error.message, code: error.code }); + } console.error('Failed to delete user:', error); return res.status(500).json({ error: 'Failed to delete user' }); } diff --git a/backend/services/adminUserMutationService.ts b/backend/services/adminUserMutationService.ts new file mode 100644 index 0000000000..86a81f2c3b --- /dev/null +++ b/backend/services/adminUserMutationService.ts @@ -0,0 +1,55 @@ +import { randomUUID } from 'crypto'; +import AdminUserMutationLock from '../models/AdminUserMutationLock'; +import User from '../models/User'; + +export class AdminUserMutationError extends Error { + constructor(public status: number, public code: string, message: string) { + super(message); + } +} + +const LOCK_ID = 'global-admin-user-mutations'; +const writeConcern = { w: 'majority' as const, wtimeout: 5000 }; + +// Standalone Mongo is supported. A primary-key insert serializes online role, +// ban and deletion handlers across processes without requiring transactions. +// A crash fails closed until operator recovery; see docs/runbooks/admin-user-mutation-lock.md. +export const withAdminUserMutationLock = async (operation: () => Promise): Promise => { + const owner = randomUUID(); + try { + await AdminUserMutationLock.create([{ _id: LOCK_ID, owner, acquiredAt: new Date() }], { writeConcern }); + } catch (error) { + if ((error as { code?: number }).code === 11000) { + throw new AdminUserMutationError(503, 'ADMIN_MUTATION_BUSY', + 'Another administrator change is in progress. Retry shortly; persistent refusal needs operator recovery.'); + } + throw error; + } + // A rejected write can still have an unknown outcome. Do not release on + // callback failure: the original database write might complete later. + const value = await operation(); + const released = await AdminUserMutationLock.deleteOne({ _id: LOCK_ID, owner }, { writeConcern }); + if (released.deletedCount !== 1) { + throw new AdminUserMutationError(503, 'ADMIN_MUTATION_LOCK_LOST', + 'Administrator mutation lock changed unexpectedly.'); + } + return value; +}; + +// The scheduler's applying purge uses the same lock without a human actor. +// HTTP callers must use this authorization-checking entry point. +export const withAdminUserMutation = async (actorId: string, operation: () => Promise): Promise => { + const outcome = await withAdminUserMutationLock(async () => { + // The request may have passed adminAuth before a competing demotion. Read + // authorization again only after acquiring the shared mutation lock. + const actor = await User.findById(actorId); + if (!actor || actor.role !== 'admin' || actor.banned || actor.isBot) { + // A known refusal has not attempted a write, so normal unlock is safe. + return { ok: false as const, error: new AdminUserMutationError(403, 'ADMIN_ACCESS_CHANGED', + 'Administrator access changed. Sign in again.') }; + } + return { ok: true as const, value: await operation() }; + }); + if (!outcome.ok) throw outcome.error; + return outcome.value; +}; diff --git a/backend/services/syntheticAccountPurgeService.ts b/backend/services/syntheticAccountPurgeService.ts index 69be60f78b..cc6b9abf6a 100644 --- a/backend/services/syntheticAccountPurgeService.ts +++ b/backend/services/syntheticAccountPurgeService.ts @@ -1,3 +1,4 @@ +import { withAdminUserMutationLock } from './adminUserMutationService'; /* * Purge the throwaway accounts the stranger journey signs up (TASK-260). * @@ -94,7 +95,7 @@ const loadPool = (): PgPool | null => { return require('../config/db-pg').pool as PgPool; }; -export const purgeSyntheticAccounts = async ( +const purgeSyntheticAccountsUnlocked = async ( { apply = false, minAgeMs = DEFAULT_MIN_AGE_MS, now = new Date(), pool = loadPool() }: { apply?: boolean; minAgeMs?: number; now?: Date; pool?: PgPool | null } = {}, ): Promise => { @@ -123,6 +124,7 @@ export const purgeSyntheticAccounts = async ( username: SYNTHETIC_USERNAME, email: SYNTHETIC_EMAIL, isBot: { $ne: true }, + role: { $ne: 'admin' }, createdAt: { $lt: new Date(now.getTime() - minAgeMs) }, }).select('_id username email isBot createdAt').lean(); report.examined = candidates.length; @@ -131,7 +133,8 @@ export const purgeSyntheticAccounts = async ( if (!isSyntheticAccount(human)) continue; // the query and the predicate must both agree const humanId = String(human._id); const scoutUsername = `scout-${perUserInstanceId(humanId)}`; - const scoutBot = await User.findOne({ username: scoutUsername, isBot: true }).select('_id').lean(); + const scoutBot = await User.findOne({ username: scoutUsername, isBot: true, role: { $ne: 'admin' } }) + .select('_id').lean(); const seatRows = await AgentRegistry.find({ 'publisher.userId': human._id, ephemeral: true }) .select('agentName').lean(); const skippedSeats: { name: string; reason: string }[] = []; @@ -147,7 +150,11 @@ export const purgeSyntheticAccounts = async ( continue; } const bot = await User.findOne({ username: agentName, isBot: true, email: `${agentName}@agents.commonly.local` }) - .select('_id username createdAt').lean(); + .select('_id username createdAt role').lean(); + if (bot?.role === 'admin') { + skippedSeats.push({ name: agentName, reason: 'its bot user is an administrator' }); + continue; + } if (bot && (!bot.createdAt || !human.createdAt || new Date(bot.createdAt) < new Date(human.createdAt))) { skippedSeats.push({ name: agentName, reason: 'its bot user predates the account' }); continue; @@ -227,6 +234,15 @@ export const purgeSyntheticAccounts = async ( return report; }; +// Applying the purge can remove User rows. Keep selection and deletion in the +// same critical section as online role writers so a candidate cannot become an +// administrator between those steps. Dry runs perform no destructive writes. +export const purgeSyntheticAccounts = async ( + options: Parameters[0] = {}, +): Promise => (options.apply + ? withAdminUserMutationLock(() => purgeSyntheticAccountsUnlocked(options)) + : purgeSyntheticAccountsUnlocked(options)); + // CJS compat, same shape as sibling services. module.exports = { purgeSyntheticAccounts, isSyntheticAccount, SYNTHETIC_USERNAME, SYNTHETIC_EMAIL_DOMAIN, diff --git a/docs/runbooks/admin-user-mutation-lock.md b/docs/runbooks/admin-user-mutation-lock.md new file mode 100644 index 0000000000..81a0967d94 --- /dev/null +++ b/docs/runbooks/admin-user-mutation-lock.md @@ -0,0 +1,66 @@ +# Administrator user mutation lock + +Online administrator role changes, account bans and administrative user deletion +and the applying synthetic-account purge share one Mongo lock. The handler +acquires it before re-reading the actor, +reading the target, counting administrators and writing the change. A competing +request receives HTTP 503 `ADMIN_MUTATION_BUSY` and can retry. Role/deletion +writers still enforce their last-administrator and self-change rules. An actor +who passed authentication before losing their role is refused inside the lock. + +The lock uses Mongo's unique `_id` (`global-admin-user-mutations`) in +`admin_user_mutation_locks`, with a random owner and acquisition timestamp. +Acquisition and release require majority write acknowledgement. This works on +standalone Mongo as well as replica sets; it requires no multi-document +transaction. It serializes supported online handlers, not direct database edits +or offline maintenance scripts. Run those only with online writers stopped. + +There is deliberately no TTL, expiry or automatic takeover. A paused worker can +resume and write its User row after an expiring lease has been stolen. A +separate lock document cannot fence that write on standalone Mongo. Preserving +an administrator takes precedence over automatically recovering mutation +availability. A worker crash, a callback failure or an uncertain database acknowledgement may leave +this small lock row behind. Callback failures retain it because a rejected +write may still complete later; known pre-write authorization refusals release +it safely. Reads and unrelated application work continue; +administrator role, ban and deletion writes fail closed until recovery. + +## Deploying the change + +Older binaries do not honor this lock. Pause administrator user writes and the +synthetic-account purge during the rollout, drain old writers and their pending +database operations, and enable them only after every API and purge worker uses +the guarded version. A mixed-version deployment does not provide serialization. +Direct database role edits and offline maintenance also require quiescence. + +## Recover a persistent busy response + +1. Confirm the lock row and record its exact `owner` value and `acquiredAt`. + A recent row can simply be a request still in flight. Age alone never proves + that a writer is dead. +2. Stop **all API processes, scheduler/purge workers and maintenance commands that can perform these user mutations** on + every instance using this database. Confirm that the original holder cannot + resume, and drain or cancel its server-side database operations. Verify no + writes from those connections remain in flight. Stopping a process alone + does not prove that an already submitted database write has finished. An HTTP timeout, an expired pod lease or a disconnected client is + insufficient. If quiescence cannot be established, leave the lock in place. +3. Inspect the current User rows and administrator count. A failed response can + have committed its User change before release failed; do not blindly repeat + the old mutation. If no usable administrator exists because of a historical + defect or out-of-band edit, restore one while writers remain stopped. +4. Delete the lock using both its fixed `_id` and the recorded `owner` as the + selector. Do not bulk-delete this collection. Confirm exactly one row was + removed, or investigate a changed owner before proceeding. +5. Restart the writers and verify a single authorized mutation and the remaining + administrator count. Resume ordinary traffic only after this succeeds. + +Any future online writer that can remove an administrator, ban an account or +promote a deletion target must use the same critical section. A self-serve +account-deletion path may instead refuse administrators and use atomic target +predicates to exclude a concurrent promotion. Tests must exercise that boundary +when integrating the two changes. + +The scheduled synthetic-account purge shares the lock for its applying pass, +excludes administrator candidates and preserves administrator bot rows. A dry +run does not acquire the lock. A long purge can temporarily refuse administrator +mutations; a failed purge uses the same crash-recovery procedure. From 234b9766c6505294aed999faf287a9d969aaa096 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 12:57:04 -0700 Subject: [PATCH 2/2] Release administrator mutation lock on pre-write read failures Signal before submitting the first handler or purge cleanup write. A read-only failure safely releases the owner-matched lock; a rejected submitted write retains it because its outcome can still arrive late. Add actor/count recovery, handler write rejection and purge boundary regressions. Written by Sprint Impl, a Commonly agent https://commonly.me/v2/pods/6addd4d47f8fc798a6f66bbb#message-85722 Co-authored-by: sprint-impl Agent-Model: codex/gpt-6.1-sol/medium --- .../service/adminPauseLeavesHostedRow.test.js | 2 +- .../service/adminRoleConcurrency.test.js | 41 ++++++++++++++++++- .../__tests__/unit/routes/admin.users.test.js | 2 +- .../syntheticAccountPurgeService.test.js | 27 ++++++++++++ backend/routes/admin/users.ts | 9 ++-- backend/services/adminUserMutationService.ts | 35 +++++++++++----- .../services/syntheticAccountPurgeService.ts | 11 ++++- docs/runbooks/admin-user-mutation-lock.md | 7 ++-- 8 files changed, 111 insertions(+), 23 deletions(-) diff --git a/backend/__tests__/service/adminPauseLeavesHostedRow.test.js b/backend/__tests__/service/adminPauseLeavesHostedRow.test.js index 6c9e4e3a3e..8c37b195bd 100644 --- a/backend/__tests__/service/adminPauseLeavesHostedRow.test.js +++ b/backend/__tests__/service/adminPauseLeavesHostedRow.test.js @@ -1,7 +1,7 @@ // This fixture stubs User and auth; real lock/authorization behavior is tested // across Mongo in service/adminRoleConcurrency.test.js. jest.mock('../../services/adminUserMutationService', () => ({ - withAdminUserMutation: (_actor, operation) => operation(), + withAdminUserMutation: (_actor, operation) => operation(() => {}), AdminUserMutationError: class extends Error {}, })); diff --git a/backend/__tests__/service/adminRoleConcurrency.test.js b/backend/__tests__/service/adminRoleConcurrency.test.js index ff2c74b046..f8e1c1115d 100644 --- a/backend/__tests__/service/adminRoleConcurrency.test.js +++ b/backend/__tests__/service/adminRoleConcurrency.test.js @@ -69,13 +69,49 @@ describe('Administrator role concurrency', () => { expect(operation).not.toHaveBeenCalled(); expect(await Lock.countDocuments()).toBe(0); await User.updateOne({ _id: alice._id }, { $set: { role: 'admin' } }); - await expect(withAdminUserMutation(String(alice._id), async () => { throw new Error('write failure'); })) + await expect(withAdminUserMutation(String(alice._id), async (beginWrite) => { beginWrite(); throw new Error('write failure'); })) .rejects.toThrow('write failure'); expect(await Lock.countDocuments()).toBe(1); await expect(withAdminUserMutation(String(alice._id), async () => 'next writer')) .rejects.toMatchObject({ code: 'ADMIN_MUTATION_BUSY' }); }); + it.each(['actor', 'count'])('releases the lock after a pre-write %s read fails', async (stage) => { + const [alice, bob] = await Promise.all([makeAdmin(`read-${stage}-alice`), makeAdmin(`read-${stage}-bob`)]); + const { withAdminUserMutation } = require('../../services/adminUserMutationService'); + const Lock = require('../../models/AdminUserMutationLock'); + if (stage === 'actor') { + jest.spyOn(User, 'findById').mockRejectedValueOnce(new Error('actor read unavailable')); + await expect(withAdminUserMutation(String(alice._id), jest.fn())).rejects.toThrow('actor read unavailable'); + } else { + jest.spyOn(User, 'countDocuments').mockRejectedValueOnce(new Error('count unavailable')); + const failed = await request(app).patch(`/api/admin/users/${bob._id}/role`) + .set('Authorization', `Bearer ${issueSessionToken(alice)}`).send({ role: 'user' }); + expect(failed.status).toBe(500); + } + expect(await Lock.countDocuments()).toBe(0); + const retry = await request(app).patch(`/api/admin/users/${bob._id}/role`) + .set('Authorization', `Bearer ${issueSessionToken(alice)}`).send({ role: 'user' }); + expect(retry.status).toBe(200); + expect(await User.countDocuments({ role: 'admin' })).toBe(1); + }); + + it.each(['role', 'ban', 'delete'])('retains the lock if the %s handler write rejects', async (kind) => { + const [alice, bob] = await Promise.all([makeAdmin(`write-${kind}-alice`), makeAdmin(`write-${kind}-bob`)]); + const Lock = require('../../models/AdminUserMutationLock'); + if (kind === 'ban') await User.updateOne({ _id: bob._id }, { $set: { role: 'user' } }); + const write = jest.spyOn(User.prototype, kind === 'delete' ? 'deleteOne' : 'save') + .mockRejectedValueOnce(new Error('write outcome unknown')); + try { + const pending = kind === 'delete' ? request(app).delete(`/api/admin/users/${bob._id}`) + : request(app).patch(`/api/admin/users/${bob._id}/${kind}`) + .send(kind === 'role' ? { role: 'user' } : { banned: true }); + const failed = await pending.set('Authorization', `Bearer ${issueSessionToken(alice)}`); + expect(failed.status).toBe(500); + } finally { write.mockRestore(); } + expect(await Lock.countDocuments()).toBe(1); + }); + it('does not send success before releasing the lock, and fails closed if release fails', async () => { const [alice, bob] = await Promise.all([makeAdmin('release-alice'), makeAdmin('release-bob')]); const Lock = require('../../models/AdminUserMutationLock'); @@ -119,7 +155,8 @@ describe('Administrator role concurrency', () => { const [alice, bob] = await Promise.all([makeAdmin('uncertain-alice'), makeAdmin('uncertain-bob')]); const { withAdminUserMutation } = require('../../services/adminUserMutationService'); const resume = deferred(); let late; - await expect(withAdminUserMutation(String(alice._id), async () => { + await expect(withAdminUserMutation(String(alice._id), async (beginWrite) => { + beginWrite(); late = resume.promise.then(() => User.updateOne({ _id: bob._id }, { $set: { role: 'user' } })); throw new Error('unknown write outcome'); })).rejects.toThrow('unknown write outcome'); diff --git a/backend/__tests__/unit/routes/admin.users.test.js b/backend/__tests__/unit/routes/admin.users.test.js index ba82d1a6c6..32fd411c64 100644 --- a/backend/__tests__/unit/routes/admin.users.test.js +++ b/backend/__tests__/unit/routes/admin.users.test.js @@ -1,7 +1,7 @@ // This fixture stubs User and auth; real lock/authorization behavior is tested // across Mongo in service/adminRoleConcurrency.test.js. jest.mock('../../../services/adminUserMutationService', () => ({ - withAdminUserMutation: (_actor, operation) => operation(), + withAdminUserMutation: (_actor, operation) => operation(() => {}), AdminUserMutationError: class extends Error {}, })); diff --git a/backend/__tests__/unit/services/syntheticAccountPurgeService.test.js b/backend/__tests__/unit/services/syntheticAccountPurgeService.test.js index 10ef49063c..713efe13c1 100644 --- a/backend/__tests__/unit/services/syntheticAccountPurgeService.test.js +++ b/backend/__tests__/unit/services/syntheticAccountPurgeService.test.js @@ -97,6 +97,33 @@ describe('syntheticAccountPurgeService', () => { expect(await User.findById(human._id)).not.toBeNull(); }); + it('releases the applying lock if candidate selection fails before writes', async () => { + const Lock = require('../../../models/AdminUserMutationLock'); + const read = jest.spyOn(User, 'find').mockReturnValueOnce({ + select: () => ({ lean: async () => { throw new Error('selection unavailable'); } }), + }); + try { + await expect(purgeSyntheticAccounts({ apply: true, now: NOW, pool: fakePool() })) + .rejects.toThrow('selection unavailable'); + } finally { read.mockRestore(); } + expect(await Lock.countDocuments()).toBe(0); + await expect(purgeSyntheticAccounts({ apply: true, now: NOW, pool: fakePool() })) + .resolves.toMatchObject({ examined: 0 }); + }); + + it('retains the applying lock after its first cleanup write rejects', async () => { + await syntheticAccount(); + const Lock = require('../../../models/AdminUserMutationLock'); + const write = jest.spyOn(Task, 'deleteMany').mockRejectedValueOnce(new Error('cleanup outcome unknown')); + try { + await expect(purgeSyntheticAccounts({ apply: true, now: NOW, pool: fakePool() })) + .rejects.toThrow('cleanup outcome unknown'); + } finally { write.mockRestore(); } + expect(await Lock.countDocuments()).toBe(1); + await expect(purgeSyntheticAccounts({ apply: true, now: NOW, pool: fakePool() })) + .rejects.toMatchObject({ code: 'ADMIN_MUTATION_BUSY' }); + }); + it('a dry run counts the footprint and deletes nothing', async () => { const { human, pod } = await syntheticAccount(); const report = await purgeSyntheticAccounts({ now: NOW, pool: fakePool() }); diff --git a/backend/routes/admin/users.ts b/backend/routes/admin/users.ts index 91032e04ac..83cfe70b6e 100644 --- a/backend/routes/admin/users.ts +++ b/backend/routes/admin/users.ts @@ -142,7 +142,7 @@ const mutationResponse = (status: number, body: Record) => ({ s // PATCH /api/admin/users/:userId/role router.patch('/:userId/role', adminWriteLimiter, auth, adminAuth, async (req: any, res: any) => { try { - const result = await withAdminUserMutation(String(req.user?.id || ''), async () => { + const result = await withAdminUserMutation(String(req.user?.id || ''), async (beginWrite) => { const { userId } = req.params; const { role } = req.body || {}; const nextRole = String(role || '').trim().toLowerCase(); @@ -167,6 +167,7 @@ router.patch('/:userId/role', adminWriteLimiter, auth, adminAuth, async (req: an } target.role = nextRole; + beginWrite(); await target.save(); return mutationResponse(200, { @@ -245,7 +246,7 @@ router.patch('/:userId/entitlements', adminWriteLimiter, auth, adminAuth, async // first — prevents an admin-vs-admin lockout war). router.patch('/:userId/ban', adminWriteLimiter, auth, adminAuth, async (req: any, res: any) => { try { - const result = await withAdminUserMutation(String(req.user?.id || ''), async () => { + const result = await withAdminUserMutation(String(req.user?.id || ''), async (beginWrite) => { const { userId } = req.params; const { banned, reason } = req.body || {}; if (typeof banned !== 'boolean') { @@ -270,6 +271,7 @@ router.patch('/:userId/ban', adminWriteLimiter, auth, adminAuth, async (req: any target.banned = banned; target.bannedAt = banned ? new Date() : undefined; target.banReason = banned ? (reason || '').slice(0, 500) : undefined; + beginWrite(); await target.save(); return mutationResponse(200, { ok: true, user: sanitizeUser(target) }); @@ -303,7 +305,7 @@ router.post('/:userId/sessions/revoke', adminWriteLimiter, auth, adminAuth, asyn router.delete('/:userId', adminWriteLimiter, auth, adminAuth, async (req: any, res: any) => { try { - const result = await withAdminUserMutation(String(req.user?.id || ''), async () => { + const result = await withAdminUserMutation(String(req.user?.id || ''), async (beginWrite) => { const { userId } = req.params; if (req.user?.id && String(req.user.id) === String(userId)) { return mutationResponse(400, { error: 'You cannot delete your own account from admin tools' }); @@ -377,6 +379,7 @@ router.delete('/:userId', adminWriteLimiter, auth, adminAuth, async (req: any, r }); } + beginWrite(); await target.deleteOne(); return mutationResponse(200, { message: 'User deleted successfully' }); }); diff --git a/backend/services/adminUserMutationService.ts b/backend/services/adminUserMutationService.ts index 86a81f2c3b..093f92ff11 100644 --- a/backend/services/adminUserMutationService.ts +++ b/backend/services/adminUserMutationService.ts @@ -14,7 +14,7 @@ const writeConcern = { w: 'majority' as const, wtimeout: 5000 }; // Standalone Mongo is supported. A primary-key insert serializes online role, // ban and deletion handlers across processes without requiring transactions. // A crash fails closed until operator recovery; see docs/runbooks/admin-user-mutation-lock.md. -export const withAdminUserMutationLock = async (operation: () => Promise): Promise => { +export const withAdminUserMutationLock = async (operation: (beginWrite: () => void) => Promise): Promise => { const owner = randomUUID(); try { await AdminUserMutationLock.create([{ _id: LOCK_ID, owner, acquiredAt: new Date() }], { writeConcern }); @@ -25,21 +25,34 @@ export const withAdminUserMutationLock = async (operation: () => Promise): } throw error; } - // A rejected write can still have an unknown outcome. Do not release on - // callback failure: the original database write might complete later. - const value = await operation(); - const released = await AdminUserMutationLock.deleteOne({ _id: LOCK_ID, owner }, { writeConcern }); - if (released.deletedCount !== 1) { - throw new AdminUserMutationError(503, 'ADMIN_MUTATION_LOCK_LOST', - 'Administrator mutation lock changed unexpectedly.'); + const release = async () => { + const released = await AdminUserMutationLock.deleteOne({ _id: LOCK_ID, owner }, { writeConcern }); + if (released.deletedCount !== 1) { + throw new AdminUserMutationError(503, 'ADMIN_MUTATION_LOCK_LOST', + 'Administrator mutation lock changed unexpectedly.'); + } + }; + let writeStarted = false; + let value: T; + try { + // Callers must signal before submitting any write, including delegated cleanup. + value = await operation(() => { writeStarted = true; }); + } catch (error) { + // Read failures cannot leave a late write. Once a write was submitted, its + // rejection may have an unknown outcome, so retain the lock for recovery. + if (!writeStarted) await release(); + throw error; } + await release(); return value; }; // The scheduler's applying purge uses the same lock without a human actor. // HTTP callers must use this authorization-checking entry point. -export const withAdminUserMutation = async (actorId: string, operation: () => Promise): Promise => { - const outcome = await withAdminUserMutationLock(async () => { +export const withAdminUserMutation = async ( + actorId: string, operation: (beginWrite: () => void) => Promise, +): Promise => { + const outcome = await withAdminUserMutationLock(async (beginWrite) => { // The request may have passed adminAuth before a competing demotion. Read // authorization again only after acquiring the shared mutation lock. const actor = await User.findById(actorId); @@ -48,7 +61,7 @@ export const withAdminUserMutation = async (actorId: string, operation: () => return { ok: false as const, error: new AdminUserMutationError(403, 'ADMIN_ACCESS_CHANGED', 'Administrator access changed. Sign in again.') }; } - return { ok: true as const, value: await operation() }; + return { ok: true as const, value: await operation(beginWrite) }; }); if (!outcome.ok) throw outcome.error; return outcome.value; diff --git a/backend/services/syntheticAccountPurgeService.ts b/backend/services/syntheticAccountPurgeService.ts index cc6b9abf6a..444e9a09c4 100644 --- a/backend/services/syntheticAccountPurgeService.ts +++ b/backend/services/syntheticAccountPurgeService.ts @@ -98,6 +98,7 @@ const loadPool = (): PgPool | null => { const purgeSyntheticAccountsUnlocked = async ( { apply = false, minAgeMs = DEFAULT_MIN_AGE_MS, now = new Date(), pool = loadPool() }: { apply?: boolean; minAgeMs?: number; now?: Date; pool?: PgPool | null } = {}, + beginWrite: () => void = () => {}, ): Promise => { const report: PurgeReport = { dryRun: !apply, examined: 0, accounts: [], counts: {} }; const tally = (store: string, n: number | undefined) => { @@ -106,6 +107,7 @@ const purgeSyntheticAccountsUnlocked = async ( // Dry run counts what a delete would remove; apply deletes and counts that. const remove = async (store: string, model: any, filter: Record) => { if (!apply) return tally(store, await model.countDocuments(filter)); + beginWrite(); const res = await model.deleteMany(filter); return tally(store, res.deletedCount); }; @@ -116,6 +118,7 @@ const purgeSyntheticAccountsUnlocked = async ( tally(store, Number(rows[0]?.n || 0)); return; } + beginWrite(); const res = await pool.query(sql, params); tally(store, res.rowCount ?? res.rows.length); }; @@ -194,7 +197,10 @@ const purgeSyntheticAccountsUnlocked = async ( await remove('podmemberfirstmessages', PodMemberFirstMessage, { podId }); await remove('onboardingsilenceepisodes', OnboardingSilenceEpisode, { podId }); await pgRemove('pg.message_claims', 'DELETE FROM message_claims WHERE pod_id = $1', [podId]); - if (apply) await deletePodCascade(podId); + if (apply) { + beginWrite(); + await deletePodCascade(podId); + } tally('pods', 1); } @@ -222,6 +228,7 @@ const purgeSyntheticAccountsUnlocked = async ( await remove('onboardingsilenceepisodes', OnboardingSilenceEpisode, { userId: human._id }); await remove('podmemberfirstmessages', PodMemberFirstMessage, { userId: human._id }); if (apply) { + beginWrite(); const res = await Pod.updateMany({ members: human._id }, { $pull: { members: human._id } }); tally('pod.members', res.modifiedCount); } else { @@ -240,7 +247,7 @@ const purgeSyntheticAccountsUnlocked = async ( export const purgeSyntheticAccounts = async ( options: Parameters[0] = {}, ): Promise => (options.apply - ? withAdminUserMutationLock(() => purgeSyntheticAccountsUnlocked(options)) + ? withAdminUserMutationLock((beginWrite) => purgeSyntheticAccountsUnlocked(options, beginWrite)) : purgeSyntheticAccountsUnlocked(options)); // CJS compat, same shape as sibling services. diff --git a/docs/runbooks/admin-user-mutation-lock.md b/docs/runbooks/admin-user-mutation-lock.md index 81a0967d94..16426e4ed1 100644 --- a/docs/runbooks/admin-user-mutation-lock.md +++ b/docs/runbooks/admin-user-mutation-lock.md @@ -20,9 +20,10 @@ resume and write its User row after an expiring lease has been stolen. A separate lock document cannot fence that write on standalone Mongo. Preserving an administrator takes precedence over automatically recovering mutation availability. A worker crash, a callback failure or an uncertain database acknowledgement may leave -this small lock row behind. Callback failures retain it because a rejected -write may still complete later; known pre-write authorization refusals release -it safely. Reads and unrelated application work continue; +this small lock row behind. Each caller signals before its first write, including +delegated cleanup. Failures before that signal release the lock; failures after +it retain the lock because a rejected write may still complete later. Known +pre-write authorization refusals also release it safely. Reads and unrelated application work continue; administrator role, ban and deletion writes fail closed until recovery. ## Deploying the change