From b1b9b1b201b569eb31e81785a328017f368b2262 Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Mon, 30 Dec 2024 14:38:39 +0000 Subject: [PATCH 1/4] fix: Add user to AuthFactor serializer --- src/api/serializers/auth_factor.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/api/serializers/auth_factor.py b/src/api/serializers/auth_factor.py index 6069fa8f..b8284313 100644 --- a/src/api/serializers/auth_factor.py +++ b/src/api/serializers/auth_factor.py @@ -13,12 +13,10 @@ class AuthFactorSerializer(ModelSerializer[User, AuthFactor]): class Meta: model = AuthFactor - fields = [ - "id", - "type", - ] + fields = ["id", "type", "user"] extra_kwargs = { "id": {"read_only": True}, + "user": {"read_only": True}, } # pylint: disable-next=missing-function-docstring From 6e1352c35859c8961ee67b822dc459e862c4d06b Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Thu, 2 Jan 2025 17:09:38 +0000 Subject: [PATCH 2/4] Only update email address on verification --- src/api/auth/token_generators.py | 24 +++++++++++- src/api/serializers/user.py | 26 +++++++++++++ src/api/serializers/user_test.py | 63 ++++++++++++++++++++++++++++++-- src/api/signals/user.py | 37 ------------------- src/api/signals/user_test.py | 56 ---------------------------- src/api/views/user.py | 10 +++++ 6 files changed, 119 insertions(+), 97 deletions(-) diff --git a/src/api/auth/token_generators.py b/src/api/auth/token_generators.py index 73e5340e..81ea7a40 100644 --- a/src/api/auth/token_generators.py +++ b/src/api/auth/token_generators.py @@ -28,13 +28,15 @@ def _get_audience(self, user_or_pk: t.Union[User, t.Any]): pk = user_or_pk.pk if isinstance(user_or_pk, User) else user_or_pk return f"user:{pk}" - def make_token(self, user_or_pk: t.Union[User, t.Any]): + def make_token(self, user_or_pk: t.Union[User, t.Any], new_email: str = ""): """Generate a token used to verify user's email address. https://pyjwt.readthedocs.io/en/stable/usage.html Args: user: The user to generate a token for. + new_email: The user's new email address to be verified, if updating + it. Returns: A token used to verify user's email address. @@ -46,6 +48,7 @@ def make_token(self, user_or_pk: t.Union[User, t.Any]): + timedelta(seconds=settings.EMAIL_VERIFICATION_TIMEOUT) ), "aud": [self._get_audience(user_or_pk)], + "new_email": new_email, }, key=settings.SECRET_KEY, algorithm="HS256", @@ -78,5 +81,24 @@ def check_token(self, user_or_pk: t.Union[User, t.Any], token: str): return True + def get_new_email(self, user_or_pk: t.Union[User, t.Any], token: str): + """Retrieve token's new_email value. + + Args: + user: The user to check. + token: The token to check. + + Returns: + The token's new_email value. + """ + decoded_jwt = jwt.decode( + jwt=token, + key=settings.SECRET_KEY, + audience=self._get_audience(user_or_pk), + algorithms=["HS256"], + ) + + return decoded_jwt["new_email"] + email_verification_token_generator = EmailVerificationTokenGenerator() diff --git a/src/api/serializers/user.py b/src/api/serializers/user.py index ba0a3cac..73dce3ae 100644 --- a/src/api/serializers/user.py +++ b/src/api/serializers/user.py @@ -119,6 +119,32 @@ def update(self, instance: AnyUser, validated_data: DataDict): if password is not None: instance.set_password(password) + new_email = validated_data.pop("email", None) + if new_email is not None and new_email != instance.email: + send_mail( + settings.DOTDIGITAL_CAMPAIGN_IDS["Email change notification"], + to_addresses=[instance.email], + personalization_values={"NEW_EMAIL_ADDRESS": instance.email}, + ) + + verify_email_address_link = settings.SERVICE_BASE_URL + reverse( + "user-verify-email-address", + kwargs={ + "pk": instance.pk, + "token": email_verification_token_generator.make_token( + instance.pk, new_email=new_email + ), + }, + ) + + send_mail( + settings.DOTDIGITAL_CAMPAIGN_IDS["Verify changed user email"], + to_addresses=[new_email], + personalization_values={ + "VERIFICATION_LINK": verify_email_address_link + }, + ) + return super().update(instance, validated_data) diff --git a/src/api/serializers/user_test.py b/src/api/serializers/user_test.py index f5d24267..28586fba 100644 --- a/src/api/serializers/user_test.py +++ b/src/api/serializers/user_test.py @@ -5,7 +5,7 @@ import typing as t from datetime import date -from unittest.mock import Mock, patch +from unittest.mock import Mock, call, patch from codeforlife.tests import ModelSerializerTestCase from codeforlife.user.models import ( @@ -16,10 +16,15 @@ StudentUser, User, ) +from django.conf import settings from django.contrib.auth.hashers import make_password from django.db.models import Count +from django.urls import reverse -from ..auth import password_reset_token_generator +from ..auth import ( + email_verification_token_generator, + password_reset_token_generator, +) from .user import ( BaseUserSerializer, CreateUserSerializer, @@ -132,7 +137,7 @@ def test_validate_password__invalid_password(self): instance=user, ) - def test_update(self): + def test_update__password(self): """Updating a user's password saves the password's hash.""" user = User.objects.first() assert user @@ -157,6 +162,58 @@ def test_update(self): assert user.check_password(password) + @patch("src.api.signals.user.send_mail") + def test_update__email(self, send_mail: Mock): + """Updating the email field sends a verification email.""" + user = User.objects.first() + assert user + + previous_email = user.email + email = "example@codeforelife.com" + assert previous_email != email + user.email = email + + with patch.object( + email_verification_token_generator, + "make_token", + return_value=email_verification_token_generator.make_token( + user, new_email=email + ), + ) as make_token: + user.save() + + make_token.assert_called_once_with(user.pk) + + send_mail.assert_has_calls( + [ + call( + settings.DOTDIGITAL_CAMPAIGN_IDS[ + "Email change notification" + ], + to_addresses=[previous_email], + personalization_values={"NEW_EMAIL_ADDRESS": email}, + ), + call( + settings.DOTDIGITAL_CAMPAIGN_IDS[ + "Verify changed user email" + ], + to_addresses=[email], + personalization_values={ + "VERIFICATION_LINK": ( + settings.SERVICE_BASE_URL + + reverse( + "user-verify-email-address", + kwargs={ + "pk": user.pk, + "token": make_token.return_value, + }, + ) + ) + }, + ), + ] + ) + class TestCreateTeacherSerializer( ModelSerializerTestCase[User, IndependentUser] diff --git a/src/api/signals/user.py b/src/api/signals/user.py index 35dc3f25..4a1dce1f 100644 --- a/src/api/signals/user.py +++ b/src/api/signals/user.py @@ -100,43 +100,6 @@ def user__post_save( # TODO: add nullable date_of_birth field to user model and send # verification email to independents in new schema. - elif instance.email != "": - if post_save.check_previous_values( - instance, - { - "email": lambda value: ( - isinstance(value, str) - and value.lower() not in ["", instance.email.lower()] - ) - }, - ): - previous_email = post_save.get_previous_value( - instance, "email", str - ) - - send_mail( - settings.DOTDIGITAL_CAMPAIGN_IDS["Email change notification"], - to_addresses=[previous_email], - personalization_values={"NEW_EMAIL_ADDRESS": instance.email}, - ) - - verify_email_address_link = settings.SERVICE_BASE_URL + reverse( - "user-verify-email-address", - kwargs={ - "pk": instance.pk, - "token": email_verification_token_generator.make_token( - instance.pk - ), - }, - ) - - send_mail( - settings.DOTDIGITAL_CAMPAIGN_IDS["Verify changed user email"], - to_addresses=[instance.email], - personalization_values={ - "VERIFICATION_LINK": verify_email_address_link - }, - ) # TODO: remove in new schema elif ( instance.email == "" diff --git a/src/api/signals/user_test.py b/src/api/signals/user_test.py index e2bf31bc..070cb1bf 100644 --- a/src/api/signals/user_test.py +++ b/src/api/signals/user_test.py @@ -3,14 +3,8 @@ Created on 03/04/2024 at 14:44:42(+01:00). """ -from unittest.mock import Mock, call, patch - from codeforlife.user.models import TeacherUser, User, UserProfile -from django.conf import settings from django.test import TestCase -from django.urls import reverse - -from ..auth import email_verification_token_generator # pylint: disable-next=missing-class-docstring @@ -40,53 +34,3 @@ def test_pre_save__username(self): user.save() assert user.username == email - - @patch("src.api.signals.user.send_mail") - def test_post_save__email_change_notification(self, send_mail: Mock): - """Updating the email field sends a verification email.""" - user = TeacherUser.objects.first() - assert user - - previous_email = user.email - email = "example@codeforelife.com" - assert previous_email != email - user.email = email - - with patch.object( - email_verification_token_generator, - "make_token", - return_value=email_verification_token_generator.make_token(user), - ) as make_token: - user.save() - - make_token.assert_called_once_with(user.pk) - - send_mail.assert_has_calls( - [ - call( - settings.DOTDIGITAL_CAMPAIGN_IDS[ - "Email change notification" - ], - to_addresses=[previous_email], - personalization_values={"NEW_EMAIL_ADDRESS": email}, - ), - call( - settings.DOTDIGITAL_CAMPAIGN_IDS[ - "Verify changed user email" - ], - to_addresses=[email], - personalization_values={ - "VERIFICATION_LINK": ( - settings.SERVICE_BASE_URL - + reverse( - "user-verify-email-address", - kwargs={ - "pk": user.pk, - "token": make_token.return_value, - }, - ) - ) - }, - ), - ] - ) diff --git a/src/api/views/user.py b/src/api/views/user.py index 900c735c..93d248d1 100644 --- a/src/api/views/user.py +++ b/src/api/views/user.py @@ -184,6 +184,16 @@ def verify_email_address(self, request: Request, **url_params: str): serializer.is_valid(raise_exception=True) serializer.save() + new_email = email_verification_token_generator.get_new_email( + user, url_params["token"] + ) + print(new_email) + if new_email != "": + user.email = new_email + user.username = new_email + user.save() + print("user saved!") + return Response( status=status.HTTP_303_SEE_OTHER, headers={ From a8765608fc0315c08398af6fd98eef61ba96777d Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Fri, 3 Jan 2025 14:20:04 +0000 Subject: [PATCH 3/4] Ignore duplicate code for now --- src/api/serializers/user.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/api/serializers/user.py b/src/api/serializers/user.py index 73dce3ae..4893b391 100644 --- a/src/api/serializers/user.py +++ b/src/api/serializers/user.py @@ -127,6 +127,7 @@ def update(self, instance: AnyUser, validated_data: DataDict): personalization_values={"NEW_EMAIL_ADDRESS": instance.email}, ) + # pylint: disable-next=duplicate-code verify_email_address_link = settings.SERVICE_BASE_URL + reverse( "user-verify-email-address", kwargs={ From c2e80fec704a6cde4dad48a24d6eeb16b8378319 Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Mon, 6 Jan 2025 17:01:39 +0000 Subject: [PATCH 4/4] Improve flow --- src/api/auth/token_generators.py | 1 + src/api/serializers/user.py | 54 ++++++++-------- src/api/serializers/user_test.py | 106 +++++++++++++++---------------- 3 files changed, 81 insertions(+), 80 deletions(-) diff --git a/src/api/auth/token_generators.py b/src/api/auth/token_generators.py index 81ea7a40..16c46843 100644 --- a/src/api/auth/token_generators.py +++ b/src/api/auth/token_generators.py @@ -41,6 +41,7 @@ def make_token(self, user_or_pk: t.Union[User, t.Any], new_email: str = ""): Returns: A token used to verify user's email address. """ + print("MAKING TOKEN") return jwt.encode( payload={ "exp": ( diff --git a/src/api/serializers/user.py b/src/api/serializers/user.py index 4893b391..4edbf511 100644 --- a/src/api/serializers/user.py +++ b/src/api/serializers/user.py @@ -119,33 +119,6 @@ def update(self, instance: AnyUser, validated_data: DataDict): if password is not None: instance.set_password(password) - new_email = validated_data.pop("email", None) - if new_email is not None and new_email != instance.email: - send_mail( - settings.DOTDIGITAL_CAMPAIGN_IDS["Email change notification"], - to_addresses=[instance.email], - personalization_values={"NEW_EMAIL_ADDRESS": instance.email}, - ) - - # pylint: disable-next=duplicate-code - verify_email_address_link = settings.SERVICE_BASE_URL + reverse( - "user-verify-email-address", - kwargs={ - "pk": instance.pk, - "token": email_verification_token_generator.make_token( - instance.pk, new_email=new_email - ), - }, - ) - - send_mail( - settings.DOTDIGITAL_CAMPAIGN_IDS["Verify changed user email"], - to_addresses=[new_email], - personalization_values={ - "VERIFICATION_LINK": verify_email_address_link - }, - ) - return super().update(instance, validated_data) @@ -309,6 +282,33 @@ def update(self, instance, validated_data): # TODO: Send email in signal to teacher of selected class to # notify them of join request. + new_email = validated_data.pop("email", None) + if new_email is not None and new_email != instance.email: + send_mail( + settings.DOTDIGITAL_CAMPAIGN_IDS["Email change notification"], + to_addresses=[instance.email], + personalization_values={"NEW_EMAIL_ADDRESS": instance.email}, + ) + + # pylint: disable-next=duplicate-code + verify_email_address_link = settings.SERVICE_BASE_URL + reverse( + "user-verify-email-address", + kwargs={ + "pk": instance.pk, + "token": email_verification_token_generator.make_token( + instance.pk, new_email=new_email + ), + }, + ) + + send_mail( + settings.DOTDIGITAL_CAMPAIGN_IDS["Verify changed user email"], + to_addresses=[new_email], + personalization_values={ + "VERIFICATION_LINK": verify_email_address_link + }, + ) + return super().update(instance, validated_data) diff --git a/src/api/serializers/user_test.py b/src/api/serializers/user_test.py index 28586fba..46d1c081 100644 --- a/src/api/serializers/user_test.py +++ b/src/api/serializers/user_test.py @@ -162,58 +162,6 @@ def test_update__password(self): assert user.check_password(password) - @patch("src.api.signals.user.send_mail") - def test_update__email(self, send_mail: Mock): - """Updating the email field sends a verification email.""" - user = User.objects.first() - assert user - - previous_email = user.email - email = "example@codeforelife.com" - assert previous_email != email - user.email = email - - with patch.object( - email_verification_token_generator, - "make_token", - return_value=email_verification_token_generator.make_token( - user, new_email=email - ), - ) as make_token: - user.save() - - make_token.assert_called_once_with(user.pk) - - send_mail.assert_has_calls( - [ - call( - settings.DOTDIGITAL_CAMPAIGN_IDS[ - "Email change notification" - ], - to_addresses=[previous_email], - personalization_values={"NEW_EMAIL_ADDRESS": email}, - ), - call( - settings.DOTDIGITAL_CAMPAIGN_IDS[ - "Verify changed user email" - ], - to_addresses=[email], - personalization_values={ - "VERIFICATION_LINK": ( - settings.SERVICE_BASE_URL - + reverse( - "user-verify-email-address", - kwargs={ - "pk": user.pk, - "token": make_token.return_value, - }, - ) - ) - }, - ), - ] - ) - class TestCreateTeacherSerializer( ModelSerializerTestCase[User, IndependentUser] @@ -321,7 +269,7 @@ def test_validate_requesting_to_join_class__no_longer_accepts_requests( error_code="no_longer_accepts_requests", ) - def test_update(self): + def test_update__requesting_to_join_class(self): """Can update the class an independent user is requesting join.""" self.assert_update( instance=self.indy_user, @@ -333,6 +281,58 @@ def test_update(self): new_data={"new_student": {"pending_class_request": self.class_2}}, ) + @patch("src.api.signals.user.send_mail") + def test_update__email(self, send_mail: Mock): + """Updating the email field sends a verification email.""" + user = User.objects.first() + assert user + + previous_email = user.email + email = "example@codeforlife.com" + assert previous_email != email + user.email = email + + with patch.object( + email_verification_token_generator, + "make_token", + return_value=email_verification_token_generator.make_token( + user, new_email=email + ), + ) as make_token: + user.save() + + make_token.assert_called_once_with(user.pk, new_email=email) + + send_mail.assert_has_calls( + [ + call( + settings.DOTDIGITAL_CAMPAIGN_IDS[ + "Email change notification" + ], + to_addresses=[previous_email], + personalization_values={"NEW_EMAIL_ADDRESS": email}, + ), + call( + settings.DOTDIGITAL_CAMPAIGN_IDS[ + "Verify changed user email" + ], + to_addresses=[email], + personalization_values={ + "VERIFICATION_LINK": ( + settings.SERVICE_BASE_URL + + reverse( + "user-verify-email-address", + kwargs={ + "pk": user.pk, + "token": make_token.return_value, + }, + ) + ) + }, + ), + ] + ) + class TestHandleIndependentUserJoinClassRequestSerializer( ModelSerializerTestCase[User, IndependentUser]