From 98f33859980adc5cf90bd49b9580c0fe719bfcf9 Mon Sep 17 00:00:00 2001 From: Chad Whitacre Date: Wed, 22 Feb 2017 17:17:31 -0500 Subject: [PATCH 1/2] Remove misguided throttling attempt --- defaults.env | 3 --- gratipay/exceptions.py | 3 --- gratipay/models/participant/email.py | 16 ++-------------- gratipay/wireup.py | 1 - tests/py/test_email.py | 25 ++++--------------------- tests/py/test_take_over.py | 6 +++--- www/~/%username/emails/modify.json.spt | 2 +- 7 files changed, 10 insertions(+), 46 deletions(-) diff --git a/defaults.env b/defaults.env index 892ce37217..9424e04a24 100644 --- a/defaults.env +++ b/defaults.env @@ -98,9 +98,6 @@ TEAM_REVIEW_REPO=gratipay/test-gremlin TEAM_REVIEW_USERNAME= TEAM_REVIEW_TOKEN= -# anything Postgres can interpret as an interval -RESEND_VERIFICATION_THRESHOLD="3 minutes" - RAISE_SIGNIN_NOTIFICATIONS=no # speeds up npm syncing; should be true on production and Travis diff --git a/gratipay/exceptions.py b/gratipay/exceptions.py index 9ba475f410..a670d520ba 100644 --- a/gratipay/exceptions.py +++ b/gratipay/exceptions.py @@ -46,9 +46,6 @@ class EmailNotVerified(ProblemChangingEmail): class TooManyEmailAddresses(ProblemChangingEmail): msg = "You've reached the maximum number of email addresses we allow." -class ResendingTooFast(ProblemChangingEmail): - msg = "Sorry, please try resending the verification email again in a minute or two." - class ProblemChangingNumber(Exception): def __str__(self): diff --git a/gratipay/models/participant/email.py b/gratipay/models/participant/email.py index 6bbc375e04..6598cd3c9d 100644 --- a/gratipay/models/participant/email.py +++ b/gratipay/models/participant/email.py @@ -9,7 +9,7 @@ import gratipay from gratipay.exceptions import EmailAlreadyVerified, EmailTaken, CannotRemovePrimaryEmail -from gratipay.exceptions import EmailNotVerified, TooManyEmailAddresses, ResendingTooFast +from gratipay.exceptions import EmailNotVerified, TooManyEmailAddresses from gratipay.security.crypto import constant_time_compare from gratipay.utils import encode_for_querystring @@ -41,16 +41,13 @@ class Email(object): """ - def add_email(self, email, resend_threshold='3 minutes'): + def add_email(self, email): """Add an email address for a participant. This is called when adding a new email address, and when resending the verification email for an unverified email address. :param unicode email: the email address to add - :param unicode resend_threshold: the time interval within which a - previous call to this function will cause the current call to fail - with ``ResendingTooFast`` :returns: ``None`` @@ -58,8 +55,6 @@ def add_email(self, email, resend_threshold='3 minutes'): this participant :raises EmailTaken: if the email is verified for a different participant :raises TooManyEmailAddresses: if the participant already has 10 emails - :raises ResendingTooFast: if the participant has added an email within the - time limit specified by ``resend_threshold`` """ @@ -83,13 +78,6 @@ def add_email(self, email, resend_threshold='3 minutes'): nonce = str(uuid.uuid4()) verification_start = utcnow() - nrecent = self.db.one( "SELECT count(*) FROM emails WHERE address=%s AND " - "%s - verification_start < %s" - , (email, verification_start, resend_threshold) - ) - if nrecent: - raise ResendingTooFast() - try: with self.db.get_cursor() as c: self.app.add_event(c, 'participant', dict(id=self.id, action='add', values=dict(email=email))) diff --git a/gratipay/wireup.py b/gratipay/wireup.py index e024f55462..132515c411 100644 --- a/gratipay/wireup.py +++ b/gratipay/wireup.py @@ -395,7 +395,6 @@ def env(): TEAM_REVIEW_USERNAME = unicode, TEAM_REVIEW_TOKEN = unicode, RAISE_SIGNIN_NOTIFICATIONS = is_yesish, - RESEND_VERIFICATION_THRESHOLD = unicode, REQUIRE_YAJL = is_yesish, GUNICORN_OPTS = unicode, ) diff --git a/tests/py/test_email.py b/tests/py/test_email.py index cc85892013..93ef9ffdf0 100644 --- a/tests/py/test_email.py +++ b/tests/py/test_email.py @@ -2,10 +2,9 @@ import json import sys -import time from gratipay.exceptions import CannotRemovePrimaryEmail, EmailTaken, EmailNotVerified -from gratipay.exceptions import TooManyEmailAddresses, ResendingTooFast +from gratipay.exceptions import TooManyEmailAddresses from gratipay.testing import P from gratipay.testing.email import QueuedEmailHarness, SentEmailHarness from gratipay.models.participant import email as _email @@ -13,19 +12,14 @@ from gratipay.cli import queue_branch_email as _queue_branch_email -class AliceAndResend(QueuedEmailHarness): +class Alice(QueuedEmailHarness): def setUp(self): QueuedEmailHarness.setUp(self) self.alice = self.make_participant('alice', claimed_time='now') - self._old_threshold = self.client.website.env.resend_verification_threshold - self.client.website.env.resend_verification_threshold = '0 seconds' - def tearDown(self): - self.client.website.env.resend_verification_threshold = self._old_threshold - -class TestEndpoints(AliceAndResend): +class TestEndpoints(Alice): def hit_email_spt(self, action, address, user='alice', should_fail=False): f = self.client.PxST if should_fail else self.client.POST @@ -210,7 +204,7 @@ def test_remove_email(self): self.hit_email_spt('remove', 'alice@example.com') -class TestFunctions(AliceAndResend): +class TestFunctions(Alice): def test_cannot_update_email_to_already_verified(self): bob = self.make_participant('bob', claimed_time='now') @@ -241,17 +235,6 @@ def test_cannot_add_too_many_emails(self): with self.assertRaises(TooManyEmailAddresses): self.alice.add_email('alice@gratipay.coop') - def test_cannot_resend_verification_too_frequently(self): - self.alice.add_email('alice@gratipay.coop') - time.sleep(0.05) - with self.assertRaises(ResendingTooFast): - self.alice.add_email('alice@gratipay.coop', '0.1 seconds') - - def test_can_resend_verification_after_a_while(self): - self.alice.add_email('alice@gratipay.coop') - time.sleep(0.15) - self.alice.add_email('alice@gratipay.coop', '0.1 seconds') - def test_html_escaping(self): self.alice.add_email("foo'bar@example.com") last_email = self.get_last_email() diff --git a/tests/py/test_take_over.py b/tests/py/test_take_over.py index 5ed14d1b34..9923466ce8 100644 --- a/tests/py/test_take_over.py +++ b/tests/py/test_take_over.py @@ -190,10 +190,10 @@ def test_email_addresses_merging(self): alice.verify_email('alice@example.org', alice.get_email('alice@example.org').nonce) bob_github = self.make_elsewhere('github', 2, 'bob') bob = bob_github.opt_in('bob')[0].participant - bob.add_email('alice@example.com', '0 seconds') + bob.add_email('alice@example.com') bob.verify_email('alice@example.com', bob.get_email('alice@example.com').nonce) - bob.add_email('alice@example.net', '0 seconds') - bob.add_email('bob@example.net', '0 seconds') + bob.add_email('alice@example.net') + bob.add_email('bob@example.net') alice.take_over(bob_github, have_confirmation=True) alice_emails = {e.address: e for e in alice.get_emails()} diff --git a/www/~/%username/emails/modify.json.spt b/www/~/%username/emails/modify.json.spt index 78c2461e1d..04ea0d8042 100644 --- a/www/~/%username/emails/modify.json.spt +++ b/www/~/%username/emails/modify.json.spt @@ -28,7 +28,7 @@ if not participant.email_lang: msg = None if action in ('add-email', 'resend'): try: - participant.add_email(address, website.env.resend_verification_threshold) + participant.add_email(address) except EmailTaken: raise Response(400, _( "{email_address} is already linked to a different Gratipay account." , email_address=address From 54141309567e3515abc542b1ae7877a0250923ce Mon Sep 17 00:00:00 2001 From: Chad Whitacre Date: Fri, 3 Mar 2017 09:39:06 -0500 Subject: [PATCH 2/2] Implement throttling based on participant --- defaults.env | 5 ++- gratipay/application.py | 2 +- gratipay/billing/payday.py | 1 + gratipay/cli/queue_branch_email.py | 2 +- gratipay/email.py | 35 +++++++++++++----- gratipay/exceptions.py | 4 ++ gratipay/models/participant/email.py | 5 +++ gratipay/testing/email.py | 11 +++++- gratipay/wireup.py | 4 +- sql/branch.sql | 5 +++ tests/py/test_email.py | 49 +++++++++++++++++++++---- www/%team/set-status.json.spt | 1 + www/~/%username/identities/%country.spt | 1 + www/~/%username/payout-status.spt | 16 ++++++-- 14 files changed, 116 insertions(+), 25 deletions(-) create mode 100644 sql/branch.sql diff --git a/defaults.env b/defaults.env index 9424e04a24..361ac6f8bc 100644 --- a/defaults.env +++ b/defaults.env @@ -73,9 +73,12 @@ OPENSTREETMAP_CALLBACK=http://127.0.0.1:8537/on/openstreetmap/associate OPENSTREETMAP_API_URL=http://www.openstreetmap.org/api/0.6 OPENSTREETMAP_AUTH_URL=http://www.openstreetmap.org +EMAIL_QUEUE_FLUSH_EVERY=60 +EMAIL_QUEUE_SLEEP_FOR=1 +EMAIL_QUEUE_ALLOW_UP_TO=3 + UPDATE_CTA_EVERY=300 CHECK_DB_EVERY=600 -FLUSH_EMAIL_QUEUE_EVERY=60 OPTIMIZELY_ID= INCLUDE_PIWIK=no SENTRY_DSN= diff --git a/gratipay/application.py b/gratipay/application.py index e2bb6a6938..e1f9a6d9f7 100644 --- a/gratipay/application.py +++ b/gratipay/application.py @@ -52,7 +52,7 @@ def install_periodic_jobs(self, website, env, db): cron = Cron(website) cron(env.update_cta_every, lambda: utils.update_cta(website)) cron(env.check_db_every, db.self_check, True) - cron(env.flush_email_queue_every, self.email_queue.flush, True) + cron(env.email_queue_flush_every, self.email_queue.flush, True) def add_event(self, c, type, payload): diff --git a/gratipay/billing/payday.py b/gratipay/billing/payday.py index bb6077b26e..b13da730a6 100644 --- a/gratipay/billing/payday.py +++ b/gratipay/billing/payday.py @@ -477,6 +477,7 @@ def notify_participants(self): exchange=dict(id=e.id, amount=e.amount, fee=e.fee, note=e.note), nteams=nteams, top_team=top_team, + _user_initiated=False ) diff --git a/gratipay/cli/queue_branch_email.py b/gratipay/cli/queue_branch_email.py index fe66a8ff31..f9e8938f23 100644 --- a/gratipay/cli/queue_branch_email.py +++ b/gratipay/cli/queue_branch_email.py @@ -86,4 +86,4 @@ def prompt(msg): _print( "{:>4} queuing for {} ({}={})".format(i, p.email_address, p.username, p.id) , file=sys.stderr ) - app.email_queue.put(p, 'branch', include_unsubscribe=False) + app.email_queue.put(p, 'branch', include_unsubscribe=False, _user_initiated=False) diff --git a/gratipay/email.py b/gratipay/email.py index 8be0bd5f5b..901cf7dc83 100644 --- a/gratipay/email.py +++ b/gratipay/email.py @@ -13,6 +13,7 @@ from jinja2 import Environment from markupsafe import escape as htmlescape +from gratipay.exceptions import Throttled from gratipay.models.participant import Participant from gratipay.utils import find_files, i18n @@ -35,6 +36,8 @@ def __init__(self, env, db, tell_sentry, root): self.db = db self.tell_sentry = tell_sentry + self.sleep_for = env.email_queue_sleep_for + self.allow_up_to = env.email_queue_allow_up_to templates = {} templates_dir = os.path.join(root, 'emails') @@ -52,21 +55,35 @@ def _have_ses(self, env): and env.aws_ses_default_region - def put(self, to, template, **context): + def put(self, to, template, _user_initiated=True, **context): """Put an email message on the queue. :param Participant to: the participant to send the email message to :param unicode template: the name of the template to use when rendering - the email, corresponding to a filename in ``emails/`` without the file - extension + the email, corresponding to a filename in ``emails/`` without the + file extension + :param bool _user_initiated: user-initiated emails are throttled; + system-initiated messages don't count against throttling :param dict context: the values to use when rendering the template + :raise Throttled: if the participant already has a few messages in the + queue (that they put there); the specific number is tunable with + the ``EMAIL_QUEUE_ALLOW_UP_TO`` envvar. + + :returns: ``None`` + """ - self.db.run(""" - INSERT INTO email_queue - (participant, spt_name, context) - VALUES (%s, %s, %s) - """, (to.id, template, pickle.dumps(context))) + with self.db.get_cursor() as cursor: + cursor.run(""" + INSERT INTO email_queue + (participant, spt_name, context, user_initiated) + VALUES (%s, %s, %s, %s) + """, (to.id, template, pickle.dumps(context), _user_initiated)) + if _user_initiated: + n = cursor.one('SELECT count(*) FROM email_queue ' + 'WHERE participant=%s AND user_initiated', (to.id,)) + if n > self.allow_up_to: + raise Throttled() def flush(self): @@ -87,7 +104,7 @@ def flush(self): r = self._flush_one(rec) self.db.run("DELETE FROM email_queue WHERE id = %s", (rec.id,)) if r == 1: - sleep(1) + sleep(self.sleep_for) nsent += r return nsent diff --git a/gratipay/exceptions.py b/gratipay/exceptions.py index a670d520ba..983a4a4f18 100644 --- a/gratipay/exceptions.py +++ b/gratipay/exceptions.py @@ -47,6 +47,10 @@ class TooManyEmailAddresses(ProblemChangingEmail): msg = "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 ProblemChangingNumber(Exception): def __str__(self): return self.msg diff --git a/gratipay/models/participant/email.py b/gratipay/models/participant/email.py index 6598cd3c9d..38ca03bc32 100644 --- a/gratipay/models/participant/email.py +++ b/gratipay/models/participant/email.py @@ -55,6 +55,7 @@ def add_email(self, email): this participant :raises EmailTaken: if the email is verified for a different participant :raises TooManyEmailAddresses: if the participant already has 10 emails + :raises Throttled: if the participant adds too many emails too quickly """ @@ -113,6 +114,10 @@ def add_email(self, email): , 'verification_notice' , new_email=email , include_unsubscribe=False + + # Don't count this one against their sending quota. + # It's going to their own verified address, anyway. + , _user_initiated=False ) diff --git a/gratipay/testing/email.py b/gratipay/testing/email.py index 1bd1666c74..79118cb351 100644 --- a/gratipay/testing/email.py +++ b/gratipay/testing/email.py @@ -8,6 +8,15 @@ class _AbstractEmailHarness(Harness): + def setUp(self): + Harness.setUp(self) + self.__sleep_for = self.app.email_queue.sleep_for + self.app.email_queue.sleep_for = 0 + + def tearDown(self): + Harness.tearDown(self) + self.app.email_queue.sleep_for = self.__sleep_for + def _get_last_email(self): raise NotImplementedError @@ -45,7 +54,7 @@ class SentEmailHarness(_AbstractEmailHarness): """ def setUp(self): - Harness.setUp(self) + _AbstractEmailHarness.setUp(self) self.mailer_patcher = mock.patch.object(self.app.email_queue._mailer, 'send_email') self.mailer = self.mailer_patcher.start() self.addCleanup(self.mailer_patcher.stop) diff --git a/gratipay/wireup.py b/gratipay/wireup.py index 132515c411..f6c36b73ff 100644 --- a/gratipay/wireup.py +++ b/gratipay/wireup.py @@ -386,7 +386,9 @@ def env(): OPENSTREETMAP_AUTH_URL = unicode, UPDATE_CTA_EVERY = int, CHECK_DB_EVERY = int, - FLUSH_EMAIL_QUEUE_EVERY = int, + EMAIL_QUEUE_FLUSH_EVERY = int, + EMAIL_QUEUE_SLEEP_FOR = int, + EMAIL_QUEUE_ALLOW_UP_TO = int, OPTIMIZELY_ID = unicode, SENTRY_DSN = unicode, LOG_METRICS = is_yesish, diff --git a/sql/branch.sql b/sql/branch.sql new file mode 100644 index 0000000000..962f3faf65 --- /dev/null +++ b/sql/branch.sql @@ -0,0 +1,5 @@ +BEGIN; + + ALTER TABLE email_queue ADD COLUMN user_initiated bool NOT NULL DEFAULT TRUE; + +END; diff --git a/tests/py/test_email.py b/tests/py/test_email.py index 93ef9ffdf0..7f0512996f 100644 --- a/tests/py/test_email.py +++ b/tests/py/test_email.py @@ -3,8 +3,10 @@ import json import sys +from pytest import raises + from gratipay.exceptions import CannotRemovePrimaryEmail, EmailTaken, EmailNotVerified -from gratipay.exceptions import TooManyEmailAddresses +from gratipay.exceptions import TooManyEmailAddresses, Throttled from gratipay.testing import P from gratipay.testing.email import QueuedEmailHarness, SentEmailHarness from gratipay.models.participant import email as _email @@ -34,11 +36,13 @@ def verify_email(self, email, nonce, username='alice', should_fail=False): f = self.client.GxT if should_fail else self.client.GET return f(url, auth_as=username) - def verify_and_change_email(self, old_email, new_email, username='alice'): + def verify_and_change_email(self, old_email, new_email, username='alice', _flush=True): self.hit_email_spt('add-email', old_email) nonce = P(username).get_email(old_email).nonce self.verify_email(old_email, nonce) self.hit_email_spt('add-email', new_email) + if _flush: + self.app.email_queue.flush() def test_participant_can_add_email(self): response = self.hit_email_spt('add-email', 'alice@gratipay.com') @@ -66,9 +70,10 @@ def test_verification_email_doesnt_contain_unsubscribe(self): assert "To stop receiving" not in last_email['body_text'] def test_adding_second_email_sends_verification_notice(self): - self.verify_and_change_email('alice1@example.com', 'alice2@example.com') + self.verify_and_change_email('alice1@example.com', 'alice2@example.com', _flush=False) assert self.count_email_messages() == 3 last_email = self.get_last_email() + self.app.email_queue.flush() assert last_email['to'] == 'alice ' expected = "We are connecting alice2@example.com to the alice account on Gratipay" assert expected in last_email['body_text'] @@ -206,12 +211,15 @@ def test_remove_email(self): class TestFunctions(Alice): + def add_and_verify(self, participant, address): + participant.add_email('alice@gratipay.com') + nonce = participant.get_email('alice@gratipay.com').nonce + r = participant.verify_email('alice@gratipay.com', nonce) + assert r == _email.VERIFICATION_SUCCEEDED + def test_cannot_update_email_to_already_verified(self): bob = self.make_participant('bob', claimed_time='now') - self.alice.add_email('alice@gratipay.com') - nonce = self.alice.get_email('alice@gratipay.com').nonce - r = self.alice.verify_email('alice@gratipay.com', nonce) - assert r == _email.VERIFICATION_SUCCEEDED + self.add_and_verify(self.alice, 'alice@gratipay.com') with self.assertRaises(EmailTaken): bob.add_email('alice@gratipay.com') @@ -225,12 +233,15 @@ def test_cannot_add_too_many_emails(self): self.alice.add_email('alice@gratipay.com') self.alice.add_email('alice@gratipay.net') self.alice.add_email('alice@gratipay.org') + self.app.email_queue.flush() self.alice.add_email('alice@gratipay.co.uk') self.alice.add_email('alice@gratipay.io') self.alice.add_email('alice@gratipay.co') + self.app.email_queue.flush() self.alice.add_email('alice@gratipay.eu') self.alice.add_email('alice@gratipay.asia') self.alice.add_email('alice@gratipay.museum') + self.app.email_queue.flush() self.alice.add_email('alice@gratipay.py') with self.assertRaises(TooManyEmailAddresses): self.alice.add_email('alice@gratipay.coop') @@ -241,6 +252,30 @@ def test_html_escaping(self): assert 'foo'bar' in last_email['body_html'] assert ''' not in last_email['body_text'] + def test_queueing_email_is_throttled(self): + self.app.email_queue.put(self.alice, "verification") + self.app.email_queue.put(self.alice, "branch") + self.app.email_queue.put(self.alice, "verification_notice") + raises(Throttled, self.app.email_queue.put, self.alice, "branch") + + def test_only_user_initiated_messages_count_towards_throttling(self): + self.app.email_queue.put(self.alice, "verification") + self.app.email_queue.put(self.alice, "verification", _user_initiated=False) + self.app.email_queue.put(self.alice, "branch") + self.app.email_queue.put(self.alice, "branch", _user_initiated=False) + self.app.email_queue.put(self.alice, "verification_notice") + self.app.email_queue.put(self.alice, "verification_notice", _user_initiated=False) + raises(Throttled, self.app.email_queue.put, self.alice, "branch") + + def test_flushing_queue_resets_throttling(self): + self.add_and_verify(self.alice, 'alice@example.com') + assert self.app.email_queue.flush() == 1 + self.app.email_queue.put(self.alice, "verification") + self.app.email_queue.put(self.alice, "branch") + self.app.email_queue.put(self.alice, "verification_notice") + assert self.app.email_queue.flush() == 3 + self.app.email_queue.put(self.alice, "verification_notice") + class FlushEmailQueue(SentEmailHarness): diff --git a/www/%team/set-status.json.spt b/www/%team/set-status.json.spt index a1ab771234..343bc15d62 100644 --- a/www/%team/set-status.json.spt +++ b/www/%team/set-status.json.spt @@ -43,6 +43,7 @@ with website.db.get_cursor() as c: , team_name=team.name , review_url=team.review_url , include_unsubscribe=False + , _user_initiated=False ) [---] application/json via json_dump diff --git a/www/~/%username/identities/%country.spt b/www/~/%username/identities/%country.spt index 1ae4ebef76..e24c4dd216 100644 --- a/www/~/%username/identities/%country.spt +++ b/www/~/%username/identities/%country.spt @@ -44,6 +44,7 @@ if identity is not None and participant != user.participant: , country_name=country_name , country_code=country.code , include_unsubscribe=False + , _user_initiated=False ) # handle POST requests diff --git a/www/~/%username/payout-status.spt b/www/~/%username/payout-status.spt index 496881d6d2..94a4ca0e35 100644 --- a/www/~/%username/payout-status.spt +++ b/www/~/%username/payout-status.spt @@ -40,10 +40,18 @@ with website.db.get_cursor() as c: action='set', values=dict(status_of_1_0_payout=new_status) )) - if new_status == 'rejected': - website.app.email_queue.put(participant, 'rejected-1.0', include_unsubscribe=False) - elif new_status == 'pending-payout': - website.app.email_queue.put(participant, 'approved-1.0', include_unsubscribe=False) +template = None +if new_status == 'rejected': + template = 'rejected-1.0' +elif new_status == 'pending-payout': + template = 'approved-1.0' + +if template: + website.app.email_queue.put( participant + , template + , include_unsubscribe=False + , _user_initiated=False + ) out = {"status": new_status} [---] application/json via json_dump