From f0c1de74b9576608bf078aad65ecd2b5b6ab6914 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Thu, 23 Nov 2023 17:38:42 +0100 Subject: [PATCH] fix: 2fa check on controllers which are annotated as `@PublicPage` and also used authenticated --- changelog/unreleased/41123 | 3 + core/Middleware/TwoFactorMiddleware.php | 19 +-- .../Middleware/TwoFactorMiddlewareTest.php | 122 ++++++++++-------- 3 files changed, 84 insertions(+), 60 deletions(-) create mode 100644 changelog/unreleased/41123 diff --git a/changelog/unreleased/41123 b/changelog/unreleased/41123 new file mode 100644 index 000000000000..4f216a14463e --- /dev/null +++ b/changelog/unreleased/41123 @@ -0,0 +1,3 @@ +Bugfix: check 2FA on controllers which are accessible publicly and authenticated + +https://github.com/owncloud/core/pull/41123 diff --git a/core/Middleware/TwoFactorMiddleware.php b/core/Middleware/TwoFactorMiddleware.php index 9c0707a7b9be..de48be00f73d 100644 --- a/core/Middleware/TwoFactorMiddleware.php +++ b/core/Middleware/TwoFactorMiddleware.php @@ -82,11 +82,6 @@ public function __construct( * @throws UserAlreadyLoggedInException */ public function beforeController($controller, $methodName) { - if ($this->reflector->hasAnnotation('PublicPage')) { - // Don't block public pages - return; - } - if ($controller instanceof LoginController && $methodName === 'logout') { // Don't block the logout page, to allow canceling the 2FA return; @@ -98,22 +93,30 @@ public function beforeController($controller, $methodName) { throw new \UnexpectedValueException('User isLoggedIn but session does not contain user'); } if ($this->twoFactorManager->isTwoFactorAuthenticated($user)) { - $this->checkTwoFactor($controller, $methodName); + $this->checkTwoFactor($controller); } elseif ($controller instanceof TwoFactorChallengeController) { // two-factor authentication is in progress. throw new UserAlreadyLoggedInException('Grant access to the two-factor controllers'); } + # user is fine + return; } + if ($this->reflector->hasAnnotation('PublicPage')) { + // Don't block public pages + return; + } + + # no user in session and also not on a public page - no 2fa required + // TODO: dont check/enforce 2FA if a auth token is used } /** * @param $controller - * @param $methodName * @throws TwoFactorAuthRequiredException * @throws UserAlreadyLoggedInException */ - private function checkTwoFactor($controller, $methodName) { + private function checkTwoFactor($controller): void { // If two-factor auth is in progress disallow access to any controllers // defined within "LoginController". $needsSecondFactor = $this->twoFactorManager->needsSecondFactor(); diff --git a/tests/Core/Middleware/TwoFactorMiddlewareTest.php b/tests/Core/Middleware/TwoFactorMiddlewareTest.php index 23ac87b3694f..d24ebd6000eb 100644 --- a/tests/Core/Middleware/TwoFactorMiddlewareTest.php +++ b/tests/Core/Middleware/TwoFactorMiddlewareTest.php @@ -24,33 +24,37 @@ namespace Tests\Core\Middleware; use OC\AppFramework\Http\Request; +use OC\Authentication\Exceptions\TwoFactorAuthRequiredException; +use OC\Authentication\Exceptions\UserAlreadyLoggedInException; use OC\Authentication\TwoFactorAuth\Manager; use OC\Core\Middleware\TwoFactorMiddleware; +use OCP\AppFramework\Http\RedirectResponse; use OCP\AppFramework\Utility\IControllerMethodReflector; use OCP\IConfig; use OCP\IURLGenerator; use OCP\IUserSession; use OCP\Security\ISecureRandom; +use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; +use OCP\IUser; +use OC\Core\Controller\TwoFactorChallengeController; class TwoFactorMiddlewareTest extends TestCase { - /** @var Manager|\PHPUnit\Framework\MockObject\MockObject */ + /** @var Manager|MockObject */ private $twoFactorManager; - /** @var IUserSession|\PHPUnit\Framework\MockObject\MockObject */ + /** @var IUserSession|MockObject */ private $userSession; - /** @var IURLGenerator|\PHPUnit\Framework\MockObject\MockObject */ + /** @var IURLGenerator|MockObject */ private $urlGenerator; - /** @var IControllerMethodReflector|\PHPUnit\Framework\MockObject\MockObject */ + /** @var IControllerMethodReflector|MockObject */ private $reflector; - /** @var Request */ - private $request; + private Request $request; - /** @var TwoFactorMiddleware */ - private $middleware; + private TwoFactorMiddleware $middleware; protected function setUp(): void { parent::setUp(); @@ -74,14 +78,14 @@ protected function setUp(): void { $this->middleware = new TwoFactorMiddleware($this->twoFactorManager, $this->userSession, $this->urlGenerator, $this->reflector, $this->request); } - public function testBeforeControllerNotLoggedIn() { + public function testBeforeControllerNotLoggedIn(): void { $this->reflector->expects($this->once()) ->method('hasAnnotation') ->with('PublicPage') - ->will($this->returnValue(false)); + ->willReturn(false); $this->userSession->expects($this->once()) ->method('isLoggedIn') - ->will($this->returnValue(false)); + ->willReturn(false); $this->userSession->expects($this->never()) ->method('getUser'); @@ -89,117 +93,131 @@ public function testBeforeControllerNotLoggedIn() { $this->middleware->beforeController(null, 'index'); } - public function testBeforeControllerPublicPage() { + /** + * @throws TwoFactorAuthRequiredException + * @throws UserAlreadyLoggedInException + */ + public function testBeforeControllerPublicPage(): void { $this->reflector->expects($this->once()) ->method('hasAnnotation') ->with('PublicPage') - ->will($this->returnValue(true)); - $this->userSession->expects($this->never()) - ->method('isLoggedIn'); + ->willReturn(true); + $this->userSession->expects($this->once()) + ->method('isLoggedIn') + ->willReturn(false); $this->middleware->beforeController(null, 'create'); } - public function testBeforeControllerNoTwoFactorCheckNeeded() { - $user = $this->createMock('\OCP\IUser'); + /** + * @throws TwoFactorAuthRequiredException + * @throws UserAlreadyLoggedInException + */ + public function testBeforeControllerNoTwoFactorCheckNeeded(): void { + $user = $this->createMock(IUser::class); - $this->reflector->expects($this->once()) + $this->reflector->expects($this->never()) ->method('hasAnnotation') - ->with('PublicPage') - ->will($this->returnValue(false)); + ->with('PublicPage'); $this->userSession->expects($this->once()) ->method('isLoggedIn') - ->will($this->returnValue(true)); + ->willReturn(true); $this->userSession->expects($this->once()) ->method('getUser') - ->will($this->returnValue($user)); + ->willReturn($user); $this->twoFactorManager->expects($this->once()) ->method('isTwoFactorAuthenticated') ->with($user) - ->will($this->returnValue(false)); + ->willReturn(false); $this->middleware->beforeController(null, 'index'); } /** + * @throws UserAlreadyLoggedInException */ - public function testBeforeControllerTwoFactorAuthRequired() { - $this->expectException(\OC\Authentication\Exceptions\TwoFactorAuthRequiredException::class); + public function testBeforeControllerTwoFactorAuthRequired(): void { + $this->expectException(TwoFactorAuthRequiredException::class); - $user = $this->createMock('\OCP\IUser'); + $user = $this->createMock(IUser::class); - $this->reflector->expects($this->once()) + $this->reflector->expects($this->never()) ->method('hasAnnotation') - ->with('PublicPage') - ->will($this->returnValue(false)); + ->with('PublicPage'); $this->userSession->expects($this->once()) ->method('isLoggedIn') - ->will($this->returnValue(true)); + ->willReturn(true); $this->userSession->expects($this->once()) ->method('getUser') - ->will($this->returnValue($user)); + ->willReturn($user); $this->twoFactorManager->expects($this->once()) ->method('isTwoFactorAuthenticated') ->with($user) - ->will($this->returnValue(true)); + ->willReturn(true); $this->twoFactorManager->expects($this->once()) ->method('needsSecondFactor') - ->will($this->returnValue(true)); + ->willReturn(true); $this->middleware->beforeController(null, 'index'); } /** + * @throws TwoFactorAuthRequiredException */ - public function testBeforeControllerUserAlreadyLoggedIn() { - $this->expectException(\OC\Authentication\Exceptions\UserAlreadyLoggedInException::class); + public function testBeforeControllerUserAlreadyLoggedIn(): void { + $this->expectException(UserAlreadyLoggedInException::class); - $user = $this->createMock('\OCP\IUser'); + $user = $this->createMock(IUser::class); - $this->reflector->expects($this->once()) + $this->reflector->expects($this->never()) ->method('hasAnnotation') - ->with('PublicPage') - ->will($this->returnValue(false)); + ->with('PublicPage'); $this->userSession->expects($this->once()) ->method('isLoggedIn') - ->will($this->returnValue(true)); + ->willReturn(true); $this->userSession->expects($this->once()) ->method('getUser') - ->will($this->returnValue($user)); + ->willReturn($user); $this->twoFactorManager->expects($this->once()) ->method('isTwoFactorAuthenticated') ->with($user) - ->will($this->returnValue(true)); + ->willReturn(true); $this->twoFactorManager->expects($this->once()) ->method('needsSecondFactor') - ->will($this->returnValue(false)); + ->willReturn(false); - $twoFactorChallengeController = $this->getMockBuilder('\OC\Core\Controller\TwoFactorChallengeController') + $twoFactorChallengeController = $this->getMockBuilder(TwoFactorChallengeController::class) ->disableOriginalConstructor() ->getMock(); $this->middleware->beforeController($twoFactorChallengeController, 'index'); } - public function testAfterExceptionTwoFactorAuthRequired() { - $ex = new \OC\Authentication\Exceptions\TwoFactorAuthRequiredException(); + /** + * @throws \Exception + */ + public function testAfterExceptionTwoFactorAuthRequired(): void { + $ex = new TwoFactorAuthRequiredException(); $this->urlGenerator->expects($this->once()) ->method('linkToRoute') ->with('core.TwoFactorChallenge.selectChallenge') - ->will($this->returnValue('test/url')); - $expected = new \OCP\AppFramework\Http\RedirectResponse('test/url'); + ->willReturn('test/url'); + $expected = new RedirectResponse('test/url'); $this->assertEquals($expected, $this->middleware->afterException(null, 'index', $ex)); } - public function testAfterException() { - $ex = new \OC\Authentication\Exceptions\UserAlreadyLoggedInException(); + /** + * @throws \Exception + */ + public function testAfterException(): void { + $ex = new UserAlreadyLoggedInException(); $this->urlGenerator->expects($this->once()) ->method('getAbsoluteUrl') ->with('') - ->will($this->returnValue('http://cloud.local/')); - $expected = new \OCP\AppFramework\Http\RedirectResponse('http://cloud.local/'); + ->willReturn('http://cloud.local/'); + $expected = new RedirectResponse('http://cloud.local/'); $this->assertEquals($expected, $this->middleware->afterException(null, 'index', $ex)); }