diff --git a/app/Http/Controllers/OauthController.php b/app/Http/Controllers/OauthController.php index 66ddcdd1ee..c837cc49c8 100644 --- a/app/Http/Controllers/OauthController.php +++ b/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('/'); diff --git a/app/Services/Auth/OauthLoginService.php b/app/Services/Auth/OauthLoginService.php index 15d9ddc2fd..32524c7fa6 100644 --- a/app/Services/Auth/OauthLoginService.php +++ b/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 $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'); diff --git a/tests/Feature/OauthControllerTest.php b/tests/Feature/OauthControllerTest.php index 4671183ae7..dee651cc2f 100644 --- a/tests/Feature/OauthControllerTest.php +++ b/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([ diff --git a/tests/Feature/OidcOauthControllerTest.php b/tests/Feature/OidcOauthControllerTest.php index 084347f66d..3812d78b18 100644 --- a/tests/Feature/OidcOauthControllerTest.php +++ b/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']);