launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/email-authentication-errors into lp:launchpad.

Commit message:
send users emails about signing errors.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1057026 in Launchpad itself: "IOError processing bug mail when signing key is not registered"
  https://bugs.launchpad.net/launchpad/+bug/1057026

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/email-authentication-errors/+merge/126801

OOPS-a337346d65f5d5476f10a93df6e3abcb shows a user send a bug report via
email. It was signed with an unknown key. An IOError was raised when
lib/lp/bugs/mail/errortemplates/key-not-registered.txt could not be
found. The actual error template is located at
lib/lp/services/mail/errortemplates/key-not-registered.txt.

services.mail.helpers.ensure_not_weakly_authenticated passes the
error_templates arg to get_error_message, but since the helpers module
manages key-not-registered.txt (and not-signed.txt) None should be
passed for error_templates.



--------------------------------------------------------------------

RULES

    Pre-implementation: no one
    * Update services.mail.helpers.ensure_not_weakly_authenticated
      to not pass the error_templates arg to get_error_message when
      it is working with the special messages.
    * I think we need a test for both special error templates that
      ensure_not_weakly_authenticated passes None for error_templates.

    ADDENDUM:
    * No callsite uses the no_key_template kwarg. It can be moved
      entirely into the method to avoid naming/path confusion.
    * The error_template kwarg is used only only once. The wording
      between the bugs template and the standard template is a little
      different. If I find common wording, we can remove all the kwargs
      and the unused bug template.

QA

    * Clear the staging mail box.
    * send an email to new@xxxxxxxxxxxxxxxxxxxxxxxxxxxx that is
      signed with a key not registered in Lp with this body:
        affects gdp
      testing 123.
    * Check the staging mailbox and verify there is a message stating that
      your OpenPGP key isn't imported into Launchpad.


LINT

    lib/lp/bugs/mail/handler.py
    lib/lp/bugs/tests/bugs-emailinterface.txt
    lib/lp/services/mail/helpers.py
    lib/lp/services/mail/errortemplates/not-signed.txt
    lib/lp/services/mail/tests/test_helpers.py


TEST

    ./bin/test -vvc -t NotWeaklyAuthenticated lp.services.mail.tests
    ./bin/test -vvc -t bugs-emailinterface.txt lp.bugs.tests


IMPLEMENTATION

The initial fix was easy. I added a test based on
test_weakly_authenticated_with_sig that passed all the kwargs to produce
the IOError. I fixed the code to never pass None for error_templates.
    lib/lp/services/mail/helpers.py
    lib/lp/services/mail/tests/test_helpers.py

Then I looked at all the callsites for ensure_not_weakly_authenticated
and realised that I can remove no_key_template now, and with a small
change to bugs.mail, I could remove the other two kwargs. I merged the two
not-signed email templates, they are both a single sentence. I liked the
how the bug mail sentence ended so I combined the sentences. I then found
the message was tested twice. I deleted the first test because the section
was not about errors. Once the kwargs were gone, my test was unneeded, so
I deleted it.
Note, I think bugs-emailinterface.txt duplicates a lot of tests in
lp.bugs.mail.tests. We might be able to delete 1,500 lines of code. Once
    lib/lp/bugs/mail/handler.py
    lib/lp/bugs/tests/bugs-emailinterface.txt
    lib/lp/services/mail/helpers.py
    lib/lp/services/mail/errortemplates/not-signed.txt
    lib/lp/services/mail/tests/test_helpers.py
-- 
https://code.launchpad.net/~sinzui/launchpad/email-authentication-errors/+merge/126801
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/email-authentication-errors into lp:launchpad.

=== removed file 'lib/lp/bugs/mail/errortemplates/unauthenticated-bug-creation.txt'
--- lib/lp/bugs/mail/errortemplates/unauthenticated-bug-creation.txt	2010-11-25 04:42:51 +0000
+++ lib/lp/bugs/mail/errortemplates/unauthenticated-bug-creation.txt	1970-01-01 00:00:00 +0000
@@ -1,2 +0,0 @@
-To report bugs by e-mail, you need to sign the message with an OpenPGP key
-that is registered in Launchpad.

=== modified file 'lib/lp/bugs/mail/handler.py'
--- lib/lp/bugs/mail/handler.py	2012-06-29 08:40:05 +0000
+++ lib/lp/bugs/mail/handler.py	2012-09-27 21:05:25 +0000
@@ -227,9 +227,7 @@
         # We send a different failure message for attempts to create a new
         # bug.
         elif to_user.lower() == 'new':
-            ensure_not_weakly_authenticated(signed_msg, CONTEXT,
-                'unauthenticated-bug-creation.txt',
-                error_templates=error_templates)
+            ensure_not_weakly_authenticated(signed_msg, CONTEXT)
         elif len(commands) > 0:
             ensure_not_weakly_authenticated(signed_msg, CONTEXT)
         if to_user.lower() == 'new':

=== modified file 'lib/lp/bugs/tests/bugs-emailinterface.txt'
--- lib/lp/bugs/tests/bugs-emailinterface.txt	2012-09-07 21:00:46 +0000
+++ lib/lp/bugs/tests/bugs-emailinterface.txt	2012-09-27 21:05:25 +0000
@@ -469,17 +469,6 @@
     >>> print bug_one.title
     Better summary
 
-A different error message is sent, though:
-
-    >>> print_latest_email()
-    Subject: Submit Request Failure
-    To: test@xxxxxxxxxxxxx
-    <BLANKLINE>
-    ...
-    The message you sent included commands to modify the bug report, but you
-    didn't sign the message with your OpenPGP key.
-    ...
-
 
 If we don't include any commands in the comment, it will be added
 to the bug:
@@ -1824,8 +1813,9 @@
     To: test@xxxxxxxxxxxxx
     <BLANKLINE>
     ...
-    To report bugs by e-mail, you need to sign the message with an OpenPGP key
-    that is registered in Launchpad.
+    The message you sent included commands to modify the bug report,
+    but you didn't sign the message with an OpenPGP key that is
+    registered in Launchpad.
     ...
 
 A submit without specifying on what we want to file the bug on:

=== modified file 'lib/lp/services/mail/errortemplates/not-signed.txt'
--- lib/lp/services/mail/errortemplates/not-signed.txt	2011-08-11 22:02:03 +0000
+++ lib/lp/services/mail/errortemplates/not-signed.txt	2012-09-27 21:05:25 +0000
@@ -1,2 +1,3 @@
 The message you sent included commands to modify the %(context)s,
-but you didn't sign the message with your OpenPGP key.
+but you didn't sign the message with an OpenPGP key that is
+registered in Launchpad.

=== modified file 'lib/lp/services/mail/helpers.py'
--- lib/lp/services/mail/helpers.py	2012-06-20 08:15:51 +0000
+++ lib/lp/services/mail/helpers.py	2012-09-27 21:05:25 +0000
@@ -21,6 +21,10 @@
 from lp.services.webapp.interfaces import ILaunchBag
 
 
+NO_KEY_TEMPLATE = 'key-not-registered.txt'
+NOT_SIGNED_TEMPLATE = 'not-signed.txt'
+
+
 class IncomingEmailError(Exception):
     """Indicates that something went wrong processing the mail."""
 
@@ -169,9 +173,7 @@
 
 
 def ensure_not_weakly_authenticated(signed_msg, context,
-                                    error_template='not-signed.txt',
-                                    no_key_template='key-not-registered.txt',
-                                    error_templates=None):
+                                    error_template='not-signed.txt'):
     """Make sure that the current principal is not weakly authenticated.
 
     NB: While handling an email, the authentication state is stored partly in
@@ -188,14 +190,12 @@
     if IWeaklyAuthenticatedPrincipal.providedBy(cur_principal):
         if signed_msg.signature is None:
             error_message = get_error_message(
-                error_template, error_templates=error_templates,
-                context=context)
+                NOT_SIGNED_TEMPLATE, None, context=context)
         else:
             import_url = canonical_url(
                 getUtility(ILaunchBag).user) + '/+editpgpkeys'
             error_message = get_error_message(
-                no_key_template, error_templates,
-                import_url=import_url, context=context)
+                NO_KEY_TEMPLATE, None, import_url=import_url, context=context)
         raise IncomingEmailError(error_message)
 
 

=== modified file 'lib/lp/services/mail/tests/test_helpers.py'
--- lib/lp/services/mail/tests/test_helpers.py	2012-06-20 08:15:51 +0000
+++ lib/lp/services/mail/tests/test_helpers.py	2012-09-27 21:05:25 +0000
@@ -177,7 +177,8 @@
             signed_msg, 'test')
         self.assertEqual(
             "The message you sent included commands to modify the test,\n"
-            "but you didn't sign the message with your OpenPGP key.\n",
+            "but you didn't sign the message with an OpenPGP key that is\n"
+            "registered in Launchpad.\n",
             error.message)
 
     def test_weakly_authenticated_with_sig(self):