From 57050146f686d724a9c7ac2c099a6e8b8d591b76 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Tue, 2 Jan 2018 21:13:32 +0100 Subject: [PATCH] Move passwordconfirmation to its own midleware Add tests Signed-off-by: Roeland Jago Douma --- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + .../DependencyInjection/DIContainer.php | 17 ++- .../PasswordConfirmationMiddleware.php | 81 +++++++++++ .../Security/SecurityMiddleware.php | 29 +--- .../PasswordConfirmationMiddlewareTest.php | 129 ++++++++++++++++++ .../Security/SecurityMiddlewareTest.php | 11 +- 7 files changed, 228 insertions(+), 41 deletions(-) create mode 100644 lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php create mode 100644 tests/lib/AppFramework/Middleware/Security/PasswordConfirmationMiddlewareTest.php diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 738054cd37..974a634386 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -335,6 +335,7 @@ return array( 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\NotLoggedInException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/NotLoggedInException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\SecurityException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/SecurityException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\StrictCookieMissingException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/StrictCookieMissingException.php', + 'OC\\AppFramework\\Middleware\\Security\\PasswordConfirmationMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\RateLimitingMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/RateLimitingMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\SameSiteCookieMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\SecurityMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 7ffbd4c788..fc71d085f0 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -365,6 +365,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\NotLoggedInException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/NotLoggedInException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\SecurityException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/SecurityException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\StrictCookieMissingException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/StrictCookieMissingException.php', + 'OC\\AppFramework\\Middleware\\Security\\PasswordConfirmationMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\RateLimitingMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/RateLimitingMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\SameSiteCookieMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\SecurityMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php', diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index 1d8a54982b..612728d135 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -54,6 +54,7 @@ use OCP\AppFramework\Http\IOutput; use OCP\AppFramework\IApi; use OCP\AppFramework\IAppContainer; use OCP\AppFramework\QueryException; +use OCP\AppFramework\Utility\ITimeFactory; use OCP\Files\Folder; use OCP\Files\IAppData; use OCP\GlobalScale\IConfig; @@ -227,17 +228,26 @@ class DIContainer extends SimpleContainer implements IAppContainer { $server->getNavigationManager(), $server->getURLGenerator(), $server->getLogger(), - $server->getSession(), $c['AppName'], $app->isLoggedIn(), $app->isAdminUser(), $server->getContentSecurityPolicyManager(), $server->getCsrfTokenManager(), $server->getContentSecurityPolicyNonceManager(), - $server->getAppManager(), - $server->getUserSession() + $server->getAppManager() ); + }); + $this->registerService(OC\AppFramework\Middleware\Security\PasswordConfirmationMiddleware::class, function ($c) use ($app) { + /** @var \OC\Server $server */ + $server = $app->getServer(); + + return new OC\AppFramework\Middleware\Security\PasswordConfirmationMiddleware( + $c['ControllerMethodReflector'], + $server->getSession(), + $server->getUserSession(), + $server->query(ITimeFactory::class) + ); }); $this->registerService('BruteForceMiddleware', function($c) use ($app) { @@ -310,6 +320,7 @@ class DIContainer extends SimpleContainer implements IAppContainer { $dispatcher->registerMiddleware($c['CORSMiddleware']); $dispatcher->registerMiddleware($c['OCSMiddleware']); $dispatcher->registerMiddleware($c['SecurityMiddleware']); + $dispatcher->registerMiddleware($c[OC\AppFramework\Middleware\Security\PasswordConfirmationMiddleware::class]); $dispatcher->registerMiddleware($c['TwoFactorMiddleware']); $dispatcher->registerMiddleware($c['BruteForceMiddleware']); $dispatcher->registerMiddleware($c['RateLimitingMiddleware']); diff --git a/lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php b/lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php new file mode 100644 index 0000000000..463e7cd93c --- /dev/null +++ b/lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php @@ -0,0 +1,81 @@ + + * + * @author Roeland Jago Douma + * + * @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\Middleware\Security\Exceptions\NotConfirmedException; +use OC\AppFramework\Utility\ControllerMethodReflector; +use OCP\AppFramework\Controller; +use OCP\AppFramework\Middleware; +use OCP\AppFramework\Utility\ITimeFactory; +use OCP\ISession; +use OCP\IUserSession; + +class PasswordConfirmationMiddleware extends Middleware { + /** @var ControllerMethodReflector */ + private $reflector; + /** @var ISession */ + private $session; + /** @var IUserSession */ + private $userSession; + /** @var ITimeFactory */ + private $timeFactory; + + /** + * PasswordConfirmationMiddleware constructor. + * + * @param ControllerMethodReflector $reflector + * @param ISession $session + * @param IUserSession $userSession + * @param ITimeFactory $timeFactory + */ + public function __construct(ControllerMethodReflector $reflector, + ISession $session, + IUserSession $userSession, + ITimeFactory $timeFactory) { + $this->reflector = $reflector; + $this->session = $session; + $this->userSession = $userSession; + $this->timeFactory = $timeFactory; + } + + /** + * @param Controller $controller + * @param string $methodName + * @throws NotConfirmedException + */ + public function beforeController($controller, $methodName) { + if ($this->reflector->hasAnnotation('PasswordConfirmationRequired')) { + $user = $this->userSession->getUser(); + $backendClassName = ''; + if ($user !== null) { + $backendClassName = $user->getBackendClassName(); + } + + $lastConfirm = (int) $this->session->get('last-password-confirm'); + // we can't check the password against a SAML backend, so skip password confirmation in this case + if ($backendClassName !== 'user_saml' && $lastConfirm < ($this->timeFactory->getTime() - (30 * 60 + 15))) { // allow 15 seconds delay + throw new NotConfirmedException(); + } + } + } +} diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index 0fa76a45d2..c147b5b247 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -33,7 +33,6 @@ namespace OC\AppFramework\Middleware\Security; use OC\AppFramework\Middleware\Security\Exceptions\AppNotEnabledException; use OC\AppFramework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException; use OC\AppFramework\Middleware\Security\Exceptions\NotAdminException; -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; @@ -50,12 +49,10 @@ use OCP\AppFramework\Http\Response; use OCP\AppFramework\Http\JSONResponse; use OCP\AppFramework\OCSController; use OCP\INavigationManager; -use OCP\ISession; use OCP\IURLGenerator; use OCP\IRequest; use OCP\ILogger; use OCP\AppFramework\Controller; -use OCP\IUserSession; use OCP\Util; use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; @@ -78,8 +75,6 @@ class SecurityMiddleware extends Middleware { private $urlGenerator; /** @var ILogger */ private $logger; - /** @var ISession */ - private $session; /** @var bool */ private $isLoggedIn; /** @var bool */ @@ -92,8 +87,6 @@ class SecurityMiddleware extends Middleware { private $cspNonceManager; /** @var IAppManager */ private $appManager; - /** @var IUserSession */ - private $userSession; /** * @param IRequest $request @@ -101,7 +94,6 @@ class SecurityMiddleware extends Middleware { * @param INavigationManager $navigationManager * @param IURLGenerator $urlGenerator * @param ILogger $logger - * @param ISession $session * @param string $appName * @param bool $isLoggedIn * @param bool $isAdminUser @@ -109,22 +101,19 @@ class SecurityMiddleware extends Middleware { * @param CSRFTokenManager $csrfTokenManager * @param ContentSecurityPolicyNonceManager $cspNonceManager * @param IAppManager $appManager - * @param IUserSession $userSession */ public function __construct(IRequest $request, ControllerMethodReflector $reflector, INavigationManager $navigationManager, IURLGenerator $urlGenerator, ILogger $logger, - ISession $session, $appName, $isLoggedIn, $isAdminUser, ContentSecurityPolicyManager $contentSecurityPolicyManager, CsrfTokenManager $csrfTokenManager, ContentSecurityPolicyNonceManager $cspNonceManager, - IAppManager $appManager, - IUserSession $userSession + IAppManager $appManager ) { $this->navigationManager = $navigationManager; $this->request = $request; @@ -132,14 +121,12 @@ class SecurityMiddleware extends Middleware { $this->appName = $appName; $this->urlGenerator = $urlGenerator; $this->logger = $logger; - $this->session = $session; $this->isLoggedIn = $isLoggedIn; $this->isAdminUser = $isAdminUser; $this->contentSecurityPolicyManager = $contentSecurityPolicyManager; $this->csrfTokenManager = $csrfTokenManager; $this->cspNonceManager = $cspNonceManager; $this->appManager = $appManager; - $this->userSession = $userSession; } /** @@ -170,20 +157,6 @@ class SecurityMiddleware extends Middleware { } } - if ($this->reflector->hasAnnotation('PasswordConfirmationRequired')) { - $user = $this->userSession->getUser(); - $backendClassName = ''; - if ($user !== null) { - $backendClassName = $user->getBackendClassName(); - } - - $lastConfirm = (int) $this->session->get('last-password-confirm'); - // we can't check the password against a SAML backend, so skip password confirmation in this case - if ($backendClassName !== 'user_saml' && $lastConfirm < (time() - (30 * 60 + 15))) { // allow 15 seconds delay - throw new NotConfirmedException(); - } - } - // Check for strict cookie requirement if($this->reflector->hasAnnotation('StrictCookieRequired') || !$this->reflector->hasAnnotation('NoCSRFRequired')) { if(!$this->request->passesStrictCookieCheck()) { diff --git a/tests/lib/AppFramework/Middleware/Security/PasswordConfirmationMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/PasswordConfirmationMiddlewareTest.php new file mode 100644 index 0000000000..2c610736f4 --- /dev/null +++ b/tests/lib/AppFramework/Middleware/Security/PasswordConfirmationMiddlewareTest.php @@ -0,0 +1,129 @@ + + * + * @author Roeland Jago Douma + * + * @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\Exceptions\NotConfirmedException; +use OC\AppFramework\Middleware\Security\PasswordConfirmationMiddleware; +use OC\AppFramework\Utility\ControllerMethodReflector; +use OCP\AppFramework\Controller; +use OCP\AppFramework\Utility\ITimeFactory; +use OCP\ISession; +use OCP\IUser; +use OCP\IUserSession; +use Test\TestCase; + +class PasswordConfirmationMiddlewareTest extends TestCase { + /** @var ControllerMethodReflector */ + private $reflector; + /** @var ISession|\PHPUnit_Framework_MockObject_MockObject */ + private $session; + /** @var IUserSession|\PHPUnit_Framework_MockObject_MockObject */ + private $userSession; + /** @var IUser|\PHPUnit_Framework_MockObject_MockObject */ + private $user; + /** @var PasswordConfirmationMiddleware */ + private $middleware; + /** @var Controller */ + private $contoller; + /** @var ITimeFactory|\PHPUnit_Framework_MockObject_MockObject */ + private $timeFactory; + + protected function setUp() { + $this->reflector = new ControllerMethodReflector(); + $this->session = $this->createMock(ISession::class); + $this->userSession = $this->createMock(IUserSession::class); + $this->user = $this->createMock(IUser::class); + $this->contoller = $this->createMock(Controller::class); + $this->timeFactory = $this->createMock(ITimeFactory::class); + + $this->middleware = new PasswordConfirmationMiddleware( + $this->reflector, + $this->session, + $this->userSession, + $this->timeFactory + ); + } + + public function testNoAnnotation() { + $this->reflector->reflect(__CLASS__, __FUNCTION__); + $this->session->expects($this->never()) + ->method($this->anything()); + $this->userSession->expects($this->never()) + ->method($this->anything()); + + $this->middleware->beforeController($this->contoller, __FUNCTION__); + } + + /** + * @TestAnnotation + */ + public function testDifferentAnnotation() { + $this->reflector->reflect(__CLASS__, __FUNCTION__); + $this->session->expects($this->never()) + ->method($this->anything()); + $this->userSession->expects($this->never()) + ->method($this->anything()); + + $this->middleware->beforeController($this->contoller, __FUNCTION__); + } + + /** + * @PasswordConfirmationRequired + * @dataProvider testProvider + */ + public function testAnnotation($backend, $lastConfirm, $currentTime, $exception) { + $this->reflector->reflect(__CLASS__, __FUNCTION__); + + $this->user->method('getBackendClassName') + ->willReturn($backend); + $this->userSession->method('getUser') + ->willReturn($this->user); + + $this->session->method('get') + ->with('last-password-confirm') + ->willReturn($lastConfirm); + + $this->timeFactory->method('getTime') + ->willReturn($currentTime); + + $thrown = false; + try { + $this->middleware->beforeController($this->contoller, __FUNCTION__); + } catch (NotConfirmedException $e) { + $thrown = true; + } + + $this->assertSame($exception, $thrown); + } + + public function testProvider() { + return [ + ['foo', 2000, 4000, true], + ['foo', 2000, 3000, false], + ['user_saml', 2000, 4000, false], + ['user_saml', 2000, 3000, false], + ['foo', 2000, 3815, false], + ['foo', 2000, 3816, true], + ]; + } +} diff --git a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php index 411878fc8c..151d6935e7 100644 --- a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php @@ -64,8 +64,6 @@ class SecurityMiddlewareTest extends \Test\TestCase { private $secException; /** @var SecurityException */ private $secAjaxException; - /** @var ISession|\PHPUnit_Framework_MockObject_MockObject */ - private $session; /** @var IRequest|\PHPUnit_Framework_MockObject_MockObject */ private $request; /** @var ControllerMethodReflector */ @@ -95,7 +93,6 @@ class SecurityMiddlewareTest extends \Test\TestCase { $this->logger = $this->createMock(ILogger::class); $this->navigationManager = $this->createMock(INavigationManager::class); $this->urlGenerator = $this->createMock(IURLGenerator::class); - $this->session = $this->createMock(ISession::class); $this->request = $this->createMock(IRequest::class); $this->contentSecurityPolicyManager = $this->createMock(ContentSecurityPolicyManager::class); $this->csrfTokenManager = $this->createMock(CsrfTokenManager::class); @@ -104,10 +101,6 @@ class SecurityMiddlewareTest extends \Test\TestCase { $this->appManager->expects($this->any()) ->method('isEnabledForUser') ->willReturn(true); - $this->userSession = $this->createMock(IUserSession::class); - $user = $this->createMock(IUser::class); - $user->expects($this->any())->method('getBackendClassName')->willReturn('user_ldap'); - $this->userSession->expects($this->any())->method('getUser')->willReturn($user); $this->middleware = $this->getMiddleware(true, true); $this->secException = new SecurityException('hey', false); $this->secAjaxException = new SecurityException('hey', true); @@ -125,15 +118,13 @@ class SecurityMiddlewareTest extends \Test\TestCase { $this->navigationManager, $this->urlGenerator, $this->logger, - $this->session, 'files', $isLoggedIn, $isAdminUser, $this->contentSecurityPolicyManager, $this->csrfTokenManager, $this->cspNonceManager, - $this->appManager, - $this->userSession + $this->appManager ); }