Do not store user session if it has not already been initialized

[aschempp]

see #1288

[leofeyer]

I am pretty sure that the fix is wrong.

core-bundle/src/EventListener/UserSessionListener.php

Lines 79 to 85 in e4f0d29

	$user = $token->getUser();
	if (!$user instanceof User) {
	return;
	}
	$session = $user->session;

The user object is incomplete upon login, because it has not been created via User::loadUserByUsername() but via User::unserialize(). That's why $user->session is empty.

• My approach ensures that $user->session is set upon login.

• Your approach ensures that the empty session is not stored in the database.

So in your approach, the session will be empty for the login request! And we don't know if there are other listeners (e.g. storing the referer) that also rely on the session not being empty?

[aschempp]

The user object is incomplete upon login, because it has not been created via User::loadUserByUsername() but via User::unserialize().

This cannot be true. First of all, the login process obviously does not have a token in the session, otherwise a login would not be required. So there is nothing to be unserialized or incomplete. On subsequent requests the user is unserialized from the session, but the refreshUser method will fetch the database object again before the firewall is successful.

[leofeyer]

I have dumped the user object in line 86:

core-bundle/src/EventListener/UserSessionListener.php

Line 86 in e4f0d29

Array
(
    [id] => 1
    [tstamp] => 1514978172
    [username] => k.jones
    [name] => Kevin Jones
    [email] => k.jones@example.com
    [language] => de
    [backendTheme] => flexible
    [uploader] => FileUpload
    [showHelp] => 1
    [thumbnails] => 1
    [useRTE] => 1
    [useCE] => 1
    [password] => $2y$10$mASa6zAiFYfIWirQbLepx.q5B4t52o9KU8NwOIGBY6mfF6..emjbK
    [pwChange] => 
    [admin] => 1
    [groups] => 
    [inherit] => 
    [modules] => 
    [themes] => 
    [pagemounts] => Array
        (
        )

    [alpty] => 
    [filemounts] => Array
        (
            [0] => a:0:{}
        )

    [fop] => 
    [forms] => 
    [formp] => 
    [disable] => 
    [start] => 
    [stop] => 
    [session] => Array
        (
        )

    [dateAdded] => 1257428510
    [lastLogin] => 1515481605
    [currentLogin] => 1515481617
    [loginCount] => 3
    [locked] => 0
    [calendars] => 
    [calendarp] => 
    [calendarfeeds] => 
    [calendarfeedp] => 
    [news] => 
    [newp] => 
    [newsfeeds] => 
    [newsfeedp] => 
    [newsletters] => 
    [newsletterp] => 
    [faqs] => 
    [faqp] => 
    [imageSizes] => 
    [amg] => 
    [fullscreen] => 
)

You are right that the object is not incomplete (e.g. dateAdded is not serialized). But why is there no session data then?

[aschempp]

Maybe the session data is simply not unserialized?

[leofeyer]

The session data is never serialized so it cannot be unserialized, can it?

[aschempp]

It sure is serialized, tl_user.session is a database field?

[leofeyer]

I'm talking about this:

core-bundle/src/Resources/contao/library/Contao/User.php

Lines 642 to 645 in e4f0d29

	public function serialize()
	{
	return serialize(array($this->id, $this->username, $this->disable, $this->admin, $this->groups));
	}

[leofeyer]

I have added the changes (with unit test) in 5a721c6. Although it works, I don't like that the listener is now stateful. Is this ok for us?

[aschempp]

The only other option I can think of is to only register the kernel.response event if kernel.request is called. That's how the firewall works (e.g. ContextListener).

[leofeyer]

Brilliant. Changed in 82b22c9.