From 2e979de7808fcbf00f67978c4e37ccc1fcb10291 Mon Sep 17 00:00:00 2001 From: Chad Whitacre Date: Tue, 28 Mar 2017 11:43:44 -0400 Subject: [PATCH] Clean up error handling - use LocalizedErrorResponse to deduplicate error messages - don't include email address in error message to reduce spoofing surface --- gratipay/exceptions.py | 30 +++++++++++++++----------- gratipay/models/participant/email.py | 10 ++++----- gratipay/utils/i18n.py | 8 +++++-- tests/py/test_email.py | 7 ++++-- www/~/%username/emails/modify.json.spt | 16 ++------------ 5 files changed, 36 insertions(+), 35 deletions(-) diff --git a/gratipay/exceptions.py b/gratipay/exceptions.py index 7124918859..3e9aeaa1b1 100644 --- a/gratipay/exceptions.py +++ b/gratipay/exceptions.py @@ -4,7 +4,7 @@ from __future__ import print_function, unicode_literals -from aspen import Response +from gratipay.utils.i18n import LocalizedErrorResponse class ProblemChangingUsername(Exception): @@ -27,31 +27,37 @@ class UsernameAlreadyTaken(ProblemChangingUsername): msg = "The username '{}' is already taken." -class ProblemChangingEmail(Response): - def __init__(self, *args): - Response.__init__(self, 400, self.msg.format(*args)) +class ProblemChangingEmail(LocalizedErrorResponse): + pass class EmailAlreadyVerified(ProblemChangingEmail): - msg = "{} is already verified for this Gratipay account." + def lazy_body(self, _): + return _("You have already added and verified that address.") class EmailTaken(ProblemChangingEmail): - msg = "{} is already connected to a different Gratipay account." + def lazy_body(self, _): + return _("That address is already linked to a different Gratipay account.") class CannotRemovePrimaryEmail(ProblemChangingEmail): - msg = "You cannot remove your primary email address." + def lazy_body(self, _): + return _("You cannot remove your primary email address.") class EmailNotOnFile(ProblemChangingEmail): - msg = "The email address '{}' is not on file for this package." + def lazy_body(self, _): + return _("That email address is not on file for this package.") class EmailNotVerified(ProblemChangingEmail): - msg = "The email address '{}' is not verified." + def lazy_body(self, _): + return _("That email address is not verified.") class TooManyEmailAddresses(ProblemChangingEmail): - msg = "You've reached the maximum number of email addresses we allow." + def lazy_body(self, _): + return _("You've reached the maximum number of email addresses we allow.") -class Throttled(Exception): - msg = "You've initiated too many emails too quickly. Please try again in a minute or two." +class Throttled(LocalizedErrorResponse): + def lazy_body(self, _): + return _("You've initiated too many emails too quickly. Please try again in a minute or two.") class ProblemChangingNumber(Exception): diff --git a/gratipay/models/participant/email.py b/gratipay/models/participant/email.py index a2162c3749..822334e972 100644 --- a/gratipay/models/participant/email.py +++ b/gratipay/models/participant/email.py @@ -87,7 +87,7 @@ def validate_email_verification_request(self, c, email, *packages): """Given a cursor, email, and packages, return ``None`` or raise. """ if not all(email in p.emails for p in packages): - raise EmailNotOnFile(email) + raise EmailNotOnFile() owner_id = c.one(""" SELECT participant_id @@ -98,17 +98,17 @@ def validate_email_verification_request(self, c, email, *packages): if owner_id: if owner_id != self.id: - raise EmailTaken(email) + raise EmailTaken() elif packages: pass # allow reverify if claiming packages else: - raise EmailAlreadyVerified(email) + raise EmailAlreadyVerified() if len(self.get_emails()) > 9: if owner_id and owner_id == self.id and packages: pass # they're using an already-verified email to verify packages else: - raise TooManyEmailAddresses(email) + raise TooManyEmailAddresses() def get_email_verification_link(self, c, email, *packages): @@ -197,7 +197,7 @@ def update_email(self, email): """Set the email address for the participant. """ if not getattr(self.get_email(email), 'verified', False): - raise EmailNotVerified(email) + raise EmailNotVerified() username = self.username with self.db.get_cursor() as c: self.app.add_event( c diff --git a/gratipay/utils/i18n.py b/gratipay/utils/i18n.py index c67512f876..3db83c4474 100644 --- a/gratipay/utils/i18n.py +++ b/gratipay/utils/i18n.py @@ -263,9 +263,13 @@ def set_locale(): class LocalizedErrorResponse(Response): - def __init__(self, code, lazy_body, **kw): + def __repr__(self): + return "<{}: {}>".format(self.__class__.__name__, self) + + def __init__(self, code=400, lazy_body=None, **kw): Response.__init__(self, code, '', **kw) - self.lazy_body = lazy_body + if lazy_body: + self.lazy_body = lazy_body def render_body(self, state): f = self.lazy_body diff --git a/tests/py/test_email.py b/tests/py/test_email.py index 44ba1bcdb6..d7ae909ec3 100644 --- a/tests/py/test_email.py +++ b/tests/py/test_email.py @@ -7,7 +7,7 @@ from gratipay.exceptions import CannotRemovePrimaryEmail, EmailTaken, EmailNotVerified from gratipay.exceptions import TooManyEmailAddresses, Throttled, EmailAlreadyVerified -from gratipay.exceptions import EmailNotOnFile +from gratipay.exceptions import EmailNotOnFile, ProblemChangingEmail from gratipay.testing import P, Harness from gratipay.testing.email import QueuedEmailHarness, SentEmailHarness from gratipay.models.participant import email as _email @@ -36,7 +36,10 @@ def hit_email_spt(self, action, address, user='alice', should_fail=False): f = self.client.PxST if should_fail else self.client.POST data = {'action': action, 'address': address} headers = {b'HTTP_ACCEPT_LANGUAGE': b'en'} - return f('/~alice/emails/modify.json', data, auth_as=user, **headers) + response = f('/~alice/emails/modify.json', data, auth_as=user, **headers) + if issubclass(response.__class__, (Throttled, ProblemChangingEmail)): + response.render_body({'_': lambda a: a}) + return response def verify_email(self, email, nonce, username='alice', should_fail=False): # Email address is encoded in url. diff --git a/www/~/%username/emails/modify.json.spt b/www/~/%username/emails/modify.json.spt index 82f08a82b4..be4b4ac8cd 100644 --- a/www/~/%username/emails/modify.json.spt +++ b/www/~/%username/emails/modify.json.spt @@ -28,20 +28,8 @@ if not participant.email_lang: msg = None if action in ('add-email', 'resend'): - try: - participant.start_email_verification(address) - except EmailTaken: - raise Response(400, _( "{email_address} is already linked to a different Gratipay account." - , email_address=address - )) - except EmailAlreadyVerified: - raise Response(400, _( "You have already added and verified {email_address}." - , email_address=address - )) - except Throttled: - raise Response(400, _("You've initiated too many emails too quickly. Please try again in a minute or two.")) - else: - msg = _("A verification email has been sent to {email_address}.", email_address=address) + participant.start_email_verification(address) + msg = _("Check your inbox for a verification email.") elif action == 'set-primary': participant.update_email(address) elif action == 'remove':