Add bruteforce protection to changePersonalPassword

While the risk is actually quite low because one would already have the user session and could potentially do other havoc it makes sense to throttle here in case of invalid previous password attempts.

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

diff --git a/settings/Controller/ChangePasswordController.php b/settings/Controller/ChangePasswordController.php
index b82751bcac2a5a2341b5f04e5531b018df7a3f26..2f61d36c3ff9c65d795c2800f18768976d49f372 100644 (file)
--- a/settings/Controller/ChangePasswordController.php
+++ b/settings/Controller/ChangePasswordController.php
@@ -85,6 +85,7 @@ class ChangePasswordController extends Controller {
        /**
         * @NoAdminRequired
         * @NoSubadminRequired
+        * @BruteForceProtection(action=changePersonalPassword)
         *
         * @param string $oldpassword
         * @param string $newpassword
@@ -95,12 +96,14 @@ class ChangePasswordController extends Controller {
                /** @var IUser $user */
                $user = $this->userManager->checkPassword($this->userId, $oldpassword);
                if ($user === false) {
-                       return new JSONResponse([
+                       $response = new JSONResponse([
                                'status' => 'error',
                                'data' => [
                                        'message' => $this->l->t('Wrong password'),
                                ],
                        ]);
+                       $response->throttle();
+                       return $response;
                }
 
                try {

diff --git a/tests/Core/Controller/ChangePasswordControllerTest.php b/tests/Core/Controller/ChangePasswordControllerTest.php
index 869ef98b514d670d355cf4c3c066e4f100b8813f..c426bae9974a2857e96bf3ff8b7cc252c0699c78 100644 (file)
--- a/tests/Core/Controller/ChangePasswordControllerTest.php
+++ b/tests/Core/Controller/ChangePasswordControllerTest.php
@@ -25,45 +25,40 @@ use OC\HintException;
 use OC\Settings\Controller\ChangePasswordController;
 use OC\User\Session;
 use OCP\App\IAppManager;
+use OCP\AppFramework\Http\JSONResponse;
 use OCP\IGroupManager;
 use OCP\IL10N;
+use OCP\IRequest;
 use OCP\IUserManager;
 
 class ChangePasswordControllerTest extends \Test\TestCase {
-
        /** @var string */
        private $userId = 'currentUser';
-
-       /** @var IUserManager */
+       /** @var IUserManager|\PHPUnit_Framework_MockObject_MockObject */
        private $userManager;
-
-       /** @var Session */
+       /** @var Session|\PHPUnit_Framework_MockObject_MockObject */
        private $userSession;
-
-       /** @var IGroupManager */
+       /** @var IGroupManager|\PHPUnit_Framework_MockObject_MockObject */
        private $groupManager;
-
-       /** @var IAppManager */
+       /** @var IAppManager|\PHPUnit_Framework_MockObject_MockObject */
        private $appManager;
-
-       /** @var IL10N */
+       /** @var IL10N|\PHPUnit_Framework_MockObject_MockObject */
        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->userManager = $this->createMock(IUserManager::class);
+               $this->userSession = $this->createMock(Session::class);
+               $this->groupManager = $this->createMock(IGroupManager::class);
+               $this->appManager = $this->createMock(IAppManager::class);
+               $this->l = $this->createMock(IL10N::class);
                $this->l->method('t')->will($this->returnArgument(0));
 
-               $request = $this->getMockBuilder('OCP\IRequest')->getMock();
+               /** @var IRequest|\PHPUnit_Framework_MockObject_MockObject $request */
+               $request = $this->createMock(IRequest::class);
 
                $this->controller = new ChangePasswordController(
                        'core',
@@ -83,16 +78,16 @@ class ChangePasswordControllerTest extends \Test\TestCase {
                        ->with($this->userId, 'old')
                        ->willReturn(false);
 
-               $expects = [
+               $expects = new JSONResponse([
                        'status' => 'error',
                        'data' => [
                                'message' => 'Wrong password',
                        ],
-               ];
-
-               $res = $this->controller->changePersonalPassword('old', 'new');
+               ]);
+               $expects->throttle();
 
-               $this->assertEquals($expects, $res->getData());
+               $actual = $this->controller->changePersonalPassword('old', 'new');
+               $this->assertEquals($expects, $actual);
        }
 
        public function testChangePersonalPasswordCommonPassword() {
@@ -107,16 +102,15 @@ class ChangePasswordControllerTest extends \Test\TestCase {
                        ->with('new')
                        ->will($this->throwException(new HintException('Common password')));
 
-               $expects = [
+               $expects = new JSONResponse([
                        'status' => 'error',
                        'data' => [
                                'message' => 'Common password',
                        ],
-               ];
-
-               $res = $this->controller->changePersonalPassword('old', 'new');
+               ]);
 
-               $this->assertEquals($expects, $res->getData());
+               $actual = $this->controller->changePersonalPassword('old', 'new');
+               $this->assertEquals($expects, $actual);
        }
 
        public function testChangePersonalPasswordNoNewPassword() {
@@ -147,13 +141,12 @@ class ChangePasswordControllerTest extends \Test\TestCase {
                        ->with('new')
                        ->willReturn(false);
 
-               $expects = [
+               $expects = new JSONResponse([
                        'status' => 'error',
-               ];
+               ]);
 
-               $res = $this->controller->changePersonalPassword('old', 'new');
-
-               $this->assertEquals($expects, $res->getData());
+               $actual = $this->controller->changePersonalPassword('old', 'new');
+               $this->assertEquals($expects, $actual);
        }
 
        public function testChangePersonalPassword() {
@@ -172,15 +165,14 @@ class ChangePasswordControllerTest extends \Test\TestCase {
                        ->method('updateSessionTokenPassword')
                        ->with('new');
 
-               $expects = [
+               $expects = new JSONResponse([
                        'status' => 'success',
                        'data' => [
                                'message' => 'Saved',
                        ],
-               ];
-
-               $res = $this->controller->changePersonalPassword('old', 'new');
+               ]);
 
-               $this->assertEquals($expects, $res->getData());
+               $actual = $this->controller->changePersonalPassword('old', 'new');
+               $this->assertEquals($expects, $actual);
        }
 }