Harden middleware check

These annotations will allow for extra checks. And thus make it harder
to break things.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

diff --git a/core/Controller/TwoFactorChallengeController.php b/core/Controller/TwoFactorChallengeController.php
index e2a0b5423ab6321c159c7d9d4f453b311e3e10ff..07e77352bacb99c5e0eae2748f7e70c614904e20 100644 (file)
--- 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 7b32c0dd8959f855cdc5e8aedaaab52ad32bca46..b8ca7d9da9e6f8cea5cdcb22d19e0f98adfad20a 100644 (file)
--- 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 70566760184d1d51916828b28f0723c443247681..06aea147a2348608c42e955d7d9a10af088c3545 100644 (file)
--- 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');
+       }
+
 }