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>

2 files changed, 191 insertions, 75 deletions

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]
-		];
-	}
 }