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

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(
+            $auth = AuthManager::getInstance();
-                'writeSession' => $this->modifiesSession
-            ));
 
             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(
+        $auth = AuthManager::getInstance();
-            'writeSession' => $this->modifiesSession
-        ));
         $auth->removeAuthorization();
         $this->redirectToLogin();
     }

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
      *

library/Icinga/Application/Web.php

@@ -232,12 +232,7 @@ class Web extends ApplicationBootstrap
      */
     private function setupUser()
     {
-        $authenticationManager = AuthenticationManager::getInstance(
+        $authenticationManager = AuthenticationManager::getInstance();
-            null,
-            array(
-                'writeSession' => true
-            )
-        );
 
         if ($authenticationManager->isAuthenticated() === true) {
             $user = $authenticationManager->getUser();

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();

library/Icinga/Authentication/PhpSession.php

@@ -51,21 +51,7 @@ class PhpSession extends Session
      *
      * @var string
      */
-    const SESSION_NAME = 'Icinga2Web';
+    private $sessionName = 'Icingaweb2';
-
-    /**
-     * Flag if session is open
-     *
-     * @var bool
-     */
-    private $isOpen = false;
-
-    /**
-     * Flag if session is flushed
-     *
-     * @var bool
-     */
-    private $isFlushed = false;
 
     /**
      * 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
+     * Open a PHP session
-     *
-     * @return bool
      */
-    private function sessionCanBeChanged()
+    private function open()
     {
-        if ($this->isFlushed) {
+        session_name($this->sessionName);
-            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_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;
+        session_write_close();
-    }
-
-    /**
-     * 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;
     }
 
     /**
      * 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()) {
+        $this->open();
-            return false;
-        }
         foreach ($this->getAll() as $key => $value) {
             $_SESSION[$key] = $value;
         }
-        if ($keepOpen) {
+        session_write_close();
-            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;
     }
 
     /**
@@ -238,12 +137,12 @@ class PhpSession extends Session
      */
     public function purge()
     {
-        if ($this->ensureOpen()) {
+        $this->open();
-            $_SESSION = array();
+        $_SESSION = array();
-            session_destroy();
+        $this->setAll(array(), true);
-            $this->clearCookies();
+        session_destroy();
-            $this->close();
+        $this->clearCookies();
-        }
+        session_write_close();
     }
 
     /**

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);
+    abstract public function write();
-
-    /**
-     * Close session
-     */
-    abstract public function close();
 
     /**
      * Purge session

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;
+    }
 }

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(
+        return !AuthManager::getInstance()->isAuthenticated();
-            null,
-            array('writeSession' => $this->modifiesSession)
-        )->isAuthenticated();
     }
 
     /**

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
+     * Test the creation of a PhpSession object
-    *
+     *
-    *   @runInSeparateProcess
+     * @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();
+        $session->purge();
-        $this->assertNotEquals(session_id(), '', 'Asserting a Session ID being available after PhpSession::open()');
+        $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
+     * @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
      */
     public function testPurgeSession()
     {
-        $this->assertEquals(session_id(), '', 'Asserting test precondition: session not being setup yet ');
         $session = $this->getSession();
-        $session->open();
+        $session->set('key2', 'value2');
-        $this->assertNotEquals(session_id(), '', 'Asserting a Session ID being available after PhpSession::open()');
         $session->purge();
-        $this->assertEquals(session_id(), '', 'Asserting no Session ID being available after PhpSession::purge()');
+        $session->read();
+        $this->assertEquals(null, $session->get('key2'));
     }
 }