Opened 4 years ago

Last modified 2 years ago

#6933 enhancement new

Allow custom session cookie names

Reported by: Adi Roiban Owned by: Vladislav Dmitrievich Turbanov
Priority: normal Milestone:
Component: web Keywords:
Cc: jknight Branch: branches/custom-session-name-6933-4
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl, rwall, adiroiban, glyph

Description

Right now twisted session cookie is hardcoded.

Users wanting to use a different cookie name would need to overwrite whole t.w.s.Request.getSession() method.

Maybe we can find an API to specify the session name.

Attachments (6)

6933-1.diff (2.4 KB) - added by Adi Roiban 4 years ago.
6933-2.diff (111.3 KB) - added by Adi Roiban 4 years ago.
6933-3.diff (7.0 KB) - added by Adi Roiban 4 years ago.
6933-4.diff (7.4 KB) - added by Adi Roiban 4 years ago.
6933-5.diff (7.5 KB) - added by Adi Roiban 3 years ago.
custom-cookie-prefix.patch (10.9 KB) - added by Vladislav Dmitrievich Turbanov 2 years ago.
Custom cookie prefixes in the Site objects.

Download all attachments as: .zip

Change History (36)

comment:1 Changed 4 years ago by DefaultCC Plugin

Cc: jknight added

comment:2 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

I have created an initial fix.

Web diff is here: https://github.com/chevah/twisted/compare/6933-session-cookie-name

Live diff file is here: https://github.com/chevah/twisted/compare/6933-session-cookie-name.diff

I have attached a patch.

Please let me know if documentation is enough.

Thanks!

Changed 4 years ago by Adi Roiban

Attachment: 6933-1.diff added

comment:3 Changed 4 years ago by hawkowl

Owner: set to hawkowl

comment:4 Changed 4 years ago by hawkowl

Keywords: review removed
Owner: changed from hawkowl to Adi Roiban

Hi adiroiban, thanks for the patch!

  1. Do you mean "CUSTOM_NAME" on line 454 of test_web.py?
  1. Could you please add documentation on how to use this feature? I think the best place would be in https://twistedmatrix.com/trac/browser/trunk/docs/projects/web/howto/using-twistedweb.rst#L419 . You may need to rebase your patch on github, since the Sphinx migration is very new.
  1. I think the topfiles should say "twisted.web.server.Request allows specifying a custom name for the session cookie" (missing 'the').

Please address these three points and resubmit.

Thanks again!

  • hawkowl

comment:5 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: changed from Adi Roiban to hawkowl

Many thanks for the review.

I have updated the documentation.

This depends on a #6954 for the documentation links.

I have create a new method in IRequest called getSessioCookieName. I find this API much better since external code can use this method to find exact name of the session cookie.

Not sure if we should keep sessionCookieBaseName. I think that we can make it private. Please let me know if you want it removed from the public interface.

I have updated the test.

I could not find any test for getSession method, so I added a few tests.

I have updated the topfile.

Thanks!

Changed 4 years ago by Adi Roiban

Attachment: 6933-2.diff added

comment:6 Changed 4 years ago by hawkowl

Owner: hawkowl deleted

comment:7 Changed 4 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Adi Roiban

The attached patch includes tons and tons of unrelated (bogus) changes. Please attach a patch including only the intended changes. Thanks.

comment:8 Changed 4 years ago by Jean-Paul Calderone

I have create a new method in IRequest called getSessioCookieName. I find this API much better since external code can use this method to find exact name of the session cookie.

I'd prefer we find a way to address this functionality without expanding the interface. IRequest is already massively overburdened by obscure, mostly useless functionality. 99% of web applications developed with Twisted (or perhaps 100% even) have gotten by for 15 years without needing to customize the session cookie name. Do we really need to expand IRequest now to support this use case?

Changed 4 years ago by Adi Roiban

Attachment: 6933-3.diff added

comment:9 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

Sorry for the bad diff. ... i did git diff ...trunk instead of git diff trunk... I have attached an updated version.

I have removed the method from the interface and only moved into the Request implementation.

I still find that Request.sitepath needs to be documented as I have no idea why and where it is used.

Please let me know if this diff is ok.

The live diff from github should alwasy be good.

Sorry for all this problem and for using git and github, but I find it very hard to manage local branches with SVN.

comment:10 Changed 4 years ago by Richard Wall

Owner: set to Richard Wall
Status: newassigned

comment:11 Changed 4 years ago by Richard Wall

Author: rwall
Branch: branches/custom-session-name-6933

(In [41774]) Branching to 'custom-session-name-6933'

comment:12 Changed 4 years ago by Richard Wall

Keywords: review removed
Owner: changed from Richard Wall to Adi Roiban
Status: assignednew

Thanks Adi,

This looks like a very useful feature. I can understand why you wouldn't necessarily want your users to see a TWISTED_SESSION cookie set by eg a corporate site.

I created a branch but had some problems applying your patch.

First it contained git style a / b prefixes ...but that's only a minor niggle

[richard@zorin custom-session-name-6933]$ patch  -p0 < ../../6933-3.diff
can't find file to patch at input line 5
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/docs/projects/web/howto/using-twistedweb.rst b/docs/projects/web/howto/using-twistedweb.rst
|index 0be5c2f..70c491e 100644
|--- a/docs/projects/web/howto/using-twistedweb.rst
|+++ b/docs/projects/web/howto/using-twistedweb.rst
--------------------------
File to patch: ^C

More importantly, the patch doesn't seem to apply cleanly anymore.

[richard@zorin custom-session-name-6933]$ patch  -p1 < ../../6933-3.diff
patching file docs/projects/web/howto/using-twistedweb.rst
patching file twisted/web/server.py
patching file twisted/web/test/test_web.py
Hunk #1 succeeded at 8 with fuzz 1 (offset 1 line).
Hunk #2 FAILED at 18.
Hunk #3 succeeded at 468 (offset 1 line).
1 out of 3 hunks FAILED -- saving rejects to file twisted/web/test/test_web.py.rej
patching file twisted/web/topfiles/6933.feature
[richard@zorin custom-session-name-6933]$ less twisted/web/test/test_web.py.rej
[richard@zorin custom-session-name-6933]$ cat twisted/web/test/test_web.py.rej
--- twisted/web/test/test_web.py
+++ twisted/web/test/test_web.py
@@ -18,7 +18,7 @@
 from twisted.web import server, resource
 from twisted.internet import task
 from twisted.web import iweb, http, error
-from twisted.python import log
+from twisted.python import components, log

 from twisted.web.test.requesthelper import DummyChannel, DummyRequest

Can you supply an updated patch?

My comments on the patch content are:

  1. docs/projects/web/howto/using-twistedweb.rst
    1. The new documentation is ok, but it doesn't really tell me how I can tell t.w.server.Site to use my Request subclass.
    2. I guess it involves setting server.Site.requestFactory -- can you expand the documentation to show a full example of a server that sets a custom session cookie name.
  2. twisted/web/server.py
    1. "getSessionCookieName" -- I question whether this new public method is really necessary.
      1. The problem as I see it is that it means users have to create a Request subclass in order to customise the session name.
      2. Instead why not add a new "sessionCookieBaseName" parameter to server.Site.init, which is saved as a private instance variable "server.Site._sessionCookieBaseName". That variable can then be used directly in "getSession".
      3. Or you might still use a *private* method for contructing the full session cookie name "_sessionCookieName". And that method can then be unit tested directly.
      4. Customising the session name can then be achieved by assigning a curried Request constructor to server.Site.requestFactory eg mysite.requestFactory = functools.partial(Request, sessionCookieBaseName='MYSITE_SESSION')
        1. It would be even better if server.Site accepted a "requestFactory" argument, but that's another ticket.
    2. Perhaps "sessionCookieBaseName" would be better named "sessionCookiePrefix"
    3. "A C{bytes} giving the base name " might be better documented as "@ivar sessionCookiePrefix: The prefix for the session cookie name; @type sessionCookiePrefix: L{bytes}"
  3. twisted/web/test/test_web.py
    1. The patch seems to include lots of test content that has already been merged to trunk
    2. test_getSessionCookieName
      1. This seems to be the relevant test
      2. But if you agree with my suggested approach above, this will hopefully be simplified.

Ok. So those are my thoughts. I'm not 100% sure about my suggested alternative approach so perhaps you could discuss it in #twisted-dev, #twisted-web or on the twisted-python mailing list.

Please submit a new patch against trunk containing only the relevant changes.

Ignore the branch I created, I didn't commit anything to it in the end.

Thanks again.

-RichardW.

comment:13 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

Sorry for the git style prefixes. I have updated my git repo configuration and future patches should be better.

I can understand why you wouldn't

necessarily want your users to see a TWISTED_SESSION cookie set by eg a corporate site.

Or you want to run multiple twisted sites, on the same host ... since cookies do not take port number into account.

If you want to hide Twisted, then you also need to change 'server_signature' from Request.

My comments on the patch content are:

  1. docs/projects/web/howto/using-twistedweb.rst
    1. The new documentation is ok, but it doesn't really tell me how I

can tell t.w.server.Site to use my Request subclass.

  1. I guess it involves setting server.Site.requestFactory -- can you

expand the documentation to show a full example of a server that sets a custom session cookie name.

I have expanded the documentation and fixed the link. I have submitted my patch to pydoctor... but it will take some time until Twisted will support anchors in api links.

  1. twisted/web/server.py
    1. "getSessionCookieName" -- I question whether this new public method

is really necessary.

  1. The problem as I see it is that it means users have to create a

Request subclass in order to customise the session name.

  1. Instead why not add a new "sessionCookieBaseName" parameter to server.Site.init, which is saved as a private instance

variable

"server.Site._sessionCookieBaseName". That variable can then be

used directly in "getSession".

  1. Or you might still use a *private* method for contructing the

full

session cookie name "_sessionCookieName". And that method can

then be

unit tested directly.

  1. Customising the session name can then be achieved by assigning a curried Request constructor to server.Site.requestFactory eg mysite.requestFactory = functools.partial(Request, sessionCookieBaseName='MYSITE_SESSION')

I went with current approach since 'requestFactory' is not passed as ini argument, either.

The whole changes/code are in the Request class and not Site class. This means that Request should rely on a new public method on Site class. I tried to reduce the dependency between the 2 objects.

Maybe is a think of taste, but for me using functools.partial looks like a smart/cute hack an is not better than subclassing request.

The configuration architecture/design issues was also raised in #6972

There is also ticket #6932 which tries to define custom behavior for HTTPOnly and Secure cookie flags used when creation session cookie... If we go on the suggestion road, we would have to add more arguments to Site.init. Also there is the 'server_signature' configuration.

So maybe we should start with passing requestFactory to init.

So maybe we need a new ticket which implements some generic configuration for an HTTP site - 'the right way TM'.

I will start a discussion on Twisted list.

  1. It would be even better if server.Site accepted a

"requestFactory"

argument, but that's another ticket.

I have create a new ticket for that #7016

  1. Perhaps "sessionCookieBaseName" would be better named

"sessionCookiePrefix"

I did not wanted to add prefix to not force future changes to place it at the start.

They way self.sitepath is used in cookie name, feels like a quick, undocumented hack and I hope that in the future this will be improved.

  1. "A C{bytes} giving the base name " might be better documented as

"@ivar sessionCookiePrefix: The prefix for the session cookie name; @type sessionCookiePrefix: L{bytes}"

I added the @type meta.

  1. twisted/web/test/test_web.py
    1. The patch seems to include lots of test content that has already

been merged to trunk

Sorry for that. I could not find any test for getSession method, so I added a few tests. This is why you got those *unrelated* tests.

If you want I can create a separate ticket/patch for that... but it is just more bureaucracy work and since this ticket is about session, I thought that it is easier to add the tests here.

  1. test_getSessionCookieName
    1. This seems to be the relevant test
    2. But if you agree with my suggested approach above, this will

hopefully be simplified.

I find the test simple enough :) ... but I do find that all those interdependencies between Site/Channel/Request complicates writing tests.

Ok. So those are my thoughts. I'm not 100% sure about my suggested alternative approach so perhaps you could discuss it in #twisted-dev, #twisted-web or on the twisted-python mailing list.

OK. No problem. I fully understand. Please see my comments. I will raise this over the mailing list.

Please submit a new patch against trunk containing only the relevant changes.

I have attached a new full patch merged with latest trunk and without git prefix.

Thanks!

Changed 4 years ago by Adi Roiban

Attachment: 6933-4.diff added

comment:14 Changed 3 years ago by Jean-Paul Calderone

This is a duplicate of #4122 (but I closed #4122 instead of this one because so much has happened here already).

comment:15 Changed 3 years ago by Glyph

In the future, please use the "preview" button when you make a trac comment, the markup on the previous comment is so garbled I can't really understand who is saying what :).

comment:16 Changed 3 years ago by hawkowl

Owner: set to hawkowl

comment:17 Changed 3 years ago by hawkowl

Author: rwallhawkowl, rwall
Branch: branches/custom-session-name-6933branches/custom-session-name-6933-2

(In [43333]) Branching to custom-session-name-6933-2.

comment:18 Changed 3 years ago by hawkowl

Keywords: review removed

Applied the patch -- the code seems to work, and it's better than it was before, so I'm happy with that.

However, one test failed due to a non-existing assert method of TestCase, and I fixed up the documentation to be a bit clearer -- so if I could get someone to check that, then I'm happy for a merge.

comment:19 Changed 3 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

comment:20 Changed 3 years ago by hawkowl

Oh, also, the tests fail on Python 3.

comment:21 Changed 3 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Adi Roiban

Thanks. A few more points:

  1. The docs include use of string literals. These should be bytes or unicode string literals, not native string literals (as appropriate in each place - a quick glance suggests they should all be bytes).
  2. The docs say "It can be changed by changing the default value of sessionCookieBaseName". The application developer can't change the default value for this attribute (that would be a change to Twisted). They can only assign a new value.
  3. The example imports twisted.internet.reactor but doesn't use it.
  4. initial_time doesn't follow the naming convention. Likewise for session_raw_cookie.
  5. The news fragment doesn't follow the documented convention (fails to use "now") and is missing a closing full stop.
  6. The test method docstring for test_getSessionCookieName also overuses "default".
  7. How does DummySession help with testing? The docstring should be more specific.
  8. Please define DummySession at module scope.
  9. DummySession may not be needed at all. The test methods could advance the clock explicitly instead.
  10. Some of the tests cover multiple different pieces of behavior and should be split into two or more tests.
  11. What part of this change is test_getSessionComponent exercising?

Thanks again.

comment:22 Changed 3 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted
  1. I have updated the docs to be explicit about bytes.
  1. I tried to improve the documentation. Please suggest a better phrase. Thanks!
  1. I have removed the reactor import from documentation.
  1. True. Should be fixed. Sorry about all that.
  1. Done.
  1. Done.
  1. True. Hope I have explained better what it does. I have also renamed it.
  1. Done. Since it is only used inside RequestTests I thought it a good idea to keep the encapsulation.
  1. I prefer to keep the TaskClockSession (was DummySession) so that I can use only public API in the test and don't touch any internal state.
  1. Can you please be more explicit here. Which tests should be split in two or more tests? I took another look but I could not identify them.
  1. It is not exercising any part of this change, but I could not find a test where getSession is called with a sessionInterface different than None. This is why I wrote it together with the other tests.

I have attached a new diff based on latest trunk. Thanks for the review!

Changed 3 years ago by Adi Roiban

Attachment: 6933-5.diff added

comment:23 Changed 3 years ago by Glyph

Author: hawkowl, rwallhawkowl, glyph, rwall
Branch: branches/custom-session-name-6933-2branches/custom-session-name-6933-3

(In [43943]) Branching to custom-session-name-6933-3.

comment:24 Changed 3 years ago by Glyph

Keywords: review removed
Owner: set to Adi Roiban

Hi adi. Thanks for the patch. This is an ugly area of twisted.web which definitely needs the maintenance.

Some feedback that needs dealing with:

  1. After applying the new patch to a branch and forcing builds, it looks like there are numerous test failures.
  2. It looks violate the (silly, but still policy ;-)) "semantic newlines" standard.

And some more subjective points:

  1. I would really prefer a public interface for this extension mechanism other than "subclass and set some attributes". I realize there are other tickets in flight to make more of these settable as parameters, but this is still proposing more subclassing and more subclassing in the docs. One way to make this more parameterized might be to have getSession take a parameter about which application's session to look up; if you pass in your own session store, perhaps it could have its own cookie name?
  2. Might not the sessions attribute be a more appropriate place to put this than on the Request or Site object?
  3. Method names that start with get and have no arguments are sort of a holdover from other languages. If this is going to be a method where it is right now, it could just be a property, since it takes no inputs, which will make it easier to replicate since you could just set an attribute to override it.

comment:25 Changed 3 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted
  1. Fixed the failure
  2. Fixed the newlines.

  1. I can not pass to getSession as it needs the cookie value... and to get the cookie I need the cookieName ... so we need a new method in order not to break the current API.
  2. I need the cookie name to get a session... I can not place it on the session as I don't have a session yet.
  3. Done.

I think that current method is fine as it is just one specific implementation of iweb.IRequest and we don't need to update any interface for it.

If you are not happy, maybe we can make the new names private and mark them as experimental and wait for more feedback.

I have simplified the code to only use a _sessionCookieName property. I would make this property private so that it is not accessed outside of the Request class... but since it is documented it is a public API.

Not sure how to handle this.

Please take a look at latest changes.

Sending latest changes to buildbot.

Thanks!

comment:26 Changed 3 years ago by Glyph

Keywords: review removed
Owner: set to Adi Roiban

Thanks for your continued work on this, adi. This one has had a truly epic number of reviews, so your patience is all that much more appreciated :-).

  1. There's a test failure on the python 3 builder; related to a repr() I think?
  2. We should not be documenting private attributes as a part of the public interface. Any time a leading underscore shows up in an identifier in a .rst file, that's a dealbreaker :). If it's important enough to have public narrative documentation, it's more than important enough to have a public interface.

To the subjective points though,

I can not pass to getSession as it needs the cookie value... and to get the cookie I need the cookieName ... so we need a new method in order not to break the current API.

I am not entirely sure what you mean by this, but reflecting on my comment here I agree it doesn't really make sense to make the existing getSession API customizable in this way. (We should probably re-visit the whole notion of sessions in twisted.web at some point in the future, though.)

I need the cookie name to get a session... I can not place it on the session as I don't have a session yet.

If you read my previous comment carefully, you'll see that I said "the sessions attribute". I realize in retrospect that this was really unclear. I was referring to the sessions attribute of the Site.

What I was suggesting then was that instead of doing this:

class RequestWithCustomCookie(Request):
    sessionCookieName = b'A_COOKIE_NAME'

site = Site(...)
site.requestFactory = RequestWithCustomCookie

which conflates the responsibility of generating Request objects (and therefore processing HTTP semantics themselves) with the responsibility of selecting an appropriate session name, instead we could do something like this:

site = Site(...)
from twisted.web.something import SessionStore
site.sessions = SessionStore(cookiePrefix=b'A_COOKIE_NAME')

but in the course of writing this explanation I realize that that's probably a bit too elaborate. We'd need a big new public SessionStore API, that would need to be documented, it would need to implement a full mapping-object proxy, and that's already far more complexity than we need for this.

Lots of logic and state associated with session maintenance already lives on the Site. Particularly the unfortunate sitepath attribute is already maintained there; that attribute is currently the only input into the cookie-name-generation process.

I should be clear when I call things "unfortunate", so just to be specific: sitepath is unfortunate because:

  1. it's part of Request's internal state, but it's maintained (set) by Site, so Request is in an incomplete state for a portion of its lifecycle, and this is additional coupling.
  2. its semantics are not clear
  3. it's not documented

So my goal in recommending such a design is to avoid more random coupling and interactions, and to provide as straightforward as possible a way for a user to provide this input. Ideally, such an interface would not involve the caller creating any "code" objects (classes, functions) since that involves a much broader contract than a "data" object (bytes, in this case). To say nothing of inheritance :-).

In that spirit, here's a simpler recommendation:

site = Site(resource, sessionCookiePrefix=b"A_COOKIE_NAME")`

Which... I can see rwall already recommended over a year ago :-). Oops.

In your response to that review (in the garbled comment that I found very hard to read, but in the course of this have gone back to puzzle out), you said:

I went with current approach since 'requestFactory' is not passed as __init__ argument, either.

Two wrongs don't make a right ;-).

It's too bad rwall didn't do a re-review, and it seems that both I and exarkun lost the plot a little bit on that particular recommendation. So hopefully I'm just clarifying his original intent, and I'm sorry to do it so long after the fact.

The current interface for customizing Site objects by mutating them after the fact or subclassing them is just bad, as it introduces tons of unnecessary additional coupling. As we make changes and maintain it, we should try to reduce this and make the interface more minimal and straightforward.

In terms of how to implement this: you will want sessionCookiePrefix to be a keyword-only argument (just pull it out of **kwargs, basically) so as to avoid messing with the existing argument order. Then, Request can simply ask the Site to generate the cookie name, which makes sense, since it's the Site which is generating the Session itself.

Thinking in terms of refactoring things so that sitepath could maybe be deprecated (it's state from Site which Request doesn't need except for this one method), Request.getSession could stop accessing sitepath directly, and instead do something like self.site._cookieNameForRequest(request). We might even make that public and customizable in the future so that the full cookie name might be specified. But I digress.

Sorry that was so long! I just wanted to give you a thorough idea of how you might address the 2nd feedback point at the top. Hopefully responding to it will be much easier than writing it or reading it. I think that a sessionCookiePrefix argument to Site should be relatively easy to adjust in the implementation, and easier and shorter to explain in the docs.

comment:27 Changed 3 years ago by Adi Roiban

OK. Thanks for your review.

It looks like the whole twisted.web cookie handling design is a mess up so maybe I should just wait for someone else to get into this problem so that we could have 2 usecases for custom cookies and then discuss a new design.

I needed this functionality 14 months ago and for my Twisted fork I already have a fix. So at this stage I really don't care about this ticket.

I just used it to exercise the review process and struggle to find a way to do reviews in Trac... which I find painful.

Changed 2 years ago by Vladislav Dmitrievich Turbanov

Attachment: custom-cookie-prefix.patch added

Custom cookie prefixes in the Site objects.

comment:28 Changed 2 years ago by Vladislav Dmitrievich Turbanov

Keywords: review added
Owner: Adi Roiban deleted

I've submitted a patch for proposed cookie prefixes inside the Site instances. Adopted the tests provided by adiroiban as I find them really useful.

comment:29 Changed 2 years ago by Adi Roiban

Author: hawkowl, glyph, rwallhawkowl, rwall, adiroiban, glyph
Branch: branches/custom-session-name-6933-3branches/custom-session-name-6933-4

(In [45139]) Branching to custom-session-name-6933-4.

comment:30 Changed 2 years ago by Adi Roiban

Keywords: review removed
Owner: set to Vladislav Dmitrievich Turbanov

Thanks for your contribution.

I have sent your patch to the full test suite but some builders are failing... I guess that you forgot about py2.6 support... but I also see pyflakes errors.

Please note that all new methods should be fully documented. Especially public methods like makeSessionCookieName. It should have a full documentation for sitepath argument and formal rtype epydoc markup.


Do we need that each makeSessionCookieName implementation to care about sitepath? Maybe we can just have makeSessionCookieName() as public method name ... or just make it a property.


For test_makeSessionCookieName the docstring is A custom prefix for session cookies.. From this docstring I don't understand what this test tries to do.


For @ivar sessionCookiePrefix make sure that lines are properly indented.

Do we want to have sessionCookiePrefix as a public member?


Please note that this is just a high level review.

Please consider updating the patch so that the test will pass.

Thanks!

Note: See TracTickets for help on using tickets.