Merge ~jugmac00/launchpad:prevent-email-disclosure into launchpad:master

[Jürgen Gmach]

Prevent email address disclosure for mirror notifications

This is WIP:

There are a couple of more possibly problematic spots.

https://git.launchpad.net/launchpad/tree/lib/lp/soyuz/mail/binarypackagebuild.py#n166

https://git.launchpad.net/launchpad/tree/lib/lp/translations/scripts/po_import.py#n145

https://git.launchpad.net/launchpad/tree/lib/lp/translations/scripts/translations_to_branch.py#n297

[Colin Watson (cjwatson)]

This looks good as far as it goes.

https://git.launchpad.net/launchpad/tree/lib/lp/soyuz/mail/binarypackagebuild.py#n166 isn't a problem: an SPR's creator is a single person.

The importer in https://git.launchpad.net/launchpad/tree/lib/lp/translations/scripts/po_import.py#n145 sometimes ends up being ~rosetta-admins, which is a large team, so let's add a loop there.

https://git.launchpad.net/launchpad/tree/lib/lp/translations/scripts/translations_to_branch.py#n297 is marginal, but I'd be inclined to leave it alone, at least for now; I think the case where a team explicitly owns something is a bit different from the mirror-admins and rosetta-admins cases where the ownership is rather more, um, ambient.

[Jürgen Gmach (jugmac00)]

Thanks, Colin!

I updated the code in `po_import.py` accordingly.

The MP is ready for review.

[Colin Watson (cjwatson)]

review: Approve

diff --git a/lib/lp/registry/model/distributionmirror.py b/lib/lp/registry/model/distributionmirror.py
index 8d3bd82..2454187 100644
--- a/lib/lp/registry/model/distributionmirror.py
+++ b/lib/lp/registry/model/distributionmirror.py
@@ -385,13 +385,14 @@ class DistributionMirror(SQLBase):
         message = template % replacements
         subject = "Launchpad: Verification of %s failed" % self.name
 
-        mirror_admin_address = get_contact_email_addresses(
+        mirror_admin_addresses = get_contact_email_addresses(
             self.distribution.mirror_admin)
-        simple_sendmail(fromaddress, mirror_admin_address, subject, message)
+        for admin_address in mirror_admin_addresses:
+            simple_sendmail(fromaddress, admin_address, subject, message)
 
         if notify_owner:
-            owner_address = get_contact_email_addresses(self.owner)
-            if len(owner_address) > 0:
+            owner_addresses = get_contact_email_addresses(self.owner)
+            for owner_address in owner_addresses:
                 simple_sendmail(fromaddress, owner_address, subject, message)
 
     def newProbeRecord(self, log_file):
diff --git a/lib/lp/registry/tests/test_distributionmirror.py b/lib/lp/registry/tests/test_distributionmirror.py
index 08dae43..8f1423d 100644
--- a/lib/lp/registry/tests/test_distributionmirror.py
+++ b/lib/lp/registry/tests/test_distributionmirror.py
@@ -170,14 +170,22 @@ class TestDistributionMirror(TestCaseWithFactory):
         mirror.disable(notify_owner=True, log=log)
         # A notification was sent to the owner and other to the mirror admins.
         transaction.commit()
-        self.assertEqual(len(stub.test_emails), 2)
+        self.assertEqual(len(stub.test_emails), 3)
+
+        # In order to prevent data disclosure, emails have to be sent to one
+        # person each, ie it is not allowed to have multiple recipients in an
+        # email's `to` field.
+        for email in stub.test_emails:
+            number_of_to_addresses = len(email[1])
+            self.assertLess(number_of_to_addresses, 2)
+
         stub.test_emails = []
 
         mirror.disable(notify_owner=True, log=log)
         # Again, a notification was sent to the owner and other to the mirror
         # admins.
         transaction.commit()
-        self.assertEqual(len(stub.test_emails), 2)
+        self.assertEqual(len(stub.test_emails), 3)
         stub.test_emails = []
 
         # For mirrors that have been probed more than once, we'll only notify
@@ -187,7 +195,7 @@ class TestDistributionMirror(TestCaseWithFactory):
         mirror.disable(notify_owner=True, log=log)
         # A notification was sent to the owner and other to the mirror admins.
         transaction.commit()
-        self.assertEqual(len(stub.test_emails), 2)
+        self.assertEqual(len(stub.test_emails), 3)
         stub.test_emails = []
 
         # We can always disable notifications to the owner by passing
@@ -195,7 +203,7 @@ class TestDistributionMirror(TestCaseWithFactory):
         mirror.enabled = True
         mirror.disable(notify_owner=False, log=log)
         transaction.commit()
-        self.assertEqual(len(stub.test_emails), 1)
+        self.assertEqual(len(stub.test_emails), 2)
         stub.test_emails = []
 
         mirror.enabled = False
@@ -217,12 +225,12 @@ class TestDistributionMirror(TestCaseWithFactory):
         transaction.commit()
         stub.test_emails = []
 
-        # Disabling the mirror results in a single notification to the
-        # mirror admins.
+        # Disabling the mirror results in one notification for each of
+        # the three mirror admins.
         self.factory.makeMirrorProbeRecord(mirror)
         mirror.disable(notify_owner=True, log="It broke.")
         transaction.commit()
-        self.assertEqual(len(stub.test_emails), 1)
+        self.assertEqual(len(stub.test_emails), 3)
 
     def test_permissions_for_resubmit(self):
         self.assertRaises(
diff --git a/lib/lp/translations/scripts/po_import.py b/lib/lp/translations/scripts/po_import.py
index 0cb9200..8b7d9b7 100644
--- a/lib/lp/translations/scripts/po_import.py
+++ b/lib/lp/translations/scripts/po_import.py
@@ -140,11 +140,8 @@ class TranslationsImport(LaunchpadCronScript):
             katie = getUtility(ILaunchpadCelebrities).katie
             if entry.importer == katie:
                 # Email import state to Debian imports email.
-                to_email = None
-            else:
-                to_email = get_contact_email_addresses(entry.importer)
-
-            if to_email:
+                return
+            for to_email in get_contact_email_addresses(entry.importer):
                 text = MailWrapper().format(mail_body)
                 simple_sendmail(from_email, to_email, mail_subject, text)
 
diff --git a/lib/lp/translations/scripts/tests/test_translations_import.py b/lib/lp/translations/scripts/tests/test_translations_import.py
index 07d8776..ffc61fd 100644
--- a/lib/lp/translations/scripts/tests/test_translations_import.py
+++ b/lib/lp/translations/scripts/tests/test_translations_import.py
@@ -188,6 +188,25 @@ class TestTranslationsImport(TestCaseWithFactory):
         self.assertEqual(
             [self.owner.preferredemail.email], self._getEmailRecipients())
 
+    def test_notifies_uploader_separately(self):
+        """Do not put several addresses into one `to`-field"""
+        p1 = self.factory.makePerson()
+        p2 = self.factory.makePerson()
+        uploader = self.factory.makeTeam(
+            members=[p1, p2],
+        )
+        entry = self._makeApprovedEntry(uploader=uploader)
+        transaction.commit()
+        self.script._importEntry(entry)
+        transaction.commit()
+
+        # three emails get generated
+        # and each has only one recipient
+        self.assertEqual(3, len(stub.test_emails))
+        for email in stub.test_emails:
+            number_of_to_addresses = len(email[1])
+            self.assertEqual(1, number_of_to_addresses)
+
     def test_does_not_notify_vcs_imports(self):
         vcs_imports = getUtility(ILaunchpadCelebrities).vcs_imports
         entry = self._makeApprovedEntry(vcs_imports)