Skip to content

Commit 1c12fa2

Browse files
author
Alexander Paliarush
committed
MAGETWO-47583: checkAdminPasswordChange is NOT throwing exception when previous password is used
1 parent e7f5b9f commit 1c12fa2

File tree

9 files changed

+138
-221
lines changed

9 files changed

+138
-221
lines changed

‎app/code/Magento/User/Model/ResourceModel/User.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -539,7 +539,7 @@ public function getOldPasswords($user, $retainLimit = 4)
539539
$userId = (int)$user->getId();
540540
$table = $this->getTable('admin_passwords');
541541

542-
// purge expired passwords, except that should retain
542+
// purge expired passwords, except those which should be retained
543543
$retainPasswordIds = $this->getConnection()->fetchCol(
544544
$this->getConnection()
545545
->select()
@@ -556,7 +556,7 @@ public function getOldPasswords($user, $retainLimit = 4)
556556
}
557557
$this->getConnection()->delete($table, $where);
558558

559-
// now get all remained passwords
559+
// get all remaining passwords
560560
return $this->getConnection()->fetchCol(
561561
$this->getConnection()
562562
->select()

‎app/code/Magento/User/Model/User.php

Lines changed: 31 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -256,9 +256,9 @@ protected function _getValidationRulesBeforeSave()
256256
}
257257

258258
/**
259-
* Validate customer attribute values.
260-
* For existing customer password + confirmation will be validated only when password is set
261-
* (i.e. its change is requested)
259+
* Validate admin user data.
260+
*
261+
* Existing user password confirmation will be validated only when password is set
262262
*
263263
* @return bool|string[]
264264
*/
@@ -272,8 +272,35 @@ public function validate()
272272
return $validator->getMessages();
273273
}
274274

275-
return true;
275+
return $this->validatePasswordChange();
276+
}
277+
278+
/**
279+
* Make sure admin password was changed.
280+
*
281+
* New password is compared to at least 4 previous passwords to prevent setting them again
282+
*
283+
* @return bool|string[]
284+
*/
285+
protected function validatePasswordChange()
286+
{
287+
$password = $this->getPassword();
288+
if ($password && !$this->getForceNewPassword() && $this->getId()) {
289+
$errorMessage = __('Sorry, but this password has already been used. Please create another.');
290+
// Check if password is equal to the current one
291+
if ($this->_encryptor->isValidHash($password, $this->getOrigData('password'))) {
292+
return [$errorMessage];
293+
}
276294

295+
// Check whether password was used before
296+
$passwordHash = $this->_encryptor->getHash($password, false);
297+
foreach ($this->getResource()->getOldPasswords($this) as $oldPasswordHash) {
298+
if ($passwordHash === $oldPasswordHash) {
299+
return [$errorMessage];
300+
}
301+
}
302+
}
303+
return true;
277304
}
278305

279306
/**

‎app/code/Magento/User/Observer/Backend/CheckAdminPasswordChangeObserver.php

Lines changed: 0 additions & 82 deletions
This file was deleted.

‎app/code/Magento/User/Observer/Backend/TrackAdminNewPasswordObserver.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ public function __construct(
7171
}
7272

7373
/**
74-
* Save new admin password
74+
* Save current admin password to prevent its usage when changed in the future.
7575
*
7676
* @param EventObserver $observer
7777
* @return void
@@ -81,7 +81,7 @@ public function execute(EventObserver $observer)
8181
/* @var $user \Magento\User\Model\User */
8282
$user = $observer->getEvent()->getObject();
8383
if ($user->getId()) {
84-
$password = $user->getNewPassword();
84+
$password = $user->getCurrentPassword();
8585
$passwordLifetime = $this->observerConfig->getAdminPasswordLifetime();
8686
if ($passwordLifetime && $password && !$user->getForceNewPassword()) {
8787
$passwordHash = $this->encryptor->getHash($password, false);

‎app/code/Magento/User/Test/Unit/Model/UserTest.php

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -610,4 +610,104 @@ public function testIsResetPasswordLinkTokenExpiredIsNotExpiredToken()
610610
$this->userDataMock->expects($this->once())->method('getResetPasswordLinkExpirationPeriod')->willReturn(1);
611611
$this->assertFalse($this->model->isResetPasswordLinkTokenExpired());
612612
}
613+
614+
public function testCheckPasswordChangeEqualToCurrent()
615+
{
616+
/** @var $validatorMock \Magento\Framework\Validator\DataObject|\PHPUnit_Framework_MockObject_MockObject */
617+
$validatorMock = $this->getMockBuilder('Magento\Framework\Validator\DataObject')
618+
->disableOriginalConstructor()
619+
->setMethods([])
620+
->getMock();
621+
$this->validatorObjectFactoryMock->expects($this->once())->method('create')->willReturn($validatorMock);
622+
$this->validationRulesMock->expects($this->once())
623+
->method('addUserInfoRules')
624+
->with($validatorMock);
625+
$validatorMock->expects($this->once())->method('isValid')->willReturn(true);
626+
627+
$newPassword = "NEWmYn3wpassw0rd";
628+
$oldPassword = "OLDmYn3wpassw0rd";
629+
$this->model->setPassword($newPassword)
630+
->setId(1)
631+
->setOrigData('password', $oldPassword);
632+
$this->encryptorMock->expects($this->once())
633+
->method('isValidHash')
634+
->with($newPassword, $oldPassword)
635+
->willReturn(true);
636+
$result = $this->model->validate();
637+
$this->assertInternalType('array', $result);
638+
$this->assertCount(1, $result);
639+
$this->assertContains("Sorry, but this password has already been used.", (string)$result[0]);
640+
}
641+
642+
public function testCheckPasswordChangeEqualToPrevious()
643+
{
644+
/** @var $validatorMock \Magento\Framework\Validator\DataObject|\PHPUnit_Framework_MockObject_MockObject */
645+
$validatorMock = $this->getMockBuilder('Magento\Framework\Validator\DataObject')
646+
->disableOriginalConstructor()
647+
->setMethods([])
648+
->getMock();
649+
$this->validatorObjectFactoryMock->expects($this->once())->method('create')->willReturn($validatorMock);
650+
$this->validationRulesMock->expects($this->once())
651+
->method('addUserInfoRules')
652+
->with($validatorMock);
653+
$validatorMock->expects($this->once())->method('isValid')->willReturn(true);
654+
655+
$newPassword = "NEWmYn3wpassw0rd";
656+
$newPasswordHash = "new password hash";
657+
$oldPassword = "OLDmYn3wpassw0rd";
658+
$this->model->setPassword($newPassword)
659+
->setId(1)
660+
->setOrigData('password', $oldPassword);
661+
$this->encryptorMock->expects($this->once())
662+
->method('isValidHash')
663+
->with($newPassword, $oldPassword)
664+
->willReturn(false);
665+
666+
$this->encryptorMock->expects($this->once())
667+
->method('getHash')
668+
->with($newPassword, false)
669+
->willReturn($newPasswordHash);
670+
671+
$this->resourceMock->expects($this->once())->method('getOldPasswords')->willReturn(['hash1', $newPasswordHash]);
672+
673+
$result = $this->model->validate();
674+
$this->assertInternalType('array', $result);
675+
$this->assertCount(1, $result);
676+
$this->assertContains("Sorry, but this password has already been used.", (string)$result[0]);
677+
}
678+
679+
public function testCheckPasswordChangeValid()
680+
{
681+
/** @var $validatorMock \Magento\Framework\Validator\DataObject|\PHPUnit_Framework_MockObject_MockObject */
682+
$validatorMock = $this->getMockBuilder('Magento\Framework\Validator\DataObject')
683+
->disableOriginalConstructor()
684+
->setMethods([])
685+
->getMock();
686+
$this->validatorObjectFactoryMock->expects($this->once())->method('create')->willReturn($validatorMock);
687+
$this->validationRulesMock->expects($this->once())
688+
->method('addUserInfoRules')
689+
->with($validatorMock);
690+
$validatorMock->expects($this->once())->method('isValid')->willReturn(true);
691+
692+
$newPassword = "NEWmYn3wpassw0rd";
693+
$newPasswordHash = "new password hash";
694+
$oldPassword = "OLDmYn3wpassw0rd";
695+
$this->model->setPassword($newPassword)
696+
->setId(1)
697+
->setOrigData('password', $oldPassword);
698+
$this->encryptorMock->expects($this->once())
699+
->method('isValidHash')
700+
->with($newPassword, $oldPassword)
701+
->willReturn(false);
702+
703+
$this->encryptorMock->expects($this->once())
704+
->method('getHash')
705+
->with($newPassword, false)
706+
->willReturn($newPasswordHash);
707+
708+
$this->resourceMock->expects($this->once())->method('getOldPasswords')->willReturn(['hash1', 'hash2']);
709+
710+
$result = $this->model->validate();
711+
$this->assertTrue($result);
712+
}
613713
}

0 commit comments

Comments
 (0)