launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06979
[Merge] lp:~rharding/launchpad/email_notice_959482 into lp:launchpad
Richard Harding has proposed merging lp:~rharding/launchpad/email_notice_959482 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~rharding/launchpad/email_notice_959482/+merge/99995
= 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/removes 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 ==
We create a new event that can be triggered. The event expects to get the bit of user data that has changed and will generate an email notification with a short description on what was altered.
Note that in order to send a notification that the preferred email has changed, we actually need to notify the previous preferred email. This required us to change the PersonNotification to allow us to specify a specific email address since we can no longer just notify the updated preferred email.
== Tests ==
test_subscribers
test_personnotification
== Demo and Q/A ==
Changing any of the listed properties should schedule an email with the correct note about the change.
== Lint ==
Linting changed files:
lib/lp/registry/configure.zcml
lib/lp/registry/subscribers.py
lib/lp/registry/browser/person.py
lib/lp/registry/emailtemplates/person-details-change.txt
lib/lp/registry/model/person.py
lib/lp/registry/model/personnotification.py
lib/lp/registry/tests/test_personnotification.py
lib/lp/registry/tests/test_subscribers.py
lib/lp/services/identity/model/emailaddress.py
lib/lp/services/verification/model/logintoken.py
--
https://code.launchpad.net/~rharding/launchpad/email_notice_959482/+merge/99995
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rharding/launchpad/email_notice_959482 into lp:launchpad.
=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py 2012-03-02 07:53:53 +0000
+++ lib/lp/registry/browser/person.py 2012-04-03 15:00:32 +0000
@@ -2649,7 +2649,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/configure.zcml'
--- lib/lp/registry/configure.zcml 2012-03-22 15:12:19 +0000
+++ lib/lp/registry/configure.zcml 2012-04-03 15:00:32 +0000
@@ -1072,6 +1072,9 @@
<subscriber
for="lp.registry.interfaces.product.IProduct zope.lifecycleevent.interfaces.IObjectModifiedEvent"
handler="lp.registry.subscribers.product_licenses_modified"/>
+ <subscriber
+ for="lp.registry.interfaces.person.IPersonViewRestricted zope.lifecycleevent.interfaces.IObjectModifiedEvent"
+ handler="lp.registry.subscribers.person_alteration_security_notice"/>
<class
class="lp.registry.model.mailinglist.MailingList">
<allow
=== 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-03 15:00:32 +0000
@@ -0,0 +1,8 @@
+Hello %(user_name)s, your account has recently changed on Launchpad.net.
+
+Change:
+%(field_changed)s
+
+If you did not intend to 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/model/person.py'
--- lib/lp/registry/model/person.py 2012-03-29 06:02:46 +0000
+++ lib/lp/registry/model/person.py 2012-04-03 15:00:32 +0000
@@ -40,6 +40,8 @@
import weakref
from lazr.delegates import delegates
+from lazr.lifecycle.event import ObjectModifiedEvent
+from lazr.lifecycle.snapshot import Snapshot
from lazr.restful.utils import (
get_current_browser_request,
smartquote,
@@ -97,6 +99,7 @@
classImplements,
implementer,
implements,
+ providedBy,
)
from zope.lifecycleevent import ObjectCreatedEvent
from zope.publisher.interfaces import Unauthorized
@@ -2672,6 +2675,8 @@
"interface. %s doesn't." % email)
assert email.personID == self.id
+ person_before_mod = Snapshot(self, providing=providedBy(self))
+
existing_preferred_email = IMasterStore(EmailAddress).find(
EmailAddress, personID=self.id,
status=EmailAddressStatus.PREFERRED).one()
@@ -2685,6 +2690,11 @@
# Now we update our cache of the preferredemail.
get_property_cache(self).preferredemail = email
+ # Make sure we notify that the property was changed, but only if we've
+ # changed the preferred email and not set an initial one.
+ if person_before_mod.preferredemail:
+ notify(ObjectModifiedEvent(self, person_before_mod,
+ ['preferredemail'], user=self))
@cachedproperty
def preferredemail(self):
@@ -4619,6 +4629,14 @@
keytext = StringCol(dbName='keytext', notNull=True)
comment = StringCol(dbName='comment', notNull=True)
+ def destroySelf(self):
+ """We trigger some events on removal."""
+ # For security reasons we want to notify the preferred email address
+ # that this sshkey has been removed.
+ notify(ObjectModifiedEvent(self.person, self.person,
+ ['removedsshkey'], user=self.person))
+ super(SSHKey, self).destroySelf()
+
class SSHKeySet:
implements(ISSHKeySet)
@@ -4646,6 +4664,11 @@
else:
raise SSHKeyAdditionError
+ # We need to make sure we notify the user that the ssh key is added in
+ # case they didn't request this change.
+ notify(ObjectModifiedEvent(person, person,
+ ['newsshkey'], user=person))
+
return SSHKey(person=person, keytype=keytype, keytext=keytext,
comment=comment)
=== modified file 'lib/lp/registry/model/personnotification.py'
--- lib/lp/registry/model/personnotification.py 2011-12-30 06:14:56 +0000
+++ lib/lp/registry/model/personnotification.py 2012-04-03 15:00:32 +0000
@@ -62,12 +62,20 @@
"""See `IPersonNotification`."""
return len(self.to_addresses) > 0
- def send(self, logger=None):
- """See `IPersonNotification`."""
- if not self.can_send:
+ def send(self, logger=None, sendto=None):
+ """See `IPersonNotification`.
+
+ :param sendto: Name/email tuple to provide a custom recipient address
+ for cases when preferredemail isn't the intended target.
+ """
+ if not self.can_send and not sendto:
raise AssertionError(
"Can't send a notification to a person without an email.")
- to_addresses = self.to_addresses
+ if sendto:
+ # We're expecting a tuple of (name, email) for the sendto.
+ to_addresses = [format_address(*sendto)]
+ else:
+ to_addresses = self.to_addresses
if logger:
logger.info("Sending notification to %r." % to_addresses)
from_addr = config.canonical.bounce_address
=== modified file 'lib/lp/registry/subscribers.py'
--- lib/lp/registry/subscribers.py 2012-03-22 23:21:24 +0000
+++ lib/lp/registry/subscribers.py 2012-04-03 15:00:32 +0000
@@ -14,8 +14,12 @@
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.registry.model.personnotification import PersonNotification
from lp.services.config import config
from lp.services.mail.helpers import get_email_template
from lp.services.mail.sendmail import (
@@ -29,6 +33,15 @@
)
+PERSON_DATA_MONITORED = {
+ 'preferredemail': 'Preferred email address changed.',
+ 'newemail': 'Email address added.',
+ 'removedemail': 'Email address removed.',
+ 'newsshkey': 'SSH key added.',
+ 'removedsshkey': 'SSH key removed.',
+}
+
+
def product_licenses_modified(product, event):
"""Send a notification if licenses changed and a license is special."""
if not event.edited_fields:
@@ -145,3 +158,66 @@
naked_product.reviewer_whiteboard = whiteboard
else:
naked_product.reviewer_whiteboard += '\n' + whiteboard
+
+
+def person_alteration_security_notice(person, event):
+ """Send a notification if important details on a person change."""
+ if not event.edited_fields:
+ return
+
+ # We want to keep tabs on which fields changed so we can attempt to have
+ # an intelligent reply message on what just happened.
+ changed_fields = set(PERSON_DATA_MONITORED.keys()) & set(event.edited_fields)
+
+ if changed_fields:
+ user = IPersonViewRestricted(event.user)
+ original_object = event.object_before_modification
+ prev_preferred_email = original_object.preferredemail.email
+
+ # In theory we could have a list of changed fields, but in practice we
+ # don't see that. Shortcutting to just grab the first changed field.
+ notification = PersonAlterationSecurityNotification(
+ changed_fields.pop(), user,
+ override_noticeto=(user.displayname, prev_preferred_email))
+ notification.send()
+
+
+class PersonAlterationSecurityNotification(object):
+ """Schedule an email notification to the user about account changes"""
+
+ def __init__(self, field, user, override_noticeto=None):
+ """Notify the user that their account has changed
+
+ :param field: the bit of account data that's altered
+ :param user: the user that changed
+ """
+ self.changed = field
+ self.user = user
+ self.notification = PersonNotification()
+ self.notification.person = user
+ self.override_noticeto = override_noticeto
+
+ def getTemplateName(self):
+ """Return the name of the email template to use in the notification."""
+ return 'person-details-change.txt'
+
+ def send(self):
+ """Send the notification to the user about their account change."""
+ self.notification.subject = (
+ "Your Launchpad.net account details have changed."
+ )
+ tpl_substitutions = dict(
+ user_displayname=self.user.displayname,
+ user_name=self.user.name,
+ field_changed=PERSON_DATA_MONITORED[self.changed]
+ )
+ template = get_email_template(
+ self.getTemplateName(), app='registry')
+ message = template % tpl_substitutions
+ self.notification.body = message
+ if self.override_noticeto:
+ self.notification.send(
+ sendto=(self.user.displayname, self.override_noticeto))
+ else:
+ self.notification.send()
+ return True
=== modified file 'lib/lp/registry/tests/test_personnotification.py'
--- lib/lp/registry/tests/test_personnotification.py 2012-01-01 02:58:52 +0000
+++ lib/lp/registry/tests/test_personnotification.py 2012-04-03 15:00:32 +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):
@@ -64,6 +66,19 @@
self.assertEqual([email], notification.to_addresses)
self.assertTrue(notification.can_send)
+ def test_to_specified_email(self):
+ """We might want to notify a non-preferred email address."""
+ user = self.factory.makePerson()
+ note = PersonNotification()
+ note.person = user
+ note.body = 'body'
+ note.subject = 'subject'
+ pop_notifications()
+ note.send(send_to=(user.displayname, 'testing@xxxxxx'))
+ notifications = pop_notifications()
+ self.assertEqual(1, len(notifications))
+ self.assertTrue('testing@xxxxxx' in notifications[0].get('To'))
+
class TestPersonNotificationManager(TestCaseWithFactory):
"""Tests for the PersonNotificationManager use in scripts."""
=== modified file 'lib/lp/registry/tests/test_subscribers.py'
--- lib/lp/registry/tests/test_subscribers.py 2012-03-22 23:21:24 +0000
+++ lib/lp/registry/tests/test_subscribers.py 2012-04-03 15:00:32 +0000
@@ -8,20 +8,28 @@
from datetime import datetime
from lazr.lifecycle.event import ObjectModifiedEvent
+from lazr.lifecycle.interfaces import IObjectModifiedEvent
import pytz
+from zope.component import getUtility
from zope.security.proxy import removeSecurityProxy
+from lp.registry.interfaces.person import IPersonViewRestricted
from lp.registry.interfaces.product import License
from lp.registry.subscribers import (
LicenseNotification,
+ person_alteration_security_notice,
product_licenses_modified,
)
+from lp.services.verification.interfaces.logintoken import ILoginTokenSet
+from lp.services.verification.interfaces.authtoken import LoginTokenType
from lp.services.webapp.publisher import get_current_browser_request
from lp.testing import (
login_person,
logout,
+ person_logged_in,
TestCaseWithFactory,
)
+from lp.testing.event import TestEventListener
from lp.testing.layers import DatabaseFunctionalLayer
from lp.testing.mail_helpers import pop_notifications
@@ -258,3 +266,189 @@
product.commercial_subscription.date_expires.date().isoformat())
self.assertEqual(message, notification.getCommercialUseMessage())
self.assertEqual(message, notification.getCommercialUseMessage())
+
+
+class TestPersonDataModifiedHandler(TestCaseWithFactory):
+ """When some details of a person change, we need to notify the user."""
+ layer = DatabaseFunctionalLayer
+
+ def test_handler_generates_notification(self):
+ """Manually firing event should generate a proper notification."""
+ person = self.factory.makePerson(email='test@xxxxxxx')
+ login_person(person)
+ pop_notifications()
+ # After/before objects and list of edited fields.
+ event = ObjectModifiedEvent(person, person, ['preferredemail'])
+ person_alteration_security_notice(person, event)
+ notifications = pop_notifications()
+ self.assertEqual(1, len(notifications))
+ self.assertTrue('test@xxxxxxx' in notifications[0].get('To'))
+
+ def test_event_generates_notification(self):
+ """Triggering the event should generate a proper notification."""
+ person = self.factory.makePerson(email='test@xxxxxxx')
+ login_person(person)
+ pop_notifications()
+ new_email = self.factory.makeEmail('test@xxxxxxxx', person)
+ person.setPreferredEmail(new_email)
+ notifications = pop_notifications()
+ self.assertEqual(1, len(notifications))
+ self.assertTrue('test@xxxxxxx' in notifications[0].get('To'))
+ self.assertTrue(
+ 'Preferred email address' in notifications[0].as_string())
+
+
+class TestPersonAlterationEvent(TestCaseWithFactory):
+ """Test that the events are fired when the person is changed."""
+
+ layer = DatabaseFunctionalLayer
+ event_listener = None
+
+ def setup_event_listener(self):
+ self.events = []
+ if self.event_listener is None:
+ self.event_listener = TestEventListener(
+ IPersonViewRestricted, IObjectModifiedEvent, self.on_event)
+ else:
+ self.event_listener._active = True
+ self.addCleanup(self.event_listener.unregister)
+
+ def on_event(self, thing, event):
+ self.events.append(event)
+
+ def test_change_preferredemail(self):
+ # The project_reviewed property is not reset, if the new licenses
+ # are identical to the current licenses.
+ pop_notifications()
+ person = self.factory.makePerson(email='test@xxxxxxx')
+ new_email = self.factory.makeEmail('test@xxxxxxxx', person)
+ self.setup_event_listener()
+ with person_logged_in(person):
+ person.setPreferredEmail(new_email)
+ # Assert form within the context manager to get access to the
+ # email values.
+ self.assertEqual('test@xxxxxxxx', person.preferredemail.email)
+ self.assertEqual(1, len(self.events))
+
+ evt = self.events[0]
+ self.assertEqual(person, evt.object)
+ self.assertEqual('test@xxxxxxx',
+ evt.object_before_modification.preferredemail.email)
+ self.assertEqual(['preferredemail'], evt.edited_fields)
+
+ def test_no_event_on_no_change(self):
+ """If there's no change to the preferred email there's no event"""
+ pop_notifications()
+ person = self.factory.makePerson(email='test@xxxxxxx')
+ self.setup_event_listener()
+ with person_logged_in(person):
+ person.displayname = 'changed'
+ # Assert form within the context manager to get access to the
+ # email values.
+ self.assertEqual('test@xxxxxxx', person.preferredemail.email)
+ self.assertEqual(0, len(self.events))
+
+ def test_removed_email_address(self):
+ """When an email address is removed we should notify."""
+ pop_notifications()
+ self.setup_event_listener()
+ person = self.factory.makePerson(email='test@xxxxxxx')
+
+ with person_logged_in(person):
+ secondary_email = self.factory.makeEmail('test@xxxxxxxxxx', person)
+ secondary_email.destroySelf()
+ # We should only have one email address, the preferred.
+ self.assertEqual('test@xxxxxxx', person.preferredemail.email)
+ # The preferred email doesn't show in the list of validated emails
+ # so there are none left once the destroy is done.
+ self.assertEqual(0, person.validatedemails.count())
+ self.assertEqual(1, len(self.events))
+ evt = self.events[0]
+ self.assertEqual(person, evt.object)
+ self.assertEqual(['removedemail'], evt.edited_fields)
+
+ # The notice of this should be going to the preferred email user.
+ notifications = pop_notifications()
+ self.assertTrue('test@xxxxxxx' in notifications[0].get('To'))
+ self.assertTrue(
+ 'Email address removed' in notifications[0].as_string())
+
+ def test_new_email_request(self):
+ """When an email address is added we should notify.
+
+ We want to send the notification when the new email address is
+ requested. This doesn't actually create an email address yet. It
+ builds a LoginToken that the user must ack before the email address is
+ added. The issue is security in alerting the owner of the account that
+ a new address is being requested, so we need to notify at the time of
+ request and not wait for the user to ack the new email address.
+ """
+ pop_notifications()
+ self.setup_event_listener()
+ person = self.factory.makePerson(email='test@xxxxxxx')
+
+ with person_logged_in(person):
+ secondary_email = self.factory.makeEmail('test@xxxxxxxxxx', person)
+ # The way that a new email address gets requested is through the
+ # LoginToken done in the browser/person action_add_email.
+ getUtility(ILoginTokenSet).new(person,
+ person.preferredemail.email,
+ secondary_email.email,
+ LoginTokenType.VALIDATEEMAIL)
+ self.assertEqual(1, len(self.events))
+ evt = self.events[0]
+ self.assertEqual(person, evt.object)
+ self.assertEqual(['newemail'], evt.edited_fields)
+
+ # The notice of this should be going to the preferred email user.
+ notifications = pop_notifications()
+ self.assertTrue('test@xxxxxxx' in notifications[0].get('To'))
+ self.assertTrue(
+ 'Email address added' in notifications[0].as_string())
+
+ def test_new_ssh_key(self):
+ """We also want the notification when users add ssh keys."""
+ pop_notifications()
+ self.setup_event_listener()
+ person = self.factory.makePerson(email='test@xxxxxxx')
+
+ with person_logged_in(person):
+ # The factory method generates a fresh ssh key through the
+ # SSHKeySet that we're bound into. The view uses the same ssh key
+ # set .new method so it's safe to just let the factory trigger our
+ # event for us.
+ self.factory.makeSSHKey(person)
+ self.assertEqual(1, len(self.events))
+ evt = self.events[0]
+ self.assertEqual(person, evt.object)
+ self.assertEqual(['newsshkey'], evt.edited_fields)
+
+ # The notice of this should be going to the preferred email user.
+ notifications = pop_notifications()
+ self.assertTrue('test@xxxxxxx' in notifications[0].get('To'))
+ self.assertTrue(
+ 'SSH key added' in notifications[0].as_string())
+
+ def test_remove_ssh_key(self):
+ """Notifications should fire when we remove an ssh key."""
+ pop_notifications()
+ self.setup_event_listener()
+ person = self.factory.makePerson(email='test@xxxxxxx')
+
+ with person_logged_in(person):
+ sshkey = self.factory.makeSSHKey(person)
+ # Make sure to clear notifications/events before we remove the key.
+ pop_notifications()
+ self.events = []
+ sshkey.destroySelf()
+ self.assertEqual(1, len(self.events))
+ evt = self.events[0]
+ self.assertEqual(person, evt.object)
+ self.assertEqual(['removedsshkey'], evt.edited_fields)
+
+ # The notice of this should be going to the preferred email user.
+ notifications = pop_notifications()
+ self.assertTrue('test@xxxxxxx' in notifications[0].get('To'))
+ self.assertTrue(
+ 'SSH key removed' in notifications[0].as_string())
+
=== modified file 'lib/lp/services/identity/model/emailaddress.py'
--- lib/lp/services/identity/model/emailaddress.py 2012-01-05 00:23:45 +0000
+++ lib/lp/services/identity/model/emailaddress.py 2012-04-03 15:00:32 +0000
@@ -19,7 +19,10 @@
ForeignKey,
StringCol,
)
+
+from zope.event import notify
from zope.interface import implements
+from lazr.lifecycle.event import ObjectModifiedEvent
from lp.app.validators.email import valid_email
from lp.services.database.enumcol import EnumCol
@@ -85,6 +88,12 @@
for subscription in MailingListSubscription.selectBy(
email_address=self):
subscription.destroySelf()
+
+ # We need to notify the preferred email address that this address has
+ # been removed.
+ notify(ObjectModifiedEvent(self.person, self.person,
+ ['removedemail'], user=self.person))
+
super(EmailAddress, self).destroySelf()
@property
=== modified file 'lib/lp/services/verification/model/logintoken.py'
--- lib/lp/services/verification/model/logintoken.py 2012-01-05 00:23:45 +0000
+++ lib/lp/services/verification/model/logintoken.py 2012-04-03 15:00:32 +0000
@@ -19,8 +19,10 @@
)
from storm.expr import And
from zope.component import getUtility
+from zope.event import notify
from zope.interface import implements
from zope.security.proxy import removeSecurityProxy
+from lazr.lifecycle.event import ObjectModifiedEvent
from lp.app.errors import NotFoundError
from lp.app.validators.email import valid_email
@@ -392,6 +394,12 @@
raise ValueError(
"tokentype is not an item of LoginTokenType: %s" % tokentype)
+ # We want to alert the preferred email that this new address has been
+ # requested.
+ if tokentype == LoginTokenType.VALIDATEEMAIL:
+ notify(ObjectModifiedEvent(requester, requester,
+ ['newemail'], user=requester))
+
token = create_unique_token_for_table(20, LoginToken.token)
return LoginToken(requester=requester, requesteremail=requesteremail,
email=email, token=token, tokentype=tokentype,
