Fix otp (#305)

* install latest py package

* bulk anonymize students

* fix test cases

* fix permissions

* new python package

* fix enable/disable otp

* merge from dev

* new py package

* feedback

* house keeping

backend/Pipfile

@@ -7,7 +7,7 @@ name = "pypi"
 # Before adding a new package, check it's not listed under [packages] at
 #   https://github.com/ocadotechnology/codeforlife-package-python/blob/{ref}/Pipfile
 # Replace "{ref}" in the above URL with the ref set below.
-codeforlife = {ref = "v0.13.6", git = "https://github.com/ocadotechnology/codeforlife-package-python.git"}
+codeforlife = {ref = "v0.13.7", git = "https://github.com/ocadotechnology/codeforlife-package-python.git"}
 # TODO: check if we need the below packages
 whitenoise = "==6.5.0"
 django-pipeline = "==2.0.8"
@@ -34,7 +34,7 @@ google-cloud-container = "==2.3.0"
 # Before adding a new package, check it's not listed under [dev-packages] at
 #   https://github.com/ocadotechnology/codeforlife-package-python/blob/{ref}/Pipfile
 # Replace "{ref}" in the above URL with the ref set below.
-codeforlife = {ref = "v0.13.6", git = "https://github.com/ocadotechnology/codeforlife-package-python.git", extras = ["dev"]}
+codeforlife = {ref = "v0.13.7", git = "https://github.com/ocadotechnology/codeforlife-package-python.git", extras = ["dev"]}
 # TODO: check if we need the below packages
 django-selenium-clean = "==0.3.3"
 django-test-migrations = "==1.2.0"

backend/api/fixtures/school_2.json

@@ -24,7 +24,16 @@
     "pk": 25,
     "fields": {
       "user": 25,
-      "is_verified": true
+      "is_verified": true,
+      "otp_secret": "KI6FA34LPRQU265KQBFYS2MTDYHE2EIG"
+    }
+  },
+  {
+    "model": "user.authfactor",
+    "pk": 1,
+    "fields": {
+      "user": 25,
+      "type": "otp"
     }
   },
   {
@@ -61,7 +70,16 @@
     "pk": 26,
     "fields": {
       "user": 26,
-      "is_verified": true
+      "is_verified": true,
+      "otp_secret": "KI6FA34LPRQU265KQBFYS2MTDYHE2EIG"
+    }
+  },
+  {
+    "model": "user.authfactor",
+    "pk": 2,
+    "fields": {
+      "user": 26,
+      "type": "otp"
     }
   },
   {

backend/api/serializers/auth_factor.py

@@ -3,9 +3,10 @@
 Created on 23/01/2024 at 11:05:37(+00:00).
 """
 
-from codeforlife.request import Request
+import pyotp
 from codeforlife.serializers import ModelSerializer
 from codeforlife.user.models import AuthFactor
+from rest_framework import serializers
 
 
 # pylint: disable-next=missing-class-docstring
@@ -20,7 +21,36 @@ class Meta:
             "id": {"read_only": True},
         }
 
+    # pylint: disable-next=missing-function-docstring
+    def validate_type(self, value: str):
+        if AuthFactor.objects.filter(
+            user=self.request_user, type=value
+        ).exists():
+            raise serializers.ValidationError(
+                "You already have this authentication factor enabled.",
+                code="already_exists",
+            )
+
+        return value
+
     def create(self, validated_data):
-        request: Request = self.context["request"]
-        validated_data["user"] = request.user
+        user = self.request_user
+        if not user.userprofile.otp_secret:
+            user.userprofile.otp_secret = pyotp.random_base32()
+            user.userprofile.save()
+
+        validated_data["user"] = user
         return super().create(validated_data)
+
+    def to_representation(self, instance):
+        representation = super().to_representation(instance)
+
+        if (
+            self.view.action == "create"
+            and instance.type == AuthFactor.Type.OTP
+        ):
+            representation[
+                "totp_provisioning_uri"
+            ] = self.request_user.totp_provisioning_uri
+
+        return representation

backend/api/tests/serializers/test_auth_factor.py

@@ -0,0 +1,92 @@
+"""
+© Ocado Group
+Created on 15/02/2024 at 15:44:25(+00:00).
+"""
+
+from unittest.mock import patch
+
+import pyotp
+from codeforlife.tests import ModelSerializerTestCase
+from codeforlife.user.models import AuthFactor, TeacherUser
+
+from ...serializers import AuthFactorSerializer
+from ...views import AuthFactorViewSet
+
+
+# pylint: disable-next=missing-class-docstring
+class TestAuthFactorSerializer(ModelSerializerTestCase[AuthFactor]):
+    model_serializer_class = AuthFactorSerializer
+    fixtures = ["school_2", "non_school_teacher"]
+
+    def setUp(self):
+        self.multi_auth_factor_teacher_user = TeacherUser.objects.get(
+            email="[email protected]"
+        )
+        assert self.multi_auth_factor_teacher_user.auth_factors.exists()
+
+        self.uni_auth_factor_teacher_user = TeacherUser.objects.get(
+            email="[email protected]"
+        )
+        assert not self.uni_auth_factor_teacher_user.auth_factors.exists()
+
+    # test: validate
+
+    def test_validate_type__already_exists(self):
+        """Cannot enable an already enabled auth factor."""
+        auth_factor = self.multi_auth_factor_teacher_user.auth_factors.first()
+        assert auth_factor
+
+        self.assert_validate_field(
+            "type",
+            auth_factor.type,
+            error_code="already_exists",
+            context={
+                "request": self.request_factory.post(
+                    user=self.multi_auth_factor_teacher_user
+                )
+            },
+        )
+
+    # test: create
+
+    def test_create(self):
+        """Enabling OTP will ensure the user's OTP-secret is set."""
+        assert not self.uni_auth_factor_teacher_user.otp_secret
+
+        otp_secret = pyotp.random_base32()
+        with patch.object(pyotp, "random_base32", return_value=otp_secret):
+            self.assert_create(
+                validated_data={"type": AuthFactor.Type.OTP},
+                context={
+                    "request": self.request_factory.post(
+                        user=self.uni_auth_factor_teacher_user
+                    )
+                },
+                new_data={
+                    "user": {
+                        "id": self.uni_auth_factor_teacher_user.id,
+                        "userprofile": {"otp_secret": otp_secret},
+                    },
+                },
+            )
+
+    # test: to representation
+
+    def test_to_representation(self):
+        """User's TOTP provisioning URI is returned when enabling OTP."""
+        assert self.multi_auth_factor_teacher_user.otp_secret
+
+        self.assert_to_representation(
+            instance=AuthFactor(type=AuthFactor.Type.OTP),
+            new_data={
+                "totp_provisioning_uri": (
+                    self.multi_auth_factor_teacher_user.totp_provisioning_uri
+                ),
+            },
+            context={
+                "view": AuthFactorViewSet(action="create"),
+                "request": self.request_factory.post(
+                    user=self.multi_auth_factor_teacher_user
+                ),
+            },
+        )

backend/api/tests/views/test_auth_factor.py

@@ -3,9 +3,10 @@
 Created on 23/01/2024 at 11:22:16(+00:00).
 """
 
+from codeforlife.permissions import AllowNone
 from codeforlife.tests import ModelViewSetTestCase
-from codeforlife.user.models import AuthFactor, User, UserProfile
-from rest_framework import status
+from codeforlife.user.models import AuthFactor, TeacherUser
+from codeforlife.user.permissions import IsTeacher
 
 from ...views import AuthFactorViewSet
 
@@ -14,79 +15,73 @@
 class TestAuthFactorViewSet(ModelViewSetTestCase[AuthFactor]):
     basename = "auth-factor"
     model_view_set_class = AuthFactorViewSet
+    fixtures = ["school_2", "non_school_teacher"]
 
     def setUp(self):
-        self.one_factor_credentials = {
-            "email": "[email protected]",
-            "password": "password",
-        }
-        self.one_factor_user = User.objects.create_user(
-            first_name="One",
-            last_name="Factor",
-            username=self.one_factor_credentials["email"],
-            **self.one_factor_credentials,
+        self.multi_auth_factor_teacher_user = TeacherUser.objects.get(
+            email="[email protected]"
         )
-        UserProfile.objects.create(user=self.one_factor_user)
-
-        self.two_factor_credentials = {
-            "email": "[email protected]",
-            "password": "password",
-        }
-        self.two_factor_user = User.objects.create_user(
-            first_name="Two",
-            last_name="Factor",
-            username=self.two_factor_credentials["email"],
-            **self.two_factor_credentials,
+        assert self.multi_auth_factor_teacher_user.auth_factors.exists()
+
+        self.uni_auth_factor_teacher_user = TeacherUser.objects.get(
+            email="[email protected]"
         )
-        UserProfile.objects.create(user=self.two_factor_user)
-        self.two_auth_factor = AuthFactor.objects.create(
-            user=self.two_factor_user,
-            type=AuthFactor.Type.OTP,
+        assert not self.uni_auth_factor_teacher_user.auth_factors.exists()
+
+    # test: get queryset
+
+    def test_get_queryset(self):
+        """Can only access your own auth factors."""
+        self.assert_get_queryset(
+            list(self.multi_auth_factor_teacher_user.auth_factors.all()),
+            request=self.client.request_factory.get(
+                user=self.multi_auth_factor_teacher_user
+            ),
         )
 
-    def test_retrieve(self):
-        """
-        Retrieving a single auth factor is forbidden.
-        """
+    # test: get permissions
 
-        user = self.client.login(**self.two_factor_credentials)
-        assert user == self.two_factor_user
+    def test_get_permissions__bulk(self):
+        """Cannot perform any bulk action."""
+        self.assert_get_permissions([AllowNone()], action="bulk")
 
-        self.client.retrieve(self.two_auth_factor, status.HTTP_403_FORBIDDEN)
+    def test_get_permissions__retrieve(self):
+        """Cannot retrieve a single auth factor."""
+        self.assert_get_permissions([AllowNone()], action="retrieve")
 
-    def test_list(self):
-        """
-        Can list enabled auth-factors.
-        """
-
-        user = self.client.login(**self.two_factor_credentials)
-        assert user == self.two_factor_user
-
-        # Need to have another two auth-factor user to ensure some data is
-        # filtered out.
-        AuthFactor.objects.create(
-            user=self.one_factor_user,
-            type=AuthFactor.Type.OTP,
-        )
+    def test_get_permissions__list(self):
+        """Only a teacher-user can list all auth factors."""
+        self.assert_get_permissions([IsTeacher()], action="list")
 
-        self.client.list([self.two_auth_factor])
+    def test_get_permissions__create(self):
+        """Only a teacher-user can enable an auth factor."""
+        self.assert_get_permissions([IsTeacher()], action="create")
 
-    def test_create(self):
-        """
-        Can enable an auth-factor.
-        """
+    def test_get_permissions__destroy(self):
+        """Only a teacher-user can disable an auth factor."""
+        self.assert_get_permissions([IsTeacher()], action="destroy")
+
+    # test: generic actions
+
+    def test_list(self):
+        """Can list enabled auth-factors."""
+        self.client.login_as(self.multi_auth_factor_teacher_user)
+
+        self.client.list(
+            list(self.multi_auth_factor_teacher_user.auth_factors.all())
+        )
 
-        user = self.client.login(**self.one_factor_credentials)
-        assert user == self.one_factor_user
+    def test_create__otp(self):
+        """Can enable OTP."""
+        self.client.login_as(self.uni_auth_factor_teacher_user)
 
         self.client.create({"type": "otp"})
 
     def test_destroy(self):
-        """
-        Can disable an auth-factor.
-        """
+        """Can disable an auth-factor."""
+        self.client.login_as(self.multi_auth_factor_teacher_user)
 
-        user = self.client.login(**self.two_factor_credentials)
-        assert user == self.two_factor_user
+        auth_factor = self.multi_auth_factor_teacher_user.auth_factors.first()
+        assert auth_factor
 
-        self.client.destroy(self.two_auth_factor)
+        self.client.destroy(auth_factor)

backend/api/views/auth_factor.py

@@ -4,7 +4,8 @@
 """
 
 from codeforlife.permissions import AllowNone
-from codeforlife.user.models import AuthFactor, User
+from codeforlife.user.models import AuthFactor
+from codeforlife.user.permissions import IsTeacher
 from codeforlife.views import ModelViewSet
 
 from ..serializers import AuthFactorSerializer
@@ -16,11 +17,10 @@ class AuthFactorViewSet(ModelViewSet[AuthFactor]):
     serializer_class = AuthFactorSerializer
 
     def get_queryset(self):
-        user: User = self.request.user  # type: ignore[assignment]
-        return AuthFactor.objects.filter(user=user)
+        return AuthFactor.objects.filter(user=self.request_user)
 
     def get_permissions(self):
-        if self.action == "retrieve":
+        if self.action in ["retrieve", "bulk"]:
             return [AllowNone()]
 
-        return super().get_permissions()
+        return [IsTeacher()]