Merge lp:~jcsackett/launchpad/anonymous-api-access-emails-681815 into lp:launchpad

[j.c.sackett]

[r=benji,edwin-grubbs][ui=none][bug=681815] Patches a hole in the API allowing anonymous gathering of emails.

Summary
=======
The webservice currently has a hole through which a person can anonymously collect email addresses. This is because the security check employed for it allows email addresses to be returned if they're attached to a user. To be consistent with the web interface, you shouldn't get email addresses unless you're logged in.

This branch changes the unauthenticated check so that it never allows emails to be returned.

Proposed fix
============
Change the ViewEmailAddress checkUnauthenticated to always return false; there is never a circumstance in which the API should anonymously return emails.

Preimplementation talk
======================
Spoke with Curtis Hovey.

Implementation details
======================
As in Proposed.

Tests
=====
bin/test -t registry.*webservice.*xx-person\.txt

Demo & QA
=========
There are several scripts attached to the bug that should pass if this whole is patched.

Lint
====
make lint output:

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/security.py
  lib/lp/registry/stories/webservice/xx-person.txt

./lib/canonical/launchpad/security.py
     733: E302 expected 2 blank lines, found 1
    1266: E302 expected 2 blank lines, found 1
    1476: E302 expected 2 blank lines, found 1
./lib/lp/registry/stories/webservice/xx-person.txt
      14: want exceeds 78 characters.
      16: want exceeds 78 characters.
      20: want exceeds 78 characters.
      38: want exceeds 78 characters.
      41: want exceeds 78 characters.
      44: want exceeds 78 characters.
      61: want exceeds 78 characters.
      63: want exceeds 78 characters.
      67: want exceeds 78 characters.
      69: want exceeds 78 characters.
      72: want exceeds 78 characters.
      86: want exceeds 78 characters.
      87: want exceeds 78 characters.
      90: want exceeds 78 characters.
      93: want exceeds 78 characters.
      95: want exceeds 78 characters.
     161: source exceeds 78 characters.
     162: source exceeds 78 characters.
     163: want has trailing whitespace.
     191: narrative exceeds 78 characters.
     266: source exceeds 78 characters.
     349: narrative has trailing whitespace.
     359: narrative has trailing whitespace.
     482: source exceeds 78 characters.
     506: source exceeds 78 characters.
     569: source exceeds 78 characters.
     822: source has bad indentation.

The blank line error in security.py is because of lint's issues with comments and blanklines.

The wants over 78 lines don't appear to be something that can easily be moved about; they are arguably more readable in their current form.

[Benji York (benji)]

This branch looks good. The reST-ification of the section headings is a nice touch.

As for the long "want" lines, the ones I looked at where of the form "foo: bar\n", so could be wrapped to "foo:\n bar\n" (i.e., line break at the colon and indent the next line) and still retain their readability. However, I agree that their current arrangement isn't too bad either.

review: Approve (code*)

[j.c.sackett (jcsackett)]

I've pushed up a change which makes this a better test, but it's still going to pass in either event; oddly, while in test mode, this security hole doesn't seem to exist.

I encourage any thoughts as to how to deal with this; otherwise, I think the tests show this patch hasn't broken anything, and the scripts attached to the bug can be used to QA it once it's live on qastaging.

[j.c.sackett (jcsackett)]

The big change is the new tests. I've attached a diff of just those.

[Edwin Grubbs (edwin-grubbs)]

Hi JC,

Thanks for adding tests that actually fail without your changes to checkUnauthenticated(). I like benji's suggestion to wrap lines in the doctests to make lint complain less.

-Edwin

review: Approve (code)

[j.c.sackett (jcsackett)]

Edwin, Benji--

I've made the changes to make lint happy.

Thanks for the review, guys!

=== modified file 'lib/canonical/launchpad/browser/tests/test_logintoken.py'
--- lib/canonical/launchpad/browser/tests/test_logintoken.py	2010-10-04 19:50:45 +0000
+++ lib/canonical/launchpad/browser/tests/test_logintoken.py	2010-12-13 13:34:01 +0000
@@ -4,6 +4,7 @@
 import unittest
 
 from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
 
 from canonical.launchpad.browser.logintoken import (
     ClaimTeamView,
@@ -29,7 +30,7 @@
     def setUp(self):
         TestCaseWithFactory.setUp(self)
         self.person = self.factory.makePerson(name='test-user')
-        self.email = self.person.preferredemail.email
+        self.email = removeSecurityProxy(self.person).preferredemail.email
         self.expected_next_url = 'http://127.0.0.1/~test-user'
 
     def test_ClaimTeamView(self):

=== modified file 'lib/canonical/launchpad/doc/emailaddress.txt'
--- lib/canonical/launchpad/doc/emailaddress.txt	2010-11-15 21:19:45 +0000
+++ lib/canonical/launchpad/doc/emailaddress.txt	2010-12-13 13:34:01 +0000
@@ -12,6 +12,7 @@
 
 We can get an email address through IEmailAddressSet.getByEmail().
 
+    >>> login_person(person)
     >>> email = emailset.getByEmail(person.preferredemail.email)
     >>> email == person.preferredemail
     True

=== modified file 'lib/canonical/launchpad/doc/location-widget.txt'
--- lib/canonical/launchpad/doc/location-widget.txt	2010-11-16 18:55:26 +0000
+++ lib/canonical/launchpad/doc/location-widget.txt	2010-12-13 13:34:01 +0000
@@ -28,7 +28,7 @@
 map around the location where he seems to be, based on the IP address of
 the request.
 
-    >>> login(salgado.preferredemail.email)
+    >>> login_person(salgado)
     >>> widget = LocationWidget(bound_field, request)
     >>> widget.zoom
     7
@@ -65,7 +65,7 @@
 
     >>> kamion = getUtility(IPersonSet).getByName('kamion')
     >>> bound_field = field.bind(kamion)
-    >>> login(kamion.preferredemail.email)
+    >>> login_person(kamion)
     >>> widget = LocationWidget(bound_field, request)
     >>> widget.zoom
     9
@@ -148,7 +148,7 @@
     >>> bound_field = field.bind(kamion)
     >>> request = LaunchpadTestRequest(
     ...     environ={'REMOTE_ADDR': '201.13.165.145'})
-    >>> login(kamion.preferredemail.email)
+    >>> login_person(kamion)
     >>> widget = LocationWidget(bound_field, request)
     >>> print widget()
     <input...name="field.location.latitude"...type="hidden"...

=== modified file 'lib/canonical/launchpad/doc/notification-recipient-set.txt'
--- lib/canonical/launchpad/doc/notification-recipient-set.txt	2010-10-18 22:24:59 +0000
+++ lib/canonical/launchpad/doc/notification-recipient-set.txt	2010-12-13 13:34:01 +0000
@@ -123,6 +123,7 @@
 other person:
 
     >>> ubuntu_team = person_set.getByName('ubuntu-team')
+    >>> login_person(ubuntu_team.teamowner)
     >>> print ubuntu_team.preferredemail.email
     [e-mail]
     >>> recipients.add(ubuntu_team, 'You are notified for fun.', 'Fun')

=== modified file 'lib/canonical/launchpad/doc/vocabulary-json.txt'
--- lib/canonical/launchpad/doc/vocabulary-json.txt	2010-10-03 15:30:06 +0000
+++ lib/canonical/launchpad/doc/vocabulary-json.txt	2010-12-13 13:34:01 +0000
@@ -96,7 +96,7 @@
             {
                 "api_uri": "/~name16",
                 "css": "sprite person",
-                "description": "[e-mail]",
+                "description": "<email address hidden>",
                 "title": "Foo Bar",
                 "value": "name16"
             }

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2010-12-07 14:15:37 +0000
+++ lib/canonical/launchpad/security.py	2010-12-13 13:34:01 +0000
@@ -2422,11 +2422,8 @@
 
     def checkUnauthenticated(self):
         """See `AuthorizationBase`."""
-        # Email addresses without an associated Person cannot be seen by
-        # anonymous users.
-        if self.obj.person is None:
-            return False
-        return not self.obj.person.hide_email_addresses
+        # Anonymous users can never see email addresses.
+        return False
 
     def checkAccountAuthenticated(self, account):
         """Can the user see the details of this email address?

=== modified file 'lib/canonical/launchpad/webapp/tests/test_login.py'
--- lib/canonical/launchpad/webapp/tests/test_login.py	2010-10-27 14:20:21 +0000
+++ lib/canonical/launchpad/webapp/tests/test_login.py	2010-12-13 13:34:01 +0000
@@ -188,10 +188,11 @@
     def test_full_fledged_account(self):
         # In the common case we just login and redirect to the URL specified
         # in the 'starting_url' query arg.
-        person = self.factory.makePerson()
+        test_email = '[e-mail]'
+        person = self.factory.makePerson(email=test_email)
         with SRegResponse_fromSuccessResponse_stubbed():
             view, html = self._createViewWithResponse(
-                person.account, email=person.preferredemail.email)
+                person.account, email=test_email)
         self.assertTrue(view.login_called)
         response = view.request.response
         self.assertEquals(httplib.TEMPORARY_REDIRECT, response.getStatus())

=== modified file 'lib/canonical/launchpad/webapp/tests/test_loginsource.py'
--- lib/canonical/launchpad/webapp/tests/test_loginsource.py	2010-10-20 20:51:26 +0000
+++ lib/canonical/launchpad/webapp/tests/test_loginsource.py	2010-12-13 13:34:01 +0000
@@ -4,6 +4,7 @@
 import unittest
 
 from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
 
 from canonical.launchpad.ftests import (
     ANONYMOUS,
@@ -29,8 +30,8 @@
         """
         principal = self.login_source.getPrincipal(self.mark.account.id)
         self.assertEqual(principal.access_level, AccessLevel.WRITE_PRIVATE)
-        principal = self.login_source.getPrincipalByLogin(
-            self.mark.preferredemail.email)
+        marks_email = removeSecurityProxy(self.mark).preferredemail.email
+        principal = self.login_source.getPrincipalByLogin(marks_email)
         self.assertEqual(principal.access_level, AccessLevel.WRITE_PRIVATE)
 
     def test_given_access_level_is_used(self):
@@ -41,7 +42,7 @@
             self.mark.account.id, access_level=AccessLevel.WRITE_PUBLIC)
         self.assertEqual(principal.access_level, AccessLevel.WRITE_PUBLIC)
         principal = self.login_source.getPrincipalByLogin(
-            self.mark.preferredemail.email, AccessLevel.READ_PUBLIC)
+            removeSecurityProxy(self.mark).preferredemail.email, AccessLevel.READ_PUBLIC)
         self.assertEqual(principal.access_level, AccessLevel.READ_PUBLIC)
 
 

=== modified file 'lib/lp/blueprints/stories/standalone/subscribing.txt'
--- lib/lp/blueprints/stories/standalone/subscribing.txt	2010-07-30 12:56:27 +0000
+++ lib/lp/blueprints/stories/standalone/subscribing.txt	2010-12-13 13:34:01 +0000
@@ -235,7 +235,7 @@
 We subscribe the Ubuntu Team and an email is sent to the team's
 preferred email address.
 
-    >>> login(ANONYMOUS)
+    >>> login('[e-mail]')
     >>> ([message['To'] for message in pop_notifications()] ==
     ...     [str(ubuntu_team.preferredemail.email)])
     True
@@ -250,7 +250,7 @@
 We modified the Ubuntu Team's subscription and again, an email is sent
 to the team's preferred email address.
 
-    >>> login(ANONYMOUS)
+    >>> login('[e-mail]')
     >>> ([message['To'] for message in pop_notifications()] ==
     ...     [str(ubuntu_team.preferredemail.email)])
     True

=== modified file 'lib/lp/code/browser/tests/test_branch.py'
--- lib/lp/code/browser/tests/test_branch.py	2010-11-09 18:42:38 +0000
+++ lib/lp/code/browser/tests/test_branch.py	2010-12-13 13:34:01 +0000
@@ -172,7 +172,7 @@
         """Registering a mirrored branch requests a mirror."""
         arbitrary_person = self.factory.makePerson()
         arbitrary_product = self.factory.makeProduct()
-        login(arbitrary_person.preferredemail.email)
+        login_person(arbitrary_person)
         try:
             add_view = BranchAddView(arbitrary_person, self.request)
             add_view.initialize()

=== modified file 'lib/lp/code/mail/branch.py'
--- lib/lp/code/mail/branch.py	2010-08-20 20:31:18 +0000
+++ lib/lp/code/mail/branch.py	2010-12-13 13:34:01 +0000
@@ -5,6 +5,7 @@
 
 __metaclass__ = type
 
+from zope.security.proxy import removeSecurityProxy
 
 from canonical.launchpad.mail import format_address
 from canonical.launchpad.webapp import canonical_url
@@ -192,7 +193,7 @@
                     RecipientReason.forBranchSubscriber(
                     subscription, recipient, rationale)
         from_address = format_address(
-            user.displayname, user.preferredemail.email)
+            user.displayname, removeSecurityProxy(user).preferredemail.email)
         return cls(
             '[Branch %(unique_name)s]', 'branch-modified.txt',
             actual_recipients, from_address, delta=delta,
@@ -304,4 +305,5 @@
 
     @staticmethod
     def _format_user_address(user):
-        return format_address(user.displayname, user.preferredemail.email)
+        naked_email = removeSecurityProxy(user).preferredemail.email
+        return format_address(user.displayname, naked_email)

=== modified file 'lib/lp/code/mail/tests/test_branch.py'
--- lib/lp/code/mail/tests/test_branch.py	2010-10-04 19:50:45 +0000
+++ lib/lp/code/mail/tests/test_branch.py	2010-12-13 13:34:01 +0000
@@ -5,6 +5,8 @@
 
 from unittest import TestLoader
 
+from zope.security.proxy import removeSecurityProxy
+
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.code.enums import (
     BranchSubscriptionDiffSize,
@@ -259,8 +261,9 @@
         mailer = BranchMailer.forRevision(
             branch, 1, '[e-mail]', 'content', 'diff',
             'Testing %j foo')
+        branch_owner_email = removeSecurityProxy(branch.owner).preferredemail.email
         self.assertEqual('Testing %j foo', mailer._getSubject(
-                branch.owner.preferredemail.email))
+                branch_owner_email))
 
 
 def test_suite():

=== modified file 'lib/lp/code/mail/tests/test_sourcepackagerecipebuild.py'
--- lib/lp/code/mail/tests/test_sourcepackagerecipebuild.py	2010-10-04 19:50:45 +0000
+++ lib/lp/code/mail/tests/test_sourcepackagerecipebuild.py	2010-12-13 13:34:01 +0000
@@ -48,7 +48,7 @@
 
     def makeStatusEmail(self, build):
         mailer = SourcePackageRecipeBuildMailer.forStatus(build)
-        email = build.requester.preferredemail.email
+        email = removeSecurityProxy(build.requester).preferredemail.email
         return mailer.generateEmail(email, build.requester)
 
     def test_generateEmail(self):

=== modified file 'lib/lp/code/model/recipebuilder.py'
--- lib/lp/code/model/recipebuilder.py	2010-11-17 14:00:22 +0000
+++ lib/lp/code/model/recipebuilder.py	2010-12-13 13:34:01 +0000
@@ -12,6 +12,7 @@
 
 from zope.component import adapts
 from zope.interface import implements
+from zope.security.proxy import removeSecurityProxy
 
 from canonical.config import config
 from lp.buildmaster.interfaces.builder import CannotBuild
@@ -72,7 +73,11 @@
             args["author_email"] = config.canonical.noreply_from_address
         else:
             args["author_name"] = requester.displayname
-            args["author_email"] = requester.preferredemail.email
+            # We have to remove the security proxy here b/c there's not a
+            # logged in entity, and anonymous email lookups aren't allowed.
+            # Don't keep the naked requester around though.
+            args["author_email"] = removeSecurityProxy(
+                requester).preferredemail.email
         args["recipe_text"] = str(self.build.recipe.builder_recipe)
         args['archive_purpose'] = self.build.archive.purpose.name
         args["ogrecomponent"] = get_primary_current_component(

=== modified file 'lib/lp/code/model/tests/test_recipebuilder.py'
--- lib/lp/code/model/tests/test_recipebuilder.py	2010-11-27 07:17:34 +0000
+++ lib/lp/code/model/tests/test_recipebuilder.py	2010-12-13 13:34:01 +0000
@@ -298,6 +298,7 @@
         # dispatchBuildToSlave will fail when there is not chroot tarball
         # available for the distroseries to build for.
         job = self.makeJob()
+        test_publisher = SoyuzTestPublisher()
         builder = MockBuilder("bob-de-bouwer", OkSlave())
         processorfamily = ProcessorFamilySet().getByProcessorName('386')
         builder.processor = processorfamily.processors[0]

=== modified file 'lib/lp/code/stories/feeds/xx-revision-atom.txt'
--- lib/lp/code/stories/feeds/xx-revision-atom.txt	2010-10-18 22:24:59 +0000
+++ lib/lp/code/stories/feeds/xx-revision-atom.txt	2010-12-13 13:34:01 +0000
@@ -32,10 +32,11 @@
     >>> fooey_branch = factory.makeProductBranch(
     ...     name='feature-x', product=fooey, owner=mike)
 
+    >>> from zope.security.proxy import removeSecurityProxy
     >>> def makeRevision(author, rev_id, log_body):
     ...     global factory, date_generator
     ...     return factory.makeRevision(
-    ...         author=author.preferredemail.email,
+    ...         author=removeSecurityProxy(author).preferredemail.email,
     ...         revision_date=date_generator.next(),
     ...         rev_id=rev_id, log_body=log_body)
     >>> ignored = fooey_branch.createBranchRevision(

=== modified file 'lib/lp/code/tests/test_directbranchcommit.py'
--- lib/lp/code/tests/test_directbranchcommit.py	2010-10-02 11:41:43 +0000
+++ lib/lp/code/tests/test_directbranchcommit.py	2010-12-13 13:34:01 +0000
@@ -5,6 +5,8 @@
 
 __metaclass__ = type
 
+from zope.security.proxy import removeSecurityProxy
+
 from canonical.testing.layers import (
     DatabaseFunctionalLayer,
     ZopelessDatabaseLayer,
@@ -309,7 +311,8 @@
         committer = DirectBranchCommit(branch)
         self.addCleanup(committer.unlock)
         self.assertIn(
-            branch.owner.preferredemail.email, committer.getBzrCommitterID())
+            removeSecurityProxy(branch.owner).preferredemail.email,
+            committer.getBzrCommitterID())
 
     def test_falls_back_to_noreply(self):
         # If all else fails, getBzrCommitterID uses the noreply

=== modified file 'lib/lp/registry/browser/tests/person-views.txt'
--- lib/lp/registry/browser/tests/person-views.txt	2010-11-01 03:46:17 +0000
+++ lib/lp/registry/browser/tests/person-views.txt	2010-12-13 13:34:01 +0000
@@ -106,14 +106,16 @@
 can see them
 
 Mark has a registered email address, and he has chosen to disclose it to
-the world.
+anyone in Launchpad..
 
+    >>> login('[e-mail]')
     >>> mark = person_set.getByEmail('[e-mail]')
     >>> mark.preferredemail.email
     u'[e-mail]'
 
     >>> mark.hide_email_addresses
     False
+    >>> logout()
 
 Anonymous users cannot see any Launchpad user's email addresses. The
 email addresses state is LOGIN_REQUIRED, there is no description, nor
@@ -332,7 +334,7 @@
 
     >>> from lp.blueprints.enums import SpecificationImplementationStatus
 
-    >>> login(user.preferredemail.email)
+    >>> login_person(user)
     >>> product = factory.makeProduct(name="tool", owner=user)
     >>> spec = factory.makeSpecification(
     ...     product=product, title='Specs need stories')

=== modified file 'lib/lp/registry/browser/tests/test_person_webservice.py'
--- lib/lp/registry/browser/tests/test_person_webservice.py	2010-10-04 19:50:45 +0000
+++ lib/lp/registry/browser/tests/test_person_webservice.py	2010-12-13 13:34:01 +0000
@@ -5,12 +5,52 @@
 
 import unittest
 
+from zope.security.management import endInteraction
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.launchpad.ftests import login
 from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
 from canonical.testing.layers import DatabaseFunctionalLayer
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    launchpadlib_for,
+    launchpadlib_for_anonymous,
+    TestCaseWithFactory,
+    )
+
+
+class TestPersonEmailSecurity(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestPersonEmailSecurity, self).setUp()
+        self.target = self.factory.makePerson(name='target')
+        self.email_one = self.factory.makeEmail(
+                '[e-mail]', self.target)
+        self.email_two = self.factory.makeEmail(
+                '[e-mail]', self.target)
+
+    def test_logged_in_can_access(self):
+        # A logged in launchpadlib connection can see confirmed email
+        # addresses.
+        accessor = self.factory.makePerson()
+        lp = launchpadlib_for("test", accessor.name)
+        person = lp.people['target']
+        emails = sorted(list(person.confirmed_email_addresses))
+        self.assertNotEqual(
+                sorted([self.email_one, self.email_two]),
+                len(emails))
+
+    def test_anonymous_cannot_access(self):
+        # An anonymous launchpadlib connection cannot see email addresses.
+
+        # Need to endInteraction() because launchpadlib_for_anonymous() will
+        # setup a new one.
+        endInteraction()
+        lp = launchpadlib_for_anonymous('test', version='devel')
+        person = lp.people['target']
+        emails = list(person.confirmed_email_addresses)
+        self.assertEqual([], emails)
 
 
 class TestPersonRepresentation(TestCaseWithFactory):

=== modified file 'lib/lp/registry/browser/tests/user-to-user-views.txt'
--- lib/lp/registry/browser/tests/user-to-user-views.txt	2010-08-17 15:57:37 +0000
+++ lib/lp/registry/browser/tests/user-to-user-views.txt	2010-12-13 13:34:01 +0000
@@ -24,7 +24,7 @@
 ...No Priv would start by going to Salgado's +contactuser page.
 
     >>> from canonical.launchpad.ftests import login
-    >>> login(no_priv.preferredemail.email)
+    >>> login_person(no_priv)
     >>> view = create_view(no_priv, salgado)
 
 This contact is allowed.

=== modified file 'lib/lp/registry/doc/message-holds.txt'
--- lib/lp/registry/doc/message-holds.txt	2010-10-26 02:18:08 +0000
+++ lib/lp/registry/doc/message-holds.txt	2010-12-13 13:34:01 +0000
@@ -378,10 +378,10 @@
 
 
 == Posting privileges ==
-
 Message approvals are also used to derived posting privileges for non-team
 members.  To start with, only the team owner can post to the list.
 
+    >>> login('[e-mail]')
     >>> team_three, list_three = mailinglists_helper.new_team(
     ...     'test-three', True)
     >>> sorted(email.email for email in list_three.getSenderAddresses())

=== modified file 'lib/lp/registry/doc/person.txt'
--- lib/lp/registry/doc/person.txt	2010-11-02 05:48:54 +0000
+++ lib/lp/registry/doc/person.txt	2010-12-13 13:34:01 +0000
@@ -639,6 +639,7 @@
 If a team has a contact email address, all notifications we send to the
 team will go to that address.
 
+    >>> login('[e-mail]')
     >>> ubuntu_team = personset.getByName('ubuntu-team')
     >>> ubuntu_team.preferredemail.email
     u'[e-mail]'
@@ -750,11 +751,11 @@
     # First we'll define a utility function to help us displaying
     # the results.
 
-    >>> emailset = getUtility(IEmailAddressSet)
+    >>> naked_emailset = removeSecurityProxy(getUtility(IEmailAddressSet))
     >>> def print_people(results):
     ...     for person in results:
     ...         emails = [email.email
-    ...                   for email in emailset.getByPerson(person)]
+    ...                   for email in naked_emailset.getByPerson(person)]
     ...         print "%s (%s): %s" % (
     ...             person.displayname, person.name, emails)
 

=== modified file 'lib/lp/registry/scripts/personnotification.py'
--- lib/lp/registry/scripts/personnotification.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/scripts/personnotification.py	2010-12-13 13:34:01 +0000
@@ -15,6 +15,7 @@
 
 import pytz
 from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
 
 from canonical.config import config
 from lp.registry.interfaces.personnotification import IPersonNotificationSet
@@ -44,7 +45,7 @@
                 continue
             self.logger.info(
                 "Sending notification to %s <%s>."
-                % (person.name, person.preferredemail.email))
+                % (person.name, removeSecurityProxy(person).preferredemail.email))
             notification.send()
             notifications_sent = True
             # Commit after each email sent, so that we won't re-mail the

=== modified file 'lib/lp/registry/stories/webservice/xx-person.txt'
--- lib/lp/registry/stories/webservice/xx-person.txt	2010-11-22 15:30:05 +0000
+++ lib/lp/registry/stories/webservice/xx-person.txt	2010-12-13 13:34:01 +0000
@@ -1,4 +1,5 @@
-= People and teams =
+People and teams
+================
 
 Since we use a single class (Person) to represent a person or a team,
 representations of people and teams are supposed to have nearly the
@@ -10,13 +11,16 @@
     >>> pprint_entry(salgado)
     admins_collection_link: u'http://.../~salgado/admins'
     archive_link: None
-    confirmed_email_addresses_collection_link: u'http://.../~salgado/confirmed_email_addresses'
+    confirmed_email_addresses_collection_link:
+        u'http://.../~salgado/confirmed_email_addresses'
     date_created: u'2005-06-06T08:59:51.596025+00:00'
-    deactivated_members_collection_link: u'http://.../~salgado/deactivated_members'
+    deactivated_members_collection_link:
+        u'http://.../~salgado/deactivated_members'
     display_name: u'Guilherme Salgado'
     expired_members_collection_link: u'http://.../~salgado/expired_members'
     gpg_keys_collection_link: u'http://.../~salgado/gpg_keys'
-    hardware_submissions_collection_link: u'http://.../~salgado/hardware_submissions'
+    hardware_submissions_collection_link:
+        u'http://.../~salgado/hardware_submissions'
     hide_email_addresses: False
     homepage_content: None
     invited_members_collection_link: u'http://.../~salgado/invited_members'
@@ -34,13 +38,16 @@
     mailing_list_auto_subscribe_policy: u'Ask me when I join a team'
     members_collection_link: u'http://.../~salgado/members'
     members_details_collection_link: u'http://.../~salgado/members_details'
-    memberships_details_collection_link: u'http://.../~salgado/memberships_details'
+    memberships_details_collection_link:
+        u'http://.../~salgado/memberships_details'
     mugshot_link: u'http://.../~salgado/mugshot'
     name: u'salgado'
-    open_membership_invitations_collection_link: u'http://.../~salgado/open_membership_invitations'
+    open_membership_invitations_collection_link:
+        u'http://.../~salgado/open_membership_invitations'
     participants_collection_link: u'http://.../~salgado/participants'
     ppas_collection_link: u'http://.../~salgado/ppas'
-    preferred_email_address_link: u'http://.../~salgado/+email/[e-mail]'
+    preferred_email_address_link:
+        u'http://.../~salgado/+email/[e-mail]'
     private: False
     proposed_members_collection_link: u'http://.../~salgado/proposed_members'
     resource_type_link: u'http://.../#person'
@@ -57,18 +64,23 @@
     >>> pprint_entry(ubuntu_team)
     admins_collection_link: u'http://.../~ubuntu-team/admins'
     archive_link: None
-    confirmed_email_addresses_collection_link: u'http://.../~ubuntu-team/confirmed_email_addresses'
+    confirmed_email_addresses_collection_link:
+        u'http://.../~ubuntu-team/confirmed_email_addresses'
     date_created: u'2005-06-06T08:59:51.605760+00:00'
-    deactivated_members_collection_link: u'http://.../~ubuntu-team/deactivated_members'
+    deactivated_members_collection_link:
+        u'http://.../~ubuntu-team/deactivated_members'
     default_membership_period: None
     default_renewal_period: None
     display_name: u'Ubuntu Team'
-    expired_members_collection_link: u'http://.../~ubuntu-team/expired_members'
+    expired_members_collection_link:
+        u'http://.../~ubuntu-team/expired_members'
     gpg_keys_collection_link: u'http://.../~ubuntu-team/gpg_keys'
-    hardware_submissions_collection_link: u'http://.../~ubuntu-team/hardware_submissions'
+    hardware_submissions_collection_link:
+        u'http://.../~ubuntu-team/hardware_submissions'
     hide_email_addresses: False
     homepage_content: None
-    invited_members_collection_link: u'http://.../~ubuntu-team/invited_members'
+    invited_members_collection_link:
+        u'http://.../~ubuntu-team/invited_members'
     irc_nicknames_collection_link: u'http://.../~ubuntu-team/irc_nicknames'
     is_probationary: False
     is_team: True
@@ -82,16 +94,21 @@
     longitude: None
     mailing_list_auto_subscribe_policy: u'Ask me when I join a team'
     members_collection_link: u'http://.../~ubuntu-team/members'
-    members_details_collection_link: u'http://.../~ubuntu-team/members_details'
-    memberships_details_collection_link: u'http://.../~ubuntu-team/memberships_details'
+    members_details_collection_link:
+        u'http://.../~ubuntu-team/members_details'
+    memberships_details_collection_link:
+        u'http://.../~ubuntu-team/memberships_details'
     mugshot_link: u'http://.../~ubuntu-team/mugshot'
     name: u'ubuntu-team'
-    open_membership_invitations_collection_link: u'http://.../~ubuntu-team/open_membership_invitations'
+    open_membership_invitations_collection_link:
+        u'http://.../~ubuntu-team/open_membership_invitations'
     participants_collection_link: u'http://.../~ubuntu-team/participants'
     ppas_collection_link: u'http://.../~ubuntu-team/ppas'
-    preferred_email_address_link: u'http://.../~ubuntu-team/+email/[e-mail]'
+    preferred_email_address_link:
+        u'http://.../~ubuntu-team/+email/[e-mail]'
     private: False
-    proposed_members_collection_link: u'http://.../~ubuntu-team/proposed_members'
+    proposed_members_collection_link:
+        u'http://.../~ubuntu-team/proposed_members'
     renewal_policy: u'invite them to apply for renewal'
     resource_type_link: u'http://.../#team'
     self_link: u'http://.../~ubuntu-team'
@@ -113,13 +130,15 @@
     []
 
 
-== Links to related things ==
+Links to related things
+-----------------------
 
 As seen above, many attributes of a person are actually links to other
 things (or collections).
 
 
-=== Email addresses ===
+Email addresses
+...............
 
 Apart from the link to the preferred email, there is a link to the
 collection of other confirmed email addresses of that person/team.
@@ -151,7 +170,8 @@
     HTTP/1.1 404 Not Found
     ...
 
-== SSH keys ===
+SSH keys
+........
 
 People have SSH keys which we can manipulate over the API.
 
@@ -175,8 +195,8 @@
     >>> ssh_key = factory.makeSSHKey(ssh_user)
     >>> logout()
 
-Now when we get the sshkey collection for 'sssh-user' again, the key should show
-up:
+Now when we get the sshkey collection for 'sssh-user' again, the key should
+show up:
 
     >>> keys = webservice.get(sshkeys).jsonBody()
     >>> print_self_link_of_entries(keys)
@@ -192,7 +212,8 @@
     resource_type_link: u'http://.../#ssh_key'
     self_link: u'http://.../~ssh-user/+ssh-keys/...'
 
-=== GPG keys ===
+GPG keys
+........
 
 People have GPG keys which we can manipulate over the API.
 
@@ -230,7 +251,8 @@
     self_link: u'http://.../~name12/+gpg-keys/...'
 
 
-=== Team memberships ===
+Team memberships
+................
 
 A person is linked to their team memberships.
 
@@ -248,7 +270,8 @@
 And to all membership invitations sent to it.
 
     >>> lp_team = webservice.get("/~launchpad").jsonBody()
-    >>> lp_invitations = lp_team['open_membership_invitations_collection_link']
+    >>> lp_invitations = lp_team[
+    ...     'open_membership_invitations_collection_link']
     >>> lp_invitations
     u'http://.../~launchpad/open_membership_invitations'
 
@@ -331,7 +354,7 @@
     >>> print webservice.get(
     ...     salgado_landscape['self_link']).jsonBody()['status']
     Deactivated
-    
+
     >>> print webservice.named_post(
     ...     salgado_landscape['self_link'], 'setStatus', {},
     ...     status='Approved', silent=True)
@@ -341,7 +364,7 @@
     >>> print webservice.get(
     ...     salgado_landscape['self_link']).jsonBody()['status']
     Approved
-    
+
     >>> print webservice.named_post(
     ...     salgado_landscape['self_link'], 'setStatus', {},
     ...     status='Deactivated', silent=True)
@@ -357,7 +380,8 @@
     ...
 
 
-=== Members ===
+Members
+.......
 
 A list of team memberships is distinct from a list of a team's
 members. Members are people; memberships are TeamMemberships. You've
@@ -427,7 +451,8 @@
     http://.../~karl
 
 
-=== Sub-teams and super-teams ===
+Sub-teams and super-teams
+.........................
 
 Teams can be members of other teams, and sometimes it's useful to know
 which teams are members of any given team as well as the ones it is a
@@ -442,7 +467,8 @@
     http://.../~guadamen
 
 
-=== Wiki names ===
+Wiki names
+..........
 
 All wiki names associated to a person/team are also linked to that
 person/team.
@@ -461,7 +487,8 @@
 Wiki names are first-class objects with their own URLs and
 representations too.
 
-    >>> wiki_name = sorted(webservice.get(wikis_link).jsonBody()['entries'])[0]
+    >>> wiki_name = sorted(
+    ...     webservice.get(wikis_link).jsonBody()['entries'])[0]
     >>> pprint_entry(wiki_name)
     person_link: u'http://.../~salgado'
     resource_type_link: u'http://.../#wiki_name'
@@ -485,7 +512,8 @@
     ...    u'wikiname': 'MrExample'}
     >>> response = webservice.patch(
     ...     wiki_name['self_link'], 'application/json', dumps(patch))
-    >>> wiki_name = sorted(webservice.get(wikis_link).jsonBody()['entries'])[0]
+    >>> wiki_name = sorted(
+    ...     webservice.get(wikis_link).jsonBody()['entries'])[0]
     >>> print wiki_name['url']
     http://www.example.com/MrExample
 
@@ -503,7 +531,8 @@
     Only URIs with the following schemes may be used: http, https
 
 
-=== Jabber IDs ===
+Jabber IDs
+..........
 
 Jabber IDs of a person are also linked.
 
@@ -534,7 +563,8 @@
     ...
 
 
-=== IRC nicknames ===
+IRC nicknames
+.............
 
 The same for IRC nicknames
 
@@ -546,7 +576,8 @@
 
 Anonymous listing is possible.
 
-    >>> print_self_link_of_entries(anon_webservice.get(irc_ids_link).jsonBody())
+    >>> print_self_link_of_entries(
+    ...     anon_webservice.get(irc_ids_link).jsonBody())
     http://.../~mark/+ircnick/1
 
 IRC IDs are first-class objects with their own URLs and representations
@@ -568,7 +599,8 @@
     ...
 
 
-=== PPAs ===
+PPAs
+....
 
 We can get to the person's default PPA via the 'archive' property:
 
@@ -634,7 +666,8 @@
     [u'http://mark:[e-mail]/mark/p3a/ubuntu']
 
 
-== Custom operations ==
+Custom operations
+-----------------
 
 IPerson supports a bunch of operations.
 
@@ -669,7 +702,8 @@
     ---
 
 
-=== Team membership operations ===
+Team membership operations
+..........................
 
 Joining and leaving teams:
 
@@ -791,14 +825,15 @@
     # http://.../~mailing-list-experts
 
 
-== Restrictions ==
+Restrictions
+------------
 
 A team can't be its own owner.
 
-  >>> import simplejson
-  >>> doc = {'team_owner_link' : webservice.getAbsoluteUrl("/~admins")}
-  >>> print webservice.patch(
-  ...    "/~admins", 'application/json', simplejson.dumps(doc))
-  HTTP/1.1 400 Bad Request
-  ...
-  team_owner_link: Constraint not satisfied.
+    >>> import simplejson
+    >>> doc = {'team_owner_link' : webservice.getAbsoluteUrl("/~admins")}
+    >>> print webservice.patch(
+    ...    "/~admins", 'application/json', simplejson.dumps(doc))
+    HTTP/1.1 400 Bad Request
+    ...
+    team_owner_link: Constraint not satisfied.

=== modified file 'lib/lp/registry/tests/test_personset.py'
--- lib/lp/registry/tests/test_personset.py	2010-10-04 19:50:45 +0000
+++ lib/lp/registry/tests/test_personset.py	2010-12-13 13:34:01 +0000
@@ -172,13 +172,13 @@
             "when purchasing an application via Software Center.")
 
     def test_existing_person(self):
-        person = self.factory.makePerson()
+        email = '[e-mail]'
+        person = self.factory.makePerson(email=email)
         openid_ident = removeSecurityProxy(
             person.account).openid_identifiers.any().identifier
         person_set = getUtility(IPersonSet)
 
-        result, db_updated = self.callGetOrCreate(
-            openid_ident, email=person.preferredemail.email)
+        result, db_updated = self.callGetOrCreate(openid_ident, email=email)
 
         self.assertEqual(person, result)
         self.assertFalse(db_updated)

=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py	2010-11-22 22:00:28 +0000
+++ lib/lp/registry/tests/test_product.py	2010-12-13 13:34:01 +0000
@@ -12,10 +12,7 @@
 import transaction
 from zope.component import getUtility
 
-from canonical.launchpad.ftests import (
-    login,
-    syncUpdate,
-    )
+from canonical.launchpad.ftests import syncUpdate
 from canonical.launchpad.testing.pages import (
     find_main_content,
     get_feedback_messages,
@@ -37,7 +34,11 @@
     UnDeactivateable,
     )
 from lp.registry.model.productlicense import ProductLicense
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    login,
+    login_person,
+    TestCaseWithFactory,
+    )
 
 
 class TestProduct(TestCaseWithFactory):
@@ -339,7 +340,7 @@
         super(BugSupervisorTestCase, self).setUp()
         self.person = self.factory.makePerson()
         self.product = self.factory.makeProduct(owner=self.person)
-        login(self.person.preferredemail.email)
+        login_person(self.person)
 
     def testPersonCanSetSelfAsSupervisor(self):
         # A person can set themselves as bug supervisor for a product.

=== modified file 'lib/lp/registry/tests/test_project.py'
--- lib/lp/registry/tests/test_project.py	2010-10-04 19:50:45 +0000
+++ lib/lp/registry/tests/test_project.py	2010-12-13 13:34:01 +0000
@@ -17,6 +17,7 @@
 from lp.registry.interfaces.projectgroup import IProjectGroupSet
 from lp.testing import (
     launchpadlib_for,
+    login_person,
     TestCaseWithFactory,
     )
 
@@ -36,7 +37,7 @@
             name="razzle-dazzle", owner=self.person,
             description="Giving 110% at all times.")
         self.projectset = getUtility(IProjectGroupSet)
-        login(self.person.preferredemail.email)
+        login_person(self.person)
 
     def testSearchNoMatch(self):
         # Search for a string that does not exist.

=== modified file 'lib/lp/services/mail/sendmail.py'
--- lib/lp/services/mail/sendmail.py	2010-10-20 01:23:52 +0000
+++ lib/lp/services/mail/sendmail.py	2010-12-13 13:34:01 +0000
@@ -46,6 +46,7 @@
 from lazr.restful.utils import get_current_browser_request
 from zope.app import zapi
 from zope.security.proxy import isinstance as zisinstance
+from zope.security.proxy import removeSecurityProxy
 from zope.sendmail.interfaces import IMailDelivery
 
 from canonical.config import config
@@ -155,7 +156,8 @@
 
 def format_address_for_person(person):
     """Helper function to call format_address for a person."""
-    return format_address(person.displayname, person.preferredemail.email)
+    email_address = removeSecurityProxy(person.preferredemail).email
+    return format_address(person.displayname, email_address)
 
 
 def simple_sendmail(from_addr, to_addrs, subject, body, headers=None,

=== modified file 'lib/lp/services/mailman/testing/__init__.py'
--- lib/lp/services/mailman/testing/__init__.py	2010-10-06 11:46:51 +0000
+++ lib/lp/services/mailman/testing/__init__.py	2010-12-13 13:34:01 +0000
@@ -17,6 +17,7 @@
     mm_cfg,
     )
 from Mailman.Queue import XMLRPCRunner
+from zope.security.proxy import removeSecurityProxy
 
 from canonical.testing.layers import DatabaseFunctionalLayer
 
@@ -56,7 +57,7 @@
         # This utility is based on mailman/tests/TestBase.py.
         mlist = MailList.MailList()
         team = lp_mailing_list.team
-        owner_email = team.teamowner.preferredemail.email
+        owner_email = removeSecurityProxy(team.teamowner).preferredemail.email
         mlist.Create(team.name, owner_email, 'password')
         mlist.host_name = 'lists.launchpad.dev'
         mlist.web_page_url = 'http://lists.launchpad.dev/mailman/'

=== modified file 'lib/lp/services/mailman/tests/test_lpmoderate.py'
--- lib/lp/services/mailman/tests/test_lpmoderate.py	2010-10-26 15:47:24 +0000
+++ lib/lp/services/mailman/tests/test_lpmoderate.py	2010-12-13 13:34:01 +0000
@@ -7,6 +7,7 @@
 
 from Mailman import Errors
 from Mailman.Handlers import LPModerate
+from zope.security.proxy import removeSecurityProxy
 
 from canonical.testing.layers import LaunchpadFunctionalLayer
 from lp.services.mailman.testing import MailmanTestCase
@@ -31,7 +32,7 @@
             'team-1', 'team-1-owner')
         self.mm_list = self.makeMailmanList(self.mailing_list)
         self.lp_user = self.factory.makePerson()
-        self.lp_user_email = self.lp_user.preferredemail.email
+        self.lp_user_email = removeSecurityProxy(self.lp_user).preferredemail.email
 
     def tearDown(self):
         super(TestLPModerateTestCase, self).tearDown()
@@ -47,7 +48,8 @@
 
     def test_process_message_from_subscriber(self):
         # Messages from subscribers silently complete the process.
-        subscriber_email = self.team.teamowner.preferredemail.email
+        subscriber_email = removeSecurityProxy(
+            self.team.teamowner).preferredemail.email
         message = self.makeMailmanMessage(
             self.mm_list, subscriber_email, 'subject', 'any content.')
         msg_data = {}

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-12-09 11:03:53 +0000
+++ lib/lp/testing/factory.py	2010-12-13 13:34:01 +0000
@@ -1315,7 +1315,7 @@
         if author is None:
             author = self.getUniqueString('author')
         elif IPerson.providedBy(author):
-            author = author.preferredemail.email
+            author = removeSecurityProxy(author).preferredemail.email
         if revision_date is None:
             revision_date = datetime.now(pytz.UTC)
         if parent_ids is None:
@@ -1626,9 +1626,10 @@
         mail = SignedMessage()
         if email_address is None:
             person = self.makePerson()
-            email_address = person.preferredemail.email
+            email_address = removeSecurityProxy(person).preferredemail.email
         mail['From'] = email_address
-        mail['To'] = self.makePerson().preferredemail.email
+        mail['To'] = removeSecurityProxy(
+            self.makePerson()).preferredemail.email
         if subject is None:
             subject = self.getUniqueString('subject')
         mail['Subject'] = subject
@@ -2716,7 +2717,7 @@
         if dsc_maintainer_rfc822 is None:
             dsc_maintainer_rfc822 = '%s <%s>' % (
                 maintainer.displayname,
-                maintainer.preferredemail.email)
+                removeSecurityProxy(maintainer).preferredemail.email)
 
         if creator is None:
             creator = self.makePerson()
@@ -3105,7 +3106,7 @@
         msg['Message-Id'] = make_msgid('launchpad')
         msg['Date'] = formatdate()
         msg['To'] = to
-        msg['From'] = sender.preferredemail.email
+        msg['From'] = removeSecurityProxy(sender).preferredemail.email
         msg['Subject'] = 'Sample'
 
         if attachments is None:
@@ -3142,7 +3143,7 @@
             timezone=0)
         email = None
         if sender is not None:
-            email = sender.preferredemail.email
+            email = removeSecurityProxy(sender).preferredemail.email
         return self.makeSignedMessage(
             body='My body', subject='My subject',
             attachment_contents=''.join(md.to_lines()),

=== modified file 'lib/lp/testing/tests/test_login.py'
--- lib/lp/testing/tests/test_login.py	2010-10-26 15:47:24 +0000
+++ lib/lp/testing/tests/test_login.py	2010-12-13 13:34:01 +0000
@@ -122,8 +122,8 @@
 
     def test_login_with_email(self):
         # login() logs a person in by email.
-        person = self.factory.makePerson()
-        email = person.preferredemail.email
+        email = '[e-mail]'
+        person = self.factory.makePerson(email=email)
         logout()
         login(email)
         self.assertLoggedIn(person)