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>

core/Controller/LoginController.php

@@ -29,7 +29,6 @@
 namespace OC\Core\Controller;
 
 use OC\Authentication\TwoFactorAuth\Manager;
-use OC\Security\Bruteforce\Throttler;
 use OC\User\Session;
 use OC_App;
 use OC_Util;
@@ -64,8 +63,6 @@ class LoginController extends Controller {
 	private $logger;
 	/** @var Manager */
 	private $twoFactorManager;
-	/** @var Throttler */
-	private $throttler;
 
 	/**
 	 * @param string $appName
@@ -77,7 +74,6 @@ class LoginController extends Controller {
 	 * @param IURLGenerator $urlGenerator
 	 * @param ILogger $logger
 	 * @param Manager $twoFactorManager
-	 * @param Throttler $throttler
 	 */
 	public function __construct($appName,
 						 IRequest $request,
@@ -87,8 +83,7 @@ class LoginController extends Controller {
 						 IUserSession $userSession,
 						 IURLGenerator $urlGenerator,
 						 ILogger $logger,
-						 Manager $twoFactorManager,
+						 Manager $twoFactorManager) {
-						 Throttler $throttler) {
 		parent::__construct($appName, $request);
 		$this->userManager = $userManager;
 		$this->config = $config;
@@ -97,7 +92,6 @@ class LoginController extends Controller {
 		$this->urlGenerator = $urlGenerator;
 		$this->logger = $logger;
 		$this->twoFactorManager = $twoFactorManager;
-		$this->throttler = $throttler;
 	}
 
 	/**
@@ -203,6 +197,7 @@ class LoginController extends Controller {
 	 * @PublicPage
 	 * @UseSession
 	 * @NoCSRFRequired
+	 * @BruteForceProtection(action=login)
 	 *
 	 * @param string $user
 	 * @param string $password
@@ -216,8 +211,6 @@ class LoginController extends Controller {
 		if(!is_string($user)) {
 			throw new \InvalidArgumentException('Username must be string');
 		}
-		$currentDelay = $this->throttler->getDelay($this->request->getRemoteAddress(), 'login');
-		$this->throttler->sleepDelay($this->request->getRemoteAddress(), 'login');
 
 		// If the user is already logged in and the CSRF check does not pass then
 		// simply redirect the user to the correct page as required. This is the
@@ -245,16 +238,14 @@ class LoginController extends Controller {
 			}
 		}
 		if ($loginResult === false) {
-			$this->throttler->registerAttempt('login', $this->request->getRemoteAddress(), ['user' => $originalUser]);
+			// Read current user and append if possible - we need to return the unmodified user otherwise we will leak the login name
-			if($currentDelay === 0) {
+			$args = !is_null($user) ? ['user' => $originalUser] : [];
-				$this->throttler->sleepDelay($this->request->getRemoteAddress(), 'login');
+			$response = new RedirectResponse($this->urlGenerator->linkToRoute('core.login.showLoginForm', $args));
-			}
+			$response->throttle();
 			$this->session->set('loginMessages', [
 				['invalidpassword'], []
 			]);
-			// Read current user and append if possible - we need to return the unmodified user otherwise we will leak the login name
+			return $response;
-			$args = !is_null($user) ? ['user' => $originalUser] : [];
-			return new RedirectResponse($this->urlGenerator->linkToRoute('core.login.showLoginForm', $args));
 		}
 		// TODO: remove password checks from above and let the user session handle failures
 		// requires https://github.com/owncloud/core/pull/24616
@@ -305,6 +296,7 @@ class LoginController extends Controller {
 	/**
 	 * @NoAdminRequired
 	 * @UseSession
+	 * @BruteForceProtection(action=sudo)
 	 *
 	 * @license GNU AGPL version 3 or any later version
 	 *
@@ -312,18 +304,12 @@ class LoginController extends Controller {
 	 * @return DataResponse
 	 */
 	public function confirmPassword($password) {
-		$currentDelay = $this->throttler->getDelay($this->request->getRemoteAddress(), 'sudo');
-		$this->throttler->sleepDelay($this->request->getRemoteAddress(), 'sudo');
-
 		$loginName = $this->userSession->getLoginName();
 		$loginResult = $this->userManager->checkPassword($loginName, $password);
 		if ($loginResult === false) {
-			$this->throttler->registerAttempt('sudo', $this->request->getRemoteAddress(), ['user' => $loginName]);
+			$response = new DataResponse([], Http::STATUS_FORBIDDEN);
-			if ($currentDelay === 0) {
+			$response->throttle();
-				$this->throttler->sleepDelay($this->request->getRemoteAddress(), 'sudo');
+			return $response;
-			}
-
-			return new DataResponse([], Http::STATUS_FORBIDDEN);
 		}
 
 		$confirmTimestamp = time();

lib/composer/composer/autoload_classmap.php

@@ -290,6 +290,7 @@ return array(
     'OC\\AppFramework\\Http\\Request' => $baseDir . '/lib/private/AppFramework/Http/Request.php',
     'OC\\AppFramework\\Middleware\\MiddlewareDispatcher' => $baseDir . '/lib/private/AppFramework/Middleware/MiddlewareDispatcher.php',
     'OC\\AppFramework\\Middleware\\OCSMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/OCSMiddleware.php',
+    'OC\\AppFramework\\Middleware\\Security\\BruteForceMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php',
     'OC\\AppFramework\\Middleware\\Security\\CORSMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php',
     'OC\\AppFramework\\Middleware\\Security\\Exceptions\\AppNotEnabledException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/AppNotEnabledException.php',
     'OC\\AppFramework\\Middleware\\Security\\Exceptions\\CrossSiteRequestForgeryException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/CrossSiteRequestForgeryException.php',

lib/composer/composer/autoload_static.php

@@ -320,6 +320,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c
         'OC\\AppFramework\\Http\\Request' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Http/Request.php',
         'OC\\AppFramework\\Middleware\\MiddlewareDispatcher' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/MiddlewareDispatcher.php',
         'OC\\AppFramework\\Middleware\\OCSMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/OCSMiddleware.php',
+        'OC\\AppFramework\\Middleware\\Security\\BruteForceMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php',
         'OC\\AppFramework\\Middleware\\Security\\CORSMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php',
         'OC\\AppFramework\\Middleware\\Security\\Exceptions\\AppNotEnabledException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/AppNotEnabledException.php',
         'OC\\AppFramework\\Middleware\\Security\\Exceptions\\CrossSiteRequestForgeryException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/CrossSiteRequestForgeryException.php',

lib/private/AppFramework/DependencyInjection/DIContainer.php

@@ -224,12 +224,22 @@ class DIContainer extends SimpleContainer implements IAppContainer {
 				$app->isAdminUser(),
 				$server->getContentSecurityPolicyManager(),
 				$server->getCsrfTokenManager(),
-				$server->getContentSecurityPolicyNonceManager(),
+				$server->getContentSecurityPolicyNonceManager()
-				$server->getBruteForceThrottler()
 			);
 
 		});
 
+		$this->registerService('BruteForceMiddleware', function($c) use ($app) {
+			/** @var \OC\Server $server */
+			$server = $app->getServer();
+
+			return new OC\AppFramework\Middleware\Security\BruteForceMiddleware(
+				$c['ControllerMethodReflector'],
+				$server->getBruteForceThrottler(),
+				$server->getRequest()
+			);
+		});
+
 		$this->registerService('RateLimitingMiddleware', function($c) use ($app) {
 			/** @var \OC\Server $server */
 			$server = $app->getServer();
@@ -281,7 +291,8 @@ class DIContainer extends SimpleContainer implements IAppContainer {
 			$dispatcher->registerMiddleware($c['CORSMiddleware']);
 			$dispatcher->registerMiddleware($c['OCSMiddleware']);
 			$dispatcher->registerMiddleware($c['SecurityMiddleware']);
-			$dispatcher->registerMiddleWare($c['TwoFactorMiddleware']);
+			$dispatcher->registerMiddleware($c['TwoFactorMiddleware']);
+			$dispatcher->registerMiddleware($c['BruteForceMiddleware']);
 			$dispatcher->registerMiddleware($c['RateLimitingMiddleware']);
 
 			foreach($middleWares as $middleWare) {

lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php

@@ -0,0 +1,83 @@
+<?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 OC\AppFramework\Middleware\Security;
+
+use OC\AppFramework\Utility\ControllerMethodReflector;
+use OC\Security\Bruteforce\Throttler;
+use OCP\AppFramework\Http\Response;
+use OCP\AppFramework\Middleware;
+use OCP\IRequest;
+
+/**
+ * Class BruteForceMiddleware performs the bruteforce protection for controllers
+ * that are annotated with @BruteForceProtection(action=$action) whereas $action
+ * is the action that should be logged within the database.
+ *
+ * @package OC\AppFramework\Middleware\Security
+ */
+class BruteForceMiddleware extends Middleware {
+	/** @var ControllerMethodReflector */
+	private $reflector;
+	/** @var Throttler */
+	private $throttler;
+	/** @var IRequest */
+	private $request;
+
+	/**
+	 * @param ControllerMethodReflector $controllerMethodReflector
+	 * @param Throttler $throttler
+	 * @param IRequest $request
+	 */
+	public function __construct(ControllerMethodReflector $controllerMethodReflector,
+								Throttler $throttler,
+								IRequest $request) {
+		$this->reflector = $controllerMethodReflector;
+		$this->throttler = $throttler;
+		$this->request = $request;
+	}
+
+	/**
+	 * {@inheritDoc}
+	 */
+	public function beforeController($controller, $methodName) {
+		parent::beforeController($controller, $methodName);
+
+		if($this->reflector->hasAnnotation('BruteForceProtection')) {
+			$action = $this->reflector->getAnnotationParameter('BruteForceProtection', 'action');
+			$this->throttler->sleepDelay($this->request->getRemoteAddress(), $action);
+		}
+	}
+
+	/**
+	 * {@inheritDoc}
+	 */
+	public function afterController($controller, $methodName, Response $response) {
+		if($this->reflector->hasAnnotation('BruteForceProtection') && $response->isThrottled()) {
+			$action = $this->reflector->getAnnotationParameter('BruteForceProtection', 'action');
+			$ip = $this->request->getRemoteAddress();
+			$this->throttler->sleepDelay($ip, $action);
+			$this->throttler->registerAttempt($action, $ip);
+		}
+
+		return parent::afterController($controller, $methodName, $response);
+	}
+}

lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php

@@ -36,7 +36,6 @@ use OC\AppFramework\Middleware\Security\Exceptions\NotConfirmedException;
 use OC\AppFramework\Middleware\Security\Exceptions\NotLoggedInException;
 use OC\AppFramework\Middleware\Security\Exceptions\StrictCookieMissingException;
 use OC\AppFramework\Utility\ControllerMethodReflector;
-use OC\Security\Bruteforce\Throttler;
 use OC\Security\CSP\ContentSecurityPolicyManager;
 use OC\Security\CSP\ContentSecurityPolicyNonceManager;
 use OC\Security\CSRF\CsrfTokenManager;
@@ -88,8 +87,6 @@ class SecurityMiddleware extends Middleware {
 	private $csrfTokenManager;
 	/** @var ContentSecurityPolicyNonceManager */
 	private $cspNonceManager;
-	/** @var Throttler */
-	private $throttler;
 
 	/**
 	 * @param IRequest $request
@@ -104,7 +101,6 @@ class SecurityMiddleware extends Middleware {
 	 * @param ContentSecurityPolicyManager $contentSecurityPolicyManager
 	 * @param CSRFTokenManager $csrfTokenManager
 	 * @param ContentSecurityPolicyNonceManager $cspNonceManager
-	 * @param Throttler $throttler
 	 */
 	public function __construct(IRequest $request,
 								ControllerMethodReflector $reflector,
@@ -117,8 +113,7 @@ class SecurityMiddleware extends Middleware {
 								$isAdminUser,
 								ContentSecurityPolicyManager $contentSecurityPolicyManager,
 								CsrfTokenManager $csrfTokenManager,
-								ContentSecurityPolicyNonceManager $cspNonceManager,
+								ContentSecurityPolicyNonceManager $cspNonceManager) {
-								Throttler $throttler) {
 		$this->navigationManager = $navigationManager;
 		$this->request = $request;
 		$this->reflector = $reflector;
@@ -131,10 +126,8 @@ class SecurityMiddleware extends Middleware {
 		$this->contentSecurityPolicyManager = $contentSecurityPolicyManager;
 		$this->csrfTokenManager = $csrfTokenManager;
 		$this->cspNonceManager = $cspNonceManager;
-		$this->throttler = $throttler;
 	}
 
-
 	/**
 	 * This runs all the security checks before a method call. The
 	 * security checks are determined by inspecting the controller method
@@ -191,12 +184,6 @@ class SecurityMiddleware extends Middleware {
 			}
 		}
 
-		if($this->reflector->hasAnnotation('BruteForceProtection')) {
-			$action = $this->reflector->getAnnotationParameter('BruteForceProtection', 'action');
-			$this->throttler->sleepDelay($this->request->getRemoteAddress(), $action);
-			$this->throttler->registerAttempt($action, $this->request->getRemoteAddress());
-		}
-
 		/**
 		 * FIXME: Use DI once available
 		 * Checks if app is enabled (also includes a check whether user is allowed to access the resource)

lib/public/AppFramework/Http/Response.php

@@ -81,6 +81,8 @@ class Response {
 	/** @var ContentSecurityPolicy|null Used Content-Security-Policy */
 	private $contentSecurityPolicy = null;
 
+	/** @var bool */
+	private $throttled = false;
 
 	/**
 	 * Caches the response
@@ -322,5 +324,22 @@ class Response {
 		return $this;
 	}
 
+	/**
+	 * Marks the response as to throttle. Will be throttled when the
+	 * @BruteForceProtection annotation is added.
+	 *
+	 * @since 12.0.0
+	 */
+	public function throttle() {
+		$this->throttled = true;
+	}
 
+	/**
+	 * Whether the current response is throttled.
+	 *
+	 * @since 12.0.0
+	 */
+	public function isThrottled() {
+		return $this->throttled;
+	}
 }

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->twoFactorManager
-			$this->throttler
 		);
 	}
 
@@ -287,27 +283,10 @@ class LoginControllerTest extends TestCase {
 		$password = 'secret';
 		$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, ''));
 	}
 
@@ -340,23 +320,10 @@ class LoginControllerTest extends TestCase {
 		$password = 'secret';
 		$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));
@@ -400,23 +367,10 @@ class LoginControllerTest extends TestCase {
 		$password = 'secret';
 		$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));
@@ -450,23 +404,10 @@ class LoginControllerTest extends TestCase {
 		$password = 'secret';
 		$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()
@@ -490,23 +431,10 @@ class LoginControllerTest extends TestCase {
 		$originalUrl = 'another%20url';
 		$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()
@@ -534,23 +462,10 @@ class LoginControllerTest extends TestCase {
 		$originalUrl = 'another%20url';
 		$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)
@@ -584,23 +499,10 @@ class LoginControllerTest extends TestCase {
 		$challengeUrl = 'challenge/url';
 		$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));
@@ -651,23 +553,10 @@ class LoginControllerTest extends TestCase {
 		$provider1 = $this->getMockBuilder('\OCP\Authentication\TwoFactorAuth\IProvider')->getMock();
 		$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));
@@ -731,33 +620,17 @@ class LoginControllerTest extends TestCase {
 			->method('linkToRoute')
 			->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));
 	}
 }

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());
+	}
 }

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);
+	}
+}

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->cspNonceManager
-			$this->bruteForceThrottler
 		);
 	}
 
@@ -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]
-		];
-	}
 }