Re: Possible session key creation regression in 1.4
On Fri, May 18, 2012 at 1:38 PM, Aymeric Augustin
<aymeric.augustin.2003@...> wrote:
> Le 18 mai 2012 à 11:51, Tom Evans <tevans.uk@...> a écrit :
>
>> On Wed, May 16, 2012 at 4:38 PM, Aymeric Augustin
>> <aymeric.augustin@...> wrote:
>>> 2012/5/16 Tom Evans <tevans.uk@...>:
>>>> So, is the session key being available part of the API, or is relying
>>>> on the session key existing incorrect?
>>>
>>> Hi Tom,
>>>
>>> Accessing the session key before saving the session is incorrect.
>>>
>>
>> Accessing the session key before saving the session is incorrect, but
>> there is nothing in the session API to determine if a session is
>> saved.
>
> if session.session_key == None:
> ...
>
>> I don't see a good way to support 1.3 and 1.4 or in 1.3 to work around the bug that this fixes
>
> If I understand correctly, you're writing a pluggable app and you want to make it compatible with Django
1.3 and 1.4.
>
> You didn't know this bug existed in 1.3, so I don't understand your sudden urge to work around it in your
application. Just keep your current code as is and if your users complain, show them that it's a bug in
Django and tell them that they should upgrade.
That may be fine for open source code, it's not appropriate here. I
need to support my features in Django 1.2 and upwards. We have seen
strange things where the session id that we extract is not correct,
and this bug, which I was not aware of, explains that issue. Therefore
I do need to work around it, now that I am aware of it.
Saying to my users, "oh, and now I don't support the version of Django
you use any more, drop all your other projects and upgrade now" is not
acceptable.
>
>> without explicitly saving the session
>> object each time prior to accessing the session key, which is not a
>> particularly clever way of doing things. This API needs to be looked at.
>
> The following sounds perfectly simple and reasonable to me:
>
> if session.session_key == None:
> session.save()
>
> The if clause will match only under Django 1.4, since under 1.3 session_key isn't None when it should. So
you support automatically both versions.
Grrr - session_key should never be None. This should be part of the
explicit contract of the session API - if I'm looking at the
session_key, I want to know what is OR WILL BE the session id. If the
session needs to be saved in order to generate the key, it should be
saved by the backend.
Or to put it another way; my code is never interested in a None
session_key. A valid session never has a key of None. Therefore I now
have to put code in to explicitly ensure that I have a valid
session_key
The bug that was fixed was that accessing session_key before the
session was saved could potentially give you a different session_key
than the one which eventually ends up being used. The fix was to make
the session_key property return None when unsaved - changing the API -
rather than at that point determining what session_key should be used,
which would have kept the same API.
Due to the API changing, and Django not merging bug fixes back through
branches, I now know that I have an issue with 1.3 and below clients
that must be accounted for, and that I must ensure the session is
saved before accessing it, as this ensures in both 1.3 and 1.4 that
the session.session_key is the key for the users session.
This means that each and every place I access session_key in code that
must work in 1.3 and 1.4, I must put in place code like this:
# We cannot access session.session_key before the session is saved
# Django provides no way of determining if the session has been saved
# so we must always save it here. However, there is a race in the
# session creation that may cause it to fail on key collision.
while True:
try:
request.session.save()
except IntegrityError:
# In <=1.4, session.save() does not generate a new session key
# if it conflicts, therefore we must do it for Django.
if django.VERSION[0] < 2 and django.VERSION[1] < 5:
# There is no API to request a new session key without churning
# the session. Use internal API.
request.session._session_key = request.session._get_new_session_key()
continue
else:
break
session_identifier = SSOIdentifier(identifier=request.session.session_key)
Yeah, that API totally rocks. Totally better than determining what the
session_key is when someone wants to know what the session_key is and
we haven't saved the session yet.
>
> I'm sorry, but it's hard to read your criticism in a constructive way when the solution is obvious and
doesn't need any changes in Django's APIs.
>
> You raised several related points in your next email; could you create tickets on Trac so we can verify and
fix these issues?
>
> Thanks!
It was only one point. Ticket 18344.
Cheers
Tom
--
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers@...
To unsubscribe from this group, send email to django-developers+unsubscribe <at> googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
)
RSS Feed