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

[Gavin Panella]

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.

[Gavin Panella (allenap)]

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'

[Eleanor Berger (intellectronica)]

review: Approve

=== modified file 'lib/lp/bugs/doc/externalbugtracker-trac.txt'
--- lib/lp/bugs/doc/externalbugtracker-trac.txt	2009-06-15 11:10:49 +0000
+++ lib/lp/bugs/doc/externalbugtracker-trac.txt	2010-01-11 14:05:35 +0000
@@ -48,15 +48,46 @@
     >>> chosen_trac.baseurl
     'http://example.com'
 
-In the unlikely case where the token would be valid, a 200 would be
-returned. This also causes a TracLPPlugin to be used.
+Some Trac instances in the wild return HTTP 200 when the resource is
+not found (HTTP 404 would be more appropriate). A distinguishing
+difference between a 200 response from a broken Trac and a response
+from a Trac with the plugin installed (when we've accidentally passed
+a valid token, which is very unlikely), is that the broken Trac will
+not include a "trac_auth" cookie.
 
+    >>> from httplib import HTTPMessage
+    >>> from urllib import addinfourl
     >>> from StringIO import StringIO
+
+    >>> class TracReturning200ForPageNotFound(Trac):
+    ...     def urlopen(self, url):
+    ...         print url
+    ...         return addinfourl(
+    ...             StringIO(''), HTTPMessage(StringIO('')), url)
+
     >>> class TracHavingLPPlugin200(Trac):
     ...     def urlopen(self, url):
     ...         print url
-    ...         return StringIO('')
-    >>> trac = TracHavingLPPlugin401('http://example.com/')
+    ...         return addinfourl(
+    ...             StringIO(''), HTTPMessage(
+    ...                 StringIO('Set-Cookie: trac_auth=1234')), url)
+
+The plain, non-plugin, external bug tracker is selected for broken
+Trac installations:
+
+    >>> trac = TracReturning200ForPageNotFound('http://example.com/')
+    >>> chosen_trac = trac.getExternalBugTrackerToUse()
+    http://example.com/launchpad-auth/check
+
+    >>> isinstance(chosen_trac, TracLPPlugin)
+    False
+    >>> chosen_trac.baseurl
+    'http://example.com'
+
+In the event that our deliberately bogus token is considered valid,
+the external bug tracker that groks the plugin is selected:
+
+    >>> trac = TracHavingLPPlugin200('http://example.com/')
     >>> chosen_trac = trac.getExternalBugTrackerToUse()
     http://example.com/launchpad-auth/check
 

=== modified file 'lib/lp/bugs/externalbugtracker/trac.py'
--- lib/lp/bugs/externalbugtracker/trac.py	2010-01-06 14:35:52 +0000
+++ lib/lp/bugs/externalbugtracker/trac.py	2010-01-11 14:05:35 +0000
@@ -12,13 +12,13 @@
 import urllib2
 import xmlrpclib
 
+from Cookie import SimpleCookie
 from cookielib import CookieJar
 from datetime import datetime
 from email.Utils import parseaddr
 from zope.component import getUtility
 from zope.interface import implements
 
-from canonical.cachedproperty import cachedproperty
 from canonical.config import config
 from lp.bugs.externalbugtracker.base import (
     BugNotFound, BugTrackerAuthenticationError, ExternalBugTracker,
@@ -48,6 +48,7 @@
 # Fault code constants for the LP Plugin
 FAULT_TICKET_NOT_FOUND = 1001
 
+
 class Trac(ExternalBugTracker):
     """An ExternalBugTracker instance for handling Trac bugtrackers."""
 
@@ -61,11 +62,26 @@
         # Any token will do.
         auth_url = urlappend(base_auth_url, 'check')
         try:
-            self.urlopen(auth_url)
+            response = self.urlopen(auth_url)
         except urllib2.HTTPError, error:
-            if error.code != 401:
-                return self
-        return TracLPPlugin(self.baseurl)
+            # If the error is HTTP 401 Unauthorized then we're
+            # probably talking to the LP plugin.
+            if error.code == 401:
+                return TracLPPlugin(self.baseurl)
+            else:
+                return self
+        else:
+            # If the response contains a trac_auth cookie then we're
+            # talking to the LP plugin. However, it's unlikely that
+            # the remote system will authorize the bogus auth token we
+            # sent, so this check is really intended to detect broken
+            # Trac instances that return HTTP 200 for a missing page.
+            for set_cookie in response.headers.getheaders('Set-Cookie'):
+                cookie = SimpleCookie(set_cookie)
+                if 'trac_auth' in cookie:
+                    return TracLPPlugin(self.baseurl)
+            else:
+                return self
 
     def supportsSingleExports(self, bug_ids):
         """Return True if the Trac instance provides CSV exports for single
@@ -83,11 +99,11 @@
                 # We try to retrive the ticket in HTML form, since that will
                 # tell us whether or not it is actually a valid ticket
                 ticket_id = int(bug_ids.pop())
-                html_data = self.urlopen(html_ticket_url % ticket_id)
+                self.urlopen(html_ticket_url % ticket_id)
             except (ValueError, urllib2.HTTPError):
                 # If we get an HTTP error we can consider the ticket to be
                 # invalid. If we get a ValueError then the ticket_id couldn't
-                # be intified and it's of no use to us anyway.
+                # be identified and it's of no use to us anyway.
                 pass
             else:
                 # If we didn't get an error we can try to get the ticket in
@@ -345,7 +361,7 @@
         auth_url = urlappend(base_auth_url, token_text)
 
         try:
-            response = self.urlopen(auth_url)
+            self.urlopen(auth_url)
         except urllib2.HTTPError, error:
             raise BugTrackerAuthenticationError(
                 self.baseurl, '%s "%s"' % (error.code, error.msg))
@@ -481,7 +497,7 @@
             launchpad_bug_id = 0
 
         try:
-            timestamp = self._server.launchpad.set_launchpad_bug(
+            self._server.launchpad.set_launchpad_bug(
                 remote_bug, launchpad_bug_id)
         except xmlrpclib.Fault, fault:
             # Deal with "Ticket does not exist" faults. We re-raise
@@ -490,4 +506,3 @@
                 raise BugNotFound(remote_bug)
             else:
                 raise
-