Adjust existing bruteforce protection code

- Moves code to annotation
- Adds the `throttle()` call on the responses on existing annotations

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

diff --git a/apps/federatedfilesharing/lib/Controller/MountPublicLinkController.php b/apps/federatedfilesharing/lib/Controller/MountPublicLinkController.php
index 9f848fbbb7832857ff5d52dc8783524684c4ba50..5cdba0cfffd4ea7845391988dec09ef636a22511 100644 (file)
--- a/apps/federatedfilesharing/lib/Controller/MountPublicLinkController.php
+++ b/apps/federatedfilesharing/lib/Controller/MountPublicLinkController.php
@@ -148,10 +148,12 @@ class MountPublicLinkController extends Controller {
                $authenticated = $this->session->get('public_link_authenticated') === $share->getId() ||
                        $this->shareManager->checkPassword($share, $password);
                if (!empty($storedPassword) && !$authenticated ) {
-                       return new JSONResponse(
+                       $response = new JSONResponse(
                                ['message' => 'No permission to access the share'],
                                Http::STATUS_BAD_REQUEST
                        );
+                       $response->throttle();
+                       return $response;
                }
 
                $share->setSharedWith($shareWith);

diff --git a/apps/files_sharing/lib/Controller/ShareController.php b/apps/files_sharing/lib/Controller/ShareController.php
index 732a1d32ee76f1e3c050461e1201a1aa1f654fd3..759d5ee41632a4d8a1014c5e4e57bfc822a310bb 100644 (file)
--- a/apps/files_sharing/lib/Controller/ShareController.php
+++ b/apps/files_sharing/lib/Controller/ShareController.php
@@ -182,7 +182,9 @@ class ShareController extends Controller {
                        return new RedirectResponse($this->urlGenerator->linkToRoute('files_sharing.sharecontroller.showShare', array('token' => $token)));
                }
 
-               return new TemplateResponse($this->appName, 'authenticate', array('wrongpw' => true), 'guest');
+               $response = new TemplateResponse($this->appName, 'authenticate', array('wrongpw' => true), 'guest');
+               $response->throttle();
+               return $response;
        }
 
        /**

diff --git a/apps/files_sharing/tests/Controller/ShareControllerTest.php b/apps/files_sharing/tests/Controller/ShareControllerTest.php
index c9a1d5ecb24645a42fb1916ad11b02d3e0dff579..62adca53f4c3e1f80c287d573d08dc67e63360c9 100644 (file)
--- a/apps/files_sharing/tests/Controller/ShareControllerTest.php
+++ b/apps/files_sharing/tests/Controller/ShareControllerTest.php
@@ -280,6 +280,7 @@ class ShareControllerTest extends \Test\TestCase {
 
                $response = $this->shareController->authenticate('token', 'invalidpassword');
                $expectedResponse =  new TemplateResponse($this->appName, 'authenticate', array('wrongpw' => true), 'guest');
+               $expectedResponse->throttle();
                $this->assertEquals($expectedResponse, $response);
        }
 

diff --git a/apps/user_ldap/lib/Controller/ConfigAPIController.php b/apps/user_ldap/lib/Controller/ConfigAPIController.php
index 7d51b0aafe4be47191c96fcebc35968224f17735..54800ef24eb2784365e2d004c5fd2825d91d9024 100644 (file)
--- a/apps/user_ldap/lib/Controller/ConfigAPIController.php
+++ b/apps/user_ldap/lib/Controller/ConfigAPIController.php
@@ -25,7 +25,6 @@ namespace OCA\User_LDAP\Controller;
 
 use OC\CapabilitiesManager;
 use OC\Core\Controller\OCSController;
-use OC\Security\Bruteforce\Throttler;
 use OC\Security\IdentityProof\Manager;
 use OCA\User_LDAP\Configuration;
 use OCA\User_LDAP\Helper;
@@ -52,7 +51,6 @@ class ConfigAPIController extends OCSController {
                CapabilitiesManager $capabilitiesManager,
                IUserSession $userSession,
                IUserManager $userManager,
-               Throttler $throttler,
                Manager $keyManager,
                Helper $ldapHelper,
                ILogger $logger
@@ -63,7 +61,6 @@ class ConfigAPIController extends OCSController {
                        $capabilitiesManager,
                        $userSession,
                        $userManager,
-                       $throttler,
                        $keyManager
                );
 

diff --git a/core/Controller/LostController.php b/core/Controller/LostController.php
index 4597124897be745841660396448ebfa8992a8ab6..7a2590094b5228cd03b0d30b6574ec39a63a97a1 100644 (file)
--- a/core/Controller/LostController.php
+++ b/core/Controller/LostController.php
@@ -32,6 +32,7 @@ namespace OC\Core\Controller;
 
 use OCA\Encryption\Exceptions\PrivateKeyMissingException;
 use \OCP\AppFramework\Controller;
+use OCP\AppFramework\Http\JSONResponse;
 use \OCP\AppFramework\Http\TemplateResponse;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Defaults;
@@ -207,17 +208,21 @@ class LostController extends Controller {
         * @BruteForceProtection(action=passwordResetEmail)
         *
         * @param string $user
-        * @return array
+        * @return JSONResponse
         */
        public function email($user){
                // FIXME: use HTTP error codes
                try {
                        $this->sendEmail($user);
                } catch (\Exception $e){
-                       return $this->error($e->getMessage());
+                       $response = new JSONResponse($this->error($e->getMessage()));
+                       $response->throttle();
+                       return $response;
                }
 
-               return $this->success();
+               $response = new JSONResponse($this->success());
+               $response->throttle();
+               return $response;
        }
 
        /**

diff --git a/core/Controller/OCSController.php b/core/Controller/OCSController.php
index 1deb5e958bdfb7e619a3179f25d136d9fb341ddf..a709ab7b07bf2672924315aa1f4c22bf49f449a3 100644 (file)
--- a/core/Controller/OCSController.php
+++ b/core/Controller/OCSController.php
@@ -22,7 +22,6 @@
 namespace OC\Core\Controller;
 
 use OC\CapabilitiesManager;
-use OC\Security\Bruteforce\Throttler;
 use OC\Security\IdentityProof\Manager;
 use OCP\AppFramework\Http\DataResponse;
 use OCP\IRequest;
@@ -39,8 +38,6 @@ class OCSController extends \OCP\AppFramework\OCSController {
        private $userManager;
        /** @var Manager */
        private $keyManager;
-       /** @var Throttler */
-       private $throttler;
 
        /**
         * OCSController constructor.
@@ -50,7 +47,6 @@ class OCSController extends \OCP\AppFramework\OCSController {
         * @param CapabilitiesManager $capabilitiesManager
         * @param IUserSession $userSession
         * @param IUserManager $userManager
-        * @param Throttler $throttler
         * @param Manager $keyManager
         */
        public function __construct($appName,
@@ -58,13 +54,11 @@ class OCSController extends \OCP\AppFramework\OCSController {
                                                                CapabilitiesManager $capabilitiesManager,
                                                                IUserSession $userSession,
                                                                IUserManager $userManager,
-                                                               Throttler $throttler,
                                                                Manager $keyManager) {
                parent::__construct($appName, $request);
                $this->capabilitiesManager = $capabilitiesManager;
                $this->userSession = $userSession;
                $this->userManager = $userManager;
-               $this->throttler = $throttler;
                $this->keyManager = $keyManager;
        }
 
@@ -107,6 +101,7 @@ class OCSController extends \OCP\AppFramework\OCSController {
 
        /**
         * @PublicPage
+        * @BruteForceProtection(action=login)
         *
         * @param string $login
         * @param string $password
@@ -114,7 +109,6 @@ class OCSController extends \OCP\AppFramework\OCSController {
         */
        public function personCheck($login = '', $password = '') {
                if ($login !== '' && $password !== '') {
-                       $this->throttler->sleepDelay($this->request->getRemoteAddress(), 'login');
                        if ($this->userManager->checkPassword($login, $password)) {
                                return new DataResponse([
                                        'person' => [
@@ -122,8 +116,10 @@ class OCSController extends \OCP\AppFramework\OCSController {
                                        ]
                                ]);
                        }
-                       $this->throttler->registerAttempt('login', $this->request->getRemoteAddress());
-                       return new DataResponse(null, 102);
+
+                       $response = new DataResponse(null, 102);
+                       $response->throttle();
+                       return $response;
                }
                return new DataResponse(null, 101);
        }

diff --git a/tests/Core/Controller/LostControllerTest.php b/tests/Core/Controller/LostControllerTest.php
index 539fe016c8b5934d6e3cefdfa2a2f169550949e9..ab3f022c971ecde940fdf0f8b35a258d99fb142e 100644 (file)
--- a/tests/Core/Controller/LostControllerTest.php
+++ b/tests/Core/Controller/LostControllerTest.php
@@ -23,6 +23,7 @@ namespace Tests\Core\Controller;
 
 use OC\Core\Controller\LostController;
 use OC\Mail\Message;
+use OCP\AppFramework\Http\JSONResponse;
 use OCP\AppFramework\Http\TemplateResponse;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Defaults;
@@ -245,7 +246,7 @@ class LostControllerTest extends \Test\TestCase {
                $this->assertEquals($expectedResponse, $response);
        }
 
-       public function testEmailUnsucessful() {
+       public function testEmailUnsuccessful() {
                $existingUser = 'ExistingUser';
                $nonExistingUser = 'NonExistingUser';
                $this->userManager
@@ -258,11 +259,12 @@ class LostControllerTest extends \Test\TestCase {
 
                // With a non existing user
                $response = $this->lostController->email($nonExistingUser);
-               $expectedResponse = [
+               $expectedResponse = new JSONResponse([
                        'status' => 'error',
                        'msg' => 'Couldn\'t send reset email. Please make sure your username is correct.'
-               ];
-               $this->assertSame($expectedResponse, $response);
+               ]);
+               $expectedResponse->throttle();
+               $this->assertEquals($expectedResponse, $response);
 
                // With no mail address
                $this->config
@@ -271,11 +273,12 @@ class LostControllerTest extends \Test\TestCase {
                        ->with($existingUser, 'settings', 'email')
                        ->will($this->returnValue(null));
                $response = $this->lostController->email($existingUser);
-               $expectedResponse = [
+               $expectedResponse = new JSONResponse([
                        'status' => 'error',
                        'msg' => 'Couldn\'t send reset email. Please make sure your username is correct.'
-               ];
-               $this->assertSame($expectedResponse, $response);
+               ]);
+               $expectedResponse->throttle();
+               $this->assertEquals($expectedResponse, $response);
        }
 
        public function testEmailSuccessful() {
@@ -355,8 +358,9 @@ class LostControllerTest extends \Test\TestCase {
                        )->willReturn('encryptedToken');
 
                $response = $this->lostController->email('ExistingUser');
-               $expectedResponse = array('status' => 'success');
-               $this->assertSame($expectedResponse, $response);
+               $expectedResponse = new JSONResponse(['status' => 'success']);
+               $expectedResponse->throttle();
+               $this->assertEquals($expectedResponse, $response);
        }
 
        public function testEmailWithMailSuccessful() {
@@ -441,8 +445,9 @@ class LostControllerTest extends \Test\TestCase {
                        )->willReturn('encryptedToken');
 
                $response = $this->lostController->email('test@example.com');
-               $expectedResponse = array('status' => 'success');
-               $this->assertSame($expectedResponse, $response);
+               $expectedResponse = new JSONResponse(['status' => 'success']);
+               $expectedResponse->throttle();
+               $this->assertEquals($expectedResponse, $response);
        }
 
        public function testEmailCantSendException() {
@@ -522,8 +527,9 @@ class LostControllerTest extends \Test\TestCase {
                        )->willReturn('encryptedToken');
 
                $response = $this->lostController->email('ExistingUser');
-               $expectedResponse = ['status' => 'error', 'msg' => 'Couldn\'t send reset email. Please contact your administrator.'];
-               $this->assertSame($expectedResponse, $response);
+               $expectedResponse = new JSONResponse(['status' => 'error', 'msg' => 'Couldn\'t send reset email. Please contact your administrator.']);
+               $expectedResponse->throttle();
+               $this->assertEquals($expectedResponse, $response);
        }
 
        public function testSetPasswordUnsuccessful() {
@@ -692,8 +698,9 @@ class LostControllerTest extends \Test\TestCase {
                        ->willReturn($user);
 
                $response = $this->lostController->email('ExistingUser');
-               $expectedResponse = ['status' => 'error', 'msg' => 'Could not send reset email because there is no email address for this username. Please contact your administrator.'];
-               $this->assertSame($expectedResponse, $response);
+               $expectedResponse = new JSONResponse(['status' => 'error', 'msg' => 'Could not send reset email because there is no email address for this username. Please contact your administrator.']);
+               $expectedResponse->throttle();
+               $this->assertEquals($expectedResponse, $response);
        }
 
        public function testSetPasswordEncryptionDontProceed() {

diff --git a/tests/Core/Controller/OCSControllerTest.php b/tests/Core/Controller/OCSControllerTest.php
index 7241df9317cd544c202a5b968913312a56c71c9e..e6066a801427308724d78992810e16e521636316 100644 (file)
--- a/tests/Core/Controller/OCSControllerTest.php
+++ b/tests/Core/Controller/OCSControllerTest.php
@@ -42,8 +42,6 @@ class OCSControllerTest extends TestCase {
        private $userSession;
        /** @var IUserManager|\PHPUnit_Framework_MockObject_MockObject */
        private $userManager;
-       /** @var Throttler|\PHPUnit_Framework_MockObject_MockObject */
-       private $throttler;
        /** @var Manager|\PHPUnit_Framework_MockObject_MockObject */
        private $keyManager;
        /** @var OCSController */
@@ -56,7 +54,6 @@ class OCSControllerTest extends TestCase {
                $this->capabilitiesManager = $this->createMock(CapabilitiesManager::class);
                $this->userSession = $this->createMock(IUserSession::class);
                $this->userManager = $this->createMock(IUserManager::class);
-               $this->throttler = $this->createMock(Throttler::class);
                $this->keyManager = $this->createMock(Manager::class);
 
                $this->controller = new OCSController(
@@ -65,7 +62,6 @@ class OCSControllerTest extends TestCase {
                        $this->capabilitiesManager,
                        $this->userSession,
                        $this->userManager,
-                       $this->throttler,
                        $this->keyManager
                );
        }
@@ -117,16 +113,6 @@ class OCSControllerTest extends TestCase {
        }
 
        public function testPersonCheckValid() {
-               $this->request->method('getRemoteAddress')
-                       ->willReturn('1.2.3.4');
-
-               $this->throttler->expects($this->once())
-                       ->method('sleepDelay')
-                       ->with('1.2.3.4');
-
-               $this->throttler->expects($this->never())
-                       ->method('registerAttempt');
-
                $this->userManager->method('checkPassword')
                        ->with(
                                $this->equalTo('user'),
@@ -138,25 +124,10 @@ class OCSControllerTest extends TestCase {
                                'personid' => 'user'
                        ]
                ]);
-
                $this->assertEquals($expected, $this->controller->personCheck('user', 'pass'));
        }
 
        public function testPersonInvalid() {
-               $this->request->method('getRemoteAddress')
-                       ->willReturn('1.2.3.4');
-
-               $this->throttler->expects($this->once())
-                       ->method('sleepDelay')
-                       ->with('1.2.3.4');
-
-               $this->throttler->expects($this->once())
-                       ->method('registerAttempt')
-                       ->with(
-                               $this->equalTo('login'),
-                               $this->equalTo('1.2.3.4')
-                       );
-
                $this->userManager->method('checkPassword')
                        ->with(
                                $this->equalTo('user'),
@@ -164,20 +135,11 @@ class OCSControllerTest extends TestCase {
                        )->willReturn(false);
 
                $expected = new DataResponse(null, 102);
-
+               $expected->throttle();
                $this->assertEquals($expected, $this->controller->personCheck('user', 'wrongpass'));
        }
 
        public function testPersonNoLogin() {
-               $this->request->method('getRemoteAddress')
-                       ->willReturn('1.2.3.4');
-
-               $this->throttler->expects($this->never())
-                       ->method('sleepDelay');
-
-               $this->throttler->expects($this->never())
-                       ->method('registerAttempt');
-
                $this->userManager->method('checkPassword')
                        ->with(
                                $this->equalTo('user'),
@@ -185,7 +147,6 @@ class OCSControllerTest extends TestCase {
                        )->willReturn(false);
 
                $expected = new DataResponse(null, 101);
-
                $this->assertEquals($expected, $this->controller->personCheck('', ''));
        }