[2.1][HttpFoundation] Added some basic meta-data to Session #3718

Merged
merged 8 commits into from Apr 3, 2012

Projects

None yet

5 participants

@ghost
ghost commented Mar 28, 2012

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
References the following tickets: #2171
Todo: -

Session data is stored as an encoded string against a single id. If we want to store meta-data about the session, that data has to be stored as part of the session data to ensure the meta-data can persist using any session save handler.

This patch makes it much easier to determine the logic of session expiration. In general a session expiry can be dealt with by the gc handlers, however, in some applications more specific expiry rules might be required.

Session expiry may also be more complex than a simple, session was idle for x seconds. For example, in Zikula there are three security settings, Low, Medium and High. The rules for session expiry are more complex as under the Medium setting, a session will expire after x minutes idle time, unless the rememberme option was ticked on login. If so, the session will not idle. This gives the user some control over their experience. Under the high security setting, then there is no option, sessions will expire after the idle time is reached and login the UI has the rememberme checkbox removed.

The other advantage is that under this methodology, there can be a UI experience on expiry, like "Sorry, your session expired due to being idle for 10 minutes".

Keeping in the spirit of Symfony2 Components, I am seeking to make session handling flexible enough to accommodate these general requirements without specifically covering expiration rules. It would mean that it would be up to the implementing application to specifcally check and expire session after starting it.

Expiration might look something like this:

$session->start();
if (time() - $session->getMetadataBag()->getLastUpdate() > $maxIdleTime) {
    $session->invalidate();
    throw new SessionExpired();
}

This commit also brings the ability to change the cookie_lifetime when migrating a session. This means one could move from a default of browser only session cookie to long-lived cookie when changing from a anonymous to a logged in user for example.

$session->migrate($destroy, $lifetime);
@jalliot jalliot and 1 other commented on an outdated diff Mar 29, 2012
src/Symfony/Component/HttpFoundation/Session/MetaBag.php
+ /**
+ * @var array
+ */
+ protected $meta = array();
+
+ /**
+ * Unix timestamp.
+ *
+ * @var integer
+ */
+ private $lastUsed;
+
+ /**
+ * Constructor.
+ *
+ * @param string $storageKey The key used to store flashes in the session.
@jalliot
jalliot Mar 29, 2012 Contributor

typo here: flashes -> meta

@stloyd stloyd commented on the diff Mar 29, 2012
src/Symfony/Component/HttpFoundation/Session/Session.php
+ *
+ * @return \ArrayIterator An \ArrayIterator instance
+ */
+ public function getIterator()
+ {
+ return new \ArrayIterator($this->storage->getBag($this->attributeName)->all());
+ }
+
+ /**
+ * Returns the number of attributes.
+ *
+ * @return int The number of attributes
+ */
+ public function count()
+ {
+ return count($this->storage->getBag($this->attributeName)->all());
@stloyd
stloyd Mar 29, 2012 Contributor

Wouldn't be simpler to implement such count in that bag ?

@ghost
ghost Mar 29, 2012

This is not part of this PR - the method were just moved within the file.

@ghost
ghost commented Mar 30, 2012

@fabpot I have removed [WIP] status.

@Tobion Tobion and 1 other commented on an outdated diff Mar 30, 2012
src/Symfony/Component/HttpFoundation/Session/Session.php
@@ -189,6 +210,16 @@ public function setName($name)
}
/**
+ * Gets session meta.
+ *
+ * @return MetaBag
+ */
+ public function getMeta()
@Tobion
Tobion Mar 30, 2012 Member

I disagree with the naming.

  1. The word meta alone makes no sense here. I guess you mean metadata.
  2. The method is missing the Bag suffix to be in line with getFlashBag

So the method name should be getMetadataBag and the class must also be renamed accordingly.

@ghost
ghost Mar 31, 2012

To be honest, getFlashBag() is only named as such because it conflicts with a method kept for BC. While this is a bag, it's also not really a bag in the sense of being optional. I agree that maybe Metadata is more explicit but meta does actually mean the same thing, abstract information which is what meta-data is. I would feel more inclined to remove Bag suffix in all references to this particular bag and just call it Metadata getMetadata() etc.

@Tobion Tobion and 1 other commented on an outdated diff Mar 30, 2012
...Component/HttpFoundation/Session/SessionInterface.php
@@ -71,23 +71,30 @@ function setName($name);
* Clears all session attributes and flashes and regenerates the
* session and deletes the old session from persistence.
*
+ * @param integer $lifetime Sets the cookie lifetime for the session cookie. null will leave the
@Tobion Tobion and 1 other commented on an outdated diff Mar 30, 2012
...Component/HttpFoundation/Session/SessionInterface.php
/**
* Migrates the current session to a new session id while maintaining all
* session attributes.
*
- * @param Boolean $destroy Whether to delete the old session or leave it to garbage collection.
+ * @param Boolean $destroy Whether to delete the old session or leave it to garbage collection.
+ * @param integer $lifetime Sets the cookie lifetime for the session cookie. null will leave the
@Tobion Tobion and 1 other commented on an outdated diff Mar 30, 2012
...Component/HttpFoundation/Session/SessionInterface.php
@@ -164,4 +171,27 @@ function remove($name);
* @api
*/
function clear();
+
+ /**
+ * Registers a SessionBagInterface with the session.
+ *
+ * @param SessionBagInterface $bag
+ */
+ public function registerBag(SessionBagInterface $bag);
+
+ /**
+ * Get's a bag instance.
@Tobion Tobion commented on an outdated diff Mar 30, 2012
.../Component/HttpFoundation/Session/Storage/MetaBag.php
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\HttpFoundation\Session\Storage;
+
+use Symfony\Component\HttpFoundation\Session\SessionBagInterface;
+
+/**
+ * Metadata container.
+ *
+ * Adds standard meta data to the session.
@Tobion
Tobion Mar 30, 2012 Member

typo: metadata (multiple occurences!)
standard is not defined here. Either remove it or better explain what metadata is involved.

@Tobion Tobion commented on an outdated diff Mar 30, 2012
.../Component/HttpFoundation/Session/Storage/MetaBag.php
+ /**
+ * @var array
+ */
+ protected $meta = array();
+
+ /**
+ * Unix timestamp.
+ *
+ * @var integer
+ */
+ private $lastUsed;
+
+ /**
+ * Constructor.
+ *
+ * @param string $storageKey The key used to store bag in the session.
@Tobion
Tobion Mar 30, 2012 Member

usually there is no dot at the end of @param and @return (multiple occurences)
typo: the bag

@Tobion Tobion commented on an outdated diff Mar 30, 2012
.../Component/HttpFoundation/Session/Storage/MetaBag.php
+ * {@inheritdoc}
+ */
+ public function initialize(array &$meta)
+ {
+ $this->meta = &$meta;
+
+ if (isset($meta['created'])) {
+ $this->lastUsed = $this->meta['lastused'];
+ $this->meta['lastused'] = time();
+ } else {
+ $this->stampCreated();
+ }
+ }
+
+ /**
+ * Gets the lifetime that this cooke was set with.
@Tobion
Tobion Mar 30, 2012 Member

typo: cookie

@Tobion Tobion commented on the diff Mar 31, 2012
...ponent/HttpFoundation/Session/Storage/MetadataBag.php
+ */
+ protected $data = array();
+
+ /**
+ * Unix timestamp.
+ *
+ * @var integer
+ */
+ private $lastUsed;
+
+ /**
+ * Constructor.
+ *
+ * @param string $storageKey The key used to store bag in the session.
+ */
+ public function __construct($storageKey = '_sf2_meta')
@Tobion
Tobion Mar 31, 2012 Member

You missed _sf2_metadata

@ghost
ghost Mar 31, 2012

the keys here dont matter, it's purely internal to avoid name clashes - it just needs to be sufficiently obscure.

@Tobion
Tobion Mar 31, 2012 Member

I know. For God's sake, it's just for consistency.

@Tobion Tobion and 1 other commented on an outdated diff Mar 31, 2012
...ponent/HttpFoundation/Session/Storage/MetadataBag.php
+
+namespace Symfony\Component\HttpFoundation\Session\Storage;
+
+use Symfony\Component\HttpFoundation\Session\SessionBagInterface;
+
+/**
+ * Metadata container.
+ *
+ * Adds metadata to the session.
+ *
+ * @author Drak <drak@zikula.org>
+ */
+class MetadataBag implements SessionBagInterface
+{
+ const CREATED = 'c';
+ const LASTUSED = 'u';
@Tobion
Tobion Mar 31, 2012 Member

coding standards: LAST_USED
And I don't see the difference between $this->data[self::LASTUSED] and $this->lastUsed

@ghost
ghost Mar 31, 2012

They are different, the getter provides the last used time of the session, as the array key stores now, which will be the the last used of next time the session is loaded.

@Tobion
Tobion Mar 31, 2012 Member

I see. Would be nice to explain this in the Phpdoc as it helps to understand the difference.

Drak added some commits Mar 28, 2012
Drak [HttpFoundation] Added some basic meta-data to Session
This commit allows applications to know certain meta-data about the session
Session storage is designed to only store some data against a session ID
so this method is necessary to be compatible with any session handler, including
native handlers.
29bd787
Drak [HttpFoundation] Refactored for moved tests location. d9fd14f
Drak [HttpFoundation] Changed meta-data responsibility to
SessionStorageInterface

Added cookie_lifetime to the meta-data.  This allows to know how old
a cookie is and when the cookie will expire.
402254c
Drak [HttpFoundation] Add methods to interface ec3f88f
Drak [HttpFoundation] Add ability to force the lifetime (allows update of …
…session cookie expiry-time)
39141e8
Drak [HttpFoundation] Added the ability to change the session cookie lifet…
…ime on migrate().

This is a very important option which allows the cookie lifetime to be changed on migrate.
For example when a user converts from an anonymous session to a logged in session one might
wish to change from a persistent cookie to browser session (e.g. a banking application).
2f03b31
@ghost
ghost commented Mar 31, 2012

NB: This PR has been rebased and the tests relocated as per recent master changes.

@Tobion Tobion commented on the diff Mar 31, 2012
...pFoundation/Tests/Session/Storage/MetadataBagTest.php
+ protected function setUp()
+ {
+ $this->bag = new MetadataBag();
+ $this->array = array(MetadataBag::CREATED => 1234567, MetadataBag::UPDATED => 12345678, MetadataBag::LIFETIME => 0);
+ $this->bag->initialize($this->array);
+ }
+
+ protected function tearDown()
+ {
+ $this->array = array();
+ $this->bag = null;
+ }
+
+ public function testInitialize()
+ {
+ $p = new \ReflectionProperty('Symfony\Component\HttpFoundation\Session\Storage\MetadataBag', 'meta');
@Tobion
Tobion Mar 31, 2012 Member

This is not used anywhere. And meta property doesn't even exist.

@ghost
ghost Mar 31, 2012

$p is used a little down in lines 57 and 65. The property value change was missing when I renamed the class, it's been fixed now.

@ghost
ghost commented Apr 3, 2012

@fabpot - ping

@fabpot
Member
fabpot commented Apr 3, 2012

@drak: again, after I merge this PR, can you make sure that everything is documented properly in the documentation? Probably within a new cookbook article. Thanks.

@fabpot fabpot added a commit that referenced this pull request Apr 3, 2012
@fabpot fabpot merged branch drak/sessionmeta (PR #3718)
Commits
-------

8a0e6d2 [HttpFoundation] Update changelog.
4fc04fa [HttpFoundation] Renamed MetaBag to MetadataBag
2f03b31 [HttpFoundation] Added the ability to change the session cookie lifetime on migrate().
39141e8 [HttpFoundation] Add ability to force the lifetime (allows update of session cookie expiry-time)
ec3f88f [HttpFoundation] Add methods to interface
402254c [HttpFoundation] Changed meta-data responsibility to SessionStorageInterface
d9fd14f [HttpFoundation] Refactored for moved tests location.
29bd787 [HttpFoundation] Added some basic meta-data to Session

Discussion
----------

[2.1][HttpFoundation] Added some basic meta-data to Session

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
References the following tickets: #2171
Todo: -

Session data is stored as an encoded string against a single id.  If we want to store meta-data about the session, that data has to be stored as part of the session data to ensure the meta-data can persist using any session save handler.

This patch makes it much easier to determine the logic of session expiration.  In general a session expiry can be dealt with by the gc handlers, however, in some applications more specific expiry rules might be required.

Session expiry may also be more complex than a simple, session was idle for x seconds.  For example, in Zikula there are three security settings, Low, Medium and High.  The rules for session expiry are more complex as under the Medium setting, a session will expire after x minutes idle time, unless the rememberme option was ticked on login.  If so, the session will not idle.  This gives the user some control over their experience.  Under the high security setting, then there is no option, sessions will expire after the idle time is reached and login the UI has the rememberme checkbox removed.

The other advantage is that under this methodology, there can be a UI experience on expiry, like "Sorry, your session expired due to being idle for 10 minutes".

Keeping in the spirit of Symfony2 Components, I am seeking to make session handling flexible enough to accommodate these general requirements without specifically covering expiration rules. It would mean that it would be up to the implementing application to specifcally check and expire session after starting it.

Expiration might look something like this:

    $session->start();
    if (time() - $session->getMetadataBag()->getLastUpdate() > $maxIdleTime) {
        $session->invalidate();
        throw new SessionExpired();
    }

This commit also brings the ability to change the `cookie_lifetime` when migrating a session. This means one could move from a default of browser only session cookie to long-lived cookie when changing from a anonymous to a logged in user for example.

    $session->migrate($destroy, $lifetime);

---------------------------------------------------------------------------

by drak at 2012-03-30T18:18:43Z

@fabpot I have removed [WIP] status.

---------------------------------------------------------------------------

by drak at 2012-03-31T13:34:57Z

NB: This PR has been rebased and the tests relocated as per recent master changes.

---------------------------------------------------------------------------

by drak at 2012-04-03T02:16:43Z

@fabpot - ping
b9de0be
@fabpot fabpot merged commit 8a0e6d2 into symfony:master Apr 3, 2012
@stof
Member
stof commented Apr 3, 2012

@fabpot This is already documented. @weaverryan merged the doc PR a few hours ago and these changes were part of it

@ghost
ghost commented Apr 3, 2012

Yes, the PR was symfony/symfony-docs#1191

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment