From 0bceb6e19ff97e9318f6bd8eab363f9eb0dc5c25 Mon Sep 17 00:00:00 2001 From: Mikki Weesenaar Date: Fri, 13 May 2022 13:26:29 +0200 Subject: [PATCH 1/4] Enforcing a redirect to setup of otp device when none available for user --- tests/test_views_login.py | 14 ++--- tests/test_views_mixins.py | 53 ++++++++++++++++++- .../two_factor/core/setup_complete.html | 4 ++ two_factor/views/core.py | 10 +++- 4 files changed, 72 insertions(+), 9 deletions(-) diff --git a/tests/test_views_login.py b/tests/test_views_login.py index 92ec5c3c6..5e7f214ed 100644 --- a/tests/test_views_login.py +++ b/tests/test_views_login.py @@ -38,7 +38,7 @@ def test_valid_login(self, mock_signal): response = self._post({'auth-username': 'bouke@example.com', 'auth-password': 'secret', 'login_view-current_step': 'auth'}) - self.assertRedirects(response, resolve_url(settings.LOGIN_REDIRECT_URL)) + self.assertRedirects(response, reverse('two_factor:setup')) # No signal should be fired for non-verified user logins. self.assertFalse(mock_signal.called) @@ -80,7 +80,8 @@ def test_valid_login_with_allowed_external_redirect(self): {'auth-username': 'bouke@example.com', 'auth-password': 'secret', 'login_view-current_step': 'auth'}) - self.assertRedirects(response, redirect_url, fetch_redirect_response=False) + self.assertEqual(self.client.session.get('next'), redirect_url) + self.assertRedirects(response, reverse('two_factor:setup'), fetch_redirect_response=False) def test_valid_login_with_disallowed_external_redirect(self): redirect_url = 'https://test.disallowed-success-url.com' @@ -90,7 +91,7 @@ def test_valid_login_with_disallowed_external_redirect(self): {'auth-username': 'bouke@example.com', 'auth-password': 'secret', 'login_view-current_step': 'auth'}) - self.assertRedirects(response, reverse('two_factor:profile'), fetch_redirect_response=False) + self.assertRedirects(response, reverse('two_factor:setup'), fetch_redirect_response=False) @mock.patch('two_factor.views.core.time') def test_valid_login_primary_key_stored(self, mock_time): @@ -395,12 +396,12 @@ def test_login_different_user_on_existing_session(self, mock_logger): response = self._post({'auth-username': 'bouke@example.com', 'auth-password': 'secret', 'login_view-current_step': 'auth'}) - self.assertRedirects(response, resolve_url(settings.LOGIN_REDIRECT_URL)) + self.assertRedirects(response, reverse('two_factor:setup')) response = self._post({'auth-username': 'vedran@example.com', 'auth-password': 'secret', 'login_view-current_step': 'auth'}) - self.assertRedirects(response, resolve_url(settings.LOGIN_REDIRECT_URL)) + self.assertRedirects(response, reverse('two_factor:setup')) def test_missing_management_data(self): # missing management data @@ -431,8 +432,7 @@ def test_login_different_user_with_otp_on_existing_session(self): response = self._post({'auth-username': 'bouke@example.com', 'auth-password': 'secret', 'login_view-current_step': 'auth'}) - self.assertRedirects(response, - resolve_url(settings.LOGIN_REDIRECT_URL)) + self.assertRedirects(response, reverse('two_factor:setup')) response = self._post({'auth-username': 'vedran@example.com', 'auth-password': 'secret', diff --git a/tests/test_views_mixins.py b/tests/test_views_mixins.py index a5fae597b..a0f26e099 100644 --- a/tests/test_views_mixins.py +++ b/tests/test_views_mixins.py @@ -1,8 +1,12 @@ +from binascii import unhexlify + from django.conf import settings from django.shortcuts import resolve_url from django.test import TestCase +from django.urls import reverse +from django_otp.oath import totp -from .utils import UserMixin +from .utils import UserMixin, method_registry class OTPRequiredMixinTest(UserMixin, TestCase): @@ -53,3 +57,50 @@ def test_verified(self): self.login_user() response = self.client.get('/secure/') self.assertEqual(response.status_code, 200) + + @method_registry(['generator']) + def test_valid_login_with_redirect_field_name_without_device(self): + self.create_user() + protected_url = '/secure/' + + # Open URL that is protected + # assert go to login page + # assert the next param not in the session (but still in url) + response = self.client.get(protected_url) + redirect_url = "%s?%s%s" % (reverse('two_factor:login'), 'next=', protected_url) + self.assertRedirects(response, redirect_url) + self.assertEqual(self.client.session.get('next'), None) + + # Log in given the last redirect + # assert redirect to setup + response = self.client.post( + redirect_url, + {'auth-username': 'bouke@example.com', + 'auth-password': 'secret', + 'login_view-current_step': 'auth'}) + self.assertRedirects(response, reverse('two_factor:setup')) + self.assertEqual(self.client.session.get('next'), protected_url) + + # Setup the device accordingly + # assert redirect to setup completed + # assert button for redirection to the original page + response = self.client.post( + reverse('two_factor:setup'), + data={'setup_view-current_step': 'welcome'}) + self.assertContains(response, 'Token:') + self.assertContains(response, 'autofocus="autofocus"') + self.assertContains(response, 'inputmode="numeric"') + self.assertContains(response, 'autocomplete="one-time-code"') + + key = response.context_data['keys'].get('generator') + bin_key = unhexlify(key.encode()) + response = self.client.post( + reverse('two_factor:setup'), + data={'setup_view-current_step': 'generator', + 'generator-token': totp(bin_key)}, + follow=True, + ) + + self.assertRedirects(response, reverse('two_factor:setup_complete')) + self.assertContains(response, 'Continue where I left off') + self.assertContains(response, '{% trans "Add Phone Number" %}

{% endif %} + {% if next %} + {% trans "Continue where I left off" %} + {% endif %} {% endblock %} diff --git a/two_factor/views/core.py b/two_factor/views/core.py index 5e1e9b0b5..da6f7ae31 100644 --- a/two_factor/views/core.py +++ b/two_factor/views/core.py @@ -182,7 +182,13 @@ def done(self, form_list, **kwargs): samesite=getattr(settings, 'TWO_FACTOR_REMEMBER_COOKIE_SAMESITE', 'Lax'), ) - return response + return response + + # If the user does not have a device. + else: + if self.request.GET.get('next'): + self.request.session['next'] = redirect_to + return redirect('two_factor:setup') # Copied from django.conrib.auth.views.LoginView (Branch: stable/1.11.x) # https://github.com/django/django/blob/58df8aa40fe88f753ba79e091a52f236246260b3/django/contrib/auth/views.py#L63 @@ -608,8 +614,10 @@ class SetupCompleteView(TemplateView): template_name = 'two_factor/core/setup_complete.html' def get_context_data(self): + redirect_url = self.request.session.pop('next', None) return { 'phone_methods': get_available_phone_methods(), + 'next': redirect_url } From 48d1ae0f29a93a22c3afd689ea5a0a02030b66dd Mon Sep 17 00:00:00 2001 From: Mikki Weesenaar Date: Mon, 30 May 2022 22:54:47 +0200 Subject: [PATCH 2/4] Rework after feedback. --- tests/test_views_mixins.py | 4 +-- .../two_factor/core/setup_complete.html | 4 --- two_factor/views/core.py | 36 +++++++++++++++---- 3 files changed, 31 insertions(+), 13 deletions(-) diff --git a/tests/test_views_mixins.py b/tests/test_views_mixins.py index a0f26e099..dfbfe9595 100644 --- a/tests/test_views_mixins.py +++ b/tests/test_views_mixins.py @@ -101,6 +101,4 @@ def test_valid_login_with_redirect_field_name_without_device(self): follow=True, ) - self.assertRedirects(response, reverse('two_factor:setup_complete')) - self.assertContains(response, 'Continue where I left off') - self.assertContains(response, '{% trans "Add Phone Number" %}

{% endif %} - {% if next %} - {% trans "Continue where I left off" %} - {% endif %} {% endblock %} diff --git a/two_factor/views/core.py b/two_factor/views/core.py index da6f7ae31..e46cca90a 100644 --- a/two_factor/views/core.py +++ b/two_factor/views/core.py @@ -187,7 +187,7 @@ def done(self, form_list, **kwargs): # If the user does not have a device. else: if self.request.GET.get('next'): - self.request.session['next'] = redirect_to + self.request.session['next'] = self.get_success_url() return redirect('two_factor:setup') # Copied from django.conrib.auth.views.LoginView (Branch: stable/1.11.x) @@ -400,7 +400,7 @@ def dispatch(self, request, *args, **kwargs): @class_view_decorator(never_cache) @class_view_decorator(login_required) -class SetupView(IdempotentSessionWizardView): +class SetupView(SuccessURLAllowedHostsMixin, IdempotentSessionWizardView): """ View for handling OTP setup using a wizard. @@ -423,6 +423,27 @@ class SetupView(IdempotentSessionWizardView): # Other forms are dynamically added in get_form_list() ) + # Copied from django.conrib.auth.views.LoginView (Branch: stable/1.11.x) + # https://github.com/django/django/blob/58df8aa40fe88f753ba79e091a52f236246260b3/django/contrib/auth/views.py#L63 + def get_success_url(self): + url = self.get_redirect_url() + return url or reverse('two_factor:setup_complete') + + # Copied from django.conrib.auth.views.LoginView (Branch: stable/1.11.x) + # https://github.com/django/django/blob/58df8aa40fe88f753ba79e091a52f236246260b3/django/contrib/auth/views.py#L67 + def get_redirect_url(self): + """Return the user-originating redirect URL if it's safe.""" + redirect_to = self.request.POST.get( + REDIRECT_FIELD_NAME, + self.request.GET.get(REDIRECT_FIELD_NAME, '') + ) + url_is_safe = url_has_allowed_host_and_scheme( + url=redirect_to, + allowed_hosts=self.get_success_url_allowed_hosts(), + require_https=self.request.is_secure(), + ) + return redirect_to if url_is_safe else '' + def get_method(self): method_data = self.storage.validated_step_data.get('method', {}) method_key = method_data.get('method', None) @@ -433,7 +454,7 @@ def get(self, request, *args, **kwargs): Start the setup wizard. Redirect if already enabled. """ if default_device(self.request.user): - return redirect(self.success_url) + return redirect(self.get_success_url()) return super().get(request, *args, **kwargs) def get_form(self, step=None, **kwargs): @@ -501,7 +522,7 @@ def done(self, form_list, **kwargs): raise NotImplementedError("Unknown method '%s'" % method.code) django_otp.login(self.request, device) - return redirect(self.success_url) + return redirect(self.get_success_url()) def get_form_kwargs(self, step=None): kwargs = {} @@ -613,11 +634,14 @@ class SetupCompleteView(TemplateView): """ template_name = 'two_factor/core/setup_complete.html' + def get(self, request, *args, **kwargs): + if request.session.get('next'): + return redirect(request.session.get('next')) + return super().get(request, *args, **kwargs) + def get_context_data(self): - redirect_url = self.request.session.pop('next', None) return { 'phone_methods': get_available_phone_methods(), - 'next': redirect_url } From ee0d2b6c1c0358d8bea9522865507d4fca7259c9 Mon Sep 17 00:00:00 2001 From: Darrel O'Pry Date: Thu, 23 Jun 2022 08:22:42 -0400 Subject: [PATCH 3/4] fix: replace use of SuccessURLAllowedHostsMixin with RedirectUrlMixin --- two_factor/views/core.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/two_factor/views/core.py b/two_factor/views/core.py index e46cca90a..def045333 100644 --- a/two_factor/views/core.py +++ b/two_factor/views/core.py @@ -400,7 +400,7 @@ def dispatch(self, request, *args, **kwargs): @class_view_decorator(never_cache) @class_view_decorator(login_required) -class SetupView(SuccessURLAllowedHostsMixin, IdempotentSessionWizardView): +class SetupView(RedirectURLMixin, IdempotentSessionWizardView): """ View for handling OTP setup using a wizard. From 6144f37a2f6f8a8b1e34963aed451294a77fb9e6 Mon Sep 17 00:00:00 2001 From: Darrel O'Pry Date: Thu, 23 Jun 2022 08:40:42 -0400 Subject: [PATCH 4/4] chore: Add Changelog entry --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 35838b852..7cde32a7d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +## Unreleased + +### Added +- Enforcing a redirect to setup of otp device when none available for user [#550](https://github.com/jazzband/django-two-factor-auth/pull/500) + ## 1.14.0 ### Added