Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shopping cart is emptied after reset password procedure #14530

Closed
evgenyvas opened this issue Apr 4, 2018 · 13 comments
Closed

Shopping cart is emptied after reset password procedure #14530

evgenyvas opened this issue Apr 4, 2018 · 13 comments
Assignees
Labels
Component: Customer Event: balance-cd Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release

Comments

@evgenyvas
Copy link

evgenyvas commented Apr 4, 2018

Preconditions

  1. PHP 7.0.27
  2. MariaDB 10.1.26
  3. Magento 2.1.12

Steps to reproduce

  1. As a guest, put some items in shopping cart
  2. Login -> Forgot Password
  3. Enter Email, press "Reset Password" button
  4. From mail press "Set a new password"
  5. On "Forgot Password" page input new password. Shopping cart is still have items in it.
  6. Press button "Set a new password"
  7. Shopping cart is empty now.

Expected result

  1. Shopping cart not empty after setting new password.

Actual result

  1. Shopping cart empty after setting new password.

There are many users with random generated password, after transfer from old CMS. While checkout process they see account is existed, try to reset password and after that they see shopping cart is empty. It's embarrasing.

@magento-engcom-team magento-engcom-team added the Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed label Apr 4, 2018
@evgenyvas
Copy link
Author

As a workaround, I commented two lines in vendor/magento/module-customer/Model/AccountManagement.php, function resetPassword
$this->sessionManager->destroy();
$this->destroyCustomerSessions($customer->getId());
Did I break anything?

    public function resetPassword($email, $resetToken, $newPassword)
    {
        $customer = $this->customerRepository->get($email);
        //Validate Token and new password strength
        $this->validateResetPasswordToken($customer->getId(), $resetToken);
        $this->checkPasswordStrength($newPassword);
        //Update secure data
        $customerSecure = $this->customerRegistry->retrieveSecureData($customer->getId());
        $customerSecure->setRpToken(null);
        $customerSecure->setRpTokenCreatedAt(null);
        $customerSecure->setPasswordHash($this->createPasswordHash($newPassword));
        //$this->sessionManager->destroy();
        //$this->destroyCustomerSessions($customer->getId());
        $this->customerRepository->save($customer);

        return true;
    }

@peterjaap
Copy link
Contributor

peterjaap commented May 22, 2018

@evgenyvas you only have to comment out $this->sessionManager->destroy();. The destroyCustomerSessions method destroys customer sessions except the active one. But the active one is destroyed in $this->sessionManager->destroy();.

This appears to be introduced in 2.2.3; https://github.com/magento/magento2/blame/a952969a8c08928d356fab8d0fb35f4dbe5fe9ce/app/code/Magento/Customer/Model/AccountManagement.php#L613

This is obviously very annoying for customers.

For now, I just use a composer patch to fix this;

From 1de4b953e0da7ee29d586d770d79d857c3c9ca33 Mon Sep 17 00:00:00 2001
From: peterjaap <peterjaap@elgentos.nl>
Date: Tue, 22 May 2018 14:33:51 +0200
Subject: [PATCH 1/1] Uncommented destroying of active session after resetting
 password

Signed-off-by: peterjaap <peterjaap@elgentos.nl>
---
 Model/AccountManagement.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Model/AccountManagement.php b/Model/AccountManagement.php
index c1231a1..4469e5e 100644
--- a/Model/AccountManagement.php
+++ b/Model/AccountManagement.php
@@ -596,7 +596,7 @@ class AccountManagement implements AccountManagementInterface
         $customerSecure->setRpToken(null);
         $customerSecure->setRpTokenCreatedAt(null);
         $customerSecure->setPasswordHash($this->createPasswordHash($newPassword));
-        $this->sessionManager->destroy();
+//        $this->sessionManager->destroy();
         $this->destroyCustomerSessions($customer->getId());
         $this->customerRepository->save($customer);
 
-- 
2.17.0

@ghost ghost self-assigned this Jul 16, 2018
@ghost ghost added Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Component: Customer labels Jul 16, 2018
@ghost
Copy link

ghost commented Jul 16, 2018

Hi @evgenyvas thank you for your report.
We've acknowledged the issue and added to our backlog.

@ghost ghost added Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Jul 16, 2018
@ghost ghost removed their assignment Jul 16, 2018
maximbaibakov added a commit to maximbaibakov/magento2 that referenced this issue Aug 11, 2018
Do not destory current session once you have requested to reset password using "Password forgot function"
@maximbaibakov maximbaibakov self-assigned this Aug 11, 2018
LucasCalazans added a commit to LucasCalazans/magento2 that referenced this issue Sep 16, 2018
LucasCalazans added a commit to LucasCalazans/magento2 that referenced this issue Sep 16, 2018
@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Sep 18, 2018

@engcom-backlog-nazar wasn't this issue fixed by #14973 ? It looks like duplicate of #12362

@ghost
Copy link

ghost commented Sep 18, 2018

Hi @ihor-sviziev No, just checked now still exist this issue

@ghost
Copy link

ghost commented Sep 18, 2018

@ihor-sviziev This PR can fix this issue-> #17517

@peterjaap
Copy link
Contributor

Backward patch for 2.2.6;

diff --git a/Model/AccountManagement.php b/Model/AccountManagement.php
index 8f25651..6e40f1a 100644
--- a/Model/AccountManagement.php
+++ b/Model/AccountManagement.php
@@ -670,7 +670,7 @@ class AccountManagement implements AccountManagementInterface
         $customerSecure->setRpTokenCreatedAt(null);
         $customerSecure->setPasswordHash($this->createPasswordHash($newPassword));
         $this->getAuthentication()->unlock($customer->getId());
-        $this->sessionManager->destroy();
+//        $this->sessionManager->destroy();
         $this->destroyCustomerSessions($customer->getId());
         $this->customerRepository->save($customer);
 
-- 
2.17.1


@peterjaap
Copy link
Contributor

Backward patch for 2.2.7;

diff --git a/Model/AccountManagement.php b/Model/AccountManagement.php
index 8f25651..404d5e8 100644
--- a/Model/AccountManagement.php
+++ b/Model/AccountManagement.php
@@ -670,7 +670,7 @@ class AccountManagement implements AccountManagementInterface
         $customerSecure->setRpTokenCreatedAt(null);
         $customerSecure->setPasswordHash($this->createPasswordHash($newPassword));
         $this->getAuthentication()->unlock($customer->getId());
-        $this->sessionManager->destroy();
+        // $this->sessionManager->destroy(); // uncommented by patch
         $this->destroyCustomerSessions($customer->getId());
         $this->customerRepository->save($customer);
 
-- 
2.17.1

@hryvinskyi
Copy link
Member

The same error on Magento 2.2.7
This commit: bc8d3d5 does not fix this error

@peterjaap
Copy link
Contributor

Backward patch for 2.2.8;

diff --git a/Model/AccountManagement.php b/Model/AccountManagement.php
index 6387555..a753a7e 100644
--- a/Model/AccountManagement.php
+++ b/Model/AccountManagement.php
@@ -690,7 +690,7 @@ class AccountManagement implements AccountManagementInterface
         $customerSecure->setPasswordHash($this->createPasswordHash($newPassword));
         $this->getAuthentication()->unlock($customer->getId());
         $this->destroyCustomerSessions($customer->getId());
-        $this->sessionManager->destroy();
+        // $this->sessionManager->destroy(); // uncommented by patch
         $this->customerRepository->save($customer);
 
         return true;
-- 
2.17.1

@ghost ghost assigned LucasCalazans Apr 3, 2019
@sdzhepa
Copy link
Contributor

sdzhepa commented Apr 30, 2019

Hello @evgenyvas @peterjaap @hryvinskyi @maximbaibakov @LucasCalazans

Thank you for contribution and collaboration!

The corresponding internal ticket MAGETWO-93628was fixed and closed by Magento team

Delivered to 2.3-develop branch and should be available with 2.3.2 release
Please see details in the next commits:

Internal ticket MAGETWO-93627 currently in the delivery queue and will be merged into 2.2-develop soon.
Should be available with 2.2.9 release

@sdzhepa sdzhepa closed this as completed Apr 30, 2019
@orlangur
Copy link
Contributor

@sdzhepa please establish practice of squashing changes properly, is not so easy to understand what happened there from 5 commits.

@sidolov
Copy link
Contributor

sidolov commented Apr 30, 2019

Hi @orlangur , we'll reach out our core teams with your suggestion. Thank you for the input.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Customer Event: balance-cd Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Development

No branches or pull requests

10 participants