Social Login: Fixed queries unscoped to driver

Prevents the same IDs from different systems leading to access of other
accounts, and allows registration of same ids under different services.

Thanks to Google and Ada Logics for discovery and reporting.

.forgejo/SECURITY.md

@@ -8,18 +8,32 @@ since we aim to provide a fairly stable upgrade path for new versions.
 
 ## Security Notifications
 
-If you'd like to be notified of new potential security concerns you can [sign-up to the BookStack security mailing list](https://updates.bookstackapp.com/signup/bookstack-security-updates).
+If you'd like to be notified of new potential security concerns, you can [sign-up to the BookStack security mailing list](https://updates.bookstackapp.com/signup/bookstack-security-updates).
 
 ## Reporting a Vulnerability
 
-If you've found an issue that likely has no impact to existing users (For example, an issue only in the development branch)
+If you've found an issue that likely has no impact on existing users (For example, an issue only in the development branch)
 feel free to raise it via a standard Codeberg bug report issue.
 
-If the issue could have a security impact to BookStack instances, 
+If the issue could have a security impact on BookStack instances, 
 please directly contact the lead maintainer via email Dan Brown using the [details found here](https://www.bookstackapp.com/links/contact/).
 
-Please be patient while the vulnerability is being reviewed. Deploying the fix to address the vulnerability
-can often take a little time due to the amount of preparation required, to ensure the vulnerability has
-been covered, and to create the content required to adequately notify the user-base.
+When contacting us, please note any names (and optionally any profile/company/website links) that you'd like to be used in
+any attribution within our release notes and content.
 
-Thank you for keeping BookStack instances safe!
+Please be patient while the vulnerability is being reviewed. Deploying the fix to address the vulnerability
+can often take a little time due to the amount of preparation required to ensure the vulnerability has
+been covered and to create the content required to adequately notify the user-base.
+
+Thank you for keeping BookStack instances safe!
+
+### CVE Creation
+
+We're generally happy for (and prefer) researchers to raise CVEs for issues they've discovered.
+We ask that you first confirm with us to ensure the vulnerability is valid and that it hasn't yet been discovered
+and reported by someone else. 
+
+We can raise CVEs ourselves, but we would only go to the effort for security issues with a significant level of risk to users.
+We typically won't pursue CVEs if there's a lesser level of risk.
+
+_Note: Our reporting may change as the Cyber Resilience Act comes into effect._

app/Access/SocialAuthService.php

@@ -55,8 +55,10 @@ class SocialAuthService
      */
     public function handleRegistrationCallback(string $socialDriver, SocialUser $socialUser): SocialUser
     {
+        $socialDriver = trim(strtolower($socialDriver));
+
         // Check social account has not already been used
-        if (SocialAccount::query()->where('driver_id', '=', $socialUser->getId())->exists()) {
+        if ($this->getSocialAccountById($socialDriver, $socialUser->getId())) {
             throw new UserRegistrationException(trans('errors.social_account_in_use', ['socialAccount' => $socialDriver]), '/login');
         }
 
@@ -69,6 +71,19 @@ class SocialAuthService
         return $socialUser;
     }
 
+    /**
+     * Get an existing social account by its ID.
+     */
+    protected function getSocialAccountById(string $socialDriver, string $socialId): SocialAccount|null
+    {
+        $socialDriver = trim(strtolower($socialDriver));
+
+        return SocialAccount::query()
+            ->where('driver', '=', $socialDriver)
+            ->where('driver_id', '=', $socialId)
+            ->first();
+    }
+
     /**
      * Get the social user details via the social driver.
      *
@@ -93,7 +108,7 @@ class SocialAuthService
         $socialId = $socialUser->getId();
 
         // Get any attached social accounts or users
-        $socialAccount = SocialAccount::query()->where('driver_id', '=', $socialId)->first();
+        $socialAccount = $this->getSocialAccountById($socialDriver, $socialId);
         $isLoggedIn = auth()->check();
         $currentUser = user();
         $titleCaseDriver = Str::title($socialDriver);
@@ -153,7 +168,7 @@ class SocialAuthService
     public function newSocialAccount(string $socialDriver, SocialUser $socialUser): SocialAccount
     {
         return new SocialAccount([
-            'driver'    => $socialDriver,
+            'driver'    => trim(strtolower($socialDriver)),
             'driver_id' => $socialUser->getId(),
             'avatar'    => $socialUser->getAvatar(),
         ]);
@@ -164,6 +179,7 @@ class SocialAuthService
      */
     public function detachSocialAccount(string $socialDriver): void
     {
+        $socialDriver = trim(strtolower($socialDriver));
         user()->socialAccounts()->where('driver', '=', $socialDriver)->delete();
     }
 

tests/Auth/SocialAuthTest.php

@@ -3,6 +3,7 @@
 namespace Tests\Auth;
 
 use BookStack\Access\SocialAccount;
+use BookStack\Access\SocialAuthService;
 use BookStack\Activity\ActivityType;
 use BookStack\Users\Models\User;
 use Illuminate\Support\Facades\DB;
@@ -239,4 +240,60 @@ class SocialAuthTest extends TestCase
         $user = $user->whereEmail($user->email)->first();
         $this->assertDatabaseHas('social_accounts', ['user_id' => $user->id]);
     }
+
+    public function test_registration_allows_creation_with_id_which_already_used_by_another_user_and_service()
+    {
+        $user = $this->users->editor();
+        SocialAccount::factory()->create(['driver' => 'google', 'driver_id' => '123abc', 'user_id' => $user->id]);
+
+        $this->setSettings(['registration-enabled' => 'true']);
+        config(['GITHUB_APP_ID' => 'abc123', 'GITHUB_APP_SECRET' => '123abc']);
+        session()->put('social-callback', 'register');
+
+        $socialUser = new \Laravel\Socialite\One\User();
+        $socialUser->id = '123abc';
+        $socialUser->email = 'barrygithub@example.com';
+        $socialUser->name = 'BarryGitHub';
+        $socialUser->avatar = 'avatar_placeholder';
+
+        $serviceMock = $this->partialMockService(SocialAuthService::class);
+        $serviceMock->shouldReceive('getSocialUser')->andReturn($socialUser);
+
+        $resp = $this->get('/login/service/github/callback');
+        $resp->assertRedirect('/');
+
+        $this->assertTrue(auth()->check());
+        $this->assertDatabaseHas('social_accounts', [
+            'user_id' => user()->id,
+            'driver' => 'github',
+            'driver_id' => '123abc'
+        ]);
+        $this->assertDatabaseHas('social_accounts', [
+            'user_id' => $user->id,
+            'driver' => 'google',
+            'driver_id' => '123abc'
+        ]);
+    }
+
+    public function test_login_account_link_restrained_by_service()
+    {
+        config([
+            'GOOGLE_APP_ID' => 'abc123', 'GOOGLE_APP_SECRET' => '123abc',
+        ]);
+        $existingUser = $this->users->viewer();
+        SocialAccount::factory()->create(['driver' => 'github', 'driver_id' => 'logintest123', 'user_id' => $existingUser->id]);
+        session()->put('social-callback', 'login');
+
+        $socialUser = new \Laravel\Socialite\One\User();
+        $socialUser->id = 'logintest123';
+        $serviceMock = $this->partialMockService(SocialAuthService::class);
+        $serviceMock->shouldReceive('getSocialUser')->andReturn($socialUser);
+
+        $resp = $this->followingRedirects()->get('/login/service/google/callback');
+
+        $this->assertFalse(auth()->check());
+        $this->assertNotEquals($existingUser->id, user()->id);
+        $resp->assertSee('login-form');
+        $resp->assertSee(trans('errors.social_account_not_used', ['socialAccount' => 'Google']));
+    }
 }

tests/TestCase.php

@@ -17,6 +17,8 @@ use Illuminate\Support\Facades\File;
 use Illuminate\Support\Facades\Log;
 use Illuminate\Testing\Assert as PHPUnit;
 use Illuminate\Testing\Constraints\HasInDatabase;
+use Mockery;
+use Mockery\MockInterface;
 use Monolog\Handler\TestHandler;
 use Monolog\Logger;
 use Ssddanbrown\AssertHtml\TestsHtml;
@@ -106,6 +108,19 @@ abstract class TestCase extends BaseTestCase
         }
     }
 
+    /**
+     * Creates a partial mock of the given service class from a default
+     * container resolved instance. Compared to the default $this->partialMock function,
+     * this resolves mock of a service with its dependencies resolved from the container.
+     */
+    protected function partialMockService(string $service): MockInterface
+    {
+        $realService = app($service);
+        $serviceMock = Mockery::mock($realService)->makePartial();
+        $this->instance($service, $serviceMock);
+        return $serviceMock;
+    }
+
     /**
      * Mock the http client used in BookStack http calls.
      */