Em 31-05-2011 06:31, Christian Johansen escreveu:
On Fri, May 27, 2011 at 10:40, Christian Johansen <[email protected] <mailto:[email protected]>> wrote:But since you're asking so critically I have to admit I'm not entirely sure if the session check is entirely required. However, it did seem to be the thing that caused all the tests to fail? Maybe Marius has better memory than me here?The only case we could think of that uses the session without the user being logged in is in the case of flash messages. However, flash messages are only used in actions that are POST-ed to, in which case the user should be on https already. So I think we can remove that part. I will fix this method on master.Finally remember the actual reason why we have the session expiry check. The reason is caching. Rails is very eager when it comes to session cookies. Basically as soon as you touch the session object, Rails _will_ send a cookie. The cookies prevent us from utilizing our cache frontends properly. So in this case, merely checking that the user is logged in will cause a session cookie to be sent. I ended up with this:def using_session? !request.session_options[:expire_after].nil? end def ssl_allowed? request.ssl? end def ssl_required? GitoriousConfig["use_ssl"] && using_session? && logged_in? end Which at least reads clearer to me. What do you think?
Much clear undoubtedly! But still, if caching is a concern here, this should be stated at least as a comment there.
Anyway, there should be a test for this if this is important, since it seems to be broken. At least for me, on master, if I delete all my cookies and make a request to localhost:3000, I'll get a cookie from Gitorious.
But this won't solve the original problem anyway. I managed to fix most tests. The only missing tests at this moment are the SSL enforcement tests and one test for OpenID, which I didn't implement yet.
Regarding the SSL tests, the problem is not in ssl_required? method, since it is not even reached. It seems there is some before_filter (maybe from Devise or Warden) that is returning false before ssl_required? is checked. I'll need to investigate this.
But good work on your refactoring anyway! Best regards! -- To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected]
