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.

diff --git a/lib/base.php b/lib/base.php
index 34cbfe3066c66aa1c7c3e44a8b7112ecc64d6dc8..2cace2a0a06a84155c58e9b3855450044df63cf9 100644 (file)
--- 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 dcae1648fe19986152ad878b86e8585563ed7e5d..b600874412b3629a7c104ef2dbf45fd4dec5ec09 100644 (file)
--- a/lib/private/session/cryptosessiondata.php
+++ b/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
         */

diff --git a/lib/private/session/internal.php b/lib/private/session/internal.php
index 0b6152acf128c96b8fd7d70f741a5cc962d0e86e..8be3356c6db420b7acf105d6a5adbb28fc033b0e 100644 (file)
--- 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 ff95efc5345c775998816d5e913d3d6bf07feb5f..c609008745704330be9d3b0412b826cf981f6be7 100644 (file)
--- a/lib/private/session/memory.php
+++ b/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
         */

diff --git a/lib/private/user.php b/lib/private/user.php
index cfa60d675feff231ab438feb37c3a93804ce4096..fa1cea9072f7e7fc29956a75cd5e51a5506e8819 100644 (file)
--- 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 ba702c9f365b70d6b45ed86edd716aab5ddb9583..be38b1b1d8e537cb81afdb0aacc9873259695030 100644 (file)
--- 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 aee635d7a9d5ca2e4bc40fe15744a8f19e14a201..89a181ad0fdffaf290704deac9b66a9384275d54 100644 (file)
--- 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);
 }

diff --git a/tests/lib/user/session.php b/tests/lib/user/session.php
index d9dace2ef05389735e636c95980e3f5624e532df..ffd4f96d8016168da5b0e58c37b765924352dfe7 100644 (file)
--- a/tests/lib/user/session.php
+++ b/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