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

[Backport 2.2-develop] #11409: Too many password reset requests even when disabled in settings #11435

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions app/code/Magento/Security/Model/Config.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,17 @@ class Config implements ConfigInterface
*/
const XML_PATH_ADMIN_AREA = 'admin/security/';

/**
* Configuration path to frontend area
*/
const XML_PATH_FRONTEND_AREA = 'customer/password/';

/**
* Configuration path to fronted area
* @deprecated
* @see \Magento\Security\Model\Config::XML_PATH_FRONTEND_AREA
*/
const XML_PATH_FRONTED_AREA = 'customer/password/';
const XML_PATH_FRONTED_AREA = self::XML_PATH_FRONTEND_AREA;

/**
* Configuration path to admin account sharing
Expand Down Expand Up @@ -134,7 +141,7 @@ protected function getXmlPathPrefix()
if ($this->scope->getCurrentScope() == \Magento\Framework\App\Area::AREA_ADMINHTML) {
return self::XML_PATH_ADMIN_AREA;
}
return self::XML_PATH_FRONTED_AREA;
return self::XML_PATH_FRONTEND_AREA;
}

/**
Expand Down
26 changes: 20 additions & 6 deletions app/code/Magento/Security/Model/Plugin/AccountManagement.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@
*/
namespace Magento\Security\Model\Plugin;

use Magento\Security\Model\SecurityManager;
use Magento\Customer\Model\AccountManagement as AccountManagementOriginal;
use Magento\Framework\App\ObjectManager;
use Magento\Framework\Config\ScopeInterface;
use Magento\Framework\Exception\SecurityViolationException;
use Magento\Security\Model\PasswordResetRequestEvent;
use Magento\Security\Model\SecurityManager;

/**
* Magento\Customer\Model\AccountManagement decorator
Expand All @@ -30,21 +32,29 @@ class AccountManagement
*/
protected $passwordRequestEvent;

/**
* @var ScopeInterface
*/
private $scope;

/**
* AccountManagement constructor.
*
* @param \Magento\Framework\App\RequestInterface $request
* @param SecurityManager $securityManager
* @param int $passwordRequestEvent
* @param ScopeInterface $scope
*/
public function __construct(
\Magento\Framework\App\RequestInterface $request,
\Magento\Security\Model\SecurityManager $securityManager,
$passwordRequestEvent = PasswordResetRequestEvent::CUSTOMER_PASSWORD_RESET_REQUEST
$passwordRequestEvent = PasswordResetRequestEvent::CUSTOMER_PASSWORD_RESET_REQUEST,
ScopeInterface $scope = null
) {
$this->request = $request;
$this->securityManager = $securityManager;
$this->passwordRequestEvent = $passwordRequestEvent;
$this->scope = $scope ?: ObjectManager::getInstance()->get(ScopeInterface::class);
}

/**
Expand All @@ -63,10 +73,14 @@ public function beforeInitiatePasswordReset(
$template,
$websiteId = null
) {
$this->securityManager->performSecurityCheck(
$this->passwordRequestEvent,
$email
);
if ($this->scope->getCurrentScope() == \Magento\Framework\App\Area::AREA_FRONTEND
|| $this->passwordRequestEvent == PasswordResetRequestEvent::ADMIN_PASSWORD_RESET_REQUEST) {
$this->securityManager->performSecurityCheck(
$this->passwordRequestEvent,
$email
);
}

return [$email, $template, $websiteId];
}
}
2 changes: 1 addition & 1 deletion app/code/Magento/Security/Test/Unit/Model/ConfigTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ protected function getXmlPathPrefix($scope)
if ($scope == \Magento\Framework\App\Area::AREA_ADMINHTML) {
return \Magento\Security\Model\Config::XML_PATH_ADMIN_AREA;
}
return \Magento\Security\Model\Config::XML_PATH_FRONTED_AREA;
return \Magento\Security\Model\Config::XML_PATH_FRONTEND_AREA;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@

namespace Magento\Security\Test\Unit\Model\Plugin;

use Magento\Customer\Model\AccountManagement;
use Magento\Framework\App\Area;
use Magento\Framework\Config\ScopeInterface;
use Magento\Framework\TestFramework\Unit\Helper\ObjectManager;
use Magento\Security\Model\PasswordResetRequestEvent;

/**
* Test class for \Magento\Security\Model\Plugin\AccountManagement testing
Expand All @@ -19,20 +23,25 @@ class AccountManagementTest extends \PHPUnit\Framework\TestCase
protected $model;

/**
* @var \Magento\Framework\App\RequestInterface
* @var \Magento\Framework\App\RequestInterface|\PHPUnit_Framework_MockObject_MockObject
*/
protected $request;

/**
* @var \Magento\Security\Model\SecurityManager
* @var \Magento\Security\Model\SecurityManager|\PHPUnit_Framework_MockObject_MockObject
*/
protected $securityManager;

/**
* @var \Magento\Customer\Model\AccountManagement
* @var AccountManagement|\PHPUnit_Framework_MockObject_MockObject
*/
protected $accountManagement;

/**
* @var ScopeInterface|\PHPUnit_Framework_MockObject_MockObject
*/
private $scope;

/**
* @var \Magento\Framework\TestFramework\Unit\Helper\ObjectManager
*/
Expand All @@ -46,35 +55,45 @@ public function setUp()
{
$this->objectManager = new ObjectManager($this);

$this->request = $this->createMock(\Magento\Framework\App\RequestInterface::class);
$this->request = $this->createMock(\Magento\Framework\App\RequestInterface::class);

$this->securityManager = $this->createPartialMock(
\Magento\Security\Model\SecurityManager::class,
['performSecurityCheck']
);

$this->accountManagement = $this->createMock(\Magento\Customer\Model\AccountManagement::class);
$this->accountManagement = $this->createMock(AccountManagement::class);
$this->scope = $this->createMock(ScopeInterface::class);
}

/**
* @param $area
* @param $passwordRequestEvent
* @param $expectedTimes
* @dataProvider beforeInitiatePasswordResetDataProvider
*/
public function testBeforeInitiatePasswordReset($area, $passwordRequestEvent, $expectedTimes)
{
$email = 'test@example.com';
$template = AccountManagement::EMAIL_RESET;

$this->model = $this->objectManager->getObject(
\Magento\Security\Model\Plugin\AccountManagement::class,
[
'passwordRequestEvent' => $passwordRequestEvent,
'request' => $this->request,
'securityManager' => $this->securityManager
'securityManager' => $this->securityManager,
'scope' => $this->scope
]
);
}

/**
* @return void
*/
public function testBeforeInitiatePasswordReset()
{
$email = 'test@example.com';
$template = \Magento\Customer\Model\AccountManagement::EMAIL_RESET;
$this->scope->expects($this->once())
->method('getCurrentScope')
->willReturn($area);

$this->securityManager->expects($this->once())
$this->securityManager->expects($this->exactly($expectedTimes))
->method('performSecurityCheck')
->with(\Magento\Security\Model\PasswordResetRequestEvent::CUSTOMER_PASSWORD_RESET_REQUEST, $email)
->with($passwordRequestEvent, $email)
->willReturnSelf();

$this->model->beforeInitiatePasswordReset(
Expand All @@ -83,4 +102,18 @@ public function testBeforeInitiatePasswordReset()
$template
);
}

/**
* @return array
*/
public function beforeInitiatePasswordResetDataProvider()
{
return [
[Area::AREA_ADMINHTML, PasswordResetRequestEvent::CUSTOMER_PASSWORD_RESET_REQUEST, 0],
[Area::AREA_ADMINHTML, PasswordResetRequestEvent::ADMIN_PASSWORD_RESET_REQUEST, 1],
[Area::AREA_FRONTEND, PasswordResetRequestEvent::CUSTOMER_PASSWORD_RESET_REQUEST, 1],
// This should never happen, but let's cover it with tests
[Area::AREA_FRONTEND, PasswordResetRequestEvent::ADMIN_PASSWORD_RESET_REQUEST, 1],
];
}
}
2 changes: 1 addition & 1 deletion app/code/Magento/Security/etc/adminhtml/di.xml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
</type>
<type name="Magento\Security\Model\Plugin\AccountManagement">
<arguments>
<argument name="passwordRequestEvent" xsi:type="const">Magento\Security\Model\PasswordResetRequestEvent::ADMIN_PASSWORD_RESET_REQUEST</argument>
<argument name="passwordRequestEvent" xsi:type="const">Magento\Security\Model\PasswordResetRequestEvent::CUSTOMER_PASSWORD_RESET_REQUEST</argument>
</arguments>
</type>
<type name="Magento\Security\Model\SecurityManager">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,12 @@
use Magento\Customer\Api\Data\CustomerInterface as Customer;
use Magento\Customer\Model\AccountManagement;
use Magento\Framework\Exception\InputException;
use Magento\Framework\Exception\NoSuchEntityException;
use Magento\Framework\Webapi\Exception as HTTPExceptionCodes;
use Magento\Newsletter\Model\Subscriber;
use Magento\Security\Model\Config;
use Magento\TestFramework\Helper\Bootstrap;
use Magento\TestFramework\Helper\Customer as CustomerHelper;
use Magento\TestFramework\TestCase\WebapiAbstract;
use Magento\Framework\Webapi\Exception as HTTPExceptionCodes;
use Magento\Security\Model\Config;
use Magento\Newsletter\Model\Plugin\CustomerPlugin;
use Magento\Framework\Webapi\Rest\Request as RestRequest;
use Magento\Newsletter\Model\Subscriber;
use Magento\Customer\Model\Data\Customer as CustomerData;

/**
* Test class for Magento\Customer\Api\AccountManagementInterface
Expand Down Expand Up @@ -112,16 +108,16 @@ public function setUp()
$this->initSubscriber();

if ($this->config->getConfigDataValue(
Config::XML_PATH_FRONTED_AREA .
Config::XML_PATH_FRONTEND_AREA .
Config::XML_PATH_PASSWORD_RESET_PROTECTION_TYPE
) != 0) {
$this->configValue = $this->config
->getConfigDataValue(
Config::XML_PATH_FRONTED_AREA .
Config::XML_PATH_FRONTEND_AREA .
Config::XML_PATH_PASSWORD_RESET_PROTECTION_TYPE
);
$this->config->setDataByPath(
Config::XML_PATH_FRONTED_AREA . Config::XML_PATH_PASSWORD_RESET_PROTECTION_TYPE,
Config::XML_PATH_FRONTEND_AREA . Config::XML_PATH_PASSWORD_RESET_PROTECTION_TYPE,
0
);
$this->config->save();
Expand Down Expand Up @@ -150,7 +146,7 @@ public function tearDown()
}
}
$this->config->setDataByPath(
Config::XML_PATH_FRONTED_AREA . Config::XML_PATH_PASSWORD_RESET_PROTECTION_TYPE,
Config::XML_PATH_FRONTEND_AREA . Config::XML_PATH_PASSWORD_RESET_PROTECTION_TYPE,
$this->configValue
);
$this->config->save();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,11 @@ class ResetPasswordTest extends \Magento\TestFramework\TestCase\AbstractBackendC
protected $baseControllerUrl = 'http://localhost/index.php/backend/customer/index/';

/**
* Checks reset password functionality with default settings and customer reset request event.
* Checks reset password functionality with no restrictive settings and customer reset request event.
* Admin is not affected by this security check, so reset password email must be sent.
*
* @magentoConfigFixture current_store admin/security/limit_password_reset_requests_method 1
* @magentoConfigFixture current_store admin/security/min_time_between_password_reset_requests 10
* @magentoConfigFixture current_store customer/password/limit_password_reset_requests_method 0
* @magentoConfigFixture current_store customer/password/min_time_between_password_reset_requests 0
* @magentoDataFixture Magento/Customer/_files/customer.php
*/
public function testResetPasswordSuccess()
Expand All @@ -40,11 +41,57 @@ public function testResetPasswordSuccess()
$this->assertRedirect($this->stringStartsWith($this->baseControllerUrl . 'edit'));
}

/**
* Checks reset password functionality with default restrictive min time between
* password reset requests and customer reset request event.
* Admin is not affected by this security check, so reset password email must be sent.
*
* @magentoConfigFixture current_store customer/password/max_number_password_reset_requests 0
* @magentoConfigFixture current_store customer/password/min_time_between_password_reset_requests 10
* @magentoDataFixture Magento/Customer/_files/customer.php
*/
public function testResetPasswordMinTimeError()
{
$this->passwordResetRequestEventCreate(
\Magento\Security\Model\PasswordResetRequestEvent::CUSTOMER_PASSWORD_RESET_REQUEST
);
$this->getRequest()->setPostValue(['customer_id' => '1']);
$this->dispatch('backend/customer/index/resetPassword');
$this->assertSessionMessages(
$this->equalTo(['The customer will receive an email with a link to reset password.']),
\Magento\Framework\Message\MessageInterface::TYPE_SUCCESS
);
$this->assertRedirect($this->stringStartsWith($this->baseControllerUrl . 'edit'));
}

/**
* Checks reset password functionality with default restrictive limited number
* password reset requests and customer reset request event.
* Admin is not affected by this security check, so reset password email must be sent.
*
* @magentoConfigFixture current_store customer/password/max_number_password_reset_requests 1
* @magentoConfigFixture current_store customer/password/min_time_between_password_reset_requests 0
* @magentoDataFixture Magento/Customer/_files/customer.php
*/
public function testResetPasswordLimitError()
{
$this->passwordResetRequestEventCreate(
\Magento\Security\Model\PasswordResetRequestEvent::CUSTOMER_PASSWORD_RESET_REQUEST
);
$this->getRequest()->setPostValue(['customer_id' => '1']);
$this->dispatch('backend/customer/index/resetPassword');
$this->assertSessionMessages(
$this->equalTo(['The customer will receive an email with a link to reset password.']),
\Magento\Framework\Message\MessageInterface::TYPE_SUCCESS
);
$this->assertRedirect($this->stringStartsWith($this->baseControllerUrl . 'edit'));
}

/**
* Checks reset password functionality with default settings, customer and admin reset request events.
*
* @magentoConfigFixture current_store admin/security/limit_password_reset_requests_method 1
* @magentoConfigFixture current_store admin/security/min_time_between_password_reset_requests 10
* @magentoConfigFixture current_store customer/password/limit_password_reset_requests_method 1
* @magentoConfigFixture current_store customer/password/min_time_between_password_reset_requests 10
* @magentoConfigFixture current_store contact/email/recipient_email hello@example.com
* @magentoDataFixture Magento/Customer/_files/customer.php
*/
Expand All @@ -59,10 +106,8 @@ public function testResetPasswordWithSecurityViolationException()
$this->getRequest()->setPostValue(['customer_id' => '1']);
$this->dispatch('backend/customer/index/resetPassword');
$this->assertSessionMessages(
$this->equalTo(
['Too many password reset requests. Please wait and try again or contact hello@example.com.']
),
\Magento\Framework\Message\MessageInterface::TYPE_ERROR
$this->equalTo(['The customer will receive an email with a link to reset password.']),
\Magento\Framework\Message\MessageInterface::TYPE_SUCCESS
);
$this->assertRedirect($this->stringStartsWith($this->baseControllerUrl . 'edit'));
}
Expand Down