Merge ~cjwatson/launchpad:simplify-appserverlayer-browser into launchpad:master

[Colin Watson]

Refactor lp.testing.browser using zope.testbrowser.wsgi

This test browser instance is used for a few tests that use
AppServerLayer and make out-of-process connections to the test app
server via a test browser.

The old mechanize-based test browser is going away, so we need to do
something. A reasonably future-proof approach seems to be to use the
new WSGI test browser and fool it into connecting to the app server
using WSGIProxy2 (which in fact WebTest also uses in some situations).
We'll still need some slight adjustments here to upgrade to newer
versions of zope.testbrowser, but they should be much more manageable.

We do need to take some special care in a few places:

 * TestOpenIDReplayAttack can't pass in a custom mech_browser any more.
   For now, we switch off redirect handling entirely and follow
   redirections manually; this will need further rearrangements for
   zope.testbrowser >= 5.0.0.

 * Both the httplib and the requests clients offered by WSGIProxy2
   incorrectly fold multiple Set-Cookie headers in the response into
   one, joining them with commas; this causes test failures.
   Fortunately, the urllib3 client doesn't have this flaw, but we do
   need to take care to disable certificate verification.

 * The new WSGI test browser needs to be monkey-patched to allow talking
   to our various test hosts, as by default it only allows localhost,
   127.0.0.1, *.example.com, *.example.net, and *.example.org.

[Ioana Lasc (ilasc)]

review: Approve

diff --git a/constraints.txt b/constraints.txt
index ada9fd0..6a36571 100644
--- a/constraints.txt
+++ b/constraints.txt
@@ -362,6 +362,7 @@ waitress==1.3.1
 WebOb==1.8.5
 WebTest==2.0.33
 wheel==0.29.0
+WSGIProxy2==0.4.6
 wsgiref==0.1.2
 z3c.pt==2.2.3
 z3c.ptcompat==0.5.7
diff --git a/lib/lp/services/webapp/tests/no-anonymous-session-cookies.txt b/lib/lp/services/webapp/tests/no-anonymous-session-cookies.txt
index fbd726e..68477f6 100644
--- a/lib/lp/services/webapp/tests/no-anonymous-session-cookies.txt
+++ b/lib/lp/services/webapp/tests/no-anonymous-session-cookies.txt
@@ -54,9 +54,13 @@ time for browsers with bad system clocks.
     >>> # XXX 2010-05-08 bac bug=577596 This work-around for the fact
     >>> # that loggerhead is not running needs to be replaced with
     >>> # something more robust and clear.
+    >>> import warnings
     >>> from urllib2 import HTTPError, URLError
+    >>> from urllib3.exceptions import InsecureRequestWarning
     >>> try:
-    ...     browser.getControl('Log Out').click()
+    ...     with warnings.catch_warnings():
+    ...         warnings.simplefilter('ignore', InsecureRequestWarning)
+    ...         browser.getControl('Log Out').click()
     ... except (HTTPError, URLError):
     ...     pass
 
diff --git a/lib/lp/services/webapp/tests/test_cookie_authentication.py b/lib/lp/services/webapp/tests/test_cookie_authentication.py
index 9b011fb..0dbe9dc 100644
--- a/lib/lp/services/webapp/tests/test_cookie_authentication.py
+++ b/lib/lp/services/webapp/tests/test_cookie_authentication.py
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test harness for running the cookie-authentication.txt tests."""
@@ -9,10 +9,7 @@ __all__ = []
 
 import unittest
 
-from lp.testing.browser import (
-    setUp,
-    tearDown,
-    )
+from lp.testing.browser import setUp
 from lp.testing.layers import AppServerLayer
 from lp.testing.systemdocs import LayeredDocFileSuite
 
@@ -23,6 +20,5 @@ def test_suite():
     # page (+login), which cannot be used through the normal testbrowser that
     # goes straight to zope's publication instead of making HTTP requests.
     suite.addTest(LayeredDocFileSuite(
-        'cookie-authentication.txt', setUp=setUp, tearDown=tearDown,
-        layer=AppServerLayer))
+        'cookie-authentication.txt', setUp=setUp, layer=AppServerLayer))
     return suite
diff --git a/lib/lp/services/webapp/tests/test_login.py b/lib/lp/services/webapp/tests/test_login.py
index 2ae9c2e..eaa0d3e 100644
--- a/lib/lp/services/webapp/tests/test_login.py
+++ b/lib/lp/services/webapp/tests/test_login.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 """Test harness for running the new-login.txt tests."""
 
@@ -20,10 +20,8 @@ from datetime import (
 import httplib
 import unittest
 import urllib
-import urllib2
 import urlparse
 
-import mechanize
 from openid.consumer.consumer import (
     FAILURE,
     SUCCESS,
@@ -33,6 +31,7 @@ from openid.extensions import (
     sreg,
     )
 from openid.yadis.discover import DiscoveryFailure
+from six.moves.urllib.error import HTTPError
 from testtools.matchers import (
     Contains,
     ContainsDict,
@@ -76,7 +75,6 @@ from lp.testing import (
 from lp.testing.browser import (
     Browser,
     setUp,
-    tearDown,
     )
 from lp.testing.fixture import ZopeViewReplacementFixture
 from lp.testing.layers import (
@@ -661,21 +659,6 @@ class TestOpenIDCallbackRedirects(TestCaseWithFactory):
 urls_redirected_to = []
 
 
-class MyHTTPRedirectHandler(mechanize.HTTPRedirectHandler):
-    """Custom HTTPRedirectHandler which stores the URLs redirected to."""
-
-    def redirect_request(self, req, fp, code, msg, headers, newurl):
-        urls_redirected_to.append(newurl)
-        return mechanize.HTTPRedirectHandler.redirect_request(
-            self, req, fp, code, msg, headers, newurl)
-
-
-class MyMechanizeBrowser(mechanize.Browser):
-    """Custom Browser which uses MyHTTPRedirectHandler to handle redirects."""
-    handler_classes = mechanize.Browser.handler_classes.copy()
-    handler_classes['_redirect'] = MyHTTPRedirectHandler
-
-
 def fill_login_form_and_submit(browser, email_address):
     assert browser.getControl(name='field.email') is not None, (
         "We don't seem to be looking at a login form.")
@@ -687,7 +670,8 @@ class TestOpenIDReplayAttack(TestCaseWithFactory):
     layer = AppServerLayer
 
     def test_replay_attacks_do_not_succeed(self):
-        browser = Browser(mech_browser=MyMechanizeBrowser())
+        browser = Browser()
+        browser.mech_browser.set_handle_redirect(False)
         browser.open('%s/+login' % self.layer.appserver_root_url())
         # On a JS-enabled browser this page would've been auto-submitted
         # (thanks to the onload handler), but here we have to do it manually.
@@ -695,7 +679,17 @@ class TestOpenIDReplayAttack(TestCaseWithFactory):
         browser.getControl('Continue').click()
 
         self.assertEqual('Login', browser.title)
-        fill_login_form_and_submit(browser, '[e-mail]')
+        redirection = self.assertRaises(
+            HTTPError,
+            fill_login_form_and_submit, browser, '[e-mail]')
+        self.assertEqual(httplib.FOUND, redirection.code)
+        callback_url = redirection.hdrs['Location']
+        self.assertIn('+openid-callback', callback_url)
+        callback_redirection = self.assertRaises(
+            HTTPError, browser.open, callback_url)
+        self.assertEqual(
+            httplib.TEMPORARY_REDIRECT, callback_redirection.code)
+        browser.open(callback_redirection.hdrs['Location'])
         login_status = extract_text(
             find_tag_by_id(browser.contents, 'logincontrol'))
         self.assertIn('Sample Person (name12)', login_status)
@@ -704,9 +698,6 @@ class TestOpenIDReplayAttack(TestCaseWithFactory):
         # was used to complete the authentication and open it on a different
         # browser with a fresh set of cookies.
         replay_browser = Browser()
-        [callback_url] = [
-            url for url in urls_redirected_to if '+openid-callback' in url]
-        self.assertIsNot(None, callback_url)
         replay_browser.open(callback_url)
         login_status = extract_text(
             find_tag_by_id(replay_browser.contents, 'logincontrol'))
@@ -741,7 +732,7 @@ class TestMissingServerShowsNiceErrorPage(TestCase):
         fixture.replacement = OpenIDLoginThatFailsDiscovery
         self.useFixture(fixture)
         browser = TestBrowser()
-        self.assertRaises(urllib2.HTTPError,
+        self.assertRaises(HTTPError,
                           browser.open, 'http://launchpad.test/+login')
         self.assertEqual('503 Service Unavailable',
                          browser.headers.get('status'))
@@ -946,5 +937,5 @@ def test_suite():
     suite = unittest.TestSuite()
     suite.addTest(unittest.TestLoader().loadTestsFromName(__name__))
     suite.addTest(LayeredDocFileSuite(
-        'login.txt', setUp=setUp, tearDown=tearDown, layer=AppServerLayer))
+        'login.txt', setUp=setUp, layer=AppServerLayer))
     return suite
diff --git a/lib/lp/services/webapp/tests/test_no_anonymous_session_cookies.py b/lib/lp/services/webapp/tests/test_no_anonymous_session_cookies.py
index 15c1446..c1f18ab 100644
--- a/lib/lp/services/webapp/tests/test_no_anonymous_session_cookies.py
+++ b/lib/lp/services/webapp/tests/test_no_anonymous_session_cookies.py
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test harness for running the no-anonymous-session-cookies.txt tests."""
@@ -9,10 +9,7 @@ __all__ = []
 
 import unittest
 
-from lp.testing.browser import (
-    setUp,
-    tearDown,
-    )
+from lp.testing.browser import setUp
 from lp.testing.layers import AppServerLayer
 from lp.testing.systemdocs import LayeredDocFileSuite
 
@@ -23,6 +20,5 @@ def test_suite():
     # page (+login), which cannot be used through the normal testbrowser that
     # goes straight to zope's publication instead of making HTTP requests.
     suite.addTest(LayeredDocFileSuite(
-        'no-anonymous-session-cookies.txt', setUp=setUp, tearDown=tearDown,
-        layer=AppServerLayer))
+        'no-anonymous-session-cookies.txt', setUp=setUp, layer=AppServerLayer))
     return suite
diff --git a/lib/lp/services/webservice/doc/launchpadlib.txt b/lib/lp/services/webservice/doc/launchpadlib.txt
index 5edbf92..c3c0cde 100644
--- a/lib/lp/services/webservice/doc/launchpadlib.txt
+++ b/lib/lp/services/webservice/doc/launchpadlib.txt
@@ -3,7 +3,8 @@
 Just to show that we're actually talking to the appserver, first check to see
 if a specific user exists...
 
-    >>> browser = Browser('[e-mail]:test')
+    >>> browser = Browser()
+    >>> browser.addHeader('Authorization', 'Basic [e-mail]:test')
     >>> from lp.testing.layers import BaseLayer
     >>> root_url = BaseLayer.appserver_root_url()
     >>> browser.open(root_url)
diff --git a/lib/lp/services/webservice/tests/test_doc.py b/lib/lp/services/webservice/tests/test_doc.py
index cd5acc3..ce59e0e 100644
--- a/lib/lp/services/webservice/tests/test_doc.py
+++ b/lib/lp/services/webservice/tests/test_doc.py
@@ -1,4 +1,4 @@
-# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """
@@ -37,13 +37,11 @@ special = {
     # properly isolates the database between tests.
     'launchpadlib.txt': LayeredDocFileSuite(
         '../doc/launchpadlib.txt',
-        layer=AppServerLayer,
-        setUp=browser.setUp, tearDown=browser.tearDown,),
+        layer=AppServerLayer, setUp=browser.setUp),
     'launchpadlib.txt-2': LayeredDocFileSuite(
         '../doc/launchpadlib.txt',
         id_extensions=['launchpadlib.txt-2'],
-        layer=AppServerLayer,
-        setUp=browser.setUp, tearDown=browser.tearDown,),
+        layer=AppServerLayer, setUp=browser.setUp),
     }
 
 
diff --git a/lib/lp/testing/browser.py b/lib/lp/testing/browser.py
index 9df40df..7f4284b 100644
--- a/lib/lp/testing/browser.py
+++ b/lib/lp/testing/browser.py
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """A real, socket connecting browser.
@@ -7,30 +7,24 @@ This browser performs actual socket connections to a real HTTP server.  This
 is used in tests which utilize the AppServerLayer to run the app server in a
 child process.  The Zope testing browser fakes its connections in-process, so
 that's not good enough.
-
-The browser provided here extends `zope.app.testing.testbrowser.Browser` by
-providing a close() method that delegates to the underlying mechanize browser,
-and it tracks all Browser instances to ensure that they are closed.  This
-latter prevents open socket leaks even when the doctest doesn't explicitly
-close or delete the browser instance.
 """
-from lazr.uri._uri import URI
-
 
 __metaclass__ = type
 __all__ = [
-    'Browser',
     'setUp',
-    'tearDown',
     ]
 
+import ssl
 
-import base64
-import urllib2
-import weakref
-
+from lazr.uri import URI
 import transaction
-from zope.testbrowser.browser import Browser as _Browser
+from urllib3 import PoolManager
+from wsgiproxy.proxies import TransparentProxy
+from wsgiproxy.urllib3_client import HttpClient
+from zope.testbrowser.wsgi import (
+    AuthorizationMiddleware,
+    Browser as _Browser,
+    )
 
 from lp.testing.pages import (
     extract_text,
@@ -40,65 +34,36 @@ from lp.testing.pages import (
     )
 
 
-class SocketClosingOnErrorHandler(urllib2.BaseHandler):
-    """A handler that ensures that the socket gets closed on errors.
+class TransactionMiddleware:
+    """Middleware to commit the current transaction before the test.
 
-    Interestingly enough <wink> without this, a 404 will cause mechanize to
-    leak open socket objects.
+    This is like `zope.app.wsgi.TransactionMiddleware`, but avoids ZODB.
     """
-    # Ensure that this handler is the first default error handler to execute,
-    # because right after this, the built-in default handler will raise an
-    # exception.
-    handler_order = 0
-
-    # Copy signature from base class.
-    def http_error_default(self, req, fp, code, msg, hdrs):
-        """See `urllib2.BaseHandler`."""
-        fp.close()
 
+    def __init__(self, app):
+        self.app = app
 
-# To ensure that the mechanize browser doesn't leak socket connections past
-# the end of the test, we manage a set of weak references to live browser
-# objects.  The layer can then call a function here to ensure that all live
-# browsers get properly closed.
-_live_browser_set = set()
+    def __call__(self, environ, start_response):
+        transaction.commit()
+        for entry in self.app(environ, start_response):
+            yield entry
 
 
 class Browser(_Browser):
-    """A browser subclass that knows about basic auth."""
-
-    def __init__(self, auth=None, mech_browser=None):
-        super(Browser, self).__init__(mech_browser=mech_browser)
-        # We have to add the error handler to the mechanize browser underlying
-        # the Zope browser, because it's the former that's actually doing all
-        # the work.
-        self.mech_browser.add_handler(SocketClosingOnErrorHandler())
-        if auth:
-            # Unlike the higher level Zope test browser, we actually have to
-            # encode the basic auth information.
-            userpass = base64.b64encode(auth)
-            self.addHeader('Authorization', 'Basic ' + userpass)
-        _live_browser_set.add(weakref.ref(self, self._refclose))
-
-    def _refclose(self, obj):
-        """For weak reference cleanup."""
-        self.close()
-
-    def close(self):
-        """Yay!  Zope browsers don't have a close() method."""
-        self.mech_browser.close()
-
-    def _changed(self):
-        """Ensure the current transaction is committed.
-
-        Because this browser is used in the AppServerLayer where it talks
-        real-HTTP to a child process, we need to ensure that the parent
-        process also gets its current transaction in sync with the child's
-        changes.  The easiest way to do that is to just commit the current
-        transaction.
-        """
-        super(Browser, self)._changed()
-        transaction.commit()
+
+    def __init__(self, url=None, wsgi_app=None):
+        if wsgi_app is None:
+            # urllib3 is carefully-chosen: both the httplib and requests
+            # clients incorrectly comma-join multiple Set-Cookie headers, at
+            # least on Python 2.7, which causes failures in some of the
+            # +login tests.  However, we have to go to a bit of effort to
+            # disable certificate verification to avoid problems with e.g.
+            # +logout redirecting to https://bazaar.launchpad.test/+logout.
+            client = HttpClient(pool=PoolManager(10, cert_reqs=ssl.CERT_NONE))
+            wsgi_app = AuthorizationMiddleware(
+                TransactionMiddleware(
+                    TransparentProxy(client=client)))
+        super(Browser, self).__init__(url=url, wsgi_app=wsgi_app)
 
     @property
     def vhost(self):
@@ -124,11 +89,3 @@ def setUp(test):
     test.globs['find_main_content'] = find_main_content
     test.globs['print_feedback_messages'] = print_feedback_messages
     test.globs['extract_text'] = extract_text
-
-
-def tearDown(test):
-    """Tear down appserver tests."""
-    for ref in _live_browser_set:
-        browser = ref()
-        if browser is not None:
-            browser.close()
diff --git a/lib/lp/testing/layers.py b/lib/lp/testing/layers.py
index 3ec422f..22a510f 100644
--- a/lib/lp/testing/layers.py
+++ b/lib/lp/testing/layers.py
@@ -1085,6 +1085,20 @@ class FunctionalLayer(BaseLayer):
         root = fs.connection.root()
         root[ZopePublication.root_name] = MockRootFolder()
 
+        # Allow the WSGI test browser to talk to our various test hosts.
+        def assert_allowed_host(self):
+            host = self.host
+            if ':' in host:
+                host = host.split(':')[0]
+            if host == 'localhost' or host.endswith('.test'):
+                return
+            self._allowed = False
+
+        FunctionalLayer._testbrowser_allowed = MonkeyPatch(
+            'zope.testbrowser.wsgi.WSGIConnection.assert_allowed_host',
+            assert_allowed_host)
+        FunctionalLayer._testbrowser_allowed.setUp()
+
         # Should be impossible, as the CA cannot be unloaded. Something
         # mighty nasty has happened if this is triggered.
         if not is_ca_available():
@@ -1094,6 +1108,8 @@ class FunctionalLayer(BaseLayer):
     @classmethod
     @profiled
     def testTearDown(cls):
+        FunctionalLayer._testbrowser_allowed.cleanUp()
+
         # Should be impossible, as the CA cannot be unloaded. Something
         # mighty nasty has happened if this is triggered.
         if not is_ca_available():
diff --git a/setup.py b/setup.py
index 0b8d56b..90c4424 100644
--- a/setup.py
+++ b/setup.py
@@ -233,6 +233,7 @@ setup(
         'txpkgupload',
         'virtualenv-tools3',
         'wadllib',
+        'WSGIProxy2',
         'z3c.pt',
         'z3c.ptcompat',
         'zc.zservertracelog',