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.
2016-04-11 12:08:00 +02:00 · 2016-04-11 12:08:00 +02:00 · 331e4efacb
parent 63a385d2b8
commit 331e4efacb
9 changed files with 352 additions and 48 deletions
--- 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();
 		});
--- a/core/Controller/LoginController.php
+++ b/core/Controller/LoginController.php
@ -0,0 +1,138 @@
+<?php
+/**
+ * @author Lukas Reschke <lukas@owncloud.com>
+ *
+ * @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 <http://www.gnu.org/licenses/>
+ *
+ */
+
+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'
+		);
+	}
+
+}
--- 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'],
 	]
 ]);

--- a/core/templates/login.php
+++ b/core/templates/login.php
@ -9,7 +9,7 @@ script('core', [
 ?>

 <!--[if IE 8]><style>input[type="checkbox"]{padding:0;}</style><![endif]-->
-<form method="post" name="login">
+<form method="post" name="login" action="<?php p(OC::$WEBROOT) ?>/">
 	<fieldset>
 	<?php if (!empty($_['redirect_url'])) {
 		print_unescaped('<input type="hidden" name="redirect_url" value="' . \OCP\Util::sanitizeHTML($_['redirect_url']) . '">');
--- 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'));
+		}
 	}

 	/**
--- 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');
--- 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()
 					]
--- a/tests/core/controller/LoginControllerTest.php
+++ b/tests/core/controller/LoginControllerTest.php
@ -0,0 +1,176 @@
+<?php
+/**
+ * @author Lukas Reschke <lukas@owncloud.com>
+ *
+ * @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 <http://www.gnu.org/licenses/>
+ *
+ */
+
+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', '', ''));
+	}
+}
--- 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);
 	}