Delete trac_auth_session cookie if the client sent it and rememberme is left unchecked.

If the request contains trac_auth_session cookie and the user is logging in with rememberme unchecked, expire it.

Hey, the 'cookie man' hits again. ;-)

I feel this is quite cumbersome to test, and even more to get it right. I'm glad you're seemingly after it with passion, so I'll take the chance to fix it with your help. Thanks for taking care.

As a side note: We're speaking about a different issue then what has been tracked in #9088, right?

+1 for making cookie handling more fail-proof. I've not come across, or at least not noticed such behavior, but I'll trust you, because you appear to be experienced/informed on these issues. By chance, do you know, if this is a generic issue, or rather happening only with some browser brand(s) and/or version(s)?

(In [10603]) AccountManagerPlugin: Play safe - expire left-over session cookies too, refs #9095.

This has been led by another observation of janakj, thank you very much. Added some code cleanup and seemingly missing code to restrict cookies. And denote another recently fixed issue in changelog too, refs #9093.

Replying to hasienda:

Hey, the 'cookie man' hits again. ;-)

Hehe, I'm a hit man who works for cookies ;-).

I feel this is quite cumbersome to test, and even more to get it right. I'm glad you're seemingly after it with passion, so I'll take the chance to fix it with your help. Thanks for taking care.

No problem. I have had issue with the "remember me" option on a shared computer. One of the users logged while leaving the "remember me" option unchecked and when I restarted the browser, he was still logged in.

I have been debugging that with all the patches as result. So my primary motivation is to get the cookies working properly with Trac 0.12 and then I will stop. I think most of the issues discovered might be related to the version 0.12.

As a side note: We're speaking about a different issue then what has been tracked in #9088, right?

Yes, it's a different issue. Both patches are needed.

+1 for making cookie handling more fail-proof. I've not come across, or at least not noticed such behavior, but I'll trust you, because you appear to be experienced/informed on these issues. By chance, do you know, if this is a generic issue, or rather happening only with some browser brand(s) and/or version(s)?

This is a Trac issue, not a browser issue. The browser does the right thing by resubmitting the trac_auth_session cookie because Trac fails to expire it. Here is what happens:

1) The user clicks on log out. 2) AccountManage? expires trac_auth_session 3) Trac expires trac_auth 4) Trac somehow/somewhere calls _get_name_for_cookie which extends the previously expired trac_auth_session cookie. 5) Trac sends back 302 which has expired trac_auth but extended trac_auth_session

Step 4) is the problem but I can't figure out from where _get_name_for_cookie gets called, I don't know Trac internals well enough. This problem is still there, by the way.

My latest patch fixes the issue by removing the old trac_auth_session cookie when a user logs in again, which I think is the right thing to do in any case. AccountManager should always play safe when the user is logging in and make sure that all the cookies have correct values, no matter what the browser submitted with the request.

It would still be nice to also expire the trac_auth_session cookie correctly when the user is logging out, but I haven't figured out how to fix that yet. Nevertheless, with the latest patch included it works correctly now.

Replying to janakj:

Replying to hasienda:

Hey, the 'cookie man' hits again. ;-)

Hehe, I'm a hit man who works for cookies ;-).

I feel this is quite cumbersome to test, and even more to get it right. I'm glad you're seemingly after it with passion, so I'll take the chance to fix it with your help. Thanks for taking care.

No problem. I have had issue with the "remember me" option on a shared computer. One of the users logged while leaving the "remember me" option unchecked and when I restarted the browser, he was still logged in. I have been debugging that with all the patches as result. So my primary motivation is to get the cookies working properly with Trac 0.12 and then I will stop. I think most of the issues discovered might be related to the version 0.12.

As a side note: We're speaking about a different issue then what has been tracked in #9088, right?

Yes, it's a different issue. Both patches are needed.

+1 for making cookie handling more fail-proof. I've not come across, or at least not noticed such behavior, but I'll trust you, because you appear to be experienced/informed on these issues. By chance, do you know, if this is a generic issue, or rather happening only with some browser brand(s) and/or version(s)?

This is a Trac issue, not a browser issue. The browser does the right thing by resubmitting the trac_auth_session cookie because Trac fails to expire it. Here is what happens: 1) The user clicks on log out. 2) AccountManage? expires trac_auth_session 3) Trac expires trac_auth 4) Trac somehow/somewhere calls _get_name_for_cookie which extends the previously expired trac_auth_session cookie. 5) Trac sends back 302 which has expired trac_auth but extended trac_auth_session Step 4) is the problem but I can't figure out from where _get_name_for_cookie gets called, I don't know Trac internals well enough. This problem is still there, by the way. My latest patch fixes the issue by removing the old trac_auth_session cookie when a user logs in again, which I think is the right thing to do in any case. AccountManager should always play safe when the user is logging in and make sure that all the cookies have correct values, no matter what the browser submitted with the request. It would still be nice to also expire the trac_auth_session cookie correctly when the user is logging out, but I haven't figured out how to fix that yet.

For the record, I found the issue. AccountManager._do_logout expires the trac_auth_session cookie and then calls LoginModule?._do_logout. That function attempts to get the value of req.auth which triggers the authentication process. Function _get_name_for_cookie gets called somewhere in the middle and that function extends the (previously expired) cookie.

I'll see if I can provide a patch to fix this.

Nevertheless, with the latest patch included it works correctly now.

Don't close tickets without comment and valid reason. Don't close tickets at all, if not involved with development here. Please.

Replying to hasienda:

Don't close tickets without comment and valid reason. Don't close tickets at all, if not involved with development here. Please.

So what is the suggested workflow here?

I mean I opened the ticket and submitted the patch, it has been committed into the repository. I then tested that the committed version works so it seemed natural that I should be the one closing the ticket.

-Jan

Replying to janakj:

For the record, I found the issue.

Great!

I'll see if I can provide a patch to fix this.

I see now, there is #9099, right? Thanks, will review and take care of it.

Replying to anonymous:

Replying to hasienda:

Don't close tickets without comment and valid reason. Don't close tickets at all, if not involved with development here. Please.

So what is the suggested workflow here?

Oh, I see. anonymous ticket status changes are bad in general, you see?

No need to hurry. I've introduced a growing changelog to keep an easy reference, and in most cases I don't like to close tickets after resolving the issue but after merging the fix into an official release as hinted/stated in the changelog.

Call it overcautious, but proper code should better be available to everyone, not just to developers following trunk. I have a number of duplicate tickets for already fixed, but not back-ported/released issues, just to back my thesis, if you care. It's a management decision, after I've already spent days on reading (mostly too short) commit messages and diff representations of changesets for this plugin to sort out, what had been done and what has to be done next.

Thank you anyway for contributing here, not mean to distract you in any form.

Replying to hasienda:

Replying to janakj:

For the record, I found the issue.

Great!

I'll see if I can provide a patch to fix this.

I see now, there is #9099, right? Thanks, will review and take care of it.

Yes, I submitted the patch as another ticket. Please have a look, with that last patch everything seems to work correctly now with the latest version of Trac and AccountManager.

Replying to anonymous:

Replying to hasienda:

Don't close tickets without comment and valid reason. Don't close tickets at all, if not involved with development here. Please.

So what is the suggested workflow here?

Oh, I see. anonymous ticket status changes are bad in general, you see?

Oh, I see, I'm sorry about that. I sometimes forget to re-login at track-hacks and the website does not remember my username.

No need to hurry. I've introduced a growing changelog to keep an easy reference, and in most cases I don't like to close tickets after resolving the issue but after merging the fix into an official release as hinted/stated in the changelog.

Make sense. In that case I'll just add a comment when I test the committed version.

Call it overcautious, but proper code should better be available to everyone, not just to developers following trunk. I have a number of duplicate tickets for already fixed, but not back-ported/released issues, just to back my thesis, if you care. It's a management decision, after I've already spent days on reading (mostly too short) commit messages and diff representations of changesets for this plugin to sort out, what had been done and what has to be done next. Thank you anyway for contributing here, not mean to distract you in any form.

No problem, thanks for the clarification!

-Jan

Replying to janakj:

[...] Make sense. In that case I'll just add a comment when I test the committed version.

Thanks, would be most appreciated, but no disaster, if done differently.

No problem, thanks for the clarification!

You're welcome. Glad I could make my points clear without disgusting you.

I really do enjoy your contributions. I'd value testing a dedicated 0.13 or 0.14 branch for adopting i.e. current (sane and less error prone) Trac db handling for this plugin. Maybe you'll be in again, when this happens as far as your still involved and as your time permits?

Replying to hasienda:

Replying to janakj:

[...] Make sense. In that case I'll just add a comment when I test the committed version.

Thanks, would be most appreciated, but no disaster, if done differently.

No problem, thanks for the clarification!

You're welcome. Glad I could make my points clear without disgusting you. I really do enjoy your contributions. I'd value testing a dedicated 0.13 or 0.14 branch for adopting i.e. current (sane and less error prone) Trac db handling for this plugin. Maybe you'll be in again, when this happens as far as your still involved and as your time permits?

I can't promise, but drop me a line at jan@ryngle.com when you have something you would like to have tested. Since I have some debugging experience now, I'll see if I can deploy a more experimental version to see what happens.

-Jan

(In [10606]) AccountManagerPlugin: Another (final?) fix to session cookie handling, refs #8927, #9088, #9095 and #9099.

Overriding the supplemental method _expire_cookie() looks saner than overriding (super method) trac.auth.LoginModule._do_logout() and still calling it later on as well.

Thanks to janakj for another non-trivial contribution.

(In [10618]) AccountManagerPlugin: Publish maintenance release 0.3.2, closes #9051, #9082, #9088, #9091, #9092, #9093, #9095, #9099, #9107, #9108 and #9109.

This is an update for current stable at 0.3.1 with a number of fixes for issues reported within the last weeks.

While they will go into acct_mgr-0.4 too, current code isn't ready for release yet and will introduce a number of backwards-incompatible changes. So don't hurry for acct_mgr-0.4 right now.

Just noticed what I'd call a bug in signatures.py and removed unreasonable dependency on identical absolute path for successful check. Looks like nobody else tried this by now, right? Hey folks!

(In [11086]) AccountManagerPlugin: Ensure sensible browser cookie lifetime setting, refs #8927, #9088, #9095, #9099 and #9547.

I think this is now the most intuitive setting of default cookie lifetime: auth_cookie_lifetime from section [trac] gets overwritten with AcctMgr default (30 d) as long as it's found equal to the Trac default (0).

Remember, that the AcctMgr feature still has to be switched on with the boolean option persistent_sessions, that defaults to False, if unset.