From 8149945a916447b4e7dae8182dbf0c354e7d19e8 Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Thu, 13 Apr 2017 22:50:44 +0200 Subject: [PATCH] Make BruteForceProtection annotation more clever This makes the new `@BruteForceProtection` annotation more clever and moves the relevant code into it's own middleware. Basically you can now set `@BruteForceProtection(action=$key)` as annotation and that will make the controller bruteforce protected. However, the difference to before is that you need to call `$responmse->throttle()` to increase the counter. Before the counter was increased every time which leads to all kind of unexpected problems. Signed-off-by: Lukas Reschke --- core/Controller/LoginController.php | 36 +--- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + .../DependencyInjection/DIContainer.php | 17 +- .../Security/BruteForceMiddleware.php | 83 ++++++++ .../Security/SecurityMiddleware.php | 15 +- lib/public/AppFramework/Http/Response.php | 19 ++ tests/Core/Controller/LoginControllerTest.php | 133 +----------- tests/lib/AppFramework/Http/ResponseTest.php | 5 + .../Security/BruteForceMiddlewareTest.php | 190 ++++++++++++++++++ .../Security/SecurityMiddlewareTest.php | 76 +------ 11 files changed, 329 insertions(+), 247 deletions(-) create mode 100644 lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php create mode 100644 tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php index 59abf47dc8..9f8b2b75fd 100644 --- a/core/Controller/LoginController.php +++ b/core/Controller/LoginController.php @@ -29,7 +29,6 @@ namespace OC\Core\Controller; use OC\Authentication\TwoFactorAuth\Manager; -use OC\Security\Bruteforce\Throttler; use OC\User\Session; use OC_App; use OC_Util; @@ -64,8 +63,6 @@ class LoginController extends Controller { private $logger; /** @var Manager */ private $twoFactorManager; - /** @var Throttler */ - private $throttler; /** * @param string $appName @@ -77,7 +74,6 @@ class LoginController extends Controller { * @param IURLGenerator $urlGenerator * @param ILogger $logger * @param Manager $twoFactorManager - * @param Throttler $throttler */ public function __construct($appName, IRequest $request, @@ -87,8 +83,7 @@ class LoginController extends Controller { IUserSession $userSession, IURLGenerator $urlGenerator, ILogger $logger, - Manager $twoFactorManager, - Throttler $throttler) { + Manager $twoFactorManager) { parent::__construct($appName, $request); $this->userManager = $userManager; $this->config = $config; @@ -97,7 +92,6 @@ class LoginController extends Controller { $this->urlGenerator = $urlGenerator; $this->logger = $logger; $this->twoFactorManager = $twoFactorManager; - $this->throttler = $throttler; } /** @@ -203,6 +197,7 @@ class LoginController extends Controller { * @PublicPage * @UseSession * @NoCSRFRequired + * @BruteForceProtection(action=login) * * @param string $user * @param string $password @@ -216,8 +211,6 @@ class LoginController extends Controller { if(!is_string($user)) { throw new \InvalidArgumentException('Username must be string'); } - $currentDelay = $this->throttler->getDelay($this->request->getRemoteAddress(), 'login'); - $this->throttler->sleepDelay($this->request->getRemoteAddress(), 'login'); // If the user is already logged in and the CSRF check does not pass then // simply redirect the user to the correct page as required. This is the @@ -245,16 +238,14 @@ class LoginController extends Controller { } } if ($loginResult === false) { - $this->throttler->registerAttempt('login', $this->request->getRemoteAddress(), ['user' => $originalUser]); - if($currentDelay === 0) { - $this->throttler->sleepDelay($this->request->getRemoteAddress(), 'login'); - } + // Read current user and append if possible - we need to return the unmodified user otherwise we will leak the login name + $args = !is_null($user) ? ['user' => $originalUser] : []; + $response = new RedirectResponse($this->urlGenerator->linkToRoute('core.login.showLoginForm', $args)); + $response->throttle(); $this->session->set('loginMessages', [ ['invalidpassword'], [] ]); - // Read current user and append if possible - we need to return the unmodified user otherwise we will leak the login name - $args = !is_null($user) ? ['user' => $originalUser] : []; - return new RedirectResponse($this->urlGenerator->linkToRoute('core.login.showLoginForm', $args)); + return $response; } // TODO: remove password checks from above and let the user session handle failures // requires https://github.com/owncloud/core/pull/24616 @@ -305,6 +296,7 @@ class LoginController extends Controller { /** * @NoAdminRequired * @UseSession + * @BruteForceProtection(action=sudo) * * @license GNU AGPL version 3 or any later version * @@ -312,18 +304,12 @@ class LoginController extends Controller { * @return DataResponse */ public function confirmPassword($password) { - $currentDelay = $this->throttler->getDelay($this->request->getRemoteAddress(), 'sudo'); - $this->throttler->sleepDelay($this->request->getRemoteAddress(), 'sudo'); - $loginName = $this->userSession->getLoginName(); $loginResult = $this->userManager->checkPassword($loginName, $password); if ($loginResult === false) { - $this->throttler->registerAttempt('sudo', $this->request->getRemoteAddress(), ['user' => $loginName]); - if ($currentDelay === 0) { - $this->throttler->sleepDelay($this->request->getRemoteAddress(), 'sudo'); - } - - return new DataResponse([], Http::STATUS_FORBIDDEN); + $response = new DataResponse([], Http::STATUS_FORBIDDEN); + $response->throttle(); + return $response; } $confirmTimestamp = time(); diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 51f822a0e7..29eab9c124 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -290,6 +290,7 @@ return array( 'OC\\AppFramework\\Http\\Request' => $baseDir . '/lib/private/AppFramework/Http/Request.php', 'OC\\AppFramework\\Middleware\\MiddlewareDispatcher' => $baseDir . '/lib/private/AppFramework/Middleware/MiddlewareDispatcher.php', 'OC\\AppFramework\\Middleware\\OCSMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/OCSMiddleware.php', + 'OC\\AppFramework\\Middleware\\Security\\BruteForceMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\CORSMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\AppNotEnabledException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/AppNotEnabledException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\CrossSiteRequestForgeryException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/CrossSiteRequestForgeryException.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 29c9079848..61741bf325 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -320,6 +320,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\AppFramework\\Http\\Request' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Http/Request.php', 'OC\\AppFramework\\Middleware\\MiddlewareDispatcher' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/MiddlewareDispatcher.php', 'OC\\AppFramework\\Middleware\\OCSMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/OCSMiddleware.php', + 'OC\\AppFramework\\Middleware\\Security\\BruteForceMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\CORSMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\AppNotEnabledException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/AppNotEnabledException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\CrossSiteRequestForgeryException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/CrossSiteRequestForgeryException.php', diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index d279140afb..04747485c1 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -224,12 +224,22 @@ class DIContainer extends SimpleContainer implements IAppContainer { $app->isAdminUser(), $server->getContentSecurityPolicyManager(), $server->getCsrfTokenManager(), - $server->getContentSecurityPolicyNonceManager(), - $server->getBruteForceThrottler() + $server->getContentSecurityPolicyNonceManager() ); }); + $this->registerService('BruteForceMiddleware', function($c) use ($app) { + /** @var \OC\Server $server */ + $server = $app->getServer(); + + return new OC\AppFramework\Middleware\Security\BruteForceMiddleware( + $c['ControllerMethodReflector'], + $server->getBruteForceThrottler(), + $server->getRequest() + ); + }); + $this->registerService('RateLimitingMiddleware', function($c) use ($app) { /** @var \OC\Server $server */ $server = $app->getServer(); @@ -281,7 +291,8 @@ class DIContainer extends SimpleContainer implements IAppContainer { $dispatcher->registerMiddleware($c['CORSMiddleware']); $dispatcher->registerMiddleware($c['OCSMiddleware']); $dispatcher->registerMiddleware($c['SecurityMiddleware']); - $dispatcher->registerMiddleWare($c['TwoFactorMiddleware']); + $dispatcher->registerMiddleware($c['TwoFactorMiddleware']); + $dispatcher->registerMiddleware($c['BruteForceMiddleware']); $dispatcher->registerMiddleware($c['RateLimitingMiddleware']); foreach($middleWares as $middleWare) { diff --git a/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php b/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php new file mode 100644 index 0000000000..b361f453bd --- /dev/null +++ b/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php @@ -0,0 +1,83 @@ + + * + * @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 . + * + */ + +namespace OC\AppFramework\Middleware\Security; + +use OC\AppFramework\Utility\ControllerMethodReflector; +use OC\Security\Bruteforce\Throttler; +use OCP\AppFramework\Http\Response; +use OCP\AppFramework\Middleware; +use OCP\IRequest; + +/** + * Class BruteForceMiddleware performs the bruteforce protection for controllers + * that are annotated with @BruteForceProtection(action=$action) whereas $action + * is the action that should be logged within the database. + * + * @package OC\AppFramework\Middleware\Security + */ +class BruteForceMiddleware extends Middleware { + /** @var ControllerMethodReflector */ + private $reflector; + /** @var Throttler */ + private $throttler; + /** @var IRequest */ + private $request; + + /** + * @param ControllerMethodReflector $controllerMethodReflector + * @param Throttler $throttler + * @param IRequest $request + */ + public function __construct(ControllerMethodReflector $controllerMethodReflector, + Throttler $throttler, + IRequest $request) { + $this->reflector = $controllerMethodReflector; + $this->throttler = $throttler; + $this->request = $request; + } + + /** + * {@inheritDoc} + */ + public function beforeController($controller, $methodName) { + parent::beforeController($controller, $methodName); + + if($this->reflector->hasAnnotation('BruteForceProtection')) { + $action = $this->reflector->getAnnotationParameter('BruteForceProtection', 'action'); + $this->throttler->sleepDelay($this->request->getRemoteAddress(), $action); + } + } + + /** + * {@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); + } + + return parent::afterController($controller, $methodName, $response); + } +} diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index d4d0598c94..e420a9dacc 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -36,7 +36,6 @@ use OC\AppFramework\Middleware\Security\Exceptions\NotConfirmedException; use OC\AppFramework\Middleware\Security\Exceptions\NotLoggedInException; use OC\AppFramework\Middleware\Security\Exceptions\StrictCookieMissingException; use OC\AppFramework\Utility\ControllerMethodReflector; -use OC\Security\Bruteforce\Throttler; use OC\Security\CSP\ContentSecurityPolicyManager; use OC\Security\CSP\ContentSecurityPolicyNonceManager; use OC\Security\CSRF\CsrfTokenManager; @@ -88,8 +87,6 @@ class SecurityMiddleware extends Middleware { private $csrfTokenManager; /** @var ContentSecurityPolicyNonceManager */ private $cspNonceManager; - /** @var Throttler */ - private $throttler; /** * @param IRequest $request @@ -104,7 +101,6 @@ class SecurityMiddleware extends Middleware { * @param ContentSecurityPolicyManager $contentSecurityPolicyManager * @param CSRFTokenManager $csrfTokenManager * @param ContentSecurityPolicyNonceManager $cspNonceManager - * @param Throttler $throttler */ public function __construct(IRequest $request, ControllerMethodReflector $reflector, @@ -117,8 +113,7 @@ class SecurityMiddleware extends Middleware { $isAdminUser, ContentSecurityPolicyManager $contentSecurityPolicyManager, CsrfTokenManager $csrfTokenManager, - ContentSecurityPolicyNonceManager $cspNonceManager, - Throttler $throttler) { + ContentSecurityPolicyNonceManager $cspNonceManager) { $this->navigationManager = $navigationManager; $this->request = $request; $this->reflector = $reflector; @@ -131,10 +126,8 @@ class SecurityMiddleware extends Middleware { $this->contentSecurityPolicyManager = $contentSecurityPolicyManager; $this->csrfTokenManager = $csrfTokenManager; $this->cspNonceManager = $cspNonceManager; - $this->throttler = $throttler; } - /** * This runs all the security checks before a method call. The * security checks are determined by inspecting the controller method @@ -191,12 +184,6 @@ class SecurityMiddleware extends Middleware { } } - if($this->reflector->hasAnnotation('BruteForceProtection')) { - $action = $this->reflector->getAnnotationParameter('BruteForceProtection', 'action'); - $this->throttler->sleepDelay($this->request->getRemoteAddress(), $action); - $this->throttler->registerAttempt($action, $this->request->getRemoteAddress()); - } - /** * FIXME: Use DI once available * Checks if app is enabled (also includes a check whether user is allowed to access the resource) diff --git a/lib/public/AppFramework/Http/Response.php b/lib/public/AppFramework/Http/Response.php index 051e68f314..087522386b 100644 --- a/lib/public/AppFramework/Http/Response.php +++ b/lib/public/AppFramework/Http/Response.php @@ -81,6 +81,8 @@ class Response { /** @var ContentSecurityPolicy|null Used Content-Security-Policy */ private $contentSecurityPolicy = null; + /** @var bool */ + private $throttled = false; /** * Caches the response @@ -322,5 +324,22 @@ class Response { return $this; } + /** + * Marks the response as to throttle. Will be throttled when the + * @BruteForceProtection annotation is added. + * + * @since 12.0.0 + */ + public function throttle() { + $this->throttled = true; + } + /** + * Whether the current response is throttled. + * + * @since 12.0.0 + */ + public function isThrottled() { + return $this->throttled; + } } diff --git a/tests/Core/Controller/LoginControllerTest.php b/tests/Core/Controller/LoginControllerTest.php index 9387e69c40..c9ab8e7476 100644 --- a/tests/Core/Controller/LoginControllerTest.php +++ b/tests/Core/Controller/LoginControllerTest.php @@ -55,8 +55,6 @@ class LoginControllerTest extends TestCase { private $logger; /** @var Manager|\PHPUnit_Framework_MockObject_MockObject */ private $twoFactorManager; - /** @var Throttler|\PHPUnit_Framework_MockObject_MockObject */ - private $throttler; public function setUp() { parent::setUp(); @@ -68,7 +66,6 @@ class LoginControllerTest extends TestCase { $this->urlGenerator = $this->createMock(IURLGenerator::class); $this->logger = $this->createMock(ILogger::class); $this->twoFactorManager = $this->createMock(Manager::class); - $this->throttler = $this->createMock(Throttler::class); $this->loginController = new LoginController( 'core', @@ -79,8 +76,7 @@ class LoginControllerTest extends TestCase { $this->userSession, $this->urlGenerator, $this->logger, - $this->twoFactorManager, - $this->throttler + $this->twoFactorManager ); } @@ -287,27 +283,10 @@ class LoginControllerTest extends TestCase { $password = 'secret'; $loginPageUrl = 'some url'; - $this->request - ->expects($this->exactly(5)) - ->method('getRemoteAddress') - ->willReturn('192.168.0.1'); $this->request ->expects($this->once()) ->method('passesCSRFCheck') ->willReturn(true); - $this->throttler - ->expects($this->exactly(2)) - ->method('sleepDelay') - ->with('192.168.0.1'); - $this->throttler - ->expects($this->once()) - ->method('getDelay') - ->with('192.168.0.1') - ->willReturn(0); - $this->throttler - ->expects($this->once()) - ->method('registerAttempt') - ->with('login', '192.168.0.1', ['user' => 'MyUserName']); $this->userManager->expects($this->once()) ->method('checkPasswordNoLogging') ->will($this->returnValue(false)); @@ -324,6 +303,7 @@ class LoginControllerTest extends TestCase { ->method('deleteUserValue'); $expected = new \OCP\AppFramework\Http\RedirectResponse($loginPageUrl); + $expected->throttle(); $this->assertEquals($expected, $this->loginController->tryLogin($user, $password, '')); } @@ -340,23 +320,10 @@ class LoginControllerTest extends TestCase { $password = 'secret'; $indexPageUrl = \OC_Util::getDefaultPageUrl(); - $this->request - ->expects($this->exactly(2)) - ->method('getRemoteAddress') - ->willReturn('192.168.0.1'); $this->request ->expects($this->once()) ->method('passesCSRFCheck') ->willReturn(true); - $this->throttler - ->expects($this->once()) - ->method('sleepDelay') - ->with('192.168.0.1'); - $this->throttler - ->expects($this->once()) - ->method('getDelay') - ->with('192.168.0.1') - ->willReturn(200); $this->userManager->expects($this->once()) ->method('checkPasswordNoLogging') ->will($this->returnValue($user)); @@ -400,23 +367,10 @@ class LoginControllerTest extends TestCase { $password = 'secret'; $indexPageUrl = \OC_Util::getDefaultPageUrl(); - $this->request - ->expects($this->exactly(2)) - ->method('getRemoteAddress') - ->willReturn('192.168.0.1'); $this->request ->expects($this->once()) ->method('passesCSRFCheck') ->willReturn(true); - $this->throttler - ->expects($this->once()) - ->method('sleepDelay') - ->with('192.168.0.1'); - $this->throttler - ->expects($this->once()) - ->method('getDelay') - ->with('192.168.0.1') - ->willReturn(200); $this->userManager->expects($this->once()) ->method('checkPasswordNoLogging') ->will($this->returnValue($user)); @@ -450,23 +404,10 @@ class LoginControllerTest extends TestCase { $password = 'secret'; $originalUrl = 'another%20url'; - $this->request - ->expects($this->exactly(2)) - ->method('getRemoteAddress') - ->willReturn('192.168.0.1'); $this->request ->expects($this->once()) ->method('passesCSRFCheck') ->willReturn(false); - $this->throttler - ->expects($this->once()) - ->method('sleepDelay') - ->with('192.168.0.1'); - $this->throttler - ->expects($this->once()) - ->method('getDelay') - ->with('192.168.0.1') - ->willReturn(200); $this->userSession->expects($this->once()) ->method('isLoggedIn') ->with() @@ -490,23 +431,10 @@ class LoginControllerTest extends TestCase { $originalUrl = 'another%20url'; $redirectUrl = 'http://localhost/another url'; - $this->request - ->expects($this->exactly(2)) - ->method('getRemoteAddress') - ->willReturn('192.168.0.1'); $this->request ->expects($this->once()) ->method('passesCSRFCheck') ->willReturn(false); - $this->throttler - ->expects($this->once()) - ->method('sleepDelay') - ->with('192.168.0.1'); - $this->throttler - ->expects($this->once()) - ->method('getDelay') - ->with('192.168.0.1') - ->willReturn(200); $this->userSession->expects($this->once()) ->method('isLoggedIn') ->with() @@ -534,23 +462,10 @@ class LoginControllerTest extends TestCase { $originalUrl = 'another%20url'; $redirectUrl = 'http://localhost/another url'; - $this->request - ->expects($this->exactly(2)) - ->method('getRemoteAddress') - ->willReturn('192.168.0.1'); $this->request ->expects($this->once()) ->method('passesCSRFCheck') ->willReturn(true); - $this->throttler - ->expects($this->once()) - ->method('sleepDelay') - ->with('192.168.0.1'); - $this->throttler - ->expects($this->once()) - ->method('getDelay') - ->with('192.168.0.1') - ->willReturn(200); $this->userManager->expects($this->once()) ->method('checkPasswordNoLogging') ->with('Jane', $password) @@ -584,23 +499,10 @@ class LoginControllerTest extends TestCase { $challengeUrl = 'challenge/url'; $provider = $this->getMockBuilder('\OCP\Authentication\TwoFactorAuth\IProvider')->getMock(); - $this->request - ->expects($this->exactly(2)) - ->method('getRemoteAddress') - ->willReturn('192.168.0.1'); $this->request ->expects($this->once()) ->method('passesCSRFCheck') ->willReturn(true); - $this->throttler - ->expects($this->once()) - ->method('sleepDelay') - ->with('192.168.0.1'); - $this->throttler - ->expects($this->once()) - ->method('getDelay') - ->with('192.168.0.1') - ->willReturn(200); $this->userManager->expects($this->once()) ->method('checkPasswordNoLogging') ->will($this->returnValue($user)); @@ -651,23 +553,10 @@ class LoginControllerTest extends TestCase { $provider1 = $this->getMockBuilder('\OCP\Authentication\TwoFactorAuth\IProvider')->getMock(); $provider2 = $this->getMockBuilder('\OCP\Authentication\TwoFactorAuth\IProvider')->getMock(); - $this->request - ->expects($this->exactly(2)) - ->method('getRemoteAddress') - ->willReturn('192.168.0.1'); $this->request ->expects($this->once()) ->method('passesCSRFCheck') ->willReturn(true); - $this->throttler - ->expects($this->once()) - ->method('sleepDelay') - ->with('192.168.0.1'); - $this->throttler - ->expects($this->once()) - ->method('getDelay') - ->with('192.168.0.1') - ->willReturn(200); $this->userManager->expects($this->once()) ->method('checkPasswordNoLogging') ->will($this->returnValue($user)); @@ -731,33 +620,17 @@ class LoginControllerTest extends TestCase { ->method('linkToRoute') ->with('core.login.showLoginForm', ['user' => 'john@doe.com']) ->will($this->returnValue('')); - $this->request - ->expects($this->exactly(3)) - ->method('getRemoteAddress') - ->willReturn('192.168.0.1'); $this->request ->expects($this->once()) ->method('passesCSRFCheck') ->willReturn(true); - $this->throttler - ->expects($this->once()) - ->method('getDelay') - ->with('192.168.0.1') - ->willReturn(200); - $this->throttler - ->expects($this->once()) - ->method('sleepDelay') - ->with('192.168.0.1'); - $this->throttler - ->expects($this->once()) - ->method('registerAttempt') - ->with('login', '192.168.0.1', ['user' => 'john@doe.com']); $this->config->expects($this->never()) ->method('deleteUserValue'); $this->userSession->expects($this->never()) ->method('createRememberMeToken'); $expected = new RedirectResponse(''); + $expected->throttle(); $this->assertEquals($expected, $this->loginController->tryLogin('john@doe.com', 'just wrong', null)); } } diff --git a/tests/lib/AppFramework/Http/ResponseTest.php b/tests/lib/AppFramework/Http/ResponseTest.php index 4840a5f94c..d8959face8 100644 --- a/tests/lib/AppFramework/Http/ResponseTest.php +++ b/tests/lib/AppFramework/Http/ResponseTest.php @@ -264,4 +264,9 @@ class ResponseTest extends \Test\TestCase { } + public function testThrottle() { + $this->assertFalse($this->childResponse->isThrottled()); + $this->childResponse->throttle(); + $this->assertTrue($this->childResponse->isThrottled()); + } } diff --git a/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php new file mode 100644 index 0000000000..14d3b79684 --- /dev/null +++ b/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php @@ -0,0 +1,190 @@ + + * + * @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 . + * + */ + +namespace Test\AppFramework\Middleware\Security; + +use OC\AppFramework\Middleware\Security\BruteForceMiddleware; +use OC\AppFramework\Utility\ControllerMethodReflector; +use OC\Security\Bruteforce\Throttler; +use OCP\AppFramework\Controller; +use OCP\AppFramework\Http\Response; +use OCP\Http\Client\IResponse; +use OCP\IRequest; +use Test\TestCase; + +class BruteForceMiddlewareTest extends TestCase { + /** @var ControllerMethodReflector|\PHPUnit_Framework_MockObject_MockObject */ + private $reflector; + /** @var Throttler|\PHPUnit_Framework_MockObject_MockObject */ + private $throttler; + /** @var IRequest|\PHPUnit_Framework_MockObject_MockObject */ + private $request; + /** @var BruteForceMiddleware */ + private $bruteForceMiddleware; + + public function setUp() { + parent::setUp(); + + $this->reflector = $this->createMock(ControllerMethodReflector::class); + $this->throttler = $this->createMock(Throttler::class); + $this->request = $this->createMock(IRequest::class); + + $this->bruteForceMiddleware = new BruteForceMiddleware( + $this->reflector, + $this->throttler, + $this->request + ); + } + + 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') + ->willReturn('127.0.0.1'); + $this->throttler + ->expects($this->once()) + ->method('sleepDelay') + ->with('127.0.0.1', 'login'); + + /** @var Controller|\PHPUnit_Framework_MockObject_MockObject $controller */ + $controller = $this->createMock(Controller::class); + $this->bruteForceMiddleware->beforeController($controller, 'testMethod'); + } + + public function testBeforeControllerWithoutAnnotation() { + $this->reflector + ->expects($this->once()) + ->method('hasAnnotation') + ->with('BruteForceProtection') + ->willReturn(false); + $this->reflector + ->expects($this->never()) + ->method('getAnnotationParameter'); + $this->request + ->expects($this->never()) + ->method('getRemoteAddress'); + $this->throttler + ->expects($this->never()) + ->method('sleepDelay'); + + /** @var Controller|\PHPUnit_Framework_MockObject_MockObject $controller */ + $controller = $this->createMock(Controller::class); + $this->bruteForceMiddleware->beforeController($controller, 'testMethod'); + } + + 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') + ->willReturn(true); + $this->reflector + ->expects($this->once()) + ->method('getAnnotationParameter') + ->with('BruteForceProtection', 'action') + ->willReturn('login'); + $this->request + ->expects($this->once()) + ->method('getRemoteAddress') + ->willReturn('127.0.0.1'); + $this->throttler + ->expects($this->once()) + ->method('sleepDelay') + ->with('127.0.0.1', 'login'); + $this->throttler + ->expects($this->once()) + ->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); + } + + 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'); + $this->throttler + ->expects($this->never()) + ->method('sleepDelay'); + $this->throttler + ->expects($this->never()) + ->method('registerAttempt'); + + /** @var Controller|\PHPUnit_Framework_MockObject_MockObject $controller */ + $controller = $this->createMock(Controller::class); + $this->bruteForceMiddleware->afterController($controller, 'testMethod' ,$response); + } + + public function testAfterControllerWithoutAnnotation() { + $this->reflector + ->expects($this->once()) + ->method('hasAnnotation') + ->with('BruteForceProtection') + ->willReturn(false); + $this->reflector + ->expects($this->never()) + ->method('getAnnotationParameter'); + $this->request + ->expects($this->never()) + ->method('getRemoteAddress'); + $this->throttler + ->expects($this->never()) + ->method('sleepDelay'); + + /** @var Controller|\PHPUnit_Framework_MockObject_MockObject $controller */ + $controller = $this->createMock(Controller::class); + /** @var Response|\PHPUnit_Framework_MockObject_MockObject $response */ + $response = $this->createMock(Response::class); + $this->bruteForceMiddleware->afterController($controller, 'testMethod' ,$response); + } +} diff --git a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php index 164ea48de7..17ac30b8fe 100644 --- a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php @@ -20,8 +20,6 @@ * */ - - namespace Test\AppFramework\Middleware\Security; use OC\AppFramework\Http; @@ -34,7 +32,6 @@ use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; use OC\Appframework\Middleware\Security\Exceptions\StrictCookieMissingException; use OC\AppFramework\Middleware\Security\SecurityMiddleware; use OC\AppFramework\Utility\ControllerMethodReflector; -use OC\Security\Bruteforce\Throttler; use OC\Security\CSP\ContentSecurityPolicy; use OC\Security\CSP\ContentSecurityPolicyManager; use OC\Security\CSP\ContentSecurityPolicyNonceManager; @@ -54,7 +51,6 @@ use OCP\ISession; use OCP\IURLGenerator; use OCP\Security\ISecureRandom; - class SecurityMiddlewareTest extends \Test\TestCase { /** @var SecurityMiddleware|\PHPUnit_Framework_MockObject_MockObject */ @@ -83,8 +79,6 @@ class SecurityMiddlewareTest extends \Test\TestCase { private $csrfTokenManager; /** @var ContentSecurityPolicyNonceManager|\PHPUnit_Framework_MockObject_MockObject */ private $cspNonceManager; - /** @var Throttler|\PHPUnit_Framework_MockObject_MockObject */ - private $bruteForceThrottler; protected function setUp() { parent::setUp(); @@ -99,7 +93,6 @@ class SecurityMiddlewareTest extends \Test\TestCase { $this->contentSecurityPolicyManager = $this->createMock(ContentSecurityPolicyManager::class); $this->csrfTokenManager = $this->createMock(CsrfTokenManager::class); $this->cspNonceManager = $this->createMock(ContentSecurityPolicyNonceManager::class); - $this->bruteForceThrottler = $this->getMockBuilder(Throttler::class)->disableOriginalConstructor()->getMock(); $this->middleware = $this->getMiddleware(true, true); $this->secException = new SecurityException('hey', false); $this->secAjaxException = new SecurityException('hey', true); @@ -123,8 +116,7 @@ class SecurityMiddlewareTest extends \Test\TestCase { $isAdminUser, $this->contentSecurityPolicyManager, $this->csrfTokenManager, - $this->cspNonceManager, - $this->bruteForceThrottler + $this->cspNonceManager ); } @@ -657,70 +649,4 @@ class SecurityMiddlewareTest extends \Test\TestCase { $this->assertEquals($response, $this->middleware->afterController($this->controller, 'test', $response)); } - - /** - * @dataProvider dataTestBeforeControllerBruteForce - */ - public function testBeforeControllerBruteForce($bruteForceProtectionEnabled) { - /** @var ControllerMethodReflector|\PHPUnit_Framework_MockObject_MockObject $reader */ - $reader = $this->getMockBuilder(ControllerMethodReflector::class)->disableOriginalConstructor()->getMock(); - - $middleware = new SecurityMiddleware( - $this->request, - $reader, - $this->navigationManager, - $this->urlGenerator, - $this->logger, - $this->session, - 'files', - false, - false, - $this->contentSecurityPolicyManager, - $this->csrfTokenManager, - $this->cspNonceManager, - $this->bruteForceThrottler - ); - - $reader->expects($this->any())->method('hasAnnotation') - ->willReturnCallback( - function($annotation) use ($bruteForceProtectionEnabled) { - - switch ($annotation) { - case 'BruteForceProtection': - return $bruteForceProtectionEnabled; - case 'PasswordConfirmationRequired': - case 'StrictCookieRequired': - return false; - case 'PublicPage': - case 'NoCSRFRequired': - return true; - } - - return true; - } - ); - - $reader->expects($this->any())->method('getAnnotationParameter')->willReturn('action'); - $this->request->expects($this->any())->method('getRemoteAddress')->willReturn('remoteAddress'); - - if ($bruteForceProtectionEnabled) { - $this->bruteForceThrottler->expects($this->once())->method('sleepDelay') - ->with('remoteAddress', 'action'); - $this->bruteForceThrottler->expects($this->once())->method('registerAttempt') - ->with('action', 'remoteAddress'); - } else { - $this->bruteForceThrottler->expects($this->never())->method('sleepDelay'); - $this->bruteForceThrottler->expects($this->never())->method('registerAttempt'); - } - - $middleware->beforeController($this->controller, 'test'); - - } - - public function dataTestBeforeControllerBruteForce() { - return [ - [true], - [false] - ]; - } }