From b4267f33e31ad7b627a7a959227934f67e3c4b32 Mon Sep 17 00:00:00 2001 From: Chad Whitacre Date: Fri, 31 Mar 2017 15:25:26 -0400 Subject: [PATCH 1/5] Implement package claiming --- gratipay/models/package/__init__.py | 48 +++++++++ gratipay/models/participant/email.py | 131 +++++++++++++++++-------- gratipay/models/team/__init__.py | 6 +- gratipay/models/team/package.py | 23 +++++ sql/branch.sql | 2 + tests/py/test_email.py | 122 +++++++++++++++++++---- tests/py/test_packages.py | 38 +++++++ tests/py/test_take_over.py | 11 ++- www/~/%username/emails/modify.json.spt | 2 +- www/~/%username/emails/verify.html.spt | 2 +- 10 files changed, 316 insertions(+), 69 deletions(-) create mode 100644 gratipay/models/team/package.py diff --git a/gratipay/models/package/__init__.py b/gratipay/models/package/__init__.py index 24af56d724..2e50494533 100644 --- a/gratipay/models/package/__init__.py +++ b/gratipay/models/package/__init__.py @@ -1,6 +1,9 @@ # -*- coding: utf-8 -*- from __future__ import absolute_import, division, print_function, unicode_literals +import uuid + +from gratipay.models.team import Team from postgres.orm import Model @@ -10,6 +13,14 @@ class Package(Model): """Represent a gratipackage. :-) + + Packages are entities on open source package managers; `npm + `_ is the only one we support so far. Each package + on npm has a page on Gratipay with an URL of the form ``/on/npm/foo/``. + Packages can be claimed by Gratipay participants, at which point we create + a :py:class:`~gratipay.models.team.Team` for them under the hood so they + can start accepting payments. + """ typname = 'packages' @@ -40,3 +51,40 @@ def from_names(cls, package_manager, name): """ return cls.db.one("SELECT packages.*::packages FROM packages " "WHERE package_manager=%s and name=%s", (package_manager, name)) + + + @property + def team(self): + """A computed attribute, the :py:class:`~gratipay.models.team.Team` + linked to this package if there is one, otherwise ``None``. Makes a + database call. + """ + return self.load_team(self.db) + + + def load_team(self, cursor): + """Given a database cursor, return a + :py:class:`~gratipay.models.team.Team` if there is one linked to this + package, or ``None`` if not. + """ + return cursor.one('SELECT t.*::teams FROM teams t WHERE t.id=%s', (self.team_id,)) + + + def get_or_create_linked_team(self, cursor, owner): + """Given a db cursor and :py:class:`Participant`, return a + :py:class:`~gratipay.models.team.Team`. + """ + team = self.load_team(cursor) + if not team: + slug = str(uuid.uuid4()).lower() + team = Team.insert( slug=slug + , slug_lower=slug + , name=slug + , homepage='' + , product_or_service='' + , owner=owner + , _cursor=cursor + ) + cursor.run('UPDATE packages SET team_id=%s WHERE id=%s', (team.id, self.id)) + self.set_attributes(team_id=team.id) + return team diff --git a/gratipay/models/participant/email.py b/gratipay/models/participant/email.py index fb8fafda3b..4c530dfeba 100644 --- a/gratipay/models/participant/email.py +++ b/gratipay/models/participant/email.py @@ -48,7 +48,8 @@ def start_email_verification(self, email, *packages): verification email for an unverified email address. :param unicode email: the email address to add - :param Package packages: packages to optionally also verify ownership of + :param gratipay.models.package.Package packages: packages to optionally + also verify ownership of :returns: ``None`` @@ -201,63 +202,107 @@ def start_package_claims(self, c, nonce, *packages): ) - def update_email(self, email): - """Set the email address for the participant. + def set_primary_email(self, email, cursor=None): + """Set the primary email address for the participant. """ - if not getattr(self.get_email(email), 'verified', False): - raise EmailNotVerified() - username = self.username - with self.db.get_cursor() as c: - self.app.add_event( c - , 'participant' - , dict(id=self.id, action='set', values=dict(primary_email=email)) - ) - c.run(""" - UPDATE participants - SET email_address=%(email)s - WHERE username=%(username)s - """, locals()) + if cursor: + self._set_primary_email(email, cursor) + else: + with self.db.get_cursor() as cursor: + self._set_primary_email(email, cursor) self.set_attributes(email_address=email) - def verify_email(self, email, nonce): - if '' in (email, nonce): + def _set_primary_email(self, email, cursor): + if not getattr(self.get_email(email, cursor), 'verified', False): + raise EmailNotVerified() + self.app.add_event( cursor + , 'participant' + , dict(id=self.id, action='set', values=dict(primary_email=email)) + ) + cursor.run(""" + UPDATE participants + SET email_address=%(email)s + WHERE username=%(username)s + """, dict(email=email, username=self.username)) + + + def finish_email_verification(self, email, nonce): + if '' in (email.strip(), nonce.strip()): return VERIFICATION_MISSING - r = self.get_email(email) - if r is None: - return VERIFICATION_FAILED - if r.verified: - assert r.nonce is None # and therefore, order of conditions matters - return VERIFICATION_REDUNDANT - if not constant_time_compare(r.nonce, nonce): - return VERIFICATION_FAILED - if (utcnow() - r.verification_start) > EMAIL_HASH_TIMEOUT: - return VERIFICATION_EXPIRED - try: - self.db.run(""" - UPDATE emails - SET verified=true, verification_end=now(), nonce=NULL - WHERE participant_id=%s - AND address=%s - AND verified IS NULL - """, (self.id, email)) - except IntegrityError: - return VERIFICATION_STYMIED + with self.db.get_cursor() as cursor: + record = self.get_email(email, cursor) + if record is None: + return VERIFICATION_FAILED + packages = self.get_packages_claiming(cursor, nonce) + if record.verified and not packages: + assert record.nonce is None # and therefore, order of conditions matters + return VERIFICATION_REDUNDANT + if not constant_time_compare(record.nonce, nonce): + return VERIFICATION_FAILED + if (utcnow() - record.verification_start) > EMAIL_HASH_TIMEOUT: + return VERIFICATION_EXPIRED + try: + self.finish_package_claims(cursor, nonce, *packages) + self.save_email_address(cursor, email) + except IntegrityError: + return VERIFICATION_STYMIED + return VERIFICATION_SUCCEEDED + + + def get_packages_claiming(self, cursor, nonce): + """Given a nonce, return :py:class:`~gratipay.models.package.Package` + objects associated with it. + """ + return cursor.all(""" + SELECT p.*::packages + FROM packages p + JOIN claims c + ON p.id = c.package_id + WHERE c.nonce=%s + """, (nonce,)) + + def save_email_address(self, cursor, address): + """Given an email address, modify the database. + """ + cursor.run(""" + UPDATE emails + SET verified=true, verification_end=now(), nonce=NULL + WHERE participant_id=%s + AND address=%s + AND verified IS NULL + """, (self.id, address)) if not self.email_address: - self.update_email(email) - return VERIFICATION_SUCCEEDED + self.set_primary_email(address, cursor) + + + def finish_package_claims(self, cursor, nonce, *packages): + """Create teams if needed and associate them with the packages. + """ + cursor.run('DELETE FROM claims WHERE nonce=%s', (nonce,)) + package_ids = [] + for package in packages: + package.get_or_create_linked_team(cursor, self) + package_ids.append(package.id) + self.app.add_event( cursor + , 'participant' + , dict( id=self.id + , action='finish-claim' + , values=dict(package_ids=package_ids) + ) + ) - def get_email(self, email): + def get_email(self, address, cursor=None): """Return a record for a single email address on file for this participant. """ - return self.db.one(""" + return (cursor or self.db).one(""" SELECT * FROM emails WHERE participant_id=%s AND address=%s - """, (self.id, email)) + """, (self.id, address)) def get_emails(self): diff --git a/gratipay/models/team/__init__.py b/gratipay/models/team/__init__.py index 4f371a3d1e..0e602bfac6 100644 --- a/gratipay/models/team/__init__.py +++ b/gratipay/models/team/__init__.py @@ -13,6 +13,7 @@ from .available import Available from .closing import Closing from .membership import Membership +from .package import Package from .takes import Takes from .tip_migration import TipMigration @@ -36,7 +37,7 @@ def slugize(name): return slug -class Team(Model, Available, Closing, Membership, Takes, TipMigration): +class Team(Model, Available, Closing, Membership, Package, Takes, TipMigration): """Represent a Gratipay team. """ @@ -98,9 +99,10 @@ def _from_thing(cls, thing, value): @classmethod def insert(cls, owner, **fields): + cursor = fields.pop('_cursor') if '_cursor' in fields else None fields['slug_lower'] = fields['slug'].lower() fields['owner'] = owner.username - return cls.db.one(""" + return (cursor or cls.db).one(""" INSERT INTO teams (slug, slug_lower, name, homepage, diff --git a/gratipay/models/team/package.py b/gratipay/models/team/package.py new file mode 100644 index 0000000000..f1e07a3df0 --- /dev/null +++ b/gratipay/models/team/package.py @@ -0,0 +1,23 @@ +# -*- coding: utf-8 -*- +from __future__ import absolute_import, division, print_function, unicode_literals + + +class Package(object): + """A :py:class:`~gratipay.models.team.Team` can be associated with :py:class:`Package`. + """ + + @property + def package(self): + """A computed attribute, the + :py:class:`~gratipay.models.package.Package` linked to this team if + there is one, otherwise ``None``. Makes a database call. + """ + return self.load_package(self.db) + + + def load_package(self, cursor): + """Given a database cursor, return a + :py:class:`~gratipay.models.package.Package` if there is one linked to + this package, or ``None`` if not. + """ + return cursor.one('SELECT p.*::packages FROM packages p WHERE p.team_id=%s', (self.id,)) diff --git a/sql/branch.sql b/sql/branch.sql index 8d39aa1369..8991d23794 100644 --- a/sql/branch.sql +++ b/sql/branch.sql @@ -9,4 +9,6 @@ BEGIN; , UNIQUE(nonce, package_id) ); + ALTER TABLE packages ADD COLUMN team_id bigint UNIQUE REFERENCES teams(id) ON DELETE RESTRICT; + END; diff --git a/tests/py/test_email.py b/tests/py/test_email.py index 511a7c3202..babd3640fb 100644 --- a/tests/py/test_email.py +++ b/tests/py/test_email.py @@ -2,8 +2,8 @@ import json import sys - import urllib + from pytest import raises from gratipay.exceptions import CannotRemovePrimaryEmail, EmailTaken, EmailNotVerified @@ -11,6 +11,7 @@ from gratipay.exceptions import EmailNotOnFile, ProblemChangingEmail from gratipay.testing import P, Harness from gratipay.testing.email import QueuedEmailHarness, SentEmailHarness +from gratipay.models.package import Package from gratipay.models.participant import email as _email from gratipay.utils import encode_for_querystring from gratipay.cli import queue_branch_email as _queue_branch_email @@ -25,7 +26,7 @@ def setUp(self): def add(self, participant, address, _flush=False): participant.start_email_verification(address) nonce = participant.get_email(address).nonce - r = participant.verify_email(address, nonce) + r = participant.finish_email_verification(address, nonce) assert r == _email.VERIFICATION_SUCCEEDED if _flush: self.app.email_queue.flush() @@ -57,7 +58,7 @@ def hit_email_spt(self, action, address, user='alice', package_ids=[], should_fa response.render_body({'_': lambda a: a}) return response - def verify_email(self, email, nonce, username='alice', should_fail=False): + def finish_email_verification(self, email, nonce, username='alice', should_fail=False): # Email address is encoded in url. url = '/~%s/emails/verify.html?email2=%s&nonce=%s' url %= (username, encode_for_querystring(email), nonce) @@ -67,7 +68,7 @@ def verify_email(self, email, nonce, username='alice', should_fail=False): 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.finish_email_verification(old_email, nonce) self.hit_email_spt('add-email', new_email) if _flush: self.app.email_queue.flush() @@ -134,15 +135,15 @@ def test_post_too_quickly_is_400(self): assert 'too quickly' in response.body def test_verify_email_without_adding_email(self): - response = self.verify_email('', 'sample-nonce') + response = self.finish_email_verification('', 'sample-nonce') assert 'Bad Info' in response.body def test_verify_email_wrong_nonce(self): self.hit_email_spt('add-email', 'alice@example.com') nonce = 'fake-nonce' - r = self.alice.verify_email('alice@gratipay.com', nonce) + r = self.alice.finish_email_verification('alice@gratipay.com', nonce) assert r == _email.VERIFICATION_FAILED - self.verify_email('alice@example.com', nonce) + self.finish_email_verification('alice@example.com', nonce) expected = None actual = P('alice').email_address assert expected == actual @@ -151,8 +152,8 @@ def test_verify_email_a_second_time_returns_redundant(self): address = 'alice@example.com' self.hit_email_spt('add-email', address) nonce = self.alice.get_email(address).nonce - r = self.alice.verify_email(address, nonce) - r = self.alice.verify_email(address, nonce) + r = self.alice.finish_email_verification(address, nonce) + r = self.alice.finish_email_verification(address, nonce) assert r == _email.VERIFICATION_REDUNDANT def test_verify_email_expired_nonce(self): @@ -164,18 +165,26 @@ def test_verify_email_expired_nonce(self): WHERE participant_id = %s; """, (self.alice.id,)) nonce = self.alice.get_email(address).nonce - r = self.alice.verify_email(address, nonce) + r = self.alice.finish_email_verification(address, nonce) assert r == _email.VERIFICATION_EXPIRED actual = P('alice').email_address assert actual == None - def test_verify_email(self): + def test_finish_email_verification(self): self.hit_email_spt('add-email', 'alice@example.com') nonce = self.alice.get_email('alice@example.com').nonce - self.verify_email('alice@example.com', nonce) - expected = 'alice@example.com' - actual = P('alice').email_address - assert expected == actual + assert self.finish_email_verification('alice@example.com', nonce).code == 200 + assert P('alice').email_address == 'alice@example.com' + + def test_empty_email_results_in_missing(self): + for empty in ('', ' '): + result = self.alice.finish_email_verification(empty, 'foobar') + assert result == _email.VERIFICATION_MISSING + + def test_empty_nonce_results_in_missing(self): + for empty in ('', ' '): + result = self.alice.finish_email_verification('foobar', empty) + assert result == _email.VERIFICATION_MISSING def test_email_verification_is_backwards_compatible(self): """Test email verification still works with unencoded email in verification link. @@ -202,7 +211,7 @@ def test_get_emails(self): def test_verify_email_after_update(self): self.verify_and_change_email('alice@example.com', 'alice@example.net') nonce = self.alice.get_email('alice@example.net').nonce - self.verify_email('alice@example.net', nonce) + self.finish_email_verification('alice@example.net', nonce) expected = 'alice@example.com' actual = P('alice').email_address assert expected == actual @@ -301,7 +310,7 @@ def test_cannot_update_email_to_already_verified(self): with self.assertRaises(EmailTaken): bob.start_email_verification('alice@gratipay.com') nonce = bob.get_email('alice@gratipay.com').nonce - bob.verify_email('alice@gratipay.com', nonce) + bob.finish_email_verification('alice@gratipay.com', nonce) email_alice = P('alice').email_address assert email_alice == 'alice@gratipay.com' @@ -597,7 +606,7 @@ def check(self, *package_names, **kw): def preverify(self, address='alice@example.com'): self.alice.start_email_verification(address) nonce = self.alice.get_email(address).nonce - self.alice.verify_email(address, nonce) + self.alice.finish_email_verification(address, nonce) class VerificationMessage(VerificationBase): @@ -682,3 +691,80 @@ def test_sends_notice_for_unverified_address_and_multiple_packages(self): html, text = self.check('foo', 'bar') assert ' connecting alice@example.com and 2 npm packages ' in html assert ' connecting alice@example.com and 2 npm packages ' in text + + +class PackageLinking(VerificationBase): + + address = 'alice@example.com' + + def start(self, address, *package_names): + packages = [self.make_package(name=name, emails=[address]) for name in package_names] + self.alice.start_email_verification(address, *packages) + return self.alice.get_email(address).nonce + + def check(self, *package_names): + nonce = self.start(self.address, *package_names) + retval = self.alice.finish_email_verification(self.address, nonce) + assert retval == _email.VERIFICATION_SUCCEEDED + assert self.alice.email_address == P('alice').email_address == self.address + for name in package_names: + package = Package.from_names('npm', name) + assert package.team.package == package + + + def test_preverify_preverifies(self): + assert self.alice.email_address is None + self.preverify() + assert self.alice.email_address == self.address + + + def test_unverified_address_and_no_packages_succeeds(self): + self.check() + + def test_unverified_address_and_one_package_succeeds(self): + self.check('foo') + + def test_unverified_address_and_multiple_packages_succeeds(self): + self.check('foo', 'bar') + + def test_verified_address_and_no_packages_is_a_no_go(self): + self.preverify() + raises(EmailAlreadyVerified, self.check) + + def test_verified_address_and_one_package_succeeds(self): + self.preverify() + self.check('foo') + + def test_verified_address_and_multiple_packages_succeeds(self): + self.preverify() + self.check('foo', 'bar') + + + def test_bob_cannot_steal_a_package_claim_from_alice(self): + foo = self.make_package() + self.alice.start_email_verification(self.address, foo) + nonce = self.alice.get_email(self.address).nonce + + # u so bad bob! + bob = self.make_participant('bob', claimed_time='now') + bob.start_email_verification(self.address, foo) + result = bob.finish_email_verification(self.address, nonce) # using alice's nonce, even! + assert result == _email.VERIFICATION_FAILED + assert len(bob.get_teams()) == 0 + + result = self.alice.finish_email_verification(self.address, nonce) + assert result == _email.VERIFICATION_SUCCEEDED + teams = self.alice.get_teams() + assert len(teams) == 1 + assert teams[0].package == foo + + + def test_while_we_are_at_it_that_packages_have_unique_teams_that_survive_comparison(self): + self.test_verified_address_and_multiple_packages_succeeds() + + foo = Package.from_names('npm', 'foo') + bar = Package.from_names('npm', 'bar') + + assert foo.team == foo.team + assert bar.team == bar.team + assert foo.team != bar.team diff --git a/tests/py/test_packages.py b/tests/py/test_packages.py index b97e888377..b8ea3a0bd5 100644 --- a/tests/py/test_packages.py +++ b/tests/py/test_packages.py @@ -3,6 +3,8 @@ from gratipay.models.package import NPM, Package from gratipay.testing import Harness +from psycopg2 import IntegrityError +from pytest import raises class TestPackage(Harness): @@ -14,3 +16,39 @@ def test_can_be_instantiated_from_id(self): def test_can_be_instantiated_from_names(self): self.make_package() assert Package.from_names(NPM, 'foo').name == 'foo' + + +class Linking(Harness): + + def test_package_team_is_none(self): + foo = self.make_package() + assert foo.team is None + + def test_team_package_is_none(self): + foo = self.make_team() + assert foo.package is None + + def test_can_link_to_a_new_team(self): + alice = self.make_participant('alice') + foo = self.make_package() + with self.db.get_cursor() as c: + team = foo.get_or_create_linked_team(c, alice) + assert team.package == foo + assert foo.team == team + return alice, foo, team + + def test_linking_is_idempotent(self): + alice, package, _team = self.test_can_link_to_a_new_team() + for i in range(5): + with self.db.get_cursor() as c: + team = package.get_or_create_linked_team(c, alice) + assert team == _team + + def test_team_can_only_be_linked_from_one_package(self): + alice, package, team = self.test_can_link_to_a_new_team() + bar = self.make_package(name='bar') + raises( IntegrityError + , self.db.run + , 'UPDATE packages SET team_id=%s WHERE id=%s' + , (team.id, bar.id) + ) diff --git a/tests/py/test_take_over.py b/tests/py/test_take_over.py index 31e8f13c4e..de35b707e4 100644 --- a/tests/py/test_take_over.py +++ b/tests/py/test_take_over.py @@ -153,7 +153,8 @@ def test_take_over_is_fine_with_identity_info_on_primary(self): TT = self.db.one("SELECT id FROM countries WHERE code='TT'") alice = self.make_participant('alice') alice.start_email_verification('alice@example.com') - alice.verify_email('alice@example.com', alice.get_email('alice@example.com').nonce) + nonce = alice.get_email('alice@example.com').nonce + alice.finish_email_verification('alice@example.com', nonce) alice.store_identity_info(TT, 'nothing-enforced', {}) bob_github = self.make_elsewhere('github', 2, 'bob') @@ -169,7 +170,7 @@ def test_take_over_fails_if_secondary_has_identity_info(self): bob_github = self.make_elsewhere('github', 2, 'bob') bob = bob_github.opt_in('bob')[0].participant bob.start_email_verification('bob@example.com') - bob.verify_email('bob@example.com', bob.get_email('bob@example.com').nonce) + bob.finish_email_verification('bob@example.com', bob.get_email('bob@example.com').nonce) bob.store_identity_info(TT, 'nothing-enforced', {}) pytest.raises(WontTakeOverWithIdentities, alice.take_over, bob_github) @@ -187,11 +188,13 @@ def test_email_addresses_merging(self): alice.start_email_verification('alice@example.com') alice.start_email_verification('alice@example.net') alice.start_email_verification('alice@example.org') - alice.verify_email('alice@example.org', alice.get_email('alice@example.org').nonce) + nonce = alice.get_email('alice@example.org').nonce + alice.finish_email_verification('alice@example.org', nonce) bob_github = self.make_elsewhere('github', 2, 'bob') bob = bob_github.opt_in('bob')[0].participant bob.start_email_verification('alice@example.com') - bob.verify_email('alice@example.com', bob.get_email('alice@example.com').nonce) + nonce = bob.get_email('alice@example.com').nonce + bob.finish_email_verification('alice@example.com', nonce) bob.start_email_verification('alice@example.net') bob.start_email_verification('bob@example.net') alice.take_over(bob_github, have_confirmation=True) diff --git a/www/~/%username/emails/modify.json.spt b/www/~/%username/emails/modify.json.spt index e7d7452dc4..9b75a54e4d 100644 --- a/www/~/%username/emails/modify.json.spt +++ b/www/~/%username/emails/modify.json.spt @@ -46,7 +46,7 @@ if action in ('add-email', 'resend', 'start-verification'): participant.start_email_verification(address, *packages) msg = _("Check your inbox for a verification link.") elif action == 'set-primary': - participant.update_email(address) + participant.set_primary_email(address) elif action == 'remove': participant.remove_email(address) else: diff --git a/www/~/%username/emails/verify.html.spt b/www/~/%username/emails/verify.html.spt index d607ed5a72..c93f8f01f6 100644 --- a/www/~/%username/emails/verify.html.spt +++ b/www/~/%username/emails/verify.html.spt @@ -22,7 +22,7 @@ if participant == user.participant: else: email_address = decode_from_querystring(request.qs.get('email2', ''), default='') nonce = request.qs.get('nonce', '') - result = participant.verify_email(email_address, nonce) + result = participant.finish_email_verification(email_address, nonce) if not participant.email_lang: participant.set_email_lang(request.headers.get("Accept-Language")) From c7f08a8f21a35790cb5728638e7d2d39a0d1b750 Mon Sep 17 00:00:00 2001 From: Chad Whitacre Date: Wed, 19 Apr 2017 06:55:16 -0400 Subject: [PATCH 2/5] Clean up package.team API a bit --- gratipay/models/package/__init__.py | 21 ++++++++------------- gratipay/models/participant/email.py | 2 +- tests/py/test_packages.py | 6 ++++-- 3 files changed, 13 insertions(+), 16 deletions(-) diff --git a/gratipay/models/package/__init__.py b/gratipay/models/package/__init__.py index 2e50494533..4514d15ad1 100644 --- a/gratipay/models/package/__init__.py +++ b/gratipay/models/package/__init__.py @@ -59,22 +59,13 @@ def team(self): linked to this package if there is one, otherwise ``None``. Makes a database call. """ - return self.load_team(self.db) + return self._load_team(self.db) - def load_team(self, cursor): - """Given a database cursor, return a - :py:class:`~gratipay.models.team.Team` if there is one linked to this - package, or ``None`` if not. + def ensure_team(self, cursor, owner): + """Given a db cursor and :py:class:`Participant`, insert into ``teams`` if need be. """ - return cursor.one('SELECT t.*::teams FROM teams t WHERE t.id=%s', (self.team_id,)) - - - def get_or_create_linked_team(self, cursor, owner): - """Given a db cursor and :py:class:`Participant`, return a - :py:class:`~gratipay.models.team.Team`. - """ - team = self.load_team(cursor) + team = self._load_team(cursor) if not team: slug = str(uuid.uuid4()).lower() team = Team.insert( slug=slug @@ -88,3 +79,7 @@ def get_or_create_linked_team(self, cursor, owner): cursor.run('UPDATE packages SET team_id=%s WHERE id=%s', (team.id, self.id)) self.set_attributes(team_id=team.id) return team + + + def _load_team(self, cursor): + return cursor.one('SELECT t.*::teams FROM teams t WHERE t.id=%s', (self.team_id,)) diff --git a/gratipay/models/participant/email.py b/gratipay/models/participant/email.py index 4c530dfeba..42758213e2 100644 --- a/gratipay/models/participant/email.py +++ b/gratipay/models/participant/email.py @@ -283,7 +283,7 @@ def finish_package_claims(self, cursor, nonce, *packages): cursor.run('DELETE FROM claims WHERE nonce=%s', (nonce,)) package_ids = [] for package in packages: - package.get_or_create_linked_team(cursor, self) + package.ensure_team(cursor, self) package_ids.append(package.id) self.app.add_event( cursor , 'participant' diff --git a/tests/py/test_packages.py b/tests/py/test_packages.py index b8ea3a0bd5..23f9f10e46 100644 --- a/tests/py/test_packages.py +++ b/tests/py/test_packages.py @@ -32,7 +32,8 @@ def test_can_link_to_a_new_team(self): alice = self.make_participant('alice') foo = self.make_package() with self.db.get_cursor() as c: - team = foo.get_or_create_linked_team(c, alice) + foo.ensure_team(c, alice) + team = foo._load_team(c) assert team.package == foo assert foo.team == team return alice, foo, team @@ -41,7 +42,8 @@ def test_linking_is_idempotent(self): alice, package, _team = self.test_can_link_to_a_new_team() for i in range(5): with self.db.get_cursor() as c: - team = package.get_or_create_linked_team(c, alice) + package.ensure_team(c, alice) + team = package._load_team(c) assert team == _team def test_team_can_only_be_linked_from_one_package(self): From d041eb7f593dfbb7bfea2d5edf547f1a86c759bb Mon Sep 17 00:00:00 2001 From: Chad Whitacre Date: Wed, 19 Apr 2017 07:15:01 -0400 Subject: [PATCH 3/5] Update team.package API to match --- gratipay/models/team/package.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/gratipay/models/team/package.py b/gratipay/models/team/package.py index f1e07a3df0..dd33f7b76b 100644 --- a/gratipay/models/team/package.py +++ b/gratipay/models/team/package.py @@ -12,12 +12,8 @@ def package(self): :py:class:`~gratipay.models.package.Package` linked to this team if there is one, otherwise ``None``. Makes a database call. """ - return self.load_package(self.db) + return self._load_package(self.db) - def load_package(self, cursor): - """Given a database cursor, return a - :py:class:`~gratipay.models.package.Package` if there is one linked to - this package, or ``None`` if not. - """ + def _load_package(self, cursor): return cursor.one('SELECT p.*::packages FROM packages p WHERE p.team_id=%s', (self.id,)) From eaa8699912e8571f0d6750955672795d3fbf7ff5 Mon Sep 17 00:00:00 2001 From: Chad Whitacre Date: Mon, 24 Apr 2017 08:10:39 -0400 Subject: [PATCH 4/5] Failing test for concurrency-safety in claiming --- tests/py/test_email.py | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/tests/py/test_email.py b/tests/py/test_email.py index babd3640fb..080d36ffd1 100644 --- a/tests/py/test_email.py +++ b/tests/py/test_email.py @@ -1,7 +1,9 @@ from __future__ import absolute_import, division, print_function, unicode_literals import json +import Queue import sys +import threading import urllib from pytest import raises @@ -768,3 +770,40 @@ def test_while_we_are_at_it_that_packages_have_unique_teams_that_survive_compari assert foo.team == foo.team assert bar.team == bar.team assert foo.team != bar.team + + + def test_finishing_email_verification_is_thread_safe(self): + foo = self.make_package() + self.alice.start_email_verification(self.address, foo) + nonce = self.alice.get_email(self.address).nonce + + results = {} + def finish(): + key = threading.current_thread().ident + results[key] = self.alice.finish_email_verification(self.address, nonce) + + def t(): + t = threading.Thread(target=finish) + t.daemon = True + return t + + go = Queue.Queue() + def monkey(self, *a, **kw): + old_ensure_team(self, *a, **kw) + go.get() + old_ensure_team = Package.ensure_team + Package.ensure_team = monkey + + try: + a, b = t(), t() + a.start() + b.start() + go.put('') + go.put('') + b.join() + a.join() + finally: + Package.ensure_team = old_ensure_team + + assert results[a.ident] == _email.VERIFICATION_SUCCEEDED + assert results[b.ident] == _email.VERIFICATION_REDUNDANT From 1d420940ffeb34c963da19f591ad4eaa801cbf3a Mon Sep 17 00:00:00 2001 From: Chad Whitacre Date: Mon, 24 Apr 2017 10:16:31 -0400 Subject: [PATCH 5/5] Make finish_email_verification concurrency-safe --- gratipay/models/package/__init__.py | 9 ++++++--- gratipay/models/participant/email.py | 20 ++++++++++++-------- gratipay/models/team/package.py | 5 ++++- sql/branch.sql | 5 ++++- tests/py/test_packages.py | 11 ++++++++++- 5 files changed, 36 insertions(+), 14 deletions(-) diff --git a/gratipay/models/package/__init__.py b/gratipay/models/package/__init__.py index 4514d15ad1..52b3464c0c 100644 --- a/gratipay/models/package/__init__.py +++ b/gratipay/models/package/__init__.py @@ -76,10 +76,13 @@ def ensure_team(self, cursor, owner): , owner=owner , _cursor=cursor ) - cursor.run('UPDATE packages SET team_id=%s WHERE id=%s', (team.id, self.id)) - self.set_attributes(team_id=team.id) + cursor.run('INSERT INTO teams_to_packages (team_id, package_id) ' + 'VALUES (%s, %s)', (team.id, self.id)) return team def _load_team(self, cursor): - return cursor.one('SELECT t.*::teams FROM teams t WHERE t.id=%s', (self.team_id,)) + return cursor.one( 'SELECT t.*::teams FROM teams t WHERE t.id=' + '(SELECT team_id FROM teams_to_packages tp WHERE tp.package_id=%s)' + , (self.id,) + ) diff --git a/gratipay/models/participant/email.py b/gratipay/models/participant/email.py index 42758213e2..73ee8d317b 100644 --- a/gratipay/models/participant/email.py +++ b/gratipay/models/participant/email.py @@ -231,7 +231,7 @@ def finish_email_verification(self, email, nonce): if '' in (email.strip(), nonce.strip()): return VERIFICATION_MISSING with self.db.get_cursor() as cursor: - record = self.get_email(email, cursor) + record = self.get_email(email, cursor, and_lock=True) if record is None: return VERIFICATION_FAILED packages = self.get_packages_claiming(cursor, nonce) @@ -294,15 +294,19 @@ def finish_package_claims(self, cursor, nonce, *packages): ) - def get_email(self, address, cursor=None): + def get_email(self, address, cursor=None, and_lock=False): """Return a record for a single email address on file for this participant. + + :param unicode address: the email address for which to get a record + :param Cursor cursor: a database cursor; if ``None``, we'll use ``self.db`` + :param and_lock: if True, we will acquire a write-lock on the email record before returning + :returns: a database record (a named tuple) + """ - return (cursor or self.db).one(""" - SELECT * - FROM emails - WHERE participant_id=%s - AND address=%s - """, (self.id, address)) + sql = 'SELECT * FROM emails WHERE participant_id=%s AND address=%s' + if and_lock: + sql += ' FOR UPDATE' + return (cursor or self.db).one(sql, (self.id, address)) def get_emails(self): diff --git a/gratipay/models/team/package.py b/gratipay/models/team/package.py index dd33f7b76b..50de9a622a 100644 --- a/gratipay/models/team/package.py +++ b/gratipay/models/team/package.py @@ -16,4 +16,7 @@ def package(self): def _load_package(self, cursor): - return cursor.one('SELECT p.*::packages FROM packages p WHERE p.team_id=%s', (self.id,)) + return cursor.one( 'SELECT p.*::packages FROM packages p WHERE p.id=' + '(SELECT package_id FROM teams_to_packages tp WHERE tp.team_id=%s)' + , (self.id,) + ) diff --git a/sql/branch.sql b/sql/branch.sql index 8991d23794..4052aa0ec0 100644 --- a/sql/branch.sql +++ b/sql/branch.sql @@ -9,6 +9,9 @@ BEGIN; , UNIQUE(nonce, package_id) ); - ALTER TABLE packages ADD COLUMN team_id bigint UNIQUE REFERENCES teams(id) ON DELETE RESTRICT; + CREATE TABLE teams_to_packages + ( team_id bigint UNIQUE REFERENCES teams(id) ON DELETE RESTRICT + , package_id bigint UNIQUE REFERENCES packages(id) ON DELETE RESTRICT + ); END; diff --git a/tests/py/test_packages.py b/tests/py/test_packages.py index 23f9f10e46..1d48d5e5d9 100644 --- a/tests/py/test_packages.py +++ b/tests/py/test_packages.py @@ -51,6 +51,15 @@ def test_team_can_only_be_linked_from_one_package(self): bar = self.make_package(name='bar') raises( IntegrityError , self.db.run - , 'UPDATE packages SET team_id=%s WHERE id=%s' + , 'INSERT INTO teams_to_packages (team_id, package_id) VALUES (%s, %s)' , (team.id, bar.id) ) + + def test_package_can_only_be_linked_from_one_team(self): + alice, package, team = self.test_can_link_to_a_new_team() + bar = self.make_team(name='Bar') + raises( IntegrityError + , self.db.run + , 'INSERT INTO teams_to_packages (team_id, package_id) VALUES (%s, %s)' + , (bar.id, package.id) + )