summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorRoeland Jago Douma <roeland@famdouma.nl>2019-10-25 14:42:00 +0200
committerRoeland Jago Douma <roeland@famdouma.nl>2019-10-25 15:44:37 +0200
commit2cf068463fb2da915fc576bfed0134e051885b39 (patch)
treeace41cc391e8124c293aadab8df6e28a8934b7cf
parenta1cc2b21cc4e8abc0aa04938429e73b7b1f66fef (diff)
downloadnextcloud-server-2cf068463fb2da915fc576bfed0134e051885b39.tar.gz
nextcloud-server-2cf068463fb2da915fc576bfed0134e051885b39.zip
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>
-rw-r--r--core/Controller/TwoFactorChallengeController.php3
-rw-r--r--core/Middleware/TwoFactorMiddleware.php10
-rw-r--r--tests/Core/Middleware/TwoFactorMiddlewareTest.php89
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');
+ }
+
}