From 58f563fe4571eb0c0d04843d7302243697601aef Mon Sep 17 00:00:00 2001 From: Stefan Kairinos Date: Mon, 20 Jan 2025 16:22:25 +0000 Subject: [PATCH] fix: otp flow (#388) * fix: otp flow * fix: new cfl package * fix: create auth factor --- Pipfile | 4 +- Pipfile.lock | 14 ++-- src/api/serializers/auth_factor.py | 41 ++++++---- src/api/serializers/auth_factor_test.py | 76 ++++++++++------- src/api/views/auth_factor.py | 23 +++++- src/api/views/auth_factor_test.py | 104 ++++++++++++++++++++++-- 6 files changed, 194 insertions(+), 68 deletions(-) diff --git a/Pipfile b/Pipfile index 90854507..c4ec7096 100644 --- a/Pipfile +++ b/Pipfile @@ -23,7 +23,7 @@ name = "pypi" # 5. Run `pipenv install --dev` in your terminal. [packages] -codeforlife = "==0.24.14" +codeforlife = "==0.24.15" # 🚫 Don't add [packages] below that are inherited from the CFL package. pyjwt = "==2.6.0" # TODO: upgrade to latest version # TODO: Needed by RR. Remove when RR has moved to new system. @@ -32,7 +32,7 @@ django-sekizai = "==4.1.0" django-classy-tags = "==4.1.0" [dev-packages] -codeforlife = {version = "==0.24.14", extras = ["dev"]} +codeforlife = {version = "==0.24.15", extras = ["dev"]} # codeforlife = {file = "../codeforlife-package-python", editable = true, extras = ["dev"]} # 🚫 Don't add [dev-packages] below that are inherited from the CFL package. diff --git a/Pipfile.lock b/Pipfile.lock index 17efb61a..c1ccf2fc 100644 --- a/Pipfile.lock +++ b/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "6c93e19ede94573a52660e42e4f6bc54d9f275e80be39c665cabea6186fa019e" + "sha256": "f7e8f7052f817cf600c8de439655e0b3388de89537c87572d73575a9c90c0551" }, "pipfile-spec": 6, "requires": { @@ -163,12 +163,12 @@ }, "codeforlife": { "hashes": [ - "sha256:24d2dfb26df0afd0998a2c551fd7b315dea91cfbf0d6eeaf4009917caf2a3c60", - "sha256:d10e84c5e45e2d8fe8068e25ddffd3ef7839c95b6a657afa96fd49813824d170" + "sha256:50e18f550077daea4a39279ecae3e59394a812461e77bc5d1b22e09a5e7f707d", + "sha256:e339947deebe25fa9e274c66641168a92c2a64c08459369379f7cda5a07b3efc" ], "index": "pypi", "markers": "python_version == '3.12'", - "version": "==0.24.14" + "version": "==0.24.15" }, "codeforlife-portal": { "hashes": [ @@ -1168,12 +1168,12 @@ }, "codeforlife": { "hashes": [ - "sha256:24d2dfb26df0afd0998a2c551fd7b315dea91cfbf0d6eeaf4009917caf2a3c60", - "sha256:d10e84c5e45e2d8fe8068e25ddffd3ef7839c95b6a657afa96fd49813824d170" + "sha256:50e18f550077daea4a39279ecae3e59394a812461e77bc5d1b22e09a5e7f707d", + "sha256:e339947deebe25fa9e274c66641168a92c2a64c08459369379f7cda5a07b3efc" ], "index": "pypi", "markers": "python_version == '3.12'", - "version": "==0.24.14" + "version": "==0.24.15" }, "codeforlife-portal": { "hashes": [ diff --git a/src/api/serializers/auth_factor.py b/src/api/serializers/auth_factor.py index 6069fa8f..cad99781 100644 --- a/src/api/serializers/auth_factor.py +++ b/src/api/serializers/auth_factor.py @@ -3,7 +3,8 @@ Created on 23/01/2024 at 11:05:37(+00:00). """ -import pyotp +import re + from codeforlife.serializers import ModelSerializer from codeforlife.user.models import AuthFactor, User from rest_framework import serializers @@ -11,11 +12,14 @@ # pylint: disable-next=missing-class-docstring,too-many-ancestors class AuthFactorSerializer(ModelSerializer[User, AuthFactor]): + otp = serializers.CharField(required=False, write_only=True) + class Meta: model = AuthFactor fields = [ "id", "type", + "otp", ] extra_kwargs = { "id": {"read_only": True}, @@ -33,24 +37,25 @@ def validate_type(self, value: str): return value - def create(self, validated_data): - user = self.request.auth_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) + # pylint: disable-next=missing-function-docstring + def validate_otp(self, value: str): + if not re.match(r"^[0-9]{6}$", value): + raise serializers.ValidationError("Must be 6 digits", code="format") + if not self.request.auth_user.totp.verify(value): + raise serializers.ValidationError("Invalid OTP.", code="invalid") - def to_representation(self, instance): - representation = super().to_representation(instance) + return value - if ( - self.view.action == "create" - and instance.type == AuthFactor.Type.OTP - ): - representation["totp_provisioning_uri"] = ( - self.request.auth_user.totp_provisioning_uri + def validate(self, attrs): + if attrs["type"] == "otp" and "otp" not in attrs: + raise serializers.ValidationError( + "Current OTP required to enable OTP.", + code="otp__required", ) - return representation + return attrs + + def create(self, validated_data): + validated_data["user_id"] = self.request.auth_user.id + validated_data.pop("otp", None) + return super().create(validated_data) diff --git a/src/api/serializers/auth_factor_test.py b/src/api/serializers/auth_factor_test.py index f322de1a..a52b3e45 100644 --- a/src/api/serializers/auth_factor_test.py +++ b/src/api/serializers/auth_factor_test.py @@ -3,10 +3,11 @@ Created on 15/02/2024 at 15:44:25(+00:00). """ +from unittest.mock import Mock, patch + from codeforlife.tests import ModelSerializerTestCase from codeforlife.user.models import AuthFactor, TeacherUser, User -from ..views import AuthFactorViewSet from .auth_factor import AuthFactorSerializer # pylint: disable=missing-class-docstring @@ -46,42 +47,55 @@ def test_validate_type__already_exists(self): }, ) - # test: create - - def test_create(self): - """Enabling OTP will ensure the user's OTP-secret is set.""" - user = self.uni_auth_factor_teacher_user - assert user.otp_secret - - self.assert_create( - validated_data={"type": AuthFactor.Type.OTP}, - context={"request": self.request_factory.post(user=user)}, - new_data={ - "user": { - "id": user.id, - "userprofile": {"otp_secret": user.otp_secret}, - }, - }, + def test_validate_otp__format(self): + """OTP must be 6 digits.""" + self.assert_validate_field( + name="otp", + value="12345", + error_code="format", ) - # test: to representation + @patch("codeforlife.user.models.user.TOTP.verify", return_value=False) + def test_validate_otp__invalid(self, totp__verify: Mock): + """Cannot enable the OTP without providing the current OTP.""" + user = TeacherUser.objects.filter( + # TODO: make otp_secret non-nullable + userprofile__otp_secret__isnull=False + ).first() + assert user - def test_to_representation(self): - """User's TOTP provisioning URI is returned when enabling OTP.""" - assert self.multi_auth_factor_teacher_user.otp_secret + value = "123456" - self.assert_to_representation( - instance=AuthFactor(type=AuthFactor.Type.OTP), - new_data={ - "totp_provisioning_uri": ( - self.multi_auth_factor_teacher_user.totp_provisioning_uri - ), - }, + self.assert_validate_field( + name="otp", + value=value, + error_code="invalid", context={ - "view": AuthFactorViewSet(action="create"), "request": self.request_factory.post( user=self.multi_auth_factor_teacher_user - ), + ) }, - non_model_fields={"totp_provisioning_uri"}, + ) + + totp__verify.assert_called_once_with(value) + + def test_validate__otp__required(self): + """Current OTP is required when enabling OTP.""" + self.assert_validate( + attrs={"type": AuthFactor.Type.OTP.value}, + error_code="otp__required", + ) + + def test_create__otp(self): + """Can successfully enable an auth factor.""" + user = TeacherUser.objects.exclude( + auth_factors__type__in=["otp"] + ).first() + assert user + + self.assert_create( + validated_data={"type": AuthFactor.Type.OTP, "otp": "123456"}, + non_model_fields={"otp"}, + new_data={"user": user.id}, + context={"request": self.request_factory.post(user=user)}, ) diff --git a/src/api/views/auth_factor.py b/src/api/views/auth_factor.py index 765c56b1..5b106455 100644 --- a/src/api/views/auth_factor.py +++ b/src/api/views/auth_factor.py @@ -3,10 +3,13 @@ Created on 23/01/2024 at 11:04:44(+00:00). """ +import pyotp from codeforlife.permissions import AllowNone +from codeforlife.request import Request +from codeforlife.response import Response from codeforlife.user.models import AuthFactor, User from codeforlife.user.permissions import IsTeacher -from codeforlife.views import ModelViewSet +from codeforlife.views import ModelViewSet, action from ..serializers import AuthFactorSerializer @@ -22,7 +25,12 @@ class AuthFactorViewSet(ModelViewSet[User, AuthFactor]): def get_queryset(self): queryset = AuthFactor.objects.all() user = self.request.teacher_user - if user.teacher.school and user.teacher.is_admin: + + if ( + self.action in ["list", "destroy"] + and user.teacher.school + and user.teacher.is_admin + ): return queryset.filter( user__new_teacher__school=user.teacher.school ) @@ -35,3 +43,14 @@ def get_permissions(self): return [AllowNone()] return [IsTeacher()] + + @action(detail=False, methods=["post"]) + def generate_otp_provisioning_uri(self, request: Request[User]): + """Generate a time-based one-time-password provisioning URI.""" + # TODO: make otp_secret non-nullable and delete code block + user = request.auth_user + if not user.userprofile.otp_secret: + user.userprofile.otp_secret = pyotp.random_base32() + user.userprofile.save(update_fields=["otp_secret"]) + + return Response(user.totp_provisioning_uri, content_type="text/plain") diff --git a/src/api/views/auth_factor_test.py b/src/api/views/auth_factor_test.py index a23f184a..16f9bf6c 100644 --- a/src/api/views/auth_factor_test.py +++ b/src/api/views/auth_factor_test.py @@ -3,6 +3,9 @@ Created on 23/01/2024 at 11:22:16(+00:00). """ +from unittest.mock import patch + +import pyotp from codeforlife.permissions import AllowNone from codeforlife.tests import ModelViewSetTestCase from codeforlife.user.models import ( @@ -13,6 +16,7 @@ User, ) from codeforlife.user.permissions import IsTeacher +from pyotp import TOTP from .auth_factor import AuthFactorViewSet @@ -33,9 +37,41 @@ def setUp(self): # test: get queryset - def test_get_queryset__admin(self): + def test_get_queryset__list__admin(self): + """ + Can list the author factors of all teachers in your school if you are + an admin. + """ + user = self.mfa_non_admin_school_teacher_user + admin_school_teacher_user = AdminSchoolTeacherUser.objects.filter( + new_teacher__school=user.teacher.school + ).first() + assert admin_school_teacher_user + + self.assert_get_queryset( + action="list", + values=list( + user.auth_factors.all() + | admin_school_teacher_user.auth_factors.all() + ), + request=self.client.request_factory.get( + user=admin_school_teacher_user + ), + ) + + def test_get_queryset__list__non_admin(self): + """Can only list your own auth factors if you are not an admin.""" + user = self.mfa_non_admin_school_teacher_user + + self.assert_get_queryset( + action="list", + values=list(user.auth_factors.all()), + request=self.client.request_factory.get(user=user), + ) + + def test_get_queryset__destroy__admin(self): """ - Can query the author factors of all teachers in your school if you are + Can destroy the author factors of all teachers in your school if you are an admin. """ user = self.mfa_non_admin_school_teacher_user @@ -45,6 +81,7 @@ def test_get_queryset__admin(self): assert admin_school_teacher_user self.assert_get_queryset( + action="destroy", values=list( user.auth_factors.all() | admin_school_teacher_user.auth_factors.all() @@ -54,11 +91,22 @@ def test_get_queryset__admin(self): ), ) - def test_get_queryset__non_admin(self): - """Can only query your own auth factors if you are not an admin.""" + def test_get_queryset__destroy__non_admin(self): + """Can only destroy your own auth factors if you are not an admin.""" + user = self.mfa_non_admin_school_teacher_user + + self.assert_get_queryset( + action="destroy", + values=list(user.auth_factors.all()), + request=self.client.request_factory.get(user=user), + ) + + def test_get_queryset__generate_otp_provisioning_uri(self): + """Can only generate an OTP provisioning URI yourself.""" user = self.mfa_non_admin_school_teacher_user self.assert_get_queryset( + action="generate_otp_provisioning_uri", values=list(user.auth_factors.all()), request=self.client.request_factory.get(user=user), ) @@ -85,7 +133,13 @@ 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_get_permissions__generate_otp_provisioning_uri(self): + """Only a teacher-user can generate a OTP provisioning URI.""" + self.assert_get_permissions( + [IsTeacher()], action="generate_otp_provisioning_uri" + ) + + # test: actions def test_list(self): """Can list enabled auth-factors.""" @@ -96,14 +150,23 @@ def test_list(self): def test_create__otp(self): """Can enable OTP.""" - teacher_user = TeacherUser.objects.filter( - auth_factors__isnull=True + teacher_user = TeacherUser.objects.exclude( + auth_factors__type__in=["otp"] ).first() assert teacher_user + # TODO: make "otp_secret" non-nullable and delete code block + teacher_user.userprofile.otp_secret = pyotp.random_base32() + teacher_user.userprofile.save(update_fields=["otp_secret"]) + # TODO: set password="password" on all user fixtures self.client.login_as(teacher_user, password="abc123") - self.client.create({"type": "otp"}) + self.client.create( + { + "type": AuthFactor.Type.OTP, + "otp": teacher_user.totp.now(), + } + ) def test_destroy(self): """Can disable an auth-factor.""" @@ -113,3 +176,28 @@ def test_destroy(self): self.client.login_as(user) self.client.destroy(auth_factor) + + def test_generate_otp_provisioning_uri(self): + """Can successfully generate a OTP provisioning URI.""" + user = TeacherUser.objects.exclude( + auth_factors__type__in=[AuthFactor.Type.OTP] + ).first() + assert user + + # TODO: normalize password to "password" + self.client.login_as(user, password="abc123") + + with patch.object( + TOTP, "provisioning_uri", return_value=user.totp_provisioning_uri + ) as provisioning_uri: + response = self.client.post( + self.reverse_action("generate_otp_provisioning_uri") + ) + + provisioning_uri.assert_called_once_with( + name=user.email, + issuer_name="Code for Life", + ) + + assert response.data == provisioning_uri.return_value + assert response.content_type == "text/plain"