Re: [Freeipa-devel] [PATCH 83] Cookie Expires date should be locale insensitive
On Wed, Dec 19, 2012 at 01:36:40PM -0500, John Dennis wrote: -- John Dennis jden...@redhat.com Looking to carve out IT costs? www.redhat.com/carveoutcosts/ Patch is working as expected and he code looks good to me. I just have a minor comment. I think 'import time' can be removed from both files. Although it looks like it wasn't used even before your patch I wonder if you can add the removal here? bye, Sumit ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 83] Cookie Expires date should be locale insensitive
On 12/19/2012 07:36 PM, John Dennis wrote: I tested the patch on RHEL platform and it works fine and removes the annoying error. My comments on the patch: 1) I do not think its necessary to write target branches to commit message. Also there is a typo: ipapython/cooke.py 2) As for the tests - could we for example try setting non-US locale in the test to verify that cookie lib is locale independent? Python has means to do that, (import locale; locale.setlocale(locale.LC_ALL, 'cs_CZ')). But this is not a blocker for this patch. I am sure that Petr^3 will have more comments on the code as he is reviewing it too :-) Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 83] Cookie Expires date should be locale insensitive
On 12/19/2012 07:36 PM, John Dennis wrote: The Expires attribute in a cookie is supposed to follow the RFC 822 (superseded by RFC 1123) date format. That format includes a weekday abbreviation (e.g. Tue) which must be in English according to the RFC's. ipapython/cooke.py has methods to parse and format the Expires attribute but they were based on strptime() and strftime() which respects the locale. If a non-English locale is in effect the wrong date string will be produced and/or it won't be able to parse the date string. The fix is to use the date parsing and formatting functions from email.utils which specifically follow the RFC's and are not locale sensitive. This patch also updates the unit test to use email.utils as well. The patch should be applied to the following branches: master, 3.0, 3.1 Ticket:https://fedorahosted.org/freeipa/ticket/3313 This solves the issue for me. It's better than what's there now, so It's OK as a hotfix. However, I did find something to discuss. Your comment references RFC 1123, which doesn't seem relevant at all. The cookie Expires header is defined in RFC 6265 (section 5.1.1), but email.utils.parsedate uses syntax defined in RFC 2822. Apparently the two are equivalent for common usage, but RFC 6265 specifies a more complicated algorithm. Shouldn't we follow it? To nitpick, I'm not a fan of including target branches in the commit message (they should be in the accompanying e-mail), or of documenting past bugs as comments in the code (git log/blame works better for checking history). -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 83] Cookie Expires date should be locale insensitive
On 12/20/2012 12:35 PM, Petr Viktorin wrote: On 12/19/2012 07:36 PM, John Dennis wrote: The Expires attribute in a cookie is supposed to follow the RFC 822 (superseded by RFC 1123) date format. That format includes a weekday abbreviation (e.g. Tue) which must be in English according to the RFC's. ipapython/cooke.py has methods to parse and format the Expires attribute but they were based on strptime() and strftime() which respects the locale. If a non-English locale is in effect the wrong date string will be produced and/or it won't be able to parse the date string. The fix is to use the date parsing and formatting functions from email.utils which specifically follow the RFC's and are not locale sensitive. This patch also updates the unit test to use email.utils as well. The patch should be applied to the following branches: master, 3.0, 3.1 Ticket:https://fedorahosted.org/freeipa/ticket/3313 This solves the issue for me. It's better than what's there now, so It's OK as a hotfix. However, I did find something to discuss. Your comment references RFC 1123, which doesn't seem relevant at all. The cookie Expires header is defined in RFC 6265 (section 5.1.1), but email.utils.parsedate uses syntax defined in RFC 2822. Apparently the two are equivalent for common usage, but RFC 6265 specifies a more complicated algorithm. Shouldn't we follow it? To nitpick, I'm not a fan of including target branches in the commit message (they should be in the accompanying e-mail), or of documenting past bugs as comments in the code (git log/blame works better for checking history). Pushed to master, ipa-3-1, ipa-3-0 (since we need this hotfix now). I just fixed the commit message as written above. You can open another upstream ticket to fix these comment discrepancies. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH 83] Cookie Expires date should be locale insensitive
--
John Dennis jden...@redhat.com
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
From 49213139d31fe402c616ec468d2b593685232ef9 Mon Sep 17 00:00:00 2001
From: John Dennis jden...@redhat.com
Date: Wed, 19 Dec 2012 12:47:46 -0500
Subject: [PATCH 83] Cookie Expires date should be locale insensitive
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
The Expires attribute in a cookie is supposed to follow the RFC 822
(superseded by RFC 1123) date format. That format includes a weekday
abbreviation (e.g. Tue) which must be in English according to the
RFC's.
ipapython/cooke.py has methods to parse and format the Expires
attribute but they were based on strptime() and strftime() which
respects the locale. If a non-English locale is in effect the wrong
date string will be produced and/or it won't be able to parse the date
string.
The fix is to use the date parsing and formatting functions from
email.utils which specifically follow the RFC's and are not locale
sensitive.
This patch also updates the unit test to use email.utils as well.
The patch should be applied to the following branches:
master, 3.0, 3.1
Ticket: https://fedorahosted.org/freeipa/ticket/3313
---
ipapython/cookie.py | 42 ++---
tests/test_ipapython/test_cookie.py | 15 +++--
2 files changed, 20 insertions(+), 37 deletions(-)
diff --git a/ipapython/cookie.py b/ipapython/cookie.py
index b45cb2b..bf551b5 100644
--- a/ipapython/cookie.py
+++ b/ipapython/cookie.py
@@ -20,6 +20,7 @@
import re
import time
import datetime
+import email.utils
from urllib2 import urlparse
from calendar import timegm
from ipapython.ipa_log_manager import log_mgr
@@ -172,47 +173,26 @@ class Cookie(object):
if utcoffset is not None and utcoffset.total_seconds() != 0.0:
raise ValueError(timezone is not UTC)
-# At this point we've validated as much as possible the
-# timezone is UTC or GMT but we can't use the %Z timezone
-# format specifier because the timezone in the string must be
-# 'GMT', not something equivalent to GMT, so hardcode the GMT
-# timezone string into the format.
+# Do not use strftime because it respects the locale, instead
+# use the RFC 1123 formatting function which uses only English
-return datetime.datetime.strftime(dt, '%a, %d %b %Y %H:%M:%S GMT')
+return email.utils.formatdate(cls.datetime_to_time(dt), usegmt=True)
@classmethod
def parse_datetime(cls, s):
'''
-Parse a RFC 822, RFC 1123 date string, return a datetime aware object in UTC.
-Accommodates some non-standard formats found in the wild.
+Parse a RFC 822, RFC 1123 date string, return a datetime naive object in UTC.
'''
-formats = ['%a, %d %b %Y %H:%M:%S',
- '%a, %d-%b-%Y %H:%M:%S',
- '%a, %d-%b-%y %H:%M:%S',
- '%a, %d %b %y %H:%M:%S',
- ]
s = s.strip()
-# strptime does not read the time zone and generate a tzinfo
-# object to insert in the datetime object so there is little point
-# in specifying a %Z format, instead verify GMT is specified and
-# generate the datetime object as if it were UTC.
+# Do not use strptime because it respects the locale, instead
+# use the RFC 1123 parsing function which uses only English
-if not s.endswith(' GMT'):
-raise ValueError(http date string '%s' does not end with GMT time zone % s)
-s = s[:-4]
-
-dt = None
-for format in formats:
-try:
-dt = datetime.datetime(*(time.strptime(s, format)[0:6]))
-break
-except Exception:
-continue
-
-if dt is None:
-raise ValueError(unable to parse expires datetime '%s' % s)
+try:
+dt = datetime.datetime(*email.utils.parsedate(s)[0:6])
+except Exception, e:
+raise ValueError(unable to parse expires datetime '%s': %s % (s, e))
return dt
diff --git a/tests/test_ipapython/test_cookie.py b/tests/test_ipapython/test_cookie.py
index f8c5daf..b8a2d36 100644
--- a/tests/test_ipapython/test_cookie.py
+++ b/tests/test_ipapython/test_cookie.py
@@ -20,6 +20,7 @@
import unittest
import time
import datetime
+import email.utils
import calendar
from ipapython.cookie import Cookie
@@ -129,15 +130,16 @@ class TestExpires(unittest.TestCase):
# Force microseconds to zero because cookie timestamps only have second resolution
self.now = datetime.datetime.utcnow().replace(microsecond=0)
self.now_timestamp = calendar.timegm(self.now.utctimetuple())
-self.now_string = datetime.datetime.strftime(self.now, '%a, %d %b %Y %H:%M:%S GMT')
+self.now_string = email.utils.formatdate(self.now_timestamp, usegmt=True)
