Merge lp:~adeuring/launchpad/bug-261254-fix-mantis-login into lp:launchpad

[Abel Deuring]

This branch fixes bug 261254: Launchpad couldn't connect to ALSA Bug Tracker.

The communication between the Launchpad's "Mantis bug watcher" and
a Mantis bug tracker was hampered by three issues:

1. Some time ago, ClientCookie.HTTPRedirectHandler was replaced by
   urllib2.HTTPRedirectHandler as the base class of MantisLoginHandler,
   but the cookie handler from urllib2 was not added. Since many
   Mantis trackers require users to login (be it as a guest), and since
   the credentials are stored in a cookie, access to a Mantis tracker
   was no longer possible. (I also think that things like bug filter
   settings are stored in cookies.)

2. class MantisLoginHandler works by overloading the method
   redirect_request() of ClientCookie.HTTPRedirectHandler /
   urllib2.HTTPRedirectHandler. Somehow, this method simply
   disappeared.

3. At least the Alsa bug tracker returns a HTTP 500 error when the
   URL for CSV data is accessed. This led to an exception in
   Mantis.csv_data. The new implementation simply treats this
   repsonse as an indication that the Mantis isntance does not
   provide any CSV data and that we must screen-scrape the bug data.

The code changes to fix these bugs are quite straightforward, I think.

In order to ensure that a similiar unnoticed disappearance of the
method MantisLoginHandler.redirect_request() does not happen again,
I added a unit test for it. Similary, I added a test to ensure that
MantisLoginHandler is used.

Finally, I added a test that HTTP 500 errors are treated by
Mantis.csv_data as described above. In order to test that other HTTP
errors are not swallowed by Mantis.csv_data, I moved the the
code from this cached property into the method _csv_data(), so that
I can use assertRaises.

The redirect and HTTP error tests require a sort of "fake
request/response handling", which was already implemented by the
class Urlib2TransportTestHandler, but the implementation was
"tailor-made" for some XMLRPC tests. I generalized it a bit, which
in turn required some modifications of
externalbugtracker-xmlrpc-transport.txt.

The new method setError(self, error, url) indeed needs the second
parameter:
1. Mantis._csv_data() issues two HTTP requests, and we want that
   the second request returns an error.
2. While HTTPError.__init__() wants the URL as a parameter too,
   it seems that it is nowhere stored, so we can't use something
   like seif.raise_error.url in Urlib2TransportTestHandler to check
   in Urlib2TransportTestHandler.default_open() if the current
   URL the one where the error should be returned.

Finally, "make lint" had three complaints:

    1620: [E0702, Urlib2TransportTestHandler.default_open] Raising NoneType while only classes, instances or string are allowed
    1626: [W0108 , Urlib2TransportTestHandler.default_open.<lambda>] Lambda may not be necessary
    1639: [W0108, Urlib2TransportTestHandler.default_open.<lambda>] Lambda may not be necessary

I don't think that we'll ever hae a "raise None" in line 1620.

Lambda _may_ not be necessary, but using something like

            response.geturl = req.get_full_url

is not a good idea, since response and req are instances of different
classes.

I disabled these messages.

tests:

./bin/test -vvt test_bugtracker
./bin/test -vvt tests/externalbugtracker-xmlrpc-transport.txt

No lint except the wrong messages mentioned above.

[Deryck Hodge (deryck)]

Looks good. Nice work debugging what was going on here.

Cheers,
deryck

review: Approve (code)

=== modified file 'lib/lp/bugs/externalbugtracker/mantis.py'
--- lib/lp/bugs/externalbugtracker/mantis.py	2010-03-16 16:52:42 +0000
+++ lib/lp/bugs/externalbugtracker/mantis.py	2010-04-12 09:54:39 +0000
@@ -18,9 +18,9 @@
 from canonical.launchpad.webapp.url import urlparse
 
 from lp.bugs.externalbugtracker import (
-    BugNotFound, BugWatchUpdateError, BugWatchUpdateWarning,
-    ExternalBugTracker, InvalidBugId, LookupTree, UnknownRemoteStatusError,
-    UnparseableBugData)
+    BugNotFound, BugTrackerConnectError, BugWatchUpdateError,
+    BugWatchUpdateWarning, ExternalBugTracker, InvalidBugId, LookupTree,
+    UnknownRemoteStatusError, UnparseableBugData)
 from lp.bugs.externalbugtracker.isolation import ensure_no_transaction
 from lp.bugs.interfaces.bugtask import BugTaskImportance, BugTaskStatus
 from lp.bugs.interfaces.externalbugtracker import UNKNOWN_REMOTE_IMPORTANCE
@@ -79,6 +79,10 @@
 
         return url
 
+    def redirect_request(self, request, fp, code, msg, hdrs, new_url):
+        return urllib2.HTTPRedirectHandler.redirect_request(
+            self, request, fp, code, msg, hdrs, self.rewrite_url(new_url))
+
 
 class Mantis(ExternalBugTracker):
     """An `ExternalBugTracker` for dealing with Mantis instances.
@@ -89,9 +93,13 @@
         https://dev.launchpad.net/Bugs/ExternalBugTrackers/Mantis
     """
 
-    # Custom opener that automatically sends anonymous credentials to
-    # Mantis if (and only if) needed.
-    _opener = urllib2.build_opener(MantisLoginHandler)
+    def __init__(self, baseurl):
+        super(Mantis, self).__init__(baseurl)
+        # Custom cookie aware opener that automatically sends anonymous
+        # credentials to Mantis if (and only if) needed.
+        self._cookie_handler = urllib2.HTTPCookieProcessor()
+        self._opener = urllib2.build_opener(
+            self._cookie_handler, MantisLoginHandler())
 
     @ensure_no_transaction
     def urlopen(self, request, data=None):
@@ -111,6 +119,10 @@
         If the export fails (i.e. the response is 0-length), None will
         be returned.
         """
+        return self._csv_data()
+
+    def _csv_data(self):
+        """See `csv_data()."""
         # Next step is getting our query filter cookie set up; we need
         # to do this weird submit in order to get the closed bugs
         # included in the results; the default Mantis filter excludes
@@ -157,7 +169,16 @@
         # Finally grab the full CSV export, which uses the
         # MANTIS_VIEW_ALL_COOKIE set in the previous step to specify
         # what's being viewed.
-        csv_data = self._getPage("csv_export.php")
+        try:
+            csv_data = self._getPage("csv_export.php")
+        except BugTrackerConnectError, value:
+            # Some Mantis installations simply return a 500 error
+            # when the csv_export.php page is accessed. Since the
+            # bug data may be nevertheless available from ordinary
+            # web pages, we simply ignore this error.
+            if value.error.startswith('HTTP Error 500'):
+                return None
+            raise
 
         if not csv_data:
             return None

=== modified file 'lib/lp/bugs/tests/externalbugtracker-xmlrpc-transport.txt'
--- lib/lp/bugs/tests/externalbugtracker-xmlrpc-transport.txt	2009-10-16 22:12:33 +0000
+++ lib/lp/bugs/tests/externalbugtracker-xmlrpc-transport.txt	2010-04-12 09:54:39 +0000
@@ -5,7 +5,7 @@
 which can connect through an http proxy server.
 
     >>> from lp.bugs.tests.externalbugtracker import (
-    ...     patch_transport_opener)
+    ...     Urlib2TransportTestHandler)
     >>> from lp.bugs.externalbugtracker.xmlrpc import (
     ...     UrlLib2Transport)
 
@@ -23,7 +23,8 @@
 connection.  The response returns the request-url as an XMLRPC parameter, and
 sets a cookie from the server, 'foo=bar'.
 
-    >>> patch_transport_opener(transport)
+    >>> test_handler = Urlib2TransportTestHandler()
+    >>> transport.opener.add_handler(test_handler)
 
 Before sending the request, the transport's cookie jar is empty.
 
@@ -61,6 +62,12 @@
 If an error occurs trying to make the request, an
 `xmlrpclib.ProtocolError` is raised.
 
+    >>> from urllib2 import HTTPError
+    >>> test_handler.setError(
+    ...     HTTPError(
+    ...         'http://www.example.com/xmlrpc', 500, 'Internal Error', {},
+    ...          None),
+    ...     'http://www.example.com/xmlrpc')
     >>> request_body = """<?xml version="1.0"?>
     ... <methodCall>
     ...   <methodName>examples.testError</methodName>
@@ -83,9 +90,10 @@
 to the location indicated in that response rather than the original
 location.
 
+    >>> test_handler.setRedirect('http://www.example.com/xmlrpc/redirected')
     >>> request_body = """<?xml version="1.0"?>
     ... <methodCall>
-    ...   <methodName>examples.testRedirect</methodName>
+    ...   <methodName>examples.whatever</methodName>
     ...   <params>
     ...     <param>
     ...       <value>

=== modified file 'lib/lp/bugs/tests/externalbugtracker.py'
--- lib/lp/bugs/tests/externalbugtracker.py	2010-04-09 11:07:02 +0000
+++ lib/lp/bugs/tests/externalbugtracker.py	2010-04-12 09:54:39 +0000
@@ -1,7 +1,7 @@
 # Copyright 2009 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-# pylint: disable-msg=W0231
+# pylint: disable-msg=W0231,E0702,W0108
 
 """Helper classes for testing ExternalSystem."""
 
@@ -17,7 +17,7 @@
 from StringIO import StringIO
 from datetime import datetime, timedelta
 from httplib import HTTPMessage
-from urllib2 import BaseHandler, HTTPError, Request
+from urllib2 import BaseHandler, Request
 
 from zope.component import getUtility
 
@@ -42,7 +42,6 @@
 from canonical.launchpad.interfaces.logintoken import ILoginTokenSet
 from lp.bugs.scripts import debbugs
 from canonical.launchpad.testing.systemdocs import ordered_dict_as_string
-from canonical.launchpad.webapp import urlappend
 from canonical.launchpad.xmlrpc import ExternalBugTrackerTokenAPI
 from canonical.testing.layers import LaunchpadZopelessLayer
 
@@ -1592,6 +1591,20 @@
 class Urlib2TransportTestHandler(BaseHandler):
     """A test urllib2 handler returning a hard-coded response."""
 
+    def __init__(self):
+        self.redirect_url = None
+        self.raise_error = None
+        self.accessed_urls = []
+
+    def setRedirect(self, new_url):
+        """The next call of default_open() will redirect to `url`."""
+        self.redirect_url = new_url
+
+    def setError(self, error, url):
+        """Raise `error` when `url` is accessed."""
+        self.raise_error = error
+        self.raise_url = url
+
     def default_open(self, req):
         """Catch all requests and return a hard-coded response.
 
@@ -1601,31 +1614,23 @@
         assert isinstance(req, Request), (
             'Expected a urllib2.Request, got %s' % req)
 
-        if 'testError' in req.data:
-            raise HTTPError(
-                req.get_full_url(), 500, 'Internal Error', {}, None)
-
-        elif ('testRedirect' in req.data and
-              'redirected' not in req.get_full_url()):
-            # Big hack to make calls to testRedirect act as though a 302
-            # has been received. Note the slightly cheaty check for
-            # 'redirected' in the URL. This is to stop urllib2 from
-            # whinging about infinite loops.
-            redirect_url = urlappend(
-                req.get_full_url(), 'redirected')
-
+        self.accessed_urls.append(req.get_full_url())
+        if (self.raise_error is not None and
+              req.get_full_url() == self.raise_url):
+            error = self.raise_error
+            self.raise_error = None
+            raise error
+        elif self.redirect_url is not None:
             headers = HTTPMessage(StringIO())
-            headers['location'] = redirect_url
-
+            headers['location'] = self.redirect_url
             response = StringIO()
             response.info = lambda: headers
             response.geturl = lambda: req.get_full_url()
             response.code = 302
             response.msg = 'Moved'
+            self.redirect_url = None
             response = self.parent.error(
-                'http', req, response, 302, 'Moved',
-                headers)
-
+                'http', req, response, 302, 'Moved', headers)
         else:
             xmlrpc_response = xmlrpclib.dumps(
                 (req.get_full_url(),), methodresponse=True)

=== modified file 'lib/lp/bugs/tests/test_bugtracker.py'
--- lib/lp/bugs/tests/test_bugtracker.py	2010-03-26 14:16:21 +0000
+++ lib/lp/bugs/tests/test_bugtracker.py	2010-04-12 09:54:39 +0000
@@ -4,11 +4,14 @@
 __metaclass__ = type
 
 import unittest
+from urllib2 import HTTPError, Request
 
 from datetime import datetime, timedelta
 
 from pytz import utc
 
+import transaction
+
 from zope.security.proxy import removeSecurityProxy
 from zope.testing.doctest import NORMALIZE_WHITESPACE, ELLIPSIS
 from zope.testing.doctestunit import DocTestSuite
@@ -18,13 +21,20 @@
 from canonical.launchpad.ftests import login_person
 from canonical.testing import LaunchpadFunctionalLayer
 
+from lp.bugs.externalbugtracker import (
+    BugTrackerConnectError, Mantis, MantisLoginHandler)
 from lp.bugs.interfaces.bugtracker import BugTrackerType, IBugTracker
+from lp.bugs.tests.externalbugtracker import Urlib2TransportTestHandler
 from lp.testing import TestCaseWithFactory
 
 
 class TestBugTracker(TestCaseWithFactory):
     layer = LaunchpadFunctionalLayer
 
+    def setUp(self):
+        super(TestBugTracker, self).setUp()
+        transaction.commit()
+
     def test_multi_product_constraints_observed(self):
         """BugTrackers for which multi_product=True should return None
         when no remote product is passed to getBugFilingURL().
@@ -121,6 +131,93 @@
         removeSecurityProxy(bug_message).remote_comment_id = 'brains'
         self.failUnless(bug_tracker.watches_with_unpushed_comments.is_empty())
 
+    def test_mantis_login_redirects(self):
+        # The Mantis bug tracker needs a special HTTP redirect handler
+        # in order to login in. Ensure that redirects to the page with
+        # the login form are indeed changed to redirects the form submit
+        # URL.
+        handler = MantisLoginHandler()
+        request = Request('http://mantis.example.com/some/path')
+        # Let's pretend that Mantis sent a redirect request to the
+        # login page.
+        new_request = handler.redirect_request(
+            request, None, 302, None, None,
+            'http://mantis.example.com/login_page.php'
+            '?return=%2Fview.php%3Fid%3D3301')
+        self.assertEqual(
+            'http://mantis.example.com/login.php?'
+            'username=guest&password=guest&return=%2Fview.php%3Fid%3D3301',
+            new_request.get_full_url())
+
+    def test_mantis_login_redirect_handler_is_used(self):
+        # Ensure that the special Mantis login handler is used
+        # by the Mantis tracker
+        tracker = Mantis('http://mantis.example.com')
+        test_handler = Urlib2TransportTestHandler()
+        test_handler.setRedirect('http://mantis.example.com/login_page.php'
+            '?return=%2Fsome%2Fpage')
+        opener = tracker._opener
+        opener.add_handler(test_handler)
+        opener.open('http://mantis.example.com/some/page')
+        # We should now have two entries in the test handler's list
+        # of visited URLs: The original URL we wanted to visit and the
+        # URL changed by the MantisLoginHandler.
+        self.assertEqual(
+            ['http://mantis.example.com/some/page',
+             'http://mantis.example.com/login.php?'
+             'username=guest&password=guest&return=%2Fsome%2Fpage'],
+            test_handler.accessed_urls)
+
+    def test_mantis_opener_can_handle_cookies(self):
+        # Ensure that the OpenerDirector of the Mantis bug tracker
+        # handles cookies.
+        tracker = Mantis('http://mantis.example.com')
+        test_handler = Urlib2TransportTestHandler()
+        opener = tracker._opener
+        opener.add_handler(test_handler)
+        opener.open('http://mantis.example.com', '')
+        cookies = list(tracker._cookie_handler.cookiejar)
+        self.assertEqual(1, len(cookies))
+        self.assertEqual('foo', cookies[0].name)
+        self.assertEqual('bar', cookies[0].value)
+
+    def test_mantis_csv_file_http_500_error(self):
+        # If a Mantis bug tracker returns a HTTP 500 error when the
+        # URL for CSV data is accessed, we treat this as an
+        # indication that we should screen scrape the bug data and
+        # thus set csv_data to None.
+        tracker = Mantis('http://mantis.example.com')
+        test_handler = Urlib2TransportTestHandler()
+        opener = tracker._opener
+        opener.add_handler(test_handler)
+        test_handler.setError(
+            HTTPError(
+                'http://mantis.example.com/csv_export.php', 500,
+                'Internal Error', {}, None),
+            'http://mantis.example.com/csv_export.php')
+        self.assertIs(None, tracker.csv_data)
+
+    def test_mantis_csv_file_other_http_errors(self):
+        # If the Mantis server returns other HTTP errors than 500,
+        # they appear as BugTrackerConnectErrors.
+        tracker = Mantis('http://mantis.example.com')
+        test_handler = Urlib2TransportTestHandler()
+        opener = tracker._opener
+        opener.add_handler(test_handler)
+        test_handler.setError(
+            HTTPError(
+                'http://mantis.example.com/csv_export.php', 503,
+                'Service Unavailable', {}, None),
+            'http://mantis.example.com/csv_export.php')
+        self.assertRaises(BugTrackerConnectError, tracker._csv_data)
+
+        test_handler.setError(
+            HTTPError(
+                'http://mantis.example.com/csv_export.php', 404,
+                'Not Found', {}, None),
+            'http://mantis.example.com/csv_export.php')
+        self.assertRaises(BugTrackerConnectError, tracker._csv_data)
+
 
 def test_suite():
     suite = unittest.TestSuite()