Redirect users when already logged-in on login form

2 files changed, 120 insertions, 9 deletions

diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php
index ffbcea0fedd..dbc1f3157fd 100644
--- a/core/Controller/LoginController.php
+++ b/core/Controller/LoginController.php
@@ -172,8 +172,25 @@ 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 f09f3c98118..57fd5214021 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')