DF-1160: Sign in and account creation - #7
Open
alexluckett wants to merge 46 commits into
Open
Conversation
Services become plain function modules over the mongo db live binding (collections resolved lazily inside each function), routes become static default-export arrays registered via the routes index, and createServer loses all service wiring. The Notify configuration guard now fails at module load — still boot time, one import earlier. Wire contract and behaviour are unchanged; tests move from injected fakes to jest.mock seams.
Repositories (accounts, otps, oidc artifacts, the Notify downstream client) own database and downstream-API mechanics; the signin service owns the business logic and state-machine filters; routes stay inline and translate raw service results into responses. Duplicate-key detection is exposed by the accounts repository so the service never inspects Mongo error codes.
findAccountById throws Boom.notFound for unknown accounts instead of returning null for the route to translate.
GET /accounts/{id} now includes emailVerified so the provider's claims
come from stored state instead of a hardcoded literal. Also corrects the
otps repository's Mongo filter typings surfaced by the full type check.
The otps repository's update() now returns whether a record matched, and the service treats that as the claim on each one-way state transition — a concurrent duplicate submission gets invalid instead of a second success decided from a stale read.
A resend upserts onto the same {uid, purpose} record, so a claim that
only checks the state flags could land on a superseded code's record.
Including codeHash in the claim filter makes the transition an optimistic
version check: only the exact record version that was verified can be
consumed or marked verified. The service test's in-memory reads now
return snapshots so stale-read races are genuinely exercised.
Joi validates the 6-digit pattern per hapi's validation model, so the service only ever sees well-formed codes and malformed requests are a 400 client error. The former in-service format check and its attempt-burn are gone with it.
…ne rule Ports the forms-engine-plugin Joi extension (minus its UK/international format restriction — any valid number is accepted) pending a shared library, and attaches it to POST /accounts so the service only sees real telephone numbers. The mobile-only requirement stays a service business rule behind the invalid-phone verdict.
The designer-updated journey shows one inline error for wrong or expired codes, so expiry is no longer a distinct verdict. The in-app expireAt check stays (Mongo's TTL sweep is lazy GC, not enforcement) but joins the invalid path.
Only session, interaction, grant, authorization_code and access_token can exist under this provider configuration (verified empirically against a database that has served every journey). Enabling a provider feature later means extending the list — a missing model fails loudly as a 400 on first use.
GET /otp/{uid} returns the target email so the UI no longer needs to
carry it in the redirect query string.
There is no unverified-email or phone-verification flow (checked
DF-1160/1161/1168): an account only exists after email OTP, and the
phone is capture-only. So emailVerified was always true and phoneVerified
always false — neither carried information. GET /accounts/{id} returns
{ id, email } and email_verified becomes a hardcoded claim on the UI.
Two parallel upserts for a fresh {uid, purpose} can both take the insert
path; the unique index rejects one, which previously became a 500. The
loser now retries onto the update path. Duplicate-key detection moves to
mongo.js so both repositories share it.
Claude-Session: https://claude.ai/code/session_011DbR7f2iFjZgW5oLoua4ct
A stale wrong guess racing a resend could spend, or burn, the freshly issued code. The counter increment and the burn now carry the same codeHash pin as the claim, so they only land on the record version the guess was made against. Tests also pin the attempt-budget boundary and the resend semantics (replacement, budget reset, reopening a burned interaction, and re-verification after a resend during the phone step). Claude-Session: https://claude.ai/code/session_011DbR7f2iFjZgW5oLoua4ct
oidc-provider owns artifact expiry. When it re-upserts without one, a leftover expireAt from an earlier write would let the TTL sweeper delete a live artifact, so the upsert now unsets it. Claude-Session: https://claude.ai/code/session_011DbR7f2iFjZgW5oLoua4ct
mongodb-memory-server runs the sign-in flow and the OIDC store against a genuine mongod (pinned to the platform's 6.0), covering what mocks never could: unique-index enforcement, concurrent-request races, atomic claim semantics and the startup indexes. Integration tests live in test/integration since they span modules; unit tests stay alongside their module. The repository mock-echo suites are retired in their favour. The unit layer's blind spots close too: the OIDC model allowlist is asserted against the real constant (400 for off-list models), the Notify JWT contract is asserted as literals against distinguishable test uuids, missing-key boot refusal is covered, and FIXED_LINE_OR_MOBILE regions are pinned as accepted. CI caches the mongod binary. Claude-Session: https://claude.ai/code/session_011DbR7f2iFjZgW5oLoua4ct
One file per behaviour cluster — journeys, code lifecycle, concurrency, startup indexes, oidc store — rather than one monolith or one file per scenario: each integration file boots its own mongod and server, so per-scenario files would pay that cost a dozen times over while describe blocks already name the scenarios. The shared boot/teardown and HTTP drivers live in test/helpers so the suites contain only scenarios. Claude-Session: https://claude.ai/code/session_011DbR7f2iFjZgW5oLoua4ct
The narrow allowlist test was superseded by the all-routes version; the fetch wrapper suite only restated its source, so the wrappers are now covered by the code that uses them. Integration describe titles drop the '(real Mongo)' style qualifiers — the directory already says that — and the concurrency suite documents what it proves under the event loop: invariants under any interleaving, with the collision branches forced deterministically at unit level. Claude-Session: https://claude.ai/code/session_011DbR7f2iFjZgW5oLoua4ct
jbarnsley10
reviewed
Aug 5, 2026
| env: 'NOTIFY_OTP_TEMPLATE_ID' | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Do we also need a NOTIFY_REPLY_TO_ID here?
Contributor
Author
There was a problem hiding this comment.
Added it. It's required config with no default, and we pass email_reply_to_id when we send. Emails from a real reply-to address are less likely to get marked as spam.
jbarnsley10
reviewed
Aug 6, 2026
| } | ||
| // oidc-provider payloads are stored under a `payload` field, so lookups by | ||
| // uid/grantId target the nested keys | ||
| await database.collection('session').createIndex({ 'payload.uid': 1 }, {}) |
There was a problem hiding this comment.
Does each GRANTABLE_COLLECTIONS_NAMES need this 'payload.uid' index, or is it explicitly only for 'session' like you've defined?
Contributor
Author
There was a problem hiding this comment.
Only sessions get looked up by uid, so that's the only one that needs it. Tokens get looked up by their own id and just store a pointer to the session.
jbarnsley10
reviewed
Aug 6, 2026
| if (!isDuplicateKeyError(err)) { | ||
| throw err | ||
| } | ||
| await coll().updateOne(key, update, { upsert: true }) |
There was a problem hiding this comment.
Is this a valid scenario? Should the call not just be thrown?
Contributor
Author
There was a problem hiding this comment.
Fair point, it was hiding a design flaw. I've removed it, so a racing request just fails now. I'll sort this out upstream by stopping double submissions in the UI.
jbarnsley10
reviewed
Aug 6, 2026
| request.payload | ||
| ) | ||
| await requestOtp(uid, email) | ||
| return h.response().code(204) |
Contributor
Author
There was a problem hiding this comment.
204 is appropriate because it tells the client that there's no body to read, which in this case there isn't. 200 also works but 204 is a slightly better fit.
jbarnsley10
reviewed
Aug 6, 2026
| */ | ||
| export async function requestOtp(uid, email) { | ||
| const target = email.toLowerCase() | ||
| const code = String(crypto.randomInt(0, 1_000_000)).padStart(6, '0') |
There was a problem hiding this comment.
Do we perhaps want the lowest number to be a bit more than zero e.g. 100000? Some users may think they don't need to enter leading zeros when confirming
Contributor
Author
There was a problem hiding this comment.
I did some research and this seems like quite a common implementation. I think we're fine to leave it as is
jbarnsley10
reviewed
Aug 6, 2026
| const ok = await argon2.verify(doc.codeHash, code) | ||
|
|
||
| if (!ok) { | ||
| const updated = await otpsRepository.incrementAttempts(claim) |
There was a problem hiding this comment.
Do these two DB operations need to be in a transaction? In fact, do all the DB operations in this method need to sit within a trnsaction?
Contributor
Author
There was a problem hiding this comment.
Not quite, a transaction would have rolled both writes back together and a failed OTP attempt wouldn't get counted if so.
However, your comment helped me spot that the second write was the only thing stopping a sixth guess. If it failed, or the service died in between, the record would have stayed unconsumed and they could have tried again on attempt six.
I've fixed it by checking the attempt count when we read the record. The count is already saved by then, so it'll be immediately refused.
jbarnsley10
reviewed
Aug 6, 2026
jbarnsley10
left a comment
jbarnsley10
left a comment
There was a problem hiding this comment.
Excellent, and great unit tests. Just a few questions to think about.
oidc-provider records each client assertion's jti so a captured assertion cannot be replayed within its lifetime. That store is a new model, and the allowlist rejected it, so every token request failed as an opaque 500 the moment forms-identity-ui switched its client authentication.
- name the repeated /oidc/{model}/{id} route path once
- use StatusCodes.NO_CONTENT rather than a bare 204 in five handlers
- name the one-time code's length, and derive its range from it
- rename a local in upsert() that shadowed the exported update()
Share one mongod boot timeout between the two helpers, use the named status codes rather than bare numbers, and lift lastSentCode out of the setup closure — it reads the Notify mock and closes over nothing.
Notify falls back to the service default reply-to when none is given, and a mail with no real reply-to is likelier to be treated as spam. The id is required in every environment and has no default, so a deployment without one fails at boot rather than quietly sending mail that lands in a junk folder.
A code starting with a zero invites a citizen reading "012345" to type "12345", so the range now starts at the first six digit number. That costs a tenth of the keyspace, which is immaterial against a five attempt lockout and a fifteen minute life. replay_detection was added to the model allowlist when private_key_jwt landed, but not to the list that creates TTL indexes, so those records had an expireAt no sweeper would ever act on. Both lists were the same set written twice; the allowlist now derives from the index list, so the next model cannot be added to one and missed from the other.
The startup index test asserted against 'session' and 'access_token' by name, so replay_detection could be added to the model list with no TTL index and nothing failed. It now iterates the lists themselves, which is what catches the next model added without one. Also makes the session uid index unique, matching the provider's own reference adapter. The uid is generated per sign-in, so a collision is a fault worth failing on.
A code of 1 is issued, hashed, emailed and typed as "000001". The padding is part of the code, so the whole million-value range stays in use rather than the 900,000 that excluding leading zeros left. Input stays strictly six digits at both the route and the UI — we never pad on a citizen's behalf, so what they read is what they type. Correct-code tests submit whatever was randomly generated, so a code with leading zeros only got exercised about one run in ten. The integration tests force one and walk it through the routes in both directions, and pin that "1" is turned away at route validation with the real code still working afterwards, because the attempt was never spent.
Two requests creating the same record at once left one to be rejected by the unique index, and the retry here made it succeed by landing on the update path. That papered over the real problem: both requests still send an email with a different code, so whichever arrives first may already be dead. Handled upstream instead, by not letting a citizen submit twice.
Reaching the limit marked the record used, and that write was the only thing stopping a sixth guess. If it failed, or the process died between the increment and it, the record stayed open and guessing carried on. Proven with a test: with that write failing, a sixth attempt with the right code succeeded. The count is already durable by then, so checking it when the record is read is enough. Marking the record used stays, but as tidy-up rather than the guard, alongside the expiry check that exists for the same reason.
SonarCloud flagged verifyOtp at a complexity of 12 against a limit of 10, and the count was fair. The expiry and attempt-budget checks each carried their own null guard for a document the very next line rejects anyway, so the reasons a record is refused were spread over three statements and a three-armed condition. Both are named now: isGuessable answers whether a record just read is still open to a guess, and spendAttempt does the increment plus the mark-used tidy-up. The prose explaining why each exists moves with it. Behaviour is unchanged — !isGuessable(doc) is the same isExpired || isSpent it replaces — and verifyOtp is down to a complexity of 7.
|
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.
Backend for Defra Forms sign-in: email OTP checking, account creation, and storage for the OIDC provider. Only called by forms-identity-ui over the internal network.
Architecture
Provides data storage for the upstream OIDC flow. The provider's artifacts — sessions, grants, interactions, authorization codes and access tokens — are persisted in MongoDB through per-model CRUD endpoints (/oidc/{model}/…).
{model} is a route parameter rather than a hardcoded endpoint per artifact type because the frontend's OIDC library decides which models to store and calls the same six storage operations on all of them — the endpoints are dumb storage that treat every model the same way, so one generic set of handlers covers them. To stop the parameter reaching arbitrary collections, it's checked against an allowlist of the models this configuration actually uses; anything else is rejected.
Alongside that, it owns the sign-in domain: OTP records and accounts, exposed through named endpoints the frontend calls to run the journey (/otp/*, /accounts), backed by GOV.UK Notify for code delivery.
Key changes
Routes
Domain endpoints (called by forms-identity-ui to run the journey):
POST /otp/requestPOST /otp/verifyGET /otp/{uid}POST /accountsGET /accounts/{id}sub,email)OIDC artifact storage (called by forms-identity-ui to store the OIDC provider's artifacts,
{model}allowlisted):PUT /oidc/{model}/{id}GET /oidc/{model}/{id}GET /oidc/{model}/uid/{uid}POST /oidc/{model}/{id}/consumeDELETE /oidc/{model}/{id}DELETE /oidc/grants/{grantId}Example of models: authorization_code, access_token, session
Out of scope