ENGCOM-9336: AdminSessionsManager and AdminSessionInfo - strtotime is…

…sue fix for admin login PR #34514
magento · Jan 31, 2022 · fa7788f · fa7788f
2 parents a629abe + fd191fe
commit fa7788f
Show file tree

Hide file tree

Showing 4 changed files with 99 additions and 19 deletions.
diff --git a/app/code/Magento/Security/Model/AdminSessionInfo.php b/app/code/Magento/Security/Model/AdminSessionInfo.php
@@ -47,6 +47,7 @@ class AdminSessionInfo extends \Magento\Framework\Model\AbstractModel
     /**
      * All other open sessions were terminated
      * @since 100.1.0
+     * @var bool
      */
     protected $isOtherSessionsTerminated = false;
 
@@ -133,10 +134,10 @@ public function isSessionExpired()
         $currentTime = $this->dateTime->gmtTimestamp();
         $lastUpdatedTime = $this->getUpdatedAt();
         if (!is_numeric($lastUpdatedTime)) {
-            $lastUpdatedTime = strtotime($lastUpdatedTime);
+            $lastUpdatedTime = $lastUpdatedTime === null ? 0 : strtotime($lastUpdatedTime);
         }
 
-        return $lastUpdatedTime <= ($currentTime - $lifetime) ? true : false;
+        return $lastUpdatedTime <= ($currentTime - $lifetime);
     }
 
     /**

diff --git a/app/code/Magento/Security/Model/AdminSessionsManager.php b/app/code/Magento/Security/Model/AdminSessionsManager.php
@@ -7,14 +7,18 @@
 
 namespace Magento\Security\Model;
 
+use Magento\Backend\Model\Auth\Session;
 use Magento\Framework\HTTP\PhpEnvironment\RemoteAddress;
+use Magento\Framework\Stdlib\DateTime;
+use Magento\Security\Model\ResourceModel\AdminSessionInfo\Collection;
 use Magento\Security\Model\ResourceModel\AdminSessionInfo\CollectionFactory;
 
 /**
  * Admin Sessions Manager Model
  *
  * @api
  * @since 100.1.0
+ * @SuppressWarnings(PHPMD.CookieAndSessionMisuse)
  */
 class AdminSessionsManager
 {
@@ -35,7 +39,7 @@ class AdminSessionsManager
     protected $securityConfig;
 
     /**
-     * @var \Magento\Backend\Model\Auth\Session
+     * @var Session
      * @since 100.1.0
      */
     protected $authSession;
@@ -73,20 +77,22 @@ class AdminSessionsManager
      *
      * Means that after session was prolonged
      * all other prolongs will be ignored within this period
+     *
+     * @var int
      */
     private $maxIntervalBetweenConsecutiveProlongs = 60;
 
     /**
      * @param ConfigInterface $securityConfig
-     * @param \Magento\Backend\Model\Auth\Session $authSession
+     * @param Session $authSession
      * @param AdminSessionInfoFactory $adminSessionInfoFactory
      * @param CollectionFactory $adminSessionInfoCollectionFactory
      * @param \Magento\Framework\Stdlib\DateTime\DateTime $dateTime
      * @param RemoteAddress $remoteAddress
      */
     public function __construct(
         ConfigInterface $securityConfig,
-        \Magento\Backend\Model\Auth\Session $authSession,
+        Session $authSession,
         \Magento\Security\Model\AdminSessionInfoFactory $adminSessionInfoFactory,
         \Magento\Security\Model\ResourceModel\AdminSessionInfo\CollectionFactory $adminSessionInfoCollectionFactory,
         \Magento\Framework\Stdlib\DateTime\DateTime $dateTime,
@@ -138,7 +144,7 @@ public function processProlong()
             $this->getCurrentSession()->setData(
                 'updated_at',
                 date(
-                    \Magento\Framework\Stdlib\DateTime::DATETIME_PHP_FORMAT,
+                    DateTime::DATETIME_PHP_FORMAT,
                     $this->authSession->getUpdatedAt()
                 )
             );
@@ -204,7 +210,7 @@ public function getLogoutReasonMessageByStatus($statusCode)
             case AdminSessionInfo::LOGGED_OUT_BY_LOGIN:
                 $reasonMessage = __(
                     'Someone logged into this account from another device or browser.'
-                    .' Your current session is terminated.'
+                    . ' Your current session is terminated.'
                 );
                 break;
             case AdminSessionInfo::LOGGED_OUT_MANUALLY:
@@ -241,7 +247,7 @@ public function getLogoutReasonMessage()
     /**
      * Get sessions for current user
      *
-     * @return \Magento\Security\Model\ResourceModel\AdminSessionInfo\Collection
+     * @return Collection
      * @since 100.1.0
      */
     public function getSessionsForCurrentUser()
@@ -314,7 +320,9 @@ protected function createNewSession()
     }
 
     /**
-     * @return \Magento\Security\Model\ResourceModel\AdminSessionInfo\Collection
+     * Retrieve new instance of admin session info collection
+     *
+     * @return Collection
      * @since 100.1.0
      */
     protected function createAdminSessionInfoCollection()
@@ -323,24 +331,27 @@ protected function createAdminSessionInfoCollection()
     }
 
     /**
-     * Calculates diff between now and last session updated_at
-     * and decides whether new prolong must be triggered or not
+     * Calculates diff between now and last session updated_at and decides whether new prolong must be triggered or not
      *
      * This is done to limit amount of session prolongs and updates to database
      * within some period of time - X
      * X - is calculated in getIntervalBetweenConsecutiveProlongs()
      *
-     * @see getIntervalBetweenConsecutiveProlongs()
      * @return bool
+     * @see getIntervalBetweenConsecutiveProlongs()
      */
     private function lastProlongIsOldEnough()
     {
-        $lastProlongTimestamp = strtotime($this->getCurrentSession()->getUpdatedAt());
+        $lastUpdatedTime = $this->getCurrentSession()->getUpdatedAt();
+        if ($lastUpdatedTime === null || is_numeric($lastUpdatedTime)) {
+            $lastUpdatedTime = "now";
+        }
+        $lastProlongTimestamp = strtotime($lastUpdatedTime);
         $nowTimestamp = $this->authSession->getUpdatedAt();
 
         $diff = $nowTimestamp - $lastProlongTimestamp;
 
-        return (float) $diff > $this->getIntervalBetweenConsecutiveProlongs();
+        return (float)$diff > $this->getIntervalBetweenConsecutiveProlongs();
     }
 
     /**
@@ -354,7 +365,7 @@ private function lastProlongIsOldEnough()
      */
     private function getIntervalBetweenConsecutiveProlongs()
     {
-        return (float) max(
+        return (float)max(
             1,
             min(
                 4 * log((float)$this->securityConfig->getAdminSessionLifetime()),

diff --git a/app/code/Magento/Security/Test/Unit/Model/AdminSessionInfoTest.php b/app/code/Magento/Security/Test/Unit/Model/AdminSessionInfoTest.php
@@ -127,6 +127,26 @@ public function dataProviderSessionLifetime()
         ];
     }
 
+    /**
+     * @return void
+     */
+    public function testSessionExpiredWhenUpdatedAtIsNull()
+    {
+        $timestamp = time();
+        $sessionLifetime = '1';
+
+        $this->securityConfigMock->expects($this->once())
+            ->method('getAdminSessionLifetime')
+            ->willReturn($sessionLifetime);
+
+        $this->dateTimeMock->expects($this->once())
+            ->method('gmtTimestamp')
+            ->willReturn($timestamp);
+
+        $this->model->setUpdatedAt(null);
+        $this->assertTrue($this->model->isSessionExpired());
+    }
+
     /**
      * @return void
      */

diff --git a/app/code/Magento/Security/Test/Unit/Model/AdminSessionsManagerTest.php b/app/code/Magento/Security/Test/Unit/Model/AdminSessionsManagerTest.php
@@ -58,9 +58,7 @@ class AdminSessionsManagerTest extends TestCase
     /** @var  ObjectManager */
     protected $objectManager;
 
-    /*
-     * @var RemoteAddress
-     */
+    /** @var RemoteAddress */
     protected $remoteAddressMock;
 
     /**
@@ -72,7 +70,15 @@ protected function setUp(): void
         $this->objectManager = new ObjectManager($this);
 
         $this->authSessionMock = $this->getMockBuilder(Session::class)
-            ->addMethods(['isActive', 'getStatus', 'getUser', 'getId', 'getUpdatedAt', 'getAdminSessionInfoId', 'setAdminSessionInfoId'])
+            ->addMethods([
+                'isActive',
+                'getStatus',
+                'getUser',
+                'getId',
+                'getUpdatedAt',
+                'getAdminSessionInfoId',
+                'setAdminSessionInfoId'
+            ])
             ->disableOriginalConstructor()
             ->getMock();
 
@@ -255,6 +261,48 @@ public function testProcessProlong()
         $this->model->processProlong();
     }
 
+    /**
+     * @return void
+     */
+    public function testUpdatedAtIsNull()
+    {
+        $newUpdatedAt = '2016-01-01 00:00:30';
+        $adminSessionInfoId = 50;
+        $this->authSessionMock->expects($this->any())
+            ->method('getAdminSessionInfoId')
+            ->willReturn($adminSessionInfoId);
+
+        $this->adminSessionInfoFactoryMock->expects($this->any())
+            ->method('create')
+            ->willReturn($this->currentSessionMock);
+
+        $this->currentSessionMock->expects($this->once())
+            ->method('load')
+            ->willReturnSelf();
+
+        $this->currentSessionMock->expects($this->once())
+            ->method('getUpdatedAt')
+            ->willReturn(null);
+
+        $this->authSessionMock->expects($this->once())
+            ->method('getUpdatedAt')
+            ->willReturn(strtotime($newUpdatedAt));
+
+        $this->securityConfigMock->expects($this->once())
+            ->method('getAdminSessionLifetime')
+            ->willReturn(100);
+
+        $this->currentSessionMock->expects($this->never())
+            ->method('setData')
+            ->willReturnSelf();
+
+        $this->currentSessionMock->expects($this->never())
+            ->method('save')
+            ->willReturnSelf();
+
+        $this->model->processProlong();
+    }
+
     /**
      * @return void
      */