From 0663ac37c50b05af8b8b685e8c639b39bcd91587 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Fri, 9 Aug 2024 23:56:02 +0530 Subject: [PATCH] [fix] Removed default values from fallback fields Fallback fields should not have default field set to the fallback value as it would require users to manually update those fields when the fallback value is changed. --- openwisp_radius/api/freeradius_views.py | 4 +- openwisp_radius/api/utils.py | 2 +- openwisp_radius/api/views.py | 2 +- openwisp_radius/base/models.py | 55 +------------ ...organizationradiussettings_sms_cooldown.py | 4 +- ...ttings_allowed_mobile_prefixes_and_more.py | 61 +++++++++++++++ .../migrations/0038_clean_fallbackfields.py | 60 ++++++++++++++ openwisp_radius/tasks.py | 2 +- openwisp_radius/tests/test_admin.py | 2 +- openwisp_radius/tests/test_api/test_api.py | 29 ++++--- .../tests/test_api/test_phone_verification.py | 4 +- openwisp_radius/tests/test_api/test_utils.py | 4 + openwisp_radius/tests/test_saml/test_utils.py | 2 + openwisp_radius/tests/test_social.py | 2 + openwisp_radius/utils.py | 2 +- ...ttings_allowed_mobile_prefixes_and_more.py | 78 +++++++++++++++++++ 16 files changed, 241 insertions(+), 72 deletions(-) create mode 100644 openwisp_radius/migrations/0037_alter_organizationradiussettings_allowed_mobile_prefixes_and_more.py create mode 100644 openwisp_radius/migrations/0038_clean_fallbackfields.py create mode 100644 tests/openwisp2/sample_radius/migrations/0029_alter_organizationradiussettings_allowed_mobile_prefixes_and_more.py diff --git a/openwisp_radius/api/freeradius_views.py b/openwisp_radius/api/freeradius_views.py index dd90594d..09cc485b 100644 --- a/openwisp_radius/api/freeradius_views.py +++ b/openwisp_radius/api/freeradius_views.py @@ -127,9 +127,7 @@ def _handle_mac_address_authentication(self, username, request): ) if ( not open_session - or not open_session.organization.radius_settings.get_setting( - 'mac_addr_roaming_enabled' - ) + or not open_session.organization.radius_settings.mac_addr_roaming_enabled ): return None, None username = open_session.username diff --git a/openwisp_radius/api/utils.py b/openwisp_radius/api/utils.py index aed6d98f..19d7dc70 100644 --- a/openwisp_radius/api/utils.py +++ b/openwisp_radius/api/utils.py @@ -26,7 +26,7 @@ def _needs_identity_verification(self, organization_filter_kwargs={}, org=None): org = Organization.objects.select_related('radius_settings').get( **organization_filter_kwargs ) - return org.radius_settings.get_setting('needs_identity_verification') + return org.radius_settings.needs_identity_verification except ObjectDoesNotExist: return app_settings.NEEDS_IDENTITY_VERIFICATION diff --git a/openwisp_radius/api/views.py b/openwisp_radius/api/views.py index ae8ddd0d..f59dac11 100644 --- a/openwisp_radius/api/views.py +++ b/openwisp_radius/api/views.py @@ -630,7 +630,7 @@ def create(self, *args, **kwargs): raise serializers.ValidationError(error_dict) except UserAlreadyVerified as e: raise serializers.ValidationError({'user': str(e)}) - org_cooldown = self.organization.radius_settings.get_setting('sms_cooldown') + org_cooldown = self.organization.radius_settings.sms_cooldown try: self.enforce_sms_request_cooldown(org_cooldown, phone_number) except SmsAttemptCooldownException as e: diff --git a/openwisp_radius/base/models.py b/openwisp_radius/base/models.py index c0534df6..a27d9c63 100644 --- a/openwisp_radius/base/models.py +++ b/openwisp_radius/base/models.py @@ -998,7 +998,7 @@ def save_user(self, user): RegisteredUser = swapper.load_model('openwisp_radius', 'RegisteredUser') user.save() registered_user = RegisteredUser(user=user, method='manual') - if self.organization.radius_settings.get_setting('needs_identity_verification'): + if self.organization.radius_settings.needs_identity_verification: registered_user.is_verified = True registered_user.save() self.users.add(user) @@ -1079,17 +1079,11 @@ class AbstractOrganizationRadiusSettings(UUIDModel): ) token = KeyField(max_length=32) sms_verification = FallbackBooleanChoiceField( - null=True, - blank=True, - default=None, help_text=_SMS_VERIFICATION_HELP_TEXT, fallback=app_settings.SMS_VERIFICATION_ENABLED, verbose_name=_('SMS verification'), ) needs_identity_verification = FallbackBooleanChoiceField( - null=True, - blank=True, - default=None, help_text=_IDENTITY_VERIFICATION_ENABLED_HELP_TEXT, fallback=app_settings.NEEDS_IDENTITY_VERIFICATION, ) @@ -1105,8 +1099,6 @@ class AbstractOrganizationRadiusSettings(UUIDModel): sms_message = FallbackTextField( _('SMS Message'), max_length=160, - blank=True, - null=True, help_text=_( 'SMS message template used for sending verification code.' ' Must contain "{code}" placeholder for OTP value.' @@ -1115,8 +1107,6 @@ class AbstractOrganizationRadiusSettings(UUIDModel): ) sms_cooldown = FallbackPositiveIntegerField( _('SMS Cooldown'), - blank=True, - null=True, help_text=_( 'Time period a user will have to wait before requesting' ' another SMS token (in seconds).' @@ -1133,33 +1123,22 @@ class AbstractOrganizationRadiusSettings(UUIDModel): verbose_name=_('SMS meta data'), ) freeradius_allowed_hosts = FallbackTextField( - null=True, - blank=True, help_text=_GET_IP_LIST_HELP_TEXT, - default=','.join(app_settings.FREERADIUS_ALLOWED_HOSTS), fallback=','.join(app_settings.FREERADIUS_ALLOWED_HOSTS), ) coa_enabled = FallbackBooleanChoiceField( - null=True, - blank=True, - default=None, help_text=_COA_ENABLED_HELP_TEXT, fallback=app_settings.COA_ENABLED, verbose_name=_('CoA Enabled'), ) allowed_mobile_prefixes = FallbackTextField( - null=True, - blank=True, help_text=_GET_MOBILE_PREFIX_HELP_TEXT, - default=','.join(app_settings.ALLOWED_MOBILE_PREFIXES), fallback=','.join(app_settings.ALLOWED_MOBILE_PREFIXES), ) first_name = FallbackCharChoiceField( verbose_name=_('first name'), help_text=_GET_OPTIONAL_FIELDS_HELP_TEXT, max_length=12, - null=True, - blank=True, choices=OPTIONAL_FIELD_CHOICES, fallback=OPTIONAL_SETTINGS.get('first_name', None), ) @@ -1167,8 +1146,6 @@ class AbstractOrganizationRadiusSettings(UUIDModel): verbose_name=_('last name'), help_text=_GET_OPTIONAL_FIELDS_HELP_TEXT, max_length=12, - null=True, - blank=True, choices=OPTIONAL_FIELD_CHOICES, fallback=OPTIONAL_SETTINGS.get('last_name', None), ) @@ -1176,8 +1153,6 @@ class AbstractOrganizationRadiusSettings(UUIDModel): verbose_name=_('location'), help_text=_GET_OPTIONAL_FIELDS_HELP_TEXT, max_length=12, - null=True, - blank=True, choices=OPTIONAL_FIELD_CHOICES, fallback=OPTIONAL_SETTINGS.get('location', None), ) @@ -1185,38 +1160,24 @@ class AbstractOrganizationRadiusSettings(UUIDModel): verbose_name=_('birth date'), help_text=_GET_OPTIONAL_FIELDS_HELP_TEXT, max_length=12, - null=True, - blank=True, choices=OPTIONAL_FIELD_CHOICES, fallback=OPTIONAL_SETTINGS.get('birth_date', None), ) registration_enabled = FallbackBooleanChoiceField( - null=True, - blank=True, - default=None, help_text=_REGISTRATION_ENABLED_HELP_TEXT, fallback=app_settings.REGISTRATION_API_ENABLED, ) saml_registration_enabled = FallbackBooleanChoiceField( - null=True, - blank=True, - default=None, help_text=_SAML_REGISTRATION_ENABLED_HELP_TEXT, verbose_name=_('SAML registration enabled'), fallback=app_settings.SAML_REGISTRATION_ENABLED, ) mac_addr_roaming_enabled = FallbackBooleanChoiceField( - null=True, - blank=True, - default=None, help_text=_MAC_ADDR_ROAMING_ENABLED_HELP_TEXT, verbose_name=_('MAC address roaming enabled'), fallback=app_settings.MAC_ADDR_ROAMING_ENABLED, ) social_registration_enabled = FallbackBooleanChoiceField( - null=True, - blank=True, - default=None, help_text=_SOCIAL_REGISTRATION_ENABLED_HELP_TEXT, fallback=app_settings.SOCIAL_REGISTRATION_ENABLED, ) @@ -1234,11 +1195,8 @@ class AbstractOrganizationRadiusSettings(UUIDModel): ) password_reset_url = FallbackCharField( verbose_name=_('Password reset URL'), - null=True, - blank=True, max_length=200, help_text=_PASSWORD_RESET_URL_HELP_TEXT, - default=DEFAULT_PASSWORD_RESET_URL, fallback=DEFAULT_PASSWORD_RESET_URL, validators=[password_reset_url_validator], ) @@ -1266,7 +1224,7 @@ def allowed_mobile_prefixes_list(self): return mobile_prefixes def clean(self): - if self.get_setting('sms_verification') and not self.sms_sender: + if self.sms_verification and not self.sms_sender: raise ValidationError( { 'sms_sender': _( @@ -1358,13 +1316,6 @@ def _clean_sms_message(self): } ) - def get_setting(self, field_name): - value = getattr(self, field_name) - field = self._meta.get_field(field_name) - if value is None and hasattr(field, 'fallback'): - return field.fallback - return value - def save_cache(self, *args, **kwargs): cache.set(self.organization.pk, self.token) cache.set(f'ip-{self.organization.pk}', self.freeradius_allowed_hosts_list) @@ -1469,7 +1420,7 @@ def send_token(self): ) ) org_radius_settings = org_user.organization.radius_settings - message = _(org_radius_settings.get_setting('sms_message')).format( + message = _(org_radius_settings.sms_message).format( organization=org_radius_settings.organization.name, code=self.token ) sms_message = SmsMessage( diff --git a/openwisp_radius/migrations/0035_organizationradiussettings_sms_cooldown.py b/openwisp_radius/migrations/0035_organizationradiussettings_sms_cooldown.py index 03b4b6bc..d0880276 100644 --- a/openwisp_radius/migrations/0035_organizationradiussettings_sms_cooldown.py +++ b/openwisp_radius/migrations/0035_organizationradiussettings_sms_cooldown.py @@ -4,6 +4,8 @@ import openwisp_utils.fields +from .. import settings as app_settings + class Migration(migrations.Migration): dependencies = [ @@ -16,7 +18,7 @@ class Migration(migrations.Migration): name="sms_cooldown", field=openwisp_utils.fields.FallbackPositiveIntegerField( blank=True, - fallback=30, + fallback=app_settings.SMS_COOLDOWN, help_text=( "Time period a user will have to wait before" " requesting another SMS token (in seconds)." diff --git a/openwisp_radius/migrations/0037_alter_organizationradiussettings_allowed_mobile_prefixes_and_more.py b/openwisp_radius/migrations/0037_alter_organizationradiussettings_allowed_mobile_prefixes_and_more.py new file mode 100644 index 00000000..e68cf6d9 --- /dev/null +++ b/openwisp_radius/migrations/0037_alter_organizationradiussettings_allowed_mobile_prefixes_and_more.py @@ -0,0 +1,61 @@ +# Generated by Django 4.2.14 on 2024-08-07 16:38 + +from django.db import migrations + +import openwisp_radius.base.validators +import openwisp_utils.fields + +from .. import settings as app_settings + + +class Migration(migrations.Migration): + dependencies = [ + ("openwisp_radius", "0036_organizationradiussettings_mac_addr_roaming_enabled"), + ] + + operations = [ + migrations.AlterField( + model_name="organizationradiussettings", + name="allowed_mobile_prefixes", + field=openwisp_utils.fields.FallbackTextField( + blank=True, + default=None, + fallback=','.join(app_settings.ALLOWED_MOBILE_PREFIXES), + help_text=( + "Comma separated list of international mobile prefixes" + " allowed to register via the user registration API." + ), + null=True, + ), + ), + migrations.AlterField( + model_name="organizationradiussettings", + name="freeradius_allowed_hosts", + field=openwisp_utils.fields.FallbackTextField( + blank=True, + default=None, + fallback=','.join(app_settings.FREERADIUS_ALLOWED_HOSTS), + help_text=( + "Comma separated list of IP addresses allowed to access" + " freeradius API" + ), + null=True, + ), + ), + migrations.AlterField( + model_name="organizationradiussettings", + name="password_reset_url", + field=openwisp_utils.fields.FallbackCharField( + blank=True, + default=None, + fallback=app_settings.DEFAULT_PASSWORD_RESET_URL, + help_text="Enter the URL where users can reset their password", + max_length=200, + null=True, + validators=[ + openwisp_radius.base.validators.password_reset_url_validator + ], + verbose_name="Password reset URL", + ), + ), + ] diff --git a/openwisp_radius/migrations/0038_clean_fallbackfields.py b/openwisp_radius/migrations/0038_clean_fallbackfields.py new file mode 100644 index 00000000..2c9c7548 --- /dev/null +++ b/openwisp_radius/migrations/0038_clean_fallbackfields.py @@ -0,0 +1,60 @@ +from django.db import migrations + +from openwisp_utils.fields import FallbackMixin + +from ..utils import load_model + + +def clean_fallback_fields(apps, schema_editor): + """ + This data migration is necessary to clean up the database + from unnecessary stored fallback values. In the previous + implementation, the fallback value was stored in the database. + However, this was not the intended behavior and was a bug. This migration + sets the fallback fields to None when the value in the database + is the same as the fallback value, effectively removing the + unnecessary data from the database. + """ + OrganizationRadiusSettings = load_model('OrganizationRadiusSettings') + fallback_fields = [] + fallback_field_names = [] + + for field in OrganizationRadiusSettings._meta.get_fields(): + if isinstance(field, FallbackMixin): + fallback_fields.append(field) + fallback_field_names.append(field.name) + updated_settings = [] + for radius_settings in OrganizationRadiusSettings.objects.iterator(): + changed = False + for field in fallback_fields: + if getattr(radius_settings, field.name) == field.fallback: + setattr(radius_settings, field.name, None) + changed = True + if changed: + updated_settings.append(radius_settings) + + if len(updated_settings) > 100: + OrganizationRadiusSettings.objects.bulk_update( + updated_settings, fields=[field.name for field in fallback_fields] + ) + updated_settings = [] + + if updated_settings: + OrganizationRadiusSettings.objects.bulk_update( + updated_settings, fields=[field.name for field in fallback_fields] + ) + + +class Migration(migrations.Migration): + dependencies = [ + ( + 'openwisp_radius', + '0037_alter_organizationradiussettings_allowed_mobile_prefixes_and_more', + ), + ] + + operations = [ + migrations.RunPython( + clean_fallback_fields, reverse_code=migrations.RunPython.noop + ), + ] diff --git a/openwisp_radius/tasks.py b/openwisp_radius/tasks.py index 85e34384..3a85115d 100644 --- a/openwisp_radius/tasks.py +++ b/openwisp_radius/tasks.py @@ -202,7 +202,7 @@ def get_radius_attributes(user): attributes['User-Name'] = user.username updated_sessions = [] for session in open_sessions: - if not session.organization.radius_settings.get_setting('coa_enabled'): + if not session.organization.radius_settings.coa_enabled: continue radsecret = get_radsecret_from_radacct(session) if not radsecret: diff --git a/openwisp_radius/tests/test_admin.py b/openwisp_radius/tests/test_admin.py index 461e158c..c76a55f0 100644 --- a/openwisp_radius/tests/test_admin.py +++ b/openwisp_radius/tests/test_admin.py @@ -674,7 +674,7 @@ def test_backward_compatible_default_password_reset_url(self): # The default value is set on project startup, hence # it also requires mocking. OrganizationRadiusSettings._meta.get_field('password_reset_url'), - 'default', + 'fallback', app_settings.DEFAULT_PASSWORD_RESET_URL, ): response = self.client.get(url) diff --git a/openwisp_radius/tests/test_api/test_api.py b/openwisp_radius/tests/test_api/test_api.py index 5b53279d..a208e985 100644 --- a/openwisp_radius/tests/test_api/test_api.py +++ b/openwisp_radius/tests/test_api/test_api.py @@ -27,7 +27,6 @@ ) from openwisp_utils.tests import capture_any_output, capture_stderr -from ... import settings as app_settings from ...utils import load_model from ..mixins import ApiTokenMixin, BaseTestCase, BaseTransactionTestCase from .test_freeradius_api import AcctMixin @@ -358,17 +357,29 @@ def test_radius_user_serializer(self): }, ) + # The fallback value is set on project startup, hence it also requires mocking. @mock.patch.object( - app_settings, - 'OPTIONAL_REGISTRATION_FIELDS', - { - 'first_name': 'disabled', - 'last_name': 'allowed', - 'birth_date': 'disabled', - 'location': 'mandatory', - }, + OrganizationRadiusSettings._meta.get_field('first_name'), + 'fallback', + 'disabled', + ) + @mock.patch.object( + OrganizationRadiusSettings._meta.get_field('last_name'), + 'fallback', + 'allowed', + ) + @mock.patch.object( + OrganizationRadiusSettings._meta.get_field('birth_date'), + 'fallback', + 'disabled', + ) + @mock.patch.object( + OrganizationRadiusSettings._meta.get_field('location'), + 'fallback', + 'mandatory', ) def test_optional_fields_registration(self): + self.default_org.radius_settings.refresh_from_db() self._superuser_login() url = reverse('radius:rest_register', args=[self.default_org.slug]) params = { diff --git a/openwisp_radius/tests/test_api/test_phone_verification.py b/openwisp_radius/tests/test_api/test_phone_verification.py index 758e30fb..0d5241a1 100644 --- a/openwisp_radius/tests/test_api/test_phone_verification.py +++ b/openwisp_radius/tests/test_api/test_phone_verification.py @@ -252,7 +252,7 @@ def test_create_phone_token_400_sms_request_cooldown(self): self.assertEqual(response.status_code, 201) self.assertEqual( response.data['cooldown'], - self.default_org.radius_settings.get_setting('sms_cooldown'), + self.default_org.radius_settings.sms_cooldown, ) with freeze_time(start_time + timedelta(seconds=15)): @@ -270,7 +270,7 @@ def test_create_phone_token_400_sms_request_cooldown(self): self.assertEqual(response.status_code, 201) self.assertEqual( response.data['cooldown'], - self.default_org.radius_settings.get_setting('sms_cooldown'), + self.default_org.radius_settings.sms_cooldown, ) @capture_any_output() diff --git a/openwisp_radius/tests/test_api/test_utils.py b/openwisp_radius/tests/test_api/test_utils.py index a921dde7..2beffe09 100644 --- a/openwisp_radius/tests/test_api/test_utils.py +++ b/openwisp_radius/tests/test_api/test_utils.py @@ -29,6 +29,8 @@ def test_is_registration_enabled(self): with self.subTest('Test registration enabled set to None'): org.radius_settings.registration_enabled = None + org.radius_settings.save(update_fields=['registration_enabled']) + org.radius_settings.refresh_from_db(fields=['registration_enabled']) self.assertEqual( get_organization_radius_settings(org, 'registration_enabled'), app_settings.REGISTRATION_API_ENABLED, @@ -62,6 +64,8 @@ def test_is_sms_verification_enabled(self): with self.subTest('Test sms verification enabled set to None'): org.radius_settings.sms_verification = None + org.radius_settings.save(update_fields=['sms_verification']) + org.radius_settings.refresh_from_db(fields=['sms_verification']) self.assertEqual( get_organization_radius_settings(org, 'sms_verification'), app_settings.SMS_VERIFICATION_ENABLED, diff --git a/openwisp_radius/tests/test_saml/test_utils.py b/openwisp_radius/tests/test_saml/test_utils.py index 35af5870..d6db5ee1 100644 --- a/openwisp_radius/tests/test_saml/test_utils.py +++ b/openwisp_radius/tests/test_saml/test_utils.py @@ -28,6 +28,8 @@ def test_is_saml_authentication_enabled(self): with self.subTest('Test saml_registration_enabled set to None'): org.radius_settings.saml_registration_enabled = None + org.radius_settings.save(update_fields=['saml_registration_enabled']) + org.radius_settings.refresh_from_db(fields=['saml_registration_enabled']) self.assertEqual( get_organization_radius_settings(org, 'saml_registration_enabled'), app_settings.SAML_REGISTRATION_ENABLED, diff --git a/openwisp_radius/tests/test_social.py b/openwisp_radius/tests/test_social.py index ccefcd9b..6db74c43 100644 --- a/openwisp_radius/tests/test_social.py +++ b/openwisp_radius/tests/test_social.py @@ -156,6 +156,8 @@ def test_is_social_authentication_enabled(self): with self.subTest('Test social_registration_enabled set to None'): org.radius_settings.social_registration_enabled = None + org.radius_settings.save(update_fields=['social_registration_enabled']) + org.radius_settings.refresh_from_db(fields=['social_registration_enabled']) self.assertEqual( get_organization_radius_settings(org, 'social_registration_enabled'), app_settings.SOCIAL_REGISTRATION_ENABLED, diff --git a/openwisp_radius/utils.py b/openwisp_radius/utils.py index 35203f4b..499059fb 100644 --- a/openwisp_radius/utils.py +++ b/openwisp_radius/utils.py @@ -196,7 +196,7 @@ def update_user_related_records(sender, instance, created, **kwargs): def get_organization_radius_settings(organization, radius_setting): try: - return organization.radius_settings.get_setting(radius_setting) + return getattr(organization.radius_settings, radius_setting) except ObjectDoesNotExist: logger.exception( f'Got exception while accessing radius_settings for {organization.name}' diff --git a/tests/openwisp2/sample_radius/migrations/0029_alter_organizationradiussettings_allowed_mobile_prefixes_and_more.py b/tests/openwisp2/sample_radius/migrations/0029_alter_organizationradiussettings_allowed_mobile_prefixes_and_more.py new file mode 100644 index 00000000..aaa69353 --- /dev/null +++ b/tests/openwisp2/sample_radius/migrations/0029_alter_organizationradiussettings_allowed_mobile_prefixes_and_more.py @@ -0,0 +1,78 @@ +# Generated by Django 4.2.11 on 2024-07-17 11:32 + +from django.db import migrations + +import openwisp_radius.base.validators +import openwisp_utils.fields +from openwisp_radius import settings as app_settings + + +class Migration(migrations.Migration): + dependencies = [ + ( + "sample_radius", + "0028_alter_organizationradiussettings_allowed_mobile_prefixes_and_more", + ), + ] + + operations = [ + migrations.AlterField( + model_name="organizationradiussettings", + name="allowed_mobile_prefixes", + field=openwisp_utils.fields.FallbackTextField( + blank=True, + default=None, + fallback=",".join(app_settings.ALLOWED_MOBILE_PREFIXES), + help_text=( + "Comma separated list of international mobile prefixes" + " allowed to register via the user registration API." + ), + null=True, + ), + ), + migrations.AlterField( + model_name="organizationradiussettings", + name="freeradius_allowed_hosts", + field=openwisp_utils.fields.FallbackTextField( + blank=True, + default=None, + fallback=",".join(app_settings.FREERADIUS_ALLOWED_HOSTS), + help_text=( + "Comma separated list of IP addresses allowed to access" + " freeradius API" + ), + null=True, + ), + ), + migrations.AlterField( + model_name="organizationradiussettings", + name="password_reset_url", + field=openwisp_utils.fields.FallbackCharField( + blank=True, + default=None, + fallback=app_settings.DEFAULT_PASSWORD_RESET_URL, + help_text="Enter the URL where users can reset their password", + max_length=200, + null=True, + validators=[ + openwisp_radius.base.validators.password_reset_url_validator + ], + verbose_name="Password reset URL", + ), + ), + migrations.AlterField( + model_name="organizationradiussettings", + name="sms_cooldown", + field=openwisp_utils.fields.FallbackPositiveIntegerField( + blank=True, + default=None, + fallback=app_settings.SMS_COOLDOWN, + help_text=( + "Time period a user will have to wait before requesting " + "another SMS token (in seconds)." + ), + null=True, + verbose_name="SMS Cooldown", + ), + ), + ]