improve(Utilisateurs): seul les utilisateurs staff peuvent avoir un 2FA. Les utilisateurs superuser doivent avoir un 2FA

data/models/user.py

@@ -1,15 +1,20 @@
 from dirtyfields import DirtyFieldsMixin
 from django.contrib.auth.base_user import BaseUserManager
 from django.contrib.auth.models import AbstractUser
+from django.core.validators import ValidationError
 from django.db import models
 from django.db.models import Count, F, Q
 from django.utils import timezone
 from django.utils.translation import gettext_lazy as _
+from django.db.models.signals import pre_save
+from django.dispatch import receiver
+from django_otp.plugins.otp_totp.models import TOTPDevice
 
 from common.utils import utils as utils_utils
 from data.fields import ChoiceArrayField
 from data.models.geo import Department
 from data.utils import optimize_image
+from data.validators import user as user_validators
 from macantine import brevo
 
 
@@ -271,13 +276,23 @@ def reset_brevo_fields_if_email_changed(self):
             if "email" in self.get_dirty_fields():
                 self.reset_brevo_fields(with_save=False)
 
-    def save(self, **kwargs):
+    def save(self, skip_validations=False, **kwargs):
         self.normalize_fields()
         self.lowercase_fields()
         self.optimize_avatar()
         self.reset_brevo_fields_if_email_changed()
+        if not skip_validations:
+            self.full_clean(exclude=["password"])
         super().save(**kwargs)
 
+    def clean(self, *args, **kwargs):
+        validation_errors = utils_utils.merge_validation_errors(
+            user_validators.validate_user_non_staff(self),
+            user_validators.validate_user_superuser(self),
+        )
+        if validation_errors:
+            raise ValidationError(validation_errors)
+
     @property
     def has_mtm_data(self):
         return self.creation_mtm_source or self.creation_mtm_campaign or self.creation_mtm_medium
@@ -335,3 +350,9 @@ def get_brevo_data(self):
             **data_canteen_fields_dict,
             **data_canteen_diagnostic_fields_dict,
         }
+
+
+@receiver(pre_save, sender=TOTPDevice)
+def validate_totp_device_user_is_staff(sender, instance, **kwargs):
+    if instance.user and not instance.user.is_staff:
+        raise ValidationError("Seul les utilisateurs staff sont autorisés à configurer un appareil 2FA (OTP).")

data/tests/test_user.py

@@ -1,6 +1,7 @@
 from django.test import TestCase, TransactionTestCase
 from django.utils import timezone
 from freezegun import freeze_time
+from django.core.exceptions import ValidationError
 
 from data.factories import CanteenFactory, UserFactory, DiagnosticFactory
 from data.models import Canteen, User
@@ -218,3 +219,64 @@ def test_update_brevo_fields_on_save(self):
 
         self.assertEqual(user.brevo_last_update_date, None)
         self.assertFalse(user.brevo_is_deleted)
+
+
+class UserTOTPDeviceTest(TestCase):
+    def test_can_create_and_save_non_staff_without_totp_device(self):
+        user = UserFactory.build(is_staff=False, is_superuser=False)
+        user.save()
+        self.assertFalse(user.is_staff)
+        self.assertFalse(user.is_superuser)
+
+    def test_cannot_save_non_staff_with_totp_device(self):
+        from django_otp.plugins.otp_totp.models import TOTPDevice
+
+        user = UserFactory.build(is_staff=False, is_superuser=False)
+        user.save()
+        self.assertFalse(user.is_staff)
+        self.assertFalse(user.is_superuser)
+
+        self.assertRaises(ValidationError, TOTPDevice.objects.create, user=user, name="test-device")
+
+    def test_cannot_create_superuser_without_staff(self):
+        user = UserFactory.build(is_staff=False, is_superuser=True)
+        self.assertRaises(ValidationError, user.save)
+
+    def test_can_create_and_save_staff_without_totp_device(self):
+        user = UserFactory.build(is_staff=True, is_superuser=False)
+        user.save()
+        self.assertFalse(user.is_superuser)
+
+    def test_cannot_save_superuser_without_totp_device(self):
+        user = UserFactory.build(is_staff=True, is_superuser=False)
+        user.save()
+        self.assertFalse(user.is_superuser)
+
+        user.is_superuser = True
+        self.assertRaises(ValidationError, user.save)
+
+    def test_cannot_save_superuser_with_static_device_but_without_totp_device(self):
+        from django_otp.plugins.otp_static.models import StaticDevice, StaticToken
+
+        user = UserFactory.build(is_staff=True, is_superuser=False)
+        user.save()
+        self.assertFalse(user.is_superuser)
+
+        device = StaticDevice.objects.create(user=user, name="backup", confirmed=True)
+        StaticToken.objects.create(device=device, token="123456")
+
+        user.is_superuser = True
+        self.assertRaises(ValidationError, user.save)
+
+    def test_can_save_superuser_with_totp_device(self):
+        from django_otp.plugins.otp_totp.models import TOTPDevice
+
+        user = UserFactory.build(is_staff=True, is_superuser=False)
+        user.save()
+        self.assertFalse(user.is_superuser)
+
+        TOTPDevice.objects.create(user=user, name="test-device", confirmed=True)
+
+        user.is_superuser = True
+        user.save()
+        self.assertTrue(user.is_superuser)

data/validators/user.py

@@ -0,0 +1,47 @@
+from common.utils import utils as utils_utils
+
+
+def validate_user_non_staff(instance):
+    """
+    - extra validation:
+        - a user cannot have a TOTP device if they are not staff
+    """
+    errors = {}
+    if not instance.is_staff:
+        if instance.pk:
+            if instance.totpdevice_set.exists():
+                utils_utils.add_validation_error(
+                    errors,
+                    "is_staff",
+                    "Un utilisateur ne peut pas avoir d'appareil 2FA (OTP) s'il n'est pas staff.",
+                )
+    return errors
+
+
+def validate_user_superuser(instance):
+    """
+    - extra validation:
+        - a user cannot become superuser if they are not staff
+        - a user cannot become superuser if they don't have a confirmed TOTP device
+    """
+    errors = {}
+    if instance.is_superuser:
+        if not instance.is_staff:
+            utils_utils.add_validation_error(
+                errors,
+                "is_superuser",
+                "Un utilisateur ne peut pas devenir superuser s'il n'est pas staff.",
+            )
+        if not instance.pk:
+            utils_utils.add_validation_error(
+                errors,
+                "is_superuser",
+                "Créer d'abord un utilisateur staff, puis configurer un appareil 2FA (OTP), avant de pouvoir devenir superuser.",
+            )
+        elif not instance.totpdevice_set.filter(confirmed=True).exists():
+            utils_utils.add_validation_error(
+                errors,
+                "is_superuser",
+                "Un utilisateur ne peut devenir superuser que s'il a déjà configuré un appareil 2FA (OTP).",
+            )
+    return errors

macantine/admin.py

@@ -1,6 +1,6 @@
 from django.contrib import admin
-from django_otp.admin import OTPAdminAuthenticationForm, OTPAdminSite
 from django.urls import path, reverse
+from django_otp.admin import OTPAdminAuthenticationForm, OTPAdminSite
 
 from data.admin.sector import sector_textchoices_admin_view
 from data.admin.textchoices import CANTEEN_TEXTCHOICES_PAGES, canteen_textchoices_admin_view

macantine/tests/test_admin.py

@@ -7,7 +7,6 @@
 
 from data.factories import UserFactory
 
-
 User = get_user_model()
 
 
@@ -20,13 +19,6 @@ def setUp(self):
             is_staff=True,
             is_superuser=False,
         )
-        self.superuser_no_otp = UserFactory(
-            email="superuser@example.com",
-            first_name="Super",
-            last_name="User",
-            is_staff=True,
-            is_superuser=True,
-        )
         self.user_no_staff_not_superuser = UserFactory(
             email="nonstaff@example.com",
             first_name="Non",
@@ -55,7 +47,11 @@ def test_staff_non_superuser_without_otp_can_access_admin(self):
         self.assertEqual(response.status_code, 200)
 
     def test_superuser_without_otp_redirected_to_login(self):
-        self.client.force_login(self.superuser_no_otp)
+        # Set user as superuser
+        self.staff_not_superuser_no_otp.is_superuser = True
+        self.staff_not_superuser_no_otp.save(skip_validations=True)
+
+        self.client.force_login(self.staff_not_superuser_no_otp)
         response = self.client.get(reverse("admin:index"))
 
         self.assertEqual(response.status_code, 302)
@@ -81,15 +77,16 @@ def test_staff_non_superuser_can_login_without_otp(self):
         self.assertEqual(final_response.status_code, 200)
 
     def test_staff_user_with_otp_can_access_admin(self):
-        # Set user password & device
-        self.staff_not_superuser_no_otp.set_password("testPw1234#!")
-        self.staff_not_superuser_no_otp.save(update_fields=["password"])
+        # Add totp device
         device = TOTPDevice.objects.create(
             user=self.staff_not_superuser_no_otp,
             name="test",
             confirmed=True,
         )
         totp_code = totp(device.bin_key, device.step, device.t0)
+        # set user password
+        self.staff_not_superuser_no_otp.set_password("testPw1234#!")
+        self.staff_not_superuser_no_otp.save(update_fields=["password"])
 
         response = self.client.post(
             reverse("admin:login"),
@@ -110,20 +107,22 @@ def test_staff_user_with_otp_can_access_admin(self):
         self.assertEqual(final_response.status_code, 200)
 
     def test_superuser_with_otp_can_access_admin(self):
-        # Set user password & device
-        self.superuser_no_otp.set_password("testPw1234#!")
-        self.superuser_no_otp.save(update_fields=["password"])
+        # Add totp device
         device = TOTPDevice.objects.create(
-            user=self.superuser_no_otp,
+            user=self.staff_not_superuser_no_otp,
             name="test",
             confirmed=True,
         )
         totp_code = totp(device.bin_key, device.step, device.t0)
+        # set user as superuser & password
+        self.staff_not_superuser_no_otp.is_superuser = True
+        self.staff_not_superuser_no_otp.set_password("testPw1234#!")
+        self.staff_not_superuser_no_otp.save(update_fields=["password"])
 
         response = self.client.post(
             reverse("admin:login"),
             {
-                "username": self.superuser_no_otp.username,
+                "username": self.staff_not_superuser_no_otp.username,
                 "password": "testPw1234#!",
                 "otp_token": totp_code,
                 "next": reverse("admin:index"),
@@ -139,11 +138,12 @@ def test_superuser_with_otp_can_access_admin(self):
         self.assertEqual(final_response.status_code, 200)
 
     def test_staff_user_with_static_token_can_access_admin(self):
-        # Set user password & backup device
-        self.staff_not_superuser_no_otp.set_password("testPw1234#!")
-        self.staff_not_superuser_no_otp.save(update_fields=["password"])
+        # Add static device
         device = StaticDevice.objects.create(user=self.staff_not_superuser_no_otp, name="backup", confirmed=True)
         static_token = StaticToken.objects.create(device=device, token="123456")
+        # set user password
+        self.staff_not_superuser_no_otp.set_password("testPw1234#!")
+        self.staff_not_superuser_no_otp.save(update_fields=["password"])
 
         response = self.client.post(
             reverse("admin:login"),
@@ -164,16 +164,18 @@ def test_staff_user_with_static_token_can_access_admin(self):
         self.assertEqual(final_response.status_code, 200)
 
     def test_superuser_with_static_token_can_access_admin(self):
-        # Set user password & backup device
-        self.superuser_no_otp.set_password("testPw1234#!")
-        self.superuser_no_otp.save(update_fields=["password"])
-        device = StaticDevice.objects.create(user=self.superuser_no_otp, name="backup", confirmed=True)
+        # Add static device
+        device = StaticDevice.objects.create(user=self.staff_not_superuser_no_otp, name="backup", confirmed=True)
         static_token = StaticToken.objects.create(device=device, token="123456")
+        # set user as superuser & password
+        self.staff_not_superuser_no_otp.is_superuser = True
+        self.staff_not_superuser_no_otp.set_password("testPw1234#!")
+        self.staff_not_superuser_no_otp.save(update_fields=["password"], skip_validations=True)
 
         response = self.client.post(
             reverse("admin:login"),
             {
-                "username": self.superuser_no_otp.username,
+                "username": self.staff_not_superuser_no_otp.username,
                 "password": "testPw1234#!",
                 "otp_token": static_token.token,
                 "next": reverse("admin:index"),
@@ -200,15 +202,15 @@ def test_login_url_not_affected_by_global_setting(self):
 
 class MaCantineAdminSiteCustomUrlsTest(TestCase):
     def setUp(self):
-        self.staff_not_superuser_no_otp = UserFactory(
+        self.staff_not_superuser = UserFactory(
             email="staff@example.com",
             first_name="Staff",
             last_name="User",
             is_staff=True,
-            is_superuser=True,
+            is_superuser=False,
         )
-        device = TOTPDevice.objects.create(user=self.staff_not_superuser_no_otp, name="test", confirmed=True)
-        self.client.force_login(self.staff_not_superuser_no_otp)
+        device = TOTPDevice.objects.create(user=self.staff_not_superuser, name="test", confirmed=True)
+        self.client.force_login(self.staff_not_superuser)
         # Mark the OTP device as verified for this session, as django_otp.login() would
         session = self.client.session
         session["otp_device_id"] = device.persistent_id
@@ -223,6 +225,10 @@ def test_custom_urls_registered_canteen(self):
         self.assertEqual(response.status_code, 200)
 
     def test_synthetic_data_models_in_app_list(self):
+        # user needs to be superuser to see synthetic models in app list
+        self.staff_not_superuser.is_superuser = True
+        self.staff_not_superuser.save()
+
         response = self.client.get(reverse("admin:index"))
 
         self.assertEqual(response.status_code, 200)