Improve OAuth sign-in handling (#11907)

app/Http/Controllers/OauthController.php

@@ -24,12 +24,8 @@ class OauthController extends Controller
             $oauthUser = get_socialite_provider($oauthSetting->provider)->user();
             $user = $oauthLoginService->login($oauthSetting->provider, $oauthUser, $oauthSetting);
 
-            $team = $user->resolveStoredTeam();
-            if (! $team && $user->teams()->count() === 0) {
-                $team = $user->recreate_personal_team();
-            }
-            if ($team) {
-                session(['currentTeam' => $user->currentTeam = $team]);
+            if ($oauthLoginService->requiresTwoFactorChallenge($user)) {
+                return redirect()->route('two-factor.login');
             }
 
             return redirect('/');

app/Services/Auth/OauthLoginService.php

@@ -12,6 +12,7 @@ use Illuminate\Support\Facades\Auth;
 use Illuminate\Support\Facades\DB;
 use Illuminate\Support\Facades\Hash;
 use Illuminate\Support\Str;
+use Laravel\Fortify\Events\TwoFactorAuthenticationChallenged;
 use Symfony\Component\HttpKernel\Exception\HttpException;
 
 class OauthLoginService
@@ -27,9 +28,21 @@ class OauthLoginService
             ? $this->resolveOidcUser($oauthUser, $oauthSetting, $email)
             : $this->resolveOauthUser($oauthUser, $oauthSetting, $email);
 
-        Auth::login($user);
         $team = $user->currentTeam() ?? $user->teams()->first() ?? $user->recreate_personal_team();
         session(['currentTeam' => $user->currentTeam = $team]);
+
+        if ($this->requiresTwoFactorChallenge($user)) {
+            Auth::logout();
+            session()->put([
+                'login.id' => $user->getKey(),
+                'login.remember' => false,
+            ]);
+            TwoFactorAuthenticationChallenged::dispatch($user);
+
+            return $user;
+        }
+
+        Auth::login($user);
         auditLog('auth.user.oauth_login_succeeded', [
             'team_id' => $team?->id,
             'resource' => 'user',
@@ -43,6 +56,11 @@ class OauthLoginService
         return $user;
     }
 
+    public function requiresTwoFactorChallenge(User $user): bool
+    {
+        return $user->hasEnabledTwoFactorAuthentication();
+    }
+
     private function resolveOauthUser(object $oauthUser, OauthSetting $oauthSetting, string $email): User
     {
         $provider = $oauthSetting->provider;
@@ -76,7 +94,15 @@ class OauthLoginService
                     return $identity->user;
                 }
 
+                if (! $this->hasVerifiedEmail($provider, $rawClaims)) {
+                    throw new HttpException(403, 'OAuth provider did not verify the email address');
+                }
+
                 $user = User::whereEmail($email)->first();
+                if ($user?->oauthIdentities()->exists()) {
+                    throw new HttpException(403, 'OAuth identity cannot be linked to this account');
+                }
+
                 if (! $user) {
                     if (! $this->canCreateUser($oauthSetting)) {
                         throw new HttpException(403, 'Registration is disabled');
@@ -102,6 +128,23 @@ class OauthLoginService
         }
     }
 
+    /**
+     * GitHub and Bitbucket select only verified primary email addresses in
+     * their Socialite providers. Other providers must return an explicit
+     * boolean verification claim in the raw provider response.
+     *
+     * @param  array<string, mixed>  $rawClaims
+     */
+    private function hasVerifiedEmail(string $provider, array $rawClaims): bool
+    {
+        return match ($provider) {
+            'github', 'bitbucket' => true,
+            'discord' => data_get($rawClaims, 'verified') === true,
+            'google' => data_get($rawClaims, 'verified_email') === true,
+            default => data_get($rawClaims, 'email_verified') === true,
+        };
+    }
+
     private function resolveOidcUser(object $oauthUser, OauthSetting $oauthSetting, string $email): User
     {
         $issuer = $oauthUser instanceof OidcUser && filled($oauthUser->issuer)
@@ -153,6 +196,10 @@ class OauthLoginService
                     throw new HttpException(403, 'OIDC provider must verify the email address before linking to an existing account');
                 }
 
+                if ($user?->oauthIdentities()->exists()) {
+                    throw new HttpException(403, 'OAuth identity cannot be linked to this account');
+                }
+
                 if (! $user) {
                     if (! $this->canCreateUser($oauthSetting)) {
                         throw new HttpException(403, 'Registration is disabled');

tests/Feature/OauthControllerTest.php

@@ -10,7 +10,9 @@ use Illuminate\Foundation\Testing\RefreshDatabase;
 use Illuminate\Support\Facades\DB;
 use Illuminate\Support\Facades\Event;
 use Illuminate\Support\Once;
+use Laravel\Fortify\Features;
 use Laravel\Socialite\Facades\Socialite;
+use SocialiteProviders\Discord\Provider as DiscordProvider;
 use Symfony\Component\HttpKernel\Exception\HttpException;
 
 uses(RefreshDatabase::class);
@@ -47,6 +49,7 @@ it('logs in an existing user when the oauth provider returns a mixed-case email'
         'email' => 'UserName@example.edu',
         'name' => 'Example User',
         'id' => 'google-user-id',
+        'user' => ['verified_email' => true],
     ]);
 
     Socialite::shouldReceive('driver')->once()->with('google')->andReturn($provider);
@@ -83,6 +86,7 @@ it('never moves an existing oauth identity when the provider email changes', fun
         'email' => 'new@example.com',
         'name' => 'Example User',
         'id' => 'google-user-id',
+        'user' => ['verified_email' => true],
     ]);
 
     Socialite::shouldReceive('driver')->once()->with('google')->andReturn($provider);
@@ -95,6 +99,39 @@ it('never moves an existing oauth identity when the provider email changes', fun
         ->and($identity->user_id)->not->toBe($otherUser->id);
 });
 
+it('keeps an existing oauth identity working without new email verification evidence', function () {
+    $user = User::factory()->create(['email' => 'existing@example.com']);
+    OauthIdentity::create([
+        'user_id' => $user->id,
+        'provider' => 'discord',
+        'issuer' => 'discord',
+        'provider_user_id' => 'existing-discord-id',
+        'email' => $user->email,
+    ]);
+    $discordSetting = OauthSetting::create([
+        'provider' => 'discord',
+        'client_id' => 'discord-client-id',
+        'client_secret' => 'discord-client-secret',
+        'enabled' => true,
+    ]);
+
+    $resolvedUser = app(OauthLoginService::class)->login('discord', (object) [
+        'email' => 'changed@example.com',
+        'name' => 'Existing Discord User',
+        'id' => 'existing-discord-id',
+        'user' => ['verified' => false],
+    ], $discordSetting);
+
+    expect($resolvedUser->is($user))->toBeTrue();
+    $this->assertAuthenticatedAs($user);
+    $this->assertDatabaseHas('oauth_identities', [
+        'user_id' => $user->id,
+        'provider' => 'discord',
+        'provider_user_id' => 'existing-discord-id',
+        'email' => 'changed@example.com',
+    ]);
+});
+
 it('continues oauth login when another request creates the identity first', function () {
     $user = User::factory()->create(['email' => 'race@example.com']);
     $eventName = 'eloquent.creating: '.OauthIdentity::class;
@@ -117,6 +154,7 @@ it('continues oauth login when another request creates the identity first', func
             'email' => 'race@example.com',
             'name' => 'Race User',
             'id' => 'google-race-id',
+            'user' => ['verified_email' => true],
         ], OauthSetting::where('provider', 'google')->firstOrFail());
     } finally {
         Event::forget($eventName);
@@ -127,6 +165,160 @@ it('continues oauth login when another request creates the identity first', func
     $this->assertAuthenticatedAs($user);
 });
 
+it('requires strict Discord email verification values', function (mixed $verified) {
+    $existingUser = User::factory()->create(['email' => 'existing@example.com']);
+    $discordSetting = OauthSetting::create([
+        'provider' => 'discord',
+        'client_id' => 'discord-client-id',
+        'client_secret' => 'discord-client-secret',
+        'enabled' => true,
+    ]);
+
+    expect(fn () => app(OauthLoginService::class)->login('discord', (object) [
+        'email' => 'existing@example.com',
+        'name' => 'Discord User',
+        'id' => 'discord-user-id',
+        'user' => ['verified' => $verified],
+    ], $discordSetting))->toThrow(HttpException::class);
+
+    $this->assertGuest();
+    expect(OauthIdentity::where('user_id', $existingUser->id)->exists())->toBeFalse();
+})->with([
+    'unverified' => false,
+    'string true' => 'true',
+    'integer true' => 1,
+]);
+
+it('keeps the Discord verified claim in the raw Socialite user payload', function () {
+    $provider = (new ReflectionClass(DiscordProvider::class))->newInstanceWithoutConstructor();
+    $mapUser = new ReflectionMethod(DiscordProvider::class, 'mapUserToObject');
+
+    $oauthUser = $mapUser->invoke($provider, [
+        'id' => 'discord-user-id',
+        'username' => 'Discord User',
+        'discriminator' => '0',
+        'email' => 'user@example.com',
+        'verified' => false,
+        'avatar' => null,
+    ]);
+
+    expect($oauthUser->user['verified'])->toBeFalse();
+});
+
+it('registers a new user from a verified provider identity', function () {
+    InstanceSettings::findOrFail(0)->update(['is_registration_enabled' => true]);
+
+    $user = app(OauthLoginService::class)->login('google', (object) [
+        'email' => 'verified@example.com',
+        'name' => 'Verified User',
+        'id' => 'verified-google-id',
+        'user' => ['verified_email' => true],
+    ], OauthSetting::where('provider', 'google')->firstOrFail());
+
+    expect($user->email)->toBe('verified@example.com');
+    $this->assertAuthenticatedAs($user);
+    $this->assertDatabaseHas('oauth_identities', [
+        'user_id' => $user->id,
+        'provider' => 'google',
+        'provider_user_id' => 'verified-google-id',
+    ]);
+});
+
+it('does not link another provider identity to an account by shared email', function () {
+    $user = User::factory()->create(['email' => 'shared@example.com']);
+    OauthIdentity::create([
+        'user_id' => $user->id,
+        'provider' => 'github',
+        'issuer' => 'github',
+        'provider_user_id' => 'github-user-id',
+        'email' => 'shared@example.com',
+    ]);
+
+    expect(fn () => app(OauthLoginService::class)->login('google', (object) [
+        'email' => 'shared@example.com',
+        'name' => 'Other Provider User',
+        'id' => 'google-user-id',
+        'user' => ['verified_email' => true],
+    ], OauthSetting::where('provider', 'google')->firstOrFail()))->toThrow(HttpException::class);
+
+    $this->assertGuest();
+    expect(OauthIdentity::count())->toBe(1);
+});
+
+it('sends an OAuth user with confirmed two factor authentication to the Fortify challenge', function () {
+    config()->set('fortify.features', [Features::twoFactorAuthentication(['confirm' => true])]);
+    $user = User::factory()->create([
+        'email' => 'two-factor@example.com',
+        'two_factor_secret' => encrypt('secret'),
+        'two_factor_confirmed_at' => now(),
+    ]);
+    OauthIdentity::create([
+        'user_id' => $user->id,
+        'provider' => 'google',
+        'issuer' => 'google',
+        'provider_user_id' => 'two-factor-google-id',
+        'email' => $user->email,
+    ]);
+
+    $provider = Mockery::mock();
+    $provider->shouldReceive('setConfig')->once()->andReturnSelf();
+    $provider->shouldReceive('with')->once()->andReturnSelf();
+    $provider->shouldReceive('user')->once()->andReturn((object) [
+        'email' => $user->email,
+        'name' => $user->name,
+        'id' => 'two-factor-google-id',
+        'user' => ['verified_email' => true],
+    ]);
+    Socialite::shouldReceive('driver')->once()->with('google')->andReturn($provider);
+
+    $this->get(route('auth.callback', 'google'))
+        ->assertRedirect(route('two-factor.login'))
+        ->assertSessionHas('login.id', $user->id);
+
+    $this->assertGuest();
+});
+
+it('completes OAuth login without a challenge when two factor authentication is not enabled for the user', function () {
+    $user = User::factory()->create(['email' => 'without-two-factor@example.com']);
+    OauthIdentity::create([
+        'user_id' => $user->id,
+        'provider' => 'google',
+        'issuer' => 'google',
+        'provider_user_id' => 'plain-google-id',
+        'email' => $user->email,
+    ]);
+
+    $provider = Mockery::mock();
+    $provider->shouldReceive('setConfig')->once()->andReturnSelf();
+    $provider->shouldReceive('with')->once()->andReturnSelf();
+    $provider->shouldReceive('user')->once()->andReturn((object) [
+        'email' => $user->email,
+        'name' => $user->name,
+        'id' => 'plain-google-id',
+        'user' => ['verified_email' => true],
+    ]);
+    Socialite::shouldReceive('driver')->once()->with('google')->andReturn($provider);
+
+    $this->get(route('auth.callback', 'google'))->assertRedirect('/');
+
+    $this->assertAuthenticatedAs($user);
+});
+
+it('rejects redirect requests for a disabled provider', function () {
+    $this->withoutVite();
+    OauthSetting::where('provider', 'google')->update(['enabled' => false]);
+
+    $this->get(route('auth.redirect', 'google'))->assertForbidden();
+    $this->assertGuest();
+});
+
+it('rejects callback requests for a disabled provider', function () {
+    OauthSetting::where('provider', 'google')->update(['enabled' => false]);
+
+    $this->from('/login')->get(route('auth.callback', 'google'))->assertRedirect('/login');
+    $this->assertGuest();
+});
+
 it('rejects oauth logins when the provider does not return an email address', function (?string $providerEmail) {
     config()->set('app.maintenance.driver', 'file');
     InstanceSettings::firstOrCreate([

tests/Feature/OidcOauthControllerTest.php

@@ -184,6 +184,24 @@ it('rejects linking an unverified oidc email to an existing local account', func
     ]);
 });
 
+it('rejects linking a new oidc identity to an account with another provider identity', function () {
+    $user = User::factory()->create(['email' => 'shared@example.com']);
+    OauthIdentity::create([
+        'user_id' => $user->id,
+        'provider' => 'github',
+        'issuer' => 'github',
+        'provider_user_id' => 'github-user-id',
+        'email' => $user->email,
+    ]);
+
+    fakeOidcProvider(['email' => $user->email, 'email_verified' => true]);
+
+    $this->from('/login')->get(route('auth.callback', 'oidc'))->assertRedirect('/login');
+
+    $this->assertGuest();
+    expect(OauthIdentity::count())->toBe(1);
+});
+
 it('rejects new oidc users when neither normal nor provider registration is enabled', function () {
     fakeOidcProvider(['email' => 'blocked@example.com']);