From b123c0572bc057b4935a076accd487e14d7e15a5 Mon Sep 17 00:00:00 2001 From: SKairinos Date: Mon, 2 Oct 2023 16:09:16 +0100 Subject: [PATCH 01/11] return otp_bypass_token_exists flag --- backend/api/views.py | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/backend/api/views.py b/backend/api/views.py index 709e324..4d3db86 100644 --- a/backend/api/views.py +++ b/backend/api/views.py @@ -2,6 +2,7 @@ from codeforlife.mixins import CronMixin from codeforlife.request import HttpRequest +from codeforlife.user.models import AuthFactor from common.models import UserSession from django.contrib.auth import login from django.contrib.auth.views import LoginView as _LoginView @@ -58,15 +59,21 @@ def form_valid(self, form: BaseAuthForm): # Save session (with data). self.request.session.save() - return JsonResponse( - { - "auth_factors": list( - self.request.user.session.session_auth_factors.values_list( - "auth_factor__type", flat=True - ) + response_data = { + "auth_factors": list( + self.request.user.session.session_auth_factors.values_list( + "auth_factor__type", flat=True ) - } - ) + ), + "otp_bypass_token_exists": False, + } + + if AuthFactor.Type.OTP in response_data["auth_factors"]: + response_data[ + "otp_bypass_token_exists" + ] = self.request.user.otp_bypass_tokens.exists() + + return JsonResponse(response_data) def form_invalid(self, form: BaseAuthForm): return JsonResponse(form.errors, status=status.HTTP_400_BAD_REQUEST) From ff0750a1389f0589a94d8870363f90fa1cc887a8 Mon Sep 17 00:00:00 2001 From: SKairinos Date: Mon, 2 Oct 2023 16:10:57 +0100 Subject: [PATCH 02/11] fix unit test --- backend/api/tests/test_views.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/backend/api/tests/test_views.py b/backend/api/tests/test_views.py index e58dbca..50d899d 100644 --- a/backend/api/tests/test_views.py +++ b/backend/api/tests/test_views.py @@ -29,7 +29,11 @@ def test_post__otp(self): assert response.status_code == 200 self.assertDictEqual( - response.json(), {"auth_factors": [AuthFactor.Type.OTP]} + response.json(), + { + "auth_factors": [AuthFactor.Type.OTP], + "otp_bypass_token_exists": False, + }, ) self.user.userprofile.otp_secret = pyotp.random_base32() @@ -45,7 +49,13 @@ def test_post__otp(self): ) assert response.status_code == 200 - self.assertDictEqual(response.json(), {"auth_factors": []}) + self.assertDictEqual( + response.json(), + { + "auth_factors": [], + "otp_bypass_token_exists": False, + }, + ) class TestClearExpiredView(CronTestCase): From f038fe47337b1ad60a5fd40e00ce39534486d463 Mon Sep 17 00:00:00 2001 From: SKairinos Date: Mon, 2 Oct 2023 16:15:13 +0100 Subject: [PATCH 03/11] remove unused key --- backend/api/views.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/backend/api/views.py b/backend/api/views.py index 4d3db86..b33bd6e 100644 --- a/backend/api/views.py +++ b/backend/api/views.py @@ -64,8 +64,7 @@ def form_valid(self, form: BaseAuthForm): self.request.user.session.session_auth_factors.values_list( "auth_factor__type", flat=True ) - ), - "otp_bypass_token_exists": False, + ) } if AuthFactor.Type.OTP in response_data["auth_factors"]: From 2593026bcb752a8d146806f696a4da57abb18fb1 Mon Sep 17 00:00:00 2001 From: SKairinos Date: Mon, 2 Oct 2023 16:26:23 +0100 Subject: [PATCH 04/11] fix unit test --- backend/api/tests/test_views.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/backend/api/tests/test_views.py b/backend/api/tests/test_views.py index 50d899d..338e35e 100644 --- a/backend/api/tests/test_views.py +++ b/backend/api/tests/test_views.py @@ -51,10 +51,7 @@ def test_post__otp(self): assert response.status_code == 200 self.assertDictEqual( response.json(), - { - "auth_factors": [], - "otp_bypass_token_exists": False, - }, + {"auth_factors": []}, ) From aaa30586232816002496b5da8fd9083658a7020a Mon Sep 17 00:00:00 2001 From: SKairinos Date: Thu, 5 Oct 2023 16:15:05 +0100 Subject: [PATCH 05/11] return non-http-only session cookie --- backend/Pipfile | 2 +- backend/Pipfile.lock | 10 +++---- backend/api/permissions.py | 12 +++++++++ backend/api/urls.py | 22 +++++++++++---- backend/api/views.py | 55 +++++++++++++++++++++++++++++--------- 5 files changed, 77 insertions(+), 24 deletions(-) create mode 100644 backend/api/permissions.py diff --git a/backend/Pipfile b/backend/Pipfile index 162707b..dd08af0 100644 --- a/backend/Pipfile +++ b/backend/Pipfile @@ -4,7 +4,7 @@ verify_ssl = true name = "pypi" [packages] -codeforlife = {ref = "v0.8.0", git = "https://github.com/ocadotechnology/codeforlife-package-python.git"} +codeforlife = {ref = "2fa_flow", git = "https://github.com/ocadotechnology/codeforlife-package-python.git"} django = "==3.2.20" djangorestframework = "==3.13.1" django-cors-headers = "==4.1.0" diff --git a/backend/Pipfile.lock b/backend/Pipfile.lock index 520d772..797aa5d 100644 --- a/backend/Pipfile.lock +++ b/backend/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "9b5dd7292210fab043685afe18e5fc517103bc85902a80699ebf09b26c760ee3" + "sha256": "734ed27a364493f87e0ff68d17d84b27cb13bff187f2795448dcba7535639caa" }, "pipfile-spec": 6, "requires": { @@ -155,7 +155,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "5fb23069bb2ca1ccd9a1e9e5d3db1841d2758fd1" + "ref": "b0583b490590bb29edd291813e3078fc042dfb33" }, "codeforlife-portal": { "hashes": [ @@ -1141,11 +1141,11 @@ }, "platformdirs": { "hashes": [ - "sha256:b45696dab2d7cc691a3226759c0d3b00c47c8b6e293d96f6436f733303f77f6d", - "sha256:d7c24979f292f916dc9cbf8648319032f551ea8c49a4c9bf2fb556a02070ec1d" + "sha256:cf8ee52a3afdb965072dcc652433e0c7e3e40cf5ea1477cd4b3b1d2eb75495b3", + "sha256:e9d171d00af68be50e9202731309c4e658fd8bc76f55c11c7dd760d023bda68e" ], "markers": "python_version >= '3.7'", - "version": "==3.10.0" + "version": "==3.11.0" }, "pluggy": { "hashes": [ diff --git a/backend/api/permissions.py b/backend/api/permissions.py new file mode 100644 index 0000000..d82aa0f --- /dev/null +++ b/backend/api/permissions.py @@ -0,0 +1,12 @@ +from codeforlife.user.models import User +from rest_framework.permissions import BasePermission +from rest_framework.request import Request +from rest_framework.views import View + + +class UserHasSessionAuthFactors(BasePermission): + def has_permission(self, request: Request, view: View): + return ( + isinstance(request.user, User) + and request.user.session.session_auth_factors.exists() + ) diff --git a/backend/api/urls.py b/backend/api/urls.py index 6df0a75..7218a4a 100644 --- a/backend/api/urls.py +++ b/backend/api/urls.py @@ -1,16 +1,28 @@ from django.urls import include, path, re_path -from .views import ClearExpiredView, LoginView +from .views import ClearExpiredView, LoginOptionsView, LoginView urlpatterns = [ path( "session/", include( [ - re_path( - r"^login/(?P
email|username|user-id|otp|otp-bypass-token)/$", - LoginView.as_view(), - name="login", + path( + "login/", + include( + [ + path( + "options/", + LoginOptionsView.as_view(), + name="login-options", + ), + re_path( + r"^(?Pemail|username|user-id|otp|otp-bypass-token)/$", + LoginView.as_view(), + name="login", + ), + ] + ), ), path( "clear-expired/", diff --git a/backend/api/views.py b/backend/api/views.py index b33bd6e..80728cd 100644 --- a/backend/api/views.py +++ b/backend/api/views.py @@ -1,14 +1,15 @@ import logging from codeforlife.mixins import CronMixin -from codeforlife.request import HttpRequest -from codeforlife.user.models import AuthFactor +from codeforlife.request import HttpRequest, Request +from codeforlife.user.models import AuthFactor, User from common.models import UserSession +from django.conf import settings from django.contrib.auth import login from django.contrib.auth.views import LoginView as _LoginView from django.contrib.sessions.models import Session, SessionManager from django.core import management -from django.http import JsonResponse +from django.http import HttpResponse, JsonResponse from rest_framework import status from rest_framework.response import Response from rest_framework.views import APIView @@ -21,6 +22,7 @@ UserIdAuthForm, UsernameAuthForm, ) +from .permissions import UserHasSessionAuthFactors # TODO: add 2FA logic @@ -59,23 +61,50 @@ def form_valid(self, form: BaseAuthForm): # Save session (with data). self.request.session.save() - response_data = { - "auth_factors": list( + response = HttpResponse() + + # Create a non-HTTP-only session cookie with the pending auth factors. + response.set_cookie( + key="sessionid_httponly_false", + value=",".join( self.request.user.session.session_auth_factors.values_list( "auth_factor__type", flat=True ) - ) - } + ), + max_age=( + None + if settings.SESSION_EXPIRE_AT_BROWSER_CLOSE + else settings.SESSION_COOKIE_AGE + ), + secure=settings.SESSION_COOKIE_SECURE, + samesite=settings.SESSION_COOKIE_SAMESITE, + domain=settings.SESSION_COOKIE_DOMAIN, + httponly=False, + ) + + return response + + def form_invalid(self, form: BaseAuthForm): + return JsonResponse(form.errors, status=status.HTTP_400_BAD_REQUEST) + - if AuthFactor.Type.OTP in response_data["auth_factors"]: +class LoginOptionsView(APIView): + http_method_names = ["get"] + permission_classes = [UserHasSessionAuthFactors] + + def get(self, request: Request): + user: User = request.user + session_auth_factors = user.session.session_auth_factors + + response_data = {"id": user.id} + if session_auth_factors.filter( + auth_factor__type=AuthFactor.Type.OTP + ).exists(): response_data[ "otp_bypass_token_exists" - ] = self.request.user.otp_bypass_tokens.exists() - - return JsonResponse(response_data) + ] = user.otp_bypass_tokens.exists() - def form_invalid(self, form: BaseAuthForm): - return JsonResponse(form.errors, status=status.HTTP_400_BAD_REQUEST) + return Response(response_data) class ClearExpiredView(CronMixin, APIView): From aa896c836bd533638d0c2393b70075b9ef4adf36 Mon Sep 17 00:00:00 2001 From: SKairinos Date: Thu, 5 Oct 2023 16:28:45 +0100 Subject: [PATCH 06/11] fix unit test --- backend/api/tests/test_views.py | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/backend/api/tests/test_views.py b/backend/api/tests/test_views.py index 338e35e..8cb9f15 100644 --- a/backend/api/tests/test_views.py +++ b/backend/api/tests/test_views.py @@ -4,6 +4,7 @@ from codeforlife.tests import CronTestCase from codeforlife.user.models import AuthFactor, User from django.core import management +from django.http import HttpResponse from django.test import TestCase from django.urls import reverse from django.utils import timezone @@ -13,6 +14,10 @@ class TestLoginView(TestCase): def setUp(self): self.user = User.objects.get(id=2) + def _get_session_auth_factors(self, response: HttpResponse): + cookie = response.cookies["sessionid_httponly_false"].value + return [af for af in cookie.split(",") if af != ""] + def test_post__otp(self): AuthFactor.objects.create( user=self.user, @@ -28,13 +33,7 @@ def test_post__otp(self): ) assert response.status_code == 200 - self.assertDictEqual( - response.json(), - { - "auth_factors": [AuthFactor.Type.OTP], - "otp_bypass_token_exists": False, - }, - ) + assert self._get_session_auth_factors(response) == [AuthFactor.Type.OTP] self.user.userprofile.otp_secret = pyotp.random_base32() self.user.userprofile.save() @@ -49,10 +48,7 @@ def test_post__otp(self): ) assert response.status_code == 200 - self.assertDictEqual( - response.json(), - {"auth_factors": []}, - ) + assert self._get_session_auth_factors(response) == [] class TestClearExpiredView(CronTestCase): From 3007f1ab6e1d6535a8f5bf2e0f197fe16f341259 Mon Sep 17 00:00:00 2001 From: SKairinos Date: Mon, 9 Oct 2023 07:27:43 +0100 Subject: [PATCH 07/11] rename endpoint --- backend/api/urls.py | 8 ++++---- backend/api/views.py | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/backend/api/urls.py b/backend/api/urls.py index 7218a4a..ce64773 100644 --- a/backend/api/urls.py +++ b/backend/api/urls.py @@ -1,6 +1,6 @@ from django.urls import include, path, re_path -from .views import ClearExpiredView, LoginOptionsView, LoginView +from .views import AuthFactorsView, ClearExpiredView, LoginView urlpatterns = [ path( @@ -12,9 +12,9 @@ include( [ path( - "options/", - LoginOptionsView.as_view(), - name="login-options", + "auth_factors/", + AuthFactorsView.as_view(), + name="session-auth-factors", ), re_path( r"^(?Pemail|username|user-id|otp|otp-bypass-token)/$", diff --git a/backend/api/views.py b/backend/api/views.py index 80728cd..1f69fa8 100644 --- a/backend/api/views.py +++ b/backend/api/views.py @@ -88,7 +88,7 @@ def form_invalid(self, form: BaseAuthForm): return JsonResponse(form.errors, status=status.HTTP_400_BAD_REQUEST) -class LoginOptionsView(APIView): +class AuthFactorsView(APIView): http_method_names = ["get"] permission_classes = [UserHasSessionAuthFactors] From 828ccd2e247380f7ecdc0627cc7d8af485ce6d25 Mon Sep 17 00:00:00 2001 From: SKairinos Date: Mon, 9 Oct 2023 07:29:07 +0100 Subject: [PATCH 08/11] kebab case --- backend/api/urls.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/api/urls.py b/backend/api/urls.py index ce64773..bc35688 100644 --- a/backend/api/urls.py +++ b/backend/api/urls.py @@ -12,7 +12,7 @@ include( [ path( - "auth_factors/", + "auth-factors/", AuthFactorsView.as_view(), name="session-auth-factors", ), From eef807e25d3c13a214ec4fb7033175d0efe751d6 Mon Sep 17 00:00:00 2001 From: SKairinos Date: Mon, 9 Oct 2023 07:32:11 +0100 Subject: [PATCH 09/11] undo options change --- backend/api/urls.py | 8 ++++---- backend/api/views.py | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/backend/api/urls.py b/backend/api/urls.py index bc35688..7218a4a 100644 --- a/backend/api/urls.py +++ b/backend/api/urls.py @@ -1,6 +1,6 @@ from django.urls import include, path, re_path -from .views import AuthFactorsView, ClearExpiredView, LoginView +from .views import ClearExpiredView, LoginOptionsView, LoginView urlpatterns = [ path( @@ -12,9 +12,9 @@ include( [ path( - "auth-factors/", - AuthFactorsView.as_view(), - name="session-auth-factors", + "options/", + LoginOptionsView.as_view(), + name="login-options", ), re_path( r"^(?Pemail|username|user-id|otp|otp-bypass-token)/$", diff --git a/backend/api/views.py b/backend/api/views.py index 1f69fa8..80728cd 100644 --- a/backend/api/views.py +++ b/backend/api/views.py @@ -88,7 +88,7 @@ def form_invalid(self, form: BaseAuthForm): return JsonResponse(form.errors, status=status.HTTP_400_BAD_REQUEST) -class AuthFactorsView(APIView): +class LoginOptionsView(APIView): http_method_names = ["get"] permission_classes = [UserHasSessionAuthFactors] From ce2fb7a03de2a0234a04f426b160de18bfbc846f Mon Sep 17 00:00:00 2001 From: SKairinos Date: Thu, 12 Oct 2023 10:38:43 +0100 Subject: [PATCH 10/11] use new cfl package --- backend/Pipfile | 2 +- backend/Pipfile.lock | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/backend/Pipfile b/backend/Pipfile index dd08af0..07d8745 100644 --- a/backend/Pipfile +++ b/backend/Pipfile @@ -4,7 +4,7 @@ verify_ssl = true name = "pypi" [packages] -codeforlife = {ref = "2fa_flow", git = "https://github.com/ocadotechnology/codeforlife-package-python.git"} +codeforlife = {ref = "v0.8.3", git = "https://github.com/ocadotechnology/codeforlife-package-python.git"} django = "==3.2.20" djangorestframework = "==3.13.1" django-cors-headers = "==4.1.0" diff --git a/backend/Pipfile.lock b/backend/Pipfile.lock index 797aa5d..115a4d6 100644 --- a/backend/Pipfile.lock +++ b/backend/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "734ed27a364493f87e0ff68d17d84b27cb13bff187f2795448dcba7535639caa" + "sha256": "35a1f0e3d41bff90adb93acc781bbf0db41767aac9a22db6baadcdcfccd9490c" }, "pipfile-spec": 6, "requires": { @@ -155,7 +155,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "b0583b490590bb29edd291813e3078fc042dfb33" + "ref": "f11c55524e8ef4a78a07f996978f2a11b553f326" }, "codeforlife-portal": { "hashes": [ From 769bc8657cc6217d597971089f17050f8e65f8b8 Mon Sep 17 00:00:00 2001 From: SKairinos Date: Thu, 12 Oct 2023 13:24:42 +0100 Subject: [PATCH 11/11] feedback --- backend/api/tests/test_views.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/backend/api/tests/test_views.py b/backend/api/tests/test_views.py index 8cb9f15..4591149 100644 --- a/backend/api/tests/test_views.py +++ b/backend/api/tests/test_views.py @@ -15,8 +15,13 @@ def setUp(self): self.user = User.objects.get(id=2) def _get_session_auth_factors(self, response: HttpResponse): - cookie = response.cookies["sessionid_httponly_false"].value - return [af for af in cookie.split(",") if af != ""] + return [ + auth_factor + for auth_factor in response.cookies[ + "sessionid_httponly_false" + ].value.split(",") + if auth_factor != "" + ] def test_post__otp(self): AuthFactor.objects.create(