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
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/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 Wed, Dec 19, 2012 at 01:36:40PM -0500, John Dennis wrote: > > -- > John Dennis > > 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
