misc: review next_url management on profile, password reset and registration views (#76835)

All forms used on a view handling a next_url must inherit from
NextUrlFormMixin and the view from NextUrlViewMixin or
SuccessUrlViewMixin (the latter if the view will redirect itself to this
url).

NextUrlViewMixin automatically create a form class inheriting from
NextUrlFormMixin if needed. The form class only need to declare
NextUrlFormMixin when it has a direct use of the next_url field, for
example PasswordResetForm needs it to make the reset link.

The new behaviour is more Django-esque as success_url is used as the
next_url of last resort as implemented in Django generic views.

The password change done views was removed as it's not the flow we
wanted, after password change people are by default returned to the
account_management view.

Next url support is removed from the password change view, there is no
need for it, we should always return to the account management page.
Dead code was also removed (get_context_data()).

src/authentic2/cbv.py

@@ -14,12 +14,13 @@
 # You should have received a copy of the GNU Affero General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-from django.conf import settings
-from django.contrib.auth import REDIRECT_FIELD_NAME
+import functools
+
 from django.forms import Form
 from django.utils.decorators import method_decorator
 from django.views.decorators.csrf import csrf_exempt, ensure_csrf_cookie
 
+from .forms.utils import NextUrlFormMixin
 from .utils import hooks
 from .utils import misc as utils_misc
 from .utils.views import csrf_token_check
@@ -47,39 +48,35 @@ class ValidateCSRFMixin:
         return super().form_valid(*args, **kwargs)
 
 
-class RedirectToNextURLViewMixin:
+@functools.cache
+def make_next_url_form_class(form_class):
+    if issubclass(form_class, NextUrlFormMixin):
+        return form_class
+    return type(f'NextURL{form_class.__name__}', (form_class, NextUrlFormMixin), {})
+
+
+class NextUrlViewMixin:
+    '''Mixin to FormView of forms using NextUrlFormMixin'''
+
+    next_url = None
+
+    def setup(self, request, *args, **kwargs):
+        super().setup(request, *args, **kwargs)
+        self.next_url = utils_misc.select_next_url(request, self.next_url, include_post=True)
+
+    def get_form_class(self):
+        return make_next_url_form_class(super().get_form_class())
+
+    def get_form_kwargs(self):
+        kwargs = super().get_form_kwargs()
+        kwargs.setdefault('initial', {})['next_url'] = self.next_url
+        kwargs['request'] = self.request
+        return kwargs
+
+
+class SuccessUrlViewMixin(NextUrlViewMixin):
     def get_success_url(self):
-        if REDIRECT_FIELD_NAME in self.request.GET:
-            return self.request.GET[REDIRECT_FIELD_NAME]
-        return settings.LOGIN_REDIRECT_URL
-
-
-class NextURLViewMixin(RedirectToNextURLViewMixin):
-    """Make a view handle a next parameter, if it's not present it is
-    automatically generated from the Referrer or from the value
-    returned by the method get_next_url_default().
-    """
-
-    next_url_default = '..'
-
-    def get_next_url_default(self):
-        return self.next_url_default
-
-    def dispatch(self, request, *args, **kwargs):
-        if REDIRECT_FIELD_NAME in request.GET:
-            pass
-        else:
-            next_url = request.headers.get('Referer') or self.next_url_default
-            return utils_misc.redirect(
-                request,
-                request.path,
-                keep_params=True,
-                params={
-                    REDIRECT_FIELD_NAME: next_url,
-                },
-                status=303,
-            )
-        return super().dispatch(request, *args, **kwargs)
+        return self.next_url or super().get_success_url()
 
 
 class TemplateNamesMixin:

src/authentic2/forms/passwords.py

@@ -42,9 +42,7 @@ from .utils import NextUrlFormMixin
 logger = logging.getLogger(__name__)
 
 
-class PasswordResetForm(HoneypotForm):
-    next_url = forms.CharField(widget=forms.HiddenInput, required=False)
-
+class PasswordResetForm(NextUrlFormMixin, HoneypotForm):
     email = ValidatedEmailField(label=_('Email'), required=False)
 
     phone = PhoneField(
@@ -273,9 +271,7 @@ class SetPasswordForm(NotifyOfPasswordChange, PasswordResetMixin, auth_forms.Set
         return new_password1
 
 
-class PasswordChangeForm(
-    NotifyOfPasswordChange, NextUrlFormMixin, PasswordResetMixin, auth_forms.PasswordChangeForm
-):
+class PasswordChangeForm(NotifyOfPasswordChange, PasswordResetMixin, auth_forms.PasswordChangeForm):
     old_password = PasswordField(label=_('Old password'))
     new_password1 = NewPasswordField(label=_('New password'))
     new_password2 = CheckPasswordField(label=_('New password confirmation'))

src/authentic2/forms/utils.py

@@ -15,19 +15,23 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 from django import forms
-from django.contrib.auth import REDIRECT_FIELD_NAME
 
-from ..middleware import StoreRequestMiddleware
+from authentic2.utils.misc import good_next_url
 
 
-class NextUrlFormMixin(forms.Form):
+class FormNeedsRequest:
+    def __init__(self, *args, **kwargs):
+        self.request = kwargs.pop('request')
+        super().__init__(*args, **kwargs)
+
+
+class NextUrlFormMixin(FormNeedsRequest, forms.Form):
+    '''Use with authentic.cbv.NextUrlViewMixin.'''
+
     next_url = forms.CharField(widget=forms.HiddenInput(), required=False)
 
-    def __init__(self, *args, **kwargs):
-        next_url = kwargs.pop('next_url', None)
-        request = StoreRequestMiddleware.get_request()
-        if not next_url and request:
-            next_url = request.GET.get(REDIRECT_FIELD_NAME)
-        super().__init__(*args, **kwargs)
-        if next_url:
-            self.fields['next_url'].initial = next_url
+    def clean_next_url(self):
+        next_url = self.cleaned_data.get('next_url')
+        if not good_next_url(self.request, next_url):
+            return ''
+        return next_url

src/authentic2/templates/registration/password_change_done.html

@@ -1,10 +0,0 @@
-{% extends "authentic2/base-page.html" %}
-{% load i18n %}
-
-{% block title %}
-  {{ view.title }}
-{% endblock %}
-
-{% block content %}
-  <p>{% trans "Password changed" %}</p>
-{% endblock %}

src/authentic2/urls.py

@@ -16,7 +16,6 @@
 
 from django.conf import settings
 from django.contrib import admin
-from django.contrib.auth import views as dj_auth_views
 from django.contrib.auth.decorators import login_required
 from django.contrib.staticfiles.views import serve
 from django.urls import include, path, re_path
@@ -75,11 +74,6 @@ accounts_urlpatterns = [
     path('', views.profile, name='account_management'),
     # Password change
     path('password/change/', views.password_change, name='password_change'),
-    path(
-        'password/change/done/',
-        dj_auth_views.PasswordChangeDoneView.as_view(),
-        name='password_change_done',
-    ),
     # permament redirections for views moved to root
     path('register/', RedirectView.as_view(permanent=True, pattern_name='registration_register')),
     path('register/complete/', RedirectView.as_view(permanent=True, pattern_name='registration_complete')),

src/authentic2/views.py

@@ -87,10 +87,11 @@ class HomeURLMixin:
         return super().dispatch(request, *args, **kwargs)
 
 
-class EditProfile(HomeURLMixin, cbv.HookMixin, cbv.TemplateNamesMixin, UpdateView):
+class EditProfile(HomeURLMixin, cbv.SuccessUrlViewMixin, cbv.HookMixin, cbv.TemplateNamesMixin, UpdateView):
     model = User
     template_names = ['profiles/edit_profile.html', 'authentic2/accounts_edit.html']
     title = _('Edit account data')
+    success_url = reverse_lazy('account_management')
 
     def get_template_names(self):
         template_names = []
@@ -133,7 +134,8 @@ class EditProfile(HomeURLMixin, cbv.HookMixin, cbv.TemplateNamesMixin, UpdateVie
             fields = [field for field in fields if field in default_fields]
         return fields, labels
 
-    def get_form_class(self):
+    @property
+    def form_class(self):
         if 'scope' in self.kwargs:
             scopes = [self.kwargs['scope']]
         else:
@@ -152,19 +154,9 @@ class EditProfile(HomeURLMixin, cbv.HookMixin, cbv.TemplateNamesMixin, UpdateVie
     def get_object(self):
         return self.request.user
 
-    def get_form_kwargs(self, **kwargs):
-        kwargs = super().get_form_kwargs(**kwargs)
-        kwargs['next_url'] = utils_misc.select_next_url(self.request, reverse('account_management'))
-        return kwargs
-
-    def get_success_url(self):
-        return utils_misc.select_next_url(
-            self.request, default=reverse('account_management'), field_name='next_url', include_post=True
-        )
-
     def post(self, request, *args, **kwargs):
         if 'cancel' in request.POST:
-            return utils_misc.redirect(request, self.get_success_url())
+            return utils_misc.redirect(request, self.next_url or str(self.success_url))
         return super().post(request, *args, **kwargs)
 
     def form_valid(self, form):
@@ -1105,7 +1097,7 @@ def csrf_failure_view(request, reason=''):
     return HttpResponseRedirect(request.get_full_path())
 
 
-class PasswordResetView(FormView):
+class PasswordResetView(cbv.NextUrlViewMixin, FormView):
     '''Ask for an email and send a password reset link by mail'''
 
     form_class = passwords_forms.PasswordResetForm
@@ -1136,9 +1128,6 @@ class PasswordResetView(FormView):
     def get_form_kwargs(self, **kwargs):
         kwargs = super().get_form_kwargs(**kwargs)
         kwargs['password_authenticator'] = self.authenticator
-        initial = kwargs.setdefault('initial', {})
-        if next_url := utils_misc.select_next_url(self.request, ''):
-            initial['next_url'] = next_url
         return kwargs
 
     def get_context_data(self, **kwargs):
@@ -1325,11 +1314,12 @@ class TokenLoginView(RedirectView):
 token_login = TokenLoginView.as_view()
 
 
-class PasswordResetConfirmView(cbv.RedirectToNextURLViewMixin, FormView):
+class PasswordResetConfirmView(cbv.SuccessUrlViewMixin, FormView):
     """Validate password reset link, show a set password form and login
     the user.
     """
 
+    success_url = reverse_lazy('auth_homepage')
     form_class = passwords_forms.SetPasswordForm
     title = _('Password Reset')
 
@@ -1400,20 +1390,15 @@ class PasswordResetConfirmView(cbv.RedirectToNextURLViewMixin, FormView):
         hooks.call_hooks('event', name='password-reset-confirm', user=form.user, token=self.token, form=form)
         logger.info('password reset for user %s with token %r', self.user, self.token.uuid)
         self.token.delete()
-        return self.finish()
-
-    def finish(self):
-        response = utils_misc.simulate_authentication(
-            self.request, self.user, 'email', next_url=self.next_url
-        )
+        utils_misc.simulate_authentication(self.request, self.user, 'email')
         self.request.journal.record('user.password.reset')
-        return response
+        return super().form_valid(form)
 
 
 password_reset_confirm = PasswordResetConfirmView.as_view()
 
 
-class BaseRegistrationView(HomeURLMixin, FormView):
+class BaseRegistrationView(HomeURLMixin, cbv.NextUrlViewMixin, FormView):
     form_class = registration_forms.RegistrationForm
     template_name = 'registration/registration_form.html'
     title = _('Registration')
@@ -1445,12 +1430,7 @@ class BaseRegistrationView(HomeURLMixin, FormView):
                 return HttpResponseBadRequest('invalid token', content_type='text/plain')
             if 'ou' in self.token:
                 self.ou = OU.objects.get(pk=self.token['ou'])
-            if self.token.get(REDIRECT_FIELD_NAME):
-                self.next_url = self.token.pop(REDIRECT_FIELD_NAME)
-        elif (next_url := request.GET.get(REDIRECT_FIELD_NAME)) and utils_misc.good_next_url(
-            request, next_url
-        ):
-            self.next_url = next_url
+        self.next_url = self.token.pop(REDIRECT_FIELD_NAME, self.next_url)
         set_home_url(request, self.next_url)
         return super().dispatch(request, *args, **kwargs)
 
@@ -1597,7 +1577,8 @@ class BaseRegistrationView(HomeURLMixin, FormView):
             return self.form_invalid(form)
 
         for field in form.cleaned_data:
-            self.token[field] = form.cleaned_data[field]
+            if field in app_settings.A2_PRE_REGISTRATION_FIELDS:
+                self.token[field] = form.cleaned_data[field]
 
         self.token.pop(REDIRECT_FIELD_NAME, None)
         self.token.pop('email', None)
@@ -1621,7 +1602,7 @@ class BaseRegistrationView(HomeURLMixin, FormView):
         return context
 
 
-class InputSMSCodeView(cbv.ValidateCSRFMixin, FormView):
+class InputSMSCodeView(cbv.ValidateCSRFMixin, cbv.NextUrlViewMixin, FormView):
     template_name = 'registration/sms_input_code.html'
     form_class = registration_forms.InputSMSCodeForm
     success_url = '/accounts/'
@@ -2272,23 +2253,21 @@ registration_complete = RegistrationCompleteView.as_view()
 class PasswordChangeView(HomeURLMixin, DjPasswordChangeView):
     title = _('Password Change')
     do_not_call_in_templates = True
+    success_url = reverse_lazy('account_management')
+
+    def redirect(self):
+        return HttpResponseRedirect(self.get_success_url())
 
     def dispatch(self, request, *args, **kwargs):
-        if 'next_url' in request.POST and request.POST['next_url']:
-            self.post_change_redirect = request.POST['next_url']
-        elif REDIRECT_FIELD_NAME in request.GET:
-            self.post_change_redirect = request.GET[REDIRECT_FIELD_NAME]
-        else:
-            self.post_change_redirect = reverse('account_management')
         if not utils_misc.user_can_change_password(request=request):
             messages.warning(request, _('Password change is forbidden'))
-            return utils_misc.redirect(request, self.post_change_redirect)
+            return self.redirect()
         hooks.call_hooks('password_change_view', request=self.request)
         return super().dispatch(request, *args, **kwargs)
 
     def post(self, request, *args, **kwargs):
         if 'cancel' in request.POST:
-            return utils_misc.redirect(request, self.post_change_redirect)
+            return self.redirect()
         return super().post(request, *args, **kwargs)
 
     def form_valid(self, form):
@@ -2303,17 +2282,13 @@ class PasswordChangeView(HomeURLMixin, DjPasswordChangeView):
         self.request.journal.record('user.password.change', session=self.request.session)
         return response
 
-    def get_form_class(self):
+    @property
+    def form_class(self):
         if self.request.user.has_usable_password():
             return passwords_forms.PasswordChangeForm
         else:
             return passwords_forms.SetPasswordForm
 
-    def get_context_data(self, **kwargs):
-        ctx = super().get_context_data(**kwargs)
-        ctx[REDIRECT_FIELD_NAME] = self.post_change_redirect
-        return ctx
-
 
 password_change = decorators.setting_enabled('A2_REGISTRATION_CAN_CHANGE_PASSWORD')(
     PasswordChangeView.as_view()

tests/test_all.py

@@ -235,7 +235,7 @@ class UserProfileTests(TestCase):
             kwargs = {'%s-%s' % (form.prefix, k): v for k, v in kwargs.items()}
 
         response = self.client.post(reverse('profile_edit'), kwargs)
-        new = {'custom': 'random data', 'next_url': '', 'national_number': 'xx20153566342yy'}
+        new = {'custom': 'random data', 'national_number': 'xx20153566342yy'}
         assert_event('user.profile.edit', user=self.user, session=self.client.session, old={}, new=new)
 
         self.assertEqual(response.status_code, 302)

tests/test_password_reset.py

@@ -510,12 +510,4 @@ def test_open_redirection(db, rf, app):
     password_reset.setup(request)
     form = password_reset.get_form()
     assert form.is_valid()
-    assert form.cleaned_data['next_url'] == BAD_URL
-
-    # not a problem, because... the form is protected by a CSRF token, it's
-    # impossible to initialize the form from elsewhere, next_url will revert to
-    # ''
-    response = app.post('/password/reset/', {'next_url': BAD_URL, 'email': 'john.doe@example.com'})
-    response = response.follow()
-    assert response.pyquery('.messages').text() == 'The page is out of date, it was reloaded for you'
-    assert response.form['next_url'].value == ''
+    assert form.cleaned_data['next_url'] == ''

tests/test_registration.py

@@ -33,6 +33,7 @@ from authentic2.forms.registration import RegistrationCompletionForm
 from authentic2.models import Attribute, AttributeValue, SMSCode, Token
 from authentic2.utils import misc as utils_misc
 from authentic2.validators import EmailValidator
+from authentic2.views import RegistrationView
 
 from .utils import assert_event, get_link_from_mail, login
 
@@ -1407,3 +1408,20 @@ def test_already_logged(db, app, simple_user):
     # then we can register.
     resp = resp.follow()
     assert resp.form['email']
+
+
+def test_open_redirection(app, rf, db):
+    BAD_URL = 'https://bad.url.com/'
+
+    request = rf.get(f'/register/?next={BAD_URL}')
+
+    register = RegistrationView()
+    register.setup(request)
+    assert register.get_form_kwargs()['initial'].get('next_url') != BAD_URL
+
+    request = rf.post('/register/', {'next_url': BAD_URL, 'email': 'john.doe@example.com'})
+    register = RegistrationView()
+    register.setup(request)
+    form = register.get_form()
+    assert form.is_valid()
+    assert form.cleaned_data['next_url'] == ''

tests/test_views.py

@@ -85,7 +85,7 @@ def test_password_change(app, simple_user):
     resp.form['new_password2'] = 'hopAbcde1'
     resp = resp.form.submit()
 
-    assert resp.location == '/accounts/password/change/done/'
+    assert resp.location == '/accounts/'
 
     new_session_key = app.session.session_key
     assert old_session_key != new_session_key, 'session\'s key has not been cycled'