From 8222ad515706d62cceb14428c959b83a69ccbc8b Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Mon, 18 Apr 2016 12:14:07 +0200 Subject: [PATCH] Move logout to controller Testable code. Yay. --- core/Application.php | 3 +- core/Controller/LoginController.php | 24 +++++++- core/routes.php | 1 + lib/base.php | 24 +------- lib/private/api.php | 2 +- lib/private/appframework/http/request.php | 2 +- lib/private/user.php | 18 +++--- lib/public/irequest.php | 2 +- lib/public/user.php | 2 +- tests/core/controller/LoginControllerTest.php | 55 ++++++++++++++++++- 10 files changed, 95 insertions(+), 38 deletions(-) diff --git a/core/Application.php b/core/Application.php index 805208d469..0a54386a2c 100644 --- a/core/Application.php +++ b/core/Application.php @@ -97,7 +97,8 @@ class Application extends App { $c->query('UserManager'), $c->query('Config'), $c->query('Session'), - $c->query('UserSession') + $c->query('UserSession'), + $c->query('URLGenerator') ); }); diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php index faed7e291e..796706d364 100644 --- a/core/Controller/LoginController.php +++ b/core/Controller/LoginController.php @@ -28,6 +28,7 @@ use OCP\AppFramework\Http\TemplateResponse; use OCP\IConfig; use OCP\IRequest; use OCP\ISession; +use OCP\IURLGenerator; use OCP\IUser; use OCP\IUserManager; use OCP\IUserSession; @@ -41,6 +42,8 @@ class LoginController extends Controller { private $session; /** @var IUserSession */ private $userSession; + /** @var IURLGenerator */ + private $urlGenerator; /** * @param string $appName @@ -49,18 +52,37 @@ class LoginController extends Controller { * @param IConfig $config * @param ISession $session * @param IUserSession $userSession + * @param IURLGenerator $urlGenerator */ function __construct($appName, IRequest $request, IUserManager $userManager, IConfig $config, ISession $session, - IUserSession $userSession) { + IUserSession $userSession, + IURLGenerator $urlGenerator) { parent::__construct($appName, $request); $this->userManager = $userManager; $this->config = $config; $this->session = $session; $this->userSession = $userSession; + $this->urlGenerator = $urlGenerator; + } + + /** + * @NoAdminRequired + * @UseSession + * + * @return RedirectResponse + */ + public function logout() { + $loginToken = $this->request->getCookie('oc_token'); + if (!is_null($loginToken)) { + $this->config->deleteUserValue($this->userSession->getUser()->getUID(), 'login_token', $loginToken); + } + $this->userSession->logout(); + + return new RedirectResponse($this->urlGenerator->linkToRouteAbsolute('core.login.showLoginForm')); } /** diff --git a/core/routes.php b/core/routes.php index bcad9d0dad..2b7a19f7d8 100644 --- a/core/routes.php +++ b/core/routes.php @@ -43,6 +43,7 @@ $application->registerRoutes($this, [ ['name' => 'avatar#getTmpAvatar', 'url' => '/avatar/tmp', 'verb' => 'GET'], ['name' => 'avatar#postAvatar', 'url' => '/avatar/', 'verb' => 'POST'], ['name' => 'login#showLoginForm', 'url' => '/login', 'verb' => 'GET'], + ['name' => 'login#logout', 'url' => '/logout', 'verb' => 'GET'], ] ]); diff --git a/lib/base.php b/lib/base.php index 2796758836..6bc0fecf04 100644 --- a/lib/base.php +++ b/lib/base.php @@ -858,7 +858,7 @@ class OC { } } - if (!self::$CLI and (!isset($_GET["logout"]) or ($_GET["logout"] !== 'true'))) { + if (!self::$CLI) { try { if (!$systemConfig->getValue('maintenance', false) && !self::checkUpgrade(false)) { OC_App::loadApps(array('filesystem', 'logging')); @@ -897,31 +897,13 @@ class OC { return; } - // Redirect to index if the logout link is accessed without valid session - // this is needed to prevent "Token expired" messages while login if a session is expired - // @see https://github.com/owncloud/core/pull/8443#issuecomment-42425583 - if(isset($_GET['logout']) && !OC_User::isLoggedIn()) { - header("Location: " . \OC::$server->getURLGenerator()->getAbsoluteURL('/')); - return; - } - // Someone is logged in if (OC_User::isLoggedIn()) { OC_App::loadApps(); OC_User::setupBackends(); OC_Util::setupFS(); - if (isset($_GET["logout"]) and ($_GET["logout"])) { - OC_JSON::callCheck(); - if (isset($_COOKIE['oc_token'])) { - \OC::$server->getConfig()->deleteUserValue(OC_User::getUser(), 'login_token', $_COOKIE['oc_token']); - } - OC_User::logout(); - // redirect to webroot and add slash if webroot is empty - header("Location: " . \OC::$server->getURLGenerator()->getAbsoluteURL('/')); - } else { - // Redirect to default application - OC_Util::redirectToDefaultPage(); - } + // Redirect to default application + OC_Util::redirectToDefaultPage(); } else { // Not handled and not logged in self::handleLogin(); diff --git a/lib/private/api.php b/lib/private/api.php index 12a78f1424..bab879c95f 100644 --- a/lib/private/api.php +++ b/lib/private/api.php @@ -179,7 +179,7 @@ class OC_API { $response = self::mergeResponses($responses); $format = self::requestedFormat(); if (self::$logoutRequired) { - OC_User::logout(); + \OC::$server->getUserSession()->logout(); } self::respond($response, $format); diff --git a/lib/private/appframework/http/request.php b/lib/private/appframework/http/request.php index c8525d1d14..7cd8cedcfd 100644 --- a/lib/private/appframework/http/request.php +++ b/lib/private/appframework/http/request.php @@ -368,7 +368,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { /** * Shortcut for getting cookie variables * @param string $key the key that will be taken from the $_COOKIE array - * @return array the value in the $_COOKIE element + * @return string the value in the $_COOKIE element */ public function getCookie($key) { return isset($this->cookies[$key]) ? $this->cookies[$key] : null; diff --git a/lib/private/user.php b/lib/private/user.php index 26062f503d..8767a8d5b6 100644 --- a/lib/private/user.php +++ b/lib/private/user.php @@ -267,15 +267,6 @@ class OC_User { } } - /** - * Logs the current user out and kills all the session data - * - * Logout, destroys session - */ - public static function logout() { - self::getUserSession()->logout(); - } - /** * Tries to login the user with HTTP Basic Authentication */ @@ -342,7 +333,14 @@ class OC_User { return $backend->getLogoutAttribute(); } - return 'href="' . link_to('', 'index.php') . '?logout=true&requesttoken=' . urlencode(\OCP\Util::callRegister()) . '"'; + $logoutUrl = \OC::$server->getURLGenerator()->linkToRouteAbsolute( + 'core.login.logout', + [ + 'requesttoken' => \OCP\Util::callRegister(), + ] + ); + + return 'href="'.$logoutUrl.'"'; } /** diff --git a/lib/public/irequest.php b/lib/public/irequest.php index a0040aa464..296c70f4ec 100644 --- a/lib/public/irequest.php +++ b/lib/public/irequest.php @@ -129,7 +129,7 @@ interface IRequest { * Shortcut for getting cookie variables * * @param string $key the key that will be taken from the $_COOKIE array - * @return array the value in the $_COOKIE element + * @return string the value in the $_COOKIE element * @since 6.0.0 */ public function getCookie($key); diff --git a/lib/public/user.php b/lib/public/user.php index 825e77aef6..64ac92d210 100644 --- a/lib/public/user.php +++ b/lib/public/user.php @@ -119,7 +119,7 @@ class User { * @since 5.0.0 */ public static function logout() { - \OC_User::logout(); + \OC::$server->getUserSession()->logout(); } /** diff --git a/tests/core/controller/LoginControllerTest.php b/tests/core/controller/LoginControllerTest.php index f0ffe57d7b..f9a6080892 100644 --- a/tests/core/controller/LoginControllerTest.php +++ b/tests/core/controller/LoginControllerTest.php @@ -26,6 +26,7 @@ use OCP\AppFramework\Http\TemplateResponse; use OCP\IConfig; use OCP\IRequest; use OCP\ISession; +use OCP\IURLGenerator; use OCP\IUserManager; use OCP\IUserSession; use Test\TestCase; @@ -43,6 +44,8 @@ class LoginControllerTest extends TestCase { private $session; /** @var IUserSession */ private $userSession; + /** @var IURLGenerator */ + private $urlGenerator; public function setUp() { parent::setUp(); @@ -51,6 +54,7 @@ class LoginControllerTest extends TestCase { $this->config = $this->getMock('\\OCP\\IConfig'); $this->session = $this->getMock('\\OCP\\ISession'); $this->userSession = $this->getMock('\\OCP\\IUserSession'); + $this->urlGenerator = $this->getMock('\\OCP\\IURLGenerator'); $this->loginController = new LoginController( 'core', @@ -58,10 +62,59 @@ class LoginControllerTest extends TestCase { $this->userManager, $this->config, $this->session, - $this->userSession + $this->userSession, + $this->urlGenerator ); } + public function testLogoutWithoutToken() { + $this->request + ->expects($this->once()) + ->method('getCookie') + ->with('oc_token') + ->willReturn(null); + $this->config + ->expects($this->never()) + ->method('deleteUserValue'); + $this->urlGenerator + ->expects($this->once()) + ->method('linkToRouteAbsolute') + ->with('core.login.showLoginForm') + ->willReturn('/login'); + + $expected = new RedirectResponse('/login'); + $this->assertEquals($expected, $this->loginController->logout()); + } + + public function testLogoutWithToken() { + $this->request + ->expects($this->once()) + ->method('getCookie') + ->with('oc_token') + ->willReturn('MyLoginToken'); + $user = $this->getMock('\\OCP\\IUser'); + $user + ->expects($this->once()) + ->method('getUID') + ->willReturn('JohnDoe'); + $this->userSession + ->expects($this->once()) + ->method('getUser') + ->willReturn($user); + $this->config + ->expects($this->once()) + ->method('deleteUserValue') + ->with('JohnDoe', 'login_token', 'MyLoginToken'); + $this->urlGenerator + ->expects($this->once()) + ->method('linkToRouteAbsolute') + ->with('core.login.showLoginForm') + ->willReturn('/login'); + + $expected = new RedirectResponse('/login'); + $this->assertEquals($expected, $this->loginController->logout()); + } + public function testShowLoginFormForLoggedInUsers() { $this->userSession ->expects($this->once())