launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-1489674 into lp:launchpad.

Commit message:
Fix BugNotificationBuilder to always removeSecurityProxy before it touches a potentially private person.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1489674 in Launchpad itself: "BugNotificationBuilder.build crashes with Unauthorized on invisible private teams"
  https://bugs.launchpad.net/launchpad/+bug/1489674

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-1489674/+merge/269480

BugNotificationBuilder.build has to handle invisible private teams, and already uses removeSecurityProxy in places, but it fails to bypass the proxy when retrieving expanded_notification_footers. See OOPS-cf057b82417ad315c3b7a858ca8237ba for an example.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-1489674 into lp:launchpad.

=== modified file 'lib/lp/bugs/mail/bugnotificationbuilder.py'
--- lib/lp/bugs/mail/bugnotificationbuilder.py	2015-07-31 14:46:31 +0000
+++ lib/lp/bugs/mail/bugnotificationbuilder.py	2015-08-28 09:05:40 +0000
@@ -215,7 +215,7 @@
         # XXX cjwatson 2015-07-31: This is cloned-and-hacked from
         # BaseMailer; it would ultimately be better to convert bug
         # notifications to that framework.
-        if to_person.expanded_notification_footers:
+        if removeSecurityProxy(to_person).expanded_notification_footers:
             lines = []
             for key, value in headers:
                 if key.startswith('X-Launchpad-'):

=== modified file 'lib/lp/bugs/mail/tests/test_bugnotificationbuilder.py'
--- lib/lp/bugs/mail/tests/test_bugnotificationbuilder.py	2015-07-31 14:46:31 +0000
+++ lib/lp/bugs/mail/tests/test_bugnotificationbuilder.py	2015-08-28 09:05:40 +0000
@@ -6,16 +6,21 @@
 from datetime import datetime
 
 import pytz
+from zope.security.interfaces import Unauthorized
 
 from lp.bugs.mail.bugnotificationbuilder import BugNotificationBuilder
-from lp.testing import TestCaseWithFactory
-from lp.testing.layers import ZopelessDatabaseLayer
+from lp.registry.enums import PersonVisibility
+from lp.testing import (
+    person_logged_in,
+    TestCaseWithFactory,
+    )
+from lp.testing.layers import DatabaseFunctionalLayer
 
 
 class TestBugNotificationBuilder(TestCaseWithFactory):
     """Test emails sent when subscribed by someone else."""
 
-    layer = ZopelessDatabaseLayer
+    layer = DatabaseFunctionalLayer
 
     def setUp(self):
         # Run the tests as a logged-in user.
@@ -71,9 +76,26 @@
         # Recipients with expanded_notification_footers receive an expanded
         # footer on messages.
         utc_now = datetime.now(pytz.UTC)
-        self.bug.owner.expanded_notification_footers = True
+        with person_logged_in(self.bug.owner):
+            self.bug.owner.expanded_notification_footers = True
         message = self.builder.build(
             'from', self.bug.owner, 'body', 'subject', utc_now, filters=[])
         self.assertIn(
             "\n-- \nLaunchpad-Notification-Type: bug\n",
             message.get_payload(decode=True))
+
+    def test_private_team(self):
+        # Recipients can be invisible private teams, as
+        # BugNotificationBuilder runs in the context of the user making
+        # the change. They work fine.
+        private_team = self.factory.makeTeam(
+            visibility=PersonVisibility.PRIVATE, email="private@xxxxxxxxxxx")
+        random = self.factory.makePerson()
+        with person_logged_in(random):
+            self.assertRaises(
+                Unauthorized, getattr, private_team,
+                'expanded_notification_footers')
+            utc_now = datetime.now(pytz.UTC)
+            message = self.builder.build(
+                'from', private_team, 'body', 'subject', utc_now, filters=[])
+        self.assertIn("private@xxxxxxxxxxx", str(message))