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>

3 files changed, 98 insertions, 17 deletions

diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php
index d279140afbb..04747485c13 100644
--- a/lib/private/AppFramework/DependencyInjection/DIContainer.php
+++ b/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->getBruteForceThrottler()
+				$server->getContentSecurityPolicyNonceManager()
 			);
 
 		});
 
+		$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) {
diff --git a/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php b/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php
new file mode 100644
index 00000000000..b361f453bdb
--- /dev/null
+++ b/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);
+	}
+}
diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php
index d4d0598c94f..e420a9dacc0 100644
--- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php
+++ b/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,
-								Throttler $throttler) {
+								ContentSecurityPolicyNonceManager $cspNonceManager) {
 		$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)