feat: stateful single-use magic link authentication - #49
Conversation
Replace the replayable signed-URL magic link with a stateful, single-use engine (MagicLinkManager) backed by hashed, opaque tokens stored in magic_link_tokens. - Single-use by row deletion; generating a new link invalidates the user's previous link for that channel - Config-driven channels (web/mobile/custom) with extensible URL building - Optional browser/device binding and confirmation-required flow - Single GET/POST route for login + confirmation (web + API) - Lifecycle events: MagicLinkGenerated / Consumed / Rejected - neev:clean-magic-links command to prune expired tokens
The old tests exercised signed-URL redemption, which 1fc7a47 replaced with stateful tokens. Redeem real tokens via MagicLinkManager, and add coverage for single-use replay and previous-link invalidation.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
sambhav-aggarwal
left a comment
There was a problem hiding this comment.
Reviewed against current main and the design principles / RFC 003 direction. The architecture is exactly right — stateful single-use tokens with hashed-at-rest storage, invalidate-previous, channel awareness, lifecycle events, and tenant scoping via BelongsToTenant (cross-tenant redemption is blocked for free). This is where magic links should go, and MagicLinkManager::resolve() is precisely where RFC 003's enforce_method policy check will later plug in — no structural conflict. But it's not mergeable yet: one flow is broken end-to-end, and one default re-creates the problem the PR sets out to fix.
Blocking
1. The web (Blade) flow is broken end-to-end.
UserAuthController::sendLoginLinkbuildsroute('login.link.verify', …)— that route name is registered nowhere →RouteNotFoundException(500) the moment a Blade user requests a link.- The existing
login.linkroute (GET /login/{id}) still points atUserAuthController::loginUsingLink— a method this PR renamed toverifyLoginLink→ fatal error on any old-style hit. stubs/blade/views/auth/confirm-login-link.blade.phpalso posts to the unregisteredlogin.link.verify.- CI is green only because the rewritten
MagicLinkTestcovers the API flow exclusively. Please add web feature tests (send → click → session login; confirmation variant) — they would have caught all of this.
Fix: register Route::match(['get','post'], '/login/link/verify', [UserAuthController::class, 'verifyLoginLink'])->name('login.link.verify') in the blade-gated section (throttled), and remove or repoint the dead /login/{id} route.
2. The default config re-creates the scanner problem — worse than before. With require_confirmation => false (the default), a GET consumes the single-use token. Corporate mail scanners (Outlook SafeLinks, Mimecast) prefetch GET links — so the scanner consumes the link before the human ever clicks, and every user behind such a gateway is locked out. The old replayable signed links at least survived prefetch. The PR's own docblock calls the confirmation GET "scanner-safe", which concedes the vector. Recommendation: never consume on GET — GET always validates only; the web flow renders the confirm page (or an auto-submit form) and the API returns confirmation_required for the frontend to POST. If you want to keep the toggle, its default must be true. (Related: the maintainer-approved email-verification work in #51 exists partly because of this same scanner behaviour.)
3. bind_to_browser => true bricks links that captured no binding source. Generation via a sessionless API request with no X-Device-Id/binding stores fingerprint = null; at redemption bindingMatches(null, …) returns false → BINDING_MISMATCH → the link can never be redeemed. Either refuse to generate when binding is enabled but no source exists, or treat a null stored fingerprint as unbound. Fail-closed is right only if generation guarantees a fingerprint.
Important
- UPGRADING.md entry is missing and this is a breaking behaviour change: links are single-use now; default expiry drops 60 → 10 minutes; the API redemption contract changes from signed-URL params to a
tokenparam (frontends'/login-linkpages must forwardtoken); clients must handle the newconfirmation_requiredauth_state. - Rebase needed (branch is CONFLICTING): v0.5.0 was released — your CHANGELOG entry must move under the new
[Unreleased]. - The redemption and validate routes have no throttle. Token entropy (32 bytes) makes brute force moot, but
throttle:10,1is cheap hygiene, consistent with the neighbouring routes. - The config comment describes a
'signed_url' | 'stateful'driver choice, but nodriverkey exists — stateful is simply the implementation now (the right call). Delete the stale comment so nobody looks for the knob.
Nits
- New events live in
Events\MagicLink\*— existing events are flat underEvents\; also add them to the events table indocs/README.md. meta_datais astring(255) column with anarraycast — works for one fingerprint, but make itjson/textso it doesn't surprise the next field added.- Route name
neev.loginUsingLink.validatemixes naming conventions with its unnamed-prefix sibling.
Happy to re-review quickly once the web flow + scanner default are addressed — the core engine itself is in good shape.
Replace the stateless signed-URL magic-link flow with opaque, single-use tokens stored hashed in a new magic_link_tokens table. Adds channel-aware link generation, optional browser/device binding, a confirmation-required step (scanner-safe GET), unverified-user policy, lifecycle events, and the neev:clean-magic-links command.
Replace the replayable signed-URL magic link with a stateful, single-use engine (MagicLinkManager) backed by hashed, opaque tokens stored in magic_link_tokens.
Description
Type of Change
Checklist
composer lintpassescomposer analysepassescomposer testpassesCHANGELOG.md