|Summary:||Creating a session cookie with a specific default domain.|
|Component:||Catalina||Assignee:||Tomcat Developers Mailing List <dev>|
diffs for Globals.java and Request.java
diffs using an context-param instead of a system property
diffs using a context attribute to set the cookie domain
cleaned up diffs, documentation
Description donn.aiken 2009-11-21 10:43:04 UTC
Created attachment 24580 [details] diffs for Globals.java and Request.java A request was made on the Tomcat email list to allow for the session cookie created to be created with a specific default domain in a fashion similar to the WebLogic server. This patch allows the default domain to be set. The user needs to set a specific System property upon startup of the container, and the session cookie will be created with the specified domain. The change includes modifications to Globals.java to specify the parameter, and Request.java to actually implement the setting of the created cookie. The diffs are attached.
Comment 1 Mark Thomas 2009-11-21 16:53:30 UTC
+1 to the idea but the implementation needs some tweaks. I'd suggest implementing this as a context attribute, in a similar manner to the httpOnly setting. It allows more control and I am generally -1 for doing config with a system proprty that could be done elsewhere.
Comment 2 donn.aiken 2009-11-21 18:11:21 UTC
Created attachment 24581 [details] diffs using an context-param instead of a system property Mark -- I think you're right. I initially did it the other way to be symmetrical with the other System properties regarding session. Hopefully these revised patches are more to your liking. Thanks, Donn
Comment 3 donn.aiken 2009-11-24 17:18:34 UTC
Created attachment 24607 [details] diffs using a context attribute to set the cookie domain Mark - I re-read your note and I think got the message this time. If you feel this should be done at a different level than Context, please let me know. Thanks, Donn
Comment 4 donn.aiken 2009-12-01 16:25:09 UTC
Hello - Is there anything else I can do to ask people to look this over for possible inclusion? I think the last set of diffs implement the change as requested. (SetCookieDomain3.diff is the filename for the attachment, id=24607) Thanks.
Comment 5 Mark Thomas 2009-12-01 16:42:58 UTC
The general approach is good. Some misc comments in no particular order: - Use 4 spaces rather than tabs - Remove the changes that just add/remove whitespace at the end of a line - Some methods are missing JavaDocs - Provide the spelling corrections as a separate patch (makes things easier to review) - The new attribute needs documenting. - Some thought needs to be given to how this will interact with the session cookie config that will come in the Tomcat 7.
Comment 6 donn.aiken 2009-12-01 18:37:27 UTC
(In reply to comment #5) > The general approach is good. > > Some misc comments in no particular order: > - Use 4 spaces rather than tabs > - Remove the changes that just add/remove whitespace at the end of a line > - Some methods are missing JavaDocs > - Provide the spelling corrections as a separate patch (makes things easier to > review) > - The new attribute needs documenting. > - Some thought needs to be given to how this will interact with the session > cookie config that will come in the Tomcat 7. Thank you very much for the feedback. I believe this set of diffs much closer to what what you're looking for. I have not looked at the cookie handing in TC 7 yet, however.
Comment 7 donn.aiken 2009-12-01 18:38:32 UTC
Created attachment 24656 [details] cleaned up diffs, documentation Thank you very much for the feedback. I believe this set of diffs much closer to what what you're looking for. I have not looked at the cookie handing in TC 7 yet, however.
Comment 8 Mark Thomas 2010-03-10 08:29:14 UTC
Comment 9 Mark Thomas 2010-03-10 12:56:12 UTC
I've applied a patch to Tomcat 7 based on your patch. I plan to make a couple of related changes (bug 48379) to Tomcat 7 and then apply a combined patch to Tomcat 6. I'm marking this issue as a duplicate of 48379 and will track further progress in bug 48376. *** This bug has been marked as a duplicate of bug 48379 ***