diff --git a/gratipay/billing/payday.py b/gratipay/billing/payday.py index b13da730a6..fb724591b6 100644 --- a/gratipay/billing/payday.py +++ b/gratipay/billing/payday.py @@ -476,8 +476,7 @@ def notify_participants(self): 'charge_'+e.status, exchange=dict(id=e.id, amount=e.amount, fee=e.fee, note=e.note), nteams=nteams, - top_team=top_team, - _user_initiated=False + top_team=top_team ) diff --git a/gratipay/cli/queue_branch_email.py b/gratipay/cli/queue_branch_email.py index df75e95908..58734a66f3 100644 --- a/gratipay/cli/queue_branch_email.py +++ b/gratipay/cli/queue_branch_email.py @@ -90,4 +90,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, _user_initiated=False) + app.email_queue.put(p, 'branch', include_unsubscribe=False) diff --git a/gratipay/email.py b/gratipay/email.py index b851634521..6724aad795 100644 --- a/gratipay/email.py +++ b/gratipay/email.py @@ -13,7 +13,6 @@ 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 @@ -55,35 +54,24 @@ def _have_ses(self, env): and env.aws_ses_default_region - def put(self, to, template, _user_initiated=True, **context): + def put(self, to, template, **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 - :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`` """ 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() + (participant, spt_name, context) + VALUES (%s, %s, %s) + """, (to.id, template, pickle.dumps(context))) def flush(self): diff --git a/gratipay/exceptions.py b/gratipay/exceptions.py index cece213bdc..cfd2cccaf8 100644 --- a/gratipay/exceptions.py +++ b/gratipay/exceptions.py @@ -44,20 +44,19 @@ def lazy_body(self, _): class EmailNotOnFile(ProblemChangingEmail): def lazy_body(self, _): - return _("That email address is not on file for this package.") + return _("That email address is not on file for this package.") class EmailNotVerified(ProblemChangingEmail): def lazy_body(self, _): - return _("That email address is not verified.") + return _("That email address is not verified.") class TooManyEmailAddresses(ProblemChangingEmail): def lazy_body(self, _): - return _("You've reached the maximum number of email addresses we allow.") + return _("You've reached the maximum number of email addresses we allow.") - -class Throttled(LocalizedErrorResponse): +class TooManyVerifications(ProblemChangingEmail): def lazy_body(self, _): - return _("You've initiated too many emails too quickly. Please try again in a minute or two.") + return _( "You have an outstanding verification request that must be completed before you can verify a different email address.") class ProblemChangingNumber(Exception): diff --git a/gratipay/models/participant/email.py b/gratipay/models/participant/email.py index 97864d4153..7d28fc109c 100644 --- a/gratipay/models/participant/email.py +++ b/gratipay/models/participant/email.py @@ -9,7 +9,8 @@ import gratipay from gratipay.exceptions import EmailAlreadyVerified, EmailTaken, CannotRemovePrimaryEmail -from gratipay.exceptions import EmailNotVerified, TooManyEmailAddresses, EmailNotOnFile +from gratipay.exceptions import EmailNotVerified, EmailNotOnFile +from gratipay.exceptions import TooManyEmailAddresses, TooManyVerifications from gratipay.security.crypto import constant_time_compare from gratipay.utils import encode_for_querystring @@ -72,7 +73,8 @@ def start_email_verification(self, email, *packages): :raises EmailNotOnFile: if the email address is not on file for any of the packages :raises TooManyEmailAddresses: if the participant already has 10 emails - :raises Throttled: if the participant adds too many emails too quickly + :raises TooManyVerifications: if the participant already has a + verification attempt outstanding for a different email address """ with self.db.get_cursor() as c: @@ -91,11 +93,6 @@ def start_email_verification(self, email, *packages): if self.email_address and self.email_address != email: self.app.email_queue.put( self , 'verification-notice' - - # Don't count this one against their sending quota. - # It's going to their own verified address, anyway. - , _user_initiated=False - , **kwargs ) @@ -106,6 +103,16 @@ def validate_email_verification_request(self, c, email, *packages): if not all(email in p.emails for p in packages): raise EmailNotOnFile() + unverified_email = c.one(""" + SELECT address + FROM emails + WHERE participant_id=%(participant_id)s + AND not (verified IS true) + """, dict(participant_id=self.id)) + + if unverified_email and unverified_email != email: + raise TooManyVerifications() + owner_id = c.one(""" SELECT participant_id FROM emails diff --git a/gratipay/models/team/review_status.py b/gratipay/models/team/review_status.py index fcc3b8a854..71cb376f72 100644 --- a/gratipay/models/team/review_status.py +++ b/gratipay/models/team/review_status.py @@ -56,5 +56,4 @@ def _set_is_approved(self, cursor, is_approved, recorder): , team_name=self.name , review_url=self.review_url , include_unsubscribe=False - , _user_initiated=False ) diff --git a/gratipay/project_review_process.py b/gratipay/project_review_process.py index 756d7d2688..730f827ad9 100644 --- a/gratipay/project_review_process.py +++ b/gratipay/project_review_process.py @@ -63,7 +63,6 @@ def start(self, *teams): , 'project-review' , review_url=team.review_url , include_unsubscribe=False - , _user_initiated=False ) return review_url diff --git a/sql/branch.sql b/sql/branch.sql new file mode 100644 index 0000000000..6d1b1a0124 --- /dev/null +++ b/sql/branch.sql @@ -0,0 +1 @@ +ALTER TABLE email_queue DROP COLUMN user_initiated; diff --git a/tests/py/test_email.py b/tests/py/test_email.py index 1f9f44713e..69ca29e7d7 100644 --- a/tests/py/test_email.py +++ b/tests/py/test_email.py @@ -5,7 +5,6 @@ import mock from pytest import raises -from gratipay.exceptions import Throttled from gratipay.testing import Harness from gratipay.testing.email import SentEmailHarness @@ -14,13 +13,17 @@ class TestPut(SentEmailHarness): def setUp(self): SentEmailHarness.setUp(self) - self.alice = self.make_participant('alice', claimed_time='now', email_address='alice@example.com') + self.alice = self.make_participant( 'alice' + , claimed_time='now' + , email_address='alice@example.com' + ) - def test_queueing_email_is_throttled(self): + def test_put_puts(self): + assert self.app.email_queue.flush() == 0 self.app.email_queue.put(self.alice, "base") self.app.email_queue.put(self.alice, "base") self.app.email_queue.put(self.alice, "base") - raises(Throttled, self.app.email_queue.put, self.alice, "base") + assert self.app.email_queue.flush() == 3 def test_queueing_email_writes_timestamp(self): self.app.email_queue.put(self.alice, "base") @@ -28,22 +31,6 @@ def test_queueing_email_writes_timestamp(self): ctime = self.db.one("SELECT EXTRACT(epoch FROM ctime) FROM email_queue") assert abs(ctime - time.time()) < 300 - def test_only_user_initiated_messages_count_towards_throttling(self): - self.app.email_queue.put(self.alice, "base") - self.app.email_queue.put(self.alice, "base", _user_initiated=False) - self.app.email_queue.put(self.alice, "base") - self.app.email_queue.put(self.alice, "base", _user_initiated=False) - self.app.email_queue.put(self.alice, "base") - self.app.email_queue.put(self.alice, "base", _user_initiated=False) - raises(Throttled, self.app.email_queue.put, self.alice, "branch") - - def test_flushing_queue_resets_throttling(self): - self.app.email_queue.put(self.alice, "base") - self.app.email_queue.put(self.alice, "base") - self.app.email_queue.put(self.alice, "base") - assert self.app.email_queue.flush() == 3 - self.app.email_queue.put(self.alice, "base") - class TestFlush(SentEmailHarness): diff --git a/tests/py/test_packages.py b/tests/py/test_packages.py index edeb9b1d4f..d07cd17579 100644 --- a/tests/py/test_packages.py +++ b/tests/py/test_packages.py @@ -169,14 +169,14 @@ def test_ordering(self): nonce = alice.get_email('primary').nonce alice.finish_email_verification('primary', nonce) - # linked to self but unverified - alice.start_email_verification('lumail') - # verified for self alice.start_email_verification('amail') nonce = alice.get_email('amail').nonce alice.finish_email_verification('amail', nonce) + # linked to self but unverified + alice.start_email_verification('lumail') + # unlinked pass # cmail, email -- tests alphabetization within grouping diff --git a/tests/py/test_participant_emails.py b/tests/py/test_participant_emails.py index 9b5ebfb49c..69708d9981 100644 --- a/tests/py/test_participant_emails.py +++ b/tests/py/test_participant_emails.py @@ -10,7 +10,7 @@ from pytest import raises from gratipay.exceptions import CannotRemovePrimaryEmail, EmailTaken, EmailNotVerified -from gratipay.exceptions import TooManyEmailAddresses, Throttled, EmailAlreadyVerified +from gratipay.exceptions import TooManyEmailAddresses, TooManyVerifications, EmailAlreadyVerified from gratipay.exceptions import EmailNotOnFile, ProblemChangingEmail from gratipay.testing import P, Harness from gratipay.testing.email import QueuedEmailHarness @@ -57,7 +57,7 @@ def hit_email_spt(self, action, address, user='alice', package_ids=[], should_fa , auth_as=user , HTTP_ACCEPT_LANGUAGE=b'en' ) - if issubclass(response.__class__, (ProblemChangingEmail, Throttled)): + if issubclass(response.__class__, (ProblemChangingEmail, TooManyVerifications)): response.render_body({'_': lambda a: a}) return response @@ -109,6 +109,24 @@ def test_verifying_second_email_sends_verification_notice(self): expected = "We are connecting alice2@example.com to the alice account on Gratipay" assert expected in last_email['body_text'] + def test_can_only_have_one_verification_outstanding_at_a_time(self): + response = self.hit_email_spt('add-email', 'alice@example.com') + assert response.code == 200 + response = self.hit_email_spt('add-email', 'alice@example.net', should_fail=True) + assert response.code == 400 + assert response.__class__ is TooManyVerifications + assert response.body == 'You have an outstanding verification request that must be ' \ + 'completed before you can verify a different email address.' + + def test_can_resend_verification_message_for_same_email(self): + response = self.hit_email_spt('add-email', 'alice@example.com') + assert response.code == 200 + response = self.hit_email_spt('add-email', 'alice@example.net', should_fail=True) + assert response.code == 400 + assert response.__class__ is TooManyVerifications + assert response.body == 'You have an outstanding verification request that must be ' \ + 'completed before you can verify a different email address.' + def test_post_anon_returns_401(self): response = self.hit_email_spt('add-email', 'anon@example.com', user=None, should_fail=True) assert response.code == 401 @@ -129,14 +147,6 @@ def test_post_with_looooong_address_is_400(self): response = self.hit_email_spt('add-email', ('a'*243) + '@example.com', should_fail=True) assert response.code == 400 - def test_post_too_quickly_is_400(self): - self.hit_email_spt('add-email', 'alice@example.com') - self.hit_email_spt('add-email', 'alice+a@example.com') - self.hit_email_spt('add-email', 'alice+b@example.com') - response = self.hit_email_spt('add-email', 'alice+c@example.com', should_fail=True) - assert response.code == 400 - assert 'too quickly' in response.body - def test_verify_email_without_adding_email(self): response = self.hit_verify_spt('', 'sample-nonce') assert 'Bad Info' in response.body diff --git a/tests/py/test_take_over.py b/tests/py/test_take_over.py index 5d1c61d719..e87d3c0f78 100644 --- a/tests/py/test_take_over.py +++ b/tests/py/test_take_over.py @@ -184,41 +184,37 @@ def test_idempotent(self): self.db.self_check() def test_email_addresses_merging(self): - # person starts a bunch of email verifications as alice + # person verifies an email as alice alice = self.make_participant('alice') alice.start_email_verification('alice@example.com') - alice.start_email_verification('alice@example.net') - alice.start_email_verification('alice@example.org') + nonce = alice.get_email('alice@example.com').nonce + alice.finish_email_verification('alice@example.com', nonce) - # person finishes one of them, as alice - nonce = alice.get_email('alice@example.org').nonce - alice.finish_email_verification('alice@example.org', nonce) + # person starts another verification as alice + alice.start_email_verification('alice@example.org') # person signs up as bob bob_github = self.make_elsewhere('github', 2, 'bob') bob = bob_github.opt_in('bob')[0].participant - # person verifies one of the same emails as alice, using bob - bob.start_email_verification('alice@example.com') - nonce = bob.get_email('alice@example.com').nonce - bob.finish_email_verification('alice@example.com', nonce) - - # ... starts verification for another of the same - bob.start_email_verification('alice@example.net') - - # ... and for a new email + # verifies an email bob.start_email_verification('bob@example.net') + nonce = bob.get_email('bob@example.net').nonce + bob.finish_email_verification('bob@example.net', nonce) + + # and starts verification for alice's unverified email + bob.start_email_verification('alice@example.org') # Now: alice takes over bob! alice.take_over(bob_github, have_confirmation=True) # alice gets all the addresses, verified or not alice_emails = {e.address: e for e in alice.get_emails()} - assert len(alice_emails) == 4 + assert len(alice_emails) == 3 assert alice_emails['alice@example.com'].verified - assert alice_emails['alice@example.org'].verified - assert not alice_emails['alice@example.net'].verified - assert not alice_emails['bob@example.net'].verified + assert alice_emails['bob@example.net'].verified + assert not alice_emails['alice@example.org'].verified + assert alice.email_address == 'alice@example.com' # bob has no email addresses assert not Participant.from_id(bob.id).get_emails() diff --git a/www/~/%username/identities/%country.spt b/www/~/%username/identities/%country.spt index e24c4dd216..1ae4ebef76 100644 --- a/www/~/%username/identities/%country.spt +++ b/www/~/%username/identities/%country.spt @@ -44,7 +44,6 @@ 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 94a4ca0e35..18e1ff3d89 100644 --- a/www/~/%username/payout-status.spt +++ b/www/~/%username/payout-status.spt @@ -50,7 +50,6 @@ if template: website.app.email_queue.put( participant , template , include_unsubscribe=False - , _user_initiated=False ) out = {"status": new_status} diff --git a/www/~/%username/toggle-is-suspicious.json.spt b/www/~/%username/toggle-is-suspicious.json.spt index 4f80e0e47e..571fc548eb 100644 --- a/www/~/%username/toggle-is-suspicious.json.spt +++ b/www/~/%username/toggle-is-suspicious.json.spt @@ -46,7 +46,6 @@ with website.db.get_cursor() as c: website.app.email_queue.put( participant , 'suspicious-'+status , include_unsubscribe=False - , _user_initiated=False ) [---] application/json via json_dump