From b12ece0ff715a423e0f293f2648f3aa4bc55e445 Mon Sep 17 00:00:00 2001 From: Drak Date: Wed, 14 Mar 2012 17:38:34 +0545 Subject: [PATCH] [HttpFoundation][FrameworkBundle] Separate out mock session storage and stop polluting global namespace. This makes mock sessions truly mock and not to interfere with global namespace. Add getters and setters for session name and ID. --- .../EventListener/SessionListener.php | 7 ++ .../EventListener/TestSessionListener.php | 13 ++- .../EventListener/TestSessionListenerTest.php | 7 +- .../Component/HttpFoundation/Request.php | 3 +- .../HttpFoundation/Session/Session.php | 33 +++++- .../Session/SessionInterface.php | 36 ++++++ .../Handler/NativeFileSessionHandler.php | 2 +- .../Storage/MockArraySessionStorage.php | 106 ++++++++++++++---- .../Storage/MockFileSessionStorage.php | 50 ++++----- .../Session/Storage/Proxy/AbstractProxy.php | 48 ++++++++ .../Session/Storage/SessionStorage.php | 36 +++++- .../Storage/SessionStorageInterface.php | 32 +++++- .../Component/HttpFoundation/RequestTest.php | 6 +- .../Handler/NativeFileSessionHandlerTest.php | 2 +- .../Storage/MockFileSessionStorageTest.php | 7 +- 15 files changed, 312 insertions(+), 76 deletions(-) diff --git a/src/Symfony/Bundle/FrameworkBundle/EventListener/SessionListener.php b/src/Symfony/Bundle/FrameworkBundle/EventListener/SessionListener.php index 83fdec4b74..ad396347d5 100644 --- a/src/Symfony/Bundle/FrameworkBundle/EventListener/SessionListener.php +++ b/src/Symfony/Bundle/FrameworkBundle/EventListener/SessionListener.php @@ -28,7 +28,14 @@ use Symfony\Component\EventDispatcher\EventSubscriberInterface; */ class SessionListener implements EventSubscriberInterface { + /** + * @var ContainerInterface + */ private $container; + + /** + * @var boolean + */ private $autoStart; public function __construct(ContainerInterface $container, $autoStart = false) diff --git a/src/Symfony/Bundle/FrameworkBundle/EventListener/TestSessionListener.php b/src/Symfony/Bundle/FrameworkBundle/EventListener/TestSessionListener.php index 9010342d94..8f4536de8e 100644 --- a/src/Symfony/Bundle/FrameworkBundle/EventListener/TestSessionListener.php +++ b/src/Symfony/Bundle/FrameworkBundle/EventListener/TestSessionListener.php @@ -43,15 +43,16 @@ class TestSessionListener implements EventSubscriberInterface } // bootstrap the session - if ($this->container->has('session')) { - $this->container->get('session'); + if (!$this->container->has('session')) { + return; } + $session = $this->container->get('session'); $cookies = $event->getRequest()->cookies; - if ($cookies->has(session_name())) { - session_id($cookies->get(session_name())); + if ($cookies->has($session->getName())) { + $session->setId($cookies->get($session->getName())); } else { - session_id(''); + $session->setId(''); } } @@ -72,7 +73,7 @@ class TestSessionListener implements EventSubscriberInterface $params = session_get_cookie_params(); - $event->getResponse()->headers->setCookie(new Cookie(session_name(), $session->getId(), 0 === $params['lifetime'] ? 0 : time() + $params['lifetime'], $params['path'], $params['domain'], $params['secure'], $params['httponly'])); + $event->getResponse()->headers->setCookie(new Cookie($session->getName(), $session->getId(), 0 === $params['lifetime'] ? 0 : time() + $params['lifetime'], $params['path'], $params['domain'], $params['secure'], $params['httponly'])); } } diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/EventListener/TestSessionListenerTest.php b/src/Symfony/Bundle/FrameworkBundle/Tests/EventListener/TestSessionListenerTest.php index fe551ba360..4c577f292f 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Tests/EventListener/TestSessionListenerTest.php +++ b/src/Symfony/Bundle/FrameworkBundle/Tests/EventListener/TestSessionListenerTest.php @@ -94,8 +94,13 @@ class TestSessionListenerTest extends \PHPUnit_Framework_TestCase private function getSession() { - return $this->getMockBuilder('Symfony\Component\HttpFoundation\Session\Session') + $mock = $this->getMockBuilder('Symfony\Component\HttpFoundation\Session\Session') ->disableOriginalConstructor() ->getMock(); + + // set return value for getName() + $mock->expects($this->any())->method('getName')->will($this->returnValue('MOCKSESSID')); + + return $mock; } } diff --git a/src/Symfony/Component/HttpFoundation/Request.php b/src/Symfony/Component/HttpFoundation/Request.php index f38b1e7d0f..eaa453e1b4 100644 --- a/src/Symfony/Component/HttpFoundation/Request.php +++ b/src/Symfony/Component/HttpFoundation/Request.php @@ -496,7 +496,8 @@ class Request public function hasPreviousSession() { // the check for $this->session avoids malicious users trying to fake a session cookie with proper name - return $this->cookies->has(session_name()) && null !== $this->session; + $sessionName = $this->hasSession() ? $this->session->getName() : null; + return $this->cookies->has($sessionName) && $this->hasSession(); } /** diff --git a/src/Symfony/Component/HttpFoundation/Session/Session.php b/src/Symfony/Component/HttpFoundation/Session/Session.php index 6f2c6a4262..9f2e3326e8 100644 --- a/src/Symfony/Component/HttpFoundation/Session/Session.php +++ b/src/Symfony/Component/HttpFoundation/Session/Session.php @@ -17,6 +17,7 @@ use Symfony\Component\HttpFoundation\Session\Attribute\AttributeBagInterface; use Symfony\Component\HttpFoundation\Session\Flash\FlashBag; use Symfony\Component\HttpFoundation\Session\Flash\FlashBagInterface; use Symfony\Component\HttpFoundation\Session\SessionBagInterface; +use Symfony\Component\HttpFoundation\Session\Storage\SessionStorage; /** * Session. @@ -140,11 +141,7 @@ class Session implements SessionInterface } /** - * Returns the session ID - * - * @return mixed The session ID - * - * @api + * {@inheritdoc} */ public function getId() { @@ -152,7 +149,31 @@ class Session implements SessionInterface } /** - * Registers a SessionBagInterface with the sessio. + * {@inheritdoc} + */ + public function setId($id) + { + $this->storage->setId($id); + } + + /** + * {@inheritdoc} + */ + public function getName() + { + return $this->storage->getName(); + } + + /** + * {@inheritdoc} + */ + public function setName($name) + { + $this->storage->setName($name); + } + + /** + * Registers a SessionBagInterface with the session. * * @param SessionBagInterface $bag */ diff --git a/src/Symfony/Component/HttpFoundation/Session/SessionInterface.php b/src/Symfony/Component/HttpFoundation/Session/SessionInterface.php index 741969b10e..4e4962de4e 100644 --- a/src/Symfony/Component/HttpFoundation/Session/SessionInterface.php +++ b/src/Symfony/Component/HttpFoundation/Session/SessionInterface.php @@ -29,6 +29,42 @@ interface SessionInterface */ function start(); + /** + * Returns the session ID. + * + * @return string The session ID. + * + * @api + */ + function getId(); + + /** + * Sets the session ID + * + * @param string $id + * + * @api + */ + function setId($id); + + /** + * Returns the session name. + * + * @return mixed The session name. + * + * @api + */ + function getName(); + + /** + * Sets the session name. + * + * @param string $name + * + * @api + */ + function setName($name); + /** * Invalidates the current session. * diff --git a/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/NativeFileSessionHandler.php b/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/NativeFileSessionHandler.php index 5acf004d15..202d8d0f90 100644 --- a/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/NativeFileSessionHandler.php +++ b/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/NativeFileSessionHandler.php @@ -12,7 +12,7 @@ namespace Symfony\Component\HttpFoundation\Session\Storage\Handler; /** - * NativeFileSessionStorage. + * NativeFileSessionHandler. * * Native session handler using PHP's built in file storage. * diff --git a/src/Symfony/Component/HttpFoundation/Session/Storage/MockArraySessionStorage.php b/src/Symfony/Component/HttpFoundation/Session/Storage/MockArraySessionStorage.php index 482f3b6c20..6f1e279f41 100644 --- a/src/Symfony/Component/HttpFoundation/Session/Storage/MockArraySessionStorage.php +++ b/src/Symfony/Component/HttpFoundation/Session/Storage/MockArraySessionStorage.php @@ -11,7 +11,7 @@ namespace Symfony\Component\HttpFoundation\Session\Storage; -use Symfony\Component\HttpFoundation\Session\Storage\Handler\NullSessionHandler; +use Symfony\Component\HttpFoundation\Session\SessionBagInterface; /** * MockArraySessionStorage mocks the session for unit tests. @@ -25,21 +25,41 @@ use Symfony\Component\HttpFoundation\Session\Storage\Handler\NullSessionHandler; * @author Bulat Shakirzyanov * @author Drak */ -class MockArraySessionStorage extends SessionStorage +class MockArraySessionStorage implements SessionStorageInterface { /** * @var string */ - protected $sessionId; + protected $id = ''; + + /** + * @var string + */ + protected $name; + + /** + * @var boolean + */ + protected $started = false; + + /** + * @var boolean + */ + protected $closed = false; /** * @var array */ - protected $sessionData = array(); + protected $data = array(); - public function __construct(array $options = array()) + /** + * Constructor. + * + * @param string $name Session name + */ + public function __construct($name = 'MOCKSESSID') { - parent::__construct($options, new NullSessionHandler()); + $this->name = $name; } /** @@ -49,7 +69,7 @@ class MockArraySessionStorage extends SessionStorage */ public function setSessionData(array $array) { - $this->sessionData = $array; + $this->data = $array; } /** @@ -61,11 +81,11 @@ class MockArraySessionStorage extends SessionStorage return true; } - $this->started = true; - $this->loadSession($this->sessionData); + if (empty($this->id)) { + $this->id = $this->generateId(); + } - $this->sessionId = $this->generateSessionId(); - session_id($this->sessionId); + $this->loadSession(); return true; } @@ -80,8 +100,7 @@ class MockArraySessionStorage extends SessionStorage $this->start(); } - $this->sessionId = $this->generateSessionId(); - session_id($this->sessionId); + $this->id = $this->generateId(); return true; } @@ -91,11 +110,35 @@ class MockArraySessionStorage extends SessionStorage */ public function getId() { - if (!$this->started) { - return ''; + return $this->id; + } + + /** + * {@inheritdoc} + */ + public function setId($id) + { + if ($this->started) { + throw new \LogicException('Cannot set session ID after the session has started.'); } - return $this->sessionId; + $this->id = $id; + } + + /** + * {@inheritdoc} + */ + public function getName() + { + return $this->name; + } + + /** + * {@inheritdoc} + */ + public function setName($name) + { + $this->name = $name; } /** @@ -118,10 +161,18 @@ class MockArraySessionStorage extends SessionStorage } // clear out the session - $this->sessionData = array(); + $this->data = array(); // reconnect the bags to the session - $this->loadSession($this->sessionData); + $this->loadSession(); + } + + /** + * {@inheritdoc} + */ + public function registerBag(SessionBagInterface $bag) + { + $this->bags[$bag->getName()] = $bag; } /** @@ -143,10 +194,25 @@ class MockArraySessionStorage extends SessionStorage /** * Generates a session ID. * + * This doesn't need to be particularly cryptographically secure since this is just + * a mock. + * * @return string */ - protected function generateSessionId() + protected function generateId() { - return sha1(uniqid(mt_rand(), true)); + return sha1(uniqid(mt_rand())); + } + + protected function loadSession() + { + foreach ($this->bags as $bag) { + $key = $bag->getStorageKey(); + $this->data[$key] = isset($this->data[$key]) ? $this->data[$key] : array(); + $bag->initialize($this->data[$key]); + } + + $this->started = true; + $this->closed = false; } } diff --git a/src/Symfony/Component/HttpFoundation/Session/Storage/MockFileSessionStorage.php b/src/Symfony/Component/HttpFoundation/Session/Storage/MockFileSessionStorage.php index 184f76e747..24457319f9 100644 --- a/src/Symfony/Component/HttpFoundation/Session/Storage/MockFileSessionStorage.php +++ b/src/Symfony/Component/HttpFoundation/Session/Storage/MockFileSessionStorage.php @@ -16,7 +16,9 @@ namespace Symfony\Component\HttpFoundation\Session\Storage; * functional testing when done in a single PHP process. * * No PHP session is actually started since a session can be initialized - * and shutdown only once per PHP execution cycle. + * and shutdown only once per PHP execution cycle and this class does + * not pollute any session related globals, including session_*() functions + * or session.* PHP ini directives. * * @author Drak */ @@ -31,11 +33,9 @@ class MockFileSessionStorage extends MockArraySessionStorage * Constructor. * * @param string $savePath Path of directory to save session files. - * @param array $options Session options. - * - * @see SessionStorage::__construct() + * @param string $name Session name. */ - public function __construct($savePath = null, array $options = array()) + public function __construct($savePath = null, $name = 'MOCKSESSID') { if (null === $savePath) { $savePath = sys_get_temp_dir(); @@ -47,7 +47,7 @@ class MockFileSessionStorage extends MockArraySessionStorage $this->savePath = $savePath; - parent::__construct($options); + parent::__construct($name); } /** @@ -59,12 +59,10 @@ class MockFileSessionStorage extends MockArraySessionStorage return true; } - if (!session_id()) { - session_id($this->generateSessionId()); + if (!$this->id) { + $this->id = $this->generateId(); } - $this->sessionId = session_id(); - $this->read(); $this->started = true; @@ -81,10 +79,7 @@ class MockFileSessionStorage extends MockArraySessionStorage $this->destroy(); } - session_id($this->generateSessionId()); - $this->sessionId = session_id(); - - $this->save(); + $this->id = $this->generateId(); return true; } @@ -92,23 +87,15 @@ class MockFileSessionStorage extends MockArraySessionStorage /** * {@inheritdoc} */ - public function getId() + public function save() { - if (!$this->started) { - return ''; - } - - return $this->sessionId; + file_put_contents($this->getFilePath(), serialize($this->data)); } /** - * {@inheritdoc} + * Deletes a session from persistent storage. + * Deliberately leaves session data in memory intact. */ - public function save() - { - file_put_contents($this->getFilePath(), serialize($this->sessionData)); - } - private function destroy() { if (is_file($this->getFilePath())) { @@ -121,16 +108,19 @@ class MockFileSessionStorage extends MockArraySessionStorage * * @return string File path */ - public function getFilePath() + private function getFilePath() { - return $this->savePath.'/'.$this->sessionId.'.sess'; + return $this->savePath.'/'.$this->id.'.mocksess'; } + /** + * Reads session from storage and loads session. + */ private function read() { $filePath = $this->getFilePath(); - $this->sessionData = is_readable($filePath) && is_file($filePath) ? unserialize(file_get_contents($filePath)) : array(); + $this->data = is_readable($filePath) && is_file($filePath) ? unserialize(file_get_contents($filePath)) : array(); - $this->loadSession($this->sessionData); + $this->loadSession(); } } diff --git a/src/Symfony/Component/HttpFoundation/Session/Storage/Proxy/AbstractProxy.php b/src/Symfony/Component/HttpFoundation/Session/Storage/Proxy/AbstractProxy.php index 0cd4d73d96..09f9efa091 100644 --- a/src/Symfony/Component/HttpFoundation/Session/Storage/Proxy/AbstractProxy.php +++ b/src/Symfony/Component/HttpFoundation/Session/Storage/Proxy/AbstractProxy.php @@ -84,4 +84,52 @@ abstract class AbstractProxy { $this->active = (bool) $flag; } + + /** + * Gets the session ID. + * + * @return string + */ + public function getId() + { + return session_id(); + } + + /** + * Sets the session ID. + * + * @param string $id + */ + public function setId($id) + { + if ($this->isActive()) { + throw new \LogicException('Cannot change the ID of an active session'); + } + + session_id($id); + } + + /** + * Gets the session name. + * + * @return string + */ + public function getName() + { + return session_name(); + } + + /** + * Sets the session name. + * + * @param string $name + */ + public function setName($name) + { + if ($this->isActive()) { + throw new \LogicException('Cannot change the name of an active session'); + } + + session_name($name); + } } diff --git a/src/Symfony/Component/HttpFoundation/Session/Storage/SessionStorage.php b/src/Symfony/Component/HttpFoundation/Session/Storage/SessionStorage.php index 47a6044e87..194318aa2e 100644 --- a/src/Symfony/Component/HttpFoundation/Session/Storage/SessionStorage.php +++ b/src/Symfony/Component/HttpFoundation/Session/Storage/SessionStorage.php @@ -93,6 +93,12 @@ class SessionStorage implements SessionStorageInterface ini_set('session.cache_limiter', ''); // disable by default because it's managed by HeaderBag (if used) ini_set('session.use_cookies', 1); + if (version_compare(phpversion(), '5.4.0', '>=')) { + session_register_shutdown(); + } else { + register_shutdown_function('session_write_close'); + } + $this->setOptions($options); $this->setSaveHandler($handler); } @@ -151,7 +157,31 @@ class SessionStorage implements SessionStorageInterface return ''; // returning empty is consistent with session_id() behaviour } - return session_id(); + return $this->saveHandler->getId(); + } + + /** + * {@inheritdoc} + */ + public function setId($id) + { + return $this->saveHandler->setId($id); + } + + /** + * {@inheritdoc} + */ + public function getName() + { + return $this->saveHandler->getName(); + } + + /** + * {@inheritdoc} + */ + public function setName($name) + { + $this->saveHandler->setName($name); } /** @@ -275,7 +305,7 @@ class SessionStorage implements SessionStorageInterface if ($this->saveHandler instanceof \SessionHandlerInterface) { if (version_compare(phpversion(), '5.4.0', '>=')) { - session_set_save_handler($this->saveHandler, true); + session_set_save_handler($this->saveHandler, false); } else { session_set_save_handler( array($this->saveHandler, 'open'), @@ -285,8 +315,6 @@ class SessionStorage implements SessionStorageInterface array($this->saveHandler, 'destroy'), array($this->saveHandler, 'gc') ); - - register_shutdown_function('session_write_close'); } } } diff --git a/src/Symfony/Component/HttpFoundation/Session/Storage/SessionStorageInterface.php b/src/Symfony/Component/HttpFoundation/Session/Storage/SessionStorageInterface.php index 6065a550dc..8bf2e5d32a 100644 --- a/src/Symfony/Component/HttpFoundation/Session/Storage/SessionStorageInterface.php +++ b/src/Symfony/Component/HttpFoundation/Session/Storage/SessionStorageInterface.php @@ -37,12 +37,39 @@ interface SessionStorageInterface /** * Returns the session ID * - * @return mixed The session ID or false if the session has not started. + * @return string The session ID or empty. * * @api */ function getId(); + /** + * Sets the session ID + * + * @param string $id + * + * @api + */ + function setId($id); + + /** + * Returns the session name + * + * @return mixed The session name. + * + * @api + */ + function getName(); + + /** + * Sets the session name + * + * @param string $name + * + * @api + */ + function setName($name); + /** * Regenerates id that represents this storage. * @@ -51,6 +78,9 @@ interface SessionStorageInterface * or functional testing where a real PHP session would interfere * with testing. * + * Note regenerate+destroy should not clear the session data in memory + * only delete the session data from persistent storage. + * * @param Boolean $destroy Destroy session when regenerating? * * @return Boolean True if session regenerated, false if error diff --git a/tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php b/tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php index 3e672da273..136b8fc95b 100644 --- a/tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php +++ b/tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php @@ -866,7 +866,7 @@ class RequestTest extends \PHPUnit_Framework_TestCase public function testHasSession() { - $request = new Request; + $request = new Request(); $this->assertFalse($request->hasSession()); $request->setSession(new Session(new MockArraySessionStorage())); @@ -875,10 +875,10 @@ class RequestTest extends \PHPUnit_Framework_TestCase public function testHasPreviousSession() { - $request = new Request; + $request = new Request(); $this->assertFalse($request->hasPreviousSession()); - $request->cookies->set(session_name(), 'foo'); + $request->cookies->set('MOCKSESSID', 'foo'); $this->assertFalse($request->hasPreviousSession()); $request->setSession(new Session(new MockArraySessionStorage())); $this->assertTrue($request->hasPreviousSession()); diff --git a/tests/Symfony/Tests/Component/HttpFoundation/Session/Storage/Handler/NativeFileSessionHandlerTest.php b/tests/Symfony/Tests/Component/HttpFoundation/Session/Storage/Handler/NativeFileSessionHandlerTest.php index 08b8644ebf..37eb40cbe9 100644 --- a/tests/Symfony/Tests/Component/HttpFoundation/Session/Storage/Handler/NativeFileSessionHandlerTest.php +++ b/tests/Symfony/Tests/Component/HttpFoundation/Session/Storage/Handler/NativeFileSessionHandlerTest.php @@ -6,7 +6,7 @@ use Symfony\Component\HttpFoundation\Session\Storage\Handler\NativeFileSessionHa use Symfony\Component\HttpFoundation\Session\Storage\SessionStorage; /** - * Test class for NativeFileSessionStorage. + * Test class for NativeFileSessionHandler. * * @author Drak * diff --git a/tests/Symfony/Tests/Component/HttpFoundation/Session/Storage/MockFileSessionStorageTest.php b/tests/Symfony/Tests/Component/HttpFoundation/Session/Storage/MockFileSessionStorageTest.php index 770bcda8f0..c0453df977 100644 --- a/tests/Symfony/Tests/Component/HttpFoundation/Session/Storage/MockFileSessionStorageTest.php +++ b/tests/Symfony/Tests/Component/HttpFoundation/Session/Storage/MockFileSessionStorageTest.php @@ -69,6 +69,7 @@ class MockFileSessionStorageTest extends \PHPUnit_Framework_TestCase public function testSave() { $this->storage->start(); + $id = $this->storage->getId(); $this->assertNotEquals('108', $this->storage->getBag('attributes')->get('new')); $this->assertFalse($this->storage->getBag('flashes')->has('newkey')); $this->storage->getBag('attributes')->set('new', '108'); @@ -76,6 +77,7 @@ class MockFileSessionStorageTest extends \PHPUnit_Framework_TestCase $this->storage->save(); $storage = $this->getStorage(); + $storage->setId($id); $storage->start(); $this->assertEquals('108', $storage->getBag('attributes')->get('new')); $this->assertTrue($storage->getBag('flashes')->has('newkey')); @@ -90,13 +92,14 @@ class MockFileSessionStorageTest extends \PHPUnit_Framework_TestCase $storage1->save(); $storage2 = $this->getStorage(); + $storage2->setId($storage1->getId()); $storage2->start(); $this->assertEquals('bar', $storage2->getBag('attributes')->get('foo'), 'values persist between instances'); } - private function getStorage(array $options = array()) + private function getStorage() { - $storage = new MockFileSessionStorage($this->sessionDir, $options); + $storage = new MockFileSessionStorage($this->sessionDir); $storage->registerBag(new FlashBag); $storage->registerBag(new AttributeBag);