launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/basemailer-permissive-only into lp:launchpad.

Commit message:
Require users of BaseMailer to use a permissive security policy.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/basemailer-permissive-only/+merge/270966

It's very easy for code that sends mail notifications to end up crashing when a private team that the actor cannot see is subscribed to the modified object.  To avoid this, it must either make lots of very careful removeSecurityProxy calls, or run with a permissive security policy.  The latter is much easier to get right.  Therefore, require all users of BaseMailer to use a permissive security policy, now that they all comply with this.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/basemailer-permissive-only into lp:launchpad.

=== modified file 'lib/lp/registry/tests/test_teammembership.py'
--- lib/lp/registry/tests/test_teammembership.py	2015-09-02 02:46:18 +0000
+++ lib/lp/registry/tests/test_teammembership.py	2015-09-14 14:25:20 +0000
@@ -82,6 +82,7 @@
     DatabaseFunctionalLayer,
     DatabaseLayer,
     LaunchpadZopelessLayer,
+    ZopelessDatabaseLayer,
     )
 from lp.testing.mail_helpers import pop_notifications
 from lp.testing.matchers import HasQueryCount
@@ -1070,7 +1071,7 @@
 
 class TestTeamMembershipSendExpirationWarningEmail(TestCaseWithFactory):
     """Test the behaviour of sendExpirationWarningEmail()."""
-    layer = DatabaseFunctionalLayer
+    layer = ZopelessDatabaseLayer
 
     def setUp(self):
         super(TestTeamMembershipSendExpirationWarningEmail, self).setUp()

=== modified file 'lib/lp/services/mail/basemailer.py'
--- lib/lp/services/mail/basemailer.py	2015-09-11 12:20:23 +0000
+++ lib/lp/services/mail/basemailer.py	2015-09-14 14:25:20 +0000
@@ -14,6 +14,7 @@
 
 from zope.component import getUtility
 from zope.error.interfaces import IErrorReportingUtility
+from zope.security.management import getSecurityPolicy
 
 from lp.services.mail.helpers import get_email_template
 from lp.services.mail.mailwrapper import MailWrapper
@@ -24,6 +25,7 @@
     MailController,
     )
 from lp.services.utils import text_delta
+from lp.services.webapp.authorization import LaunchpadPermissiveSecurityPolicy
 
 
 class BaseMailer:
@@ -60,6 +62,20 @@
         :param wrap: Wrap body text using `MailWrapper`.
         :param force_wrap: See `MailWrapper.format`.
         """
+        # Running mail notifications with web security is too fragile: it's
+        # easy to end up with subtle bugs due to such things as
+        # subscriptions from private teams that are inaccessible to the user
+        # with the current interaction.  BaseMailer always sends one mail
+        # per recipient and thus never leaks information to other users, so
+        # it's safer to require a permissive security policy.
+        #
+        # When converting other notification code to BaseMailer, it may be
+        # necessary to move notifications into jobs, to move unit tests to a
+        # Zopeless-based layer, or to use the permissive_security_policy
+        # context manager.
+        assert getSecurityPolicy() == LaunchpadPermissiveSecurityPolicy, (
+            "BaseMailer may only be used with a permissive security policy.")
+
         self._subject_template = subject
         self._template_name = template_name
         self._recipients = NotificationRecipientSet()

=== modified file 'lib/lp/testing/mail_helpers.py'
--- lib/lp/testing/mail_helpers.py	2015-09-11 12:20:23 +0000
+++ lib/lp/testing/mail_helpers.py	2015-09-14 14:25:20 +0000
@@ -22,7 +22,6 @@
 from lp.services.job.runner import JobRunner
 from lp.services.log.logger import DevNullLogger
 from lp.services.mail import stub
-from lp.testing.dbuser import dbuser
 
 
 def pop_notifications(sort_key=None, commit=True):
@@ -137,6 +136,9 @@
     extended to run those jobs, so that testing emails doesn't require a
     bunch of different function calls to process different queues.
     """
+    # Circular import.
+    from lp.testing.pages import permissive_security_policy
+
     # Commit the transaction to make sure that the JobRunner can find
     # the queued jobs.
     transaction.commit()
@@ -149,6 +151,7 @@
             ):
         job_source = getUtility(interface)
         logger = DevNullLogger()
-        with dbuser(getattr(config, interface.__name__).dbuser):
+        dbuser_name = getattr(config, interface.__name__).dbuser
+        with permissive_security_policy(dbuser_name):
             runner = JobRunner.fromReady(job_source, logger)
             runner.runAll()