Refactor observers, update tests (#22833: Short-term admin accounts)

Author: lfolco

app/code/Magento/Security/Model/Plugin/UserValidationRules.php

@@ -28,6 +28,8 @@ public function __construct(\Magento\Security\Model\UserExpiration\Validator $va
     }
 
     /**
+     * Add the Expires At validator to user validation rules.
+     *
      * @param \Magento\User\Model\UserValidationRules $userValidationRules
      * @param \Magento\Framework\Validator\DataObject $result
      * @return \Magento\Framework\Validator\DataObject

app/code/Magento/Security/Model/ResourceModel/UserExpiration.php

@@ -21,6 +21,8 @@ class UserExpiration extends \Magento\Framework\Model\ResourceModel\Db\AbstractD
     protected $_isPkAutoIncrement = false;
 
     /**
+     * Define main table
+     *
      * @return void
      */
     protected function _construct()

app/code/Magento/Security/Model/ResourceModel/UserExpiration/Collection.php

@@ -18,6 +18,8 @@ class Collection extends \Magento\Framework\Model\ResourceModel\Db\Collection\Ab
     protected $_idFieldName = 'user_id';
 
     /**
+     * Initialize collection
+     *
      * @return void
      */
     protected function _construct()
@@ -53,7 +55,8 @@ public function addActiveExpiredUsersFilter($now = null): Collection
 
     /**
      * Filter collection by user id.
-     * @param array $userIds
+     *
+     * @param int[] $userIds
      * @return Collection
      */
     public function addUserIdsFilter($userIds = []): Collection
@@ -64,12 +67,12 @@ public function addUserIdsFilter($userIds = []): Collection
     /**
      * Get any expired records for the given user.
      *
-     * @param $userId
+     * @param string $userId
      * @return Collection
      */
-    public function addExpiredRecordsForUserFilter($userId): Collection
+    public function addExpiredRecordsForUserFilter(string $userId): Collection
     {
         return $this->addActiveExpiredUsersFilter()
-            ->addFieldToFilter('main_table.user_id', $userId);
+            ->addFieldToFilter('main_table.user_id', (int)$userId);
     }
 }

app/code/Magento/Security/Model/UserExpirationManager.php

@@ -90,6 +90,7 @@ public function deactivateExpiredUsers(?array $userIds = null): void
             $currentSessions = $this->adminSessionInfoCollectionFactory->create()
                 ->addFieldToFilter('user_id', ['in' => $expiredRecords->getAllIds()])
                 ->filterExpiredSessions($this->securityConfig->getAdminSessionLifetime());
+            /** @var \Magento\Security\Model\AdminSessionInfo $currentSession */
             $currentSessions->setDataToAll('status', \Magento\Security\Model\AdminSessionInfo::LOGGED_OUT)
                 ->save();
         }
@@ -107,12 +108,14 @@ public function deactivateExpiredUsers(?array $userIds = null): void
     /**
      * Check if the given user is expired.
      * // TODO: check users expired an hour ago (timezone stuff)
+     *
      * @param \Magento\User\Model\User $user
      * @return bool
      */
     public function userIsExpired(\Magento\User\Model\User $user): bool
     {
         $isExpired = false;
+        /** @var \Magento\Security\Model\UserExpiration $expiredRecord */
         $expiredRecord = $this->userExpirationCollectionFactory->create()
             ->addExpiredRecordsForUserFilter($user->getId())
             ->getFirstItem();

app/code/Magento/Security/Observer/BeforeAdminUserAuthenticate.php renamed to app/code/Magento/Security/Observer/AdminUserAuthenticateBefore.php

@@ -13,11 +13,11 @@
 use Magento\Security\Model\UserExpirationManager;
 
 /**
- * Class BeforeAdminUserAuthenticate
+ * Check for expired users.
  *
  * @package Magento\Security\Observer
  */
-class BeforeAdminUserAuthenticate implements ObserverInterface
+class AdminUserAuthenticateBefore implements ObserverInterface
 {
     /**
      * @var UserExpirationManager
@@ -29,6 +29,12 @@ class BeforeAdminUserAuthenticate implements ObserverInterface
      */
     private $user;
 
+    /**
+     * AdminUserAuthenticateBefore constructor.
+     *
+     * @param UserExpirationManager $userExpirationManager
+     * @param \Magento\User\Model\User $user
+     */
     public function __construct(
         \Magento\Security\Model\UserExpirationManager $userExpirationManager,
         \Magento\User\Model\User $user
@@ -38,6 +44,8 @@ public function __construct(
     }
 
     /**
+     * Check for expired user when logging in.
+     *
      * @param Observer $observer
      * @return void
      * @throws AuthenticationException

app/code/Magento/Security/Observer/AfterAdminUserLoad.php

@@ -25,6 +25,12 @@ class AfterAdminUserLoad implements ObserverInterface
      */
     private $userExpirationResource;
 
+    /**
+     * AfterAdminUserLoad constructor.
+     *
+     * @param \Magento\Security\Model\UserExpirationFactory $userExpirationFactory
+     * @param \Magento\Security\Model\ResourceModel\UserExpiration $userExpirationResource
+     */
     public function __construct(
         \Magento\Security\Model\UserExpirationFactory $userExpirationFactory,
         \Magento\Security\Model\ResourceModel\UserExpiration $userExpirationResource
@@ -35,6 +41,8 @@ public function __construct(
     }
 
     /**
+     * Set the user expiration date onto the user.
+     *
      * @param Observer $observer
      * @return void
      */

app/code/Magento/Security/Observer/AfterAdminUserSave.php

@@ -25,6 +25,12 @@ class AfterAdminUserSave implements ObserverInterface
      */
     private $userExpirationResource;
 
+    /**
+     * AfterAdminUserSave constructor.
+     *
+     * @param \Magento\Security\Model\UserExpirationFactory $userExpirationFactory
+     * @param \Magento\Security\Model\ResourceModel\UserExpiration $userExpirationResource
+     */
     public function __construct(
         \Magento\Security\Model\UserExpirationFactory $userExpirationFactory,
         \Magento\Security\Model\ResourceModel\UserExpiration $userExpirationResource
@@ -35,6 +41,8 @@ public function __construct(
     }
 
     /**
+     * Save user expiration.
+     *
      * @param Observer $observer
      * @return void
      */

app/code/Magento/Security/Test/Unit/Observer/BeforeAdminUserAuthenticateTest.php

@@ -8,7 +8,7 @@
 namespace Magento\Security\Test\Unit\Observer;
 
 /**
- * Test for \Magento\Security\Observer\BeforeAdminUserAuthenticate
+ * Test for \Magento\Security\Observer\AdminUserAuthenticateBefore
  *
  * @package Magento\Security\Test\Unit\Observer
  */
@@ -30,7 +30,7 @@ class BeforeAdminUserAuthenticateTest extends \PHPUnit\Framework\TestCase
     private $userMock;
 
     /**
-     * @var \Magento\Security\Observer\BeforeAdminUserAuthenticate
+     * @var \Magento\Security\Observer\AdminUserAuthenticateBefore
      */
     private $observer;
 
@@ -64,7 +64,7 @@ protected function setUp()
         );
         $this->userMock = $this->createPartialMock(\Magento\User\Model\User::class, ['loadByUsername', 'getId']);
         $this->observer = $this->objectManager->getObject(
-            \Magento\Security\Observer\BeforeAdminUserAuthenticate::class,
+            \Magento\Security\Observer\AdminUserAuthenticateBefore::class,
             [
                 'userExpirationManager' => $this->userExpirationManagerMock,
                 'user' => $this->userMock,

app/code/Magento/Security/etc/crontab.xml

@@ -13,7 +13,7 @@
         <job name="security_clean_password_reset_request_event_records" instance="Magento\Security\Model\SecurityManager" method="cleanExpiredRecords">
             <schedule>0 0 * * *</schedule>
         </job>
-        <job name="security_expire_users" instance="Magento\Security\Model\UserExpirationManager" method="deactivateExpiredUsers">
+        <job name="security_deactivate_expired_users" instance="Magento\Security\Model\UserExpirationManager" method="deactivateExpiredUsers">
             <schedule>0 * * * *</schedule>
         </job>
     </group>

app/code/Magento/Security/etc/events.xml

@@ -14,6 +14,6 @@
         <observer name="add_user_expiration" instance="Magento\Security\Observer\AfterAdminUserLoad"/>
     </event>
     <event name="admin_user_authenticate_before">
-        <observer name="check_user_expiration" instance="Magento\Security\Observer\BeforeAdminUserAuthenticate"/>
+        <observer name="check_user_expiration" instance="Magento\Security\Observer\AdminUserAuthenticateBefore"/>
     </event>
 </config>

dev/tests/integration/testsuite/Magento/Security/Model/Plugin/AuthSessionTest.php

@@ -142,6 +142,9 @@ public function testProcessProlong()
     }
 
     /**
+     * Test processing prolong with an expired user.
+     *
+     * @magentoDbIsolation enabled
      */
     public function testProcessProlongWithExpiredUser()
     {

dev/tests/integration/testsuite/Magento/Security/Model/ResourceModel/UserExpiration/CollectionTest.php

@@ -38,25 +38,39 @@ protected function setUp()
     /**
      * @magentoDataFixture Magento/Security/_files/expired_users.php
      */
-    public function testExpiredActiveUsersFilter()
+    public function testAddExpiredActiveUsersFilter()
     {
         /** @var \Magento\Security\Model\ResourceModel\UserExpiration\Collection $collectionModel */
         $collectionModel = $this->collectionModelFactory->create();
         $collectionModel->addActiveExpiredUsersFilter();
-        static::assertGreaterThanOrEqual(1, $collectionModel->getSize());
+        static::assertEquals(1, $collectionModel->getSize());
     }
 
     /**
      * @magentoDataFixture Magento/Security/_files/expired_users.php
      */
-    public function testGetExpiredRecordsForUser()
+    public function testAddUserIdsFilter()
+    {
+        $adminUserNameFromFixture = 'adminUserExpired';
+        $user = $this->objectManager->create(\Magento\User\Model\User::class);
+        $user->loadByUsername($adminUserNameFromFixture);
+
+        /** @var \Magento\Security\Model\ResourceModel\UserExpiration\Collection $collectionModel */
+        $collectionModel = $this->collectionModelFactory->create()->addUserIdsFilter([$user->getId()]);
+        static::assertEquals(1, $collectionModel->getSize());
+    }
+
+    /**
+     * @magentoDataFixture Magento/Security/_files/expired_users.php
+     */
+    public function testAddExpiredRecordsForUserFilter()
     {
         $adminUserNameFromFixture = 'adminUserExpired';
         $user = $this->objectManager->create(\Magento\User\Model\User::class);
         $user->loadByUsername($adminUserNameFromFixture);
 
         /** @var \Magento\Security\Model\ResourceModel\UserExpiration\Collection $collectionModel */
         $collectionModel = $this->collectionModelFactory->create()->addExpiredRecordsForUserFilter($user->getId());
-        static::assertGreaterThanOrEqual(1, $collectionModel->getSize());
+        static::assertEquals(1, $collectionModel->getSize());
     }
 }