From 8b694200eebc505af8ba5f8a2ebe26261705679e Mon Sep 17 00:00:00 2001 From: ascpixi <44982772+ascpixi@users.noreply.github.com> Date: Thu, 23 Jul 2026 15:40:00 -0400 Subject: [PATCH] fix(admin): hard-reject banned users' queued projects and revert pending approval hours --- backend/src/admin/admin-ban.spec.ts | 254 ++++++++++++++++++ backend/src/admin/admin.service.ts | 170 +++++++++--- ...1784800000000-ZeroBanLeakedPendingHours.ts | 29 ++ 3 files changed, 420 insertions(+), 33 deletions(-) create mode 100644 backend/src/admin/admin-ban.spec.ts create mode 100644 backend/src/migrations/1784800000000-ZeroBanLeakedPendingHours.ts diff --git a/backend/src/admin/admin-ban.spec.ts b/backend/src/admin/admin-ban.spec.ts new file mode 100644 index 0000000..876e130 --- /dev/null +++ b/backend/src/admin/admin-ban.spec.ts @@ -0,0 +1,254 @@ +import { AdminService } from './admin.service'; + +// banUser must silently hard-reject a banned user's queued projects, and must +// discard the pending (not-yet-paid-out) first-pass approval of a +// 'fraud_pending' project — otherwise the stale overrideHours starve the +// Hackatime cap check if the ban is undone ("Cannot approve Xh of new work — +// Hackatime shows only Yh..."). + +function build(overrides: { + projects: any[]; + submission?: any; + review?: any; + getPerms?: jest.Mock; +}) { + const saved: { projects: any[]; reviews: any[] } = { + projects: [], + reviews: [], + }; + const submissionUpdates: any[] = []; + + const service = new AdminService( + { get: jest.fn() } as any, // configService + { + findOne: jest + .fn() + .mockResolvedValue({ id: 'user-1', email: 'u@x.com', name: 'U' }), + } as any, // userRepo + { delete: jest.fn() } as any, // sessionRepo + { + // Honors the where.userId filter so the sweep (all queued projects) and + // rejectQueuedProjects (one user's projects) see the right subsets. + find: jest + .fn() + .mockImplementation((opts: { where?: { userId?: string } }) => + Promise.resolve( + overrides.projects.filter( + (p: { userId?: string }) => + !opts?.where?.userId || p.userId === opts.where.userId, + ), + ), + ), + save: jest.fn().mockImplementation((p: object) => { + saved.projects.push({ ...p }); + return Promise.resolve(p); + }), + update: jest.fn(), + } as any, // projectRepo + {} as any, // auditLogRepo + {} as any, // newsRepo + { + findOne: jest.fn().mockResolvedValue(overrides.review ?? null), + save: jest.fn().mockImplementation((r: object) => { + saved.reviews.push({ ...r }); + return Promise.resolve(r); + }), + } as any, // reviewRepo + {} as any, // shopRepo + { find: jest.fn().mockResolvedValue([]) } as any, // orderRepo + { + findOne: jest.fn().mockResolvedValue(overrides.submission ?? null), + update: jest.fn().mockImplementation((where: object, set: object) => { + submissionUpdates.push({ where, set }); + return Promise.resolve(); + }), + } as any, // submissionRepo + {} as any, // eventRepo + { + updatePerms: jest.fn(), + getPerms: overrides.getPerms ?? jest.fn(), + } as any, // rsvpService + { log: jest.fn() } as any, // auditLogService + {} as any, // hcaService + {} as any, // airtableSync + {} as any, // slackService + {} as any, // slackNotify + { refundOrder: jest.fn() } as any, // shopService + ); + + return { service, saved, submissionUpdates }; +} + +describe('AdminService.banUser queued-project sweep', () => { + it('hard-rejects and reverts the pending approval delta of a fraud_pending project', async () => { + const { service, saved } = build({ + projects: [ + { + id: 'p1', + userId: 'user-1', + status: 'fraud_pending', + overrideHours: 3.4, + internalHours: 4.4, + pipesGranted: 0, + isGolden: true, + }, + ], + submission: { + id: 's1', + status: 'unreviewed', + overrideHours: 3.4, + internalHours: 4.4, + }, + review: { + id: 'r1', + submissionId: 's1', + status: 'approved', + returnedById: null, + }, + }); + + await service.banUser('user-1', 'admin-1'); + + expect(saved.projects).toHaveLength(1); + expect(saved.projects[0]).toMatchObject({ + id: 'p1', + status: 'rejected', + overrideHours: 0, + internalHours: 0, + isGolden: false, + }); + // The pending first-pass approval is invalidated, not left as a live + // approval, and attributed to the banning admin. + expect(saved.reviews).toHaveLength(1); + expect(saved.reviews[0]).toMatchObject({ + id: 'r1', + status: 'returned', + returnedById: 'admin-1', + }); + }); + + it('preserves the standing prior approval under a pending reship delta', async () => { + // Prior approval paid out 3.0h (pipes granted); the pending reship added + // 2.0h on top. Discarding the pending approval must return the project to + // its 3.0h baseline, not zero. + const { service, saved } = build({ + projects: [ + { + id: 'p1', + userId: 'user-1', + status: 'fraud_pending', + overrideHours: 5, + internalHours: 5, + pipesGranted: 3, + isGolden: false, + }, + ], + submission: { + id: 's2', + status: 'unreviewed', + overrideHours: 2, + internalHours: 2, + }, + review: { + id: 'r2', + submissionId: 's2', + status: 'approved', + returnedById: null, + }, + }); + + await service.banUser('user-1', 'admin-1'); + + expect(saved.projects[0]).toMatchObject({ + status: 'rejected', + overrideHours: 3, + internalHours: 3, + }); + }); + + it('leaves the hours of an unreviewed project untouched', async () => { + // An 'unreviewed' resubmission on top of a paid-out approval keeps its + // cumulative hours — only the status changes. + const { service, saved } = build({ + projects: [ + { + id: 'p1', + userId: 'user-1', + status: 'unreviewed', + overrideHours: 6, + internalHours: 6, + pipesGranted: 6, + isGolden: true, + }, + ], + }); + + await service.banUser('user-1', 'admin-1'); + + expect(saved.projects[0]).toMatchObject({ + status: 'rejected', + overrideHours: 6, + internalHours: 6, + isGolden: true, + }); + expect(saved.reviews).toHaveLength(0); + }); +}); + +// The hourly/boot sweep catches bans applied directly in Airtable (which never +// go through banUser) and backfills users banned before the sweep existed. +describe('AdminService.sweepBannedUsersQueuedProjects', () => { + const prevEnv = process.env.NODE_ENV; + afterEach(() => { + process.env.NODE_ENV = prevEnv; + }); + + it('rejects queued projects of banned owners only', async () => { + process.env.NODE_ENV = 'production'; + const { service, saved } = build({ + projects: [ + { + id: 'p1', + userId: 'user-1', + status: 'unreviewed', + user: { id: 'user-1', email: 'banned@x.com', name: 'B' }, + }, + { + id: 'p2', + userId: 'user-2', + status: 'unreviewed', + user: { id: 'user-2', email: 'ok@x.com', name: 'O' }, + }, + ], + getPerms: jest + .fn() + .mockImplementation((email: string) => + Promise.resolve(email === 'banned@x.com' ? 'Banned' : 'User'), + ), + }); + + await service.sweepBannedUsersQueuedProjects(); + + expect(saved.projects.map((p: { id: string }) => p.id)).toEqual(['p1']); + expect(saved.projects[0]).toMatchObject({ status: 'rejected' }); + }); + + it('never rejects when the Airtable perms lookup fails', async () => { + process.env.NODE_ENV = 'production'; + const { service, saved } = build({ + projects: [ + { + id: 'p1', + userId: 'user-1', + status: 'unreviewed', + user: { id: 'user-1', email: 'x@x.com', name: 'X' }, + }, + ], + getPerms: jest.fn().mockRejectedValue(new Error('airtable down')), + }); + + await service.sweepBannedUsersQueuedProjects(); + + expect(saved.projects).toHaveLength(0); + }); +}); diff --git a/backend/src/admin/admin.service.ts b/backend/src/admin/admin.service.ts index 6142b76..f86745e 100644 --- a/backend/src/admin/admin.service.ts +++ b/backend/src/admin/admin.service.ts @@ -4,6 +4,7 @@ import { ForbiddenException, Logger, NotFoundException, + OnApplicationBootstrap, } from '@nestjs/common'; import { ConfigService } from '@nestjs/config'; import { InjectRepository } from '@nestjs/typeorm'; @@ -53,7 +54,7 @@ const VALID_PERMS = [ const ELEVATED_PERMS = ['Fulfiller', 'Super Admin']; @Injectable() -export class AdminService { +export class AdminService implements OnApplicationBootstrap { private readonly logger = new Logger(AdminService.name); private static readonly AI_BREAKDOWN_CATEGORIES = new Set([ 'ai coding', @@ -461,26 +462,8 @@ export class AdminService { await this.sessionRepo.delete({ userId }); // 3. Pull the user's in-flight work out of the review/audit queues so - // reviewers don't waste time on a banned account. 'unreviewed' projects - // sit in the first-review queue and 'fraud_pending' ones in the audit - // queue; both move to 'changes_needed' — the same terminal state a - // ban-via-project review leaves a project in. Open submissions are - // flipped too so nothing keeps the project queued. - const queuedProjects = await this.projectRepo.find({ - where: { userId, status: In(['unreviewed', 'fraud_pending']) }, - select: ['id'], - }); - if (queuedProjects.length > 0) { - const projectIds = queuedProjects.map((p) => p.id); - await this.projectRepo.update( - { id: In(projectIds) }, - { status: 'changes_needed' }, - ); - await this.submissionRepo.update( - { projectId: In(projectIds), status: 'unreviewed' }, - { status: 'changes_needed' }, - ); - } + // reviewers don't waste time on a banned account. + const queuedCount = await this.rejectQueuedProjects(userId, adminId); // 4. Refund the user's pending shop orders so they drop out of the // fulfilment queue. refundOrder restocks the item and cascade-deletes @@ -499,8 +482,8 @@ export class AdminService { // 5. Audit log on the banned user's record const identifier = user.name || user.slackId || user.hcaSub; const cleanup = - queuedProjects.length || pendingOrders.length - ? ` (cleared ${queuedProjects.length} queued project${queuedProjects.length === 1 ? '' : 's'}, ${pendingOrders.length} pending order${pendingOrders.length === 1 ? '' : 's'})` + queuedCount || pendingOrders.length + ? ` (cleared ${queuedCount} queued project${queuedCount === 1 ? '' : 's'}, ${pendingOrders.length} pending order${pendingOrders.length === 1 ? '' : 's'})` : ''; await this.auditLogService.log(userId, 'admin_ban', `Banned user ${identifier}${cleanup}`); @@ -510,6 +493,118 @@ export class AdminService { } } + /** + * Silently hard-reject a banned user's in-flight work. 'unreviewed' projects + * sit in the first-review queue and 'fraud_pending' ones in the audit queue; + * both go straight to the terminal 'rejected' status — no resubmission, and + * deliberately no builder DM (unlike a reviewer rejection). + * + * A 'fraud_pending' project carries a not-yet-finalised first-pass approval: + * reviewProject already bumped the project's hours by the submitted delta, + * but no pipes were granted. Discard that pending approval the same way the + * second-pass return path does — revert the delta, drop golden, and + * invalidate the review row — so the stale hours can't starve the Hackatime + * cap check ("Cannot approve Xh of new work...") if the ban is ever undone. + * + * Returns the number of projects rejected. + */ + private async rejectQueuedProjects( + userId: string, + adminId?: string, + ): Promise { + const queuedProjects = await this.projectRepo.find({ + where: { userId, status: In(['unreviewed', 'fraud_pending']) }, + }); + for (const project of queuedProjects) { + if (project.status === 'fraud_pending') { + const submission = await this.submissionRepo.findOne({ + where: { projectId: project.id, status: 'unreviewed' }, + order: { createdAt: 'DESC' }, + }); + const subOverride = submission?.overrideHours ?? 0; + const subInternal = submission?.internalHours ?? 0; + if (subOverride > 0) { + project.overrideHours = Math.max( + 0, + Math.round(((project.overrideHours ?? 0) - subOverride) * 10) / 10, + ); + } + if (subInternal > 0) { + project.internalHours = Math.max( + 0, + Math.round(((project.internalHours ?? 0) - subInternal) * 10) / 10, + ); + } + project.isGolden = false; + if (submission) { + const firstPass = await this.reviewRepo.findOne({ + where: { submissionId: submission.id, status: 'approved' }, + order: { createdAt: 'DESC' }, + }); + if (firstPass) { + firstPass.status = 'returned'; + firstPass.returnedById = adminId ?? null; + await this.reviewRepo.save(firstPass); + } + } + } + project.status = 'rejected'; + await this.projectRepo.save(project); + } + if (queuedProjects.length > 0) { + await this.submissionRepo.update( + { + projectId: In(queuedProjects.map((p) => p.id)), + status: 'unreviewed', + }, + { status: 'rejected' }, + ); + } + return queuedProjects.length; + } + + // Safety net for bans applied directly in Airtable (Perms → 'Banned') + // without going through banUser: those never trigger the queue sweep, so the + // banned user's projects would sit in the review queue indefinitely. Checks + // every queued project owner's Airtable perms and silently hard-rejects the + // queued work of anyone banned. The boot run doubles as the backfill for + // users banned before this sweep existed. + @Cron('30 * * * *', { name: 'reject-banned-user-queued-projects' }) + async sweepBannedUsersQueuedProjects(): Promise { + if (process.env.NODE_ENV !== 'production') return; + const queued = await this.projectRepo.find({ + where: { status: In(['unreviewed', 'fraud_pending']) }, + relations: ['user'], + }); + const owners = new Map(); + for (const project of queued) { + if (project.user) owners.set(project.user.id, project.user); + } + for (const owner of owners.values()) { + let perms: string | null; + try { + perms = await this.rsvpService.getPerms(owner.email); + } catch { + // Airtable hiccup — never reject on a failed lookup; the next sweep + // will retry. + continue; + } + if (perms !== 'Banned') continue; + const count = await this.rejectQueuedProjects(owner.id); + this.logger.warn( + `Rejected ${count} queued project(s) of banned user ${owner.id} (${owner.name || owner.slackId || owner.hcaSub})`, + ); + } + } + + onApplicationBootstrap(): void { + if (process.env.NODE_ENV !== 'production') return; + // Fire-and-forget so a slow Airtable never delays boot. + void this.sweepBannedUsersQueuedProjects().catch((err) => + this.logger.warn(`Banned-user queue sweep failed at boot: ${err}`), + ); + } + async banAndRejectProject( projectId: string, reviewerId: string, @@ -534,12 +629,26 @@ export class AdminService { await this.userRepo.save(project.user); } - // 1. Reject the project - project.status = 'changes_needed'; - project.isGolden = false; - await this.projectRepo.save(project); + // 1. Ban the user — pass the requester's tier so a non-Super-Admin (e.g. a + // Fraud Reviewer) cannot ban a Fulfiller/Super Admin through the project + // -review path, matching the direct /ban endpoint's elevated-target guard. + // Runs before the rejection below on purpose: banUser's queue sweep must + // still see this project in its queued status to discard a pending + // (fraud_pending) approval's hours, and its elevated-target guard then + // aborts the whole operation before anything is mutated. + await this.banUser(project.userId, undefined, requesterIsSuperAdmin); + + // 2. Reject the project — terminal 'rejected', matching the queue sweep + // banUser just ran (a banned user's projects are dead, not fixable). + // Targeted update, not a save of the entity loaded above — banUser may + // have just reverted its pending-approval hours, and saving the stale + // entity would resurrect them. + await this.projectRepo.update(projectId, { + status: 'rejected', + isGolden: false, + }); - // 2. Save review record + // 3. Save review record const review = this.reviewRepo.create({ projectId, reviewerId, @@ -551,11 +660,6 @@ export class AdminService { }); await this.reviewRepo.save(review); - // 3. Ban the user — pass the requester's tier so a non-Super-Admin (e.g. a - // Fraud Reviewer) cannot ban a Fulfiller/Super Admin through the project - // -review path, matching the direct /ban endpoint's elevated-target guard. - await this.banUser(project.userId, undefined, requesterIsSuperAdmin); - // 4. Audit logs await this.auditLogService.log(project.userId, 'project_reviewed', `Project "${project.name}" was rejected`); await this.auditLogService.log(project.userId, 'admin_ban', `Banned via project review of "${project.name}"`); diff --git a/backend/src/migrations/1784800000000-ZeroBanLeakedPendingHours.ts b/backend/src/migrations/1784800000000-ZeroBanLeakedPendingHours.ts new file mode 100644 index 0000000..e92e24b --- /dev/null +++ b/backend/src/migrations/1784800000000-ZeroBanLeakedPendingHours.ts @@ -0,0 +1,29 @@ +import { MigrationInterface, QueryRunner } from 'typeorm'; + +export class ZeroBanLeakedPendingHours1784800000000 implements MigrationInterface { + async up(q: QueryRunner): Promise { + // Repair projects whose pending first-pass approval hours leaked when + // the author was banned: banUser kicked 'fraud_pending' projects to + // 'changes_needed' without reverting the not-yet-paid-out hours delta + // that reviewProject had already added. After an unban + resubmit, the + // stale override_hours starve the Hackatime cap check ("Cannot approve + // Xh of new work — Hackatime shows only Yh of new time since last + // approval") even though nothing was ever paid out. + // + // Invariant restored here: an unreviewed/changes_needed project with no + // pipes granted has no standing approval, so its approved-hours + // counters must be zero. Projects with pipes_granted > 0 keep their + // hours — those reflect a prior approval that still stands. + await q.query(` + UPDATE "projects" + SET "override_hours" = 0, "internal_hours" = 0 + WHERE "status" IN ('unreviewed', 'changes_needed') + AND COALESCE("pipes_granted", 0) = 0 + AND COALESCE("override_hours", 0) > 0 + `); + } + + async down(): Promise { + // Data repair — not reversible. + } +}