Fix/oauth second factor and recovery codes - #6
Merged
Conversation
No new table — magic-link tokens are single-use, expiring, hashed tokens tied to a user, exactly what VerificationStore (already used for email-change confirmation) already models. PurposeMagicLink is a separate value from PurposeEmailVerify even though both are 'click a link in your email': the Purpose check on read is what stops a leaked/guessed email-verification link from ever being replayed as a login link, or vice versa. Also adds a magic_link_requested audit event type.
Deliberately a separate interface from EmailSender rather than a new method added to it — EmailSender already shipped in an earlier release, and adding a required method to an existing interface would break every consuming app's existing implementation at compile time. The two are also genuinely different concerns for the app to word differently: 'confirm your new email' reads nothing like 'click to log in,' and EmailSender.SendVerification has no way to signal which one it's sending.
Pulls the 'check confirmed second-factor methods, then either pause with *ErrSecondFactorRequired or finish the login' logic out of Login into a standalone completePrimaryAuth, shared by any primary authentication path — magic-link login (landing in the next few commits) reuses it verbatim rather than reimplementing the same check slightly differently, which is exactly the kind of drift that could let a new login method accidentally bypass the second-factor gate. Pure extraction — Login's own behavior is unchanged.
RequestMagicLink logs in an existing account only — it never creates one. Returns nil for a nonexistent email exactly as it would for a real one, and never calls the sender in that case, to avoid leaking which emails are registered; a genuine delivery failure for an existing account still propagates, since that's an operational concern distinct from enumeration. CompleteMagicLink marks the token used immediately after validation, before any second-factor check or session creation, so a link can never be replayed even if something later in the call fails. It routes through the same completePrimaryAuth gate password login uses — an account with TOTP/a passkey enrolled pauses here exactly as it would after a correct password. magicLinkTTL is fixed at 15 minutes, not configurable — same reasoning as mfaPendingTTL: a passwordless login link is a bearer credential for the account it's mailed to, and a tuning knob here invites widening it well past what 'click the link you just got' actually needs.
Covers: sending only for existing accounts and never revealing which emails aren't registered, single-use enforcement, expiry, a wrong-purpose token (e.g. email-change) correctly rejected as a login token, and an account with TOTP enrolled correctly pausing for a second factor on completion instead of logging straight in.
- Config.MagicLinkSender (optional, notify.MagicLinkSender); requires Config.Verifications to also be set (validated in New). - New facade functions: RequestMagicLink, CompleteMagicLink, each returning cryden.ErrMagicLinkNotConfigured if called without Config.MagicLinkSender set.
Also fixes a stale 'not in v2' line that still listed WebAuthn and was about to incorrectly list magic links too, now that both are built; notes passwordless-primary passkey login as the planned fast-follow this and WebAuthn's shared plumbing sets up.
Runnable end-to-end check with no database dependency: go run ./cmd/smoketest/magic-link. Walks request-for-nonexistent-email (silently returns nil, sender never called), request-and-complete for a real account, single-use enforcement, a garbage token, and — using a real generated TOTP code, not a stub — an account with TOTP enrolled correctly pausing on *auth.ErrSecondFactorRequired instead of logging straight in.
Step 6 (enrolling TOTP to verify the second-factor pause) called cryden.EnrollTOTP against an engine built without Config.TOTP or Config.EncryptionKey set, so it failed immediately with ErrTOTPNotConfigured before ever reaching the actual check this step exists to verify.
CodeHash uses the same fast SHA-256 hash as refresh tokens (token.HashToken), not bcrypt — a recovery code is a high-entropy random value generated by the engine, not a user-chosen secret, so there's no weak-guessing risk a slow hash would defend against. Also adds recovery_codes_generated/recovery_code_used/ recovery_code_failed audit event types.
For tests and local experimentation only, matching the existing in-memory store conventions (not a supported production backend).
code_hash is the primary key directly rather than a separate id column — it's already globally unique on its own (random, high- entropy), so a separate surrogate key would add nothing.
LoginWithOAuth predated the second-factor work and did its own inline
session issuance — an account with TOTP/a passkey enrolled would log
straight in via a linked OAuth identity with NO second-factor check
at all. It now takes totpStore/webauthnStore/recoveryCodeStore params
(all nil-safe) and routes through the same completePrimaryAuth gate
password/magic-link login use. Confirming an OAuth identity proves the
primary factor, exactly like a correct password; it was never meant
to bypass a confirmed second one.
The pre-fix code also tagged its login_success audit event with which
provider was used. completePrimaryAuth/finishLogin gained an
extraMetadata param specifically to preserve that detail through the
shared helper — LoginWithOAuth passes {"provider": provider}, other
callers pass nil.
While threading recoveryCodeStore through (added alongside TOTP/
WebAuthn in the same signature change, since Go requires every call
site to move together or the build breaks), completePrimaryAuth also
gained the recovery-code safety property: "recovery_code" is only
ever added to Methods when a real factor (totp/webauthn) is also
present. An account that disabled its last real second factor but
still has unconsumed recovery codes sitting in storage must never have
those codes silently become a permanent standalone backdoor.
Updates existing lockout_test.go/login_test.go/login_totp_test.go/
magiclink_test.go/oauth_test.go call sites for both signature changes.
Covers: an account with TOTP confirmed pauses on the second OAuth login for the same linked identity instead of logging straight in, and the login_success audit event still gets tagged with which provider was used — the detail the pre-fix inline code recorded, confirming it survived the move into completePrimaryAuth.
GenerateRecoveryCodes requires the account to already have a confirmed
TOTP secret or a registered passkey (ErrNoSecondFactorEnrolled
otherwise) — codes exist to recover access to a real second factor,
not to stand in as one on their own. Always replaces the previous
batch in full; every old code, used or not, stops working the moment
a new batch is generated. Raw codes are returned exactly once — the
engine only ever stores their hashes.
CompleteLoginWithRecoveryCode normalizes case/whitespace before
hashing, since these get retyped by hand and the formatting
("ABCDE-FGHIJ") is purely for readability.
Covers: generation rejected with no second factor enrolled, 10 unique codes produced, regeneration invalidating the previous batch entirely, single-use enforcement, case/whitespace-insensitive matching, a wrong code rejected, and the two Login-level safety properties: recovery_code is only ever advertised alongside a real factor, and leftover codes never gate login on their own once the real factor is disabled.
- Config.RecoveryCodes (optional, store.RecoveryCodeStore). - New facade functions: GenerateRecoveryCodes, CompleteLoginWithRecoveryCode, each returning cryden.ErrRecoveryCodesNotConfigured if called without Config.RecoveryCodes set.
Runnable end-to-end check with no database dependency: go run ./cmd/smoketest/recovery-codes. Walks generation rejected with no second factor, a real batch of 10 codes, login completion, single-use enforcement, a wrong code, regeneration invalidating the previous batch, and the property that matters most: disabling the account's only real second factor and confirming leftover recovery codes stop gating login entirely rather than becoming a standalone backdoor.
Missed in the earlier signature-change commit — this file's Login() call sites still had the pre-recovery-codes arity and would have failed to compile.
…ator NewCryptoRandTokenGenerator enforces a 128-bit minimum meant for session/refresh tokens and rejected the 5-byte length used here outright — GenerateRecoveryCodes failed unconditionally with ErrTokenByteLengthTooShort before ever producing a code. Recovery codes are generated directly via crypto/rand instead, using 8 bytes (64 bits) per code — short and human-typeable by design, single-use, and rate-limited the same as any other login attempt, so the 128-bit session-token bar was never the right minimum for this value in the first place. Codes are now formatted as four dash- separated 4-character hex groups (e.g. a1b2-c3d4-e5f6-a7b8); hashing strips the dashes (and case/whitespace) first, so formatting is purely cosmetic.
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
1 participant
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.
No description provided.