added server side time_to_live of session data #8

Merged
merged 3 commits into from Jun 12, 2014

Projects

None yet

2 participants

@mkristian

added server side time_to_live of session data, when the session_data is expires the session gets reset.

first I thinking to implement it on application level, but I think the right place it the here (#7)

I know that invasive regarding adding a default timeout of 30 minutes but since the version is 0.0.4 I guess that is OK. security should start restrictive IMO.

@namelessjon
Collaborator

This seems to result in a session that will expire after thirty minutes, irrespective of activity in the session. Would it be better to have a 'rolling' timeout so you're logged out for inactivity, rather than just logged out?

@mkristian

yes, the was the intention. I guess I messed it up when I added the per session timeout. let me see the code . . .

@mkristian

the session actually lives on the client. for each request from the client the "timeout timestamp" wiil be set afresh inside the session (load_session method deletes the timestamp, commit_session sets it)). when the client side sits inactive for 30 minutes the next request to the server will reset the session since it timed out.

I guess that is what you meant with rolling timeout.

probably the naming is not clear enough in code + descriptions - at tleast the spec I adjusted to show this behaviour.

@namelessjon
Collaborator

Ah, missed the session.delete. Brain parsed it as session.fetch. Which doesn't even make sense, I know.

@mkristian

any chance to get this in. what can I do or improve it to see it accepted ? really would like use this feature in production on my side !

@namelessjon
Collaborator

Apologies, I forgot about this.

I like the functionality and I think it's useful enough to have it implemented at the middleware level. I don't like how long it makes the methods, with nested if's. Not that they were the nicest and neatest before this change, so maybe I'm being unreasonable.

@mkristian

it looks clearer now and easier to understand ;)

@namelessjon
Collaborator

Oh, that looks a lot clearer 👍

I'll merge when I get home this evening, probably.

@mkristian
@namelessjon namelessjon merged commit 2138c04 into cvonkleist:master Jun 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment