From 0cd74f3f90d141dc8d3f94758c7e86cfa6cce6f8 Mon Sep 17 00:00:00 2001 From: razyd Date: Sun, 11 Oct 2026 11:39:32 +0200 Subject: [PATCH 1/2] fix(notifications): send manual notifications only to active users - user-repository.ts: getActiveIds returns which of the given ids exist and are active. - notification-controller.ts: the group and general manual notifications answer 400 NOTIFICATIONS_INVALID_RECIPIENTS when any recipient is inactive or unknown, before anything is written. - error-codes.ts, errors.json (es, en), errorCodeMessage.ts: the new code and its texts. - manual-recipients.test.ts: both endpoints refuse a bad recipient, send to active ones and skip the lookup without userIds. Why: an inactive user got the notice, and an unknown id broke the recipients insert after the notification was created. Checked with curl against the local database: unknown or mixed ids answer 400, an active one 201. --- backend/config/error-codes.ts | 1 + .../notifications/notification-controller.ts | 27 ++++++ backend/repositories/auth/user-repository.ts | 12 ++- .../notifications/manual-recipients.test.ts | 85 +++++++++++++++++++ frontend/app/lib/helpers/errorCodeMessage.ts | 1 + frontend/messages/en/errors.json | 1 + frontend/messages/es/errors.json | 1 + 7 files changed, 127 insertions(+), 1 deletion(-) create mode 100644 backend/tests/notifications/manual-recipients.test.ts diff --git a/backend/config/error-codes.ts b/backend/config/error-codes.ts index 39d90afe..c74d2454 100644 --- a/backend/config/error-codes.ts +++ b/backend/config/error-codes.ts @@ -387,6 +387,7 @@ export const ERROR_CODES = { NOTIFICATIONS_MISSING_REQUIRED_FIELDS: 'NOTIFICATIONS_MISSING_REQUIRED_FIELDS', NOTIFICATIONS_INVALID_SCHEDULED_DATE: 'NOTIFICATIONS_INVALID_SCHEDULED_DATE', NOTIFICATIONS_INVALID_MODULE: 'NOTIFICATIONS_INVALID_MODULE', + NOTIFICATIONS_INVALID_RECIPIENTS: 'NOTIFICATIONS_INVALID_RECIPIENTS', NOTIFICATIONS_NOT_FOUND: 'NOTIFICATIONS_NOT_FOUND', NOTIFICATIONS_FETCH_ERROR: 'NOTIFICATIONS_FETCH_ERROR', NOTIFICATIONS_FETCH_UNREAD_ERROR: 'NOTIFICATIONS_FETCH_UNREAD_ERROR', diff --git a/backend/controllers/notifications/notification-controller.ts b/backend/controllers/notifications/notification-controller.ts index fbb74b21..7e5e9637 100644 --- a/backend/controllers/notifications/notification-controller.ts +++ b/backend/controllers/notifications/notification-controller.ts @@ -9,6 +9,7 @@ import { NotificationModule, } from '../../models/notifications/index' import { ERROR_CODES, SUCCESS_CODES } from '../../config/error-codes.js' +import { UserRepository } from '../../repositories/auth/user-repository.js' import { logger } from '../../config/logger.js' import { generalNotificationSchema, @@ -351,6 +352,19 @@ export class NotificationController { } } + // Only users that exist and are active can receive it (checked before + // creating the notification, so a bad id leaves nothing behind) + if ( + userIds?.length && + (await UserRepository.getActiveIds(userIds)).length !== userIds.length + ) { + return res.status(400).json({ + success: false, + error: ERROR_CODES.NOTIFICATIONS_INVALID_RECIPIENTS, + code: ERROR_CODES.NOTIFICATIONS_INVALID_RECIPIENTS, + }) + } + await NotificationGeneratorService.generateManualNotification( groupId, title, @@ -421,6 +435,19 @@ export class NotificationController { } } + // Only users that exist and are active can receive it (checked before + // creating the notification, so a bad id leaves nothing behind) + if ( + userIds?.length && + (await UserRepository.getActiveIds(userIds)).length !== userIds.length + ) { + return res.status(400).json({ + success: false, + error: ERROR_CODES.NOTIFICATIONS_INVALID_RECIPIENTS, + code: ERROR_CODES.NOTIFICATIONS_INVALID_RECIPIENTS, + }) + } + await NotificationGeneratorService.generateGeneralNotification({ title, message, diff --git a/backend/repositories/auth/user-repository.ts b/backend/repositories/auth/user-repository.ts index 1ca7ad7d..7de783fb 100644 --- a/backend/repositories/auth/user-repository.ts +++ b/backend/repositories/auth/user-repository.ts @@ -5,7 +5,7 @@ import bcrypt from 'bcrypt' import crypto from 'crypto' import { SALT_ROUNDS } from '../../config/config.js' import db from '../../config/db.js' -import { ResultSetHeader } from 'mysql2' +import { ResultSetHeader, type RowDataPacket } from 'mysql2' import { logger } from '../../config/logger.js' import type { User, @@ -226,6 +226,16 @@ export class UserRepository { } } + /** Of the given ids, the ones that exist and are active */ + static async getActiveIds(ids: readonly string[]): Promise { + if (ids.length === 0) return [] + const [rows] = await db.query<(RowDataPacket & { id: string })[]>( + 'SELECT id FROM users WHERE is_active = 1 AND id IN (?)', + [ids] + ) + return rows.map((r) => r.id) + } + /** * Obtener por ID con JOIN directo */ diff --git a/backend/tests/notifications/manual-recipients.test.ts b/backend/tests/notifications/manual-recipients.test.ts new file mode 100644 index 00000000..27ae10cb --- /dev/null +++ b/backend/tests/notifications/manual-recipients.test.ts @@ -0,0 +1,85 @@ +// tests/notifications/manual-recipients.test.ts +// Regression tests: manual notifications accepted any user id. An inactive +// user got the notice, and an unknown one broke the recipients insert after +// the notification was created, leaving it without recipients. Both are now +// refused with NOTIFICATIONS_INVALID_RECIPIENTS before anything is written. + +import { describe, it, expect, vi, beforeEach } from 'vitest' +import type { Request, Response } from 'express' + +vi.mock('../../repositories/auth/user-repository.js', () => ({ + UserRepository: { getActiveIds: vi.fn() }, +})) +vi.mock('../../services/notifications/notification-generator-service.js', () => ({ + NotificationGeneratorService: { + generateManualNotification: vi.fn(), + generateGeneralNotification: vi.fn(), + }, +})) + +const { NotificationController } = + await import('../../controllers/notifications/notification-controller.js') +const { UserRepository } = await import('../../repositories/auth/user-repository.js') +const { NotificationGeneratorService } = + await import('../../services/notifications/notification-generator-service.js') +const { ERROR_CODES } = await import('../../config/error-codes.js') + +const ACTIVE = '11111111-1111-4111-8111-111111111111' +const INACTIVE = '22222222-2222-4222-8222-222222222222' + +function call( + handler: (req: Request, res: Response) => Promise, + body: object, + params = {} +) { + const res = { status: vi.fn().mockReturnThis(), json: vi.fn().mockReturnThis() } + const req = { user: { id: 'u1' }, params, body } as unknown as Request + return handler(req, res as unknown as Response).then(() => res) +} + +const endpoints = [ + ['general', NotificationController.createGeneralNotification, {}], + ['group', NotificationController.createManualNotification, { id: '5' }], +] as const + +describe('manual notification recipients', () => { + beforeEach(() => vi.clearAllMocks()) + + it.each(endpoints)( + '%s: refuses a recipient that is inactive or unknown', + async (_, handler, params) => { + vi.mocked(UserRepository.getActiveIds).mockResolvedValue([ACTIVE]) + + const res = await call( + handler, + { title: 'T', message: 'M', userIds: [ACTIVE, INACTIVE] }, + params + ) + + expect(res.status).toHaveBeenCalledWith(400) + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ code: ERROR_CODES.NOTIFICATIONS_INVALID_RECIPIENTS }) + ) + expect(NotificationGeneratorService.generateGeneralNotification).not.toHaveBeenCalled() + expect(NotificationGeneratorService.generateManualNotification).not.toHaveBeenCalled() + } + ) + + it.each(endpoints)('%s: sends when every recipient is active', async (_, handler, params) => { + vi.mocked(UserRepository.getActiveIds).mockResolvedValue([ACTIVE]) + + const res = await call(handler, { title: 'T', message: 'M', userIds: [ACTIVE] }, params) + + expect(res.status).toHaveBeenCalledWith(201) + }) + + it.each(endpoints)( + '%s: without userIds it does not look users up', + async (_, handler, params) => { + const res = await call(handler, { title: 'T', message: 'M' }, params) + + expect(res.status).toHaveBeenCalledWith(201) + expect(UserRepository.getActiveIds).not.toHaveBeenCalled() + } + ) +}) diff --git a/frontend/app/lib/helpers/errorCodeMessage.ts b/frontend/app/lib/helpers/errorCodeMessage.ts index 09360dee..dce040b1 100644 --- a/frontend/app/lib/helpers/errorCodeMessage.ts +++ b/frontend/app/lib/helpers/errorCodeMessage.ts @@ -300,6 +300,7 @@ const TRANSLATED = new Set([ 'NOTIFICATIONS_INVALID_GROUP_ID', 'NOTIFICATIONS_INVALID_ID', 'NOTIFICATIONS_INVALID_MODULE', + 'NOTIFICATIONS_INVALID_RECIPIENTS', 'NOTIFICATIONS_INVALID_SCHEDULED_DATE', 'NOTIFICATIONS_MARK_ALL_ERROR', 'NOTIFICATIONS_MARK_ERROR', diff --git a/frontend/messages/en/errors.json b/frontend/messages/en/errors.json index 409b8e3a..d89c323f 100644 --- a/frontend/messages/en/errors.json +++ b/frontend/messages/en/errors.json @@ -404,6 +404,7 @@ "NOTIFICATIONS_MISSING_REQUIRED_FIELDS": "Missing required fields: title, message", "NOTIFICATIONS_INVALID_SCHEDULED_DATE": "Invalid date format for scheduled_for", "NOTIFICATIONS_INVALID_MODULE": "Invalid module. Allowed values: groups, parking, logbooks, system", + "NOTIFICATIONS_INVALID_RECIPIENTS": "Some recipient does not exist or is not active", "NOTIFICATIONS_NOT_FOUND": "Notification not found", "NOTIFICATIONS_FETCH_ERROR": "Error fetching notifications", "NOTIFICATIONS_FETCH_UNREAD_ERROR": "Error fetching unread notifications", diff --git a/frontend/messages/es/errors.json b/frontend/messages/es/errors.json index c36f2733..88cdecfc 100644 --- a/frontend/messages/es/errors.json +++ b/frontend/messages/es/errors.json @@ -404,6 +404,7 @@ "NOTIFICATIONS_MISSING_REQUIRED_FIELDS": "Faltan campos obligatorios: título, mensaje", "NOTIFICATIONS_INVALID_SCHEDULED_DATE": "Formato de fecha inválido para scheduled_for", "NOTIFICATIONS_INVALID_MODULE": "Módulo inválido. Valores permitidos: groups, parking, logbooks, system", + "NOTIFICATIONS_INVALID_RECIPIENTS": "Algún destinatario no existe o no está activo", "NOTIFICATIONS_NOT_FOUND": "Notificación no encontrada", "NOTIFICATIONS_FETCH_ERROR": "Error al obtener notificaciones", "NOTIFICATIONS_FETCH_UNREAD_ERROR": "Error al obtener notificaciones no leídas", From 53df25891cdfa69ad95f4a2e84e391db22cb6dcd Mon Sep 17 00:00:00 2001 From: razyd Date: Mon, 12 Oct 2026 19:44:32 +0200 Subject: [PATCH 2/2] test(notifications): mock the database pool in the manual recipients test - manual-recipients.test.ts: mock config/db.js, which the controller loads through NotificationRepository. Why: CI has no backend/.env, so loading the pool config threw "Falta LOCAL_DB_PASSWORD" and the file failed before any test ran; locally the .env hid it. Reproduced with LOCAL_DB_PASSWORD empty; now the whole suite passes that way. --- backend/tests/notifications/manual-recipients.test.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/backend/tests/notifications/manual-recipients.test.ts b/backend/tests/notifications/manual-recipients.test.ts index 27ae10cb..021e0fd8 100644 --- a/backend/tests/notifications/manual-recipients.test.ts +++ b/backend/tests/notifications/manual-recipients.test.ts @@ -7,6 +7,9 @@ import { describe, it, expect, vi, beforeEach } from 'vitest' import type { Request, Response } from 'express' +// The controller also loads NotificationRepository: without this the pool's +// config needs a real database password (CI has none) +vi.mock('../../config/db.js', () => ({ default: { query: vi.fn(), getConnection: vi.fn() } })) vi.mock('../../repositories/auth/user-repository.js', () => ({ UserRepository: { getActiveIds: vi.fn() }, }))