make phonenumber plugin really a plugin

Author: jpaniagualaconich

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'
 

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

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):

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',

tests/test_views_profile.py

@@ -6,56 +6,37 @@
 
 
 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()
         self.user = self.create_user()
         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'})

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

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')

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):

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.')

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/<int:pk>/',
+        'unregister/<int:pk>/',
         PhoneDeleteView.as_view(),
         name='phone_delete',
     ),

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)

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):

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):