Merge pull request #109 from ADmad/identity-override

Prevent logged in user override.
ADmad · Oct 5, 2021 · 882aa06 · 882aa06
2 parents dbe863d + dfabb2a
commit 882aa06
Show file tree

Hide file tree

Showing 5 changed files with 178 additions and 36 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -70,7 +70,7 @@ jobs:
         php-version: '7.4'
         extensions: mbstring, intl
         coverage: none
-        tools: cs2pr, psalm:^4.7, phpstan:^0.12
+        tools: cs2pr, psalm:4.7, phpstan:0.12
 
     - name: Composer Install
       run: composer install

diff --git a/README.md b/README.md
@@ -268,17 +268,10 @@ class SocialAuthListener implements EventListenerInterface
      */
     public function beforeRedirect(EventInterface $event, $url, string $status, ServerRequest $request): void
     {
-        $messages = (array)$request->getSession()->read('Flash.flash');
-
         // Set flash message
         switch ($status) {
             case SocialAuthMiddleware::AUTH_STATUS_SUCCESS:
-                $messages[] = [
-                    'message' => __('You are now logged in'),
-                    'key' => 'flash',
-                    'element' => 'flash/success',
-                    'params' => [],
-                ];
+                $request->getFlash()->error('You are now logged in.');
                 break;
 
             // Auth through provider failed. Details will be logged in
@@ -289,16 +282,13 @@ class SocialAuthListener implements EventListenerInterface
             // user has been authenticated through provider but your finder has
             // a condition to not return an inactivated user.
             case SocialAuthMiddleware::AUTH_STATUS_FINDER_FAILURE:
-                $messages[] = [
-                    'message' => __('Authentication failed'),
-                    'key' => 'flash',
-                    'element' => 'flash/error',
-                    'params' => [],
-                ];
+                $request->getFlash()->error('Authentication failed.');
                 break;
-        }
 
-        $request->getSession()->write('Flash.flash', $messages);
+            case SocialAuthMiddleware::AUTH_STATUS_IDENTITY_MISMATCH:
+                $request->getFlash()->error('The social profile is already linked to another user.');
+                break;
+        }
 
         // You can return a modified redirect URL if needed.
     }

diff --git a/src/Middleware/SocialAuthMiddleware.php b/src/Middleware/SocialAuthMiddleware.php
@@ -96,6 +96,13 @@ class SocialAuthMiddleware implements MiddlewareInterface, EventDispatcherInterf
      */
     public const AUTH_STATUS_FINDER_FAILURE = 'finder_failure';
 
+    /**
+     * User associated with social profile mismatches with user record in session.
+     *
+     * @var string
+     */
+    public const AUTH_STATUS_IDENTITY_MISMATCH = 'identity_mismatch';
+
     /**
      * Default config.
      *
@@ -251,6 +258,7 @@ protected function _handleCallbackAction(ServerRequest $request): Response
         $config = $this->getConfig();
         $providerName = $request->getParam('provider');
         $response = new Response();
+        $session = $request->getSession();
 
         $profile = $this->_getProfile($providerName, $request);
         if (!$profile) {
@@ -261,7 +269,7 @@ protected function _handleCallbackAction(ServerRequest $request): Response
             );
         }
 
-        $user = $this->_getUser($profile, $request->getSession());
+        $user = $this->_getUser($profile, $session);
         if (!$user) {
             $redirectUrl = $this->_triggerBeforeRedirect($request, $config['loginUrl'], $this->_error);
 
@@ -282,7 +290,10 @@ protected function _handleCallbackAction(ServerRequest $request): Response
             $user = $user->toArray();
         }
 
-        $request->getSession()->write($config['sessionKey'], $user);
+        if (!$session->check($config['sessionKey'])) {
+            $session->renew();
+            $session->write($config['sessionKey'], $user);
+        }
 
         $redirectUrl = $this->_triggerBeforeRedirect($request, $this->_getRedirectUrl($request));
 
@@ -312,6 +323,36 @@ protected function _setupModelInstances(): void
      * @return \Cake\Datasource\EntityInterface|null
      */
     protected function _getProfile($providerName, ServerRequest $request): ?EntityInterface
+    {
+        $return = $this->_getSocialIdentity($providerName, $request);
+        if ($return === null) {
+            return null;
+        }
+
+        /** @var \Cake\Datasource\EntityInterface|null $profile */
+        $profile = $this->_profileModel->find()
+            ->where([
+                $this->_profileModel->aliasField('provider') => $providerName,
+                $this->_profileModel->aliasField('identifier') => $return['identity']->id,
+            ])
+            ->first();
+
+        return $this->_patchProfile(
+            $providerName,
+            $return['identity'],
+            $return['access_token'],
+            $profile ?: null
+        );
+    }
+
+    /**
+     * Get social identity.
+     *
+     * @param string $providerName Provider name.
+     * @param \Cake\Http\ServerRequest $request Request instance.
+     * @return array{identity: \SocialConnect\Common\Entity\User, access_token: \SocialConnect\Provider\AccessTokenInterface}|null
+     */
+    protected function _getSocialIdentity($providerName, ServerRequest $request): ?array
     {
         try {
             $provider = $this->_getService($request)->getProvider($providerName);
@@ -333,20 +374,7 @@ protected function _getProfile($providerName, ServerRequest $request): ?EntityIn
             return null;
         }
 
-        /** @var \Cake\Datasource\EntityInterface|null $profile */
-        $profile = $this->_profileModel->find()
-            ->where([
-                $this->_profileModel->aliasField('provider') => $providerName,
-                $this->_profileModel->aliasField('identifier') => $identity->id,
-            ])
-            ->first();
-
-        return $this->_patchProfile(
-            $providerName,
-            $identity,
-            $accessToken,
-            $profile ?: null
-        );
+        return ['identity' => $identity, 'access_token' => $accessToken];
     }
 
     /**
@@ -359,12 +387,27 @@ protected function _getProfile($providerName, ServerRequest $request): ?EntityIn
      */
     protected function _getUser(EntityInterface $profile, CakeSession $session): ?EntityInterface
     {
-        $user = null;
-
         /** @var string $userPkField */
         $userPkField = $this->_userModel->getPrimaryKey();
 
-        if ($profile->get('user_id')) {
+        $sessionUserId = $session->read($this->getConfig('sessionKey') . '.' . $userPkField);
+        $profileUserId = $profile->get('user_id');
+
+        // If the social profile is already connected to a user and that user
+        // is not the same as one currently set in session then it's an error.
+        if (
+            $sessionUserId &&
+            $profileUserId &&
+            $sessionUserId !== $profileUserId
+        ) {
+            $this->_error = self::AUTH_STATUS_IDENTITY_MISMATCH;
+
+            return null;
+        }
+
+        $user = null;
+
+        if ($profileUserId) {
             /** @var \Cake\Datasource\EntityInterface $user */
             $user = $this->_userModel->find()
                 ->where([

diff --git a/tests/Fixture/UsersFixture.php b/tests/Fixture/UsersFixture.php
@@ -0,0 +1,43 @@
+<?php
+declare(strict_types=1);
+
+namespace ADmad\SocialAuth\Test\Fixture;
+
+use Cake\TestSuite\Fixture\TestFixture;
+
+/**
+ * UsersFixture
+ */
+class UsersFixture extends TestFixture
+{
+    /**
+     * Fields
+     *
+     * @var array
+     */
+    // phpcs:disable
+    public $fields = [
+        'id' => ['type' => 'integer', 'length' => null, 'unsigned' => false, 'null' => false, 'default' => null, 'comment' => '', 'autoIncrement' => true, 'precision' => null],
+        'email' => ['type' => 'string', 'length' => 255, 'null' => true, 'default' => null, 'collate' => 'utf8mb4_unicode_ci', 'comment' => '', 'precision' => null],
+        '_constraints' => [
+            'primary' => ['type' => 'primary', 'columns' => ['id'], 'length' => []],
+        ],
+        '_options' => [
+            'engine' => 'InnoDB',
+            'collation' => 'utf8mb4_unicode_ci'
+        ],
+    ];
+    // phpcs:enable
+
+    /**
+     * Init method
+     *
+     * @return void
+     */
+    public function init(): void
+    {
+        $this->records = [];
+
+        parent::init();
+    }
+}
diff --git a/tests/TestCase/Middleware/SocialAuthMiddlewareTest.php b/tests/TestCase/Middleware/SocialAuthMiddlewareTest.php
@@ -4,10 +4,16 @@
 namespace ADmad\SocialAuth\Test\TestCase\Middleware;
 
 use ADmad\SocialAuth\Middleware\SocialAuthMiddleware;
+use ADmad\SocialAuth\Model\Entity\SocialProfile;
+use ADmad\SocialAuth\Model\Table\SocialProfilesTable;
+use Cake\Event\EventInterface;
+use Cake\Event\EventManager;
 use Cake\Http\Exception\MethodNotAllowedException;
 use Cake\Http\Response;
 use Cake\Http\ServerRequestFactory;
 use Cake\TestSuite\TestCase;
+use SocialConnect\Common\Entity\User;
+use SocialConnect\Provider\AccessTokenInterface;
 use SocialConnect\Provider\Session\Dummy;
 use TestApp\Http\TestRequestHandler;
 
@@ -17,6 +23,11 @@
  */
 class SocialAuthMiddlewareTest extends TestCase
 {
+    protected $fixtures = [
+        'plugin.ADmad/SocialAuth.Users',
+        'plugin.ADmad/SocialAuth.SocialProfiles',
+    ];
+
     public function setUp(): void
     {
         parent::setUp();
@@ -97,4 +108,59 @@ public function testLoginUrlException()
         $middleware = new SocialAuthMiddleware();
         $middleware->process($request, $this->handler);
     }
+
+    /**
+     * Test that IDENTITY_MISMATCH_ERROR occurs when a social profile is already
+     * associated with a user and that user does not match the user record set
+     * in the session.
+     *
+     * @return void
+     * @see https://github.com/ADmad/cakephp-social-auth/pull/108
+     */
+    public function testIdentityMismatchError()
+    {
+        $request = ServerRequestFactory::fromGlobals([
+            'REQUEST_URI' => '/social-auth/callback/facebook',
+        ]);
+        $request = $request->withAttribute('params', [
+            'plugin' => 'ADmad/SocialAuth',
+            'controller' => 'Auth',
+            'action' => 'callback',
+            'provider' => 'facebook',
+        ]);
+        $request->getSession()->write('Auth', ['id' => 1]);
+
+        $this->getTableLocator()->get(SocialProfilesTable::class)
+            ->save(new SocialProfile([
+                'user_id' => 2,
+                'provider' => 'facebook',
+                'identifier' => 'fbid',
+                'email' => '[email protected]',
+                'access_token' => '',
+            ]));
+
+        $middleware = $this->getMockBuilder(SocialAuthMiddleware::class)
+            ->onlyMethods(['_getSocialIdentity'])
+            ->getMock();
+
+        $accessToken = $this->getMockBuilder(AccessTokenInterface::class)
+            ->getMock();
+
+        $identity = new User();
+        $identity->id = 'fbid';
+        $identity->email = '[email protected]';
+
+        $middleware->expects($this->once())
+            ->method('_getSocialIdentity')
+            ->willReturn(['identity' => $identity, 'access_token' => $accessToken]);
+
+        EventManager::instance()->on(
+            SocialAuthMiddleware::EVENT_BEFORE_REDIRECT,
+            function (EventInterface $event, $url, string $status) {
+                $this->assertSame(SocialAuthMiddleware::AUTH_STATUS_IDENTITY_MISMATCH, $status);
+            }
+        );
+
+        $middleware->process($request, $this->handler);
+    }
 }