feat(middleware): Migrate BruteForceProtection annotation to PHP Attribute and allow multiple

Signed-off-by: Joas Schilling <coding@schilljs.com>

6 files changed, 266 insertions, 67 deletions

diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php
index 57a828dbefb..0866a979348 100644
--- a/lib/composer/composer/autoload_classmap.php
+++ b/lib/composer/composer/autoload_classmap.php
@@ -35,6 +35,7 @@ return array(
     'OCP\\AppFramework\\Db\\QBMapper' => $baseDir . '/lib/public/AppFramework/Db/QBMapper.php',
     'OCP\\AppFramework\\Db\\TTransactional' => $baseDir . '/lib/public/AppFramework/Db/TTransactional.php',
     'OCP\\AppFramework\\Http' => $baseDir . '/lib/public/AppFramework/Http.php',
+    'OCP\\AppFramework\\Http\\Attribute\\BruteForceProtection' => $baseDir . '/lib/public/AppFramework/Http/Attribute/BruteForceProtection.php',
     'OCP\\AppFramework\\Http\\Attribute\\UseSession' => $baseDir . '/lib/public/AppFramework/Http/Attribute/UseSession.php',
     'OCP\\AppFramework\\Http\\ContentSecurityPolicy' => $baseDir . '/lib/public/AppFramework/Http/ContentSecurityPolicy.php',
     'OCP\\AppFramework\\Http\\DataDisplayResponse' => $baseDir . '/lib/public/AppFramework/Http/DataDisplayResponse.php',
diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php
index e9d1ba50024..0b8eda8d08d 100644
--- a/lib/composer/composer/autoload_static.php
+++ b/lib/composer/composer/autoload_static.php
@@ -68,6 +68,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
         'OCP\\AppFramework\\Db\\QBMapper' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Db/QBMapper.php',
         'OCP\\AppFramework\\Db\\TTransactional' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Db/TTransactional.php',
         'OCP\\AppFramework\\Http' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http.php',
+        'OCP\\AppFramework\\Http\\Attribute\\BruteForceProtection' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/BruteForceProtection.php',
         'OCP\\AppFramework\\Http\\Attribute\\UseSession' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/UseSession.php',
         'OCP\\AppFramework\\Http\\ContentSecurityPolicy' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/ContentSecurityPolicy.php',
         'OCP\\AppFramework\\Http\\DataDisplayResponse' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/DataDisplayResponse.php',
diff --git a/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php b/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php
index 069d04a9e75..ed43befd121 100644
--- a/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php
+++ b/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php
@@ -3,6 +3,7 @@
 declare(strict_types=1);
 
 /**
+ * @copyright Copyright (c) 2023 Joas Schilling <coding@schilljs.com>
  * @copyright Copyright (c) 2017 Lukas Reschke <lukas@statuscode.ch>
  *
  * @author Christoph Wurst <christoph@winzerhof-wurst.at>
@@ -31,6 +32,7 @@ use OC\AppFramework\Utility\ControllerMethodReflector;
 use OC\Security\Bruteforce\Throttler;
 use OCP\AppFramework\Controller;
 use OCP\AppFramework\Http;
+use OCP\AppFramework\Http\Attribute\BruteForceProtection;
 use OCP\AppFramework\Http\Response;
 use OCP\AppFramework\Http\TooManyRequestsResponse;
 use OCP\AppFramework\Middleware;
@@ -38,6 +40,7 @@ use OCP\AppFramework\OCS\OCSException;
 use OCP\AppFramework\OCSController;
 use OCP\IRequest;
 use OCP\Security\Bruteforce\MaxDelayReached;
+use ReflectionMethod;
 
 /**
  * Class BruteForceMiddleware performs the bruteforce protection for controllers
@@ -68,6 +71,20 @@ class BruteForceMiddleware extends Middleware {
 		if ($this->reflector->hasAnnotation('BruteForceProtection')) {
 			$action = $this->reflector->getAnnotationParameter('BruteForceProtection', 'action');
 			$this->throttler->sleepDelayOrThrowOnMax($this->request->getRemoteAddress(), $action);
+		} else {
+			$reflectionMethod = new ReflectionMethod($controller, $methodName);
+			$attributes = $reflectionMethod->getAttributes(BruteForceProtection::class);
+
+			if (!empty($attributes)) {
+				$remoteAddress = $this->request->getRemoteAddress();
+
+				foreach ($attributes as $attribute) {
+					/** @var BruteForceProtection $protection */
+					$protection = $attribute->newInstance();
+					$action = $protection->getAction();
+					$this->throttler->sleepDelayOrThrowOnMax($remoteAddress, $action);
+				}
+			}
 		}
 	}
 
@@ -75,11 +92,32 @@ class BruteForceMiddleware extends Middleware {
 	 * {@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, $response->getThrottleMetadata());
+		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)) {
+					$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);
+						}
+					}
+				}
+			}
 		}
 
 		return parent::afterController($controller, $methodName, $response);
diff --git a/lib/public/AppFramework/Http/Attribute/BruteForceProtection.php b/lib/public/AppFramework/Http/Attribute/BruteForceProtection.php
new file mode 100644
index 00000000000..386889769cb
--- /dev/null
+++ b/lib/public/AppFramework/Http/Attribute/BruteForceProtection.php
@@ -0,0 +1,52 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * @copyright Copyright (c) 2023 Joas Schilling <coding@schilljs.com>
+ *
+ * @author Joas Schilling <coding@schilljs.com>
+ *
+ * @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 OCP\AppFramework\Http\Attribute;
+
+use Attribute;
+
+/**
+ * Attribute for controller methods that want to protect passwords, keys, tokens
+ * or other data against brute force
+ *
+ * @since 27.0.0
+ */
+#[Attribute(Attribute::TARGET_METHOD | Attribute::IS_REPEATABLE)]
+class BruteForceProtection {
+	/**
+	 * @since 27.0.0
+	 */
+	public function __construct(
+		protected string $action
+	) {
+	}
+
+	/**
+	 * @since 27.0.0
+	 */
+	public function getAction(): string {
+		return $this->action;
+	}
+}
diff --git a/lib/public/AppFramework/Http/Attribute/UseSession.php b/lib/public/AppFramework/Http/Attribute/UseSession.php
index 79185919def..a6bac011d59 100644
--- a/lib/public/AppFramework/Http/Attribute/UseSession.php
+++ b/lib/public/AppFramework/Http/Attribute/UseSession.php
@@ -2,7 +2,7 @@
 
 declare(strict_types=1);
 
-/*
+/**
  * @copyright 2023 Christoph Wurst <christoph@winzerhof-wurst.at>
  *
  * @author 2023 Christoph Wurst <christoph@winzerhof-wurst.at>
diff --git a/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php
index 7dfcfe22261..b3dff10f6bc 100644
--- a/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php
+++ b/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php
@@ -1,5 +1,6 @@
 <?php
 /**
+ * @copyright Copyright (c) 2023 Joas Schilling <coding@schilljs.com>
  * @copyright Copyright (c) 2017 Lukas Reschke <lukas@statuscode.ch>
  *
  * @license GNU AGPL version 3 or any later version
@@ -25,12 +26,33 @@ use OC\AppFramework\Middleware\Security\BruteForceMiddleware;
 use OC\AppFramework\Utility\ControllerMethodReflector;
 use OC\Security\Bruteforce\Throttler;
 use OCP\AppFramework\Controller;
+use OCP\AppFramework\Http\Attribute\BruteForceProtection;
 use OCP\AppFramework\Http\Response;
 use OCP\IRequest;
 use Test\TestCase;
 
+class TestController extends Controller {
+	/**
+	 * @BruteForceProtection(action=login)
+	 */
+	public function testMethodWithAnnotation() {
+	}
+
+	public function testMethodWithoutAnnotation() {
+	}
+
+	#[BruteForceProtection(action: 'single')]
+	public function singleAttribute(): void {
+	}
+
+	#[BruteForceProtection(action: 'first')]
+	#[BruteForceProtection(action: 'second')]
+	public function multipleAttributes(): void {
+	}
+}
+
 class BruteForceMiddlewareTest extends TestCase {
-	/** @var ControllerMethodReflector|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var ControllerMethodReflector */
 	private $reflector;
 	/** @var Throttler|\PHPUnit\Framework\MockObject\MockObject */
 	private $throttler;
@@ -41,7 +63,7 @@ class BruteForceMiddlewareTest extends TestCase {
 	protected function setUp(): void {
 		parent::setUp();
 
-		$this->reflector = $this->createMock(ControllerMethodReflector::class);
+		$this->reflector = new ControllerMethodReflector();
 		$this->throttler = $this->createMock(Throttler::class);
 		$this->request = $this->createMock(IRequest::class);
 
@@ -53,16 +75,6 @@ class BruteForceMiddlewareTest extends TestCase {
 	}
 
 	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')
@@ -72,20 +84,45 @@ class BruteForceMiddlewareTest extends TestCase {
 			->method('sleepDelayOrThrowOnMax')
 			->with('127.0.0.1', 'login');
 
-		/** @var Controller|\PHPUnit\Framework\MockObject\MockObject $controller */
-		$controller = $this->createMock(Controller::class);
-		$this->bruteForceMiddleware->beforeController($controller, 'testMethod');
+		$controller = new TestController('test', $this->request);
+		$this->reflector->reflect($controller, 'testMethodWithAnnotation');
+		$this->bruteForceMiddleware->beforeController($controller, 'testMethodWithAnnotation');
 	}
 
-	public function testBeforeControllerWithoutAnnotation() {
-		$this->reflector
+	public function testBeforeControllerWithSingleAttribute(): void {
+		$this->request
 			->expects($this->once())
-			->method('hasAnnotation')
-			->with('BruteForceProtection')
-			->willReturn(false);
-		$this->reflector
-			->expects($this->never())
-			->method('getAnnotationParameter');
+			->method('getRemoteAddress')
+			->willReturn('::1');
+		$this->throttler
+			->expects($this->once())
+			->method('sleepDelayOrThrowOnMax')
+			->with('::1', 'single');
+
+		$controller = new TestController('test', $this->request);
+		$this->reflector->reflect($controller, 'singleAttribute');
+		$this->bruteForceMiddleware->beforeController($controller, 'singleAttribute');
+	}
+
+	public function testBeforeControllerWithMultipleAttributes(): void {
+		$this->request
+			->expects($this->once())
+			->method('getRemoteAddress')
+			->willReturn('::1');
+		$this->throttler
+			->expects($this->exactly(2))
+			->method('sleepDelayOrThrowOnMax')
+			->withConsecutive(
+				['::1', 'first'],
+				['::1', 'second'],
+			);
+
+		$controller = new TestController('test', $this->request);
+		$this->reflector->reflect($controller, 'multipleAttributes');
+		$this->bruteForceMiddleware->beforeController($controller, 'multipleAttributes');
+	}
+
+	public function testBeforeControllerWithoutAnnotation() {
 		$this->request
 			->expects($this->never())
 			->method('getRemoteAddress');
@@ -93,19 +130,14 @@ class BruteForceMiddlewareTest extends TestCase {
 			->expects($this->never())
 			->method('sleepDelayOrThrowOnMax');
 
-		/** @var Controller|\PHPUnit\Framework\MockObject\MockObject $controller */
-		$controller = $this->createMock(Controller::class);
-		$this->bruteForceMiddleware->beforeController($controller, 'testMethod');
+		$controller = new TestController('test', $this->request);
+		$this->reflector->reflect($controller, 'testMethodWithoutAnnotation');
+		$this->bruteForceMiddleware->beforeController($controller, 'testMethodWithoutAnnotation');
 	}
 
 	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')
@@ -114,11 +146,6 @@ class BruteForceMiddlewareTest extends TestCase {
 			->expects($this->once())
 			->method('getThrottleMetadata')
 			->willReturn([]);
-		$this->reflector
-			->expects($this->once())
-			->method('getAnnotationParameter')
-			->with('BruteForceProtection', 'action')
-			->willReturn('login');
 		$this->request
 			->expects($this->once())
 			->method('getRemoteAddress')
@@ -132,26 +159,18 @@ class BruteForceMiddlewareTest extends TestCase {
 			->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);
+		$controller = new TestController('test', $this->request);
+		$this->reflector->reflect($controller, 'testMethodWithAnnotation');
+		$this->bruteForceMiddleware->afterController($controller, 'testMethodWithAnnotation', $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');
@@ -162,20 +181,108 @@ class BruteForceMiddlewareTest extends TestCase {
 			->expects($this->never())
 			->method('registerAttempt');
 
-		/** @var Controller|\PHPUnit\Framework\MockObject\MockObject $controller */
-		$controller = $this->createMock(Controller::class);
-		$this->bruteForceMiddleware->afterController($controller, 'testMethod', $response);
+		$controller = new TestController('test', $this->request);
+		$this->reflector->reflect($controller, 'testMethodWithAnnotation');
+		$this->bruteForceMiddleware->afterController($controller, 'testMethodWithAnnotation', $response);
 	}
 
-	public function testAfterControllerWithoutAnnotation() {
-		$this->reflector
+	public function testAfterControllerWithSingleAttribute(): void {
+		/** @var Response|\PHPUnit\Framework\MockObject\MockObject $response */
+		$response = $this->createMock(Response::class);
+		$response
 			->expects($this->once())
-			->method('hasAnnotation')
-			->with('BruteForceProtection')
-			->willReturn(false);
-		$this->reflector
-			->expects($this->never())
-			->method('getAnnotationParameter');
+			->method('isThrottled')
+			->willReturn(true);
+		$response
+			->expects($this->once())
+			->method('getThrottleMetadata')
+			->willReturn([]);
+
+		$this->request
+			->expects($this->once())
+			->method('getRemoteAddress')
+			->willReturn('::1');
+		$this->throttler
+			->expects($this->once())
+			->method('sleepDelay')
+			->with('::1', 'single');
+		$this->throttler
+			->expects($this->once())
+			->method('registerAttempt')
+			->with('single', '::1');
+
+		$controller = new TestController('test', $this->request);
+		$this->reflector->reflect($controller, 'singleAttribute');
+		$this->bruteForceMiddleware->afterController($controller, 'singleAttribute', $response);
+	}
+
+	public function testAfterControllerWithMultipleAttributesGeneralMatch(): void {
+		/** @var Response|\PHPUnit\Framework\MockObject\MockObject $response */
+		$response = $this->createMock(Response::class);
+		$response
+			->expects($this->once())
+			->method('isThrottled')
+			->willReturn(true);
+		$response
+			->expects($this->once())
+			->method('getThrottleMetadata')
+			->willReturn([]);
+
+		$this->request
+			->expects($this->once())
+			->method('getRemoteAddress')
+			->willReturn('::1');
+		$this->throttler
+			->expects($this->exactly(2))
+			->method('sleepDelay')
+			->withConsecutive(
+				['::1', 'first'],
+				['::1', 'second'],
+			);
+		$this->throttler
+			->expects($this->exactly(2))
+			->method('registerAttempt')
+			->withConsecutive(
+				['first', '::1'],
+				['second', '::1'],
+			);
+
+		$controller = new TestController('test', $this->request);
+		$this->reflector->reflect($controller, 'multipleAttributes');
+		$this->bruteForceMiddleware->afterController($controller, 'multipleAttributes', $response);
+	}
+
+	public function testAfterControllerWithMultipleAttributesSpecificMatch(): void {
+		/** @var Response|\PHPUnit\Framework\MockObject\MockObject $response */
+		$response = $this->createMock(Response::class);
+		$response
+			->expects($this->once())
+			->method('isThrottled')
+			->willReturn(true);
+		$response
+			->expects($this->once())
+			->method('getThrottleMetadata')
+			->willReturn(['action' => 'second']);
+
+		$this->request
+			->expects($this->once())
+			->method('getRemoteAddress')
+			->willReturn('::1');
+		$this->throttler
+			->expects($this->once())
+			->method('sleepDelay')
+			->with('::1', 'second');
+		$this->throttler
+			->expects($this->once())
+			->method('registerAttempt')
+			->with('second', '::1');
+
+		$controller = new TestController('test', $this->request);
+		$this->reflector->reflect($controller, 'multipleAttributes');
+		$this->bruteForceMiddleware->afterController($controller, 'multipleAttributes', $response);
+	}
+
+	public function testAfterControllerWithoutAnnotation() {
 		$this->request
 			->expects($this->never())
 			->method('getRemoteAddress');
@@ -183,10 +290,10 @@ class BruteForceMiddlewareTest extends TestCase {
 			->expects($this->never())
 			->method('sleepDelay');
 
-		/** @var Controller|\PHPUnit\Framework\MockObject\MockObject $controller */
-		$controller = $this->createMock(Controller::class);
+		$controller = new TestController('test', $this->request);
+		$this->reflector->reflect($controller, 'testMethodWithoutAnnotation');
 		/** @var Response|\PHPUnit\Framework\MockObject\MockObject $response */
 		$response = $this->createMock(Response::class);
-		$this->bruteForceMiddleware->afterController($controller, 'testMethod', $response);
+		$this->bruteForceMiddleware->afterController($controller, 'testMethodWithoutAnnotation', $response);
 	}
 }