Redirect users when already logged-in on login form

diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php
index ffbcea0feddebb802673dacd0004ff5fa359b07b..dbc1f3157fd79dcbd11064373e3ec7195dd8907d 100644 (file)
--- 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 f09f3c9811889713cf99abbf899a0e8409578eba..57fd5214021a8cbe621deafb4286dad69025602e 100644 (file)
--- 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')