From 80edbe8f38089a2d6330d851679fe4887052c5de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arthur=20Parient=C3=A9?= <41431456+arthurpar06@users.noreply.github.com> Date: Tue, 23 Jan 2024 23:47:51 +0100 Subject: [PATCH] OAuth improvements (#1735) * Do not create account if email already exists in OAuth * Remove DB constraints in Discord OAuth * Apply fixes from StyleCI * Add flash to login_layout * Fix logoutProvider for tests * Update OAuth Callback * Remove register with discord * Remove DISCORD_BOT_TOKEN * Add OAuthTest * Apply fixes from StyleCI * Update avatar in OAuthTest * Debug test * Revert "Debug test" This reverts commit ddef2f64b3f5c6e999202353fd6fcafc37091240. * Debug test * Apply fixes from StyleCI * Still trying to debug tests * Add avatar to UserFactory * Remove debug stuff * Update OAuthTest * Return discord_id in API * Check for UserState in OAuthController * Update OAuthTest * Apply fixes from StyleCI * Retrieve discord_private_channel_id * Apply fixes from StyleCI --------- Co-authored-by: StyleCI Bot Co-authored-by: Nabeel S --- app/Database/factories/UserFactory.php | 1 + ...30_drop_user_oauth_tokens_foreign_keys.php | 26 ++ app/Http/Controllers/Auth/OAuthController.php | 88 +++--- app/Http/Resources/User.php | 1 + app/Models/User.php | 1 - app/Services/UserService.php | 31 +++ config/services.php | 2 +- .../default/auth/login_layout.blade.php | 1 + .../layouts/default/auth/register.blade.php | 6 - tests/OAuthTest.php | 253 ++++++++++++++++++ 10 files changed, 370 insertions(+), 40 deletions(-) create mode 100644 app/Database/migrations/2023_12_24_091030_drop_user_oauth_tokens_foreign_keys.php create mode 100644 tests/OAuthTest.php diff --git a/app/Database/factories/UserFactory.php b/app/Database/factories/UserFactory.php index 33639a34..6abed468 100644 --- a/app/Database/factories/UserFactory.php +++ b/app/Database/factories/UserFactory.php @@ -50,6 +50,7 @@ class UserFactory extends Factory 'state' => UserState::ACTIVE, 'remember_token' => $this->faker->unique()->text(5), 'email_verified_at' => now(), + 'avatar' => '', ]; } } diff --git a/app/Database/migrations/2023_12_24_091030_drop_user_oauth_tokens_foreign_keys.php b/app/Database/migrations/2023_12_24_091030_drop_user_oauth_tokens_foreign_keys.php new file mode 100644 index 00000000..97008e34 --- /dev/null +++ b/app/Database/migrations/2023_12_24_091030_drop_user_oauth_tokens_foreign_keys.php @@ -0,0 +1,26 @@ +dropForeign(['user_id']); + break; + } + } + }); + } + + public function down(): void + { + // + } +}; diff --git a/app/Http/Controllers/Auth/OAuthController.php b/app/Http/Controllers/Auth/OAuthController.php index 0b3b78fb..2d8b6b15 100644 --- a/app/Http/Controllers/Auth/OAuthController.php +++ b/app/Http/Controllers/Auth/OAuthController.php @@ -3,13 +3,15 @@ namespace App\Http\Controllers\Auth; use App\Contracts\Controller; -use App\Models\Airline; -use App\Models\Airport; +use App\Models\Enums\UserState; use App\Models\User; use App\Models\UserOAuthToken; use App\Services\UserService; use Illuminate\Http\RedirectResponse; +use Illuminate\Http\Request; use Illuminate\Support\Facades\Auth; +use Illuminate\Support\Facades\Log; +use Illuminate\View\View; use Laravel\Socialite\Facades\Socialite; class OAuthController extends Controller @@ -37,7 +39,7 @@ class OAuthController extends Controller } } - public function handleProviderCallback(string $provider): RedirectResponse + public function handleProviderCallback(string $provider, Request $request): View|RedirectResponse { $providerUser = null; @@ -75,14 +77,51 @@ class OAuthController extends Controller 'last_refreshed_at' => now(), ]); + if ($provider === 'discord') { + $this->userSvc->retrieveDiscordPrivateChannelId($user); + } + flash()->success(ucfirst($provider).' account linked!'); return redirect(route('frontend.profile.index')); } - $user = User::where($provider.'_id', $providerUser->getId())->first(); + $user = User::where($provider.'_id', $providerUser->getId())->orWhere('email', $providerUser->getEmail())->first(); if ($user) { + $user->update([ + $provider.'_id' => $providerUser->getId(), + 'lastlogin_at' => now(), + ]); + + if (setting('general.record_user_ip', true)) { + $user->update([ + 'last_ip' => $request->ip(), + ]); + } + + // We don't want to log in a non-active user + if ($user->state !== UserState::ACTIVE && $user->state !== UserState::ON_LEAVE) { + Log::info('Trying to login '.$user->ident.', state '.UserState::label($user->state)); + + // Log them out + Auth::logout(); + $request->session()->invalidate(); + + // Redirect to one of the error pages + if ($user->state === UserState::PENDING) { + return view('auth.pending'); + } + + if ($user->state === UserState::REJECTED) { + return view('auth.rejected'); + } + + if ($user->state === UserState::SUSPENDED) { + return view('auth.suspended'); + } + } + $tokens = UserOAuthToken::updateOrCreate([ 'user_id' => $user->id, 'provider' => $provider, @@ -94,31 +133,15 @@ class OAuthController extends Controller Auth::login($user); + if ($provider === 'discord') { + $this->userSvc->retrieveDiscordPrivateChannelId($user); + } + return redirect(route('frontend.dashboard.index')); } - $attrs = [ - 'name' => $providerUser->getName(), - 'email' => $providerUser->getEmail(), - 'avatar' => $providerUser->getAvatar(), - 'airline_id' => Airline::select('id')->first()->id, - 'home_airport_id' => Airport::select('id')->where('hub', true)->first()->id, - $provider.'_id' => $providerUser->getId(), - ]; - - $user = $this->userSvc->createUser($attrs); - - UserOAuthToken::create([ - 'user_id' => $user->id, - 'provider' => $provider, - 'token' => $providerUser->token, - 'refresh_token' => $providerUser->refreshToken, - 'last_refreshed_at' => now(), - ]); - - Auth::login($user); - - return redirect(route('frontend.profile.edit', ['profile' => $user->id])); + flash()->error('No user linked to this account found. Please register first.'); + return redirect(url('/login')); } public function logoutProvider(string $provider): RedirectResponse @@ -130,15 +153,16 @@ class OAuthController extends Controller $user = Auth::user(); $otherProviders = UserOAuthToken::where('user_id', $user->id)->where('provider', '!=', $provider)->count(); - if (empty($user->password) && $otherProviders === 0) { - flash()->error('You cannot unlink your only login method!'); - return redirect()->route('frontend.profile.index'); - } - $user->update([ - $provider.'_id' => null, + $provider.'_id' => '', ]); + if ($provider === 'discord' && $user->discord_private_channel_id) { + $user->update([ + 'discord_private_channel_id' => '', + ]); + } + flash()->success(ucfirst($provider).' account unlinked!'); return redirect()->route('frontend.profile.index'); diff --git a/app/Http/Resources/User.php b/app/Http/Resources/User.php index 21f75211..e7b3b0c9 100644 --- a/app/Http/Resources/User.php +++ b/app/Http/Resources/User.php @@ -18,6 +18,7 @@ class User extends Resource 'name' => $this->name_private, 'name_private' => $this->name_private, 'avatar' => $this->resolveAvatarUrl(), + 'discord_id' => $this->discord_id, 'rank_id' => $this->rank_id, 'home_airport' => $this->home_airport_id, 'curr_airport' => $this->curr_airport_id, diff --git a/app/Models/User.php b/app/Models/User.php index 6995ed2d..59200e00 100755 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -123,7 +123,6 @@ class User extends Authenticatable implements LaratrustUser, MustVerifyEmail 'api_key', 'email', 'name', - 'discord_id', 'discord_private_channel_id', 'password', 'last_ip', diff --git a/app/Services/UserService.php b/app/Services/UserService.php index 06e66e83..1b9611ba 100644 --- a/app/Services/UserService.php +++ b/app/Services/UserService.php @@ -24,6 +24,8 @@ use App\Repositories\UserRepository; use App\Support\Units\Time; use App\Support\Utils; use Carbon\Carbon; +use GuzzleHttp\Client; +use GuzzleHttp\Exception\GuzzleException; use Illuminate\Auth\Events\Registered; use Illuminate\Support\Collection; use Illuminate\Support\Facades\Hash; @@ -602,4 +604,33 @@ class UserService extends Service return $user; } + + public function retrieveDiscordPrivateChannelId(User $user): void + { + if (is_null(config('services.discord.bot_token'))) { + return; + } + + try { + $httpClient = new Client(); + + $response = $httpClient->post('https://discord.com/api/users/@me/channels', [ + 'headers' => [ + 'Authorization' => 'Bot '.config('services.discord.bot_token'), + ], + 'json' => [ + 'recipient_id' => $user->discord_id, + ], + ]); + + $privateChannel = json_decode($response->getBody()->getContents(), true, 512, JSON_THROW_ON_ERROR)['id']; + $user->update([ + 'discord_private_channel_id' => $privateChannel, + ]); + } catch (\Exception $e) { + Log::error('Discord OAuth Error: '.$e->getMessage()); + } catch (GuzzleException $e) { + Log::error('Discord OAuth Error: '.$e->getMessage()); + } + } } diff --git a/config/services.php b/config/services.php index 562a99a1..07124bff 100755 --- a/config/services.php +++ b/config/services.php @@ -36,7 +36,7 @@ return [ 'redirect' => '/oauth/discord/callback', // optional - 'token' => env('DISCORD_BOT_TOKEN', null), + 'bot_token' => env('DISCORD_BOT_TOKEN', null), 'allow_gif_avatars' => (bool) env('DISCORD_AVATAR_GIF', true), 'avatar_default_extension' => env('DISCORD_EXTENSION_DEFAULT', 'png'), // only pick from jpg, png, webp ], diff --git a/resources/views/layouts/default/auth/login_layout.blade.php b/resources/views/layouts/default/auth/login_layout.blade.php index 1786b82f..9d19e7f7 100644 --- a/resources/views/layouts/default/auth/login_layout.blade.php +++ b/resources/views/layouts/default/auth/login_layout.blade.php @@ -22,6 +22,7 @@
+@include('flash::message')
@yield('content') diff --git a/resources/views/layouts/default/auth/register.blade.php b/resources/views/layouts/default/auth/register.blade.php index 5567741b..2b95cace 100644 --- a/resources/views/layouts/default/auth/register.blade.php +++ b/resources/views/layouts/default/auth/register.blade.php @@ -139,12 +139,6 @@
- @if(config('services.discord.enabled')) - - @lang('auth.loginwith', ['provider' => 'Discord']) - - @endif - {{ Form::submit(__('auth.register'), [ 'id' => 'register_button', 'class' => 'btn btn-primary', diff --git a/tests/OAuthTest.php b/tests/OAuthTest.php new file mode 100644 index 00000000..17b5e56c --- /dev/null +++ b/tests/OAuthTest.php @@ -0,0 +1,253 @@ +drivers as $driver) { + Config::set('services.'.$driver.'.enabled', true); + } + } + + /** + * Simulate what would be returned by the OAuth provider + * + * @return LegacyMockInterface|MockInterface + */ + protected function getMockedProvider(): LegacyMockInterface|MockInterface + { + $abstractUser = \Mockery::mock('Laravel\Socialite\Two\User') + ->allows([ + 'getId' => 123456789, + 'getName' => 'OAuth user', + 'getEmail' => 'oauth.user@phpvms.net', + 'getAvatar' => 'https://en.gravatar.com/userimage/12856995/aa6c0527a723abfd5fb9e246f0ff8af4.png', + ]); + + $abstractUser->token = 'token'; + $abstractUser->refreshToken = 'refresh_token'; + + return \Mockery::mock('Laravel\Socialite\Contracts\Provider') + ->allows([ + 'user' => $abstractUser, + ]); + } + + /** + * Try to link a logged-in user to an OAuth account from profile + * + * @return void + */ + public function testLinkAccountFromProfile(): void + { + $user = User::factory()->create([ + 'name' => 'OAuth user', + 'email' => 'oauth.user@phpvms.net', + ]); + Auth::login($user); + + foreach ($this->drivers as $driver) { + Socialite::shouldReceive('driver')->with($driver)->andReturn($this->getMockedProvider()); + + $this->get(route('oauth.callback', ['provider' => $driver])) + ->assertRedirect(route('frontend.profile.index')); + + $user->refresh(); + $this->assertEquals(123456789, $user->{$driver.'_id'}); + + $tokens = $user->oauth_tokens()->where('provider', $driver)->first(); + + $this->assertNotNull($tokens); + $this->assertEquals('token', $tokens->token); + $this->assertEquals('refresh_token', $tokens->refresh_token); + $this->assertTrue($tokens->last_refreshed_at->diffInSeconds(now()) <= 2); + } + } + + /** + * Try to link a non-logged-in user from the login page using its email + * + * @return void + */ + public function testLinkAccountFromLogin(): void + { + $user = User::factory()->create([ + 'name' => 'OAuth user', + 'email' => 'oauth.user@phpvms.net', + ]); + + foreach ($this->drivers as $driver) { + Socialite::shouldReceive('driver')->with($driver)->andReturn($this->getMockedProvider()); + + $this->get(route('oauth.callback', ['provider' => $driver])) + ->assertRedirect(route('frontend.dashboard.index')); + + $user->refresh(); + $this->assertEquals(123456789, $user->{$driver.'_id'}); + $this->assertTrue($user->lastlogin_at->diffInSeconds(now()) <= 2); + + $tokens = $user->oauth_tokens()->where('provider', $driver)->first(); + + $this->assertNotNull($tokens); + $this->assertEquals('token', $tokens->token); + $this->assertEquals('refresh_token', $tokens->refresh_token); + $this->assertTrue($tokens->last_refreshed_at->diffInSeconds(now()) <= 2); + } + } + + /** + * Try to log in an already linked user + * + * @return void + */ + public function testLoginWithLinkedAccount(): void + { + $user = User::factory()->create([ + 'name' => 'OAuth user', + 'email' => 'oauth.user@phpvms.net', + 'discord_id' => 123456789, + ]); + + foreach ($this->drivers as $driver) { + UserOAuthToken::create([ + 'user_id' => $user->id, + 'provider' => $driver, + 'token' => 'token', + 'refresh_token' => 'refresh_token', + 'last_refreshed_at' => now(), + ]); + + Socialite::shouldReceive('driver')->with($driver)->andReturn($this->getMockedProvider()); + + $this->get(route('oauth.callback', ['provider' => $driver])) + ->assertRedirect(route('frontend.dashboard.index')); + + $user->refresh(); + $this->assertEquals(123456789, $user->{$driver.'_id'}); + $this->assertTrue($user->lastlogin_at->diffInSeconds(now()) <= 2); + + $tokens = $user->oauth_tokens()->where('provider', $driver)->first(); + + $this->assertNotNull($tokens); + $this->assertEquals('token', $tokens->token); + $this->assertEquals('refresh_token', $tokens->refresh_token); + $this->assertTrue($tokens->last_refreshed_at->diffInSeconds(now()) <= 2); + } + } + + /** + * Try to log in a user with a pending account + * + * @return void + */ + public function testLoginWithPendingAccount(): void + { + $user = User::factory()->create([ + 'name' => 'OAuth user', + 'email' => 'oauth.user@phpvms.net', + 'state' => UserState::PENDING, + ]); + + foreach ($this->drivers as $driver) { + Socialite::shouldReceive('driver')->with($driver)->andReturn($this->getMockedProvider()); + + $this->get(route('oauth.callback', ['provider' => $driver])) + ->assertViewIs('auth.pending'); + } + } + + /** + * Try to log in someone not in DB + * + * @return void + */ + public function testNoAccountFound() + { + foreach ($this->drivers as $driver) { + Socialite::shouldReceive('driver')->with($driver)->andReturn($this->getMockedProvider()); + + $this->get(route('oauth.callback', ['provider' => $driver])) + ->assertRedirect(url('/login')); + } + } + + /** + * Try to unlink an account from profile + * + * @return void + */ + public function testUnlinkAccount(): void + { + $user = User::factory()->create([ + 'name' => 'OAuth user', + 'email' => 'oauth.user@phpvms.net', + ]); + + foreach ($this->drivers as $driver) { + $user->update([ + $driver.'_id' => 123456789, + ]); + + Auth::login($user); + + $this->get(route('oauth.logout', ['provider' => $driver])) + ->assertRedirect(route('frontend.profile.index')); + + $user->refresh(); + $this->assertEmpty($user->{$driver.'_id'}); + } + } + + /** + * Try to access a non-existing provider callback + * + * @return void + */ + public function testNonExistingProvider(): void + { + $this->expectException(NotFoundHttpException::class); + + $this->get(route('oauth.redirect', ['provider' => 'aze'])) + ->assertStatus(404); + + $this->get(route('oauth.callback', ['provider' => 'aze'])) + ->assertStatus(404); + } + + /** + * Try to access a disabled provider callback + * + * @return void + */ + public function testDisabledProvider(): void + { + $originalConfigValue = config('services.discord.enabled'); + Config::set('services.discord.enabled', false); + + $this->expectException(NotFoundHttpException::class); + + $this->get(route('oauth.redirect', ['provider' => 'discord'])) + ->assertStatus(404); + $this->get(route('oauth.callback', ['provider' => 'discord'])) + ->assertStatus(404); + + Config::set('services.discord.enabled', $originalConfigValue); + } +}