diff --git a/src/Auth/Services/UserOtpService.php b/src/Auth/Services/UserOtpService.php index 393d6b6..8a43380 100644 --- a/src/Auth/Services/UserOtpService.php +++ b/src/Auth/Services/UserOtpService.php @@ -5,6 +5,7 @@ namespace Modules\Core\Auth\Services; use Illuminate\Contracts\Auth\Authenticatable; use Illuminate\Http\Request; use Illuminate\Support\Facades\Auth; +use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Event; use Illuminate\Support\Facades\Mail; @@ -29,17 +30,25 @@ use Modules\Core\Auth\Mail\UserOtpMail; * here: doing our own on top would run a SECOND merge attempt with a * hardcoded policy that ignores whatever the consumer configured. * - * generateAndSend()'s find-or-create already triggers the full - * Customer/User pairing cascade for a genuinely new email — see - * Modules\Core\Auth\Events\UserCreated's own docblock and - * Modules\Core\Customer\Listeners\CreateCustomerForUser. + * generateAndSend() does NOT create a User row for an email it hasn't + * seen before — it used to (firstOrCreate() ran unconditionally), which + * meant this login FORM was effectively a registration form: anyone could + * create a real User (and, via UserCreated's own cascade, a paired + * Customer) for any email address they liked, whether or not a single + * correct code was ever entered. A genuinely new email's pending code now + * lives in the cache (see pendingKey()), keyed by email, with no DB row + * at all — firstOrCreate() and UserCreated only fire from validate(), and + * only once the code has actually been proven correct. An email that + * already has a User row is unaffected: its OTP state still lives on that + * row's own otp_code/otp_expires_at/otp_attempts columns exactly as + * before, so a returning shopper's login is unchanged. * * Two independent throttles, both configured under core.auth.otp — see * config/core.php's own comment for why they're separate: max_attempts * caps wrong guesses against ONE code; generation_limit caps how often a * NEW code can be requested for the same email at all (closes both the * "regenerate to reset my guess count" loophole and mail-bombing one - * inbox). + * inbox). Both apply identically whether or not a User row exists yet. * * validate() also records a UserSessionService entry for the new login — * see that class's own docblock for the "logout everywhere" registry @@ -74,28 +83,27 @@ class UserOtpService RateLimiter::hit($limiterKey, (int) config('core.auth.otp.generation_decay_minutes', 10) * 60); $model = config('auth.providers.users.model'); - $user = $model::firstOrCreate(['email' => $email]); - - // wasRecentlyCreated is Eloquent's own "did firstOrCreate() just - // INSERT, or did it find an existing row" flag — the only reliable - // way to tell them apart from firstOrCreate()'s return value alone. - // Without this check, a genuinely new signup never fired - // UserCreated at all (this class's own docblock claimed the - // Customer/User pairing cascade "already triggers" here, which was - // false as written — see Modules\Core\Customer\Listeners\ - // CreateCustomerForUser, which depends entirely on this event). - if ($user->wasRecentlyCreated) { - Event::dispatch(new UserCreated($user)); - } - + $user = $model::where('email', $email)->first(); $code = str_pad((string) random_int(0, 999999), self::CODE_LENGTH, '0', STR_PAD_LEFT); - $user->otp_code = $code; - $user->otp_expires_at = now()->addMinutes(self::EXPIRY_MINUTES); - $user->otp_attempts = 0; - $user->save(); + if ($user) { + $user->otp_code = $code; + $user->otp_expires_at = now()->addMinutes(self::EXPIRY_MINUTES); + $user->otp_attempts = 0; + $user->save(); + } else { + // No row yet — deliberately not created here. See this + // class's own docblock for why: creating one on every + // generateAndSend() call let anyone mint real User/Customer + // rows for an email nobody proved they owned. + Cache::put($this->pendingKey($email), [ + 'code' => $code, + 'expires_at' => now()->addMinutes(self::EXPIRY_MINUTES)->timestamp, + 'attempts' => 0, + ], now()->addMinutes(self::EXPIRY_MINUTES)); + } - Mail::to($user->email)->send(new UserOtpMail($user->name ?? $user->email, $code)); + Mail::to($email)->send(new UserOtpMail($user->name ?? $email, $code)); return true; } @@ -106,19 +114,44 @@ class UserOtpService * a fresh one via generateAndSend() (itself throttled independently * — see this class's own docblock) rather than being able to keep * guessing against a still-live code for the rest of its 10-minute - * expiry window. + * expiry window. Applies identically to the cache-backed (no User row + * yet) and DB-backed (existing User row) paths. */ public function validate(string $email, string $code, ?Request $request = null): ?Authenticatable { $model = config('auth.providers.users.model'); + $existing = $model::where('email', $email)->exists(); - // lockForUpdate() + a transaction make the read-check-increment-save - // below atomic across concurrent requests for the same user — without - // it, two guesses fired in parallel can each read the same - // pre-increment otp_attempts value and both save past - // max_attempts, letting an attacker exceed the lockout by - // parallelizing requests instead of sending them serially. - $result = DB::transaction(function () use ($model, $email, $code) { + $result = $existing + ? $this->validateExisting($model, $email, $code) + : $this->validatePending($model, $email, $code); + + if (! $result) { + return null; + } + + RateLimiter::clear($this->generationLimiterKey($email)); + + Auth::login($result); + + $this->sessions->record($result, $request); + + Event::dispatch(new UserAuthenticated($result)); + + return $result; + } + + /** + * lockForUpdate() + a transaction make the read-check-increment-save + * atomic across concurrent requests for the same user — without it, + * two guesses fired in parallel can each read the same pre-increment + * otp_attempts value and both save past max_attempts, letting an + * attacker exceed the lockout by parallelizing requests instead of + * sending them serially. + */ + private function validateExisting(string $model, string $email, string $code): ?Authenticatable + { + return DB::transaction(function () use ($model, $email, $code) { $user = $model::where('email', $email)->lockForUpdate()->first(); if (! $user || ! $user->otp_expires_at || now()->isAfter($user->otp_expires_at)) { @@ -146,24 +179,64 @@ class UserOtpService return $user; }); + } - if (! $result) { - return null; - } + /** + * No User row exists yet, so there's nothing to lockForUpdate() — + * Cache::lock() is the equivalent guard against two parallel guesses + * against the same pending signup both reading the same pre-increment + * attempts count. The User (and, via UserCreated, its paired Customer) + * is only ever created here, once the code has actually been proven + * correct — never from generateAndSend(). + */ + private function validatePending(string $model, string $email, string $code): ?Authenticatable + { + $key = $this->pendingKey($email); - RateLimiter::clear($this->generationLimiterKey($email)); + return Cache::lock("{$key}:lock", 10)->block(5, function () use ($model, $email, $code, $key) { + $pending = Cache::get($key); - Auth::login($result); + if (! $pending || now()->timestamp > $pending['expires_at']) { + return null; + } - $this->sessions->record($result, $request); + if (! hash_equals((string) $pending['code'], $code)) { + $pending['attempts']++; - Event::dispatch(new UserAuthenticated($result)); + if ($pending['attempts'] >= (int) config('core.auth.otp.max_attempts', 5)) { + Cache::forget($key); + } else { + Cache::put($key, $pending, now()->addMinutes(self::EXPIRY_MINUTES)); + } - return $result; + return null; + } + + Cache::forget($key); + + $user = $model::firstOrCreate(['email' => $email]); + + // wasRecentlyCreated is Eloquent's own "did firstOrCreate() + // just INSERT, or did it find an existing row" flag. Always + // true here in practice (validatePending() only runs when no + // row existed moments ago), but checked anyway rather than + // assumed, in case of an extremely unlikely race with a + // signup completed through some other path in between. + if ($user->wasRecentlyCreated) { + Event::dispatch(new UserCreated($user)); + } + + return $user; + }); } private function generationLimiterKey(string $email): string { return 'otp-generate:'.strtolower($email); } + + private function pendingKey(string $email): string + { + return 'otp-pending:'.strtolower($email); + } }