Merge pull request #837 from nextcloud/when-logged-in-then-just-redirect-to-redirected-page

When logged in then just redirect to redirected page

2 files changed, 132 insertions, 21 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..a322289d200 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,84 @@ 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');
+		$user = $this->createMock('\OCP\IUser');
 		$user->expects($this->any())
 			->method('getUID')
 			->will($this->returnValue('jane'));
@@ -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')
@@ -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')