[OPF#1314] Log out users with empty session activity time when session lifetime is ... #244
Conversation
|
I started a discussion about logging out users when enabling session lifetime here: And changelog entry missing: https://www.openproject.org/projects/openproject/wiki/Code_Review_Guidelines 😃 |
ghost
assigned 
|
|
||
| flash[:warning] = I18n.t('notice_forced_logout', :ttl_time => Setting.session_ttl) | ||
| redirect_to(:controller => "account", :action => "login", :back_url => url) | ||
| if Setting.session_ttl_enabled? && Setting.session_ttl.to_i >= 5 && (session[:updated_at].nil? || session_expired?) |
There was a problem hiding this comment.
I like the session_expired? method :)
Why don't you put the check session[:updated_at].nil? into the session_expired? method?
I'd expect a method with that name to do such a check, actually it does the opposite (session[:updated_at] && ...)
There was a problem hiding this comment.
I'd also put Setting.session_ttl.to_i >= 5 there, and probably also Setting.session_ttl_enabled?, so a session never expires when session expiry is disabled.
There was a problem hiding this comment.
Yes, that's an alternative. Moving these conditions into the session_expired? method means "if session expiry is turned off, sessions never expire". Now it reads more like "if session expiry is turned off, we don't check for session expiry".
There was a problem hiding this comment.
I'm more looking at what one would expect from session_expired? when looking at the method alone.
To correctly read as your definition, you'd at least have to put the session[:updated_at].nil? and Setting.session_ttl.to_i >= 5 into session_expired?. Otherwise a session can expire(according to the method) despite a too low TTL that should not result in expiring sessions or a session doesn't expire despite not having an updated_at value, which it always should.
Generally, I'd also expect a method checking for session expiry to return false when session expiry is disabled, since a session is never expired then. But if you have another opinion here, I can live with that.
|
I noticed a bug: When logging out, you can never use the application as an Anonymous user again, probably since the anonymous session doesn't have the updated_at attribute. This also means I can't login, since every attempt to login is catched by the before_filter and I'm "logged out" again, meaning the session is reset and the updated_at attribute is removed. We might need to skip session lifetime checking for the AnonymousUser to fix this. Also have a look at what happens after logging in, here the session is also reset. This happens every time self.logged_user is set. This might also remove the update_at attribute and log out the user on the next request, but I'm not sure about that. I'm done with the review now 😉 |
|
Damn, not true. The changelog entry is still missing. |
|
So now I can access the page as anonymous user, but I still can't login. When I try, I'm immediately logged out again with the session expired flash message. |
|
Fixed the login bug and added cukes. |
…dministrator turns on session expiration.
|
Thanks for the changes! Internal CI says this works :) |
[#1314] Log out users with empty session activity time when session lifetime is ...
...enabled.
https://www.openproject.org/issues/1314