fix: let authenticated users connect Google/GitHub from Settings
The Connect button on /settings/authentication pointed at the
auth.{provider}.redirect routes that live behind `guest` middleware,
so authenticated users were bounced to /app/home before reaching
Socialite. The OAuth callback also needed to handle two flows
(signup/login vs link to current user) but had no branch for the
second case — meaning a different-email GitHub account would have
been registered as a new user, logging the original session out.
Splits the flows by intent:
- New `app.authentication.connect-provider` route in the auth group,
handled by the settings controller (where it sits next to
disconnect-provider). Replaces the OAuth signup link as the
Connect button's target.
- Auth callbacks moved out of the guest group (still one URL per
provider, since OAuth apps only register one) and gain a single
Auth::check() branch that calls connectToCurrentUser().
- connectToCurrentUser() rejects if the provider id already belongs
to a different user; otherwise sets it on the current user and
redirects back to settings with a flash message.
This commit is contained in:
parent
c543bec6af
commit
d89f6ceede
10 changed files with 249 additions and 11 deletions
|
|
@ -14,6 +14,7 @@
|
|||
use Illuminate\Validation\Rule;
|
||||
use Inertia\Inertia;
|
||||
use Inertia\Response;
|
||||
use Laravel\Socialite\Facades\Socialite;
|
||||
|
||||
class AuthenticationController extends Controller
|
||||
{
|
||||
|
|
@ -63,6 +64,16 @@ public function destroyOtherSessions(Request $request): RedirectResponse
|
|||
return back()->with('flash.success', __('settings.authentication.sessions.flash_logged_out'));
|
||||
}
|
||||
|
||||
public function connectProvider(string $provider): RedirectResponse
|
||||
{
|
||||
abort_unless(in_array($provider, self::PROVIDERS, true), 404);
|
||||
|
||||
return match ($provider) {
|
||||
'google' => Socialite::driver('google-auth')->redirect(),
|
||||
'github' => Socialite::driver('github')->scopes(['read:user', 'user:email'])->redirect(),
|
||||
};
|
||||
}
|
||||
|
||||
public function disconnectProvider(Request $request, string $provider): RedirectResponse
|
||||
{
|
||||
abort_unless(in_array($provider, self::PROVIDERS, true), 404);
|
||||
|
|
|
|||
|
|
@ -35,6 +35,10 @@ public function callback(): RedirectResponse
|
|||
return redirect()->route('login');
|
||||
}
|
||||
|
||||
if (Auth::check()) {
|
||||
return $this->connectToCurrentUser(Auth::user(), (string) $githubUser->getId());
|
||||
}
|
||||
|
||||
$user = User::where('github_id', (string) $githubUser->getId())
|
||||
->when($githubUser->getEmail(), fn ($query, $email) => $query->orWhere('email', $email))
|
||||
->first();
|
||||
|
|
@ -52,6 +56,25 @@ public function callback(): RedirectResponse
|
|||
return $this->registerNewUser($githubUser);
|
||||
}
|
||||
|
||||
private function connectToCurrentUser(User $user, string $githubId): RedirectResponse
|
||||
{
|
||||
$existing = User::where('github_id', $githubId)
|
||||
->where('id', '!=', $user->id)
|
||||
->first();
|
||||
|
||||
if ($existing) {
|
||||
return redirect()->route('app.authentication.edit')
|
||||
->with('flash.error', __('settings.authentication.providers.flash_already_linked', ['provider' => 'GitHub']));
|
||||
}
|
||||
|
||||
if ($user->github_id !== $githubId) {
|
||||
$user->update(['github_id' => $githubId]);
|
||||
}
|
||||
|
||||
return redirect()->route('app.authentication.edit')
|
||||
->with('flash.success', __('settings.authentication.providers.flash_connected', ['provider' => 'GitHub']));
|
||||
}
|
||||
|
||||
private function loginExistingUser(User $user, string $githubId): RedirectResponse
|
||||
{
|
||||
if (! $user->github_id) {
|
||||
|
|
|
|||
|
|
@ -33,6 +33,10 @@ public function callback(): RedirectResponse
|
|||
return redirect()->route('login');
|
||||
}
|
||||
|
||||
if (Auth::check()) {
|
||||
return $this->connectToCurrentUser(Auth::user(), $googleUser->getId());
|
||||
}
|
||||
|
||||
$user = User::where('google_id', $googleUser->getId())
|
||||
->orWhere('email', $googleUser->getEmail())
|
||||
->first();
|
||||
|
|
@ -44,6 +48,25 @@ public function callback(): RedirectResponse
|
|||
return $this->registerNewUser($googleUser);
|
||||
}
|
||||
|
||||
private function connectToCurrentUser(User $user, string $googleId): RedirectResponse
|
||||
{
|
||||
$existing = User::where('google_id', $googleId)
|
||||
->where('id', '!=', $user->id)
|
||||
->first();
|
||||
|
||||
if ($existing) {
|
||||
return redirect()->route('app.authentication.edit')
|
||||
->with('flash.error', __('settings.authentication.providers.flash_already_linked', ['provider' => 'Google']));
|
||||
}
|
||||
|
||||
if ($user->google_id !== $googleId) {
|
||||
$user->update(['google_id' => $googleId]);
|
||||
}
|
||||
|
||||
return redirect()->route('app.authentication.edit')
|
||||
->with('flash.success', __('settings.authentication.providers.flash_connected', ['provider' => 'Google']));
|
||||
}
|
||||
|
||||
private function loginExistingUser(User $user, string $googleId): RedirectResponse
|
||||
{
|
||||
if (! $user->google_id) {
|
||||
|
|
|
|||
|
|
@ -100,6 +100,8 @@
|
|||
'connect' => 'Connect',
|
||||
'disconnect' => 'Disconnect',
|
||||
'flash_disconnected' => ':provider disconnected successfully.',
|
||||
'flash_connected' => ':provider connected successfully.',
|
||||
'flash_already_linked' => 'That :provider account is already linked to another user.',
|
||||
'flash_cannot_disconnect' => 'You cannot disconnect your only sign-in method. Set a password or connect another provider first.',
|
||||
],
|
||||
],
|
||||
|
|
|
|||
|
|
@ -100,6 +100,8 @@
|
|||
'connect' => 'Conectar',
|
||||
'disconnect' => 'Desconectar',
|
||||
'flash_disconnected' => ':provider desconectada correctamente.',
|
||||
'flash_connected' => ':provider conectada correctamente.',
|
||||
'flash_already_linked' => 'Esa cuenta de :provider ya está vinculada a otro usuario.',
|
||||
'flash_cannot_disconnect' => 'No puedes desconectar tu único método de inicio de sesión. Define una contraseña o conecta otro proveedor primero.',
|
||||
],
|
||||
],
|
||||
|
|
|
|||
|
|
@ -100,6 +100,8 @@
|
|||
'connect' => 'Conectar',
|
||||
'disconnect' => 'Desconectar',
|
||||
'flash_disconnected' => ':provider desconectada com sucesso.',
|
||||
'flash_connected' => ':provider conectada com sucesso.',
|
||||
'flash_already_linked' => 'Essa conta do :provider já está vinculada a outro usuário.',
|
||||
'flash_cannot_disconnect' => 'Você não pode desconectar seu único método de login. Defina uma senha ou conecte outro provedor primeiro.',
|
||||
],
|
||||
],
|
||||
|
|
|
|||
|
|
@ -26,11 +26,9 @@ import { Separator } from '@/components/ui/separator';
|
|||
import AppLayout from '@/layouts/AppLayout.vue';
|
||||
import { isMobileDevice, parseBrowserName, parseOsName } from '@/lib/userAgent';
|
||||
import { settings as settingsHub } from '@/routes/app';
|
||||
import { edit as editAuthentication } from '@/routes/app/authentication';
|
||||
import { connectProvider, edit as editAuthentication } from '@/routes/app/authentication';
|
||||
import { preferences as notificationPreferences } from '@/routes/app/notifications';
|
||||
import { edit as editProfile } from '@/routes/app/profile';
|
||||
import { redirect as githubRedirect } from '@/routes/auth/github';
|
||||
import { redirect as googleRedirect } from '@/routes/auth/google';
|
||||
import type { BreadcrumbItem } from '@/types';
|
||||
|
||||
type Session = {
|
||||
|
|
@ -66,10 +64,6 @@ const tabs = computed(() => [
|
|||
{ name: 'notifications', label: trans('settings.nav.notifications'), href: notificationPreferences().url },
|
||||
]);
|
||||
|
||||
const providerRedirects: Record<ConnectedAccount['provider'], () => { url: string }> = {
|
||||
google: googleRedirect,
|
||||
github: githubRedirect,
|
||||
};
|
||||
|
||||
const passwordHeading = computed(() =>
|
||||
props.hasPassword
|
||||
|
|
@ -335,7 +329,7 @@ const logoutDialogOpen = ref(false);
|
|||
</Form>
|
||||
<Link
|
||||
v-else-if="!account.connected"
|
||||
:href="providerRedirects[account.provider]().url"
|
||||
:href="connectProvider(account.provider).url"
|
||||
>
|
||||
<Button
|
||||
variant="outline"
|
||||
|
|
|
|||
|
|
@ -249,6 +249,8 @@
|
|||
->name('app.authentication.update-password');
|
||||
Route::delete('settings/authentication/sessions', [AuthenticationController::class, 'destroyOtherSessions'])
|
||||
->name('app.authentication.destroy-other-sessions');
|
||||
Route::get('settings/authentication/providers/{provider}/connect', [AuthenticationController::class, 'connectProvider'])
|
||||
->name('app.authentication.connect-provider');
|
||||
Route::delete('settings/authentication/providers/{provider}', [AuthenticationController::class, 'disconnectProvider'])
|
||||
->name('app.authentication.disconnect-provider');
|
||||
|
||||
|
|
|
|||
|
|
@ -31,12 +31,16 @@
|
|||
Route::post('/reset-password', [NewPasswordController::class, 'store'])->name('password.store');
|
||||
|
||||
Route::get('/auth/google/redirect', [GoogleController::class, 'redirect'])->name('auth.google.redirect');
|
||||
Route::get('/auth/google/callback', [GoogleController::class, 'callback'])->name('auth.google.callback');
|
||||
|
||||
Route::get('/auth/github/redirect', [GitHubController::class, 'redirect'])->name('auth.github.redirect');
|
||||
Route::get('/auth/github/callback', [GitHubController::class, 'callback'])->name('auth.github.callback');
|
||||
});
|
||||
|
||||
// Callbacks must be reachable by both guests (signup/login flow) and
|
||||
// authenticated users (connect-from-settings flow). Branching on
|
||||
// `Auth::check()` inside the callback is safe because the redirect that
|
||||
// initiated the round-trip enforces the right middleware.
|
||||
Route::get('/auth/google/callback', [GoogleController::class, 'callback'])->name('auth.google.callback');
|
||||
Route::get('/auth/github/callback', [GitHubController::class, 'callback'])->name('auth.github.callback');
|
||||
|
||||
Route::middleware(['auth'])->group(function () {
|
||||
Route::get('/register/success', SignupSuccessController::class)->name('register.success');
|
||||
|
||||
|
|
|
|||
175
tests/Feature/Auth/ConnectProviderTest.php
Normal file
175
tests/Feature/Auth/ConnectProviderTest.php
Normal file
|
|
@ -0,0 +1,175 @@
|
|||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
use App\Models\User;
|
||||
use Laravel\Socialite\Facades\Socialite;
|
||||
use Laravel\Socialite\Two\User as SocialiteUser;
|
||||
|
||||
test('authenticated user can hit the connect-provider route for github', function () {
|
||||
$user = User::factory()->create();
|
||||
|
||||
$driver = Mockery::mock();
|
||||
$driver->shouldReceive('scopes')->andReturnSelf();
|
||||
$driver->shouldReceive('redirect')->andReturn(redirect('https://github.com/login/oauth/authorize'));
|
||||
Socialite::shouldReceive('driver')->with('github')->andReturn($driver);
|
||||
|
||||
$this->actingAs($user)
|
||||
->get(route('app.authentication.connect-provider', 'github'))
|
||||
->assertRedirect('https://github.com/login/oauth/authorize');
|
||||
});
|
||||
|
||||
test('authenticated user can hit the connect-provider route for google', function () {
|
||||
$user = User::factory()->create();
|
||||
|
||||
$driver = Mockery::mock();
|
||||
$driver->shouldReceive('redirect')->andReturn(redirect('https://accounts.google.com/o/oauth2/auth'));
|
||||
Socialite::shouldReceive('driver')->with('google-auth')->andReturn($driver);
|
||||
|
||||
$this->actingAs($user)
|
||||
->get(route('app.authentication.connect-provider', 'google'))
|
||||
->assertRedirect('https://accounts.google.com/o/oauth2/auth');
|
||||
});
|
||||
|
||||
test('connect-provider route rejects unknown provider', function () {
|
||||
$user = User::factory()->create();
|
||||
|
||||
$this->actingAs($user)
|
||||
->get(route('app.authentication.connect-provider', 'twitter'))
|
||||
->assertNotFound();
|
||||
});
|
||||
|
||||
test('connect-provider route requires authentication', function () {
|
||||
$this->get(route('app.authentication.connect-provider', 'github'))
|
||||
->assertRedirect(route('login'));
|
||||
});
|
||||
|
||||
test('authenticated callback connects github to the current user', function () {
|
||||
$user = User::factory()->create([
|
||||
'email' => 'me@example.com',
|
||||
'google_id' => 'g-me',
|
||||
'github_id' => null,
|
||||
]);
|
||||
|
||||
$socialiteUser = new SocialiteUser;
|
||||
$socialiteUser->id = 'gh-me';
|
||||
$socialiteUser->name = 'Me';
|
||||
$socialiteUser->email = 'me@example.com';
|
||||
|
||||
Socialite::shouldReceive('driver')
|
||||
->with('github')
|
||||
->andReturn($driver = Mockery::mock());
|
||||
|
||||
$driver->shouldReceive('user')->andReturn($socialiteUser);
|
||||
|
||||
$this->actingAs($user)
|
||||
->get(route('auth.github.callback'))
|
||||
->assertRedirect(route('app.authentication.edit'))
|
||||
->assertSessionHas('flash.success');
|
||||
|
||||
expect($user->fresh()->github_id)->toBe('gh-me');
|
||||
});
|
||||
|
||||
test('authenticated callback links github by current user, not by email', function () {
|
||||
$user = User::factory()->create([
|
||||
'email' => 'work@example.com',
|
||||
'google_id' => 'g-me',
|
||||
'github_id' => null,
|
||||
]);
|
||||
|
||||
$socialiteUser = new SocialiteUser;
|
||||
$socialiteUser->id = 'gh-personal';
|
||||
$socialiteUser->name = 'Me';
|
||||
$socialiteUser->email = 'personal@example.com';
|
||||
|
||||
Socialite::shouldReceive('driver')
|
||||
->with('github')
|
||||
->andReturn($driver = Mockery::mock());
|
||||
|
||||
$driver->shouldReceive('user')->andReturn($socialiteUser);
|
||||
|
||||
$this->actingAs($user)
|
||||
->get(route('auth.github.callback'))
|
||||
->assertRedirect(route('app.authentication.edit'))
|
||||
->assertSessionHas('flash.success');
|
||||
|
||||
expect($user->fresh()->github_id)->toBe('gh-personal');
|
||||
expect(User::where('email', 'personal@example.com')->exists())->toBeFalse();
|
||||
$this->assertAuthenticatedAs($user);
|
||||
});
|
||||
|
||||
test('authenticated callback rejects when github account is already linked to another user', function () {
|
||||
User::factory()->create(['github_id' => 'gh-taken']);
|
||||
|
||||
$me = User::factory()->create(['email' => 'me@example.com', 'github_id' => null]);
|
||||
|
||||
$socialiteUser = new SocialiteUser;
|
||||
$socialiteUser->id = 'gh-taken';
|
||||
$socialiteUser->name = 'Me';
|
||||
$socialiteUser->email = 'me@example.com';
|
||||
|
||||
Socialite::shouldReceive('driver')
|
||||
->with('github')
|
||||
->andReturn($driver = Mockery::mock());
|
||||
|
||||
$driver->shouldReceive('user')->andReturn($socialiteUser);
|
||||
|
||||
$this->actingAs($me)
|
||||
->get(route('auth.github.callback'))
|
||||
->assertRedirect(route('app.authentication.edit'))
|
||||
->assertSessionHas('flash.error');
|
||||
|
||||
expect($me->fresh()->github_id)->toBeNull();
|
||||
$this->assertAuthenticatedAs($me);
|
||||
});
|
||||
|
||||
test('authenticated callback connects google to the current user', function () {
|
||||
$user = User::factory()->create([
|
||||
'email' => 'me@example.com',
|
||||
'github_id' => 'gh-me',
|
||||
'google_id' => null,
|
||||
]);
|
||||
|
||||
$socialiteUser = new SocialiteUser;
|
||||
$socialiteUser->id = 'g-me';
|
||||
$socialiteUser->name = 'Me';
|
||||
$socialiteUser->email = 'me@example.com';
|
||||
|
||||
Socialite::shouldReceive('driver')
|
||||
->with('google-auth')
|
||||
->andReturn($driver = Mockery::mock());
|
||||
|
||||
$driver->shouldReceive('user')->andReturn($socialiteUser);
|
||||
|
||||
$this->actingAs($user)
|
||||
->get(route('auth.google.callback'))
|
||||
->assertRedirect(route('app.authentication.edit'))
|
||||
->assertSessionHas('flash.success');
|
||||
|
||||
expect($user->fresh()->google_id)->toBe('g-me');
|
||||
});
|
||||
|
||||
test('authenticated callback rejects when google account is already linked to another user', function () {
|
||||
User::factory()->create(['google_id' => 'g-taken']);
|
||||
|
||||
$me = User::factory()->create(['email' => 'me@example.com', 'google_id' => null]);
|
||||
|
||||
$socialiteUser = new SocialiteUser;
|
||||
$socialiteUser->id = 'g-taken';
|
||||
$socialiteUser->name = 'Me';
|
||||
$socialiteUser->email = 'me@example.com';
|
||||
|
||||
Socialite::shouldReceive('driver')
|
||||
->with('google-auth')
|
||||
->andReturn($driver = Mockery::mock());
|
||||
|
||||
$driver->shouldReceive('user')->andReturn($socialiteUser);
|
||||
|
||||
$this->actingAs($me)
|
||||
->get(route('auth.google.callback'))
|
||||
->assertRedirect(route('app.authentication.edit'))
|
||||
->assertSessionHas('flash.error');
|
||||
|
||||
expect($me->fresh()->google_id)->toBeNull();
|
||||
$this->assertAuthenticatedAs($me);
|
||||
});
|
||||
Loading…
Reference in a new issue