OXDEV-8351 Add proper error handling for duplicate emails

OXID-eSales · Nov 14, 2024 · 7372031 · 7372031
1 parent 9d3df12
commit 7372031
Show file tree

Hide file tree

Showing 12 changed files with 235 additions and 78 deletions.
diff --git a/CHANGELOG-7.3.md b/CHANGELOG-7.3.md
@@ -8,6 +8,7 @@
 
 ### Fixed
 - Shop ID resolution considers SSL language URLs
+- Email existence check when changing from user to guest email
 
 ### Removed
 - PHPUnit v10 support

diff --git a/source/Application/Component/UserComponent.php b/source/Application/Component/UserComponent.php
@@ -654,59 +654,60 @@ protected function changeUserWithoutRedirect()
         if (!$user) {
             return;
         }
+
+        $currentEmail = $user->getFieldData('oxusername');
+        $password = $user->getFieldData('oxpassword');
         $shippingAddress = $this->getShippingAddress();
         $billingAddress = $this->getBillingAddress();
+        $newEmail = $billingAddress['oxuser__oxusername'] ?? '';
 
-        $username = $user->getFieldData('oxusername');
-        $password = $user->getFieldData('oxpassword');
         try {
-            $newName = $billingAddress['oxuser__oxusername'] ?? '';
+            $isUsernameUpdated = $this->isUserNameUpdated($currentEmail ?? '', $newEmail);
+            if ($isUsernameUpdated && $this->isGuestUser($user)) {
+                $this->deleteExistingGuestUser($newEmail);
+            }
             if (
-                $this->isGuestUser($user)
-                && $this->isUserNameUpdated($user->oxuser__oxusername->value ?? '', $newName)
+                !$this->isGuestUser($user)
+                && $isUsernameUpdated
+                && $user->checkIfEmailExists($newEmail, true)
             ) {
-                $this->deleteExistingGuestUser($newName);
+                Registry::getUtilsView()->addErrorToDisplay('ERROR_MESSAGE_USER_USEREXISTS');
+                return false;
             }
-            $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
+                $isUsernameUpdated
             );
+
+            $this->resetPermissions();
+
+            $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;
         } 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,46 @@ 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->checkIfEmailExists($aParams['oxuser__oxusername'])) {
+            if (
+                isset($aParams['oxuser__oxusername']) &&
+                ($aParams['oxuser__oxusername'] !== $oUser->getRawFieldData('oxusername')) &&
+                $oUser->checkIfEmailExists($aParams['oxuser__oxusername'], true)
+            ) {
                 $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 +165,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
@@ -1718,7 +1718,7 @@ protected function update()
      *
      * @return bool
      */
-    public function checkIfEmailExists($sEmail)
+    public function checkIfEmailExists($sEmail, $changeMail = false)
     {
         $myConfig = \OxidEsales\Eshop\Core\Registry::getConfig();
         // We force reading from master to prevent issues with slow replications or open transactions (see ESDEV-3804).
@@ -1749,7 +1749,7 @@ public function checkIfEmailExists($sEmail)
                         // exists admin with same login - must not allow
                         $blExists = true;
                         break;
-                    } elseif ($oRs->fields[0] == $iShopId && $oRs->fields[2]) {
+                    } elseif ($oRs->fields[0] == $iShopId && $oRs->fields[2] || $changeMail) {
                         // exists same login (with password) in same shop
                         $blExists = true;
                         break;

diff --git a/source/Application/translations/de/lang.php b/source/Application/translations/de/lang.php
@@ -163,7 +163,7 @@
 'ERROR_MESSAGE_USER_NOVALUES'                                 => 'E-Mail und Passwort müssen ausgefüllt sein!',
 'ERROR_MESSAGE_USER_USERCREATIONFAILED'                       => 'Fehler beim Anlegen des Benutzers!',
 'ERROR_MESSAGE_USER_UPDATE_FAILED'                            => 'Fehler beim Aktualisieren der Benutzerdaten!',
-'ERROR_MESSAGE_USER_USEREXISTS'                               => '%s konnte nicht registriert werden. Haben Sie bereits ein Kundenkonto bei uns?',
+'ERROR_MESSAGE_USER_USEREXISTS'                               => 'Bei dieser E-Mail-Adresse ist ein Fehler aufgetreten. Bitte wenden Sie sich an unseren Support.',
 'ERROR_MESSAGE_VERSION_EXPIRED1'                              => 'Ihre Version ist leider abgelaufen. Bitte kontaktieren Sie',
 'ERROR_MESSAGE_VOUCHER_INCORRECTPRICE'                        => 'Einkaufswert ist zu niedrig für diesen Gutschein!',
 'ERROR_MESSAGE_VOUCHER_ISRESERVED'                            => 'Gutschein ist reserviert!',

diff --git a/source/Application/translations/en/lang.php b/source/Application/translations/en/lang.php
@@ -163,7 +163,7 @@
 'ERROR_MESSAGE_USER_NOVALUES'                                 => 'E-mail address and password have to be entered!',
 'ERROR_MESSAGE_USER_USERCREATIONFAILED'                       => 'Error creating the user!',
 'ERROR_MESSAGE_USER_UPDATE_FAILED'                            => 'Error while updating the user data!',
-'ERROR_MESSAGE_USER_USEREXISTS'                               => 'Not possible to register %s. Maybe you have already registered?',
+'ERROR_MESSAGE_USER_USEREXISTS'                               => 'An error has occurred with this e-mail address. Please contact our support.',
 'ERROR_MESSAGE_VERSION_EXPIRED1'                              => 'Your version is expired. Please contact',
 'ERROR_MESSAGE_VOUCHER_INCORRECTPRICE'                        => 'The total price is too low for this coupon!',
 'ERROR_MESSAGE_VOUCHER_ISRESERVED'                            => 'This coupon is reserved!',

diff --git a/source/Core/InputValidator.php b/source/Core/InputValidator.php
@@ -10,7 +10,9 @@
 use OxidEsales\Eshop\Application\Model\Address;
 use OxidEsales\Eshop\Application\Model\User;
 use OxidEsales\Eshop\Core\Exception\ArticleInputException;
+use OxidEsales\Eshop\Core\Exception\InputException;
 use OxidEsales\Eshop\Core\Exception\StandardException;
+use OxidEsales\Eshop\Core\Exception\UserException;
 use OxidEsales\Eshop\Core\Registry;
 use OxidEsales\Eshop\Core\Str;
 use OxidEsales\EshopCommunity\Core\Di\ContainerFacade;
@@ -107,34 +109,37 @@ public function validateBasketAmount($amount)
      */
     public function checkLogin($user, $login, $invAddress)
     {
-        $login = (isset($invAddress['oxuser__oxusername'])) ? $invAddress['oxuser__oxusername'] : $login;
+        $login = (isset($invAddress['oxuser__oxusername']))
+            ? $invAddress['oxuser__oxusername']
+            : $login;
+
+        if (
+            isset($user->oxuser__oxpassword->value) &&
+            $user->oxuser__oxpassword->value &&
+            $login != $user->oxuser__oxusername->value
+        ) {
+            $newPassword = (isset($invAddress['oxuser__oxpassword']) && $invAddress['oxuser__oxpassword'])
+                ? $invAddress['oxuser__oxpassword']
+                : Registry::getRequest()->getRequestEscapedParameter('user_password');
 
-        // check only for users with password during registration
-        // if user wants to change user name - we must check if passwords are ok before changing
-        if (isset($user->oxuser__oxpassword->value) && $user->oxuser__oxpassword->value && $login != $user->oxuser__oxusername->value) {
-            // on this case password must be taken directly from request
-            $newPassword = (isset($invAddress['oxuser__oxpassword']) && $invAddress['oxuser__oxpassword']) ? $invAddress['oxuser__oxpassword'] : Registry::getRequest()->getRequestEscapedParameter('user_password');
             if (!$newPassword) {
-                // 1. user forgot to enter password
-                $exception = oxNew(\OxidEsales\Eshop\Core\Exception\InputException::class);
-                $exception->setMessage(\OxidEsales\Eshop\Core\Registry::getLang()->translateString('ERROR_MESSAGE_INPUT_NOTALLFIELDS'));
+                $message = Registry::getLang()->translateString('ERROR_MESSAGE_INPUT_NOTALLFIELDS');
+                $exception = oxNew(InputException::class, $message);
 
                 return $this->addValidationError("oxuser__oxpassword", $exception);
             } else {
-                // 2. entered wrong password
                 if (!$user->isSamePassword($newPassword)) {
-                    $exception = oxNew(\OxidEsales\Eshop\Core\Exception\UserException::class);
-                    $exception->setMessage(\OxidEsales\Eshop\Core\Registry::getLang()->translateString('ERROR_MESSAGE_PASSWORD_DO_NOT_MATCH'));
+                    $message = Registry::getLang()->translateString('ERROR_MESSAGE_PASSWORD_DO_NOT_MATCH');
+                    $exception = oxNew(UserException::class, $message);
 
                     return $this->addValidationError("oxuser__oxpassword", $exception);
                 }
             }
         }
 
         if ($user->checkIfEmailExists($login)) {
-            //if exists then we do not allow to do that
-            $exception = oxNew(\OxidEsales\Eshop\Core\Exception\UserException::class);
-            $exception->setMessage(sprintf(\OxidEsales\Eshop\Core\Registry::getLang()->translateString('ERROR_MESSAGE_USER_USEREXISTS'), $login));
+            $message = Registry::getLang()->translateString('ERROR_MESSAGE_USER_USEREXISTS');
+            $exception = oxNew(UserException::class, $message);
 
             return $this->addValidationError("oxuser__oxusername", $exception);
         }

diff --git a/tests/Codeception/Acceptance/Admin/UserCest.php b/tests/Codeception/Acceptance/Admin/UserCest.php
@@ -14,6 +14,7 @@
 use OxidEsales\Codeception\Admin\DataObject\AdminUser;
 use OxidEsales\Codeception\Admin\DataObject\AdminUserAddresses;
 use OxidEsales\Codeception\Admin\DataObject\AdminUserExtendedInfo;
+use OxidEsales\Codeception\Module\Translation\Translator;
 use OxidEsales\EshopCommunity\Tests\Codeception\Support\AcceptanceTester;
 
 #[Group('admin')]
@@ -197,6 +198,39 @@ public function updatePassword(AcceptanceTester $I): void
             ->login($userData['userLoginName'], $newPass);
     }
 
+    public function testChangeUserEmail(AcceptanceTester $I): void
+    {
+        $I->wantToTest('changing user email addresses with validation in admin');
+
+        $userData = Fixtures::get('existingUser');
+        $guestUserData = Fixtures::get('existingGuestUser');
+        $adminUserData = Fixtures::get('adminUser');
+        $newEmail = '[email protected]';
+
+        $I->amGoingTo('login as admin and find the test user');
+        $adminUsersPage = $I->loginAdmin()
+            ->openUsers()
+            ->findByUserName($userData['userLoginName']);
+
+        $I->amGoingTo('try to change email to an existing guest user email');
+        $adminUsersPage->updateUsername($guestUserData['userLoginName']);
+        $I->expect('to see an error message about existing user');
+        $I->see(Translator::translate('EXCEPTION_USER_USEREXISTS'));
+
+        $I->amGoingTo('try to change email to an existing admin email');
+        $adminUsersPage->updateUsername($adminUserData['userLoginName']);
+        $I->expect('to see an error message about existing user');
+        $I->see(Translator::translate('EXCEPTION_USER_USEREXISTS'));
+
+        $I->amGoingTo('change user email to a new valid email address');
+        $adminUsersPage->updateUsername($newEmail);
+        $I->expect('not to see any error message');
+        $I->dontSee(Translator::translate('EXCEPTION_USER_USEREXISTS'));
+
+        $I->amGoingTo('change the email back to the original one');
+        $adminUsersPage->updateUsername($userData['userLoginName']);
+    }
+
     private function createAdminTestUser(
         AcceptanceTester $I,
         AdminUser $user,