From 96f3d301ea7f698aeb24e1cf6036110f844db15b Mon Sep 17 00:00:00 2001 From: Dread Date: Tue, 1 Sep 2026 15:15:50 -0700 Subject: [PATCH 1/5] feat(auth): pre-persist registration webhook so rejected signups never leave orphaned Kratos identities The after-registration web_hook runs with `response.parse: false`, which in Kratos v1.0.0 is a post-persist hook: the identity is committed before the api is called, so every rejection inside createAccountWithPhoneIdentifier (DuplicateKeyForPersistError, IbexError, InvalidCarrierTypeForPhoneMetadataError) or a pod dying mid-hook leaves a logged-in identity with no account. Those orphans crash-looped every api replica on 2026-09-01. Add a validation-only pre-persist hook, POST /kratos/preregistration, for a web_hook configured `response.parse: true`: Kratos then calls it before persisting, and a 4xx with the `messages` body aborts the sign-up with nothing written. It checks the callback secret, schema, phone, carrier metadata and that no users document already binds the phone; it never writes and never inspects identity_id (the nil uuid at that point). The existing post-persist /registration hook still creates the account, so a failed Kratos persist can never strand a Mongo account either. When Kratos hands the rejected flow back as a 400, map our message ids to PhoneAlreadyExistsError / PhoneNotAllowedForRegistrationError instead of the blanket LikelyUserAlreadyExistError; unrecognised 400s keep that reading. Deploy the api before the charts/deployments hook config, and TEST first. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01EtQzyzQ38thfC2F8BLK4u8 --- dev/ory/kratos.yml | 43 +++-- src/app/authentication/index.ts | 1 + .../validate-preregistration-payload.ts | 56 ++++++ src/domain/authentication/errors.ts | 4 + src/domain/authentication/index.types.d.ts | 22 ++- .../authentication/kratos-hook-messages.ts | 77 ++++++++ .../registration-payload-validator.ts | 79 ++++++-- src/graphql/error-map.ts | 7 + src/servers/event-handlers/kratos.ts | 85 ++++++++- src/services/kratos/auth-phone-no-password.ts | 11 +- .../kratos/registration-flow-error.ts | 42 +++++ .../validate-preregistration-payload.spec.ts | 134 +++++++++++++ .../kratos-hook-messages.spec.ts | 71 +++++++ .../preregistration-payload-validator.spec.ts | 170 +++++++++++++++++ test/flash/unit/graphql/error-map.spec.ts | 8 + .../kratos-preregistration-route.spec.ts | 178 ++++++++++++++++++ .../kratos/registration-flow-error.spec.ts | 104 ++++++++++ 17 files changed, 1052 insertions(+), 40 deletions(-) create mode 100644 src/app/authentication/validate-preregistration-payload.ts create mode 100644 src/domain/authentication/kratos-hook-messages.ts create mode 100644 src/services/kratos/registration-flow-error.ts create mode 100644 test/flash/unit/app/authentication/validate-preregistration-payload.spec.ts create mode 100644 test/flash/unit/domain/authentication/kratos-hook-messages.spec.ts create mode 100644 test/flash/unit/domain/authentication/preregistration-payload-validator.spec.ts create mode 100644 test/flash/unit/servers/event-handlers/kratos-preregistration-route.spec.ts create mode 100644 test/flash/unit/services/kratos/registration-flow-error.spec.ts diff --git a/dev/ory/kratos.yml b/dev/ory/kratos.yml index 7001bbd1a..bdf3ea38c 100644 --- a/dev/ory/kratos.yml +++ b/dev/ory/kratos.yml @@ -111,25 +111,30 @@ selfservice: after: password: hooks: - # we are not sure if we need this hook yet. - # this could be used to check if the user is already registered in the backend - # before creating the user in kratos - # otherwise response: parse: false happens after kratos user creation - # - # - # - hook: web_hook - # config: - # url: http://bats-tests:4012/kratos/preregistration - # method: POST - # response: - # parse: true - # body: file:///home/ory/body.jsonnet # TODO: use a base64 encoding instead - # auth: - # type: api_key - # config: - # name: Authorization - # value: The-Value-of-My-Key - # in: header + # Pre-persist validation. `response.parse: true` makes Kratos run + # this hook BEFORE the identity is written (web_hook.go, + # ExecutePostRegistrationPrePersistHook): a 4xx with a `messages` + # body aborts the sign-up and nothing is persisted. The api only + # validates here (phone, carrier metadata, phone not already bound) + # and must not write — the account is created by the post-persist + # /registration hook below, once the identity exists. Note + # ctx.identity.id is the nil uuid at this point. + - hook: web_hook + config: + url: http://bats-tests:4012/kratos/preregistration + method: POST + response: + parse: true + body: file:///home/ory/body.jsonnet # TODO: use a base64 encoding instead + auth: + type: api_key + config: + name: Authorization + value: The-Value-of-My-Key + in: header + # Post-persist account creation (unchanged). parse: false → runs + # after the identity is committed; a failure here leaves an identity + # without an account, which the session middleware self-heals. - hook: web_hook config: url: http://bats-tests:4012/kratos/registration diff --git a/src/app/authentication/index.ts b/src/app/authentication/index.ts index 4bb1b6a73..b11509281 100644 --- a/src/app/authentication/index.ts +++ b/src/app/authentication/index.ts @@ -7,3 +7,4 @@ export * from "./logout" export * from "./phone" export * from "./request-code" export * from "./totp" +export * from "./validate-preregistration-payload" diff --git a/src/app/authentication/validate-preregistration-payload.ts b/src/app/authentication/validate-preregistration-payload.ts new file mode 100644 index 000000000..6a27aba40 --- /dev/null +++ b/src/app/authentication/validate-preregistration-payload.ts @@ -0,0 +1,56 @@ +import { KRATOS_CALLBACK_API_KEY } from "@config" + +import { CallbackSecretValidator } from "@domain/authentication/secret-validator" +import { PreRegistrationPayloadValidator } from "@domain/authentication/registration-payload-validator" +import { PhoneAlreadyExistsError } from "@domain/authentication/errors" +import { CouldNotFindUserFromPhoneError } from "@domain/errors" + +import { addAttributesToCurrentSpan } from "@services/tracing" +import { SchemaIdType } from "@services/kratos" +import { UsersRepository } from "@services/mongoose" + +// Pre-persist half of registration. Kratos calls this BEFORE it commits the +// identity (web_hook with `response.parse: true`); a rejection here aborts the +// sign-up with nothing written anywhere. It therefore must not write anything +// either: every check is read-only, and the account itself is still created by +// the post-persist /registration hook once the identity exists. +// +// `identity_id` is absent/nil at this point and is never inspected. +export const validatePreRegistrationPayload = async ({ + secret, + body, +}: { + secret: string | undefined + body: { + identity_id?: string | null + phone?: string + schema_id?: string + transient_payload?: { phoneMetadata?: RawPhoneMetadataPayload } | null + flow_id?: string | null + flow_type?: string | null + } +}): Promise => { + addAttributesToCurrentSpan({ + "preregistration.schema_id": body.schema_id, + }) + + const isValidKey = CallbackSecretValidator(KRATOS_CALLBACK_API_KEY).authorize(secret) + if (isValidKey instanceof Error) { + return isValidKey + } + + const payload = PreRegistrationPayloadValidator( + SchemaIdType.PhoneNoPasswordV0, + ).validate(body) + if (payload instanceof Error) return payload + + // The post-persist hook upserts the users document by phone, which has a + // unique index. A document already holding this phone is exactly the + // DuplicateKeyForPersistError that used to strand the identity; reject it + // here while nothing has been committed. + const existing = await UsersRepository().findByPhone(payload.phone) + if (existing instanceof CouldNotFindUserFromPhoneError) return true + if (existing instanceof Error) return existing + + return new PhoneAlreadyExistsError() +} diff --git a/src/domain/authentication/errors.ts b/src/domain/authentication/errors.ts index a2d3a9308..35a799062 100644 --- a/src/domain/authentication/errors.ts +++ b/src/domain/authentication/errors.ts @@ -6,6 +6,10 @@ export class LikelyUserAlreadyExistError extends AuthenticationError {} export class AccountHasPositiveBalanceError extends AuthenticationError {} export class PhoneAlreadyExistsError extends AuthenticationError {} +// The pre-persist registration hook rejected the phone (unparseable, or its +// carrier metadata failed validation). Distinct from "already exists": the +// user cannot fix it by logging in instead. +export class PhoneNotAllowedForRegistrationError extends AuthenticationError {} export class EmailCodeInvalidError extends AuthenticationError {} export class EmailUnverifiedError extends AuthenticationError {} diff --git a/src/domain/authentication/index.types.d.ts b/src/domain/authentication/index.types.d.ts index 8dc709a49..955843f9a 100644 --- a/src/domain/authentication/index.types.d.ts +++ b/src/domain/authentication/index.types.d.ts @@ -75,15 +75,35 @@ type RegistrationPayload = { phone: PhoneNumber phoneMetadata: PhoneMetadata | undefined } +type RawPhoneMetadataPayload = Record> + type RegistrationPayloadValidator = { validate(rawBody: { identity_id?: string phone?: string schema_id?: string - transient_payload?: { phoneMetadata?: Record> } + transient_payload?: { phoneMetadata?: RawPhoneMetadataPayload } }): RegistrationPayload | ValidationError } +// What the pre-persist registration hook can know: the identity has no id yet +// (Kratos sends the nil uuid), so only the phone and its metadata are checked. +type PreRegistrationPayload = { + phone: PhoneNumber + phoneMetadata: PhoneMetadata | undefined +} + +type PreRegistrationPayloadValidator = { + validate(rawBody: { + identity_id?: string | null + phone?: string + schema_id?: string + transient_payload?: { phoneMetadata?: RawPhoneMetadataPayload } | null + flow_id?: string | null + flow_type?: string | null + }): PreRegistrationPayload | ValidationError +} + interface IAuthWithPhonePasswordlessService { loginToken(args: { phone: PhoneNumber diff --git a/src/domain/authentication/kratos-hook-messages.ts b/src/domain/authentication/kratos-hook-messages.ts new file mode 100644 index 000000000..81a75ff46 --- /dev/null +++ b/src/domain/authentication/kratos-hook-messages.ts @@ -0,0 +1,77 @@ +// Contract between the api and Kratos for flow-interrupting web hooks. +// +// A web_hook configured with `response.parse: true` runs BEFORE Kratos persists +// the identity (selfservice/hook/web_hook.go, ExecutePostRegistrationPrePersistHook). +// Answering it with a 4xx and this body aborts the registration and surfaces +// the messages on the flow; nothing is written. The ids are Flash-private +// (Kratos' own message ids live below 5000000 in text/message_*.go, so a +// dedicated 41xxxxx block cannot collide) and are what the api matches on when +// Kratos hands the rejected flow back to it — never the text. + +export const KratosHookMessageId = { + PhoneNotAllowed: 4100001, + PhoneAlreadyRegistered: 4100002, + PayloadInvalid: 4100003, + Unauthorized: 4100401, + InternalError: 4100500, +} as const + +export type KratosHookMessageId = + (typeof KratosHookMessageId)[keyof typeof KratosHookMessageId] + +export const KratosHookMessageText: Record = { + [KratosHookMessageId.PhoneNotAllowed]: "This phone number can't be used to sign up.", + [KratosHookMessageId.PhoneAlreadyRegistered]: + "This phone number is already registered.", + [KratosHookMessageId.PayloadInvalid]: "Sign-up request was invalid. Please try again.", + [KratosHookMessageId.Unauthorized]: "Sign-up is temporarily unavailable.", + [KratosHookMessageId.InternalError]: + "Sign-up is temporarily unavailable. Please try again.", +} + +export const KRATOS_HOOK_PHONE_INSTANCE_PTR = "#/traits/phone" + +export type KratosHookRejectionBody = { + messages: { + instance_ptr: string + messages: { id: number; text: string; type: "error" }[] + }[] +} + +// Exact shape Kratos' parseWebhookResponse decodes for status >= 400. +export const kratosHookRejection = ( + id: KratosHookMessageId, +): KratosHookRejectionBody => ({ + messages: [ + { + instance_ptr: KRATOS_HOOK_PHONE_INSTANCE_PTR, + messages: [{ id, text: KratosHookMessageText[id], type: "error" }], + }, + ], +}) + +// When the hook rejects, Kratos answers the api's updateRegistrationFlow call +// with HTTP 400 and the flow; our messages land on the matching ui node (or on +// ui.messages when no node matches the instance pointer). Collect every id in +// either place so the caller can map the rejection back to a domain error. +export const kratosHookMessageIdsFromFlow = (flowLike: unknown): number[] => { + if (!flowLike || typeof flowLike !== "object") return [] + const ui = (flowLike as { ui?: unknown }).ui + if (!ui || typeof ui !== "object") return [] + + const ids: number[] = [] + const collect = (messages: unknown) => { + if (!Array.isArray(messages)) return + for (const msg of messages) { + const id = (msg as { id?: unknown })?.id + if (typeof id === "number") ids.push(id) + } + } + + collect((ui as { messages?: unknown }).messages) + const nodes = (ui as { nodes?: unknown }).nodes + if (Array.isArray(nodes)) { + for (const node of nodes) collect((node as { messages?: unknown })?.messages) + } + return ids +} diff --git a/src/domain/authentication/registration-payload-validator.ts b/src/domain/authentication/registration-payload-validator.ts index e54b2795c..e42174d66 100644 --- a/src/domain/authentication/registration-payload-validator.ts +++ b/src/domain/authentication/registration-payload-validator.ts @@ -6,6 +6,30 @@ import { UnsupportedSchemaTypeError, } from "./errors" +type RawPhoneMetadata = RawPhoneMetadataPayload | undefined + +// Shared by both hooks: the phone and its carrier metadata are the only parts +// of the payload that exist before Kratos persists the identity. +const validatePhoneAndMetadata = ({ + phoneRaw, + rawPhoneMetadata, +}: { + phoneRaw: string + rawPhoneMetadata: RawPhoneMetadata +}): PreRegistrationPayload | ValidationError => { + const phoneChecked = checkedToPhoneNumber(phoneRaw) + if (phoneChecked instanceof Error) return phoneChecked + + let phoneMetadata: PhoneMetadata | undefined = undefined + if (rawPhoneMetadata !== undefined) { + const validated = PhoneMetadataValidator().validate(rawPhoneMetadata) + if (validated instanceof Error) return validated + phoneMetadata = validated + } + + return { phone: phoneChecked, phoneMetadata } +} + export const RegistrationPayloadValidator = ( schemaId: SchemaId, ): RegistrationPayloadValidator => { @@ -13,7 +37,7 @@ export const RegistrationPayloadValidator = ( identity_id?: string phone?: string schema_id?: string - transient_payload?: { phoneMetadata?: Record> } + transient_payload?: { phoneMetadata?: RawPhoneMetadataPayload } }): RegistrationPayload | ValidationError => { const { identity_id: userIdRaw, @@ -33,23 +57,52 @@ export const RegistrationPayloadValidator = ( const userIdChecked = checkedToUserId(userIdRaw) if (userIdChecked instanceof Error) return userIdChecked - const phoneChecked = checkedToPhoneNumber(phoneRaw) - if (phoneChecked instanceof Error) return phoneChecked + const checked = validatePhoneAndMetadata({ + phoneRaw, + rawPhoneMetadata: transient_payload?.phoneMetadata, + }) + if (checked instanceof Error) return checked + + return { + userId: userIdChecked, + phone: checked.phone, + phoneMetadata: checked.phoneMetadata, + } + } + + return { + validate, + } +} - const rawPhoneMetadata = transient_payload?.phoneMetadata +// For the pre-persist hook. `identity_id` is deliberately ignored: Kratos +// only assigns the real id at persist time, so the hook receives the nil uuid +// ("00000000-0000-0000-0000-000000000000"). Nothing may be looked up by it. +export const PreRegistrationPayloadValidator = ( + schemaId: SchemaId, +): PreRegistrationPayloadValidator => { + const validate = (rawBody: { + identity_id?: string | null + phone?: string + schema_id?: string + transient_payload?: { phoneMetadata?: RawPhoneMetadataPayload } | null + flow_id?: string | null + flow_type?: string | null + }): PreRegistrationPayload | ValidationError => { + const { phone: phoneRaw, schema_id: schemaIdRaw, transient_payload } = rawBody - let phoneMetadata: PhoneMetadata | undefined = undefined - if (rawPhoneMetadata !== undefined) { - const validated = PhoneMetadataValidator().validate(rawPhoneMetadata) - if (validated instanceof Error) return validated - phoneMetadata = validated + if (!(phoneRaw && schemaIdRaw)) { + return new MissingRegistrationPayloadPropertiesError() } - return { - userId: userIdChecked, - phone: phoneChecked, - phoneMetadata, + if (schemaIdRaw !== schemaId) { + return new UnsupportedSchemaTypeError() } + + return validatePhoneAndMetadata({ + phoneRaw, + rawPhoneMetadata: transient_payload?.phoneMetadata, + }) } return { diff --git a/src/graphql/error-map.ts b/src/graphql/error-map.ts index 76c08a923..dffbcd697 100644 --- a/src/graphql/error-map.ts +++ b/src/graphql/error-map.ts @@ -527,6 +527,13 @@ export const mapError = (error: ApplicationError): CustomApolloError => { case "PhoneAlreadyExistsError": return new PhoneAlreadyExistsError({ logger: baseLogger }) + // The pre-persist registration hook refused the number (unparseable, or + // carrier metadata failed validation). A policy answer, not a bug: it + // must not fall into the catch-all that tells the user to retry. + case "PhoneNotAllowedForRegistrationError": + message = "This phone number can't be used to sign up" + return new ValidationInternalError({ message, logger: baseLogger }) + case "EmailAlreadyExistsError": return new EmailAlreadyExistsError({ logger: baseLogger }) diff --git a/src/servers/event-handlers/kratos.ts b/src/servers/event-handlers/kratos.ts index 757856e12..4bbb99425 100644 --- a/src/servers/event-handlers/kratos.ts +++ b/src/servers/event-handlers/kratos.ts @@ -3,13 +3,26 @@ import express from "express" import { wrapAsyncToRunInSpan } from "@services/tracing" import { baseLogger } from "@services/logger" +import { maskPhone } from "@services/alerts/ops-events" import { Authentication } from "@app" import { - SecretForAuthNCallbackError, + PhoneAlreadyExistsError, RegistrationPayloadValidationError, + SecretForAuthNCallbackError, } from "@domain/authentication/errors" +import { + KratosHookMessageId, + kratosHookRejection, +} from "@domain/authentication/kratos-hook-messages" +import { InvalidPhoneNumber } from "@domain/errors" +import { + InvalidCarrierForPhoneMetadataError, + InvalidCarrierTypeForPhoneMetadataError, + InvalidCountryCodeForPhoneMetadataError, + PhoneMetadataValidationError, +} from "@domain/users/errors" const errorResponseMessages: { [key: string]: string } = { MissingSecretForAuthNCallbackError: "missing authorization header", @@ -25,6 +38,76 @@ const kratosCallback = express.Router({ caseSensitive: true }) kratosCallback.use(cors({ origin: true, credentials: true })) kratosCallback.use(express.json()) +// Pre-persist hook (Kratos web_hook with `response.parse: true`). Runs before +// the identity is written, so a 4xx here aborts the sign-up with nothing +// persisted. Kratos decodes the body of every non-2xx answer as the `messages` +// shape and a 200 as JSON, so this route never answers plain text: a body it +// cannot parse turns into an opaque "webhook failed" for the user. +// +// Validation only — the account is created by /registration below, after the +// identity exists. `identity_id` arrives as the nil uuid and is ignored. +kratosCallback.post( + "/preregistration", + wrapAsyncToRunInSpan({ + namespace: "preregistration", + fn: async (req: express.Request, res: express.Response) => { + const secret = req.headers.authorization + const body = req.body ?? {} + const phone = typeof body.phone === "string" ? maskPhone(body.phone) : undefined + + const result = await Authentication.validatePreRegistrationPayload({ + secret, + body, + }) + + if (result instanceof Error) { + switch (true) { + case result instanceof SecretForAuthNCallbackError: + baseLogger.error({ err: result.name }, "preregistration: bad callback secret") + res.status(401).json(kratosHookRejection(KratosHookMessageId.Unauthorized)) + return + + case result instanceof PhoneAlreadyExistsError: + baseLogger.warn({ phone, rejection: result.name }, "preregistration rejected") + res + .status(400) + .json(kratosHookRejection(KratosHookMessageId.PhoneAlreadyRegistered)) + return + + case result instanceof InvalidPhoneNumber: + case result instanceof PhoneMetadataValidationError: + case result instanceof InvalidCarrierForPhoneMetadataError: + case result instanceof InvalidCarrierTypeForPhoneMetadataError: + case result instanceof InvalidCountryCodeForPhoneMetadataError: + baseLogger.warn({ phone, rejection: result.name }, "preregistration rejected") + res.status(400).json(kratosHookRejection(KratosHookMessageId.PhoneNotAllowed)) + return + + case result instanceof RegistrationPayloadValidationError: + baseLogger.warn( + { phone, rejection: result.name, schemaId: body.schema_id }, + "preregistration rejected", + ) + res.status(400).json(kratosHookRejection(KratosHookMessageId.PayloadInvalid)) + return + + default: + baseLogger.error( + { err: result, phone }, + "preregistration: unexpected error, sign-up aborted", + ) + res.status(500).json(kratosHookRejection(KratosHookMessageId.InternalError)) + return + } + } + + res.status(200).json({}) + }, + }), +) + +// Post-persist hook (`response.parse: false`): the identity is already +// committed when this runs. Creates the account and wallets. kratosCallback.post( "/registration", wrapAsyncToRunInSpan({ diff --git a/src/services/kratos/auth-phone-no-password.ts b/src/services/kratos/auth-phone-no-password.ts index e2c427c93..936af08d0 100644 --- a/src/services/kratos/auth-phone-no-password.ts +++ b/src/services/kratos/auth-phone-no-password.ts @@ -19,6 +19,7 @@ import { UnknownKratosError, } from "./errors" import { kratosAdmin, kratosPublic, toDomainIdentityPhone } from "./private" +import { mapRegistrationFlowRejection } from "./registration-flow-error" import { SchemaIdType } from "./schema" // login with phone @@ -173,9 +174,8 @@ export const AuthWithPhonePasswordlessService = (): IAuthWithPhonePasswordlessSe return { authToken, kratosUserId } } catch (err) { - if (err instanceof Error && err.message === "Request failed with status code 400") { - return new LikelyUserAlreadyExistError(err.message || err) - } + const rejection = mapRegistrationFlowRejection(err) + if (rejection) return rejection return new UnknownKratosError(err) } @@ -265,9 +265,8 @@ export const AuthWithPhonePasswordlessService = (): IAuthWithPhonePasswordlessSe const kratosUserId = result.data.identity.id as UserId return { cookiesToSendBackToClient, kratosUserId } } catch (err) { - if (err instanceof Error && err.message === "Request failed with status code 400") { - return new LikelyUserAlreadyExistError(err.message || err) - } + const rejection = mapRegistrationFlowRejection(err) + if (rejection) return rejection return new UnknownKratosError(err) } } diff --git a/src/services/kratos/registration-flow-error.ts b/src/services/kratos/registration-flow-error.ts new file mode 100644 index 000000000..bf19ef773 --- /dev/null +++ b/src/services/kratos/registration-flow-error.ts @@ -0,0 +1,42 @@ +import { + LikelyUserAlreadyExistError, + PhoneAlreadyExistsError, + PhoneNotAllowedForRegistrationError, +} from "@domain/authentication/errors" +import { + KratosHookMessageId, + kratosHookMessageIdsFromFlow, +} from "@domain/authentication/kratos-hook-messages" + +const isStatus400 = (err: unknown): boolean => + err instanceof Error && err.message === "Request failed with status code 400" + +// Kratos answers a self-service registration with 400 for two very different +// reasons: the identifier already exists, or the pre-persist web hook rejected +// the sign-up. The rejected flow carries our own message ids; anything else +// keeps the historical "likely already exists" reading. +export const mapRegistrationFlowRejection = ( + err: unknown, +): + | PhoneAlreadyExistsError + | PhoneNotAllowedForRegistrationError + | LikelyUserAlreadyExistError + | null => { + if (!isStatus400(err)) return null + + const data = (err as { response?: { data?: unknown } }).response?.data + const ids = kratosHookMessageIdsFromFlow(data) + + if (ids.includes(KratosHookMessageId.PhoneAlreadyRegistered)) { + return new PhoneAlreadyExistsError() + } + + if ( + ids.includes(KratosHookMessageId.PhoneNotAllowed) || + ids.includes(KratosHookMessageId.PayloadInvalid) + ) { + return new PhoneNotAllowedForRegistrationError() + } + + return new LikelyUserAlreadyExistError((err as Error).message) +} diff --git a/test/flash/unit/app/authentication/validate-preregistration-payload.spec.ts b/test/flash/unit/app/authentication/validate-preregistration-payload.spec.ts new file mode 100644 index 000000000..822ae95b0 --- /dev/null +++ b/test/flash/unit/app/authentication/validate-preregistration-payload.spec.ts @@ -0,0 +1,134 @@ +import { validatePreRegistrationPayload } from "@app/authentication/validate-preregistration-payload" +import { + InvalidSecretForAuthNCallbackError, + MissingSecretForAuthNCallbackError, + PhoneAlreadyExistsError, + UnsupportedSchemaTypeError, +} from "@domain/authentication/errors" +import { CouldNotFindUserFromPhoneError, UnknownRepositoryError } from "@domain/errors" +import { InvalidCarrierTypeForPhoneMetadataError } from "@domain/users/errors" +import { SchemaIdType } from "@services/kratos" +import { UsersRepository } from "@services/mongoose" + +const CALLBACK_KEY = "unit-test-callback-key" + +jest.mock("@config", () => ({ + KRATOS_CALLBACK_API_KEY: "unit-test-callback-key", +})) + +jest.mock("@services/tracing", () => ({ + addAttributesToCurrentSpan: jest.fn(), + recordExceptionInCurrentSpan: jest.fn(), +})) + +jest.mock("@services/kratos", () => ({ + SchemaIdType: { PhoneNoPasswordV0: "phone_no_password_v0" }, +})) + +const mockFindByPhone = jest.fn() +jest.mock("@services/mongoose", () => ({ + UsersRepository: jest.fn(() => ({ findByPhone: mockFindByPhone })), +})) + +const NIL_UUID = "00000000-0000-0000-0000-000000000000" +const phone = "+18765550123" +const carrier = { + error_code: "", + mobile_country_code: "338", + mobile_network_code: "050", + name: "Digicel", + type: "mobile", +} + +const body = (overrides: Record = {}) => ({ + identity_id: NIL_UUID, + phone, + schema_id: SchemaIdType.PhoneNoPasswordV0, + transient_payload: { phoneMetadata: { carrier, countryCode: "JM" } }, + flow_id: "flow", + flow_type: "api", + ...overrides, +}) + +describe("validatePreRegistrationPayload", () => { + beforeEach(() => { + mockFindByPhone.mockReset() + mockFindByPhone.mockResolvedValue(new CouldNotFindUserFromPhoneError()) + }) + + it("passes when the phone is valid and not bound to any users document", async () => { + const result = await validatePreRegistrationPayload({ + secret: CALLBACK_KEY, + body: body(), + }) + + expect(result).toBe(true) + expect(UsersRepository).toHaveBeenCalled() + expect(mockFindByPhone).toHaveBeenCalledWith(phone) + }) + + it("never writes and never looks anything up by the nil identity id", async () => { + const repo = UsersRepository() as unknown as Record + await validatePreRegistrationPayload({ secret: CALLBACK_KEY, body: body() }) + + expect(Object.keys(repo)).toStrictEqual(["findByPhone"]) + expect(mockFindByPhone).toHaveBeenCalledTimes(1) + expect(mockFindByPhone.mock.calls[0][0]).not.toBe(NIL_UUID) + }) + + it("rejects a missing or wrong secret before touching the payload", async () => { + expect( + await validatePreRegistrationPayload({ secret: undefined, body: body() }), + ).toBeInstanceOf(MissingSecretForAuthNCallbackError) + expect( + await validatePreRegistrationPayload({ secret: "wrong", body: body() }), + ).toBeInstanceOf(InvalidSecretForAuthNCallbackError) + expect(mockFindByPhone).not.toHaveBeenCalled() + }) + + it("rejects an invalid carrier type without touching the repository", async () => { + const result = await validatePreRegistrationPayload({ + secret: CALLBACK_KEY, + body: body({ + transient_payload: { + phoneMetadata: { carrier: { ...carrier, type: "" }, countryCode: "NG" }, + }, + }), + }) + + expect(result).toBeInstanceOf(InvalidCarrierTypeForPhoneMetadataError) + expect(mockFindByPhone).not.toHaveBeenCalled() + }) + + it("rejects an unsupported schema", async () => { + expect( + await validatePreRegistrationPayload({ + secret: CALLBACK_KEY, + body: body({ schema_id: "email_no_password_v0" }), + }), + ).toBeInstanceOf(UnsupportedSchemaTypeError) + }) + + it("rejects a phone already bound to a users document (the DuplicateKey source)", async () => { + mockFindByPhone.mockResolvedValue({ id: "other-user", phone }) + + const result = await validatePreRegistrationPayload({ + secret: CALLBACK_KEY, + body: body(), + }) + + expect(result).toBeInstanceOf(PhoneAlreadyExistsError) + }) + + it("surfaces repository failures so the route answers 500 and the sign-up aborts", async () => { + const repoError = new UnknownRepositoryError("mongo down") + mockFindByPhone.mockResolvedValue(repoError) + + const result = await validatePreRegistrationPayload({ + secret: CALLBACK_KEY, + body: body(), + }) + + expect(result).toBe(repoError) + }) +}) diff --git a/test/flash/unit/domain/authentication/kratos-hook-messages.spec.ts b/test/flash/unit/domain/authentication/kratos-hook-messages.spec.ts new file mode 100644 index 000000000..79c78568a --- /dev/null +++ b/test/flash/unit/domain/authentication/kratos-hook-messages.spec.ts @@ -0,0 +1,71 @@ +import { + KRATOS_HOOK_PHONE_INSTANCE_PTR, + KratosHookMessageId, + KratosHookMessageText, + kratosHookMessageIdsFromFlow, + kratosHookRejection, +} from "@domain/authentication/kratos-hook-messages" + +describe("kratos hook messages", () => { + it("builds the exact body Kratos' parseWebhookResponse decodes for a >= 400 answer", () => { + expect(kratosHookRejection(KratosHookMessageId.PhoneNotAllowed)).toStrictEqual({ + messages: [ + { + instance_ptr: "#/traits/phone", + messages: [ + { + id: 4100001, + text: "This phone number can't be used to sign up.", + type: "error", + }, + ], + }, + ], + }) + expect(KRATOS_HOOK_PHONE_INSTANCE_PTR).toBe("#/traits/phone") + }) + + it("has a distinct id and user-readable text for every rejection kind", () => { + const ids = Object.values(KratosHookMessageId) + expect(new Set(ids).size).toBe(ids.length) + for (const id of ids) { + expect(id).toBeGreaterThanOrEqual(4100000) + expect(KratosHookMessageText[id]).toMatch(/\S/) + expect(kratosHookRejection(id).messages[0].messages[0]).toStrictEqual({ + id, + text: KratosHookMessageText[id], + type: "error", + }) + } + }) + + describe("kratosHookMessageIdsFromFlow", () => { + it("collects ids from ui.messages and from every node's messages", () => { + const flow = { + id: "flow-id", + ui: { + messages: [{ id: 4000007, text: "already exists", type: "error" }], + nodes: [ + { attributes: { name: "traits.phone" }, messages: [] }, + { + attributes: { name: "traits.phone" }, + messages: [{ id: 4100002, text: "registered", type: "error" }], + }, + ], + }, + } + + expect(kratosHookMessageIdsFromFlow(flow)).toStrictEqual([4000007, 4100002]) + }) + + it("returns nothing for non-flow payloads", () => { + expect(kratosHookMessageIdsFromFlow(undefined)).toStrictEqual([]) + expect(kratosHookMessageIdsFromFlow("string")).toStrictEqual([]) + expect(kratosHookMessageIdsFromFlow({})).toStrictEqual([]) + expect(kratosHookMessageIdsFromFlow({ ui: { nodes: "nope" } })).toStrictEqual([]) + expect( + kratosHookMessageIdsFromFlow({ ui: { messages: [{ id: "4100001" }] } }), + ).toStrictEqual([]) + }) + }) +}) diff --git a/test/flash/unit/domain/authentication/preregistration-payload-validator.spec.ts b/test/flash/unit/domain/authentication/preregistration-payload-validator.spec.ts new file mode 100644 index 000000000..29b59dd3b --- /dev/null +++ b/test/flash/unit/domain/authentication/preregistration-payload-validator.spec.ts @@ -0,0 +1,170 @@ +import { + MissingRegistrationPayloadPropertiesError, + UnsupportedSchemaTypeError, +} from "@domain/authentication/errors" +import { + PreRegistrationPayloadValidator, + RegistrationPayloadValidator, +} from "@domain/authentication/registration-payload-validator" +import { InvalidPhoneNumber } from "@domain/errors" +import { + InvalidCarrierForPhoneMetadataError, + InvalidCarrierTypeForPhoneMetadataError, + InvalidCountryCodeForPhoneMetadataError, +} from "@domain/users/errors" + +import { SchemaIdType } from "@services/kratos" + +import { randomPhone, randomUserId } from "test/galoy/helpers/random" + +// Kratos assigns the identity id only at persist time; the pre-persist hook +// receives this placeholder and the validator must never look at it. +const NIL_UUID = "00000000-0000-0000-0000-000000000000" + +const validCarrier = { + error_code: "", + mobile_country_code: "338", + mobile_network_code: "050", + name: "Digicel", + type: "mobile", +} + +describe("PreRegistrationPayloadValidator", () => { + const validator = PreRegistrationPayloadValidator(SchemaIdType.PhoneNoPasswordV0) + + it("passes a valid body with the nil identity id Kratos sends pre-persist", () => { + const phone = randomPhone() + + const validated = validator.validate({ + identity_id: NIL_UUID, + phone, + schema_id: SchemaIdType.PhoneNoPasswordV0, + transient_payload: { phoneMetadata: { carrier: validCarrier, countryCode: "JM" } }, + }) + + expect(validated).toStrictEqual({ + phone, + phoneMetadata: { carrier: validCarrier, countryCode: "JM" }, + }) + }) + + it("does not require identity_id at all", () => { + const phone = randomPhone() + + for (const identity_id of [undefined, null, "not-a-uuid"]) { + const validated = validator.validate({ + identity_id, + phone, + schema_id: SchemaIdType.PhoneNoPasswordV0, + }) + expect(validated).toStrictEqual({ phone, phoneMetadata: undefined }) + } + }) + + it("passes without transient metadata (null or absent)", () => { + const phone = randomPhone() + + expect( + validator.validate({ phone, schema_id: SchemaIdType.PhoneNoPasswordV0 }), + ).toStrictEqual({ phone, phoneMetadata: undefined }) + expect( + validator.validate({ + phone, + schema_id: SchemaIdType.PhoneNoPasswordV0, + transient_payload: null, + }), + ).toStrictEqual({ phone, phoneMetadata: undefined }) + }) + + it("returns missing inputs error when phone or schema_id is absent", () => { + expect(validator.validate({})).toBeInstanceOf( + MissingRegistrationPayloadPropertiesError, + ) + expect( + validator.validate({ + identity_id: NIL_UUID, + schema_id: SchemaIdType.PhoneNoPasswordV0, + }), + ).toBeInstanceOf(MissingRegistrationPayloadPropertiesError) + expect( + validator.validate({ identity_id: NIL_UUID, phone: randomPhone() }), + ).toBeInstanceOf(MissingRegistrationPayloadPropertiesError) + }) + + it("returns unsupported schema error", () => { + expect( + validator.validate({ phone: randomPhone(), schema_id: "email_no_password_v0" }), + ).toBeInstanceOf(UnsupportedSchemaTypeError) + }) + + it("returns invalid phone error", () => { + expect( + validator.validate({ + phone: "invalid-phone", + schema_id: SchemaIdType.PhoneNoPasswordV0, + }), + ).toBeInstanceOf(InvalidPhoneNumber) + }) + + it("rejects a carrier type outside the known set (today's orphan source)", () => { + const validated = validator.validate({ + identity_id: NIL_UUID, + phone: randomPhone(), + schema_id: SchemaIdType.PhoneNoPasswordV0, + transient_payload: { + phoneMetadata: { carrier: { ...validCarrier, type: "" }, countryCode: "NG" }, + }, + }) + expect(validated).toBeInstanceOf(InvalidCarrierTypeForPhoneMetadataError) + }) + + it("rejects malformed carrier metadata", () => { + expect( + validator.validate({ + phone: randomPhone(), + schema_id: SchemaIdType.PhoneNoPasswordV0, + transient_payload: { + phoneMetadata: { carrier: "not-an-object", countryCode: "JM" } as never, + }, + }), + ).toBeInstanceOf(InvalidCarrierForPhoneMetadataError) + + expect( + validator.validate({ + phone: randomPhone(), + schema_id: SchemaIdType.PhoneNoPasswordV0, + transient_payload: { + phoneMetadata: { carrier: validCarrier } as never, + }, + }), + ).toBeInstanceOf(InvalidCountryCodeForPhoneMetadataError) + }) +}) + +describe("RegistrationPayloadValidator (post-persist hook) is unchanged", () => { + const validator = RegistrationPayloadValidator(SchemaIdType.PhoneNoPasswordV0) + + it("still requires a real identity id", () => { + const rawUserId = randomUserId() + const phone = randomPhone() + + expect( + validator.validate({ + identity_id: rawUserId, + phone, + schema_id: SchemaIdType.PhoneNoPasswordV0, + transient_payload: { + phoneMetadata: { carrier: validCarrier, countryCode: "JM" }, + }, + }), + ).toStrictEqual({ + userId: rawUserId, + phone, + phoneMetadata: { carrier: validCarrier, countryCode: "JM" }, + }) + + expect( + validator.validate({ phone, schema_id: SchemaIdType.PhoneNoPasswordV0 }), + ).toBeInstanceOf(MissingRegistrationPayloadPropertiesError) + }) +}) diff --git a/test/flash/unit/graphql/error-map.spec.ts b/test/flash/unit/graphql/error-map.spec.ts index 85e8d37b2..606d27336 100644 --- a/test/flash/unit/graphql/error-map.spec.ts +++ b/test/flash/unit/graphql/error-map.spec.ts @@ -8,9 +8,17 @@ import { } from "@services/bridge/errors" import { IbexError, InsufficientIbexBalance } from "@services/ibex/errors" import { PhoneCountryNotAllowedError } from "@domain/users/errors" +import { PhoneNotAllowedForRegistrationError } from "@domain/authentication/errors" import { InvalidPhoneNumber } from "@domain/errors" describe("error-map", () => { + it("maps PhoneNotAllowedForRegistrationError to a user-readable validation error, not the catch-all", () => { + const result = mapError(new PhoneNotAllowedForRegistrationError()) + + expect(result.extensions.code).not.toBe("UNEXPECTED_CLIENT_ERROR") + expect(result.message).toBe("This phone number can't be used to sign up") + }) + it("maps BridgeWithdrawalNotFoundError to BRIDGE_WITHDRAWAL_NOT_FOUND", () => { const result = mapError(new BridgeWithdrawalNotFoundError()) diff --git a/test/flash/unit/servers/event-handlers/kratos-preregistration-route.spec.ts b/test/flash/unit/servers/event-handlers/kratos-preregistration-route.spec.ts new file mode 100644 index 000000000..0f3d8dab7 --- /dev/null +++ b/test/flash/unit/servers/event-handlers/kratos-preregistration-route.spec.ts @@ -0,0 +1,178 @@ +import { Request, Response } from "express" + +import { Authentication } from "@app" +import { + InvalidSecretForAuthNCallbackError, + MissingRegistrationPayloadPropertiesError, + PhoneAlreadyExistsError, +} from "@domain/authentication/errors" +import { InvalidPhoneNumber, UnknownRepositoryError } from "@domain/errors" +import { InvalidCarrierTypeForPhoneMetadataError } from "@domain/users/errors" +import kratosCallback from "@servers/event-handlers/kratos" + +jest.mock("@app", () => ({ + Authentication: { + validatePreRegistrationPayload: jest.fn(), + createAccountFromRegistrationPayload: jest.fn(), + }, +})) + +const mockValidate = Authentication.validatePreRegistrationPayload as jest.MockedFunction< + typeof Authentication.validatePreRegistrationPayload +> + +type RouteLayer = { + route?: { path: string; stack: { handle: (req: Request, res: Response) => unknown }[] } +} + +const handlerFor = (path: string) => { + const layer = (kratosCallback as unknown as { stack: RouteLayer[] }).stack.find( + (l) => l.route?.path === path, + ) + if (!layer?.route) throw new Error(`no route registered at ${path}`) + return layer.route.stack[0].handle +} + +const makeRes = () => { + const res = { status: jest.fn(), json: jest.fn(), send: jest.fn() } + res.status.mockReturnValue(res) + return res as unknown as Response & { + status: jest.Mock + json: jest.Mock + send: jest.Mock + } +} + +// Kratos sends the nil uuid here: the identity is not persisted yet. +const NIL_UUID = "00000000-0000-0000-0000-000000000000" + +const makeReq = (bodyOverrides: Record = {}) => + ({ + headers: { authorization: "callback-key" }, + body: { + identity_id: NIL_UUID, + phone: "+2348012345678", + schema_id: "phone_no_password_v0", + transient_payload: { + phoneMetadata: { carrier: { type: "mobile" }, countryCode: "NG" }, + }, + flow_id: "flow", + flow_type: "api", + ...bodyOverrides, + }, + }) as unknown as Request + +const rejection = (id: number, text: string) => ({ + messages: [{ instance_ptr: "#/traits/phone", messages: [{ id, text, type: "error" }] }], +}) + +describe("POST /kratos/preregistration", () => { + const handler = handlerFor("/preregistration") + + beforeEach(() => mockValidate.mockReset()) + + it("answers 200 with an empty JSON object when validation passes", async () => { + mockValidate.mockResolvedValue(true) + const res = makeRes() + + await handler(makeReq(), res) + + expect(mockValidate).toHaveBeenCalledWith({ + secret: "callback-key", + body: expect.objectContaining({ identity_id: NIL_UUID, phone: "+2348012345678" }), + }) + expect(res.status).toHaveBeenCalledWith(200) + expect(res.json).toHaveBeenCalledWith({}) + expect(res.send).not.toHaveBeenCalled() + }) + + it("answers 400 with the Kratos messages body on a carrier-type rejection", async () => { + mockValidate.mockResolvedValue(new InvalidCarrierTypeForPhoneMetadataError()) + const res = makeRes() + + await handler(makeReq(), res) + + expect(res.status).toHaveBeenCalledWith(400) + expect(res.json).toHaveBeenCalledWith( + rejection(4100001, "This phone number can't be used to sign up."), + ) + }) + + it("answers 400 'not allowed' for an unparseable phone", async () => { + mockValidate.mockResolvedValue(new InvalidPhoneNumber()) + const res = makeRes() + + await handler(makeReq({ phone: "nope" }), res) + + expect(res.status).toHaveBeenCalledWith(400) + expect(res.json).toHaveBeenCalledWith( + rejection(4100001, "This phone number can't be used to sign up."), + ) + }) + + it("answers 400 'already registered' when the phone is bound to another identity", async () => { + mockValidate.mockResolvedValue(new PhoneAlreadyExistsError()) + const res = makeRes() + + await handler(makeReq(), res) + + expect(res.status).toHaveBeenCalledWith(400) + expect(res.json).toHaveBeenCalledWith( + rejection(4100002, "This phone number is already registered."), + ) + }) + + it("answers 400 'payload invalid' when the hook body itself is malformed", async () => { + mockValidate.mockResolvedValue(new MissingRegistrationPayloadPropertiesError()) + const res = makeRes() + + await handler(makeReq({ phone: undefined }), res) + + expect(res.status).toHaveBeenCalledWith(400) + expect(res.json).toHaveBeenCalledWith( + rejection(4100003, "Sign-up request was invalid. Please try again."), + ) + }) + + it("answers 401 as JSON on a bad callback secret", async () => { + mockValidate.mockResolvedValue(new InvalidSecretForAuthNCallbackError()) + const res = makeRes() + + await handler(makeReq(), res) + + expect(res.status).toHaveBeenCalledWith(401) + expect(res.json).toHaveBeenCalledWith( + rejection(4100401, "Sign-up is temporarily unavailable."), + ) + expect(res.send).not.toHaveBeenCalled() + }) + + it("answers 500 as JSON (never plain text) on an unexpected error", async () => { + mockValidate.mockResolvedValue(new UnknownRepositoryError("mongo down")) + const res = makeRes() + + await handler(makeReq(), res) + + expect(res.status).toHaveBeenCalledWith(500) + expect(res.json).toHaveBeenCalledWith( + rejection(4100500, "Sign-up is temporarily unavailable. Please try again."), + ) + expect(res.send).not.toHaveBeenCalled() + }) + + it("tolerates a missing body", async () => { + mockValidate.mockResolvedValue(new MissingRegistrationPayloadPropertiesError()) + const res = makeRes() + + await handler({ headers: {}, body: undefined } as unknown as Request, res) + + expect(mockValidate).toHaveBeenCalledWith({ secret: undefined, body: {} }) + expect(res.status).toHaveBeenCalledWith(400) + }) +}) + +describe("POST /kratos/registration is unchanged", () => { + it("is still registered after the pre-registration route", () => { + expect(() => handlerFor("/registration")).not.toThrow() + }) +}) diff --git a/test/flash/unit/services/kratos/registration-flow-error.spec.ts b/test/flash/unit/services/kratos/registration-flow-error.spec.ts new file mode 100644 index 000000000..f3778c1fb --- /dev/null +++ b/test/flash/unit/services/kratos/registration-flow-error.spec.ts @@ -0,0 +1,104 @@ +import { + LikelyUserAlreadyExistError, + PhoneAlreadyExistsError, + PhoneNotAllowedForRegistrationError, +} from "@domain/authentication/errors" +import { KratosHookMessageId } from "@domain/authentication/kratos-hook-messages" +import { mapRegistrationFlowRejection } from "@services/kratos/registration-flow-error" + +// Shape @ory/client (axios) throws when Kratos answers updateRegistrationFlow +// with 400: the message is the axios one, the flow rides in response.data. +const axios400 = (flow: unknown) => + Object.assign(new Error("Request failed with status code 400"), { + response: { status: 400, data: flow }, + }) + +const flowWithHookMessage = (id: number) => ({ + id: "flow", + ui: { + messages: [], + nodes: [ + { + attributes: { name: "traits.phone" }, + messages: [{ id, text: "x", type: "error" }], + }, + ], + }, +}) + +describe("mapRegistrationFlowRejection", () => { + it("ignores anything that is not a 400", () => { + expect( + mapRegistrationFlowRejection(new Error("Request failed with status code 500")), + ).toBeNull() + expect(mapRegistrationFlowRejection(new Error("ECONNREFUSED"))).toBeNull() + expect(mapRegistrationFlowRejection("400")).toBeNull() + expect(mapRegistrationFlowRejection(undefined)).toBeNull() + }) + + it("maps the pre-hook 'already registered' id to PhoneAlreadyExistsError", () => { + expect( + mapRegistrationFlowRejection( + axios400(flowWithHookMessage(KratosHookMessageId.PhoneAlreadyRegistered)), + ), + ).toBeInstanceOf(PhoneAlreadyExistsError) + }) + + it("maps the pre-hook 'not allowed' and 'payload invalid' ids to PhoneNotAllowedForRegistrationError", () => { + expect( + mapRegistrationFlowRejection( + axios400(flowWithHookMessage(KratosHookMessageId.PhoneNotAllowed)), + ), + ).toBeInstanceOf(PhoneNotAllowedForRegistrationError) + expect( + mapRegistrationFlowRejection( + axios400(flowWithHookMessage(KratosHookMessageId.PayloadInvalid)), + ), + ).toBeInstanceOf(PhoneNotAllowedForRegistrationError) + }) + + it("prefers 'already registered' when both ids are present", () => { + const flow = { + ui: { + messages: [{ id: KratosHookMessageId.PhoneNotAllowed, text: "x", type: "error" }], + nodes: [ + { + messages: [ + { + id: KratosHookMessageId.PhoneAlreadyRegistered, + text: "y", + type: "error", + }, + ], + }, + ], + }, + } + expect(mapRegistrationFlowRejection(axios400(flow))).toBeInstanceOf( + PhoneAlreadyExistsError, + ) + }) + + it("keeps the historical reading for a 400 without our ids (Kratos duplicate identifier)", () => { + const kratosDuplicate = { + ui: { + messages: [ + { + id: 4000007, + text: "An account with the same identifier exists already.", + type: "error", + }, + ], + }, + } + const mapped = mapRegistrationFlowRejection(axios400(kratosDuplicate)) + expect(mapped).toBeInstanceOf(LikelyUserAlreadyExistError) + expect((mapped as Error).message).toBe("Request failed with status code 400") + }) + + it("keeps the historical reading for a 400 with no body at all", () => { + expect( + mapRegistrationFlowRejection(new Error("Request failed with status code 400")), + ).toBeInstanceOf(LikelyUserAlreadyExistError) + }) +}) From 85a3e48e36a7404fb99f8a1dc94ac166b34a140a Mon Sep 17 00:00:00 2001 From: Dread Date: Tue, 1 Sep 2026 16:40:33 -0700 Subject: [PATCH 2/5] =?UTF-8?q?fix(ci):=20typos=20spelling=20=E2=80=94=20u?= =?UTF-8?q?nparseable=20->=20unparsable=20(comments=20+=20test=20names)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/domain/authentication/errors.ts | 2 +- src/graphql/error-map.ts | 2 +- .../servers/event-handlers/kratos-preregistration-route.spec.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/domain/authentication/errors.ts b/src/domain/authentication/errors.ts index 35a799062..64b1a4cee 100644 --- a/src/domain/authentication/errors.ts +++ b/src/domain/authentication/errors.ts @@ -6,7 +6,7 @@ export class LikelyUserAlreadyExistError extends AuthenticationError {} export class AccountHasPositiveBalanceError extends AuthenticationError {} export class PhoneAlreadyExistsError extends AuthenticationError {} -// The pre-persist registration hook rejected the phone (unparseable, or its +// The pre-persist registration hook rejected the phone (unparsable, or its // carrier metadata failed validation). Distinct from "already exists": the // user cannot fix it by logging in instead. export class PhoneNotAllowedForRegistrationError extends AuthenticationError {} diff --git a/src/graphql/error-map.ts b/src/graphql/error-map.ts index dffbcd697..aa8aa0bcb 100644 --- a/src/graphql/error-map.ts +++ b/src/graphql/error-map.ts @@ -527,7 +527,7 @@ export const mapError = (error: ApplicationError): CustomApolloError => { case "PhoneAlreadyExistsError": return new PhoneAlreadyExistsError({ logger: baseLogger }) - // The pre-persist registration hook refused the number (unparseable, or + // The pre-persist registration hook refused the number (unparsable, or // carrier metadata failed validation). A policy answer, not a bug: it // must not fall into the catch-all that tells the user to retry. case "PhoneNotAllowedForRegistrationError": diff --git a/test/flash/unit/servers/event-handlers/kratos-preregistration-route.spec.ts b/test/flash/unit/servers/event-handlers/kratos-preregistration-route.spec.ts index 0f3d8dab7..3c809e795 100644 --- a/test/flash/unit/servers/event-handlers/kratos-preregistration-route.spec.ts +++ b/test/flash/unit/servers/event-handlers/kratos-preregistration-route.spec.ts @@ -98,7 +98,7 @@ describe("POST /kratos/preregistration", () => { ) }) - it("answers 400 'not allowed' for an unparseable phone", async () => { + it("answers 400 'not allowed' for an unparsable phone", async () => { mockValidate.mockResolvedValue(new InvalidPhoneNumber()) const res = makeRes() From 6f55b1a418176974a62bb0bc2a989bbd3071b096 Mon Sep 17 00:00:00 2001 From: Dread Date: Tue, 1 Sep 2026 17:23:04 -0700 Subject: [PATCH 3/5] fix(auth): address review on the pre-persist registration hook - PhoneAlreadyRegisteredError: the hook's "already registered" id no longer maps to PhoneAlreadyExistsError, whose GraphQL text ("one phone per account") was written for the add-phone flow. Sign-up callers have no account; the new error reads "This phone number is already registered. Contact support if you can't sign in". - RegistrationHookFailedError (Critical, KratosError): ids 4100003/4100401/ 4100500 mean Kratos config and the api disagree or infra is down. They now land in the unexpected-error catch-all under that name instead of being reported as a phone-policy answer or as "user already exists". - Hook route: RegistrationPayloadValidationError logs at error, not warn. - Carrier errors now extend PhoneMetadataValidationError, so the route's parent-class case is real instead of dead. - kratos-hook-messages: state the actual collision argument (Kratos v1.0.0 text/id.go allocates 10000-wide blocks up to 4070000 and 5000000; nothing at 4100000), and pin the block in a test. - validate-preregistration-payload.spec: assert the repository's update is never called instead of inspecting the mock's own shape. - Drop the duplicate RegistrationPayloadValidator describe; the phoneMetadata round-trip and invalid-carrier cases move to its own spec. - Integration: drive the real self-service registration against the compose Kratos with the hook routes served from jest. Asserts a phone bound to a stale users doc and invalid carrier metadata are refused with the mapped domain error and leave no identity behind, and that an accepted sign-up still gets its account from the post-persist hook. Adds kratos to integration-deps and a bats-tests network alias so Kratos can reach the suite in CI. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01EtQzyzQ38thfC2F8BLK4u8 --- docker-compose.yml | 9 + .../validate-preregistration-payload.ts | 4 +- src/domain/authentication/errors.ts | 7 +- .../authentication/kratos-hook-messages.ts | 18 +- src/domain/users/errors.ts | 6 +- src/graphql/error-map.ts | 9 + src/servers/event-handlers/kratos.ts | 22 +-- src/services/kratos/errors.ts | 9 + .../kratos/registration-flow-error.ts | 29 ++- .../kratos-preregistration-hook.spec.ts | 174 ++++++++++++++++++ .../validate-preregistration-payload.spec.ts | 26 ++- .../kratos-hook-messages.spec.ts | 10 + .../preregistration-payload-validator.spec.ts | 72 +++----- .../registration-payload-validator.spec.ts | 47 +++++ test/flash/unit/graphql/error-map.spec.ts | 27 ++- .../kratos-preregistration-route.spec.ts | 85 +++++++-- .../kratos/registration-flow-error.spec.ts | 63 +++++-- 17 files changed, 503 insertions(+), 114 deletions(-) create mode 100644 test/flash/integration/authentication/kratos-preregistration-hook.spec.ts diff --git a/docker-compose.yml b/docker-compose.yml index e33ba70a8..ec3880c02 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -39,6 +39,7 @@ services: depends_on: - redis - mongodb + - kratos # - bitcoind # - bitcoind-signer # - stablesats @@ -177,6 +178,14 @@ services: - ${TMP_ENV_CI:-.env.ci} volumes: - ./:/repo + # dev/ory/kratos.yml points the registration web hooks at + # http://bats-tests:4012 (the bats container; host-gateway under the local + # override). The integration suite serves those hooks from inside jest, so + # Kratos must resolve that name to this container too. + networks: + default: + aliases: + - bats-tests oathkeeper: image: oryd/oathkeeper:v0.40.4-distroless ports: [] diff --git a/src/app/authentication/validate-preregistration-payload.ts b/src/app/authentication/validate-preregistration-payload.ts index 6a27aba40..63282c9bb 100644 --- a/src/app/authentication/validate-preregistration-payload.ts +++ b/src/app/authentication/validate-preregistration-payload.ts @@ -2,7 +2,7 @@ import { KRATOS_CALLBACK_API_KEY } from "@config" import { CallbackSecretValidator } from "@domain/authentication/secret-validator" import { PreRegistrationPayloadValidator } from "@domain/authentication/registration-payload-validator" -import { PhoneAlreadyExistsError } from "@domain/authentication/errors" +import { PhoneAlreadyRegisteredError } from "@domain/authentication/errors" import { CouldNotFindUserFromPhoneError } from "@domain/errors" import { addAttributesToCurrentSpan } from "@services/tracing" @@ -52,5 +52,5 @@ export const validatePreRegistrationPayload = async ({ if (existing instanceof CouldNotFindUserFromPhoneError) return true if (existing instanceof Error) return existing - return new PhoneAlreadyExistsError() + return new PhoneAlreadyRegisteredError() } diff --git a/src/domain/authentication/errors.ts b/src/domain/authentication/errors.ts index 64b1a4cee..da226a2d5 100644 --- a/src/domain/authentication/errors.ts +++ b/src/domain/authentication/errors.ts @@ -6,8 +6,13 @@ export class LikelyUserAlreadyExistError extends AuthenticationError {} export class AccountHasPositiveBalanceError extends AuthenticationError {} export class PhoneAlreadyExistsError extends AuthenticationError {} +// The pre-persist registration hook found the phone already bound to a users +// document. Raised on the sign-up path, where the caller has no account and no +// session — so it must not read as "one phone per account" +// (PhoneAlreadyExistsError belongs to the add-phone-to-account flow). +export class PhoneAlreadyRegisteredError extends AuthenticationError {} // The pre-persist registration hook rejected the phone (unparsable, or its -// carrier metadata failed validation). Distinct from "already exists": the +// carrier metadata failed validation). Distinct from "already registered": the // user cannot fix it by logging in instead. export class PhoneNotAllowedForRegistrationError extends AuthenticationError {} diff --git a/src/domain/authentication/kratos-hook-messages.ts b/src/domain/authentication/kratos-hook-messages.ts index 81a75ff46..7920a45b7 100644 --- a/src/domain/authentication/kratos-hook-messages.ts +++ b/src/domain/authentication/kratos-hook-messages.ts @@ -3,10 +3,20 @@ // A web_hook configured with `response.parse: true` runs BEFORE Kratos persists // the identity (selfservice/hook/web_hook.go, ExecutePostRegistrationPrePersistHook). // Answering it with a 4xx and this body aborts the registration and surfaces -// the messages on the flow; nothing is written. The ids are Flash-private -// (Kratos' own message ids live below 5000000 in text/message_*.go, so a -// dedicated 41xxxxx block cannot collide) and are what the api matches on when -// Kratos hands the rejected flow back to it — never the text. +// the messages on the flow; nothing is written. The ids are Flash-private and +// are what the api matches on when Kratos hands the rejected flow back to it — +// never the text. +// +// Why 4100000: Kratos allocates its message ids in 10000-wide blocks +// (text/id.go, v1.0.0) — info 1010000 login, 1020000 logout, 1030000 mfa, +// 1040000 registration, 1050000 settings, 1060000 recovery, 1070000 node +// labels, 1080000 verification; validation errors 4000000 generic, 4010000 +// login, 4040000 registration, 4050000 settings, 4060000 recovery, 4070000 +// verification; and 5000000 for system errors. Nothing is allocated at +// 4100000, so ids in [4100000, 4110000) cannot be mistaken for one of Kratos' +// own when the api reads a rejected flow. Any id inside a Kratos block (e.g. +// 4040099) could collide with a future Kratos release; re-check text/id.go when +// bumping the Kratos image. export const KratosHookMessageId = { PhoneNotAllowed: 4100001, diff --git a/src/domain/users/errors.ts b/src/domain/users/errors.ts index b55369b83..b1a33b609 100644 --- a/src/domain/users/errors.ts +++ b/src/domain/users/errors.ts @@ -13,6 +13,6 @@ export class InvalidPhoneMetadataForOnboardingError extends UnauthorizedPhoneErr } export class PhoneMetadataValidationError extends ValidationError {} -export class InvalidCarrierForPhoneMetadataError extends ValidationError {} -export class InvalidCarrierTypeForPhoneMetadataError extends ValidationError {} -export class InvalidCountryCodeForPhoneMetadataError extends ValidationError {} +export class InvalidCarrierForPhoneMetadataError extends PhoneMetadataValidationError {} +export class InvalidCarrierTypeForPhoneMetadataError extends PhoneMetadataValidationError {} +export class InvalidCountryCodeForPhoneMetadataError extends PhoneMetadataValidationError {} diff --git a/src/graphql/error-map.ts b/src/graphql/error-map.ts index aa8aa0bcb..5357d244f 100644 --- a/src/graphql/error-map.ts +++ b/src/graphql/error-map.ts @@ -534,6 +534,14 @@ export const mapError = (error: ApplicationError): CustomApolloError => { message = "This phone number can't be used to sign up" return new ValidationInternalError({ message, logger: baseLogger }) + // Same hook, phone already bound to a users document. The caller is on the + // sign-up path with no account and no session, so this must not reuse + // PhoneAlreadyExistsError's "one phone per account" text. + case "PhoneAlreadyRegisteredError": + message = + "This phone number is already registered. Contact support if you can't sign in" + return new ValidationInternalError({ message, logger: baseLogger }) + case "EmailAlreadyExistsError": return new EmailAlreadyExistsError({ logger: baseLogger }) @@ -953,6 +961,7 @@ export const mapError = (error: ApplicationError): CustomApolloError => { case "MissingTotpKratosError": case "IncompatibleSchemaUpgradeError": case "UnknownKratosError": + case "RegistrationHookFailedError": case "BriaEventError": case "BriaPayloadError": case "KratosError": diff --git a/src/servers/event-handlers/kratos.ts b/src/servers/event-handlers/kratos.ts index 4bbb99425..10fa450c0 100644 --- a/src/servers/event-handlers/kratos.ts +++ b/src/servers/event-handlers/kratos.ts @@ -8,7 +8,7 @@ import { maskPhone } from "@services/alerts/ops-events" import { Authentication } from "@app" import { - PhoneAlreadyExistsError, + PhoneAlreadyRegisteredError, RegistrationPayloadValidationError, SecretForAuthNCallbackError, } from "@domain/authentication/errors" @@ -17,12 +17,7 @@ import { kratosHookRejection, } from "@domain/authentication/kratos-hook-messages" import { InvalidPhoneNumber } from "@domain/errors" -import { - InvalidCarrierForPhoneMetadataError, - InvalidCarrierTypeForPhoneMetadataError, - InvalidCountryCodeForPhoneMetadataError, - PhoneMetadataValidationError, -} from "@domain/users/errors" +import { PhoneMetadataValidationError } from "@domain/users/errors" const errorResponseMessages: { [key: string]: string } = { MissingSecretForAuthNCallbackError: "missing authorization header", @@ -67,26 +62,27 @@ kratosCallback.post( res.status(401).json(kratosHookRejection(KratosHookMessageId.Unauthorized)) return - case result instanceof PhoneAlreadyExistsError: + case result instanceof PhoneAlreadyRegisteredError: baseLogger.warn({ phone, rejection: result.name }, "preregistration rejected") res .status(400) .json(kratosHookRejection(KratosHookMessageId.PhoneAlreadyRegistered)) return + // Every carrier-metadata failure extends PhoneMetadataValidationError. case result instanceof InvalidPhoneNumber: case result instanceof PhoneMetadataValidationError: - case result instanceof InvalidCarrierForPhoneMetadataError: - case result instanceof InvalidCarrierTypeForPhoneMetadataError: - case result instanceof InvalidCountryCodeForPhoneMetadataError: baseLogger.warn({ phone, rejection: result.name }, "preregistration rejected") res.status(400).json(kratosHookRejection(KratosHookMessageId.PhoneNotAllowed)) return + // Not a user error: the body Kratos' template produced does not match + // what the api expects (missing field, unknown schema_id). Kratos + // config and the api disagree, so every sign-up is failing. case result instanceof RegistrationPayloadValidationError: - baseLogger.warn( + baseLogger.error( { phone, rejection: result.name, schemaId: body.schema_id }, - "preregistration rejected", + "preregistration: hook payload rejected, Kratos config and api disagree", ) res.status(400).json(kratosHookRejection(KratosHookMessageId.PayloadInvalid)) return diff --git a/src/services/kratos/errors.ts b/src/services/kratos/errors.ts index bce7636ce..fbc164028 100644 --- a/src/services/kratos/errors.ts +++ b/src/services/kratos/errors.ts @@ -46,3 +46,12 @@ export class CodeExpiredKratosError extends KratosError {} export class UnknownKratosError extends KratosError { level = ErrorLevel.Critical } + +// The pre-persist registration hook answered with an id that is never a +// phone-policy answer: the hook payload Kratos sent was malformed, the callback +// secret did not match, or the api could not reach its repository. Kratos +// config and the api disagree, or infra is down — a deploy defect, not a user +// error, so it must never be reported as one. +export class RegistrationHookFailedError extends KratosError { + level = ErrorLevel.Critical +} diff --git a/src/services/kratos/registration-flow-error.ts b/src/services/kratos/registration-flow-error.ts index bf19ef773..ac9d303de 100644 --- a/src/services/kratos/registration-flow-error.ts +++ b/src/services/kratos/registration-flow-error.ts @@ -1,6 +1,6 @@ import { LikelyUserAlreadyExistError, - PhoneAlreadyExistsError, + PhoneAlreadyRegisteredError, PhoneNotAllowedForRegistrationError, } from "@domain/authentication/errors" import { @@ -8,9 +8,21 @@ import { kratosHookMessageIdsFromFlow, } from "@domain/authentication/kratos-hook-messages" +import { RegistrationHookFailedError } from "./errors" + const isStatus400 = (err: unknown): boolean => err instanceof Error && err.message === "Request failed with status code 400" +// Ids the hook answers with when it could not evaluate the sign-up at all: +// malformed hook payload (Kratos' body template and the api disagree), bad +// callback secret, repository failure. None of them says anything about the +// phone, so none may be reported as a phone-policy answer. +const HOOK_FAILURE_IDS: ReadonlySet = new Set([ + KratosHookMessageId.PayloadInvalid, + KratosHookMessageId.Unauthorized, + KratosHookMessageId.InternalError, +]) + // Kratos answers a self-service registration with 400 for two very different // reasons: the identifier already exists, or the pre-persist web hook rejected // the sign-up. The rejected flow carries our own message ids; anything else @@ -18,8 +30,9 @@ const isStatus400 = (err: unknown): boolean => export const mapRegistrationFlowRejection = ( err: unknown, ): - | PhoneAlreadyExistsError + | PhoneAlreadyRegisteredError | PhoneNotAllowedForRegistrationError + | RegistrationHookFailedError | LikelyUserAlreadyExistError | null => { if (!isStatus400(err)) return null @@ -28,15 +41,17 @@ export const mapRegistrationFlowRejection = ( const ids = kratosHookMessageIdsFromFlow(data) if (ids.includes(KratosHookMessageId.PhoneAlreadyRegistered)) { - return new PhoneAlreadyExistsError() + return new PhoneAlreadyRegisteredError() } - if ( - ids.includes(KratosHookMessageId.PhoneNotAllowed) || - ids.includes(KratosHookMessageId.PayloadInvalid) - ) { + if (ids.includes(KratosHookMessageId.PhoneNotAllowed)) { return new PhoneNotAllowedForRegistrationError() } + const failureId = ids.find((id) => HOOK_FAILURE_IDS.has(id)) + if (failureId !== undefined) { + return new RegistrationHookFailedError(`hook message id ${failureId}`) + } + return new LikelyUserAlreadyExistError((err as Error).message) } diff --git a/test/flash/integration/authentication/kratos-preregistration-hook.spec.ts b/test/flash/integration/authentication/kratos-preregistration-hook.spec.ts new file mode 100644 index 000000000..20eab3155 --- /dev/null +++ b/test/flash/integration/authentication/kratos-preregistration-hook.spec.ts @@ -0,0 +1,174 @@ +/** + * The unit specs around the pre-persist registration hook each mock the other + * side: the route spec mocks the app, the app spec mocks Mongo, the flow-error + * spec hand-builds the Kratos 400. None of them executes the one claim the + * hook exists for — that a rejected sign-up leaves no Kratos identity behind — + * and that claim hinges on Kratos internals: `response.parse: true` running + * the hook BEFORE persist (selfservice/hook/web_hook.go), and the rejection + * messages landing where kratosHookMessageIdsFromFlow reads them. + * + * This drives the real self-service registration flow against the compose + * Kratos (v1.0.0, dev/ory mounted) with the api's hook routes served from this + * process on the port dev/ory/kratos.yml targets, so it can return "no" about: + * + * - a phone already bound to a users document (the DuplicateKey source) is + * refused as PhoneAlreadyRegisteredError and no identity exists afterwards; + * - carrier metadata the api does not accept — carried through Kratos' + * transient_payload and body.jsonnet — is refused as + * PhoneNotAllowedForRegistrationError, again with nothing persisted; + * - an accepted sign-up still gets its account from the post-persist hook. + */ +import { Server } from "http" + +import axios from "axios" +import express from "express" + +import { GALOY_API_PORT, KRATOS_PUBLIC_API } from "@config" +import { + PhoneAlreadyRegisteredError, + PhoneNotAllowedForRegistrationError, +} from "@domain/authentication/errors" +import { CouldNotFindUserFromPhoneError } from "@domain/errors" +import { CarrierType } from "@domain/phone-provider" +import kratosCallback from "@servers/event-handlers/kratos" +import { AuthWithPhonePasswordlessService } from "@services/kratos" +import { kratosAdmin } from "@services/kratos/private" +import { AccountsRepository, UsersRepository } from "@services/mongoose" + +import { randomPhone, randomUserId } from "test/galoy/helpers" + +const validMetadata: PhoneMetadata = { + carrier: { + error_code: "", + mobile_country_code: "310", + mobile_network_code: "260", + name: "T-Mobile USA", + type: CarrierType.Mobile, + }, + countryCode: "US", +} + +// Raw admin lookup, independent of the api's IdentityRepository, so the +// assertion is about what Kratos stored and not about how the api reads it. +const identityIdsFor = async (phone: PhoneNumber): Promise => { + const { data } = await kratosAdmin.listIdentities({ credentialsIdentifier: phone }) + return data.map((identity) => identity.id) +} + +// Kratos migrates its schema on first boot; depends_on only waits for the +// container to start. +const waitForKratos = async (): Promise => { + const deadline = Date.now() + 120_000 + let lastError: unknown + while (Date.now() < deadline) { + try { + await axios.get(`${KRATOS_PUBLIC_API}/health/ready`, { timeout: 2000 }) + return + } catch (err) { + lastError = err + } + await new Promise((resolve) => setTimeout(resolve, 1000)) + } + throw new Error(`Kratos at ${KRATOS_PUBLIC_API} never became ready: ${lastError}`) +} + +// dev/ory/kratos.yml calls http://bats-tests:4012/kratos/{preregistration, +// registration}. Locally that name is the host gateway; in the integration +// container it is a network alias for this container. Either way the hooks +// must be answered by the code under test, from this process. +const serveHooks = (): Promise => + new Promise((resolve, reject) => { + const app = express() + app.use("/kratos", kratosCallback) + const server = app.listen(GALOY_API_PORT, "0.0.0.0") + server.once("listening", () => resolve(server)) + server.once("error", (err: NodeJS.ErrnoException) => { + reject( + err.code === "EADDRINUSE" + ? new Error( + `port ${GALOY_API_PORT} is already bound (a running api?). Kratos would ` + + "call that process instead of this suite; stop it and re-run.", + ) + : err, + ) + }) + }) + +describe("Kratos self-service registration through the pre-persist hook", () => { + const authService = AuthWithPhonePasswordlessService() + let hooks: Server + + beforeAll(async () => { + await waitForKratos() + hooks = await serveHooks() + }, 150_000) + + afterAll(async () => { + if (hooks) await new Promise((resolve) => hooks.close(() => resolve())) + }) + + it("persists the identity and the post-persist hook creates the account when nothing rejects", async () => { + const phone = randomPhone() + + const result = await authService.createIdentityWithSession({ + phone, + phoneMetadata: validMetadata, + }) + if (result instanceof Error) throw result + + expect(result.authToken).toEqual(expect.any(String)) + expect(await identityIdsFor(phone)).toStrictEqual([result.kratosUserId]) + + const user = await UsersRepository().findByPhone(phone) + if (user instanceof Error) throw user + expect(user.id).toBe(result.kratosUserId) + // transient_payload survived body.jsonnet into the post-persist hook. Only + // countryCode is asserted: the users schema's `carrier.type: { types, enum }` + // (a pre-existing typo for `type`) stores carrier as `{ _id, enum: [] }`, + // which is a schema defect outside this hook's scope. + expect(user.phoneMetadata?.countryCode).toBe(validMetadata.countryCode) + + const account = await AccountsRepository().findByUserId(result.kratosUserId) + expect(account).not.toBeInstanceOf(Error) + }) + + it("refuses a phone already bound to a users document and leaves no identity behind", async () => { + const phone = randomPhone() + const stale = await UsersRepository().update({ + id: randomUserId(), + phone, + deviceTokens: [] as DeviceToken[], + }) + if (stale instanceof Error) throw stale + + const result = await authService.createIdentityWithSession({ + phone, + phoneMetadata: validMetadata, + }) + + expect(result).toBeInstanceOf(PhoneAlreadyRegisteredError) + expect(await identityIdsFor(phone)).toStrictEqual([]) + + // The stale document was read, not rewritten: still the seeded id. + const user = await UsersRepository().findByPhone(phone) + if (user instanceof Error) throw user + expect(user.id).toBe(stale.id) + }) + + it("refuses carrier metadata the api does not accept and leaves nothing behind on either side", async () => { + const phone = randomPhone() + const phoneMetadata = { + ...validMetadata, + carrier: { ...validMetadata.carrier, type: "" }, + } as unknown as PhoneMetadata + + const result = await authService.createIdentityWithSession({ phone, phoneMetadata }) + + expect(result).toBeInstanceOf(PhoneNotAllowedForRegistrationError) + expect(await identityIdsFor(phone)).toStrictEqual([]) + // No identity means the post-persist hook never ran either. + expect(await UsersRepository().findByPhone(phone)).toBeInstanceOf( + CouldNotFindUserFromPhoneError, + ) + }) +}) diff --git a/test/flash/unit/app/authentication/validate-preregistration-payload.spec.ts b/test/flash/unit/app/authentication/validate-preregistration-payload.spec.ts index 822ae95b0..88c82f9c4 100644 --- a/test/flash/unit/app/authentication/validate-preregistration-payload.spec.ts +++ b/test/flash/unit/app/authentication/validate-preregistration-payload.spec.ts @@ -2,7 +2,7 @@ import { validatePreRegistrationPayload } from "@app/authentication/validate-pre import { InvalidSecretForAuthNCallbackError, MissingSecretForAuthNCallbackError, - PhoneAlreadyExistsError, + PhoneAlreadyRegisteredError, UnsupportedSchemaTypeError, } from "@domain/authentication/errors" import { CouldNotFindUserFromPhoneError, UnknownRepositoryError } from "@domain/errors" @@ -26,8 +26,14 @@ jest.mock("@services/kratos", () => ({ })) const mockFindByPhone = jest.fn() +const mockFindById = jest.fn() +const mockUpdate = jest.fn() jest.mock("@services/mongoose", () => ({ - UsersRepository: jest.fn(() => ({ findByPhone: mockFindByPhone })), + UsersRepository: jest.fn(() => ({ + findById: mockFindById, + findByPhone: mockFindByPhone, + update: mockUpdate, + })), })) const NIL_UUID = "00000000-0000-0000-0000-000000000000" @@ -53,6 +59,8 @@ const body = (overrides: Record = {}) => ({ describe("validatePreRegistrationPayload", () => { beforeEach(() => { mockFindByPhone.mockReset() + mockFindById.mockReset() + mockUpdate.mockReset() mockFindByPhone.mockResolvedValue(new CouldNotFindUserFromPhoneError()) }) @@ -68,14 +76,22 @@ describe("validatePreRegistrationPayload", () => { }) it("never writes and never looks anything up by the nil identity id", async () => { - const repo = UsersRepository() as unknown as Record await validatePreRegistrationPayload({ secret: CALLBACK_KEY, body: body() }) - expect(Object.keys(repo)).toStrictEqual(["findByPhone"]) + expect(mockUpdate).not.toHaveBeenCalled() + expect(mockFindById).not.toHaveBeenCalled() expect(mockFindByPhone).toHaveBeenCalledTimes(1) expect(mockFindByPhone.mock.calls[0][0]).not.toBe(NIL_UUID) }) + it("does not write even when it rejects", async () => { + mockFindByPhone.mockResolvedValue({ id: "other-user", phone }) + + await validatePreRegistrationPayload({ secret: CALLBACK_KEY, body: body() }) + + expect(mockUpdate).not.toHaveBeenCalled() + }) + it("rejects a missing or wrong secret before touching the payload", async () => { expect( await validatePreRegistrationPayload({ secret: undefined, body: body() }), @@ -117,7 +133,7 @@ describe("validatePreRegistrationPayload", () => { body: body(), }) - expect(result).toBeInstanceOf(PhoneAlreadyExistsError) + expect(result).toBeInstanceOf(PhoneAlreadyRegisteredError) }) it("surfaces repository failures so the route answers 500 and the sign-up aborts", async () => { diff --git a/test/flash/unit/domain/authentication/kratos-hook-messages.spec.ts b/test/flash/unit/domain/authentication/kratos-hook-messages.spec.ts index 79c78568a..70f6c1040 100644 --- a/test/flash/unit/domain/authentication/kratos-hook-messages.spec.ts +++ b/test/flash/unit/domain/authentication/kratos-hook-messages.spec.ts @@ -25,6 +25,16 @@ describe("kratos hook messages", () => { expect(KRATOS_HOOK_PHONE_INSTANCE_PTR).toBe("#/traits/phone") }) + it("keeps every id inside the 4100000 block no Kratos allocation touches", () => { + // text/id.go (Kratos v1.0.0): info blocks 1010000..1080000, validation + // blocks 4000000, 4010000, 4040000, 4050000, 4060000, 4070000, system + // 5000000 — each 10000 wide. 4100000 is the first free block above them. + for (const id of Object.values(KratosHookMessageId)) { + expect(id).toBeGreaterThanOrEqual(4100000) + expect(id).toBeLessThan(4110000) + } + }) + it("has a distinct id and user-readable text for every rejection kind", () => { const ids = Object.values(KratosHookMessageId) expect(new Set(ids).size).toBe(ids.length) diff --git a/test/flash/unit/domain/authentication/preregistration-payload-validator.spec.ts b/test/flash/unit/domain/authentication/preregistration-payload-validator.spec.ts index 29b59dd3b..ca7380355 100644 --- a/test/flash/unit/domain/authentication/preregistration-payload-validator.spec.ts +++ b/test/flash/unit/domain/authentication/preregistration-payload-validator.spec.ts @@ -2,20 +2,18 @@ import { MissingRegistrationPayloadPropertiesError, UnsupportedSchemaTypeError, } from "@domain/authentication/errors" -import { - PreRegistrationPayloadValidator, - RegistrationPayloadValidator, -} from "@domain/authentication/registration-payload-validator" +import { PreRegistrationPayloadValidator } from "@domain/authentication/registration-payload-validator" import { InvalidPhoneNumber } from "@domain/errors" import { InvalidCarrierForPhoneMetadataError, InvalidCarrierTypeForPhoneMetadataError, InvalidCountryCodeForPhoneMetadataError, + PhoneMetadataValidationError, } from "@domain/users/errors" import { SchemaIdType } from "@services/kratos" -import { randomPhone, randomUserId } from "test/galoy/helpers/random" +import { randomPhone } from "test/galoy/helpers/random" // Kratos assigns the identity id only at persist time; the pre-persist hook // receives this placeholder and the validator must never look at it. @@ -116,55 +114,29 @@ describe("PreRegistrationPayloadValidator", () => { }, }) expect(validated).toBeInstanceOf(InvalidCarrierTypeForPhoneMetadataError) + // The hook route matches on the parent class. + expect(validated).toBeInstanceOf(PhoneMetadataValidationError) }) it("rejects malformed carrier metadata", () => { - expect( - validator.validate({ - phone: randomPhone(), - schema_id: SchemaIdType.PhoneNoPasswordV0, - transient_payload: { - phoneMetadata: { carrier: "not-an-object", countryCode: "JM" } as never, - }, - }), - ).toBeInstanceOf(InvalidCarrierForPhoneMetadataError) - - expect( - validator.validate({ - phone: randomPhone(), - schema_id: SchemaIdType.PhoneNoPasswordV0, - transient_payload: { - phoneMetadata: { carrier: validCarrier } as never, - }, - }), - ).toBeInstanceOf(InvalidCountryCodeForPhoneMetadataError) - }) -}) - -describe("RegistrationPayloadValidator (post-persist hook) is unchanged", () => { - const validator = RegistrationPayloadValidator(SchemaIdType.PhoneNoPasswordV0) - - it("still requires a real identity id", () => { - const rawUserId = randomUserId() - const phone = randomPhone() - - expect( - validator.validate({ - identity_id: rawUserId, - phone, - schema_id: SchemaIdType.PhoneNoPasswordV0, - transient_payload: { - phoneMetadata: { carrier: validCarrier, countryCode: "JM" }, - }, - }), - ).toStrictEqual({ - userId: rawUserId, - phone, - phoneMetadata: { carrier: validCarrier, countryCode: "JM" }, + const badCarrier = validator.validate({ + phone: randomPhone(), + schema_id: SchemaIdType.PhoneNoPasswordV0, + transient_payload: { + phoneMetadata: { carrier: "not-an-object", countryCode: "JM" } as never, + }, }) + expect(badCarrier).toBeInstanceOf(InvalidCarrierForPhoneMetadataError) + expect(badCarrier).toBeInstanceOf(PhoneMetadataValidationError) - expect( - validator.validate({ phone, schema_id: SchemaIdType.PhoneNoPasswordV0 }), - ).toBeInstanceOf(MissingRegistrationPayloadPropertiesError) + const noCountry = validator.validate({ + phone: randomPhone(), + schema_id: SchemaIdType.PhoneNoPasswordV0, + transient_payload: { + phoneMetadata: { carrier: validCarrier } as never, + }, + }) + expect(noCountry).toBeInstanceOf(InvalidCountryCodeForPhoneMetadataError) + expect(noCountry).toBeInstanceOf(PhoneMetadataValidationError) }) }) diff --git a/test/flash/unit/domain/authentication/registration-payload-validator.spec.ts b/test/flash/unit/domain/authentication/registration-payload-validator.spec.ts index 12af6861d..475abc619 100644 --- a/test/flash/unit/domain/authentication/registration-payload-validator.spec.ts +++ b/test/flash/unit/domain/authentication/registration-payload-validator.spec.ts @@ -4,6 +4,7 @@ import { } from "@domain/authentication/errors" import { RegistrationPayloadValidator } from "@domain/authentication/registration-payload-validator" import { InvalidPhoneNumber, InvalidUserId } from "@domain/errors" +import { InvalidCarrierTypeForPhoneMetadataError } from "@domain/users/errors" import { SchemaIdType } from "@services/kratos" @@ -30,6 +31,52 @@ describe("RegistrationPayloadValidator", () => { expect(validated).toStrictEqual(expectedPayload) }) + it("carries validated phone metadata through", () => { + const rawUserId = randomUserId() + const rawPhone = randomPhone() + const carrier = { + error_code: "", + mobile_country_code: "338", + mobile_network_code: "050", + name: "Digicel", + type: "mobile", + } + + const validated = validator.validate({ + identity_id: rawUserId, + phone: rawPhone, + schema_id: SchemaIdType.PhoneNoPasswordV0, + transient_payload: { phoneMetadata: { carrier, countryCode: "JM" } }, + }) + + expect(validated).toStrictEqual({ + userId: rawUserId, + phone: rawPhone, + phoneMetadata: { carrier, countryCode: "JM" }, + }) + }) + + it("returns invalid carrier type error", () => { + const validated = validator.validate({ + identity_id: randomUserId(), + phone: randomPhone(), + schema_id: SchemaIdType.PhoneNoPasswordV0, + transient_payload: { + phoneMetadata: { + carrier: { + error_code: "", + mobile_country_code: "", + mobile_network_code: "", + name: "", + type: "", + }, + countryCode: "NG", + }, + }, + }) + expect(validated).toBeInstanceOf(InvalidCarrierTypeForPhoneMetadataError) + }) + it("returns missing inputs error", () => { const identity_id = "identity_id" const phone = "phone" diff --git a/test/flash/unit/graphql/error-map.spec.ts b/test/flash/unit/graphql/error-map.spec.ts index 606d27336..33823b226 100644 --- a/test/flash/unit/graphql/error-map.spec.ts +++ b/test/flash/unit/graphql/error-map.spec.ts @@ -8,7 +8,11 @@ import { } from "@services/bridge/errors" import { IbexError, InsufficientIbexBalance } from "@services/ibex/errors" import { PhoneCountryNotAllowedError } from "@domain/users/errors" -import { PhoneNotAllowedForRegistrationError } from "@domain/authentication/errors" +import { + PhoneAlreadyRegisteredError, + PhoneNotAllowedForRegistrationError, +} from "@domain/authentication/errors" +import { RegistrationHookFailedError } from "@services/kratos/errors" import { InvalidPhoneNumber } from "@domain/errors" describe("error-map", () => { @@ -19,6 +23,27 @@ describe("error-map", () => { expect(result.message).toBe("This phone number can't be used to sign up") }) + it("maps PhoneAlreadyRegisteredError to sign-up wording, not the add-phone PHONE_ALREADY_ATTACHED_ERROR", () => { + const result = mapError(new PhoneAlreadyRegisteredError()) + + expect(result.extensions.code).toBe("INVALID_INPUT") + expect(result.extensions.code).not.toBe("PHONE_ALREADY_ATTACHED_ERROR") + expect(result.message).toBe( + "This phone number is already registered. Contact support if you can't sign in", + ) + expect(result.message).not.toMatch(/this account/) + }) + + it("maps RegistrationHookFailedError to the unexpected-error catch-all under its own name", () => { + const result = mapError(new RegistrationHookFailedError("hook message id 4100500")) + + expect(result.extensions.code).toBe("UNEXPECTED_CLIENT_ERROR") + expect(result.message).toContain( + "RegistrationHookFailedError: hook message id 4100500", + ) + expect(result.message).not.toContain("LikelyUserAlreadyExistError") + }) + it("maps BridgeWithdrawalNotFoundError to BRIDGE_WITHDRAWAL_NOT_FOUND", () => { const result = mapError(new BridgeWithdrawalNotFoundError()) diff --git a/test/flash/unit/servers/event-handlers/kratos-preregistration-route.spec.ts b/test/flash/unit/servers/event-handlers/kratos-preregistration-route.spec.ts index 3c809e795..360235740 100644 --- a/test/flash/unit/servers/event-handlers/kratos-preregistration-route.spec.ts +++ b/test/flash/unit/servers/event-handlers/kratos-preregistration-route.spec.ts @@ -4,11 +4,18 @@ import { Authentication } from "@app" import { InvalidSecretForAuthNCallbackError, MissingRegistrationPayloadPropertiesError, - PhoneAlreadyExistsError, + PhoneAlreadyRegisteredError, + UnsupportedSchemaTypeError, } from "@domain/authentication/errors" import { InvalidPhoneNumber, UnknownRepositoryError } from "@domain/errors" -import { InvalidCarrierTypeForPhoneMetadataError } from "@domain/users/errors" +import { + InvalidCarrierForPhoneMetadataError, + InvalidCarrierTypeForPhoneMetadataError, + InvalidCountryCodeForPhoneMetadataError, + PhoneMetadataValidationError, +} from "@domain/users/errors" import kratosCallback from "@servers/event-handlers/kratos" +import { baseLogger } from "@services/logger" jest.mock("@app", () => ({ Authentication: { @@ -86,17 +93,26 @@ describe("POST /kratos/preregistration", () => { expect(res.send).not.toHaveBeenCalled() }) - it("answers 400 with the Kratos messages body on a carrier-type rejection", async () => { - mockValidate.mockResolvedValue(new InvalidCarrierTypeForPhoneMetadataError()) - const res = makeRes() - - await handler(makeReq(), res) - - expect(res.status).toHaveBeenCalledWith(400) - expect(res.json).toHaveBeenCalledWith( - rejection(4100001, "This phone number can't be used to sign up."), - ) - }) + it.each([ + ["carrier type", new InvalidCarrierTypeForPhoneMetadataError()], + ["carrier shape", new InvalidCarrierForPhoneMetadataError()], + ["country code", new InvalidCountryCodeForPhoneMetadataError()], + ])( + "answers 400 with the Kratos messages body on a %s rejection", + async (_label, err) => { + // The route matches the parent class; every carrier error must be one. + expect(err).toBeInstanceOf(PhoneMetadataValidationError) + mockValidate.mockResolvedValue(err) + const res = makeRes() + + await handler(makeReq(), res) + + expect(res.status).toHaveBeenCalledWith(400) + expect(res.json).toHaveBeenCalledWith( + rejection(4100001, "This phone number can't be used to sign up."), + ) + }, + ) it("answers 400 'not allowed' for an unparsable phone", async () => { mockValidate.mockResolvedValue(new InvalidPhoneNumber()) @@ -111,7 +127,7 @@ describe("POST /kratos/preregistration", () => { }) it("answers 400 'already registered' when the phone is bound to another identity", async () => { - mockValidate.mockResolvedValue(new PhoneAlreadyExistsError()) + mockValidate.mockResolvedValue(new PhoneAlreadyRegisteredError()) const res = makeRes() await handler(makeReq(), res) @@ -134,6 +150,47 @@ describe("POST /kratos/preregistration", () => { ) }) + it("logs a malformed hook payload at error, not warn: Kratos config and the api disagree", async () => { + const errorSpy = jest.spyOn(baseLogger, "error").mockImplementation(() => undefined) + const warnSpy = jest.spyOn(baseLogger, "warn").mockImplementation(() => undefined) + try { + mockValidate.mockResolvedValue(new UnsupportedSchemaTypeError()) + + await handler(makeReq({ schema_id: "email_no_password_v0" }), makeRes()) + + expect(errorSpy).toHaveBeenCalledWith( + expect.objectContaining({ + rejection: "UnsupportedSchemaTypeError", + schemaId: "email_no_password_v0", + }), + expect.stringContaining("Kratos config and api disagree"), + ) + expect(warnSpy).not.toHaveBeenCalled() + } finally { + errorSpy.mockRestore() + warnSpy.mockRestore() + } + }) + + it("logs a phone-policy rejection at warn: expected, not a defect", async () => { + const errorSpy = jest.spyOn(baseLogger, "error").mockImplementation(() => undefined) + const warnSpy = jest.spyOn(baseLogger, "warn").mockImplementation(() => undefined) + try { + mockValidate.mockResolvedValue(new InvalidCarrierTypeForPhoneMetadataError()) + + await handler(makeReq(), makeRes()) + + expect(warnSpy).toHaveBeenCalledWith( + expect.objectContaining({ rejection: "InvalidCarrierTypeForPhoneMetadataError" }), + "preregistration rejected", + ) + expect(errorSpy).not.toHaveBeenCalled() + } finally { + errorSpy.mockRestore() + warnSpy.mockRestore() + } + }) + it("answers 401 as JSON on a bad callback secret", async () => { mockValidate.mockResolvedValue(new InvalidSecretForAuthNCallbackError()) const res = makeRes() diff --git a/test/flash/unit/services/kratos/registration-flow-error.spec.ts b/test/flash/unit/services/kratos/registration-flow-error.spec.ts index f3778c1fb..081cb09e8 100644 --- a/test/flash/unit/services/kratos/registration-flow-error.spec.ts +++ b/test/flash/unit/services/kratos/registration-flow-error.spec.ts @@ -1,9 +1,11 @@ import { LikelyUserAlreadyExistError, PhoneAlreadyExistsError, + PhoneAlreadyRegisteredError, PhoneNotAllowedForRegistrationError, } from "@domain/authentication/errors" import { KratosHookMessageId } from "@domain/authentication/kratos-hook-messages" +import { RegistrationHookFailedError } from "@services/kratos/errors" import { mapRegistrationFlowRejection } from "@services/kratos/registration-flow-error" // Shape @ory/client (axios) throws when Kratos answers updateRegistrationFlow @@ -36,28 +38,43 @@ describe("mapRegistrationFlowRejection", () => { expect(mapRegistrationFlowRejection(undefined)).toBeNull() }) - it("maps the pre-hook 'already registered' id to PhoneAlreadyExistsError", () => { - expect( - mapRegistrationFlowRejection( - axios400(flowWithHookMessage(KratosHookMessageId.PhoneAlreadyRegistered)), - ), - ).toBeInstanceOf(PhoneAlreadyExistsError) + it("maps the pre-hook 'already registered' id to PhoneAlreadyRegisteredError, never the add-phone error", () => { + const mapped = mapRegistrationFlowRejection( + axios400(flowWithHookMessage(KratosHookMessageId.PhoneAlreadyRegistered)), + ) + + expect(mapped).toBeInstanceOf(PhoneAlreadyRegisteredError) + // PhoneAlreadyExistsError reads "one phone per account" on the GraphQL + // boundary; a caller on the sign-up path has no account. + expect(mapped).not.toBeInstanceOf(PhoneAlreadyExistsError) }) - it("maps the pre-hook 'not allowed' and 'payload invalid' ids to PhoneNotAllowedForRegistrationError", () => { + it("maps the pre-hook 'not allowed' id to PhoneNotAllowedForRegistrationError", () => { expect( mapRegistrationFlowRejection( axios400(flowWithHookMessage(KratosHookMessageId.PhoneNotAllowed)), ), ).toBeInstanceOf(PhoneNotAllowedForRegistrationError) - expect( - mapRegistrationFlowRejection( - axios400(flowWithHookMessage(KratosHookMessageId.PayloadInvalid)), - ), - ).toBeInstanceOf(PhoneNotAllowedForRegistrationError) }) - it("prefers 'already registered' when both ids are present", () => { + it.each([ + ["payload invalid", KratosHookMessageId.PayloadInvalid], + ["unauthorized", KratosHookMessageId.Unauthorized], + ["internal error", KratosHookMessageId.InternalError], + ])( + "maps the '%s' id to RegistrationHookFailedError carrying the id, not to a phone-policy answer", + (_label, id) => { + const mapped = mapRegistrationFlowRejection(axios400(flowWithHookMessage(id))) + + expect(mapped).toBeInstanceOf(RegistrationHookFailedError) + expect((mapped as Error).message).toBe(`hook message id ${id}`) + expect(mapped).not.toBeInstanceOf(PhoneNotAllowedForRegistrationError) + expect(mapped).not.toBeInstanceOf(PhoneAlreadyRegisteredError) + expect(mapped).not.toBeInstanceOf(LikelyUserAlreadyExistError) + }, + ) + + it("prefers 'already registered' when both phone ids are present", () => { const flow = { ui: { messages: [{ id: KratosHookMessageId.PhoneNotAllowed, text: "x", type: "error" }], @@ -75,7 +92,25 @@ describe("mapRegistrationFlowRejection", () => { }, } expect(mapRegistrationFlowRejection(axios400(flow))).toBeInstanceOf( - PhoneAlreadyExistsError, + PhoneAlreadyRegisteredError, + ) + }) + + it("prefers a phone-policy id over a hook-failure id when both are present", () => { + const flow = { + ui: { + messages: [{ id: KratosHookMessageId.InternalError, text: "x", type: "error" }], + nodes: [ + { + messages: [ + { id: KratosHookMessageId.PhoneNotAllowed, text: "y", type: "error" }, + ], + }, + ], + }, + } + expect(mapRegistrationFlowRejection(axios400(flow))).toBeInstanceOf( + PhoneNotAllowedForRegistrationError, ) }) From 6c6f199f783681a6b1427be062f5cefbc6f0457c Mon Sep 17 00:00:00 2001 From: Dread Date: Tue, 1 Sep 2026 17:46:34 -0700 Subject: [PATCH 4/5] fix(quickstart): splice the pre-persist registration hook into the re-rendered kratos.yml quickstart/dev/ory/kratos.yml is not derived from dev/ory/kratos.yml: re-render.sh vendir-syncs upstream galoy at the pinned ref and only rewrites the hosts, and both the Quickstart CI job and `make smoke-env-up` run `make re-render` before boot. Upstream still ships the /kratos/preregistration hook commented out, so the one environment that runs the real api container against a real Kratos kept post-persist-only hooks and kept minting the orphaned identities this branch exists to stop; a hand edit of the rendered file is overwritten on the next re-render. - quickstart/bin/splice-kratos-preregistration-hook.sh: after the host rewrite, insert the `response.parse: true` web_hook ahead of the /registration hook and retire upstream's commented-out draft. No-op when the hook is already there; exits 1 when the /registration anchor is missing or laid out differently, so a re-render can never silently drop the hook. POSIX awk/sed, verified on BSD awk (macOS) and mawk (ubuntu). - quickstart/bin/re-render.sh: call it. - quickstart/dev/ory/kratos.yml: regenerated with `make re-render` (ytt v0.55.1, vendir v0.46.1, galoy 6906f1b); only the hooks list changed. Kratos v1.0.0 boots on it. - test/flash/unit/dev/kratos-registration-hooks.spec.ts: both dev configs must run the pre-persist hook before the post-persist one and the session hook last (fails on the previous rendered file), plus the splice script's behaviour on upstream's input, idempotency and both refusal paths. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01EtQzyzQ38thfC2F8BLK4u8 --- quickstart/bin/re-render.sh | 5 + .../bin/splice-kratos-preregistration-hook.sh | 105 +++++++++ quickstart/dev/ory/kratos.yml | 36 ++- .../dev/kratos-registration-hooks.spec.ts | 209 ++++++++++++++++++ 4 files changed, 336 insertions(+), 19 deletions(-) create mode 100755 quickstart/bin/splice-kratos-preregistration-hook.sh create mode 100644 test/flash/unit/dev/kratos-registration-hooks.spec.ts diff --git a/quickstart/bin/re-render.sh b/quickstart/bin/re-render.sh index 90b306d31..93fb1e28a 100755 --- a/quickstart/bin/re-render.sh +++ b/quickstart/bin/re-render.sh @@ -42,6 +42,11 @@ rewrite_flash_quickstart_hosts() { rewrite_flash_quickstart_hosts +# Upstream galoy ships the pre-persist registration hook commented out; the +# api needs it live or rejected sign-ups leave orphaned identities (see the +# script header). Must run after the host rewrite: it anchors on the flash host. +"${REPO_ROOT}/quickstart/bin/splice-kratos-preregistration-hook.sh" dev/ory/kratos.yml + ytt -f ./docker-compose.tmpl.yml -f ${GALOY_ROOT_DIR}/docker-compose.yml -f ${GALOY_ROOT_DIR}/docker-compose.override.yml > docker-compose.yml pushd ${GALOY_ROOT_DIR} diff --git a/quickstart/bin/splice-kratos-preregistration-hook.sh b/quickstart/bin/splice-kratos-preregistration-hook.sh new file mode 100755 index 000000000..417b2c223 --- /dev/null +++ b/quickstart/bin/splice-kratos-preregistration-hook.sh @@ -0,0 +1,105 @@ +#!/bin/bash +# +# Splice the pre-persist registration web hook into a freshly vendir-synced +# quickstart/dev/ory/kratos.yml. +# +# Upstream galoy ships that hook commented out ("we are not sure if we need +# this hook yet"). The api's POST /kratos/preregistration route is what keeps a +# rejected sign-up from leaving an orphaned Kratos identity behind, and Kratos +# only calls it when told to, with `response.parse: true` (pre-persist), ahead +# of the post-persist /registration hook. The root dev/ory/kratos.yml carries +# the real entry for the integration suite; the quickstart copy is regenerated +# from upstream by re-render.sh, so a hand edit there is lost on the next +# `make re-render` -- which is why the entry is spliced in here instead. +# +# Usage: splice-kratos-preregistration-hook.sh +# Run after the bats-tests -> flash host rewrite; it anchors on the flash host. +# +# Exits 0 and leaves the file alone when an uncommented preregistration hook is +# already there. Exits 1 when the /registration hook cannot be found in the +# expected shape, so a re-render can never silently produce a config without +# the hook. test/flash/unit/dev/kratos-registration-hooks.spec.ts covers it. + +set -e +set -o pipefail + +file=${1:?usage: $0 } + +registration_url='http://flash:4012/kratos/registration' +preregistration_url='http://flash:4012/kratos/preregistration' + +if grep -Eq "^[[:space:]]*url: ${preregistration_url}[[:space:]]*\$" "${file}"; then + echo "${file}: pre-persist registration hook already present, nothing to splice" >&2 + exit 0 +fi + +# Anchor on the post-persist hook, expected as three consecutive lines: +# - hook: web_hook +# config: +# url: http://flash:4012/kratos/registration +url_lines=$(grep -nE "^[[:space:]]*url: ${registration_url}[[:space:]]*\$" "${file}" | cut -d: -f1 || true) +if [ "$(printf '%s\n' "${url_lines}" | grep -c .)" -ne 1 ]; then + echo "${file}: expected exactly one 'url: ${registration_url}' line, got: '${url_lines}'" >&2 + exit 1 +fi +url_line=${url_lines} +hook_line=$((url_line - 2)) +if [ "${hook_line}" -lt 1 ] \ + || ! sed -n "${hook_line}p" "${file}" | grep -Eq '^[[:space:]]*- hook: web_hook[[:space:]]*$' \ + || ! sed -n "$((url_line - 1))p" "${file}" | grep -Eq '^[[:space:]]*config:[[:space:]]*$'; then + echo "${file}: the /registration hook (line ${url_line}) is not laid out as '- hook: web_hook' / 'config:' / 'url:'; update $(basename "$0")" >&2 + exit 1 +fi + +indent=$(sed -n "${hook_line}p" "${file}" | sed -E 's/^([[:space:]]*).*/\1/') + +# Upstream keeps its commented-out draft of this very hook right above the +# /registration entry. The real entry supersedes it, so drop the run of comment +# lines leading into the anchor when it mentions the hook; keep any other +# comment. +drop_from=${hook_line} +while [ "${drop_from}" -gt 1 ] \ + && sed -n "$((drop_from - 1))p" "${file}" | grep -Eq '^[[:space:]]*(#.*)?$'; do + drop_from=$((drop_from - 1)) +done +if [ "${drop_from}" -lt "${hook_line}" ] \ + && ! sed -n "${drop_from},$((hook_line - 1))p" "${file}" | grep -q 'kratos/preregistration'; then + drop_from=${hook_line} +fi + +# Mirrors the entry in dev/ory/kratos.yml at the repo root, host rewritten. +block=$(cat <<'BLOCK' +# Pre-persist validation, spliced in by quickstart/bin/re-render.sh (upstream +# ships it commented out). `response.parse: true` makes Kratos run this hook +# BEFORE the identity is written: a 4xx with a `messages` body aborts the +# sign-up and nothing is persisted. See dev/ory/kratos.yml at the repo root. +- hook: web_hook + config: + url: http://flash:4012/kratos/preregistration + method: POST + response: + parse: true + body: file:///home/ory/body.jsonnet + auth: + type: api_key + config: + name: Authorization + value: The-Value-of-My-Key + in: header +BLOCK +) + +tmp="${file}.splice.tmp" +# The block goes through the environment: awk -v would interpret escapes and +# some awks choke on embedded newlines there. +SPLICE_BLOCK="${block}" awk \ + -v hook_line="${hook_line}" -v drop_from="${drop_from}" -v indent="${indent}" ' + FNR >= drop_from && FNR < hook_line && $0 ~ /^[[:space:]]*(#.*)?$/ { next } + FNR == hook_line { + n = split(ENVIRON["SPLICE_BLOCK"], lines, "\n") + for (i = 1; i <= n; i++) print indent lines[i] + } + { print } +' "${file}" > "${tmp}" +mv "${tmp}" "${file}" +echo "${file}: spliced pre-persist /kratos/preregistration hook ahead of /kratos/registration" >&2 diff --git a/quickstart/dev/ory/kratos.yml b/quickstart/dev/ory/kratos.yml index 38f4009f8..3ca0d7672 100644 --- a/quickstart/dev/ory/kratos.yml +++ b/quickstart/dev/ory/kratos.yml @@ -111,25 +111,23 @@ selfservice: after: password: hooks: - # we are not sure if we need this hook yet. - # this could be used to check if the user is already registered in the backend - # before creating the user in kratos - # otherwise response: parse: false happens after kratos user creation - # - # - # - hook: web_hook - # config: - # url: http://flash:4012/kratos/preregistration - # method: POST - # response: - # parse: true - # body: file:///home/ory/body.jsonnet # TODO: use a base64 encoding instead - # auth: - # type: api_key - # config: - # name: Authorization - # value: The-Value-of-My-Key - # in: header + # Pre-persist validation, spliced in by quickstart/bin/re-render.sh (upstream + # ships it commented out). `response.parse: true` makes Kratos run this hook + # BEFORE the identity is written: a 4xx with a `messages` body aborts the + # sign-up and nothing is persisted. See dev/ory/kratos.yml at the repo root. + - hook: web_hook + config: + url: http://flash:4012/kratos/preregistration + method: POST + response: + parse: true + body: file:///home/ory/body.jsonnet + auth: + type: api_key + config: + name: Authorization + value: The-Value-of-My-Key + in: header - hook: web_hook config: url: http://flash:4012/kratos/registration diff --git a/test/flash/unit/dev/kratos-registration-hooks.spec.ts b/test/flash/unit/dev/kratos-registration-hooks.spec.ts new file mode 100644 index 000000000..e274609e7 --- /dev/null +++ b/test/flash/unit/dev/kratos-registration-hooks.spec.ts @@ -0,0 +1,209 @@ +/** + * Kratos runs a registration web_hook with `response.parse: true` BEFORE the + * identity is persisted and one with `parse: false` after it. The api leans + * on that order: /kratos/preregistration refuses a sign-up while nothing has + * been written yet, /kratos/registration creates the account once the + * identity exists. Both dev configs must wire it that way — the root + * dev/ory/kratos.yml (integration suite, hooks served at bats-tests:4012) and + * the vendir-synced quickstart/dev/ory/kratos.yml (Quickstart CI and the smoke + * stack, hooks at flash:4012). + * + * The quickstart copy is regenerated from upstream galoy, which ships the + * pre-persist hook commented out, so quickstart/bin/re-render.sh splices it + * in. This spec is what turns a re-render that lost the splice into a red + * build instead of a stack that quietly mints orphaned identities again. + */ +import { spawnSync } from "child_process" +import { mkdtempSync, readFileSync, writeFileSync } from "fs" +import { tmpdir } from "os" +import { join } from "path" + +import { load as loadYaml } from "js-yaml" + +const repoRoot = join(__dirname, "..", "..", "..", "..") + +type WebHook = { + hook: "web_hook" + config: { + url: string + method: string + response?: { parse?: boolean; ignore?: boolean } + body: string + auth: unknown + } +} +type Hook = WebHook | { hook: string } + +const isWebHook = (hook: Hook): hook is WebHook => hook.hook === "web_hook" + +const registrationHooks = (yamlText: string): Hook[] => { + const doc = loadYaml(yamlText) as { + selfservice: { flows: { registration: { after: { password: { hooks: Hook[] } } } } } + } + return doc.selfservice.flows.registration.after.password.hooks +} + +const expectPrePersistThenPostPersist = (hooks: Hook[], host: string) => { + const webHooks = hooks.filter(isWebHook) + expect(webHooks.map((hook) => hook.config.url)).toEqual([ + `http://${host}:4012/kratos/preregistration`, + `http://${host}:4012/kratos/registration`, + ]) + + const [prePersist, postPersist] = webHooks + // parse: true is what moves the hook ahead of persist (web_hook.go, + // ExecutePostRegistrationPrePersistHook); ignore: true would discard the 4xx + // the api answers with. + expect(prePersist.config.response).toEqual({ parse: true }) + expect(postPersist.config.response).toEqual({ parse: false }) + + // Same transport on both, or the api's callback auth rejects one of them. + expect(prePersist.config.method).toBe("POST") + expect(postPersist.config.method).toBe("POST") + expect(prePersist.config.body).toBe(postPersist.config.body) + expect(prePersist.config.auth).toEqual(postPersist.config.auth) + + // The session is issued only once both hooks have run. + expect(hooks[hooks.length - 1]).toEqual({ hook: "session" }) +} + +describe("registration hooks in the dev Kratos configs", () => { + it("dev/ory/kratos.yml (integration suite) runs the pre-persist hook before the post-persist one", () => { + const text = readFileSync(join(repoRoot, "dev/ory/kratos.yml"), "utf8") + const hooks = registrationHooks(text) + + expect(hooks).toHaveLength(3) + expectPrePersistThenPostPersist(hooks, "bats-tests") + }) + + it("quickstart/dev/ory/kratos.yml (Quickstart CI, smoke stack) does too", () => { + const text = readFileSync(join(repoRoot, "quickstart/dev/ory/kratos.yml"), "utf8") + const hooks = registrationHooks(text) + + expect(hooks).toHaveLength(3) + expectPrePersistThenPostPersist(hooks, "flash") + }) +}) + +describe("quickstart/bin/splice-kratos-preregistration-hook.sh", () => { + const script = join(repoRoot, "quickstart/bin/splice-kratos-preregistration-hook.sh") + + // What vendir sync hands re-render.sh once the hosts are rewritten: upstream + // galoy's registration section, pre-persist hook still commented out. + const upstream = `selfservice: + flows: + login: + ui_url: http://localhost:3000/login + lifespan: 10m + + # this below make phone authentication fails even if there is no email in the schema + # after: + # password: + # hooks: + # - hook: require_verified_address + + registration: + lifespan: 10m + ui_url: http://localhost:3000/register + after: + password: + hooks: + # we are not sure if we need this hook yet. + # this could be used to check if the user is already registered in the backend + # before creating the user in kratos + # otherwise response: parse: false happens after kratos user creation + # + # + # - hook: web_hook + # config: + # url: http://flash:4012/kratos/preregistration + # method: POST + # response: + # parse: true + # body: file:///home/ory/body.jsonnet # TODO: use a base64 encoding instead + # auth: + # type: api_key + # config: + # name: Authorization + # value: The-Value-of-My-Key + # in: header + - hook: web_hook + config: + url: http://flash:4012/kratos/registration + method: POST + response: + parse: false + body: file:///home/ory/body.jsonnet # TODO: use a base64 encoding instead + auth: + type: api_key + config: + name: Authorization + value: The-Value-of-My-Key + in: header + - hook: session + +log: + level: debug +` + + const runOn = (text: string) => { + const file = join(mkdtempSync(join(tmpdir(), "kratos-splice-")), "kratos.yml") + writeFileSync(file, text) + const result = spawnSync("bash", [script, file], { encoding: "utf8" }) + return { result, output: readFileSync(file, "utf8") } + } + + it("splices the pre-persist hook ahead of /registration and retires upstream's commented-out draft", () => { + const { result, output } = runOn(upstream) + + expect(result.status).toBe(0) + expectPrePersistThenPostPersist(registrationHooks(output), "flash") + + expect(output).not.toMatch(/we are not sure if we need this hook yet/) + expect(output).not.toMatch(/^\s*#.*kratos\/preregistration/m) + expect( + output.match(/^\s*url: http:\/\/flash:4012\/kratos\/preregistration$/gm), + ).toHaveLength(1) + + // Unrelated comments and everything around the hooks list stay untouched. + expect(output).toMatch(/this below make phone authentication fails/) + expect(output).toMatch(/- hook: require_verified_address/) + expect(output.endsWith("- hook: session\n\nlog:\n level: debug\n")).toBe(true) + }) + + it("leaves a file that already carries the hook alone", () => { + const spliced = runOn(upstream).output + + const { result, output } = runOn(spliced) + + expect(result.status).toBe(0) + expect(output).toBe(spliced) + }) + + it("refuses a file it cannot find the /registration anchor in, and leaves it as it was", () => { + const unrewritten = upstream.replace( + "http://flash:4012/kratos/registration", + "http://bats-tests:4012/kratos/registration", + ) + + const { result, output } = runOn(unrewritten) + + expect(result.status).toBe(1) + expect(result.stderr).toMatch(/expected exactly one/) + expect(output).toBe(unrewritten) + }) + + it("refuses when the /registration hook is not laid out the way it anchors on", () => { + const reordered = upstream.replace( + " config:\n url: http://flash:4012/kratos/registration\n method: POST\n", + " config:\n method: POST\n url: http://flash:4012/kratos/registration\n", + ) + expect(reordered).not.toBe(upstream) + + const { result, output } = runOn(reordered) + + expect(result.status).toBe(1) + expect(result.stderr).toMatch(/not laid out/) + expect(output).toBe(reordered) + }) +}) From d7d0ff17d6a934a437ff159b0c89b006bfcdf47d Mon Sep 17 00:00:00 2001 From: Dread Date: Tue, 1 Sep 2026 17:58:12 -0700 Subject: [PATCH 5/5] fix(quickstart): refuse a preregistration hook that is not pre-persist instead of calling it present Round-3 review finding: the splice script's no-op branch only checked that a `url: .../kratos/preregistration` line existed. If upstream ever ships that hook with `parse: false` (post-persist), the script would report "already present" and Quickstart would run with the orphan-creating shape while every check stays green. The no-op now also requires `parse: true` within four lines of that url; anything else exits 1 with a message that says what to fix. Spec case flips the spliced hook's own parse line (not the comment) and asserts the refusal leaves the file untouched. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01EtQzyzQ38thfC2F8BLK4u8 --- .../bin/splice-kratos-preregistration-hook.sh | 14 +++++++++++--- .../unit/dev/kratos-registration-hooks.spec.ts | 16 ++++++++++++++++ 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/quickstart/bin/splice-kratos-preregistration-hook.sh b/quickstart/bin/splice-kratos-preregistration-hook.sh index 417b2c223..7ee0874eb 100755 --- a/quickstart/bin/splice-kratos-preregistration-hook.sh +++ b/quickstart/bin/splice-kratos-preregistration-hook.sh @@ -28,9 +28,17 @@ file=${1:?usage: $0 } registration_url='http://flash:4012/kratos/registration' preregistration_url='http://flash:4012/kratos/preregistration' -if grep -Eq "^[[:space:]]*url: ${preregistration_url}[[:space:]]*\$" "${file}"; then - echo "${file}: pre-persist registration hook already present, nothing to splice" >&2 - exit 0 +existing_line=$(grep -nE "^[[:space:]]*url: ${preregistration_url}[[:space:]]*\$" "${file}" | head -1 | cut -d: -f1 || true) +if [ -n "${existing_line}" ]; then + # A hook at that url only does its job pre-persist. If upstream ever ships + # it with `parse: false` (or drops `response` entirely) this must fail + # loudly, not report "already present". + if sed -n "${existing_line},$((existing_line + 4))p" "${file}" | grep -Eq '^[[:space:]]*parse: true[[:space:]]*$'; then + echo "${file}: pre-persist registration hook already present, nothing to splice" >&2 + exit 0 + fi + echo "${file}: a /kratos/preregistration hook is present but is not pre-persist (no 'response.parse: true' within 4 lines of line ${existing_line}); fix it by hand" >&2 + exit 1 fi # Anchor on the post-persist hook, expected as three consecutive lines: diff --git a/test/flash/unit/dev/kratos-registration-hooks.spec.ts b/test/flash/unit/dev/kratos-registration-hooks.spec.ts index e274609e7..eff373737 100644 --- a/test/flash/unit/dev/kratos-registration-hooks.spec.ts +++ b/test/flash/unit/dev/kratos-registration-hooks.spec.ts @@ -180,6 +180,22 @@ log: expect(output).toBe(spliced) }) + it("refuses a file whose /kratos/preregistration hook is not pre-persist, and leaves it as it was", () => { + const spliced = runOn(upstream).output + // Flip the config line of the spliced hook itself, not the comment above it. + const postPersist = spliced.replace( + /(url: http:\/\/flash:4012\/kratos\/preregistration[\s\S]*?parse: )true/, + "$1false", + ) + expect(postPersist).not.toBe(spliced) + + const { result, output } = runOn(postPersist) + + expect(result.status).toBe(1) + expect(result.stderr).toContain("not pre-persist") + expect(output).toBe(postPersist) + }) + it("refuses a file it cannot find the /registration anchor in, and leaves it as it was", () => { const unrewritten = upstream.replace( "http://flash:4012/kratos/registration",