Merge lp:~canonical-isd-hackers/canonical-identity-provider/log-request into lp:canonical-identity-provider/release

[David Owen]

Extracted log_request and helpers from process_exception into new util module.

process_exception customized the behavior of ExceptionReporter by monkey-patching two method references. This patch instead overrides the customized method through inheritance, and no monkey-patching is needed.

Other than that, this simply moves code into the new util module and slightly renames some things.

[Simon Davy (bloodearnest)]

LGTM

=== modified file 'identityprovider/middleware/exception.py'
--- identityprovider/middleware/exception.py	2011-07-06 18:09:03 +0000
+++ identityprovider/middleware/exception.py	2011-12-15 22:40:29 +0000
@@ -1,18 +1,10 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-import logging
-import re
 import sys
 import traceback
 
-from django.conf import settings
-from django.db import connection
-from django.views import debug
-from django.views.debug import ExceptionReporter
-
-debug.HIDDEN_SETTINGS = re.compile(
-    'SECRET|PASSWORD|PROFANITIES_LIST|PRIVATE|secret|password|private')
+from .util import log_request
 
 
 class ConsoleExceptionMiddleware(object):
@@ -22,90 +14,5 @@
 
 class LogExceptionMiddleware(object):
 
-    def get_reporter(self, request, *exc_info):
-        safe = self.sanitize_request(request)
-        reporter = ExceptionReporter(safe, *exc_info)
-        reporter.get_traceback_frames = self.get_traceback_frames
-        self._get_lines_from_file = reporter._get_lines_from_file
-        return reporter
-
     def process_exception(self, request, exception):
-        """Log the traceback, so that it can be kept in the oops file"""
-        reporter = self.get_reporter(request, *sys.exc_info())
-        template_debug = settings.TEMPLATE_DEBUG
-        settings.TEMPLATE_DEBUG = True
-        try:
-            logging.exception('Unhandled exception in application')
-            logging.warn(reporter.get_traceback_html())
-            logging.warn(traceback.format_exc())
-            for query in connection.queries:
-                logging.warn("time: %(time)s sql: %(sql)s" % query)
-        finally:
-            settings.TEMPLATE_DEBUG = template_debug
-
-    def sanitize_request(self, request):
-        """Remove sensitive from the request before it is displayed"""
-        dup_post = request.POST.copy()
-        for key in request.POST.iterkeys():
-            if debug.HIDDEN_SETTINGS.search(key):
-                dup_post[key] = '********'
-        request.POST = dup_post
-        dup_get = request.GET.copy()
-        for key in request.GET.iterkeys():
-            if debug.HIDDEN_SETTINGS.search(key):
-                dup_get[key] = '********'
-        request.GET = dup_get
-        return request
-
-    def sanitize_vars(self, items):
-        sanitized = []
-        for item in items:
-            if debug.HIDDEN_SETTINGS.search(item[0]):
-                sanitized.append((item[0], '********'))
-            else:
-                sanitized.append(item)
-        return sanitized
-
-    # This function was taken from the django project.
-    # Please see the license file in the third-party/django directory.
-    def get_traceback_frames(self):
-        frames = []
-        tb = sys.exc_info()[2]
-        while tb is not None:
-            # support for __traceback_hide__ which is used by a few libraries
-            # to hide internal frames.
-            if tb.tb_frame.f_locals.get('__traceback_hide__'):
-                tb = tb.tb_next
-                continue
-            filename = tb.tb_frame.f_code.co_filename
-            function = tb.tb_frame.f_code.co_name
-            lineno = tb.tb_lineno - 1
-            loader = tb.tb_frame.f_globals.get('__loader__')
-            module_name = tb.tb_frame.f_globals.get('__name__')
-            pre_context_lineno, pre_context, context_line, post_context = \
-                    self._get_lines_from_file(filename, lineno, 7, loader,
-                                              module_name)
-            if pre_context_lineno is not None:
-                frames.append({
-                    'tb': tb,
-                    'filename': filename,
-                    'function': function,
-                    'lineno': lineno + 1,
-                    'vars': self.sanitize_vars(tb.tb_frame.f_locals.items()),
-                    'id': id(tb),
-                    'pre_context': pre_context,
-                    'context_line': context_line,
-                    'post_context': post_context,
-                    'pre_context_lineno': pre_context_lineno + 1,
-                })
-            tb = tb.tb_next
-
-        if not frames:
-            frames = [{
-                'filename': '<unknown>',
-                'function': '?',
-                'lineno': '?',
-                'context_line': '???',
-            }]
-
-        return frames
+        log_request(request)

=== added file 'identityprovider/middleware/util.py'
--- identityprovider/middleware/util.py	1970-01-01 00:00:00 +0000
+++ identityprovider/middleware/util.py	2011-12-15 22:40:29 +0000
@@ -0,0 +1,101 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+import logging
+import re
+import sys
+import traceback
+
+from django.conf import settings
+from django.db import connection
+from django.views import debug
+
+debug.HIDDEN_SETTINGS = re.compile(
+    'SECRET|PASSWORD|PROFANITIES_LIST|PRIVATE|secret|password|private')
+
+
+def _sanitize_dict(dirty):
+    clean = dirty.copy()
+    for key in dirty.iterkeys():
+        if debug.HIDDEN_SETTINGS.search(key):
+            clean[key] = '********'
+    return clean
+
+def _sanitize_request(request):
+    """Remove sensitive information from the request before it is
+    displayed."""
+    request.GET = _sanitize_dict(request.GET)
+    request.POST = _sanitize_dict(request.POST)
+    return request
+
+def _sanitize_vars(items):
+    sanitized = []
+    for item in items:
+        if debug.HIDDEN_SETTINGS.search(item[0]):
+            sanitized.append((item[0], '********'))
+        else:
+            sanitized.append(item)
+    return sanitized
+
+
+class _ExceptionReporter(debug.ExceptionReporter):
+
+    # This function was taken from the django project.
+    # Please see the license file in the third-party/django directory.
+    def get_traceback_frames(self):
+        frames = []
+        tb = sys.exc_info()[2]
+        while tb is not None:
+            # support for __traceback_hide__ which is used by a few libraries
+            # to hide internal frames.
+            if tb.tb_frame.f_locals.get('__traceback_hide__'):
+                tb = tb.tb_next
+                continue
+            filename = tb.tb_frame.f_code.co_filename
+            function = tb.tb_frame.f_code.co_name
+            lineno = tb.tb_lineno - 1
+            loader = tb.tb_frame.f_globals.get('__loader__')
+            module_name = tb.tb_frame.f_globals.get('__name__')
+            pre_context_lineno, pre_context, context_line, post_context = \
+                    self._get_lines_from_file(filename, lineno, 7, loader,
+                                              module_name)
+            if pre_context_lineno is not None:
+                frames.append({
+                    'tb': tb,
+                    'filename': filename,
+                    'function': function,
+                    'lineno': lineno + 1,
+                    'vars': _sanitize_vars(tb.tb_frame.f_locals.items()),
+                    'id': id(tb),
+                    'pre_context': pre_context,
+                    'context_line': context_line,
+                    'post_context': post_context,
+                    'pre_context_lineno': pre_context_lineno + 1,
+                })
+            tb = tb.tb_next
+
+        if not frames:
+            frames = [{
+                'filename': '<unknown>',
+                'function': '?',
+                'lineno': '?',
+                'context_line': '???',
+            }]
+
+        return frames
+
+
+def log_request(request):
+    """Log the request, so that it can be kept in the oops file"""
+    request = _sanitize_request(request)
+    reporter = _ExceptionReporter(request, *sys.exc_info())
+    template_debug = settings.TEMPLATE_DEBUG
+    settings.TEMPLATE_DEBUG = True
+    try:
+        logging.exception('Unhandled exception in application')
+        logging.warn(reporter.get_traceback_html())
+        logging.warn(traceback.format_exc())
+        for query in connection.queries:
+            logging.warn("time: %(time)s sql: %(sql)s" % query)
+    finally:
+        settings.TEMPLATE_DEBUG = template_debug

=== modified file 'identityprovider/tests/test_middleware.py'
--- identityprovider/tests/test_middleware.py	2011-08-31 13:37:21 +0000
+++ identityprovider/tests/test_middleware.py	2011-12-15 22:40:29 +0000
@@ -13,7 +13,7 @@
 
 from identityprovider.tests.utils import MockRequest
 from identityprovider.models import Account, EmailAddress, OpenIDRPConfig
-from identityprovider.middleware.exception import LogExceptionMiddleware
+from identityprovider.middleware import exception, util as middleware_util
 from identityprovider.middleware.useraccount import (
     UserAccountConversionMiddleware)
 
@@ -107,7 +107,7 @@
         request.GET['get_private'] = self.GET_PRIVATE
         request.GET['get_public'] = self.GET_PUBLIC
         self.request = request
-        self.middleware = LogExceptionMiddleware()
+        self.middleware = exception.LogExceptionMiddleware()
         settings.PRIVATE_SETTING = self.PRIVATE_SETTING
         settings.PUBLIC_SETTING = self.PUBLIC_SETTING
 
@@ -115,8 +115,8 @@
         try:
             x = 2 / 0
         except:
-            reporter = self.middleware.get_reporter(self.request,
-                                                    *sys.exc_info())
+            reporter = middleware_util._ExceptionReporter(self.request,
+                                                          *sys.exc_info())
             html = reporter.get_traceback_html()
         self.assertFalse(self.PRIVATE_SETTING in html)
         self.assertTrue(self.PUBLIC_SETTING in html)
@@ -126,14 +126,14 @@
             FRAME_PASSWORD = self.FRAME_VAR
             public_variable = 2 / 0
         except:
-            reporter = self.middleware.get_reporter(self.request,
-                                                    *sys.exc_info())
+            reporter = middleware_util._ExceptionReporter(self.request,
+                                                          *sys.exc_info())
             html = reporter.get_traceback_html()
         self.assertFalse(self.FRAME_VAR in html)
         self.assertTrue('public_variable' in html)
 
     def test_request_sanitization(self):
-        req = self.middleware.sanitize_request(self.request)
+        req = middleware_util._sanitize_request(self.request)
         #  Make sure sensitive data is no longer in the request
         self.assertFalse(self.POST_PASSWORD in req.POST.values())
         self.assertFalse(self.POST_SECRET in req.POST.values())