Skip to content
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.

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

[Request] Let hasPreviousSession take REMEMBERME cookie into account #15266

Closed
rvanlaak opened this issue Jul 13, 2015 · 1 comment
Closed

[Request] Let hasPreviousSession take REMEMBERME cookie into account #15266

rvanlaak opened this issue Jul 13, 2015 · 1 comment

Comments

@rvanlaak
Copy link
Contributor

Problem

The Request::hasPreviousSession check currently does not take the remember-me cookie into account. We show account buttons on our website, but only 5% of the users are logged in (eg. need a session). When users login and enable 'remember me', all is okay for that session, but in case of IS_AUTHENTICATED_REMEMBERED (eg. expired session) the proper buttons aren't shown.

{% if app.request.hasPreviousSession and app.user is not null and is_granted('ROLE_USER') %}
    <a id="adminLink" href="{{ url('admin') }}">...</a>
{% else %}
    <a id="loginLink" href="{{ url('login') }}">...</a>
{% endif %}

Solution

Can be reproduced by removing the PHPSESSID cookie via the developer tools. I think that hasPreviousSession should also take IS_AUTHENTICATED_REMEMBERED into account. Was able to solve this by also checking the cookies for the remember me:

// Symfony\Component\HttpFoundation\Request

public function hasPreviousSession()
{
    // the check for $this->session avoids malicious users trying to fake a session cookie with proper name
    return $this->hasSession() && ($this->cookies->has($this->session->getName()) || $this->cookies->has('REMEMBERME'));
}

As Request is not aware of the configured remember_me cookie name this fix is not the right one.

Discussion

In case not taking the remember_me cookie into account is the right behavior, I think the usage of hasPreviousSession in the following docs should be discussed:

{% if app.request.hasPreviousSession %}

then should be

{% if app.request.session.isStarted() %}

... what also solved the above use case.

@xabbuh
Copy link
Member

xabbuh commented Nov 13, 2018

I don't think this is a bug. A remember me cookie has nothing to do with a session and can not be taken into account in hasPreviousSession().

@xabbuh xabbuh closed this as completed Nov 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants