Commit 17346560 authored by Kazuhiko Shiozaki's avatar Kazuhiko Shiozaki

Zope4: response.cookies format is different in Zope >= 4.8.

parent d603304e
Pipeline #24653 failed with stage
in 0 seconds
...@@ -464,12 +464,13 @@ class TestOAuth2(ERP5TypeTestCase): ...@@ -464,12 +464,13 @@ class TestOAuth2(ERP5TypeTestCase):
)) ))
if response.body: if response.body:
response.headers.setdefault('content-type', 'text/html; charset=utf-8') response.headers.setdefault('content-type', 'text/html; charset=utf-8')
# response.cookies can contain 'max_age': 0 in Zope < 4.8 but 'Max-Age': '0' in Zope >= 4.8.
return ( return (
response.status, response.status,
response_header_dict, response_header_dict,
{ {
name: ( name: (
cookie_dict if cookie_dict.get('max_age', True) else cookie_dict if cookie_dict.get('max_age', cookie_dict.get('Max-Age')) not in (0, '0') else
None None
) for name, cookie_dict in six.iteritems(cookie_dict) ) for name, cookie_dict in six.iteritems(cookie_dict)
}, },
......
  • mentioned in commit e16c35c5

    Toggle commit list
  • Isn't cookie_dict.get('max_age', True) or cookie_dict.get('Max-Age') != '0' simpler ?

    This avoids relying on what the older value was, keeping the condition unchanged.

    This also clearly expresses what the new case is.

    And it prevents mixing up both cases.

  • One thing I don't understand is that in https://lab.nexedi.com/nexedi/erp5/blob/173465601b8167295d39426342da6034ba8dd1e8/bt5/erp5_oauth2_authorisation/TestTemplateItem/portal_components/test.erp5.testOAuth2Server.py#L437-440 we have:

                  cookie_attribute_name = cookie_attribute_name.lower()
                  if cookie_attribute_name == 'max-age':
                    cookie_attribute_name = 'max_age'
                    cookie_attribute_value = int(cookie_attribute_value, 10)

    this looks like we don't need to "normalize" here because it should already have been done there.

    above, we do

          cookie_dict = response.cookies.copy()          # <-- get "cookies from responses" (what are they exactly ?)
          response_header_dict = {}
          for item_list in (
            six.iteritems(response.headers),
            response.accumulated_headers,
          ):
            for key, value in item_list:
              key = key.lower()
              if key == 'set-cookie':               # <-- handle the "set-cookie" header responsible for setting new cookies (and deleting cookies)

    so isn't the problem that we don't include the cookie to be deleted in the set-cookie header ?

  • this looks like we don't need to "normalize" here because it should already have been done there.

    I did not check the context of this change. You are kind of right, at least to the extent that some normalisation happens.

    What this actually means is that cookie_dict needs to also be normalised instead of the after-the-fact approach implemented here.

    cookie_dict = response.cookies.copy() # <-- get "cookies from responses" (what are they exactly ?)

    HTTPResponse has no less than 3 ways of sending cookies to the client:

    • the cookies attribute
    • headers could contain a Set-Cookie item
    • the accumulated_headers attribute, which receives more response headers, and possibly multiple of each kind. This is the least pythonic approach, but useful when interfacing a library which does not know about HTTPResponse and just produces a list of response headers

    The normalisation code you cite handles the last 2 ways. This means the first way should probably also be normalised, if this is actually where those new-style cookies are coming from.

    Edited by Vincent Pelletier
  • Isn't cookie_dict.get('max_age', True) or cookie_dict.get('Max-Age') != '0' simpler ?

    This is what I tried first and did not work because bool(0) is False.

    >>> cookie_dict = {'max_age': 0} # Zope < 4.8 case
    >>> cookie_dict.get('max_age', True) or cookie_dict.get('Max-Age') != '0'
    True
    >>> cookie_dict.get('max_age', cookie_dict.get('Max-Age')) not in (0, '0')
    False
  • HTTPResponse has no less than 3 ways of sending cookies to the client:

    Maybe we can use one of the followings ? (I checked only Zope 4.8 code)

    (Pdb) pp response._cookie_list()
    [('Set-Cookie',
      '__ac=deleted; Path=/; Expires=Wed, 31 Dec 1997 23:59:59 GMT; Max-Age=0'),
     ('Set-Cookie',
      '__Site-test_rt=deleted; Path=/; Expires=Wed, 31-Dec-97 23:59:59 GMT; Secure; Max-Age=0'),
     ('Set-Cookie',
      '__Site-test_at=deleted; Path=/; Expires=Wed, 31-Dec-97 23:59:59 GMT; Secure; Max-Age=0')]
    
    (Pdb) pp response.listHeaders()
    [('X-Powered-By', 'Zope (www.zope.dev), Python (www.python.org)'),
     ('Content-Length', '40'),
     ('Content-Type', 'text/plain; charset=utf-8'),
     ('Location', 'http://10.0.129.248:2201/erp5/logged_out'),
     ('Cache-Control', 'private'),
     ('Set-Cookie',
      '__ac=deleted; Path=/; Expires=Wed, 31 Dec 1997 23:59:59 GMT; Max-Age=0'),
     ('Set-Cookie',
      '__Site-test_rt=deleted; Path=/; Expires=Wed, 31-Dec-97 23:59:59 GMT; Secure; Max-Age=0'),
     ('Set-Cookie',
      '__Site-test_at=deleted; Path=/; Expires=Wed, 31-Dec-97 23:59:59 GMT; Secure; Max-Age=0')]
  • Thanks @vpelletier and @kazuhiko , this makes sense, response.setCookie modifies response.cookies and it since Zope 4.8 it uses convertCookieParameter which normalize to 'Max-Age'. In Zope 2 there was no normalization, so it's now necessary to have the cookies from responses.cookies normalized as well. Doing the normalization in response.listHeaders() seems good, and it seems to work also on Zope 2, isn't it ?

    Edited by Jérome Perrin
  • This is what I tried first and did not work because bool(0) is False.

    I know that. I meant and not or:

    >>> cookie_dict = {'max_age': 0}
    >>> cookie_dict.get('max_age', True) and cookie_dict.get('Max-Age') != '0'
    0
    >>> cookie_dict = {'Max-Age': '0'}
    >>> cookie_dict.get('max_age', cookie_dict.get('Max-Age')) not in (0, '0')
    False
    >>> cookie_dict = {}
    >>> cookie_dict.get('max_age', cookie_dict.get('Max-Age')) not in (0, '0')
    True
    Edited by Vincent Pelletier
  • Maybe we can use one of the followings ? (I checked only Zope 4.8 code)

    Do these cover the 3 attributes I mentioned (and that the code lists) ?

  • Doing the normalization in response.listHeaders() seems good, and it seems to work also on Zope 2, isn't it ?

    Does listHeader pull values from all 3 sources ? AFAIK on Zope 2 accumulated_headers is only used once: dumped to the client socket when writing the response.

  • ( and it's used in .finalize which is used in publisher, so it's what's used to write the response )

  • Ah, I think I understand what you mean:

    • stop accessing the 3 attributes separately
    • just call listHeaders
    • parse cookies from their serialised representation, always, so the same normalisation code is used for all

    Is this correct ?

    I think there is no strong reason to access these headersattributes separately, so it should be fine yes.

    Edited by Vincent Pelletier
  • Yes, my understanding was something like this as well.

    More or less replacing https://lab.nexedi.com/nexedi/erp5/blob/173465601b8167295d39426342da6034ba8dd1e8/bt5/erp5_oauth2_authorisation/TestTemplateItem/portal_components/test.erp5.testOAuth2Server.py#L412-418

          cookie_dict = response.cookies.copy()
          response_header_dict = {}
          for item_list in (
            six.iteritems(response.headers),
            response.accumulated_headers,
          ):
            for key, value in item_list:

    by something like

          cookie_dict = {}
          response_header_dict = {}
          for key, value in response.listHeaders():
  • @jerome What you suggest works perfectly.

    @vpelletier shall I commit this change (i.e. what Jérome suggested above) to master ?

    Edited by Kazuhiko Shiozaki
  • mentioned in commit 58026b9c

    Toggle commit list
  • @vpelletier shall I commit this change (i.e. what Jérome suggested above) to master ?

    Yes please.

  • Done in a274cdfd. Without indentation change, only the following change :

    --- bt5/erp5_oauth2_authorisation/TestTemplateItem/portal_components/test.erp5.testOAuth2Server.py
    +++ bt5/erp5_oauth2_authorisation/TestTemplateItem/portal_components/test.erp5.testOAuth2Server.py
    @@ -409,13 +409,9 @@ class TestOAuth2(ERP5TypeTestCase):
           exc = None
         finally:
           setSecurityManager(current_security_manager)
    -      cookie_dict = response.cookies.copy()
    +      cookie_dict = {}
           response_header_dict = {}
    -      for item_list in (
    -        six.iteritems(response.headers),
    -        response.accumulated_headers,
    -      ):
    -        for key, value in item_list:
    +      for key, value in response.listHeaders():
             key = key.lower()
             if key == 'set-cookie':
               # XXX: minimal Set-Cookie parser
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment