Launchpad itself

Merge lp:~allenap/launchpad/trac-sniffing-broken-bug-504310 into lp:launchpad

  1. trac-sniffing-broken-bug-504310
  2. Merge into devel
Proposed by Gavin Panella
Status: Merged
Approved by: Gavin Panella
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~allenap/launchpad/trac-sniffing-broken-bug-504310
Merge into: lp:launchpad
Diff against target: 149 lines (+60/-14)
2 files modified
lib/lp/bugs/doc/externalbugtracker-trac.txt (+35/-4)
lib/lp/bugs/externalbugtracker/trac.py (+25/-10)
To merge this branch: bzr merge lp:~allenap/launchpad/trac-sniffing-broken-bug-504310
Reviewer Review Type Date Requested Status
Eleanor Berger (community) Approve
Review via email: mp+17139@code.launchpad.net

Commit message

Work around broken Trac installations that return HTTP 200 when the resource is not found. This has caused Launchpad to believe that the trac-launchpad plugin is installed when it is not.

To post a comment you must log in.
Revision history for this message
Gavin Panella (allenap) wrote :

Pre-implementation call with Graham Binns.

Recently we've noticed that several Trac installations return HTTP 200 for page not found (i.e. should be a 404). This breaks the sniffing we do to detect the presence of the Launchpad plugin <https://launchpad.net/trac-launchpad>.

The fix proposed here is to check all HTTP 200 responses for the presence of a trac_auth cookie. This should not be present for a should-be-a-404 response. In the very very unlikely event that the authentication token we pass when sniffing (a deliberately bogus token: "check") is valid, the trac_auth cookie will be present.

I've tested this with http://trac.macports.org/ - one of the main culprits of broken 404 behaviour - and it works fine.

Lint free, other than the usual pylint nonsense.

Test: bin/test -vvt 'external.*bug'

Revision history for this message
Eleanor Berger (intellectronica) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/bugs/doc/externalbugtracker-trac.txt'
2--- lib/lp/bugs/doc/externalbugtracker-trac.txt 2009-06-15 11:10:49 +0000
3+++ lib/lp/bugs/doc/externalbugtracker-trac.txt 2010-01-11 14:05:35 +0000
4@@ -48,15 +48,46 @@
5 >>> chosen_trac.baseurl
6 'http://example.com'
7
8-In the unlikely case where the token would be valid, a 200 would be
9-returned. This also causes a TracLPPlugin to be used.
10+Some Trac instances in the wild return HTTP 200 when the resource is
11+not found (HTTP 404 would be more appropriate). A distinguishing
12+difference between a 200 response from a broken Trac and a response
13+from a Trac with the plugin installed (when we've accidentally passed
14+a valid token, which is very unlikely), is that the broken Trac will
15+not include a "trac_auth" cookie.
16
17+ >>> from httplib import HTTPMessage
18+ >>> from urllib import addinfourl
19 >>> from StringIO import StringIO
20+
21+ >>> class TracReturning200ForPageNotFound(Trac):
22+ ... def urlopen(self, url):
23+ ... print url
24+ ... return addinfourl(
25+ ... StringIO(''), HTTPMessage(StringIO('')), url)
26+
27 >>> class TracHavingLPPlugin200(Trac):
28 ... def urlopen(self, url):
29 ... print url
30- ... return StringIO('')
31- >>> trac = TracHavingLPPlugin401('http://example.com/')
32+ ... return addinfourl(
33+ ... StringIO(''), HTTPMessage(
34+ ... StringIO('Set-Cookie: trac_auth=1234')), url)
35+
36+The plain, non-plugin, external bug tracker is selected for broken
37+Trac installations:
38+
39+ >>> trac = TracReturning200ForPageNotFound('http://example.com/')
40+ >>> chosen_trac = trac.getExternalBugTrackerToUse()
41+ http://example.com/launchpad-auth/check
42+
43+ >>> isinstance(chosen_trac, TracLPPlugin)
44+ False
45+ >>> chosen_trac.baseurl
46+ 'http://example.com'
47+
48+In the event that our deliberately bogus token is considered valid,
49+the external bug tracker that groks the plugin is selected:
50+
51+ >>> trac = TracHavingLPPlugin200('http://example.com/')
52 >>> chosen_trac = trac.getExternalBugTrackerToUse()
53 http://example.com/launchpad-auth/check
54
55
56=== modified file 'lib/lp/bugs/externalbugtracker/trac.py'
57--- lib/lp/bugs/externalbugtracker/trac.py 2010-01-06 14:35:52 +0000
58+++ lib/lp/bugs/externalbugtracker/trac.py 2010-01-11 14:05:35 +0000
59@@ -12,13 +12,13 @@
60 import urllib2
61 import xmlrpclib
62
63+from Cookie import SimpleCookie
64 from cookielib import CookieJar
65 from datetime import datetime
66 from email.Utils import parseaddr
67 from zope.component import getUtility
68 from zope.interface import implements
69
70-from canonical.cachedproperty import cachedproperty
71 from canonical.config import config
72 from lp.bugs.externalbugtracker.base import (
73 BugNotFound, BugTrackerAuthenticationError, ExternalBugTracker,
74@@ -48,6 +48,7 @@
75 # Fault code constants for the LP Plugin
76 FAULT_TICKET_NOT_FOUND = 1001
77
78+
79 class Trac(ExternalBugTracker):
80 """An ExternalBugTracker instance for handling Trac bugtrackers."""
81
82@@ -61,11 +62,26 @@
83 # Any token will do.
84 auth_url = urlappend(base_auth_url, 'check')
85 try:
86- self.urlopen(auth_url)
87+ response = self.urlopen(auth_url)
88 except urllib2.HTTPError, error:
89- if error.code != 401:
90- return self
91- return TracLPPlugin(self.baseurl)
92+ # If the error is HTTP 401 Unauthorized then we're
93+ # probably talking to the LP plugin.
94+ if error.code == 401:
95+ return TracLPPlugin(self.baseurl)
96+ else:
97+ return self
98+ else:
99+ # If the response contains a trac_auth cookie then we're
100+ # talking to the LP plugin. However, it's unlikely that
101+ # the remote system will authorize the bogus auth token we
102+ # sent, so this check is really intended to detect broken
103+ # Trac instances that return HTTP 200 for a missing page.
104+ for set_cookie in response.headers.getheaders('Set-Cookie'):
105+ cookie = SimpleCookie(set_cookie)
106+ if 'trac_auth' in cookie:
107+ return TracLPPlugin(self.baseurl)
108+ else:
109+ return self
110
111 def supportsSingleExports(self, bug_ids):
112 """Return True if the Trac instance provides CSV exports for single
113@@ -83,11 +99,11 @@
114 # We try to retrive the ticket in HTML form, since that will
115 # tell us whether or not it is actually a valid ticket
116 ticket_id = int(bug_ids.pop())
117- html_data = self.urlopen(html_ticket_url % ticket_id)
118+ self.urlopen(html_ticket_url % ticket_id)
119 except (ValueError, urllib2.HTTPError):
120 # If we get an HTTP error we can consider the ticket to be
121 # invalid. If we get a ValueError then the ticket_id couldn't
122- # be intified and it's of no use to us anyway.
123+ # be identified and it's of no use to us anyway.
124 pass
125 else:
126 # If we didn't get an error we can try to get the ticket in
127@@ -345,7 +361,7 @@
128 auth_url = urlappend(base_auth_url, token_text)
129
130 try:
131- response = self.urlopen(auth_url)
132+ self.urlopen(auth_url)
133 except urllib2.HTTPError, error:
134 raise BugTrackerAuthenticationError(
135 self.baseurl, '%s "%s"' % (error.code, error.msg))
136@@ -481,7 +497,7 @@
137 launchpad_bug_id = 0
138
139 try:
140- timestamp = self._server.launchpad.set_launchpad_bug(
141+ self._server.launchpad.set_launchpad_bug(
142 remote_bug, launchpad_bug_id)
143 except xmlrpclib.Fault, fault:
144 # Deal with "Ticket does not exist" faults. We re-raise
145@@ -490,4 +506,3 @@
146 raise BugNotFound(remote_bug)
147 else:
148 raise
149-