Merge lp:~sinzui/launchpad/email-authentication-errors into lp:launchpad

Proposed by Curtis Hovey
Status: Merged
Approved by: j.c.sackett
Approved revision: no longer in the source branch.
Merged at revision: 16051
Proposed branch: lp:~sinzui/launchpad/email-authentication-errors
Merge into: lp:launchpad
Diff against target: 121 lines (+15/-27)
6 files modified
lib/lp/bugs/mail/errortemplates/unauthenticated-bug-creation.txt (+0/-2)
lib/lp/bugs/mail/handler.py (+1/-3)
lib/lp/bugs/tests/bugs-emailinterface.txt (+3/-13)
lib/lp/services/mail/errortemplates/not-signed.txt (+2/-1)
lib/lp/services/mail/helpers.py (+7/-7)
lib/lp/services/mail/tests/test_helpers.py (+2/-1)
To merge this branch: bzr merge lp:~sinzui/launchpad/email-authentication-errors
Reviewer Review Type Date Requested Status
Launchpad code reviewers Pending
Review via email: mp+126801@code.launchpad.net

Commit message

send users emails about signing errors.

Description of the change

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 <email address hidden> 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

To post a comment you must log in.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== removed file 'lib/lp/bugs/mail/errortemplates/unauthenticated-bug-creation.txt'
2--- lib/lp/bugs/mail/errortemplates/unauthenticated-bug-creation.txt 2010-11-25 04:42:51 +0000
3+++ lib/lp/bugs/mail/errortemplates/unauthenticated-bug-creation.txt 1970-01-01 00:00:00 +0000
4@@ -1,2 +0,0 @@
5-To report bugs by e-mail, you need to sign the message with an OpenPGP key
6-that is registered in Launchpad.
7
8=== modified file 'lib/lp/bugs/mail/handler.py'
9--- lib/lp/bugs/mail/handler.py 2012-06-29 08:40:05 +0000
10+++ lib/lp/bugs/mail/handler.py 2012-09-27 21:05:25 +0000
11@@ -227,9 +227,7 @@
12 # We send a different failure message for attempts to create a new
13 # bug.
14 elif to_user.lower() == 'new':
15- ensure_not_weakly_authenticated(signed_msg, CONTEXT,
16- 'unauthenticated-bug-creation.txt',
17- error_templates=error_templates)
18+ ensure_not_weakly_authenticated(signed_msg, CONTEXT)
19 elif len(commands) > 0:
20 ensure_not_weakly_authenticated(signed_msg, CONTEXT)
21 if to_user.lower() == 'new':
22
23=== modified file 'lib/lp/bugs/tests/bugs-emailinterface.txt'
24--- lib/lp/bugs/tests/bugs-emailinterface.txt 2012-09-07 21:00:46 +0000
25+++ lib/lp/bugs/tests/bugs-emailinterface.txt 2012-09-27 21:05:25 +0000
26@@ -469,17 +469,6 @@
27 >>> print bug_one.title
28 Better summary
29
30-A different error message is sent, though:
31-
32- >>> print_latest_email()
33- Subject: Submit Request Failure
34- To: test@canonical.com
35- <BLANKLINE>
36- ...
37- The message you sent included commands to modify the bug report, but you
38- didn't sign the message with your OpenPGP key.
39- ...
40-
41
42 If we don't include any commands in the comment, it will be added
43 to the bug:
44@@ -1824,8 +1813,9 @@
45 To: test@canonical.com
46 <BLANKLINE>
47 ...
48- To report bugs by e-mail, you need to sign the message with an OpenPGP key
49- that is registered in Launchpad.
50+ The message you sent included commands to modify the bug report,
51+ but you didn't sign the message with an OpenPGP key that is
52+ registered in Launchpad.
53 ...
54
55 A submit without specifying on what we want to file the bug on:
56
57=== modified file 'lib/lp/services/mail/errortemplates/not-signed.txt'
58--- lib/lp/services/mail/errortemplates/not-signed.txt 2011-08-11 22:02:03 +0000
59+++ lib/lp/services/mail/errortemplates/not-signed.txt 2012-09-27 21:05:25 +0000
60@@ -1,2 +1,3 @@
61 The message you sent included commands to modify the %(context)s,
62-but you didn't sign the message with your OpenPGP key.
63+but you didn't sign the message with an OpenPGP key that is
64+registered in Launchpad.
65
66=== modified file 'lib/lp/services/mail/helpers.py'
67--- lib/lp/services/mail/helpers.py 2012-06-20 08:15:51 +0000
68+++ lib/lp/services/mail/helpers.py 2012-09-27 21:05:25 +0000
69@@ -21,6 +21,10 @@
70 from lp.services.webapp.interfaces import ILaunchBag
71
72
73+NO_KEY_TEMPLATE = 'key-not-registered.txt'
74+NOT_SIGNED_TEMPLATE = 'not-signed.txt'
75+
76+
77 class IncomingEmailError(Exception):
78 """Indicates that something went wrong processing the mail."""
79
80@@ -169,9 +173,7 @@
81
82
83 def ensure_not_weakly_authenticated(signed_msg, context,
84- error_template='not-signed.txt',
85- no_key_template='key-not-registered.txt',
86- error_templates=None):
87+ error_template='not-signed.txt'):
88 """Make sure that the current principal is not weakly authenticated.
89
90 NB: While handling an email, the authentication state is stored partly in
91@@ -188,14 +190,12 @@
92 if IWeaklyAuthenticatedPrincipal.providedBy(cur_principal):
93 if signed_msg.signature is None:
94 error_message = get_error_message(
95- error_template, error_templates=error_templates,
96- context=context)
97+ NOT_SIGNED_TEMPLATE, None, context=context)
98 else:
99 import_url = canonical_url(
100 getUtility(ILaunchBag).user) + '/+editpgpkeys'
101 error_message = get_error_message(
102- no_key_template, error_templates,
103- import_url=import_url, context=context)
104+ NO_KEY_TEMPLATE, None, import_url=import_url, context=context)
105 raise IncomingEmailError(error_message)
106
107
108
109=== modified file 'lib/lp/services/mail/tests/test_helpers.py'
110--- lib/lp/services/mail/tests/test_helpers.py 2012-06-20 08:15:51 +0000
111+++ lib/lp/services/mail/tests/test_helpers.py 2012-09-27 21:05:25 +0000
112@@ -177,7 +177,8 @@
113 signed_msg, 'test')
114 self.assertEqual(
115 "The message you sent included commands to modify the test,\n"
116- "but you didn't sign the message with your OpenPGP key.\n",
117+ "but you didn't sign the message with an OpenPGP key that is\n"
118+ "registered in Launchpad.\n",
119 error.message)
120
121 def test_weakly_authenticated_with_sig(self):