[HttpFoundation] MongoDbSessionHandler::read() now checks for valid session age

[bzikarsky]

This PR is a follow-up to #12516 and replaces the old one.

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	no
Fixed tickets	-
License	MIT
Doc PR	TODO

As discussed there: Sessions which are older than GC age should never be read.
This PR adds the expiry-datetime on session-write and changes session-read and session-gc accordingly.

We still need to update the documentation with some clarifications, as described here:

• [HttpFoundation] MongoDbSessionHandler::read() now checks for valid session age #12516 (comment)
• [HttpFoundation] MongoDbSessionHandler::read() now checks for valid session age #12516 (comment)

My experience with the Symfony Docs from a developer perspective is very limited, so help would be very appreciated.

[fabpot]

Thank you @bzikarsky.

[Tobion]

Approach looks ok. But to me this seems to be a BC break. Old entries don't have the expiry field and thus GC doesn't work for them. Or am I missing something?

[bzikarsky]

You are right, currently old (pre-patch) sessions wouldn't even be cleaned up. No one thought of this yet. Oh well.

We can now fix:

a) only gc() to garbage-collect all sessions without expiry -- pre-patch session get timed out immediately, but we don't have a lot of "BC baggage"

b) gc() to garbage-collect sessions without expiry in the old way and change read() to assume all sessions without expiry to be valid (old sessions are treated exactly the same as before, which we considered buggy)

c) gc() as in b), but read() to validate sessions without expiry to the same rules as gc() did before the patch. Essentially valid session-age would be enforced on all new and old sessions. This solution has probably the highest overhead to ensure BC.

I'd go with a) personally. It's very lightweight and I think a framework upgrade allows for sessions to be invalidated.

What do you think? (/cc @jmikola)

[jmikola]

My understanding is that all three options would require $or logic in the gc() remove criteria. In (a), we would be removing all documents where expiry_field is out of range or it doesn't exist. I was initially concerned that the existence check wouldn't use an index, but I was reminded that it was only a temporary bug and has since been fixed (SERVER-10608).

For (b) and (c), which use the same gc() logic, we would still need $or logic. The clauses would be an expiry_field out of range or the time_field range check combined with expiry_field not existing. The second clause seems less performant, especially if users won't be indexing time_field. Based on the current code, time_field isn't event used, so I'm not sure why we're even still storing it (perhaps just for metadata, since _id is not an ObjectId that includes the creation timestamp). I'm not suggesting we remove time_field, but I don't think we should query on it.

That said, I'm in favor of (a). If we must use $or clauses in the remove criteria (effectively two queries), (a) at least relies on a single index we assume will exist. If we are concerned with sessions being invalidated during upgrades, it would be trivial to draft a script that, given a lifetime integer, can migrate documents in the sessions collection and create the necessary expiry_field values based on the time_field values. We could include that in release notes if we think folks would actually take notice and make use of it.