From e839eb9b5c425a5ffd661798a72164204fe8e87d Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 28 Feb 2023 22:26:22 +0100 Subject: feat(middleware): Migrate BruteForceProtection annotation to PHP Attribute and allow multiple Signed-off-by: Joas Schilling --- .../Security/BruteForceMiddlewareTest.php | 229 +++++++++++++++------ 1 file changed, 168 insertions(+), 61 deletions(-) (limited to 'tests') 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 @@ * @copyright Copyright (c) 2017 Lukas Reschke * * @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); } } -- cgit v1.2.3 From 2b4986167975355238d982ba8579e0cccf6bf5aa Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 8 Mar 2023 12:08:53 +0100 Subject: Add a debug message when throttling without defining Signed-off-by: Joas Schilling --- .../DependencyInjection/DIContainer.php | 3 +- .../Middleware/Security/BruteForceMiddleware.php | 19 +++++------ .../Security/BruteForceMiddlewareTest.php | 39 ++++++++++++++++++---- 3 files changed, 44 insertions(+), 17 deletions(-) (limited to 'tests') diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index 9b202f07fbf..9a9740b7bcc 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -292,7 +292,8 @@ class DIContainer extends SimpleContainer implements IAppContainer { new OC\AppFramework\Middleware\Security\BruteForceMiddleware( $c->get(IControllerMethodReflector::class), $c->get(OC\Security\Bruteforce\Throttler::class), - $c->get(IRequest::class) + $c->get(IRequest::class), + $c->get(LoggerInterface::class) ) ); $dispatcher->registerMiddleware( diff --git a/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php b/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php index ed43befd121..ba8c7f45b49 100644 --- a/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php @@ -40,6 +40,7 @@ use OCP\AppFramework\OCS\OCSException; use OCP\AppFramework\OCSController; use OCP\IRequest; use OCP\Security\Bruteforce\MaxDelayReached; +use Psr\Log\LoggerInterface; use ReflectionMethod; /** @@ -50,16 +51,12 @@ use ReflectionMethod; * @package OC\AppFramework\Middleware\Security */ class BruteForceMiddleware extends Middleware { - private ControllerMethodReflector $reflector; - private Throttler $throttler; - private IRequest $request; - - public function __construct(ControllerMethodReflector $controllerMethodReflector, - Throttler $throttler, - IRequest $request) { - $this->reflector = $controllerMethodReflector; - $this->throttler = $throttler; - $this->request = $request; + public function __construct( + protected ControllerMethodReflector $reflector, + protected Throttler $throttler, + protected IRequest $request, + protected LoggerInterface $logger, + ) { } /** @@ -116,6 +113,8 @@ class BruteForceMiddleware extends Middleware { $this->throttler->registerAttempt($action, $ip, $metaData); } } + } else { + $this->logger->debug('Response for ' . get_class($controller) . '::' . $methodName . ' got bruteforce throttled but has no annotation nor attribute defined.'); } } } diff --git a/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php index b3dff10f6bc..388b2fbf534 100644 --- a/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php @@ -29,6 +29,7 @@ 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 { @@ -58,6 +59,8 @@ class BruteForceMiddlewareTest extends TestCase { 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 { @@ -66,15 +69,17 @@ class BruteForceMiddlewareTest extends TestCase { $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() { + public function testBeforeControllerWithAnnotation(): void { $this->request ->expects($this->once()) ->method('getRemoteAddress') @@ -122,7 +127,7 @@ class BruteForceMiddlewareTest extends TestCase { $this->bruteForceMiddleware->beforeController($controller, 'multipleAttributes'); } - public function testBeforeControllerWithoutAnnotation() { + public function testBeforeControllerWithoutAnnotation(): void { $this->request ->expects($this->never()) ->method('getRemoteAddress'); @@ -135,7 +140,7 @@ class BruteForceMiddlewareTest extends TestCase { $this->bruteForceMiddleware->beforeController($controller, 'testMethodWithoutAnnotation'); } - public function testAfterControllerWithAnnotationAndThrottledRequest() { + public function testAfterControllerWithAnnotationAndThrottledRequest(): void { /** @var Response|\PHPUnit\Framework\MockObject\MockObject $response */ $response = $this->createMock(Response::class); $response @@ -164,7 +169,7 @@ class BruteForceMiddlewareTest extends TestCase { $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); $response @@ -282,7 +287,7 @@ class BruteForceMiddlewareTest extends TestCase { $this->bruteForceMiddleware->afterController($controller, 'multipleAttributes', $response); } - public function testAfterControllerWithoutAnnotation() { + public function testAfterControllerWithoutAnnotation(): void { $this->request ->expects($this->never()) ->method('getRemoteAddress'); @@ -296,4 +301,26 @@ class BruteForceMiddlewareTest extends TestCase { $response = $this->createMock(Response::class); $this->bruteForceMiddleware->afterController($controller, 'testMethodWithoutAnnotation', $response); } + + public function testAfterControllerWithThrottledResponseButUnhandled(): void { + $this->request + ->expects($this->never()) + ->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); + $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); + } } -- cgit v1.2.3