diff --git a/.env.example b/.env.example index 4e38edbf..5ec0da26 100644 --- a/.env.example +++ b/.env.example @@ -123,7 +123,10 @@ THREADS_CLIENT_SECRET= THREADS_CLIENT_REDIRECT="${APP_URL}/accounts/threads/callback" # Google (https://console.cloud.google.com) -# Used for YouTube social account connection AND Google login/signup +# Used for YouTube social account connection AND Google login/signup. +# Register BOTH callback URLs in the OAuth app's authorized redirect URIs: +# - ${APP_URL}/auth/google/callback (signup/login) +# - ${APP_URL}/settings/authentication/providers/google/callback (link from Settings) GOOGLE_AUTH_ENABLED=false GOOGLE_CLIENT_ID= GOOGLE_CLIENT_SECRET= @@ -131,7 +134,10 @@ GOOGLE_CLIENT_REDIRECT="${APP_URL}/accounts/youtube/callback" GOOGLE_AUTH_CALLBACK="${APP_URL}/auth/google/callback" # GitHub (https://github.com/settings/developers) -# Used for GitHub login/signup +# Used for GitHub login/signup. +# Register BOTH callback URLs in the OAuth app's authorized redirect URIs: +# - ${APP_URL}/auth/github/callback (signup/login) +# - ${APP_URL}/settings/authentication/providers/github/callback (link from Settings) GITHUB_AUTH_ENABLED=false GITHUB_CLIENT_ID= GITHUB_CLIENT_SECRET= diff --git a/app/Http/Controllers/App/Settings/AuthenticationController.php b/app/Http/Controllers/App/Settings/AuthenticationController.php index cb7d29be..7c74f22e 100644 --- a/app/Http/Controllers/App/Settings/AuthenticationController.php +++ b/app/Http/Controllers/App/Settings/AuthenticationController.php @@ -15,6 +15,7 @@ use Inertia\Inertia; use Inertia\Response; use Laravel\Socialite\Facades\Socialite; +use Laravel\Socialite\Two\AbstractProvider; class AuthenticationController extends Controller { @@ -68,9 +69,54 @@ public function connectProvider(string $provider): RedirectResponse { abort_unless(in_array($provider, self::PROVIDERS, true), 404); + return $this->driver($provider)->redirect(); + } + + public function connectProviderCallback(Request $request, string $provider): RedirectResponse + { + abort_unless(in_array($provider, self::PROVIDERS, true), 404); + + try { + $providerUser = $this->driver($provider)->user(); + } catch (\Exception) { + return redirect()->route('app.authentication.edit') + ->with('flash.error', __('settings.authentication.providers.flash_already_linked', ['provider' => ucfirst($provider)])); + } + + $user = $request->user(); + $column = "{$provider}_id"; + $providerId = (string) $providerUser->getId(); + + $existing = User::where($column, $providerId) + ->where('id', '!=', $user->id) + ->first(); + + if ($existing) { + return redirect()->route('app.authentication.edit') + ->with('flash.error', __('settings.authentication.providers.flash_already_linked', ['provider' => ucfirst($provider)])); + } + + if ($user->{$column} !== $providerId) { + $user->update([$column => $providerId]); + } + + return redirect()->route('app.authentication.edit') + ->with('flash.success', __('settings.authentication.providers.flash_connected', ['provider' => ucfirst($provider)])); + } + + /** + * Build a Socialite driver for the connect flow with its dedicated + * callback URL. The signup/login flow uses the driver's default + * redirect URL configured in `config/services.php`; here we override + * so each flow round-trips through its own route. + */ + private function driver(string $provider): AbstractProvider + { + $callback = route('app.authentication.connect-provider.callback', $provider); + return match ($provider) { - 'google' => Socialite::driver('google-auth')->redirect(), - 'github' => Socialite::driver('github')->scopes(['read:user', 'user:email'])->redirect(), + 'google' => Socialite::driver('google-auth')->redirectUrl($callback), + 'github' => Socialite::driver('github')->scopes(['read:user', 'user:email'])->redirectUrl($callback), }; } diff --git a/app/Http/Controllers/Auth/GitHubController.php b/app/Http/Controllers/Auth/GitHubController.php index 7e6e0014..05be8a6b 100644 --- a/app/Http/Controllers/Auth/GitHubController.php +++ b/app/Http/Controllers/Auth/GitHubController.php @@ -35,10 +35,6 @@ 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(); @@ -56,25 +52,6 @@ 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) { diff --git a/app/Http/Controllers/Auth/GoogleController.php b/app/Http/Controllers/Auth/GoogleController.php index ba69067e..822b7a8e 100644 --- a/app/Http/Controllers/Auth/GoogleController.php +++ b/app/Http/Controllers/Auth/GoogleController.php @@ -33,10 +33,6 @@ 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(); @@ -48,25 +44,6 @@ 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) { diff --git a/routes/app.php b/routes/app.php index 85dbafc1..1a1dad3d 100644 --- a/routes/app.php +++ b/routes/app.php @@ -251,6 +251,8 @@ ->name('app.authentication.destroy-other-sessions'); Route::get('settings/authentication/providers/{provider}/connect', [AuthenticationController::class, 'connectProvider']) ->name('app.authentication.connect-provider'); + Route::get('settings/authentication/providers/{provider}/callback', [AuthenticationController::class, 'connectProviderCallback']) + ->name('app.authentication.connect-provider.callback'); Route::delete('settings/authentication/providers/{provider}', [AuthenticationController::class, 'disconnectProvider']) ->name('app.authentication.disconnect-provider'); diff --git a/routes/auth.php b/routes/auth.php index 3e25ba03..1b79eb45 100644 --- a/routes/auth.php +++ b/routes/auth.php @@ -31,15 +31,11 @@ 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/github/redirect', [GitHubController::class, 'redirect'])->name('auth.github.redirect'); -}); + Route::get('/auth/google/callback', [GoogleController::class, 'callback'])->name('auth.google.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::get('/auth/github/redirect', [GitHubController::class, 'redirect'])->name('auth.github.redirect'); + 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'); diff --git a/tests/Feature/Auth/ConnectProviderTest.php b/tests/Feature/Auth/ConnectProviderTest.php index 262b97b1..00b0c3b1 100644 --- a/tests/Feature/Auth/ConnectProviderTest.php +++ b/tests/Feature/Auth/ConnectProviderTest.php @@ -4,13 +4,15 @@ use App\Models\User; use Laravel\Socialite\Facades\Socialite; +use Laravel\Socialite\Two\AbstractProvider; 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 = Mockery::mock(AbstractProvider::class); $driver->shouldReceive('scopes')->andReturnSelf(); + $driver->shouldReceive('redirectUrl')->andReturnSelf(); $driver->shouldReceive('redirect')->andReturn(redirect('https://github.com/login/oauth/authorize')); Socialite::shouldReceive('driver')->with('github')->andReturn($driver); @@ -22,7 +24,8 @@ test('authenticated user can hit the connect-provider route for google', function () { $user = User::factory()->create(); - $driver = Mockery::mock(); + $driver = Mockery::mock(AbstractProvider::class); + $driver->shouldReceive('redirectUrl')->andReturnSelf(); $driver->shouldReceive('redirect')->andReturn(redirect('https://accounts.google.com/o/oauth2/auth')); Socialite::shouldReceive('driver')->with('google-auth')->andReturn($driver); @@ -44,7 +47,7 @@ ->assertRedirect(route('login')); }); -test('authenticated callback connects github to the current user', function () { +test('connect-provider callback connects github to the current user', function () { $user = User::factory()->create([ 'email' => 'me@example.com', 'google_id' => 'g-me', @@ -56,21 +59,21 @@ $socialiteUser->name = 'Me'; $socialiteUser->email = 'me@example.com'; - Socialite::shouldReceive('driver') - ->with('github') - ->andReturn($driver = Mockery::mock()); - + $driver = Mockery::mock(AbstractProvider::class); + $driver->shouldReceive('scopes')->andReturnSelf(); + $driver->shouldReceive('redirectUrl')->andReturnSelf(); $driver->shouldReceive('user')->andReturn($socialiteUser); + Socialite::shouldReceive('driver')->with('github')->andReturn($driver); $this->actingAs($user) - ->get(route('auth.github.callback')) + ->get(route('app.authentication.connect-provider.callback', 'github')) ->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 () { +test('connect-provider callback links github by current user, not by email', function () { $user = User::factory()->create([ 'email' => 'work@example.com', 'google_id' => 'g-me', @@ -82,14 +85,14 @@ $socialiteUser->name = 'Me'; $socialiteUser->email = 'personal@example.com'; - Socialite::shouldReceive('driver') - ->with('github') - ->andReturn($driver = Mockery::mock()); - + $driver = Mockery::mock(AbstractProvider::class); + $driver->shouldReceive('scopes')->andReturnSelf(); + $driver->shouldReceive('redirectUrl')->andReturnSelf(); $driver->shouldReceive('user')->andReturn($socialiteUser); + Socialite::shouldReceive('driver')->with('github')->andReturn($driver); $this->actingAs($user) - ->get(route('auth.github.callback')) + ->get(route('app.authentication.connect-provider.callback', 'github')) ->assertRedirect(route('app.authentication.edit')) ->assertSessionHas('flash.success'); @@ -98,7 +101,7 @@ $this->assertAuthenticatedAs($user); }); -test('authenticated callback rejects when github account is already linked to another user', function () { +test('connect-provider 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]); @@ -108,14 +111,14 @@ $socialiteUser->name = 'Me'; $socialiteUser->email = 'me@example.com'; - Socialite::shouldReceive('driver') - ->with('github') - ->andReturn($driver = Mockery::mock()); - + $driver = Mockery::mock(AbstractProvider::class); + $driver->shouldReceive('scopes')->andReturnSelf(); + $driver->shouldReceive('redirectUrl')->andReturnSelf(); $driver->shouldReceive('user')->andReturn($socialiteUser); + Socialite::shouldReceive('driver')->with('github')->andReturn($driver); $this->actingAs($me) - ->get(route('auth.github.callback')) + ->get(route('app.authentication.connect-provider.callback', 'github')) ->assertRedirect(route('app.authentication.edit')) ->assertSessionHas('flash.error'); @@ -123,7 +126,7 @@ $this->assertAuthenticatedAs($me); }); -test('authenticated callback connects google to the current user', function () { +test('connect-provider callback connects google to the current user', function () { $user = User::factory()->create([ 'email' => 'me@example.com', 'github_id' => 'gh-me', @@ -135,21 +138,20 @@ $socialiteUser->name = 'Me'; $socialiteUser->email = 'me@example.com'; - Socialite::shouldReceive('driver') - ->with('google-auth') - ->andReturn($driver = Mockery::mock()); - + $driver = Mockery::mock(AbstractProvider::class); + $driver->shouldReceive('redirectUrl')->andReturnSelf(); $driver->shouldReceive('user')->andReturn($socialiteUser); + Socialite::shouldReceive('driver')->with('google-auth')->andReturn($driver); $this->actingAs($user) - ->get(route('auth.google.callback')) + ->get(route('app.authentication.connect-provider.callback', 'google')) ->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 () { +test('connect-provider 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]); @@ -159,17 +161,29 @@ $socialiteUser->name = 'Me'; $socialiteUser->email = 'me@example.com'; - Socialite::shouldReceive('driver') - ->with('google-auth') - ->andReturn($driver = Mockery::mock()); - + $driver = Mockery::mock(AbstractProvider::class); + $driver->shouldReceive('redirectUrl')->andReturnSelf(); $driver->shouldReceive('user')->andReturn($socialiteUser); + Socialite::shouldReceive('driver')->with('google-auth')->andReturn($driver); $this->actingAs($me) - ->get(route('auth.google.callback')) + ->get(route('app.authentication.connect-provider.callback', 'google')) ->assertRedirect(route('app.authentication.edit')) ->assertSessionHas('flash.error'); expect($me->fresh()->google_id)->toBeNull(); $this->assertAuthenticatedAs($me); }); + +test('connect-provider callback rejects unknown provider', function () { + $user = User::factory()->create(); + + $this->actingAs($user) + ->get(route('app.authentication.connect-provider.callback', 'twitter')) + ->assertNotFound(); +}); + +test('connect-provider callback requires authentication', function () { + $this->get(route('app.authentication.connect-provider.callback', 'github')) + ->assertRedirect(route('login')); +});