OXDEV-8351 Add proper error handling for duplicate emails

OXID-eSales · Nov 11, 2024 · dbae3a5 · dbae3a5
1 parent ea2dbd3
commit dbae3a5
Show file tree

Hide file tree

Showing 4 changed files with 58 additions and 63 deletions.
diff --git a/source/Application/Component/UserComponent.php b/source/Application/Component/UserComponent.php
@@ -644,69 +644,68 @@ protected function saveDeliveryAddressState()
      */
     protected function changeUserWithoutRedirect()
     {
-        $session = Registry::getSession();
-
-        if (!$session->checkSessionChallenge()) {
+        if (!Registry::getSession()->checkSessionChallenge()) {
             return;
         }
 
         $user = $this->getUser();
         if (!$user) {
             return;
         }
+
+        $currentEmail = $user->getFieldData('oxusername');
+        $password = $user->getFieldData('oxpassword');
         $shippingAddress = $this->getShippingAddress();
         $billingAddress = $this->getBillingAddress();
 
-        $username = $user->getFieldData('oxusername');
-        $password = $user->getFieldData('oxpassword');
+        $newEmail = $billingAddress['oxuser__oxusername'] ?? '';
+        $isEmailChanged = ($newEmail !== '' && $newEmail !== $currentEmail);
+
+        if ($isEmailChanged && $user->isEmailInUse($newEmail)) {
+            Registry::getUtilsView()->addErrorToDisplay('ERROR_MESSAGE_USER_USEREXISTS');
+            return false;
+        }
+
         try {
-            $newName = $billingAddress['oxuser__oxusername'] ?? '';
-            if (
-                $this->isGuestUser($user)
-                && $this->isUserNameUpdated($user->oxuser__oxusername->value ?? '', $newName)
-            ) {
-                $this->deleteExistingGuestUser($newName);
+            if ($isEmailChanged && $this->isGuestUser($user)) {
+                $this->deleteExistingGuestUser($newEmail);
             }
-            $user->changeUserData($username, $password, $password, $billingAddress, $shippingAddress);
+
+            $user->changeUserData($currentEmail, $password, $password, $billingAddress, $shippingAddress);
 
             $isSubscriptionRequested = Registry::getRequest()->getRequestEscapedParameter('blnewssubscribed');
-            $userSubscriptionStatus = $isSubscriptionRequested
-                ?? $user->getNewsSubscription()->getOptInStatus();
-            // check if email address changed, if so, force check newsletter subscription settings.
-            $billingUsername = $billingAddress['oxuser__oxusername'] ?? null;
-            $forceSubscriptionCheck = ($billingUsername !== null && $billingUsername !== $username);
+            $userSubscriptionStatus = $isSubscriptionRequested ?? $user->getNewsSubscription()->getOptInStatus();
             $isSubscriptionEmailRequested = Registry::getConfig()->getConfigParam('blOrderOptInEmail');
+
             $this->_blNewsSubscriptionStatus = $user->setNewsSubscription(
                 $userSubscriptionStatus,
                 $isSubscriptionEmailRequested,
-                $forceSubscriptionCheck
+                $isEmailChanged
             );
+
+            $this->resetPermissions();
+
+            $orderRemark = Registry::getRequest()->getRequestParameter('order_remark', true);
+            $session = Registry::getSession();
+            if ($orderRemark) {
+                $session->setVariable('ordrem', $orderRemark);
+            } else {
+                $session->deleteVariable('ordrem');
+            }
+
+            if ($basket = $session->getBasket()) {
+                $basket->setBasketUser(null);
+                $basket->onUpdate();
+            }
+
+            return true;
         } catch (UserException | ConnectionException | InputException $exception) {
             Registry::getUtilsView()->addErrorToDisplay($exception, false, true);
-
             return;
         } catch (\Throwable) {
             Registry::getUtilsView()->addErrorToDisplay('ERROR_MESSAGE_USER_UPDATE_FAILED', false, true);
             return false;
         }
-
-        $this->resetPermissions();
-
-        // order remark
-        $orderRemark = Registry::getRequest()->getRequestParameter('order_remark', true);
-
-        if ($orderRemark) {
-            $session->setVariable('ordrem', $orderRemark);
-        } else {
-            $session->deleteVariable('ordrem');
-        }
-
-        if ($basket = $session->getBasket()) {
-            $basket->setBasketUser(null);
-            $basket->onUpdate();
-        }
-
-        return true;
     }
 
     /**

diff --git a/source/Application/Controller/Admin/UserMain.php b/source/Application/Controller/Admin/UserMain.php
@@ -7,6 +7,7 @@
 
 namespace OxidEsales\EshopCommunity\Application\Controller\Admin;
 
+use OxidEsales\Eshop\Application\Model\User;
 use OxidEsales\Eshop\Core\Registry;
 use stdClass;
 use Exception;
@@ -26,7 +27,7 @@ public function render()
         parent::render();
 
         // malladmin stuff
-        $oAuthUser = oxNew(\OxidEsales\Eshop\Application\Model\User::class);
+        $oAuthUser = oxNew(User::class);
         $oAuthUser->loadAdminUser();
         $blisMallAdmin = $oAuthUser->oxuser__oxrights->value == "malladmin";
 
@@ -52,7 +53,7 @@ public function render()
         $soxId = $this->_aViewData["oxid"] = $this->getEditObjectId();
         if (isset($soxId) && $soxId != "-1") {
             // load object
-            $oUser = oxNew(\OxidEsales\Eshop\Application\Model\User::class);
+            $oUser = oxNew(User::class);
             $oUser->load($soxId);
             $this->_aViewData["edit"] = $oUser;
 
@@ -102,50 +103,42 @@ public function save()
     {
         parent::save();
 
-        //allow admin information edit only for MALL admins
         $soxId = $this->getEditObjectId();
         if ($this->allowAdminEdit($soxId)) {
             $aParams = Registry::getRequest()->getRequestEscapedParameter("editval");
 
-            // checkbox handling
             if (!isset($aParams['oxuser__oxactive'])) {
                 $aParams['oxuser__oxactive'] = 0;
             }
 
-            $oUser = oxNew(\OxidEsales\Eshop\Application\Model\User::class);
+            $oUser = oxNew(User::class);
             if ($soxId != "-1") {
                 $oUser->load($soxId);
             } else {
                 $aParams['oxuser__oxid'] = null;
             }
 
-            //setting new password
             if (($sNewPass = Registry::getRequest()->getRequestEscapedParameter("newPassword"))) {
                 $oUser->setPassword($sNewPass);
             }
 
-            //FS#2167 V checks for already used email
-
-            if (isset($aParams['oxuser__oxusername']) && $oUser->checkIfEmailReallyExists($aParams['oxuser__oxusername'])) {
+            if (isset($aParams['oxuser__oxusername']) && $oUser->isEmailInUse($aParams['oxuser__oxusername'])) {
                 $this->_sSaveError = 'EXCEPTION_USER_USEREXISTS';
 
                 return;
             }
 
             $oUser->assign($aParams);
 
-            //seting shop id for ONLY for new created user
             if ($soxId == "-1") {
                 $this->onUserCreation($oUser);
             }
 
-            // A. changing field type to save birth date correctly
             $oUser->oxuser__oxbirthdate->fldtype = 'char';
 
             try {
                 $oUser->save();
 
-                // set oxid if inserted
                 $this->setEditObjectId($oUser->getId());
             } catch (Exception $oExcp) {
                 $this->_sSaveError = $oExcp->getMessage();
@@ -168,9 +161,9 @@ protected function calculateAdditionalRights($userRights)
     /**
      * Additional actions on user creation.
      *
-     * @param \OxidEsales\Eshop\Application\Model\User $user
+     * @param User $user
      *
-     * @return \OxidEsales\Eshop\Application\Model\User
+     * @return User
      */
     protected function onUserCreation($user)
     {

diff --git a/source/Application/Model/User.php b/source/Application/Model/User.php
@@ -1724,6 +1724,9 @@ protected function update()
      * @param string $sEmail user email/login
      *
      * @return bool
+     *
+     * @deprecated since v7.3.0 (2024-11-11); Use isEmailInUse() instead.
+     *              This method will be removed in v8.0
      */
     public function checkIfEmailExists($sEmail)
     {
@@ -1770,26 +1773,26 @@ public function checkIfEmailExists($sEmail)
         return $blExists;
     }
 
-    public function checkIfEmailReallyExists(string $email): bool
+    public function isEmailInUse(string $email): bool
     {
         $queryBuilder = ContainerFacade::get(QueryBuilderFactoryInterface::class)->create();
 
-        $result = $queryBuilder
-            ->select('COUNT(*)')
+        $query = $queryBuilder
+            ->select('1')
             ->from('oxuser')
             ->where('oxusername = :email')
             ->setParameter(':email', $email);
 
-        // If we have an ID (editing existing user), exclude the current user
-        if ($currentId = $this->getId()) {
-            $result
-                ->andWhere('oxid != :currentId')
-                ->setParameter(':currentId', $currentId);
+
+        $userId = $this->getId();
+        if ($userId) {
+            $query
+                ->andWhere('oxid != :excludeUserId')
+                ->setParameter(':excludeUserId', $userId);
         }
 
-        return (bool) $result
-            ->execute()
-            ->fetchOne();
+
+        return (bool) $query->execute()->fetchOne();
     }
 
     /**

diff --git a/source/Core/InputValidator.php b/source/Core/InputValidator.php
@@ -131,7 +131,7 @@ public function checkLogin($user, $login, $invAddress)
             }
         }
 
-        if ($user->checkIfEmailReallyExists($login)) {
+        if ($user->checkIfEmailExists($login)) {
             //if exists then we do not allow to do that
             $message = \OxidEsales\Eshop\Core\Registry::getLang()->translateString('ERROR_MESSAGE_USER_USEREXISTS');
             $exception = oxNew(\OxidEsales\Eshop\Core\Exception\UserException::class, $message);