diff --git a/app/Http/Controllers/OauthController.php b/app/Http/Controllers/OauthController.php index c28290919a..c837cc49c8 100644 --- a/app/Http/Controllers/OauthController.php +++ b/app/Http/Controllers/OauthController.php @@ -3,17 +3,12 @@ namespace App\Http\Controllers; use App\Models\OauthSetting; -use App\Services\Auth\OauthIdentityIssuer; use App\Services\Auth\OauthLoginService; -use Illuminate\Http\RedirectResponse; -use Illuminate\Http\Request; use Illuminate\Support\Facades\Log; use Symfony\Component\HttpKernel\Exception\HttpException; class OauthController extends Controller { - public const string PASSWORD_CONFIRMATION_SESSION_KEY = 'oauth.password_confirmation'; - public function redirect(string $provider) { $oauthSetting = $this->enabledProvider($provider); @@ -22,44 +17,8 @@ class OauthController extends Controller return $socialiteProvider->redirect(); } - /** - * Start confirming a destructive action by re-authenticating the logged-in - * user through an OAuth provider the user has a linked identity with. - */ - public function confirm(Request $request, string $provider) + public function callback(string $provider, OauthLoginService $oauthLoginService) { - $user = $request->user(); - $oauthSetting = $this->enabledProvider($provider); - $linkedIdentities = $user->oauthIdentities()->where('provider', $oauthSetting->provider); - if (! $oauthSetting->isOidc()) { - $linkedIdentities->where('issuer', OauthIdentityIssuer::forSetting($oauthSetting)); - } - if (! $linkedIdentities->exists()) { - abort(403, 'No identity from this provider is linked to your account.'); - } - - $request->session()->put(self::PASSWORD_CONFIRMATION_SESSION_KEY, [ - 'user_id' => $user->getKey(), - 'provider' => $oauthSetting->provider, - 'return_to' => $this->sameOriginUrl(url()->previous()), - ]); - - $socialiteProvider = get_socialite_provider($oauthSetting->provider); - $parameters = socialite_reauthentication_parameters($oauthSetting); - if ($parameters !== []) { - $socialiteProvider->with($parameters); - } - - return $socialiteProvider->redirect(); - } - - public function callback(Request $request, string $provider, OauthLoginService $oauthLoginService) - { - $pendingConfirmation = $request->session()->pull(self::PASSWORD_CONFIRMATION_SESSION_KEY); - if (is_array($pendingConfirmation) && $request->user()) { - return $this->confirmCallback($request, $provider, $pendingConfirmation, $oauthLoginService); - } - try { $oauthSetting = $this->enabledProvider($provider); $oauthUser = get_socialite_provider($oauthSetting->provider)->user(); @@ -79,67 +38,6 @@ class OauthController extends Controller } } - /** - * Complete an OAuth confirmation. The returned identity must already be - * linked to the logged-in user; the session is never switched to another - * user and no identity is linked or updated. - * - * @param array{user_id?: mixed, provider?: mixed, return_to?: mixed} $pendingConfirmation - */ - private function confirmCallback(Request $request, string $provider, array $pendingConfirmation, OauthLoginService $oauthLoginService): RedirectResponse - { - $user = $request->user(); - - try { - $oauthSetting = $this->enabledProvider($provider); - if ( - data_get($pendingConfirmation, 'user_id') !== $user->getKey() - || data_get($pendingConfirmation, 'provider') !== $oauthSetting->provider - ) { - throw new HttpException(403, 'OAuth confirmation does not match the current session'); - } - - $oauthUser = get_socialite_provider($oauthSetting->provider)->user(); - if (! $oauthLoginService->identityBelongsToUser($user, $oauthSetting, $oauthUser)) { - throw new HttpException(403, 'OAuth identity is not linked to the current user'); - } - } catch (\Exception $e) { - auditLog('auth.oauth.password_confirmation_failed', [ - 'provider' => $provider, - 'actor_id' => $user->getKey(), - 'actor_email' => $user->email, - 'exception_class' => $e::class, - ], 'warning'); - Log::warning('OAuth password confirmation failed.', [ - 'provider' => $provider, - 'user_id' => $user->getKey(), - 'exception_class' => $e::class, - 'exception_message' => $e->getMessage(), - ]); - - abort(403, 'Confirmation failed. Sign in with the account linked to your Coolify user.'); - } - - $request->session()->passwordConfirmed(); - auditLog('auth.oauth.password_confirmed', [ - 'provider' => $oauthSetting->provider, - 'actor_id' => $user->getKey(), - 'actor_email' => $user->email, - ]); - - return redirect()->to($this->sameOriginUrl(data_get($pendingConfirmation, 'return_to'))); - } - - private function sameOriginUrl(mixed $url): string - { - $home = url('/'); - if (! is_string($url) || ($url !== $home && ! str_starts_with($url, $home.'/') && ! str_starts_with($url, $home.'?'))) { - return $home; - } - - return $url; - } - private function logCallbackFailure(string $provider, \Throwable $exception): void { auditLog('auth.oauth.callback_failed', [ diff --git a/app/Http/Middleware/DecideWhatToDoWithUser.php b/app/Http/Middleware/DecideWhatToDoWithUser.php index 603ba1ed8f..6babdb69a0 100644 --- a/app/Http/Middleware/DecideWhatToDoWithUser.php +++ b/app/Http/Middleware/DecideWhatToDoWithUser.php @@ -12,13 +12,6 @@ class DecideWhatToDoWithUser { public function handle(Request $request, Closure $next): Response { - // OAuth confirmation of destructive actions only re-authenticates the - // logged-in user and returns to the page it started from, so it must not - // be redirected to onboarding, subscription, or team selection. - if ($request->routeIs('auth.confirm', 'auth.callback')) { - return $next($request); - } - if (auth()?->user()?->teams?->count() === 0) { $currentTeam = auth()->user()?->recreate_personal_team(); refreshSession($currentTeam); diff --git a/app/Models/OauthSetting.php b/app/Models/OauthSetting.php index 06be4c043a..7765e41160 100644 --- a/app/Models/OauthSetting.php +++ b/app/Models/OauthSetting.php @@ -82,23 +82,6 @@ class OauthSetting extends Model return __("auth.login.{$this->provider}"); } - /** - * Human-readable provider name, e.g. for "Confirm with Google". - */ - public function providerName(): string - { - if ($this->isOidc()) { - return filled($this->custom_label) ? $this->custom_label : 'SSO'; - } - - return match ($this->provider) { - 'azure' => 'Microsoft', - 'github' => 'GitHub', - 'gitlab' => 'GitLab', - default => str($this->provider)->headline()->value(), - }; - } - public function isOidc(): bool { return $this->provider === 'oidc'; diff --git a/app/Models/User.php b/app/Models/User.php index e8429ac41a..3a2d26edb6 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -10,7 +10,6 @@ use App\Notifications\TransactionalEmails\ResetPassword as TransactionalEmailsRe use App\Services\ChangelogService; use App\Traits\DeletesUserSessions; use DateTimeInterface; -use Illuminate\Database\Eloquent\Collection; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Relations\HasMany; use Illuminate\Foundation\Auth\User as Authenticatable; @@ -604,34 +603,12 @@ class User extends Authenticatable implements SendsEmail } /** - * Whether destructive actions must be confirmed. Users with a password - * confirm with it; users with a linked OAuth identity can also confirm by - * re-authenticating through their provider. Only users with neither have - * no way to confirm. + * Whether destructive actions must be confirmed with the account password. + * Users with a linked OAuth identity only confirm with the dialog's typed + * confirmation, and users without a password have no way to confirm. */ public function requiresPasswordConfirmation(): bool { - return $this->hasPassword() || $this->hasSsoIdentity(); - } - - /** - * Enabled OAuth providers the user can re-authenticate with to confirm - * destructive actions. - * - * @return Collection - */ - public function oauthConfirmationProviders(): Collection - { - $providers = $this->oauthIdentities()->distinct()->pluck('provider'); - if ($providers->isEmpty()) { - return new Collection; - } - - return OauthSetting::query() - ->whereIn('provider', $providers) - ->where('enabled', true) - ->get() - ->filter(fn (OauthSetting $setting): bool => $setting->couldBeEnabled()) - ->values(); + return $this->hasPassword() && ! $this->hasSsoIdentity(); } } diff --git a/app/Services/Auth/OauthLoginService.php b/app/Services/Auth/OauthLoginService.php index eece071e02..f3b5b166e3 100644 --- a/app/Services/Auth/OauthLoginService.php +++ b/app/Services/Auth/OauthLoginService.php @@ -72,35 +72,6 @@ class OauthLoginService return $user->hasEnabledTwoFactorAuthentication(); } - /** - * Whether the identity returned by the provider is already linked to the - * given user. Used to confirm destructive actions through OAuth: it never - * links, updates, or creates identities or users, and never logs anyone in. - */ - public function identityBelongsToUser(User $user, OauthSetting $oauthSetting, object $oauthUser): bool - { - if ($oauthSetting->provider === 'oidc') { - [$issuer, $subject] = $this->oidcIssuerAndSubject($oauthUser); - $providerUserId = $subject; - } else { - $issuer = OauthIdentityIssuer::forSetting($oauthSetting); - $providerUserId = $oauthUser->id ?? null; - if (is_int($providerUserId)) { - $providerUserId = (string) $providerUserId; - } - } - - if (! is_string($issuer) || $issuer === '' || ! is_string($providerUserId) || trim($providerUserId) === '') { - return false; - } - - return $user->oauthIdentities() - ->where('provider', $oauthSetting->provider) - ->where('issuer', $issuer) - ->where('provider_user_id', $providerUserId) - ->exists(); - } - /** * @return array{0: mixed, 1: mixed} */ diff --git a/bootstrap/helpers/shared.php b/bootstrap/helpers/shared.php index bf8624ee16..94a35140c0 100644 --- a/bootstrap/helpers/shared.php +++ b/bootstrap/helpers/shared.php @@ -4922,14 +4922,13 @@ function formatContainerStatus(string $status): string * Check if the password step of a destructive action should be skipped. * Returns true if: * - Two-step confirmation is globally disabled - * - User has neither a password nor a linked OAuth identity (no way to confirm) - * - User confirmed recently (password or OAuth re-authentication), using the - * same `auth.password_confirmed_at` session value and timeout as Laravel + * - User has a linked OAuth identity (they only use the dialog's typed confirmation) + * - User has no password (no way to confirm) + * - User confirmed their password recently (`auth.password_confirmed_at` + * within `auth.password_timeout`) * - * Users with a linked OAuth identity are never skipped silently: they confirm - * with their password or by re-authenticating through their provider. - * - * Used by modal-confirmation.blade.php to determine if password step should be shown. + * Used by modal-confirmation.blade.php to determine if password step should be shown, + * and by verifyPasswordConfirmation() to enforce the same rule on the server. * * @return bool True if password confirmation should be skipped */ @@ -4963,7 +4962,6 @@ function hasRecentPasswordConfirmation(): bool /** * Verify password for two-step confirmation. * Skips verification in the cases listed in shouldSkipPasswordConfirmation(). - * Users without a password must confirm through their OAuth provider first. * * @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 @@ -4976,15 +4974,8 @@ function verifyPasswordConfirmation(mixed $password, ?Component $component = nul return true; } - $user = Auth::user(); - if (! $user->hasPassword()) { - $component?->addError('password', 'Please confirm with your sign-in provider first.'); - - return false; - } - // Verify the password - if (! is_string($password) || ! Hash::check($password, $user->password)) { + if (! is_string($password) || ! Hash::check($password, Auth::user()->password)) { $component?->addError('password', 'The provided password is incorrect.'); return false; diff --git a/bootstrap/helpers/socialite.php b/bootstrap/helpers/socialite.php index 1f989aaadb..5f7e8d868a 100644 --- a/bootstrap/helpers/socialite.php +++ b/bootstrap/helpers/socialite.php @@ -91,24 +91,3 @@ function get_socialite_provider(string $provider) return $socialite; } - -/** - * Authorization parameters that ask the provider to re-authenticate the user - * instead of reusing an existing provider session, where the provider - * supports it. Used to confirm destructive actions through OAuth. - * - * Socialite's with() replaces earlier parameters, so provider defaults set in - * get_socialite_provider() (Google's hd) are repeated here. - * - * @return array - */ -function socialite_reauthentication_parameters(OauthSetting $oauthSetting): array -{ - return match ($oauthSetting->provider) { - 'google' => ['hd' => $oauthSetting->tenant, 'prompt' => 'select_account', 'max_age' => 0], - 'oidc', 'authentik', 'zitadel', 'clerk' => ['prompt' => 'login', 'max_age' => 0], - 'azure', 'infomaniak' => ['prompt' => 'login'], - 'github' => ['prompt' => 'select_account'], - default => [], - }; -} diff --git a/resources/views/components/modal-confirmation.blade.php b/resources/views/components/modal-confirmation.blade.php index d2a9f0475e..5b40f01f56 100644 --- a/resources/views/components/modal-confirmation.blade.php +++ b/resources/views/components/modal-confirmation.blade.php @@ -34,14 +34,8 @@ use Illuminate\View\ComponentSlot; // Global setting to disable ALL two-step confirmation (text + password) $disableTwoStepConfirmation = data_get(InstanceSettings::get(), 'disable_two_step_confirmation'); - // Skip ONLY the password step (disabled globally, recently confirmed, or no way to confirm) + // Skip ONLY the password step (disabled globally, OAuth users, users without a password, or recently confirmed) $skipPasswordConfirmation = shouldSkipPasswordConfirmation(); - // Users with a linked OAuth identity can confirm by re-authenticating with their provider - $confirmingUser = auth()->user(); - $confirmWithPasswordInput = $confirmingUser?->hasPassword() ?? false; - $oauthConfirmationProviders = ($confirmWithPassword && ! $skipPasswordConfirmation && $confirmingUser) - ? $confirmingUser->oauthConfirmationProviders() - : collect(); if ($temporaryDisableTwoStepConfirmation) { $disableTwoStepConfirmation = false; // Password confirmation requirement is not affected by temporary two-step disable @@ -356,96 +350,61 @@ @if (!$skipPasswordConfirmation)
- @if ($confirmWithPasswordInput && $oauthConfirmationProviders->isNotEmpty()) - Please enter your password or confirm with your sign-in provider to confirm this destructive action. - @elseif ($confirmWithPasswordInput) - Please enter your password to confirm this destructive action. - @elseif ($oauthConfirmationProviders->isNotEmpty()) - Please confirm with your sign-in provider to confirm this destructive action. - @else - Your sign-in provider is not available, so this action cannot be confirmed. Please ask an administrator. - @endif + Please enter your password to confirm this destructive action. - @if ($confirmWithPasswordInput) -
- @php - $passwordConfirm = Str::uuid(); - @endphp - -
- - -
-

-

- @error('password') -

{{ $message }}

- @enderror -
- @endif - @if ($oauthConfirmationProviders->isNotEmpty()) -
- @if ($confirmWithPasswordInput) -
Or confirm with your sign-in provider:
- @endif -
- @foreach ($oauthConfirmationProviders as $oauthConfirmationProvider) - - Confirm with {{ $oauthConfirmationProvider->providerName() }} - - @endforeach -
-

- You will return to this page. Then start the action again; it will not ask for confirmation for a while. -

- @if (! $confirmWithPasswordInput) - @error('password') -

{{ $message }}

- @enderror - @endif -
- @endif +
+ @php + $passwordConfirm = Str::uuid(); + @endphp + +
+ + +
+

+

+ @error('password') +

{{ $message }}

+ @enderror +
Back - @if ($confirmWithPasswordInput) - - - - - @endif + }); + "> + + +
@endif diff --git a/routes/web.php b/routes/web.php index 9029fbee5b..4300ff674f 100644 --- a/routes/web.php +++ b/routes/web.php @@ -129,7 +129,6 @@ Route::post('/auth/link', [Controller::class, 'acceptLink'])->middleware('thrott Route::get('/auth/{provider}/redirect', [OauthController::class, 'redirect'])->name('auth.redirect'); Route::get('/auth/{provider}/callback', [OauthController::class, 'callback'])->name('auth.callback'); -Route::get('/auth/{provider}/confirm', [OauthController::class, 'confirm'])->middleware('auth')->name('auth.confirm'); // Local/testing previews for HTTP error pages and the Laravel debug renderer (never in production). if (app()->environment(['local', 'testing'])) { diff --git a/tests/Feature/OauthIdentityInstanceTest.php b/tests/Feature/OauthIdentityInstanceTest.php index a237aac5fd..7ac26d2ece 100644 --- a/tests/Feature/OauthIdentityInstanceTest.php +++ b/tests/Feature/OauthIdentityInstanceTest.php @@ -8,6 +8,7 @@ use App\Services\Auth\OauthIdentityIssuer; use App\Services\Auth\OauthLoginService; use Illuminate\Foundation\Testing\RefreshDatabase; use Illuminate\Support\Facades\Auth; +use Illuminate\Support\Facades\Route; use Illuminate\Support\Once; use Symfony\Component\HttpKernel\Exception\HttpException; @@ -211,25 +212,18 @@ describe('login', function () { }); describe('confirmation', function () { - it('matches the linked identity only on the same instance', function () { - $user = User::factory()->create(['email' => 'user@example.com']); - $setting = instanceOauthSetting('gitlab', ['base_url' => 'https://gitlab.example.com/']); - $service = app(OauthLoginService::class); - $service->login('gitlab', instanceOauthUser(1, 'user@example.com', ['confirmed_at' => '2026-01-01T00:00:00Z']), $setting); - - expect($service->identityBelongsToUser($user, $setting, instanceOauthUser(1, 'user@example.com')))->toBeTrue(); - - $setting->update(['base_url' => 'https://gitlab.attacker.example']); - - expect($service->identityBelongsToUser($user, $setting->refresh(), instanceOauthUser(1, 'user@example.com')))->toBeFalse(); - }); - - it('does not start a confirmation without an identity from the configured instance', function () { + it('has no OAuth re-authentication route for confirming destructive actions', function () { $user = User::factory()->create(['email' => 'user@example.com']); linkInstanceIdentity($user, 'gitlab', 'https://gitlab.example.com', '1'); - instanceOauthSetting('gitlab', ['base_url' => 'https://gitlab.attacker.example']); + instanceOauthSetting('gitlab', ['base_url' => 'https://gitlab.example.com']); - $this->actingAs($user)->get(route('auth.confirm', 'gitlab'))->assertForbidden(); + expect(Route::has('auth.confirm'))->toBeFalse(); + + // The path falls through to the catch-all route and stays inside Coolify. + $response = $this->actingAs($user)->get('/auth/gitlab/confirm'); + + $response->assertRedirect(); + expect($response->headers->get('Location'))->toStartWith(url('/')); }); }); diff --git a/tests/Feature/OauthPasswordConfirmationTest.php b/tests/Feature/OauthPasswordConfirmationTest.php index 6548a23214..528e2565b7 100644 --- a/tests/Feature/OauthPasswordConfirmationTest.php +++ b/tests/Feature/OauthPasswordConfirmationTest.php @@ -1,7 +1,5 @@ shouldReceive('setConfig')->andReturnSelf(); - $provider->shouldReceive('with')->andReturnSelf(); - $provider->shouldReceive('user')->andReturn((object) [ - 'email' => $email, - 'name' => 'Provider User', - 'id' => $providerUserId, - 'user' => ['email_verified' => true, 'hd' => 'example.com'], - ]); - - Socialite::shouldReceive('driver')->with('google')->andReturn($provider); -} - -function pendingConfirmationFor(User $user, string $provider = 'google', string $returnTo = 'http://localhost/servers'): array -{ - return [OauthController::PASSWORD_CONFIRMATION_SESSION_KEY => [ - 'user_id' => $user->id, - 'provider' => $provider, - 'return_to' => $returnTo, - ]]; -} - describe('confirmation requirement', function () { - it('requires password confirmation for a user with a password and a linked identity', function () { + it('skips the password for a user with a password and a linked identity', function () { $user = User::factory()->create(['password' => Hash::make('secret-password')]); linkGoogleIdentity($user, 'google-id'); $this->actingAs($user); - expect($user->requiresPasswordConfirmation())->toBeTrue() - ->and(shouldSkipPasswordConfirmation())->toBeFalse() - ->and(verifyPasswordConfirmation('wrong-password'))->toBeFalse() - ->and(verifyPasswordConfirmation('secret-password'))->toBeTrue(); + expect($user->requiresPasswordConfirmation())->toBeFalse() + ->and(shouldSkipPasswordConfirmation())->toBeTrue() + ->and(verifyPasswordConfirmation(''))->toBeTrue() + ->and(verifyPasswordConfirmation('wrong-password'))->toBeTrue(); }); - it('requires confirmation for a user with a linked identity and no password', function () { + it('skips the password for a user with a linked identity and no password', function () { $user = User::factory()->create(); $user->forceFill(['password' => null])->save(); linkGoogleIdentity($user, 'google-id'); $this->actingAs($user); + expect($user->requiresPasswordConfirmation())->toBeFalse() + ->and(shouldSkipPasswordConfirmation())->toBeTrue() + ->and(verifyPasswordConfirmation(''))->toBeTrue(); + }); + + it('requires the correct password for a user without a linked identity', function () { + $user = User::factory()->create(['password' => Hash::make('secret-password')]); + $this->actingAs($user); + expect($user->requiresPasswordConfirmation())->toBeTrue() ->and(shouldSkipPasswordConfirmation())->toBeFalse() - ->and(verifyPasswordConfirmation(''))->toBeFalse(); + ->and(verifyPasswordConfirmation('wrong-password'))->toBeFalse() + ->and(verifyPasswordConfirmation(''))->toBeFalse() + ->and(verifyPasswordConfirmation(['secret-password']))->toBeFalse() + ->and(verifyPasswordConfirmation('secret-password'))->toBeTrue(); }); it('keeps skipping confirmation for a user without a password and without an identity', function () { @@ -103,221 +90,48 @@ describe('confirmation requirement', function () { ->and(shouldSkipPasswordConfirmation())->toBeTrue(); }); - it('accepts a recent confirmation from the session', function () { - $user = User::factory()->create(); - linkGoogleIdentity($user, 'google-id'); + it('skips the password for everyone when two-step confirmation is disabled', function () { + InstanceSettings::query()->update(['disable_two_step_confirmation' => true]); + Once::flush(); + $user = User::factory()->create(['password' => Hash::make('secret-password')]); $this->actingAs($user); - session()->put('auth.password_confirmed_at', now()->unix()); - expect(shouldSkipPasswordConfirmation())->toBeTrue() - ->and(verifyPasswordConfirmation(''))->toBeTrue(); - }); - - it('rejects an expired confirmation from the session', function () { - $user = User::factory()->create(); - linkGoogleIdentity($user, 'google-id'); - $this->actingAs($user); - - session()->put('auth.password_confirmed_at', now()->subSeconds(config('auth.password_timeout') + 1)->unix()); - - expect(shouldSkipPasswordConfirmation())->toBeFalse() - ->and(verifyPasswordConfirmation(''))->toBeFalse(); + ->and(verifyPasswordConfirmation('wrong-password'))->toBeTrue(); }); }); -describe('confirmation redirect', function () { - it('requires an authenticated user', function () { - $this->get(route('auth.confirm', 'google'))->assertRedirect(route('login')); - }); - - it('rejects a user without an identity for the provider', function () { - $user = User::factory()->create(); - - $this->actingAs($user)->get(route('auth.confirm', 'google'))->assertForbidden(); - - expect(session(OauthController::PASSWORD_CONFIRMATION_SESSION_KEY))->toBeNull(); - }); - - it('rejects a disabled provider', function () { - $user = User::factory()->create(); - linkGoogleIdentity($user, 'google-id'); - OauthSetting::where('provider', 'google')->update(['enabled' => false]); - - $this->actingAs($user)->get(route('auth.confirm', 'google'))->assertForbidden(); - }); - - it('redirects to the provider with re-authentication parameters and remembers the pending confirmation', function () { +describe('oauth confirmation route', function () { + it('no longer exists', function () { $user = User::factory()->create(); linkGoogleIdentity($user, 'google-id'); - $provider = Mockery::mock(); - $provider->shouldReceive('setConfig')->andReturnSelf(); - $provider->shouldReceive('with')->once()->with(['hd' => 'example.com'])->andReturnSelf(); - $provider->shouldReceive('with')->once()->with([ - 'hd' => 'example.com', - 'prompt' => 'select_account', - 'max_age' => 0, - ])->andReturnSelf(); - $provider->shouldReceive('redirect')->once()->andReturn(redirect('https://accounts.google.com/o/oauth2/auth')); - Socialite::shouldReceive('driver')->with('google')->andReturn($provider); + expect(Route::has('auth.confirm'))->toBeFalse(); - $this->actingAs($user) - ->from('/servers') - ->get(route('auth.confirm', 'google')) - ->assertRedirect('https://accounts.google.com/o/oauth2/auth'); + Socialite::shouldReceive('driver')->never(); - expect(session(OauthController::PASSWORD_CONFIRMATION_SESSION_KEY))->toBe([ - 'user_id' => $user->id, - 'provider' => 'google', - 'return_to' => url('/servers'), - ]); + // The path falls through to the catch-all route and stays inside Coolify. + $response = $this->actingAs($user)->get('/auth/google/confirm'); + + $response->assertRedirect(); + expect($response->headers->get('Location'))->toStartWith(url('/')); }); - it('does not remember an external return URL', function () { - $user = User::factory()->create(); - linkGoogleIdentity($user, 'google-id'); + it('keeps the normal login flow and does not mark the session as password confirmed', function () { + $user = User::factory()->create(['email' => 'owner@example.com']); + linkGoogleIdentity($user, 'owner-google-id'); + $provider = Mockery::mock(); $provider->shouldReceive('setConfig')->andReturnSelf(); $provider->shouldReceive('with')->andReturnSelf(); - $provider->shouldReceive('redirect')->andReturn(redirect('https://accounts.google.com/o/oauth2/auth')); + $provider->shouldReceive('user')->andReturn((object) [ + 'email' => 'owner@example.com', + 'name' => 'Provider User', + 'id' => 'owner-google-id', + 'user' => ['email_verified' => true, 'hd' => 'example.com'], + ]); Socialite::shouldReceive('driver')->with('google')->andReturn($provider); - $this->actingAs($user) - ->withHeader('referer', 'https://evil.example.com/phish') - ->get(route('auth.confirm', 'google')); - - expect(session(OauthController::PASSWORD_CONFIRMATION_SESSION_KEY.'.return_to'))->toBe(url('/')); - }); -}); - -describe('confirmation callback', function () { - it('marks the session as password confirmed when the linked identity of the same user returns', function () { - $user = User::factory()->create(['email' => 'owner@example.com']); - $identity = linkGoogleIdentity($user, 'owner-google-id'); - fakeGoogleConfirmation('owner-google-id', 'owner@example.com'); - - $this->actingAs($user) - ->withSession(pendingConfirmationFor($user)) - ->get(route('auth.callback', 'google')) - ->assertRedirect('http://localhost/servers'); - - $this->assertAuthenticatedAs($user); - expect(session('auth.password_confirmed_at'))->toBeInt() - ->and(session(OauthController::PASSWORD_CONFIRMATION_SESSION_KEY))->toBeNull() - ->and(shouldSkipPasswordConfirmation())->toBeTrue() - ->and($identity->fresh()->user_id)->toBe($user->id); - }); - - it('rejects the identity of another user and does not switch users', function () { - $user = User::factory()->create(['email' => 'owner@example.com']); - linkGoogleIdentity($user, 'owner-google-id'); - $otherUser = User::factory()->create(['email' => 'other@example.com']); - linkGoogleIdentity($otherUser, 'other-google-id'); - fakeGoogleConfirmation('other-google-id', 'other@example.com'); - - $this->actingAs($user) - ->withSession(pendingConfirmationFor($user)) - ->get(route('auth.callback', 'google')) - ->assertForbidden(); - - $this->assertAuthenticatedAs($user); - expect(session('auth.password_confirmed_at'))->toBeNull(); - }); - - it('rejects an unknown identity without linking or creating a user', function () { - InstanceSettings::query()->update(['is_registration_enabled' => true]); - $user = User::factory()->create(['email' => 'owner@example.com']); - linkGoogleIdentity($user, 'owner-google-id'); - fakeGoogleConfirmation('unknown-google-id', 'owner@example.com'); - - $this->actingAs($user) - ->withSession(pendingConfirmationFor($user)) - ->get(route('auth.callback', 'google')) - ->assertForbidden(); - - $this->assertAuthenticatedAs($user); - expect(session('auth.password_confirmed_at'))->toBeNull() - ->and(OauthIdentity::where('provider_user_id', 'unknown-google-id')->exists())->toBeFalse() - ->and(User::count())->toBe(1); - }); - - it('rejects a pending confirmation that belongs to another user', function () { - $user = User::factory()->create(['email' => 'owner@example.com']); - linkGoogleIdentity($user, 'owner-google-id'); - $otherUser = User::factory()->create(['email' => 'other@example.com']); - linkGoogleIdentity($otherUser, 'other-google-id'); - fakeGoogleConfirmation('other-google-id', 'other@example.com'); - - $this->actingAs($user) - ->withSession(pendingConfirmationFor($otherUser)) - ->get(route('auth.callback', 'google')) - ->assertForbidden(); - - $this->assertAuthenticatedAs($user); - expect(session('auth.password_confirmed_at'))->toBeNull(); - }); - - it('rejects a pending confirmation for a different provider', function () { - OauthSetting::create([ - 'provider' => 'github', - 'client_id' => 'client-id', - 'client_secret' => 'client-secret', - 'enabled' => true, - ]); - $user = User::factory()->create(['email' => 'owner@example.com']); - linkGoogleIdentity($user, 'owner-google-id'); - fakeGoogleConfirmation('owner-google-id', 'owner@example.com'); - - $this->actingAs($user) - ->withSession(pendingConfirmationFor($user, 'github')) - ->get(route('auth.callback', 'google')) - ->assertForbidden(); - - expect(session('auth.password_confirmed_at'))->toBeNull(); - }); - - it('confirms an OIDC identity by issuer and subject', function () { - OauthSetting::create([ - 'provider' => 'oidc', - 'enabled' => true, - 'client_id' => 'client-id', - 'client_secret' => 'client-secret', - 'base_url' => 'https://idp.example.com', - 'redirect_uri' => 'https://coolify.example.com/auth/oidc/callback', - ]); - $user = User::factory()->create(['email' => 'oidc@example.com']); - OauthIdentity::create([ - 'user_id' => $user->id, - 'provider' => 'oidc', - 'issuer' => 'https://idp.example.com', - 'provider_user_id' => 'oidc-subject', - 'email' => $user->email, - ]); - $oidcUser = (new OidcUser)->setRaw([ - 'iss' => 'https://idp.example.com', - 'sub' => 'oidc-subject', - 'email' => 'oidc@example.com', - 'email_verified' => true, - ])->map(['id' => 'oidc-subject', 'email' => 'oidc@example.com', 'name' => 'OIDC User']); - $provider = Mockery::mock(); - $provider->shouldReceive('setConfig')->andReturnSelf(); - $provider->shouldReceive('user')->andReturn($oidcUser); - Socialite::shouldReceive('driver')->with('oidc')->andReturn($provider); - - $this->actingAs($user) - ->withSession(pendingConfirmationFor($user, 'oidc')) - ->get(route('auth.callback', 'oidc')) - ->assertRedirect('http://localhost/servers'); - - expect(session('auth.password_confirmed_at'))->toBeInt(); - }); - - it('keeps the normal login flow when no confirmation is pending', function () { - $user = User::factory()->create(['email' => 'owner@example.com']); - linkGoogleIdentity($user, 'owner-google-id'); - fakeGoogleConfirmation('owner-google-id', 'owner@example.com'); - $this->get(route('auth.callback', 'google'))->assertRedirect('/'); $this->assertAuthenticatedAs($user); @@ -333,37 +147,29 @@ describe('confirmation modal', function () { return Blade::render(''); } - it('offers OAuth confirmation next to the password for a user with a linked identity', function () { + it('shows only the typed confirmation for a user with a linked identity', function () { $user = User::factory()->create(); linkGoogleIdentity($user, 'google-id'); $this->actingAs($user); $html = renderConfirmationModal(); - expect($html)->toContain('Confirm with Google') - ->and($html)->toContain(route('auth.confirm', 'google')) - ->and($html)->toContain('type="password"'); + expect($html)->toContain('x-model="userConfirmationText"') + ->and($html)->toContain('confirmWithText: true') + ->and($html)->toContain('confirmWithPassword: false') + ->and($html)->not->toContain('type="password"') + ->and($html)->not->toContain('Confirm with Google') + ->and($html)->not->toContain('/auth/google/confirm'); }); - it('offers only OAuth confirmation for a user without a password', function () { - $user = User::factory()->create(); - $user->forceFill(['password' => null])->save(); - linkGoogleIdentity($user, 'google-id'); - $this->actingAs($user); - - $html = renderConfirmationModal(); - - expect($html)->toContain('Confirm with Google') - ->and($html)->not->toContain('type="password"'); - }); - - it('does not offer OAuth confirmation for a user without an identity', function () { + it('shows the password step for a user without an identity', function () { $user = User::factory()->create(); $this->actingAs($user); $html = renderConfirmationModal(); - expect($html)->not->toContain('Confirm with Google') - ->and($html)->toContain('type="password"'); + expect($html)->toContain('confirmWithPassword: true') + ->and($html)->toContain('type="password"') + ->and($html)->not->toContain('Confirm with Google'); }); }); diff --git a/tests/Feature/Subscription/SubscriptionActionsTest.php b/tests/Feature/Subscription/SubscriptionActionsTest.php index 698c84d95c..f42eb05dd6 100644 --- a/tests/Feature/Subscription/SubscriptionActionsTest.php +++ b/tests/Feature/Subscription/SubscriptionActionsTest.php @@ -3,6 +3,7 @@ use App\Actions\Stripe\RefundSubscription; use App\Livewire\Subscription\Actions; use App\Models\InstanceSettings; +use App\Models\OauthIdentity; use App\Models\Subscription; use App\Models\Team; use App\Models\User; @@ -68,3 +69,32 @@ describe('cancelImmediately with refund option', function () { ->assertReturned('Invalid password.'); }); }); + +describe('password confirmation', function () { + test('a user with a linked oauth identity cancels without a password', function () { + OauthIdentity::create([ + 'user_id' => $this->user->id, + 'provider' => 'oidc', + 'issuer' => 'https://idp.example.com', + 'provider_user_id' => 'oauth-user-id', + ]); + $mock = Mockery::mock(RefundSubscription::class); + $mock->shouldReceive('execute')->once()->andReturn(['success' => true, 'error' => null]); + $this->instance(RefundSubscription::class, $mock); + + Livewire::test(Actions::class) + ->call('cancelImmediately', '', ['refundLatestPayment']) + ->assertDispatched('success') + ->assertRedirect(route('subscription.index')); + }); + + test('a user without a linked oauth identity is rejected with an empty password', function () { + $mock = Mockery::mock(RefundSubscription::class); + $mock->shouldNotReceive('execute'); + $this->instance(RefundSubscription::class, $mock); + + Livewire::test(Actions::class) + ->call('refundSubscription', '') + ->assertReturned('Invalid password.'); + }); +}); diff --git a/tests/v4/Feature/DangerDeleteResourceTest.php b/tests/v4/Feature/DangerDeleteResourceTest.php index 6ce81cfc81..25cdf671a2 100644 --- a/tests/v4/Feature/DangerDeleteResourceTest.php +++ b/tests/v4/Feature/DangerDeleteResourceTest.php @@ -88,7 +88,7 @@ test('delete redirects before dispatching resource cleanup after the response', Queue::assertPushed(DeleteResourceJob::class, fn (DeleteResourceJob $job) => $job->resource->is($service)); }); -test('delete requires confirmation for an oauth user', function () { +test('delete succeeds without a password for a user with a linked oauth identity', function () { OauthIdentity::create([ 'user_id' => $this->user->id, 'provider' => 'oidc', @@ -97,27 +97,47 @@ test('delete requires confirmation for an oauth user', function () { ]); Livewire::test(Danger::class, ['resource' => $this->application]) + ->set('projectUuid', $this->project->uuid) + ->set('environmentUuid', $this->environment->uuid) ->call('delete', '') - ->assertHasErrors('password'); + ->assertHasNoErrors() + ->assertRedirectToRoute('project.resource.index', [ + 'project_uuid' => $this->project->uuid, + 'environment_uuid' => $this->environment->uuid, + ]); + + Queue::assertPushed(DeleteResourceJob::class, fn (DeleteResourceJob $job) => $job->resource->is($this->application)); +}); + +test('delete still asks a user with a linked oauth identity for the typed confirmation but not for a password', 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]) + ->assertSee('x-model="userConfirmationText"', false) + ->assertSee('confirmWithText: true', false) + ->assertSee('confirmWithPassword: false', false) + ->assertDontSee('type="password"', false) + ->assertDontSee('Confirm with', false); +}); + +test('delete rejects an empty password for a user without a linked oauth identity', function () { + Livewire::test(Danger::class, ['resource' => $this->application]) + ->call('delete', '') + ->assertHasErrors('password') + ->assertReturned('The provided password is incorrect.'); Queue::assertNotPushed(DeleteResourceJob::class); }); -test('delete succeeds without password for an oauth user after confirming through the provider', function () { - OauthIdentity::create([ - 'user_id' => $this->user->id, - 'provider' => 'oidc', - 'issuer' => 'https://idp.example.com', - 'provider_user_id' => 'oauth-user-id', - ]); - session()->passwordConfirmed(); - +test('delete asks a user without a linked oauth identity for the password', function () { Livewire::test(Danger::class, ['resource' => $this->application]) - ->call('delete', '') - ->assertHasNoErrors(); - - expect(Application::find($this->application->id))->not->toBeNull(); - Queue::assertPushed(DeleteResourceJob::class, fn (DeleteResourceJob $job) => $job->resource->is($this->application)); + ->assertSee('confirmWithPassword: true', false) + ->assertSee('type="password"', false); }); test('delete applies selectedActions from checkbox state', function () {