From c0a64db505cde0d36cb68c08418ac7bf106675b2 Mon Sep 17 00:00:00 2001 From: Dread Date: Mon, 24 Aug 2026 14:02:14 -0700 Subject: [PATCH 1/9] feat(admin): add accountDetailsByNpub query Support tooling needs to resolve a Nostr pubkey to a Flash account so the Chatwoot contact created by the nostr-dm-bridge can be enriched with the real username/phone/email. The repository lookup (findByNpub) already existed; this exposes it on the admin GraphQL API alongside the other accountDetailsBy* queries. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_015xSxhf5Kijfib4PbExGESk --- src/app/admin/index.ts | 7 +++ src/graphql/admin/queries.ts | 2 + .../root/query/account-details-by-npub.ts | 28 ++++++++++ src/graphql/admin/schema.graphql | 6 +- .../admin/account-details-by-npub.spec.ts | 55 +++++++++++++++++++ 5 files changed, 97 insertions(+), 1 deletion(-) create mode 100644 src/graphql/admin/root/query/account-details-by-npub.ts create mode 100644 test/flash/unit/graphql/admin/account-details-by-npub.spec.ts diff --git a/src/app/admin/index.ts b/src/app/admin/index.ts index b01b57848..7f81d5849 100644 --- a/src/app/admin/index.ts +++ b/src/app/admin/index.ts @@ -20,6 +20,13 @@ export const getAccountByUsername = async (username: string) => { return accounts.findByUsername(usernameValid) } +export const getAccountByNpub = async (npub: string) => { + // Format validation happens at the GraphQL boundary (Npub scalar); + // the repository query is an exact $eq match so a raw string is safe. + const accounts = AccountsRepository() + return accounts.findByNpub(npub as Npub) +} + export const getAccountByUserPhone = async (phone: PhoneNumber) => { // TODO: replace by getAccountByUserPhone // but need to change the integration admin query test first diff --git a/src/graphql/admin/queries.ts b/src/graphql/admin/queries.ts index 89d215e72..c26a897ba 100644 --- a/src/graphql/admin/queries.ts +++ b/src/graphql/admin/queries.ts @@ -13,6 +13,7 @@ import AccountDetailsByUserEmailQuery from "./root/query/account-details-by-emai import ListWalletIdsQuery from "./root/query/all-walletids" import WalletQuery from "./root/query/wallet" import AccountDetailsByAccountId from "./root/query/account-details-by-account-id" +import AccountDetailsByNpubQuery from "./root/query/account-details-by-npub" import MerchantsPendingApprovalQuery from "./root/query/merchants-pending-approval-listing" import IdDocumentReadUrlQuery from "./root/query/id-document-read-url" import NotificationTopicsQuery from "./root/query/notification-topics" @@ -29,6 +30,7 @@ export const queryFields = { accountDetailsByUsername: AccountDetailsByUsernameQuery, accountDetailsByEmail: AccountDetailsByUserEmailQuery, accountDetailsByAccountId: AccountDetailsByAccountId, + accountDetailsByNpub: AccountDetailsByNpubQuery, transactionById: TransactionByIdQuery, transactionDetailsById: TransactionDetailsByIdQuery, transactionsByHash: TransactionsByHashQuery, diff --git a/src/graphql/admin/root/query/account-details-by-npub.ts b/src/graphql/admin/root/query/account-details-by-npub.ts new file mode 100644 index 000000000..c2993ce33 --- /dev/null +++ b/src/graphql/admin/root/query/account-details-by-npub.ts @@ -0,0 +1,28 @@ +import { GT } from "@graphql/index" + +import GraphQLAccount from "@graphql/admin/types/object/account" +import Npub from "@graphql/shared/types/scalar/npub" +import { mapError } from "@graphql/error-map" + +import { Admin } from "@app" + +const AccountDetailsByNpubQuery = GT.Field({ + type: GT.NonNull(GraphQLAccount), + args: { + npub: { type: GT.NonNull(Npub) }, + }, + resolve: async (parent, { npub }) => { + if (npub instanceof Error) { + throw npub + } + + const account = await Admin.getAccountByNpub(npub) + if (account instanceof Error) { + throw mapError(account) + } + + return account + }, +}) + +export default AccountDetailsByNpubQuery diff --git a/src/graphql/admin/schema.graphql b/src/graphql/admin/schema.graphql index c4579b2b2..05b5ef67b 100644 --- a/src/graphql/admin/schema.graphql +++ b/src/graphql/admin/schema.graphql @@ -489,6 +489,7 @@ type PriceOfOneSettlementMinorUnitInDisplayMinorUnit implements PriceInterface { type Query { accountDetailsByAccountId(accountId: ID!): AuditedAccount! accountDetailsByEmail(email: EmailAddress!): AuditedAccount! + accountDetailsByNpub(npub: npub!): AuditedAccount! accountDetailsByUserPhone(phone: Phone!): AuditedAccount! accountDetailsByUsername(username: Username!): AuditedAccount! allLevels: [AccountLevel!]! @@ -871,4 +872,7 @@ enum WalletCurrency { } """Unique identifier of a wallet""" -scalar WalletId \ No newline at end of file +scalar WalletId + +"""Nostr Identity public key""" +scalar npub \ No newline at end of file diff --git a/test/flash/unit/graphql/admin/account-details-by-npub.spec.ts b/test/flash/unit/graphql/admin/account-details-by-npub.spec.ts new file mode 100644 index 000000000..38ab8f7f2 --- /dev/null +++ b/test/flash/unit/graphql/admin/account-details-by-npub.spec.ts @@ -0,0 +1,55 @@ +const mockGetAccountByNpub = jest.fn() + +jest.mock("@app/index", () => ({ + Admin: { + getAccountByNpub: (...args: unknown[]) => mockGetAccountByNpub(...args), + }, +})) + +import { CouldNotFindAccountFromUsernameError } from "@domain/errors" +import AccountDetailsByNpubQuery from "@graphql/admin/root/query/account-details-by-npub" + +const VALID_NPUB = "npub1" + "q".repeat(58) + +const resolveQuery = async (npub: unknown) => { + const resolve = AccountDetailsByNpubQuery.resolve as unknown as ( + source: null, + args: { npub: unknown }, + ctx: Record, + ) => Promise + + return resolve(null, { npub }, {}) +} + +describe("accountDetailsByNpub", () => { + beforeEach(() => { + jest.clearAllMocks() + }) + + it("returns the account for a known npub", async () => { + const account = { id: "account-id", username: "jaceth2009", npub: VALID_NPUB } + mockGetAccountByNpub.mockResolvedValue(account) + + const result = await resolveQuery(VALID_NPUB) + + expect(mockGetAccountByNpub).toHaveBeenCalledWith(VALID_NPUB) + expect(result).toBe(account) + }) + + it("throws a mapped error when no account matches", async () => { + mockGetAccountByNpub.mockResolvedValue( + new CouldNotFindAccountFromUsernameError(VALID_NPUB), + ) + + await expect(resolveQuery(VALID_NPUB)).rejects.toThrow() + }) + + it("rethrows scalar validation errors without hitting the app layer", async () => { + const validationError = new Error("Invalid value for Npub") + + await expect(resolveQuery(validationError)).rejects.toThrow( + "Invalid value for Npub", + ) + expect(mockGetAccountByNpub).not.toHaveBeenCalled() + }) +}) From 9bfe6386a81821e11606e809e00626d9e4fa3d0a Mon Sep 17 00:00:00 2001 From: Dread Date: Mon, 24 Aug 2026 17:11:23 -0700 Subject: [PATCH 2/9] fix(admin): harden npub lookup after review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review fixes for the accountDetailsByNpub query. - accounts.npub gets a unique partial index plus a migration that lowercases existing values, keeps the oldest account in each duplicate group and unsets the rest, then builds the index. setNpub now refuses an npub already claimed by another account (NpubNotAvailableError) instead of silently overwriting, so the support-desk resolver can no longer surface one customer's phone, email and level under another's identity. - Npub scalar: parseLiteral's success branch had no `return`, so graphql-js read it as failed coercion and rejected every inline-literal npub as malformed. parseValue now lowercases too, so both paths normalise identically. - findByNpub drops the `.collation(...)` (bech32 is lowercase-only, and a non-simple collation cannot use the new index) and returns CouldNotFindAccountFromNpubError, so a miss no longer reports "Account does not exist for username npub1…". - Admin.getAccountByNpub takes a branded Npub and validates via a new checkedToNpub rather than laundering a raw string with `as Npub`; it moves to its own module so it is unit-testable without importing the admin barrel. - The query field is typed (GT.Field), so the `npub instanceof Error` guard is checked by the compiler. Tests: the admin spec is now a schema-execution spec over the real field, real scalar and real error map (variable form, inline-literal form, case normalisation, malformed rejection, npub-worded 404, SDL registration), plus new specs for getAccountByNpub, setNpub's duplicate refusal, findByNpub's query shape and error class, and the schema index declaration. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_015xSxhf5Kijfib4PbExGESk --- src/app/accounts/set-npub.ts | 25 +- src/app/admin/get-account-by-npub.ts | 19 ++ src/app/admin/index.ts | 8 +- src/app/errors.ts | 2 + src/domain/errors.ts | 1 + src/domain/nostr/errors.ts | 7 + src/domain/nostr/index.ts | 13 + .../root/query/account-details-by-npub.ts | 9 +- src/graphql/error-map.ts | 9 + src/graphql/shared/types/scalar/npub.ts | 10 +- .../20260824120000-accounts-unique-npub.ts | 170 +++++++++++++ src/services/mongoose/accounts.ts | 13 +- src/services/mongoose/schema.ts | 12 + test/flash/unit/app/accounts/set-npub.spec.ts | 81 +++++++ .../app/admin/get-account-by-npub.spec.ts | 52 ++++ .../admin/account-details-by-npub.spec.ts | 227 ++++++++++++++++-- .../mongoose/account-npub-index.spec.ts | 23 ++ .../mongoose/accounts-find-by-npub.spec.ts | 60 +++++ 18 files changed, 696 insertions(+), 45 deletions(-) create mode 100644 src/app/admin/get-account-by-npub.ts create mode 100644 src/domain/nostr/errors.ts create mode 100644 src/migrations/20260824120000-accounts-unique-npub.ts create mode 100644 test/flash/unit/app/accounts/set-npub.spec.ts create mode 100644 test/flash/unit/app/admin/get-account-by-npub.spec.ts create mode 100644 test/flash/unit/services/mongoose/account-npub-index.spec.ts create mode 100644 test/flash/unit/services/mongoose/accounts-find-by-npub.spec.ts diff --git a/src/app/accounts/set-npub.ts b/src/app/accounts/set-npub.ts index e3b0a0203..e3f7dfd68 100644 --- a/src/app/accounts/set-npub.ts +++ b/src/app/accounts/set-npub.ts @@ -1,7 +1,6 @@ +import { checkedToNpub, NpubNotAvailableError } from "@domain/nostr" +import { CouldNotFindError } from "@domain/errors" import { AccountsRepository } from "@services/mongoose" -import { checkValidNpub } from "@domain/nostr" - -import { ValidationError } from "ajv" export const setNpub = async ({ id, @@ -11,10 +10,24 @@ export const setNpub = async ({ npub: Npub }): Promise => { const accountsRepo = AccountsRepository() - if (!checkValidNpub(npub)) - throw new ValidationError([{ message: "Invalid npub format" }]) + + const npubChecked = checkedToNpub(npub) + if (npubChecked instanceof Error) return npubChecked + + // An npub is an identity, and since this PR it is also a support-desk + // lookup key: `findByNpub` is a `findOne`, so two accounts claiming the same + // npub would resolve nondeterministically and could paint one customer's + // phone/email/level onto another's contact card. Refuse the claim here, and + // let the unique index on `accounts.npub` catch the concurrent-write race. + const existing = await accountsRepo.findByNpub(npubChecked) + if (!(existing instanceof Error)) { + if (existing.id !== id) return new NpubNotAvailableError(npubChecked) + return existing + } + if (!(existing instanceof CouldNotFindError)) return existing + const account = await accountsRepo.findById(id) if (account instanceof Error) return account - account.npub = npub + account.npub = npubChecked return accountsRepo.update(account) } diff --git a/src/app/admin/get-account-by-npub.ts b/src/app/admin/get-account-by-npub.ts new file mode 100644 index 000000000..286935793 --- /dev/null +++ b/src/app/admin/get-account-by-npub.ts @@ -0,0 +1,19 @@ +import { checkedToNpub } from "@domain/nostr" +import { AccountsRepository } from "@services/mongoose" + +/** + * Lives in its own module rather than inline in `index.ts` so it can be unit + * tested against a mocked repository — importing the admin barrel drags in the + * notification and invite stacks, which open connections at import time. + */ +export const getAccountByNpub = async (npub: Npub) => { + // Mirrors getAccountByUsername: the branded type is the contract, and + // checkedToNpub is defence in depth for callers that are not the GraphQL + // boundary (scripts, backfills, a future REST shim) — they get a validation + // error rather than a silent not-found on a malformed value. + const npubValid = checkedToNpub(npub) + if (npubValid instanceof Error) return npubValid + + const accounts = AccountsRepository() + return accounts.findByNpub(npubValid) +} diff --git a/src/app/admin/index.ts b/src/app/admin/index.ts index 7f81d5849..fdfc0d75f 100644 --- a/src/app/admin/index.ts +++ b/src/app/admin/index.ts @@ -4,6 +4,7 @@ export * from "./update-user-phone" export * from "./send-cashout-notification" export * from "./send-user-notification" export * from "./invite" +export * from "./get-account-by-npub" // Re-export query functions from invite module for admin GraphQL compatibility export { getInviteById, listInvites } from "../invite/queries" @@ -20,13 +21,6 @@ export const getAccountByUsername = async (username: string) => { return accounts.findByUsername(usernameValid) } -export const getAccountByNpub = async (npub: string) => { - // Format validation happens at the GraphQL boundary (Npub scalar); - // the repository query is an exact $eq match so a raw string is safe. - const accounts = AccountsRepository() - return accounts.findByNpub(npub as Npub) -} - export const getAccountByUserPhone = async (phone: PhoneNumber) => { // TODO: replace by getAccountByUserPhone // but need to change the integration admin query test first diff --git a/src/app/errors.ts b/src/app/errors.ts index 687b49fe7..1fabd9921 100644 --- a/src/app/errors.ts +++ b/src/app/errors.ts @@ -19,6 +19,7 @@ import * as PubSubErrors from "@domain/pubsub/errors" import * as CaptchaErrors from "@domain/captcha/errors" import * as AuthenticationErrors from "@domain/authentication/errors" import * as UserErrors from "@domain/users/errors" +import * as NostrErrors from "@domain/nostr/errors" import * as CashWalletCutoverErrors from "@app/cash-wallet-cutover/errors" import * as LedgerFacadeErrors from "@services/ledger/domain/errors" @@ -54,6 +55,7 @@ export const ApplicationErrors = { ...CaptchaErrors, ...AuthenticationErrors, ...UserErrors, + ...NostrErrors, ...CashWalletCutoverErrors, ...KratosErrors, diff --git a/src/domain/errors.ts b/src/domain/errors.ts index c350acf5c..7edc4f29a 100644 --- a/src/domain/errors.ts +++ b/src/domain/errors.ts @@ -63,6 +63,7 @@ export class CouldNotFindLnPaymentFromHashError extends CouldNotFindError { export class CouldNotFindAccountFromUuidError extends CouldNotFindError {} export class CouldNotFindAccountFromUsernameError extends CouldNotFindError {} +export class CouldNotFindAccountFromNpubError extends CouldNotFindError {} export class CouldNotFindAccountFromPhoneError extends CouldNotFindError {} export class CouldNotFindMerchantFromUsernameError extends CouldNotFindError {} export class CouldNotFindMerchantFromIdError extends CouldNotFindError {} diff --git a/src/domain/nostr/errors.ts b/src/domain/nostr/errors.ts new file mode 100644 index 000000000..5c2448bb7 --- /dev/null +++ b/src/domain/nostr/errors.ts @@ -0,0 +1,7 @@ +import { DomainError, ValidationError } from "@domain/shared" + +export class NostrError extends DomainError {} + +export class InvalidNpubError extends ValidationError {} + +export class NpubNotAvailableError extends NostrError {} diff --git a/src/domain/nostr/index.ts b/src/domain/nostr/index.ts index 3433b79b7..1f503be58 100644 --- a/src/domain/nostr/index.ts +++ b/src/domain/nostr/index.ts @@ -1,3 +1,16 @@ +import { InvalidNpubError } from "./errors" + +export * from "./errors" + export const checkValidNpub = (npub: string): boolean => { return npub.startsWith("npub1") && npub.length === 63 } + +// Mirrors `checkedToUsername` / `checkedToAccountUuid`: the branded `Npub` type +// only means something if there is a single place that mints it. Callers that +// are not the GraphQL boundary (scripts, backfills, REST shims) get a +// validation error instead of a silent not-found on a malformed value. +export const checkedToNpub = (npub: string): Npub | ValidationError => { + if (!checkValidNpub(npub)) return new InvalidNpubError(npub) + return npub.toLowerCase() as Npub +} diff --git a/src/graphql/admin/root/query/account-details-by-npub.ts b/src/graphql/admin/root/query/account-details-by-npub.ts index c2993ce33..b62dc293f 100644 --- a/src/graphql/admin/root/query/account-details-by-npub.ts +++ b/src/graphql/admin/root/query/account-details-by-npub.ts @@ -6,7 +6,14 @@ import { mapError } from "@graphql/error-map" import { Admin } from "@app" -const AccountDetailsByNpubQuery = GT.Field({ +const AccountDetailsByNpubQuery = GT.Field< + null, + GraphQLAdminContext, + { + // FIXME: doesn't respect the input: {} pattern + npub: Npub | ValidationError + } +>({ type: GT.NonNull(GraphQLAccount), args: { npub: { type: GT.NonNull(Npub) }, diff --git a/src/graphql/error-map.ts b/src/graphql/error-map.ts index 8c291ef1e..08981cb43 100644 --- a/src/graphql/error-map.ts +++ b/src/graphql/error-map.ts @@ -112,6 +112,10 @@ export const mapError = (error: ApplicationError): CustomApolloError => { message = `Account does not exist for username ${error.message}` return new NotFoundError({ message, logger: baseLogger }) + case "CouldNotFindAccountFromNpubError": + message = `Account does not exist for npub ${error.message}` + return new NotFoundError({ message, logger: baseLogger }) + case "CouldNotFindMerchantFromUsernameError": message = `Merchant does not exist for username ${error.message}` return new NotFoundError({ message, logger: baseLogger }) @@ -381,6 +385,10 @@ export const mapError = (error: ApplicationError): CustomApolloError => { message = "username is immutable" return new UsernameError({ message, logger: baseLogger }) + case "NpubNotAvailableError": + message = "npub is already linked to another account" + return new ValidationInternalError({ message, logger: baseLogger }) + case "InvalidWalletId": message = "Invalid walletId for account." return new ValidationInternalError({ message, logger: baseLogger }) @@ -766,6 +774,7 @@ export const mapError = (error: ApplicationError): CustomApolloError => { case "LnRouteValidationError": case "BadAmountForRouteError": case "InvalidUsername": + case "InvalidNpubError": case "InvalidDeviceId": case "InvalidIdentityPassword": case "InvalidIdentityUsername": diff --git a/src/graphql/shared/types/scalar/npub.ts b/src/graphql/shared/types/scalar/npub.ts index fb3a6d48c..f4ea1b41c 100644 --- a/src/graphql/shared/types/scalar/npub.ts +++ b/src/graphql/shared/types/scalar/npub.ts @@ -5,19 +5,25 @@ import { GT } from "@graphql/index" const Npub = GT.Scalar({ name: "npub", description: "Nostr Identity public key", + // Both coercion paths must normalise identically: bech32 npubs are a + // lowercase-only charset, so lowercasing here is what lets the repository + // query be a plain, index-backed `$eq` with no collation. parseValue(value) { if (typeof value !== "string") { return new InputValidationError({ message: "Invalid type for Npub" }) } else if (!checkValidNpub(value)) return new InputValidationError({ message: "Invalid value for Npub" }) - return value.toString() + return value.toLowerCase() }, parseLiteral(ast) { if (ast.kind !== GT.Kind.STRING) return new InputValidationError({ message: "Invalid type for Npub" }) else if (!checkValidNpub(ast.value)) return new InputValidationError({ message: "Invalid value for Npub" }) - else ast.value.toLowerCase() + // NOTE: the `return` is load-bearing. graphql-js reads `undefined` from + // parseLiteral as failed coercion, so dropping it rejects every valid + // inline-literal npub at validation time. + else return ast.value.toLowerCase() }, }) diff --git a/src/migrations/20260824120000-accounts-unique-npub.ts b/src/migrations/20260824120000-accounts-unique-npub.ts new file mode 100644 index 000000000..e655b2b58 --- /dev/null +++ b/src/migrations/20260824120000-accounts-unique-npub.ts @@ -0,0 +1,170 @@ +/* eslint-disable @typescript-eslint/ban-ts-comment */ +// @ts-nocheck +/* eslint @typescript-eslint/no-var-requires: "off" */ + +/** + * Migration: unique index on accounts.npub + * + * Background + * ---------- + * `npub` had neither an index nor a uniqueness constraint, and `setNpub` did no + * duplicate checking — it wrote whatever the caller sent. `findByNpub` is a + * `findOne`, so two accounts holding the same npub resolved to whichever + * document Mongo scanned first. That became support-facing the moment + * `accountDetailsByNpub` shipped: a support agent opening a DM from an npub + * would see username / level / phone / email off an arbitrary one of the + * colliding accounts. + * + * What this migration does + * ------------------------ + * 1. Lowercases any non-lowercase npub. The old `findByNpub` carried a + * case-insensitive collation which has been dropped (bech32 is a + * lowercase-only charset, and the collation blocked index use), so stored + * values must be normalised or they stop being findable. + * 2. Audits for duplicate npubs. For each group it keeps the OLDEST account + * (earliest created_at) and UNSETS npub on the rest — accounts are never + * deleted or merged here. Every unset is logged with account id + npub so + * support can reach out and have the losing owners re-link. + * 3. Creates the unique partial index. + * + * `partialFilterExpression: { npub: { $type: "string" } }` rather than + * `sparse: true`: a sparse index still indexes documents holding an explicit + * `npub: null`, and the second such document would collide. + * + * Rollback (down) + * --------------- + * Drops the unique index. The lowercasing and the unsets are NOT reverted — + * they are data repairs, and restoring known-ambiguous npubs would reintroduce + * the identity collision. + */ + +const COLLECTION = "accounts" +const INDEX_NAME = "npub_1" + +module.exports = { + async up(db) { + const col = db.collection(COLLECTION) + + const exists = await db.listCollections({ name: COLLECTION }).toArray() + if (exists.length === 0) { + // Fresh database (this is what CI's clean-migration run sees). + // createIndex creates the collection, and there is nothing to repair. + await col.createIndex( + { npub: 1 }, + { + unique: true, + name: INDEX_NAME, + partialFilterExpression: { npub: { $type: "string" } }, + }, + ) + console.log(`[migration] ${COLLECTION} did not exist; created "${INDEX_NAME}".`) + return + } + + // ── Step 1: normalise case ─────────────────────────────────────────────── + // Server-side, so this does not stream the collection through the migration + // process. The ids are listed first purely so the repair is auditable. + const mixedCase = await col + .aggregate( + [ + { $match: { npub: { $type: "string" } } }, + { $match: { $expr: { $ne: ["$npub", { $toLower: "$npub" }] } } }, + { $project: { _id: 1, id: 1, npub: 1 } }, + ], + { allowDiskUse: true }, + ) + .toArray() + + for (const doc of mixedCase) { + console.log(`[migration] accountId=${doc.id} npub will be lowercased: ${doc.npub}`) + } + + if (mixedCase.length > 0) { + await col.updateMany({ npub: { $type: "string" } }, [ + { $set: { npub: { $toLower: "$npub" } } }, + ]) + } + console.log(`[migration] Normalised ${mixedCase.length} npub value(s) to lowercase.`) + + // ── Step 2: find and resolve duplicate npub groups ─────────────────────── + // Only the fields needed to pick a winner are pushed — `$$ROOT` would risk + // the 16MB per-group limit on a large accounts collection. + const duplicates = await col + .aggregate( + [ + { $match: { npub: { $type: "string" } } }, + { + $group: { + _id: "$npub", + count: { $sum: 1 }, + docs: { $push: { _id: "$_id", id: "$id", created_at: "$created_at" } }, + }, + }, + { $match: { count: { $gt: 1 } } }, + ], + { allowDiskUse: true }, + ) + .toArray() + + if (duplicates.length > 0) { + console.log( + `[migration] Found ${duplicates.length} npub(s) claimed by more than one account. Resolving...`, + ) + + for (const group of duplicates) { + // Oldest account keeps the npub — it is the likeliest original owner. + const sorted = group.docs.sort( + (a, b) => new Date(a.created_at).getTime() - new Date(b.created_at).getTime(), + ) + const [winner, ...losers] = sorted + const loserIds = losers.map((d) => d._id) + + console.log( + `[migration] npub=${group._id} — keeping accountId=${winner.id} (_id=${winner._id}), unsetting npub on ${loserIds.length} account(s): ${losers + .map((d) => d.id) + .join(", ")}`, + ) + + await col.updateMany({ _id: { $in: loserIds } }, { $unset: { npub: "" } }) + } + + console.log("[migration] Duplicate npub resolution complete.") + } else { + console.log("[migration] No duplicate npub values found. Proceeding.") + } + + // ── Step 3: drop any stale index, then create the unique partial index ─── + const existingIndexes = await col.indexes() + const stale = existingIndexes.find((idx) => idx.name === INDEX_NAME && !idx.unique) + if (stale) { + await col.dropIndex(INDEX_NAME) + console.log(`[migration] Dropped existing non-unique index "${INDEX_NAME}".`) + } + + await col.createIndex( + { npub: 1 }, + { + unique: true, + name: INDEX_NAME, + partialFilterExpression: { npub: { $type: "string" } }, + }, + ) + console.log(`[migration] Created unique index "${INDEX_NAME}" on ${COLLECTION}.`) + }, + + async down(db) { + const col = db.collection(COLLECTION) + + const exists = await db.listCollections({ name: COLLECTION }).toArray() + if (exists.length === 0) return + + const existingIndexes = await col.indexes() + const hasUniqueIndex = existingIndexes.some( + (idx) => idx.name === INDEX_NAME && idx.unique, + ) + if (hasUniqueIndex) { + await col.dropIndex(INDEX_NAME) + console.log(`[migration] Dropped unique index "${INDEX_NAME}".`) + } + }, +} diff --git a/src/services/mongoose/accounts.ts b/src/services/mongoose/accounts.ts index 779545639..b89ebd765 100644 --- a/src/services/mongoose/accounts.ts +++ b/src/services/mongoose/accounts.ts @@ -4,6 +4,7 @@ import { AccountStatus } from "@domain/accounts" import { CouldNotFindAccountError, CouldNotFindAccountFromKratosIdError, + CouldNotFindAccountFromNpubError, CouldNotFindAccountFromUsernameError, CouldNotFindAccountFromUuidError, RepositoryError, @@ -77,14 +78,16 @@ export const AccountsRepository = (): IAccountsRepository => { } } + // No `.collation(...)` here on purpose. bech32 npubs are a lowercase-only + // charset and both scalar coercion paths lowercase, so case-insensitivity + // buys nothing — and a non-simple collation would stop the query from using + // the unique `{ npub: 1 }` index, leaving every support-desk lookup a + // collection scan. const findByNpub = async (npub: Npub): Promise => { try { - const result = await Account.findOne({ npub: { $eq: npub } }).collation({ - locale: "en", - strength: 2, - }) + const result = await Account.findOne({ npub: { $eq: npub } }) if (!result) { - return new CouldNotFindAccountFromUsernameError(npub) + return new CouldNotFindAccountFromNpubError(npub) } return translateToAccount(result) } catch (err) { diff --git a/src/services/mongoose/schema.ts b/src/services/mongoose/schema.ts index a23a2ad37..766c729e3 100644 --- a/src/services/mongoose/schema.ts +++ b/src/services/mongoose/schema.ts @@ -390,6 +390,18 @@ AccountSchema.index({ AccountSchema.index({ bridgeEthereumAddress: 1 }, { sparse: true }) +// An npub identifies a customer to the admin/support lookup, so it must resolve +// to exactly one account. `partialFilterExpression` rather than `sparse` — a +// sparse index still indexes documents that hold an explicit `npub: null`, and +// the second such document would collide. Mirrors the `username` index above. +AccountSchema.index( + { npub: 1 }, + { + unique: true, + partialFilterExpression: { npub: { $type: "string" } }, + }, +) + export const Account = mongoose.model("Account", AccountSchema) const QuizSchema = new Schema({ diff --git a/test/flash/unit/app/accounts/set-npub.spec.ts b/test/flash/unit/app/accounts/set-npub.spec.ts new file mode 100644 index 000000000..f0f21a82d --- /dev/null +++ b/test/flash/unit/app/accounts/set-npub.spec.ts @@ -0,0 +1,81 @@ +/** + * `setNpub` used to write whatever the caller sent, with no duplicate check. + * Combined with `findByNpub` being a `findOne`, two accounts claiming the same + * npub resolved nondeterministically — and since `accountDetailsByNpub` shipped, + * that resolution paints a customer's phone/email/level onto a support agent's + * screen. These tests pin the refusal. + */ +const findByNpub = jest.fn() +const findById = jest.fn() +const update = jest.fn() + +jest.mock("@services/mongoose", () => ({ + AccountsRepository: () => ({ findByNpub, findById, update }), +})) + +import { CouldNotFindAccountFromNpubError, UnknownRepositoryError } from "@domain/errors" +import { InvalidNpubError, NpubNotAvailableError } from "@domain/nostr" +import { setNpub } from "@app/accounts/set-npub" + +const NPUB = ("npub1" + "q".repeat(58)) as Npub +const ACCOUNT_ID = "account-id" as AccountId +const OTHER_ACCOUNT_ID = "other-account-id" as AccountId + +describe("Accounts.setNpub", () => { + beforeEach(() => { + findByNpub.mockReset().mockResolvedValue(new CouldNotFindAccountFromNpubError(NPUB)) + findById.mockReset().mockResolvedValue({ id: ACCOUNT_ID }) + update.mockReset().mockImplementation(async (account) => account) + }) + + it("links an unclaimed npub", async () => { + const result = await setNpub({ id: ACCOUNT_ID, npub: NPUB }) + + expect(result).toEqual({ id: ACCOUNT_ID, npub: NPUB }) + expect(update).toHaveBeenCalledWith({ id: ACCOUNT_ID, npub: NPUB }) + }) + + it("refuses an npub already claimed by another account", async () => { + // THE IDENTITY COLLISION. Restoring a nostr key on a second account — or + // deliberately pasting a victim's npub — used to just overwrite. + findByNpub.mockResolvedValue({ id: OTHER_ACCOUNT_ID, npub: NPUB }) + + const result = await setNpub({ id: ACCOUNT_ID, npub: NPUB }) + + expect(result).toBeInstanceOf(NpubNotAvailableError) + expect(update).not.toHaveBeenCalled() + }) + + it("is idempotent when the account already owns the npub", async () => { + const owned = { id: ACCOUNT_ID, npub: NPUB } + findByNpub.mockResolvedValue(owned) + + expect(await setNpub({ id: ACCOUNT_ID, npub: NPUB })).toBe(owned) + expect(update).not.toHaveBeenCalled() + }) + + it("rejects a malformed npub without touching the repository", async () => { + const result = await setNpub({ id: ACCOUNT_ID, npub: "nope" as Npub }) + + expect(result).toBeInstanceOf(InvalidNpubError) + expect(findByNpub).not.toHaveBeenCalled() + expect(update).not.toHaveBeenCalled() + }) + + it("normalises case before claiming", async () => { + await setNpub({ id: ACCOUNT_ID, npub: ("npub1" + "Q".repeat(58)) as Npub }) + + expect(findByNpub).toHaveBeenCalledWith(NPUB) + expect(update).toHaveBeenCalledWith({ id: ACCOUNT_ID, npub: NPUB }) + }) + + it("does not claim the npub when the uniqueness probe itself fails", async () => { + // A repository failure is not evidence that the npub is free. + findByNpub.mockResolvedValue(new UnknownRepositoryError("mongo down")) + + const result = await setNpub({ id: ACCOUNT_ID, npub: NPUB }) + + expect(result).toBeInstanceOf(UnknownRepositoryError) + expect(update).not.toHaveBeenCalled() + }) +}) diff --git a/test/flash/unit/app/admin/get-account-by-npub.spec.ts b/test/flash/unit/app/admin/get-account-by-npub.spec.ts new file mode 100644 index 000000000..0c16e93a7 --- /dev/null +++ b/test/flash/unit/app/admin/get-account-by-npub.spec.ts @@ -0,0 +1,52 @@ +/** + * `Admin.getAccountByNpub` is the only genuinely new app-layer code behind the + * admin npub lookup, and it was previously reachable only through a mocked + * `@app` barrel — i.e. not covered at all. Here the repository is the mock and + * the app function is real. + */ +const findByNpub = jest.fn() + +jest.mock("@services/mongoose", () => ({ + AccountsRepository: () => ({ findByNpub }), + UsersRepository: () => ({}), +})) + +import { InvalidNpubError } from "@domain/nostr" +import { getAccountByNpub } from "@app/admin/get-account-by-npub" + +const NPUB = ("npub1" + "q".repeat(58)) as Npub + +describe("Admin.getAccountByNpub", () => { + beforeEach(() => { + findByNpub.mockReset() + }) + + it("hands a valid npub to the repository", async () => { + const account = { id: "account-id", npub: NPUB } + findByNpub.mockResolvedValue(account) + + expect(await getAccountByNpub(NPUB)).toBe(account) + expect(findByNpub).toHaveBeenCalledWith(NPUB) + }) + + it("normalises case before querying", async () => { + // The repository query is a plain `$eq` with no collation, so normalisation + // has to happen before it or a mixed-case npub is a false not-found. + findByNpub.mockResolvedValue({ id: "account-id" }) + + await getAccountByNpub(("npub1" + "Q".repeat(58)) as Npub) + + expect(findByNpub).toHaveBeenCalledWith(NPUB) + }) + + it("rejects a malformed npub instead of querying with it", async () => { + // Defence in depth for callers that are NOT the GraphQL boundary — a + // script, a backfill, a REST shim. Before the fix the parameter was a raw + // `string` laundered with `as Npub`, so those callers got a silent + // not-found rather than a validation error. + const result = await getAccountByNpub("not-an-npub" as Npub) + + expect(result).toBeInstanceOf(InvalidNpubError) + expect(findByNpub).not.toHaveBeenCalled() + }) +}) diff --git a/test/flash/unit/graphql/admin/account-details-by-npub.spec.ts b/test/flash/unit/graphql/admin/account-details-by-npub.spec.ts index 38ab8f7f2..d7a8aef41 100644 --- a/test/flash/unit/graphql/admin/account-details-by-npub.spec.ts +++ b/test/flash/unit/graphql/admin/account-details-by-npub.spec.ts @@ -1,55 +1,234 @@ +/** + * Schema-execution spec for the admin `accountDetailsByNpub` query. + * + * Deliberately NOT a direct `.resolve(...)` call. Calling the resolver by hand + * covers only the eight lines of boilerplate and leaves the two things that + * actually broke untested: the `Npub` scalar's coercion (its `parseLiteral` + * shipped without a `return`, rejecting every inline-literal npub) and the + * query's registration on the admin schema (deleting that line was invisible + * to the old spec). + * + * So: real field definition, real `Npub` scalar, real resolver, real error map, + * and the committed SDL for registration. Only the app layer is mocked — it is + * the boundary this file is not responsible for. + */ const mockGetAccountByNpub = jest.fn() +const mockGetUser = jest.fn() -jest.mock("@app/index", () => ({ +jest.mock("@app", () => ({ Admin: { getAccountByNpub: (...args: unknown[]) => mockGetAccountByNpub(...args), }, + Users: { + getUser: (...args: unknown[]) => mockGetUser(...args), + }, + Accounts: { getAccountCapabilities: jest.fn() }, + Wallets: { listWalletsByAccountId: jest.fn() }, + Merchants: { getMerchantsByUsername: jest.fn() }, })) -import { CouldNotFindAccountFromUsernameError } from "@domain/errors" +import fs from "fs" +import path from "path" + +import { + graphql, + parse, + validate, + GraphQLSchema, + GraphQLObjectType, + GraphQLFieldConfig, +} from "graphql" + +import { CouldNotFindAccountFromNpubError } from "@domain/errors" import AccountDetailsByNpubQuery from "@graphql/admin/root/query/account-details-by-npub" const VALID_NPUB = "npub1" + "q".repeat(58) +const UPPERCASE_NPUB = "npub1" + "Q".repeat(58) +const MALFORMED_NPUB = "npub1toshort" -const resolveQuery = async (npub: unknown) => { - const resolve = AccountDetailsByNpubQuery.resolve as unknown as ( - source: null, - args: { npub: unknown }, - ctx: Record, - ) => Promise +/** + * The consumer's actual document — frappe-flash-admin + * `admin_panel/api/support_lookup.py` paints these fields onto a Chatwoot + * contact card. + */ +const SUPPORT_LOOKUP_QUERY = ` + query accountDetailsByNpub($npub: npub!) { + accountDetailsByNpub(npub: $npub) { + npub + username + level + owner { + phone + } + } + } +` - return resolve(null, { npub }, {}) +const literalQuery = (npub: string) => ` + query { + accountDetailsByNpub(npub: "${npub}") { + npub + username + level + owner { + phone + } + } + } +` + +// One-field schema over the REAL field definition, so the scalar and the +// resolver both run. Two things this deliberately is not. It is not +// `buildSchema` over the checked-in SDL: that yields a default resolver and a +// stub scalar, and would pass with the feature deleted. And it is not +// `@graphql/admin/queries`: importing that barrel drags in the cash-wallet +// cutover query, which constructs Redis clients at import time and hangs the +// runner — registration is asserted against the committed SDL instead. +const adminSchema = () => + new GraphQLSchema({ + query: new GraphQLObjectType({ + name: "Query", + fields: { + accountDetailsByNpub: AccountDetailsByNpubQuery as unknown as GraphQLFieldConfig< + unknown, + unknown + >, + }, + }), + }) + +const account = { + id: "account-id", + uuid: "5a9f6f45-0a3a-4b0a-9f3e-1e0f9b1b1b1b", + username: "jaceth2009", + npub: VALID_NPUB, + level: 1, + kratosUserId: "kratos-user-id", +} + +const owner = { + id: "kratos-user-id", + phone: "+18765550100", + language: "en", + createdAt: new Date(1787340000 * 1000), } -describe("accountDetailsByNpub", () => { +describe("admin accountDetailsByNpub", () => { beforeEach(() => { jest.clearAllMocks() + mockGetUser.mockResolvedValue(owner) + }) + + it("is exposed on the published admin schema", () => { + // Guards the registration in src/graphql/admin/queries.ts. Unwire it and + // every resolver-level test still passes green, so assert the field the + // consumer actually calls exists on the committed SDL — which `yarn + // check:sdl` keeps in lockstep with the code. + const sdl = fs.readFileSync( + path.join(__dirname, "../../../../../src/graphql/admin/schema.graphql"), + "utf8", + ) + expect(sdl).toContain("accountDetailsByNpub(npub: npub!): AuditedAccount!") + expect(sdl).toContain("scalar npub") }) - it("returns the account for a known npub", async () => { - const account = { id: "account-id", username: "jaceth2009", npub: VALID_NPUB } + it("validates the consumer's document against the real field", () => { + expect(validate(adminSchema(), parse(SUPPORT_LOOKUP_QUERY))).toHaveLength(0) + }) + + it("resolves the support-lookup document through a variable", async () => { mockGetAccountByNpub.mockResolvedValue(account) - const result = await resolveQuery(VALID_NPUB) + const result = await graphql({ + schema: adminSchema(), + source: SUPPORT_LOOKUP_QUERY, + variableValues: { npub: VALID_NPUB }, + }) + expect(result.errors).toBeUndefined() expect(mockGetAccountByNpub).toHaveBeenCalledWith(VALID_NPUB) - expect(result).toBe(account) + expect(result.data?.accountDetailsByNpub).toEqual({ + npub: VALID_NPUB, + username: "jaceth2009", + level: "ONE", + owner: { phone: "+18765550100" }, + }) }) - it("throws a mapped error when no account matches", async () => { - mockGetAccountByNpub.mockResolvedValue( - new CouldNotFindAccountFromUsernameError(VALID_NPUB), - ) + it("resolves the same npub written as an inline literal", async () => { + // THE REGRESSION. `parseLiteral` returned undefined on its success branch, + // which graphql-js reads as failed coercion: every inline-literal npub was + // rejected at validation time with "Expected value of type npub!" — a + // perfectly valid key reported as malformed to anyone in GraphiQL. + mockGetAccountByNpub.mockResolvedValue(account) + + const source = literalQuery(VALID_NPUB) + expect(validate(adminSchema(), parse(source))).toHaveLength(0) + + const result = await graphql({ schema: adminSchema(), source }) - await expect(resolveQuery(VALID_NPUB)).rejects.toThrow() + expect(result.errors).toBeUndefined() + expect(mockGetAccountByNpub).toHaveBeenCalledWith(VALID_NPUB) + expect(result.data?.accountDetailsByNpub).toMatchObject({ npub: VALID_NPUB }) }) - it("rethrows scalar validation errors without hitting the app layer", async () => { - const validationError = new Error("Invalid value for Npub") + it("normalises case identically on both coercion paths", async () => { + // The repository query is a plain `$eq` with no collation, so the scalar is + // the only thing standing between a mixed-case npub and a false not-found. + mockGetAccountByNpub.mockResolvedValue(account) - await expect(resolveQuery(validationError)).rejects.toThrow( - "Invalid value for Npub", - ) + await graphql({ + schema: adminSchema(), + source: SUPPORT_LOOKUP_QUERY, + variableValues: { npub: UPPERCASE_NPUB }, + }) + await graphql({ schema: adminSchema(), source: literalQuery(UPPERCASE_NPUB) }) + + expect(mockGetAccountByNpub).toHaveBeenNthCalledWith(1, VALID_NPUB) + expect(mockGetAccountByNpub).toHaveBeenNthCalledWith(2, VALID_NPUB) + }) + + it("rejects a malformed npub variable before it reaches the app layer", async () => { + const result = await graphql({ + schema: adminSchema(), + source: SUPPORT_LOOKUP_QUERY, + variableValues: { npub: MALFORMED_NPUB }, + }) + + expect(result.errors?.length).toBeGreaterThan(0) + expect(mockGetAccountByNpub).not.toHaveBeenCalled() + }) + + it("rejects a malformed npub literal before it reaches the app layer", async () => { + const result = await graphql({ + schema: adminSchema(), + source: literalQuery(MALFORMED_NPUB), + }) + + expect(result.errors?.length).toBeGreaterThan(0) expect(mockGetAccountByNpub).not.toHaveBeenCalled() }) + + it("reports a miss as an npub miss, not a username miss", async () => { + // The not-found path of a support-desk lookup lands in logs and in front of + // humans. Before the fix the repository returned + // CouldNotFindAccountFromUsernameError, so the operator was told + // "Account does not exist for username npub1…". + mockGetAccountByNpub.mockResolvedValue( + new CouldNotFindAccountFromNpubError(VALID_NPUB), + ) + + const result = await graphql({ + schema: adminSchema(), + source: SUPPORT_LOOKUP_QUERY, + variableValues: { npub: VALID_NPUB }, + }) + + expect(result.errors?.[0].message).toBe( + `Account does not exist for npub ${VALID_NPUB}`, + ) + expect(result.errors?.[0].message).not.toContain("username") + // The consumer keys off the NOT_FOUND code, so the 404 path is unchanged. + expect(result.errors?.[0].extensions?.code).toBe("NOT_FOUND") + }) }) diff --git a/test/flash/unit/services/mongoose/account-npub-index.spec.ts b/test/flash/unit/services/mongoose/account-npub-index.spec.ts new file mode 100644 index 000000000..e2fcd2c3c --- /dev/null +++ b/test/flash/unit/services/mongoose/account-npub-index.spec.ts @@ -0,0 +1,23 @@ +import { Account } from "@services/mongoose/schema" + +describe("accounts.npub index", () => { + it("is unique, so an npub can only resolve to one account", () => { + // Without this, `setNpub`'s duplicate check is racy and `findByNpub` — now + // a support-desk identity resolver — can return either of two colliding + // accounts. The migration 20260824120000-accounts-unique-npub builds it in + // prod after deduping. + const npubIndex = Account.schema + .indexes() + .find(([fields]) => Object.keys(fields).join(",") === "npub") + + expect(npubIndex).toBeDefined() + + const [, options] = npubIndex as [Record, Record] + expect(options.unique).toBe(true) + // `partialFilterExpression`, not `sparse`: a sparse index still indexes + // documents holding an explicit `npub: null`, and the second such document + // would collide. + expect(options.partialFilterExpression).toEqual({ npub: { $type: "string" } }) + expect(options.sparse).toBeUndefined() + }) +}) diff --git a/test/flash/unit/services/mongoose/accounts-find-by-npub.spec.ts b/test/flash/unit/services/mongoose/accounts-find-by-npub.spec.ts new file mode 100644 index 000000000..29a09672d --- /dev/null +++ b/test/flash/unit/services/mongoose/accounts-find-by-npub.spec.ts @@ -0,0 +1,60 @@ +import { CouldNotFindAccountFromNpubError } from "@domain/errors" +import { AccountsRepository } from "@services/mongoose/accounts" + +const findOne = jest.fn() + +jest.mock("@services/mongoose/schema", () => ({ + Account: { findOne: (...args: unknown[]) => findOne(...args) }, +})) + +jest.mock("@services/mongoose/utils", () => ({ + toObjectId: jest.fn((id) => id), + fromObjectId: jest.fn((id) => id), + parseRepositoryError: jest.fn((err) => err), +})) + +const NPUB = ("npub1" + "q".repeat(58)) as Npub + +const accountRecord = { + _id: "account-id", + id: "5a9f6f45-0a3a-4b0a-9f3e-1e0f9b1b1b1b", + created_at: new Date(), + npub: NPUB, + username: "jaceth2009", + level: 1, + statusHistory: [{ status: "active" }], + contacts: [], + earn: [], +} + +describe("AccountsRepository.findByNpub", () => { + beforeEach(() => { + findOne.mockReset() + }) + + it("queries npub with a plain $eq and no collation", async () => { + // `findOne` here resolves directly — it exposes no `.collation()`. That is + // the assertion: a non-simple collation cannot use the unique `{ npub: 1 }` + // index, so every support-desk lookup would be a collection scan. The old + // implementation chained `.collation({ locale: "en", strength: 2 })` and + // would blow up on this mock. + findOne.mockResolvedValue(accountRecord) + + const result = await AccountsRepository().findByNpub(NPUB) + + expect(findOne).toHaveBeenCalledWith({ npub: { $eq: NPUB } }) + expect(result).not.toBeInstanceOf(Error) + expect((result as Account).npub).toBe(NPUB) + }) + + it("reports a miss as an npub miss, not a username miss", async () => { + // The wrong error class renders as "Account does not exist for username + // npub1…" in logs and in front of a support agent. + findOne.mockResolvedValue(null) + + const result = await AccountsRepository().findByNpub(NPUB) + + expect(result).toBeInstanceOf(CouldNotFindAccountFromNpubError) + expect((result as Error).message).toBe(NPUB) + }) +}) From 2ff50979a7dd8dabc862745d5137dc26dbd8d788 Mon Sep 17 00:00:00 2001 From: Dread Date: Mon, 24 Aug 2026 17:23:17 -0700 Subject: [PATCH 3/9] fix(admin): stop the npub migration cementing the wrong owner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review fixes on the accounts.npub uniqueness work. - Migration no longer picks a duplicate-group "winner" by created_at. The oldest account is often an abandoned one left behind by a phone reset, while the live handset holding the nostr key is newer — and picking wrong is unrecoverable in-product, since setNpub refuses an already-claimed npub and no admin mutation can release one. Every account in the group now has npub unset and logged; first re-link wins, and the unique index makes that race safe. The header comment stops promising a re-link path the code refuses and documents the literal mongo recovery command instead. - The lowercase repair now targets the ids the audit scan already collected instead of `{ npub: { $type: "string" } }`, which rewrote every npub-bearing account — oplog churn and index re-touching during the deploy window for zero additional repairs. - setNpub translates a lost concurrent-write race into NpubNotAvailableError. The unique index raises E11000, which parseRepositoryError turns into DuplicateKeyForPersistError, which error-map buckets into UnexpectedClientError — telling a user who lost a benign race that the backend broke, and logging it as unexpected. - Adds test/flash/unit/migrations/accounts-unique-npub.spec.ts. Both destructive branches were unreachable in CI (`make test-migrate` runs against a clean database), so the first `$unset: { npub: "" }` would have been against real customer identities. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_015xSxhf5Kijfib4PbExGESk --- src/app/accounts/set-npub.ts | 15 +- .../20260824120000-accounts-unique-npub.ts | 58 ++-- test/flash/unit/app/accounts/set-npub.spec.ts | 26 +- .../migrations/accounts-unique-npub.spec.ts | 254 ++++++++++++++++++ 4 files changed, 332 insertions(+), 21 deletions(-) create mode 100644 test/flash/unit/migrations/accounts-unique-npub.spec.ts diff --git a/src/app/accounts/set-npub.ts b/src/app/accounts/set-npub.ts index e3f7dfd68..68cadc3f5 100644 --- a/src/app/accounts/set-npub.ts +++ b/src/app/accounts/set-npub.ts @@ -1,5 +1,5 @@ import { checkedToNpub, NpubNotAvailableError } from "@domain/nostr" -import { CouldNotFindError } from "@domain/errors" +import { CouldNotFindError, DuplicateKeyForPersistError } from "@domain/errors" import { AccountsRepository } from "@services/mongoose" export const setNpub = async ({ @@ -29,5 +29,16 @@ export const setNpub = async ({ const account = await accountsRepo.findById(id) if (account instanceof Error) return account account.npub = npubChecked - return accountsRepo.update(account) + + const updated = await accountsRepo.update(account) + // The probe above and this write are not atomic, so two concurrent claims on + // the same npub can both pass it. The loser's write trips the unique index, + // which `parseRepositoryError` surfaces as `DuplicateKeyForPersistError` — + // an error `error-map` buckets into `UnexpectedClientError` ("please contact + // support"). It is not unexpected: it is the same refusal as the probe, and + // the caller deserves the same answer. + if (updated instanceof DuplicateKeyForPersistError) { + return new NpubNotAvailableError(npubChecked) + } + return updated } diff --git a/src/migrations/20260824120000-accounts-unique-npub.ts b/src/migrations/20260824120000-accounts-unique-npub.ts index e655b2b58..d08fcd1a0 100644 --- a/src/migrations/20260824120000-accounts-unique-npub.ts +++ b/src/migrations/20260824120000-accounts-unique-npub.ts @@ -21,10 +21,14 @@ * case-insensitive collation which has been dropped (bech32 is a * lowercase-only charset, and the collation blocked index use), so stored * values must be normalised or they stop being findable. - * 2. Audits for duplicate npubs. For each group it keeps the OLDEST account - * (earliest created_at) and UNSETS npub on the rest — accounts are never - * deleted or merged here. Every unset is logged with account id + npub so - * support can reach out and have the losing owners re-link. + * 2. Audits for duplicate npubs. When a group is found, npub is UNSET on EVERY + * account in it — including the oldest. Accounts are never deleted or + * merged here. Guessing an owner by created_at is worse than releasing the + * key: the oldest account is often an abandoned one left behind by a phone + * reset, while the live handset actually holding the nostr secret key is + * the newer account. Once the unique index exists, whoever re-links first + * wins, and the index makes that race safe. Every unset is logged with + * account id + npub so support can tell the owners to re-link from the app. * 3. Creates the unique partial index. * * `partialFilterExpression: { npub: { $type: "string" } }` rather than @@ -36,6 +40,17 @@ * Drops the unique index. The lowercasing and the unsets are NOT reverted — * they are data repairs, and restoring known-ambiguous npubs would reintroduce * the identity collision. + * + * Manual recovery + * --------------- + * `setNpub` refuses an npub already held by another account + * (`NpubNotAvailableError`) and the admin API exposes no mutation that can + * unset or reassign one. So if an npub ends up on the wrong account, the only + * way to free it is a hand-written write against mongo: + * + * db.accounts.updateOne({ id: "" }, { $unset: { npub: "" } }) + * + * After that, the rightful owner re-links from the app. */ const COLLECTION = "accounts" @@ -80,14 +95,18 @@ module.exports = { } if (mixedCase.length > 0) { - await col.updateMany({ npub: { $type: "string" } }, [ + // Scoped to the ids just collected. Filtering on `{ npub: { $type: + // "string" } }` instead would rewrite every npub-bearing account — + // oplog churn and index re-touching during the deploy window for zero + // additional repairs. + await col.updateMany({ _id: { $in: mixedCase.map((d) => d._id) } }, [ { $set: { npub: { $toLower: "$npub" } } }, ]) } console.log(`[migration] Normalised ${mixedCase.length} npub value(s) to lowercase.`) - // ── Step 2: find and resolve duplicate npub groups ─────────────────────── - // Only the fields needed to pick a winner are pushed — `$$ROOT` would risk + // ── Step 2: find and release duplicate npub groups ─────────────────────── + // Only the fields needed for the audit log are pushed — `$$ROOT` would risk // the 16MB per-group limit on a large accounts collection. const duplicates = await col .aggregate( @@ -108,27 +127,30 @@ module.exports = { if (duplicates.length > 0) { console.log( - `[migration] Found ${duplicates.length} npub(s) claimed by more than one account. Resolving...`, + `[migration] Found ${duplicates.length} npub(s) claimed by more than one account. Releasing...`, ) for (const group of duplicates) { - // Oldest account keeps the npub — it is the likeliest original owner. - const sorted = group.docs.sort( - (a, b) => new Date(a.created_at).getTime() - new Date(b.created_at).getTime(), - ) - const [winner, ...losers] = sorted - const loserIds = losers.map((d) => d._id) + // Every account in the group loses the npub, including the oldest. + // There is no way to tell from mongo which account still holds the + // nostr secret key, and picking wrong is unrecoverable in-product: + // `setNpub` refuses an already-claimed npub and no admin mutation can + // release one. Releasing the key lets the real owner re-link from the + // app; the unique index makes the re-link race safe. + const ids = group.docs.map((d) => d._id) console.log( - `[migration] npub=${group._id} — keeping accountId=${winner.id} (_id=${winner._id}), unsetting npub on ${loserIds.length} account(s): ${losers - .map((d) => d.id) + `[migration] npub=${group._id} — releasing from ${ids.length} account(s): ${group.docs + .map((d) => `${d.id} (_id=${d._id}, created_at=${d.created_at})`) .join(", ")}`, ) - await col.updateMany({ _id: { $in: loserIds } }, { $unset: { npub: "" } }) + await col.updateMany({ _id: { $in: ids } }, { $unset: { npub: "" } }) } - console.log("[migration] Duplicate npub resolution complete.") + console.log( + "[migration] Duplicate npub release complete. Affected owners must re-link from the app.", + ) } else { console.log("[migration] No duplicate npub values found. Proceeding.") } diff --git a/test/flash/unit/app/accounts/set-npub.spec.ts b/test/flash/unit/app/accounts/set-npub.spec.ts index f0f21a82d..2736367f1 100644 --- a/test/flash/unit/app/accounts/set-npub.spec.ts +++ b/test/flash/unit/app/accounts/set-npub.spec.ts @@ -13,7 +13,11 @@ jest.mock("@services/mongoose", () => ({ AccountsRepository: () => ({ findByNpub, findById, update }), })) -import { CouldNotFindAccountFromNpubError, UnknownRepositoryError } from "@domain/errors" +import { + CouldNotFindAccountFromNpubError, + DuplicateKeyForPersistError, + UnknownRepositoryError, +} from "@domain/errors" import { InvalidNpubError, NpubNotAvailableError } from "@domain/nostr" import { setNpub } from "@app/accounts/set-npub" @@ -69,6 +73,26 @@ describe("Accounts.setNpub", () => { expect(update).toHaveBeenCalledWith({ id: ACCOUNT_ID, npub: NPUB }) }) + it("reports the lost concurrent-write race as NpubNotAvailable, not an unexpected error", async () => { + // Both callers pass the probe; the loser's write trips the unique index. + // `DuplicateKeyForPersistError` maps to UnexpectedClientError ("contact + // support") at the GraphQL edge — the wrong answer for a benign race. + update.mockResolvedValue(new DuplicateKeyForPersistError()) + + const result = await setNpub({ id: ACCOUNT_ID, npub: NPUB }) + + expect(result).toBeInstanceOf(NpubNotAvailableError) + expect(result).not.toBeInstanceOf(DuplicateKeyForPersistError) + }) + + it("passes through other repository failures from the write", async () => { + update.mockResolvedValue(new UnknownRepositoryError("mongo down")) + + const result = await setNpub({ id: ACCOUNT_ID, npub: NPUB }) + + expect(result).toBeInstanceOf(UnknownRepositoryError) + }) + it("does not claim the npub when the uniqueness probe itself fails", async () => { // A repository failure is not evidence that the npub is free. findByNpub.mockResolvedValue(new UnknownRepositoryError("mongo down")) diff --git a/test/flash/unit/migrations/accounts-unique-npub.spec.ts b/test/flash/unit/migrations/accounts-unique-npub.spec.ts new file mode 100644 index 000000000..721182893 --- /dev/null +++ b/test/flash/unit/migrations/accounts-unique-npub.spec.ts @@ -0,0 +1,254 @@ +/* eslint @typescript-eslint/no-var-requires: "off" */ + +/** + * The two destructive branches of the `accounts.npub` migration — case + * normalisation and duplicate release — are unreachable in CI: `make + * test-migrate` runs against a clean database, so the collection is empty and + * both branches short-circuit. Without this file, the first execution of + * `$unset: { npub: "" }` would be against real customer identities. + * + * The stub below is a miniature mongo: `updateMany` actually mutates the + * documents, so the pipeline's ordering (lowercase, then group) is exercised + * rather than asserted on canned return values. + */ + +type Doc = { + _id: string + id: string + npub?: string + created_at?: string +} + +type IndexSpec = { name: string; unique?: boolean; key: Record } + +const isDuplicatePipeline = (pipeline: Record[]) => + pipeline.some((stage) => "$group" in stage) + +const makeDb = ({ docs, indexes }: { docs: Doc[]; indexes?: IndexSpec[] }) => { + const state = { docs, indexes: indexes ?? ([] as IndexSpec[]) } + + const aggregate = jest.fn((pipeline: Record[]) => ({ + toArray: async () => { + const withNpub = state.docs.filter((d) => typeof d.npub === "string") + + if (!isDuplicatePipeline(pipeline)) { + return withNpub + .filter((d) => d.npub !== (d.npub as string).toLowerCase()) + .map((d) => ({ _id: d._id, id: d.id, npub: d.npub })) + } + + const groups = new Map() + for (const d of withNpub) { + const key = d.npub as string + groups.set(key, [...(groups.get(key) ?? []), d]) + } + return [...groups.entries()] + .filter(([, members]) => members.length > 1) + .map(([npub, members]) => ({ + _id: npub, + count: members.length, + docs: members.map((d) => ({ + _id: d._id, + id: d.id, + created_at: d.created_at, + })), + })) + }, + })) + + const updateMany = jest.fn( + async ( + filter: { _id: { $in: string[] } }, + update: Record | Record[], + ) => { + const targeted = filter._id.$in + state.docs = state.docs.map((doc) => { + if (!targeted.includes(doc._id)) return doc + if (Array.isArray(update)) return { ...doc, npub: doc.npub?.toLowerCase() } + const released = { ...doc } + delete released.npub + return released + }) + return { modifiedCount: targeted.length } + }, + ) + + const createIndex = jest.fn( + async (key: Record, opts: { name: string; unique?: boolean }) => { + state.indexes = [...state.indexes, { key, ...opts }] + return opts.name + }, + ) + + const dropIndex = jest.fn(async (name: string) => { + state.indexes = state.indexes.filter((idx) => idx.name !== name) + }) + + const collection = jest.fn(() => ({ + aggregate, + updateMany, + createIndex, + dropIndex, + indexes: async () => state.indexes, + })) + + const db = { + collection, + listCollections: () => ({ toArray: async () => [{ name: "accounts" }] }), + } + + return { db, state, aggregate, updateMany, createIndex, dropIndex } +} + +const migration = require("../../../../src/migrations/20260824120000-accounts-unique-npub") + +const LOWER = "npub1" + "q".repeat(58) +const MIXED = "npub1" + "Q".repeat(58) + +describe("migration: accounts unique npub", () => { + beforeEach(() => { + jest.spyOn(console, "log").mockImplementation(() => undefined) + }) + + afterEach(() => { + jest.restoreAllMocks() + }) + + it("lowercases only the offending documents", async () => { + const { db, state, updateMany } = makeDb({ + docs: [ + { _id: "1", id: "acct-1", npub: MIXED, created_at: "2024-01-01" }, + { + _id: "2", + id: "acct-2", + npub: "npub1" + "z".repeat(58), + created_at: "2024-01-01", + }, + ], + }) + + await migration.up(db) + + expect(state.docs.find((d) => d._id === "1")?.npub).toEqual(LOWER) + // The write is scoped to the ids the audit scan found — not to every + // npub-bearing account, which would churn the oplog for zero repairs. + expect(updateMany).toHaveBeenCalledWith({ _id: { $in: ["1"] } }, [ + { $set: { npub: { $toLower: "$npub" } } }, + ]) + }) + + it("does not write at all when every npub is already lowercase", async () => { + const { db, updateMany } = makeDb({ + docs: [{ _id: "1", id: "acct-1", npub: LOWER, created_at: "2024-01-01" }], + }) + + await migration.up(db) + + expect(updateMany).not.toHaveBeenCalled() + }) + + it("lowercases before the duplicate scan, so case-variant collisions are caught", async () => { + const { db, state } = makeDb({ + docs: [ + { _id: "1", id: "acct-1", npub: MIXED, created_at: "2024-01-01" }, + { _id: "2", id: "acct-2", npub: LOWER, created_at: "2025-01-01" }, + ], + }) + + await migration.up(db) + + // Same key once normalised — both must be released. + expect(state.docs.every((d) => d.npub === undefined)).toBe(true) + }) + + it("releases the npub from EVERY account in a duplicate group, including the oldest", async () => { + // Picking a winner by created_at is unrecoverable when it picks wrong: + // `setNpub` refuses an already-claimed npub and no admin mutation can + // release one, so the loser could never re-link. + const { db, state, updateMany } = makeDb({ + docs: [ + { _id: "old", id: "acct-old", npub: LOWER, created_at: "2024-01-01" }, + { _id: "new", id: "acct-new", npub: LOWER, created_at: "2025-01-01" }, + ], + }) + + await migration.up(db) + + expect(state.docs.map((d) => d.npub)).toEqual([undefined, undefined]) + expect(updateMany).toHaveBeenCalledWith( + { _id: { $in: ["old", "new"] } }, + { $unset: { npub: "" } }, + ) + }) + + it("leaves uncontested npubs alone", async () => { + const { db, state } = makeDb({ + docs: [ + { _id: "1", id: "acct-1", npub: LOWER, created_at: "2024-01-01" }, + { + _id: "2", + id: "acct-2", + npub: "npub1" + "z".repeat(58), + created_at: "2024-01-01", + }, + { _id: "3", id: "acct-3", created_at: "2024-01-01" }, + ], + }) + + await migration.up(db) + + expect(state.docs.map((d) => d.npub)).toEqual([ + LOWER, + "npub1" + "z".repeat(58), + undefined, + ]) + }) + + it("creates the unique index with the string-typed partial filter", async () => { + const { db, createIndex } = makeDb({ docs: [] }) + + await migration.up(db) + + // `sparse: true` would still index documents holding an explicit + // `npub: null`, and the second such document would collide. + expect(createIndex).toHaveBeenCalledWith( + { npub: 1 }, + { + unique: true, + name: "npub_1", + partialFilterExpression: { npub: { $type: "string" } }, + }, + ) + }) + + it("drops a stale non-unique npub_1 before creating the unique one", async () => { + const { db, dropIndex, state } = makeDb({ + docs: [], + indexes: [{ name: "npub_1", key: { npub: 1 } }], + }) + + await migration.up(db) + + expect(dropIndex).toHaveBeenCalledWith("npub_1") + expect(state.indexes.filter((i) => i.name === "npub_1")).toEqual([ + { + key: { npub: 1 }, + unique: true, + name: "npub_1", + partialFilterExpression: { npub: { $type: "string" } }, + }, + ]) + }) + + it("down drops the unique index and leaves the data repairs in place", async () => { + const { db, state, updateMany } = makeDb({ + docs: [{ _id: "1", id: "acct-1", created_at: "2024-01-01" }], + indexes: [{ name: "npub_1", key: { npub: 1 }, unique: true }], + }) + + await migration.down(db) + + expect(state.indexes).toEqual([]) + expect(updateMany).not.toHaveBeenCalled() + }) +}) From 09bc5e1f77f60a6b5ba55aa6ac59791dee2d3c4e Mon Sep 17 00:00:00 2001 From: Dread Date: Mon, 24 Aug 2026 17:41:27 -0700 Subject: [PATCH 4/9] fix(admin): make an npub claim revocable, and normalise the public lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings on the npub work. An npub claim is now permanently unique and still carries no proof of key control. `userUpdateNpub` takes a bare npub from any authenticated account, so anyone can read a victim's npub off a public relay and claim it on a throwaway account first; `setNpub` then refuses the real owner forever, and the support desk resolves the victim's DMs to the squatter's contact card. The admin surface was read-only, so the only remedy was a hand-written `$unset` against prod mongo. Adds `accountReleaseNpub` to the admin schema, backed by `Accounts.releaseNpub` and a repository `unsetNpub` — `update` cannot clear the field because mongoose strips undefined keys from an update doc. Releasing is not reassigning: the key goes back to unclaimed and whoever holds the secret re-links from the app. The migration header's manual-recovery recipe is replaced by a pointer at the mutation. `Accounts.findByNpub` is the twin of `Admin.getAccountByNpub` but never got the same validation. This branch dropped the case-insensitive collation from the repository query, which made normalisation mandatory — and this path had none, so any non-GraphQL caller (script, backfill, REST shim) passing a mixed-case npub got a silent not-found on a real user, surfacing as `isFlashNpub: false`. It now runs `checkedToNpub` like its admin twin, and moved out of the barrel so it can be unit tested against a mocked repository. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_015xSxhf5Kijfib4PbExGESk --- src/app/accounts/find-by-npub.ts | 20 ++++ src/app/accounts/index.ts | 6 +- src/app/accounts/release-npub.ts | 23 ++++ src/domain/accounts/index.types.d.ts | 1 + src/graphql/admin/mutations.ts | 2 + .../root/mutation/account-release-npub.ts | 46 ++++++++ src/graphql/admin/schema.graphql | 5 + .../20260824120000-accounts-unique-npub.ts | 15 +-- src/services/mongoose/accounts.ts | 20 ++++ .../unit/app/accounts/find-by-npub.spec.ts | 47 ++++++++ .../unit/app/accounts/release-npub.spec.ts | 55 +++++++++ .../admin/account-release-npub.spec.ts | 106 ++++++++++++++++++ .../mongoose/accounts-unset-npub.spec.ts | 59 ++++++++++ 13 files changed, 392 insertions(+), 13 deletions(-) create mode 100644 src/app/accounts/find-by-npub.ts create mode 100644 src/app/accounts/release-npub.ts create mode 100644 src/graphql/admin/root/mutation/account-release-npub.ts create mode 100644 test/flash/unit/app/accounts/find-by-npub.spec.ts create mode 100644 test/flash/unit/app/accounts/release-npub.spec.ts create mode 100644 test/flash/unit/graphql/admin/account-release-npub.spec.ts create mode 100644 test/flash/unit/services/mongoose/accounts-unset-npub.spec.ts diff --git a/src/app/accounts/find-by-npub.ts b/src/app/accounts/find-by-npub.ts new file mode 100644 index 000000000..564324053 --- /dev/null +++ b/src/app/accounts/find-by-npub.ts @@ -0,0 +1,20 @@ +import { checkedToNpub } from "@domain/nostr" +import { AccountsRepository } from "@services/mongoose" + +/** + * Twin of `Admin.getAccountByNpub`, normalised for the same reason: the + * repository query is a plain `$eq` with no collation, so a caller that is not + * the GraphQL boundary (a script, a backfill, a REST shim) passing a mixed-case + * npub gets a silent not-found on a real user — which the public + * `isFlashNpub` query reports as `isFlashNpub: false`. + * + * Lives outside `index.ts` so it can be unit tested against a mocked + * repository — importing the accounts barrel drags in the notification stack. + */ +export const findByNpub = async (npub: Npub): Promise => { + const npubChecked = checkedToNpub(npub) + if (npubChecked instanceof Error) return npubChecked + + const accounts = AccountsRepository() + return accounts.findByNpub(npubChecked) +} diff --git a/src/app/accounts/index.ts b/src/app/accounts/index.ts index f29ff4e21..a7b9b6407 100644 --- a/src/app/accounts/index.ts +++ b/src/app/accounts/index.ts @@ -33,6 +33,8 @@ export * from "./enable-notification-category" export * from "./enable-notification-channel" export * from "./disable-notification-channel" export * from "./set-npub" +export * from "./release-npub" +export * from "./find-by-npub" export * from "./update-external-wallet" const accounts = AccountsRepository() @@ -43,10 +45,6 @@ export const getAccount = async ( return accounts.findById(accountId) } -export const findByNpub = async (npub: Npub): Promise => { - return accounts.findByNpub(npub) -} - export const getAccountFromUserId = async ( kratosUserId: UserId, ): Promise => { diff --git a/src/app/accounts/release-npub.ts b/src/app/accounts/release-npub.ts new file mode 100644 index 000000000..f8499bfab --- /dev/null +++ b/src/app/accounts/release-npub.ts @@ -0,0 +1,23 @@ +import { checkedToAccountId } from "@domain/accounts" +import { AccountsRepository } from "@services/mongoose" + +/** + * The escape hatch for `setNpub`'s refusal. `userUpdateNpub` takes a bare npub + * from any authenticated account with no proof of key control, so anyone can + * read a victim's npub off a public relay and claim it first. The unique index + * then makes that permanent: the rightful owner gets `NpubNotAvailableError` + * forever, and the support desk resolves their DMs to the squatter's contact + * card. Support needs to be able to free the key from the admin panel rather + * than hand-writing an `$unset` against prod mongo. + * + * Releasing is deliberately not reassigning: the key goes back to unclaimed and + * whoever actually holds the secret re-links from the app. + */ +export const releaseNpub = async (id: string): Promise => { + const accountsRepo = AccountsRepository() + + const idChecked = checkedToAccountId(id) + if (idChecked instanceof Error) return idChecked + + return accountsRepo.unsetNpub(idChecked) +} diff --git a/src/domain/accounts/index.types.d.ts b/src/domain/accounts/index.types.d.ts index 0017d6e19..987326ad0 100644 --- a/src/domain/accounts/index.types.d.ts +++ b/src/domain/accounts/index.types.d.ts @@ -196,6 +196,7 @@ interface IAccountsRepository { findByUsername(username: Username): Promise // listBusinessesForMap(): Promise findByNpub(npub: Npub): Promise + unsetNpub(accountId: AccountId): Promise update(account: Account): Promise transitionBridgeKycStatus( diff --git a/src/graphql/admin/mutations.ts b/src/graphql/admin/mutations.ts index e20b3a4c8..ebd7ad7a9 100644 --- a/src/graphql/admin/mutations.ts +++ b/src/graphql/admin/mutations.ts @@ -2,6 +2,7 @@ import { GT } from "@graphql/index" import AccountUpdateLevelMutation from "@graphql/admin/root/mutation/account-update-level" import AccountUpdateStatusMutation from "@graphql/admin/root/mutation/account-update-status" +import AccountReleaseNpubMutation from "@graphql/admin/root/mutation/account-release-npub" import BusinessUpdateMapInfoMutation from "@graphql/admin/root/mutation/business-update-map-info" import CashWalletCutoverUpdateMutation from "@graphql/admin/root/mutation/cash-wallet-cutover-update" import CashWalletCutoverRollbackMutation from "@graphql/admin/root/mutation/cash-wallet-cutover-rollback" @@ -21,6 +22,7 @@ export const mutationFields = { userUpdatePhone: UserUpdatePhoneMutation, accountUpdateLevel: AccountUpdateLevelMutation, accountUpdateStatus: AccountUpdateStatusMutation, + accountReleaseNpub: AccountReleaseNpubMutation, merchantMapValidate: MerchantMapValidateMutation, merchantMapDelete: MerchantMapDeleteMutation, businessUpdateMapInfo: BusinessUpdateMapInfoMutation, diff --git a/src/graphql/admin/root/mutation/account-release-npub.ts b/src/graphql/admin/root/mutation/account-release-npub.ts new file mode 100644 index 000000000..82d30aaaa --- /dev/null +++ b/src/graphql/admin/root/mutation/account-release-npub.ts @@ -0,0 +1,46 @@ +import { GT } from "@graphql/index" + +import AccountDetailPayload from "@graphql/admin/types/payload/account-detail" +import { Accounts } from "@app" +import { mapAndParseErrorForGqlResponse } from "@graphql/error-map" + +const AccountReleaseNpubInput = GT.Input({ + name: "AccountReleaseNpubInput", + fields: () => ({ + accountId: { + type: GT.NonNullID, + }, + }), +}) + +const AccountReleaseNpubMutation = GT.Field< + null, + GraphQLAdminContext, + { + input: { accountId: string | Error } + } +>({ + extensions: { + complexity: 120, + }, + type: GT.NonNull(AccountDetailPayload), + args: { + input: { type: GT.NonNull(AccountReleaseNpubInput) }, + }, + resolve: async (_, args) => { + const { accountId } = args.input + + if (accountId instanceof Error) { + return { errors: [{ message: accountId.message }] } + } + + const account = await Accounts.releaseNpub(accountId) + if (account instanceof Error) { + return { errors: [mapAndParseErrorForGqlResponse(account)] } + } + + return { errors: [], accountDetails: account } + }, +}) + +export default AccountReleaseNpubMutation diff --git a/src/graphql/admin/schema.graphql b/src/graphql/admin/schema.graphql index 05b5ef67b..bf3c10f75 100644 --- a/src/graphql/admin/schema.graphql +++ b/src/graphql/admin/schema.graphql @@ -26,6 +26,10 @@ enum AccountLevel { ZERO } +input AccountReleaseNpubInput { + accountId: ID! +} + enum AccountStatus { ACTIVE CLOSED @@ -429,6 +433,7 @@ type MerchantPayload { } type Mutation { + accountReleaseNpub(input: AccountReleaseNpubInput!): AccountDetailPayload! accountUpdateLevel(input: AccountUpdateLevelInput!): AccountDetailPayload! accountUpdateStatus(input: AccountUpdateStatusInput!): AccountDetailPayload! businessDeleteMapInfo(input: BusinessDeleteMapInfoInput!): AccountDetailPayload! diff --git a/src/migrations/20260824120000-accounts-unique-npub.ts b/src/migrations/20260824120000-accounts-unique-npub.ts index d08fcd1a0..2666e45d2 100644 --- a/src/migrations/20260824120000-accounts-unique-npub.ts +++ b/src/migrations/20260824120000-accounts-unique-npub.ts @@ -41,16 +41,13 @@ * they are data repairs, and restoring known-ambiguous npubs would reintroduce * the identity collision. * - * Manual recovery - * --------------- + * Recovery + * -------- * `setNpub` refuses an npub already held by another account - * (`NpubNotAvailableError`) and the admin API exposes no mutation that can - * unset or reassign one. So if an npub ends up on the wrong account, the only - * way to free it is a hand-written write against mongo: - * - * db.accounts.updateOne({ id: "" }, { $unset: { npub: "" } }) - * - * After that, the rightful owner re-links from the app. + * (`NpubNotAvailableError`), so once the index exists a wrong or squatted claim + * locks the rightful owner out. Support frees the key from the admin panel with + * the `accountReleaseNpub` mutation, which unsets it on the holding account; the + * owner then re-links from the app. No hand-written write against prod mongo. */ const COLLECTION = "accounts" diff --git a/src/services/mongoose/accounts.ts b/src/services/mongoose/accounts.ts index b89ebd765..c708cbda0 100644 --- a/src/services/mongoose/accounts.ts +++ b/src/services/mongoose/accounts.ts @@ -95,6 +95,25 @@ export const AccountsRepository = (): IAccountsRepository => { } } + // An npub claim is permanently unique and carries no proof of key control, so + // support needs a way to hand one back. `update` cannot do it: mongoose strips + // undefined keys from an update doc, so clearing the field takes an explicit + // $unset — and the partial index only covers string npubs, so the released key + // is immediately re-claimable by whoever actually holds the secret key. + const unsetNpub = async (accountId: AccountId): Promise => { + try { + const result = await Account.findOneAndUpdate( + { _id: toObjectId(accountId) }, + { $unset: { npub: "" } }, + { new: true }, + ) + if (!result) return new CouldNotFindAccountError() + return translateToAccount(result) + } catch (err) { + return parseRepositoryError(err) + } + } + const update = async ({ id, level, @@ -296,6 +315,7 @@ export const AccountsRepository = (): IAccountsRepository => { findByUuid, findByUsername, findByNpub, + unsetNpub, update, transitionBridgeKycStatus, updateBridgeFields, diff --git a/test/flash/unit/app/accounts/find-by-npub.spec.ts b/test/flash/unit/app/accounts/find-by-npub.spec.ts new file mode 100644 index 000000000..557676dbf --- /dev/null +++ b/test/flash/unit/app/accounts/find-by-npub.spec.ts @@ -0,0 +1,47 @@ +/** + * `Accounts.findByNpub` is the twin of `Admin.getAccountByNpub` but never got + * the same normalisation. Once the case-insensitive collation came off the + * repository query, normalising here stopped being optional. + */ +const findByNpub = jest.fn() + +jest.mock("@services/mongoose", () => ({ + AccountsRepository: () => ({ findByNpub }), +})) + +import { InvalidNpubError } from "@domain/nostr" +import { findByNpub as findAccountByNpub } from "@app/accounts/find-by-npub" + +const NPUB = ("npub1" + "q".repeat(58)) as Npub + +describe("Accounts.findByNpub", () => { + beforeEach(() => { + findByNpub.mockReset() + }) + + it("hands a valid npub to the repository", async () => { + const account = { id: "account-id", npub: NPUB } + findByNpub.mockResolvedValue(account) + + expect(await findAccountByNpub(NPUB)).toBe(account) + expect(findByNpub).toHaveBeenCalledWith(NPUB) + }) + + it("normalises case before querying", async () => { + // The repository query is a plain `$eq` with no collation. Unnormalised, + // a mixed-case npub from a script or a backfill is a silent not-found on a + // real user — which the public `isFlashNpub` query reports as false. + findByNpub.mockResolvedValue({ id: "account-id" }) + + await findAccountByNpub(("npub1" + "Q".repeat(58)) as Npub) + + expect(findByNpub).toHaveBeenCalledWith(NPUB) + }) + + it("rejects a malformed npub instead of querying with it", async () => { + const result = await findAccountByNpub("not-an-npub" as Npub) + + expect(result).toBeInstanceOf(InvalidNpubError) + expect(findByNpub).not.toHaveBeenCalled() + }) +}) diff --git a/test/flash/unit/app/accounts/release-npub.spec.ts b/test/flash/unit/app/accounts/release-npub.spec.ts new file mode 100644 index 000000000..77f202215 --- /dev/null +++ b/test/flash/unit/app/accounts/release-npub.spec.ts @@ -0,0 +1,55 @@ +/** + * An npub claim carries no proof of key control and, since the unique index, + * is permanent: `setNpub` refuses an npub already held by another account, so + * whoever claims a key first keeps it — including someone who read it off a + * public relay. `releaseNpub` is the revocation path. Before it, freeing a + * squatted key meant a hand-written `$unset` against prod mongo. + */ +const unsetNpub = jest.fn() + +jest.mock("@services/mongoose", () => ({ + AccountsRepository: () => ({ unsetNpub }), +})) + +import { InvalidAccountIdError } from "@domain/accounts" +import { CouldNotFindAccountError, UnknownRepositoryError } from "@domain/errors" +import { releaseNpub } from "@app/accounts/release-npub" + +const ACCOUNT_ID = "5f4c9a2b1e7d3f8a6b0c4d2e" + +describe("Accounts.releaseNpub", () => { + beforeEach(() => { + unsetNpub.mockReset().mockResolvedValue({ id: ACCOUNT_ID, username: "jaceth2009" }) + }) + + it("clears the npub on the holding account", async () => { + const result = await releaseNpub(ACCOUNT_ID) + + expect(unsetNpub).toHaveBeenCalledWith(ACCOUNT_ID) + expect(result).not.toBeInstanceOf(Error) + expect((result as Account).npub).toBeUndefined() + }) + + it("reports an unknown account instead of reporting a release", async () => { + unsetNpub.mockResolvedValue(new CouldNotFindAccountError()) + + const result = await releaseNpub(ACCOUNT_ID) + + expect(result).toBeInstanceOf(CouldNotFindAccountError) + }) + + it("rejects a malformed account id without writing", async () => { + const result = await releaseNpub("not-an-account-id") + + expect(result).toBeInstanceOf(InvalidAccountIdError) + expect(unsetNpub).not.toHaveBeenCalled() + }) + + it("passes through a repository failure", async () => { + // A failed write is not a release: the operator must not be told the key is + // free and send the owner off to re-link. + unsetNpub.mockResolvedValue(new UnknownRepositoryError("mongo down")) + + expect(await releaseNpub(ACCOUNT_ID)).toBeInstanceOf(UnknownRepositoryError) + }) +}) diff --git a/test/flash/unit/graphql/admin/account-release-npub.spec.ts b/test/flash/unit/graphql/admin/account-release-npub.spec.ts new file mode 100644 index 000000000..65961485a --- /dev/null +++ b/test/flash/unit/graphql/admin/account-release-npub.spec.ts @@ -0,0 +1,106 @@ +/** + * `accountReleaseNpub` is the admin-side half of the npub squat remedy: without + * it the admin surface is read-only and a wrongly-claimed npub can only be + * freed by hand-editing prod mongo. These tests pin the resolver's contract and + * its registration on the published admin schema. + */ +const mockReleaseNpub = jest.fn() + +jest.mock("@app", () => ({ + Accounts: { + releaseNpub: (...args: unknown[]) => mockReleaseNpub(...args), + getAccountCapabilities: jest.fn(), + }, + Admin: { getAccountByNpub: jest.fn() }, + Users: { getUser: jest.fn() }, + Wallets: { listWalletsByAccountId: jest.fn() }, + Merchants: { getMerchantsByUsername: jest.fn() }, +})) + +import fs from "fs" +import path from "path" + +import { InvalidAccountIdError } from "@domain/accounts" +import { CouldNotFindAccountError } from "@domain/errors" +import AccountReleaseNpubMutation from "@graphql/admin/root/mutation/account-release-npub" + +const ACCOUNT_ID = "5f4c9a2b1e7d3f8a6b0c4d2e" + +type Result = { + errors: { message: string; code?: string }[] + accountDetails?: { id: string; npub?: string } +} + +// Mirrors what graphql-admin-server's Apollo `context` fn builds from the +// decoded admin JWT. +const adminContext = () => ({ + logger: { error: jest.fn() }, + user: { id: "support-user-id", roles: ["support"], ip: "127.0.0.1" }, +}) + +const resolveMutation = async (input: Record): Promise => { + const resolve = AccountReleaseNpubMutation.resolve as unknown as ( + source: null, + args: { input: Record }, + ctx: Record, + ) => Promise + + return resolve(null, { input }, adminContext()) +} + +describe("accountReleaseNpub", () => { + beforeEach(() => { + jest.clearAllMocks() + mockReleaseNpub.mockResolvedValue({ id: ACCOUNT_ID, username: "jaceth2009" }) + }) + + it("is exposed on the published admin schema", () => { + // Guards the registration in src/graphql/admin/mutations.ts — unwire it and + // every resolver-level test here still passes green. + const sdl = fs.readFileSync( + path.join(__dirname, "../../../../../src/graphql/admin/schema.graphql"), + "utf8", + ) + expect(sdl).toContain( + "accountReleaseNpub(input: AccountReleaseNpubInput!): AccountDetailPayload!", + ) + expect(sdl).toContain("input AccountReleaseNpubInput {") + }) + + it("releases the npub and returns the account detail", async () => { + const result = await resolveMutation({ accountId: ACCOUNT_ID }) + + expect(mockReleaseNpub).toHaveBeenCalledWith(ACCOUNT_ID) + expect(result.errors).toEqual([]) + expect(result.accountDetails?.npub).toBeUndefined() + }) + + it("reports an unknown account instead of reporting a release", async () => { + // A support agent told the key is free would send the rightful owner off to + // re-link, and `setNpub` would refuse them again. + mockReleaseNpub.mockResolvedValue(new CouldNotFindAccountError()) + + const result = await resolveMutation({ accountId: ACCOUNT_ID }) + + expect(result.accountDetails).toBeUndefined() + expect(result.errors).toHaveLength(1) + expect(result.errors[0].message).toContain("CouldNotFindAccountError") + }) + + it("reports a malformed account id", async () => { + mockReleaseNpub.mockResolvedValue(new InvalidAccountIdError("not-an-account-id")) + + const result = await resolveMutation({ accountId: "not-an-account-id" }) + + expect(result.accountDetails).toBeUndefined() + expect(result.errors).toHaveLength(1) + expect(result.errors[0].message).toContain("InvalidAccountIdError") + }) + + it("surfaces an input coercion failure without calling the app layer", async () => { + const result = await resolveMutation({ accountId: new Error("Invalid value for ID") }) + + expect(result.errors).toEqual([{ message: "Invalid value for ID" }]) + expect(mockReleaseNpub).not.toHaveBeenCalled() + }) +}) diff --git a/test/flash/unit/services/mongoose/accounts-unset-npub.spec.ts b/test/flash/unit/services/mongoose/accounts-unset-npub.spec.ts new file mode 100644 index 000000000..d72c9e989 --- /dev/null +++ b/test/flash/unit/services/mongoose/accounts-unset-npub.spec.ts @@ -0,0 +1,59 @@ +import { CouldNotFindAccountError } from "@domain/errors" +import { AccountsRepository } from "@services/mongoose/accounts" + +const findOneAndUpdate = jest.fn() + +jest.mock("@services/mongoose/schema", () => ({ + Account: { findOneAndUpdate: (...args: unknown[]) => findOneAndUpdate(...args) }, +})) + +jest.mock("@services/mongoose/utils", () => ({ + toObjectId: jest.fn((id) => id), + fromObjectId: jest.fn((id) => id), + parseRepositoryError: jest.fn((err) => err), +})) + +const ACCOUNT_ID = "5f4c9a2b1e7d3f8a6b0c4d2e" as AccountId + +const accountRecord = { + _id: ACCOUNT_ID, + id: "5a9f6f45-0a3a-4b0a-9f3e-1e0f9b1b1b1b", + created_at: new Date(), + username: "jaceth2009", + level: 1, + statusHistory: [{ status: "active" }], + contacts: [], + earn: [], +} + +describe("AccountsRepository.unsetNpub", () => { + beforeEach(() => { + findOneAndUpdate.mockReset() + }) + + it("removes the field rather than blanking it", async () => { + // `$unset`, not `npub: undefined` (mongoose strips undefined keys from an + // update doc, so that is a silent no-op) and not `npub: null` (the partial + // index excludes non-strings, so a null would sit there unindexed and keep + // failing lookups). Removed means the key is genuinely unclaimed again. + findOneAndUpdate.mockResolvedValue(accountRecord) + + const result = await AccountsRepository().unsetNpub(ACCOUNT_ID) + + expect(findOneAndUpdate).toHaveBeenCalledWith( + { _id: ACCOUNT_ID }, + { $unset: { npub: "" } }, + { new: true }, + ) + expect(result).not.toBeInstanceOf(Error) + expect((result as Account).npub).toBeUndefined() + }) + + it("reports an unknown account", async () => { + findOneAndUpdate.mockResolvedValue(null) + + expect(await AccountsRepository().unsetNpub(ACCOUNT_ID)).toBeInstanceOf( + CouldNotFindAccountError, + ) + }) +}) From 8b7aa2113c2326711e5dff262b9363a0f3f7207e Mon Sep 17 00:00:00 2001 From: Dread Date: Mon, 24 Aug 2026 18:11:00 -0700 Subject: [PATCH 5/9] fix(admin): attribute, harden and complete the npub release path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on accountReleaseNpub. Attribution: the resolver dropped `ctx`, so nothing recorded who freed which key. The admin server's Apollo context never assigns `req.gqlContext`, so its Pino line logs the actor as undefined, and neither the account document nor the payload retained the npub that was removed — a SystemManager, or anyone with a stolen admin JWT, could release a victim's npub and re-claim it from a throwaway via `userUpdateNpub` with no trace. The resolver now passes `ctx.user.id` down as `releasedByUserId` (matching userUpdatePhone / accountUpdateStatus / cashWalletCutoverRollback), the app layer reads the account first and emits a structured `baseLogger.info({ accountId, previousNpub, releasedByUserId })`, and `previousNpub` comes back on the payload. Registration guard: the spec read the checked-in SDL off disk, which proves nothing about wiring — `MutationType` spreads `unauthed` and `authed` into identical SDL, so moving the field out of the shield-guarded bucket was invisible to both that test and `check:sdl`. It now asserts against `mutationFields.authed` / `mutationFields.unauthed` directly. Reassignment: release-then-re-link pits a human against the squatter's script, since `userUpdateNpub` needs no proof of key control. `reassignToAccountId` hands the freed key straight to the rightful owner. This repository has no MongoDB sessions anywhere, so the two writes are not atomic; the residual window is documented rather than claimed away, the target is validated before anything is freed, and the unique partial index is what guarantees the claim cannot collide. Silent no-op release: `$unset` on an account holding no npub still matches `_id`, so `findOneAndUpdate` returned the document and the operator was told the release succeeded — then sent the customer off to re-link, where `setNpub` refuses them because the squatter still holds the key. The update now runs with `new: false` and refuses with `NoNpubToReleaseError` when the pre-update document carries no npub. Operator-facing errors: both realistic mistakes surfaced as "contact support" or a leaked class name. Adds `CouldNotFindAccountFromIdError` mapped to `NotFoundError` and moves `InvalidAccountIdError` into the `ValidationInternalError` bucket; the tests now assert the user-facing code and message instead of pinning the internal class name. Vacuous assertions: three `expect(...npub).toBeUndefined()` checks ran against fixtures that never carried an npub. The mocks now hold one and the assertions verify the code drops it. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_015xSxhf5Kijfib4PbExGESk --- src/app/accounts/release-npub.ts | 94 ++++++++++- src/domain/accounts/index.types.d.ts | 5 +- src/domain/errors.ts | 5 + src/domain/nostr/errors.ts | 2 + .../root/mutation/account-release-npub.ts | 38 +++-- src/graphql/admin/schema.graphql | 10 +- .../types/payload/account-release-npub.ts | 27 ++++ src/graphql/error-map.ts | 17 +- src/services/mongoose/accounts.ts | 36 ++++- .../unit/app/accounts/release-npub.spec.ts | 151 ++++++++++++++++-- .../admin/account-release-npub.spec.ts | 139 +++++++++++++--- .../mongoose/accounts-unset-npub.spec.ts | 50 +++++- 12 files changed, 514 insertions(+), 60 deletions(-) create mode 100644 src/graphql/admin/types/payload/account-release-npub.ts diff --git a/src/app/accounts/release-npub.ts b/src/app/accounts/release-npub.ts index f8499bfab..24267d85f 100644 --- a/src/app/accounts/release-npub.ts +++ b/src/app/accounts/release-npub.ts @@ -1,6 +1,20 @@ import { checkedToAccountId } from "@domain/accounts" +import { + CouldNotFindAccountFromIdError, + CouldNotFindError, + DuplicateKeyForPersistError, + NoNpubToReleaseError, +} from "@domain/errors" +import { AccountAlreadyHasNpubError, NpubNotAvailableError } from "@domain/nostr" +import { baseLogger } from "@services/logger" import { AccountsRepository } from "@services/mongoose" +export type NpubRelease = { + account: Account + previousNpub: Npub + reassignedTo?: Account +} + /** * The escape hatch for `setNpub`'s refusal. `userUpdateNpub` takes a bare npub * from any authenticated account with no proof of key control, so anyone can @@ -10,14 +24,86 @@ import { AccountsRepository } from "@services/mongoose" * card. Support needs to be able to free the key from the admin panel rather * than hand-writing an `$unset` against prod mongo. * - * Releasing is deliberately not reassigning: the key goes back to unclaimed and - * whoever actually holds the secret re-links from the app. + * A bare release loses the race it exists to win: the squatter is the party + * polling `isFlashNpub`, so telling the victim to go re-link pits a human + * against a script. `reassignToAccountId` closes that by handing the key + * straight to the rightful owner. This repository has no MongoDB sessions + * anywhere, so the two writes are not a transaction: the key is unclaimed for + * the round-trip between them, and a claim that lands in that window makes the + * reassignment fail with `NpubNotAvailableError` — the release still stands, so + * the operator must retry the reassignment rather than assume it applied. The + * target is read and checked before the release so that everything knowable up + * front fails before the key is freed; the unique partial index is what + * guarantees the reassignment cannot collide. + * + * `releasedByUserId` is the whole attribution trail. Neither the account + * document nor the payload retains the npub that was removed, and the admin + * server never assigns `req.gqlContext`, so the Pino request log records the + * actor as undefined — the structured log line below is the only record that a + * given admin took a given key off a given account. */ -export const releaseNpub = async (id: string): Promise => { +export const releaseNpub = async ({ + id, + releasedByUserId, + reassignToAccountId, +}: { + id: string + releasedByUserId: UserId + reassignToAccountId?: string +}): Promise => { const accountsRepo = AccountsRepository() const idChecked = checkedToAccountId(id) if (idChecked instanceof Error) return idChecked - return accountsRepo.unsetNpub(idChecked) + const targetIdChecked = + reassignToAccountId === undefined + ? undefined + : checkedToAccountId(reassignToAccountId) + if (targetIdChecked instanceof Error) return targetIdChecked + + const holder = await accountsRepo.findById(idChecked) + if (holder instanceof CouldNotFindError) { + return new CouldNotFindAccountFromIdError(idChecked) + } + if (holder instanceof Error) return holder + + const previousNpub = holder.npub + if (previousNpub === undefined) return new NoNpubToReleaseError(idChecked) + + let target: Account | undefined + if (targetIdChecked !== undefined) { + const found = await accountsRepo.findById(targetIdChecked) + if (found instanceof CouldNotFindError) { + return new CouldNotFindAccountFromIdError(targetIdChecked) + } + if (found instanceof Error) return found + // Also catches `reassignToAccountId === id`, where the target is the holder + // and there is nothing to move. + if (found.npub !== undefined) return new AccountAlreadyHasNpubError(targetIdChecked) + target = found + } + + const released = await accountsRepo.unsetNpub(idChecked) + if (released instanceof Error) return released + + baseLogger.info( + { + accountId: idChecked, + previousNpub, + releasedByUserId, + reassignedToAccountId: targetIdChecked, + }, + "admin released an npub claim", + ) + + if (target === undefined) return { account: released, previousNpub } + + const reassigned = await accountsRepo.claimNpub(target.id, previousNpub) + if (reassigned instanceof DuplicateKeyForPersistError) { + return new NpubNotAvailableError(previousNpub) + } + if (reassigned instanceof Error) return reassigned + + return { account: released, previousNpub, reassignedTo: reassigned } } diff --git a/src/domain/accounts/index.types.d.ts b/src/domain/accounts/index.types.d.ts index 987326ad0..2ad8e8dae 100644 --- a/src/domain/accounts/index.types.d.ts +++ b/src/domain/accounts/index.types.d.ts @@ -80,7 +80,9 @@ type Account = { readonly uuid: AccountUuid readonly createdAt: Date username: Username - npub: Npub + // Optional: an account holds an npub only once it links one, and `releaseNpub` + // takes it back off. + npub?: Npub defaultWalletId: WalletId withdrawFee: Satoshis // TODO: make it optional. only save when not default value from yaml level: AccountLevel @@ -197,6 +199,7 @@ interface IAccountsRepository { // listBusinessesForMap(): Promise findByNpub(npub: Npub): Promise unsetNpub(accountId: AccountId): Promise + claimNpub(accountId: AccountId, npub: Npub): Promise update(account: Account): Promise transitionBridgeKycStatus( diff --git a/src/domain/errors.ts b/src/domain/errors.ts index 7edc4f29a..b0257d407 100644 --- a/src/domain/errors.ts +++ b/src/domain/errors.ts @@ -61,9 +61,14 @@ export class CouldNotFindLnPaymentFromHashError extends CouldNotFindError { level = ErrorLevel.Critical } +export class CouldNotFindAccountFromIdError extends CouldNotFindError {} export class CouldNotFindAccountFromUuidError extends CouldNotFindError {} export class CouldNotFindAccountFromUsernameError extends CouldNotFindError {} export class CouldNotFindAccountFromNpubError extends CouldNotFindError {} +// An account that holds no npub has nothing to release. Distinct from +// "account not found" because `$unset` on a document without the field is a +// no-op that still matches, so the write alone cannot tell the two apart. +export class NoNpubToReleaseError extends CouldNotFindError {} export class CouldNotFindAccountFromPhoneError extends CouldNotFindError {} export class CouldNotFindMerchantFromUsernameError extends CouldNotFindError {} export class CouldNotFindMerchantFromIdError extends CouldNotFindError {} diff --git a/src/domain/nostr/errors.ts b/src/domain/nostr/errors.ts index 5c2448bb7..33521c912 100644 --- a/src/domain/nostr/errors.ts +++ b/src/domain/nostr/errors.ts @@ -5,3 +5,5 @@ export class NostrError extends DomainError {} export class InvalidNpubError extends ValidationError {} export class NpubNotAvailableError extends NostrError {} + +export class AccountAlreadyHasNpubError extends NostrError {} diff --git a/src/graphql/admin/root/mutation/account-release-npub.ts b/src/graphql/admin/root/mutation/account-release-npub.ts index 82d30aaaa..0558fb9c5 100644 --- a/src/graphql/admin/root/mutation/account-release-npub.ts +++ b/src/graphql/admin/root/mutation/account-release-npub.ts @@ -1,6 +1,6 @@ import { GT } from "@graphql/index" -import AccountDetailPayload from "@graphql/admin/types/payload/account-detail" +import AccountReleaseNpubPayload from "@graphql/admin/types/payload/account-release-npub" import { Accounts } from "@app" import { mapAndParseErrorForGqlResponse } from "@graphql/error-map" @@ -10,6 +10,11 @@ const AccountReleaseNpubInput = GT.Input({ accountId: { type: GT.NonNullID, }, + // Hands the freed key straight to the rightful owner. Omitted, the key goes + // back to unclaimed and whoever polls for it first gets it. + reassignToAccountId: { + type: GT.ID, + }, }), }) @@ -17,29 +22,40 @@ const AccountReleaseNpubMutation = GT.Field< null, GraphQLAdminContext, { - input: { accountId: string | Error } + input: { accountId: string | Error; reassignToAccountId?: string | Error } } >({ extensions: { complexity: 120, }, - type: GT.NonNull(AccountDetailPayload), + type: GT.NonNull(AccountReleaseNpubPayload), args: { input: { type: GT.NonNull(AccountReleaseNpubInput) }, }, - resolve: async (_, args) => { - const { accountId } = args.input + resolve: async (_, args, ctx) => { + const { accountId, reassignToAccountId } = args.input + const supportUser = ctx.user.id - if (accountId instanceof Error) { - return { errors: [{ message: accountId.message }] } + if (accountId instanceof Error) return { errors: [{ message: accountId.message }] } + if (reassignToAccountId instanceof Error) { + return { errors: [{ message: reassignToAccountId.message }] } } - const account = await Accounts.releaseNpub(accountId) - if (account instanceof Error) { - return { errors: [mapAndParseErrorForGqlResponse(account)] } + const released = await Accounts.releaseNpub({ + id: accountId, + releasedByUserId: supportUser, + reassignToAccountId, + }) + if (released instanceof Error) { + return { errors: [mapAndParseErrorForGqlResponse(released)] } } - return { errors: [], accountDetails: account } + return { + errors: [], + accountDetails: released.account, + previousNpub: released.previousNpub, + reassignedTo: released.reassignedTo, + } }, }) diff --git a/src/graphql/admin/schema.graphql b/src/graphql/admin/schema.graphql index bf3c10f75..e249d7385 100644 --- a/src/graphql/admin/schema.graphql +++ b/src/graphql/admin/schema.graphql @@ -28,6 +28,14 @@ enum AccountLevel { input AccountReleaseNpubInput { accountId: ID! + reassignToAccountId: ID +} + +type AccountReleaseNpubPayload { + accountDetails: AuditedAccount + errors: [Error] + previousNpub: String + reassignedTo: AuditedAccount } enum AccountStatus { @@ -433,7 +441,7 @@ type MerchantPayload { } type Mutation { - accountReleaseNpub(input: AccountReleaseNpubInput!): AccountDetailPayload! + accountReleaseNpub(input: AccountReleaseNpubInput!): AccountReleaseNpubPayload! accountUpdateLevel(input: AccountUpdateLevelInput!): AccountDetailPayload! accountUpdateStatus(input: AccountUpdateStatusInput!): AccountDetailPayload! businessDeleteMapInfo(input: BusinessDeleteMapInfoInput!): AccountDetailPayload! diff --git a/src/graphql/admin/types/payload/account-release-npub.ts b/src/graphql/admin/types/payload/account-release-npub.ts new file mode 100644 index 000000000..dbfc5c364 --- /dev/null +++ b/src/graphql/admin/types/payload/account-release-npub.ts @@ -0,0 +1,27 @@ +import { GT } from "@graphql/index" +import IError from "@graphql/shared/types/abstract/error" + +import GraphQLAccount from "../object/account" + +// Not `AccountDetailPayload`: `accountDetails` is the account the key was taken +// off, so it no longer carries the npub, and nothing else in the response says +// which key was freed or where it went. +const AccountReleaseNpubPayload = GT.Object({ + name: "AccountReleaseNpubPayload", + fields: () => ({ + errors: { + type: GT.List(IError), + }, + accountDetails: { + type: GraphQLAccount, + }, + previousNpub: { + type: GT.String, + }, + reassignedTo: { + type: GraphQLAccount, + }, + }), +}) + +export default AccountReleaseNpubPayload diff --git a/src/graphql/error-map.ts b/src/graphql/error-map.ts index 08981cb43..691ae8937 100644 --- a/src/graphql/error-map.ts +++ b/src/graphql/error-map.ts @@ -104,6 +104,10 @@ export const mapError = (error: ApplicationError): CustomApolloError => { message = `User does not exist for id ${error.message}` return new NotFoundError({ message, logger: baseLogger }) + case "CouldNotFindAccountFromIdError": + message = `Account does not exist for id ${error.message}` + return new NotFoundError({ message, logger: baseLogger }) + case "CouldNotFindAccountFromUuidError": message = `Account does not exist for uuid ${error.message}` return new NotFoundError({ message, logger: baseLogger }) @@ -116,6 +120,10 @@ export const mapError = (error: ApplicationError): CustomApolloError => { message = `Account does not exist for npub ${error.message}` return new NotFoundError({ message, logger: baseLogger }) + case "NoNpubToReleaseError": + message = `Account ${error.message} has no npub linked` + return new NotFoundError({ message, logger: baseLogger }) + case "CouldNotFindMerchantFromUsernameError": message = `Merchant does not exist for username ${error.message}` return new NotFoundError({ message, logger: baseLogger }) @@ -389,6 +397,14 @@ export const mapError = (error: ApplicationError): CustomApolloError => { message = "npub is already linked to another account" return new ValidationInternalError({ message, logger: baseLogger }) + case "AccountAlreadyHasNpubError": + message = "the receiving account already has an npub linked" + return new ValidationInternalError({ message, logger: baseLogger }) + + case "InvalidAccountIdError": + message = `Invalid account id ${error.message}` + return new ValidationInternalError({ message, logger: baseLogger }) + case "InvalidWalletId": message = "Invalid walletId for account." return new ValidationInternalError({ message, logger: baseLogger }) @@ -897,7 +913,6 @@ export const mapError = (error: ApplicationError): CustomApolloError => { case "CaptchaError": case "InvalidNonHodlInvoiceError": case "InvalidAccountError": - case "InvalidAccountIdError": case "InvalidMinutesError": case "InvalidWalletForAccountError": case "AuthenticationError": diff --git a/src/services/mongoose/accounts.ts b/src/services/mongoose/accounts.ts index c708cbda0..b4e38c98c 100644 --- a/src/services/mongoose/accounts.ts +++ b/src/services/mongoose/accounts.ts @@ -3,10 +3,12 @@ import { OnboardingEarn } from "@config" import { AccountStatus } from "@domain/accounts" import { CouldNotFindAccountError, + CouldNotFindAccountFromIdError, CouldNotFindAccountFromKratosIdError, CouldNotFindAccountFromNpubError, CouldNotFindAccountFromUsernameError, CouldNotFindAccountFromUuidError, + NoNpubToReleaseError, RepositoryError, } from "@domain/errors" import { UsdDisplayCurrency } from "@domain/fiat" @@ -100,14 +102,43 @@ export const AccountsRepository = (): IAccountsRepository => { // undefined keys from an update doc, so clearing the field takes an explicit // $unset — and the partial index only covers string npubs, so the released key // is immediately re-claimable by whoever actually holds the secret key. + // + // `new: false` is load-bearing. `$unset` against a document that never held an + // npub is a no-op that still matches on `_id`, so the post-update document is + // identical in both cases and the operator would be told a release happened + // when nothing was freed — then send the rightful owner off to re-link, where + // `setNpub` refuses them because the squatter still holds the key. The + // pre-update document is the only thing that can tell the two apart. const unsetNpub = async (accountId: AccountId): Promise => { try { - const result = await Account.findOneAndUpdate( + const before = await Account.findOneAndUpdate( { _id: toObjectId(accountId) }, { $unset: { npub: "" } }, + { new: false }, + ) + if (!before) return new CouldNotFindAccountFromIdError(accountId) + if (typeof before.npub !== "string") return new NoNpubToReleaseError(accountId) + return { ...translateToAccount(before), npub: undefined } + } catch (err) { + return parseRepositoryError(err) + } + } + + // The reassignment half of a release. A targeted `$set` rather than a + // read-modify-write through `update`, so the unique partial index is the only + // thing that decides whether the claim lands: a concurrent claimant trips it + // and `parseRepositoryError` surfaces `DuplicateKeyForPersistError`. + const claimNpub = async ( + accountId: AccountId, + npub: Npub, + ): Promise => { + try { + const result = await Account.findOneAndUpdate( + { _id: toObjectId(accountId) }, + { $set: { npub } }, { new: true }, ) - if (!result) return new CouldNotFindAccountError() + if (!result) return new CouldNotFindAccountFromIdError(accountId) return translateToAccount(result) } catch (err) { return parseRepositoryError(err) @@ -316,6 +347,7 @@ export const AccountsRepository = (): IAccountsRepository => { findByUsername, findByNpub, unsetNpub, + claimNpub, update, transitionBridgeKycStatus, updateBridgeFields, diff --git a/test/flash/unit/app/accounts/release-npub.spec.ts b/test/flash/unit/app/accounts/release-npub.spec.ts index 77f202215..df10ea891 100644 --- a/test/flash/unit/app/accounts/release-npub.spec.ts +++ b/test/flash/unit/app/accounts/release-npub.spec.ts @@ -5,43 +5,101 @@ * public relay. `releaseNpub` is the revocation path. Before it, freeing a * squatted key meant a hand-written `$unset` against prod mongo. */ +const findById = jest.fn() const unsetNpub = jest.fn() +const claimNpub = jest.fn() +const info = jest.fn() jest.mock("@services/mongoose", () => ({ - AccountsRepository: () => ({ unsetNpub }), + AccountsRepository: () => ({ findById, unsetNpub, claimNpub }), +})) + +jest.mock("@services/logger", () => ({ + baseLogger: { info: (...args: unknown[]) => info(...args) }, })) import { InvalidAccountIdError } from "@domain/accounts" -import { CouldNotFindAccountError, UnknownRepositoryError } from "@domain/errors" +import { + CouldNotFindAccountError, + CouldNotFindAccountFromIdError, + DuplicateKeyForPersistError, + NoNpubToReleaseError, + UnknownRepositoryError, +} from "@domain/errors" +import { AccountAlreadyHasNpubError, NpubNotAvailableError } from "@domain/nostr" import { releaseNpub } from "@app/accounts/release-npub" -const ACCOUNT_ID = "5f4c9a2b1e7d3f8a6b0c4d2e" +const ACCOUNT_ID = "5f4c9a2b1e7d3f8a6b0c4d2e" as AccountId +const TARGET_ACCOUNT_ID = "6a1b2c3d4e5f60718293a4b5" as AccountId +const SUPPORT_USER_ID = "support-user-id" as UserId +const NPUB = `npub1${"q".repeat(58)}` as Npub + +const release = (overrides: Record = {}) => + releaseNpub({ id: ACCOUNT_ID, releasedByUserId: SUPPORT_USER_ID, ...overrides }) describe("Accounts.releaseNpub", () => { beforeEach(() => { + findById + .mockReset() + .mockResolvedValue({ id: ACCOUNT_ID, username: "jaceth2009", npub: NPUB }) unsetNpub.mockReset().mockResolvedValue({ id: ACCOUNT_ID, username: "jaceth2009" }) + claimNpub.mockReset() + info.mockReset() }) - it("clears the npub on the holding account", async () => { - const result = await releaseNpub(ACCOUNT_ID) + it("clears the npub on the holding account and reports which key was freed", async () => { + const result = await release() expect(unsetNpub).toHaveBeenCalledWith(ACCOUNT_ID) expect(result).not.toBeInstanceOf(Error) - expect((result as Account).npub).toBeUndefined() + expect(result).toMatchObject({ previousNpub: NPUB }) + }) + + it("logs the actor, the account and the key that was removed", async () => { + // The only attribution that exists: the account document keeps no trace of + // a removed npub, and the admin server's Pino line logs `gqlContext.user` + // as undefined because its Apollo context never assigns `req.gqlContext`. + await release() + + expect(info).toHaveBeenCalledWith( + expect.objectContaining({ + accountId: ACCOUNT_ID, + previousNpub: NPUB, + releasedByUserId: SUPPORT_USER_ID, + }), + expect.any(String), + ) }) it("reports an unknown account instead of reporting a release", async () => { - unsetNpub.mockResolvedValue(new CouldNotFindAccountError()) + findById.mockResolvedValue(new CouldNotFindAccountError()) + + const result = await release() - const result = await releaseNpub(ACCOUNT_ID) + expect(result).toBeInstanceOf(CouldNotFindAccountFromIdError) + expect(unsetNpub).not.toHaveBeenCalled() + }) + + it("refuses an account that holds no npub", async () => { + // `$unset` on a document without the field is a no-op that still matches, + // so without this the operator is told a release happened and sends the + // owner off to re-link — where the squatter still holds the key. + findById.mockResolvedValue({ id: ACCOUNT_ID, username: "jaceth2009" }) + + const result = await release() - expect(result).toBeInstanceOf(CouldNotFindAccountError) + expect(result).toBeInstanceOf(NoNpubToReleaseError) + expect(unsetNpub).not.toHaveBeenCalled() }) it("rejects a malformed account id without writing", async () => { - const result = await releaseNpub("not-an-account-id") + const result = await releaseNpub({ + id: "not-an-account-id", + releasedByUserId: SUPPORT_USER_ID, + }) expect(result).toBeInstanceOf(InvalidAccountIdError) + expect(findById).not.toHaveBeenCalled() expect(unsetNpub).not.toHaveBeenCalled() }) @@ -50,6 +108,77 @@ describe("Accounts.releaseNpub", () => { // free and send the owner off to re-link. unsetNpub.mockResolvedValue(new UnknownRepositoryError("mongo down")) - expect(await releaseNpub(ACCOUNT_ID)).toBeInstanceOf(UnknownRepositoryError) + expect(await release()).toBeInstanceOf(UnknownRepositoryError) + }) + + describe("reassignment", () => { + beforeEach(() => { + findById.mockImplementation(async (id: AccountId) => + id === ACCOUNT_ID + ? { id: ACCOUNT_ID, username: "jaceth2009", npub: NPUB } + : { id: TARGET_ACCOUNT_ID, username: "rightful-owner" }, + ) + claimNpub.mockResolvedValue({ + id: TARGET_ACCOUNT_ID, + username: "rightful-owner", + npub: NPUB, + }) + }) + + it("hands the freed key to the target", async () => { + const result = await release({ reassignToAccountId: TARGET_ACCOUNT_ID }) + + expect(unsetNpub).toHaveBeenCalledWith(ACCOUNT_ID) + expect(claimNpub).toHaveBeenCalledWith(TARGET_ACCOUNT_ID, NPUB) + expect(result).toMatchObject({ + previousNpub: NPUB, + reassignedTo: { npub: NPUB }, + }) + }) + + it("refuses a target that already holds an npub, before freeing anything", async () => { + findById.mockImplementation(async (id: AccountId) => + id === ACCOUNT_ID + ? { id: ACCOUNT_ID, username: "jaceth2009", npub: NPUB } + : { id: TARGET_ACCOUNT_ID, username: "rightful-owner", npub: "npub1other" }, + ) + + const result = await release({ reassignToAccountId: TARGET_ACCOUNT_ID }) + + expect(result).toBeInstanceOf(AccountAlreadyHasNpubError) + expect(unsetNpub).not.toHaveBeenCalled() + }) + + it("refuses an unknown target, before freeing anything", async () => { + findById.mockImplementation(async (id: AccountId) => + id === ACCOUNT_ID + ? { id: ACCOUNT_ID, username: "jaceth2009", npub: NPUB } + : new CouldNotFindAccountError(), + ) + + const result = await release({ reassignToAccountId: TARGET_ACCOUNT_ID }) + + expect(result).toBeInstanceOf(CouldNotFindAccountFromIdError) + expect(unsetNpub).not.toHaveBeenCalled() + }) + + it("rejects a malformed target id without writing", async () => { + const result = await release({ reassignToAccountId: "not-an-account-id" }) + + expect(result).toBeInstanceOf(InvalidAccountIdError) + expect(unsetNpub).not.toHaveBeenCalled() + }) + + it("reports a claim lost in the window between the two writes", async () => { + // The release and the claim are not one transaction — this repository has + // no mongo sessions — so a squatter can take the key back in between. The + // unique index catches it; the operator must be told the reassignment did + // not land. + claimNpub.mockResolvedValue(new DuplicateKeyForPersistError()) + + const result = await release({ reassignToAccountId: TARGET_ACCOUNT_ID }) + + expect(result).toBeInstanceOf(NpubNotAvailableError) + }) }) }) diff --git a/test/flash/unit/graphql/admin/account-release-npub.spec.ts b/test/flash/unit/graphql/admin/account-release-npub.spec.ts index 65961485a..f066e0ebf 100644 --- a/test/flash/unit/graphql/admin/account-release-npub.spec.ts +++ b/test/flash/unit/graphql/admin/account-release-npub.spec.ts @@ -2,10 +2,20 @@ * `accountReleaseNpub` is the admin-side half of the npub squat remedy: without * it the admin surface is read-only and a wrongly-claimed npub can only be * freed by hand-editing prod mongo. These tests pin the resolver's contract and - * its registration on the published admin schema. + * its registration on the admin schema. */ const mockReleaseNpub = jest.fn() +// The admin mutation barrel is imported below for the registration guard, and +// it drags in siblings whose service graph opens a redis connection at import +// time — which never resolves in a unit run. +jest.mock("@services/redis/connection", () => ({ + redis: { on: jest.fn() }, + redisPubSub: { publish: jest.fn(), asyncIterator: jest.fn() }, + redisCache: { cache: jest.fn(), invalidate: jest.fn() }, + disconnectAll: jest.fn(), +})) + jest.mock("@app", () => ({ Accounts: { releaseNpub: (...args: unknown[]) => mockReleaseNpub(...args), @@ -17,18 +27,21 @@ jest.mock("@app", () => ({ Merchants: { getMerchantsByUsername: jest.fn() }, })) -import fs from "fs" -import path from "path" - import { InvalidAccountIdError } from "@domain/accounts" -import { CouldNotFindAccountError } from "@domain/errors" +import { CouldNotFindAccountFromIdError, NoNpubToReleaseError } from "@domain/errors" +import { AccountAlreadyHasNpubError } from "@domain/nostr" +import { mutationFields } from "@graphql/admin/mutations" import AccountReleaseNpubMutation from "@graphql/admin/root/mutation/account-release-npub" const ACCOUNT_ID = "5f4c9a2b1e7d3f8a6b0c4d2e" +const TARGET_ACCOUNT_ID = "6a1b2c3d4e5f60718293a4b5" +const NPUB = `npub1${"q".repeat(58)}` type Result = { errors: { message: string; code?: string }[] accountDetails?: { id: string; npub?: string } + previousNpub?: string + reassignedTo?: { id: string; npub?: string } } // Mirrors what graphql-admin-server's Apollo `context` fn builds from the @@ -51,50 +64,116 @@ const resolveMutation = async (input: Record): Promise describe("accountReleaseNpub", () => { beforeEach(() => { jest.clearAllMocks() - mockReleaseNpub.mockResolvedValue({ id: ACCOUNT_ID, username: "jaceth2009" }) + mockReleaseNpub.mockResolvedValue({ + account: { id: ACCOUNT_ID, username: "jaceth2009" }, + previousNpub: NPUB, + }) }) - it("is exposed on the published admin schema", () => { - // Guards the registration in src/graphql/admin/mutations.ts — unwire it and - // every resolver-level test here still passes green. - const sdl = fs.readFileSync( - path.join(__dirname, "../../../../../src/graphql/admin/schema.graphql"), - "utf8", + it("is registered as an authed admin mutation", () => { + // The source of truth, not the checked-in SDL: `MutationType` spreads + // `unauthed` and `authed` into identical SDL, so `check:sdl` stays green + // when the field is moved out of the graphql-shield-guarded bucket and + // becomes callable by any holder of a role-less ERPNext JWT. + expect(mutationFields.authed).toHaveProperty( + "accountReleaseNpub", + AccountReleaseNpubMutation, ) - expect(sdl).toContain( - "accountReleaseNpub(input: AccountReleaseNpubInput!): AccountDetailPayload!", - ) - expect(sdl).toContain("input AccountReleaseNpubInput {") + expect(mutationFields.unauthed).not.toHaveProperty("accountReleaseNpub") }) - it("releases the npub and returns the account detail", async () => { + it("releases the npub and reports which key was freed", async () => { const result = await resolveMutation({ accountId: ACCOUNT_ID }) - expect(mockReleaseNpub).toHaveBeenCalledWith(ACCOUNT_ID) + expect(mockReleaseNpub).toHaveBeenCalledWith({ + id: ACCOUNT_ID, + releasedByUserId: "support-user-id", + reassignToAccountId: undefined, + }) + expect(result.errors).toEqual([]) + expect(result.previousNpub).toBe(NPUB) + }) + + it("attributes the release to the calling admin", async () => { + // The account document keeps no trace of a removed npub, and the admin + // server never assigns `req.gqlContext`, so its Pino line logs the actor as + // undefined. Dropping `ctx` here would leave no record of who freed a key. + await resolveMutation({ accountId: ACCOUNT_ID }) + + expect(mockReleaseNpub.mock.calls[0][0]).toMatchObject({ + releasedByUserId: "support-user-id", + }) + }) + + it("passes the reassignment target through and returns the new holder", async () => { + mockReleaseNpub.mockResolvedValue({ + account: { id: ACCOUNT_ID, username: "jaceth2009" }, + previousNpub: NPUB, + reassignedTo: { id: TARGET_ACCOUNT_ID, npub: NPUB }, + }) + + const result = await resolveMutation({ + accountId: ACCOUNT_ID, + reassignToAccountId: TARGET_ACCOUNT_ID, + }) + + expect(mockReleaseNpub).toHaveBeenCalledWith({ + id: ACCOUNT_ID, + releasedByUserId: "support-user-id", + reassignToAccountId: TARGET_ACCOUNT_ID, + }) expect(result.errors).toEqual([]) - expect(result.accountDetails?.npub).toBeUndefined() + expect(result.reassignedTo?.npub).toBe(NPUB) }) - it("reports an unknown account instead of reporting a release", async () => { + it("reports an unknown account as a not-found, not an internal code", async () => { // A support agent told the key is free would send the rightful owner off to - // re-link, and `setNpub` would refuse them again. - mockReleaseNpub.mockResolvedValue(new CouldNotFindAccountError()) + // re-link, and `setNpub` would refuse them again. Both realistic operator + // mistakes — wrong id, and the uuid pasted where the ObjectId goes — used + // to surface as a leaked class name or "contact support". + mockReleaseNpub.mockResolvedValue(new CouldNotFindAccountFromIdError(ACCOUNT_ID)) const result = await resolveMutation({ accountId: ACCOUNT_ID }) expect(result.accountDetails).toBeUndefined() expect(result.errors).toHaveLength(1) - expect(result.errors[0].message).toContain("CouldNotFindAccountError") + expect(result.errors[0].code).toBe("NOT_FOUND") + expect(result.errors[0].message).toContain(ACCOUNT_ID) + expect(result.errors[0].message).not.toContain("contact support") + expect(result.errors[0].message).not.toContain("CouldNotFind") }) - it("reports a malformed account id", async () => { + it("reports a malformed account id as a validation failure", async () => { mockReleaseNpub.mockResolvedValue(new InvalidAccountIdError("not-an-account-id")) const result = await resolveMutation({ accountId: "not-an-account-id" }) expect(result.accountDetails).toBeUndefined() expect(result.errors).toHaveLength(1) - expect(result.errors[0].message).toContain("InvalidAccountIdError") + expect(result.errors[0].code).toBe("INVALID_INPUT") + expect(result.errors[0].message).not.toContain("contact support") + expect(result.errors[0].message).not.toContain("InvalidAccountIdError") + }) + + it("reports an account that holds no npub as a not-found", async () => { + mockReleaseNpub.mockResolvedValue(new NoNpubToReleaseError(ACCOUNT_ID)) + + const result = await resolveMutation({ accountId: ACCOUNT_ID }) + + expect(result.errors[0].code).toBe("NOT_FOUND") + expect(result.previousNpub).toBeUndefined() + }) + + it("reports a reassignment target that already holds a key", async () => { + mockReleaseNpub.mockResolvedValue(new AccountAlreadyHasNpubError(TARGET_ACCOUNT_ID)) + + const result = await resolveMutation({ + accountId: ACCOUNT_ID, + reassignToAccountId: TARGET_ACCOUNT_ID, + }) + + expect(result.errors[0].code).toBe("INVALID_INPUT") + expect(result.previousNpub).toBeUndefined() }) it("surfaces an input coercion failure without calling the app layer", async () => { @@ -103,4 +182,14 @@ describe("accountReleaseNpub", () => { expect(result.errors).toEqual([{ message: "Invalid value for ID" }]) expect(mockReleaseNpub).not.toHaveBeenCalled() }) + + it("surfaces a coercion failure on the reassignment target too", async () => { + const result = await resolveMutation({ + accountId: ACCOUNT_ID, + reassignToAccountId: new Error("Invalid value for ID"), + }) + + expect(result.errors).toEqual([{ message: "Invalid value for ID" }]) + expect(mockReleaseNpub).not.toHaveBeenCalled() + }) }) diff --git a/test/flash/unit/services/mongoose/accounts-unset-npub.spec.ts b/test/flash/unit/services/mongoose/accounts-unset-npub.spec.ts index d72c9e989..0981d6f1a 100644 --- a/test/flash/unit/services/mongoose/accounts-unset-npub.spec.ts +++ b/test/flash/unit/services/mongoose/accounts-unset-npub.spec.ts @@ -1,4 +1,4 @@ -import { CouldNotFindAccountError } from "@domain/errors" +import { CouldNotFindAccountFromIdError, NoNpubToReleaseError } from "@domain/errors" import { AccountsRepository } from "@services/mongoose/accounts" const findOneAndUpdate = jest.fn() @@ -14,6 +14,7 @@ jest.mock("@services/mongoose/utils", () => ({ })) const ACCOUNT_ID = "5f4c9a2b1e7d3f8a6b0c4d2e" as AccountId +const NPUB = `npub1${"q".repeat(58)}` as Npub const accountRecord = { _id: ACCOUNT_ID, @@ -36,16 +37,18 @@ describe("AccountsRepository.unsetNpub", () => { // update doc, so that is a silent no-op) and not `npub: null` (the partial // index excludes non-strings, so a null would sit there unindexed and keep // failing lookups). Removed means the key is genuinely unclaimed again. - findOneAndUpdate.mockResolvedValue(accountRecord) + findOneAndUpdate.mockResolvedValue({ ...accountRecord, npub: NPUB }) const result = await AccountsRepository().unsetNpub(ACCOUNT_ID) expect(findOneAndUpdate).toHaveBeenCalledWith( { _id: ACCOUNT_ID }, { $unset: { npub: "" } }, - { new: true }, + { new: false }, ) expect(result).not.toBeInstanceOf(Error) + // The document read back is the pre-update one, which still carries the + // npub; the account handed to the caller must not. expect((result as Account).npub).toBeUndefined() }) @@ -53,7 +56,46 @@ describe("AccountsRepository.unsetNpub", () => { findOneAndUpdate.mockResolvedValue(null) expect(await AccountsRepository().unsetNpub(ACCOUNT_ID)).toBeInstanceOf( - CouldNotFindAccountError, + CouldNotFindAccountFromIdError, + ) + }) + + it("refuses an account that held no npub", async () => { + // `$unset` on a document without the field is a no-op that still matches on + // `_id`, so the write itself reports success. Support pastes the wrong + // account id, is told the key is free, and sends the customer off to + // re-link — where `setNpub` refuses them because the squatter still has it. + findOneAndUpdate.mockResolvedValue(accountRecord) + + expect(await AccountsRepository().unsetNpub(ACCOUNT_ID)).toBeInstanceOf( + NoNpubToReleaseError, + ) + }) +}) + +describe("AccountsRepository.claimNpub", () => { + beforeEach(() => { + findOneAndUpdate.mockReset() + }) + + it("sets the key on the receiving account", async () => { + findOneAndUpdate.mockResolvedValue({ ...accountRecord, npub: NPUB }) + + const result = await AccountsRepository().claimNpub(ACCOUNT_ID, NPUB) + + expect(findOneAndUpdate).toHaveBeenCalledWith( + { _id: ACCOUNT_ID }, + { $set: { npub: NPUB } }, + { new: true }, + ) + expect((result as Account).npub).toBe(NPUB) + }) + + it("reports an unknown account", async () => { + findOneAndUpdate.mockResolvedValue(null) + + expect(await AccountsRepository().claimNpub(ACCOUNT_ID, NPUB)).toBeInstanceOf( + CouldNotFindAccountFromIdError, ) }) }) From c268f3bd3f4132cc982b3e990c70626a28649832 Mon Sep 17 00:00:00 2001 From: Dread Date: Mon, 24 Aug 2026 18:34:26 -0700 Subject: [PATCH 6/9] fix(admin): make a half-applied npub release legible, and unblock null-npub targets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The release and the reassignment are two writes with no transaction around them. Everything below follows from that, plus the audit trail being the only record the admin server keeps of who did what. A claim that fails after the release landed no longer collapses to a bare error. `releaseNpub` returns the populated `NpubRelease` carrying `reassignmentError`, and `AccountReleaseNpubPayload` gained a matching field, so `accountDetails` and `previousNpub` survive. Without them the operator was told only "npub is already linked to another account", could not tell the key had already left the holder, and found re-running the mutation refused with `NoNpubToReleaseError`. `previousNpub` is what they feed to `accountDetailsByNpub` to find the squatter and release it from there. The failure is also logged at error level. The release log line called the target `reassignedToAccountId` before the claim had been attempted, so a lost claim left an audit line asserting a reassignment that never happened. It is `reassignToAccountId` now — intent — and a second line records the actual outcome once the claim resolves. Every rejection path now logs `admin npub release refused` with the actor, the id as given, and a reason. A stolen admin token enumerating account ids used to leave one line for the id that happened to hold a key and nothing for the rest; the admin server's pino-http line cannot fill the gap, as it carries neither the actor nor the body. The target's npub check used `!== undefined` while the repository used `typeof !== "string"`. `AccountRecord.npub` is `Npub | null` and the migration deliberately leaves pre-existing `npub: null` documents alone, so a legacy account was rejected as already holding a key and could never receive a reassignment. Both layers check `typeof` now. The holder `findById` is gone. It was a second round-trip and the staler of the two reads: `unsetNpub` already reads the pre-update document to decide whether anything was freed, so it is the only reader that cannot disagree with the key the `$unset` removed. It returns `{ account, previousNpub }`. Target validation still runs before the release. The migration's duplicate-release comment claimed no admin mutation can release an npub, contradicting its own recovery section and this mutation. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_015xSxhf5Kijfib4PbExGESk --- src/app/accounts/release-npub.ts | 112 +++++++-- src/domain/accounts/index.types.d.ts | 11 +- .../root/mutation/account-release-npub.ts | 12 +- src/graphql/admin/schema.graphql | 1 + .../types/payload/account-release-npub.ts | 9 + .../20260824120000-accounts-unique-npub.ts | 9 +- src/services/mongoose/accounts.ts | 18 +- .../unit/app/accounts/release-npub.spec.ts | 237 +++++++++++++++--- .../admin/account-release-npub.spec.ts | 46 +++- .../mongoose/accounts-unset-npub.spec.ts | 15 +- 10 files changed, 400 insertions(+), 70 deletions(-) diff --git a/src/app/accounts/release-npub.ts b/src/app/accounts/release-npub.ts index 24267d85f..52a0cde1f 100644 --- a/src/app/accounts/release-npub.ts +++ b/src/app/accounts/release-npub.ts @@ -3,7 +3,6 @@ import { CouldNotFindAccountFromIdError, CouldNotFindError, DuplicateKeyForPersistError, - NoNpubToReleaseError, } from "@domain/errors" import { AccountAlreadyHasNpubError, NpubNotAvailableError } from "@domain/nostr" import { baseLogger } from "@services/logger" @@ -13,6 +12,10 @@ export type NpubRelease = { account: Account previousNpub: Npub reassignedTo?: Account + // Set when the release landed and the claim meant to follow it did not. This + // is not an alternative to the release: the key is off the holder either way, + // and re-running the mutation now fails with `NoNpubToReleaseError`. + reassignmentError?: ApplicationError } /** @@ -30,17 +33,24 @@ export type NpubRelease = { * straight to the rightful owner. This repository has no MongoDB sessions * anywhere, so the two writes are not a transaction: the key is unclaimed for * the round-trip between them, and a claim that lands in that window makes the - * reassignment fail with `NpubNotAvailableError` — the release still stands, so - * the operator must retry the reassignment rather than assume it applied. The - * target is read and checked before the release so that everything knowable up - * front fails before the key is freed; the unique partial index is what - * guarantees the reassignment cannot collide. + * reassignment fail with `NpubNotAvailableError`. The release still stands, so + * that failure comes back as `reassignmentError` on an otherwise populated + * `NpubRelease` rather than as a bare error — a bare error reads as "nothing + * happened", and the operator would neither know the key is now unclaimed nor + * that recovering it means finding its current holder with + * `accountDetailsByNpub` and releasing it from there. The target is read and + * checked before the release so that everything knowable up front fails before + * the key is freed; the unique partial index is what guarantees the + * reassignment cannot collide. * * `releasedByUserId` is the whole attribution trail. Neither the account * document nor the payload retains the npub that was removed, and the admin * server never assigns `req.gqlContext`, so the Pino request log records the - * actor as undefined — the structured log line below is the only record that a - * given admin took a given key off a given account. + * actor as undefined — the structured log lines below are the only record that + * a given admin took a given key off a given account. Refusals are logged for + * the same reason: a stolen admin token sweeping account ids leaves one line + * for the release that worked and, without them, nothing at all for the probes + * that did not. */ export const releaseNpub = async ({ id, @@ -53,57 +63,105 @@ export const releaseNpub = async ({ }): Promise => { const accountsRepo = AccountsRepository() + const refuse = (reason: string, error: E): E => { + baseLogger.warn( + { accountId: id, releasedByUserId, reassignToAccountId, reason }, + "admin npub release refused", + ) + return error + } + const idChecked = checkedToAccountId(id) - if (idChecked instanceof Error) return idChecked + if (idChecked instanceof Error) return refuse("malformed account id", idChecked) const targetIdChecked = reassignToAccountId === undefined ? undefined : checkedToAccountId(reassignToAccountId) - if (targetIdChecked instanceof Error) return targetIdChecked - - const holder = await accountsRepo.findById(idChecked) - if (holder instanceof CouldNotFindError) { - return new CouldNotFindAccountFromIdError(idChecked) + if (targetIdChecked instanceof Error) { + return refuse("malformed reassignment target id", targetIdChecked) } - if (holder instanceof Error) return holder - - const previousNpub = holder.npub - if (previousNpub === undefined) return new NoNpubToReleaseError(idChecked) let target: Account | undefined if (targetIdChecked !== undefined) { const found = await accountsRepo.findById(targetIdChecked) if (found instanceof CouldNotFindError) { - return new CouldNotFindAccountFromIdError(targetIdChecked) + return refuse( + "unknown reassignment target", + new CouldNotFindAccountFromIdError(targetIdChecked), + ) } if (found instanceof Error) return found // Also catches `reassignToAccountId === id`, where the target is the holder // and there is nothing to move. - if (found.npub !== undefined) return new AccountAlreadyHasNpubError(targetIdChecked) + // + // `typeof` rather than `!== undefined`: the field is `Npub | null` on the + // record and the migration deliberately leaves pre-existing `npub: null` + // documents alone, so an account that has never linked a key can arrive + // here holding an explicit null. That is not a claim, and treating it as + // one would make such an account permanently ineligible to receive one. + if (typeof found.npub === "string") { + return refuse( + "reassignment target already holds an npub", + new AccountAlreadyHasNpubError(targetIdChecked), + ) + } target = found } + // The holder's existence and its npub are both established by `unsetNpub` + // off the pre-update document, so this branch carries the refusals that a + // separate holder read used to make here — plus genuine write failures. const released = await accountsRepo.unsetNpub(idChecked) - if (released instanceof Error) return released + if (released instanceof Error) return refuse(released.name, released) + + const { account, previousNpub } = released baseLogger.info( { accountId: idChecked, previousNpub, releasedByUserId, - reassignedToAccountId: targetIdChecked, + // Intent, not outcome. The claim has not been attempted yet and can still + // lose to a concurrent one; an investigator reading this line must not + // conclude the key reached the target. + reassignToAccountId: targetIdChecked, }, "admin released an npub claim", ) - if (target === undefined) return { account: released, previousNpub } + if (target === undefined) return { account, previousNpub } const reassigned = await accountsRepo.claimNpub(target.id, previousNpub) - if (reassigned instanceof DuplicateKeyForPersistError) { - return new NpubNotAvailableError(previousNpub) + if (reassigned instanceof Error) { + baseLogger.error( + { + accountId: idChecked, + previousNpub, + reassignToAccountId: targetIdChecked, + releasedByUserId, + }, + "npub released but reassignment failed", + ) + return { + account, + previousNpub, + reassignmentError: + reassigned instanceof DuplicateKeyForPersistError + ? new NpubNotAvailableError(previousNpub) + : reassigned, + } } - if (reassigned instanceof Error) return reassigned - return { account: released, previousNpub, reassignedTo: reassigned } + baseLogger.info( + { + accountId: idChecked, + previousNpub, + releasedByUserId, + reassignedToAccountId: target.id, + }, + "admin reassigned a released npub", + ) + + return { account, previousNpub, reassignedTo: reassigned } } diff --git a/src/domain/accounts/index.types.d.ts b/src/domain/accounts/index.types.d.ts index 2ad8e8dae..c50b5858b 100644 --- a/src/domain/accounts/index.types.d.ts +++ b/src/domain/accounts/index.types.d.ts @@ -187,6 +187,15 @@ type AccountValidator = { validateWalletForAccount(wallet: Wallet): true | ValidationError } +// `unsetNpub` reads the pre-update document to decide whether anything was +// actually freed, so it is the only place the removed npub still exists — the +// updated document no longer carries it. Handing it back saves the caller a +// second read that could disagree with what the `$unset` removed. +type NpubUnset = { + account: Account + previousNpub: Npub +} + interface IAccountsRepository { listUnlockedAccounts(): AsyncGenerator | RepositoryError findById(accountId: AccountId): Promise @@ -198,7 +207,7 @@ interface IAccountsRepository { findByUsername(username: Username): Promise // listBusinessesForMap(): Promise findByNpub(npub: Npub): Promise - unsetNpub(accountId: AccountId): Promise + unsetNpub(accountId: AccountId): Promise claimNpub(accountId: AccountId, npub: Npub): Promise update(account: Account): Promise diff --git a/src/graphql/admin/root/mutation/account-release-npub.ts b/src/graphql/admin/root/mutation/account-release-npub.ts index 0558fb9c5..c1913e50b 100644 --- a/src/graphql/admin/root/mutation/account-release-npub.ts +++ b/src/graphql/admin/root/mutation/account-release-npub.ts @@ -50,11 +50,21 @@ const AccountReleaseNpubMutation = GT.Field< return { errors: [mapAndParseErrorForGqlResponse(released)] } } + // A post-release reassignment failure is reported twice on purpose: in + // `errors` because something did fail, and in `reassignmentError` because + // `errors` alone is indistinguishable from a mutation that changed nothing + // — and here the key has already left the holder. + const reassignmentError = + released.reassignmentError === undefined + ? undefined + : mapAndParseErrorForGqlResponse(released.reassignmentError) + return { - errors: [], + errors: reassignmentError === undefined ? [] : [reassignmentError], accountDetails: released.account, previousNpub: released.previousNpub, reassignedTo: released.reassignedTo, + reassignmentError, } }, }) diff --git a/src/graphql/admin/schema.graphql b/src/graphql/admin/schema.graphql index e249d7385..c3c7e53b2 100644 --- a/src/graphql/admin/schema.graphql +++ b/src/graphql/admin/schema.graphql @@ -36,6 +36,7 @@ type AccountReleaseNpubPayload { errors: [Error] previousNpub: String reassignedTo: AuditedAccount + reassignmentError: Error } enum AccountStatus { diff --git a/src/graphql/admin/types/payload/account-release-npub.ts b/src/graphql/admin/types/payload/account-release-npub.ts index dbfc5c364..95cb4a7a9 100644 --- a/src/graphql/admin/types/payload/account-release-npub.ts +++ b/src/graphql/admin/types/payload/account-release-npub.ts @@ -21,6 +21,15 @@ const AccountReleaseNpubPayload = GT.Object({ reassignedTo: { type: GraphQLAccount, }, + // The release and the reassignment are two writes, not a transaction, so + // the second can fail with the first already applied. That cannot be + // reported by discarding the payload: `accountDetails` and `previousNpub` + // are what tell the operator the key is now unclaimed, that re-running the + // mutation will refuse with `NoNpubToReleaseError`, and which key to hunt + // down with `accountDetailsByNpub`. + reassignmentError: { + type: IError, + }, }), }) diff --git a/src/migrations/20260824120000-accounts-unique-npub.ts b/src/migrations/20260824120000-accounts-unique-npub.ts index 2666e45d2..fc65f3149 100644 --- a/src/migrations/20260824120000-accounts-unique-npub.ts +++ b/src/migrations/20260824120000-accounts-unique-npub.ts @@ -130,10 +130,11 @@ module.exports = { for (const group of duplicates) { // Every account in the group loses the npub, including the oldest. // There is no way to tell from mongo which account still holds the - // nostr secret key, and picking wrong is unrecoverable in-product: - // `setNpub` refuses an already-claimed npub and no admin mutation can - // release one. Releasing the key lets the real owner re-link from the - // app; the unique index makes the re-link race safe. + // nostr secret key, and picking wrong locks the real owner out: + // `setNpub` refuses an already-claimed npub, so they would need a + // support-desk release before they could re-link at all. Releasing + // every claim here lets the real owner re-link straight from the app; + // the unique index makes the re-link race safe. const ids = group.docs.map((d) => d._id) console.log( diff --git a/src/services/mongoose/accounts.ts b/src/services/mongoose/accounts.ts index b4e38c98c..75bf7a83e 100644 --- a/src/services/mongoose/accounts.ts +++ b/src/services/mongoose/accounts.ts @@ -108,8 +108,17 @@ export const AccountsRepository = (): IAccountsRepository => { // identical in both cases and the operator would be told a release happened // when nothing was freed — then send the rightful owner off to re-link, where // `setNpub` refuses them because the squatter still holds the key. The - // pre-update document is the only thing that can tell the two apart. - const unsetNpub = async (accountId: AccountId): Promise => { + // pre-update document is the only thing that can tell the two apart, which + // also makes it the only read that cannot disagree with the key this `$unset` + // removed — hence `previousNpub` comes back with the account rather than the + // caller re-reading it. + // + // `typeof` rather than a null check: the field is `Npub | null` and legacy + // documents holding an explicit null predate the partial index, which only + // covers strings. A null is not a claim and there is nothing to release. + const unsetNpub = async ( + accountId: AccountId, + ): Promise => { try { const before = await Account.findOneAndUpdate( { _id: toObjectId(accountId) }, @@ -118,7 +127,10 @@ export const AccountsRepository = (): IAccountsRepository => { ) if (!before) return new CouldNotFindAccountFromIdError(accountId) if (typeof before.npub !== "string") return new NoNpubToReleaseError(accountId) - return { ...translateToAccount(before), npub: undefined } + return { + account: { ...translateToAccount(before), npub: undefined }, + previousNpub: before.npub, + } } catch (err) { return parseRepositoryError(err) } diff --git a/test/flash/unit/app/accounts/release-npub.spec.ts b/test/flash/unit/app/accounts/release-npub.spec.ts index df10ea891..d1c38503c 100644 --- a/test/flash/unit/app/accounts/release-npub.spec.ts +++ b/test/flash/unit/app/accounts/release-npub.spec.ts @@ -9,13 +9,19 @@ const findById = jest.fn() const unsetNpub = jest.fn() const claimNpub = jest.fn() const info = jest.fn() +const warn = jest.fn() +const error = jest.fn() jest.mock("@services/mongoose", () => ({ AccountsRepository: () => ({ findById, unsetNpub, claimNpub }), })) jest.mock("@services/logger", () => ({ - baseLogger: { info: (...args: unknown[]) => info(...args) }, + baseLogger: { + info: (...args: unknown[]) => info(...args), + warn: (...args: unknown[]) => warn(...args), + error: (...args: unknown[]) => error(...args), + }, })) import { InvalidAccountIdError } from "@domain/accounts" @@ -39,12 +45,15 @@ const release = (overrides: Record = {}) => describe("Accounts.releaseNpub", () => { beforeEach(() => { - findById - .mockReset() - .mockResolvedValue({ id: ACCOUNT_ID, username: "jaceth2009", npub: NPUB }) - unsetNpub.mockReset().mockResolvedValue({ id: ACCOUNT_ID, username: "jaceth2009" }) + findById.mockReset() + unsetNpub.mockReset().mockResolvedValue({ + account: { id: ACCOUNT_ID, username: "jaceth2009" }, + previousNpub: NPUB, + }) claimNpub.mockReset() info.mockReset() + warn.mockReset() + error.mockReset() }) it("clears the npub on the holding account and reports which key was freed", async () => { @@ -55,6 +64,17 @@ describe("Accounts.releaseNpub", () => { expect(result).toMatchObject({ previousNpub: NPUB }) }) + it("takes the freed key from the write itself rather than re-reading the holder", async () => { + // A second read is a later answer: if the holder re-links between the two, + // the log and the reassignment would carry a key the `$unset` never + // removed. `unsetNpub` reads the pre-update document and is the only + // reader that cannot disagree with itself. + const result = await release() + + expect(findById).not.toHaveBeenCalled() + expect(result).toMatchObject({ previousNpub: NPUB }) + }) + it("logs the actor, the account and the key that was removed", async () => { // The only attribution that exists: the account document keeps no trace of // a removed npub, and the admin server's Pino line logs `gqlContext.user` @@ -72,24 +92,24 @@ describe("Accounts.releaseNpub", () => { }) it("reports an unknown account instead of reporting a release", async () => { - findById.mockResolvedValue(new CouldNotFindAccountError()) + unsetNpub.mockResolvedValue(new CouldNotFindAccountFromIdError(ACCOUNT_ID)) const result = await release() expect(result).toBeInstanceOf(CouldNotFindAccountFromIdError) - expect(unsetNpub).not.toHaveBeenCalled() + expect(claimNpub).not.toHaveBeenCalled() }) it("refuses an account that holds no npub", async () => { // `$unset` on a document without the field is a no-op that still matches, // so without this the operator is told a release happened and sends the // owner off to re-link — where the squatter still holds the key. - findById.mockResolvedValue({ id: ACCOUNT_ID, username: "jaceth2009" }) + unsetNpub.mockResolvedValue(new NoNpubToReleaseError(ACCOUNT_ID)) const result = await release() expect(result).toBeInstanceOf(NoNpubToReleaseError) - expect(unsetNpub).not.toHaveBeenCalled() + expect(claimNpub).not.toHaveBeenCalled() }) it("rejects a malformed account id without writing", async () => { @@ -111,13 +131,87 @@ describe("Accounts.releaseNpub", () => { expect(await release()).toBeInstanceOf(UnknownRepositoryError) }) + describe("refusal logging", () => { + /** + * A stolen admin token enumerating account ids produces one line for the id + * that happened to hold a key and nothing at all for the rest, which is the + * shape an investigator needs to see the sweep. The admin server's Pino + * request line cannot supply it: it carries neither the actor nor the body. + */ + const refusals: [string, () => Promise, string][] = [ + [ + "a malformed account id", + () => releaseNpub({ id: "not-an-account-id", releasedByUserId: SUPPORT_USER_ID }), + // The id as given, not a checked one — there is no checked one here, + // and the string the caller sent is what an investigator matches on. + "not-an-account-id", + ], + [ + "a malformed reassignment target id", + () => release({ reassignToAccountId: "not-an-account-id" }), + ACCOUNT_ID, + ], + [ + "an unknown holder", + async () => { + unsetNpub.mockResolvedValue(new CouldNotFindAccountFromIdError(ACCOUNT_ID)) + return release() + }, + ACCOUNT_ID, + ], + [ + "a holder with no npub", + async () => { + unsetNpub.mockResolvedValue(new NoNpubToReleaseError(ACCOUNT_ID)) + return release() + }, + ACCOUNT_ID, + ], + [ + "an unknown reassignment target", + async () => { + findById.mockResolvedValue(new CouldNotFindAccountError()) + return release({ reassignToAccountId: TARGET_ACCOUNT_ID }) + }, + ACCOUNT_ID, + ], + [ + "a reassignment target that already holds a key", + async () => { + findById.mockResolvedValue({ id: TARGET_ACCOUNT_ID, npub: NPUB }) + return release({ reassignToAccountId: TARGET_ACCOUNT_ID }) + }, + ACCOUNT_ID, + ], + ] + + it.each(refusals)( + "logs a refused release for %s", + async (_label, run, expectedAccountId) => { + await run() + + expect(warn).toHaveBeenCalledWith( + expect.objectContaining({ + accountId: expectedAccountId, + releasedByUserId: SUPPORT_USER_ID, + reason: expect.any(String), + }), + expect.any(String), + ) + expect(info).not.toHaveBeenCalled() + }, + ) + + it("does not log a refusal when the release succeeds", async () => { + await release() + + expect(warn).not.toHaveBeenCalled() + }) + }) + describe("reassignment", () => { beforeEach(() => { - findById.mockImplementation(async (id: AccountId) => - id === ACCOUNT_ID - ? { id: ACCOUNT_ID, username: "jaceth2009", npub: NPUB } - : { id: TARGET_ACCOUNT_ID, username: "rightful-owner" }, - ) + findById.mockResolvedValue({ id: TARGET_ACCOUNT_ID, username: "rightful-owner" }) claimNpub.mockResolvedValue({ id: TARGET_ACCOUNT_ID, username: "rightful-owner", @@ -136,12 +230,25 @@ describe("Accounts.releaseNpub", () => { }) }) + it("accepts a target carrying a legacy null npub", async () => { + // The partial index only covers string npubs and the migration leaves + // `npub: null` documents alone, so an account that never linked a key can + // still hold an explicit null. Reading that as a claim would make the + // account permanently unable to receive a reassignment. + findById.mockResolvedValue({ id: TARGET_ACCOUNT_ID, npub: null }) + + const result = await release({ reassignToAccountId: TARGET_ACCOUNT_ID }) + + expect(result).not.toBeInstanceOf(Error) + expect(claimNpub).toHaveBeenCalledWith(TARGET_ACCOUNT_ID, NPUB) + }) + it("refuses a target that already holds an npub, before freeing anything", async () => { - findById.mockImplementation(async (id: AccountId) => - id === ACCOUNT_ID - ? { id: ACCOUNT_ID, username: "jaceth2009", npub: NPUB } - : { id: TARGET_ACCOUNT_ID, username: "rightful-owner", npub: "npub1other" }, - ) + findById.mockResolvedValue({ + id: TARGET_ACCOUNT_ID, + username: "rightful-owner", + npub: "npub1other", + }) const result = await release({ reassignToAccountId: TARGET_ACCOUNT_ID }) @@ -150,11 +257,7 @@ describe("Accounts.releaseNpub", () => { }) it("refuses an unknown target, before freeing anything", async () => { - findById.mockImplementation(async (id: AccountId) => - id === ACCOUNT_ID - ? { id: ACCOUNT_ID, username: "jaceth2009", npub: NPUB } - : new CouldNotFindAccountError(), - ) + findById.mockResolvedValue(new CouldNotFindAccountError()) const result = await release({ reassignToAccountId: TARGET_ACCOUNT_ID }) @@ -169,16 +272,86 @@ describe("Accounts.releaseNpub", () => { expect(unsetNpub).not.toHaveBeenCalled() }) - it("reports a claim lost in the window between the two writes", async () => { - // The release and the claim are not one transaction — this repository has - // no mongo sessions — so a squatter can take the key back in between. The - // unique index catches it; the operator must be told the reassignment did - // not land. - claimNpub.mockResolvedValue(new DuplicateKeyForPersistError()) + it("records the target as intent before the claim, and as outcome after", async () => { + // The claim can still lose to a concurrent one, so the release line must + // not assert where the key went — an investigator reading it would put + // the key on an account that never received it. + await release({ reassignToAccountId: TARGET_ACCOUNT_ID }) - const result = await release({ reassignToAccountId: TARGET_ACCOUNT_ID }) + expect(info).toHaveBeenNthCalledWith( + 1, + expect.objectContaining({ reassignToAccountId: TARGET_ACCOUNT_ID }), + expect.any(String), + ) + expect(info.mock.calls[0][0]).not.toHaveProperty("reassignedToAccountId") + expect(info).toHaveBeenNthCalledWith( + 2, + expect.objectContaining({ + reassignedToAccountId: TARGET_ACCOUNT_ID, + previousNpub: NPUB, + releasedByUserId: SUPPORT_USER_ID, + }), + expect.any(String), + ) + }) + + describe("when the claim fails after the release landed", () => { + beforeEach(() => { + claimNpub.mockResolvedValue(new DuplicateKeyForPersistError()) + }) - expect(result).toBeInstanceOf(NpubNotAvailableError) + it("reports a claim lost in the window between the two writes", async () => { + // The release and the claim are not one transaction — this repository + // has no mongo sessions — so a squatter can take the key back in + // between. The unique index catches it; the operator must be told the + // reassignment did not land. + const result = await release({ reassignToAccountId: TARGET_ACCOUNT_ID }) + + expect(result).toMatchObject({ + reassignmentError: expect.any(NpubNotAvailableError), + }) + }) + + it("still reports the release that did land", async () => { + // Returning a bare error would discard both, leaving the operator + // unaware that the key is now unclaimed and that re-running the + // mutation refuses with `NoNpubToReleaseError`. `previousNpub` is what + // they feed to `accountDetailsByNpub` to find the new holder. + const result = await release({ reassignToAccountId: TARGET_ACCOUNT_ID }) + + expect(result).not.toBeInstanceOf(Error) + expect(result).toMatchObject({ + account: { id: ACCOUNT_ID }, + previousNpub: NPUB, + }) + expect(result).not.toMatchObject({ reassignedTo: expect.anything() }) + }) + + it("logs the partial application rather than a reassignment", async () => { + await release({ reassignToAccountId: TARGET_ACCOUNT_ID }) + + expect(error).toHaveBeenCalledWith( + expect.objectContaining({ + accountId: ACCOUNT_ID, + previousNpub: NPUB, + reassignToAccountId: TARGET_ACCOUNT_ID, + releasedByUserId: SUPPORT_USER_ID, + }), + expect.any(String), + ) + expect(info).toHaveBeenCalledTimes(1) + }) + + it("passes a non-collision claim failure through the same way", async () => { + claimNpub.mockResolvedValue(new UnknownRepositoryError("mongo down")) + + const result = await release({ reassignToAccountId: TARGET_ACCOUNT_ID }) + + expect(result).toMatchObject({ + previousNpub: NPUB, + reassignmentError: expect.any(UnknownRepositoryError), + }) + }) }) }) }) diff --git a/test/flash/unit/graphql/admin/account-release-npub.spec.ts b/test/flash/unit/graphql/admin/account-release-npub.spec.ts index f066e0ebf..95a66818f 100644 --- a/test/flash/unit/graphql/admin/account-release-npub.spec.ts +++ b/test/flash/unit/graphql/admin/account-release-npub.spec.ts @@ -29,7 +29,7 @@ jest.mock("@app", () => ({ import { InvalidAccountIdError } from "@domain/accounts" import { CouldNotFindAccountFromIdError, NoNpubToReleaseError } from "@domain/errors" -import { AccountAlreadyHasNpubError } from "@domain/nostr" +import { AccountAlreadyHasNpubError, NpubNotAvailableError } from "@domain/nostr" import { mutationFields } from "@graphql/admin/mutations" import AccountReleaseNpubMutation from "@graphql/admin/root/mutation/account-release-npub" @@ -42,6 +42,7 @@ type Result = { accountDetails?: { id: string; npub?: string } previousNpub?: string reassignedTo?: { id: string; npub?: string } + reassignmentError?: { message: string; code?: string } } // Mirrors what graphql-admin-server's Apollo `context` fn builds from the @@ -176,6 +177,49 @@ describe("accountReleaseNpub", () => { expect(result.previousNpub).toBeUndefined() }) + describe("a reassignment that failed after the release landed", () => { + // The two writes are not a transaction, so the app layer reports this as a + // populated release carrying `reassignmentError` rather than as an error. + // Collapsing it back to `{ errors }` here would throw away the only signal + // that the key has already left the holder. + beforeEach(() => { + mockReleaseNpub.mockResolvedValue({ + account: { id: ACCOUNT_ID, username: "jaceth2009" }, + previousNpub: NPUB, + reassignmentError: new NpubNotAvailableError(NPUB), + }) + }) + + it("still returns the account the key was taken off, and the key", async () => { + const result = await resolveMutation({ + accountId: ACCOUNT_ID, + reassignToAccountId: TARGET_ACCOUNT_ID, + }) + + expect(result.accountDetails).toMatchObject({ id: ACCOUNT_ID }) + expect(result.previousNpub).toBe(NPUB) + expect(result.reassignedTo).toBeUndefined() + }) + + it("names the failure as the reassignment's, not the release's", async () => { + const result = await resolveMutation({ + accountId: ACCOUNT_ID, + reassignToAccountId: TARGET_ACCOUNT_ID, + }) + + expect(result.reassignmentError?.message).not.toContain("NpubNotAvailable") + expect(result.errors).toHaveLength(1) + expect(result.errors[0]).toEqual(result.reassignmentError) + }) + }) + + it("leaves the reassignment error unset on a clean release", async () => { + const result = await resolveMutation({ accountId: ACCOUNT_ID }) + + expect(result.reassignmentError).toBeUndefined() + expect(result.errors).toEqual([]) + }) + it("surfaces an input coercion failure without calling the app layer", async () => { const result = await resolveMutation({ accountId: new Error("Invalid value for ID") }) diff --git a/test/flash/unit/services/mongoose/accounts-unset-npub.spec.ts b/test/flash/unit/services/mongoose/accounts-unset-npub.spec.ts index 0981d6f1a..72da69358 100644 --- a/test/flash/unit/services/mongoose/accounts-unset-npub.spec.ts +++ b/test/flash/unit/services/mongoose/accounts-unset-npub.spec.ts @@ -49,7 +49,20 @@ describe("AccountsRepository.unsetNpub", () => { expect(result).not.toBeInstanceOf(Error) // The document read back is the pre-update one, which still carries the // npub; the account handed to the caller must not. - expect((result as Account).npub).toBeUndefined() + expect((result as NpubUnset).account.npub).toBeUndefined() + // Nothing else can report it. The updated document no longer holds it, and + // a second read of the account is a later, potentially different, answer. + expect((result as NpubUnset).previousNpub).toBe(NPUB) + }) + + it("treats a legacy null npub as nothing to release", async () => { + // The partial index only covers strings, so documents predating it can + // still hold an explicit `npub: null`. `$unset` on one frees nothing. + findOneAndUpdate.mockResolvedValue({ ...accountRecord, npub: null }) + + expect(await AccountsRepository().unsetNpub(ACCOUNT_ID)).toBeInstanceOf( + NoNpubToReleaseError, + ) }) it("reports an unknown account", async () => { From ddedf74313ab038dbc3c165cbda5f9f17adcee68 Mon Sep 17 00:00:00 2001 From: Dread Date: Tue, 25 Aug 2026 11:46:22 -0700 Subject: [PATCH 7/9] fix(admin): guard the npub reassignment write, and de-duplicate the lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings on the npub release path. `claimNpub` was an unguarded `$set` on the target account. `releaseNpub` checks the target holds no npub, but that check is a read from before the release round-trip: if the target linked a different key via `userUpdateNpub` in that window, the reassignment silently overwrote the just-claimed key, which became unclaimed with no log line saying so. The unique partial index cannot catch it — it prevents duplicates, not overwrites. The filter now re-checks at write time with `npub: { $not: { $type: "string" } }` (not `$exists: false`, because legacy documents hold an explicit `npub: null`, which is not a claim and must not block a reassignment). A no-match is ambiguous between "no such account" and "claimed a key since the caller checked", so one follow-up read disambiguates; the concurrent-claim case surfaces as `AccountAlreadyHasNpubError` in `reassignmentError`, alongside the existing collision path. `Admin.getAccountByNpub` was a line-for-line copy of `Accounts.findByNpub` — same validation, same normalisation rationale, same repository call. Two copies invite drift: the next fix would land in one and not the other. The admin module is now a re-export of the accounts one. Its spec still imports via the admin path, so coverage of the re-export's registration survives. Tests: `claimNpub` asserts the guard is in the filter and that a concurrently-claimed target is refused rather than overwritten; `releaseNpub` asserts the refusal reaches the operator as `reassignmentError` on an otherwise-landed release, with no `reassignedTo`. Both fail against the pre-fix code. --- src/app/accounts/find-by-npub.ts | 3 +- src/app/accounts/release-npub.ts | 7 +++-- src/app/admin/get-account-by-npub.ts | 23 ++++---------- src/domain/accounts/index.types.d.ts | 9 +++++- src/domain/nostr/index.types.d.ts | 2 ++ src/services/mongoose/accounts.ts | 31 ++++++++++++++++--- .../unit/app/accounts/find-by-npub.spec.ts | 5 +-- .../unit/app/accounts/release-npub.spec.ts | 19 ++++++++++++ .../app/admin/get-account-by-npub.spec.ts | 11 ++++--- .../mongoose/accounts-unset-npub.spec.ts | 31 +++++++++++++++++-- 10 files changed, 106 insertions(+), 35 deletions(-) diff --git a/src/app/accounts/find-by-npub.ts b/src/app/accounts/find-by-npub.ts index 564324053..92364c1b5 100644 --- a/src/app/accounts/find-by-npub.ts +++ b/src/app/accounts/find-by-npub.ts @@ -2,7 +2,8 @@ import { checkedToNpub } from "@domain/nostr" import { AccountsRepository } from "@services/mongoose" /** - * Twin of `Admin.getAccountByNpub`, normalised for the same reason: the + * Also re-exported as `Admin.getAccountByNpub` — one implementation, two + * barrels, so fixes cannot drift between them. Normalised because the * repository query is a plain `$eq` with no collation, so a caller that is not * the GraphQL boundary (a script, a backfill, a REST shim) passing a mixed-case * npub gets a silent not-found on a real user — which the public diff --git a/src/app/accounts/release-npub.ts b/src/app/accounts/release-npub.ts index 52a0cde1f..5357b9013 100644 --- a/src/app/accounts/release-npub.ts +++ b/src/app/accounts/release-npub.ts @@ -40,8 +40,11 @@ export type NpubRelease = { * that recovering it means finding its current holder with * `accountDetailsByNpub` and releasing it from there. The target is read and * checked before the release so that everything knowable up front fails before - * the key is freed; the unique partial index is what guarantees the - * reassignment cannot collide. + * the key is freed; the unique partial index guarantees the reassignment + * cannot collide with a concurrent claim of the same key, and `claimNpub`'s + * own write-time filter refuses a target that linked a different key after the + * pre-release check — surfaced as `AccountAlreadyHasNpubError` in + * `reassignmentError`, never a silent overwrite of the target's key. * * `releasedByUserId` is the whole attribution trail. Neither the account * document nor the payload retains the npub that was removed, and the admin diff --git a/src/app/admin/get-account-by-npub.ts b/src/app/admin/get-account-by-npub.ts index 286935793..8961d7fac 100644 --- a/src/app/admin/get-account-by-npub.ts +++ b/src/app/admin/get-account-by-npub.ts @@ -1,19 +1,8 @@ -import { checkedToNpub } from "@domain/nostr" -import { AccountsRepository } from "@services/mongoose" - /** - * Lives in its own module rather than inline in `index.ts` so it can be unit - * tested against a mocked repository — importing the admin barrel drags in the - * notification and invite stacks, which open connections at import time. + * `Admin.getAccountByNpub` is `Accounts.findByNpub` under the admin barrel's + * name — same validation, same normalisation rationale, same repository call. + * A re-export rather than a copy so a fix (e.g. a bech32 validation upgrade) + * cannot land in one and not the other. The admin unit spec stays pointed at + * this module so registration coverage of the re-export survives. */ -export const getAccountByNpub = async (npub: Npub) => { - // Mirrors getAccountByUsername: the branded type is the contract, and - // checkedToNpub is defence in depth for callers that are not the GraphQL - // boundary (scripts, backfills, a future REST shim) — they get a validation - // error rather than a silent not-found on a malformed value. - const npubValid = checkedToNpub(npub) - if (npubValid instanceof Error) return npubValid - - const accounts = AccountsRepository() - return accounts.findByNpub(npubValid) -} +export { findByNpub as getAccountByNpub } from "@app/accounts/find-by-npub" diff --git a/src/domain/accounts/index.types.d.ts b/src/domain/accounts/index.types.d.ts index c50b5858b..6de63af7f 100644 --- a/src/domain/accounts/index.types.d.ts +++ b/src/domain/accounts/index.types.d.ts @@ -208,7 +208,14 @@ interface IAccountsRepository { // listBusinessesForMap(): Promise findByNpub(npub: Npub): Promise unsetNpub(accountId: AccountId): Promise - claimNpub(accountId: AccountId, npub: Npub): Promise + // `AccountAlreadyHasNpubError` in the union is the write-time guard: the + // caller's "target holds no npub" check is a read from before the release, + // and a key the target claims in that window must refuse the reassignment + // rather than be silently overwritten. + claimNpub( + accountId: AccountId, + npub: Npub, + ): Promise update(account: Account): Promise transitionBridgeKycStatus( diff --git a/src/domain/nostr/index.types.d.ts b/src/domain/nostr/index.types.d.ts index cbc34a598..4ca131adc 100644 --- a/src/domain/nostr/index.types.d.ts +++ b/src/domain/nostr/index.types.d.ts @@ -1 +1,3 @@ type Npub = `npub1${string}` + +type AccountAlreadyHasNpubError = import("./errors").AccountAlreadyHasNpubError diff --git a/src/services/mongoose/accounts.ts b/src/services/mongoose/accounts.ts index 75bf7a83e..45ddcecd3 100644 --- a/src/services/mongoose/accounts.ts +++ b/src/services/mongoose/accounts.ts @@ -12,6 +12,7 @@ import { RepositoryError, } from "@domain/errors" import { UsdDisplayCurrency } from "@domain/fiat" +import { AccountAlreadyHasNpubError } from "@domain/nostr" import { Account } from "@services/mongoose/schema" @@ -138,19 +139,39 @@ export const AccountsRepository = (): IAccountsRepository => { // The reassignment half of a release. A targeted `$set` rather than a // read-modify-write through `update`, so the unique partial index is the only - // thing that decides whether the claim lands: a concurrent claimant trips it - // and `parseRepositoryError` surfaces `DuplicateKeyForPersistError`. + // thing that decides whether the claim lands on the key side: a concurrent + // claimant of the same npub trips it and `parseRepositoryError` surfaces + // `DuplicateKeyForPersistError`. + // + // The filter guards the target side. The caller checks the target holds no + // npub before releasing, but that check is a read from before the release + // round-trip: if the target links a different key via `userUpdateNpub` in + // that window, an unguarded `$set` would silently overwrite the just-claimed + // key — which becomes unclaimed with no log line saying so. The unique index + // cannot catch this: it prevents duplicates, not overwrites. `$not: + // { $type: "string" }` rather than `$exists: false` because legacy documents + // predating the partial index hold an explicit `npub: null`, which is not a + // claim and must not block a reassignment. + // + // A no-match is ambiguous between "no such account" and "account claimed a + // key since the caller checked", so one follow-up read disambiguates. The + // claim did not land in either case, so a stale answer from that read still + // reports a refusal — the conservative outcome. const claimNpub = async ( accountId: AccountId, npub: Npub, - ): Promise => { + ): Promise => { try { const result = await Account.findOneAndUpdate( - { _id: toObjectId(accountId) }, + { _id: toObjectId(accountId), npub: { $not: { $type: "string" } } }, { $set: { npub } }, { new: true }, ) - if (!result) return new CouldNotFindAccountFromIdError(accountId) + if (!result) { + const existing = await Account.findOne({ _id: toObjectId(accountId) }) + if (!existing) return new CouldNotFindAccountFromIdError(accountId) + return new AccountAlreadyHasNpubError(accountId) + } return translateToAccount(result) } catch (err) { return parseRepositoryError(err) diff --git a/test/flash/unit/app/accounts/find-by-npub.spec.ts b/test/flash/unit/app/accounts/find-by-npub.spec.ts index 557676dbf..80eb4569f 100644 --- a/test/flash/unit/app/accounts/find-by-npub.spec.ts +++ b/test/flash/unit/app/accounts/find-by-npub.spec.ts @@ -1,6 +1,7 @@ /** - * `Accounts.findByNpub` is the twin of `Admin.getAccountByNpub` but never got - * the same normalisation. Once the case-insensitive collation came off the + * `Accounts.findByNpub` is the one implementation behind both this name and + * `Admin.getAccountByNpub`, which re-exports it. It never had the normalisation + * the admin path did; once the case-insensitive collation came off the * repository query, normalising here stopped being optional. */ const findByNpub = jest.fn() diff --git a/test/flash/unit/app/accounts/release-npub.spec.ts b/test/flash/unit/app/accounts/release-npub.spec.ts index d1c38503c..92ebe0f0c 100644 --- a/test/flash/unit/app/accounts/release-npub.spec.ts +++ b/test/flash/unit/app/accounts/release-npub.spec.ts @@ -352,6 +352,25 @@ describe("Accounts.releaseNpub", () => { reassignmentError: expect.any(UnknownRepositoryError), }) }) + + it("reports a target that claimed a different key after the pre-release check", async () => { + // The "target holds no npub" check above is a read from before the + // release round-trip. If the target links a different key via + // `userUpdateNpub` in that window, `claimNpub`'s write-time filter + // refuses rather than silently overwriting the just-claimed key — and + // the refusal must reach the operator as a reassignment failure on an + // otherwise-landed release, not vanish. + claimNpub.mockResolvedValue(new AccountAlreadyHasNpubError(TARGET_ACCOUNT_ID)) + + const result = await release({ reassignToAccountId: TARGET_ACCOUNT_ID }) + + expect(result).not.toBeInstanceOf(Error) + expect(result).toMatchObject({ + previousNpub: NPUB, + reassignmentError: expect.any(AccountAlreadyHasNpubError), + }) + expect(result).not.toMatchObject({ reassignedTo: expect.anything() }) + }) }) }) }) diff --git a/test/flash/unit/app/admin/get-account-by-npub.spec.ts b/test/flash/unit/app/admin/get-account-by-npub.spec.ts index 0c16e93a7..eda9fefe4 100644 --- a/test/flash/unit/app/admin/get-account-by-npub.spec.ts +++ b/test/flash/unit/app/admin/get-account-by-npub.spec.ts @@ -1,8 +1,11 @@ /** - * `Admin.getAccountByNpub` is the only genuinely new app-layer code behind the - * admin npub lookup, and it was previously reachable only through a mocked - * `@app` barrel — i.e. not covered at all. Here the repository is the mock and - * the app function is real. + * `Admin.getAccountByNpub` is the admin barrel's name for `Accounts.findByNpub` + * — a re-export, not a second implementation, so a fix cannot land in one and + * not the other. The import path here is deliberately the admin module rather + * than the accounts one: it is what proves the re-export is actually wired, so + * the behaviour the admin GraphQL resolver depends on stays covered even though + * the code lives elsewhere. The repository is the mock and the app function is + * real. */ const findByNpub = jest.fn() diff --git a/test/flash/unit/services/mongoose/accounts-unset-npub.spec.ts b/test/flash/unit/services/mongoose/accounts-unset-npub.spec.ts index 72da69358..6a92b7834 100644 --- a/test/flash/unit/services/mongoose/accounts-unset-npub.spec.ts +++ b/test/flash/unit/services/mongoose/accounts-unset-npub.spec.ts @@ -1,10 +1,15 @@ import { CouldNotFindAccountFromIdError, NoNpubToReleaseError } from "@domain/errors" +import { AccountAlreadyHasNpubError } from "@domain/nostr" import { AccountsRepository } from "@services/mongoose/accounts" const findOneAndUpdate = jest.fn() +const findOne = jest.fn() jest.mock("@services/mongoose/schema", () => ({ - Account: { findOneAndUpdate: (...args: unknown[]) => findOneAndUpdate(...args) }, + Account: { + findOneAndUpdate: (...args: unknown[]) => findOneAndUpdate(...args), + findOne: (...args: unknown[]) => findOne(...args), + }, })) jest.mock("@services/mongoose/utils", () => ({ @@ -89,15 +94,22 @@ describe("AccountsRepository.unsetNpub", () => { describe("AccountsRepository.claimNpub", () => { beforeEach(() => { findOneAndUpdate.mockReset() + findOne.mockReset() }) - it("sets the key on the receiving account", async () => { + it("sets the key only on an account that holds none", async () => { findOneAndUpdate.mockResolvedValue({ ...accountRecord, npub: NPUB }) const result = await AccountsRepository().claimNpub(ACCOUNT_ID, NPUB) + // The npub guard in the filter is the write-time re-check: the caller's + // "target holds no npub" read happens before the release round-trip, so a + // key the target claims in that window must fail the match rather than be + // silently overwritten. `$not: { $type: "string" }` and not + // `$exists: false`, because legacy documents hold `npub: null`, which is + // not a claim. expect(findOneAndUpdate).toHaveBeenCalledWith( - { _id: ACCOUNT_ID }, + { _id: ACCOUNT_ID, npub: { $not: { $type: "string" } } }, { $set: { npub: NPUB } }, { new: true }, ) @@ -106,9 +118,22 @@ describe("AccountsRepository.claimNpub", () => { it("reports an unknown account", async () => { findOneAndUpdate.mockResolvedValue(null) + findOne.mockResolvedValue(null) expect(await AccountsRepository().claimNpub(ACCOUNT_ID, NPUB)).toBeInstanceOf( CouldNotFindAccountFromIdError, ) }) + + it("refuses to overwrite a key the account claimed concurrently", async () => { + // The unique index cannot catch this case — it prevents duplicates, not + // overwrites. Without the filter guard, the $set would land, the target's + // just-claimed key would become unclaimed, and nothing would log it. + findOneAndUpdate.mockResolvedValue(null) + findOne.mockResolvedValue({ ...accountRecord, npub: `npub1${"z".repeat(58)}` }) + + expect(await AccountsRepository().claimNpub(ACCOUNT_ID, NPUB)).toBeInstanceOf( + AccountAlreadyHasNpubError, + ) + }) }) From 329897bcace93034f3b7337b1c9edf51b8222fa9 Mon Sep 17 00:00:00 2001 From: Dread Date: Tue, 25 Aug 2026 12:05:09 -0700 Subject: [PATCH 8/9] fix(admin): name which reassignment failed, and reach the npub index MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review fixes. Say which failure it was. Three causes reach the single "npub released but reassignment failed" line and each has a different recovery — the key is gone (hunt the holder with accountDetailsByNpub), the key is unclaimed (retry against another target), or the write failed (retry the same call). The line carried none of that, and the admin server never assigns req.gqlContext, so there is no request log to fall back on. It now logs `reason` off the raw repository error, before the DuplicateKeyForPersistError -> NpubNotAvailable mapping, plus the message. Covered by a table-driven spec that also asserts the three reasons are distinct. Exercise the repository layer against a real database. The guards this PR added are index- and filter-shaped, and asserting a filter literal back at a mocked mongoose model cannot report on whether mongo matches a missing field and an explicit `npub: null` while rejecting a string. New integration spec covers claimNpub onto no-field / legacy-null / already-held targets, the unique partial index tripping on a second claimant, findByNpub, unsetNpub, and releaseNpub's reassignment end to end. It lives under test/flash/integration/accounts/ rather than .../integration/services/ because the integration jest config ignores the services and wallet directories, and a spec there would never run. That spec immediately returned "no": findByNpub was planning as a COLLSCAN. Dropping the case-insensitive collation was only half the problem — the index is partial on `{ npub: { $type: "string" } }`, and mongo will not select a partial index unless the query provably matches a subset of its filter, which it cannot derive from an equality against a string literal. Restating the type predicate in the query makes it an IXSCAN without changing the result set. The plan assertion is taken from the filter the repository actually sends, captured via mongoose's debug hook, so it cannot pass by restating the filter. Document the rollout ordering on the migration. It builds the first unique index on a field that holds duplicates in prod, and the schema now declares that index too, so a pod that boots ahead of it hits E11000 in syncIndexes, rethrows at src/services/mongodb/index.ts:107, logs one "server error" line and never starts either Apollo server — up, ready, no listener. The chart already gates this with the wait-for-mongodb-migrate initContainer on every galoy workload; the docstring now names that guarantee and the two ways to lose it (bumping the app image digest without the migrate image digest, and force-rolls out of band). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01NEoz7nBtdtsHyuYG5wNPQV --- src/app/accounts/release-npub.ts | 17 + .../20260824120000-accounts-unique-npub.ts | 32 ++ src/services/mongoose/accounts.ts | 13 +- test/flash/integration/accounts/npub.spec.ts | 351 ++++++++++++++++++ .../unit/app/accounts/release-npub.spec.ts | 58 +++ .../mongoose/accounts-find-by-npub.spec.ts | 10 +- 6 files changed, 479 insertions(+), 2 deletions(-) create mode 100644 test/flash/integration/accounts/npub.spec.ts diff --git a/src/app/accounts/release-npub.ts b/src/app/accounts/release-npub.ts index 5357b9013..1e5895738 100644 --- a/src/app/accounts/release-npub.ts +++ b/src/app/accounts/release-npub.ts @@ -143,6 +143,23 @@ export const releaseNpub = async ({ previousNpub, reassignToAccountId: targetIdChecked, releasedByUserId, + // Which failure, not just that one happened. Three distinct causes + // reach this branch and each has a different recovery, so a line that + // does not name the cause hands the operator all three procedures at + // once. `DuplicateKeyForPersistError` (surfaced to the caller as + // `NpubNotAvailableError`): someone else claimed the key in the window + // between the two writes — it is gone, find the new holder with + // `accountDetailsByNpub` and release it from there. + // `AccountAlreadyHasNpubError`: the target linked a different key + // after the pre-release check — the released key is still unclaimed, + // so re-run against another target. Anything else (e.g. + // `UnknownRepositoryError`): the write failed, retry the same call. + // + // Taken off the raw repository error, before the + // `DuplicateKeyForPersistError` → `NpubNotAvailableError` mapping + // below, so the line names the failure that actually happened. + reason: reassigned.name, + reassignmentError: reassigned.message, }, "npub released but reassignment failed", ) diff --git a/src/migrations/20260824120000-accounts-unique-npub.ts b/src/migrations/20260824120000-accounts-unique-npub.ts index fc65f3149..48ea465bd 100644 --- a/src/migrations/20260824120000-accounts-unique-npub.ts +++ b/src/migrations/20260824120000-accounts-unique-npub.ts @@ -35,6 +35,38 @@ * `sparse: true`: a sparse index still indexes documents holding an explicit * `npub: null`, and the second such document would collide. * + * Rollout + * ------- + * THIS MIGRATION MUST COMPLETE BEFORE ANY POD RUNNING THE NEW IMAGE BOOTS. + * It is the first unique index on a field that holds duplicates in prod, and + * `schema.ts` now declares that index on `AccountSchema` too. + * `graphql-main-server.ts` boots with `setupMongoConnection(true)`, which runs + * `syncIndexes()` across every model. Against un-deduped data `createIndex` + * rejects with E11000, `setupMongoConnection` rethrows + * (`src/services/mongodb/index.ts:107`), and the `.catch` in + * `graphql-main-server.ts` logs a single "server error" line and returns: + * `bootstrap()` and both Apollo servers never start, the process stays alive + * on mongoose's open handles, and the pod reports Ready with no listener. One + * log line is the only clue. + * + * The chart already enforces the ordering: every galoy workload — api, + * websocket, trigger, exporter, the ibex/bridge/fygaro webhooks and the + * cronjobs — carries a `wait-for-mongodb-migrate` initContainer + * (`groundnuty/k8s-wait-for` `job-wr`) that blocks on the per-revision + * `-mongodb-migrate-` Job and fails if that Job fails, so + * the app container cannot start ahead of the migration + * (`charts/flash/templates/api-deployment.yaml`, + * `charts/flash/templates/galoy-migration-job.yaml`). + * + * Two ways to lose that guarantee, both of which produce the dead-pod failure + * above: bumping `galoy.images.app.digest` without bumping + * `galoy.images.mongodbMigrate.digest` in the same release (the migrate Job + * then runs an older image that does not contain this file, succeeds, and lets + * the new app through against un-migrated data), and force-rolling or + * recreating a Deployment out of band so the initContainer's Job is not the + * one carrying this migration. If a pod is up with no listener, check that the + * revision's migrate Job ran this migration before touching anything else. + * * Rollback (down) * --------------- * Drops the unique index. The lowercasing and the unsets are NOT reverted — diff --git a/src/services/mongoose/accounts.ts b/src/services/mongoose/accounts.ts index 45ddcecd3..086eb564c 100644 --- a/src/services/mongoose/accounts.ts +++ b/src/services/mongoose/accounts.ts @@ -86,9 +86,20 @@ export const AccountsRepository = (): IAccountsRepository => { // buys nothing — and a non-simple collation would stop the query from using // the unique `{ npub: 1 }` index, leaving every support-desk lookup a // collection scan. + // + // The redundant-looking `$type: "string"` is what actually gets the index + // used. The index is partial on `{ npub: { $type: "string" } }`, and mongo + // only picks a partial index when the query provably matches a subset of the + // partial filter. It cannot derive that from an equality against a string + // literal — `$type` is not one of the predicates its implication check + // understands — so `{ npub: { $eq: npub } }` alone plans as a COLLSCAN + // (verified on 6.0 in test/flash/integration/accounts/npub.spec.ts). + // Restating the type predicate makes it an IXSCAN. It cannot change the + // result set: `npub` is always a string here, so any document that could + // match the equality is a string-typed one. const findByNpub = async (npub: Npub): Promise => { try { - const result = await Account.findOne({ npub: { $eq: npub } }) + const result = await Account.findOne({ npub: { $eq: npub, $type: "string" } }) if (!result) { return new CouldNotFindAccountFromNpubError(npub) } diff --git a/test/flash/integration/accounts/npub.spec.ts b/test/flash/integration/accounts/npub.spec.ts new file mode 100644 index 000000000..b9ba6e51e --- /dev/null +++ b/test/flash/integration/accounts/npub.spec.ts @@ -0,0 +1,351 @@ +/** + * The npub repository layer added by this PR is almost entirely index- and + * filter-shaped, and a mocked mongoose model cannot report on any of it: the + * unit specs assert that `claimNpub` passes `{ npub: { $not: { $type: + * "string" } } }` and that the schema declares a unique partial index, but + * nothing there executes either against a database. If the filter were wrong, + * the guard would be silently inert — a reassignment onto a legacy `npub: + * null` account would come back `AccountAlreadyHasNpubError` forever — and the + * unit suite would stay green. + * + * These run against the real collection, so they can return "no" about the + * three claims the comments make: + * + * - `$not: { $type: "string" }` rather than `$exists: false`, because legacy + * documents predating the partial index hold an explicit `npub: null` and + * that is not a claim (`accounts.ts` `claimNpub`, `release-npub.ts`). + * - the unique partial index is what makes a concurrent claim of the same key + * fail rather than duplicate (`schema.ts`, migration + * 20260824120000-accounts-unique-npub). + * - `findByNpub` reaches that index instead of scanning the collection + * (`accounts.ts`). This one already came back "no" once: dropping the + * collation was not sufficient, because mongo will not use an index whose + * partial filter is `$type`-shaped for a bare equality. + */ +import mongoose from "mongoose" + +import { releaseNpub } from "@app/accounts/release-npub" +import { + CouldNotFindAccountFromNpubError, + DuplicateKeyForPersistError, + NoNpubToReleaseError, +} from "@domain/errors" +import { AccountAlreadyHasNpubError, checkedToNpub } from "@domain/nostr" +import { AccountsRepository } from "@services/mongoose" +import { Account as AccountModel } from "@services/mongoose/schema" +import { toObjectId } from "@services/mongoose/utils" + +import { createUser } from "test/galoy/helpers" + +const SUPPORT_USER_ID = "support-user-id" as UserId + +// bech32's charset — "1", "b", "i" and "o" are excluded. Values only have to be +// unique and well-formed; nothing here decodes them. +const CHARSET = "qpzry9x8gf2tvdw0s3jn54khce6mua7l" +const RUN = BigInt(Math.floor(Math.random() * 2 ** 40)) +let minted = 0n + +const uniqueNpub = (): Npub => { + let remaining = RUN * 1_000_000n + minted++ + let body = "" + while (remaining > 0n) { + body = CHARSET[Number(remaining % 32n)] + body + remaining /= 32n + } + // Minted through the same checker the GraphQL boundary uses, so a value that + // would not survive validation cannot make these tests pass. + const npub = checkedToNpub(`npub1${body.padStart(58, CHARSET[0])}`) + if (npub instanceof Error) throw npub + return npub +} + +const newAccountId = async (): Promise => { + const user = await createUser() + return user.account.id +} + +// Legacy shape: written before the partial index existed, so it can hold an +// explicit null. `setNpub` never produces this — only direct mongo does. +const setLegacyNullNpub = async (accountId: AccountId) => { + await AccountModel.collection.updateOne( + { _id: toObjectId(accountId) }, + { $set: { npub: null } }, + ) +} + +const npubOf = async (accountId: AccountId): Promise => { + const doc = await AccountModel.collection.findOne({ + _id: toObjectId(accountId), + }) + return doc?.npub +} + +const claim = async (accountId: AccountId, npub: Npub) => { + const claimed = await AccountsRepository().claimNpub(accountId, npub) + if (claimed instanceof Error) throw claimed + return claimed +} + +// The filter the repository actually handed mongo, rather than one restated in +// the test. Mongoose's debug hook is the only seam that reports it without +// re-opening the connection with `monitorCommands`. +const captureFilter = async ( + run: () => Promise, +): Promise> => { + const sent: Record[] = [] + mongoose.set( + "debug", + (collectionName: string, methodName: string, ...args: unknown[]) => { + if ( + collectionName === AccountModel.collection.collectionName && + methodName === "findOne" + ) { + sent.push(args[0] as Record) + } + }, + ) + try { + await run() + } finally { + mongoose.set("debug", false) + } + if (sent.length !== 1) { + throw new Error(`expected exactly one findOne, saw ${sent.length}`) + } + return sent[0] +} + +describe("accounts npub persistence", () => { + it("has the unique partial index on the live collection", async () => { + // `schema.ts` declaring it is not the same as mongo holding it, and every + // other assertion below leans on the real index existing. + const indexes = (await AccountModel.collection.indexes()) as Record[] + const npubIndex = indexes.find( + (idx) => JSON.stringify(idx.key) === JSON.stringify({ npub: 1 }), + ) + + expect(npubIndex).toBeDefined() + expect(npubIndex?.unique).toBe(true) + expect(npubIndex?.partialFilterExpression).toEqual({ npub: { $type: "string" } }) + }) + + describe("claimNpub", () => { + it("lands on an account with no npub field", async () => { + const accountId = await newAccountId() + const npub = uniqueNpub() + + const result = await AccountsRepository().claimNpub(accountId, npub) + + expect(result).not.toBeInstanceOf(Error) + expect((result as Account).npub).toBe(npub) + expect(await npubOf(accountId)).toBe(npub) + }) + + it("lands on an account holding a legacy explicit null", async () => { + // The case `$not: { $type: "string" }` exists for. `$exists: false` would + // refuse here, and the account could never receive a key again. + const accountId = await newAccountId() + await setLegacyNullNpub(accountId) + const npub = uniqueNpub() + + const result = await AccountsRepository().claimNpub(accountId, npub) + + expect(result).not.toBeInstanceOf(Error) + expect(await npubOf(accountId)).toBe(npub) + }) + + it("refuses an account already holding a key, and leaves that key alone", async () => { + const accountId = await newAccountId() + const held = uniqueNpub() + await claim(accountId, held) + + const result = await AccountsRepository().claimNpub(accountId, uniqueNpub()) + + expect(result).toBeInstanceOf(AccountAlreadyHasNpubError) + // The unique index cannot catch an overwrite — only the filter can. If + // the guard were inert this would be the new key, and the old one would + // be unclaimed with nothing logging it. + expect(await npubOf(accountId)).toBe(held) + }) + + it("reports a duplicate when a second account claims the same key", async () => { + const first = await newAccountId() + const second = await newAccountId() + const npub = uniqueNpub() + + await claim(first, npub) + + const result = await AccountsRepository().claimNpub(second, npub) + + expect(result).toBeInstanceOf(DuplicateKeyForPersistError) + expect(await npubOf(second)).toBeUndefined() + }) + + it("lets two accounts hold a legacy null without colliding", async () => { + // `partialFilterExpression` rather than `sparse`: a sparse index would + // index both nulls and reject the second one. + const first = await newAccountId() + const second = await newAccountId() + + await setLegacyNullNpub(first) + await setLegacyNullNpub(second) + + expect(await npubOf(first)).toBeNull() + expect(await npubOf(second)).toBeNull() + }) + }) + + describe("findByNpub", () => { + it("resolves the claiming account", async () => { + const accountId = await newAccountId() + const npub = uniqueNpub() + await claim(accountId, npub) + + const found = await AccountsRepository().findByNpub(npub) + + expect(found).not.toBeInstanceOf(Error) + expect((found as Account).id).toBe(accountId) + }) + + it("reports an unclaimed key rather than an arbitrary account", async () => { + expect(await AccountsRepository().findByNpub(uniqueNpub())).toBeInstanceOf( + CouldNotFindAccountFromNpubError, + ) + }) + + it("uses the index rather than scanning the collection", async () => { + // Dropping the case-insensitive collation was supposed to make this + // lookup an index hit — it is the support desk's identity resolver and + // runs on every inbound nostr DM, plus once per `setNpub` as the + // duplicate probe. The collation was only half of it: the index is + // partial on `{ npub: { $type: "string" } }`, and mongo will not pick a + // partial index unless the query provably matches a subset of that + // filter, which it cannot derive from a bare equality. `{ npub: { $eq } + // }` alone plans as a COLLSCAN on 6.0. + // + // The plan is taken from the filter the repository actually sent, not + // one restated here — restating it would pass no matter what + // `findByNpub` does. + const probe = uniqueNpub() + const sent = await captureFilter(() => AccountsRepository().findByNpub(probe)) + + const plan = (await AccountModel.collection + .find(sent) + .explain("queryPlanner")) as unknown as { + queryPlanner: { winningPlan: unknown } + } + + expect(JSON.stringify(plan.queryPlanner.winningPlan)).toContain("IXSCAN") + }) + }) + + describe("unsetNpub", () => { + it("removes the field and reports the key it freed", async () => { + const accountId = await newAccountId() + const npub = uniqueNpub() + await claim(accountId, npub) + + const released = await AccountsRepository().unsetNpub(accountId) + + expect(released).not.toBeInstanceOf(Error) + expect((released as NpubUnset).previousNpub).toBe(npub) + // Removed, not blanked: the partial index only covers strings, so a null + // left behind would sit unindexed and keep failing lookups. + expect(await npubOf(accountId)).toBeUndefined() + expect(await AccountsRepository().findByNpub(npub)).toBeInstanceOf( + CouldNotFindAccountFromNpubError, + ) + }) + + it("frees the key for whoever actually holds the secret key", async () => { + const squatter = await newAccountId() + const owner = await newAccountId() + const npub = uniqueNpub() + await claim(squatter, npub) + + const released = await AccountsRepository().unsetNpub(squatter) + if (released instanceof Error) throw released + + expect(await AccountsRepository().claimNpub(owner, npub)).not.toBeInstanceOf(Error) + }) + + it("refuses an account holding a legacy explicit null", async () => { + const accountId = await newAccountId() + await setLegacyNullNpub(accountId) + + expect(await AccountsRepository().unsetNpub(accountId)).toBeInstanceOf( + NoNpubToReleaseError, + ) + }) + + it("refuses an account that held nothing", async () => { + const accountId = await newAccountId() + + expect(await AccountsRepository().unsetNpub(accountId)).toBeInstanceOf( + NoNpubToReleaseError, + ) + }) + }) + + describe("releaseNpub with reassignment", () => { + it("moves the key to a target that never held one", async () => { + const squatter = await newAccountId() + const owner = await newAccountId() + const npub = uniqueNpub() + await claim(squatter, npub) + + const result = await releaseNpub({ + id: squatter, + releasedByUserId: SUPPORT_USER_ID, + reassignToAccountId: owner, + }) + + expect(result).not.toBeInstanceOf(Error) + expect(result).toMatchObject({ previousNpub: npub, reassignedTo: { id: owner } }) + expect(await npubOf(squatter)).toBeUndefined() + expect(await npubOf(owner)).toBe(npub) + }) + + it("moves the key to a target holding a legacy explicit null", async () => { + // The end-to-end shape of the bug the `$not: { $type: "string" }` filter + // and the `typeof` pre-check exist to avoid: an owner whose account + // predates the partial index would otherwise be told + // `AccountAlreadyHasNpubError` forever, with the key already released. + const squatter = await newAccountId() + const owner = await newAccountId() + await setLegacyNullNpub(owner) + const npub = uniqueNpub() + await claim(squatter, npub) + + const result = await releaseNpub({ + id: squatter, + releasedByUserId: SUPPORT_USER_ID, + reassignToAccountId: owner, + }) + + expect(result).not.toBeInstanceOf(Error) + expect(result).not.toMatchObject({ reassignmentError: expect.anything() }) + expect(await npubOf(owner)).toBe(npub) + }) + + it("refuses a target already holding a key, before freeing anything", async () => { + const squatter = await newAccountId() + const owner = await newAccountId() + const npub = uniqueNpub() + const ownerNpub = uniqueNpub() + await claim(squatter, npub) + await claim(owner, ownerNpub) + + const result = await releaseNpub({ + id: squatter, + releasedByUserId: SUPPORT_USER_ID, + reassignToAccountId: owner, + }) + + expect(result).toBeInstanceOf(AccountAlreadyHasNpubError) + // Nothing was freed — re-running with a different target must still work. + expect(await npubOf(squatter)).toBe(npub) + expect(await npubOf(owner)).toBe(ownerNpub) + }) + }) +}) diff --git a/test/flash/unit/app/accounts/release-npub.spec.ts b/test/flash/unit/app/accounts/release-npub.spec.ts index 92ebe0f0c..138d30d71 100644 --- a/test/flash/unit/app/accounts/release-npub.spec.ts +++ b/test/flash/unit/app/accounts/release-npub.spec.ts @@ -371,6 +371,64 @@ describe("Accounts.releaseNpub", () => { }) expect(result).not.toMatchObject({ reassignedTo: expect.anything() }) }) + + describe("naming which failure it was", () => { + /** + * Three different causes reach the one "npub released but reassignment + * failed" line, and each has its own recovery: the key is gone and must + * be hunted with `accountDetailsByNpub`; the key is unclaimed and the + * call should be re-run against another target; or the write failed and + * the same call should be retried. Without a discriminator the operator + * gets all three procedures at 3am behind one identical line — and the + * admin server never assigns `req.gqlContext`, so there is no request + * log to fall back on. + */ + const causes: [string, ApplicationError, string][] = [ + // The raw repository error, not the `NpubNotAvailableError` the + // caller is handed: logging the mapped name would hide that this was + // the unique index firing. + [ + "a key claimed by someone else in the window", + new DuplicateKeyForPersistError(), + "DuplicateKeyForPersistError", + ], + [ + "a target that self-claimed a different key", + new AccountAlreadyHasNpubError(TARGET_ACCOUNT_ID), + "AccountAlreadyHasNpubError", + ], + [ + "a write that failed outright", + new UnknownRepositoryError("mongo down"), + "UnknownRepositoryError", + ], + ] + + it.each(causes)("names %s", async (_label, failure, expectedReason) => { + claimNpub.mockResolvedValue(failure) + + await release({ reassignToAccountId: TARGET_ACCOUNT_ID }) + + expect(error).toHaveBeenCalledWith( + expect.objectContaining({ reason: expectedReason }), + expect.any(String), + ) + }) + + it("gives every cause a distinct reason", async () => { + // A shared or empty `name` would satisfy the assertions above while + // still collapsing the three recoveries into one line. + const reasons: unknown[] = [] + for (const [, failure] of causes) { + error.mockReset() + claimNpub.mockResolvedValue(failure) + await release({ reassignToAccountId: TARGET_ACCOUNT_ID }) + reasons.push(error.mock.calls[0][0].reason) + } + + expect(new Set(reasons).size).toBe(causes.length) + }) + }) }) }) }) diff --git a/test/flash/unit/services/mongoose/accounts-find-by-npub.spec.ts b/test/flash/unit/services/mongoose/accounts-find-by-npub.spec.ts index 29a09672d..ddb7362f2 100644 --- a/test/flash/unit/services/mongoose/accounts-find-by-npub.spec.ts +++ b/test/flash/unit/services/mongoose/accounts-find-by-npub.spec.ts @@ -38,11 +38,19 @@ describe("AccountsRepository.findByNpub", () => { // index, so every support-desk lookup would be a collection scan. The old // implementation chained `.collation({ locale: "en", strength: 2 })` and // would blow up on this mock. + // + // The `$type` alongside the `$eq` is not decoration: the index is partial + // on `{ npub: { $type: "string" } }`, and mongo will not use a partial + // index unless the query provably matches a subset of its filter — which + // it cannot derive from an equality against a string literal. Without the + // restated predicate the lookup plans as a COLLSCAN. This mock cannot see + // that; `test/flash/integration/accounts/npub.spec.ts` explains the real + // plan and is what actually caught it. findOne.mockResolvedValue(accountRecord) const result = await AccountsRepository().findByNpub(NPUB) - expect(findOne).toHaveBeenCalledWith({ npub: { $eq: NPUB } }) + expect(findOne).toHaveBeenCalledWith({ npub: { $eq: NPUB, $type: "string" } }) expect(result).not.toBeInstanceOf(Error) expect((result as Account).npub).toBe(NPUB) }) From 3d6fb749c9c498b015767545b66763aa6d92556e Mon Sep 17 00:00:00 2001 From: Dread Date: Tue, 25 Aug 2026 12:25:08 -0700 Subject: [PATCH 9/9] feat(admin): add accountAssignNpub so a half-applied reassignment is recoverable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review residual on #495, and it is the gap the PR's own docblock warns about. releaseNpub is two writes with no transaction. When the second one fails, the key is unclaimed and the account it came off no longer holds it — so accountReleaseNpub cannot be re-run against that account (NoNpubToReleaseError), and the only other npub write in the codebase is userUpdateNpub, which is self-service and subject to the very race the reassignment exists to win. release-npub.ts:30-33 states it plainly: telling the victim to go re-link pits a human against a script. Support had nothing to run. accountAssignNpub is that second write on its own. It adds no new authority: the unique partial index still refuses a key another account holds (NpubNotAvailableError), and claimNpub's write-time `$not: { $type: "string" }` filter still refuses a target that already holds one (AccountAlreadyHasNpubError) rather than overwriting it. It can only ever fill a hole. Takes the `npub` scalar rather than String, so it validates and normalises at the boundary like accountDetailsByNpub — a mutation that mints a permanent identity claim should not accept a looser input than the query that reads one. Also fixes the recovery guidance the previous round added, which was wrong for two of the three causes it named: it told the operator to "re-run against another target" or "retry the same call", both of which re-enter unsetNpub and answer NoNpubToReleaseError. An operator following that at 3am concludes the release never happened and stops, while the key sits unclaimed for whatever script is polling isFlashNpub. Every branch now names the same real remedy, and the per-cause notes say whether assignment will succeed or the key must be hunted down first. 8 new unit tests. SDL regenerated (check:sdl green), tsc + eslint + build clean, all 90 npub tests pass. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01NEoz7nBtdtsHyuYG5wNPQV --- src/app/accounts/assign-npub.ts | 75 ++++++++++ src/app/accounts/index.ts | 1 + src/app/accounts/release-npub.ts | 22 ++- src/graphql/admin/mutations.ts | 2 + .../root/mutation/account-assign-npub.ts | 56 ++++++++ src/graphql/admin/schema.graphql | 11 ++ .../types/payload/account-assign-npub.ts | 21 +++ .../unit/app/accounts/assign-npub.spec.ts | 132 ++++++++++++++++++ 8 files changed, 315 insertions(+), 5 deletions(-) create mode 100644 src/app/accounts/assign-npub.ts create mode 100644 src/graphql/admin/root/mutation/account-assign-npub.ts create mode 100644 src/graphql/admin/types/payload/account-assign-npub.ts create mode 100644 test/flash/unit/app/accounts/assign-npub.spec.ts diff --git a/src/app/accounts/assign-npub.ts b/src/app/accounts/assign-npub.ts new file mode 100644 index 000000000..9a030c1ef --- /dev/null +++ b/src/app/accounts/assign-npub.ts @@ -0,0 +1,75 @@ +import { checkedToAccountId } from "@domain/accounts" +import { DuplicateKeyForPersistError } from "@domain/errors" +import { checkedToNpub, NpubNotAvailableError } from "@domain/nostr" +import { baseLogger } from "@services/logger" +import { AccountsRepository } from "@services/mongoose" + +/** + * The other half of `releaseNpub`'s reassignment, reachable on its own. + * + * `releaseNpub` is two writes and no transaction: the unset lands, then the + * claim. When the claim fails — mongo hiccups, or the target linked a key in + * the window — the key is unclaimed and the account it was taken off no longer + * holds it, so `accountReleaseNpub` cannot be re-run against that account: it + * returns `NoNpubToReleaseError`. Before this existed the only remaining npub + * write was `userUpdateNpub`, which is self-service and subject to the very + * race the reassignment exists to win — the squatter is the party polling + * `isFlashNpub`, so the desk's only advice was to pit a human against a script. + * This makes the second write retryable by an admin. + * + * Safety comes from the same two mechanisms the reassignment already relies on, + * not from new ones: the unique partial index refuses a key another account + * holds (surfaced as `NpubNotAvailableError`), and `claimNpub`'s write-time + * `$not: { $type: "string" }` filter refuses a target that already holds a key + * (`AccountAlreadyHasNpubError`) rather than silently overwriting it. So this + * can only ever fill a hole; it cannot take a key from anyone. + * + * `assignedByUserId` is the attribution trail, for the reason spelled out in + * `release-npub.ts`: the admin server never assigns `req.gqlContext`, so these + * structured lines are the only record of who moved a key. Refusals are logged + * too — a stolen admin token sweeping ids should not be able to probe silently. + */ +export const assignNpub = async ({ + id, + npub, + assignedByUserId, +}: { + id: string + npub: string + assignedByUserId: UserId +}): Promise => { + const idChecked = checkedToAccountId(id) + if (idChecked instanceof Error) return idChecked + + const npubChecked = checkedToNpub(npub) + if (npubChecked instanceof Error) return npubChecked + + const accountsRepo = AccountsRepository() + + const claimed = await accountsRepo.claimNpub(idChecked, npubChecked) + if (claimed instanceof Error) { + baseLogger.error( + { + accountId: idChecked, + npub: npubChecked, + assignedByUserId, + reason: claimed.name, + error: claimed.message, + }, + "admin npub assignment refused", + ) + // Same mapping the reassignment path uses, so one cause reads identically + // wherever it surfaces: the key belongs to someone else. + if (claimed instanceof DuplicateKeyForPersistError) { + return new NpubNotAvailableError(npubChecked) + } + return claimed + } + + baseLogger.info( + { accountId: idChecked, npub: npubChecked, assignedByUserId }, + "admin assigned an npub claim", + ) + + return claimed +} diff --git a/src/app/accounts/index.ts b/src/app/accounts/index.ts index a7b9b6407..145b32609 100644 --- a/src/app/accounts/index.ts +++ b/src/app/accounts/index.ts @@ -34,6 +34,7 @@ export * from "./enable-notification-channel" export * from "./disable-notification-channel" export * from "./set-npub" export * from "./release-npub" +export * from "./assign-npub" export * from "./find-by-npub" export * from "./update-external-wallet" diff --git a/src/app/accounts/release-npub.ts b/src/app/accounts/release-npub.ts index 1e5895738..ab3c1ca5c 100644 --- a/src/app/accounts/release-npub.ts +++ b/src/app/accounts/release-npub.ts @@ -146,14 +146,26 @@ export const releaseNpub = async ({ // Which failure, not just that one happened. Three distinct causes // reach this branch and each has a different recovery, so a line that // does not name the cause hands the operator all three procedures at - // once. `DuplicateKeyForPersistError` (surfaced to the caller as + // once. + // + // What is true for ALL of them: the release already landed, so this + // account no longer holds the key and `accountReleaseNpub` cannot be + // re-run against it — it answers `NoNpubToReleaseError`. Recovery is + // `accountAssignNpub(accountId, npub)`, which is the second write on + // its own and carries the same two guards (unique index, write-time + // refusal of a target that already holds a key). + // + // `DuplicateKeyForPersistError` (surfaced to the caller as // `NpubNotAvailableError`): someone else claimed the key in the window - // between the two writes — it is gone, find the new holder with - // `accountDetailsByNpub` and release it from there. + // between the two writes — it is gone, so assignment will refuse too; + // find the new holder with `accountDetailsByNpub` and release it from + // there first. // `AccountAlreadyHasNpubError`: the target linked a different key // after the pre-release check — the released key is still unclaimed, - // so re-run against another target. Anything else (e.g. - // `UnknownRepositoryError`): the write failed, retry the same call. + // so assign it to the right account before anyone polling + // `isFlashNpub` takes it. + // Anything else (e.g. `UnknownRepositoryError`): the write failed and + // the key is unclaimed — assign it to the intended target. // // Taken off the raw repository error, before the // `DuplicateKeyForPersistError` → `NpubNotAvailableError` mapping diff --git a/src/graphql/admin/mutations.ts b/src/graphql/admin/mutations.ts index ebd7ad7a9..e963753a1 100644 --- a/src/graphql/admin/mutations.ts +++ b/src/graphql/admin/mutations.ts @@ -2,6 +2,7 @@ import { GT } from "@graphql/index" import AccountUpdateLevelMutation from "@graphql/admin/root/mutation/account-update-level" import AccountUpdateStatusMutation from "@graphql/admin/root/mutation/account-update-status" +import AccountAssignNpubMutation from "@graphql/admin/root/mutation/account-assign-npub" import AccountReleaseNpubMutation from "@graphql/admin/root/mutation/account-release-npub" import BusinessUpdateMapInfoMutation from "@graphql/admin/root/mutation/business-update-map-info" import CashWalletCutoverUpdateMutation from "@graphql/admin/root/mutation/cash-wallet-cutover-update" @@ -23,6 +24,7 @@ export const mutationFields = { accountUpdateLevel: AccountUpdateLevelMutation, accountUpdateStatus: AccountUpdateStatusMutation, accountReleaseNpub: AccountReleaseNpubMutation, + accountAssignNpub: AccountAssignNpubMutation, merchantMapValidate: MerchantMapValidateMutation, merchantMapDelete: MerchantMapDeleteMutation, businessUpdateMapInfo: BusinessUpdateMapInfoMutation, diff --git a/src/graphql/admin/root/mutation/account-assign-npub.ts b/src/graphql/admin/root/mutation/account-assign-npub.ts new file mode 100644 index 000000000..e3959344e --- /dev/null +++ b/src/graphql/admin/root/mutation/account-assign-npub.ts @@ -0,0 +1,56 @@ +import { GT } from "@graphql/index" + +import AccountAssignNpubPayload from "@graphql/admin/types/payload/account-assign-npub" +import { Accounts } from "@app" +import { mapAndParseErrorForGqlResponse } from "@graphql/error-map" +import Npub from "@graphql/shared/types/scalar/npub" + +const AccountAssignNpubInput = GT.Input({ + name: "AccountAssignNpubInput", + fields: () => ({ + accountId: { + type: GT.NonNullID, + }, + // The `npub` scalar, not String: it validates and normalises at the + // boundary, the same way `accountDetailsByNpub` does. A mutation that + // mints a permanent identity claim should not accept a looser input than + // the query that merely reads one. + npub: { + type: GT.NonNull(Npub), + }, + }), +}) + +const AccountAssignNpubMutation = GT.Field< + null, + GraphQLAdminContext, + { input: { accountId: string | Error; npub: Npub | ValidationError } } +>({ + extensions: { + complexity: 120, + }, + type: GT.NonNull(AccountAssignNpubPayload), + args: { + input: { type: GT.NonNull(AccountAssignNpubInput) }, + }, + resolve: async (_, args, ctx) => { + const { accountId, npub } = args.input + const supportUser = ctx.user.id + + if (accountId instanceof Error) return { errors: [{ message: accountId.message }] } + if (npub instanceof Error) return { errors: [{ message: npub.message }] } + + const assigned = await Accounts.assignNpub({ + id: accountId, + npub, + assignedByUserId: supportUser, + }) + if (assigned instanceof Error) { + return { errors: [mapAndParseErrorForGqlResponse(assigned)] } + } + + return { errors: [], accountDetails: assigned } + }, +}) + +export default AccountAssignNpubMutation diff --git a/src/graphql/admin/schema.graphql b/src/graphql/admin/schema.graphql index c3c7e53b2..b40c21224 100644 --- a/src/graphql/admin/schema.graphql +++ b/src/graphql/admin/schema.graphql @@ -1,3 +1,13 @@ +input AccountAssignNpubInput { + accountId: ID! + npub: npub! +} + +type AccountAssignNpubPayload { + accountDetails: AuditedAccount + errors: [Error] +} + type AccountCapabilities { """An approved bank account is on file for payouts.""" bankPayout: Boolean! @@ -442,6 +452,7 @@ type MerchantPayload { } type Mutation { + accountAssignNpub(input: AccountAssignNpubInput!): AccountAssignNpubPayload! accountReleaseNpub(input: AccountReleaseNpubInput!): AccountReleaseNpubPayload! accountUpdateLevel(input: AccountUpdateLevelInput!): AccountDetailPayload! accountUpdateStatus(input: AccountUpdateStatusInput!): AccountDetailPayload! diff --git a/src/graphql/admin/types/payload/account-assign-npub.ts b/src/graphql/admin/types/payload/account-assign-npub.ts new file mode 100644 index 000000000..63977f1f2 --- /dev/null +++ b/src/graphql/admin/types/payload/account-assign-npub.ts @@ -0,0 +1,21 @@ +import { GT } from "@graphql/index" +import IError from "@graphql/shared/types/abstract/error" + +import GraphQLAccount from "../object/account" + +// `accountDetails` carries the npub this time (unlike the release payload, +// where the key has just been taken off), so the operator can confirm from the +// response alone that the key landed on the intended account. +const AccountAssignNpubPayload = GT.Object({ + name: "AccountAssignNpubPayload", + fields: () => ({ + errors: { + type: GT.List(IError), + }, + accountDetails: { + type: GraphQLAccount, + }, + }), +}) + +export default AccountAssignNpubPayload diff --git a/test/flash/unit/app/accounts/assign-npub.spec.ts b/test/flash/unit/app/accounts/assign-npub.spec.ts new file mode 100644 index 000000000..d02d22953 --- /dev/null +++ b/test/flash/unit/app/accounts/assign-npub.spec.ts @@ -0,0 +1,132 @@ +/** + * `releaseNpub` is two writes and no transaction. When the second one fails the + * key is unclaimed and the account it came off no longer holds it, so + * `accountReleaseNpub` cannot be re-run against it. `assignNpub` is that second + * write on its own — the only admin path that can finish a half-applied + * reassignment before the squatter's poller re-takes the key. + */ +const claimNpub = jest.fn() +const info = jest.fn() +const error = jest.fn() + +jest.mock("@services/mongoose", () => ({ + AccountsRepository: () => ({ claimNpub }), +})) + +jest.mock("@services/logger", () => ({ + baseLogger: { + info: (...args: unknown[]) => info(...args), + warn: jest.fn(), + error: (...args: unknown[]) => error(...args), + }, +})) + +import { InvalidAccountIdError } from "@domain/accounts" +import { + CouldNotFindAccountFromIdError, + DuplicateKeyForPersistError, + UnknownRepositoryError, +} from "@domain/errors" +import { + AccountAlreadyHasNpubError, + InvalidNpubError, + NpubNotAvailableError, +} from "@domain/nostr" +import { assignNpub } from "@app/accounts/assign-npub" + +const ACCOUNT_ID = "5f4c9a2b1e7d3f8a6b0c4d2e" as AccountId +const SUPPORT_USER_ID = "support-user-id" as UserId +const NPUB = `npub1${"q".repeat(58)}` + +const assign = (overrides: Record = {}) => + assignNpub({ + id: ACCOUNT_ID, + npub: NPUB, + assignedByUserId: SUPPORT_USER_ID, + ...overrides, + }) + +describe("assignNpub", () => { + beforeEach(() => jest.clearAllMocks()) + + it("claims the key for the account and returns it", async () => { + const account = { id: ACCOUNT_ID, npub: NPUB } + claimNpub.mockResolvedValue(account) + + const result = await assign() + + expect(claimNpub).toHaveBeenCalledWith(ACCOUNT_ID, NPUB) + expect(result).toBe(account) + }) + + it("records who assigned the key", async () => { + claimNpub.mockResolvedValue({ id: ACCOUNT_ID, npub: NPUB }) + + await assign() + + // The admin server never assigns req.gqlContext, so this line is the only + // record that an admin moved a key. + expect(info).toHaveBeenCalledWith( + expect.objectContaining({ + accountId: ACCOUNT_ID, + npub: NPUB, + assignedByUserId: SUPPORT_USER_ID, + }), + expect.any(String), + ) + }) + + it("rejects a malformed account id before touching the repository", async () => { + const result = await assign({ id: "not-an-object-id" }) + + expect(result).toBeInstanceOf(InvalidAccountIdError) + expect(claimNpub).not.toHaveBeenCalled() + }) + + it("rejects a malformed npub before touching the repository", async () => { + const result = await assign({ npub: "not-an-npub" }) + + expect(result).toBeInstanceOf(InvalidNpubError) + expect(claimNpub).not.toHaveBeenCalled() + }) + + it("reports a key held by someone else as unavailable, not as a raw duplicate", async () => { + // Same mapping the reassignment path uses, so one cause reads identically + // wherever it surfaces. + claimNpub.mockResolvedValue(new DuplicateKeyForPersistError()) + + const result = await assign() + + expect(result).toBeInstanceOf(NpubNotAvailableError) + }) + + it("refuses rather than overwriting when the target already holds a key", async () => { + claimNpub.mockResolvedValue(new AccountAlreadyHasNpubError(ACCOUNT_ID)) + + const result = await assign() + + expect(result).toBeInstanceOf(AccountAlreadyHasNpubError) + }) + + it("passes through a missing account", async () => { + claimNpub.mockResolvedValue(new CouldNotFindAccountFromIdError(ACCOUNT_ID)) + + const result = await assign() + + expect(result).toBeInstanceOf(CouldNotFindAccountFromIdError) + }) + + it("logs every refusal, so a token sweeping account ids cannot probe silently", async () => { + claimNpub.mockResolvedValue(new UnknownRepositoryError()) + + await assign() + + expect(error).toHaveBeenCalledWith( + expect.objectContaining({ + accountId: ACCOUNT_ID, + assignedByUserId: SUPPORT_USER_ID, + }), + expect.any(String), + ) + }) +})