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", + ), + ), + ]