launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:fix-timeout-oops-id into launchpad:master.

Commit message:
Cache Person.name for attach_http_request

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/377401

Commit e6db69425adec2ce77d67d210b6bf1cead298bb9 introduced a regression: if the request timed out, then the attempt to fetch principal.person.name in attach_http_request could itself raise RequestExpired if the relevant property wasn't already cached, causing the OOPS to be lost.

Instead, we cache Person.name on LaunchpadPrincipal earlier in the request, allowing us to use it safely while building an OOPS report.

Testing this also involves lifting TestErrorReportingUtility up to LaunchpadZopelessLayer, but I think that's a reasonable price to pay for making it more realistic.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:fix-timeout-oops-id into launchpad:master.

diff --git a/lib/lp/services/webapp/authentication.py b/lib/lp/services/webapp/authentication.py
index e9223c0..c48bb33 100644
--- a/lib/lp/services/webapp/authentication.py
+++ b/lib/lp/services/webapp/authentication.py
@@ -264,9 +264,14 @@ class LaunchpadPrincipal:
         self.scope_url = scope_url
         self.account = account
         self.person = IPerson(account, None)
+        self.person_name = (
+            self.person.name if self.person is not None else None)
 
     def getLogin(self):
-        return self.title
+        if self.person_name is not None:
+            return self.person_name
+        else:
+            return self.title
 
 
 def get_oauth_authorization(request):
diff --git a/lib/lp/services/webapp/errorlog.py b/lib/lp/services/webapp/errorlog.py
index ad63464..cdb99c3 100644
--- a/lib/lp/services/webapp/errorlog.py
+++ b/lib/lp/services/webapp/errorlog.py
@@ -26,7 +26,6 @@ from zope.event import notify
 from zope.exceptions.exceptionformatter import format_exception
 from zope.interface import implementer
 from zope.publisher.interfaces.xmlrpc import IXMLRPCRequest
-from zope.security.proxy import removeSecurityProxy
 from zope.traversing.namespace import view
 
 from lp.app import versioninfo
@@ -41,7 +40,6 @@ from lp.services.webapp.adapter import (
 from lp.services.webapp.interfaces import (
     IErrorReportEvent,
     IErrorReportRequest,
-    ILaunchpadPrincipal,
     IUnloggedException,
     )
 from lp.services.webapp.opstats import OpStats
@@ -180,13 +178,7 @@ def attach_http_request(report, context):
     missing = object()
     principal = getattr(request, 'principal', missing)
 
-    person = (
-        removeSecurityProxy(principal.person)
-        if ILaunchpadPrincipal.providedBy(principal)
-        else None)
-    if person is not None:
-        login = person.name
-    elif safe_hasattr(principal, 'getLogin'):
+    if safe_hasattr(principal, 'getLogin'):
         login = principal.getLogin()
     elif principal is missing or principal is None:
         # Request has no principal (e.g. scriptrequest)
diff --git a/lib/lp/services/webapp/interfaces.py b/lib/lp/services/webapp/interfaces.py
index 4f87817..6134afe 100644
--- a/lib/lp/services/webapp/interfaces.py
+++ b/lib/lp/services/webapp/interfaces.py
@@ -593,6 +593,9 @@ class ILaunchpadPrincipal(IPrincipal):
 
     person = Attribute("The IPerson the principal represents.")
 
+    person_name = Attribute(
+        "The name of the IPerson the principal represents.")
+
 
 #
 # Browser notifications
diff --git a/lib/lp/services/webapp/tests/test_errorlog.py b/lib/lp/services/webapp/tests/test_errorlog.py
index ad1437c..7067684 100644
--- a/lib/lp/services/webapp/tests/test_errorlog.py
+++ b/lib/lp/services/webapp/tests/test_errorlog.py
@@ -17,11 +17,8 @@ import oops_amqp
 import pytz
 import testtools
 from timeline.timeline import Timeline
-from zope.authentication.interfaces import IUnauthenticatedPrincipal
-from zope.interface import (
-    directlyProvides,
-    implementer,
-    )
+from zope.interface import directlyProvides
+from zope.principalregistry.principalregistry import UnauthenticatedPrincipal
 from zope.publisher.browser import TestRequest
 from zope.publisher.interfaces import NotFound
 from zope.publisher.interfaces.xmlrpc import IXMLRPCRequest
@@ -32,9 +29,9 @@ from lp.app.errors import (
     TranslationUnavailable,
     )
 from lp.layers import WebServiceLayer
-from lp.registry.interfaces.person import IPerson
-from lp.registry.interfaces.role import IPersonRoles
 from lp.services.config import config
+from lp.services.database.sqlbase import flush_database_caches
+from lp.services.webapp.authentication import LaunchpadPrincipal
 from lp.services.webapp.errorlog import (
     _filter_session_statement,
     _is_sensitive,
@@ -44,12 +41,11 @@ from lp.services.webapp.errorlog import (
     ScriptRequest,
     )
 from lp.services.webapp.interfaces import (
-    ILaunchpadPrincipal,
     IUnloggedException,
     NoReferrerError,
     )
-from lp.testing import TestCase
-from lp.testing.layers import LaunchpadLayer
+from lp.testing import TestCaseWithFactory
+from lp.testing.layers import LaunchpadZopelessLayer
 
 
 UTC = pytz.utc
@@ -59,10 +55,10 @@ class ArbitraryException(Exception):
     """Used to test handling of exceptions in OOPS reports."""
 
 
-class TestErrorReportingUtility(TestCase):
+class TestErrorReportingUtility(TestCaseWithFactory):
 
     # want rabbit
-    layer = LaunchpadLayer
+    layer = LaunchpadZopelessLayer
 
     def setUp(self):
         super(TestErrorReportingUtility, self).setUp()
@@ -152,8 +148,9 @@ class TestErrorReportingUtility(TestCase):
 
         # topic is obtained from the request
         self.assertEqual('IFoo:+foo-template', report['topic'])
-        self.assertEqual(u'Login, 42, title, description |\u25a0|',
-                report['username'])
+        self.assertEqual(
+            u'account-name, 42, account-name, description |\u25a0|',
+            report['username'])
         self.assertEqual('http://localhost:9000/foo', report['url'])
         self.assertEqual({
             'CONTENT_LENGTH': '0',
@@ -174,19 +171,21 @@ class TestErrorReportingUtility(TestCase):
         utility = ErrorReportingUtility()
         utility._main_publishers[0].__call__ = lambda report: []
 
-        request = TestRequestWithPrincipal()
+        # Attach a person to the request; the report uses their name.
+        person = self.factory.makePerson(name='my-username')
+        request = TestRequestWithPrincipal(account=person.account)
 
-        # Set a fake person attached to the request.
-        # Report should use their name (instead of principal.getLogin)
-        request.principal.person = FakePerson()
-        request.principal.person.name = 'my_username'
+        # Make sure Storm would have to reload person.name if it were used.
+        flush_database_caches()
 
         try:
             raise ArbitraryException('xyz\nabc')
         except ArbitraryException:
-            report = utility.raising(sys.exc_info(), request)
-        self.assertEqual(u'my_username, 42, title, description |\u25a0|',
-                         report['username'])
+            report = self.assertStatementCount(
+                0, utility.raising, sys.exc_info(), request)
+        self.assertEqual(
+            u'my-username, 42, account-name, description |\u25a0|',
+            report['username'])
 
     def test_raising_request_with_principal_person_set_to_none(self):
         """
@@ -200,17 +199,17 @@ class TestErrorReportingUtility(TestCase):
 
         request = TestRequestWithPrincipal()
 
-        # Explicitly sets principal.person to None,
-        # and expects OOPS to fallback to getLogin method (which should
-        # handle this case)
+        # Explicitly sets principal.person to None; the report falls back to
+        # the account's display name.
         request.principal.person = None
 
         try:
             raise ArbitraryException('xyz\nabc')
         except ArbitraryException:
             report = utility.raising(sys.exc_info(), request)
-        self.assertEqual(u'Login, 42, title, description |\u25a0|',
-                         report['username'])
+        self.assertEqual(
+            u'account-name, 42, account-name, description |\u25a0|',
+            report['username'])
 
     def test_raising_with_xmlrpc_request(self):
         # Test ErrorReportingUtility.raising() with an XML-RPC request.
@@ -578,39 +577,19 @@ class TestSensitiveRequestVariables(testtools.TestCase):
         self.assertTrue(_is_sensitive(request, 'oauth_signature'))
 
 
-@implementer(IUnauthenticatedPrincipal)
-class UnauthenticatedPrincipal:
-    id = 0
-    title = ''
-    description = ''
-
-
 class TestRequestWithUnauthenticatedPrincipal(TestRequest):
-    principal = UnauthenticatedPrincipal()
-
-
-class FakePerson:
-    """Used to test attach_http_request"""
-
-
-@implementer(ILaunchpadPrincipal)
-class AuthenticatedPrincipal:
-    def __init__(self):
-        self.id = 42
-        self.title = u'title'
-        # non ASCII description
-        self.description = u'description |\N{BLACK SQUARE}|'
-        self.person = None
-
-    @staticmethod
-    def getLogin():
-        return u'Login'
+    principal = UnauthenticatedPrincipal(
+        u'Anonymous', u'Anonymous', u'Anonymous User')
 
 
 class TestRequestWithPrincipal(TestRequest):
-    def __init__(self, *args, **kw):
+    def __init__(self, account=None, *args, **kw):
         super(TestRequestWithPrincipal, self).__init__(*args, **kw)
-        self.setPrincipal(AuthenticatedPrincipal())
+        self.setPrincipal(LaunchpadPrincipal(
+            42, u'account-name',
+            # non-ASCII description
+            u'description |\N{BLACK SQUARE}|',
+            account))
 
     def setInWSGIEnvironment(self, key, value):
         self._orig_env[key] = value
diff --git a/lib/lp/services/webapp/tests/test_launchpad_login_source.txt b/lib/lp/services/webapp/tests/test_launchpad_login_source.txt
index c00cebb..ff9e54d 100644
--- a/lib/lp/services/webapp/tests/test_launchpad_login_source.txt
+++ b/lib/lp/services/webapp/tests/test_launchpad_login_source.txt
@@ -28,12 +28,16 @@ The corresponding Account and Person records are also in the
 principal, to make it easier for the security machinery to get at
 it. It needs to get it quite often, so having the Person record
 directly on the Principal improves performance quite a lot, even
-compared to getting it from storm's cache.
+compared to getting it from storm's cache. We also store the person's name
+so that OOPS handlers can use it without needing to make another database
+query (which may be impossible due to a timeout).
 
     >>> principal.account == account
     True
     >>> principal.person == person
     True
+    >>> principal.person_name == person.name
+    True
 
 The principal's account and person are security proxied.