From 3963446a999a85f8af5930ce5910b38f765e52ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Mart=C3=ADnez?= Date: Fri, 13 Oct 2017 17:24:58 +0200 Subject: [PATCH] Too many password reset requests even when disabled in settings #11409 --- .../Model/Plugin/AccountManagement.php | 26 ++++++-- .../Model/Plugin/AccountManagementTest.php | 63 ++++++++++++++----- .../Magento/Security/etc/adminhtml/di.xml | 2 +- .../Adminhtml/Index/ResetPasswordTest.php | 63 ++++++++++++++++--- 4 files changed, 123 insertions(+), 31 deletions(-) diff --git a/app/code/Magento/Security/Model/Plugin/AccountManagement.php b/app/code/Magento/Security/Model/Plugin/AccountManagement.php index dea54b194880d..9476bf46df338 100644 --- a/app/code/Magento/Security/Model/Plugin/AccountManagement.php +++ b/app/code/Magento/Security/Model/Plugin/AccountManagement.php @@ -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 @@ -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); } /** @@ -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]; } } diff --git a/app/code/Magento/Security/Test/Unit/Model/Plugin/AccountManagementTest.php b/app/code/Magento/Security/Test/Unit/Model/Plugin/AccountManagementTest.php index 0935dc003d5b3..ce90ad532db67 100644 --- a/app/code/Magento/Security/Test/Unit/Model/Plugin/AccountManagementTest.php +++ b/app/code/Magento/Security/Test/Unit/Model/Plugin/AccountManagementTest.php @@ -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 @@ -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 */ @@ -53,28 +62,38 @@ public function setUp() ['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( @@ -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], + ]; + } } diff --git a/app/code/Magento/Security/etc/adminhtml/di.xml b/app/code/Magento/Security/etc/adminhtml/di.xml index 6f07fb580490e..c1188c2d405cf 100644 --- a/app/code/Magento/Security/etc/adminhtml/di.xml +++ b/app/code/Magento/Security/etc/adminhtml/di.xml @@ -17,7 +17,7 @@ - Magento\Security\Model\PasswordResetRequestEvent::ADMIN_PASSWORD_RESET_REQUEST + Magento\Security\Model\PasswordResetRequestEvent::CUSTOMER_PASSWORD_RESET_REQUEST diff --git a/dev/tests/integration/testsuite/Magento/Customer/Controller/Adminhtml/Index/ResetPasswordTest.php b/dev/tests/integration/testsuite/Magento/Customer/Controller/Adminhtml/Index/ResetPasswordTest.php index fcbe7c5106617..b5ca783d68cf2 100644 --- a/dev/tests/integration/testsuite/Magento/Customer/Controller/Adminhtml/Index/ResetPasswordTest.php +++ b/dev/tests/integration/testsuite/Magento/Customer/Controller/Adminhtml/Index/ResetPasswordTest.php @@ -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() @@ -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 */ @@ -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')); }