launchpad-reviewers team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

Gary Poster has proposed merging lp:~gary/launchpad/bug553368 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #553368 in Launchpad itself: "Better handling of errors when trying to login when the OP is down"
  https://bugs.launchpad.net/launchpad/+bug/553368

For more details, see:
https://code.launchpad.net/~gary/launchpad/bug553368/+merge/64108

This branch provides a simple fix for the linked bug: an OpenID DiscoveryError returns a 503 and a hopefully helpful error page, but no OOPS is logged.

Writing a test for this was a bit tricky because I didn't want to use a monkeypatch, and because I wanted to show that the full reported error was squashed.  I ended up with a test fixture for replacing one view with another in the Zope component registry, which seems to work pretty well.  I suspect it has limited usefulness, but it is general, and was easy enough to factor it out as a fixture, so I did so.
-- 
https://code.launchpad.net/~gary/launchpad/bug553368/+merge/64108
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/bug553368 into lp:launchpad.

=== modified file 'lib/canonical/launchpad/webapp/login.py'
--- lib/canonical/launchpad/webapp/login.py	2011-05-27 21:25:58 +0000
+++ lib/canonical/launchpad/webapp/login.py	2011-06-10 02:26:30 +0000
@@ -22,6 +22,7 @@
     setDefaultFetcher,
     Urllib2Fetcher,
     )
+from openid.yadis.discover import DiscoveryFailure
 import transaction
 from z3c.ptcompat import ViewPageTemplateFile
 from zope.app.security.interfaces import IUnauthenticatedPrincipal
@@ -182,6 +183,9 @@
     """A view which initiates the OpenID handshake with our provider."""
     _openid_session_ns = 'OPENID'
 
+    _discovery_failure_template = ViewPageTemplateFile(
+        '../../../lp/app/templates/launchpad-discoveryfailure.pt')
+
     def _getConsumer(self):
         session = ISession(self.request)[self._openid_session_ns]
         openid_store = getUtility(IOpenIDConsumerStore)
@@ -204,6 +208,9 @@
         try:
             self.openid_request = consumer.begin(
                 allvhosts.configs[openid_vhost].rooturl)
+        except DiscoveryFailure:
+            self.request.response.setStatus(503) # Service Unavailable
+            return self._discovery_failure_template()
         finally:
             timeline_action.finish()
         self.openid_request.addExtension(

=== modified file 'lib/canonical/launchpad/webapp/tests/test_login.py'
--- lib/canonical/launchpad/webapp/tests/test_login.py	2011-05-31 16:43:11 +0000
+++ lib/canonical/launchpad/webapp/tests/test_login.py	2011-06-10 02:26:30 +0000
@@ -26,10 +26,13 @@
     SUCCESS,
     )
 from openid.extensions import sreg
+from openid.yadis.discover import DiscoveryFailure
+import urllib2
 from zope.component import getUtility
 from zope.security.management import newInteraction
 from zope.security.proxy import removeSecurityProxy
 from zope.session.interfaces import ISession
+from zope.testbrowser.testing import Browser as TestBrowser
 
 from canonical.launchpad.interfaces.account import (
     AccountStatus,
@@ -49,7 +52,11 @@
     )
 from canonical.launchpad.testing.systemdocs import LayeredDocFileSuite
 from canonical.launchpad.webapp.dbpolicy import MasterDatabasePolicy
-from canonical.launchpad.webapp.interfaces import IStoreSelector
+from canonical.launchpad.webapp.errorlog import globalErrorUtility
+from canonical.launchpad.webapp.interfaces import (
+    ILaunchpadApplication,
+    IStoreSelector,
+    )
 from canonical.launchpad.webapp.login import (
     OpenIDCallbackView,
     OpenIDLogin,
@@ -65,8 +72,10 @@
 from lp.services.timeline.requesttimeline import get_request_timeline
 from lp.testing import (
     logout,
+    TestCase,
     TestCaseWithFactory,
     )
+from lp.testing.fixture import ZopeViewReplacementFixture
 from lp.testopenid.interfaces.server import ITestOpenIDPersistentIdentity
 
 
@@ -597,6 +606,40 @@
                           extract_text(error_msg))
 
 
+class FakeHTTPResponse:
+    status = 500
+
+
+class OpenIDConsumerThatFailsDiscovery:
+
+    def begin(self, url):
+        raise DiscoveryFailure(
+            'HTTP Response status from identity URL host is not 200. '
+            'Got status 500', FakeHTTPResponse)
+
+
+class TestMissingServerDoesNotOops(TestCase):
+    layer = DatabaseFunctionalLayer
+
+    def test_missing_openid_server_does_not_cause_oops(self):
+        fixture = ZopeViewReplacementFixture('+login', ILaunchpadApplication)
+        class OpenIDLoginThatFailsDiscovery(fixture.original):
+            def _getConsumer(self):
+                return OpenIDConsumerThatFailsDiscovery()
+        fixture.replacement = OpenIDLoginThatFailsDiscovery
+        self.useFixture(fixture)
+        last_oops = globalErrorUtility.getLastOopsReport()
+        browser = TestBrowser()
+        self.assertRaises(urllib2.HTTPError,
+                          browser.open, 'http://launchpad.dev/+login')
+        self.assertEquals('503 Service Unavailable',
+                          browser.headers.get('status'))
+        # No OOPS was generated as a result of the exception
+        self.assertNoNewOops(last_oops)
+        self.assertTrue(
+            'OpenID Provider Is Unavailable at This Time' in browser.contents)
+
+
 class FakeOpenIDRequest:
     extensions = None
     return_to = None

=== added file 'lib/lp/app/templates/launchpad-discoveryfailure.pt'
--- lib/lp/app/templates/launchpad-discoveryfailure.pt	1970-01-01 00:00:00 +0000
+++ lib/lp/app/templates/launchpad-discoveryfailure.pt	2011-06-10 02:26:30 +0000
@@ -0,0 +1,19 @@
+<html
+  xmlns="http://www.w3.org/1999/xhtml";
+  xmlns:tal="http://xml.zope.org/namespaces/tal";
+  xmlns:metal="http://xml.zope.org/namespaces/metal";
+  xmlns:i18n="http://xml.zope.org/namespaces/i18n";
+  metal:use-macro="view/macro:page/locationless"
+  i18n:domain="launchpad"
+>
+  <body>
+    <div class="top-portlet"  metal:fill-slot="main">
+      <h1 class="exception">OpenID Provider Is Unavailable at This Time</h1>
+      <p>The openid provider was unavailable.  Please try again in a
+      moment.</p>
+      <p>You can also join <a href="irc://irc.freenode.net/launchpad">the
+      #launchpad IRC support channel on irc.freenode.net</a> for further
+      assistance.</p>
+    </div>
+  </body>
+</html>

=== modified file 'lib/lp/testing/fixture.py'
--- lib/lp/testing/fixture.py	2010-10-17 21:21:07 +0000
+++ lib/lp/testing/fixture.py	2011-06-10 02:26:30 +0000
@@ -13,6 +13,13 @@
     getGlobalSiteManager,
     provideHandler,
     )
+from zope.interface import Interface
+from zope.publisher.interfaces.browser import IDefaultBrowserLayer
+from zope.security.checker import (
+    defineChecker,
+    getCheckerForInstancesOf,
+    undefineChecker,
+    )
 
 
 class ZopeEventHandlerFixture(Fixture):
@@ -27,3 +34,49 @@
         gsm = getGlobalSiteManager()
         provideHandler(self._handler)
         self.addCleanup(gsm.unregisterHandler, self._handler)
+
+
+class ZopeViewReplacementFixture(Fixture):
+    """A fixture that allows you to temporarily replace one view with another.
+    
+    This will not work with the AppServerLayer.
+    """
+
+    def __init__(self, name, context_interface,
+                 request_interface=IDefaultBrowserLayer,
+                 replacement=None):
+        super(ZopeViewReplacementFixture, self).__init__()
+        self.name = name
+        self.context_interface = context_interface
+        self.request_interface = request_interface
+        self.gsm = getGlobalSiteManager()
+        # It can be convenient--bordering on necessary--to use this original
+        # class as a base for the replacement.
+        self.original = self.gsm.adapters.registered(
+            (context_interface, request_interface), Interface, name)
+        self.checker = getCheckerForInstancesOf(self.original)
+        if self.original is None:
+            # The adapter registry does not provide good methods to introspect
+            # it. If it did, we might try harder here.
+            raise ValueError(
+                'No existing view to replace.  Wrong request interface?  '
+                'Try a layer.')
+        self.replacement = replacement
+
+    def setUp(self):
+        super(ZopeViewReplacementFixture, self).setUp()
+        if self.replacement is None:
+            raise ValueError('replacement is not set')
+        self.gsm.adapters.register(
+            (self.context_interface, self.request_interface), Interface,
+             self.name, self.replacement)
+        # The same checker should be sufficient.  If it ever isn't, we
+        # can add more flexibility then.
+        defineChecker(self.replacement, self.checker)
+
+    def tearDown(self):
+        super(ZopeViewReplacementFixture, self).tearDown()
+        undefineChecker(self.replacement)
+        self.gsm.adapters.register(
+            (self.context_interface, self.request_interface), Interface,
+             self.name, self.original)