Merge lp:~rharding/launchpad/email_notice_959482 into lp:launchpad

[Richard Harding]

Add a notification email to a user whenever significant changes to their account are taking place.

= Update =
This branch has been changed dramatically from the original submission. We've ditched the event and gone straight to sending emails via simple_sendmail. Resubmitting for code review since it's so different.

= Summary =
Launchpad should make more noise when important user information is changed in case it came from a malicious user. The frequency should be low enough that the extra warnings are worth the extra email notifications sent to users.

== Proposed Fix ==
Whenever a user adds an email address, changes their preferred email address, or adds/removes ssh keys, Launchpad should send a notification to the person that the information was changed with a note on how to proceed if those changes were not intentional.

== Implementation Details ==
A new method has been added to the Person interface/model, `security_field_changed` which generates an email message to the user that the data has been changed and instructions on what to do if this was not done by the owner of the account.

== Tests ==

*Note* We had to adjust many tests that validate and work with emails and ssh
keys since any time that they wanted to check that a confirmation email or
such was sent we also had to deal with the security notification getting sent
out as well.

lib/lp/registry/browser/tests/test_productseries_views.py
lib/lp/registry/browser/tests/test_sshkey.py
lib/lp/registry/doc/person.txt
lib/lp/registry/doc/sshkey.txt
lib/lp/registry/stories/person/xx-add-email.txt
lib/lp/registry/stories/person/xx-add-sshkey.txt
lib/lp/registry/stories/person/xx-validate-email.txt
lib/lp/registry/stories/webservice/xx-person.txt
lib/lp/registry/tests/test_personnotification.py
lib/lp/services/authserver/tests/test_authserver.py
lib/lp/services/mail/doc/sending-mail.txt
lib/lp/services/verification/browser/tests/logintoken-corner-cases.txt
lib/lp/services/verification/browser/tests/test_logintoken.py

== Demo and Q/A ==
Changing any of the listed properties should schedule an email with the correct note about the change.

== LoC ==
This is a security issue I've worked with Robert on to get into place with the
smallest LoC hit possible. There's no way to add this without adding LoC and
delaying release until I can find other places to reduce code would
inadvisable. Robert is requested as a reviewer to provide an exemption at this
time for this branch.

[Graham Binns (gmb)]

Hi Rick,

The code looks good. I have some comments about tests but they're all of a documentation / comments nature. r=me with the changes below.

[0]

General comment on review docstrings: Your docstrings are all of the form:

 """We want the frobber to go boing when pressed."""

Test docstrings should be a statement of expected behaviour. So,
docstrings need to read as follows:

 """The frobber goes boing when pressed."""

I know this seems like mindless pedantry, but it saves time when
scanning through tests, and I'm a firm believer in being both a) a
strong TDD advocate and b) lazy.

[1]

366 + def test_change_preferredemail(self):
367 + # The project_reviewed property is not reset, if the new licenses
368 + # are identical to the current licenses.

I'm guessing this is a C-P error :).

[2]

375 + # Assert form within the context manager to get access to the
376 + # email values.

s/form/from.

This occurrs a couple of times in the diff. I think you can actually get rid of the comments, since asserts-within-context-managers are a pretty common pattern now.

[3]

426 + We want to send the notification when the new email address is
427 + requested. This doesn't actually create an email address yet. It
428 + builds a LoginToken that the user must ack before the email address is
429 + added. The issue is security in alerting the owner of the account that
430 + a new address is being requested, so we need to notify at the time of
431 + request and not wait for the user to ack the new email address.
432 + """

This bit of documentation is useful. So it should be either next to the
code in question or in a doctest. Putting it in the docstring of the
unit test looks sensible, but you have to know which test to look for to
be able to find it.

review: Approve (code)

[Robert Collins (lifeless)]

This is an interesting branch.

I note that no discussion took place about the 400 odd additional LoC. Thats a shame - we really need to enforce that.

Here are some initial thoughts:
296 + >>> found = False
297 + >>> for from_addr, to_addrs, raw_msg in stub.test_emails:
298 + ... if 'token' in raw_msg:
299 + ... found = True
300 + ... token_url = get_token_url_from_email(raw_msg)
301 + ... token_url
302 + ... to_addrs

This is duplicated in the diff. I hear that functions can allow code reuse.

PersonAlterationSecurityNotification is spectacularly fat. It wants to be a function. It has one caller, and no direct tests, so just inline it into the call site.

The call site spends most of its time reconstructing what changed. I think the branch will shrink substantially if you just call the notifier when an appropriate change has happened.

tl;dr - I think you can halve the length of this branch without affecting clarity, test coverage or reusability.

review: Needs Fixing

[Robert Collins (lifeless)]

More notes while going through this in detail:
 - teams - how are they handled? the code is user specific but nothing in person.py prevents a team having ssh/gpg keys. Probably a separate issue, but it would be good not to add new OOPSes :).

 - oauth keys (adding only)
 - gpg keys (adding only)

We can notify on removals but additions are the things that matter from a security perspective.

 - LoginToken.new isn't quite the right place to trap outbound email validations; logintokens are used for a wide range of things including remote bugtracker logins, team merges and so forth. This is a bit unfortunate, because we have to choose between maximum safety (always notify) and user friendliness (don't double notify someone when they try to merge a team / claim an account). I've moved it to the code path relevant for additional-email-validation only.

I've pushed a non-event based branch to lp:~lifeless/launchpad/email_notice_959482 which you're welcome to pull from if you want to. It is missing 4 small and obvious tweaks to *existing* tests to note that an additional mail is sent as well (rather than the complex action-at-a-distant tests involving subscribers that you had). The resulting branch should be about +200 -60 lines. Still an increase, but much more pithy, and I think you'll agree that the email content people receive will be more useful.

[Robert Collins (lifeless)]

This looks pretty good and nice and compact :). it might be an idea - to check coverage - to stub out security_field_changed, run the whole test suite, and make sure there is one failure for each call site. If not, we're still missing coverage.

review: Approve

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2012-04-11 11:34:02 +0000
+++ lib/lp/registry/browser/person.py	2012-04-17 12:02:19 +0000
@@ -2657,7 +2657,7 @@
 
         found = []
         notfound = []
-        # verify if we have multiple entries to deactive
+        # Verify if we have multiple entries to activate.
         if not isinstance(key_ids, list):
             key_ids = [key_ids]
 

=== modified file 'lib/lp/registry/browser/tests/test_productseries_views.py'
--- lib/lp/registry/browser/tests/test_productseries_views.py	2012-04-11 11:34:02 +0000
+++ lib/lp/registry/browser/tests/test_productseries_views.py	2012-04-17 12:02:19 +0000
@@ -29,9 +29,9 @@
         product = self.factory.makeProduct()
         series = self.factory.makeProductSeries(product=product)
         person = product.owner
-        self.factory.makeSSHKey(person=person)
         branch_url = "lp:~%s/%s/%s" % (person.name, product.name, series.name)
         with person_logged_in(person):
+            self.factory.makeSSHKey(person=person)
             view = create_initialized_view(series, '+code-summary')
             self.assertThat(view(), Contains(branch_url))
 

=== modified file 'lib/lp/registry/browser/tests/test_sshkey.py'
--- lib/lp/registry/browser/tests/test_sshkey.py	2012-04-11 11:34:02 +0000
+++ lib/lp/registry/browser/tests/test_sshkey.py	2012-04-17 12:02:19 +0000
@@ -9,7 +9,10 @@
 
 from lp.registry.interfaces.ssh import ISSHKeySet
 from lp.services.webapp import canonical_url
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    TestCaseWithFactory,
+    person_logged_in,
+    )
 from lp.testing.layers import DatabaseFunctionalLayer
 from lp.testing.pages import (
     extract_text,
@@ -24,11 +27,12 @@
     def test_canonical_url(self):
         # The canonical URL of a GPG key is ssh-keys
         person = self.factory.makePerson()
-        sshkey = self.factory.makeSSHKey(person)
-        self.assertEqual(
-            '%s/+ssh-keys/%s' % (
-                canonical_url(person, rootsite='api'), sshkey.id),
-            canonical_url(sshkey))
+        with person_logged_in(person):
+            sshkey = self.factory.makeSSHKey(person)
+            self.assertEqual(
+                '%s/+ssh-keys/%s' % (
+                    canonical_url(person, rootsite='api'), sshkey.id),
+                canonical_url(sshkey))
 
 
 class TestSSHKeyView(TestCaseWithFactory):
@@ -38,15 +42,16 @@
     def test_escaped_message_when_removing_key(self):
         """Confirm that messages are escaped when removing keys."""
         person = self.factory.makePerson()
-        public_key = "ssh-rsa %s x<script>alert()</script>example.com" % (
-            self.getUniqueString())
-        # Add the key for the user here,
-        # since we only care about testing removal.
-        getUtility(ISSHKeySet).new(person, public_key)
-        browser = self.getUserBrowser(
-            canonical_url(person) + '/+editsshkeys', user=person)
-        browser.getControl('Remove').click()
-        msg = 'Key "x&lt;script&gt;alert()&lt;/script&gt;example.com" removed'
-        self.assertEqual(
-            extract_text(find_tags_by_class(browser.contents, 'message')[0]),
-            msg)
+        with person_logged_in(person):
+            public_key = "ssh-rsa %s x<script>alert()</script>example.com" % (
+                self.getUniqueString())
+            # Add the key for the user here,
+            # since we only care about testing removal.
+            getUtility(ISSHKeySet).new(person, public_key)
+            browser = self.getUserBrowser(
+                canonical_url(person) + '/+editsshkeys', user=person)
+            browser.getControl('Remove').click()
+            msg = 'Key "x&lt;script&gt;alert()&lt;/script&gt;example.com" removed'
+            self.assertEqual(
+                extract_text(find_tags_by_class(browser.contents, 'message')[0]),
+                msg)

=== modified file 'lib/lp/registry/doc/person.txt'
--- lib/lp/registry/doc/person.txt	2012-04-10 14:01:17 +0000
+++ lib/lp/registry/doc/person.txt	2012-04-17 12:02:19 +0000
@@ -5,6 +5,8 @@
 some people which have done work on the free software community but are
 not Launchpad users.
 
+    >>> from lp.services.mail import stub
+    >>> import transaction
     >>> from zope.component import getUtility
     >>> from lp.services.identity.interfaces.emailaddress import (
     ...     IEmailAddressSet)
@@ -162,16 +164,22 @@
     <DBItem EmailAddressStatus.VALIDATED...
 
 The user can add a new address and set it as the preferred address. The
-setPreferredEmail() method updated the address's status.
+setPreferredEmail() method updated the address's status. This will generate a
+security email notification to the original preferred email address.
 
     >>> preferred_email = emailset.new(
     ...     '[e-mail]', p)
     >>> preferred_email.status
     <DBItem EmailAddressStatus.NEW...
 
+    >>> login('[e-mail]')
     >>> p.setPreferredEmail(preferred_email)
     >>> preferred_email.status
     <DBItem EmailAddressStatus.PREFERRED...
+    >>> transaction.commit()
+    >>> efrom, eto, emsg = stub.test_emails.pop()
+    >>> eto
+    ['[e-mail]']
 
     >>> login(ANONYMOUS)
 
@@ -472,7 +480,6 @@
     # notifications are sent when we add the owner as a member of
     # the team.
 
-    >>> from lp.services.mail import stub
     >>> stub.test_emails = []
 
     >>> not_a_person.convertToTeam(team_owner=ddaa)

=== modified file 'lib/lp/registry/doc/sshkey.txt'
--- lib/lp/registry/doc/sshkey.txt	2012-04-11 11:34:02 +0000
+++ lib/lp/registry/doc/sshkey.txt	2012-04-17 12:02:19 +0000
@@ -3,11 +3,13 @@
 
 Launchpad models SSH keys in a SSHKey class.
 
+    >>> from lp.testing import login_person
     >>> from zope.component import getUtility
     >>> from lp.registry.interfaces.person import IPersonSet
     >>> from lp.registry.interfaces.ssh import ISSHKeySet
     >>> personset = getUtility(IPersonSet)
     >>> name12 = personset.getByName('name12')
+    >>> ignored = login_person(name12)
     >>> [(key.keytype, key.comment) for key in name12.sshkeys]
     [(<DBItem SSHKeyType.DSA, (2) DSA>, u'andrew@trogdor')]
 
@@ -21,6 +23,7 @@
 Adding new keys is pretty easy:
 
     >>> foobar = personset.getByName('name16')
+    >>> ignored = login_person(foobar)
     >>> key = sshkeyset.new(
     ...     foobar, "ssh-rsa zzzNOT-REALLY This is just a test key")
     >>> key, key.keytext

=== added file 'lib/lp/registry/emailtemplates/person-details-change.txt'
--- lib/lp/registry/emailtemplates/person-details-change.txt	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/emailtemplates/person-details-change.txt	2012-04-17 12:02:19 +0000
@@ -0,0 +1,3 @@
+%(field_changed)s
+
+If you did not make this change, please open a new Question on Launchpad <https://answers.launchpad.net/launchpad/+addquestion> or visit #launchpad in IRC at freenode to alert staff of the issue.

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2012-04-03 15:34:50 +0000
+++ lib/lp/registry/interfaces/person.py	2012-04-17 12:02:19 +0000
@@ -1811,6 +1811,17 @@
         DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT days.
         """
 
+    def security_field_changed(subject, change_description,
+        recipient_emails=None):
+        """Trigger email when a secured field like preferredemail changes.
+        
+        :param recipient_emails: If supplied custom email addresses to notify.
+            This is used when a new preferred email address is set.
+        :param subject: The subject to use.
+        :param change_description: A textual description to use when notifying
+            about the change.
+        """
+
 
 class IPersonSpecialRestricted(Interface):
     """IPerson methods that require launchpad.Special permission to use."""

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2012-04-11 11:34:02 +0000
+++ lib/lp/registry/model/person.py	2012-04-17 12:02:19 +0000
@@ -283,6 +283,7 @@
     get_contact_email_addresses,
     get_email_template,
     )
+from lp.services.mail.sendmail import simple_sendmail
 from lp.services.oauth.model import (
     OAuthAccessToken,
     OAuthRequestToken,
@@ -2671,13 +2672,14 @@
                 "Any person's email address must provide the IEmailAddress "
                 "interface. %s doesn't." % email)
         assert email.personID == self.id
-
         existing_preferred_email = IMasterStore(EmailAddress).find(
             EmailAddress, personID=self.id,
             status=EmailAddressStatus.PREFERRED).one()
-
         if existing_preferred_email is not None:
+            original_recipients = existing_preferred_email.email
             existing_preferred_email.status = EmailAddressStatus.VALIDATED
+        else:
+            original_recipients = None
 
         email = removeSecurityProxy(email)
         IMasterObject(email).status = EmailAddressStatus.PREFERRED
@@ -2685,6 +2687,11 @@
 
         # Now we update our cache of the preferredemail.
         get_property_cache(self).preferredemail = email
+        if original_recipients:
+            self.security_field_changed(
+                "Preferred email address changed on Launchpad.",
+                "Your preferred email address is now <%s>." % email.email,
+                original_recipients)
 
     @cachedproperty
     def preferredemail(self):
@@ -3097,6 +3104,22 @@
             return True
         return False
 
+    def security_field_changed(self, subject, change_description,
+        recipient_emails=None):
+        """See `IPerson`."""
+        tpl_substitutions = dict(
+            field_changed=change_description,
+            )
+        template = get_email_template(
+            'person-details-change.txt', app='registry')
+        body = template % tpl_substitutions
+        from_addr = config.canonical.bounce_address
+        if not recipient_emails:
+            to_addrs = self.preferredemail.email
+        else:
+            to_addrs = recipient_emails
+        simple_sendmail(from_addr, to_addrs, subject, body)
+
     def transitionVisibility(self, visibility, user):
         if self.visibility == visibility:
             return
@@ -4625,6 +4648,14 @@
     keytext = StringCol(dbName='keytext', notNull=True)
     comment = StringCol(dbName='comment', notNull=True)
 
+    def destroySelf(self):
+        # For security reasons we want to notify the preferred email address
+        # that this sshkey has been removed.
+        self.person.security_field_changed(
+            "SSH Key removed from your Launchpad account.",
+            "The SSH Key %s was removed from your account." % self.comment)
+        super(SSHKey, self).destroySelf()
+
 
 class SSHKeySet:
     implements(ISSHKeySet)
@@ -4652,6 +4683,10 @@
         else:
             raise SSHKeyAdditionError
 
+        person.security_field_changed(
+            "New SSH key added to your account.",
+            "The SSH key '%s' has been added to your account." % comment)
+
         return SSHKey(person=person, keytype=keytype, keytext=keytext,
                       comment=comment)
 

=== modified file 'lib/lp/registry/stories/person/xx-add-email.txt'
--- lib/lp/registry/stories/person/xx-add-email.txt	2012-04-11 11:34:02 +0000
+++ lib/lp/registry/stories/person/xx-add-email.txt	2012-04-17 12:02:19 +0000
@@ -9,13 +9,15 @@
 
     >>> from lp.services.mail import stub
 
-    >>> browser = setupBrowser(auth='Basic [e-mail]:test')
+    >>> orig_email = '[e-mail]'
+    >>> browser = setupBrowser(auth='Basic %s:test' % orig_email)
     >>> browser.open('http://launchpad.dev/~name12')
     >>> browser.getLink(url='+editemails').click()
     >>> browser.url
     'http://launchpad.dev/~name12/+editemails'
 
-    >>> browser.getControl('Add a new address').value = '[e-mail]'
+    >>> new_email = '[e-mail]'
+    >>> browser.getControl('Add a new address').value = new_email
     >>> browser.getControl('Add', index=1).click()
 
     >>> browser.url
@@ -24,8 +26,18 @@
     ...     print msg
     A confirmation message has been sent to...
 
-    >>> from_addr, to_addrs, raw_msg = stub.test_emails.pop()
-    >>> assert not stub.test_emails
+There should be an email for the new address and one for the original
+preferred email address that the change was made to their account. Order gets
+mixed up so you have to check both spots.
+
+    >>> to_addrs = [to_addr for from_addr, to_addr, msg in stub.test_emails]
+    >>> assert len(to_addrs) == 2
+    >>> if orig_email in to_addrs[0][0]:
+    ...     assert new_email in to_addrs[1][0]
+    ... else:
+    ...     assert new_email in to_addrs[0][0]
+    ...     assert orig_email in to_addrs[1][0]
+    >>> stub.test_emails = []
 
 Trying to add the same email again (while it's unconfirmed) will only cause
 a new email to be sent with a new link. The links in the old and new email are
@@ -42,16 +54,24 @@
     A confirmation message has been sent to...
 
     # Extract the link (from the email we just sent) the user will have to
-    # use to finish the registration process.
+    # use to finish the registration process. There will be two emails so we
+    # must check them both in order to make sure it's sent.
     >>> from lp.services.verification.tests.logintoken import (
     ...     get_token_url_from_email)
-    >>> from_addr, to_addrs, raw_msg = stub.test_emails.pop()
-    >>> token_url = get_token_url_from_email(raw_msg)
+    >>> def check_tokenemail_and_reset():
+    ...     found, url, toaddr = False, None, None
+    ...     for from_addr, to_addrs, raw_msg in stub.test_emails:
+    ...         if 'token' in raw_msg:
+    ...             found = True
+    ...             url = get_token_url_from_email(raw_msg)
+    ...             toaddr = to_addrs
+    ...     assert found
+    ...     return url, toaddr
+    >>> token_url, toaddrs = check_tokenemail_and_reset()
     >>> token_url
     'http://launchpad.dev/token/...'
-    >>> to_addrs
+    >>> toaddrs
     ['[e-mail]']
-    >>> assert not stub.test_emails
 
 Follow the token link, to confirm the new email address.
 
@@ -103,13 +123,11 @@
 
     # Extract the link (from the email we just sent) the user will have to
     # use to finish the registration process.
-    >>> from_addr, to_addrs, raw_msg = stub.test_emails.pop()
-    >>> token_url = get_token_url_from_email(raw_msg)
+    >>> token_url, toaddrs = check_tokenemail_and_reset()
     >>> token_url
     'http://launchpad.dev/token/...'
-    >>> to_addrs
+    >>> toaddrs
     ['[e-mail]']
-    >>> assert not stub.test_emails
 
 Follow the token link, to confirm the new email address.
 

=== modified file 'lib/lp/registry/stories/person/xx-add-sshkey.txt'
--- lib/lp/registry/stories/person/xx-add-sshkey.txt	2012-01-15 11:06:57 +0000
+++ lib/lp/registry/stories/person/xx-add-sshkey.txt	2012-04-17 12:02:19 +0000
@@ -148,6 +148,20 @@
     ...
     UnexpectedFormData: ...
 
+If he removes a key then he will get a security warning email notification
+that the key has been removed.
+
+    >>> import email
+    >>> from lp.services.mail import stub
+    >>> stub.test_emails = []
+    >>> browser.open('http://launchpad.dev/~salgado/+editsshkeys')
+    >>> browser.getControl('Remove', index=0).click()
+    >>> from_addr, to_addr, msg = stub.test_emails.pop()
+    >>> to_addr
+    ['[e-mail]']
+    >>> payload = email.message_from_string(msg).get_payload()
+    >>> assert payload.startswith('The SSH Key')
+
 
 == Blacklisted keys ==
 

=== modified file 'lib/lp/registry/stories/person/xx-validate-email.txt'
--- lib/lp/registry/stories/person/xx-validate-email.txt	2012-04-11 11:34:02 +0000
+++ lib/lp/registry/stories/person/xx-validate-email.txt	2012-04-17 12:02:19 +0000
@@ -24,10 +24,14 @@
 Retrieve the email and make sure it was sent to the right address.
 
     >>> import email
-    >>> from_addr, to_addrs, raw_msg = stub.test_emails.pop()
-    >>> msg = email.message_from_string(raw_msg)
-    >>> print msg.get('to')
-    [e-mail]
+    >>> found = False
+    >>> raw_msg = None
+    >>> for from_addr, to_addrs, orig_msg in stub.test_emails:
+    ...     msg = email.message_from_string(orig_msg)
+    ...     if msg.get('to') == '[e-mail]':
+    ...        raw_msg = orig_msg
+    ...        found = True
+    >>> assert found
 
 Visit the token URL mentioned in the email, and get redirected to
 +validateemail.

=== modified file 'lib/lp/registry/stories/webservice/xx-person.txt'
--- lib/lp/registry/stories/webservice/xx-person.txt	2012-04-11 11:34:02 +0000
+++ lib/lp/registry/stories/webservice/xx-person.txt	2012-04-17 12:02:19 +0000
@@ -184,7 +184,7 @@
 The sample person "ssh-user" doesn't have any keys to begin with:
 
     >>> login('[e-mail]')
-    >>> person = factory.makePerson(name="ssh-user")
+    >>> person = factory.makePerson(name="ssh-user", email='[e-mail]')
     >>> logout()
     >>> sample_person = webservice.get("/~ssh-user").jsonBody()
     >>> sshkeys = sample_person['sshkeys_collection_link']
@@ -192,13 +192,21 @@
     http://.../~ssh-user/sshkeys
     >>> print_self_link_of_entries(webservice.get(sshkeys).jsonBody())
 
-Let's give "ssh-user" a key via the back door of our internal Python APIs:
+Let's give "ssh-user" a key via the back door of our internal Python APIs.
+This setting of the ssh key should trigger a notice that the key has been
+added.
 
     >>> from zope.component import getUtility
-    >>> from lp.registry.interfaces.person import IPersonSet
-    >>> login(ANONYMOUS)
-    >>> ssh_user = getUtility(IPersonSet).getByName('ssh-user')
-    >>> ssh_key = factory.makeSSHKey(ssh_user)
+    >>> from lp.services.mail import stub
+    >>> import transaction
+    >>> from lp.testing import person_logged_in
+    >>> with person_logged_in(person):
+    ...     ssh_key = factory.makeSSHKey(person)
+    ...     transaction.commit()
+    ...     efrom, eto, emsg = stub.test_emails.pop()
+    ...     eto
+    ['[e-mail]']
+
     >>> logout()
 
 Now when we get the sshkey collection for 'sssh-user' again, the key should

=== modified file 'lib/lp/registry/subscribers.py'
--- lib/lp/registry/subscribers.py	2012-04-11 11:34:02 +0000
+++ lib/lp/registry/subscribers.py	2012-04-17 12:02:19 +0000
@@ -14,7 +14,10 @@
 import pytz
 from zope.security.proxy import removeSecurityProxy
 
-from lp.registry.interfaces.person import IPerson
+from lp.registry.interfaces.person import (
+    IPerson,
+    IPersonViewRestricted,
+    )
 from lp.registry.interfaces.product import License
 from lp.services.config import config
 from lp.services.mail.helpers import get_email_template

=== modified file 'lib/lp/registry/tests/test_personnotification.py'
--- lib/lp/registry/tests/test_personnotification.py	2012-04-11 11:34:02 +0000
+++ lib/lp/registry/tests/test_personnotification.py	2012-04-17 12:02:19 +0000
@@ -18,6 +18,7 @@
 from zope.security.proxy import removeSecurityProxy
 
 from lp.registry.interfaces.personnotification import IPersonNotificationSet
+from lp.registry.model.personnotification import PersonNotification
 from lp.registry.scripts.personnotification import PersonNotificationManager
 from lp.services.config import config
 from lp.testing import (
@@ -25,6 +26,7 @@
     TestCaseWithFactory,
     )
 from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.mail_helpers import pop_notifications
 
 
 class TestPersonNotification(TestCaseWithFactory):

=== modified file 'lib/lp/services/authserver/tests/test_authserver.py'
--- lib/lp/services/authserver/tests/test_authserver.py	2012-04-11 11:34:02 +0000
+++ lib/lp/services/authserver/tests/test_authserver.py	2012-04-17 12:02:19 +0000
@@ -9,7 +9,10 @@
 from zope.publisher.xmlrpc import TestRequest
 
 from lp.services.authserver.xmlrpc import AuthServerAPIView
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    TestCaseWithFactory,
+    person_logged_in,
+    )
 from lp.testing.layers import DatabaseFunctionalLayer
 from lp.xmlrpc import faults
 from lp.xmlrpc.interfaces import IPrivateApplication
@@ -48,8 +51,9 @@
         # name of the key type (RSA or DSA) and the text of the keys under
         # 'keys' in the dict.
         new_person = self.factory.makePerson()
-        key = self.factory.makeSSHKey(person=new_person)
-        self.assertEqual(
-            dict(id=new_person.id, name=new_person.name,
-                 keys=[(key.keytype.title, key.keytext)]),
-            self.authserver.getUserAndSSHKeys(new_person.name))
+        with person_logged_in(new_person):
+            key = self.factory.makeSSHKey(person=new_person)
+            self.assertEqual(
+                dict(id=new_person.id, name=new_person.name,
+                     keys=[(key.keytype.title, key.keytext)]),
+                self.authserver.getUserAndSSHKeys(new_person.name))

=== modified file 'lib/lp/services/identity/model/emailaddress.py'
--- lib/lp/services/identity/model/emailaddress.py	2012-04-11 11:34:02 +0000
+++ lib/lp/services/identity/model/emailaddress.py	2012-04-17 12:02:19 +0000
@@ -19,6 +19,7 @@
     ForeignKey,
     StringCol,
     )
+
 from zope.interface import implements
 
 from lp.app.validators.email import valid_email

=== modified file 'lib/lp/services/mail/doc/sending-mail.txt'
--- lib/lp/services/mail/doc/sending-mail.txt	2012-04-11 11:34:02 +0000
+++ lib/lp/services/mail/doc/sending-mail.txt	2012-04-17 12:02:19 +0000
@@ -82,10 +82,13 @@
     ...     body=u'Content')
 
     >>> transaction.commit()
-    >>> from_addr, to_addr, raw_message = stub.test_emails.pop()
-    >>> msg = email.message_from_string(raw_message)
-    >>> msg['From']
-    'Sample Person <[e-mail]>'
+    >>> found = False
+    >>> for from_addr, to_addr, raw_message in stub.test_emails:
+    ...     msg = email.message_from_string(raw_message)
+    ...     if msg['From'] == 'Sample Person <[e-mail]>':
+    ...         found = True
+    >>> assert found
+    >>> stub.test_emails = []
 
 
 To make a header appear more than once in the sent message (e.g.

=== modified file 'lib/lp/services/verification/browser/tests/logintoken-corner-cases.txt'
--- lib/lp/services/verification/browser/tests/logintoken-corner-cases.txt	2012-04-11 11:34:02 +0000
+++ lib/lp/services/verification/browser/tests/logintoken-corner-cases.txt	2012-04-17 12:02:19 +0000
@@ -14,6 +14,7 @@
 redirect to the default token view. This would happen if for example the
 user tried to re-post the form after validating one of their email addresses.
 
+    >>> from lp.testing import login_person
     >>> from lp.services.verification.browser.logintoken import (
     ...     ValidateEmailView)
     >>> from lp.services.verification.interfaces.authtoken import (
@@ -24,6 +25,7 @@
     >>> from lp.registry.interfaces.person import IPersonSet
 
     >>> foo_bar = getUtility(IPersonSet).getByName('name16')
+    >>> ignored = login_person(foo_bar)
     >>> token = getUtility(ILoginTokenSet).new(
     ...     requester=foo_bar, requesteremail='[e-mail]',
     ...     email='[e-mail]', tokentype=LoginTokenType.VALIDATEEMAIL)

=== modified file 'lib/lp/services/verification/browser/tests/test_logintoken.py'
--- lib/lp/services/verification/browser/tests/test_logintoken.py	2012-04-11 11:34:02 +0000
+++ lib/lp/services/verification/browser/tests/test_logintoken.py	2012-04-17 12:02:19 +0000
@@ -12,7 +12,10 @@
     )
 from lp.services.verification.interfaces.authtoken import LoginTokenType
 from lp.services.verification.interfaces.logintoken import ILoginTokenSet
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    TestCaseWithFactory,
+    person_logged_in,
+    )
 from lp.testing.deprecated import LaunchpadFormHarness
 from lp.testing.layers import DatabaseFunctionalLayer
 
@@ -45,10 +48,11 @@
         self._testCancelAction(ValidateGPGKeyView, token)
 
     def test_ValidateEmailView(self):
-        token = getUtility(ILoginTokenSet).new(
-            self.person, self.email, '[e-mail]',
-            LoginTokenType.VALIDATEEMAIL)
-        self._testCancelAction(ValidateEmailView, token)
+        with person_logged_in(self.person):
+            token = getUtility(ILoginTokenSet).new(
+                self.person, self.email, '[e-mail]',
+                LoginTokenType.VALIDATEEMAIL)
+            self._testCancelAction(ValidateEmailView, token)
 
     def _testCancelAction(self, view_class, token):
         """Test the 'Cancel' action of the given view, using the given token.

=== modified file 'lib/lp/services/verification/doc/logintoken.txt'
--- lib/lp/services/verification/doc/logintoken.txt	2011-12-30 06:14:56 +0000
+++ lib/lp/services/verification/doc/logintoken.txt	2012-04-17 12:02:19 +0000
@@ -34,16 +34,21 @@
     ...     LoginTokenType.VALIDATEEMAIL)
     >>> token.sendEmailValidationRequest()
     >>> transaction.commit()
-    >>> from_addr, to_addrs, raw_msg = stub.test_emails.pop()
-    >>> to_addrs
-    ['[e-mail]']
+    >>> found = False
+    >>> found_msg = None
+    >>> for from_addr, to_addrs, raw_msg in stub.test_emails:
+    ...     if to_addrs == ['[e-mail]']:
+    ...         found = True
+    ...         found_msg = raw_msg
+    >>> assert found
+    >>> stub.test_emails = []
 
 The email does not have a precedence header because the user implicitly
 requested it to complete his task.
 
     >>> import email
 
-    >>> msg = email.message_from_string(raw_msg)
+    >>> msg = email.message_from_string(found_msg)
     >>> print msg['Precedence']
     None
 

=== modified file 'lib/lp/services/verification/model/logintoken.py'
--- lib/lp/services/verification/model/logintoken.py	2012-04-11 11:34:02 +0000
+++ lib/lp/services/verification/model/logintoken.py	2012-04-17 12:02:19 +0000
@@ -114,6 +114,10 @@
         message = template % replacements
         subject = "Launchpad: Validate your email address"
         self._send_email("Launchpad Email Validator", subject, message)
+        self.requester.security_field_changed(
+            "A new email address is being added to your Launchpad account.",
+            "<%s> will be activated for your account when you follow the "
+            "instructions that were sent to <%s>." % (self.email, self.email))
 
     def sendGPGValidationRequest(self, key):
         """See ILoginToken."""
@@ -391,7 +395,6 @@
             # Aha! According to our policy, we shouldn't raise ValueError.
             raise ValueError(
                 "tokentype is not an item of LoginTokenType: %s" % tokentype)
-
         token = create_unique_token_for_table(20, LoginToken.token)
         return LoginToken(requester=requester, requesteremail=requesteremail,
                           email=email, token=token, tokentype=tokentype,