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)
Change History (55)
by , 18 years ago
Attachment: | sessions_set_get_expire.diff added |
---|
comment:1 by , 18 years ago
comment:2 by , 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 , 18 years ago
Attachment: | sessions_set_get_expire_v2.diff added |
---|
by , 18 years ago
Attachment: | sessions_set_get_expire_v3.diff added |
---|
comment:3 by , 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 , 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 , 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 , 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 , 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 , 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 , 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 , 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: | Unreviewed → Design decision needed |
comment:11 by , 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 , 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 , 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 , 17 years ago
Needs tests: | set |
---|---|
Patch needs improvement: | unset |
Triage Stage: | Design decision needed → Accepted |
comment:13 by , 17 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Accepted → Design decision needed |
Please don't do anonymous triage, or change states without giving a reason.
comment:14 by , 17 years ago
Triage Stage: | Design decision needed → Accepted |
---|
According to the Google group discussion, that was Jacob who set that
by , 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 , 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.
follow-up: 16 comment:15 by , 17 years ago
Cc: | 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.
comment:16 by , 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 , 17 years ago
Attachment: | session_middleware.8.diff added |
---|
TILL_BROWSER_CLOSE was redundent. removed it.
by , 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 , 17 years ago
Attachment: | session_middleware.11.diff added |
---|
comment:17 by , 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 ;)
comment:18 by , 17 years ago
Needs tests: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
Now it has tests too.
by , 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.
follow-up: 20 comment:19 by , 17 years ago
Triage Stage: | Ready for checkin → Accepted |
---|
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)
comment:20 by , 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.
follow-up: 22 comment:21 by , 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.
comment:22 by , 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 , 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 , 17 years ago
Attachment: | session_middleware.14.diff added |
---|
Patch supporting both custom idle-time expiration and fixed date expiration.
follow-ups: 25 26 comment:24 by , 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.
comment:25 by , 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.
comment:26 by , 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 , 17 years ago
Attachment: | session_middleware.16.diff added |
---|
updated docs and removed unnecessary import
comment:27 by , 17 years ago
Triage Stage: | Accepted → Ready 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 , 17 years ago
Attachment: | session_middleware.17.diff added |
---|
Adding "New in Django development version" markers to docs
by , 17 years ago
Attachment: | session_middleware.18.diff added |
---|
Updated to patch cleanly against [6980]
by , 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 , 17 years ago
Attachment: | session_middleware.20.diff added |
---|
trivial doc change I missed in my recent merge
by , 17 years ago
Attachment: | session_middleware.21.diff added |
---|
Actually, the change to UTC is a bit backwards incompatible and probably not necessary.
by , 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 , 17 years ago
Owner: | changed from | to
---|
comment:29 by , 17 years ago
Summary: | [patch] Add set/get_expires methods and expires property to sessions → Add set/get_expires methods and expires property to sessions |
---|
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