launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #12580
[Merge] lp:~sinzui/launchpad/email-authentication-errors into lp:launchpad
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):
Follow ups