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..a322289d20 100644 --- a/tests/Core/Controller/LoginControllerTest.php +++ b/tests/Core/Controller/LoginControllerTest.php @@ -57,14 +57,14 @@ class LoginControllerTest extends TestCase { public function setUp() { parent::setUp(); - $this->request = $this->getMock('\\OCP\\IRequest'); - $this->userManager = $this->getMock('\\OCP\\IUserManager'); - $this->config = $this->getMock('\\OCP\\IConfig'); - $this->session = $this->getMock('\\OCP\\ISession'); + $this->request = $this->createMock('\\OCP\\IRequest'); + $this->userManager = $this->createMock('\\OCP\\IUserManager'); + $this->config = $this->createMock('\\OCP\\IConfig'); + $this->session = $this->createMock('\\OCP\\ISession'); $this->userSession = $this->getMockBuilder('\\OC\\User\\Session') ->disableOriginalConstructor() ->getMock(); - $this->urlGenerator = $this->getMock('\\OCP\\IURLGenerator'); + $this->urlGenerator = $this->createMock('\\OCP\\IURLGenerator'); $this->twoFactorManager = $this->getMockBuilder('\OC\Authentication\TwoFactorAuth\Manager') ->disableOriginalConstructor() ->getMock(); @@ -110,7 +110,7 @@ class LoginControllerTest extends TestCase { ->method('getCookie') ->with('oc_token') ->willReturn('MyLoginToken'); - $user = $this->getMock('\\OCP\\IUser'); + $user = $this->createMock('\\OCP\\IUser'); $user ->expects($this->once()) ->method('getUID') @@ -217,7 +217,7 @@ class LoginControllerTest extends TestCase { ->method('getSystemValue') ->with('lost_password_link') ->willReturn(false); - $user = $this->getMock('\\OCP\\IUser'); + $user = $this->createMock('\\OCP\\IUser'); $user ->expects($this->once()) ->method('canChangePassword') @@ -255,7 +255,7 @@ class LoginControllerTest extends TestCase { ->method('getSystemValue') ->with('lost_password_link') ->willReturn(false); - $user = $this->getMock('\\OCP\\IUser'); + $user = $this->createMock('\\OCP\\IUser'); $user ->expects($this->once()) ->method('canChangePassword') @@ -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') @@ -322,7 +326,7 @@ class LoginControllerTest extends TestCase { public function testLoginWithValidCredentials() { /** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */ - $user = $this->getMock('\OCP\IUser'); + $user = $this->createMock('\OCP\IUser'); $password = 'secret'; $indexPageUrl = 'some url'; @@ -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,9 +369,44 @@ class LoginControllerTest extends TestCase { $this->assertEquals($expected, $this->loginController->tryLogin($user, $password, null)); } - public function testLoginWithValidCredentialsAndRedirectUrl() { + public function testLoginWithoutPassedCsrfCheckAndNotLoggedIn() { /** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */ - $user = $this->getMock('\OCP\IUser'); + $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')); @@ -375,6 +418,50 @@ class LoginControllerTest extends TestCase { ->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->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(true); $this->throttler ->expects($this->once()) ->method('sleepDelay') @@ -406,7 +493,7 @@ class LoginControllerTest extends TestCase { public function testLoginWithTwoFactorEnforced() { /** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */ - $user = $this->getMock('\OCP\IUser'); + $user = $this->createMock('\OCP\IUser'); $user->expects($this->any()) ->method('getUID') ->will($this->returnValue('john')); @@ -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') @@ -453,7 +544,7 @@ class LoginControllerTest extends TestCase { public function testToNotLeakLoginName() { /** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */ - $user = $this->getMock('\OCP\IUser'); + $user = $this->createMock('\OCP\IUser'); $user->expects($this->any()) ->method('getUID') ->will($this->returnValue('john')); @@ -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')