diff options
author | Joas Schilling <213943+nickvergessen@users.noreply.github.com> | 2023-03-16 19:33:13 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2023-03-16 19:33:13 +0100 |
commit | bfc37afed3000ea42bfe594afb1d763499b43a34 (patch) | |
tree | 05f311540aa5e47ca239885f728e86b413599dd0 | |
parent | 4d4a223b0527b60ab9dbcd102117efade0ecabef (diff) | |
parent | 2b4986167975355238d982ba8579e0cccf6bf5aa (diff) | |
download | nextcloud-server-bfc37afed3000ea42bfe594afb1d763499b43a34.tar.gz nextcloud-server-bfc37afed3000ea42bfe594afb1d763499b43a34.zip |
Merge pull request #36928 from nextcloud/techdebt/noid/bruteforce-protection-attribute
feat(middleware): Migrate BruteForceProtection annotation to PHP Attribute and allow multiple
7 files changed, 307 insertions, 81 deletions
diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index d21eb18cc5a..40d0c4f0cd1 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 b4f25f90d7c..65021f54adb 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/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 069d04a9e75..ba8c7f45b49 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,8 @@ use OCP\AppFramework\OCS\OCSException; use OCP\AppFramework\OCSController; use OCP\IRequest; use OCP\Security\Bruteforce\MaxDelayReached; +use Psr\Log\LoggerInterface; +use ReflectionMethod; /** * Class BruteForceMiddleware performs the bruteforce protection for controllers @@ -47,16 +51,12 @@ use OCP\Security\Bruteforce\MaxDelayReached; * @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, + ) { } /** @@ -68,6 +68,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 +89,34 @@ 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); + } + } + } else { + $this->logger->debug('Response for ' . get_class($controller) . '::' . $methodName . ' got bruteforce throttled but has no annotation nor attribute defined.'); + } + } } 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..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); } } |