Opened 9 years ago

Closed 7 years ago

#2548 closed enhancement (fixed)

Add set/get_expires methods and expires property to sessions

Reported by: Tsume Owned by: SmileyChris
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: UI/UX:

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 9 years ago.
sessions_set_get_expire_v2.diff (2.1 KB) - added by Tsume 9 years ago.
sessions_set_get_expire_v3.diff (2.1 KB) - added by Tsume 9 years ago.
session_middleware.diff (2.1 KB) - added by upadhyay@… 8 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@… 8 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@… 8 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@… 8 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@… 8 years ago.
with docs.
session_middleware.6.diff (5.7 KB) - added by upadhyay@… 8 years ago.
with docs, without newline munging.
session_middleware.7.diff (6.1 KB) - added by upadhyay@… 8 years ago.
updated my entry in authors file.
session_middleware.8.diff (4.3 KB) - added by upadhyay@… 8 years ago.
TILL_BROWSER_CLOSE was redundent. removed it.
session_middleware.9.diff (4.3 KB) - added by upadhyay@… 8 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@… 8 years ago.
crossing my fingers.
session_middleware.11.diff (5.9 KB) - added by SmileyChris 8 years ago.
session_middleware.12.diff (7.6 KB) - added by SmileyChris 8 years ago.
tests added
session_middleware.13.diff (7.9 KB) - added by upadhyay@… 8 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 SmileyChris 8 years ago.
Patch supporting both custom idle-time expiration and fixed date expiration.
session_middleware.15.diff (10.0 KB) - added by SmileyChris 8 years ago.
Updated for session backend changes
session_middleware.16.diff (10.2 KB) - added by SmileyChris 8 years ago.
updated docs and removed unnecessary import
session_middleware.17.diff (10.5 KB) - added by SmileyChris 8 years ago.
Adding "New in Django development version" markers to docs
session_middleware.18.diff (12.1 KB) - added by SmileyChris 8 years ago.
Updated to patch cleanly against [6980]
session_middleware.19.diff (12.2 KB) - added by SmileyChris 8 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 SmileyChris 8 years ago.
trivial doc change I missed in my recent merge
session_middleware.21.diff (13.8 KB) - added by SmileyChris 8 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 SmileyChris 8 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)

Changed 9 years ago by Tsume

comment:1 Changed 9 years ago by Tsume

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 Changed 9 years ago by SmileyChris

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))

Changed 9 years ago by Tsume

Changed 9 years ago by Tsume

comment:3 Changed 9 years ago by anonymous

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 Changed 9 years ago by adrian

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 Changed 9 years ago by Tsume

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 Changed 9 years ago by mtredinnick

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 Changed 9 years ago by Tsume

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 Changed 9 years ago by anonymous

  • Summary changed from [PATCH] Add set/get_expires methods and expires propery to sessions to [patch] Add set/get_expires methods and expires propery to sessions

comment:9 Changed 9 years ago by MarcDM

  • Summary changed from [patch] Add set/get_expires methods and expires propery to sessions to [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 Changed 8 years ago by Simon G. <dev@…>

  • Needs documentation set
  • Patch needs improvement set
  • Summary changed from [patch] Add set/get_expires methods and expires propery to sessions - use case to [patch] Add set/get_expires methods and expires property to sessions
  • Triage Stage changed from Unreviewed to Design decision needed

comment:11 Changed 8 years ago by jimmy@…

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).

Changed 8 years ago by upadhyay@…

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

Changed 8 years ago by upadhyay@…

previous patch broke things when SESSION_EXPIRE_AT_BROWSER_CLOSE is set to True

comment:12 Changed 8 years ago by anonymous

  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Design decision needed to Accepted

comment:13 Changed 8 years ago by Simon G. <dev@…>

  • Patch needs improvement set
  • Triage Stage changed from Accepted to Design decision needed

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

comment:14 Changed 8 years ago by SmileyChris

  • Triage Stage changed from Design decision needed to Accepted

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

Changed 8 years ago by upadhyay@…

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.

Changed 8 years ago by upadhyay@…

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

comment:15 follow-up: Changed 8 years ago by upadhyay@…

  • 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.

comment:16 in reply to: ↑ 15 Changed 8 years ago by anonymous

  • 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.

Changed 8 years ago by upadhyay@…

with docs.

Changed 8 years ago by upadhyay@…

with docs, without newline munging.

Changed 8 years ago by upadhyay@…

updated my entry in authors file.

Changed 8 years ago by upadhyay@…

TILL_BROWSER_CLOSE was redundent. removed it.

Changed 8 years ago by upadhyay@…

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

Changed 8 years ago by upadhyay@…

crossing my fingers.

Changed 8 years ago by SmileyChris

comment:17 Changed 8 years ago by SmileyChris

  • 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 ;)

Changed 8 years ago by SmileyChris

tests added

comment:18 Changed 8 years ago by SmileyChris

  • Needs tests unset
  • Triage Stage changed from Accepted to Ready for checkin

Now it has tests too.

Changed 8 years ago by upadhyay@…

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 follow-up: Changed 8 years ago by SmileyChris

  • Triage Stage changed from Ready for checkin to 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 in reply to: ↑ 19 Changed 8 years ago by upadhyay@…

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 follow-up: Changed 8 years ago by 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.

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 in reply to: ↑ 21 Changed 8 years ago by Amit Upadhyay <upadhyay@…>

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 Changed 8 years ago by SmileyChris

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.

Changed 8 years ago by SmileyChris

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

comment:24 follow-ups: Changed 8 years ago by 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.

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 in reply to: ↑ 24 Changed 8 years ago by berto

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.

Changed 8 years ago by SmileyChris

Updated for session backend changes

comment:26 in reply to: ↑ 24 Changed 8 years ago by SmileyChris

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.

Changed 8 years ago by SmileyChris

updated docs and removed unnecessary import

comment:27 Changed 8 years ago by SmileyChris

  • Triage Stage changed from Accepted to 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.

Changed 8 years ago by SmileyChris

Adding "New in Django development version" markers to docs

Changed 8 years ago by SmileyChris

Updated to patch cleanly against [6980]

Changed 8 years ago by SmileyChris

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

Changed 8 years ago by SmileyChris

trivial doc change I missed in my recent merge

Changed 8 years ago by SmileyChris

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

Changed 8 years ago by SmileyChris

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

comment:28 Changed 8 years ago by SmileyChris

  • Owner changed from nobody to SmileyChris

comment:29 Changed 7 years ago by PJCrosier

  • Summary changed from [patch] Add set/get_expires methods and expires property to sessions to Add set/get_expires methods and expires property to sessions

comment:30 Changed 7 years ago by Alex

  • Resolution set to fixed
  • Status changed from new to closed

Fixed in r7586.

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