From 404107ce5cb7b9201ae0c78e822f4a8c94e3e772 Mon Sep 17 00:00:00 2001 From: Chad Whitacre Date: Fri, 31 Mar 2017 15:25:26 -0400 Subject: [PATCH] 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"))