Merge lp:~benji/launchpad/bug-697735-2 into lp:launchpad

[Benji York]

[r=gmb][bug=697735] don't do OOPS reports for URL parameter type conversion errors

Bug 697735 is about OOPS reports that are generated when
zope.publisher.browser tries to apply a type conversion (e.g.,
?foo:int=7) and the conversion raises an exception. Generating an error
is fine, but we don't want OOPS reports logged when they occur.

This branch adds a marker interface that can be applied to an exception
that signals the OOPS reporting mechanism that no report should be
logged. Originally I added an underscore-prefixed attribute to
exceptions that shouldn't be recorded, but Gary suggested the
improvement of using a marker interface.

The several tests were added to
lib/canonical/launchpad/webapp/tests/test_errorlog.py cover the new
behavior.

A fair bit of mostly whitespace lint was fixed in
./lib/canonical/launchpad/webapp/interfaces.py as well.

The only suboptimal part of this branch is the need to monkey patch
zope.publisher.browser (see lib/lp_sitecustomize.py) so that we could
wrap the conversion functions with a try/except to mark ValueErrors as
non-oops-report-worthy. Gary and I discussed this aspect of the branch
and felt like it was a reasonable compromise.

[Graham Binns (gmb)]

Thanks for the lint cleanup along with everything else. r=me.

review: Approve (code)

=== modified file 'lib/canonical/launchpad/webapp/errorlog.py'
--- lib/canonical/launchpad/webapp/errorlog.py	2011-06-01 19:23:24 +0000
+++ lib/canonical/launchpad/webapp/errorlog.py	2011-06-15 14:58:50 +0000
@@ -40,6 +40,7 @@
     IErrorReport,
     IErrorReportEvent,
     IErrorReportRequest,
+    IUnloggedException,
     )
 from canonical.launchpad.webapp.opstats import OpStats
 from canonical.launchpad.webapp.vhosts import allvhosts
@@ -372,7 +373,18 @@
         notify(ErrorReportEvent(entry))
         return entry
 
-    def _isIgnoredException(self, strtype, request=None):
+    def _isIgnoredException(self, strtype, request=None, exception=None):
+        """Should the given exception generate an OOPS or be ignored?
+
+        Exceptions will be ignored if they
+            - are specially tagged as being ignorable by having the marker
+              interface IUnloggedException
+            - are of a type included in self._ignored_exceptions, or
+            - were requested with an off-site REFERRER header and are of a
+              type included in self._ignored_exceptions_for_offsite_referer
+        """
+        if IUnloggedException.providedBy(exception):
+            return True
         if strtype in self._ignored_exceptions:
             return True
         if strtype in self._ignored_exceptions_for_offsite_referer:
@@ -409,7 +421,7 @@
         tb_text = None
 
         strtype = str(getattr(info[0], '__name__', info[0]))
-        if self._isIgnoredException(strtype, request):
+        if self._isIgnoredException(strtype, request, info[1]):
             return
 
         if not isinstance(info[2], basestring):

=== modified file 'lib/canonical/launchpad/webapp/interfaces.py'
--- lib/canonical/launchpad/webapp/interfaces.py	2011-04-27 16:01:06 +0000
+++ lib/canonical/launchpad/webapp/interfaces.py	2011-06-15 14:58:50 +0000
@@ -178,7 +178,8 @@
 class ILink(ILinkData):
     """An object that represents a link in a menu.
 
-    The attributes name, url and linked may be set by the menus infrastructure.
+    The attributes name, url and linked may be set by the menus
+    infrastructure.
     """
 
     name = Attribute("The name of this link in Python data structures.")
@@ -262,6 +263,7 @@
             (object_url_requested_for, broken_link_in_chain)
             )
 
+
 # XXX kiko 2007-02-08: this needs reconsideration if we are to make it a truly
 # generic thing. The problem lies in the fact that half of this (user, login,
 # time zone, developer) is actually useful inside webapp/, and the other half
@@ -307,6 +309,7 @@
         connection blows up.
         '''
 
+
 #
 # Request
 #
@@ -406,6 +409,7 @@
 
 class CookieAuthLoggedInEvent:
     implements(ILoggedInEvent)
+
     def __init__(self, request, login):
         self.request = request
         self.login = login
@@ -413,6 +417,7 @@
 
 class CookieAuthPrincipalIdentifiedEvent:
     implements(IPrincipalIdentifiedEvent)
+
     def __init__(self, principal, request, login):
         self.principal = principal
         self.request = request
@@ -421,6 +426,7 @@
 
 class BasicAuthLoggedInEvent:
     implements(ILoggedInEvent, IPrincipalIdentifiedEvent)
+
     def __init__(self, request, login, principal):
         # these one from ILoggedInEvent
         self.login = login
@@ -436,6 +442,7 @@
 
 class LoggedOutEvent:
     implements(ILoggedOutEvent)
+
     def __init__(self, request):
         self.request = request
 
@@ -527,6 +534,7 @@
         you're using right now.
         """)
 
+
 class AccessLevel(DBEnumeratedType):
     """The level of access any given principal has."""
     use_template(OAuthPermission, exclude='UNAUTHORIZED')
@@ -553,10 +561,10 @@
 
 class BrowserNotificationLevel:
     """Matches the standard logging levels."""
-    DEBUG = logging.DEBUG     # A debugging message
-    INFO = logging.INFO       # simple confirmation of a change
-    WARNING = logging.WARNING # action will not be successful unless you ...
-    ERROR = logging.ERROR     # the previous action did not succeed, and why
+    DEBUG = logging.DEBUG  # debugging message
+    INFO = logging.INFO  # simple confirmation of a change
+    WARNING = logging.WARNING  # action will not be successful unless you ...
+    ERROR = logging.ERROR  # the previous action did not succeed, and why
 
     ALL_LEVELS = (DEBUG, INFO, WARNING, ERROR)
 
@@ -646,6 +654,10 @@
         """
 
 
+class IUnloggedException(Interface):
+    """An exception that should not be logged in an OOPS report (marker)."""
+
+
 class IErrorReportEvent(IObjectEvent):
     """A new error report has been created."""
 
@@ -673,6 +685,7 @@
         description=u"""an identifier for the exception, or None if no
         exception has occurred""")
 
+
 #
 # Batch Navigation
 #
@@ -720,12 +733,12 @@
 # Database policies
 #
 
-MAIN_STORE = 'main' # The main database.
+MAIN_STORE = 'main'  # The main database.
 ALL_STORES = frozenset([MAIN_STORE])
 
-DEFAULT_FLAVOR = 'default' # Default flavor for current state.
-MASTER_FLAVOR = 'master' # The master database.
-SLAVE_FLAVOR = 'slave' # A slave database.
+DEFAULT_FLAVOR = 'default'  # Default flavor for current state.
+MASTER_FLAVOR = 'master'  # The master database.
+SLAVE_FLAVOR = 'slave'  # A slave database.
 
 
 class IDatabasePolicy(Interface):
@@ -856,7 +869,6 @@
 
         request = Attribute("The request the event is about")
 
-
     class StartRequestEvent:
         """An event fired once at the start of requests.
 

=== modified file 'lib/canonical/launchpad/webapp/menu.py'
--- lib/canonical/launchpad/webapp/menu.py	2011-03-23 16:28:51 +0000
+++ lib/canonical/launchpad/webapp/menu.py	2011-06-15 14:58:50 +0000
@@ -39,7 +39,6 @@
     removeSecurityProxy,
     )
 
-from canonical.launchpad.webapp.authorization import check_permission
 from canonical.launchpad.webapp.interfaces import (
     IApplicationMenu,
     IContextMenu,
@@ -529,6 +528,10 @@
         """
         permission = self.permission
 
+        # This is imported here to forestall an import-time config read that
+        # wreaks havoc.
+        from canonical.launchpad.webapp.authorization import check_permission
+
         def enable_if_allowed(self):
             link = func(self)
             if not check_permission(permission, self.context):

=== modified file 'lib/canonical/launchpad/webapp/tests/test_errorlog.py'
--- lib/canonical/launchpad/webapp/tests/test_errorlog.py	2011-06-01 19:35:22 +0000
+++ lib/canonical/launchpad/webapp/tests/test_errorlog.py	2011-06-15 14:58:50 +0000
@@ -41,7 +41,10 @@
     OopsLoggingHandler,
     ScriptRequest,
     )
-from canonical.launchpad.webapp.interfaces import NoReferrerError
+from canonical.launchpad.webapp.interfaces import (
+    IUnloggedException,
+    NoReferrerError,
+    )
 from canonical.testing import reset_logging
 from lp.app.errors import (
     GoneError,
@@ -50,6 +53,7 @@
 from lp.services.log.uniquefileallocator import UniqueFileAllocator
 from lp.services.osutils import remove_tree
 from lp.testing import TestCase
+from lp_sitecustomize import customize_get_converter
 
 
 UTC = pytz.timezone('UTC')
@@ -967,7 +971,7 @@
         self.assertIs(None, self.error_utility.getLastOopsReport())
 
 
-class Test404Oops(testtools.TestCase):
+class TestOopsIgnoring(testtools.TestCase):
 
     def test_offsite_404_ignored(self):
         # A request originating from another site that generates a NotFound
@@ -990,6 +994,92 @@
         request = dict()
         self.assertTrue(utility._isIgnoredException('NotFound', request))
 
+    def test_marked_exception_is_ignored(self):
+        # If an exception has been marked as ignorable, then it is ignored.
+        utility = ErrorReportingUtility()
+        exception = Exception()
+        directlyProvides(exception, IUnloggedException)
+        self.assertTrue(
+            utility._isIgnoredException('RuntimeError', exception=exception))
+
+    def test_unmarked_exception_generates_oops(self):
+        # If an exception has not been marked as ignorable, then it is not.
+        utility = ErrorReportingUtility()
+        exception = Exception()
+        self.assertFalse(
+            utility._isIgnoredException('RuntimeError', exception=exception))
+
+
+class TestWrappedParameterConverter(testtools.TestCase):
+    """Make sure URL parameter type conversions don't generate OOPS reports"""
+
+    def test_return_value_untouched(self):
+        # When a converter succeeds, its return value is passed through the
+        # wrapper untouched.
+
+        class FauxZopePublisherBrowserModule:
+            def get_converter(self, type_):
+                def the_converter(value):
+                    return 'converted %r to %s' % (value, type_)
+                return the_converter
+
+        module = FauxZopePublisherBrowserModule()
+        customize_get_converter(module)
+        converter = module.get_converter('int')
+        self.assertEqual("converted '42' to int", converter('42'))
+
+    def test_value_errors_marked(self):
+        # When a ValueError is raised by the wrapped converter, the exception
+        # is marked with IUnloggedException so the OOPS machinery knows that a
+        # report should not be logged.
+
+        class FauxZopePublisherBrowserModule:
+            def get_converter(self, type_):
+                def the_converter(value):
+                    raise ValueError
+                return the_converter
+
+        module = FauxZopePublisherBrowserModule()
+        customize_get_converter(module)
+        converter = module.get_converter('int')
+        try:
+            converter(42)
+        except ValueError, e:
+            self.assertTrue(IUnloggedException.providedBy(e))
+
+    def test_other_errors_not_marked(self):
+        # When an exception other than ValueError is raised by the wrapped
+        # converter, the exception is not marked with IUnloggedException an
+        # OOPS report will be created.
+
+        class FauxZopePublisherBrowserModule:
+            def get_converter(self, type_):
+                def the_converter(value):
+                    raise RuntimeError
+                return the_converter
+
+        module = FauxZopePublisherBrowserModule()
+        customize_get_converter(module)
+        converter = module.get_converter('int')
+        try:
+            converter(42)
+        except RuntimeError, e:
+            self.assertFalse(IUnloggedException.providedBy(e))
+
+    def test_none_is_not_wrapped(self):
+        # The get_converter function that we're wrapping can return None, in
+        # that case there's no function for us to wrap and we just return None
+        # as well.
+
+        class FauxZopePublisherBrowserModule:
+            def get_converter(self, type_):
+                return None
+
+        module = FauxZopePublisherBrowserModule()
+        customize_get_converter(module)
+        converter = module.get_converter('int')
+        self.assertTrue(converter is None)
+
 
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp_sitecustomize.py'
--- lib/lp_sitecustomize.py	2011-04-05 12:41:25 +0000
+++ lib/lp_sitecustomize.py	2011-06-15 14:58:50 +0000
@@ -16,12 +16,15 @@
     )
 
 from bzrlib.branch import Branch
+from canonical.launchpad.webapp.interfaces import IUnloggedException
 from lp.services.log import loglevels
 from lp.services.log.logger import LaunchpadLogger
 from lp.services.log.mappingfilter import MappingFilter
 from lp.services.log.nullhandler import NullHandler
 from lp.services.mime import customizeMimetypes
+from zope.interface import alsoProvides
 from zope.security import checker
+import zope.publisher.browser
 
 
 def add_custom_loglevels():
@@ -136,6 +139,37 @@
     silence_transaction_logger()
 
 
+def customize_get_converter(zope_publisher_browser=zope.publisher.browser):
+    """URL parameter conversion errors shouldn't generate an OOPS report.
+
+    This injects (monkey patches) our wrapper around get_converter so improper
+    use of parameter type converters (like http://...?foo=bar:int) won't
+    generate OOPS reports.
+    """
+    original_get_converter = zope_publisher_browser.get_converter
+
+    def get_converter(*args, **kws):
+        """Get a type converter but turn off OOPS reporting if it fails."""
+        converter = original_get_converter(*args, **kws)
+
+        def wrapped_converter(v):
+            try:
+                return converter(v)
+            except ValueError, e:
+                # Mark the exception as not being OOPS-worthy.
+                alsoProvides(e, IUnloggedException)
+                raise
+
+        # The converter can be None, in which case wrapping it makes no sense,
+        # otherwise it is a function which we wrap.
+        if converter is None:
+            return None
+        else:
+            return wrapped_converter
+
+    zope_publisher_browser.get_converter = get_converter
+
+
 def main(instance_name):
     # This is called by our custom buildout-generated sitecustomize.py
     # in parts/scripts/sitecustomize.py. The instance name is sent to
@@ -161,3 +195,4 @@
     checker.BasicTypes[grouper] = checker._iteratorChecker
     silence_warnings()
     customize_logger()
+    customize_get_converter()