From 2cf068463fb2da915fc576bfed0134e051885b39 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 25 Oct 2019 14:42:00 +0200 Subject: [PATCH] Harden middleware check These annotations will allow for extra checks. And thus make it harder to break things. Signed-off-by: Roeland Jago Douma --- .../TwoFactorChallengeController.php | 3 + core/Middleware/TwoFactorMiddleware.php | 10 +++ .../Middleware/TwoFactorMiddlewareTest.php | 89 ++++++++++++++++++- 3 files changed, 98 insertions(+), 4 deletions(-) diff --git a/core/Controller/TwoFactorChallengeController.php b/core/Controller/TwoFactorChallengeController.php index e2a0b5423ab..07e77352bac 100644 --- a/core/Controller/TwoFactorChallengeController.php +++ b/core/Controller/TwoFactorChallengeController.php @@ -99,6 +99,7 @@ class TwoFactorChallengeController extends Controller { /** * @NoAdminRequired * @NoCSRFRequired + * @TwoFactorSetUpDoneRequired * * @param string $redirect_url * @return StandaloneTemplateResponse @@ -125,6 +126,7 @@ class TwoFactorChallengeController extends Controller { * @NoAdminRequired * @NoCSRFRequired * @UseSession + * @TwoFactorSetUpDoneRequired * * @param string $challengeProviderId * @param string $redirect_url @@ -175,6 +177,7 @@ class TwoFactorChallengeController extends Controller { * @NoAdminRequired * @NoCSRFRequired * @UseSession + * @TwoFactorSetUpDoneRequired * * @UserRateThrottle(limit=5, period=100) * diff --git a/core/Middleware/TwoFactorMiddleware.php b/core/Middleware/TwoFactorMiddleware.php index 7b32c0dd895..b8ca7d9da9e 100644 --- a/core/Middleware/TwoFactorMiddleware.php +++ b/core/Middleware/TwoFactorMiddleware.php @@ -88,6 +88,16 @@ class TwoFactorMiddleware extends Middleware { return; } + if ($controller instanceof TwoFactorChallengeController + && $this->userSession->getUser() !== null + && !$this->reflector->hasAnnotation('TwoFactorSetUpDoneRequired')) { + $providers = $this->twoFactorManager->getProviderSet($this->userSession->getUser()); + + if (!($providers->getProviders() === [] && !$providers->isProviderMissing())) { + throw new TwoFactorAuthRequiredException(); + } + } + if ($controller instanceof ALoginSetupController && $this->userSession->getUser() !== null && $this->twoFactorManager->needsSecondFactor($this->userSession->getUser())) { diff --git a/tests/Core/Middleware/TwoFactorMiddlewareTest.php b/tests/Core/Middleware/TwoFactorMiddlewareTest.php index 70566760184..06aea147a23 100644 --- a/tests/Core/Middleware/TwoFactorMiddlewareTest.php +++ b/tests/Core/Middleware/TwoFactorMiddlewareTest.php @@ -22,13 +22,18 @@ namespace Test\Core\Middleware; +use OC\Authentication\Exceptions\TwoFactorAuthRequiredException; +use OC\Authentication\Exceptions\UserAlreadyLoggedInException; use OC\Authentication\TwoFactorAuth\Manager; +use OC\Authentication\TwoFactorAuth\ProviderSet; +use OC\Core\Controller\TwoFactorChallengeController; use OC\Core\Middleware\TwoFactorMiddleware; use OC\AppFramework\Http\Request; use OC\User\Session; use OCP\AppFramework\Controller; use OCP\AppFramework\Utility\IControllerMethodReflector; use OCP\Authentication\TwoFactorAuth\ALoginSetupController; +use OCP\Authentication\TwoFactorAuth\IProvider; use OCP\IConfig; use OCP\IRequest; use OCP\ISession; @@ -191,14 +196,13 @@ class TwoFactorMiddlewareTest extends TestCase { public function testBeforeControllerUserAlreadyLoggedIn() { $user = $this->createMock(IUser::class); - $this->reflector->expects($this->once()) + $this->reflector ->method('hasAnnotation') - ->with('PublicPage') - ->will($this->returnValue(false)); + ->willReturn(false); $this->userSession->expects($this->once()) ->method('isLoggedIn') ->will($this->returnValue(true)); - $this->userSession->expects($this->once()) + $this->userSession ->method('getUser') ->will($this->returnValue($user)); $this->twoFactorManager->expects($this->once()) @@ -240,4 +244,81 @@ class TwoFactorMiddlewareTest extends TestCase { $this->assertEquals($expected, $this->middleware->afterException($this->controller, 'index', $ex)); } + public function testRequires2FASetupDoneAnnotated() { + $user = $this->createMock(IUser::class); + + $this->reflector + ->method('hasAnnotation') + ->will($this->returnCallback(function (string $annotation) { + return $annotation === 'TwoFactorSetUpDoneRequired'; + })); + $this->userSession->expects($this->once()) + ->method('isLoggedIn') + ->willReturn(true); + $this->userSession + ->method('getUser') + ->willReturn($user); + $this->twoFactorManager->expects($this->once()) + ->method('isTwoFactorAuthenticated') + ->with($user) + ->willReturn(true); + $this->twoFactorManager->expects($this->once()) + ->method('needsSecondFactor') + ->with($user) + ->willReturn(false); + + $this->expectException(UserAlreadyLoggedInException::class); + + $twoFactorChallengeController = $this->getMockBuilder(TwoFactorChallengeController::class) + ->disableOriginalConstructor() + ->getMock(); + $this->middleware->beforeController($twoFactorChallengeController, 'index'); + } + + public function dataRequires2FASetupDone() { + $provider = $this->createMock(IProvider::class); + $provider->method('getId') + ->willReturn('2FAftw'); + + return [ + [[], false, false], + [[], true, true], + [[$provider], false, true], + [[$provider], true, true], + ]; + } + + /** + * @dataProvider dataRequires2FASetupDone + */ + public function testRequires2FASetupDone(array $providers, bool $missingProviders, bool $expectEception) { + $user = $this->createMock(IUser::class); + + $this->reflector + ->method('hasAnnotation') + ->willReturn(false); + $this->userSession + ->method('getUser') + ->willReturn($user); + $providerSet = new ProviderSet($providers, $missingProviders); + $this->twoFactorManager->method('getProviderSet') + ->with($user) + ->willReturn($providerSet); + $this->userSession + ->method('isLoggedIn') + ->willReturn(false); + + if ($expectEception) { + $this->expectException(TwoFactorAuthRequiredException::class); + } else { + // hack to make phpunit shut up. Since we don't expect an exception here... + $this->assertTrue(true); + } + + $twoFactorChallengeController = $this->getMockBuilder(TwoFactorChallengeController::class) + ->disableOriginalConstructor() + ->getMock(); + $this->middleware->beforeController($twoFactorChallengeController, 'index'); + } + } -- 2.39.5