Opened 18 years ago

Closed 16 years ago

#2548 closed enhancement (fixed)

Add set/get_expires methods and expires property to sessions

Reported by: Tsume Owned by: Chris Beaven
Component: Contrib apps Version:
Severity: normal Keywords: sessions
Cc: upadhyay@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Added methods

How to use:
set_expires(seconds)
time = get_expires()
expires = seconds
time = expires

Planned patches for arguments like "3 weeks" or "1 year" will be created at a later time.

Attachments (25)

sessions_set_get_expire.diff (2.2 KB ) - added by Tsume 18 years ago.
sessions_set_get_expire_v2.diff (2.1 KB ) - added by Tsume 18 years ago.
sessions_set_get_expire_v3.diff (2.1 KB ) - added by Tsume 18 years ago.
session_middleware.diff (2.1 KB ) - added by upadhyay@… 17 years ago.
the patch was too old, i have fixed it to apply on a week old checkout.
session_middleware.2.diff (2.5 KB ) - added by upadhyay@… 17 years ago.
previous patch broke things when SESSION_EXPIRE_AT_BROWSER_CLOSE is set to True
session_middleware.3.diff (2.3 KB ) - added by upadhyay@… 17 years ago.
In the last patch tere still is some problem, if session is modified after the cookie is set, django will revert the cookie to expire when browser closes. The only solution seems to be to put the expires_time itself in the session so it can get preserved.
session_middleware.4.diff (4.3 KB ) - added by upadhyay@… 17 years ago.
the api has been changed, and now there is support for clearing session on browser close.
session_middleware.5.diff (28.0 KB ) - added by upadhyay@… 17 years ago.
with docs.
session_middleware.6.diff (5.7 KB ) - added by upadhyay@… 17 years ago.
with docs, without newline munging.
session_middleware.7.diff (6.1 KB ) - added by upadhyay@… 17 years ago.
updated my entry in authors file.
session_middleware.8.diff (4.3 KB ) - added by upadhyay@… 17 years ago.
TILL_BROWSER_CLOSE was redundent. removed it.
session_middleware.9.diff (4.3 KB ) - added by upadhyay@… 17 years ago.
[Looks like I am not going to stop embaressing myself today.] Updated the docs too.
session_middleware.10.diff (4.1 KB ) - added by upadhyay@… 17 years ago.
crossing my fingers.
session_middleware.11.diff (5.9 KB ) - added by Chris Beaven 17 years ago.
session_middleware.12.diff (7.6 KB ) - added by Chris Beaven 17 years ago.
tests added
session_middleware.13.diff (7.9 KB ) - added by upadhyay@… 17 years ago.
I considered and rejected storing the number of seconds in session, as there would be nothing decrementing it. So if I set the session to expire in t seconds, and if I write/access the session more often than every t seconds, the session would never expire. Fixed it.
session_middleware.14.diff (10.0 KB ) - added by Chris Beaven 17 years ago.
Patch supporting both custom idle-time expiration and fixed date expiration.
session_middleware.15.diff (10.0 KB ) - added by Chris Beaven 17 years ago.
Updated for session backend changes
session_middleware.16.diff (10.2 KB ) - added by Chris Beaven 17 years ago.
updated docs and removed unnecessary import
session_middleware.17.diff (10.5 KB ) - added by Chris Beaven 17 years ago.
Adding "New in Django development version" markers to docs
session_middleware.18.diff (12.1 KB ) - added by Chris Beaven 17 years ago.
Updated to patch cleanly against [6980]
session_middleware.19.diff (12.2 KB ) - added by Chris Beaven 17 years ago.
Oh, the last patch also added a slight fix to the db session backend (but I missed a reference to .now() -- session times are now stored in UTC
session_middleware.20.diff (12.6 KB ) - added by Chris Beaven 17 years ago.
trivial doc change I missed in my recent merge
session_middleware.21.diff (13.8 KB ) - added by Chris Beaven 17 years ago.
Actually, the change to UTC is a bit backwards incompatible and probably not necessary.
session_middleware.22.diff (13.8 KB ) - added by Chris Beaven 17 years ago.
typo on a reference to the get_expiry_age method (renamed in a recent patch from get_max_age)

Download all attachments as: .zip

Change History (55)

by Tsume, 18 years ago

comment:1 by Tsume, 18 years ago

Fixed how to entry

How to use:

request.session.set_expires(seconds)

time = request.session,get_expires()

request.session.expires = seconds

time = request.session.expires

comment:2 by Chris Beaven, 18 years ago

From a quick look at the patch, couldn't the new timedelta lines both be just

... + datetime.timedelta(seconds=(request.session.expires))

instead of

... + datetime.timedelta(seconds=(request.session.expires_time or settings.SESSION_COOKIE_AGE))

by Tsume, 18 years ago

by Tsume, 18 years ago

comment:3 by anonymous, 18 years ago

Thank you SmileyChris. I looked back at my code and thought there was room for improvement.

Critiques on convention, spelling, naming, etc are welcomed. Thanks

comment:4 by Adrian Holovaty, 18 years ago

What's the use case for this -- is the SESSION_COOKIE_AGE not finely grained enough? If we're going to allow per-request setting of the session cookie age, should we also allow per-request setting of SESSION_COOKIE_NAME, SESSION_COOKIE_DOMAIN and SESSION_COOKIE_SECURE?

comment:5 by Tsume, 18 years ago

Adrian, I'm working on more patches. I think all cookie and session information should be adjustable by the programmer. Every other framework gives this flexability and the features are generally ones used very regularly. About the session expires, the feature is very useful in any type of application which is used daily. ex. web forum, daily applications used in business, and various others which I'm trying to use Django for right now. I've ran in to some problems, which is the reason for the expires patch.

Personally, I could implement those other features you listed in if you'd like. I like contributing to a very nice framework and community. I think the features would be a good idea.

Tsume

comment:6 by Malcolm Tredinnick, 18 years ago

I am assuming this is being done at the request level to allow per-user preference settings? Otherwise, I also need the use-case clarified (which hasn't been done so far).

To address one part of the last comment: All cookie and session information should definitely not be adjustable by the programmer on per-request basis. It will break things. You can already adjust all those values on a project-wide basis. And if you have multiple sites for the one project, it is per-site configurable (since they use different settings files). That being said, the patch as it stands looks reasonable, but the extra things Adrian mentioned probably don't work.

We can't have SESSION_COOKIE_NAME being configurable on a per-user basis, because then we would not know which cookie to parse for the session information. It has to be project-wide constant. Similarly, SESSION_COOKIE_DOMAIN shouldn't be configurable on a per-user basis because if you are really operating out of multiple domains and sub-domains and paths, everything is going to be wanting to work with the cookie and the apps outside the current user's *_DOMAIN setting are going to think the cookie isn't set and create it (and stomp on the existing one). You can't have multiple cookies with the same name and overlapping domains. I think SESSION_COOKIE_SECURE is going to have the same problems: it means that HTTP-based cookies and access to the site are a possibility and how will the session code know the difference between "doesn't have a session cookie yet" and "only has the session cookie available via HTTPS"? It can't look up the user's preferences, because it doesn't have that information.

I can see a case for per-user session timeouts, though. On sites where the security is somewhat left up to the user (how aggressively do you want the site to time you out?), per-user timeouts are needed. We can't do that at the moment. That's an actual use case. So I would be +1 on the patch as it currently stands.

I would be -1 on adding things like the ability to set "3 weeks". This is a developer API. They can do the maths for the possibilities they want to offer. Adding all that extra code into core for all the possible date types we might offer just to avoid a couple of multiplications increases the amount of code we have to maintain and answer questions about.

comment:7 by Tsume, 18 years ago

mtredinnick,

Okay, I can go with the explaination for now. I hope my expires patch should be included before next release. I'd hate having to patch all my apps before deploying them :)

Tsume

comment:8 by anonymous, 18 years ago

Summary: [PATCH] Add set/get_expires methods and expires propery to sessions[patch] Add set/get_expires methods and expires propery to sessions

comment:9 by MarcDM, 18 years ago

Summary: [patch] Add set/get_expires methods and expires propery to sessions[patch] Add set/get_expires methods and expires propery to sessions - use case

I second this patch. I'm looking into using Django now to replace a custom (mod_python) handler and Cheetah. Please look at this use-case that I also posted this on the Sessions documentation page :

what if SESSION_EXPIRE_AT_BROWSER_CLOSE == True but I want to have visitors check a box that says "Remember Me" how do I then convert their session to a "Long Life" session?

Or vice versa, if I set it to SESSION_EXPIRE_AT_BROWSER_CLOSE == False, how do I make some sessions terminate when the browser closes?

comment:10 by Simon G. <dev@…>, 18 years ago

Needs documentation: set
Patch needs improvement: set
Summary: [patch] Add set/get_expires methods and expires propery to sessions - use case[patch] Add set/get_expires methods and expires property to sessions
Triage Stage: UnreviewedDesign decision needed

comment:11 by jimmy@…, 18 years ago

I also want to add something to this subject.

After reading docs and watching code (because the names are not really obvious, because the session expire time is what's most important), I think the names 'SESSION_EXPIRE_AT_BROWSER_CLOSE' and 'SESSION_COOKIE_AGE' are not correctly chosen in Django. Because the session itself does not and cannot expire if the browser closes, it is the cookie that expires; also SESSION_COOKIE_AGE is not the age of the cookie, it can be, only if the cookie does not expire on browser close, but it is actually the session age, which has nothing to do with the cookie.

I propose 'SESSION_COOKIE_EXPIRE_AT_BROWSER_CLOSE' or 'COOKIE_EXPIRE_AT_BROWSER_CLOSE' and 'SESSION_AGE'.

I know it is a change with that is not backward compatible, but this will not confuse people and thus prevents problems in the future. Users should realize that sessions and cookies are 2 different things and the relation between them.

Maybe it's also possible to have both 'SESSION_AGE' and 'SESSION_COOKIE_AGE'. But then 'SESSION_EXPIRE_AT_BROWSER_CLOSE' will still confuse people. (I would fine it more streigthforward that my cookie expires at the moment my session expires than vice versa).

by upadhyay@…, 17 years ago

Attachment: session_middleware.diff added

the patch was too old, i have fixed it to apply on a week old checkout.

by upadhyay@…, 17 years ago

Attachment: session_middleware.2.diff added

previous patch broke things when SESSION_EXPIRE_AT_BROWSER_CLOSE is set to True

comment:12 by anonymous, 17 years ago

Needs tests: set
Patch needs improvement: unset
Triage Stage: Design decision neededAccepted

comment:13 by Simon G. <dev@…>, 17 years ago

Patch needs improvement: set
Triage Stage: AcceptedDesign decision needed

Please don't do anonymous triage, or change states without giving a reason.

comment:14 by Chris Beaven, 17 years ago

Triage Stage: Design decision neededAccepted

According to the Google group discussion, that was Jacob who set that

by upadhyay@…, 17 years ago

Attachment: session_middleware.3.diff added

In the last patch tere still is some problem, if session is modified after the cookie is set, django will revert the cookie to expire when browser closes. The only solution seems to be to put the expires_time itself in the session so it can get preserved.

by upadhyay@…, 17 years ago

Attachment: session_middleware.4.diff added

the api has been changed, and now there is support for clearing session on browser close.

comment:15 by upadhyay@…, 17 years ago

Cc: upadhyay@… added
Patch needs improvement: unset

Now it looks complete to me. Both "remember me" and slashdot like "public terminal" features is possible now.

Updated API: added set_life to SessionWrapper. It takes an integer, number of seconds for which the session should be valid. It can also take django.contrib.session.models.TILL_BROWSER_CLOSE, that will quell the session at the close of browser. Calling set_life will overwrite the global/default session expiry policy.

Somebody please review and test, and set "patch needs improvements" to false.

in reply to:  15 comment:16 by anonymous, 17 years ago

Patch needs improvement: set

Oops, I accidentally removed the "patch needs improvement" flag. Someone else should go through it and remove it if they feel fit.

by upadhyay@…, 17 years ago

Attachment: session_middleware.5.diff added

with docs.

by upadhyay@…, 17 years ago

Attachment: session_middleware.6.diff added

with docs, without newline munging.

by upadhyay@…, 17 years ago

Attachment: session_middleware.7.diff added

updated my entry in authors file.

by upadhyay@…, 17 years ago

Attachment: session_middleware.8.diff added

TILL_BROWSER_CLOSE was redundent. removed it.

by upadhyay@…, 17 years ago

Attachment: session_middleware.9.diff added

[Looks like I am not going to stop embaressing myself today.] Updated the docs too.

by upadhyay@…, 17 years ago

Attachment: session_middleware.10.diff added

crossing my fingers.

by Chris Beaven, 17 years ago

Attachment: session_middleware.11.diff added

comment:17 by Chris Beaven, 17 years ago

Needs documentation: unset
Patch needs improvement: unset

Tidied up the patch a bit more - it actually works now. Deviated from the previous patch by adding a few more methods to the session wrapper - hope it's not seen as a ticket hijack ;)

by Chris Beaven, 17 years ago

Attachment: session_middleware.12.diff added

tests added

comment:18 by Chris Beaven, 17 years ago

Needs tests: unset
Triage Stage: AcceptedReady for checkin

Now it has tests too.

by upadhyay@…, 17 years ago

Attachment: session_middleware.13.diff added

I considered and rejected storing the number of seconds in session, as there would be nothing decrementing it. So if I set the session to expire in t seconds, and if I write/access the session more often than every t seconds, the session would never expire. Fixed it.

comment:19 by Chris Beaven, 17 years ago

Triage Stage: Ready for checkinAccepted

I guess we had different ideas about what should be happening. I think it is useful to be able to specify a different "age" than default, whereas you are focussing on setting the session to a limited "life".

Perhaps both should be possible. I'd find setting age more useful - why would you want to insist a session expires, even if it is being used?

(rolling back stage while we discuss)

in reply to:  19 comment:20 by upadhyay@…, 17 years ago

Replying to SmileyChris:

I guess we had different ideas about what should be happening. I think it is useful to be able to specify a different "age" than default, whereas you are focussing on setting the session to a limited "life".

What is the meaning of a session age? If I set the session age to be one week, I think it should expire in one week. How will your patch accomplish it.The expiry date is being constantly pushed on each modification, defies the purpose of "age".

I am not sure the default django session expiry managed this any better. In Session.save() one should use get_or_create, and pass expiry_date as default.

comment:21 by Chris Beaven, 17 years ago

The default session expiry uses the same method I went for - as long as the session is accessed or written to, it won't expire.

Setting a *fixed* age (what I called "life") which you are doing is interesting and perhaps useful, but different than the global method which extends the session's life every time the session is used. My patch does not accomplish your behaviour, hence why I suggested a patch which provided the functionality of both may be useful.

Perhaps with a '_fixed_age' session variable it would be possible to achieve both behaviours without too much code overhead.

in reply to:  21 comment:22 by Amit Upadhyay <upadhyay@…>, 17 years ago

Replying to SmileyChris:

The default session expiry uses the same method I went for - as long as the session is accessed or written to, it won't expire.

I am not convinced, but understand the logic now. Its kind of session idle timeout or something. This behavior was not obvious to me till this discussion, are we sure other developers understand this? Was it a feature and not a bug?

Perhaps with a '_fixed_age' session variable it would be possible to achieve both behaviours without too much code overhead.

I will give it a shot, but this will require further API changes, may be extra optional parameter in set_session_age(seconds, absolute_life=True). My concern is it will add to confusion.

comment:23 by Chris Beaven, 17 years ago

You're right, perhaps it would just add confusion. I think we should take the discussion to the group to get confirmation on the existing behaviour being correct and which is the preferred behaviour.

by Chris Beaven, 17 years ago

Attachment: session_middleware.14.diff added

Patch supporting both custom idle-time expiration and fixed date expiration.

comment:24 by berto, 17 years ago

The last patch does not apply cleanly on the trunk, but that lead me to finding SESSION_SAVE_EVERY_REQUEST. I was looking to solve the idle timeout problem where a session expires if a user does not access a site in a certain amount of time.

When used with SESSION_COOKIE_AGE, I solved my problem. I have added this to my settings.py:

SESSION_SAVE_EVERY_REQUEST = True
SESSION_COOKIE_AGE = 1800 # 30 minutes

This patch seemed to cover more than just idle timeout by the end of the thread, but Django may be able to satisfy all use-cases without this patch.

in reply to:  24 comment:25 by berto, 17 years ago

Now that I think about it, while using SESSION_SAVE_EVERY_REQUEST achieves the same thing, but it's probably a lot more overhead since I think it's saving the actual session data and sending a cookie to the client. Not having to re-save the session after every request is probably a lot more efficient.

by Chris Beaven, 17 years ago

Attachment: session_middleware.15.diff added

Updated for session backend changes

in reply to:  24 comment:26 by Chris Beaven, 17 years ago

Replying to berto:

The last patch does not apply cleanly on the trunk, but that lead me to finding SESSION_SAVE_EVERY_REQUEST. I was looking to solve the idle timeout problem where a session expires if a user does not access a site in a certain amount of time.

It does now :)

This patch seemed to cover more than just idle timeout by the end of the thread, but Django may be able to satisfy all use-cases without this patch.

This patch doesn't really deal with idle timeout any more than sessions currently do. The solution you found is probably the best way to solve your problem.

What this patch does deal with is setting individual expiry for a session, rather than only providing a global setting. The main use that comes to mind for me (for which Django can not handle very well) is making "Keep me logged in" an option at the user level.

by Chris Beaven, 17 years ago

Attachment: session_middleware.16.diff added

updated docs and removed unnecessary import

comment:27 by Chris Beaven, 17 years ago

Triage Stage: AcceptedReady for checkin

The patch now handles both custom age (rolling, just like the current session) and custom fixed life (expire explicitly at a given date/time, like Amit wanted) so I'm promoting to checkin again.

by Chris Beaven, 17 years ago

Attachment: session_middleware.17.diff added

Adding "New in Django development version" markers to docs

by Chris Beaven, 17 years ago

Attachment: session_middleware.18.diff added

Updated to patch cleanly against [6980]

by Chris Beaven, 17 years ago

Attachment: session_middleware.19.diff added

Oh, the last patch also added a slight fix to the db session backend (but I missed a reference to .now() -- session times are now stored in UTC

by Chris Beaven, 17 years ago

Attachment: session_middleware.20.diff added

trivial doc change I missed in my recent merge

by Chris Beaven, 17 years ago

Attachment: session_middleware.21.diff added

Actually, the change to UTC is a bit backwards incompatible and probably not necessary.

by Chris Beaven, 17 years ago

Attachment: session_middleware.22.diff added

typo on a reference to the get_expiry_age method (renamed in a recent patch from get_max_age)

comment:28 by Chris Beaven, 17 years ago

Owner: changed from nobody to Chris Beaven

comment:29 by Pete Crosier, 17 years ago

Summary: [patch] Add set/get_expires methods and expires property to sessionsAdd set/get_expires methods and expires property to sessions

comment:30 by Alex Gaynor, 16 years ago

Resolution: fixed
Status: newclosed

Fixed in r7586.

Note: See TracTickets for help on using tickets.
Back to Top