From 0ca0eeb266f017d7144eab3d8dd74226f1305759 Mon Sep 17 00:00:00 2001 From: SKairinos Date: Thu, 15 Feb 2024 15:21:31 +0000 Subject: [PATCH 1/9] install latest py package --- backend/Pipfile | 4 ++-- backend/Pipfile.lock | 18 +++++++++++------- backend/api/tests/views/test_klass.py | 3 +-- backend/api/tests/views/test_school.py | 3 +-- .../views/test_school_teacher_invitation.py | 3 +-- backend/api/tests/views/test_user.py | 10 ++++------ 6 files changed, 20 insertions(+), 21 deletions(-) diff --git a/backend/Pipfile b/backend/Pipfile index 06d3f99e..1363c65a 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.4", git = "https://github.com/ocadotechnology/codeforlife-package-python.git"} +codeforlife = {ref = "v0.13.5", 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.4", git = "https://github.com/ocadotechnology/codeforlife-package-python.git", extras = ["dev"]} +codeforlife = {ref = "v0.13.5", 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 5d6b7532..195e0177 100644 --- a/backend/Pipfile.lock +++ b/backend/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "a4f65f80c5f383809d2b3acdc82ede3c6cd65b81150d4d9bcd0a8a19a4d1942c" + "sha256": "37aa967cb299192f55afa30bb9b0fdfcc61ac3077e2304d60af494622e74b224" }, "pipfile-spec": 6, "requires": { @@ -168,7 +168,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "e5801cf92dc7d3edb3e20aca76a031ca8d618f76" + "ref": "bb29de5d9d045483f98a219cc457c3cb06c2c835" }, "codeforlife-portal": { "hashes": [ @@ -375,11 +375,11 @@ "grpc" ], "hashes": [ - "sha256:6fb380f49d19ee1d09a9722d0379042b7edb06c0112e4796c7a395078a043e71", - "sha256:7421474c39d396a74dfa317dddbc69188f2336835f526087c7648f91105e32ff" + "sha256:3399c92887a97d33038baa4bfd3bf07acc05d474b0171f333e1f641c1364e552", + "sha256:52bcc9d9937735f8a3986fa0bbf9135ae9cf5393a722387e5eced520e39c774a" ], "markers": "python_version >= '3.7'", - "version": "==1.34.0" + "version": "==1.34.1" }, "google-auth": { "hashes": [ @@ -806,6 +806,7 @@ "sha256:a6f5977418eff3b2d5500d54d9db50c8277a368436f4e4f8ddb1be3422870184", "sha256:f91456ead12ab3c6c2e9491cf33ba6d08357d802192379bb482f1033ade496f5" ], + "markers": "python_version >= '3.6'", "version": "==3.1.2" }, "pandas": { @@ -1304,6 +1305,7 @@ "sha256:6a33ee89877bd9abc1158129f6e94be74e2679636b8a205b43b85206c3f0bbdd", "sha256:f72f148f54442c6b056bf931dbc34f986fd0c3b0b6b5a58d013c9aef274d0c88" ], + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3, 3.4, 3.5'", "version": "==2.0.1" }, "xlwt": { @@ -1512,7 +1514,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "e5801cf92dc7d3edb3e20aca76a031ca8d618f76" + "ref": "bb29de5d9d045483f98a219cc457c3cb06c2c835" }, "codeforlife-portal": { "hashes": [ @@ -2184,6 +2186,7 @@ "sha256:a6f5977418eff3b2d5500d54d9db50c8277a368436f4e4f8ddb1be3422870184", "sha256:f91456ead12ab3c6c2e9491cf33ba6d08357d802192379bb482f1033ade496f5" ], + "markers": "python_version >= '3.6'", "version": "==3.1.2" }, "packaging": { @@ -2492,7 +2495,7 @@ "sha256:c7c6ca206e93355074ae32f7403e8ea12163b1163c976fee7d4d84027c162be5", "sha256:d45e0952f3727241918b8fd0f376f5ff6b301cc0777c6f9a556935c92d8a7d42" ], - "markers": "python_version < '3.10'", + "markers": "python_version >= '3.7'", "version": "==7.2.1" }, "pytest-cov": { @@ -2895,6 +2898,7 @@ "sha256:6a33ee89877bd9abc1158129f6e94be74e2679636b8a205b43b85206c3f0bbdd", "sha256:f72f148f54442c6b056bf931dbc34f986fd0c3b0b6b5a58d013c9aef274d0c88" ], + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3, 3.4, 3.5'", "version": "==2.0.1" }, "xlwt": { diff --git a/backend/api/tests/views/test_klass.py b/backend/api/tests/views/test_klass.py index 550a8c84..cfab847b 100644 --- a/backend/api/tests/views/test_klass.py +++ b/backend/api/tests/views/test_klass.py @@ -104,10 +104,9 @@ def test_create__other(self): Teacher can create a class with another teacher as the class owner. """ - user = self.client.login_school_teacher( + user = self.client.login_admin_school_teacher( email="admin.teacher@school1.com", password="password", - is_admin=True, ) teacher = ( diff --git a/backend/api/tests/views/test_school.py b/backend/api/tests/views/test_school.py index f5c84f5c..c4d29149 100644 --- a/backend/api/tests/views/test_school.py +++ b/backend/api/tests/views/test_school.py @@ -76,8 +76,7 @@ def test_create(self): def test_partial_update(self): """Can successfully update a school.""" - user = self.client.login_school_teacher( - is_admin=True, + user = self.client.login_admin_school_teacher( email="admin.teacher@school1.com", password="password", ) diff --git a/backend/api/tests/views/test_school_teacher_invitation.py b/backend/api/tests/views/test_school_teacher_invitation.py index 01bf2b25..97a6a6e8 100644 --- a/backend/api/tests/views/test_school_teacher_invitation.py +++ b/backend/api/tests/views/test_school_teacher_invitation.py @@ -24,10 +24,9 @@ class TestSchoolTeacherInvitationViewSet( non_school_teacher_email = "teacher@noschool.com" def _login_admin_school_teacher(self): - return self.client.login_school_teacher( + return self.client.login_admin_school_teacher( email=self.school_admin_teacher_email, password="password", - is_admin=True, ) def setUp(self): diff --git a/backend/api/tests/views/test_user.py b/backend/api/tests/views/test_user.py index cb0d9bb4..077103b5 100644 --- a/backend/api/tests/views/test_user.py +++ b/backend/api/tests/views/test_user.py @@ -30,11 +30,10 @@ class TestUserViewSet(ModelViewSetTestCase[User]): school_teacher_email = "teacher@school1.com" indy_email = "indy@man.com" - def _login_school_teacher(self): - return self.client.login_school_teacher( + def _login_non_admin_school_teacher(self): + return self.client.login_non_admin_school_teacher( email=self.school_teacher_email, password="password", - is_admin=False, ) def _get_pk_and_token_for_user(self, email: str): @@ -68,7 +67,7 @@ def test_get_permissions__partial_update__student(self): def test_bulk_create__students(self): """Teacher can bulk create students.""" - user = self._login_school_teacher() + user = self._login_non_admin_school_teacher() klass: t.Optional[Class] = user.teacher.class_teacher.first() assert klass is not None @@ -204,10 +203,9 @@ def test_reset_password__patch__indy(self): def test_partial_update__teacher(self): """Admin-school-teacher can update another teacher's profile.""" - admin_school_teacher_user = self.client.login_school_teacher( + admin_school_teacher_user = self.client.login_admin_school_teacher( email="admin.teacher@school1.com", password="password", - is_admin=True, ) other_school_teacher_user = ( From 7d2f2f5a955f45cfcc5472585cfe520e3d211b4e Mon Sep 17 00:00:00 2001 From: SKairinos Date: Thu, 15 Feb 2024 15:42:36 +0000 Subject: [PATCH 2/9] bulk anonymize students --- backend/api/tests/views/test_user.py | 28 ++++++++++++++++++++++++++++ backend/api/views/user.py | 25 ++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/backend/api/tests/views/test_user.py b/backend/api/tests/views/test_user.py index 077103b5..977adf9a 100644 --- a/backend/api/tests/views/test_user.py +++ b/backend/api/tests/views/test_user.py @@ -226,3 +226,31 @@ def test_partial_update__teacher(self): }, }, ) + + def test_bulk_destroy(self): + """School-teacher can bulk anonymize students.""" + school_teacher_user = self.client.login_school_teacher( + email="teacher@school1.com", + password="password", + ) + + student_user_queryset = User.objects.filter( + new_student__class_field__teacher=school_teacher_user.teacher, + ) + student_user_ids = list( + student_user_queryset.values_list("id", flat=True) + ) + assert student_user_ids + + self.client.bulk_destroy(student_user_ids, make_assertions=False) + + assert ( + len(student_user_ids) + == student_user_queryset.filter( + username="", + email="", + first_name="", + last_name="", + is_active=False, + ).count() + ) diff --git a/backend/api/views/user.py b/backend/api/views/user.py index 7a9346e1..16bed38c 100644 --- a/backend/api/views/user.py +++ b/backend/api/views/user.py @@ -40,6 +40,24 @@ def get_permissions(self): return super().get_permissions() + def get_queryset(self): + queryset = super().get_queryset() + if self.action == "bulk" and self.request.method in ["PATCH", "DELETE"]: + queryset = queryset.filter( + new_student__isnull=False, + new_student__class_field__isnull=False, + ) + return queryset + + def perform_bulk_destroy(self, queryset): + queryset.update( + username="", + email="", + first_name="", + last_name="", + is_active=False, + ) + @action( detail=True, methods=["get", "patch"], @@ -89,7 +107,12 @@ def reset_password( return Response() - @action(detail=False, methods=["post"], permission_classes=[AllowAny]) + @action( + detail=False, + methods=["post"], + url_path="request-password-reset", + permission_classes=[AllowAny], + ) def request_password_reset(self, request: Request): """ Generates a reset password URL to be emailed to the user if the From 5aa44db17f6487254dc0b318d2afe44220581955 Mon Sep 17 00:00:00 2001 From: SKairinos Date: Fri, 16 Feb 2024 08:37:23 +0000 Subject: [PATCH 3/9] fix test cases --- backend/Pipfile | 4 +- backend/Pipfile.lock | 18 ++--- backend/api/tests/views/test_user.py | 101 +++++++++++++++++++-------- backend/api/views/user.py | 8 +-- 4 files changed, 82 insertions(+), 49 deletions(-) diff --git a/backend/Pipfile b/backend/Pipfile index 1363c65a..ff06aea1 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.5", git = "https://github.com/ocadotechnology/codeforlife-package-python.git"} +codeforlife = {ref = "bulk_anon_students", 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.5", git = "https://github.com/ocadotechnology/codeforlife-package-python.git", extras = ["dev"]} +codeforlife = {ref = "bulk_anon_students", 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 195e0177..1e75106b 100644 --- a/backend/Pipfile.lock +++ b/backend/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "37aa967cb299192f55afa30bb9b0fdfcc61ac3077e2304d60af494622e74b224" + "sha256": "a6445322f1a1826d04758f3d99c1a979f75be1a16bb3dcb00809d8c5a9e909b9" }, "pipfile-spec": 6, "requires": { @@ -168,7 +168,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "bb29de5d9d045483f98a219cc457c3cb06c2c835" + "ref": "6987ffe0114a2c8d41f9055bbd499ce95501d65e" }, "codeforlife-portal": { "hashes": [ @@ -806,7 +806,6 @@ "sha256:a6f5977418eff3b2d5500d54d9db50c8277a368436f4e4f8ddb1be3422870184", "sha256:f91456ead12ab3c6c2e9491cf33ba6d08357d802192379bb482f1033ade496f5" ], - "markers": "python_version >= '3.6'", "version": "==3.1.2" }, "pandas": { @@ -1059,7 +1058,7 @@ "sha256:0123cacc1627ae19ddf3c27a5de5bd67ee4586fbdd6440d9748f8abb483d3e86", "sha256:961d03dc3453ebbc59dbdea9e4e11c5651520a876d0f4db161e8674aae935da9" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'", "version": "==2.8.2" }, "pytz": { @@ -1210,7 +1209,7 @@ "sha256:1e61c37477a1626458e36f7b1d82aa5c9b094fa4802892072e49de9c60c4c926", "sha256:8abb2f1d86890a2dfb989f9a77cfcfd3e47c2a354b01111771326f8aa26e0254" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'", "version": "==1.16.0" }, "sortedcontainers": { @@ -1305,7 +1304,6 @@ "sha256:6a33ee89877bd9abc1158129f6e94be74e2679636b8a205b43b85206c3f0bbdd", "sha256:f72f148f54442c6b056bf931dbc34f986fd0c3b0b6b5a58d013c9aef274d0c88" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3, 3.4, 3.5'", "version": "==2.0.1" }, "xlwt": { @@ -1514,7 +1512,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "bb29de5d9d045483f98a219cc457c3cb06c2c835" + "ref": "6987ffe0114a2c8d41f9055bbd499ce95501d65e" }, "codeforlife-portal": { "hashes": [ @@ -2186,7 +2184,6 @@ "sha256:a6f5977418eff3b2d5500d54d9db50c8277a368436f4e4f8ddb1be3422870184", "sha256:f91456ead12ab3c6c2e9491cf33ba6d08357d802192379bb482f1033ade496f5" ], - "markers": "python_version >= '3.6'", "version": "==3.1.2" }, "packaging": { @@ -2554,7 +2551,7 @@ "sha256:0123cacc1627ae19ddf3c27a5de5bd67ee4586fbdd6440d9748f8abb483d3e86", "sha256:961d03dc3453ebbc59dbdea9e4e11c5651520a876d0f4db161e8674aae935da9" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'", "version": "==2.8.2" }, "pytz": { @@ -2730,7 +2727,7 @@ "sha256:1e61c37477a1626458e36f7b1d82aa5c9b094fa4802892072e49de9c60c4c926", "sha256:8abb2f1d86890a2dfb989f9a77cfcfd3e47c2a354b01111771326f8aa26e0254" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'", "version": "==1.16.0" }, "snapshottest": { @@ -2898,7 +2895,6 @@ "sha256:6a33ee89877bd9abc1158129f6e94be74e2679636b8a205b43b85206c3f0bbdd", "sha256:f72f148f54442c6b056bf931dbc34f986fd0c3b0b6b5a58d013c9aef274d0c88" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3, 3.4, 3.5'", "version": "==2.0.1" }, "xlwt": { diff --git a/backend/api/tests/views/test_user.py b/backend/api/tests/views/test_user.py index 977adf9a..59365cb6 100644 --- a/backend/api/tests/views/test_user.py +++ b/backend/api/tests/views/test_user.py @@ -6,7 +6,12 @@ import typing as t from codeforlife.tests import ModelViewSetTestCase -from codeforlife.user.models import Class, SchoolTeacherUser, User +from codeforlife.user.models import ( + AdminSchoolTeacherUser, + Class, + SchoolTeacherUser, + User, +) from codeforlife.user.permissions import InSchool, IsTeacher from django.contrib.auth.tokens import ( PasswordResetTokenGenerator, @@ -30,6 +35,11 @@ class TestUserViewSet(ModelViewSetTestCase[User]): school_teacher_email = "teacher@school1.com" indy_email = "indy@man.com" + def setUp(self): + self.admin_school_teacher_user = AdminSchoolTeacherUser.objects.get( + email="admin.teacher@school1.com" + ) + def _login_non_admin_school_teacher(self): return self.client.login_non_admin_school_teacher( email=self.school_teacher_email, @@ -42,6 +52,8 @@ def _get_pk_and_token_for_user(self, email: str): return user.pk, token + # test: get permissions + def test_get_permissions__bulk(self): """Only school-teachers can perform bulk actions.""" self.assert_get_permissions( @@ -65,6 +77,38 @@ def test_get_permissions__partial_update__student(self): request=self.client.request_factory.patch(data={"student": {}}), ) + # test: get queryset + + def _test_get_queryset__bulk(self, request_method: str): + assert User.objects.filter( + new_teacher__school=self.admin_school_teacher_user.teacher.school + ).exists() + + student_users = list( + User.objects.filter( + new_student__class_field__teacher__school=( + self.admin_school_teacher_user.teacher.school + ) + ) + ) + assert student_users + + request = self.client.request_factory.generic( + request_method, user=self.admin_school_teacher_user + ) + + self.assert_get_queryset(student_users, action="bulk", request=request) + + def test_get_queryset__bulk__patch(self): + """Bulk partial-update can only target student-users.""" + self._test_get_queryset__bulk("patch") + + def test_get_queryset__bulk__delete(self): + """Bulk destroy can only target student-users.""" + self._test_get_queryset__bulk("delete") + + # test: bulk actions + def test_bulk_create__students(self): """Teacher can bulk create students.""" user = self._login_non_admin_school_teacher() @@ -88,6 +132,31 @@ def test_bulk_create__students(self): response.json() # TODO: make custom assertions and check for password + def test_bulk_destroy(self): + """School-teacher can bulk anonymize students.""" + self.client.login_as(self.admin_school_teacher_user) + + student_user_queryset = User.objects.filter( + new_student__class_field__teacher__school=( + self.admin_school_teacher_user.teacher.school + ), + ) + student_user_ids = list( + student_user_queryset.values_list("id", flat=True) + ) + assert student_user_ids + + self.client.bulk_destroy(student_user_ids, make_assertions=False) + + assert ( + len(student_user_ids) + == student_user_queryset.filter( + first_name="", is_active=False + ).count() + ) + + # test: reset password actions + def test_request_password_reset__invalid_email(self): """ Request password reset doesn't generate reset password URL if email @@ -201,6 +270,8 @@ def test_reset_password__patch__indy(self): self.client.patch(viewname, data={"password": "N3wPassword"}) self.client.login(email=self.indy_email, password="N3wPassword") + # test: reset generic actions + def test_partial_update__teacher(self): """Admin-school-teacher can update another teacher's profile.""" admin_school_teacher_user = self.client.login_admin_school_teacher( @@ -226,31 +297,3 @@ def test_partial_update__teacher(self): }, }, ) - - def test_bulk_destroy(self): - """School-teacher can bulk anonymize students.""" - school_teacher_user = self.client.login_school_teacher( - email="teacher@school1.com", - password="password", - ) - - student_user_queryset = User.objects.filter( - new_student__class_field__teacher=school_teacher_user.teacher, - ) - student_user_ids = list( - student_user_queryset.values_list("id", flat=True) - ) - assert student_user_ids - - self.client.bulk_destroy(student_user_ids, make_assertions=False) - - assert ( - len(student_user_ids) - == student_user_queryset.filter( - username="", - email="", - first_name="", - last_name="", - is_active=False, - ).count() - ) diff --git a/backend/api/views/user.py b/backend/api/views/user.py index 16bed38c..7cdf83fe 100644 --- a/backend/api/views/user.py +++ b/backend/api/views/user.py @@ -50,13 +50,7 @@ def get_queryset(self): return queryset def perform_bulk_destroy(self, queryset): - queryset.update( - username="", - email="", - first_name="", - last_name="", - is_active=False, - ) + queryset.update(first_name="", is_active=False) @action( detail=True, From f7ef9925919bac8711fe5d363aafccdb6eac7650 Mon Sep 17 00:00:00 2001 From: SKairinos Date: Fri, 16 Feb 2024 10:03:58 +0000 Subject: [PATCH 4/9] fix permissions --- backend/Pipfile.lock | 4 ++-- backend/api/tests/views/test_klass.py | 22 +++++++++++-------- backend/api/tests/views/test_school.py | 10 ++++----- .../views/test_school_teacher_invitation.py | 12 +++++----- backend/api/tests/views/test_user.py | 15 ++++++++----- backend/api/views/klass.py | 10 +++++---- backend/api/views/school.py | 8 +++---- .../api/views/school_teacher_invitation.py | 4 ++-- backend/api/views/user.py | 9 ++++---- 9 files changed, 53 insertions(+), 41 deletions(-) diff --git a/backend/Pipfile.lock b/backend/Pipfile.lock index 1e75106b..b282309e 100644 --- a/backend/Pipfile.lock +++ b/backend/Pipfile.lock @@ -168,7 +168,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "6987ffe0114a2c8d41f9055bbd499ce95501d65e" + "ref": "22dfe33ed8f3b85fb7e939e746b806cc12a4305d" }, "codeforlife-portal": { "hashes": [ @@ -1512,7 +1512,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "6987ffe0114a2c8d41f9055bbd499ce95501d65e" + "ref": "22dfe33ed8f3b85fb7e939e746b806cc12a4305d" }, "codeforlife-portal": { "hashes": [ diff --git a/backend/api/tests/views/test_klass.py b/backend/api/tests/views/test_klass.py index cfab847b..f39b0b2a 100644 --- a/backend/api/tests/views/test_klass.py +++ b/backend/api/tests/views/test_klass.py @@ -5,10 +5,10 @@ from datetime import timedelta -from codeforlife.permissions import AllowNone +from codeforlife.permissions import OR, AllowNone from codeforlife.tests import ModelViewSetTestCase from codeforlife.user.models import Class, Teacher -from codeforlife.user.permissions import InSchool, IsTeacher +from codeforlife.user.permissions import IsStudent, IsTeacher from django.utils import timezone from ...views import ClassViewSet @@ -36,27 +36,31 @@ def test_get_permissions__create(self): """ self.assert_get_permissions( - permissions=[IsTeacher(), InSchool()], + permissions=[IsTeacher(in_school=True)], action="create", ) def test_get_permissions__update(self): """ - Only a school-teacher can update a class. + Only an admin- or class-teacher can update a class. """ self.assert_get_permissions( - permissions=[IsTeacher(), InSchool()], + permissions=[ + OR(IsTeacher(is_admin=True), IsTeacher(in_class=True)) + ], action="update", ) def test_get_permissions__destroy(self): """ - Only a school-teacher can destroy a class. + Only an admin- or class-teacher can destroy a class. """ self.assert_get_permissions( - permissions=[IsTeacher(), InSchool()], + permissions=[ + OR(IsTeacher(is_admin=True), IsTeacher(in_class=True)) + ], action="destroy", ) @@ -66,7 +70,7 @@ def test_get_permissions__list(self): """ self.assert_get_permissions( - permissions=[IsTeacher(), InSchool()], + permissions=[IsTeacher(in_school=True)], action="list", ) @@ -76,7 +80,7 @@ def test_get_permissions__retrieve(self): """ self.assert_get_permissions( - permissions=[InSchool()], + permissions=[OR(IsStudent(), IsTeacher(in_school=True))], action="retrieve", ) diff --git a/backend/api/tests/views/test_school.py b/backend/api/tests/views/test_school.py index c4d29149..03e644cd 100644 --- a/backend/api/tests/views/test_school.py +++ b/backend/api/tests/views/test_school.py @@ -3,10 +3,10 @@ Created on 02/02/2024 at 15:31:21(+00:00). """ -from codeforlife.permissions import NOT, AllowNone +from codeforlife.permissions import OR, AllowNone from codeforlife.tests import ModelViewSetTestCase from codeforlife.user.models import School -from codeforlife.user.permissions import InSchool, IsTeacher +from codeforlife.user.permissions import IsStudent, IsTeacher from ...views import SchoolViewSet @@ -37,7 +37,7 @@ def test_get_permissions__create(self): """Only teachers not in a school can create a school.""" self.assert_get_permissions( - permissions=[IsTeacher(), NOT(InSchool())], + permissions=[IsTeacher(in_school=False)], action="create", ) @@ -45,7 +45,7 @@ def test_get_permissions__update(self): """Only admin-teachers in a school can update a school.""" self.assert_get_permissions( - permissions=[IsTeacher(is_admin=True), InSchool()], + permissions=[IsTeacher(is_admin=True)], action="update", ) @@ -53,7 +53,7 @@ def test_get_permissions__retrieve(self): """Anyone in a school can retrieve a school.""" self.assert_get_permissions( - permissions=[InSchool()], + permissions=[OR(IsStudent(), IsTeacher(in_school=True))], action="retrieve", ) diff --git a/backend/api/tests/views/test_school_teacher_invitation.py b/backend/api/tests/views/test_school_teacher_invitation.py index 97a6a6e8..97d0ec1d 100644 --- a/backend/api/tests/views/test_school_teacher_invitation.py +++ b/backend/api/tests/views/test_school_teacher_invitation.py @@ -6,7 +6,7 @@ from codeforlife.permissions import AllowNone from codeforlife.tests import ModelViewSetTestCase from codeforlife.user.models import User -from codeforlife.user.permissions import InSchool, IsTeacher +from codeforlife.user.permissions import IsTeacher from rest_framework import status from ...models import SchoolTeacherInvitation @@ -59,35 +59,35 @@ def test_get_permissions__bulk(self): def test_get_permissions__create(self): """Only admin-teachers can create an invitation.""" self.assert_get_permissions( - permissions=[IsTeacher(is_admin=True), InSchool()], + permissions=[IsTeacher(is_admin=True)], action="create", ) def test_get_permissions__partial_update(self): """Only admin-teachers can update an invitation.""" self.assert_get_permissions( - permissions=[IsTeacher(is_admin=True), InSchool()], + permissions=[IsTeacher(is_admin=True)], action="partial_update", ) def test_get_permissions__retrieve(self): """Only admin-teachers can retrieve an invitation.""" self.assert_get_permissions( - permissions=[IsTeacher(is_admin=True), InSchool()], + permissions=[IsTeacher(is_admin=True)], action="retrieve", ) def test_get_permissions__list(self): """Only admin-teachers can list invitations.""" self.assert_get_permissions( - permissions=[IsTeacher(is_admin=True), InSchool()], + permissions=[IsTeacher(is_admin=True)], action="list", ) def test_get_permissions__destroy(self): """Only admin-teachers can destroy an invitation.""" self.assert_get_permissions( - permissions=[IsTeacher(is_admin=True), InSchool()], + permissions=[IsTeacher(is_admin=True)], action="destroy", ) diff --git a/backend/api/tests/views/test_user.py b/backend/api/tests/views/test_user.py index 59365cb6..41560f1c 100644 --- a/backend/api/tests/views/test_user.py +++ b/backend/api/tests/views/test_user.py @@ -5,6 +5,7 @@ import typing as t +from codeforlife.permissions import OR from codeforlife.tests import ModelViewSetTestCase from codeforlife.user.models import ( AdminSchoolTeacherUser, @@ -12,7 +13,7 @@ SchoolTeacherUser, User, ) -from codeforlife.user.permissions import InSchool, IsTeacher +from codeforlife.user.permissions import IsTeacher from django.contrib.auth.tokens import ( PasswordResetTokenGenerator, default_token_generator, @@ -57,14 +58,16 @@ def _get_pk_and_token_for_user(self, email: str): def test_get_permissions__bulk(self): """Only school-teachers can perform bulk actions.""" self.assert_get_permissions( - permissions=[IsTeacher(), InSchool()], + permissions=[ + OR(IsTeacher(is_admin=True), IsTeacher(in_class=True)) + ], action="bulk", ) def test_get_permissions__partial_update__teacher(self): """Only admin-school-teachers can update a teacher.""" self.assert_get_permissions( - permissions=[IsTeacher(is_admin=True), InSchool()], + permissions=[IsTeacher(is_admin=True)], action="partial_update", request=self.client.request_factory.patch(data={"teacher": {}}), ) @@ -72,7 +75,9 @@ def test_get_permissions__partial_update__teacher(self): def test_get_permissions__partial_update__student(self): """Only school-teachers can update a student.""" self.assert_get_permissions( - permissions=[IsTeacher(), InSchool()], + permissions=[ + OR(IsTeacher(is_admin=True), IsTeacher(in_class=True)) + ], action="partial_update", request=self.client.request_factory.patch(data={"student": {}}), ) @@ -270,7 +275,7 @@ def test_reset_password__patch__indy(self): self.client.patch(viewname, data={"password": "N3wPassword"}) self.client.login(email=self.indy_email, password="N3wPassword") - # test: reset generic actions + # test: generic actions def test_partial_update__teacher(self): """Admin-school-teacher can update another teacher's profile.""" diff --git a/backend/api/views/klass.py b/backend/api/views/klass.py index 17e5d1ed..58c0cac4 100644 --- a/backend/api/views/klass.py +++ b/backend/api/views/klass.py @@ -3,8 +3,8 @@ Created on 23/01/2024 at 17:53:37(+00:00). """ -from codeforlife.permissions import AllowNone -from codeforlife.user.permissions import InSchool, IsTeacher +from codeforlife.permissions import OR, AllowNone +from codeforlife.user.permissions import IsTeacher from codeforlife.user.views import ClassViewSet as _ClassViewSet from ..serializers import ClassSerializer @@ -19,7 +19,9 @@ def get_permissions(self): # Bulk actions not allowed for classes. if self.action == "bulk": return [AllowNone()] - if self.action in ["create", "update", "destroy"]: - return [IsTeacher(), InSchool()] + if self.action == "create": + return [IsTeacher(in_school=True)] + if self.action in ["update", "destroy"]: + return [OR(IsTeacher(is_admin=True), IsTeacher(in_class=True))] return super().get_permissions() diff --git a/backend/api/views/school.py b/backend/api/views/school.py index 30df17f5..f4c2400a 100644 --- a/backend/api/views/school.py +++ b/backend/api/views/school.py @@ -3,8 +3,8 @@ Created on 23/01/2024 at 17:53:50(+00:00). """ -from codeforlife.permissions import NOT, AllowNone -from codeforlife.user.permissions import InSchool, IsTeacher +from codeforlife.permissions import AllowNone +from codeforlife.user.permissions import IsTeacher from codeforlife.user.views import SchoolViewSet as _SchoolViewSet from ..serializers import SchoolSerializer @@ -21,9 +21,9 @@ def get_permissions(self): return [AllowNone()] # Only teachers not in a school can create a school. if self.action == "create": - return [IsTeacher(), NOT(InSchool())] + return [IsTeacher(in_school=False)] # Only admin-teachers in a school can update a school. if self.action == "update": - return [IsTeacher(is_admin=True), InSchool()] + return [IsTeacher(is_admin=True)] return super().get_permissions() diff --git a/backend/api/views/school_teacher_invitation.py b/backend/api/views/school_teacher_invitation.py index 2039981c..bff91cb3 100644 --- a/backend/api/views/school_teacher_invitation.py +++ b/backend/api/views/school_teacher_invitation.py @@ -6,7 +6,7 @@ from codeforlife.permissions import AllowNone from codeforlife.request import Request from codeforlife.user.models import User -from codeforlife.user.permissions import InSchool, IsTeacher +from codeforlife.user.permissions import IsTeacher from codeforlife.views import ModelViewSet from django.contrib.auth.hashers import check_password from rest_framework import status @@ -31,7 +31,7 @@ def get_permissions(self): "partial_update", "destroy", ]: - return [IsTeacher(is_admin=True), InSchool()] + return [IsTeacher(is_admin=True)] if self.action == "bulk": return [AllowNone()] diff --git a/backend/api/views/user.py b/backend/api/views/user.py index 7cdf83fe..cd0b80a4 100644 --- a/backend/api/views/user.py +++ b/backend/api/views/user.py @@ -5,9 +5,10 @@ import typing as t +from codeforlife.permissions import OR from codeforlife.request import Request from codeforlife.user.models import User -from codeforlife.user.permissions import InSchool, IsTeacher +from codeforlife.user.permissions import IsTeacher from codeforlife.user.views import UserViewSet as _UserViewSet from django.contrib.auth.tokens import ( PasswordResetTokenGenerator, @@ -31,12 +32,12 @@ class UserViewSet(_UserViewSet): def get_permissions(self): if self.action == "bulk": - return [IsTeacher(), InSchool()] + return [OR(IsTeacher(is_admin=True), IsTeacher(in_class=True))] if self.action == "partial_update": if "teacher" in self.request.data: - return [IsTeacher(is_admin=True), InSchool()] + return [IsTeacher(is_admin=True)] if "student" in self.request.data: - return [IsTeacher(), InSchool()] + return [OR(IsTeacher(is_admin=True), IsTeacher(in_class=True))] return super().get_permissions() From 2c6df754444f06df9d53a4b7f46699bb65ce8dfb Mon Sep 17 00:00:00 2001 From: SKairinos Date: Fri, 16 Feb 2024 14:40:53 +0000 Subject: [PATCH 5/9] new python package --- backend/Pipfile | 4 +-- backend/Pipfile.lock | 14 ++++----- backend/api/tests/views/test_klass.py | 41 +++++++++++---------------- 3 files changed, 26 insertions(+), 33 deletions(-) diff --git a/backend/Pipfile b/backend/Pipfile index ff06aea1..b0507c23 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 = "bulk_anon_students", git = "https://github.com/ocadotechnology/codeforlife-package-python.git"} +codeforlife = {ref = "v0.13.6", 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 = "bulk_anon_students", git = "https://github.com/ocadotechnology/codeforlife-package-python.git", extras = ["dev"]} +codeforlife = {ref = "v0.13.6", 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 b282309e..390b76b6 100644 --- a/backend/Pipfile.lock +++ b/backend/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "a6445322f1a1826d04758f3d99c1a979f75be1a16bb3dcb00809d8c5a9e909b9" + "sha256": "87409c54e5f41ceef8b81accc69edfa6fc7fd474d9c22c4e0d018938b57b52a1" }, "pipfile-spec": 6, "requires": { @@ -168,7 +168,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "22dfe33ed8f3b85fb7e939e746b806cc12a4305d" + "ref": "9fd3549f852745032d9d43174ec2f25ae29b6f8f" }, "codeforlife-portal": { "hashes": [ @@ -1058,7 +1058,7 @@ "sha256:0123cacc1627ae19ddf3c27a5de5bd67ee4586fbdd6440d9748f8abb483d3e86", "sha256:961d03dc3453ebbc59dbdea9e4e11c5651520a876d0f4db161e8674aae935da9" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'", + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", "version": "==2.8.2" }, "pytz": { @@ -1209,7 +1209,7 @@ "sha256:1e61c37477a1626458e36f7b1d82aa5c9b094fa4802892072e49de9c60c4c926", "sha256:8abb2f1d86890a2dfb989f9a77cfcfd3e47c2a354b01111771326f8aa26e0254" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'", + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", "version": "==1.16.0" }, "sortedcontainers": { @@ -1512,7 +1512,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "22dfe33ed8f3b85fb7e939e746b806cc12a4305d" + "ref": "9fd3549f852745032d9d43174ec2f25ae29b6f8f" }, "codeforlife-portal": { "hashes": [ @@ -2551,7 +2551,7 @@ "sha256:0123cacc1627ae19ddf3c27a5de5bd67ee4586fbdd6440d9748f8abb483d3e86", "sha256:961d03dc3453ebbc59dbdea9e4e11c5651520a876d0f4db161e8674aae935da9" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'", + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", "version": "==2.8.2" }, "pytz": { @@ -2727,7 +2727,7 @@ "sha256:1e61c37477a1626458e36f7b1d82aa5c9b094fa4802892072e49de9c60c4c926", "sha256:8abb2f1d86890a2dfb989f9a77cfcfd3e47c2a354b01111771326f8aa26e0254" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'", + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", "version": "==1.16.0" }, "snapshottest": { diff --git a/backend/api/tests/views/test_klass.py b/backend/api/tests/views/test_klass.py index f39b0b2a..5d30d1a3 100644 --- a/backend/api/tests/views/test_klass.py +++ b/backend/api/tests/views/test_klass.py @@ -20,31 +20,24 @@ class TestClassViewSet(ModelViewSetTestCase[Class]): model_view_set_class = ClassViewSet fixtures = ["school_1"] - def test_get_permissions__bulk(self): - """ - No one is allowed to perform bulk actions. - """ + # test: get permissions + def test_get_permissions__bulk(self): + """No one is allowed to perform bulk actions.""" self.assert_get_permissions( permissions=[AllowNone()], action="bulk", ) def test_get_permissions__create(self): - """ - Only a school-teacher can create a class. - """ - + """Only school-teachers can create a class.""" self.assert_get_permissions( permissions=[IsTeacher(in_school=True)], action="create", ) def test_get_permissions__update(self): - """ - Only an admin- or class-teacher can update a class. - """ - + """Only admin-teachers or class-teachers can update a class.""" self.assert_get_permissions( permissions=[ OR(IsTeacher(is_admin=True), IsTeacher(in_class=True)) @@ -53,10 +46,7 @@ def test_get_permissions__update(self): ) def test_get_permissions__destroy(self): - """ - Only an admin- or class-teacher can destroy a class. - """ - + """Only admin-teachers or class-teachers can destroy a class.""" self.assert_get_permissions( permissions=[ OR(IsTeacher(is_admin=True), IsTeacher(in_class=True)) @@ -65,22 +55,25 @@ def test_get_permissions__destroy(self): ) def test_get_permissions__list(self): - """ - Only a school-teacher can list classes. - """ - + """Only admin-teachers and class-teachers can list classes.""" self.assert_get_permissions( - permissions=[IsTeacher(in_school=True)], + permissions=[ + OR(IsTeacher(is_admin=True), IsTeacher(in_class=True)) + ], action="list", ) def test_get_permissions__retrieve(self): """ - Any school-user can retrieve a class. + Only students, admin-teachers or class-teachers can retrieve a class. """ - self.assert_get_permissions( - permissions=[OR(IsStudent(), IsTeacher(in_school=True))], + permissions=[ + OR( + IsStudent(), + OR(IsTeacher(is_admin=True), IsTeacher(in_class=True)), + ) + ], action="retrieve", ) From 3653d7b5c496f5bbca04db395f40cf9554ec77c5 Mon Sep 17 00:00:00 2001 From: SKairinos Date: Fri, 16 Feb 2024 18:14:42 +0000 Subject: [PATCH 6/9] fix enable/disable otp --- backend/Pipfile | 4 +- backend/Pipfile.lock | 14 +-- 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 | 117 +++++++++--------- backend/api/views/auth_factor.py | 10 +- 7 files changed, 216 insertions(+), 79 deletions(-) create mode 100644 backend/api/tests/serializers/test_auth_factor.py diff --git a/backend/Pipfile b/backend/Pipfile index b0507c23..d14b766a 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 = "fix_otp", 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 = "fix_otp", 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..0b6feaf3 100644 --- a/backend/Pipfile.lock +++ b/backend/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "87409c54e5f41ceef8b81accc69edfa6fc7fd474d9c22c4e0d018938b57b52a1" + "sha256": "d07fbc5722d406e86830d46474020dcd31fb60cfb35d4c493037966d4987c0e9" }, "pipfile-spec": 6, "requires": { @@ -168,7 +168,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "9fd3549f852745032d9d43174ec2f25ae29b6f8f" + "ref": "142d44f5651e33ae72563561f253b97a355685c6" }, "codeforlife-portal": { "hashes": [ @@ -1058,7 +1058,7 @@ "sha256:0123cacc1627ae19ddf3c27a5de5bd67ee4586fbdd6440d9748f8abb483d3e86", "sha256:961d03dc3453ebbc59dbdea9e4e11c5651520a876d0f4db161e8674aae935da9" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'", "version": "==2.8.2" }, "pytz": { @@ -1209,7 +1209,7 @@ "sha256:1e61c37477a1626458e36f7b1d82aa5c9b094fa4802892072e49de9c60c4c926", "sha256:8abb2f1d86890a2dfb989f9a77cfcfd3e47c2a354b01111771326f8aa26e0254" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'", "version": "==1.16.0" }, "sortedcontainers": { @@ -1512,7 +1512,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "9fd3549f852745032d9d43174ec2f25ae29b6f8f" + "ref": "142d44f5651e33ae72563561f253b97a355685c6" }, "codeforlife-portal": { "hashes": [ @@ -2551,7 +2551,7 @@ "sha256:0123cacc1627ae19ddf3c27a5de5bd67ee4586fbdd6440d9748f8abb483d3e86", "sha256:961d03dc3453ebbc59dbdea9e4e11c5651520a876d0f4db161e8674aae935da9" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'", "version": "==2.8.2" }, "pytz": { @@ -2727,7 +2727,7 @@ "sha256:1e61c37477a1626458e36f7b1d82aa5c9b094fa4802892072e49de9c60c4c926", "sha256:8abb2f1d86890a2dfb989f9a77cfcfd3e47c2a354b01111771326f8aa26e0254" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'", "version": "==1.16.0" }, "snapshottest": { 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..85db01b5 --- /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): + """Validate type is not doing this.""" + 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..542b5e9f 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,75 @@ 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.""" + request = self.client.request_factory.get( + user=self.multi_auth_factor_teacher_user ) - def test_retrieve(self): - """ - Retrieving a single auth factor is forbidden. - """ + self.assert_get_queryset( + list(self.multi_auth_factor_teacher_user.auth_factors.all()), + request=request, + ) - user = self.client.login(**self.two_factor_credentials) - assert user == self.two_factor_user + # test: get permissions - self.client.retrieve(self.two_auth_factor, status.HTTP_403_FORBIDDEN) + def test_get_permissions__bulk(self): + """Cannot perform any bulk action.""" + self.assert_get_permissions([AllowNone()], action="bulk") - 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__retrieve(self): + """Cannot retrieve a single auth factor.""" + self.assert_get_permissions([AllowNone()], action="retrieve") - self.client.list([self.two_auth_factor]) + def test_get_permissions__list(self): + """Only a teacher-user can list all auth factors.""" + self.assert_get_permissions([IsTeacher()], action="list") - def test_create(self): - """ - Can enable an 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_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()] From 7b28aadd5657e9fd0a75737467721f8ef17dbb2e Mon Sep 17 00:00:00 2001 From: SKairinos Date: Fri, 16 Feb 2024 18:29:44 +0000 Subject: [PATCH 7/9] new py package --- backend/Pipfile | 4 ++-- backend/Pipfile.lock | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/backend/Pipfile b/backend/Pipfile index d14b766a..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 = "fix_otp", 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 = "fix_otp", 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 2a19a866..d8f3d366 100644 --- a/backend/Pipfile.lock +++ b/backend/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "d07fbc5722d406e86830d46474020dcd31fb60cfb35d4c493037966d4987c0e9" + "sha256": "9e02bb3bd37675c13e20e4e4add09570c4ead5526ab3650ee2a3d47864023155" }, "pipfile-spec": 6, "requires": { @@ -168,7 +168,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "142d44f5651e33ae72563561f253b97a355685c6" + "ref": "30953150f4d2762e421d8f798badb57ad0318fd6" }, "codeforlife-portal": { "hashes": [ @@ -1058,7 +1058,7 @@ "sha256:0123cacc1627ae19ddf3c27a5de5bd67ee4586fbdd6440d9748f8abb483d3e86", "sha256:961d03dc3453ebbc59dbdea9e4e11c5651520a876d0f4db161e8674aae935da9" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'", + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", "version": "==2.8.2" }, "pytz": { @@ -1209,7 +1209,7 @@ "sha256:1e61c37477a1626458e36f7b1d82aa5c9b094fa4802892072e49de9c60c4c926", "sha256:8abb2f1d86890a2dfb989f9a77cfcfd3e47c2a354b01111771326f8aa26e0254" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'", + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", "version": "==1.16.0" }, "sortedcontainers": { @@ -1512,7 +1512,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "142d44f5651e33ae72563561f253b97a355685c6" + "ref": "30953150f4d2762e421d8f798badb57ad0318fd6" }, "codeforlife-portal": { "hashes": [ @@ -2551,7 +2551,7 @@ "sha256:0123cacc1627ae19ddf3c27a5de5bd67ee4586fbdd6440d9748f8abb483d3e86", "sha256:961d03dc3453ebbc59dbdea9e4e11c5651520a876d0f4db161e8674aae935da9" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'", + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", "version": "==2.8.2" }, "pytz": { @@ -2727,7 +2727,7 @@ "sha256:1e61c37477a1626458e36f7b1d82aa5c9b094fa4802892072e49de9c60c4c926", "sha256:8abb2f1d86890a2dfb989f9a77cfcfd3e47c2a354b01111771326f8aa26e0254" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'", + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", "version": "==1.16.0" }, "snapshottest": { @@ -2913,4 +2913,4 @@ "version": "==3.17.0" } } -} \ No newline at end of file +} From be1b3964c79f976a9a48d95f66f2d9e5b28e1f5c Mon Sep 17 00:00:00 2001 From: SKairinos Date: Fri, 16 Feb 2024 18:33:48 +0000 Subject: [PATCH 8/9] feedback --- backend/api/tests/serializers/test_auth_factor.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/api/tests/serializers/test_auth_factor.py b/backend/api/tests/serializers/test_auth_factor.py index 85db01b5..917a306b 100644 --- a/backend/api/tests/serializers/test_auth_factor.py +++ b/backend/api/tests/serializers/test_auth_factor.py @@ -32,7 +32,7 @@ def setUp(self): # test: validate def test_validate_type__already_exists(self): - """Validate type is not doing this.""" + """Cannot enable an already enabled auth factor.""" auth_factor = self.multi_auth_factor_teacher_user.auth_factors.first() assert auth_factor From 94bbfa8083e783eea6b6e857268575f565c9a31a Mon Sep 17 00:00:00 2001 From: SKairinos Date: Fri, 16 Feb 2024 18:34:35 +0000 Subject: [PATCH 9/9] house keeping --- backend/api/tests/views/test_auth_factor.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/backend/api/tests/views/test_auth_factor.py b/backend/api/tests/views/test_auth_factor.py index 542b5e9f..4c0c3e40 100644 --- a/backend/api/tests/views/test_auth_factor.py +++ b/backend/api/tests/views/test_auth_factor.py @@ -32,13 +32,11 @@ def setUp(self): def test_get_queryset(self): """Can only access your own auth factors.""" - request = self.client.request_factory.get( - user=self.multi_auth_factor_teacher_user - ) - self.assert_get_queryset( list(self.multi_auth_factor_teacher_user.auth_factors.all()), - request=request, + request=self.client.request_factory.get( + user=self.multi_auth_factor_teacher_user + ), ) # test: get permissions