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..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; @@ -35,16 +36,61 @@ 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) { - return $user; + $provider = $oauthSetting->provider; + $providerUserId = $oauthUser->id ?? null; + 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 : []; - if (! $this->canCreateUser($oauthSetting)) { - throw new HttpException(403, 'Registration is disabled'); + $identityKey = [ + 'provider' => $provider, + 'issuer' => $provider, + 'provider_user_id' => $providerUserId, + ]; + + 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 $user; + }); + } catch (UniqueConstraintViolationException $exception) { + return OauthIdentity::where($identityKey)->first()?->user ?? throw $exception; } - - return $this->createUser($oauthUser->name ?: $email, $email, $oauthSetting); } private function resolveOidcUser(object $oauthUser, OauthSetting $oauthSetting, string $email): User @@ -68,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/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..4671183ae 100644 --- a/tests/Feature/OauthControllerTest.php +++ b/tests/Feature/OauthControllerTest.php @@ -1,11 +1,17 @@ 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('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) { @@ -83,3 +158,34 @@ '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' => [[]], + '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]); 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']);