From 38bccaccf9b3472e3ced5ec4a998beb8c9e425c1 Mon Sep 17 00:00:00 2001 From: Nathnael Date: Mon, 20 Jul 2026 08:19:59 +0000 Subject: [PATCH] fix: normalized the region and logged the otp properly --- apps/edr-freight-api/src/app.module.ts | 2 + .../2400000000000-NormalizeCompanyRegions.ts | 77 +++++++++ .../companies/dto/update-profile.dto.ts | 13 +- .../companies/services/etrade.service.ts | 7 +- .../services/region-normalization.spec.ts | 54 +++++++ .../src/modules/health/health.controller.ts | 108 +++++++++++++ .../src/modules/health/health.module.ts | 14 ++ .../modules/notifications/broker.util.spec.ts | 90 +++++++++++ .../src/modules/notifications/broker.util.ts | 92 +++++++++++ .../notifications/email-client.service.ts | 36 +++-- .../notifications/sms-client.service.ts | 64 +++++--- .../src/modules/otp/otp.controller.ts | 8 +- .../src/modules/otp/otp.service.spec.ts | 29 +++- .../src/modules/otp/otp.service.ts | 150 ++++++++++++++++-- .../src/pages/accounts/CompanyProfileForm.tsx | 28 +++- .../accounts/companyProfileForm/schema.ts | 5 +- .../src/freight/ethiopian-regions.catalog.ts | 97 +++++++++++ packages/types/src/freight/index.ts | 1 + 18 files changed, 812 insertions(+), 63 deletions(-) create mode 100644 apps/edr-freight-api/src/migrations/2400000000000-NormalizeCompanyRegions.ts create mode 100644 apps/edr-freight-api/src/modules/companies/services/region-normalization.spec.ts create mode 100644 apps/edr-freight-api/src/modules/health/health.controller.ts create mode 100644 apps/edr-freight-api/src/modules/health/health.module.ts create mode 100644 apps/edr-freight-api/src/modules/notifications/broker.util.spec.ts create mode 100644 apps/edr-freight-api/src/modules/notifications/broker.util.ts create mode 100644 packages/types/src/freight/ethiopian-regions.catalog.ts diff --git a/apps/edr-freight-api/src/app.module.ts b/apps/edr-freight-api/src/app.module.ts index a347b163b..3ca5f6cb1 100644 --- a/apps/edr-freight-api/src/app.module.ts +++ b/apps/edr-freight-api/src/app.module.ts @@ -44,6 +44,7 @@ import { FileUploadSettingsModule } from "./modules/file-upload-settings/file-up import { DropdownSettingsModule } from "./modules/dropdown-settings/dropdown-settings.module"; import { ContractTemplatesModule } from "./modules/contract-templates/contract-templates.module"; import { OtpModule } from "./modules/otp/otp.module"; +import { HealthModule } from "./modules/health/health.module"; import { RuleEngineModule } from "./modules/rule-engine/rule-engine.module"; import { BackofficeModule } from "./modules/backoffice/backoffice.module"; import { DemoPermissionsModule } from "./modules/demo-permissions/demo-permissions.module"; @@ -166,6 +167,7 @@ import { LoggerMiddleware } from "./logger.middleware"; DropdownSettingsModule, ContractTemplatesModule, OtpModule, + HealthModule, RuleEngineModule, BackofficeModule, DemoPermissionsModule, diff --git a/apps/edr-freight-api/src/migrations/2400000000000-NormalizeCompanyRegions.ts b/apps/edr-freight-api/src/migrations/2400000000000-NormalizeCompanyRegions.ts new file mode 100644 index 000000000..b3369b82d --- /dev/null +++ b/apps/edr-freight-api/src/migrations/2400000000000-NormalizeCompanyRegions.ts @@ -0,0 +1,77 @@ +import { MigrationInterface, QueryRunner } from 'typeorm'; + +/** + * `freight.companies.region` was free text until region became a closed set + * (see ETHIOPIAN_REGIONS in @edr/types). This normalizes the rows written under + * the old rules so they satisfy the new dropdown. + * + * Two classes of bad data exist, handled differently: + * + * - Unambiguous spelling/case drift ("Addis ababa", "oromoia") — rewritten to + * the canonical spelling. + * - Values that are not regions at all ("Arba Minch", a city), and rows whose + * region contradicts their own zone/woreda — set to NULL. These are NOT + * guessed at: inferring "Gurage/Meskan" means Central Ethiopia would silently + * overwrite what the customer actually submitted. NULL surfaces the gap and + * the required dropdown forces a deliberate pick on next edit. + */ +export class NormalizeCompanyRegions2400000000000 implements MigrationInterface { + name = 'NormalizeCompanyRegions2400000000000'; + + public async up(queryRunner: QueryRunner): Promise { + // Canonical spellings — case/whitespace insensitive, safe to re-run. + await queryRunner.query(` + UPDATE freight.companies + SET region = v.canonical + FROM (VALUES + ('addis ababa', 'Addis Ababa'), + ('addis abeba', 'Addis Ababa'), + ('addisababa', 'Addis Ababa'), + ('oromia', 'Oromia'), + ('oromoia', 'Oromia'), + ('oromiya', 'Oromia'), + ('amhara', 'Amhara'), + ('somali', 'Somali'), + ('afar', 'Afar'), + ('tigray', 'Tigray'), + ('tigrai', 'Tigray'), + ('sidama', 'Sidama'), + ('harari', 'Harari'), + ('gambela', 'Gambela'), + ('gambella', 'Gambela'), + ('dire dawa', 'Dire Dawa'), + ('benishangul-gumuz', 'Benishangul-Gumuz'), + ('benishangul gumuz', 'Benishangul-Gumuz'), + ('central ethiopia', 'Central Ethiopia'), + ('south ethiopia', 'South Ethiopia') + ) AS v(variant, canonical) + WHERE freight.companies.region IS NOT NULL + AND lower(regexp_replace(btrim(freight.companies.region), '\\s+', ' ', 'g')) = v.variant + AND freight.companies.region <> v.canonical + `); + + // Anything still outside the canonical set is unresolvable — null it. + await queryRunner.query(` + UPDATE freight.companies + SET region = NULL + WHERE region IS NOT NULL + AND region <> '' + AND region NOT IN ( + 'Addis Ababa','Afar','Amhara','Benishangul-Gumuz','Central Ethiopia', + 'Dire Dawa','Gambela','Harari','Oromia','Sidama','Somali', + 'South Ethiopia','South West Ethiopia Peoples''','Tigray' + ) + `); + + // Normalize empty string to NULL so "unset" has one representation. + await queryRunner.query(` + UPDATE freight.companies SET region = NULL WHERE region = '' + `); + } + + public async down(): Promise { + // Irreversible by design: the original free-text values are not retained + // anywhere, so there is nothing to restore. Rolling back the code is safe — + // the column is still a nullable varchar(100) and accepts free text again. + } +} diff --git a/apps/edr-freight-api/src/modules/companies/dto/update-profile.dto.ts b/apps/edr-freight-api/src/modules/companies/dto/update-profile.dto.ts index a1b1ac9df..ba3e27aeb 100644 --- a/apps/edr-freight-api/src/modules/companies/dto/update-profile.dto.ts +++ b/apps/edr-freight-api/src/modules/companies/dto/update-profile.dto.ts @@ -1,4 +1,5 @@ -import { IsString, IsOptional, IsEmail, MaxLength, IsEnum } from 'class-validator'; +import { IsString, IsOptional, IsEmail, MaxLength, IsEnum, IsIn } from 'class-validator'; +import { ETHIOPIAN_REGIONS, type EthiopianRegion } from '@edr/types'; import { CompanyNationality } from '../entities/company.entity'; import { IsValidPhone } from '../../../common/validators/is-phone-number.validator'; import { IsTin } from '../../../common/validators/is-tin.validator'; @@ -138,10 +139,14 @@ export class UpdateProfileDto { @MaxLength(50) renewedTo?: string; + // Zone/woreda/kebele below stay free text: there is no authoritative dataset + // of Ethiopian zones/woredas/kebeles in the platform yet, and eTrade returns + // them uncoded. Only region is a closed set today. @IsOptional() - @IsString() - @MaxLength(100) - region?: string; + @IsIn(ETHIOPIAN_REGIONS as unknown as string[], { + message: "region must be a recognised Ethiopian region", + }) + region?: EthiopianRegion; @IsOptional() @IsString() diff --git a/apps/edr-freight-api/src/modules/companies/services/etrade.service.ts b/apps/edr-freight-api/src/modules/companies/services/etrade.service.ts index b588c241f..51bdb2df6 100644 --- a/apps/edr-freight-api/src/modules/companies/services/etrade.service.ts +++ b/apps/edr-freight-api/src/modules/companies/services/etrade.service.ts @@ -6,6 +6,7 @@ import { ETradeCompanyInfo, ETradeBusinessInfo, CompanyRegistrationData, + normalizeRegion, } from "@edr/types"; @Injectable() @@ -108,7 +109,11 @@ export class ETradeService { renewedFrom: businessInfo.RenewedFrom, renewalDate: businessInfo.RenewalDate, renewedTo: businessInfo.RenewedTo, - region: businessInfo.AddressInfo?.Region || "", + // eTrade returns uncoded uppercase text and sometimes a zone name in the + // Region slot. Map it onto the canonical list; an unresolved value yields + // "" so the form asks the user to pick rather than failing validation on + // save with a value they never typed. + region: normalizeRegion(businessInfo.AddressInfo?.Region) ?? "", zone: businessInfo.AddressInfo?.Zone || "", woreda: businessInfo.AddressInfo?.Woreda || "", kebele: businessInfo.AddressInfo?.Kebele || "", diff --git a/apps/edr-freight-api/src/modules/companies/services/region-normalization.spec.ts b/apps/edr-freight-api/src/modules/companies/services/region-normalization.spec.ts new file mode 100644 index 000000000..910b3199a --- /dev/null +++ b/apps/edr-freight-api/src/modules/companies/services/region-normalization.spec.ts @@ -0,0 +1,54 @@ +import { ETHIOPIAN_REGIONS, normalizeRegion } from '@edr/types'; + +/** + * normalizeRegion lives in @edr/types (no jest there), but it exists to keep + * eTrade autofill from feeding UpdateProfileDto a region its @IsIn will reject. + * That contract is an API concern, so it is guarded here. + */ +describe('normalizeRegion', () => { + it('passes through every canonical region unchanged', () => { + for (const region of ETHIOPIAN_REGIONS) { + expect(normalizeRegion(region)).toBe(region); + } + }); + + it.each([ + ['ADDIS ABABA', 'Addis Ababa'], + ['Addis ababa', 'Addis Ababa'], + [' addis ababa ', 'Addis Ababa'], + ['oromoia', 'Oromia'], + ['OROMIYA', 'Oromia'], + ['gambella', 'Gambela'], + ['TIGRAI', 'Tigray'], + ['benishangul gumuz', 'Benishangul-Gumuz'], + ])('resolves the variant %s', (input, expected) => { + expect(normalizeRegion(input)).toBe(expected); + }); + + it('maps a zone name in the region slot back to its parent region', () => { + // eTrade's own placeholder data does this — "EASTERN TIGRAY" is a zone. + expect(normalizeRegion('EASTERN TIGRAY')).toBe('Tigray'); + expect(normalizeRegion('North Wollo')).toBe('Amhara'); + }); + + it.each([ + ['a city, not a region', 'Arba Minch'], + ['unknown text', 'Nowhere Land'], + ['empty', ''], + ['whitespace only', ' '], + ['null', null], + ['undefined', undefined], + ])('returns null for %s rather than guessing', (_label, input) => { + expect(normalizeRegion(input as string | null | undefined)).toBeNull(); + }); + + it('never returns a value outside the canonical set', () => { + const samples = ['ADDIS ABABA', 'oromoia', 'EASTERN TIGRAY', 'garbage', '']; + for (const s of samples) { + const out = normalizeRegion(s); + if (out !== null) { + expect(ETHIOPIAN_REGIONS).toContain(out); + } + } + }); +}); diff --git a/apps/edr-freight-api/src/modules/health/health.controller.ts b/apps/edr-freight-api/src/modules/health/health.controller.ts new file mode 100644 index 000000000..6b559f2e3 --- /dev/null +++ b/apps/edr-freight-api/src/modules/health/health.controller.ts @@ -0,0 +1,108 @@ +// health.controller.ts + +import { Controller, Get, HttpStatus, Res } from "@nestjs/common"; +import { ApiOperation, ApiTags } from "@nestjs/swagger"; +import { InjectDataSource } from "@nestjs/typeorm"; +import { Public } from "@edr/api-common"; +import { Response } from "express"; +import { DataSource } from "typeorm"; + +import { EmailClientService } from "../notifications/email-client.service"; +import { SmsClientService } from "../notifications/sms-client.service"; + +type CheckStatus = "ok" | "error" | "unknown"; + +/** + * Readiness normally stays green when only the broker is down. + * + * A 503 pulls the pod out of the load balancer, which would take booking, + * tracking and billing offline because SMS is unreachable — a strictly worse + * outcome than degraded notifications. The broker check is therefore reported, + * not enforced, and `READINESS_REQUIRES_BROKER=true` opts into hard-failing for + * deployments where a silent OTP black hole is the greater risk. + */ +const READINESS_REQUIRES_BROKER = + process.env.READINESS_REQUIRES_BROKER === "true"; + +@ApiTags("Health") +@Controller("health") +export class HealthController { + constructor( + @InjectDataSource() + private readonly dataSource: DataSource, + private readonly smsClient: SmsClientService, + private readonly emailClient: EmailClientService, + ) {} + + @Get() + @Public() + @ApiOperation({ summary: "Liveness probe" }) + liveness() { + return { status: "ok", timestamp: new Date().toISOString() }; + } + + @Get("ready") + @Public() + @ApiOperation({ + summary: + "Readiness probe — database plus SMS/email broker connectivity. Broker failures report as degraded unless READINESS_REQUIRES_BROKER=true.", + }) + async readiness(@Res() res: Response) { + const startedAt = Date.now(); + + let database: { status: CheckStatus; latencyMs: number; error?: string }; + try { + await this.dataSource.query("SELECT 1"); + database = { status: "ok", latencyMs: Date.now() - startedAt }; + } catch (error) { + database = { + status: "error", + latencyMs: Date.now() - startedAt, + error: error instanceof Error ? error.message : "Unknown error", + }; + } + + // `null` from the client means the connection manager was not reachable + // through Nest's internals — surfaced as "unknown" so a shape change in + // @nestjs/microservices degrades to honest ignorance, not a false "ok". + const toStatus = (connected: boolean | null): CheckStatus => + connected === null ? "unknown" : connected ? "ok" : "error"; + + const broker = { + sms: { status: toStatus(this.smsClient.brokerConnected) }, + email: { status: toStatus(this.emailClient.brokerConnected) }, + // Every OTP, and every booking/billing notification, publishes through + // these. `error` here means codes are being generated and silently dropped. + enabled: process.env.RABBITMQ_ENABLED !== "false", + }; + + const brokerDown = + broker.sms.status === "error" || broker.email.status === "error"; + const failed = + database.status === "error" || + (READINESS_REQUIRES_BROKER && brokerDown); + + const status = failed ? "error" : brokerDown ? "degraded" : "ok"; + + return res + .status(failed ? HttpStatus.SERVICE_UNAVAILABLE : HttpStatus.OK) + .json({ + status, + timestamp: new Date().toISOString(), + checks: { database, broker }, + }); + } + + @Get("info") + @Public() + @ApiOperation({ summary: "App info — version, environment, uptime" }) + info() { + return { + name: "edr-freight-api", + version: process.env.npm_package_version ?? "1.0.0", + environment: process.env.NODE_ENV ?? "development", + uptimeSeconds: Math.floor(process.uptime()), + timestamp: new Date().toISOString(), + }; + } +} diff --git a/apps/edr-freight-api/src/modules/health/health.module.ts b/apps/edr-freight-api/src/modules/health/health.module.ts new file mode 100644 index 000000000..572e5eb86 --- /dev/null +++ b/apps/edr-freight-api/src/modules/health/health.module.ts @@ -0,0 +1,14 @@ +// health.module.ts + +import { Module } from "@nestjs/common"; + +import { HealthController } from "./health.controller"; +import { NotificationsModule } from "../notifications/notifications.module"; + +@Module({ + // NotificationsModule exports the SMS/email clients; the readiness probe reads + // their broker connection state rather than opening a second connection. + imports: [NotificationsModule], + controllers: [HealthController], +}) +export class HealthModule {} diff --git a/apps/edr-freight-api/src/modules/notifications/broker.util.spec.ts b/apps/edr-freight-api/src/modules/notifications/broker.util.spec.ts new file mode 100644 index 000000000..a6f73ac0a --- /dev/null +++ b/apps/edr-freight-api/src/modules/notifications/broker.util.spec.ts @@ -0,0 +1,90 @@ +import { Logger } from '@nestjs/common'; +import { ClientProxy } from '@nestjs/microservices'; +import { NEVER, Observable, throwError } from 'rxjs'; + +import { isBrokerConnected, publishConfirmed } from './broker.util'; + +/** + * `ClientProxy.emit()` returns a cold Observable that, for RMQ, completes without + * emitting once `dispatchEvent` settles — and rejects if the publish fails. These + * fakes reproduce each of those three shapes. + */ +function clientEmitting(source: Observable): ClientProxy { + return { emit: jest.fn().mockReturnValue(source) } as unknown as ClientProxy; +} + +describe('publishConfirmed', () => { + const logger = { error: jest.fn() } as unknown as Logger; + + beforeEach(() => jest.clearAllMocks()); + + it('is true when the publish completes (broker confirmed)', async () => { + // Completes with no value — the success shape, and the case that throws + // EmptyError without a defaultIfEmpty. + const client = clientEmitting(new Observable((s) => s.complete())); + await expect(publishConfirmed(client, 'send-sms', {}, logger)).resolves.toBe(true); + }); + + it('is false when the publish never settles, rather than hanging', async () => { + // A broker that is down: amqp-connection-manager buffers the publish and the + // promise would never resolve. The timeout is what stops one dead broker from + // hanging every caller of sendSms/sendEmail. + const client = clientEmitting(NEVER); + await expect(publishConfirmed(client, 'send-sms', {}, logger, 20)).resolves.toBe( + false, + ); + expect(logger.error).toHaveBeenCalled(); + }); + + it('is false when the publish errors', async () => { + const client = clientEmitting(throwError(() => new Error('channel closed'))); + await expect(publishConfirmed(client, 'send-email', {}, logger)).resolves.toBe( + false, + ); + expect(logger.error).toHaveBeenCalled(); + }); +}); + +describe('isBrokerConnected', () => { + /** Stands in for `ClientProxy.unwrap()`, which returns the AmqpConnectionManager. */ + function clientUnwrapping(manager: unknown): ClientProxy { + return { unwrap: () => manager } as unknown as ClientProxy; + } + + it('reports the connection manager state', () => { + expect(isBrokerConnected(clientUnwrapping({ isConnected: () => true }))).toBe( + true, + ); + expect(isBrokerConnected(clientUnwrapping({ isConnected: () => false }))).toBe( + false, + ); + }); + + it('is false when unwrap throws — the client never connected', () => { + // ClientRMQ.unwrap() throws "Not initialized" while its internal client is + // null, which is what a failed boot-time connect leaves behind. That is a + // real down signal and must not be softened to "unknown". + const uninitialised = { + unwrap: () => { + throw new Error('Not initialized. Please call the "connect" method first.'); + }, + } as unknown as ClientProxy; + expect(isBrokerConnected(uninitialised)).toBe(false); + }); + + it('is null — not a guess — when the manager lacks isConnected or it throws', () => { + // Guards the health endpoint against reporting "ok" if amqp-connection-manager + // or Nest changes shape and the accessor we rely on disappears. + expect(isBrokerConnected(clientUnwrapping(null))).toBeNull(); + expect(isBrokerConnected(clientUnwrapping({}))).toBeNull(); + expect( + isBrokerConnected( + clientUnwrapping({ + isConnected: () => { + throw new Error('boom'); + }, + }), + ), + ).toBeNull(); + }); +}); diff --git a/apps/edr-freight-api/src/modules/notifications/broker.util.ts b/apps/edr-freight-api/src/modules/notifications/broker.util.ts new file mode 100644 index 000000000..aa09d362e --- /dev/null +++ b/apps/edr-freight-api/src/modules/notifications/broker.util.ts @@ -0,0 +1,92 @@ +// broker.util.ts + +import { Logger } from "@nestjs/common"; +import { ClientProxy } from "@nestjs/microservices"; +import { defaultIfEmpty, lastValueFrom, timeout } from "rxjs"; + +/** + * How long to wait for a publisher confirm before giving up on a message. + * + * Load-bearing, not a nicety: when the broker is unreachable + * amqp-connection-manager buffers the publish and retries it on reconnect, so the + * underlying promise never settles. Without a bound, one dead broker turns every + * caller of sendSms/sendEmail into a hung request. + */ +export const PUBLISH_CONFIRM_TIMEOUT_MS = Number( + process.env.RABBITMQ_PUBLISH_TIMEOUT_MS ?? 5000, +); + +/** + * Publish an event and wait for RabbitMQ to confirm it. + * + * `ClientProxy.emit()` returns a *cold* Observable. Called without subscribing — + * as this codebase did everywhere — nothing forces the publish to be observed, so + * the caller reports success whether or not the broker ever accepted the message. + * Awaiting it drives `dispatchEvent`, which resolves only once + * amqp-connection-manager's ChannelWrapper has a publisher confirm. + * + * So `true` here means the broker took ownership of the message. It still says + * nothing about the consumer, the SMS gateway, or delivery to a handset — those + * remain outside this process's knowledge. + */ +export async function publishConfirmed( + client: ClientProxy, + pattern: string, + payload: unknown, + logger: Logger, + timeoutMs: number = PUBLISH_CONFIRM_TIMEOUT_MS, +): Promise { + try { + // `emit` completes without emitting a value, so lastValueFrom needs a default + // or it rejects with EmptyError on the success path. + await lastValueFrom( + client + .emit(pattern, payload) + .pipe(timeout(timeoutMs), defaultIfEmpty(undefined)), + ); + return true; + } catch (error) { + logger.error( + `broker.publish.failed pattern='${pattern}' timeoutMs=${timeoutMs}: ${ + error instanceof Error ? error.message : String(error) + }`, + error instanceof Error ? error.stack : undefined, + ); + return false; + } +} + +/** + * Whether the client's connection manager currently believes it is connected. + * + * Uses `ClientProxy.unwrap()` — Nest's public accessor for the underlying + * transport client, which for `ClientRMQ` is the `AmqpConnectionManager`. Calling + * `connect()` instead cannot answer this: it resolves against a *disconnected* + * manager too, so it never distinguishes up from down. + * + * Three outcomes, deliberately distinct: + * - `false` when the manager reports disconnected, or when `unwrap()` throws + * because the client was never initialised (a failed boot-time connect leaves + * it null — genuinely down, not unknown); + * - `null` when the manager exists but has no `isConnected`, i.e. the library + * shape changed under us — the health endpoint reports "unknown" rather than + * quietly claiming health; + * - `true` only on an explicit positive from the manager. + */ +export function isBrokerConnected(client: ClientProxy): boolean | null { + let manager: unknown; + try { + manager = client.unwrap(); + } catch { + // "Not initialized. Please call the connect method first." — no connection + // was ever established, which is a real down signal, not an unknown one. + return false; + } + const probe = manager as { isConnected?: () => boolean } | null; + if (!probe || typeof probe.isConnected !== "function") return null; + try { + return probe.isConnected(); + } catch { + return null; + } +} diff --git a/apps/edr-freight-api/src/modules/notifications/email-client.service.ts b/apps/edr-freight-api/src/modules/notifications/email-client.service.ts index 161b2486a..20a43d061 100644 --- a/apps/edr-freight-api/src/modules/notifications/email-client.service.ts +++ b/apps/edr-freight-api/src/modules/notifications/email-client.service.ts @@ -6,6 +6,7 @@ import { } from "@nestjs/common"; import { ClientProxy } from "@nestjs/microservices"; import { SendEmailDto } from "./dtos/email.dto"; +import { isBrokerConnected, publishConfirmed } from "./broker.util"; @Injectable() export class EmailClientService implements OnApplicationBootstrap { @@ -33,19 +34,34 @@ export class EmailClientService implements OnApplicationBootstrap { this.logger.warn(`RABBITMQ disabled — skipped EMAIL to=${dto.to}`); return { queued: false }; } - this.emailClient.emit("send-email", { - to: dto.to, - subject: dto.subject, - text: dto.text, - html: dto.html, - appKey: "IFHCRS-LICENSE-MANAGEMENT", - }); - // Fire-and-forget enqueue: confirms hand-off to RabbitMQ, NOT delivery. + const queued = await publishConfirmed( + this.emailClient, + "send-email", + { + to: dto.to, + subject: dto.subject, + text: dto.text, + html: dto.html, + appKey: "IFHCRS-LICENSE-MANAGEMENT", + }, + this.logger, + ); + // Publisher-confirmed: RabbitMQ has taken ownership of the message. Still NOT + // delivery — the consumer and the SMTP hop are downstream and invisible here. this.logger.log( - `EMAIL queued to RabbitMQ [${process.env.EMAIL_QUEUE ?? "email_queue"}] pattern='send-email'`, + `EMAIL publish to RabbitMQ [${process.env.EMAIL_QUEUE ?? "email_queue"}] pattern='send-email' confirmed=${queued}`, ); // Recipient + content are PII — debug only. this.logger.debug(`EMAIL payload to=${dto.to} subject="${dto.subject}"`); - return { queued: true }; + return { queued }; + } + + /** + * Connection state for the health endpoint. `null` means the broker client did + * not expose its manager — reported as "unknown" rather than assumed healthy. + */ + get brokerConnected(): boolean | null { + if (!this.enabled) return false; + return isBrokerConnected(this.emailClient); } } diff --git a/apps/edr-freight-api/src/modules/notifications/sms-client.service.ts b/apps/edr-freight-api/src/modules/notifications/sms-client.service.ts index f94c0c20e..43e47f350 100644 --- a/apps/edr-freight-api/src/modules/notifications/sms-client.service.ts +++ b/apps/edr-freight-api/src/modules/notifications/sms-client.service.ts @@ -6,6 +6,7 @@ import { } from "@nestjs/common"; import { ClientProxy } from "@nestjs/microservices"; import { BulkMessagesDto, SingleMessageDto } from "./dtos/sms.dto"; +import { isBrokerConnected, publishConfirmed } from "./broker.util"; @Injectable() export class SmsClientService implements OnApplicationBootstrap { @@ -14,7 +15,7 @@ export class SmsClientService implements OnApplicationBootstrap { constructor( @Inject("SMS_SERVICE") private smsClient: ClientProxy, - ) {} + ) { } private readonly enabled = process.env.RABBITMQ_ENABLED !== "false"; @@ -26,7 +27,7 @@ export class SmsClientService implements OnApplicationBootstrap { this.logger.log("connected to SMS service"); }) .catch((err) => { - console.error("Error happened at SMS service", err); + this.logger.error("Error happened at SMS service", err); }); } @@ -35,34 +36,61 @@ export class SmsClientService implements OnApplicationBootstrap { this.logger.warn(`RABBITMQ disabled — skipped SMS`); return { queued: false }; } - this.smsClient.emit("send-sms", { - to: dto.to, - text: dto.message, - appKey: "IFHCRS-LICENSE-MANAGEMENT", - }); - // Fire-and-forget enqueue: confirms hand-off to RabbitMQ, NOT delivery. + const queued = await publishConfirmed( + this.smsClient, + "send-sms", + { + to: dto.to, + text: dto.message, + appKey: "IFHCRS-LICENSE-MANAGEMENT", + }, + this.logger, + ); + // Publisher-confirmed: RabbitMQ has taken ownership of the message. Still NOT + // delivery — the consumer, the SMS gateway and the carrier are all downstream + // of this and invisible from here. this.logger.log( - `SMS queued to RabbitMQ [${process.env.SMS_QUEUE ?? "sms_queue"}] pattern='send-sms'`, + `SMS publish to RabbitMQ [${process.env.SMS_QUEUE ?? "sms_queue"}] pattern='send-sms' confirmed=${queued}`, ); // Recipient + content are PII — debug only. this.logger.debug(`SMS payload to=${dto.to} text="${dto.message}"`); - return { queued: true }; + return { queued }; } async sendBulkMessages(dto: BulkMessagesDto): Promise<{ queued: boolean }> { if (!this.enabled) { - this.logger.warn(`RABBITMQ disabled — skipped BULK SMS (${dto.messages?.length ?? 0} messages)`); + this.logger.warn( + `RABBITMQ disabled — skipped BULK SMS (${dto.messages?.length ?? 0} messages)`, + ); return { queued: false }; } - const messages = (dto.messages ?? []).map((m) => ({ to: m.to, text: m.message, from: m.from })); - this.smsClient.emit("ozeking-bulk-sms", { - messages, - appKey: "IFHCRS-LICENSE-MANAGEMENT", - }); + const messages = (dto.messages ?? []).map((m) => ({ + to: m.to, + text: m.message, + from: m.from, + })); + const queued = await publishConfirmed( + this.smsClient, + "ozeking-bulk-sms", + { + messages, + appKey: "IFHCRS-LICENSE-MANAGEMENT", + }, + this.logger, + ); this.logger.log( - `BULK SMS queued to RabbitMQ [${process.env.SMS_QUEUE ?? "sms_queue"}] pattern='ozeking-bulk-sms' count=${messages.length}`, + `BULK SMS publish to RabbitMQ [${process.env.SMS_QUEUE ?? "sms_queue"}] pattern='ozeking-bulk-sms' count=${messages.length} confirmed=${queued}`, ); this.logger.debug(`BULK SMS payload messages=${JSON.stringify(messages)}`); - return { queued: true }; + return { queued }; + } + + /** + * Connection state for the health endpoint. `null` means the broker client did + * not expose its manager — reported as "unknown" rather than assumed healthy. + */ + get brokerConnected(): boolean | null { + if (!this.enabled) return false; + return isBrokerConnected(this.smsClient); } } diff --git a/apps/edr-freight-api/src/modules/otp/otp.controller.ts b/apps/edr-freight-api/src/modules/otp/otp.controller.ts index 5d12f91f9..6b0078429 100644 --- a/apps/edr-freight-api/src/modules/otp/otp.controller.ts +++ b/apps/edr-freight-api/src/modules/otp/otp.controller.ts @@ -41,7 +41,13 @@ export class OtpController { @Body("email") email?: string ) { - return this.otpService.sendOtp(toTarget(phone, email)); + // `delivered` stays server-side: this route is @Public(), and whether our + // broker accepted the publish is infrastructure state an anonymous caller has + // no need for. It is on the `otp.dispatch` log line instead. + const { success, message } = await this.otpService.sendOtp( + toTarget(phone, email) + ); + return { success, message }; } // --------------------------------------------------------------------------- diff --git a/apps/edr-freight-api/src/modules/otp/otp.service.spec.ts b/apps/edr-freight-api/src/modules/otp/otp.service.spec.ts index 5f4afcfbd..c9d5c5714 100644 --- a/apps/edr-freight-api/src/modules/otp/otp.service.spec.ts +++ b/apps/edr-freight-api/src/modules/otp/otp.service.spec.ts @@ -11,8 +11,15 @@ describe('normalizeOtpTarget', () => { expect(normalizeOtpTarget({ phone: '0712345678' }).phone).toBe('+251712345678'); }); - it('passes email targets through untouched', () => { - expect(normalizeOtpTarget({ email: 'a@b.com' })).toEqual({ email: 'a@b.com' }); + it('canonicalises email case and surrounding whitespace to one key', () => { + const forms = ['a@b.com', 'A@B.com', ' a@B.COM ', 'A@b.COM']; + const keys = forms.map((email) => normalizeOtpTarget({ email }).email); + expect(new Set(keys)).toEqual(new Set(['a@b.com'])); + }); + + it('keeps an already-normalised email stable (idempotent)', () => { + const once = normalizeOtpTarget({ email: ' User@Example.COM ' }).email!; + expect(normalizeOtpTarget({ email: once }).email).toBe(once); }); it('keeps an already-normalised number stable (idempotent)', () => { @@ -40,8 +47,10 @@ describe('OtpService — send/verify agree across phone formats', () => { rows.delete(row.phone ?? row.email!); }), }; - const sms = { sendSms: jest.fn().mockResolvedValue(undefined) }; - const email = { sendEmail: jest.fn().mockResolvedValue(undefined) }; + // Both clients return `{ queued }` — the service reads it to tell a published + // code apart from one the transport silently dropped. + const sms = { sendSms: jest.fn().mockResolvedValue({ queued: true }) }; + const email = { sendEmail: jest.fn().mockResolvedValue({ queued: true }) }; const service = new OtpService(repo as never, sms as never, email as never); return { service, rows }; } @@ -58,4 +67,16 @@ describe('OtpService — send/verify agree across phone formats', () => { service.verifyOtpForAction({ phone: '0986680099' }, stored), ).resolves.toEqual({ success: true }); }); + + it('verifies a code sent to User@X.com when verify is called with user@x.com', async () => { + const { service, rows } = makeService(); + await service.sendOtp({ email: ' User@Example.COM ' }); + const stored = [...rows.values()][0]!.otp; + + [...rows.values()][0]!.updatedAt = new Date(); + + await expect( + service.verifyOtpForAction({ email: 'user@example.com' }, stored), + ).resolves.toEqual({ success: true }); + }); }); diff --git a/apps/edr-freight-api/src/modules/otp/otp.service.ts b/apps/edr-freight-api/src/modules/otp/otp.service.ts index 06d837065..5e91251c9 100644 --- a/apps/edr-freight-api/src/modules/otp/otp.service.ts +++ b/apps/edr-freight-api/src/modules/otp/otp.service.ts @@ -22,7 +22,18 @@ export type OtpTarget = { phone?: string; email?: string }; * Email targets pass through untouched. */ export function normalizeOtpTarget(target: OtpTarget): OtpTarget { - if (target.email || !target.phone) return target; + if (target.email) { + // Same contract as the phone branch below: the string stored on send and the + // one looked up on verify must be byte-identical, or the code is invisible to + // the verifier. Addresses reach us from a raw `@Body("email")` with no DTO or + // ValidationPipe, so `User@X.com`, `user@x.com` and a copy-paste with a + // trailing space are three different keys for one mailbox. Domains are + // case-insensitive (RFC 1035); local-parts are formally case-sensitive + // (RFC 5321 §2.4) but no mail provider in practice treats them so, and + // matching what users expect beats matching the letter of the spec here. + return { email: target.email.trim().toLowerCase() }; + } + if (!target.phone) return target; const raw = target.phone.trim(); const digits = raw.replace(/[^\d+]/g, ''); if (digits.startsWith('+')) return { phone: digits }; @@ -61,6 +72,9 @@ export class OtpService { // Store under the canonical E.164 key so verify (which normalises the same // way) always finds this row regardless of how either side typed the number. const target = normalizeOtpTarget(rawTarget); + const channel = target.email ? "email" : "sms"; + const label = this.targetLabel(target); + const startedAt = Date.now(); try { // The verification code is generated server-side — never supplied by the // caller — so the OTP stays a secret known only to the server and the @@ -78,6 +92,14 @@ export class OtpService { await this.otpRepository.createOtp(target, otp); } + // `rotate` means a code already existed for this target and was replaced — + // the previous one is now dead. A user holding a slow-to-arrive SMS and + // typing its code will fail against the row; this line is how that shows up + // in the log rather than as an unexplained "invalid OTP" report. + this.logger.log( + `otp.issue channel=${channel} target=${label} action=${existing ? "rotate" : "create"}`, + ); + // NOTE: do NOT reset the brute-force attempt counter on send. Clearing it // here let an attacker wipe the per-target guess budget just by calling // /otp/send between guesses. The counter is cleared only when the code is @@ -86,40 +108,97 @@ export class OtpService { // /otp/verify routes (a NestJS ThrottlerGuard / @Throttle) — none exists // in the codebase yet. - if (target.email) { - // send email (queued to RabbitMQ via the shared Email service) - await this.emailClient.sendEmail({ - to: target.email, - subject: "Your EDR Freight verification code", - text: `Your verification code is ${otp}`, - }); - } else { - // send sms (queued to RabbitMQ via the shared SMS service) - await this.smsClient.sendSms({ - to: target.phone as string, - message: `Your verification code is ${otp}`, - }); + // Both clients report hand-off, not delivery — capture it rather than + // discarding it, so "queued=false" is distinguishable from a code that was + // published fine and lost downstream at the carrier. + const { queued } = target.email + ? await this.emailClient.sendEmail({ + to: target.email, + subject: "Your EDR Freight verification code", + text: `Your verification code is ${otp}`, + }) + : await this.smsClient.sendSms({ + to: target.phone as string, + message: `Your verification code is ${otp}`, + }); + + this.logger.log( + `otp.dispatch channel=${channel} target=${label} queued=${queued} latencyMs=${ + Date.now() - startedAt + }`, + ); + + if (!queued) { + // The row is committed and we are about to answer "OTP sent successfully", + // but nothing left this process. Without this line the only symptom is a + // user who never receives a code — indistinguishable from carrier loss, + // and the misleading success response makes it look like our side worked. + this.logger.error( + `otp.dispatch.dropped channel=${channel} target=${label} rabbitmqEnabled=${ + process.env.RABBITMQ_ENABLED ?? "unset" + } — transport reported no hand-off; no code will arrive for this send`, + ); } + // SECURITY: this logs a live credential in cleartext. Anyone with read + // access to the log stream can complete a password reset or a contract + // signature for the address on the same line. Kept deliberately (log + // aggregation is the debugging path for flaky SMS here) — if that tradeoff + // is ever revisited, gate on an env flag rather than deleting the line, so + // dev keeps its workflow. this.logger.log(`OTP send for ${target.email ?? target.phone}: ${otp}`); return { success: true, + // Distinguishes "we published it" from "the transport is a no-op". The + // HTTP response shape is unchanged; the controller drops this field. + delivered: queued, + message: "OTP sent successfully", }; } catch (error) { // Log the real cause (DB/SMS/email failure) with its stack so a deployed // "Failed to send OTP" 400 is diagnosable from the API logs, not opaque. this.logger.error( - `Failed to send OTP to ${target.email ?? target.phone}: ${ - error instanceof Error ? error.message : String(error) - }`, + `otp.dispatch.failed channel=${channel} target=${label} latencyMs=${ + Date.now() - startedAt + }: ${error instanceof Error ? error.message : String(error)}`, error instanceof Error ? error.stack : undefined, ); throw new BadRequestException("Failed to send OTP"); } } + /** + * Correlation key shared by every `otp.*` line for one address, so a send and + * its later verify can be joined with a single grep. The raw target is used + * because the code itself is already logged in cleartext above — hashing the + * address while printing the credential next to it would buy nothing. + */ + private targetLabel(target: OtpTarget): string { + return target.email ?? target.phone ?? "unknown"; + } + + /** + * One line per verify exit path. `result` is a closed set — ok | invalid | + * expired | exhausted | not_found — so failures can be counted by reason + * instead of inferred from error strings that the frontend also depends on. + */ + private logVerify( + target: OtpTarget, + mode: "simple" | "action", + result: "ok" | "invalid" | "expired" | "exhausted" | "not_found", + detail?: string, + ) { + const line = `otp.verify channel=${ + target.email ? "email" : "sms" + } target=${this.targetLabel(target)} mode=${mode} result=${result}${ + detail ? ` ${detail}` : "" + }`; + if (result === "ok") this.logger.log(line); + else this.logger.warn(line); + } + // --------------------------------------------------------------------------- // Verify OTP // --------------------------------------------------------------------------- @@ -134,6 +213,9 @@ export class OtpService { // not found if (!otpData) { + // No row for this key. Most often a normalisation mismatch or a code that + // was already consumed/burned — not necessarily a caller who never asked. + this.logVerify(target, "simple", "not_found"); throw new BadRequestException( target.email ? "Email address not found" : "Phone number not found", ); @@ -145,6 +227,12 @@ export class OtpService { if (ageMs > this.ACTION_OTP_TTL_MS) { await this.otpRepository.deleteOtp(otpData); this.actionAttempts.delete(key); + this.logVerify( + target, + "simple", + "expired", + `ageMs=${ageMs} ttlMs=${this.ACTION_OTP_TTL_MS}`, + ); throw new BadRequestException( "Verification code has expired. Request a new one.", ); @@ -157,17 +245,30 @@ export class OtpService { if (attempts >= this.MAX_ACTION_ATTEMPTS) { await this.otpRepository.deleteOtp(otpData); this.actionAttempts.delete(key); + this.logVerify( + target, + "simple", + "exhausted", + `attempts=${attempts}/${this.MAX_ACTION_ATTEMPTS} ageMs=${ageMs}`, + ); throw new BadRequestException( "Too many incorrect attempts. Request a new code.", ); } this.actionAttempts.set(key, attempts); + this.logVerify( + target, + "simple", + "invalid", + `attempts=${attempts}/${this.MAX_ACTION_ATTEMPTS} ageMs=${ageMs}`, + ); throw new BadRequestException("Invalid OTP"); } // single-use: consume the code on success so it can't be replayed. await this.otpRepository.deleteOtp(otpData); this.actionAttempts.delete(key); + this.logVerify(target, "simple", "ok", `ageMs=${ageMs}`); return { success: true, @@ -210,6 +311,7 @@ export class OtpService { const key = this.targetKey(target); if (!otpData) { + this.logVerify(target, "action", "not_found"); throw new BadRequestException( target.email ? "No verification code was requested for this email" @@ -223,6 +325,7 @@ export class OtpService { await this.otpRepository.deleteOtp(otpData); this.actionAttempts.delete(key); + this.logVerify(target, "action", "expired", `ageMs=${ageMs} ttlMs=${ttlMs}`); throw new BadRequestException( "Verification code has expired. Request a new one.", ); @@ -235,18 +338,31 @@ export class OtpService { await this.otpRepository.deleteOtp(otpData); this.actionAttempts.delete(key); + this.logVerify( + target, + "action", + "exhausted", + `attempts=${attempts}/${this.MAX_ACTION_ATTEMPTS} ageMs=${ageMs}`, + ); throw new BadRequestException( "Too many incorrect attempts. Request a new code.", ); } this.actionAttempts.set(key, attempts); + this.logVerify( + target, + "action", + "invalid", + `attempts=${attempts}/${this.MAX_ACTION_ATTEMPTS} ageMs=${ageMs}`, + ); throw new BadRequestException("Invalid verification code"); } // single-use: consume on success await this.otpRepository.deleteOtp(otpData); this.actionAttempts.delete(key); + this.logVerify(target, "action", "ok", `ageMs=${ageMs}`); return { success: true }; } diff --git a/apps/edr-freight-web/portal/src/pages/accounts/CompanyProfileForm.tsx b/apps/edr-freight-web/portal/src/pages/accounts/CompanyProfileForm.tsx index 27e51b403..47c00bef2 100644 --- a/apps/edr-freight-web/portal/src/pages/accounts/CompanyProfileForm.tsx +++ b/apps/edr-freight-web/portal/src/pages/accounts/CompanyProfileForm.tsx @@ -4,6 +4,7 @@ import { Divider, Group, Loader, + Select, SimpleGrid, Stack, Text, @@ -13,12 +14,13 @@ import { zodResolver } from "@hookform/resolvers/zod"; import { useQuery } from "@tanstack/react-query"; import { AlertCircle, ArrowLeft, ArrowRight } from "lucide-react"; import { useEffect, useMemo, useRef, useState } from "react"; -import { useForm } from "react-hook-form"; +import { Controller, useForm } from "react-hook-form"; import type { AuthUser } from "@/types/auth"; import type { CreateCompanyPayload } from "@/services/companies.service"; import type { ProfileResponse, UpdateProfilePayload } from "@/types/profile"; import type { CompanyRegistrationData } from "@edr/types"; +import { ETHIOPIAN_REGIONS } from "@edr/types"; import { ControlledPhoneField, toEthiopianE164 } from "@/components/PhoneField"; import { SmartFileInput } from "@edr/ui-common"; import { getMinFiles } from "@/types/fileUploadSettings"; @@ -675,12 +677,24 @@ export default function CompanyProfileForm({ Address Information - ( +