Bug #9415: Fix storing IMAP object in session

Don't store IMAP object in session if login failed.

While rewriting this, converted IMP_Imap to Serializable interface,
which allows us to cache one more piece of data that shouldn't change
during the session (default namespace).  Also moves all recreation of
the object to the injector, which is cleaner.

diff --git a/imp/lib/Auth.php b/imp/lib/Auth.php
index d026960..c6f4fda 100644 (file)
--- a/imp/lib/Auth.php
+++ b/imp/lib/Auth.php
@@ -80,8 +80,7 @@ class IMP_Auth
             $credentials['server'] = self::getAutoLoginServer();
         }
 
-        $factory_imap = $injector->getInstance('IMP_Injector_Factory_Imap');
-        $imp_imap = $factory_imap->create($credentials['server']);
+        $imp_imap = $injector->getInstance('IMP_Injector_Factory_Imap')->create($credentials['server']);
 
         // Check for valid IMAP Client object.
         if (!$imp_imap->ob) {
@@ -351,7 +350,7 @@ class IMP_Auth
     {
         global $browser, $conf, $injector, $prefs, $registry, $session;
 
-        $imp_imap = $injector->getInstance('IMP_Injector_Factory_Imap')->create();
+        $imp_imap = $injector->getInstance('IMP_Injector_Factory_Imap')->create(null, true);
         $ptr = $imp_imap->loadServerConfig($session->get('imp', 'server_key'));
         if ($ptr === false) {
             throw new Horde_Auth_Exception('', Horde_Auth::REASON_FAILED);

diff --git a/imp/lib/Imap.php b/imp/lib/Imap.php
index cd028e0..de04f69 100644 (file)
--- a/imp/lib/Imap.php
+++ b/imp/lib/Imap.php
@@ -13,7 +13,7 @@
  * @license  http://www.fsf.org/copyleft/gpl.html GPL
  * @package  IMP
  */
-class IMP_Imap
+class IMP_Imap implements Serializable
 {
     /**
      * The Horde_Imap_Client object.
@@ -23,13 +23,6 @@ class IMP_Imap
     public $ob = null;
 
     /**
-     * Server key for this instance.
-     *
-     * @var string
-     */
-    protected $_serverkey = '';
-
-    /**
      * Is connection read-only?
      *
      * @var array
@@ -51,63 +44,6 @@ class IMP_Imap
     protected $_uidvalid = array();
 
     /**
-     * Constructor.
-     *
-     * @param string $serverkey  Server key for this instance.
-     *
-     * @throws IMP_Exception
-     */
-    public function __construct($serverkey)
-    {
-        $this->_serverkey = $serverkey;
-
-        /* Rebuild the Horde_Imap_Client object. */
-        if (!$this->_loadImapObject()) {
-            register_shutdown_function(array($this, 'shutdown'));
-        }
-    }
-
-    /**
-     * Save the Horde_Imap_Client object on session shutdown.
-     */
-    public function shutdown()
-    {
-        /* Only need to serialize object once a session. */
-        if ($this->ob) {
-            $GLOBALS['session']->set('imp', 'imap_ob/' . $this->_serverkey, $this->ob);
-        }
-    }
-
-    /**
-     * Loads the Horde_Imap_Client object from serialized session data.
-     *
-     * @return boolean  True on success, false on error.
-     * @throws IMP_Exception
-     */
-    protected function _loadImapObject()
-    {
-        global $session;
-
-        if (!is_null($this->ob)) {
-            return true;
-        }
-
-        try {
-            if (!($this->ob = $session->get('imp', 'imap_ob/' . $this->_serverkey))) {
-                return false;
-            }
-        } catch (Exception $e) {
-            /* Throw fatal error here - should never reach here and if we
-             * do, we are out of luck. */
-            throw new IMP_Exception(_("Could not acquire mail server credentials from the session."));
-        }
-
-        $this->_postcreate($session->get('imp', 'protocol'));
-
-        return true;
-    }
-
-    /**
      * Create a new Horde_Imap_Client object.
      *
      * @param string $username  The username to authenticate with.
@@ -159,7 +95,7 @@ class IMP_Imap
         }
 
         $this->ob = $ob;
-        $this->_postcreate($protocol);
+        $this->_postcreate();
 
         return $ob;
     }
@@ -195,16 +131,13 @@ class IMP_Imap
     }
 
     /**
-     * Alter some IMP settings once we load/create the object.
-     *
-     * @param string $protocol  The protocol used to connect.
+     * Alter some IMP/Horde settings once we load/create the object.
      */
-    protected function _postcreate($protocol)
+    protected function _postcreate()
     {
         global $conf, $prefs;
 
-        switch ($protocol) {
-        case 'pop':
+        if ($this->ob instanceof Horde_Imap_Client_Socket_Pop3) {
             /* Turn some options off if we are working with POP3. */
             $conf['user']['allow_folders'] = false;
             $prefs->setValue('save_sent_mail', false);
@@ -212,7 +145,6 @@ class IMP_Imap
             $prefs->setLocked('sent_mail_folder', true);
             $prefs->setLocked('drafts_folder', true);
             $prefs->setLocked('trash_folder', true);
-            break;
         }
     }
 
@@ -460,4 +392,28 @@ class IMP_Imap
         Horde::logMessage($e, 'ERR');
     }
 
+    /* Serializable methods. */
+
+    /**
+     */
+    public function serialize()
+    {
+        return serialize(array(
+            $this->ob,
+            $this->_nsdefault
+        ));
+    }
+
+    /**
+     */
+    public function unserialize($data)
+    {
+        list(
+            $this->ob,
+            $this->_nsdefault
+        ) = unserialize($data);
+
+        $this->_postcreate();
+    }
+
 }

diff --git a/imp/lib/Injector/Factory/Imap.php b/imp/lib/Injector/Factory/Imap.php
index 2543038..c5389d0 100644 (file)
--- a/imp/lib/Injector/Factory/Imap.php
+++ b/imp/lib/Injector/Factory/Imap.php
@@ -35,25 +35,56 @@ class IMP_Injector_Factory_Imap
     private $_instances = array();
 
     /**
+     * The list of instances to save.
+     *
+     * @var array
+     */
+    private $_save = array();
+
+    /**
      * Return the IMP_Imap:: instance.
      *
-     * @param string $id  The server ID.
+     * @param string $id     The server ID.
+     * @param boolean $save  Save the instance in the session?
      *
      * @return IMP_Imap  The singleton instance.
      * @throws IMP_Exception
      */
-    public function create($id = null)
+    public function create($id = null, $save = false)
     {
+        global $session;
+
         if (is_null($id) &&
-            !($id = $GLOBALS['session']->get('imp', 'server_key'))) {
+            !($id = $session->get('imp', 'server_key'))) {
             $id = 'default';
         }
 
         if (!isset($this->_instances[$id])) {
-            $this->_instances[$id] = new IMP_Imap($id);
+            if (!($ob = $session->get('imp', 'imap_ob/' . $id))) {
+                $ob = new IMP_Imap();
+            }
+
+            $this->_instances[$id] = $ob;
+        }
+
+        if ($save && !$session->exists('imp', 'imap_ob/' . $id)) {
+            if (empty($this->_save)) {
+                register_shutdown_function(array($this, 'shutdown'));
+            }
+            $this->_save[] = $id;
         }
 
         return $this->_instances[$id];
     }
 
+    /**
+     * Saves IMP_Imap instances to the session.
+     */
+    public function shutdown()
+    {
+        foreach (array_unique($this->_save) as $id) {
+            $GLOBALS['session']->set('imp', 'imap_ob/' . $id, $this->_instances[$id]);
+        }
+    }
+
 }