From 32a6f5f1829ec9d1037fc7a0144631702ce598c2 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Mon, 15 Aug 2016 11:11:41 +0200 Subject: [PATCH 1/2] Moved ChangePassword to an actual Controller * Still no full DI because of encryption fu * Remove old "Controller" --- settings/ChangePassword/Controller.php | 173 ------------ .../Controller/ChangePasswordController.php | 253 ++++++++++++++++++ settings/routes.php | 8 +- settings/templates/personal.php | 2 +- 4 files changed, 256 insertions(+), 180 deletions(-) delete mode 100644 settings/ChangePassword/Controller.php create mode 100644 settings/Controller/ChangePasswordController.php diff --git a/settings/ChangePassword/Controller.php b/settings/ChangePassword/Controller.php deleted file mode 100644 index 496b2f8208..0000000000 --- a/settings/ChangePassword/Controller.php +++ /dev/null @@ -1,173 +0,0 @@ - - * @author Bjoern Schiessle - * @author Björn Schießle - * @author Christopher Schäpers - * @author Clark Tomlinson - * @author cmeh - * @author Florin Peter - * @author Jakob Sack - * @author Lukas Reschke - * @author Robin Appelman - * @author Sam Tuke - * @author Thomas Müller - * - * @license AGPL-3.0 - * - * This code is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License, version 3, - * as published by the Free Software Foundation. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. - * - * You should have received a copy of the GNU Affero General Public License, version 3, - * along with this program. If not, see - * - */ -namespace OC\Settings\ChangePassword; - -use OC\HintException; - -class Controller { - public static function changePersonalPassword($args) { - // Check if we are an user - \OC_JSON::callCheck(); - \OC_JSON::checkLoggedIn(); - - $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)) { - \OC_JSON::error(array("data" => array("message" => $l->t("Wrong password")) )); - exit(); - } - - try { - if (!is_null($password) && \OC_User::setPassword($username, $password)) { - \OC::$server->getUserSession()->updateSessionTokenPassword($password); - \OC_JSON::success(['data' => ['message' => $l->t('Saved')]]); - } else { - \OC_JSON::error(); - } - } catch (HintException $e) { - \OC_JSON::error(['data' => ['message' => $e->getHint()]]); - } - } - - public static function changeUserPassword($args) { - // Check if we are an user - \OC_JSON::callCheck(); - \OC_JSON::checkLoggedIn(); - - $l = new \OC_L10n('settings'); - if (isset($_POST['username'])) { - $username = $_POST['username']; - } else { - \OC_JSON::error(array('data' => array('message' => $l->t('No user supplied')) )); - exit(); - } - - $password = isset($_POST['password']) ? $_POST['password'] : null; - $recoveryPassword = isset($_POST['recoveryPassword']) ? $_POST['recoveryPassword'] : null; - - $isUserAccessible = false; - $currentUserObject = \OC::$server->getUserSession()->getUser(); - $targetUserObject = \OC::$server->getUserManager()->get($username); - if($currentUserObject !== null && $targetUserObject !== null) { - $isUserAccessible = \OC::$server->getGroupManager()->getSubAdmin()->isUserAccessible($currentUserObject, $targetUserObject); - } - - if (\OC_User::isAdminUser(\OC_User::getUser())) { - $userstatus = 'admin'; - } elseif ($isUserAccessible) { - $userstatus = 'subadmin'; - } else { - \OC_JSON::error(array('data' => array('message' => $l->t('Authentication error')) )); - exit(); - } - - if (\OC_App::isEnabled('encryption')) { - //handle the recovery case - $crypt = new \OCA\Encryption\Crypto\Crypt( - \OC::$server->getLogger(), - \OC::$server->getUserSession(), - \OC::$server->getConfig(), - \OC::$server->getL10N('encryption')); - $keyStorage = \OC::$server->getEncryptionKeyStorage(); - $util = new \OCA\Encryption\Util( - new \OC\Files\View(), - $crypt, - \OC::$server->getLogger(), - \OC::$server->getUserSession(), - \OC::$server->getConfig(), - \OC::$server->getUserManager()); - $keyManager = new \OCA\Encryption\KeyManager( - $keyStorage, - $crypt, - \OC::$server->getConfig(), - \OC::$server->getUserSession(), - new \OCA\Encryption\Session(\OC::$server->getSession()), - \OC::$server->getLogger(), - $util); - $recovery = new \OCA\Encryption\Recovery( - \OC::$server->getUserSession(), - $crypt, - \OC::$server->getSecureRandom(), - $keyManager, - \OC::$server->getConfig(), - $keyStorage, - \OC::$server->getEncryptionFilesHelper(), - new \OC\Files\View()); - $recoveryAdminEnabled = $recovery->isRecoveryKeyEnabled(); - - $validRecoveryPassword = false; - $recoveryEnabledForUser = false; - if ($recoveryAdminEnabled) { - $validRecoveryPassword = $keyManager->checkRecoveryPassword($recoveryPassword); - $recoveryEnabledForUser = $recovery->isRecoveryEnabledForUser($username); - } - - if ($recoveryEnabledForUser && $recoveryPassword === '') { - \OC_JSON::error(array('data' => array( - 'message' => $l->t('Please provide an admin recovery password, otherwise all user data will be lost') - ))); - } elseif ($recoveryEnabledForUser && ! $validRecoveryPassword) { - \OC_JSON::error(array('data' => array( - 'message' => $l->t('Wrong admin recovery password. Please check the password and try again.') - ))); - } else { // now we know that everything is fine regarding the recovery password, let's try to change the password - $result = \OC_User::setPassword($username, $password, $recoveryPassword); - if (!$result && $recoveryEnabledForUser) { - \OC_JSON::error(array( - "data" => array( - "message" => $l->t("Backend doesn't support password change, but the user's encryption key was successfully updated.") - ) - )); - } 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 ))); - } - - } - } else { // if encryption is disabled, proceed - try { - 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')))); - } - } catch (HintException $e) { - \OC_JSON::error(array('data' => array('message' => $e->getHint()))); - } - } - } -} diff --git a/settings/Controller/ChangePasswordController.php b/settings/Controller/ChangePasswordController.php new file mode 100644 index 0000000000..74abd8b57d --- /dev/null +++ b/settings/Controller/ChangePasswordController.php @@ -0,0 +1,253 @@ + + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ +namespace OC\Settings\Controller; + +use OCP\App\IAppManager; +use OCP\AppFramework\Controller; +use OCP\AppFramework\Http\JSONResponse; +use OCP\IGroupManager; +use OCP\IL10N; +use OCP\IRequest; +use OCP\IUser; +use OCP\IUserManager; +use OCP\IUserSession; + +class ChangePasswordController extends Controller { + + /** @var string */ + private $userId; + + /** @var IUserManager */ + private $userManager; + + /** @var IL10N */ + private $l; + + /** @var IGroupManager */ + private $groupManager; + + /** @var IUserSession */ + private $userSession; + + /** @var IAppManager */ + private $appManager; + + /** + * ChangePasswordController constructor. + * + * @param string $appName + * @param IRequest $request + * @param $userId + * @param IUserManager $userManager + * @param IUserSession $userSession + * @param IGroupManager $groupManager + * @param IAppManager $appManager + * @param IL10N $l + */ + public function __construct($appName, + IRequest $request, + $userId, + IUserManager $userManager, + IUserSession $userSession, + IGroupManager $groupManager, + IAppManager $appManager, + IL10N $l) { + parent::__construct($appName, $request); + + $this->userId = $userId; + $this->userManager = $userManager; + $this->userSession = $userSession; + $this->groupManager = $groupManager; + $this->appManager = $appManager; + $this->l = $l; + } + + /** + * @NoAdminRequired + * + * @param string $oldpassword + * @param string $newpassword + * + * @return JSONResponse + */ + public function changePersonalPassword($oldpassword = '', $newpassword = null) { + $user = $this->userManager->checkPassword($this->userId, $oldpassword); + if ($user === false) { + return new JSONResponse([ + 'status' => 'error', + 'data' => [ + 'message' => $this->l->t('Wrong password'), + ], + ]); + } + + /** @var IUser $user */ + if ($newpassword === null || $user->setPassword($newpassword) === false) { + return new JSONResponse([ + 'status' => 'error' + ]); + } + + $this->userSession->updateSessionTokenPassword($newpassword); + + return new JSONResponse([ + 'status' => 'success', + 'data' => [ + 'message' => $this->l->t('Saved'), + ], + ]); + } + + /** + * @NoAdminRequired + * + * @param string $username + * @param string $password + * @param string $recoveryPassword + * + * @return JSONResponse + */ + public function changeUserPassword($username = null, $password = null, $recoveryPassword = null) { + if ($username === null) { + return new JSONResponse([ + 'status' => 'error', + 'data' => [ + 'message' => $this->l->t('No user supplied'), + ], + ]); + } + + if ($password === null) { + return new JSONResponse([ + 'status' => 'error', + 'data' => [ + 'message' => $this->l->t('Unable to change password'), + ], + ]); + } + + $currentUser = $this->userSession->getUser(); + $targetUser = $this->userManager->get($username); + if ($currentUser === null || $targetUser === null || + !($this->groupManager->isAdmin($this->userId) || + $this->groupManager->getSubAdmin()->isUserAccessible($currentUser, $targetUser)) + ) { + return new JSONResponse([ + 'status' => 'error', + 'data' => [ + 'message' => $this->l->t('Authentication error'), + ], + ]); + } + + if ($this->appManager->isEnabledForUser('encryption')) { + //handle the recovery case + $crypt = new \OCA\Encryption\Crypto\Crypt( + \OC::$server->getLogger(), + \OC::$server->getUserSession(), + \OC::$server->getConfig(), + \OC::$server->getL10N('encryption')); + $keyStorage = \OC::$server->getEncryptionKeyStorage(); + $util = new \OCA\Encryption\Util( + new \OC\Files\View(), + $crypt, + \OC::$server->getLogger(), + \OC::$server->getUserSession(), + \OC::$server->getConfig(), + \OC::$server->getUserManager()); + $keyManager = new \OCA\Encryption\KeyManager( + $keyStorage, + $crypt, + \OC::$server->getConfig(), + \OC::$server->getUserSession(), + new \OCA\Encryption\Session(\OC::$server->getSession()), + \OC::$server->getLogger(), + $util); + $recovery = new \OCA\Encryption\Recovery( + \OC::$server->getUserSession(), + $crypt, + \OC::$server->getSecureRandom(), + $keyManager, + \OC::$server->getConfig(), + $keyStorage, + \OC::$server->getEncryptionFilesHelper(), + new \OC\Files\View()); + $recoveryAdminEnabled = $recovery->isRecoveryKeyEnabled(); + + $validRecoveryPassword = false; + $recoveryEnabledForUser = false; + if ($recoveryAdminEnabled) { + $validRecoveryPassword = $keyManager->checkRecoveryPassword($recoveryPassword); + $recoveryEnabledForUser = $recovery->isRecoveryEnabledForUser($username); + } + + if ($recoveryEnabledForUser && $recoveryPassword === '') { + return new JSONResponse([ + 'status' => 'error', + 'data' => [ + 'message' => $this->l->t('Please provide an admin recovery password, otherwise all user data will be lost'), + ] + ]); + } elseif ($recoveryEnabledForUser && ! $validRecoveryPassword) { + return new JSONResponse([ + 'status' => 'error', + 'data' => [ + 'message' => $this->l->t('Wrong admin recovery password. Please check the password and try again.'), + ] + ]); + } else { // now we know that everything is fine regarding the recovery password, let's try to change the password + $result = $targetUser->setPassword($password, $recoveryPassword); + if (!$result && $recoveryEnabledForUser) { + return new JSONResponse([ + 'status' => 'error', + 'data' => [ + 'message' => $this->l->t('Backend doesn\'t support password change, but the user\'s encryption key was successfully updated.'), + ] + ]); + } elseif (!$result && !$recoveryEnabledForUser) { + return new JSONResponse([ + 'status' => 'error', + 'data' => [ + 'message' => $this->l->t('Unable to change password'), + ] + ]); + } + } + } else { + if ($targetUser->setPassword($password) === false) { + return new JSONResponse([ + 'status' => 'error', + 'data' => [ + 'message' => $this->l->t('Unable to change password'), + ], + ]); + } + } + + return new JSONResponse([ + 'status' => 'success', + 'data' => [ + 'username' => $username, + ], + ]); + } +} diff --git a/settings/routes.php b/settings/routes.php index f77da543e1..64c4e54968 100644 --- a/settings/routes.php +++ b/settings/routes.php @@ -66,6 +66,8 @@ $application->registerRoutes($this, [ ['name' => 'Certificate#removeSystemRootCertificate', 'url' => '/settings/admin/certificate/{certificateIdentifier}', 'verb' => 'DELETE'], ['name' => 'AdminSettings#index', 'url' => '/settings/admin/{section}', 'verb' => 'GET', 'defaults' => ['section' => 'server']], ['name' => 'AdminSettings#form', 'url' => '/settings/admin/{section}', 'verb' => 'GET'], + ['name' => 'ChangePassword#changePersonalPassword', 'url' => '/settings/personal/changepassword', 'verb' => 'POST'], + ['name' => 'ChangePassword#changeUserPassword', 'url' => '/settings/users/changepassword', 'verb' => 'POST'], ] ]); @@ -86,15 +88,9 @@ $this->create('settings_ajax_togglegroups', '/settings/ajax/togglegroups.php') ->actionInclude('settings/ajax/togglegroups.php'); $this->create('settings_ajax_togglesubadmins', '/settings/ajax/togglesubadmins.php') ->actionInclude('settings/ajax/togglesubadmins.php'); -$this->create('settings_users_changepassword', '/settings/users/changepassword') - ->post() - ->action('OC\Settings\ChangePassword\Controller', 'changeUserPassword'); $this->create('settings_ajax_changegorupname', '/settings/ajax/changegroupname.php') ->actionInclude('settings/ajax/changegroupname.php'); // personal -$this->create('settings_personal_changepassword', '/settings/personal/changepassword') - ->post() - ->action('OC\Settings\ChangePassword\Controller', 'changePersonalPassword'); $this->create('settings_ajax_setlanguage', '/settings/ajax/setlanguage.php') ->actionInclude('settings/ajax/setlanguage.php'); // apps diff --git a/settings/templates/personal.php b/settings/templates/personal.php index 41023018b1..35ba65384b 100644 --- a/settings/templates/personal.php +++ b/settings/templates/personal.php @@ -125,7 +125,7 @@ if($_['passwordChangeSupported']) { placeholder="t('Current password');?>" autocomplete="off" autocapitalize="off" autocorrect="off" /> - From 789082e014bc7b4ecea53be088521d41407fed7b Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Mon, 15 Aug 2016 21:07:57 +0200 Subject: [PATCH 2/2] Add tests for ChangePasswordController --- .../ChangePasswordControllerTest.php | 161 ++++++++++++++++++ 1 file changed, 161 insertions(+) create mode 100644 tests/Core/Controller/ChangePasswordControllerTest.php diff --git a/tests/Core/Controller/ChangePasswordControllerTest.php b/tests/Core/Controller/ChangePasswordControllerTest.php new file mode 100644 index 0000000000..8dd4ca8db9 --- /dev/null +++ b/tests/Core/Controller/ChangePasswordControllerTest.php @@ -0,0 +1,161 @@ + + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ +namespace Tests\Core\Controller; + +use OC\Settings\Controller\ChangePasswordController; +use OC\User\Session; +use OCP\App\IAppManager; +use OCP\IGroupManager; +use OCP\IL10N; +use OCP\IUserManager; + +class ChangePasswordControllerTest extends \Test\TestCase { + + /** @var string */ + private $userId = 'currentUser'; + + /** @var IUserManager */ + private $userManager; + + /** @var Session */ + private $userSession; + + /** @var IGroupManager */ + private $groupManager; + + /** @var IAppManager */ + private $appManager; + + /** @var IL10N */ + private $l; + + /** @var ChangePasswordController */ + private $controller; + + public function setUp() { + parent::setUp(); + + $this->userManager = $this->getMockBuilder('OCP\IUserManager')->getMock(); + $this->userSession = $this->getMockBuilder('OC\User\Session')->disableOriginalConstructor()->getMock(); + $this->groupManager = $this->getMockBuilder('OCP\IGroupManager')->getMock(); + $this->appManager = $this->getMockBuilder('OCP\App\IAppManager')->getMock(); + $this->l = $this->getMockBuilder('OCP\IL10N')->getMock(); + + $this->l->method('t')->will($this->returnArgument(0)); + + $request = $this->getMockBuilder('OCP\IRequest')->getMock(); + + $this->controller = new ChangePasswordController( + 'core', + $request, + $this->userId, + $this->userManager, + $this->userSession, + $this->groupManager, + $this->appManager, + $this->l + ); + } + + public function testChangePersonalPasswordWrongPassword() { + $this->userManager->expects($this->once()) + ->method('checkPassword') + ->with($this->userId, 'old') + ->willReturn(false); + + $expects = [ + 'status' => 'error', + 'data' => [ + 'message' => 'Wrong password', + ], + ]; + + $res = $this->controller->changePersonalPassword('old', 'new'); + + $this->assertEquals($expects, $res->getData()); + } + + public function testChangePersonalPasswordNoNewPassword() { + $user = $this->getMockBuilder('OCP\IUser')->getMock(); + $this->userManager->expects($this->once()) + ->method('checkPassword') + ->with($this->userId, 'old') + ->willReturn($user); + + $expects = [ + 'status' => 'error', + ]; + + $res = $this->controller->changePersonalPassword('old'); + + $this->assertEquals($expects, $res->getData()); + } + + public function testChangePersonalPasswordCantSetPassword() { + $user = $this->getMockBuilder('OCP\IUser')->getMock(); + $this->userManager->expects($this->once()) + ->method('checkPassword') + ->with($this->userId, 'old') + ->willReturn($user); + + $user->expects($this->once()) + ->method('setPassword') + ->with('new') + ->willReturn(false); + + $expects = [ + 'status' => 'error', + ]; + + $res = $this->controller->changePersonalPassword('old', 'new'); + + $this->assertEquals($expects, $res->getData()); + } + + public function testChangePersonalPassword() { + $user = $this->getMockBuilder('OCP\IUser')->getMock(); + $this->userManager->expects($this->once()) + ->method('checkPassword') + ->with($this->userId, 'old') + ->willReturn($user); + + $user->expects($this->once()) + ->method('setPassword') + ->with('new') + ->willReturn(true); + + $this->userSession->expects($this->once()) + ->method('updateSessionTokenPassword') + ->with('new'); + + $expects = [ + 'status' => 'success', + 'data' => [ + 'message' => 'Saved', + ], + ]; + + $res = $this->controller->changePersonalPassword('old', 'new'); + + $this->assertEquals($expects, $res->getData()); + } +}