Opened 10 years ago

Closed 2 years ago

#21514 closed Bug (fixed)

Session expiry dates should be in an ISO string instead of datetime

Reported by: n142857@… Owned by: nobody
Component: contrib.sessions Version: 1.6
Severity: Normal Keywords:
Cc: prasoon2211 Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The documentation for Django 1.6 says (https://docs.djangoproject.com/en/dev/topics/http/sessions/):

Note that datetime and timedelta values are only serializable if you are using the PickleSerializer.

And that's true. For instance, using django-social-auth I got this session data:

{'facebook_state': 'zyuqNJsGiXBPubpnwV7Lgh5pqbkKiqk5', 'social_auth_last_login_backend': u'facebook', '_auth_user_id': 4, '_auth_user_backend': 'social_auth.backends.facebook.FacebookBackend', '_session_expiry': datetime.datetime(2013, 11, 18, 9, 21, 20, 75255, tzinfo=<UTC>)}.

Which will give this error on login when using JSONSerializer:

datetime.datetime(2013, 11, 18, 9, 21, 20, 75255, tzinfo=<UTC>) is not JSON serializable

Django ships now with JSONSerializer enabled, but Django is still actively putting datetimes in sessions, concretely in django/contrib/sessions/backends/base.py:251. Since it's not a Django extension which is incompatible with the default behaviour, but Django itself, I think it's Django which must be fixed.

A solution is to serialize the datetime to ISO 8601 before storing it in the session.

Change History (7)

comment:1 by Tim Graham, 10 years ago

Where is the code that calls set_expiry() with a datetime? My guess is that it's somewhere in your own project or a third party module, not in Django itself. See also the documentation for set_expiry().

comment:2 by anonymous, 10 years ago

You're right. it's django-social-auth (views.py, function complete_process) who sets an expiry date. (For future readers who use Django 1.6 and cannot log in through Facebook: use SOCIAL_AUTH_SESSION_EXPIRATION=False).

But django-social-auth does the right thing in passing a datetime, because it cannot do otherwise: passing a string to set_expiry is not documented (it's also not as easy as passing a date). So I think set_expiry should accept datetime (as it does now) and convert to string when storing.
At least, issue a warning/error in set_expiry if SESSION_SERIALIZER=JsonSerializer.

comment:3 by Aymeric Augustin, 10 years ago

Triage Stage: UnreviewedAccepted

I don't think the current behavior is the best we can do :)

comment:4 by prasoon2211, 10 years ago

Okay, the dates can of course be stored in the ISO format. But, there's a problem.

s = Session.objects.get(
    session_key=self.session_key,
    expire_date__gt=timezone.now()
    )

There are code segments like this in other files under contrib/sessions/backends directory. Here, we are comparing between two datetime objects. If we store expiry date as a string, I don't see how will we be able to make such queries, at least now without using raw sql. If there's a solution to this, then we can fix this issue.

comment:5 by prasoon2211, 10 years ago

Cc: prasoon2211 added

comment:6 by Matt Riviere, 7 years ago

If serializing/unserializing is handled within set_expiry() and get_expiry_date/age(), the majority of user code should be unaffected. In particular, what prasoon2211 mentioned would not be a problem, because the db session adapter use get_expiry_date(), so it wouldn't even see the change. session['_expiry_date'] can already be either an int, a datetime, or None, so adding an option to let it be an ISO8601 string doesn't seem that bad.

The only thing that would break, is if someone is accessing session['_expiry_date'] directly (or parsing the stored session data from outside django entirely) and expects to find a datetime. Nothing within django does it. No idea how much outside code is doing it. It is probably possible to keep backwards compatibility on that by serializing to ISO8601 only for non-pickle serializer but that seems to complicate things a bit too much.

comment:7 by Mariusz Felisiak, 2 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top