[fix] Fixes by @coderabbitai

Author: pandafy

openwisp_radius/admin.py

@@ -3,7 +3,7 @@
 from django import forms
 from django.conf import settings
 from django.contrib import admin, messages
-from django.contrib.admin import ModelAdmin, StackedInline, TabularInline
+from django.contrib.admin import ModelAdmin, StackedInline
 from django.contrib.admin.utils import model_ngettext
 from django.contrib.auth import get_user_model
 from django.core.exceptions import PermissionDenied
@@ -534,7 +534,7 @@ def has_change_permission(self, request, obj=None):
         return False
 
 
-class RegisteredUserInline(TabularInline):
+class RegisteredUserInline(StackedInline):
     model = RegisteredUser
     form = AlwaysHasChangedForm
     extra = 0

openwisp_radius/api/serializers.py

@@ -793,17 +793,26 @@ class Meta:
         ]
 
     def _get_registered_user(self, obj):
-        view = self.context.get("view")
-        organization = getattr(view, "organization", None)
-        org_reg_user = None
-        global_reg_user = None
-        for ru in obj.registered_users.all():
-            if organization and ru.organization_id == organization.pk:
-                org_reg_user = ru
-                break
-            elif ru.organization_id is None:
-                global_reg_user = ru
-        return org_reg_user or global_reg_user
+        if not hasattr(self, "_registered_user_cache"):
+            self._registered_user_cache = {}
+        if obj.pk not in self._registered_user_cache:
+            view = self.context.get("view")
+            organization = getattr(view, "organization", None)
+            org_reg_user = None
+            global_reg_user = None
+            # We iterate over .all() instead of using .filter() because callers
+            # of this serializer (e.g. validate_auth_token) prefetch
+            # "registered_users" via prefetch_related. Using .all() hits the
+            # in-memory prefetch cache (0 DB queries), whereas .filter() would
+            # bypass the cache and issue a new query every time.
+            for ru in obj.registered_users.all():
+                if organization and ru.organization_id == organization.pk:
+                    org_reg_user = ru
+                    break
+                elif ru.organization_id is None:
+                    global_reg_user = ru
+            self._registered_user_cache[obj.pk] = org_reg_user or global_reg_user
+        return self._registered_user_cache[obj.pk]
 
     def get_is_verified(self, obj):
         reg_user = self._get_registered_user(obj)

openwisp_radius/api/utils.py

@@ -9,6 +9,7 @@
 
 Organization = load_model("openwisp_users", "Organization")
 OrganizationRadiusSettings = load_model("openwisp_radius", "OrganizationRadiusSettings")
+RegisteredUser = load_model("openwisp_radius", "RegisteredUser")
 
 
 class ErrorDictMixin(object):
@@ -33,6 +34,8 @@ def _needs_identity_verification(self, organization_filter_kwargs={}, org=None):
     def is_identity_verified_strong(self, user, organization=None):
         reg_user = None
         global_reg_user = None
+        # We use all() to utilize the prefetch cache, otherwise
+        # it would cause an additional query to fetch the registered user
         for ru in user.registered_users.all():
             if organization and ru.organization_id == organization.pk:
                 reg_user = ru

openwisp_radius/api/views.py

@@ -645,7 +645,7 @@ def create(self, *args, **kwargs):
         try:
             phone_token.full_clean()
             if kwargs.get("enforce_unverified", True):
-                phone_token._validate_already_verified()
+                phone_token._validate_already_verified(organization=self.organization)
         except ValidationError as e:
             error_dict = self._get_error_dict(e)
             raise serializers.ValidationError(error_dict)
@@ -754,7 +754,9 @@ def post(self, request, *args, **kwargs):
                 _("No verification code found in the system for this user.")
             )
         try:
-            is_valid = phone_token.is_valid(serializer.data["code"])
+            is_valid = phone_token.is_valid(
+                serializer.data["code"], organization=self.organization
+            )
         except PhoneTokenException as e:
             return self._error_response(str(e))
         if not is_valid:
@@ -763,11 +765,13 @@ def post(self, request, *args, **kwargs):
             reg_user, __ = RegisteredUser.get_or_create_for_user_and_org(
                 user=user,
                 organization=self.organization,
-                defaults={"is_verified": False, "method": ""},
+                defaults={
+                    "is_verified": True,
+                    "method": "mobile_phone",
+                    "is_active": True,
+                },
             )
             reg_user.is_verified = True
-            reg_user.method = "mobile_phone"
-            user.is_active = True
             # Update username if phone_number is used as username
             if user.username == user.phone_number:
                 user.username = phone_token.phone_number

openwisp_radius/base/models.py

@@ -1058,14 +1058,20 @@ def save_user(self, user):
         OrganizationUser = swapper.load_model("openwisp_users", "OrganizationUser")
         RegisteredUser = swapper.load_model("openwisp_radius", "RegisteredUser")
         user.save()
-        registered_user = RegisteredUser(
+        registered_user, created = RegisteredUser.get_or_create_for_user_and_org(
             user=user,
-            method="manual",
             organization=self.organization,
+            defaults={
+                "method": "manual",
+                "is_verified": self.organization.radius_settings.needs_identity_verification,
+            },
         )
-        if self.organization.radius_settings.needs_identity_verification:
+        if (
+            not created
+            and self.organization.radius_settings.needs_identity_verification
+        ):
             registered_user.is_verified = True
-        registered_user.save()
+            registered_user.save()
         self.users.add(user)
         if OrganizationUser.objects.filter(
             user=user, organization=self.organization
@@ -1563,26 +1569,35 @@ def send_token(self):
         )
         sms_message.send(meta_data=org_radius_settings.sms_meta_data)
 
-    def is_valid(self, token):
+    def is_valid(self, token, organization=None):
         self.attempts += 1
         try:
-            self.verified = self.__check(token)
+            self.verified = self.__check(token, organization=organization)
         except exceptions.PhoneTokenException as phone_error:
             self.save()
             raise phone_error
         self.save()
         return self.verified
 
-    def _validate_already_verified(self):
+    def _validate_already_verified(self, organization=None):
         RegisteredUser = swapper.load_model("openwisp_radius", "RegisteredUser")
-        if RegisteredUser.objects.filter(user=self.user, is_verified=True).exists():
+        if organization is not None:
+            reg_user = RegisteredUser.get_global_or_org_specific(
+                self.user, organization
+            )
+            is_verified = reg_user is not None and reg_user.is_verified
+        else:
+            is_verified = RegisteredUser.objects.filter(
+                user=self.user, is_verified=True
+            ).exists()
+        if is_verified:
             logger.warning(f"User {self.user.pk} is already verified")
             raise exceptions.UserAlreadyVerified(
                 _("This user has been already verified.")
             )
 
-    def __check(self, token):
-        self._validate_already_verified()
+    def __check(self, token, organization=None):
+        self._validate_already_verified(organization=organization)
         if self.attempts > app_settings.SMS_TOKEN_MAX_ATTEMPTS:
             logger.warning(
                 f"User {self.user} has reached the max "
@@ -1613,6 +1628,7 @@ class AbstractRegisteredUser(UUIDModel):
     organization = models.ForeignKey(
         swapper.get_model_name("openwisp_users", "Organization"),
         on_delete=models.CASCADE,
+        related_name="registered_users",
         null=True,
         blank=True,
         verbose_name=_("organization"),
@@ -1684,13 +1700,6 @@ def clean(self):
                 _("A registration record already exists for this user/organization.")
             )
 
-    @classmethod
-    def get_for_user_and_org(cls, user, organization):
-        try:
-            return cls.objects.get(user=user, organization=organization)
-        except cls.DoesNotExist:
-            return None
-
     @classmethod
     def get_or_create_for_user_and_org(cls, user, organization, defaults=None):
         defaults = defaults or {}

openwisp_radius/integrations/monitoring/tasks.py

@@ -186,6 +186,7 @@ def post_save_radiusaccounting(
         RegisteredUser.objects.only("method")
         .filter(user__username=username)
         .filter(Q(organization_id=organization_id) | Q(organization__isnull=True))
+        .order_by("-organization_id")
         .first()
     )
     if registration_method is None:

openwisp_radius/management/commands/base/delete_unverified_users.py

@@ -2,6 +2,7 @@
 
 from django.contrib.auth import get_user_model
 from django.core.management import BaseCommand
+from django.db.models import Count, Q
 from django.utils.timezone import now
 
 from openwisp_radius.utils import load_model
@@ -33,12 +34,21 @@ def handle(self, *args, **options):
         if exclude_methods:
             exclude_methods = exclude_methods.split(",")
 
-        qs = User.objects.filter(
-            date_joined__lt=days,
-            registered_users__isnull=False,
-            registered_users__is_verified=False,
-            is_staff=False,
-        ).distinct()
+        qs = (
+            User.objects.filter(
+                date_joined__lt=days,
+                registered_users__isnull=False,
+                is_staff=False,
+            )
+            .annotate(
+                num_verified=Count(
+                    "registered_users",
+                    filter=Q(registered_users__is_verified=True),
+                )
+            )
+            .filter(num_verified=0)
+            .distinct()
+        )
         if exclude_methods:
             qs = qs.exclude(registered_users__method__in=exclude_methods)
 

openwisp_radius/migrations/0043_registereduser_add_uuid.py

@@ -31,6 +31,7 @@ def copy_registered_users_reverse(apps, schema_editor):
 class Migration(migrations.Migration):
     dependencies = [
         migrations.swappable_dependency(settings.AUTH_USER_MODEL),
+        swapper.dependency("openwisp_users", "Organization"),
         ("openwisp_radius", "0042_set_existing_batches_completed"),
     ]
 
@@ -63,6 +64,7 @@ class Migration(migrations.Migration):
                         blank=True,
                         help_text=REGISTERED_USER_ORGANIZATION_HELP_TEXT,
                         null=True,
+                        related_name="registered_users",
                         on_delete=django.db.models.deletion.CASCADE,
                         to=swapper.get_model_name("openwisp_users", "Organization"),
                         verbose_name="organization",

openwisp_radius/migrations/__init__.py

@@ -5,6 +5,7 @@
 from django.conf import settings
 from django.contrib.auth.management import create_permissions
 from django.contrib.auth.models import Permission
+from django.db.models import Case, IntegerField, Value, When
 
 from ..utils import create_default_groups
 
@@ -88,9 +89,18 @@ def copy_registered_users_ctcr_reverse(
 
     restored_objects = []
     previous_user_id = None
-    queryset = RegisteredUserNew.objects.order_by(
-        "user_id", "-is_verified", "method", "pk"
+    # Annotate each row with an explicit verification priority so that stronger
+    # methods (anything that is not '' or 'email') sort before weaker ones.
+    # Lexical ordering of 'method' would place '' first, picking the weakest.
+    method_priority = Case(
+        When(method="", then=Value(0)),
+        When(method="email", then=Value(1)),
+        default=Value(2),
+        output_field=IntegerField(),
     )
+    queryset = RegisteredUserNew.objects.annotate(
+        method_priority=method_priority
+    ).order_by("user_id", "-is_verified", "-method_priority", "pk")
     for registered_user in queryset.iterator(chunk_size=BATCH_SIZE):
         if registered_user.user_id == previous_user_id:
             continue
@@ -187,10 +197,23 @@ def migrate_registered_users_multitenant_reverse(
                 organization__isnull=True,
             ).values_list("user_id", flat=True)
         )
-        org_records = RegisteredUser.objects.filter(
-            user_id__in=user_id_batch,
-            organization__isnull=False,
-        ).order_by("user_id", "-is_verified", "method", "pk")
+        # Annotate each row with an explicit verification priority so that stronger
+        # methods (anything that is not '' or 'email') sort before weaker ones.
+        # Lexical ordering of 'method' would place '' first, picking the weakest.
+        method_priority = Case(
+            When(method="", then=Value(0)),
+            When(method="email", then=Value(1)),
+            default=Value(2),
+            output_field=IntegerField(),
+        )
+        org_records = (
+            RegisteredUser.objects.filter(
+                user_id__in=user_id_batch,
+                organization__isnull=False,
+            )
+            .annotate(method_priority=method_priority)
+            .order_by("user_id", "-is_verified", "-method_priority", "pk")
+        )
 
         to_create = []
         to_delete_pks = []

openwisp_radius/tests/test_api/test_api.py

@@ -342,6 +342,7 @@ def test_radius_user_serializer(self):
             data = RadiusUserSerializer(user, context={"view": view}).data
 
         with self.subTest("test full data"):
+            registered_user = user.registered_users.get(organization=self.default_org)
             self.assertEqual(
                 data,
                 {
@@ -353,9 +354,9 @@ def test_radius_user_serializer(self):
                     "birth_date": user.birth_date,
                     "location": user.location,
                     "is_active": user.is_active,
-                    "is_verified": user.registered_users.first().is_verified,
                     "password_expired": user.has_password_expired(),
-                    "method": user.registered_users.first().method,
+                    "is_verified": registered_user.is_verified,
+                    "method": registered_user.method,
                     "radius_user_token": user.radius_token.key,
                 },
             )