From f9f26c547c8d8eef015a32f30ef904e334d303df Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Tue, 18 Aug 2026 15:27:27 +0200 Subject: [PATCH 1/3] fix(auth): preserve OAuth identity across email changes Link OAuth logins by provider user ID before matching email, refresh identity claims on login, and skip password confirmation for SSO-linked users. --- app/Models/User.php | 6 ++- app/Services/Auth/OauthLoginService.php | 50 +++++++++++++++---- bootstrap/helpers/shared.php | 9 ++-- tests/Feature/OauthControllerTest.php | 38 ++++++++++++++ tests/v4/Feature/DangerDeleteResourceTest.php | 18 ++++++- 5 files changed, 106 insertions(+), 15 deletions(-) diff --git a/app/Models/User.php b/app/Models/User.php index e8f59835b..10303422b 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -520,10 +520,14 @@ public function hasSsoIdentity(): bool /** * Check if the user has a password set. - * OAuth users are created without passwords. */ public function hasPassword(): bool { return ! empty($this->password); } + + public function requiresPasswordConfirmation(): bool + { + return $this->hasPassword() && ! $this->hasSsoIdentity(); + } } diff --git a/app/Services/Auth/OauthLoginService.php b/app/Services/Auth/OauthLoginService.php index f656fad08..6b7acbc5b 100644 --- a/app/Services/Auth/OauthLoginService.php +++ b/app/Services/Auth/OauthLoginService.php @@ -35,16 +35,48 @@ public function login(string $provider, object $oauthUser, OauthSetting $oauthSe private function resolveOauthUser(object $oauthUser, OauthSetting $oauthSetting, string $email): User { - $user = User::whereEmail($email)->first(); - if ($user) { + $provider = $oauthSetting->provider; + $providerUserId = (string) $oauthUser->id; + $rawClaims = is_array($oauthUser->user ?? null) ? $oauthUser->user : []; + + return DB::transaction(function () use ($oauthUser, $oauthSetting, $email, $provider, $providerUserId, $rawClaims) { + $identity = OauthIdentity::where([ + 'provider' => $provider, + 'issuer' => $provider, + 'provider_user_id' => $providerUserId, + ])->first(); + + if ($identity) { + $identity->update([ + 'email' => $email, + 'raw_claims' => $rawClaims, + 'last_login_at' => now(), + ]); + + return $identity->user; + } + + $user = User::whereEmail($email)->first(); + if (! $user) { + if (! $this->canCreateUser($oauthSetting)) { + throw new HttpException(403, 'Registration is disabled'); + } + + $user = $this->createUser($oauthUser->name ?: $email, $email, $oauthSetting); + } + + OauthIdentity::create([ + 'user_id' => $user->id, + 'provider' => $provider, + 'issuer' => $provider, + 'provider_user_id' => $providerUserId, + 'email' => $email, + 'raw_claims' => $rawClaims, + 'last_login_at' => now(), + ]); + return $user; - } - - if (! $this->canCreateUser($oauthSetting)) { - throw new HttpException(403, 'Registration is disabled'); - } - - return $this->createUser($oauthUser->name ?: $email, $email, $oauthSetting); + }); } private function resolveOidcUser(object $oauthUser, OauthSetting $oauthSetting, string $email): User diff --git a/bootstrap/helpers/shared.php b/bootstrap/helpers/shared.php index cf63c4776..d3cc6d826 100644 --- a/bootstrap/helpers/shared.php +++ b/bootstrap/helpers/shared.php @@ -4544,7 +4544,7 @@ function formatContainerStatus(string $status): string * Check if password confirmation should be skipped. * Returns true if: * - Two-step confirmation is globally disabled - * - User has no password (OAuth users) + * - User has no usable local password confirmation (including SSO users) * * Used by modal-confirmation.blade.php to determine if password step should be shown. * @@ -4557,8 +4557,9 @@ function shouldSkipPasswordConfirmation(): bool return true; } - // Skip if user has no password (OAuth users) - if (! Auth::user()?->hasPassword()) { + // OAuth users may have an unusable generated password, so the linked + // identity is the source of truth for whether confirmation is possible. + if (! Auth::user()?->requiresPasswordConfirmation()) { return true; } @@ -4569,7 +4570,7 @@ function shouldSkipPasswordConfirmation(): bool * Verify password for two-step confirmation. * Skips verification if: * - Two-step confirmation is globally disabled - * - User has no password (OAuth users) + * - User has no usable local password confirmation (including SSO users) * * @param mixed $password The password to verify (may be array if skipped by frontend) * @param Component|null $component Optional Livewire component to add errors to diff --git a/tests/Feature/OauthControllerTest.php b/tests/Feature/OauthControllerTest.php index 27660a6bb..62940b0ef 100644 --- a/tests/Feature/OauthControllerTest.php +++ b/tests/Feature/OauthControllerTest.php @@ -1,6 +1,7 @@ assertRedirect('/'); $this->assertAuthenticatedAs($user); expect(User::count())->toBe(1); + expect(OauthIdentity::where([ + 'user_id' => $user->id, + 'provider' => 'google', + 'provider_user_id' => 'google-user-id', + ])->exists())->toBeTrue(); +}); + +it('never moves an existing oauth identity when the provider email changes', function () { + config()->set('app.maintenance.driver', 'file'); + + $identityOwner = User::factory()->create(['email' => 'old@example.com']); + $otherUser = User::factory()->create(['email' => 'new@example.com']); + $identity = OauthIdentity::create([ + 'user_id' => $identityOwner->id, + 'provider' => 'google', + 'issuer' => 'google', + 'provider_user_id' => 'google-user-id', + 'email' => 'old@example.com', + ]); + + $provider = Mockery::mock(); + $provider->shouldReceive('setConfig')->once()->andReturnSelf(); + $provider->shouldReceive('with')->once()->with(['hd' => 'example.com'])->andReturnSelf(); + $provider->shouldReceive('user')->once()->andReturn((object) [ + 'email' => 'new@example.com', + 'name' => 'Example User', + 'id' => 'google-user-id', + ]); + + Socialite::shouldReceive('driver')->once()->with('google')->andReturn($provider); + + $this->get(route('auth.callback', 'google'))->assertRedirect('/'); + + $this->assertAuthenticatedAs($identityOwner); + expect($identity->refresh()->user_id)->toBe($identityOwner->id) + ->and($identity->email)->toBe('new@example.com') + ->and($identity->user_id)->not->toBe($otherUser->id); }); it('rejects oauth logins when the provider does not return an email address', function (?string $providerEmail) { diff --git a/tests/v4/Feature/DangerDeleteResourceTest.php b/tests/v4/Feature/DangerDeleteResourceTest.php index 7a73f5979..4a275ad48 100644 --- a/tests/v4/Feature/DangerDeleteResourceTest.php +++ b/tests/v4/Feature/DangerDeleteResourceTest.php @@ -4,6 +4,7 @@ use App\Models\Application; use App\Models\Environment; use App\Models\InstanceSettings; +use App\Models\OauthIdentity; use App\Models\Project; use App\Models\Server; use App\Models\StandaloneDocker; @@ -18,7 +19,7 @@ uses(RefreshDatabase::class); beforeEach(function () { - InstanceSettings::create(['id' => 0]); + InstanceSettings::forceCreate(['id' => 0]); Queue::fake(); $this->user = User::factory()->create([ @@ -70,6 +71,21 @@ expect(Application::find($this->application->id))->toBeNull(); }); +test('delete succeeds without password for an oauth user', function () { + OauthIdentity::create([ + 'user_id' => $this->user->id, + 'provider' => 'oidc', + 'issuer' => 'https://idp.example.com', + 'provider_user_id' => 'oauth-user-id', + ]); + + Livewire::test(Danger::class, ['resource' => $this->application]) + ->call('delete', '') + ->assertHasNoErrors(); + + expect(Application::find($this->application->id))->toBeNull(); +}); + test('delete applies selectedActions from checkbox state', function () { $component = Livewire::test(Danger::class, ['resource' => $this->application]) ->call('delete', 'test-password', ['delete_configurations', 'docker_cleanup']); From d29139f4a317f44d4d0b5cfb5dc941c2989c1d3d Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Tue, 18 Aug 2026 15:34:37 +0200 Subject: [PATCH 2/3] fix(auth): reject OAuth logins without valid provider IDs Add validation for missing, blank, and non-scalar provider user IDs and cover the rejection cases with feature tests. --- app/Services/Auth/OauthLoginService.php | 6 ++++- tests/Feature/OauthControllerTest.php | 30 +++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/app/Services/Auth/OauthLoginService.php b/app/Services/Auth/OauthLoginService.php index 6b7acbc5b..c093e290f 100644 --- a/app/Services/Auth/OauthLoginService.php +++ b/app/Services/Auth/OauthLoginService.php @@ -36,7 +36,11 @@ public function login(string $provider, object $oauthUser, OauthSetting $oauthSe private function resolveOauthUser(object $oauthUser, OauthSetting $oauthSetting, string $email): User { $provider = $oauthSetting->provider; - $providerUserId = (string) $oauthUser->id; + $providerUserId = $oauthUser->id ?? null; + if (! is_scalar($providerUserId) || trim((string) $providerUserId) === '') { + throw new HttpException(403, 'OAuth provider did not return a valid user ID'); + } + $providerUserId = (string) $providerUserId; $rawClaims = is_array($oauthUser->user ?? null) ? $oauthUser->user : []; return DB::transaction(function () use ($oauthUser, $oauthSetting, $email, $provider, $providerUserId, $rawClaims) { diff --git a/tests/Feature/OauthControllerTest.php b/tests/Feature/OauthControllerTest.php index 62940b0ef..f0225e335 100644 --- a/tests/Feature/OauthControllerTest.php +++ b/tests/Feature/OauthControllerTest.php @@ -4,9 +4,11 @@ use App\Models\OauthIdentity; use App\Models\OauthSetting; use App\Models\User; +use App\Services\Auth\OauthLoginService; use Illuminate\Foundation\Testing\RefreshDatabase; use Illuminate\Support\Once; use Laravel\Socialite\Facades\Socialite; +use Symfony\Component\HttpKernel\Exception\HttpException; uses(RefreshDatabase::class); @@ -121,3 +123,31 @@ 'malformed email' => ['not-an-email'], 'missing domain' => ['user@'], ]); + +it('rejects oauth logins when the provider does not return a valid user id', function (mixed $invalidId) { + $oauthUser = (object) [ + 'email' => 'user@example.edu', + 'name' => 'Example User', + ]; + + if ($invalidId !== 'missing') { + $oauthUser->id = $invalidId; + } + + try { + app(OauthLoginService::class)->login('google', $oauthUser, OauthSetting::where('provider', 'google')->firstOrFail()); + } catch (HttpException $exception) { + expect($exception->getStatusCode())->toBe(403) + ->and(OauthIdentity::count())->toBe(0) + ->and(User::count())->toBe(0); + + return; + } + + $this->fail('Expected an invalid OAuth provider user ID to be rejected.'); +})->with([ + 'null id' => [null], + 'missing id' => ['missing'], + 'blank id' => [' '], + 'non-scalar id' => [[]], +]); From 3b08be1429a4a6804086095492e106cc78534c92 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Tue, 18 Aug 2026 15:43:46 +0200 Subject: [PATCH 3/3] fix(auth): handle concurrent OAuth identity creation safely Recover existing OAuth and OIDC identities after unique constraint races and reject boolean or floating-point provider IDs. --- app/Services/Auth/OauthLoginService.php | 164 ++++++++++++---------- tests/Feature/OauthControllerTest.php | 38 +++++ tests/Feature/OidcOauthControllerTest.php | 42 ++++++ 3 files changed, 170 insertions(+), 74 deletions(-) diff --git a/app/Services/Auth/OauthLoginService.php b/app/Services/Auth/OauthLoginService.php index c093e290f..2ec8f88e3 100644 --- a/app/Services/Auth/OauthLoginService.php +++ b/app/Services/Auth/OauthLoginService.php @@ -7,6 +7,7 @@ use App\Models\OauthSetting; use App\Models\Team; use App\Models\User; +use Illuminate\Database\UniqueConstraintViolationException; use Illuminate\Support\Facades\Auth; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Hash; @@ -37,50 +38,59 @@ private function resolveOauthUser(object $oauthUser, OauthSetting $oauthSetting, { $provider = $oauthSetting->provider; $providerUserId = $oauthUser->id ?? null; - if (! is_scalar($providerUserId) || trim((string) $providerUserId) === '') { + if ( + (! is_string($providerUserId) && ! is_int($providerUserId)) + || (is_string($providerUserId) && trim($providerUserId) === '') + ) { throw new HttpException(403, 'OAuth provider did not return a valid user ID'); } $providerUserId = (string) $providerUserId; $rawClaims = is_array($oauthUser->user ?? null) ? $oauthUser->user : []; - return DB::transaction(function () use ($oauthUser, $oauthSetting, $email, $provider, $providerUserId, $rawClaims) { - $identity = OauthIdentity::where([ - 'provider' => $provider, - 'issuer' => $provider, - 'provider_user_id' => $providerUserId, - ])->first(); + $identityKey = [ + 'provider' => $provider, + 'issuer' => $provider, + 'provider_user_id' => $providerUserId, + ]; - if ($identity) { - $identity->update([ + try { + return DB::transaction(function () use ($oauthUser, $oauthSetting, $email, $provider, $providerUserId, $rawClaims, $identityKey): User { + $identity = OauthIdentity::where($identityKey)->first(); + + if ($identity) { + $identity->update([ + 'email' => $email, + 'raw_claims' => $rawClaims, + 'last_login_at' => now(), + ]); + + return $identity->user; + } + + $user = User::whereEmail($email)->first(); + if (! $user) { + if (! $this->canCreateUser($oauthSetting)) { + throw new HttpException(403, 'Registration is disabled'); + } + + $user = $this->createUser($oauthUser->name ?: $email, $email, $oauthSetting); + } + + OauthIdentity::create([ + 'user_id' => $user->id, + 'provider' => $provider, + 'issuer' => $provider, + 'provider_user_id' => $providerUserId, 'email' => $email, 'raw_claims' => $rawClaims, 'last_login_at' => now(), ]); - return $identity->user; - } - - $user = User::whereEmail($email)->first(); - if (! $user) { - if (! $this->canCreateUser($oauthSetting)) { - throw new HttpException(403, 'Registration is disabled'); - } - - $user = $this->createUser($oauthUser->name ?: $email, $email, $oauthSetting); - } - - OauthIdentity::create([ - 'user_id' => $user->id, - 'provider' => $provider, - 'issuer' => $provider, - 'provider_user_id' => $providerUserId, - 'email' => $email, - 'raw_claims' => $rawClaims, - 'last_login_at' => now(), - ]); - - return $user; - }); + return $user; + }); + } catch (UniqueConstraintViolationException $exception) { + return OauthIdentity::where($identityKey)->first()?->user ?? throw $exception; + } } private function resolveOidcUser(object $oauthUser, OauthSetting $oauthSetting, string $email): User @@ -104,53 +114,59 @@ private function resolveOidcUser(object $oauthUser, OauthSetting $oauthSetting, $rawClaims = is_array($oauthUser->user ?? null) ? $oauthUser->user : []; - return DB::transaction(function () use ($oauthUser, $oauthSetting, $email, $issuer, $subject, $emailVerified, $rawClaims) { - $identity = OauthIdentity::where([ - 'provider' => 'oidc', - 'issuer' => $issuer, - 'provider_user_id' => $subject, - ])->first(); + $identityKey = [ + 'provider' => 'oidc', + 'issuer' => $issuer, + 'provider_user_id' => $subject, + ]; - if ($identity) { - $identity->update([ + try { + return DB::transaction(function () use ($oauthUser, $oauthSetting, $email, $issuer, $subject, $emailVerified, $rawClaims, $identityKey): User { + $identity = OauthIdentity::where($identityKey)->first(); + + if ($identity) { + $identity->update([ + 'email' => $email, + 'raw_claims' => $rawClaims, + 'last_login_at' => now(), + ]); + + return $identity->user; + } + + $user = User::whereEmail($email)->first(); + + // Linking a new OIDC identity to an existing local account by email + // is account takeover unless the provider attests the email. This + // guard is independent of the require_email_verified toggle, which + // only governs the broader login flow. + if ($user && ! $emailVerified) { + throw new HttpException(403, 'OIDC provider must verify the email address before linking to an existing account'); + } + + if (! $user) { + if (! $this->canCreateUser($oauthSetting)) { + throw new HttpException(403, 'Registration is disabled'); + } + + $user = $this->createUser($oauthUser->name ?: $email, $email, $oauthSetting); + } + + OauthIdentity::create([ + 'user_id' => $user->id, + 'provider' => 'oidc', + 'issuer' => $issuer, + 'provider_user_id' => $subject, 'email' => $email, 'raw_claims' => $rawClaims, 'last_login_at' => now(), ]); - return $identity->user; - } - - $user = User::whereEmail($email)->first(); - - // Linking a new OIDC identity to an existing local account by email - // is account takeover unless the provider attests the email. This - // guard is independent of the require_email_verified toggle, which - // only governs the broader login flow. - if ($user && ! $emailVerified) { - throw new HttpException(403, 'OIDC provider must verify the email address before linking to an existing account'); - } - - if (! $user) { - if (! $this->canCreateUser($oauthSetting)) { - throw new HttpException(403, 'Registration is disabled'); - } - - $user = $this->createUser($oauthUser->name ?: $email, $email, $oauthSetting); - } - - OauthIdentity::create([ - 'user_id' => $user->id, - 'provider' => 'oidc', - 'issuer' => $issuer, - 'provider_user_id' => $subject, - 'email' => $email, - 'raw_claims' => $rawClaims, - 'last_login_at' => now(), - ]); - - return $user; - }); + return $user; + }); + } catch (UniqueConstraintViolationException $exception) { + return OauthIdentity::where($identityKey)->first()?->user ?? throw $exception; + } } private function canCreateUser(OauthSetting $oauthSetting): bool diff --git a/tests/Feature/OauthControllerTest.php b/tests/Feature/OauthControllerTest.php index f0225e335..4671183ae 100644 --- a/tests/Feature/OauthControllerTest.php +++ b/tests/Feature/OauthControllerTest.php @@ -5,7 +5,10 @@ use App\Models\OauthSetting; use App\Models\User; use App\Services\Auth\OauthLoginService; +use Illuminate\Database\UniqueConstraintViolationException; use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Support\Facades\DB; +use Illuminate\Support\Facades\Event; use Illuminate\Support\Once; use Laravel\Socialite\Facades\Socialite; use Symfony\Component\HttpKernel\Exception\HttpException; @@ -92,6 +95,38 @@ ->and($identity->user_id)->not->toBe($otherUser->id); }); +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; + + Event::listen($eventName, function (OauthIdentity $identity): void { + $attributes = $identity->getAttributes(); + + DB::afterRollBack(fn () => DB::table('oauth_identities')->insert($attributes)); + + throw new UniqueConstraintViolationException( + DB::getDefaultConnection(), + 'insert into oauth_identities', + [], + new PDOException('duplicate identity'), + ); + }); + + try { + $resolvedUser = app(OauthLoginService::class)->login('google', (object) [ + 'email' => 'race@example.com', + 'name' => 'Race User', + 'id' => 'google-race-id', + ], OauthSetting::where('provider', 'google')->firstOrFail()); + } finally { + Event::forget($eventName); + } + + expect($resolvedUser->is($user))->toBeTrue() + ->and(OauthIdentity::where('provider_user_id', 'google-race-id')->count())->toBe(1); + $this->assertAuthenticatedAs($user); +}); + it('rejects oauth logins when the provider does not return an email address', function (?string $providerEmail) { config()->set('app.maintenance.driver', 'file'); InstanceSettings::firstOrCreate([ @@ -150,4 +185,7 @@ 'missing id' => ['missing'], 'blank id' => [' '], 'non-scalar id' => [[]], + 'true id' => [true], + 'false id' => [false], + 'float id' => [1.0], ]); diff --git a/tests/Feature/OidcOauthControllerTest.php b/tests/Feature/OidcOauthControllerTest.php index 21b335016..084347f66 100644 --- a/tests/Feature/OidcOauthControllerTest.php +++ b/tests/Feature/OidcOauthControllerTest.php @@ -6,7 +6,11 @@ use App\Models\OauthSetting; use App\Models\Team; use App\Models\User; +use App\Services\Auth\OauthLoginService; +use Illuminate\Database\UniqueConstraintViolationException; use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Support\Facades\DB; +use Illuminate\Support\Facades\Event; use Illuminate\Support\Facades\Log; use Illuminate\Support\Once; use Laravel\Socialite\Facades\Socialite; @@ -73,6 +77,44 @@ function fakeOidcProvider(array $claims = []): void $this->assertAuthenticatedAs($user); }); +it('continues oidc login when another request creates the identity first', function () { + $user = User::factory()->create(['email' => 'race@example.com']); + $eventName = 'eloquent.creating: '.OauthIdentity::class; + + Event::listen($eventName, function (OauthIdentity $identity): void { + $attributes = $identity->getAttributes(); + + DB::afterRollBack(fn () => DB::table('oauth_identities')->insert($attributes)); + + throw new UniqueConstraintViolationException( + DB::getDefaultConnection(), + 'insert into oauth_identities', + [], + new PDOException('duplicate identity'), + ); + }); + + try { + $resolvedUser = app(OauthLoginService::class)->login('oidc', (new OidcUser)->setRaw([ + 'iss' => 'https://idp.example.com', + 'sub' => 'oidc-race-id', + 'email' => 'race@example.com', + 'email_verified' => true, + 'name' => 'Race User', + ])->map([ + 'id' => 'oidc-race-id', + 'name' => 'Race User', + 'email' => 'race@example.com', + ]), OauthSetting::where('provider', 'oidc')->firstOrFail()); + } finally { + Event::forget($eventName); + } + + expect($resolvedUser->is($user))->toBeTrue() + ->and(OauthIdentity::where('provider_user_id', 'oidc-race-id')->count())->toBe(1); + $this->assertAuthenticatedAs($user); +}); + it('creates a new oidc user when provider registration is allowed while normal registration is disabled', function () { OauthSetting::where('provider', 'oidc')->update(['allow_registration' => true]);