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()); +});