diff --git a/apps/api/src/modules/customer-auth/customer-auth.service.ts b/apps/api/src/modules/customer-auth/customer-auth.service.ts index 6b5dc7e..40da5b2 100644 --- a/apps/api/src/modules/customer-auth/customer-auth.service.ts +++ b/apps/api/src/modules/customer-auth/customer-auth.service.ts @@ -1,5 +1,13 @@ import { createHash, createHmac, randomBytes } from 'node:crypto'; -import { HttpException, HttpStatus, Injectable, Logger, UnauthorizedException } from '@nestjs/common'; +import { + ForbiddenException, + HttpException, + HttpStatus, + Injectable, + Logger, + NotFoundException, + UnauthorizedException, +} from '@nestjs/common'; import { JwtService } from '@nestjs/jwt'; import { AdminRole, CustomerKind, MessageAudience, Prisma } from '@prisma/client'; import * as bcrypt from 'bcryptjs'; @@ -16,7 +24,7 @@ import { envList, envOr, envOrNull, pickExactUri } from '../../common/env'; import { masterCodeAllowed } from '../../common/production-checks'; import { CUSTOMER_TOKEN_TYPE, EXPERT_TOKEN_TYPE, type CustomerJwtPayload } from './customer-jwt.strategy'; import { faToEn } from './digits'; -import { SmsService, smsConfigured } from '../sms/sms.service'; +import { SmsService } from '../sms/sms.service'; /** Nest 10 has no TooManyRequestsException; 429 with a Persian message. */ class TooManyRequestsException extends HttpException { @@ -126,8 +134,8 @@ export class CustomerAuthService { * Reference: validate `username`, find an ACTIVE customer by mobile, generate * rand(1000,9999), store it, send it by SMS, return `true`. * - * Kept: the active-customer filter, the 4-digit range, the opaque `true` - * response (it never reveals whether the number exists). + * Kept: the active-customer filter, the 4-digit range, and the 404 for a + * number that is not registered (reference `firstOrFail()`). * Changed: the code is hashed, it expires, and sends are rate limited. */ async requestOtp(rawMobile: string, ip: string | undefined) { @@ -154,12 +162,23 @@ export class CustomerAuthService { select: { id: true }, })); - // Deliberately opaque: the reference used firstOrFail() and leaked a 404 - // for unknown numbers, which let anyone enumerate registered customers. - // Recorded as a deviation in docs/parity/DECISIONS.md. + // Owner ruling 2026-10-12: say so. This used to return `{ sent: true }` for + // a number nobody holds, to stop an outsider enumerating customers. The + // cost landed on real users and on support: the app moved to the code box + // and then nothing arrived, and there was no way to tell apart "the SMS + // gateway is down", "the code is late" and "this number was never + // registered" — the one case the person could actually act on. + // + // This is also what the reference did (`firstOrFail()`, + // ApiMobileController::userAuth:43 → 404), so the opaque response was the + // deviation, not this. + // + // Enumeration is still bounded by the rate limits above — assertSendAllowed + // caps a single IP at MAX_SENDS_PER_IP per window, which it did not do in + // the reference. if (!customer) { this.logger.warn(`OTP requested for unknown or inactive mobile ${mobile}`); - return { sent: true }; + throw new NotFoundException('این شماره در سیستم ثبت نشده است. لطفاً با پشتیبانی تماس بگیرید.'); } const code = String(Math.floor(1000 + Math.random() * 9000)); @@ -231,17 +250,22 @@ export class CustomerAuthService { if (existing && existing.isActive && !existing.isArchived) return { id: existing.id }; // Deliberately does NOT silently create a second account when the first // was deactivated: an operator disabled it, and that decision stands. + // + // Told apart from "no such number" on purpose: this expert DOES exist, so + // the «ثبت نشده است» message would be false and would send support + // looking for a registration problem that is not there. this.logger.warn( `Sales expert ${admin.id} has a trading account that is inactive or archived; refusing to create another.`, ); - return null; + throw new ForbiddenException('حساب شما غیرفعال است. لطفاً با پشتیبانی تماس بگیرید.'); } if (!admin.salesExpertProfile) { // Every SALES_EXPERT gets a profile on create and on role change (D17), - // so this means something bypassed both paths. - this.logger.warn(`Sales expert ${admin.id} has no SalesExpertProfile; cannot sign in to the app.`); - return null; + // so this means something bypassed both paths. A server-side defect, not + // a wrong number — so not «ثبت نشده است» either. + this.logger.error(`Sales expert ${admin.id} has no SalesExpertProfile; cannot sign in to the app.`); + throw new ForbiddenException('حساب شما کامل نیست. لطفاً با پشتیبانی تماس بگیرید.'); } // The mobile is unique on Customer, so a pre-existing customer row with @@ -313,6 +337,11 @@ export class CustomerAuthService { // One message for every failure mode, so a caller cannot tell "no code was // sent" from "wrong code" from "expired". Declared before the master-code // branch so that path can reject identically. + // Deliberately one message for every reason a code can fail: wrong code, no + // challenge, expired, unknown number. Unlike requestOtp — which now names an + // unregistered number, owner ruling 2026-10-12 — there is nothing for a real + // user to act on differently here, and step 1 already told them if the + // number was not on file, so they never reach this with an unknown number. const reject = () => new UnauthorizedException('کد وارد شده صحیح نیست'); // D20 — the delivery master code. Checked BEFORE the stored challenge so it @@ -742,9 +771,11 @@ export class CustomerAuthService { * When no gateway is configured, SmsService does not send and the code is * logged instead — outside production only (D20). * - * A failed send does NOT throw. The reference ignored Sms::Send's result, so - * a gateway outage never blocked the flow that triggered it; the customer - * simply retries, and the throttle in assertSendAllowed still applies. + * A failed send DOES throw 503 (owner ruling 2026-10-12). The reference + * ignored Sms::Send's result and so did this, which meant a gateway outage + * returned success and the app sent the person to the code box to wait for a + * message that had never been sent. See the note at the throw: D20 protects + * operations from a dead gateway, but here the SMS is the operation. */ private async deliver(mobile: string, code: string) { // The reference's text (ApiMobileController.php:47) with the brand changed @@ -759,14 +790,45 @@ export class CustomerAuthService { // persist:false — the real text carries the live code, and the admin SMS // grid lists and searches `body` (SEC-11). The redacted audit row below // takes its place. - await this.sms.send(mobile, smsText, { persist: false }); + const result = await this.sms.send(mobile, smsText, { persist: false }); // Only outside production, and only when no gateway is configured, so a // deployment can never print live codes into its logs. - if (process.env.NODE_ENV !== 'production' && !smsConfigured()) { + // + // The condition is SmsService's own verdict (`sms-not-configured`), not a + // second guess at the config: with D26's default `relay` driver, + // smsConfigured() is true on any machine that has an external IPv4, so + // testing the config here would have called a dev box "configured" and then + // failed the login below when the relay was unreachable. This is the one + // case where a failed send must not be an error — there was no gateway to + // fail, and local sign-in works off this log line. + const devNoGateway = + process.env.NODE_ENV !== 'production' && !result.ok && result.error === 'sms-not-configured'; + if (devNoGateway) { this.logger.log(`[dev] OTP for ${mobile}: ${code}`); } + // Owner ruling 2026-10-12, same reasoning as the 404 in requestOtp. + // + // SmsService NEVER throws — a dead gateway is a log line (D20) — so a failed + // send used to return success here and the app walked the person to the code + // box to wait for a message that was never sent. From the outside that is + // indistinguishable from a late SMS, which is exactly the confusion being + // removed. + // + // D20 is about not letting a failed SMS break an *operation*: an order must + // still be placed. In a login the SMS IS the operation, so staying quiet + // protects nothing. The code row stays in the table — it costs nothing and + // expires on its own — and the person is told to retry instead of being left + // watching a countdown. + if (!result.ok && !devNoGateway) { + this.logger.error(`OTP SMS to ${mobile} failed: ${result.error ?? 'unknown'}`); + throw new HttpException( + 'ارسال پیام کوتاه در حال حاضر ممکن نیست. لطفاً چند لحظه بعد دوباره تلاش کنید.', + HttpStatus.SERVICE_UNAVAILABLE, + ); + } + // An audit row, NOT the message body. // // SmsMessage.body is listed and full-text searchable on the admin panel's diff --git a/apps/api/src/modules/customer-auth/request-otp.spec.ts b/apps/api/src/modules/customer-auth/request-otp.spec.ts new file mode 100644 index 0000000..d8bd1a8 --- /dev/null +++ b/apps/api/src/modules/customer-auth/request-otp.spec.ts @@ -0,0 +1,165 @@ +import { ForbiddenException, HttpException, NotFoundException } from '@nestjs/common'; +import { AdminRole } from '@prisma/client'; + +// @nestjs/jwt v12 ships ESM only, which this CommonJS jest setup cannot load. +// Same stub as master-otp.spec.ts — the service gets a hand-built double. +jest.mock('@nestjs/jwt', () => ({ JwtService: class {} })); + +import { CustomerAuthService } from './customer-auth.service'; + +/** + * Owner ruling 2026-10-12 — step 1 of the app login must say what went wrong. + * + * Until now an unknown number got `{ sent: true }` (opaque on purpose, to stop + * customer enumeration) and a failed SMS send also got `{ sent: true }` (a dead + * gateway is only a log line, D20). Both walked the person to the code box to + * wait for a message that was never going to arrive, and nothing on screen told + * the three real causes apart: the SMS is late, the gateway is down, or the + * number was never registered. + * + * `requestOtp` had NO test coverage at all before this file, so the whole suite + * stayed green through that change — these cases exist so it cannot again. + */ +describe('CustomerAuthService.requestOtp — the caller is told what happened', () => { + const saved = { ...process.env }; + + const CUSTOMER = { id: 'cus_1', mobile: '09121112233', isActive: true, isArchived: false }; + + function build(opts: { + customer?: unknown; + expert?: unknown; + smsResult?: { ok: boolean; error?: string }; + } = {}) { + const prisma = { + customer: { + findFirst: jest.fn().mockResolvedValue('customer' in opts ? opts.customer : CUSTOMER), + findUnique: jest.fn().mockResolvedValue(null), + }, + customerOtp: { + findFirst: jest.fn().mockResolvedValue(null), + create: jest.fn().mockResolvedValue({}), + count: jest.fn().mockResolvedValue(0), + }, + smsMessage: { create: jest.fn().mockResolvedValue({}) }, + adminUser: { findFirst: jest.fn().mockResolvedValue(opts.expert ?? null) }, + }; + const sms = { send: jest.fn().mockResolvedValue(opts.smsResult ?? { ok: true }) }; + const service = new CustomerAuthService( + prisma as never, + { signAsync: jest.fn() } as never, + sms as never, + { enabled: false, attestOtpLogin: jest.fn(), browserBootstrapUrl: () => null } as never, + { newTokenId: jest.fn().mockReturnValue(null), record: jest.fn() } as never, + ); + jest.spyOn(service['logger'], 'warn').mockImplementation(() => undefined); + jest.spyOn(service['logger'], 'error').mockImplementation(() => undefined); + return { service, prisma, sms }; + } + + beforeEach(() => { + process.env.NODE_ENV = 'test'; + }); + + afterEach(() => { + process.env = { ...saved }; + }); + + it('sends the code for a registered, active customer', async () => { + const { service, sms, prisma } = build(); + await expect(service.requestOtp('09121112233', '127.0.0.1')).resolves.toEqual({ sent: true }); + expect(sms.send).toHaveBeenCalledTimes(1); + expect(prisma.customerOtp.create).toHaveBeenCalledTimes(1); + }); + + it('refuses an unregistered number with «ثبت نشده»، not a silent success', async () => { + const { service, sms } = build({ customer: null }); + await expect(service.requestOtp('09129999999', '127.0.0.1')).rejects.toThrow(NotFoundException); + await expect(service.requestOtp('09129999999', '127.0.0.1')).rejects.toThrow('ثبت نشده'); + // Nothing was sent and no challenge was stored for a number nobody holds. + expect(sms.send).not.toHaveBeenCalled(); + }); + + it('normalises Persian digits before deciding the number is unknown', async () => { + // Guards against a false «ثبت نشده» for someone typing on a Persian keypad. + const { service, prisma } = build(); + await service.requestOtp('۰۹۱۲۱۱۱۲۲۳۳', '127.0.0.1'); + expect(prisma.customer.findFirst).toHaveBeenCalledWith( + expect.objectContaining({ where: expect.objectContaining({ mobile: '09121112233' }) }), + ); + }); + + it('reports a failed SMS send as 503 instead of claiming it was sent', async () => { + const { service } = build({ smsResult: { ok: false, error: 'gateway refused' } }); + await expect(service.requestOtp('09121112233', '127.0.0.1')).rejects.toThrow(HttpException); + await expect(service.requestOtp('09121112233', '127.0.0.1')).rejects.toThrow('ارسال پیام کوتاه'); + }); + + it('still completes the login in development when there is no gateway at all', async () => { + // The local-dev path: SmsService sends nothing, returns its own + // `sms-not-configured`, and the code is logged instead — so this one failure + // must NOT become an error or nobody could sign in locally. + process.env.NODE_ENV = 'development'; + const { service } = build({ smsResult: { ok: false, error: 'sms-not-configured' } }); + await expect(service.requestOtp('09121112233', '127.0.0.1')).resolves.toEqual({ sent: true }); + }); + + it('still fails in development when a configured gateway refuses', async () => { + // D26's default `relay` driver counts as configured on any machine with an + // external IPv4, so a dev box that cannot reach the relay gets a real + // failure — which must be reported, not swallowed as "no gateway". + process.env.NODE_ENV = 'development'; + const { service } = build({ smsResult: { ok: false, error: 'relay unreachable' } }); + await expect(service.requestOtp('09121112233', '127.0.0.1')).rejects.toThrow('ارسال پیام کوتاه'); + }); + + it('refuses in production even when nothing is configured', async () => { + // The dev fallback must never let a production deployment report success + // while silently sending nothing. + process.env.NODE_ENV = 'production'; + const { service } = build({ smsResult: { ok: false, error: 'sms-not-configured' } }); + await expect(service.requestOtp('09121112233', '127.0.0.1')).rejects.toThrow('ارسال پیام کوتاه'); + }); + + /** + * A sales expert signs in through this same flow (D21). Three states return + * no trading account for an expert who DOES exist, and «ثبت نشده است» would + * be a lie in each — it would send support hunting a registration problem. + */ + describe('a sales expert whose trading account cannot be used', () => { + it('says the account is disabled, not that the number is unknown', async () => { + const { service } = build({ + customer: null, + expert: { + id: 'adm_1', + mobile: '09121112233', + role: AdminRole.SALES_EXPERT, + fullName: 'کارشناس', + firstName: 'کار', + lastName: 'شناس', + wagePercent: '0', + salesExpertProfile: { id: 'sep_1', gramLevel: '0', nationalCode: null, tradingCustomerId: 'cus_dead' }, + }, + }); + // findUnique returns null → the linked customer is gone or unusable. + await expect(service.requestOtp('09121112233', '127.0.0.1')).rejects.toThrow(ForbiddenException); + await expect(service.requestOtp('09121112233', '127.0.0.1')).rejects.toThrow('غیرفعال'); + }); + + it('says the account is incomplete when the expert has no profile', async () => { + const { service } = build({ + customer: null, + expert: { + id: 'adm_2', + mobile: '09121112233', + role: AdminRole.SALES_EXPERT, + fullName: 'کارشناس', + firstName: 'کار', + lastName: 'شناس', + wagePercent: '0', + salesExpertProfile: null, + }, + }); + await expect(service.requestOtp('09121112233', '127.0.0.1')).rejects.toThrow('کامل نیست'); + }); + }); +}); diff --git a/apps/user-app-v2/src/views/user/LoginView.vue b/apps/user-app-v2/src/views/user/LoginView.vue index 62281c6..5f39912 100644 --- a/apps/user-app-v2/src/views/user/LoginView.vue +++ b/apps/user-app-v2/src/views/user/LoginView.vue @@ -3,18 +3,18 @@ restyled into the redesigned design language (see components/home/* and HomeView). - Per the earlier decision: OTP-based flow ported structurally, but the - SMS-send step is stubbed — no SMS provider exists. What's real vs stubbed, - all carried over unchanged from the previous version: - - Step 1 (mobile number) is checked against the REAL, already-existing - GET /api/customers endpoint (client-side match on `mobile`) — no fake - endpoint invented. A non-match shows legacy's own - "اطلاعات کاربری در سیستم وجود ندارد" text, not a new message. - - Step 2 (OTP code) is NOT verified — any 4-digit code is accepted, since - there is no real code to check against. On "verify", the session store - logs in as the matched real customer, so downstream features (balance, - ledger, favourites-as-logged-in) work against real seeded data. - - `token.fireBaseToken` doesn't exist in this port (Firebase deferred). + The flow is REAL. These notes used to say it was stubbed, and that stale text + misled a reader as recently as 2026-10-12, so it is corrected rather than + deleted: + - Step 1 posts the number to the server, which looks it up itself. It no + longer pulls the customer list into the client to match on `mobile` — that + was the login leak (D8). An unregistered number is answered with 404 and + the server's Persian message is shown; owner ruling 2026-10-12, because + the earlier opaque answer walked the person to the code box to wait for an + SMS that was never going to arrive. + - Step 2 IS verified by the server: hashed code, two-minute expiry, attempt + limit (D6). Accepting any 4-digit code is long gone. + - `token.fireBaseToken` still does not exist in this port (PUSH-01). - `useOtpToken()` (WebOTP autofill) was already dead in legacy itself. -->