Move regeneration of session ID into session classes

There were code paths that nowadays call ISession::login directly thus bypassing the desired regeneration of the session ID. This moves the session regeneration deeper into the session handling and thus ensures that it is always called. Furthermore, I also added the session regeneration to the remember me cookie plus added some test case expectations for this.

lib/base.php

@@ -442,7 +442,7 @@ class OC {
 		if (!$session->exists('SID_CREATED')) {
 			$session->set('SID_CREATED', time());
 		} else if (time() - $session->get('SID_CREATED') > $sessionLifeTime / 2) {
-			session_regenerate_id(true);
+			$session->regenerateId();
 			$session->set('SID_CREATED', time());
 		}
 

lib/private/session/cryptosessiondata.php

@@ -131,6 +131,16 @@ class CryptoSessionData implements \ArrayAccess, ISession {
 		$this->session->clear();
 	}
 
+	/**
+	 * Wrapper around session_regenerate_id
+	 *
+	 * @param bool $deleteOldSession Whether to delete the old associated session file or not.
+	 * @return void
+	 */
+	public function regenerateId($deleteOldSession = true) {
+		$this->session->regenerateId($deleteOldSession);
+	}
+
 	/**
 	 * Close the session and release the lock, also writes all changed data in batch
 	 */

lib/private/session/internal.php

@@ -89,10 +89,9 @@ class Internal extends Session {
 		}
 	}
 
-
 	public function clear() {
 		session_unset();
-		@session_regenerate_id(true);
+		$this->regenerateId();
 		@session_start();
 		$_SESSION = array();
 	}
@@ -102,14 +101,35 @@ class Internal extends Session {
 		parent::close();
 	}
 
+	/**
+	 * Wrapper around session_regenerate_id
+	 *
+	 * @param bool $deleteOldSession Whether to delete the old associated session file or not.
+	 * @return void
+	 */
+	public function regenerateId($deleteOldSession = true) {
+		@session_regenerate_id($deleteOldSession);
+	}
+
+	/**
+	 * @throws \Exception
+	 */
 	public function reopen() {
 		throw new \Exception('The session cannot be reopened - reopen() is ony to be used in unit testing.');
 	}
 
+	/**
+	 * @param int $errorNumber
+	 * @param string $errorString
+	 * @throws \ErrorException
+	 */
 	public function trapError($errorNumber, $errorString) {
 		throw new \ErrorException($errorString);
 	}
 
+	/**
+	 * @throws \Exception
+	 */
 	private function validateSession() {
 		if ($this->sessionClosed) {
 			throw new \Exception('Session has been closed - no further changes to the session are allowed');

lib/private/session/memory.php

@@ -80,6 +80,13 @@ class Memory extends Session {
 		$this->data = array();
 	}
 
+	/**
+	 * Stub since the session ID does not need to get regenerated for the cache
+	 *
+	 * @param bool $deleteOldSession
+	 */
+	public function regenerateId($deleteOldSession = true) {}
+
 	/**
 	 * Helper function for PHPUnit execution - don't use in non-test code
 	 */

lib/private/user.php

@@ -162,7 +162,6 @@ class OC_User {
 	 * Log in a user and regenerate a new session - if the password is ok
 	 */
 	public static function login($loginname, $password) {
-		session_regenerate_id(true);
 		$result = self::getUserSession()->login($loginname, $password);
 		if ($result) {
 			//we need to pass the user name, which may differ from login name

lib/private/user/session.php

@@ -213,6 +213,7 @@ class Session implements IUserSession, Emitter {
 	 * @throws LoginException
 	 */
 	public function login($uid, $password) {
+		$this->session->regenerateId();
 		$this->manager->emit('\OC\User', 'preLogin', array($uid, $password));
 		$user = $this->manager->checkPassword($uid, $password);
 		if ($user !== false) {
@@ -243,6 +244,7 @@ class Session implements IUserSession, Emitter {
 	 * @return bool
 	 */
 	public function loginWithCookie($uid, $currentToken) {
+		$this->session->regenerateId();
 		$this->manager->emit('\OC\User', 'preRememberedLogin', array($uid));
 		$user = $this->manager->get($uid);
 		if (is_null($user)) {

lib/public/isession.php

@@ -86,4 +86,12 @@ interface ISession {
 	 */
 	public function close();
 
+	/**
+	 * Wrapper around session_regenerate_id
+	 *
+	 * @param bool $deleteOldSession Whether to delete the old associated session file or not.
+	 * @return void
+	 * @since 9.0.0
+	 */
+	public function regenerateId($deleteOldSession = true);
 }

tests/lib/user/session.php

@@ -95,6 +95,8 @@ class Session extends \Test\TestCase {
 
 	public function testLoginValidPasswordEnabled() {
 		$session = $this->getMock('\OC\Session\Memory', array(), array(''));
+		$session->expects($this->once())
+			->method('regenerateId');
 		$session->expects($this->exactly(2))
 			->method('set')
 			->with($this->callback(function ($key) {
@@ -148,6 +150,8 @@ class Session extends \Test\TestCase {
 		$session = $this->getMock('\OC\Session\Memory', array(), array(''));
 		$session->expects($this->never())
 			->method('set');
+		$session->expects($this->once())
+				->method('regenerateId');
 
 		$managerMethods = get_class_methods('\OC\User\Manager');
 		//keep following methods intact in order to ensure hooks are
@@ -179,10 +183,12 @@ class Session extends \Test\TestCase {
 		$userSession->login('foo', 'bar');
 	}
 
-	public function testLoginInValidPassword() {
+	public function testLoginInvalidPassword() {
 		$session = $this->getMock('\OC\Session\Memory', array(), array(''));
 		$session->expects($this->never())
 			->method('set');
+		$session->expects($this->once())
+				->method('regenerateId');
 
 		$managerMethods = get_class_methods('\OC\User\Manager');
 		//keep following methods intact in order to ensure hooks are
@@ -217,6 +223,8 @@ class Session extends \Test\TestCase {
 		$session = $this->getMock('\OC\Session\Memory', array(), array(''));
 		$session->expects($this->never())
 			->method('set');
+		$session->expects($this->once())
+				->method('regenerateId');
 
 		$manager = $this->getMock('\OC\User\Manager');
 
@@ -244,6 +252,8 @@ class Session extends \Test\TestCase {
 					}
 				},
 				'foo'));
+		$session->expects($this->once())
+				->method('regenerateId');
 
 		$managerMethods = get_class_methods('\OC\User\Manager');
 		//keep following methods intact in order to ensure hooks are
@@ -292,6 +302,8 @@ class Session extends \Test\TestCase {
 		$session = $this->getMock('\OC\Session\Memory', array(), array(''));
 		$session->expects($this->never())
 			->method('set');
+		$session->expects($this->once())
+				->method('regenerateId');
 
 		$managerMethods = get_class_methods('\OC\User\Manager');
 		//keep following methods intact in order to ensure hooks are
@@ -334,6 +346,8 @@ class Session extends \Test\TestCase {
 		$session = $this->getMock('\OC\Session\Memory', array(), array(''));
 		$session->expects($this->never())
 			->method('set');
+		$session->expects($this->once())
+				->method('regenerateId');
 
 		$managerMethods = get_class_methods('\OC\User\Manager');
 		//keep following methods intact in order to ensure hooks are