Merge pull request #36928 from nextcloud/techdebt/noid/bruteforce-protection-attribute

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

1 files changed, 198 insertions, 64 deletions

diff --git a/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php
index 7dfcfe22261..388b2fbf534 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,44 +26,60 @@ 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 Psr\Log\LoggerInterface;
 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;
 	/** @var IRequest|\PHPUnit\Framework\MockObject\MockObject */
 	private $request;
+	/** @var LoggerInterface|\PHPUnit\Framework\MockObject\MockObject */
+	private $logger;
 	private BruteForceMiddleware $bruteForceMiddleware;
 
 	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);
+		$this->logger = $this->createMock(LoggerInterface::class);
 
 		$this->bruteForceMiddleware = new BruteForceMiddleware(
 			$this->reflector,
 			$this->throttler,
-			$this->request
+			$this->request,
+			$this->logger,
 		);
 	}
 
-	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');
+	public function testBeforeControllerWithAnnotation(): void {
 		$this->request
 			->expects($this->once())
 			->method('getRemoteAddress')
@@ -72,20 +89,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(): void {
 		$this->request
 			->expects($this->never())
 			->method('getRemoteAddress');
@@ -93,19 +135,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() {
+	public function testAfterControllerWithAnnotationAndThrottledRequest(): void {
 		/** @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 +151,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 +164,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() {
+	public function testAfterControllerWithAnnotationAndNotThrottledRequest(): void {
 		/** @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 +186,123 @@ 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
+			->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(): void {
+		$this->request
 			->expects($this->never())
-			->method('getAnnotationParameter');
+			->method('getRemoteAddress');
+		$this->throttler
+			->expects($this->never())
+			->method('sleepDelay');
+
+		$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, 'testMethodWithoutAnnotation', $response);
+	}
+
+	public function testAfterControllerWithThrottledResponseButUnhandled(): void {
 		$this->request
 			->expects($this->never())
 			->method('getRemoteAddress');
@@ -183,10 +310,17 @@ 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);
+		$response->method('isThrottled')
+			->willReturn(true);
+
+		$this->logger->expects($this->once())
+			->method('debug')
+			->with('Response for Test\AppFramework\Middleware\Security\TestController::testMethodWithoutAnnotation got bruteforce throttled but has no annotation nor attribute defined.');
+
+		$this->bruteForceMiddleware->afterController($controller, 'testMethodWithoutAnnotation', $response);
 	}
 }