8000 [HttpFoundation component] Possible RedisSessionHandler and Session maxlifetime differents · Issue #34659 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpFoundation component] Possible RedisSessionHandler and Session maxlifetime differents #34659

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
rafaeltovar opened this issue Nov 27, 2019 · 4 comments
Closed

[HttpFoundation component] Possible RedisSessionHandler and Session maxlifetime differents #34659

rafaeltovar opened this issue Nov 27, 2019 · 4 comments
Labels
HttpFoundation

Comments

@rafaeltovar
Copy link
Contributor
rafaeltovar commented Nov 27, 2019 •
edited
Loading

Symfony version(s) affected: 4.4.0

Description

Option gc_maxlifetime doesn't work well, because RedisSessionHandler use a different variable for this purpose.

session.gc_maxlifetime will set on setOptions method of Session/Storage/NativeSessionStorage called on ___construct method, but Session/Storage/Handler/RedisSessionHandler set ttl property on his __construct, called before NativeSessionStorage::__construct.

How to reproduce

use Symfony\Component\HttpFoundation\Session\Storage\Handler\RedisSessionHandler;
use Symfony\Component\HttpFoundation\Session\Session;
use Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage;

ini_set('session.gc_maxlifetime',  60); // set session.gc_maxlifetime to 60

$handler = new RedisSessionHandler($predis); // $this->ttl = 60;

$storage = new NativeSessionStorage(['gc_maxlifetime' => 360], $handler); // set session.gc_maxlifetime = 360

$session = new Session($storage);
$session->start();
$session->set('uid', 1); // set expire on redis to 60

Possible Solution

// Symfony\Component\HttpFoundation\Session\Storage\Handler\RedisSessionHandler;
class RedisSessionHandler extends AbstractSessionHandler
{
    private $redis;
    /**
     * @var string Key prefix for shared environments
     */
    private $prefix;
    /**
     * @var int Time to live in seconds
     */
    private $ttl;
    /**
     * List of available options:
     *  * prefix: The prefix to use for the keys in order to avoid collision on the Redis server
     *  * ttl: The time to live in seconds.
     *
     * @param \Redis|\RedisArray|\RedisCluster|\Predis\ClientInterface|RedisProxy|RedisClusterProxy $redis
     *
     * @throws \InvalidArgumentException When unsupported client or options are passed
     */
    public function __construct($redis, array $options = [])
    {
       //...
        $this->redis = $redis;
        $this->prefix = $options['prefix'] ?? 'sf_s';
        // $this->ttl = $options['ttl'] ?? (int) ini_get('session.gc_maxlifetime');
        $this->ttl = $options['ttl'] ?? 0;
    }
    // ...
    /**
     * {@inheritdoc}
     */
    protected function doWrite($sessionId, $data): bool
    {
       $ttl = $this->ttl > 0?  $this->ttl : (int) ini_get('session.gc_maxlifetime');
        $result = $this->redis->setEx($this->prefix.$sessionId, $ttl, $data);
        return $result && !$result instanceof ErrorInterface;
    }
    // ...
}
@rafaeltovar rafaeltovar changed the title [HttpFoundation component] Possible RedisHandler and Session maxlifetime differents [HttpFoundation component] Possible RedisSessionHandler and Session maxlifetime differents Nov 27, 2019
@nicolas-grekas
Copy link
Member

I'm not sure I get the solution reading the code :)
Should we name the option gc_maxlifetime for Redis too, that's what you mean?
PR welcome if you want to give it a try.

@rafaeltovar
Copy link
Contributor Author

Check this commit:

symfony/http-foundation@a558b18#diff-e0b7ebc2540719ba74c5348a1f691f10

If we set session.gc_maxlifetime in the variable $this->ttl on Session/Storage/Handler/RedisSessionHandler::__construct, and after we initialize the Session/Storage/NativeSessionStorage with other session.gc_maxlifetime, this last value is not valid, because the time of expiration in RedisSessionHandler was setted before this.

do you understand what I try to mean?

@nicolas-grekas
Copy link
Member

Up for a PR?

@rafaeltovar
Copy link
Contributor Author

To be honest, I don't understand why the time for session expire and redis session storage expire have to be different. For me the best PR possible is revert this commit. Don't have sense on sessions if redis record expire is less than sess 718F ion maxlifetime, because if redis record expire, then session will be destroy (before session maxlifetime).

@rafaeltovar rafaeltovar mentioned this issue Dec 5, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
HttpFoundation
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@nicolas-grekas @rafaeltovar @xabbuh
0