MC-30109: Deleting an empty user model caused deleting admin role

Author: zakdma

app/code/Magento/User/Model/ResourceModel/User.php

@@ -13,6 +13,7 @@
 use Magento\Authorization\Model\UserContextInterface;
 use Magento\Framework\Acl\Data\CacheInterface;
 use Magento\Framework\App\ObjectManager;
+use Magento\Framework\Exception\LocalizedException;
 use Magento\User\Model\Backend\Config\ObserverConfig;
 use Magento\User\Model\User as ModelUser;
 
@@ -249,29 +250,33 @@ protected function _afterLoad(\Magento\Framework\Model\AbstractModel $user)
      *
      * @param \Magento\Framework\Model\AbstractModel $user
      * @return bool
-     * @throws \Magento\Framework\Exception\LocalizedException
+     * @throws LocalizedException
      */
     public function delete(\Magento\Framework\Model\AbstractModel $user)
     {
+        $uid = $user->getId();
+        if (!$uid) {
+            return false;
+        }
+
         $this->_beforeDelete($user);
         $connection = $this->getConnection();
-
-        $uid = $user->getId();
         $connection->beginTransaction();
-        if ($uid) {
-            try {
-                $connection->delete($this->getMainTable(), ['user_id = ?' => $uid]);
-                $connection->delete(
-                    $this->getTable('authorization_role'),
-                    ['user_id = ?' => $uid, 'user_type = ?' => UserContextInterface::USER_TYPE_ADMIN]
-                );
-            } catch (\Magento\Framework\Exception\LocalizedException $e) {
-                $connection->rollBack();
-                return false;
-            }
+        try {
+            $connection->delete($this->getMainTable(), ['user_id = ?' => $uid]);
+            $connection->delete(
+                $this->getTable('authorization_role'),
+                ['user_id = ?' => $uid, 'user_type = ?' => UserContextInterface::USER_TYPE_ADMIN]
+            );
+        } catch (LocalizedException $e) {
+            $connection->rollBack();
+
+            return false;
         }
+
         $connection->commit();
         $this->_afterDelete($user);
+
         return true;
     }
 

dev/tests/integration/testsuite/Magento/User/Model/ResourceModel/UserTest.php

@@ -3,37 +3,57 @@
  * Copyright © Magento, Inc. All rights reserved.
  * See COPYING.txt for license details.
  */
+declare(strict_types=1);
+
 namespace Magento\User\Model\ResourceModel;
 
 use Magento\Authorization\Model\ResourceModel\Role\Collection as UserRoleCollection;
+use Magento\Authorization\Model\ResourceModel\Role\CollectionFactory as UserRoleCollectionFactory;
 use Magento\Authorization\Model\UserContextInterface;
 use Magento\TestFramework\Helper\Bootstrap;
 use Magento\User\Model\ResourceModel\User as UserResourceModel;
 use Magento\User\Model\User;
+use Magento\User\Model\UserFactory;
 
 /**
  * @magentoAppArea adminhtml
  */
 class UserTest extends \PHPUnit\Framework\TestCase
 {
-    /** @var UserResourceModel */
+    /**
+     * @var UserResourceModel
+     */
     private $model;
 
+    /**
+     * @var UserRoleCollectionFactory
+     */
+    private $userRoleCollectionFactory;
+
+    /**
+     * @var UserFactory
+     */
+    private $userFactory;
+
+    /**
+     * @inheritdoc
+     */
     protected function setUp()
     {
-        $this->model = Bootstrap::getObjectManager()->get(
-            UserResourceModel::class
-        );
+        $this->model = Bootstrap::getObjectManager()->get(UserResourceModel::class);
+        $this->userRoleCollectionFactory = Bootstrap::getObjectManager()->get(UserRoleCollectionFactory::class);
+        $this->userFactory = Bootstrap::getObjectManager()->get(UserFactory::class);
     }
 
     /**
      * Tests if latest password is stored after user creating
      * when password lifetime config value is zero (disabled as fact)
      *
+     * @return void
      * @magentoConfigFixture current_store admin/security/password_lifetime 0
      * @magentoDataFixture Magento/User/_files/dummy_user.php
      */
-    public function testGetLatestPasswordWhenZeroPasswordLifetime()
+    public function testGetLatestPasswordWhenZeroPasswordLifetime(): void
     {
         /** @var User $user */
         $user = Bootstrap::getObjectManager()->create(
@@ -49,38 +69,48 @@ public function testGetLatestPasswordWhenZeroPasswordLifetime()
     }
 
     /**
-     * Test that user role is not deleted after deleting empty user
+     * Test that user role is not deleted after deleting empty user.
+     *
+     * @return void
      */
-    public function testDelete()
+    public function testDelete(): void
     {
         $this->checkRoleCollectionSize();
         /** @var User $user */
-        $user = Bootstrap::getObjectManager()->create(
-            User::class
-        );
+        $user = $this->userFactory->create();
         $this->model->delete($user);
         $this->checkRoleCollectionSize();
     }
 
     /**
-     * Ensure that role collection size is correct
+     * Ensure that role collection size is correct.
+     *
+     * @return void
      */
-    private function checkRoleCollectionSize()
+    private function checkRoleCollectionSize(): void
     {
         /** @var UserRoleCollection $roleCollection */
-        $roleCollection = Bootstrap::getObjectManager()->create(
-            UserRoleCollection::class
-        );
+        $roleCollection = $this->userRoleCollectionFactory->create();
         $roleCollection->setUserFilter(0, UserContextInterface::USER_TYPE_ADMIN);
         $this->assertEquals(1, $roleCollection->getSize());
     }
 
-    public function testCountAll()
+    /**
+     * Check total user count.
+     *
+     * @return void
+     */
+    public function testCountAll(): void
     {
         $this->assertSame(1, $this->model->countAll());
     }
 
-    public function testGetValidationRulesBeforeSave()
+    /**
+     * Check validation rules has correct type.
+     *
+     * @return void
+     */
+    public function testGetValidationRulesBeforeSave(): void
     {
         $rules = $this->model->getValidationRulesBeforeSave();
         $this->assertInstanceOf('Zend_Validate_Interface', $rules);