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 <bot@styleci.io>
Co-authored-by: Nabeel S <nabeelio@users.noreply.github.com>

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'            => '',
         ];
     }
 }

app/Database/migrations/2023_12_24_091030_drop_user_oauth_tokens_foreign_keys.php

@@ -0,0 +1,26 @@
+<?php
+
+use Illuminate\Database\Migrations\Migration;
+use Illuminate\Database\Schema\Blueprint;
+use Illuminate\Support\Facades\Schema;
+
+return new class() extends Migration {
+    public function up(): void
+    {
+        Schema::table('user_oauth_tokens', function (Blueprint $table) {
+            $foreignKeys = Schema::getForeignKeys('user_oauth_tokens');
+
+            foreach ($foreignKeys as $foreignKey) {
+                if (in_array('user_id', $foreignKey['columns'], true)) {
+                    $table->dropForeign(['user_id']);
+                    break;
+                }
+            }
+        });
+    }
+
+    public function down(): void
+    {
+        //
+    }
+};

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');

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,

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',

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());
+        }
+    }
 }

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
     ],

resources/views/layouts/default/auth/login_layout.blade.php

@@ -22,6 +22,7 @@
 
 <!-- End Navbar -->
 <div class="page-header">
+@include('flash::message')
 
   <div class="container">
     @yield('content')

resources/views/layouts/default/auth/register.blade.php

@@ -139,12 +139,6 @@
           </table>
 
           <div style="width: 100%; text-align: right; padding-top: 20px;">
-            @if(config('services.discord.enabled'))
-              <a href="{{ route('oauth.redirect', ['provider' => 'discord']) }}" class="btn" style="background-color:#738ADB;">
-                @lang('auth.loginwith', ['provider' => 'Discord'])
-              </a>
-            @endif
-
             {{ Form::submit(__('auth.register'), [
                 'id' => 'register_button',
                 'class' => 'btn btn-primary',

tests/OAuthTest.php

@@ -0,0 +1,253 @@
+<?php
+
+namespace Tests;
+
+use App\Models\Enums\UserState;
+use App\Models\User;
+use App\Models\UserOAuthToken;
+use Illuminate\Support\Facades\Auth;
+use Illuminate\Support\Facades\Config;
+use Laravel\Socialite\Facades\Socialite;
+use Mockery\LegacyMockInterface;
+use Mockery\MockInterface;
+use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
+
+class OAuthTest extends TestCase
+{
+    /** @var array|string[] The drivers we want to test */
+    protected array $drivers = ['discord'];
+
+    public function setUp(): void
+    {
+        parent::setUp();
+
+        foreach ($this->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);
+    }
+}