avoid matching by emails when provider cannot be trusted to verify emails

Author: jasonvarga

src/Http/Controllers/OAuthController.php

@@ -9,6 +9,7 @@
 use Statamic\Exceptions\NotFoundHttpException;
 use Statamic\Facades\OAuth;
 use Statamic\Facades\URL;
+use Statamic\Facades\User;
 use Statamic\Support\Arr;
 use Statamic\Support\Str;
 
@@ -50,7 +51,7 @@ public function handleProviderCallback(Request $request, string $provider)
             if (config('statamic.oauth.merge_user_data', true)) {
                 $user = $oauth->mergeUser($user, $providerUser);
             }
-        } elseif (config('statamic.oauth.create_user', true)) {
+        } elseif (config('statamic.oauth.create_user', true) && ! User::findByEmail($providerUser->getEmail())) {
             $user = $oauth->createUser($providerUser);
         }
 

src/OAuth/Provider.php

@@ -62,16 +62,24 @@ public function findOrCreateUser($socialite): StatamicUser
      */
     public function findUser($socialite): ?StatamicUser
     {
-        if (
-            ($user = User::findByOAuthId($this, $socialite->getId())) ||
-            ($user = User::findByEmail($socialite->getEmail()))
-        ) {
+        if ($user = User::findByOAuthId($this, $socialite->getId())) {
+            return $user;
+        }
+
+        if ($this->trustsEmails() && $user = User::findByEmail($socialite->getEmail())) {
             return $user;
         }
 
         return null;
     }
 
+    private function trustsEmails(): bool
+    {
+        return in_array($this->name, config('statamic.oauth.trusted_providers', [
+            'google', 'github', 'apple', 'bitbucket', 'slack', 'slack-openid', 'twitter-oauth-2',
+        ]));
+    }
+
     /**
      * Create a Statamic user from a Socialite user.
      *

tests/OAuth/OAuthCallbackTest.php

@@ -0,0 +1,95 @@
+<?php
+
+namespace Tests\OAuth;
+
+use Mockery;
+use PHPUnit\Framework\Attributes\Test;
+use Statamic\Facades\OAuth;
+use Statamic\Facades\User as UserFacade;
+use Statamic\OAuth\Provider;
+use Tests\PreventSavingStacheItemsToDisk;
+use Tests\TestCase;
+
+class OAuthCallbackTest extends TestCase
+{
+    use PreventSavingStacheItemsToDisk;
+
+    protected function defineEnvironment($app)
+    {
+        $app['config']->set('statamic.oauth.enabled', true);
+        $app['config']->set('statamic.oauth.providers', ['evil', 'google']);
+    }
+
+    public function tearDown(): void
+    {
+        app('files')->deleteDirectory(storage_path('statamic/oauth'));
+
+        parent::tearDown();
+    }
+
+    private function fakeProvider(string $name)
+    {
+        $socialiteUser = new FakeSocialiteUser();
+
+        $provider = Mockery::mock(Provider::class.'[getSocialiteUser]', [$name, []]);
+        $provider->shouldReceive('getSocialiteUser')->andReturn($socialiteUser);
+
+        OAuth::partialMock()->shouldReceive('provider')->with($name)->andReturn($provider);
+    }
+
+    #[Test]
+    public function an_untrusted_provider_does_not_overwrite_or_log_into_an_existing_account()
+    {
+        config(['statamic.oauth.trusted_providers' => ['google']]); // 'evil' is untrusted
+        config(['statamic.oauth.create_user' => true]);
+
+        $admin = UserFacade::make()->email('admin@target.tld')->data(['name' => 'Admin'])->makeSuper()->save();
+
+        $this->fakeProvider('evil');
+
+        $response = $this->get('/oauth/evil/callback');
+
+        // No login happened, and they were sent to the unauthorized redirect.
+        $this->assertGuest();
+        $response->assertRedirect();
+
+        // The existing super admin is untouched and no duplicate was created.
+        $this->assertCount(1, UserFacade::all());
+        $admin = $admin->fresh();
+        $this->assertTrue($admin->isSuper());
+        $this->assertEquals('Admin', $admin->name());
+    }
+
+    #[Test]
+    public function a_trusted_provider_logs_into_the_existing_account()
+    {
+        config(['statamic.oauth.trusted_providers' => ['google']]);
+
+        $admin = UserFacade::make()->email('admin@target.tld')->data(['name' => 'Admin'])->makeSuper()->save();
+
+        $this->fakeProvider('google');
+
+        $this->get('/oauth/google/callback');
+
+        $this->assertAuthenticatedAs($admin->fresh());
+        $this->assertCount(1, UserFacade::all());
+    }
+}
+
+class FakeSocialiteUser
+{
+    public function getId()
+    {
+        return 'attacker-1';
+    }
+
+    public function getName()
+    {
+        return 'Mallory';
+    }
+
+    public function getEmail()
+    {
+        return 'admin@target.tld';
+    }
+}

tests/OAuth/ProviderTest.php

@@ -22,6 +22,8 @@ public function setUp(): void
             'driver' => 'local',
             'root' => $this->tempDir = __DIR__.'/tmp',
         ]]);
+
+        config(['statamic.oauth.trusted_providers' => ['test']]);
     }
 
     public function tearDown(): void
@@ -155,6 +157,35 @@ public function it_does_not_find_or_create_a_user_via_find_user_method()
         $this->assertNull($user);
     }
 
+    #[Test]
+    public function it_does_not_find_an_existing_user_by_email_for_an_untrusted_provider()
+    {
+        config(['statamic.oauth.trusted_providers' => ['google']]);
+
+        $this->user()->save();
+
+        $foundUser = (new Provider('test'))->findUser($this->socialite());
+
+        $this->assertNull($foundUser);
+    }
+
+    #[Test]
+    public function it_still_finds_an_existing_user_by_oauth_id_for_an_untrusted_provider()
+    {
+        config(['statamic.oauth.trusted_providers' => ['google']]);
+
+        $provider = new Provider('test');
+        $savedUser = $this->user()->save();
+
+        // Link the account to the provider, then ensure a subsequent lookup matches
+        // on the stored OAuth id even though email matching is not trusted.
+        $provider->mergeUser($savedUser, $this->socialite());
+
+        $foundUser = $provider->findUser($this->socialite());
+
+        $this->assertEquals($savedUser, $foundUser);
+    }
+
     #[Test]
     public function it_finds_an_existing_user_via_find_or_create_user_method()
     {