mirror of
https://github.com/coollabsio/coolify.git
synced 2026-08-24 02:24:11 -05:00
fix(auth): preserve OAuth identity across email changes (#11357)
This commit is contained in:
+5
-1
@@ -520,10 +520,14 @@ class User extends Authenticatable implements SendsEmail
|
||||
|
||||
/**
|
||||
* 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();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -7,6 +7,7 @@ use App\Models\OauthIdentity;
|
||||
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 @@ class OauthLoginService
|
||||
|
||||
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 @@ class OauthLoginService
|
||||
|
||||
$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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -1,11 +1,17 @@
|
||||
<?php
|
||||
|
||||
use App\Models\InstanceSettings;
|
||||
use App\Models\OauthIdentity;
|
||||
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;
|
||||
|
||||
uses(RefreshDatabase::class);
|
||||
|
||||
@@ -50,6 +56,75 @@ it('logs in an existing user when the oauth provider returns a mixed-case email'
|
||||
$response->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 @@ it('rejects oauth logins when the provider does not return an email address', fu
|
||||
'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],
|
||||
]);
|
||||
|
||||
@@ -6,7 +6,11 @@ use App\Models\OauthIdentity;
|
||||
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 @@ it('logs in a user through an existing oidc identity', function () {
|
||||
$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]);
|
||||
|
||||
|
||||
@@ -4,6 +4,7 @@ use App\Livewire\Project\Shared\Danger;
|
||||
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 @@ use Livewire\Livewire;
|
||||
uses(RefreshDatabase::class);
|
||||
|
||||
beforeEach(function () {
|
||||
InstanceSettings::create(['id' => 0]);
|
||||
InstanceSettings::forceCreate(['id' => 0]);
|
||||
Queue::fake();
|
||||
|
||||
$this->user = User::factory()->create([
|
||||
@@ -70,6 +71,21 @@ test('delete succeeds with correct password and redirects', function () {
|
||||
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']);
|
||||
|
||||
Reference in New Issue
Block a user