Move logout to controller

Testable code. Yay.

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')
 			);
 		});
 

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'));
 	}
 
 	/**

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'],
 	]
 ]);
 

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();
-			}
 		} else {
 			// Not handled and not logged in
 			self::handleLogin();

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);

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;

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.'"';
 	}
 
 	/**

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);

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();
 	}
 
 	/**

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())