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.

7 files changed, 53 insertions, 7 deletions

diff --git a/lib/base.php b/lib/base.php
index 34cbfe3066c..2cace2a0a06 100644
--- a/lib/base.php
+++ b/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());
 		}
 
diff --git a/lib/private/session/cryptosessiondata.php b/lib/private/session/cryptosessiondata.php
index dcae1648fe1..b600874412b 100644
--- a/lib/private/session/cryptosessiondata.php
+++ b/lib/private/session/cryptosessiondata.php
@@ -132,6 +132,16 @@ class CryptoSessionData implements \ArrayAccess, ISession {
 	}
 
 	/**
+	 * 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
 	 */
 	public function close() {
diff --git a/lib/private/session/internal.php b/lib/private/session/internal.php
index 0b6152acf12..8be3356c6db 100644
--- a/lib/private/session/internal.php
+++ b/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();
 	}
 
-    public function reopen() {
-        throw new \Exception('The session cannot be reopened - reopen() is ony to be used in unit testing.');
-    }
+	/**
+	 * 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');
diff --git a/lib/private/session/memory.php b/lib/private/session/memory.php
index ff95efc5345..c6090087457 100644
--- a/lib/private/session/memory.php
+++ b/lib/private/session/memory.php
@@ -81,6 +81,13 @@ class Memory extends Session {
 	}
 
 	/**
+	 * 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
 	 */
 	public function reopen() {
diff --git a/lib/private/user.php b/lib/private/user.php
index cfa60d675fe..fa1cea9072f 100644
--- a/lib/private/user.php
+++ b/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
diff --git a/lib/private/user/session.php b/lib/private/user/session.php
index ba702c9f365..be38b1b1d8e 100644
--- a/lib/private/user/session.php
+++ b/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)) {
diff --git a/lib/public/isession.php b/lib/public/isession.php
index aee635d7a9d..89a181ad0fd 100644
--- a/lib/public/isession.php
+++ b/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);
 }