Skip to content

Commit

Permalink
[fix] Removed default values from fallback fields
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
pandafy authored Aug 9, 2024
1 parent 7dd725e commit 0663ac3
Show file tree
Hide file tree
Showing 16 changed files with 241 additions and 72 deletions.
4 changes: 1 addition & 3 deletions openwisp_radius/api/freeradius_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion openwisp_radius/api/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion openwisp_radius/api/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
55 changes: 3 additions & 52 deletions openwisp_radius/base/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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,
)
Expand All @@ -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.'
Expand All @@ -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).'
Expand All @@ -1133,90 +1123,61 @@ 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),
)
last_name = FallbackCharChoiceField(
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),
)
location = FallbackCharChoiceField(
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),
)
birth_date = FallbackCharChoiceField(
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,
)
Expand All @@ -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],
)
Expand Down Expand Up @@ -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': _(
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

import openwisp_utils.fields

from .. import settings as app_settings


class Migration(migrations.Migration):
dependencies = [
Expand All @@ -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)."
Expand Down
Original file line number Diff line number Diff line change
@@ -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",
),
),
]
60 changes: 60 additions & 0 deletions openwisp_radius/migrations/0038_clean_fallbackfields.py
Original file line number Diff line number Diff line change
@@ -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
),
]
2 changes: 1 addition & 1 deletion openwisp_radius/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion openwisp_radius/tests/test_admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading

0 comments on commit 0663ac3

Please sign in to comment.