From 941ce6d68ed507987c0a2615c977e04dedfb11c6 Mon Sep 17 00:00:00 2001 From: Matthias Jentsch Date: Wed, 20 Nov 2013 12:01:40 +0100 Subject: [PATCH] Fix bug that caused ajax-request to override values written to the session Authentication/Session.php and its Subclasses do not have a open/closed -state anymore. Read will refresh the session, write will always write the changes, and opening/closing will be handled internally. refs #5101 --- .../controllers/AuthenticationController.php | 17 +- .../controllers/PreferenceController.php | 9 - library/Icinga/Application/Web.php | 7 +- library/Icinga/Authentication/Manager.php | 9 +- library/Icinga/Authentication/PhpSession.php | 157 ++++-------------- library/Icinga/Authentication/Session.php | 18 +- library/Icinga/User.php | 25 ++- .../Web/Controller/ActionController.php | 16 +- .../Icinga/Authentication/PhpSessionTest.php | 53 +++--- 9 files changed, 80 insertions(+), 231 deletions(-) diff --git a/application/controllers/AuthenticationController.php b/application/controllers/AuthenticationController.php index b9a3e1216..be6a4a463 100644 --- a/application/controllers/AuthenticationController.php +++ b/application/controllers/AuthenticationController.php @@ -48,15 +48,6 @@ class AuthenticationController extends ActionController */ protected $requiresAuthentication = false; - /** - * This controller modifies the session - * - * @var bool - * - * @see \Icinga\Web\Controller\ActionController::$modifiesSession - */ - protected $modifiesSession = true; - /** * Log into the application */ @@ -69,9 +60,7 @@ class AuthenticationController extends ActionController $this->view->form->setRequest($this->_request); $this->view->title = "Icinga Web Login"; try { - $auth = AuthManager::getInstance(null, array( - 'writeSession' => $this->modifiesSession - )); + $auth = AuthManager::getInstance(); if ($auth->isAuthenticated()) { $this->redirectNow('index?_render=body'); @@ -104,9 +93,7 @@ class AuthenticationController extends ActionController public function logoutAction() { $this->_helper->layout->setLayout('inline'); - $auth = AuthManager::getInstance(null, array( - 'writeSession' => $this->modifiesSession - )); + $auth = AuthManager::getInstance(); $auth->removeAuthorization(); $this->redirectToLogin(); } diff --git a/application/controllers/PreferenceController.php b/application/controllers/PreferenceController.php index b7d64ec4f..a8245e136 100644 --- a/application/controllers/PreferenceController.php +++ b/application/controllers/PreferenceController.php @@ -40,15 +40,6 @@ use \Icinga\Form\Preference\GeneralForm; class PreferenceController extends BasePreferenceController { - /** - * This controller modifies the session - * - * @var bool - * - * @see \Icinga\Web\Controller\ActionController::$modifiesSession - */ - protected $modifiesSession = true; - /** * Create tabs for this preference controller * diff --git a/library/Icinga/Application/Web.php b/library/Icinga/Application/Web.php index a5d5da7d2..756316af8 100644 --- a/library/Icinga/Application/Web.php +++ b/library/Icinga/Application/Web.php @@ -232,12 +232,7 @@ class Web extends ApplicationBootstrap */ private function setupUser() { - $authenticationManager = AuthenticationManager::getInstance( - null, - array( - 'writeSession' => true - ) - ); + $authenticationManager = AuthenticationManager::getInstance(); if ($authenticationManager->isAuthenticated() === true) { $user = $authenticationManager->getUser(); diff --git a/library/Icinga/Authentication/Manager.php b/library/Icinga/Authentication/Manager.php index b089ea015..9445581f7 100644 --- a/library/Icinga/Authentication/Manager.php +++ b/library/Icinga/Authentication/Manager.php @@ -121,16 +121,10 @@ class Manager } if (!isset($options['sessionClass'])) { - $this->session = new PhpSession($config->session); + $this->session = new PhpSession(); } else { $this->session = $options['sessionClass']; } - - if (isset($options['writeSession']) && $options['writeSession'] === true) { - $this->session->read(true); - } else { - $this->session->read(); - } } /** @@ -353,6 +347,7 @@ class Manager return false; } + $this->user->sendMessage("Hallo ich bin nachricht."); if ($persist == true) { $this->persistCurrentUser(); $this->session->write(); diff --git a/library/Icinga/Authentication/PhpSession.php b/library/Icinga/Authentication/PhpSession.php index 14fe12525..fea5b3663 100644 --- a/library/Icinga/Authentication/PhpSession.php +++ b/library/Icinga/Authentication/PhpSession.php @@ -51,21 +51,7 @@ class PhpSession extends Session * * @var string */ - const SESSION_NAME = 'Icinga2Web'; - - /** - * Flag if session is open - * - * @var bool - */ - private $isOpen = false; - - /** - * Flag if session is flushed - * - * @var bool - */ - private $isFlushed = false; + private $sessionName = 'Icingaweb2'; /** * Configuration for cookie options @@ -96,8 +82,12 @@ class PhpSession extends Session } else { $options = PhpSession::$defaultCookieOptions; } + if (array_key_exists('test_session_name', $options)) { + $this->sessionName = $options['test_session_name']; + unset($options['test_session_name']); + } foreach ($options as $sessionVar => $value) { - if (ini_set("session.".$sessionVar, $value) === false) { + if (ini_set("session." . $sessionVar, $value) === false) { Logger::warn( 'Could not set php.ini setting %s = %s. This might affect your sessions behaviour.', $sessionVar, @@ -108,129 +98,38 @@ class PhpSession extends Session if (!is_writable(session_save_path())) { throw new ConfigurationError('Can\'t save session'); } + $this->read(); } /** - * Return true when the session has not yet been closed - * - * @return bool + * Open a PHP session */ - private function sessionCanBeChanged() + private function open() { - if ($this->isFlushed) { - Logger::error('Tried to work on a closed session, session changes will be ignored'); - return false; - } - return true; - } - - /** - * Return true when the session has not yet been opened - * - * @return bool - */ - private function sessionCanBeOpened() - { - if ($this->isOpen) { - Logger::warn('Tried to open a session more than once'); - return false; - } - return $this->sessionCanBeChanged(); - } - - /** - * Open a PHP session when possible - * - * @return bool True on success - */ - public function open() - { - if (!$this->sessionCanBeOpened()) { - return false; - } - - session_name(self::SESSION_NAME); + session_name($this->sessionName); session_start(); - $this->isOpen = true; + } + + /** + * Read all values written to the underling session and make them accessible. + */ + public function read() + { + $this->open(); $this->setAll($_SESSION); - return true; - } - - /** - * Ensure that the session is open modifiable - * - * @return bool True on success - */ - private function ensureOpen() - { - // try to open first - if (!$this->isOpen) { - if (!$this->open()) { - return false; - } - } - return true; - } - - /** - * Read all values written to the underling session and - * makes them accessible. if keepOpen is not set, the session - * is immediately closed again - * - * @param bool $keepOpen Set to true when modifying the session - * - * @return bool True on success - */ - public function read($keepOpen = false) - { - if (!$this->ensureOpen()) { - return false; - } - if ($keepOpen) { - return true; - } - $this->close(); - return true; + session_write_close(); } /** * Write all values of this session object to the underlying session implementation - * - * If keepOpen is not set, the session is closed - * - * @param bool $keepOpen Set to true when modifying the session further - * - * @return bool True on success */ - public function write($keepOpen = false) + public function write() { - if (!$this->ensureOpen()) { - return false; - } + $this->open(); foreach ($this->getAll() as $key => $value) { $_SESSION[$key] = $value; } - if ($keepOpen) { - return null; - } - $this->close(); - - return null; - } - - /** - * Close and writes the session - * - * Only call this if you want the session to be closed without any changes. - * - * @see PHPSession::write - */ - public function close() - { - if (!$this->isFlushed) { - session_write_close(); - } - $this->isFlushed = true; + session_write_close(); } /** @@ -238,12 +137,12 @@ class PhpSession extends Session */ public function purge() { - if ($this->ensureOpen()) { - $_SESSION = array(); - session_destroy(); - $this->clearCookies(); - $this->close(); - } + $this->open(); + $_SESSION = array(); + $this->setAll(array(), true); + session_destroy(); + $this->clearCookies(); + session_write_close(); } /** diff --git a/library/Icinga/Authentication/Session.php b/library/Icinga/Authentication/Session.php index 0cdd5b3bd..ad2a65f5e 100644 --- a/library/Icinga/Authentication/Session.php +++ b/library/Icinga/Authentication/Session.php @@ -41,29 +41,15 @@ abstract class Session */ private $sessionValues = array(); - /** - * Open a session or creates a new one if not exists - */ - abstract public function open(); - /** * Read all values from the underlying session implementation - * - * @param bool $keepOpen True to keep the session open */ - abstract public function read($keepOpen = false); + abstract public function read(); /** * Persists changes to the underlying session implementation - * - * @param bool $keepOpen True to keep the session open */ - abstract public function write($keepOpen = false); - - /** - * Close session - */ - abstract public function close(); + abstract public function write(); /** * Purge session diff --git a/library/Icinga/User.php b/library/Icinga/User.php index 9003e9f64..67e10e94f 100644 --- a/library/Icinga/User.php +++ b/library/Icinga/User.php @@ -31,7 +31,8 @@ namespace Icinga; use \DateTimeZone; use \InvalidArgumentException; -use Icinga\User\Preferences; +use \Icinga\User\Preferences; +use \Icinga\Authentication\PhpSession; /** * This class represents an authorized user @@ -105,6 +106,13 @@ class User */ private $preferences; + /** + * Queued notifications for this user. + * + * @var array() + */ + private $messages; + /** * Creates a user object given the provided information * @@ -334,4 +342,19 @@ class User } return new DateTimeZone($tz); } + + /** + * Send a message to this user that can be accessed in other requests. + * + * @param $msg + */ + public function sendMessage($msg) + { + $this->messages[] = $msg; + } + + public function getMessages() + { + return $this->messages; + } } diff --git a/library/Icinga/Web/Controller/ActionController.php b/library/Icinga/Web/Controller/ActionController.php index b7e2ee826..b153e7411 100755 --- a/library/Icinga/Web/Controller/ActionController.php +++ b/library/Icinga/Web/Controller/ActionController.php @@ -58,17 +58,6 @@ class ActionController extends Zend_Controller_Action */ protected $requiresAuthentication = true; - /** - * Set true when this controller modifies the session - * - * otherwise the session will be written back to disk and closed before the controller - * action is executed, leading to every modification in the session to be lost after - * the response is submitted - * - * @var bool - */ - protected $modifiesSession = false; - /** * The constructor starts benchmarking, loads the configuration and sets * other useful controller properties @@ -114,10 +103,7 @@ class ActionController extends Zend_Controller_Action return false; } - return !AuthManager::getInstance( - null, - array('writeSession' => $this->modifiesSession) - )->isAuthenticated(); + return !AuthManager::getInstance()->isAuthenticated(); } /** diff --git a/test/php/library/Icinga/Authentication/PhpSessionTest.php b/test/php/library/Icinga/Authentication/PhpSessionTest.php index 793293980..cf7e2fee9 100644 --- a/test/php/library/Icinga/Authentication/PhpSessionTest.php +++ b/test/php/library/Icinga/Authentication/PhpSessionTest.php @@ -49,67 +49,54 @@ class PhpSessionTest extends BaseTestCase { private function getSession() { - if (!is_writable('/tmp')) { $this->markTestSkipped('Could not write to session directory'); } return new PhpSession( array( 'use_cookies' => false, - 'save_path' => '/tmp' + 'save_path' => '/tmp', + 'test_session_name' => 'IcingawebUnittest' ) ); - } /** - * Test the creation of a PhpSession object - * - * @runInSeparateProcess - **/ + * Test the creation of a PhpSession object + * + * @runInSeparateProcess + */ public function testSessionCreation() { $this->getSession(); } /** - * Test PhpSession::open() + * Test PhpSession::open() * - * @runInSeparateProcess + * @runInSeparateProcess */ - public function testOpenSession() + public function testSessionReadWrite() { - $this->assertEquals(session_id(), '', 'Asserting test precondition: session not being setup yet '); $session = $this->getSession(); - $session->open(); - $this->assertNotEquals(session_id(), '', 'Asserting a Session ID being available after PhpSession::open()'); + $session->purge(); + $this->assertEquals(null, $session->get('key')); + $session->set('key', 'value'); + $session->write(); + $session->read(); + $this->assertEquals('value', $session->get('key')); } /** - * Test a session being closed by PhpSession::close() + * Test a session being closed by PhpSession::close() * - * @runInSeparateProcess - **/ - public function testCloseSession() - { - $this->assertEquals(session_id(), '', 'Asserting test precondition: session not being setup yet '); - $session = $this->getSession(); - $session->open(); - $this->assertNotEquals(session_id(), '', 'Asserting a Session ID being available after PhpSession::open()'); - $session->close(); - } - - /** - * Test if a session is correctly purged when calling PhpSession::purge() - * - * @runInSeparateProcess + * @runInSeparateProcess */ public function testPurgeSession() { - $this->assertEquals(session_id(), '', 'Asserting test precondition: session not being setup yet '); $session = $this->getSession(); - $session->open(); - $this->assertNotEquals(session_id(), '', 'Asserting a Session ID being available after PhpSession::open()'); + $session->set('key2', 'value2'); $session->purge(); - $this->assertEquals(session_id(), '', 'Asserting no Session ID being available after PhpSession::purge()'); + $session->read(); + $this->assertEquals(null, $session->get('key2')); } }