From a22d4f9fd327dd86f25b95c12510cf27326813ed Mon Sep 17 00:00:00 2001 From: Stefan Kairinos Date: Fri, 16 Feb 2024 18:38:55 +0000 Subject: [PATCH] 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 | 4 +- backend/Pipfile.lock | 6 +- backend/api/fixtures/school_2.json | 22 +++- backend/api/serializers/auth_factor.py | 36 +++++- .../api/tests/serializers/test_auth_factor.py | 92 ++++++++++++++ backend/api/tests/views/test_auth_factor.py | 115 +++++++++--------- backend/api/views/auth_factor.py | 10 +- 7 files changed, 210 insertions(+), 75 deletions(-) create mode 100644 backend/api/tests/serializers/test_auth_factor.py diff --git a/backend/Pipfile b/backend/Pipfile index b0507c23..73e310ce 100644 --- a/backend/Pipfile +++ b/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" diff --git a/backend/Pipfile.lock b/backend/Pipfile.lock index 390b76b6..d8f3d366 100644 --- a/backend/Pipfile.lock +++ b/backend/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "87409c54e5f41ceef8b81accc69edfa6fc7fd474d9c22c4e0d018938b57b52a1" + "sha256": "9e02bb3bd37675c13e20e4e4add09570c4ead5526ab3650ee2a3d47864023155" }, "pipfile-spec": 6, "requires": { @@ -168,7 +168,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "9fd3549f852745032d9d43174ec2f25ae29b6f8f" + "ref": "30953150f4d2762e421d8f798badb57ad0318fd6" }, "codeforlife-portal": { "hashes": [ @@ -1512,7 +1512,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "9fd3549f852745032d9d43174ec2f25ae29b6f8f" + "ref": "30953150f4d2762e421d8f798badb57ad0318fd6" }, "codeforlife-portal": { "hashes": [ diff --git a/backend/api/fixtures/school_2.json b/backend/api/fixtures/school_2.json index 9c4074b8..7e6e5dae 100644 --- a/backend/api/fixtures/school_2.json +++ b/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" } }, { diff --git a/backend/api/serializers/auth_factor.py b/backend/api/serializers/auth_factor.py index e04f5299..09b5ad20 100644 --- a/backend/api/serializers/auth_factor.py +++ b/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 diff --git a/backend/api/tests/serializers/test_auth_factor.py b/backend/api/tests/serializers/test_auth_factor.py new file mode 100644 index 00000000..917a306b --- /dev/null +++ b/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="teacher@school2.com" + ) + assert self.multi_auth_factor_teacher_user.auth_factors.exists() + + self.uni_auth_factor_teacher_user = TeacherUser.objects.get( + email="teacher@noschool.com" + ) + 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 + ), + }, + ) diff --git a/backend/api/tests/views/test_auth_factor.py b/backend/api/tests/views/test_auth_factor.py index d9aabad9..4c0c3e40 100644 --- a/backend/api/tests/views/test_auth_factor.py +++ b/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": "one.factor@codeforlife.com", - "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="teacher@school2.com" ) - UserProfile.objects.create(user=self.one_factor_user) - - self.two_factor_credentials = { - "email": "two.factor@codeforlife.com", - "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="teacher@noschool.com" ) - 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) diff --git a/backend/api/views/auth_factor.py b/backend/api/views/auth_factor.py index 3ea6d398..f417efb4 100644 --- a/backend/api/views/auth_factor.py +++ b/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()]