From 331e4efacb226f95551962f3a53030feced0b190 Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Mon, 11 Apr 2016 12:08:00 +0200 Subject: [PATCH] Move login form into controller First step on getting the authorisation stuff cleaned up. This is only for the login form, all other stuff is still where it is. --- core/Application.php | 14 ++ core/Controller/LoginController.php | 138 ++++++++++++++ core/routes.php | 1 + core/templates/login.php | 2 +- lib/base.php | 6 +- .../security/securitymiddleware.php | 9 +- lib/private/util.php | 41 +--- tests/core/controller/LoginControllerTest.php | 176 ++++++++++++++++++ .../security/SecurityMiddlewareTest.php | 13 +- 9 files changed, 352 insertions(+), 48 deletions(-) create mode 100644 core/Controller/LoginController.php create mode 100644 tests/core/controller/LoginControllerTest.php diff --git a/core/Application.php b/core/Application.php index 30376ee4f2..805208d469 100644 --- a/core/Application.php +++ b/core/Application.php @@ -28,6 +28,7 @@ namespace OC\Core; use OC\AppFramework\Utility\SimpleContainer; use OC\AppFramework\Utility\TimeFactory; +use OC\Core\Controller\LoginController; use \OCP\AppFramework\App; use OC\Core\Controller\LostController; use OC\Core\Controller\UserController; @@ -89,6 +90,16 @@ class Application extends App { $c->query('Logger') ); }); + $container->registerService('LoginController', function(SimpleContainer $c) { + return new LoginController( + $c->query('AppName'), + $c->query('Request'), + $c->query('UserManager'), + $c->query('Config'), + $c->query('Session'), + $c->query('UserSession') + ); + }); /** * Core class wrappers @@ -114,6 +125,9 @@ class Application extends App { $container->registerService('AvatarManager', function(SimpleContainer $c) { return $c->query('ServerContainer')->getAvatarManager(); }); + $container->registerService('Session', function(SimpleContainer $c) { + return $c->query('ServerContainer')->getSession(); + }); $container->registerService('UserSession', function(SimpleContainer $c) { return $c->query('ServerContainer')->getUserSession(); }); diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php new file mode 100644 index 0000000000..661fb04385 --- /dev/null +++ b/core/Controller/LoginController.php @@ -0,0 +1,138 @@ + + * + * @copyright Copyright (c) 2016, ownCloud, Inc. + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ + +namespace OC\Core\Controller; + +use OC\Setup; +use OCP\AppFramework\Controller; +use OCP\AppFramework\Http\RedirectResponse; +use OCP\AppFramework\Http\TemplateResponse; +use OCP\IConfig; +use OCP\IRequest; +use OCP\ISession; +use OCP\IUser; +use OCP\IUserManager; +use OCP\IUserSession; + +class LoginController extends Controller { + /** @var IUserManager */ + private $userManager; + /** @var IConfig */ + private $config; + /** @var ISession */ + private $session; + /** @var IUserSession */ + private $userSession; + + /** + * @param string $appName + * @param IRequest $request + * @param IUserManager $userManager + * @param IConfig $config + * @param ISession $session + * @param IUserSession $userSession + */ + function __construct($appName, + IRequest $request, + IUserManager $userManager, + IConfig $config, + ISession $session, + IUserSession $userSession) { + parent::__construct($appName, $request); + $this->userManager = $userManager; + $this->config = $config; + $this->session = $session; + $this->userSession = $userSession; + } + + /** + * @PublicPage + * @NoCSRFRequired + * @UseSession + * + * @param string $user + * @param string $redirect_url + * @param string $remember_login + * + * @return TemplateResponse + */ + public function showLoginForm($user, + $redirect_url, + $remember_login) { + if($this->userSession->isLoggedIn()) { + return new RedirectResponse(\OC_Util::getDefaultPageUrl()); + } + + $parameters = array(); + $loginMessages = $this->session->get('loginMessages'); + $errors = []; + $messages = []; + if(is_array($loginMessages)) { + list($errors, $messages) = $loginMessages; + } + $this->session->remove('loginMessages'); + foreach ($errors as $value) { + $parameters[$value] = true; + } + + $parameters['messages'] = $messages; + if (!empty($user)) { + $parameters['username'] = $user; + $parameters['user_autofocus'] = false; + } else { + $parameters['username'] = ''; + $parameters['user_autofocus'] = true; + } + if (!empty($redirect_url)) { + $parameters['redirect_url'] = $redirect_url; + } + + $parameters['canResetPassword'] = true; + if (!$this->config->getSystemValue('lost_password_link')) { + if (!empty($user)) { + $userObj = $this->userManager->get($user); + if ($userObj instanceof IUser) { + $parameters['canResetPassword'] = $userObj->canChangePassword(); + } + } + } + + $parameters['alt_login'] = \OC_App::getAlternativeLogIns(); + $parameters['rememberLoginAllowed'] = \OC_Util::rememberLoginAllowed(); + $parameters['rememberLoginState'] = !empty($remember_login) ? $remember_login : 0; + + if (!empty($user)) { + $parameters['username'] = $user; + $parameters['user_autofocus'] = false; + } else { + $parameters['username'] = ''; + $parameters['user_autofocus'] = true; + } + + return new TemplateResponse( + $this->appName, + 'login', + $parameters, + 'guest' + ); + } + +} diff --git a/core/routes.php b/core/routes.php index 8981eb618f..bcad9d0dad 100644 --- a/core/routes.php +++ b/core/routes.php @@ -42,6 +42,7 @@ $application->registerRoutes($this, [ ['name' => 'avatar#postCroppedAvatar', 'url' => '/avatar/cropped', 'verb' => 'POST'], ['name' => 'avatar#getTmpAvatar', 'url' => '/avatar/tmp', 'verb' => 'GET'], ['name' => 'avatar#postAvatar', 'url' => '/avatar/', 'verb' => 'POST'], + ['name' => 'login#showLoginForm', 'url' => '/login', 'verb' => 'GET'], ] ]); diff --git a/core/templates/login.php b/core/templates/login.php index 9934d4988d..f683323337 100644 --- a/core/templates/login.php +++ b/core/templates/login.php @@ -9,7 +9,7 @@ script('core', [ ?> -
+
'); diff --git a/lib/base.php b/lib/base.php index 728d9bced9..d8ef17e42e 100644 --- a/lib/base.php +++ b/lib/base.php @@ -951,7 +951,11 @@ class OC { $error[] = 'internalexception'; } - OC_Util::displayLoginPage(array_unique($error), $messages); + if(!\OC::$server->getUserSession()->isLoggedIn()) { + $loginMessages = array(array_unique($error), $messages); + \OC::$server->getSession()->set('loginMessages', $loginMessages); + header('Location: ' . \OC::$server->getURLGenerator()->linkToRoute('core.login.showLoginForm')); + } } /** diff --git a/lib/private/appframework/middleware/security/securitymiddleware.php b/lib/private/appframework/middleware/security/securitymiddleware.php index 75bcc29a92..4afd29cd06 100644 --- a/lib/private/appframework/middleware/security/securitymiddleware.php +++ b/lib/private/appframework/middleware/security/securitymiddleware.php @@ -192,9 +192,12 @@ class SecurityMiddleware extends Middleware { ); } else { if($exception instanceof NotLoggedInException) { - // TODO: replace with link to route - $url = $this->urlGenerator->getAbsoluteURL('index.php'); - $url .= '?redirect_url=' . urlencode($this->request->server['REQUEST_URI']); + $url = $this->urlGenerator->linkToRoute( + 'core.login.showLoginForm', + [ + 'redirect_url' => urlencode($this->request->server['REQUEST_URI']), + ] + ); $response = new RedirectResponse($url); } else { $response = new TemplateResponse('core', '403', ['file' => $exception->getMessage()], 'guest'); diff --git a/lib/private/util.php b/lib/private/util.php index 039bc7e915..7caa1efcf5 100644 --- a/lib/private/util.php +++ b/lib/private/util.php @@ -946,44 +946,6 @@ class OC_Util { return $errors; } - /** - * @param array $errors - * @param string[] $messages - */ - public static function displayLoginPage($errors = array(), $messages = []) { - $parameters = array(); - foreach ($errors as $value) { - $parameters[$value] = true; - } - $parameters['messages'] = $messages; - if (!empty($_REQUEST['user'])) { - $parameters["username"] = $_REQUEST['user']; - $parameters['user_autofocus'] = false; - } else { - $parameters["username"] = ''; - $parameters['user_autofocus'] = true; - } - if (isset($_REQUEST['redirect_url'])) { - $parameters['redirect_url'] = $_REQUEST['redirect_url']; - } - - $parameters['canResetPassword'] = true; - if (!\OC::$server->getSystemConfig()->getValue('lost_password_link')) { - if (isset($_REQUEST['user'])) { - $user = \OC::$server->getUserManager()->get($_REQUEST['user']); - if ($user instanceof IUser) { - $parameters['canResetPassword'] = $user->canChangePassword(); - } - } - } - - $parameters['alt_login'] = OC_App::getAlternativeLogIns(); - $parameters['rememberLoginAllowed'] = self::rememberLoginAllowed(); - $parameters['rememberLoginState'] = isset($_POST['remember_login']) ? $_POST['remember_login'] : 0; - \OC_Hook::emit('OC_Util', 'pre_displayLoginPage', array('parameters' => $parameters)); - OC_Template::printGuestPage("", "login", $parameters); - } - /** * Check if the user is logged in, redirects to home if not. With * redirect URL parameter to the request URI. @@ -993,7 +955,8 @@ class OC_Util { public static function checkLoggedIn() { // Check if we are a user if (!OC_User::isLoggedIn()) { - header('Location: ' . \OCP\Util::linkToAbsolute('', 'index.php', + header('Location: ' . \OC::$server->getURLGenerator()->linkToRoute( + 'core.login.showLoginForm', [ 'redirect_url' => \OC::$server->getRequest()->getRequestUri() ] diff --git a/tests/core/controller/LoginControllerTest.php b/tests/core/controller/LoginControllerTest.php new file mode 100644 index 0000000000..2c634d79fa --- /dev/null +++ b/tests/core/controller/LoginControllerTest.php @@ -0,0 +1,176 @@ + + * + * @copyright Copyright (c) 2016, ownCloud, Inc. + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ + +namespace OC\Core\Controller; + +use OCP\AppFramework\Http\RedirectResponse; +use OCP\AppFramework\Http\TemplateResponse; +use OCP\IConfig; +use OCP\IRequest; +use OCP\ISession; +use OCP\IUserManager; +use OCP\IUserSession; +use Test\TestCase; + +class LoginControllerTest extends TestCase { + /** @var LoginController */ + private $loginController; + /** @var IRequest */ + private $request; + /** @var IUserManager */ + private $userManager; + /** @var IConfig */ + private $config; + /** @var ISession */ + private $session; + /** @var IUserSession */ + private $userSession; + + 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->userSession = $this->getMock('\\OCP\\IUserSession'); + + $this->loginController = new LoginController( + 'core', + $this->request, + $this->userManager, + $this->config, + $this->session, + $this->userSession + ); + } + + public function testShowLoginFormForLoggedInUsers() { + $this->userSession + ->expects($this->once()) + ->method('isLoggedIn') + ->willReturn(true); + + $expectedResponse = new RedirectResponse(\OC_Util::getDefaultPageUrl()); + $this->assertEquals($expectedResponse, $this->loginController->showLoginForm('', '', '')); + } + + public function testShowLoginFormWithErrorsInSession() { + $this->userSession + ->expects($this->once()) + ->method('isLoggedIn') + ->willReturn(false); + $this->session + ->expects($this->once()) + ->method('get') + ->with('loginMessages') + ->willReturn( + [ + [ + 'ErrorArray1', + 'ErrorArray2', + ], + [ + 'MessageArray1', + 'MessageArray2', + ], + ] + ); + + $expectedResponse = new TemplateResponse( + 'core', + 'login', + [ + 'ErrorArray1' => true, + 'ErrorArray2' => true, + 'messages' => [ + 'MessageArray1', + 'MessageArray2', + ], + 'username' => '', + 'user_autofocus' => true, + 'canResetPassword' => true, + 'alt_login' => [], + 'rememberLoginAllowed' => \OC_Util::rememberLoginAllowed(), + 'rememberLoginState' => 0, + ], + 'guest' + ); + $this->assertEquals($expectedResponse, $this->loginController->showLoginForm('', '', '')); + } + + /** + * @return array + */ + public function passwordResetDataProvider() { + return [ + [ + true, + true, + ], + [ + false, + false, + ], + ]; + } + + /** + * @dataProvider passwordResetDataProvider + */ + public function testShowLoginFormWithPasswordResetOption($canChangePassword, + $expectedResult) { + $this->userSession + ->expects($this->once()) + ->method('isLoggedIn') + ->willReturn(false); + $this->config + ->expects($this->once()) + ->method('getSystemValue') + ->with('lost_password_link') + ->willReturn(false); + $user = $this->getMock('\\OCP\\IUser'); + $user + ->expects($this->once()) + ->method('canChangePassword') + ->willReturn($canChangePassword); + $this->userManager + ->expects($this->once()) + ->method('get') + ->with('LdapUser') + ->willReturn($user); + + $expectedResponse = new TemplateResponse( + 'core', + 'login', + [ + 'messages' => [], + 'username' => 'LdapUser', + 'user_autofocus' => false, + 'canResetPassword' => $expectedResult, + 'alt_login' => [], + 'rememberLoginAllowed' => \OC_Util::rememberLoginAllowed(), + 'rememberLoginState' => 0, + ], + 'guest' + ); + $this->assertEquals($expectedResponse, $this->loginController->showLoginForm('LdapUser', '', '')); + } +} diff --git a/tests/lib/appframework/middleware/security/SecurityMiddlewareTest.php b/tests/lib/appframework/middleware/security/SecurityMiddlewareTest.php index 9e71a3d096..dd4ec3af96 100644 --- a/tests/lib/appframework/middleware/security/SecurityMiddlewareTest.php +++ b/tests/lib/appframework/middleware/security/SecurityMiddlewareTest.php @@ -343,9 +343,14 @@ class SecurityMiddlewareTest extends \Test\TestCase { $this->middleware = $this->getMiddleware(false, false); $this->urlGenerator ->expects($this->once()) - ->method('getAbsoluteURL') - ->with('index.php') - ->will($this->returnValue('http://localhost/index.php')); + ->method('linkToRoute') + ->with( + 'core.login.showLoginForm', + [ + 'redirect_url' => 'owncloud%2Findex.php%2Fapps%2Fspecialapp', + ] + ) + ->will($this->returnValue('http://localhost/index.php/login?redirect_url=owncloud%2Findex.php%2Fapps%2Fspecialapp')); $this->logger ->expects($this->once()) ->method('debug') @@ -356,7 +361,7 @@ class SecurityMiddlewareTest extends \Test\TestCase { new NotLoggedInException() ); - $expected = new RedirectResponse('http://localhost/index.php?redirect_url=owncloud%2Findex.php%2Fapps%2Fspecialapp'); + $expected = new RedirectResponse('http://localhost/index.php/login?redirect_url=owncloud%2Findex.php%2Fapps%2Fspecialapp'); $this->assertEquals($expected , $response); }