From 2a3dfe4117b65226893f28ac7bd7698125db0d95 Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Mon, 14 Sep 2026 18:18:19 -0300 Subject: [PATCH] fix: connect Google from settings without hitting the guest login route The Connect button used /auth/google/login, which is the sign-in start. A signed-in user hitting that URL either bounced off guest middleware or landed on a 404. Settings now has its own connect route, and the OAuth callback links the account when you are already signed in. --- .../Controllers/Auth/SocialAuthController.php | 44 +++++++------ .../Setting/AuthenticationController.php | 13 +++- .../js/pages/Setting/Authentication/Index.vue | 18 ++++-- routes/auth.php | 32 +++++----- routes/web.php | 4 ++ tests/Feature/Auth/ConnectProviderTest.php | 62 +++++++++++++++++++ tests/Feature/Auth/SocialAuthTest.php | 27 +++++--- tests/Feature/Setting/AuthenticationTest.php | 21 +++++++ 8 files changed, 171 insertions(+), 50 deletions(-) create mode 100644 tests/Feature/Auth/ConnectProviderTest.php diff --git a/app/Http/Controllers/Auth/SocialAuthController.php b/app/Http/Controllers/Auth/SocialAuthController.php index a17183d2..9ffded72 100644 --- a/app/Http/Controllers/Auth/SocialAuthController.php +++ b/app/Http/Controllers/Auth/SocialAuthController.php @@ -11,6 +11,7 @@ use App\Http\Controllers\Auth\Concerns\PreservesAttributionParameters; use App\Models\User; use Illuminate\Auth\Events\Registered; +use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; use Inertia\Inertia; use Laravel\Socialite\Facades\Socialite; @@ -30,11 +31,6 @@ public function redirectToProvider(Request $request, string $provider) // Carry campaign parameters across the bounce out to the provider. $this->storeAttributionParameters($request); - // An authenticated user is linking, not signing in. - if ($request->user()) { - $request->session()->put('social_connect', $socialProvider->value); - } - return Inertia::location( Socialite::driver($socialProvider->value)->redirect()->getTargetUrl(), ); @@ -52,21 +48,10 @@ public function handleProviderCallback(Request $request, string $provider) $column = $socialProvider->column(); - // Linking an extra provider to the account already signed in. - if ($request->session()->pull('social_connect') === $socialProvider->value && $request->user()) { - if (LinkSocialAccount::isTaken($socialProvider, $socialUser->getId(), $request->user())) { - session()->flash('flash.banner', "That {$socialProvider->label()} account is already linked to another user."); - session()->flash('flash.bannerStyle', 'danger'); - - return redirect(route('setting.authentication.edit')); - } - - LinkSocialAccount::execute($request->user(), $socialProvider, $socialUser->getId()); - - session()->flash('flash.banner', "{$socialProvider->label()} connected."); - session()->flash('flash.bannerStyle', 'success'); - - return redirect(route('setting.authentication.edit')); + // `guest` middleware gates login/signup; `auth` gates the settings + // connect flow. The callback is reachable by both and branches here. + if ($request->user()) { + return $this->connectToCurrentUser($request->user(), $socialProvider, $socialUser->getId()); } $existingUser = User::where($column, $socialUser->getId()) @@ -99,6 +84,25 @@ public function handleProviderCallback(Request $request, string $provider) return redirect(route('links.index')); } + private function connectToCurrentUser(User $user, SocialAuthProvider $socialProvider, string $providerId): RedirectResponse + { + if (LinkSocialAccount::isTaken($socialProvider, $providerId, $user)) { + session()->flash('flash.banner', "That {$socialProvider->label()} account is already linked to another user."); + session()->flash('flash.bannerStyle', 'danger'); + + return redirect(route('setting.authentication.edit')); + } + + if ($user->{$socialProvider->column()} !== $providerId) { + LinkSocialAccount::execute($user, $socialProvider, $providerId); + } + + session()->flash('flash.banner', "{$socialProvider->label()} connected."); + session()->flash('flash.bannerStyle', 'success'); + + return redirect(route('setting.authentication.edit')); + } + private function provider(string $provider): SocialAuthProvider { $socialProvider = SocialAuthProvider::tryFrom($provider); diff --git a/app/Http/Controllers/Setting/AuthenticationController.php b/app/Http/Controllers/Setting/AuthenticationController.php index 1efc6219..aab2f9d7 100644 --- a/app/Http/Controllers/Setting/AuthenticationController.php +++ b/app/Http/Controllers/Setting/AuthenticationController.php @@ -17,6 +17,7 @@ use Illuminate\Support\Facades\DB; use Inertia\Inertia; use Inertia\Response; +use Laravel\Socialite\Facades\Socialite; class AuthenticationController extends Controller { @@ -53,6 +54,15 @@ public function destroyOtherSessions(DestroySessionsRequest $request): RedirectR return back(); } + public function connectProvider(string $provider): RedirectResponse + { + $socialProvider = SocialAuthProvider::tryFrom($provider); + + abort_unless($socialProvider?->isEnabled(), 404); + + return Socialite::driver($socialProvider->value)->redirect(); + } + public function disconnectProvider(Request $request, string $provider): RedirectResponse { $socialProvider = SocialAuthProvider::tryFrom($provider); @@ -75,7 +85,7 @@ public function disconnectProvider(Request $request, string $provider): Redirect } /** - * @return array + * @return array */ private function connectedAccounts(User $user): array { @@ -84,6 +94,7 @@ private function connectedAccounts(User $user): array 'provider' => $provider->value, 'label' => $provider->label(), 'connected' => filled($user->{"{$provider->value}_id"}), + 'enabled' => $provider->isEnabled(), ])->values()->all(); } diff --git a/resources/js/pages/Setting/Authentication/Index.vue b/resources/js/pages/Setting/Authentication/Index.vue index 808a3530..15283f93 100644 --- a/resources/js/pages/Setting/Authentication/Index.vue +++ b/resources/js/pages/Setting/Authentication/Index.vue @@ -9,15 +9,18 @@ import { Input } from '@/components/ui/input'; import { Label } from '@/components/ui/label'; import AppLayout from '@/layouts/AppLayout.vue'; import SettingsLayout from '@/layouts/settings/Layout.vue'; -import { social } from '@/routes/auth'; import { password as passwordRoute } from '@/routes/setting/authentication'; -import { destroy as destroyProvider } from '@/routes/setting/authentication/providers'; +import { + connect as connectProvider, + destroy as destroyProvider, +} from '@/routes/setting/authentication/providers'; import { destroy as destroySessions } from '@/routes/setting/authentication/sessions'; type ConnectedAccount = { provider: string; label: string; connected: boolean; + enabled: boolean; }; type Session = { @@ -213,8 +216,15 @@ const disconnect = (provider: string) => { > Disconnect - diff --git a/routes/auth.php b/routes/auth.php index 431ae4c7..cd448431 100644 --- a/routes/auth.php +++ b/routes/auth.php @@ -42,6 +42,12 @@ function () { Route::get('/invites/{id}', [InviteController::class, 'show'])->name('auth.invites.show'); Route::post('/invites/{id}', [InviteController::class, 'accept'])->name('auth.invites.accept'); + // Sign-in / register only. Connecting a provider to an account that + // is already signed in goes through settings (see connectProvider). + Route::get('/auth/{provider}/login', [SocialAuthController::class, 'redirectToProvider']) + ->whereIn('provider', ['google', 'github']) + ->name('auth.social'); + } ); @@ -66,20 +72,14 @@ function () { ); /** - * Social sign-in, reachable signed in as well as out: signed out it signs you - * in or registers you, signed in it links the provider to the account you are - * already using. These sat inside the guest group, so the Connect button on the - * authentication settings screen bounced off the middleware and never arrived. + * Callbacks must be reachable by both guests (signup/login) and authenticated + * users (connect-from-settings). The routes that start the OAuth round-trip + * enforce the right middleware, so the callback can branch on Auth::check(). + * + * Under /auth, which is what config/services.php already told each provider + * to redirect to. At the root these took two first-level segments, so + * `google` and `github` were reserved as short-link back-halves for no reason. */ -Route::middleware('web')->group(function () { - // Under /auth, which is what config/services.php already told each - // provider to redirect to. At the root these took two first-level - // segments, so `google` and `github` were reserved as short-link - // back-halves for no reason. - Route::get('/auth/{provider}/login', [SocialAuthController::class, 'redirectToProvider']) - ->whereIn('provider', ['google', 'github']) - ->name('auth.social'); - Route::get('/auth/{provider}/callback', [SocialAuthController::class, 'handleProviderCallback']) - ->whereIn('provider', ['google', 'github']) - ->name('auth.social.callback'); -}); +Route::get('/auth/{provider}/callback', [SocialAuthController::class, 'handleProviderCallback']) + ->whereIn('provider', ['google', 'github']) + ->name('auth.social.callback'); diff --git a/routes/web.php b/routes/web.php index 0e61f1f8..39460e02 100644 --- a/routes/web.php +++ b/routes/web.php @@ -70,6 +70,10 @@ function () { Route::get('/authentication', [AuthenticationController::class, 'edit'])->name('setting.authentication.edit')->withoutMiddleware(['set-store']); Route::put('/authentication/password', [AuthenticationController::class, 'updatePassword'])->name('setting.authentication.password')->withoutMiddleware(['set-store']); Route::delete('/authentication/sessions', [AuthenticationController::class, 'destroyOtherSessions'])->name('setting.authentication.sessions.destroy')->withoutMiddleware(['set-store']); + Route::get('/authentication/providers/{provider}/connect', [AuthenticationController::class, 'connectProvider']) + ->whereIn('provider', ['google', 'github']) + ->name('setting.authentication.providers.connect') + ->withoutMiddleware(['set-store']); Route::delete('/authentication/providers/{provider}', [AuthenticationController::class, 'disconnectProvider'])->name('setting.authentication.providers.destroy')->withoutMiddleware(['set-store']); // mcp diff --git a/tests/Feature/Auth/ConnectProviderTest.php b/tests/Feature/Auth/ConnectProviderTest.php new file mode 100644 index 00000000..c57f7d58 --- /dev/null +++ b/tests/Feature/Auth/ConnectProviderTest.php @@ -0,0 +1,62 @@ + true, + 'lua.auth.github' => true, + 'services.google.client_id' => 'id', + 'services.google.client_secret' => 'secret', + 'services.google.redirect' => 'https://lua.test/auth/google/callback', + 'services.github.client_id' => 'id', + 'services.github.client_secret' => 'secret', + 'services.github.redirect' => 'https://lua.test/auth/github/callback', + ]); +}); + +it('sends a signed-in user to google from the settings connect route', function () { + $driver = Mockery::mock(AbstractProvider::class); + $driver->shouldReceive('redirect')->andReturn(redirect('https://accounts.google.com/o/oauth2/auth')); + Socialite::shouldReceive('driver')->with('google')->andReturn($driver); + + $this->actingAs(User::factory()->withWorkspace()->create()) + ->get(route('setting.authentication.providers.connect', 'google')) + ->assertRedirect('https://accounts.google.com/o/oauth2/auth'); +}); + +it('sends a signed-in user to github from the settings connect route', function () { + $driver = Mockery::mock(AbstractProvider::class); + $driver->shouldReceive('redirect')->andReturn(redirect('https://github.com/login/oauth/authorize')); + Socialite::shouldReceive('driver')->with('github')->andReturn($driver); + + $this->actingAs(User::factory()->withWorkspace()->create()) + ->get(route('setting.authentication.providers.connect', 'github')) + ->assertRedirect('https://github.com/login/oauth/authorize'); +}); + +it('refuses an unknown provider on the settings connect route', function () { + $this->actingAs(User::factory()->withWorkspace()->create()) + ->get(route('setting.authentication.providers.connect', 'twitter')) + ->assertNotFound(); +}); + +it('404s the settings connect route when the provider is off', function () { + config(['lua.auth.github' => false]); + + $this->actingAs(User::factory()->withWorkspace()->create()) + ->get(route('setting.authentication.providers.connect', 'github')) + ->assertNotFound(); +}); + +it('asks a guest to sign in before connecting a provider', function () { + $this->get(route('setting.authentication.providers.connect', 'google')) + ->assertRedirect(route('login')); +}); diff --git a/tests/Feature/Auth/SocialAuthTest.php b/tests/Feature/Auth/SocialAuthTest.php index b4734635..8cb9c0e3 100644 --- a/tests/Feature/Auth/SocialAuthTest.php +++ b/tests/Feature/Auth/SocialAuthTest.php @@ -47,15 +47,10 @@ function fakeSocialite(?SocialiteUser $user): void $this->get(route('auth.social', 'google'))->assertRedirect(); }); -it('remembers you were linking when you were already signed in', function () { - fakeSocialite(fakeSocialUser('g-1', 'ada@example.com')); - - // The Connect button on the authentication screen used to bounce off the - // guest middleware and never reach this at all. +it('sends a signed-in user away from the guest login start', function () { $this->actingAs(User::factory()->withWorkspace()->create()) ->get(route('auth.social', 'google')) - ->assertRedirect() - ->assertSessionHas('social_connect', 'google'); + ->assertRedirect('/links'); }); it('refuses a provider we do not offer', function () { @@ -117,7 +112,6 @@ function fakeSocialite(?SocialiteUser $user): void fakeSocialite(fakeSocialUser('g-1', 'someone@example.com')); $this->actingAs($user) - ->withSession(['social_connect' => 'google']) ->get(route('auth.social.callback', 'google')) ->assertRedirect(route('setting.authentication.edit')); @@ -131,13 +125,28 @@ function fakeSocialite(?SocialiteUser $user): void fakeSocialite(fakeSocialUser('g-1', 'someone@example.com')); $this->actingAs($user) - ->withSession(['social_connect' => 'google']) ->get(route('auth.social.callback', 'google')) ->assertRedirect(route('setting.authentication.edit')); expect($user->fresh()->google_id)->toBeNull(); }); +it('links the provider to the signed-in user even when the emails differ', function () { + $user = User::factory()->withWorkspace()->create([ + 'email' => 'work@example.com', + 'google_id' => null, + ]); + + fakeSocialite(fakeSocialUser('g-personal', 'personal@example.com')); + + $this->actingAs($user) + ->get(route('auth.social.callback', 'google')) + ->assertRedirect(route('setting.authentication.edit')); + + expect($user->fresh()->google_id)->toBe('g-personal') + ->and(User::where('email', 'personal@example.com')->exists())->toBeFalse(); +}); + it('falls back to the nickname when the provider has no name', function () { fakeSocialite(fakeSocialUser('g-1', 'ada@example.com', name: null)); diff --git a/tests/Feature/Setting/AuthenticationTest.php b/tests/Feature/Setting/AuthenticationTest.php index f4ee6bc7..85b74b99 100644 --- a/tests/Feature/Setting/AuthenticationTest.php +++ b/tests/Feature/Setting/AuthenticationTest.php @@ -28,6 +28,27 @@ actingAs($this->user)->get(route('setting.authentication.edit'))->assertOk(); }); +it('tells the screen which social providers can be connected', function () { + config([ + 'lua.auth.google' => true, + 'lua.auth.github' => false, + 'services.google.client_id' => 'id', + 'services.google.client_secret' => 'secret', + ]); + + actingAs($this->user) + ->get(route('setting.authentication.edit')) + ->assertOk() + ->assertInertia(fn (Assert $page) => $page + ->has('connectedAccounts', 2) + ->where('connectedAccounts.0.provider', 'google') + ->where('connectedAccounts.0.enabled', true) + ->where('connectedAccounts.1.provider', 'github') + ->where('connectedAccounts.1.enabled', false) + ->etc() + ); +}); + it('updates the password when the current one is right', function () { actingAs($this->user) ->put(route('setting.authentication.password'), [