From ae7cd45b29c91db94a654121ca448354353ee11f Mon Sep 17 00:00:00 2001 From: Benjamin Dauvergne Date: Tue, 18 Jun 2019 12:05:09 +0200 Subject: [PATCH 3/3] views: require authentication for deleting account without a verified email (#28853) --- src/authentic2/authenticators.py | 8 + src/authentic2/forms/profile.py | 15 - .../authentic2/accounts_delete_request.html | 4 +- src/authentic2/views.py | 80 +++++- src/authentic2_auth_fc/authenticators.py | 1 + src/authentic2_auth_oidc/authenticators.py | 1 + src/authentic2_auth_saml/authenticators.py | 1 + tests/conftest.py | 6 +- tests/test_views.py | 257 ++++++++++-------- 9 files changed, 233 insertions(+), 140 deletions(-) diff --git a/src/authentic2/authenticators.py b/src/authentic2/authenticators.py index d8c4739a..f2ba568e 100644 --- a/src/authentic2/authenticators.py +++ b/src/authentic2/authenticators.py @@ -36,6 +36,8 @@ logger = logging.getLogger(__name__) class BaseAuthenticator: + how = () + def __init__(self, show_condition=None, **kwargs): self.show_condition = show_condition @@ -60,6 +62,7 @@ class BaseAuthenticator: class LoginPasswordAuthenticator(BaseAuthenticator): id = 'password' + how = ['password', 'password-on-https'] submit_name = 'login-password-submit' priority = 0 @@ -119,6 +122,11 @@ class LoginPasswordAuthenticator(BaseAuthenticator): form = authentication_forms.AuthenticationForm( request=request, data=data, initial=initial, preferred_ous=preferred_ous ) + if request.user.is_authenticated and request.login_token.get('action'): + form.initial['username'] = request.user.username or request.user.email + form.fields['username'].widget.attrs.pop('autofocus', None) + form.fields['username'].widget.attrs['readonly'] = 'true' + form.fields['password'].widget.attrs['autofocus'] = 'autofocus' if app_settings.A2_ACCEPT_EMAIL_AUTHENTICATION: form.fields['username'].label = _('Username or email') if app_settings.A2_USERNAME_LABEL: diff --git a/src/authentic2/forms/profile.py b/src/authentic2/forms/profile.py index ea389ffc..86164193 100644 --- a/src/authentic2/forms/profile.py +++ b/src/authentic2/forms/profile.py @@ -17,7 +17,6 @@ from django import forms from django.forms.models import modelform_factory as dj_modelform_factory -from django.utils.translation import ugettext from django.utils.translation import ugettext_lazy as _ from authentic2 import app_settings, models @@ -28,20 +27,6 @@ from .mixins import LockedFieldFormMixin from .utils import NextUrlFormMixin -class DeleteAccountForm(forms.Form): - password = forms.CharField(widget=forms.PasswordInput, label=_("Password")) - - def __init__(self, *args, **kwargs): - self.user = kwargs.pop('user') - super().__init__(*args, **kwargs) - - def clean_password(self): - password = self.cleaned_data.get('password') - if password and not self.user.check_password(password): - raise forms.ValidationError(ugettext('Password is invalid')) - return password - - class EmailChangeFormNoPassword(forms.Form): email = ValidatedEmailField(label=_('New email')) diff --git a/src/authentic2/templates/authentic2/accounts_delete_request.html b/src/authentic2/templates/authentic2/accounts_delete_request.html index 0339e948..679d45ea 100644 --- a/src/authentic2/templates/authentic2/accounts_delete_request.html +++ b/src/authentic2/templates/authentic2/accounts_delete_request.html @@ -20,13 +20,15 @@ Do you really want to delete your account? {% endblocktrans %}

+ {% if user.email_verified %}

{% blocktrans trimmed %} A validation message will be sent to {{ email }}. You will have to visit the link in this email in order to complete the deletion process. {% endblocktrans %}

- + {% endif %} + {% endblock %} diff --git a/src/authentic2/views.py b/src/authentic2/views.py index cea6ec2b..149a4511 100644 --- a/src/authentic2/views.py +++ b/src/authentic2/views.py @@ -17,6 +17,7 @@ import collections import logging import re +import time from email.utils import parseaddr from django import shortcuts @@ -294,6 +295,15 @@ email_change_verify = EmailChangeVerifyView.as_view() def login(request, template_name='authentic2/login.html', redirect_field_name=REDIRECT_FIELD_NAME): """Displays the login form and handles the login action.""" + request.login_token = token = {} + if 'token' in request.GET: + try: + token.update(crypto.loads(request.GET['token'])) + logger.debug('login: got token %s', token) + except (crypto.SignatureExpired, crypto.BadSignature, ValueError): + logger.warning('login: bad token') + methods = token.get('methods', []) + # redirect user to homepage if already connected, if setting # A2_LOGIN_REDIRECT_AUTHENTICATED_USERS_TO_HOMEPAGE is True if request.user.is_authenticated and app_settings.A2_LOGIN_REDIRECT_AUTHENTICATED_USERS_TO_HOMEPAGE: @@ -330,6 +340,8 @@ def login(request, template_name='authentic2/login.html', redirect_field_name=RE # Create blocks for authenticator in authenticators: + if methods and not set(authenticator.how) & set(methods): + continue # Legacy API if not hasattr(authenticator, 'login'): fid = authenticator.id @@ -390,6 +402,8 @@ def login(request, template_name='authentic2/login.html', redirect_field_name=RE block = blocks[0] authenticator = block['authenticator'] if hasattr(authenticator, 'autorun'): + if 'message' in token: + messages.info(request, token['message']) return authenticator.autorun(request, block['id']) # Old frontends API @@ -416,6 +430,8 @@ def login(request, template_name='authentic2/login.html', redirect_field_name=RE redirect_field_name: redirect_to, } ) + if 'message' in token: + messages.info(request, token['message']) return render(request, template_name, context) @@ -1323,17 +1339,42 @@ registration_completion = RegistrationCompletionView.as_view() class AccountDeleteView(HomeURLMixin, TemplateView): template_name = 'authentic2/accounts_delete_request.html' title = _('Request account deletion') + last_authentication_max_age = 600 # 10 minutes def dispatch(self, request, *args, **kwargs): if not app_settings.A2_REGISTRATION_CAN_DELETE_ACCOUNT: return utils_misc.redirect(request, '..') + if not self.request.user.email_verified and not self.has_recent_authentication(): + methods = [event['how'] for event in utils_misc.get_authentication_events(request)] + return utils_misc.login_require( + request, + token={ + 'action': 'account-delete', + 'message': _('You must re-authenticate to delete your account.'), + 'methods': methods, + }, + ) return super().dispatch(request, *args, **kwargs) + def has_recent_authentication(self): + age = time.time() - utils_misc.last_authentication_event(request=self.request)['when'] + return age < self.last_authentication_max_age + def post(self, request, *args, **kwargs): if 'cancel' in request.POST: return utils_misc.redirect(request, 'account_management') - utils_misc.send_account_deletion_code(self.request, self.request.user) - messages.info(request, _("An account deletion validation email has been sent to your email address.")) + if self.request.user.email_verified: + utils_misc.send_account_deletion_code(self.request, self.request.user) + messages.info( + request, _("An account deletion validation email has been sent to your email address.") + ) + else: + deletion_url = utils_misc.build_deletion_url(request, prompt=False) + return logout( + request, + next_url=deletion_url, + check_referer=False, + ) return utils_misc.redirect(request, 'account_management') def get_context_data(self, **kwargs): @@ -1346,12 +1387,14 @@ class ValidateDeletionView(TemplateView): template_name = 'authentic2/accounts_delete_validation.html' title = _('Confirm account deletion') user = None + prompt = True def dispatch(self, request, *args, **kwargs): try: deletion_token = crypto.loads( kwargs['deletion_token'], max_age=app_settings.A2_DELETION_REQUEST_LIFETIME ) + self.prompt = deletion_token.get('prompt', self.prompt) user_pk = deletion_token['user_pk'] self.user = get_user_model().objects.get(pk=user_pk) # A user account wont be deactived twice @@ -1373,20 +1416,29 @@ class ValidateDeletionView(TemplateView): messages.error(request, error) return utils_misc.redirect(request, 'auth_homepage') + def get(self, request, *args, **kwargs): + if not self.prompt: + return self.delete_account(request) + return super().get(request, *args, **kwargs) + def post(self, request, *args, **kwargs): if 'cancel' not in request.POST: - utils_misc.send_account_deletion_mail(self.request, self.user) - logger.info('deletion of account %s performed', self.user) - hooks.call_hooks('event', name='delete-account', user=self.user) - request.journal.record('user.deletion', user=self.user) - is_deleted_user_logged = self.user == request.user - self.user.delete() - messages.info(request, _('Deletion performed.')) - # No real use for cancel_url or next_url here, assuming the link - # has been received by email. We instead redirect the user to the - # homepage. - if is_deleted_user_logged: - return logout(request, check_referer=False) + return self.delete_account(request) + return utils_misc.redirect(request, 'auth_homepage') + + def delete_account(self, request): + utils_misc.send_account_deletion_mail(self.request, self.user) + logger.info('deletion of account %s performed', self.user) + hooks.call_hooks('event', name='delete-account', user=self.user) + request.journal.record('user.deletion', user=self.user) + is_deleted_user_logged = self.user == request.user + self.user.delete() + messages.info(request, _('Deletion performed.')) + # No real use for cancel_url or next_url here, assuming the link + # has been received by email. We instead redirect the user to the + # homepage. + if is_deleted_user_logged: + return logout(request, check_referer=False) return utils_misc.redirect(request, 'auth_homepage') def get_context_data(self, **kwargs): diff --git a/src/authentic2_auth_fc/authenticators.py b/src/authentic2_auth_fc/authenticators.py index 40e0d920..4a159a4c 100644 --- a/src/authentic2_auth_fc/authenticators.py +++ b/src/authentic2_auth_fc/authenticators.py @@ -28,6 +28,7 @@ from . import app_settings class FcAuthenticator(BaseAuthenticator): id = 'fc' + how = ['france-connect'] priority = -1 def enabled(self): diff --git a/src/authentic2_auth_oidc/authenticators.py b/src/authentic2_auth_oidc/authenticators.py index 9296fd00..7ff40bfd 100644 --- a/src/authentic2_auth_oidc/authenticators.py +++ b/src/authentic2_auth_oidc/authenticators.py @@ -26,6 +26,7 @@ from .models import OIDCProvider class OIDCAuthenticator(BaseAuthenticator): id = 'oidc' + how = ['oidc'] priority = 2 def enabled(self): diff --git a/src/authentic2_auth_saml/authenticators.py b/src/authentic2_auth_saml/authenticators.py index cbe20be1..404b6729 100644 --- a/src/authentic2_auth_saml/authenticators.py +++ b/src/authentic2_auth_saml/authenticators.py @@ -28,6 +28,7 @@ from . import app_settings class SAMLAuthenticator(BaseAuthenticator): id = 'saml' priority = 3 + how = ['saml'] def enabled(self): return app_settings.enable and list(get_idps()) diff --git a/tests/conftest.py b/tests/conftest.py index 095890b8..50ce6341 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -126,7 +126,11 @@ def create_user(**kwargs): @pytest.fixture def simple_user(db, ou1): return create_user( - username='user', first_name='Jôhn', last_name='Dôe', email='user@example.net', ou=get_default_ou() + username='user', + first_name='Jôhn', + last_name='Dôe', + email='user@example.net', + ou=get_default_ou(), ) diff --git a/tests/test_views.py b/tests/test_views.py index 5c7f6fa4..8e2098b7 100644 --- a/tests/test_views.py +++ b/tests/test_views.py @@ -62,115 +62,154 @@ def test_well_known_password_change(app): assert resp.location == '/accounts/password/change/' -def test_account_delete(app, simple_user, mailoutbox): - assert simple_user.is_active - assert len(mailoutbox) == 0 - page = login(app, simple_user, path=reverse('delete_account')) - assert simple_user.email in page.text - page.form.submit(name='submit').follow() - assert len(mailoutbox) == 1 - link = get_link_from_mail(mailoutbox[0]) - assert mailoutbox[0].subject == 'Validate account deletion request on testserver' - assert [simple_user.email] == mailoutbox[0].to - page = app.get(link) - # FIXME: webtest does not set the Referer header, so the logout page will always ask for - # confirmation under tests - response = page.form.submit(name='delete') - assert '_auth_user_id' not in app.session - assert User.objects.filter(id=simple_user.id).count() == 0 - assert DeletedUser.objects.filter(old_user_id=simple_user.id).count() == 1 - assert len(mailoutbox) == 2 - assert mailoutbox[1].subject == 'Account deletion on testserver' - assert mailoutbox[0].to == [simple_user.email] - assert "Deletion performed" in str(response) # Set-Cookie: messages.. - assert urlparse(response.location).path == '/' - - -def test_account_delete_when_logged_out(app, simple_user, mailoutbox): - assert simple_user.is_active - assert len(mailoutbox) == 0 - page = login(app, simple_user, path=reverse('delete_account')) - page.form.submit(name='submit').follow() - assert len(mailoutbox) == 1 - link = get_link_from_mail(mailoutbox[0]) - logout(app) - page = app.get(link) - assert ( - 'You are about to delete the account of %s.' % escape(simple_user.get_full_name()) - in page.text - ) - response = page.form.submit(name='delete') - assert User.objects.filter(id=simple_user.id).count() == 0 - assert DeletedUser.objects.filter(old_user_id=simple_user.id).count() == 1 - assert len(mailoutbox) == 2 - assert mailoutbox[1].subject == 'Account deletion on testserver' - assert mailoutbox[0].to == [simple_user.email] - assert "Deletion performed" in str(response) # Set-Cookie: messages.. - assert urlparse(response.location).path == '/' - - -def test_account_delete_by_other_user(app, simple_user, user_ou1, mailoutbox): - assert simple_user.is_active - assert user_ou1.is_active - assert len(mailoutbox) == 0 - page = login(app, simple_user, path=reverse('delete_account')) - page.form.submit(name='submit').follow() - assert len(mailoutbox) == 1 - link = get_link_from_mail(mailoutbox[0]) - logout(app) - login(app, user_ou1, path=reverse('account_management')) - page = app.get(link) - assert ( - 'You are about to delete the account of %s.' % escape(simple_user.get_full_name()) - in page.text - ) - response = page.form.submit(name='delete') - assert app.session['_auth_user_id'] == str(user_ou1.id) - assert User.objects.filter(id=simple_user.id).count() == 0 - assert DeletedUser.objects.filter(old_user_id=simple_user.id).count() == 1 - assert len(mailoutbox) == 2 - assert mailoutbox[1].subject == 'Account deletion on testserver' - assert mailoutbox[0].to == [simple_user.email] - assert "Deletion performed" in str(response) # Set-Cookie: messages.. - assert urlparse(response.location).path == '/' - - -def test_account_delete_fake_token(app, simple_user, mailoutbox): - response = ( - app.get(reverse('validate_deletion', kwargs={'deletion_token': 'thisismostlikelynotavalidtoken'})) - .follow() - .follow() - ) - assert "The account deletion request is invalid, try again" in response.text - - -def test_account_delete_expired_token(app, simple_user, mailoutbox, freezer): - freezer.move_to('2019-08-01') - page = login(app, simple_user, path=reverse('delete_account')) - page.form.submit(name='submit').follow() - freezer.move_to('2019-08-04') # Too late... - link = get_link_from_mail(mailoutbox[0]) - response = app.get(link).follow() - assert "The account deletion request is too old, try again" in response.text - - -def test_account_delete_valid_token_unexistent_user(app, simple_user, mailoutbox): - page = login(app, simple_user, path=reverse('delete_account')) - page.form.submit(name='submit').follow() - link = get_link_from_mail(mailoutbox[0]) - simple_user.delete() - response = app.get(link).follow().follow() - assert 'This account has previously been deleted.' in response.text - - -def test_account_delete_valid_token_inactive_user(app, simple_user, mailoutbox): - page = login(app, simple_user, path=reverse('delete_account')) - page.form.submit(name='submit').follow() - link = get_link_from_mail(mailoutbox[0]) - simple_user.is_active = False - simple_user.save() - response = app.get(link).maybe_follow() - assert 'This account is inactive, it cannot be deleted.' in response.text +class TestDeleteAccountEmailVerified: + @pytest.fixture + def simple_user(self, simple_user): + simple_user.email_verified = True + simple_user.save() + return simple_user + + def test_account_delete(self, app, simple_user, mailoutbox): + assert simple_user.is_active + assert len(mailoutbox) == 0 + page = login(app, simple_user, path=reverse('delete_account')) + assert simple_user.email in page.text + page.form.submit(name='submit').follow() + assert len(mailoutbox) == 1 + link = get_link_from_mail(mailoutbox[0]) + assert mailoutbox[0].subject == 'Validate account deletion request on testserver' + assert [simple_user.email] == mailoutbox[0].to + page = app.get(link) + # FIXME: webtest does not set the Referer header, so the logout page will always ask for + # confirmation under tests + response = page.form.submit(name='delete') + assert '_auth_user_id' not in app.session + assert User.objects.filter(id=simple_user.id).count() == 0 + assert DeletedUser.objects.filter(old_user_id=simple_user.id).count() == 1 + assert len(mailoutbox) == 2 + assert mailoutbox[1].subject == 'Account deletion on testserver' + assert mailoutbox[0].to == [simple_user.email] + assert "Deletion performed" in str(response) # Set-Cookie: messages.. + assert urlparse(response.location).path == '/' + + def test_account_delete_when_logged_out(self, app, simple_user, mailoutbox): + assert simple_user.is_active + assert len(mailoutbox) == 0 + page = login(app, simple_user, path=reverse('delete_account')) + page.form.submit(name='submit').follow() + assert len(mailoutbox) == 1 + link = get_link_from_mail(mailoutbox[0]) + logout(app) + page = app.get(link) + assert ( + 'You are about to delete the account of %s.' + % escape(simple_user.get_full_name()) + in page.text + ) + response = page.form.submit(name='delete') + assert User.objects.filter(id=simple_user.id).count() == 0 + assert DeletedUser.objects.filter(old_user_id=simple_user.id).count() == 1 + assert len(mailoutbox) == 2 + assert mailoutbox[1].subject == 'Account deletion on testserver' + assert mailoutbox[0].to == [simple_user.email] + assert "Deletion performed" in str(response) # Set-Cookie: messages.. + assert urlparse(response.location).path == '/' + + def test_account_delete_by_other_user(self, app, simple_user, user_ou1, mailoutbox): + assert simple_user.is_active + assert user_ou1.is_active + assert len(mailoutbox) == 0 + page = login(app, simple_user, path=reverse('delete_account')) + page.form.submit(name='submit').follow() + assert len(mailoutbox) == 1 + link = get_link_from_mail(mailoutbox[0]) + logout(app) + login(app, user_ou1, path=reverse('account_management')) + page = app.get(link) + assert ( + 'You are about to delete the account of %s.' + % escape(simple_user.get_full_name()) + in page.text + ) + response = page.form.submit(name='delete') + assert app.session['_auth_user_id'] == str(user_ou1.id) + assert User.objects.filter(id=simple_user.id).count() == 0 + assert DeletedUser.objects.filter(old_user_id=simple_user.id).count() == 1 + assert len(mailoutbox) == 2 + assert mailoutbox[1].subject == 'Account deletion on testserver' + assert mailoutbox[0].to == [simple_user.email] + assert "Deletion performed" in str(response) # Set-Cookie: messages.. + assert urlparse(response.location).path == '/' + + def test_account_delete_fake_token(self, app, simple_user, mailoutbox): + response = ( + app.get(reverse('validate_deletion', kwargs={'deletion_token': 'thisismostlikelynotavalidtoken'})) + .follow() + .follow() + ) + assert "The account deletion request is invalid, try again" in response.text + + def test_account_delete_expired_token(self, app, simple_user, mailoutbox, freezer): + freezer.move_to('2019-08-01') + page = login(app, simple_user, path=reverse('delete_account')) + page.form.submit(name='submit').follow() + freezer.move_to('2019-08-04') # Too late... + link = get_link_from_mail(mailoutbox[0]) + response = app.get(link).follow() + assert "The account deletion request is too old, try again" in response.text + + def test_account_delete_valid_token_unexistent_user(self, app, simple_user, mailoutbox): + page = login(app, simple_user, path=reverse('delete_account')) + page.form.submit(name='submit').follow() + link = get_link_from_mail(mailoutbox[0]) + simple_user.delete() + response = app.get(link).follow().follow() + assert 'This account has previously been deleted.' in response.text + + def test_account_delete_valid_token_inactive_user(self, app, simple_user, mailoutbox): + page = login(app, simple_user, path=reverse('delete_account')) + page.form.submit(name='submit').follow() + link = get_link_from_mail(mailoutbox[0]) + simple_user.is_active = False + simple_user.save() + response = app.get(link).maybe_follow() + assert 'This account is inactive, it cannot be deleted.' in response.text + + +class TestDeleteAccountEmailNotVerified: + def test_account_delete(self, app, simple_user, mailoutbox): + assert simple_user.is_active + assert len(mailoutbox) == 0 + page = login(app, simple_user, path=reverse('delete_account')) + response = page.form.submit(name='submit').follow() + assert '_auth_user_id' not in app.session + assert User.objects.filter(id=simple_user.id).count() == 0 + assert DeletedUser.objects.filter(old_user_id=simple_user.id).count() == 1 + assert len(mailoutbox) == 1 + assert mailoutbox[0].subject == 'Account deletion on testserver' + assert mailoutbox[0].to == [simple_user.email] + assert "Deletion performed" in str(response) # Set-Cookie: messages.. + assert urlparse(response.location).path == '/' + + def test_account_delete_old_authentication(self, app, simple_user, mailoutbox, freezer): + assert simple_user.is_active + assert len(mailoutbox) == 0 + login(app, simple_user) + freezer.move_to(datetime.timedelta(hours=1)) + redirect = app.get('/accounts/delete/') + login_page = redirect.follow() + assert 'You must re-authenticate' in login_page + login_page.form.set('password', simple_user.username) + page = login_page.form.submit(name='login-password-submit').follow() + response = page.form.submit(name='submit').follow() + assert '_auth_user_id' not in app.session + assert User.objects.filter(id=simple_user.id).count() == 0 + assert DeletedUser.objects.filter(old_user_id=simple_user.id).count() == 1 + assert len(mailoutbox) == 1 + assert mailoutbox[0].subject == 'Account deletion on testserver' + assert mailoutbox[0].to == [simple_user.email] + assert "Deletion performed" in str(response) # Set-Cookie: messages.. + assert urlparse(response.location).path == '/' def test_login_invalid_next(app): -- 2.34.1