From be1e64486f9e62b104e085509ca5e05b99a550ce Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Tue, 9 Aug 2016 19:01:50 +0200 Subject: [PATCH] Redirect users when already logged-in on login form --- core/Controller/LoginController.php | 34 +++++-- tests/Core/Controller/LoginControllerTest.php | 95 +++++++++++++++++++ 2 files changed, 120 insertions(+), 9 deletions(-) diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php index ffbcea0fed..dbc1f3157f 100644 --- a/core/Controller/LoginController.php +++ b/core/Controller/LoginController.php @@ -171,9 +171,26 @@ class LoginController extends Controller { ); } + /** + * @param string $redirectUrl + * @return RedirectResponse + */ + private function generateRedirect($redirectUrl) { + if (!is_null($redirectUrl) && $this->userSession->isLoggedIn()) { + $location = $this->urlGenerator->getAbsoluteURL(urldecode($redirectUrl)); + // Deny the redirect if the URL contains a @ + // This prevents unvalidated redirects like ?redirect_url=:user@domain.com + if (strpos($location, '@') === false) { + return new RedirectResponse($location); + } + } + return new RedirectResponse(OC_Util::getDefaultPageUrl()); + } + /** * @PublicPage * @UseSession + * @NoCSRFRequired * * @param string $user * @param string $password @@ -184,6 +201,13 @@ class LoginController extends Controller { $currentDelay = $this->throttler->getDelay($this->request->getRemoteAddress()); $this->throttler->sleepDelay($this->request->getRemoteAddress()); + // If the user is already logged in and the CSRF check does not pass then + // simply redirect the user to the correct page as required. This is the + // case when an user has already logged-in, in another tab. + if(!$this->request->passesCSRFCheck()) { + return $this->generateRedirect($redirect_url); + } + $originalUser = $user; // TODO: Add all the insane error handling /* @var $loginResult IUser */ @@ -223,15 +247,7 @@ class LoginController extends Controller { return new RedirectResponse($this->urlGenerator->linkToRoute('core.TwoFactorChallenge.selectChallenge')); } - if (!is_null($redirect_url) && $this->userSession->isLoggedIn()) { - $location = $this->urlGenerator->getAbsoluteURL(urldecode($redirect_url)); - // Deny the redirect if the URL contains a @ - // This prevents unvalidated redirects like ?redirect_url=:user@domain.com - if (strpos($location, '@') === false) { - return new RedirectResponse($location); - } - } - return new RedirectResponse(OC_Util::getDefaultPageUrl()); + return $this->generateRedirect($redirect_url); } } diff --git a/tests/Core/Controller/LoginControllerTest.php b/tests/Core/Controller/LoginControllerTest.php index f09f3c9811..57fd521402 100644 --- a/tests/Core/Controller/LoginControllerTest.php +++ b/tests/Core/Controller/LoginControllerTest.php @@ -292,6 +292,10 @@ class LoginControllerTest extends TestCase { ->expects($this->exactly(4)) ->method('getRemoteAddress') ->willReturn('192.168.0.1'); + $this->request + ->expects($this->once()) + ->method('passesCSRFCheck') + ->willReturn(true); $this->throttler ->expects($this->exactly(2)) ->method('sleepDelay') @@ -330,6 +334,10 @@ class LoginControllerTest extends TestCase { ->expects($this->exactly(2)) ->method('getRemoteAddress') ->willReturn('192.168.0.1'); + $this->request + ->expects($this->once()) + ->method('passesCSRFCheck') + ->willReturn(true); $this->throttler ->expects($this->once()) ->method('sleepDelay') @@ -361,6 +369,81 @@ class LoginControllerTest extends TestCase { $this->assertEquals($expected, $this->loginController->tryLogin($user, $password, null)); } + public function testLoginWithoutPassedCsrfCheckAndNotLoggedIn() { + /** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */ + $user = $this->createMock('\OCP\IUser'); + $user->expects($this->any()) + ->method('getUID') + ->will($this->returnValue('jane')); + $password = 'secret'; + $originalUrl = 'another%20url'; + + $this->request + ->expects($this->exactly(2)) + ->method('getRemoteAddress') + ->willReturn('192.168.0.1'); + $this->request + ->expects($this->once()) + ->method('passesCSRFCheck') + ->willReturn(false); + $this->throttler + ->expects($this->once()) + ->method('sleepDelay') + ->with('192.168.0.1'); + $this->throttler + ->expects($this->once()) + ->method('getDelay') + ->with('192.168.0.1') + ->willReturn(200); + $this->userSession->expects($this->once()) + ->method('isLoggedIn') + ->with() + ->will($this->returnValue(false)); + + $expected = new \OCP\AppFramework\Http\RedirectResponse(\OC_Util::getDefaultPageUrl()); + $this->assertEquals($expected, $this->loginController->tryLogin('Jane', $password, $originalUrl)); + } + + public function testLoginWithoutPassedCsrfCheckAndLoggedIn() { + /** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */ + $user = $this->createMock('\OCP\IUser'); + $user->expects($this->any()) + ->method('getUID') + ->will($this->returnValue('jane')); + $password = 'secret'; + $originalUrl = 'another%20url'; + $redirectUrl = 'http://localhost/another url'; + + $this->request + ->expects($this->exactly(2)) + ->method('getRemoteAddress') + ->willReturn('192.168.0.1'); + $this->request + ->expects($this->once()) + ->method('passesCSRFCheck') + ->willReturn(false); + $this->throttler + ->expects($this->once()) + ->method('sleepDelay') + ->with('192.168.0.1'); + $this->throttler + ->expects($this->once()) + ->method('getDelay') + ->with('192.168.0.1') + ->willReturn(200); + $this->userSession->expects($this->once()) + ->method('isLoggedIn') + ->with() + ->will($this->returnValue(true)); + $this->urlGenerator->expects($this->once()) + ->method('getAbsoluteURL') + ->with(urldecode($originalUrl)) + ->will($this->returnValue($redirectUrl)); + + $expected = new \OCP\AppFramework\Http\RedirectResponse($redirectUrl); + $this->assertEquals($expected, $this->loginController->tryLogin('Jane', $password, $originalUrl)); + } + public function testLoginWithValidCredentialsAndRedirectUrl() { /** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */ $user = $this->getMock('\OCP\IUser'); @@ -375,6 +458,10 @@ class LoginControllerTest extends TestCase { ->expects($this->exactly(2)) ->method('getRemoteAddress') ->willReturn('192.168.0.1'); + $this->request + ->expects($this->once()) + ->method('passesCSRFCheck') + ->willReturn(true); $this->throttler ->expects($this->once()) ->method('sleepDelay') @@ -417,6 +504,10 @@ class LoginControllerTest extends TestCase { ->expects($this->exactly(2)) ->method('getRemoteAddress') ->willReturn('192.168.0.1'); + $this->request + ->expects($this->once()) + ->method('passesCSRFCheck') + ->willReturn(true); $this->throttler ->expects($this->once()) ->method('sleepDelay') @@ -479,6 +570,10 @@ class LoginControllerTest extends TestCase { ->expects($this->exactly(3)) ->method('getRemoteAddress') ->willReturn('192.168.0.1'); + $this->request + ->expects($this->once()) + ->method('passesCSRFCheck') + ->willReturn(true); $this->throttler ->expects($this->once()) ->method('getDelay')