From a65ea5acf1cf6a322f419f2ef241b55ded292dd6 Mon Sep 17 00:00:00 2001 From: Sourov Biswas Date: Sun, 20 Sep 2026 10:43:39 +0600 Subject: [PATCH] Verify email addresses before they reach the suite Accounts did not skip verification so much as assert it falsely. CreateNewUser stamped email_verified_at on every registration, and IdentityEntity published `email_verified` from that column -- so the claim in every id_token and userinfo response was true by construction. The verify-email view and Fortify::verifyEmailView() were already written; only the feature flag was commented out, so the route they served did not exist. That mattered because the apps downstream match on the address. Invitations are found by `pendingFor($user->email)` and accepted on a lowercased string comparison, and the SSO callback adopts a pre-existing local account by email. An address nobody had proved they held was enough to claim either. Turn the feature on, implement MustVerifyEmail so the framework's checks actually fire, and stop stamping the column at registration. Existing users keep the verified_at they already have: they are accounts we created or know, and re-verifying them would lock them out to prove a point. Registration signs the new user in before they have opened the link, so the authorization screen is the door that had to be shut -- otherwise they walk straight out with tokens. EnsureEmailIsVerifiedForOAuth sits on the Passport route group and redirects them to the notice instead. It reads a user rather than demanding one, because that group also carries /oauth/token and /oauth/userinfo, which the client calls with no session to read. Passport applies its configured middleware as *group* middleware, which puts it ahead of the `web` group and so ahead of the session the gate reads. Left that way the gate resolves a null user and passes everyone through while looking correct, so bootstrap/app.php names it in the priority list after StartSession. Confirmed against a real server: unverified user, cold request, 302 to /email/verify with that entry and 200 without it. The request tests cannot see that difference -- Laravel's test client keeps one session store alive across the requests in a test, so the store is already started by the time the gate runs and the user resolves either way. The ordering test is the guard that does catch it. Co-Authored-By: Claude Opus 5 --- app/Actions/Fortify/CreateNewUser.php | 9 +- .../EnsureEmailIsVerifiedForOAuth.php | 42 ++++ app/Models/User.php | 9 +- bootstrap/app.php | 11 +- config/fortify.php | 2 +- config/passport.php | 17 +- routes/web.php | 4 +- tests/Feature/Auth/EmailVerificationTest.php | 208 ++++++++++++++++++ 8 files changed, 292 insertions(+), 10 deletions(-) create mode 100644 app/Http/Middleware/EnsureEmailIsVerifiedForOAuth.php create mode 100644 tests/Feature/Auth/EmailVerificationTest.php diff --git a/app/Actions/Fortify/CreateNewUser.php b/app/Actions/Fortify/CreateNewUser.php index dadf13f..3a3b847 100644 --- a/app/Actions/Fortify/CreateNewUser.php +++ b/app/Actions/Fortify/CreateNewUser.php @@ -34,14 +34,13 @@ public function create(array $input): User 'password' => $this->passwordRules(), ])->validate(); - $user = User::create([ + // Left unverified on purpose. Registration fires Registered, which + // sends the link that stamps email_verified_at, and every app in the + // suite trusts the address this returns. + return User::create([ 'name' => $input['name'], 'email' => $input['email'], 'password' => Hash::make($input['password']), ]); - - $user->forceFill(['email_verified_at' => now()])->save(); - - return $user; } } diff --git a/app/Http/Middleware/EnsureEmailIsVerifiedForOAuth.php b/app/Http/Middleware/EnsureEmailIsVerifiedForOAuth.php new file mode 100644 index 0000000..e94aa67 --- /dev/null +++ b/app/Http/Middleware/EnsureEmailIsVerifiedForOAuth.php @@ -0,0 +1,42 @@ +user(); + + if ($user instanceof MustVerifyEmail && ! $user->hasVerifiedEmail()) { + return redirect()->route('verification.notice'); + } + + return $next($request); + } +} diff --git a/app/Models/User.php b/app/Models/User.php index 51cb01a..4c73307 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -3,6 +3,7 @@ namespace App\Models; use Database\Factories\UserFactory; +use Illuminate\Contracts\Auth\MustVerifyEmail; use Illuminate\Database\Eloquent\Attributes\Fillable; use Illuminate\Database\Eloquent\Attributes\Hidden; use Illuminate\Database\Eloquent\Factories\HasFactory; @@ -14,9 +15,15 @@ use Laravel\Passport\Contracts\OAuthenticatable; use Laravel\Passport\HasApiTokens; +/** + * The MustVerifyEmail contract is what makes the verification real. Illuminate's + * base user has always carried the trait behind it, so hasVerifiedEmail() already + * answered from the column -- but nothing ever asked, because the `verified` + * middleware and Fortify's flow both test for this interface first. + */ #[Fillable(['name', 'email', 'password'])] #[Hidden(['password', 'remember_token', 'two_factor_secret', 'two_factor_recovery_codes'])] -class User extends Authenticatable implements OAuthenticatable, PasskeyUser +class User extends Authenticatable implements MustVerifyEmail, OAuthenticatable, PasskeyUser { /** @use HasFactory */ use HasApiTokens, HasFactory, Notifiable, PasskeyAuthenticatable, TwoFactorAuthenticatable; diff --git a/bootstrap/app.php b/bootstrap/app.php index 5304386..e2accb8 100644 --- a/bootstrap/app.php +++ b/bootstrap/app.php @@ -1,9 +1,11 @@ withRouting( @@ -13,7 +15,14 @@ health: '/up', ) ->withMiddleware(function (Middleware $middleware): void { - // + // Passport applies config('passport.middleware') as group middleware, + // which lands it ahead of the `web` group and so ahead of the session + // the check needs. Priority is what decides the order once the groups + // are expanded, so name it here and it runs once a user can be read. + $middleware->appendToPriorityList( + StartSession::class, + EnsureEmailIsVerifiedForOAuth::class, + ); }) ->withExceptions(function (Exceptions $exceptions): void { $exceptions->shouldRenderJsonWhen( diff --git a/config/fortify.php b/config/fortify.php index 3c76c68..cdccfe5 100644 --- a/config/fortify.php +++ b/config/fortify.php @@ -164,7 +164,7 @@ 'features' => [ Features::registration(), Features::resetPasswords(), - // Features::emailVerification(), + Features::emailVerification(), Features::updateProfileInformation(), Features::updatePasswords(), Features::twoFactorAuthentication([ diff --git a/config/passport.php b/config/passport.php index aed4358..ceedeee 100644 --- a/config/passport.php +++ b/config/passport.php @@ -1,5 +1,7 @@ 'web', - 'middleware' => [], + /* + |-------------------------------------------------------------------------- + | Passport Route Middleware + |-------------------------------------------------------------------------- + | + | Applied to every route Passport registers. The one entry here stops an + | unverified address reaching the authorization screen; it is a no-op on + | the back-channel routes, which carry no session user. + | + */ + + 'middleware' => [ + EnsureEmailIsVerifiedForOAuth::class, + ], /* |-------------------------------------------------------------------------- diff --git a/routes/web.php b/routes/web.php index c75f51a..ec27667 100644 --- a/routes/web.php +++ b/routes/web.php @@ -6,7 +6,9 @@ Route::get('/', fn () => auth()->check() ? redirect()->route('home') : redirect()->route('login')); -Route::middleware('auth')->group(function () { +// `verified` as well as `auth`: an address nobody has proved they hold should +// not be picking an app to sign in to, nor switching which account does. +Route::middleware(['auth', 'verified'])->group(function () { Route::get('/home', HomeController::class)->name('home'); Route::post('/oauth/switch-account', SwitchOAuthAccountController::class) diff --git a/tests/Feature/Auth/EmailVerificationTest.php b/tests/Feature/Auth/EmailVerificationTest.php new file mode 100644 index 0000000..342f497 --- /dev/null +++ b/tests/Feature/Auth/EmailVerificationTest.php @@ -0,0 +1,208 @@ +toBeTrue(); +}); + +test('registering leaves the address unverified and sends the link', function () { + Notification::fake(); + + $this->post('/register', [ + 'name' => 'Ada Lovelace', + 'email' => 'ada@3ag.local', + 'password' => 'a-long-enough-password', + 'password_confirmation' => 'a-long-enough-password', + ]); + + $user = User::query()->where('email', 'ada@3ag.local')->firstOrFail(); + + expect($user->email_verified_at)->toBeNull() + ->and($user->hasVerifiedEmail())->toBeFalse(); + + Notification::assertSentTo($user, VerifyEmail::class); +}); + +test('registering fires Registered, which is what sends the link', function () { + Event::fake(); + + $this->post('/register', [ + 'name' => 'Ada Lovelace', + 'email' => 'ada@3ag.local', + 'password' => 'a-long-enough-password', + 'password_confirmation' => 'a-long-enough-password', + ]); + + Event::assertDispatched(Registered::class); +}); + +test('an unverified user is held at the verification notice', function () { + $user = User::factory()->unverified()->create(); + + $this->actingAs($user)->get('/home')->assertRedirect(route('verification.notice')); +}); + +test('opening the link verifies the address', function () { + $user = User::factory()->unverified()->create(); + + $url = URL::temporarySignedRoute('verification.verify', now()->addMinutes(60), [ + 'id' => $user->getKey(), + 'hash' => sha1($user->getEmailForVerification()), + ]); + + $this->actingAs($user)->get($url)->assertRedirect(config('fortify.home').'?verified=1'); + + expect($user->fresh()->hasVerifiedEmail())->toBeTrue(); +}); + +test('changing the email address requires verifying the new one', function () { + Notification::fake(); + + $user = User::factory()->create(); + + $this->actingAs($user)->put('/user/profile-information', [ + 'name' => $user->name, + 'email' => 'moved@3ag.local', + ])->assertSessionHasNoErrors(); + + $user->refresh(); + + expect($user->email)->toBe('moved@3ag.local') + ->and($user->hasVerifiedEmail())->toBeFalse(); + + Notification::assertSentTo($user, VerifyEmail::class); +}); + +test('leaving the email address alone keeps it verified', function () { + Notification::fake(); + + $user = User::factory()->create(); + + $this->actingAs($user)->put('/user/profile-information', [ + 'name' => 'A New Name', + 'email' => $user->email, + ])->assertSessionHasNoErrors(); + + expect($user->fresh()->hasVerifiedEmail())->toBeTrue(); + + Notification::assertNothingSent(); +}); + +test('a tampered link does not verify the address', function () { + $user = User::factory()->unverified()->create(); + + $url = URL::temporarySignedRoute('verification.verify', now()->addMinutes(60), [ + 'id' => $user->getKey(), + 'hash' => sha1('someone-elses@3ag.local'), + ]); + + $this->actingAs($user)->get($url)->assertForbidden(); + + expect($user->fresh()->hasVerifiedEmail())->toBeFalse(); +}); + +/* +|-------------------------------------------------------------------------- +| The OAuth gate +|-------------------------------------------------------------------------- +| +| Registration signs the new user in before they have opened the link, so the +| authorization screen is the door that actually has to be shut. +| +| Passport applies config('passport.middleware') as group middleware, which +| puts the gate ahead of the `web` group -- and so ahead of the session it +| reads. bootstrap/app.php moves it back with appendToPriorityList(), and +| without that the gate resolves a null user and waves everyone through. +| +| The two request tests below do NOT catch that on their own: Laravel's test +| client keeps one session store alive across the requests in a test, so by +| the time the gate runs the store is already started and the user resolves +| either way. Verified against a real server instead -- unverified user, cold +| request: 302 to /email/verify with the priority entry, 200 without it. The +| ordering test is the regression guard that works in this suite. +| +*/ + +test('the gate is ordered after the session it reads', function () { + // The priority list is assembled when the HTTP kernel bootstraps, so it + // is empty until something has actually been routed. + $this->get('/login'); + + $priority = app('router')->middlewarePriority; + + $session = array_search(StartSession::class, $priority, true); + $gate = array_search(EnsureEmailIsVerifiedForOAuth::class, $priority, true); + + expect($session)->not->toBeFalse('StartSession is missing from the priority list') + ->and($gate)->not->toBeFalse('the OAuth gate is missing from the priority list -- Passport would run it before the session and it would pass everyone through') + ->and($gate)->toBeGreaterThan($session); +}); + +/** + * Sign in the way a browser does, so the user is read back out of the session. + * + * actingAs() would not do: it puts the user straight on the guard, so the + * gate finds one whether or not the session has started by the time it runs + * -- which is the exact mistake these tests exist to catch. + */ +function signInThroughTheLoginForm(User $user): void +{ + test()->post('/login', ['email' => $user->email, 'password' => 'password']); + + test()->assertAuthenticatedAs($user); +} + +function suiteClientRedirectUri(): string +{ + return 'https://productsyncmanager.test/auth/accounts/callback'; +} + +function suiteClient(): Client +{ + return app(ClientRepository::class)->createAuthorizationCodeGrantClient( + name: 'ProductSync', + redirectUris: [suiteClientRedirectUri()], + ); +} + +test('an unverified user cannot reach the authorization screen', function () { + $client = suiteClient(); + + signInThroughTheLoginForm(User::factory()->unverified()->create()); + + $this->get(authorizeUrl($client->getKey(), suiteClientRedirectUri())) + ->assertRedirect(route('verification.notice')); +}); + +test('a verified user still reaches the authorization screen', function () { + $client = suiteClient(); + + signInThroughTheLoginForm(User::factory()->create()); + + $this->get(authorizeUrl($client->getKey(), suiteClientRedirectUri())) + ->assertOk(); +}); + +test('the back channel is untouched by the gate', function () { + // No session, so no user to read: the token endpoint has to answer on its + // own terms rather than being redirected to a login page. + $this->post('/oauth/token', [ + 'grant_type' => 'authorization_code', + 'client_id' => 'nonsense', + 'code' => 'nonsense', + ])->assertJson(fn ($json) => $json->etc()); +});