amzn / amazon-payments-magento-2-plugin

Extension to enable Amazon Pay on Magento 2
https://amzn.github.io/amazon-payments-magento-2-plugin/
Apache License 2.0
109 stars 77 forks source link

Login with existing account throws error if password hash is empty #1067

Closed rafczow closed 3 years ago

rafczow commented 3 years ago

What I expected

Trying to login to account without a password, a default message about incorrect password should appear. User can use Reset password feature to set a new password.

What happened instead

User gets blank page with error: image

Steps to reproduce the issue

  1. Create user at admin area (without password) or with other 3rd party service. password_hash field of customer_entity will be NULL.
  2. Try to login at frontend using Login with Amazon Pay
  3. From amazon login page user is redirected to store page with message informing that store account already exists for given email with form requesting password.
  4. Type in anything in password field and submit form.

Expected result (*) Redirect back to form with message 'The password supplied was incorrect'

Actual result (*) User cannot login and gets blank page. PHP Fatal error: Uncaught TypeError: explode() expects parameter 2 to be string, null given in /vendor/magento/framework/Encryption/Encryptor.php:323

Your setup

Additional comment

Same issue was found in core magento module and is described in this issue: https://github.com/magento/magento2/issues/19060 and solved here: https://github.com/magento/magento2/pull/19066/files

In my store i've also changed a message from: "The password supplied was incorrect." to: "The password supplied was incorrect. To reset your password please click on Forgot your password."

To give user a hint on solution if he never set a password for his account but doesnt know about it. Im not sure if it should be included in a fix thought.

jaybeckr commented 3 years ago

Hi @rafczow - thanks for your report, and accompanying fix! I have cherry-picked your commit into our workflow to be included in the next release. This means we'll close the PR without accepting it, but your commit will still be used, and we'll close this issue once the next release goes out.

sgabhart22 commented 3 years ago

Hello @rafczow , we had some difficulty reproducing this issue on Magento 2.4.1. It looks like this type of error should have been caught here:

https://github.com/magento/magento2/blob/2.4-develop/lib/internal/Magento/Framework/Encryption/Encryptor.php#L312

Just curious if this was due to a difference in PHP versions, or perhaps error reporting level?

rafczow commented 3 years ago

@sgabhart22 After your comment i`ve done some more digging. You are right it should be caught with code you provided but it is not in 2.4.1 release (and any previous version). Linked code is 2.4-develop branch and its different than 2.4.1 release tag: https://github.com/magento/magento2/blob/2.4.1/lib/internal/Magento/Framework/Encryption/Encryptor.php (develop branch are changes not released yet but to be included in next release)

As you can see there is difference in catch argument: 2.4.1: } catch (\RuntimeException $exception) { -develop: } catch (\Throwable $exception) {

Thrown error in our case is \TypeError that extends \Error which implements \Throwable. It's not an \Exception. So 2.4.1 version of catch will not work as intended.

jaybeckr commented 3 years ago

@rafczow - this fix has been included in 5.7.0 which is now available, thanks again for reporting and fixing!