Merge pull request #38274 from nextcloud/bugfix/noid/reach-max-delay-in-afterController

fix(middleware): Also abort the request when reaching max delay in af…

2 files changed, 37 insertions, 29 deletions

diff --git a/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php b/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php
index ba8c7f45b49..2ecd26a68e1 100644
--- a/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php
+++ b/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php
@@ -90,32 +90,40 @@ class BruteForceMiddleware extends Middleware {
 	 */
 	public function afterController($controller, $methodName, Response $response) {
 		if ($response->isThrottled()) {
-			if ($this->reflector->hasAnnotation('BruteForceProtection')) {
-				$action = $this->reflector->getAnnotationParameter('BruteForceProtection', 'action');
-				$ip = $this->request->getRemoteAddress();
-				$this->throttler->sleepDelay($ip, $action);
-				$this->throttler->registerAttempt($action, $ip, $response->getThrottleMetadata());
-			} else {
-				$reflectionMethod = new ReflectionMethod($controller, $methodName);
-				$attributes = $reflectionMethod->getAttributes(BruteForceProtection::class);
-
-				if (!empty($attributes)) {
+			try {
+				if ($this->reflector->hasAnnotation('BruteForceProtection')) {
+					$action = $this->reflector->getAnnotationParameter('BruteForceProtection', 'action');
 					$ip = $this->request->getRemoteAddress();
-					$metaData = $response->getThrottleMetadata();
-
-					foreach ($attributes as $attribute) {
-						/** @var BruteForceProtection $protection */
-						$protection = $attribute->newInstance();
-						$action = $protection->getAction();
-
-						if (!isset($metaData['action']) || $metaData['action'] === $action) {
-							$this->throttler->sleepDelay($ip, $action);
-							$this->throttler->registerAttempt($action, $ip, $metaData);
+					$this->throttler->registerAttempt($action, $ip, $response->getThrottleMetadata());
+					$this->throttler->sleepDelayOrThrowOnMax($ip, $action);
+				} else {
+					$reflectionMethod = new ReflectionMethod($controller, $methodName);
+					$attributes = $reflectionMethod->getAttributes(BruteForceProtection::class);
+
+					if (!empty($attributes)) {
+						$ip = $this->request->getRemoteAddress();
+						$metaData = $response->getThrottleMetadata();
+
+						foreach ($attributes as $attribute) {
+							/** @var BruteForceProtection $protection */
+							$protection = $attribute->newInstance();
+							$action = $protection->getAction();
+
+							if (!isset($metaData['action']) || $metaData['action'] === $action) {
+								$this->throttler->registerAttempt($action, $ip, $metaData);
+								$this->throttler->sleepDelayOrThrowOnMax($ip, $action);
+							}
 						}
+					} else {
+						$this->logger->debug('Response for ' . get_class($controller) . '::' . $methodName . ' got bruteforce throttled but has no annotation nor attribute defined.');
 					}
-				} else {
-					$this->logger->debug('Response for ' . get_class($controller) . '::' . $methodName . ' got bruteforce throttled but has no annotation nor attribute defined.');
 				}
+			} catch (MaxDelayReached $e) {
+				if ($controller instanceof OCSController) {
+					throw new OCSException($e->getMessage(), Http::STATUS_TOO_MANY_REQUESTS);
+				}
+
+				return new TooManyRequestsResponse();
 			}
 		}
 
diff --git a/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php
index 388b2fbf534..faf2d24d172 100644
--- a/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php
+++ b/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php
@@ -157,7 +157,7 @@ class BruteForceMiddlewareTest extends TestCase {
 			->willReturn('127.0.0.1');
 		$this->throttler
 			->expects($this->once())
-			->method('sleepDelay')
+			->method('sleepDelayOrThrowOnMax')
 			->with('127.0.0.1', 'login');
 		$this->throttler
 			->expects($this->once())
@@ -181,7 +181,7 @@ class BruteForceMiddlewareTest extends TestCase {
 			->method('getRemoteAddress');
 		$this->throttler
 			->expects($this->never())
-			->method('sleepDelay');
+			->method('sleepDelayOrThrowOnMax');
 		$this->throttler
 			->expects($this->never())
 			->method('registerAttempt');
@@ -209,7 +209,7 @@ class BruteForceMiddlewareTest extends TestCase {
 			->willReturn('::1');
 		$this->throttler
 			->expects($this->once())
-			->method('sleepDelay')
+			->method('sleepDelayOrThrowOnMax')
 			->with('::1', 'single');
 		$this->throttler
 			->expects($this->once())
@@ -239,7 +239,7 @@ class BruteForceMiddlewareTest extends TestCase {
 			->willReturn('::1');
 		$this->throttler
 			->expects($this->exactly(2))
-			->method('sleepDelay')
+			->method('sleepDelayOrThrowOnMax')
 			->withConsecutive(
 				['::1', 'first'],
 				['::1', 'second'],
@@ -275,7 +275,7 @@ class BruteForceMiddlewareTest extends TestCase {
 			->willReturn('::1');
 		$this->throttler
 			->expects($this->once())
-			->method('sleepDelay')
+			->method('sleepDelayOrThrowOnMax')
 			->with('::1', 'second');
 		$this->throttler
 			->expects($this->once())
@@ -293,7 +293,7 @@ class BruteForceMiddlewareTest extends TestCase {
 			->method('getRemoteAddress');
 		$this->throttler
 			->expects($this->never())
-			->method('sleepDelay');
+			->method('sleepDelayOrThrowOnMax');
 
 		$controller = new TestController('test', $this->request);
 		$this->reflector->reflect($controller, 'testMethodWithoutAnnotation');
@@ -308,7 +308,7 @@ class BruteForceMiddlewareTest extends TestCase {
 			->method('getRemoteAddress');
 		$this->throttler
 			->expects($this->never())
-			->method('sleepDelay');
+			->method('sleepDelayOrThrowOnMax');
 
 		$controller = new TestController('test', $this->request);
 		$this->reflector->reflect($controller, 'testMethodWithoutAnnotation');