From 27059107f869c6a5758569c1e24f34f8543b245e Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Tue, 28 Jun 2016 11:13:15 +0200 Subject: [PATCH] add events to check passwords with the password policy app --- lib/private/server.php | 3 +- lib/private/share20/manager.php | 23 ++++++----- lib/private/user/database.php | 9 +++- settings/changepassword/controller.php | 31 +++++++++----- settings/js/personal.js | 30 +++++++++----- settings/templates/personal.php | 3 +- tests/lib/share20/managertest.php | 57 ++++++++++++++------------ tests/lib/user/database.php | 45 +++++++++++++++++++- 8 files changed, 138 insertions(+), 63 deletions(-) diff --git a/lib/private/server.php b/lib/private/server.php index 581a2b44ce..7d4b02fefe 100644 --- a/lib/private/server.php +++ b/lib/private/server.php @@ -618,7 +618,8 @@ class Server extends ServerContainer implements IServerContainer { $c->getL10N('core'), $factory, $c->getUserManager(), - $c->getRootFolder() + $c->getRootFolder(), + $c->getEventDispatcher() ); return $manager; diff --git a/lib/private/share20/manager.php b/lib/private/share20/manager.php index 35a019b233..5d53d0ef83 100644 --- a/lib/private/share20/manager.php +++ b/lib/private/share20/manager.php @@ -24,6 +24,7 @@ namespace OC\Share20; use OC\Files\Mount\MoveableMount; +use OC\HintException; use OCP\Files\IRootFolder; use OCP\Files\NotFoundException; use OCP\IUserManager; @@ -42,6 +43,8 @@ use OCP\Files\Folder; use OCP\Share\Exceptions\ShareNotFound; use OCP\Share\Exceptions\GenericShareException; +use Symfony\Component\EventDispatcher\EventDispatcher; +use Symfony\Component\EventDispatcher\GenericEvent; /** * This class is the communication hub for all sharing related operations. @@ -82,6 +85,7 @@ class Manager implements IManager { * @param IProviderFactory $factory * @param IUserManager $userManager * @param IRootFolder $rootFolder + * @param EventDispatcher $eventDispatcher */ public function __construct( ILogger $logger, @@ -93,7 +97,8 @@ class Manager implements IManager { IL10N $l, IProviderFactory $factory, IUserManager $userManager, - IRootFolder $rootFolder + IRootFolder $rootFolder, + EventDispatcher $eventDispatcher ) { $this->logger = $logger; $this->config = $config; @@ -105,6 +110,7 @@ class Manager implements IManager { $this->factory = $factory; $this->userManager = $userManager; $this->rootFolder = $rootFolder; + $this->eventDispatcher = $eventDispatcher; } /** @@ -134,16 +140,11 @@ class Manager implements IManager { } // Let others verify the password - $accepted = true; - $message = ''; - \OCP\Util::emitHook('\OC\Share', 'verifyPassword', [ - 'password' => $password, - 'accepted' => &$accepted, - 'message' => &$message - ]); - - if (!$accepted) { - throw new \Exception($message); + try { + $event = new GenericEvent($password); + $this->eventDispatcher->dispatch('OCP\PasswordPolicy::validate', $event); + } catch (HintException $e) { + throw new \Exception($e->getHint()); } } diff --git a/lib/private/user/database.php b/lib/private/user/database.php index fd273055ae..4622be1657 100644 --- a/lib/private/user/database.php +++ b/lib/private/user/database.php @@ -49,6 +49,8 @@ */ use OC\Cache\CappedMemoryCache; +use Symfony\Component\EventDispatcher\EventDispatcher; +use Symfony\Component\EventDispatcher\GenericEvent; /** * Class for user management in a SQL Database (e.g. MySQL, SQLite) @@ -59,9 +61,12 @@ class OC_User_Database extends OC_User_Backend implements \OCP\IUserBackend { /** * OC_User_Database constructor. + * + * @param EventDispatcher $eventDispatcher */ - public function __construct() { + public function __construct(EventDispatcher $eventDispatcher = null) { $this->cache = new CappedMemoryCache(); + $this->eventDispatcher = $eventDispatcher ? $eventDispatcher : \OC::$server->getEventDispatcher(); } /** @@ -113,6 +118,8 @@ class OC_User_Database extends OC_User_Backend implements \OCP\IUserBackend { */ public function setPassword($uid, $password) { if ($this->userExists($uid)) { + $event = new GenericEvent($password); + $this->eventDispatcher->dispatch('OCP\PasswordPolicy::validate', $event); $query = OC_DB::prepare('UPDATE `*PREFIX*users` SET `password` = ? WHERE `uid` = ?'); $result = $query->execute(array(\OC::$server->getHasher()->hash($password), $uid)); diff --git a/settings/changepassword/controller.php b/settings/changepassword/controller.php index 8469ec1423..7484d3a170 100644 --- a/settings/changepassword/controller.php +++ b/settings/changepassword/controller.php @@ -30,6 +30,8 @@ */ namespace OC\Settings\ChangePassword; +use OC\HintException; + class Controller { public static function changePersonalPassword($args) { // Check if we are an user @@ -39,16 +41,21 @@ class Controller { $username = \OC_User::getUser(); $password = isset($_POST['personal-password']) ? $_POST['personal-password'] : null; $oldPassword = isset($_POST['oldpassword']) ? $_POST['oldpassword'] : ''; + $l = new \OC_L10n('settings'); if (!\OC_User::checkPassword($username, $oldPassword)) { - $l = new \OC_L10n('settings'); \OC_JSON::error(array("data" => array("message" => $l->t("Wrong password")) )); exit(); } - if (!is_null($password) && \OC_User::setPassword($username, $password)) { - \OC_JSON::success(); - } else { - \OC_JSON::error(); + + try { + if (!is_null($password) && \OC_User::setPassword($username, $password)) { + \OC_JSON::success(['data' => ['message' => $l->t('Saved')]]); + } else { + \OC_JSON::error(); + } + } catch (HintException $e) { + \OC_JSON::error(['data' => ['message' => $e->getHint()]]); } } @@ -144,15 +151,19 @@ class Controller { } elseif (!$result && !$recoveryEnabledForUser) { \OC_JSON::error(array("data" => array( "message" => $l->t("Unable to change password" ) ))); } else { - \OC_JSON::success(array("data" => array( "username" => $username ))); + \OC_JSON::success(array("data" => array("username" => $username, 'message' => $l->t('Saved')))); } } } else { // if encryption is disabled, proceed - if (!is_null($password) && \OC_User::setPassword($username, $password)) { - \OC_JSON::success(array('data' => array('username' => $username))); - } else { - \OC_JSON::error(array('data' => array('message' => $l->t('Unable to change password')))); + try { + if (!is_null($password) && \OC_User::setPassword($username, $password)) { + \OC_JSON::success(array('data' => array('username' => $username, 'message' => $l->t('Saved')))); + } else { + \OC_JSON::error(array('data' => array('message' => $l->t('Unable to change password')))); + } + } catch (HintException $e) { + \OC_JSON::error(array('data' => array('message' => $e->getHint()))); } } } diff --git a/settings/js/personal.js b/settings/js/personal.js index 93820c49ff..74f61af0bb 100644 --- a/settings/js/personal.js +++ b/settings/js/personal.js @@ -185,6 +185,7 @@ $(document).ready(function () { $('#pass2').showPassword().keyup(); } $("#passwordbutton").click(function () { + OC.msg.startSaving('#password-error-msg'); var isIE8or9 = $('html').hasClass('lte9'); // FIXME - TODO - once support for IE8 and IE9 is dropped // for IE8 and IE9 this will check additionally if the typed in password @@ -201,25 +202,32 @@ $(document).ready(function () { if (data.status === "success") { $('#pass1').val(''); $('#pass2').val('').change(); - // Hide a possible errormsg and show successmsg - $('#password-changed').removeClass('hidden').addClass('inlineblock'); - $('#password-error').removeClass('inlineblock').addClass('hidden'); + OC.msg.finishedSaving('#password-error-msg', data); } else { if (typeof(data.data) !== "undefined") { - $('#password-error').text(data.data.message); + OC.msg.finishedSaving('#password-error-msg', data); } else { - $('#password-error').text(t('Unable to change password')); + OC.msg.finishedSaving('#password-error-msg', + { + 'status' : 'error', + 'data' : { + 'message' : t('core', 'Unable to change password') + } + } + ); } - // Hide a possible successmsg and show errormsg - $('#password-changed').removeClass('inlineblock').addClass('hidden'); - $('#password-error').removeClass('hidden').addClass('inlineblock'); } }); return false; } else { - // Hide a possible successmsg and show errormsg - $('#password-changed').removeClass('inlineblock').addClass('hidden'); - $('#password-error').removeClass('hidden').addClass('inlineblock'); + OC.msg.finishedSaving('#password-error-msg', + { + 'status' : 'error', + 'data' : { + 'message' : t('core', 'Unable to change password') + } + } + ); return false; } diff --git a/settings/templates/personal.php b/settings/templates/personal.php index ee79ac2392..0e1d59b010 100644 --- a/settings/templates/personal.php +++ b/settings/templates/personal.php @@ -118,8 +118,7 @@ if($_['passwordChangeSupported']) { ?>

t('Password'));?>

- - +
logger = $this->getMock('\OCP\ILogger'); $this->config = $this->getMock('\OCP\IConfig'); $this->secureRandom = $this->getMock('\OCP\Security\ISecureRandom'); @@ -81,6 +86,7 @@ class ManagerTest extends \Test\TestCase { $this->groupManager = $this->getMock('\OCP\IGroupManager'); $this->userManager = $this->getMock('\OCP\IUserManager'); $this->rootFolder = $this->getMock('\OCP\Files\IRootFolder'); + $this->eventDispatcher = $this->getMock('Symfony\Component\EventDispatcher\EventDispatcher'); $this->l = $this->getMock('\OCP\IL10N'); $this->l->method('t') @@ -100,7 +106,8 @@ class ManagerTest extends \Test\TestCase { $this->l, $this->factory, $this->userManager, - $this->rootFolder + $this->rootFolder, + $this->eventDispatcher ); $this->defaultProvider = $this->getMockBuilder('\OC\Share20\DefaultShareProvider') @@ -127,7 +134,8 @@ class ManagerTest extends \Test\TestCase { $this->l, $this->factory, $this->userManager, - $this->rootFolder + $this->rootFolder, + $this->eventDispatcher ]); } @@ -146,7 +154,7 @@ class ManagerTest extends \Test\TestCase { $group = $this->getMock('\OCP\IGroup'); $group->method('getGID')->willReturn('sharedWithGroup'); - + return [ [\OCP\Share::SHARE_TYPE_USER, 'sharedWithUser'], [\OCP\Share::SHARE_TYPE_GROUP, 'sharedWithGroup'], @@ -543,17 +551,13 @@ class ManagerTest extends \Test\TestCase { ['core', 'shareapi_enforce_links_password', 'no', 'no'], ])); - $hookListner = $this->getMockBuilder('Dummy')->setMethods(['listner'])->getMock(); - \OCP\Util::connectHook('\OC\Share', 'verifyPassword', $hookListner, 'listner'); - - $hookListner->expects($this->once()) - ->method('listner') - ->with([ - 'password' => 'password', - 'accepted' => true, - 'message' => '' - ]); - + $this->eventDispatcher->expects($this->once())->method('dispatch') + ->willReturnCallback( + function($eventName, GenericEvent $event) { + $this->assertSame('OCP\PasswordPolicy::validate', $eventName); + $this->assertSame('password', $event->getSubject()); + } + ); $result = $this->invokePrivate($this->manager, 'verifyPassword', ['password']); $this->assertNull($result); } @@ -567,8 +571,15 @@ class ManagerTest extends \Test\TestCase { ['core', 'shareapi_enforce_links_password', 'no', 'no'], ])); - $dummy = new DummyPassword(); - \OCP\Util::connectHook('\OC\Share', 'verifyPassword', $dummy, 'listner'); + $this->eventDispatcher->expects($this->once())->method('dispatch') + ->willReturnCallback( + function($eventName, GenericEvent $event) { + $this->assertSame('OCP\PasswordPolicy::validate', $eventName); + $this->assertSame('password', $event->getSubject()); + throw new HintException('message', 'password not accepted'); + } + ); + $this->invokePrivate($this->manager, 'verifyPassword', ['password']); } @@ -2022,7 +2033,8 @@ class ManagerTest extends \Test\TestCase { $this->l, $factory, $this->userManager, - $this->rootFolder + $this->rootFolder, + $this->eventDispatcher ); $share = $this->getMock('\OCP\Share\IShare'); @@ -2465,13 +2477,6 @@ class ManagerTest extends \Test\TestCase { } } -class DummyPassword { - public function listner($array) { - $array['accepted'] = false; - $array['message'] = 'password not accepted'; - } -} - class DummyFactory implements IProviderFactory { /** @var IShareProvider */ @@ -2503,4 +2508,4 @@ class DummyFactory implements IProviderFactory { public function getProviderForType($shareType) { return $this->provider; } -} \ No newline at end of file +} diff --git a/tests/lib/user/database.php b/tests/lib/user/database.php index ba44d333a8..e239ea9c3e 100644 --- a/tests/lib/user/database.php +++ b/tests/lib/user/database.php @@ -19,6 +19,9 @@ * License along with this library. If not, see . * */ +use OC\HintException; +use Symfony\Component\EventDispatcher\EventDispatcher; +use Symfony\Component\EventDispatcher\GenericEvent; /** * Class Test_User_Database @@ -28,6 +31,8 @@ class Test_User_Database extends Test_User_Backend { /** @var array */ private $users; + /** @var EventDispatcher | PHPUnit_Framework_MockObject_MockObject */ + private $eventDispatcher; public function getUser() { $user = parent::getUser(); @@ -37,7 +42,8 @@ class Test_User_Database extends Test_User_Backend { protected function setUp() { parent::setUp(); - $this->backend=new OC_User_Database(); + $this->eventDispatcher = $this->getMock('Symfony\Component\EventDispatcher\EventDispatcher'); + $this->backend=new OC_User_Database($this->eventDispatcher); } protected function tearDown() { @@ -49,4 +55,41 @@ class Test_User_Database extends Test_User_Backend { } parent::tearDown(); } + + public function testVerifyPasswordEvent() { + $user = $this->getUser(); + $this->backend->createUser($user, 'pass1'); + + $this->eventDispatcher->expects($this->once())->method('dispatch') + ->willReturnCallback( + function ($eventName, GenericEvent $event) { + $this->assertSame('OCP\PasswordPolicy::validate', $eventName); + $this->assertSame('newpass', $event->getSubject()); + } + ); + + $this->backend->setPassword($user, 'newpass'); + $this->assertSame($user, $this->backend->checkPassword($user, 'newpass')); + } + + /** + * @expectedException \OC\HintException + * @expectedExceptionMessage password change failed + */ + public function testVerifyPasswordEventFail() { + $user = $this->getUser(); + $this->backend->createUser($user, 'pass1'); + + $this->eventDispatcher->expects($this->once())->method('dispatch') + ->willReturnCallback( + function ($eventName, GenericEvent $event) { + $this->assertSame('OCP\PasswordPolicy::validate', $eventName); + $this->assertSame('newpass', $event->getSubject()); + throw new HintException('password change failed', 'password change failed'); + } + ); + + $this->backend->setPassword($user, 'newpass'); + $this->assertSame($user, $this->backend->checkPassword($user, 'newpass')); + } }