From 78c515d6c03c5731a60b6b64b38f7a0f02bbd0ef Mon Sep 17 00:00:00 2001 From: Javier Paniagua Date: Fri, 8 Sep 2023 21:42:16 +0200 Subject: [PATCH] make phonenumber plugin really a plugin --- tests/settings.py | 2 + tests/test_utils.py | 4 +- tests/test_views_login.py | 5 -- tests/test_views_phone.py | 8 +--- tests/test_views_profile.py | 59 ++++++++---------------- tests/test_views_setup.py | 12 ++--- two_factor/plugins/phonenumber/apps.py | 13 ++++-- two_factor/plugins/phonenumber/forms.py | 9 +++- two_factor/plugins/phonenumber/method.py | 22 ++++----- two_factor/plugins/phonenumber/urls.py | 4 +- two_factor/plugins/phonenumber/utils.py | 38 +++++++-------- two_factor/plugins/phonenumber/views.py | 1 + two_factor/plugins/registry.py | 4 ++ two_factor/views/profile.py | 17 +++---- 14 files changed, 90 insertions(+), 108 deletions(-) diff --git a/tests/settings.py b/tests/settings.py index c5e09e7b2..0fbf0a262 100644 --- a/tests/settings.py +++ b/tests/settings.py @@ -90,6 +90,8 @@ DEFAULT_AUTO_FIELD = 'django.db.models.AutoField' TWO_FACTOR_PATCH_ADMIN = False +TWO_FACTOR_SMS_GATEWAY = 'two_factor.gateways.fake.Fake' +TWO_FACTOR_CALL_GATEWAY = 'two_factor.gateways.fake.Fake' TWO_FACTOR_WEBAUTHN_RP_NAME = 'Test Server' diff --git a/tests/test_utils.py b/tests/test_utils.py index 64762e98e..682d56470 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -140,8 +140,8 @@ class PhoneUtilsTests(UserMixin, TestCase): def test_backup_phones(self): gateway = 'two_factor.gateways.fake.Fake' user = self.create_user() - user.phonedevice_set.create(name='default', number='+12024561111') - backup = user.phonedevice_set.create(name='backup', number='+12024561111') + user.phonedevice_set.create(name='default', number='+12024561111', method='call') + backup = user.phonedevice_set.create(name='backup', number='+12024561111', method='call') parameters = [ # with_gateway, with_user, expected_output diff --git a/tests/test_views_login.py b/tests/test_views_login.py index 2f0836aee..0be4b3334 100644 --- a/tests/test_views_login.py +++ b/tests/test_views_login.py @@ -282,7 +282,6 @@ def test_throttle_with_generator(self, mock_signal): @mock.patch('two_factor.views.core.signals.user_verified.send') @override_settings( TWO_FACTOR_PHONE_THROTTLE_FACTOR=10, - TWO_FACTOR_SMS_GATEWAY='two_factor.gateways.fake.Fake' ) def test_throttle_with_phone_sms(self, mock_signal): with freeze_time("2023-01-01") as frozen_time: @@ -310,10 +309,6 @@ def test_throttle_with_phone_sms(self, mock_signal): @mock.patch('two_factor.gateways.fake.Fake') @mock.patch('two_factor.views.core.signals.user_verified.send') - @override_settings( - TWO_FACTOR_SMS_GATEWAY='two_factor.gateways.fake.Fake', - TWO_FACTOR_CALL_GATEWAY='two_factor.gateways.fake.Fake', - ) def test_with_backup_phone(self, mock_signal, fake): user = self.create_user() for no_digits in (6, 8): diff --git a/tests/test_views_phone.py b/tests/test_views_phone.py index b561aff96..9d19135cd 100644 --- a/tests/test_views_phone.py +++ b/tests/test_views_phone.py @@ -5,7 +5,6 @@ from django.shortcuts import resolve_url from django.template import Context, Template from django.test import TestCase -from django.test.utils import override_settings from django.urls import reverse, reverse_lazy from django_otp.oath import totp from django_otp.util import random_hex @@ -23,10 +22,6 @@ from .utils import UserMixin -@override_settings( - TWO_FACTOR_SMS_GATEWAY='two_factor.gateways.fake.Fake', - TWO_FACTOR_CALL_GATEWAY='two_factor.gateways.fake.Fake', -) class PhoneSetupTest(UserMixin, TestCase): def setUp(self): super().setUp() @@ -157,10 +152,11 @@ def setUp(self): self.login_user() def test_delete(self): + self.assertEqual(len(backup_phones(self.user)), 1) response = self.client.post(reverse('two_factor:phone_delete', args=[self.backup.pk])) self.assertRedirects(response, resolve_url(settings.LOGIN_REDIRECT_URL)) - self.assertEqual(list(backup_phones(self.user)), []) + self.assertEqual(backup_phones(self.user), []) def test_cannot_delete_default(self): response = self.client.post(reverse('two_factor:phone_delete', diff --git a/tests/test_views_profile.py b/tests/test_views_profile.py index fc1787a8c..5819ed0b3 100644 --- a/tests/test_views_profile.py +++ b/tests/test_views_profile.py @@ -6,16 +6,7 @@ class ProfileTest(UserMixin, TestCase): - PHONENUMBER_PLUGIN_NAME = 'two_factor.plugins.phonenumber' - EXPECTED_BASE_CONTEXT_KEYS = { - 'default_device', - 'default_device_type', - 'backup_tokens', - } - EXPECTED_PHONENUMBER_PLUGIN_ADDITIONAL_KEYS = { - 'backup_phones', - 'available_phone_methods', - } + PHONENUMBER_PLUGIN = 'two_factor.plugins.phonenumber' def setUp(self): super().setUp() @@ -23,39 +14,29 @@ def setUp(self): self.enable_otp() self.login_user() - @classmethod - def get_installed_apps_list(cls, with_phone_number_plugin=True): - apps = set(settings.INSTALLED_APPS) - if with_phone_number_plugin: - apps.add(cls.PHONENUMBER_PLUGIN_NAME) - else: - apps.remove(cls.PHONENUMBER_PLUGIN_NAME) - return list(apps) - def get_profile(self): url = reverse('two_factor:profile') return self.client.get(url) - def test_get_profile_without_phonenumer_plugin_enabled(self): - apps_list = self.get_installed_apps_list(with_phone_number_plugin=False) - with override_settings(INSTALLED_APPS=apps_list): + def test_get_profile_without_phonenumber_plugin_enabled(self): + installed_apps = [app for app in settings.INSTALLED_APPS if app != self.PHONENUMBER_PLUGIN] + + with override_settings(INSTALLED_APPS=installed_apps): + from two_factor.plugins.registry import registry + + self.assertFalse(registry.get_method('call')) + self.assertFalse(registry.get_method('sms')) + response = self.get_profile() - context_keys = set(response.context.keys()) - self.assertTrue(self.EXPECTED_BASE_CONTEXT_KEYS.issubset(context_keys)) - # None of the phonenumber related keys are present - self.assertTrue( - self.EXPECTED_PHONENUMBER_PLUGIN_ADDITIONAL_KEYS.isdisjoint( - context_keys - ) - ) + + self.assertTrue(response.context['available_phone_methods'] == []) def test_get_profile_with_phonenumer_plugin_enabled(self): - apps_list = self.get_installed_apps_list(with_phone_number_plugin=True) - with override_settings(INSTALLED_APPS=apps_list): - response = self.get_profile() - context_keys = set(response.context.keys()) - expected_keys = ( - self.EXPECTED_BASE_CONTEXT_KEYS - | self.EXPECTED_PHONENUMBER_PLUGIN_ADDITIONAL_KEYS - ) - self.assertTrue(expected_keys.issubset(context_keys)) + from two_factor.plugins.registry import registry + + self.assertTrue(registry.get_method('call')) + self.assertTrue(registry.get_method('sms')) + + response = self.get_profile() + available_phone_method_codes = {method.code for method in response.context['available_phone_methods']} + self.assertTrue(available_phone_method_codes == {'call', 'sms'}) diff --git a/tests/test_views_setup.py b/tests/test_views_setup.py index 497fe371b..b67e02d66 100644 --- a/tests/test_views_setup.py +++ b/tests/test_views_setup.py @@ -3,7 +3,6 @@ from unittest import mock from django.test import TestCase -from django.test.utils import override_settings from django.urls import reverse from django_otp import DEVICE_ID_SESSION_KEY from django_otp.oath import totp @@ -67,8 +66,6 @@ def test_setup_only_generator_available(self): self.assertRedirects(response, reverse('two_factor:setup_complete')) self.assertEqual(1, self.user.totpdevice_set.count()) - @override_settings(TWO_FACTOR_CALL_GATEWAY='two_factor.gateways.fake.Fake', - TWO_FACTOR_SMS_GATEWAY='two_factor.gateways.fake.Fake') def test_setup_generator_with_multi_method(self): response = self.client.post( reverse('two_factor:setup'), @@ -137,7 +134,6 @@ def test_no_phone(self): self.assertContains(response, 'call') @mock.patch('two_factor.gateways.fake.Fake') - @override_settings(TWO_FACTOR_CALL_GATEWAY='two_factor.gateways.fake.Fake') def test_setup_phone_call(self, fake): response = self._post(data={'setup_view-current_step': 'welcome'}) self.assertContains(response, 'Method:') @@ -176,7 +172,6 @@ def test_setup_phone_call(self, fake): self.assertEqual(phones[0].method, 'call') @mock.patch('two_factor.gateways.fake.Fake') - @override_settings(TWO_FACTOR_SMS_GATEWAY='two_factor.gateways.fake.Fake') def test_setup_phone_sms(self, fake): response = self._post(data={'setup_view-current_step': 'welcome'}) self.assertContains(response, 'Method:') @@ -239,13 +234,12 @@ def test_suggest_backup_number(self): self.enable_otp() self.login_user() - with self.settings(TWO_FACTOR_SMS_GATEWAY=None): + with self.settings(TWO_FACTOR_SMS_GATEWAY=None, TWO_FACTOR_CALL_GATEWAY=None): response = self.client.get(reverse('two_factor:setup_complete')) self.assertNotContains(response, 'Add Phone Number') - with self.settings(TWO_FACTOR_SMS_GATEWAY='two_factor.gateways.fake.Fake'): - response = self.client.get(reverse('two_factor:setup_complete')) - self.assertContains(response, 'Add Phone Number') + response = self.client.get(reverse('two_factor:setup_complete')) + self.assertContains(response, 'Add Phone Number') def test_missing_management_data(self): # missing management data diff --git a/two_factor/plugins/phonenumber/apps.py b/two_factor/plugins/phonenumber/apps.py index dc28d9f3d..c7b2aa9e3 100644 --- a/two_factor/plugins/phonenumber/apps.py +++ b/two_factor/plugins/phonenumber/apps.py @@ -12,19 +12,22 @@ class TwoFactorPhoneNumberConfig(AppConfig): url_prefix = 'phone' def ready(self): - register_methods(self, None, None) - setting_changed.connect(register_methods) + update_registered_methods(self, None, None) + setting_changed.connect(update_registered_methods) -def register_methods(sender, setting, value, **kwargs): +def update_registered_methods(sender, setting, value, **kwargs): # This allows for dynamic registration, typically when testing. from .method import PhoneCallMethod, SMSMethod - if getattr(settings, 'TWO_FACTOR_CALL_GATEWAY', None): + installed_apps = getattr(settings, 'INSTALLED_APPS') or [] + phone_number_app_installed = 'two_factor.plugins.phonenumber' in installed_apps + + if phone_number_app_installed and getattr(settings, 'TWO_FACTOR_CALL_GATEWAY', None): registry.register(PhoneCallMethod()) else: registry.unregister('call') - if getattr(settings, 'TWO_FACTOR_SMS_GATEWAY', None): + if phone_number_app_installed and getattr(settings, 'TWO_FACTOR_SMS_GATEWAY', None): registry.register(SMSMethod()) else: registry.unregister('sms') diff --git a/two_factor/plugins/phonenumber/forms.py b/two_factor/plugins/phonenumber/forms.py index 3ce780fc9..9162f9451 100644 --- a/two_factor/plugins/phonenumber/forms.py +++ b/two_factor/plugins/phonenumber/forms.py @@ -15,9 +15,16 @@ class Meta: model = PhoneDevice fields = ['number', 'method'] + @staticmethod + def get_available_choices(): + choices = [] + for method in get_available_phone_methods(): + choices.append((method.code, method.verbose_name)) + return choices + def __init__(self, **kwargs): super().__init__(**kwargs) - self.fields['method'].choices = get_available_phone_methods() + self.fields['method'].choices = self.get_available_choices() class PhoneNumberForm(forms.ModelForm): diff --git a/two_factor/plugins/phonenumber/method.py b/two_factor/plugins/phonenumber/method.py index 8953e0f4c..674759a85 100644 --- a/two_factor/plugins/phonenumber/method.py +++ b/two_factor/plugins/phonenumber/method.py @@ -4,15 +4,17 @@ from .forms import PhoneNumberForm from .models import PhoneDevice -from .utils import backup_phones, format_phone_number, mask_phone_number +from .utils import format_phone_number, mask_phone_number class PhoneMethodBase(MethodBase): def get_devices(self, user): - return [device for device in backup_phones(user) if device.method == self.code] + return PhoneDevice.objects.filter(user=user, method=self.code) def recognize_device(self, device): - return isinstance(device, PhoneDevice) + if not isinstance(device, PhoneDevice): + return False + return device.method == self.code def get_setup_forms(self, *args): return {self.code: PhoneNumberForm} @@ -28,23 +30,21 @@ def get_device_from_setup_data(self, request, storage_data, **kwargs): def get_action(self, device): number = mask_phone_number(format_phone_number(device.number)) - if device.method == 'sms': - return _('Send text message to %s') % number - else: - return _('Call number %s') % number + return self.action % number def get_verbose_action(self, device): - if device.method == 'sms': - return _('We sent you a text message, please enter the token we sent.') - else: - return _('We are calling your phone right now, please enter the digits you hear.') + return self.verbose_action class PhoneCallMethod(PhoneMethodBase): code = 'call' verbose_name = _('Phone call') + action = _('Call number %s') + verbose_action = _('We are calling your phone right now, please enter the digits you hear.') class SMSMethod(PhoneMethodBase): code = 'sms' verbose_name = _('Text message') + action = _('Send text message to %s') + verbose_action = _('We sent you a text message, please enter the token we sent.') diff --git a/two_factor/plugins/phonenumber/urls.py b/two_factor/plugins/phonenumber/urls.py index f8f15c3f2..cbb0c56dc 100644 --- a/two_factor/plugins/phonenumber/urls.py +++ b/two_factor/plugins/phonenumber/urls.py @@ -4,12 +4,12 @@ urlpatterns = [ path( - 'account/two_factor/backup/phone/register/', + 'register/', PhoneSetupView.as_view(), name='phone_create', ), path( - 'account/two_factor/backup/phone/unregister//', + 'unregister//', PhoneDeleteView.as_view(), name='phone_delete', ), diff --git a/two_factor/plugins/phonenumber/utils.py b/two_factor/plugins/phonenumber/utils.py index 9a8ea164e..914762317 100644 --- a/two_factor/plugins/phonenumber/utils.py +++ b/two_factor/plugins/phonenumber/utils.py @@ -1,31 +1,29 @@ import re -import phonenumbers -from django.conf import settings -from django.utils.translation import gettext_lazy as _ +from two_factor.plugins.registry import registry phone_mask = re.compile(r'(?<=.{3})[0-9](?=.{2})') +def get_available_phone_methods(): + methods = [] + for code in ['sms', 'call']: + if method := registry.get_method(code): + methods.append(method) + + return methods + + def backup_phones(user): - no_gateways = ( - getattr(settings, 'TWO_FACTOR_CALL_GATEWAY', None) is None - and getattr(settings, 'TWO_FACTOR_SMS_GATEWAY', None) is None) - no_user = not user or user.is_anonymous + if not user or user.is_anonymous: + return [] - if no_gateways or no_user: - from .models import PhoneDevice - return PhoneDevice.objects.none() - return user.phonedevice_set.filter(name='backup') + phones = [] + for method in get_available_phone_methods(): + phones += list(method.get_devices(user)) -def get_available_phone_methods(): - methods = [] - if getattr(settings, 'TWO_FACTOR_CALL_GATEWAY', None): - methods.append(('call', _('Phone call'))) - if getattr(settings, 'TWO_FACTOR_SMS_GATEWAY', None): - methods.append(('sms', _('Text message'))) - return methods + return [phone for phone in phones if phone.name == 'backup'] def mask_phone_number(number): @@ -39,6 +37,8 @@ def mask_phone_number(number): :param number: str or phonenumber object :return: str """ + import phonenumbers + if isinstance(number, phonenumbers.PhoneNumber): number = format_phone_number(number) return phone_mask.sub('*', number) @@ -50,6 +50,8 @@ def format_phone_number(number): :param number: str or phonenumber object :return: str """ + import phonenumbers + if not isinstance(number, phonenumbers.PhoneNumber): number = phonenumbers.parse(number) return phonenumbers.format_number(number, phonenumbers.PhoneNumberFormat.INTERNATIONAL) diff --git a/two_factor/plugins/phonenumber/views.py b/two_factor/plugins/phonenumber/views.py index 0f5551ee7..97a352367 100644 --- a/two_factor/plugins/phonenumber/views.py +++ b/two_factor/plugins/phonenumber/views.py @@ -72,6 +72,7 @@ def get_device(self, **kwargs): """ kwargs = kwargs or {} kwargs.update(self.storage.validated_step_data.get('setup', {})) + return PhoneDevice(key=self.get_key(), **kwargs) def get_key(self): diff --git a/two_factor/plugins/registry.py b/two_factor/plugins/registry.py index 985da6511..f341cc431 100644 --- a/two_factor/plugins/registry.py +++ b/two_factor/plugins/registry.py @@ -77,6 +77,10 @@ def __init__(self): self.register(GeneratorMethod()) def register(self, method): + for registered_method in self._methods: + if method.code == registered_method.code: + return + self._methods.append(method) def unregister(self, code): diff --git a/two_factor/views/profile.py b/two_factor/views/profile.py index 1cd5cba8f..e9c45b21a 100644 --- a/two_factor/views/profile.py +++ b/two_factor/views/profile.py @@ -1,4 +1,3 @@ -from django.apps.registry import apps from django.conf import settings from django.contrib.auth.decorators import login_required from django.shortcuts import redirect, resolve_url @@ -30,24 +29,22 @@ class ProfileView(TemplateView): template_name = 'two_factor/profile/profile.html' def get_context_data(self, **kwargs): + user = self.request.user + try: - backup_tokens = self.request.user.staticdevice_set.all()[0].token_set.count() + backup_tokens = user.staticdevice_set.all()[0].token_set.count() except Exception: backup_tokens = 0 context = { - 'default_device': default_device(self.request.user), - 'default_device_type': default_device(self.request.user).__class__.__name__, + 'default_device': default_device(user), + 'default_device_type': default_device(user).__class__.__name__, 'backup_tokens': backup_tokens, + 'backup_phones': backup_phones(user), + 'available_phone_methods': get_available_phone_methods(), } - if (apps.is_installed('two_factor.plugins.phonenumber')): - context.update({ - 'backup_phones': backup_phones(self.request.user), - 'available_phone_methods': get_available_phone_methods(), - }) - return context