From 16cf16f1940a77cde394ea558f2256fd78d2eb74 Mon Sep 17 00:00:00 2001 From: Dustin Ingram Date: Fri, 16 Feb 2018 10:57:15 -0600 Subject: [PATCH] Manage emails (#2898) * Break out new email field into mixin * Add an AddEmailForm * Show errors from SaveProfileForm * Add an email TokenService * Verification email sending and templates * Add email verification view * Mange email views Adding emails, deleting emails, changing primary emails, resending verification emails. * Use a default response property * Make verify_email require Authenticated principal * Break out new email field into mixin * Add an AddEmailForm * Show errors from SaveProfileForm * Add an email TokenService * Verification email sending and templates * Add email verification view * Use a default response property * Mange email views Adding emails, deleting emails, changing primary emails, resending verification emails. * Make verify_email require Authenticated principal * Remove old/unused verify_email function * Activate user on email verification * Style email page, update messages * Fix linting errors --- dev/environment | 1 + tests/common/db/accounts.py | 1 + tests/unit/accounts/test_services.py | 10 - tests/unit/accounts/test_views.py | 163 ++++++- tests/unit/manage/test_forms.py | 9 + tests/unit/manage/test_views.py | 408 ++++++++++++++++-- tests/unit/test_email.py | 61 +++ tests/unit/test_routes.py | 5 + warehouse/accounts/__init__.py | 5 + warehouse/accounts/forms.py | 23 +- warehouse/accounts/interfaces.py | 5 - warehouse/accounts/services.py | 6 - warehouse/accounts/views.py | 53 ++- warehouse/config.py | 7 + warehouse/email.py | 31 ++ warehouse/manage/forms.py | 10 + warehouse/manage/views.py | 134 +++++- warehouse/routes.py | 5 + warehouse/static/sass/base/_typography.scss | 17 +- warehouse/static/sass/blocks/_badge.scss | 13 +- warehouse/static/sass/blocks/_dropdown.scss | 13 +- warehouse/static/sass/blocks/_footer.scss | 2 + warehouse/static/sass/blocks/_form-group.scss | 26 ++ warehouse/static/sass/blocks/_table.scss | 74 +++- .../templates/email/verify-email.body.txt | 13 + .../templates/email/verify-email.subject.txt | 1 + warehouse/templates/manage/profile.html | 145 ++++++- 27 files changed, 1152 insertions(+), 89 deletions(-) create mode 100644 warehouse/templates/email/verify-email.body.txt create mode 100644 warehouse/templates/email/verify-email.subject.txt diff --git a/dev/environment b/dev/environment index e57f52b30068..bdedbe897b36 100644 --- a/dev/environment +++ b/dev/environment @@ -34,3 +34,4 @@ MAIL_SSL=false STATUSPAGE_URL=https://2p66nmmycsj3.statuspage.io TOKEN_PASSWORD_SECRET="an insecure password reset secret key" +TOKEN_EMAIL_SECRET="an insecure email verification secret key" diff --git a/tests/common/db/accounts.py b/tests/common/db/accounts.py index d0645bc1b7c1..6bd88509196e 100644 --- a/tests/common/db/accounts.py +++ b/tests/common/db/accounts.py @@ -46,3 +46,4 @@ class Meta: user = factory.SubFactory(UserFactory) email = FuzzyEmail() verified = True + primary = True diff --git a/tests/unit/accounts/test_services.py b/tests/unit/accounts/test_services.py index f34c31037215..59f94ba2eb26 100644 --- a/tests/unit/accounts/test_services.py +++ b/tests/unit/accounts/test_services.py @@ -162,16 +162,6 @@ def test_update_user(self, user_service): assert password != user_from_db.password assert user_service.hasher.verify(password, user_from_db.password) - def test_verify_email(self, user_service): - user = UserFactory.create() - EmailFactory.create(user=user, primary=True, - verified=False) - EmailFactory.create(user=user, primary=False, - verified=False) - user_service.verify_email(user.id, user.emails[0].email) - assert user.emails[0].verified - assert not user.emails[1].verified - def test_find_by_email(self, user_service): user = UserFactory.create() EmailFactory.create(user=user, primary=True, verified=False) diff --git a/tests/unit/accounts/test_views.py b/tests/unit/accounts/test_views.py index 7097d13dff0d..9ff87e076cf0 100644 --- a/tests/unit/accounts/test_views.py +++ b/tests/unit/accounts/test_views.py @@ -18,6 +18,7 @@ import pytest from pyramid.httpexceptions import HTTPMovedPermanently, HTTPSeeOther +from sqlalchemy.orm.exc import NoResultFound from warehouse.accounts import views from warehouse.accounts.interfaces import ( @@ -25,7 +26,7 @@ TooManyFailedLogins ) -from ...common.db.accounts import UserFactory +from ...common.db.accounts import EmailFactory, UserFactory class TestFailedLoginView: @@ -695,6 +696,166 @@ def test_reset_password_password_date_changed(self, pyramid_request): ] +class TestVerifyEmail: + + def test_verify_email(self, db_request, user_service, token_service): + user = UserFactory(is_active=False) + email = EmailFactory(user=user, verified=False) + db_request.user = user + db_request.GET.update({"token": "RANDOM_KEY"}) + db_request.route_path = pretend.call_recorder(lambda name: "/") + token_service.loads = pretend.call_recorder( + lambda token: { + 'action': 'email-verify', + 'email.id': str(email.id), + } + ) + db_request.find_service = pretend.call_recorder( + lambda *a, **kwargs: token_service, + ) + db_request.session.flash = pretend.call_recorder( + lambda *a, **kw: None + ) + + result = views.verify_email(db_request) + + db_request.db.flush() + assert email.verified + assert user.is_active + assert isinstance(result, HTTPSeeOther) + assert result.headers["Location"] == "/" + assert db_request.route_path.calls == [pretend.call('manage.profile')] + assert token_service.loads.calls == [pretend.call('RANDOM_KEY')] + assert db_request.session.flash.calls == [ + pretend.call( + f"Email address {email.email} verified. " + + "You can now set this email as your primary address.", + queue="success" + ), + ] + assert db_request.find_service.calls == [ + pretend.call(ITokenService, name="email"), + ] + + @pytest.mark.parametrize( + ("exception", "message"), + [ + ( + TokenInvalid, + "Invalid token - Request a new verification link", + ), ( + TokenExpired, + "Expired token - Request a new verification link", + ), ( + TokenMissing, + "Invalid token - No token supplied" + ), + ], + ) + def test_verify_email_loads_failure( + self, pyramid_request, exception, message): + + def loads(token): + raise exception + + pyramid_request.find_service = ( + lambda *a, **kw: pretend.stub(loads=loads) + ) + pyramid_request.params = {"token": "RANDOM_KEY"} + pyramid_request.route_path = pretend.call_recorder(lambda name: "/") + pyramid_request.session.flash = pretend.call_recorder( + lambda *a, **kw: None + ) + + views.verify_email(pyramid_request) + + assert pyramid_request.route_path.calls == [ + pretend.call('manage.profile'), + ] + assert pyramid_request.session.flash.calls == [ + pretend.call(message, queue='error'), + ] + + def test_verify_email_invalid_action(self, pyramid_request): + data = { + 'action': 'invalid-action', + } + pyramid_request.find_service = ( + lambda *a, **kw: pretend.stub(loads=lambda a: data) + ) + pyramid_request.params = {"token": "RANDOM_KEY"} + pyramid_request.route_path = pretend.call_recorder(lambda name: "/") + pyramid_request.session.flash = pretend.call_recorder( + lambda *a, **kw: None + ) + + views.verify_email(pyramid_request) + + assert pyramid_request.route_path.calls == [ + pretend.call('manage.profile'), + ] + assert pyramid_request.session.flash.calls == [ + pretend.call( + "Invalid token - Not an email verification token", + queue='error', + ), + ] + + def test_verify_email_not_found(self, pyramid_request): + data = { + 'action': 'email-verify', + 'email.id': 'invalid', + } + pyramid_request.find_service = ( + lambda *a, **kw: pretend.stub(loads=lambda a: data) + ) + pyramid_request.params = {"token": "RANDOM_KEY"} + pyramid_request.route_path = pretend.call_recorder(lambda name: "/") + pyramid_request.session.flash = pretend.call_recorder( + lambda *a, **kw: None + ) + + def raise_no_result(*a): + raise NoResultFound + + pyramid_request.db = pretend.stub(query=raise_no_result) + + views.verify_email(pyramid_request) + + assert pyramid_request.route_path.calls == [ + pretend.call('manage.profile'), + ] + assert pyramid_request.session.flash.calls == [ + pretend.call('Email not found', queue='error') + ] + + def test_verify_email_already_verified(self, db_request): + user = UserFactory() + email = EmailFactory(user=user, verified=True) + data = { + 'action': 'email-verify', + 'email.id': email.id, + } + db_request.user = user + db_request.find_service = ( + lambda *a, **kw: pretend.stub(loads=lambda a: data) + ) + db_request.params = {"token": "RANDOM_KEY"} + db_request.route_path = pretend.call_recorder(lambda name: "/") + db_request.session.flash = pretend.call_recorder( + lambda *a, **kw: None + ) + + views.verify_email(db_request) + + assert db_request.route_path.calls == [ + pretend.call('manage.profile'), + ] + assert db_request.session.flash.calls == [ + pretend.call('Email already verified', queue='error') + ] + + class TestProfileCallout: def test_profile_callout_returns_user(self): diff --git a/tests/unit/manage/test_forms.py b/tests/unit/manage/test_forms.py index 8b637f658f9c..b51b26c0cd76 100644 --- a/tests/unit/manage/test_forms.py +++ b/tests/unit/manage/test_forms.py @@ -69,3 +69,12 @@ def test_validate_role_name_fails(self, value, expected): assert not form.validate() assert form.role_name.errors == [expected] + + +class TestAddEmailForm: + + def test_creation(self): + user_service = pretend.stub() + form = forms.AddEmailForm(user_service=user_service) + + assert form.user_service is user_service diff --git a/tests/unit/manage/test_views.py b/tests/unit/manage/test_views.py index a6d2bae99a7b..c1bacbd66c7d 100644 --- a/tests/unit/manage/test_views.py +++ b/tests/unit/manage/test_views.py @@ -24,6 +24,7 @@ from warehouse.accounts.interfaces import IUserService from warehouse.packaging.models import JournalEntry, Project, Role +from ...common.db.accounts import EmailFactory from ...common.db.packaging import ( JournalEntryFactory, ProjectFactory, RoleFactory, UserFactory, ) @@ -31,7 +32,7 @@ class TestManageProfile: - def test_manage_profile(self, monkeypatch): + def test_default_response(self, monkeypatch): user_service = pretend.stub() name = pretend.stub() request = pretend.stub( @@ -41,38 +42,64 @@ def test_manage_profile(self, monkeypatch): save_profile_obj = pretend.stub() save_profile_cls = pretend.call_recorder(lambda **kw: save_profile_obj) monkeypatch.setattr(views, 'SaveProfileForm', save_profile_cls) - view_class = views.ManageProfileViews(request) + add_email_obj = pretend.stub() + add_email_cls = pretend.call_recorder(lambda *a, **kw: add_email_obj) + monkeypatch.setattr(views, 'AddEmailForm', add_email_cls) + view = views.ManageProfileViews(request) - assert view_class.manage_profile() == { + assert view.default_response == { 'save_profile_form': save_profile_obj, + 'add_email_form': add_email_obj, } - assert view_class.request == request - assert view_class.user_service == user_service + assert view.request == request + assert view.user_service == user_service assert save_profile_cls.calls == [ pretend.call(name=name), ] + assert add_email_cls.calls == [ + pretend.call(user_service=user_service), + ] + + def test_manage_profile(self, monkeypatch): + user_service = pretend.stub() + name = pretend.stub() + request = pretend.stub( + find_service=lambda *a, **kw: user_service, + user=pretend.stub(name=name), + ) + monkeypatch.setattr( + views.ManageProfileViews, 'default_response', {'_': pretend.stub()} + ) + view = views.ManageProfileViews(request) + + assert view.manage_profile() == view.default_response + assert view.request == request + assert view.user_service == user_service def test_save_profile(self, monkeypatch): update_user = pretend.call_recorder(lambda *a, **kw: None) + user_service = pretend.stub(update_user=update_user) request = pretend.stub( POST={'name': 'new name'}, - user=pretend.stub(id=pretend.stub()), + user=pretend.stub(id=pretend.stub(), name=pretend.stub()), session=pretend.stub( flash=pretend.call_recorder(lambda *a, **kw: None), ), - find_service=pretend.call_recorder( - lambda iface, context: pretend.stub(update_user=update_user) - ), + find_service=lambda *a, **kw: user_service, ) save_profile_obj = pretend.stub( - validate=lambda: True, - data=request.POST, + validate=lambda: True, data=request.POST ) - save_profile_cls = pretend.call_recorder(lambda d: save_profile_obj) - monkeypatch.setattr(views, 'SaveProfileForm', save_profile_cls) - view_class = views.ManageProfileViews(request) + monkeypatch.setattr( + views, 'SaveProfileForm', lambda *a, **kw: save_profile_obj + ) + monkeypatch.setattr( + views.ManageProfileViews, 'default_response', {'_': pretend.stub()} + ) + view = views.ManageProfileViews(request) - assert view_class.save_profile() == { + assert view.save_profile() == { + **view.default_response, 'save_profile_form': save_profile_obj, } assert request.session.flash.calls == [ @@ -81,35 +108,360 @@ def test_save_profile(self, monkeypatch): assert update_user.calls == [ pretend.call(request.user.id, **request.POST) ] - assert save_profile_cls.calls == [ - pretend.call(request.POST), - ] def test_save_profile_validation_fails(self, monkeypatch): update_user = pretend.call_recorder(lambda *a, **kw: None) + user_service = pretend.stub(update_user=update_user) request = pretend.stub( POST={'name': 'new name'}, - user=pretend.stub(id=pretend.stub()), + user=pretend.stub(id=pretend.stub(), name=pretend.stub()), session=pretend.stub( flash=pretend.call_recorder(lambda *a, **kw: None), ), - find_service=pretend.call_recorder( - lambda iface, context: pretend.stub(update_user=update_user) - ), + find_service=lambda *a, **kw: user_service, ) save_profile_obj = pretend.stub(validate=lambda: False) - save_profile_cls = pretend.call_recorder(lambda d: save_profile_obj) - monkeypatch.setattr(views, 'SaveProfileForm', save_profile_cls) - view_class = views.ManageProfileViews(request) + monkeypatch.setattr( + views, 'SaveProfileForm', lambda *a, **kw: save_profile_obj + ) + monkeypatch.setattr( + views.ManageProfileViews, 'default_response', {'_': pretend.stub()} + ) + view = views.ManageProfileViews(request) - assert view_class.save_profile() == { + assert view.save_profile() == { + **view.default_response, 'save_profile_form': save_profile_obj, } assert request.session.flash.calls == [] assert update_user.calls == [] - assert save_profile_cls.calls == [ - pretend.call(request.POST), + + def test_add_email(self, monkeypatch, pyramid_config): + email_address = "test@example.com" + user_service = pretend.stub() + request = pretend.stub( + POST={'email': email_address}, + db=pretend.stub(flush=lambda: None), + session=pretend.stub( + flash=pretend.call_recorder(lambda *a, **kw: None), + ), + find_service=lambda a, **kw: user_service, + user=pretend.stub( + emails=[], name=pretend.stub(), id=pretend.stub() + ), + task=pretend.call_recorder(lambda *args, **kwargs: send_email), + ) + monkeypatch.setattr( + views, 'AddEmailForm', lambda *a, **kw: pretend.stub( + validate=lambda: True, + email=pretend.stub(data=email_address), + ) + ) + + email_obj = pretend.stub(id=pretend.stub(), email=email_address) + email_cls = pretend.call_recorder(lambda **kw: email_obj) + monkeypatch.setattr(views, 'Email', email_cls) + + send_email = pretend.call_recorder(lambda *a: None) + monkeypatch.setattr(views, 'send_email_verification_email', send_email) + + monkeypatch.setattr( + views.ManageProfileViews, 'default_response', {'_': pretend.stub()} + ) + view = views.ManageProfileViews(request) + + assert view.add_email() == view.default_response + assert request.user.emails == [email_obj] + assert email_cls.calls == [ + pretend.call( + email=email_address, + user_id=request.user.id, + primary=False, + verified=False, + ), + ] + assert request.session.flash.calls == [ + pretend.call( + f'Email {email_address} added - check your email for ' + + 'a verification link.', + queue='success', + ), + ] + assert send_email.calls == [ + pretend.call(request, email_obj), + ] + + def test_add_email_validation_fails(self, monkeypatch): + email_address = "test@example.com" + request = pretend.stub( + POST={'email': email_address}, + db=pretend.stub(flush=lambda: None), + session=pretend.stub( + flash=pretend.call_recorder(lambda *a, **kw: None), + ), + find_service=lambda a, **kw: pretend.stub(), + user=pretend.stub(emails=[], name=pretend.stub()), + ) + add_email_obj = pretend.stub( + validate=lambda: False, + email=pretend.stub(data=email_address), + ) + add_email_cls = pretend.call_recorder(lambda *a, **kw: add_email_obj) + monkeypatch.setattr(views, 'AddEmailForm', add_email_cls) + + email_obj = pretend.stub(id=pretend.stub(), email=email_address) + email_cls = pretend.call_recorder(lambda **kw: email_obj) + monkeypatch.setattr(views, 'Email', email_cls) + + monkeypatch.setattr( + views.ManageProfileViews, 'default_response', {'_': pretend.stub()} + ) + view = views.ManageProfileViews(request) + + assert view.add_email() == { + **view.default_response, + 'add_email_form': add_email_obj, + } + assert request.user.emails == [] + assert email_cls.calls == [] + assert request.session.flash.calls == [] + + def test_delete_email(self, monkeypatch): + email = pretend.stub( + id=pretend.stub(), primary=False, email=pretend.stub(), + ) + some_other_email = pretend.stub() + request = pretend.stub( + POST={'delete_email_id': email.id}, + user=pretend.stub( + id=pretend.stub(), + emails=[email, some_other_email], + name=pretend.stub(), + ), + db=pretend.stub( + query=lambda a: pretend.stub( + filter=lambda *a: pretend.stub(one=lambda: email) + ), + ), + find_service=lambda *a, **kw: pretend.stub(), + session=pretend.stub( + flash=pretend.call_recorder(lambda *a, **kw: None), + ) + ) + monkeypatch.setattr( + views.ManageProfileViews, 'default_response', {'_': pretend.stub()} + ) + view = views.ManageProfileViews(request) + + assert view.delete_email() == view.default_response + assert request.session.flash.calls == [ + pretend.call( + f'Email address {email.email} removed.', queue='success' + ) + ] + assert request.user.emails == [some_other_email] + + def test_delete_email_not_found(self, monkeypatch): + email = pretend.stub() + + def raise_no_result(): + raise NoResultFound + + request = pretend.stub( + POST={'delete_email_id': 'missing_id'}, + user=pretend.stub( + id=pretend.stub(), + emails=[email], + name=pretend.stub(), + ), + db=pretend.stub( + query=lambda a: pretend.stub( + filter=lambda *a: pretend.stub(one=raise_no_result) + ), + ), + find_service=lambda *a, **kw: pretend.stub(), + session=pretend.stub( + flash=pretend.call_recorder(lambda *a, **kw: None), + ) + ) + monkeypatch.setattr( + views.ManageProfileViews, 'default_response', {'_': pretend.stub()} + ) + view = views.ManageProfileViews(request) + + assert view.delete_email() == view.default_response + assert request.session.flash.calls == [ + pretend.call('Email address not found.', queue='error'), + ] + assert request.user.emails == [email] + + def test_delete_email_is_primary(self, monkeypatch): + email = pretend.stub(primary=True) + + request = pretend.stub( + POST={'delete_email_id': 'missing_id'}, + user=pretend.stub( + id=pretend.stub(), + emails=[email], + name=pretend.stub(), + ), + db=pretend.stub( + query=lambda a: pretend.stub( + filter=lambda *a: pretend.stub(one=lambda: email) + ), + ), + find_service=lambda *a, **kw: pretend.stub(), + session=pretend.stub( + flash=pretend.call_recorder(lambda *a, **kw: None), + ) + ) + monkeypatch.setattr( + views.ManageProfileViews, 'default_response', {'_': pretend.stub()} + ) + view = views.ManageProfileViews(request) + + assert view.delete_email() == view.default_response + assert request.session.flash.calls == [ + pretend.call( + 'Cannot remove primary email address.', queue='error' + ), + ] + assert request.user.emails == [email] + + def test_change_primary_email(self, monkeypatch, db_request): + user = UserFactory() + old_primary = EmailFactory(primary=True, user=user) + new_primary = EmailFactory(primary=False, verified=True, user=user) + + db_request.user = user + db_request.find_service = lambda *a, **kw: pretend.stub() + db_request.POST = {'primary_email_id': new_primary.id} + db_request.session.flash = pretend.call_recorder(lambda *a, **kw: None) + monkeypatch.setattr( + views.ManageProfileViews, 'default_response', {'_': pretend.stub()} + ) + view = views.ManageProfileViews(db_request) + + assert view.change_primary_email() == view.default_response + assert db_request.session.flash.calls == [ + pretend.call( + f'Email address {new_primary.email} set as primary.', + queue='success', + ) + ] + assert not old_primary.primary + assert new_primary.primary + + def test_change_primary_email_not_found(self, monkeypatch, db_request): + user = UserFactory() + old_primary = EmailFactory(primary=True, user=user) + missing_email_id = 9999 + + db_request.user = user + db_request.find_service = lambda *a, **kw: pretend.stub() + db_request.POST = {'primary_email_id': missing_email_id} + db_request.session.flash = pretend.call_recorder(lambda *a, **kw: None) + monkeypatch.setattr( + views.ManageProfileViews, 'default_response', {'_': pretend.stub()} + ) + view = views.ManageProfileViews(db_request) + + assert view.change_primary_email() == view.default_response + assert db_request.session.flash.calls == [ + pretend.call(f'Email address not found.', queue='error') + ] + assert old_primary.primary + + def test_reverify_email(self, monkeypatch): + email = pretend.stub(verified=False, email='email_address') + + request = pretend.stub( + POST={'reverify_email_id': pretend.stub()}, + db=pretend.stub( + query=lambda *a: pretend.stub( + filter=lambda *a: pretend.stub(one=lambda: email) + ), + ), + session=pretend.stub( + flash=pretend.call_recorder(lambda *a, **kw: None) + ), + find_service=lambda *a, **kw: pretend.stub(), + user=pretend.stub(id=pretend.stub), + ) + send_email = pretend.call_recorder(lambda *a: None) + monkeypatch.setattr(views, 'send_email_verification_email', send_email) + monkeypatch.setattr( + views.ManageProfileViews, 'default_response', {'_': pretend.stub()} + ) + view = views.ManageProfileViews(request) + + assert view.reverify_email() == view.default_response + assert request.session.flash.calls == [ + pretend.call( + 'Verification email for email_address resent.', + queue='success', + ), + ] + assert send_email.calls == [pretend.call(request, email)] + + def test_reverify_email_not_found(self, monkeypatch): + def raise_no_result(): + raise NoResultFound + + request = pretend.stub( + POST={'reverify_email_id': pretend.stub()}, + db=pretend.stub( + query=lambda *a: pretend.stub( + filter=lambda *a: pretend.stub(one=raise_no_result) + ), + ), + session=pretend.stub( + flash=pretend.call_recorder(lambda *a, **kw: None) + ), + find_service=lambda *a, **kw: pretend.stub(), + user=pretend.stub(id=pretend.stub), + ) + send_email = pretend.call_recorder(lambda *a: None) + monkeypatch.setattr(views, 'send_email_verification_email', send_email) + monkeypatch.setattr( + views.ManageProfileViews, 'default_response', {'_': pretend.stub()} + ) + view = views.ManageProfileViews(request) + + assert view.reverify_email() == view.default_response + assert request.session.flash.calls == [ + pretend.call('Email address not found.', queue='error'), + ] + assert send_email.calls == [] + + def test_reverify_email_already_verified(self, monkeypatch): + email = pretend.stub(verified=True, email='email_address') + + request = pretend.stub( + POST={'reverify_email_id': pretend.stub()}, + db=pretend.stub( + query=lambda *a: pretend.stub( + filter=lambda *a: pretend.stub(one=lambda: email) + ), + ), + session=pretend.stub( + flash=pretend.call_recorder(lambda *a, **kw: None) + ), + find_service=lambda *a, **kw: pretend.stub(), + user=pretend.stub(id=pretend.stub), + ) + send_email = pretend.call_recorder(lambda *a: None) + monkeypatch.setattr(views, 'send_email_verification_email', send_email) + monkeypatch.setattr( + views.ManageProfileViews, 'default_response', {'_': pretend.stub()} + ) + view = views.ManageProfileViews(request) + + assert view.reverify_email() == view.default_response + assert request.session.flash.calls == [ + pretend.call('Email is already verified.', queue='error'), ] + assert send_email.calls == [] class TestManageProjects: diff --git a/tests/unit/test_email.py b/tests/unit/test_email.py index 417546d25682..2cd71e04c863 100644 --- a/tests/unit/test_email.py +++ b/tests/unit/test_email.py @@ -156,3 +156,64 @@ def test_send_password_reset_email( assert send_email.delay.calls == [ pretend.call('Email Body', [stub_user.email], 'Email Subject'), ] + + +class TestEmailVerificationEmail: + + def test_email_verification_email( + self, pyramid_request, pyramid_config, token_service, monkeypatch): + + stub_email = pretend.stub( + id='id', + email='email', + ) + pyramid_request.method = 'POST' + token_service.dumps = pretend.call_recorder(lambda a: 'TOKEN') + pyramid_request.find_service = pretend.call_recorder( + lambda *a, **kw: token_service + ) + + subject_renderer = pyramid_config.testing_add_renderer( + 'email/verify-email.subject.txt' + ) + subject_renderer.string_response = 'Email Subject' + body_renderer = pyramid_config.testing_add_renderer( + 'email/verify-email.body.txt' + ) + body_renderer.string_response = 'Email Body' + + send_email = pretend.stub( + delay=pretend.call_recorder(lambda *args, **kwargs: None) + ) + pyramid_request.task = pretend.call_recorder( + lambda *args, **kwargs: send_email + ) + monkeypatch.setattr(email, 'send_email', send_email) + + result = email.send_email_verification_email( + pyramid_request, + email=stub_email, + ) + + assert result == { + 'token': 'TOKEN', + 'email_address': stub_email.email, + 'n_hours': token_service.max_age // 60 // 60, + } + subject_renderer.assert_() + body_renderer.assert_(token='TOKEN', email_address=stub_email.email) + assert token_service.dumps.calls == [ + pretend.call({ + 'action': 'email-verify', + 'email.id': str(stub_email.id), + }), + ] + assert pyramid_request.find_service.calls == [ + pretend.call(ITokenService, name='email'), + ] + assert pyramid_request.task.calls == [ + pretend.call(send_email), + ] + assert send_email.delay.calls == [ + pretend.call('Email Body', [stub_email.email], 'Email Subject'), + ] diff --git a/tests/unit/test_routes.py b/tests/unit/test_routes.py index 5f9fec63a8f5..7daa7c7b97be 100644 --- a/tests/unit/test_routes.py +++ b/tests/unit/test_routes.py @@ -137,6 +137,11 @@ def add_policy(name, filename): "/account/reset-password/", domain=warehouse, ), + pretend.call( + "accounts.verify-email", + "/account/verify-email/", + domain=warehouse, + ), pretend.call( "manage.profile", "/manage/profile/", diff --git a/warehouse/accounts/__init__.py b/warehouse/accounts/__init__.py index 3c78ddfca0ef..454736614d60 100644 --- a/warehouse/accounts/__init__.py +++ b/warehouse/accounts/__init__.py @@ -75,6 +75,11 @@ def includeme(config): ITokenService, name="password", ) + config.register_service_factory( + TokenServiceFactory(name="email"), + ITokenService, + name="email", + ) # Register our authentication and authorization policies config.set_authentication_policy( diff --git a/warehouse/accounts/forms.py b/warehouse/accounts/forms.py index 2f97589a7b55..d5eedc14beff 100644 --- a/warehouse/accounts/forms.py +++ b/warehouse/accounts/forms.py @@ -100,9 +100,7 @@ class NewPasswordMixin: ) -class RegistrationForm(NewPasswordMixin, NewUsernameMixin, forms.Form): - - full_name = wtforms.StringField() +class NewEmailMixin: email = wtforms.fields.html5.EmailField( validators=[ @@ -116,13 +114,6 @@ class RegistrationForm(NewPasswordMixin, NewUsernameMixin, forms.Form): ], ) - g_recaptcha_response = wtforms.StringField() - - def __init__(self, *args, recaptcha_service, user_service, **kwargs): - super().__init__(*args, **kwargs) - self.user_service = user_service - self.recaptcha_service = recaptcha_service - def validate_email(self, field): if self.user_service.find_userid_by_email(field.data) is not None: raise wtforms.validators.ValidationError( @@ -136,6 +127,18 @@ def validate_email(self, field): "from this domain. Please use a different email." ) + +class RegistrationForm( + NewPasswordMixin, NewUsernameMixin, NewEmailMixin, forms.Form): + + full_name = wtforms.StringField() + g_recaptcha_response = wtforms.StringField() + + def __init__(self, *args, recaptcha_service, user_service, **kwargs): + super().__init__(*args, **kwargs) + self.user_service = user_service + self.recaptcha_service = recaptcha_service + def validate_g_recaptcha_response(self, field): # do required data validation here due to enabled flag being required if self.recaptcha_service.enabled and not field.data: diff --git a/warehouse/accounts/interfaces.py b/warehouse/accounts/interfaces.py index e519f0c6e43b..80c85ff02109 100644 --- a/warehouse/accounts/interfaces.py +++ b/warehouse/accounts/interfaces.py @@ -73,11 +73,6 @@ def update_user(user_id, **changes): Updates the user object """ - def verify_email(user_id, email_address): - """ - verifies the user - """ - class ITokenService(Interface): diff --git a/warehouse/accounts/services.py b/warehouse/accounts/services.py index fd38d7042fc6..02c76eaef871 100644 --- a/warehouse/accounts/services.py +++ b/warehouse/accounts/services.py @@ -173,12 +173,6 @@ def update_user(self, user_id, **changes): setattr(user, attr, value) return user - def verify_email(self, user_id, email_address): - user = self.get_user(user_id) - for email in user.emails: - if email.email == email_address: - email.verified = True - @implementer(ITokenService) class TokenService: diff --git a/warehouse/accounts/views.py b/warehouse/accounts/views.py index 5b3f3fd93b99..0c0286723b14 100644 --- a/warehouse/accounts/views.py +++ b/warehouse/accounts/views.py @@ -17,9 +17,10 @@ from pyramid.httpexceptions import ( HTTPMovedPermanently, HTTPSeeOther, HTTPTooManyRequests, ) -from pyramid.security import remember, forget +from pyramid.security import Authenticated, remember, forget from pyramid.view import view_config from sqlalchemy.orm import joinedload +from sqlalchemy.orm.exc import NoResultFound from warehouse.accounts import REDIRECT_FIELD_NAME from warehouse.accounts.forms import ( @@ -29,6 +30,7 @@ IUserService, ITokenService, TokenExpired, TokenInvalid, TokenMissing, TooManyFailedLogins, ) +from warehouse.accounts.models import Email from warehouse.cache.origin import origin_cache from warehouse.email import send_password_reset_email from warehouse.packaging.models import Project, Release @@ -337,6 +339,55 @@ def _error(message): return {"form": form} +@view_config( + route_name="accounts.verify-email", + uses_session=True, + effective_principals=Authenticated, +) +def verify_email(request): + token_service = request.find_service(ITokenService, name="email") + + def _error(message): + request.session.flash(message, queue="error") + return HTTPSeeOther(request.route_path("manage.profile")) + + try: + token = request.params.get('token') + data = token_service.loads(token) + except TokenExpired: + return _error("Expired token - Request a new verification link") + except TokenInvalid: + return _error("Invalid token - Request a new verification link") + except TokenMissing: + return _error("Invalid token - No token supplied") + + # Check whether this token is being used correctly + if data.get('action') != "email-verify": + return _error("Invalid token - Not an email verification token") + + try: + email = ( + request.db.query(Email) + .filter(Email.id == data['email.id'], Email.user == request.user) + .one() + ) + except NoResultFound: + return _error("Email not found") + + if email.verified: + return _error("Email already verified") + + email.verified = True + request.user.is_active = True + + request.session.flash( + f'Email address {email.email} verified. ' + + 'You can now set this email as your primary address.', + queue='success' + ) + return HTTPSeeOther(request.route_path("manage.profile")) + + def _login_user(request, userid): # We have a session factory associated with this request, so in order # to protect against session fixation attacks we're going to make sure diff --git a/warehouse/config.py b/warehouse/config.py index 896a1862878a..9d183115ce48 100644 --- a/warehouse/config.py +++ b/warehouse/config.py @@ -173,12 +173,19 @@ def configure(settings=None): maybe_set(settings, "ga.tracking_id", "GA_TRACKING_ID") maybe_set(settings, "statuspage.url", "STATUSPAGE_URL") maybe_set(settings, "token.password.secret", "TOKEN_PASSWORD_SECRET") + maybe_set(settings, "token.email.secret", "TOKEN_EMAIL_SECRET") maybe_set( settings, "token.password.max_age", "TOKEN_PASSWORD_MAX_AGE", coercer=int, ) + maybe_set( + settings, + "token.email.max_age", + "TOKEN_EMAIL_MAX_AGE", + coercer=int, + ) maybe_set( settings, "token.default.max_age", diff --git a/warehouse/email.py b/warehouse/email.py index d398ba5dacd8..e6c077999ad4 100644 --- a/warehouse/email.py +++ b/warehouse/email.py @@ -67,3 +67,34 @@ def send_password_reset_email(request, user): # Return the fields we used, in case we need to show any of them to the # user return fields + + +def send_email_verification_email(request, email): + token_service = request.find_service(ITokenService, name='email') + + token = token_service.dumps({ + "action": "email-verify", + "email.id": email.id, + }) + + fields = { + 'token': token, + 'email_address': email.email, + 'n_hours': token_service.max_age // 60 // 60, + } + + subject = render( + 'email/verify-email.subject.txt', + fields, + request=request, + ) + + body = render( + 'email/verify-email.body.txt', + fields, + request=request, + ) + + request.task(send_email).delay(body, [email.email], subject) + + return fields diff --git a/warehouse/manage/forms.py b/warehouse/manage/forms.py index 427a88d7061e..e49b3c54bf27 100644 --- a/warehouse/manage/forms.py +++ b/warehouse/manage/forms.py @@ -13,6 +13,7 @@ import wtforms from warehouse import forms +from warehouse.accounts.forms import NewEmailMixin class RoleNameMixin: @@ -62,3 +63,12 @@ class SaveProfileForm(forms.Form): __params__ = ['name'] name = wtforms.StringField() + + +class AddEmailForm(NewEmailMixin, forms.Form): + + __params__ = ['email'] + + def __init__(self, *args, user_service, **kwargs): + super().__init__(*args, **kwargs) + self.user_service = user_service diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index 41f82ed8b70b..7bc82e6e91fc 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -19,9 +19,10 @@ from sqlalchemy.orm.exc import NoResultFound from warehouse.accounts.interfaces import IUserService -from warehouse.accounts.models import User +from warehouse.accounts.models import User, Email +from warehouse.email import send_email_verification_email from warehouse.manage.forms import ( - CreateRoleForm, ChangeRoleForm, SaveProfileForm + AddEmailForm, CreateRoleForm, ChangeRoleForm, SaveProfileForm ) from warehouse.packaging.models import JournalEntry, Role, File from warehouse.utils.project import confirm_project, remove_project @@ -40,12 +41,17 @@ def __init__(self, request): self.request = request self.user_service = request.find_service(IUserService, context=None) - @view_config(request_method="GET") - def manage_profile(self): + @property + def default_response(self): return { 'save_profile_form': SaveProfileForm(name=self.request.user.name), + 'add_email_form': AddEmailForm(user_service=self.user_service), } + @view_config(request_method="GET") + def manage_profile(self): + return self.default_response + @view_config( request_method="POST", request_param=SaveProfileForm.__params__, @@ -60,9 +66,129 @@ def save_profile(self): ) return { + **self.default_response, 'save_profile_form': form, } + @view_config( + request_method="POST", + request_param=AddEmailForm.__params__, + ) + def add_email(self): + form = AddEmailForm(self.request.POST, user_service=self.user_service) + + if form.validate(): + email = Email( + email=form.email.data, + user_id=self.request.user.id, + primary=False, + verified=False, + ) + self.request.user.emails.append(email) + self.request.db.flush() # To get the new ID + + send_email_verification_email(self.request, email) + + self.request.session.flash( + f'Email {email.email} added - check your email for ' + + 'a verification link.', + queue='success', + ) + return self.default_response + + return { + **self.default_response, + 'add_email_form': form, + } + + @view_config( + request_method="POST", + request_param=["delete_email_id"], + ) + def delete_email(self): + try: + email = self.request.db.query(Email).filter( + Email.id == self.request.POST['delete_email_id'], + Email.user_id == self.request.user.id, + ).one() + except NoResultFound: + self.request.session.flash( + 'Email address not found.', queue='error' + ) + return self.default_response + + if email.primary: + self.request.session.flash( + 'Cannot remove primary email address.', queue='error' + ) + else: + self.request.user.emails.remove(email) + self.request.session.flash( + f'Email address {email.email} removed.', queue='success' + ) + return self.default_response + + @view_config( + request_method="POST", + request_param=["primary_email_id"], + ) + def change_primary_email(self): + try: + new_primary_email = self.request.db.query(Email).filter( + Email.user_id == self.request.user.id, + Email.id == self.request.POST['primary_email_id'], + Email.verified.is_(True), + ).one() + except NoResultFound: + self.request.session.flash( + 'Email address not found.', queue='error' + ) + return self.default_response + + self.request.db.query(Email).filter( + Email.user_id == self.request.user.id, + Email.primary.is_(True), + ).update(values={'primary': False}) + + new_primary_email.primary = True + + self.request.session.flash( + f'Email address {new_primary_email.email} set as primary.', + queue='success', + ) + + return self.default_response + + @view_config( + request_method="POST", + request_param=['reverify_email_id'], + ) + def reverify_email(self): + try: + email = self.request.db.query(Email).filter( + Email.id == self.request.POST['reverify_email_id'], + Email.user_id == self.request.user.id, + ).one() + except NoResultFound: + self.request.session.flash( + 'Email address not found.', queue='error' + ) + return self.default_response + + if email.verified: + self.request.session.flash( + 'Email is already verified.', queue='error' + ) + else: + send_email_verification_email(self.request, email) + + self.request.session.flash( + f'Verification email for {email.email} resent.', + queue='success', + ) + + return self.default_response + @view_config( route_name="manage.projects", diff --git a/warehouse/routes.py b/warehouse/routes.py index eb65aa5906be..0b7c279ac7dc 100644 --- a/warehouse/routes.py +++ b/warehouse/routes.py @@ -109,6 +109,11 @@ def includeme(config): "/account/reset-password/", domain=warehouse, ) + config.add_route( + "accounts.verify-email", + "/account/verify-email/", + domain=warehouse, + ) # Management (views for logged-in users) config.add_route("manage.profile", "/manage/profile/", domain=warehouse) diff --git a/warehouse/static/sass/base/_typography.scss b/warehouse/static/sass/base/_typography.scss index 653e49b99a2e..a27499d17e19 100644 --- a/warehouse/static/sass/base/_typography.scss +++ b/warehouse/static/sass/base/_typography.scss @@ -88,7 +88,13 @@ h2:first-child, h3:first-child, h4:first-child, h5:first-child, -h6:first-child { +h6:first-child, +hr + h1, +hr + h2, +hr + h3, +hr + h4, +hr + h5, +hr + h6 { padding-top: 0; } @@ -116,7 +122,6 @@ a { background: $primary-color; } - // VERTICAL RHYTHM // Set `padding-bottom` to maintain vertical rhythm @@ -144,3 +149,11 @@ dl:last-child, figure:last-child { padding-bottom: 0; } + +hr { + margin: $spacing-unit 0; + border: 0; + height: 2px; + background: $border-color; + background-image: linear-gradient(to right, $border-color, $white); +} diff --git a/warehouse/static/sass/blocks/_badge.scss b/warehouse/static/sass/blocks/_badge.scss index 9e6d823da7fd..db9220b8cb52 100644 --- a/warehouse/static/sass/blocks/_badge.scss +++ b/warehouse/static/sass/blocks/_badge.scss @@ -22,8 +22,17 @@ .badge { font-size: $small-font-size; text-transform: uppercase; - color: darken($primary-color, 50); - background-color: $primary-color; + background-color: $brand-color; + color: $white; padding: 2px 7px; border-radius: 3px; + font-weight: 600; + + &--success { + background-color: $success-color; + } + + &--danger { + background-color: $danger-color; + } } diff --git a/warehouse/static/sass/blocks/_dropdown.scss b/warehouse/static/sass/blocks/_dropdown.scss index 6913ff93ff37..1fd3cfda0111 100644 --- a/warehouse/static/sass/blocks/_dropdown.scss +++ b/warehouse/static/sass/blocks/_dropdown.scss @@ -49,6 +49,7 @@ box-shadow: 1px 1px 2px 1px rgba(0, 0, 0, 0.05); z-index: index($z-index-scale, "dropdown"); border: 1px solid $border-color; + border-bottom: 0; // Each link has a bottom border, so we don't need one here display: none; } @@ -64,6 +65,7 @@ button.dropdown__link { display: block; padding: 15px 15px 15px 15px; + border: 0; // Remove default border on buttons border-bottom: 1px solid $border-color; background-color: $white; min-width: 175px; @@ -72,10 +74,6 @@ text-align: left; position: relative; - &:last-child { - border-bottom: 0; - } - &:hover { background-color: $base-grey; color: $text-color; @@ -123,4 +121,11 @@ padding: 15px 15px 15px 40px; } } + + &--wide { + .dropdown__link, + button.dropdown__link { + min-width: 200px; + } + } } diff --git a/warehouse/static/sass/blocks/_footer.scss b/warehouse/static/sass/blocks/_footer.scss index 262ab6a159e0..48f7478fe26e 100644 --- a/warehouse/static/sass/blocks/_footer.scss +++ b/warehouse/static/sass/blocks/_footer.scss @@ -94,6 +94,8 @@ border: 1px solid transparentize($footer-faded-color, 0.3); margin: 0 auto $spacing-unit; width: 200px; + background: none; + height: 0; } &__text { diff --git a/warehouse/static/sass/blocks/_form-group.scss b/warehouse/static/sass/blocks/_form-group.scss index 0705545b8743..37eb41852cd8 100644 --- a/warehouse/static/sass/blocks/_form-group.scss +++ b/warehouse/static/sass/blocks/_form-group.scss @@ -52,4 +52,30 @@ font-size: $small-font-size; font-style: italic; } + + &__horizontal { + display: flex; + } + + &__horizontal-left { + display: inline-block; + max-width: 100%; + } + + &__horizontal-right { + margin-left: 10px; + display: inline-block; + max-width: 100%; + } + + @media only screen and (max-width: $small-tablet) { + &__horizontal { + flex-wrap: wrap; + } + + &__horizontal-right { + margin-left: 0; + margin-top: 10px; + } + } } diff --git a/warehouse/static/sass/blocks/_table.scss b/warehouse/static/sass/blocks/_table.scss index 00fba2557b57..461fc9ee9f98 100644 --- a/warehouse/static/sass/blocks/_table.scss +++ b/warehouse/static/sass/blocks/_table.scss @@ -242,7 +242,8 @@ width: 100%; } - .dropdown__content { + .dropdown__content, + .dropdown__link { width: 100%; } } @@ -322,7 +323,8 @@ width: 100%; } - .dropdown__content { + .dropdown__content, + .dropdown__link { width: 100%; } } @@ -492,12 +494,31 @@ } } - &--history { - margin-top: 10px; + &--emails { + margin: 0 0 $spacing-unit; - .table__ip { - display: block; - font-style: italic; + tr td { + padding-right: 20px; + + &:last-of-type { + padding-right: 0; + } + } + + .table__email { + padding: 20px 20px 20px 0; + word-wrap: break-word; + word-break: break-all; + font-family: $code-font; + font-size: 90%; + } + + .table__status { + width: 190px; + } + + .table__action { + width: 100px; } @media only screen and (max-width: $small-tablet) { @@ -508,23 +529,46 @@ tbody tr, tbody tr:nth-child(2n) { display: block; - padding: 10px; + padding: 20px 0; border-bottom: 1px solid $base-grey; } - td { + tbody tr:last-of-type { + border-bottom: 0; + } + + .table__email, + .table__status, + .table__action { + width: 100%; display: block; text-align: left; - padding: 2px 0; - border: 0; + border-bottom: 0; } - .table__action { - font-weight: 600; + .table__email, + .table__status { + padding: 4px 0; } - .table__ip { - display: inline; + .table__action { + padding: 0; + + .dropdown { + display: block; + float: none; + margin: 8px 0 4px 0; + + .button { + display: block; + width: 100%; + } + + .dropdown__content, + .dropdown__link { + width: 100%; + } + } } } } diff --git a/warehouse/templates/email/verify-email.body.txt b/warehouse/templates/email/verify-email.body.txt new file mode 100644 index 000000000000..d4553669a6e8 --- /dev/null +++ b/warehouse/templates/email/verify-email.body.txt @@ -0,0 +1,13 @@ +Someone, perhaps you, has added this email address to their PyPI account: + + {{ email_address }} + +If you wish to proceed with this request, please follow the link below to +verify your email address: + + {{ request.route_url('accounts.verify-email', _query={'token': token}) }} + +This link will expire in {{ n_hours }} hours. + +If you did not make this request, you can safely ignore this email, or reply to +this email directly to communicate with the PyPI administrators. diff --git a/warehouse/templates/email/verify-email.subject.txt b/warehouse/templates/email/verify-email.subject.txt new file mode 100644 index 000000000000..97359c5e23f9 --- /dev/null +++ b/warehouse/templates/email/verify-email.subject.txt @@ -0,0 +1 @@ +PyPI Email Verification diff --git a/warehouse/templates/manage/profile.html b/warehouse/templates/manage/profile.html index e16ae9d3d316..b965a19b4de3 100644 --- a/warehouse/templates/manage/profile.html +++ b/warehouse/templates/manage/profile.html @@ -21,6 +21,59 @@ {% block title %}{{ title }}{% endblock %} +{% macro email_row(email) -%} + + + {{ email.email }} + + + {% if email.primary %} + Primary + {% endif %} + {% if email.verified %} + Verified + {% else %} + Unverified + {% endif %} + + + {% if not email.verified or user.emails|length > 1 and not email.primary%} +
+ +
+ {% if not email.verified %} +
+ + + +
+ {% endif %} + {% if user.emails|length > 1 and not email.primary %} +
+ + + +
+ {% endif %} +
+
+ {% endif %} + + +{%- endmacro %} + {% block main %}

{{ title }}

Profile Picture

@@ -39,6 +92,8 @@

Profile Picture

+
+

Account Details

@@ -49,11 +104,99 @@

Account Details

+ + {% if save_profile_form.errors.__all__ %} +
    + {% for error in save_profile_form.errors.__all__ %} +
  • {{ error }}
    • + {% endfor %} +
+ {% endif %} +
- + {{ save_profile_form.name(placeholder="No name set", class_="form-group__input") }}

Displayed on your public profile.

+ {% if save_profile_form.name.errors %} +
    + {% for error in save_profile_form.name.errors %} +
  • {{ error }}
    • + {% endfor %} +
+ {% endif %}
+ +
+ +

Account Emails

+

You can associate several emails with your account. You can use any verified email to recover your account, but only your primary email will receive notifications.

+ + {# Sort the emails as follows: + * Primary email + * Verified emails, sorted alphabetically + * Unverified emails, sorted alphabetically + #} + {% set sorted_emails = user.emails|sort(attribute="email")|sort(attribute="verified", reverse=true)|sort(attribute="primary", reverse=true) %} + + + + + + + + + + + {% for email in sorted_emails %} + {{ email_row(email) }} + {% endfor %} + +
Email AddressStatus
+ +
+ +
+ +
+
+ +
+
+ +
+
+
+
+
+ +
+ +
+
+ {{ add_email_form.email(placeholder="Your email address", autocomplete="email", required="required", class_="form-group__input") }} + {% if add_email_form.email.errors %} +
    + {% for error in add_email_form.email.errors %} +
  • {{ error }}
    • + {% endfor %} +
+ {% endif %} +
+
+ +
+
+
+
{% endblock %}