feat(admin): add accountDetailsByNpub query - #495
Merged
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015xSxhf5Kijfib4PbExGESk
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<null, GraphQLAdminContext, {...}>), 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015xSxhf5Kijfib4PbExGESk
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015xSxhf5Kijfib4PbExGESk
…okup 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015xSxhf5Kijfib4PbExGESk
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015xSxhf5Kijfib4PbExGESk
…l-npub targets
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015xSxhf5Kijfib4PbExGESk
…ookup
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.
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NEoz7nBtdtsHyuYG5wNPQV
…recoverable 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NEoz7nBtdtsHyuYG5wNPQV
islandbitcoin
approved these changes
Aug 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The in-app Nostr support channel creates Chatwoot contacts keyed only by the sender's npub — support agents see
npub1hy6r…8fq6and four "Unavailable" fields. The backend already knows the npub→account mapping (every app user's npub is synced viauserUpdateNpub), but nothing reachable exposes a reverse lookup.What
accountDetailsByNpub(npub: npub!): AuditedAccount!on the admin GraphQL API, mirroring the existingaccountDetailsBy{Username,UserPhone,Email,AccountId}queries.Admin.getAccountByNpubdelegating toAccountsRepository().findByNpub(exact$eqmatch; format validation happens at the GraphQL boundary via the existingnpubscalar).accountReleaseNpubadmin mutation — the revocation path for a squatted key, with optional reassignment to the rightful owner.accounts.npub, plus the migration that lowercases and de-duplicates before building it.yarn write-sdl.Consumed by the ERP-side support lookup endpoint (frappe-flash-admin) which relays it to the nostr-dm-bridge so it can populate the Chatwoot contact card.
Rollout — migration must land before the pods
src/migrations/20260824120000-accounts-unique-npub.tsbuilds the first unique index on a field that holds duplicates in production, andschema.tsnow declares that index onAccountSchematoo.graphql-main-server.tsboots withsetupMongoConnection(true), which runssyncIndexes()over every model: against un-deduped datacreateIndexrejects with E11000,setupMongoConnectionrethrows (src/services/mongodb/index.ts:107), and the.catchingraphql-main-server.tslogs 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.The chart already enforces the ordering, and this PR relies on it: every galoy workload — api, websocket, trigger, exporter, the ibex/bridge/fygaro webhooks and the cronjobs — carries a
wait-for-mongodb-migrateinitContainer (groundnuty/k8s-wait-forjob-wr) that blocks on the per-revision<release>-mongodb-migrate-<revision>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 producing the dead-pod failure above:
galoy.images.app.digestwithout bumpinggaloy.images.mongodbMigrate.digestin the same release — the migrate Job then runs an older image that does not contain this migration, succeeds, and lets the new app through against un-migrated data. Bump both digests together.If a pod is up with no listener after this ships, check that the revision's migrate Job ran
20260824120000-accounts-unique-npubbefore touching anything else. The same note lives in the migration's own docstring.Tests
yarn test:unit— 215 suites / 2304 tests green.yarn test:integration— includes the newtest/flash/integration/accounts/npub.spec.ts, which exercises the npub repository layer against a real mongo:claimNpubonto an account with nonpubfield, onto a legacy explicitnpub: null, onto one already holding a key (refused, held key unchanged), two accounts claiming the same key (unique partial index →DuplicateKeyForPersistError),findByNpub,unsetNpub, andreleaseNpub's reassignment end to end. It sits underintegration/accounts/rather thanintegration/services/because the integration jest config ignores theservicesandwalletdirectories.findByNpubwas planning as aCOLLSCAN. Dropping the case-insensitive collation was only half of it — mongo will not select an index whose partial filter is$type-shaped for a bare equality, so the query now restates$type: "string"alongside the$eq. 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.make check-code—tsc-check-noimplicitany,tsc-check,eslint-check,build,check:sdl(no schema diff),check-yaml,madge-checkall clean.🤖 Generated with Claude Code
https://claude.ai/code/session_015xSxhf5Kijfib4PbExGESk