Make BruteForceProtection annotation more clever

This makes the new `@BruteForceProtection` annotation more clever and moves the relevant code into it's own middleware.

Basically you can now set `@BruteForceProtection(action=$key)` as annotation and that will make the controller bruteforce protected. However, the difference to before is that you need to call `$responmse->throttle()` to increase the counter. Before the counter was increased every time which leads to all kind of unexpected problems.

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

4 files changed, 199 insertions, 205 deletions

diff --git a/tests/Core/Controller/LoginControllerTest.php b/tests/Core/Controller/LoginControllerTest.php
index 9387e69c405..c9ab8e7476d 100644
--- a/tests/Core/Controller/LoginControllerTest.php
+++ b/tests/Core/Controller/LoginControllerTest.php
@@ -55,8 +55,6 @@ class LoginControllerTest extends TestCase {
 	private $logger;
 	/** @var Manager|\PHPUnit_Framework_MockObject_MockObject */
 	private $twoFactorManager;
-	/** @var Throttler|\PHPUnit_Framework_MockObject_MockObject */
-	private $throttler;
 
 	public function setUp() {
 		parent::setUp();
@@ -68,7 +66,6 @@ class LoginControllerTest extends TestCase {
 		$this->urlGenerator = $this->createMock(IURLGenerator::class);
 		$this->logger = $this->createMock(ILogger::class);
 		$this->twoFactorManager = $this->createMock(Manager::class);
-		$this->throttler = $this->createMock(Throttler::class);
 
 		$this->loginController = new LoginController(
 			'core',
@@ -79,8 +76,7 @@ class LoginControllerTest extends TestCase {
 			$this->userSession,
 			$this->urlGenerator,
 			$this->logger,
-			$this->twoFactorManager,
-			$this->throttler
+			$this->twoFactorManager
 		);
 	}
 
@@ -288,26 +284,9 @@ class LoginControllerTest extends TestCase {
 		$loginPageUrl = 'some url';
 
 		$this->request
-			->expects($this->exactly(5))
-			->method('getRemoteAddress')
-			->willReturn('192.168.0.1');
-		$this->request
 			->expects($this->once())
 			->method('passesCSRFCheck')
 			->willReturn(true);
-		$this->throttler
-			->expects($this->exactly(2))
-			->method('sleepDelay')
-			->with('192.168.0.1');
-		$this->throttler
-			->expects($this->once())
-			->method('getDelay')
-			->with('192.168.0.1')
-			->willReturn(0);
-		$this->throttler
-			->expects($this->once())
-			->method('registerAttempt')
-			->with('login', '192.168.0.1', ['user' => 'MyUserName']);
 		$this->userManager->expects($this->once())
 			->method('checkPasswordNoLogging')
 			->will($this->returnValue(false));
@@ -324,6 +303,7 @@ class LoginControllerTest extends TestCase {
 			->method('deleteUserValue');
 
 		$expected = new \OCP\AppFramework\Http\RedirectResponse($loginPageUrl);
+		$expected->throttle();
 		$this->assertEquals($expected, $this->loginController->tryLogin($user, $password, ''));
 	}
 
@@ -341,22 +321,9 @@ class LoginControllerTest extends TestCase {
 		$indexPageUrl = \OC_Util::getDefaultPageUrl();
 
 		$this->request
-			->expects($this->exactly(2))
-			->method('getRemoteAddress')
-			->willReturn('192.168.0.1');
-		$this->request
 			->expects($this->once())
 			->method('passesCSRFCheck')
 			->willReturn(true);
-		$this->throttler
-			->expects($this->once())
-			->method('sleepDelay')
-			->with('192.168.0.1');
-		$this->throttler
-			->expects($this->once())
-			->method('getDelay')
-			->with('192.168.0.1')
-			->willReturn(200);
 		$this->userManager->expects($this->once())
 			->method('checkPasswordNoLogging')
 			->will($this->returnValue($user));
@@ -401,22 +368,9 @@ class LoginControllerTest extends TestCase {
 		$indexPageUrl = \OC_Util::getDefaultPageUrl();
 
 		$this->request
-			->expects($this->exactly(2))
-			->method('getRemoteAddress')
-			->willReturn('192.168.0.1');
-		$this->request
 			->expects($this->once())
 			->method('passesCSRFCheck')
 			->willReturn(true);
-		$this->throttler
-			->expects($this->once())
-			->method('sleepDelay')
-			->with('192.168.0.1');
-		$this->throttler
-			->expects($this->once())
-			->method('getDelay')
-			->with('192.168.0.1')
-			->willReturn(200);
 		$this->userManager->expects($this->once())
 			->method('checkPasswordNoLogging')
 			->will($this->returnValue($user));
@@ -451,22 +405,9 @@ class LoginControllerTest extends TestCase {
 		$originalUrl = 'another%20url';
 
 		$this->request
-			->expects($this->exactly(2))
-			->method('getRemoteAddress')
-			->willReturn('192.168.0.1');
-		$this->request
 			->expects($this->once())
 			->method('passesCSRFCheck')
 			->willReturn(false);
-		$this->throttler
-			->expects($this->once())
-			->method('sleepDelay')
-			->with('192.168.0.1');
-		$this->throttler
-			->expects($this->once())
-			->method('getDelay')
-			->with('192.168.0.1')
-			->willReturn(200);
 		$this->userSession->expects($this->once())
 			->method('isLoggedIn')
 			->with()
@@ -491,22 +432,9 @@ class LoginControllerTest extends TestCase {
 		$redirectUrl = 'http://localhost/another url';
 
 		$this->request
-			->expects($this->exactly(2))
-			->method('getRemoteAddress')
-			->willReturn('192.168.0.1');
-		$this->request
 			->expects($this->once())
 			->method('passesCSRFCheck')
 			->willReturn(false);
-		$this->throttler
-			->expects($this->once())
-			->method('sleepDelay')
-			->with('192.168.0.1');
-		$this->throttler
-			->expects($this->once())
-			->method('getDelay')
-			->with('192.168.0.1')
-			->willReturn(200);
 		$this->userSession->expects($this->once())
 			->method('isLoggedIn')
 			->with()
@@ -535,22 +463,9 @@ class LoginControllerTest extends TestCase {
 		$redirectUrl = 'http://localhost/another url';
 
 		$this->request
-			->expects($this->exactly(2))
-			->method('getRemoteAddress')
-			->willReturn('192.168.0.1');
-		$this->request
 			->expects($this->once())
 			->method('passesCSRFCheck')
 			->willReturn(true);
-		$this->throttler
-			->expects($this->once())
-			->method('sleepDelay')
-			->with('192.168.0.1');
-		$this->throttler
-			->expects($this->once())
-			->method('getDelay')
-			->with('192.168.0.1')
-			->willReturn(200);
 		$this->userManager->expects($this->once())
 			->method('checkPasswordNoLogging')
 			->with('Jane', $password)
@@ -585,22 +500,9 @@ class LoginControllerTest extends TestCase {
 		$provider = $this->getMockBuilder('\OCP\Authentication\TwoFactorAuth\IProvider')->getMock();
 
 		$this->request
-			->expects($this->exactly(2))
-			->method('getRemoteAddress')
-			->willReturn('192.168.0.1');
-		$this->request
 			->expects($this->once())
 			->method('passesCSRFCheck')
 			->willReturn(true);
-		$this->throttler
-			->expects($this->once())
-			->method('sleepDelay')
-			->with('192.168.0.1');
-		$this->throttler
-			->expects($this->once())
-			->method('getDelay')
-			->with('192.168.0.1')
-			->willReturn(200);
 		$this->userManager->expects($this->once())
 			->method('checkPasswordNoLogging')
 			->will($this->returnValue($user));
@@ -652,22 +554,9 @@ class LoginControllerTest extends TestCase {
 		$provider2 = $this->getMockBuilder('\OCP\Authentication\TwoFactorAuth\IProvider')->getMock();
 
 		$this->request
-			->expects($this->exactly(2))
-			->method('getRemoteAddress')
-			->willReturn('192.168.0.1');
-		$this->request
 			->expects($this->once())
 			->method('passesCSRFCheck')
 			->willReturn(true);
-		$this->throttler
-			->expects($this->once())
-			->method('sleepDelay')
-			->with('192.168.0.1');
-		$this->throttler
-			->expects($this->once())
-			->method('getDelay')
-			->with('192.168.0.1')
-			->willReturn(200);
 		$this->userManager->expects($this->once())
 			->method('checkPasswordNoLogging')
 			->will($this->returnValue($user));
@@ -732,32 +621,16 @@ class LoginControllerTest extends TestCase {
 			->with('core.login.showLoginForm', ['user' => 'john@doe.com'])
 			->will($this->returnValue(''));
 		$this->request
-			->expects($this->exactly(3))
-			->method('getRemoteAddress')
-			->willReturn('192.168.0.1');
-		$this->request
 			->expects($this->once())
 			->method('passesCSRFCheck')
 			->willReturn(true);
-		$this->throttler
-			->expects($this->once())
-			->method('getDelay')
-			->with('192.168.0.1')
-			->willReturn(200);
-		$this->throttler
-			->expects($this->once())
-			->method('sleepDelay')
-			->with('192.168.0.1');
-		$this->throttler
-			->expects($this->once())
-			->method('registerAttempt')
-			->with('login', '192.168.0.1', ['user' => 'john@doe.com']);
 		$this->config->expects($this->never())
 			->method('deleteUserValue');
 		$this->userSession->expects($this->never())
 			->method('createRememberMeToken');
 
 		$expected = new RedirectResponse('');
+		$expected->throttle();
 		$this->assertEquals($expected, $this->loginController->tryLogin('john@doe.com', 'just wrong', null));
 	}
 }
diff --git a/tests/lib/AppFramework/Http/ResponseTest.php b/tests/lib/AppFramework/Http/ResponseTest.php
index 4840a5f94c3..d8959face89 100644
--- a/tests/lib/AppFramework/Http/ResponseTest.php
+++ b/tests/lib/AppFramework/Http/ResponseTest.php
@@ -264,4 +264,9 @@ class ResponseTest extends \Test\TestCase {
 
 	}
 
+	public function testThrottle() {
+		$this->assertFalse($this->childResponse->isThrottled());
+		$this->childResponse->throttle();
+		$this->assertTrue($this->childResponse->isThrottled());
+	}
 }
diff --git a/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php
new file mode 100644
index 00000000000..14d3b796846
--- /dev/null
+++ b/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php
@@ -0,0 +1,190 @@
+<?php
+/**
+ * @copyright Copyright (c) 2017 Lukas Reschke <lukas@statuscode.ch>
+ *
+ * @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 <http://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace Test\AppFramework\Middleware\Security;
+
+use OC\AppFramework\Middleware\Security\BruteForceMiddleware;
+use OC\AppFramework\Utility\ControllerMethodReflector;
+use OC\Security\Bruteforce\Throttler;
+use OCP\AppFramework\Controller;
+use OCP\AppFramework\Http\Response;
+use OCP\Http\Client\IResponse;
+use OCP\IRequest;
+use Test\TestCase;
+
+class BruteForceMiddlewareTest extends TestCase {
+	/** @var ControllerMethodReflector|\PHPUnit_Framework_MockObject_MockObject */
+	private $reflector;
+	/** @var Throttler|\PHPUnit_Framework_MockObject_MockObject */
+	private $throttler;
+	/** @var IRequest|\PHPUnit_Framework_MockObject_MockObject */
+	private $request;
+	/** @var BruteForceMiddleware */
+	private $bruteForceMiddleware;
+
+	public function setUp() {
+		parent::setUp();
+
+		$this->reflector = $this->createMock(ControllerMethodReflector::class);
+		$this->throttler = $this->createMock(Throttler::class);
+		$this->request = $this->createMock(IRequest::class);
+
+		$this->bruteForceMiddleware = new BruteForceMiddleware(
+			$this->reflector,
+			$this->throttler,
+			$this->request
+		);
+	}
+
+	public function testBeforeControllerWithAnnotation() {
+		$this->reflector
+			->expects($this->once())
+			->method('hasAnnotation')
+			->with('BruteForceProtection')
+			->willReturn(true);
+		$this->reflector
+			->expects($this->once())
+			->method('getAnnotationParameter')
+			->with('BruteForceProtection', 'action')
+			->willReturn('login');
+		$this->request
+			->expects($this->once())
+			->method('getRemoteAddress')
+			->willReturn('127.0.0.1');
+		$this->throttler
+			->expects($this->once())
+			->method('sleepDelay')
+			->with('127.0.0.1', 'login');
+
+		/** @var Controller|\PHPUnit_Framework_MockObject_MockObject $controller */
+		$controller = $this->createMock(Controller::class);
+		$this->bruteForceMiddleware->beforeController($controller, 'testMethod');
+	}
+
+	public function testBeforeControllerWithoutAnnotation() {
+		$this->reflector
+			->expects($this->once())
+			->method('hasAnnotation')
+			->with('BruteForceProtection')
+			->willReturn(false);
+		$this->reflector
+			->expects($this->never())
+			->method('getAnnotationParameter');
+		$this->request
+			->expects($this->never())
+			->method('getRemoteAddress');
+		$this->throttler
+			->expects($this->never())
+			->method('sleepDelay');
+
+		/** @var Controller|\PHPUnit_Framework_MockObject_MockObject $controller */
+		$controller = $this->createMock(Controller::class);
+		$this->bruteForceMiddleware->beforeController($controller, 'testMethod');
+	}
+
+	public function testAfterControllerWithAnnotationAndThrottledRequest() {
+		/** @var Response|\PHPUnit_Framework_MockObject_MockObject $response */
+		$response = $this->createMock(Response::class);
+		$this->reflector
+			->expects($this->once())
+			->method('hasAnnotation')
+			->with('BruteForceProtection')
+			->willReturn(true);
+		$response
+			->expects($this->once())
+			->method('isThrottled')
+			->willReturn(true);
+		$this->reflector
+			->expects($this->once())
+			->method('getAnnotationParameter')
+			->with('BruteForceProtection', 'action')
+			->willReturn('login');
+		$this->request
+			->expects($this->once())
+			->method('getRemoteAddress')
+			->willReturn('127.0.0.1');
+		$this->throttler
+			->expects($this->once())
+			->method('sleepDelay')
+			->with('127.0.0.1', 'login');
+		$this->throttler
+			->expects($this->once())
+			->method('registerAttempt')
+			->with('login', '127.0.0.1');
+
+		/** @var Controller|\PHPUnit_Framework_MockObject_MockObject $controller */
+		$controller = $this->createMock(Controller::class);
+		$this->bruteForceMiddleware->afterController($controller, 'testMethod' ,$response);
+	}
+
+	public function testAfterControllerWithAnnotationAndNotThrottledRequest() {
+		/** @var Response|\PHPUnit_Framework_MockObject_MockObject $response */
+		$response = $this->createMock(Response::class);
+		$this->reflector
+			->expects($this->once())
+			->method('hasAnnotation')
+			->with('BruteForceProtection')
+			->willReturn(true);
+		$response
+			->expects($this->once())
+			->method('isThrottled')
+			->willReturn(false);
+		$this->reflector
+			->expects($this->never())
+			->method('getAnnotationParameter');
+		$this->request
+			->expects($this->never())
+			->method('getRemoteAddress');
+		$this->throttler
+			->expects($this->never())
+			->method('sleepDelay');
+		$this->throttler
+			->expects($this->never())
+			->method('registerAttempt');
+
+		/** @var Controller|\PHPUnit_Framework_MockObject_MockObject $controller */
+		$controller = $this->createMock(Controller::class);
+		$this->bruteForceMiddleware->afterController($controller, 'testMethod' ,$response);
+	}
+
+	public function testAfterControllerWithoutAnnotation() {
+		$this->reflector
+			->expects($this->once())
+			->method('hasAnnotation')
+			->with('BruteForceProtection')
+			->willReturn(false);
+		$this->reflector
+			->expects($this->never())
+			->method('getAnnotationParameter');
+		$this->request
+			->expects($this->never())
+			->method('getRemoteAddress');
+		$this->throttler
+			->expects($this->never())
+			->method('sleepDelay');
+
+		/** @var Controller|\PHPUnit_Framework_MockObject_MockObject $controller */
+		$controller = $this->createMock(Controller::class);
+		/** @var Response|\PHPUnit_Framework_MockObject_MockObject $response */
+		$response = $this->createMock(Response::class);
+		$this->bruteForceMiddleware->afterController($controller, 'testMethod' ,$response);
+	}
+}
diff --git a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php
index 164ea48de70..17ac30b8fe4 100644
--- a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php
+++ b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php
@@ -20,8 +20,6 @@
  *
  */
 
-
-
 namespace Test\AppFramework\Middleware\Security;
 
 use OC\AppFramework\Http;
@@ -34,7 +32,6 @@ use OC\AppFramework\Middleware\Security\Exceptions\SecurityException;
 use OC\Appframework\Middleware\Security\Exceptions\StrictCookieMissingException;
 use OC\AppFramework\Middleware\Security\SecurityMiddleware;
 use OC\AppFramework\Utility\ControllerMethodReflector;
-use OC\Security\Bruteforce\Throttler;
 use OC\Security\CSP\ContentSecurityPolicy;
 use OC\Security\CSP\ContentSecurityPolicyManager;
 use OC\Security\CSP\ContentSecurityPolicyNonceManager;
@@ -54,7 +51,6 @@ use OCP\ISession;
 use OCP\IURLGenerator;
 use OCP\Security\ISecureRandom;
 
-
 class SecurityMiddlewareTest extends \Test\TestCase {
 
 	/** @var SecurityMiddleware|\PHPUnit_Framework_MockObject_MockObject */
@@ -83,8 +79,6 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 	private $csrfTokenManager;
 	/** @var ContentSecurityPolicyNonceManager|\PHPUnit_Framework_MockObject_MockObject */
 	private $cspNonceManager;
-	/** @var  Throttler|\PHPUnit_Framework_MockObject_MockObject */
-	private $bruteForceThrottler;
 
 	protected function setUp() {
 		parent::setUp();
@@ -99,7 +93,6 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 		$this->contentSecurityPolicyManager = $this->createMock(ContentSecurityPolicyManager::class);
 		$this->csrfTokenManager = $this->createMock(CsrfTokenManager::class);
 		$this->cspNonceManager = $this->createMock(ContentSecurityPolicyNonceManager::class);
-		$this->bruteForceThrottler = $this->getMockBuilder(Throttler::class)->disableOriginalConstructor()->getMock();
 		$this->middleware = $this->getMiddleware(true, true);
 		$this->secException = new SecurityException('hey', false);
 		$this->secAjaxException = new SecurityException('hey', true);
@@ -123,8 +116,7 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 			$isAdminUser,
 			$this->contentSecurityPolicyManager,
 			$this->csrfTokenManager,
-			$this->cspNonceManager,
-			$this->bruteForceThrottler
+			$this->cspNonceManager
 		);
 	}
 
@@ -657,70 +649,4 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 
 		$this->assertEquals($response, $this->middleware->afterController($this->controller, 'test', $response));
 	}
-
-	/**
-	 * @dataProvider dataTestBeforeControllerBruteForce
-	 */
-	public function testBeforeControllerBruteForce($bruteForceProtectionEnabled) {
-		/** @var ControllerMethodReflector|\PHPUnit_Framework_MockObject_MockObject $reader */
-		$reader = $this->getMockBuilder(ControllerMethodReflector::class)->disableOriginalConstructor()->getMock();
-
-		$middleware = new SecurityMiddleware(
-			$this->request,
-			$reader,
-			$this->navigationManager,
-			$this->urlGenerator,
-			$this->logger,
-			$this->session,
-			'files',
-			false,
-			false,
-			$this->contentSecurityPolicyManager,
-			$this->csrfTokenManager,
-			$this->cspNonceManager,
-			$this->bruteForceThrottler
-		);
-
-		$reader->expects($this->any())->method('hasAnnotation')
-			->willReturnCallback(
-				function($annotation) use ($bruteForceProtectionEnabled) {
-
-					switch ($annotation) {
-						case 'BruteForceProtection':
-							return $bruteForceProtectionEnabled;
-						case 'PasswordConfirmationRequired':
-						case 'StrictCookieRequired':
-							return false;
-						case 'PublicPage':
-						case 'NoCSRFRequired':
-							return true;
-					}
-
-					return true;
-			}
-			);
-
-		$reader->expects($this->any())->method('getAnnotationParameter')->willReturn('action');
-		$this->request->expects($this->any())->method('getRemoteAddress')->willReturn('remoteAddress');
-
-		if ($bruteForceProtectionEnabled) {
-			$this->bruteForceThrottler->expects($this->once())->method('sleepDelay')
-				->with('remoteAddress', 'action');
-			$this->bruteForceThrottler->expects($this->once())->method('registerAttempt')
-				->with('action', 'remoteAddress');
-		} else {
-			$this->bruteForceThrottler->expects($this->never())->method('sleepDelay');
-			$this->bruteForceThrottler->expects($this->never())->method('registerAttempt');
-		}
-
-		$middleware->beforeController($this->controller, 'test');
-
-	}
-
-	public function dataTestBeforeControllerBruteForce() {
-		return [
-			[true],
-			[false]
-		];
-	}
 }