diff --git a/fief/services/user_manager.py b/fief/services/user_manager.py index ae4fdfd1..7fc1ead9 100644 --- a/fief/services/user_manager.py +++ b/fief/services/user_manager.py @@ -211,7 +211,7 @@ async def verify_email( user.email_verified = True await self.user_repository.update(user) - await self.email_verification_repository.delete(email_verification) + await self.email_verification_repository.delete_by_user(user.id) return user @@ -246,7 +246,7 @@ async def verify_email_by_code( user.email_verified = True await self.user_repository.update(user) - await self.email_verification_repository.delete(email_verification) + await self.email_verification_repository.delete_by_user(user.id) return user diff --git a/fief/tasks/base.py b/fief/tasks/base.py index f53f3075..a849c6c1 100644 --- a/fief/tasks/base.py +++ b/fief/tasks/base.py @@ -12,11 +12,11 @@ from pydantic import UUID4 from sqlalchemy.orm import selectinload +from fief.crypto.verify_code import generate_verify_code from fief.db import AsyncSession from fief.db.main import get_single_main_async_session from fief.logger import logger from fief.middlewares.locale import BabelMiddleware, get_babel_middleware_kwargs -from fief.crypto.verify_code import generate_verify_code from fief.models import Brand, EmailVerification, Tenant, User from fief.models.generics import BaseModel from fief.paths import EMAIL_TEMPLATES_DIRECTORY @@ -135,12 +135,12 @@ async def _create_email_verification_code( self, user_id: UUID4, email: str ) -> str: """Mint a fresh email-verification row for ``user_id`` and return the - plaintext code (the hash is what we store). Replaces any prior pending - verification for the user so only the latest link/code is valid.""" + plaintext code (the hash is what we store). Welcome delivery and its + retries must not revoke an already delivered verification link. Explicit + resends replace pending codes; successful verification consumes them all.""" code, code_hash = generate_verify_code() async with self.get_main_session() as session: repository = EmailVerificationRepository(session) - await repository.delete_by_user(user_id) await repository.create( EmailVerification(code=code_hash, email=email, user_id=user_id) ) diff --git a/fief/tasks/email_verification.py b/fief/tasks/email_verification.py index 6f344204..a956cb14 100644 --- a/fief/tasks/email_verification.py +++ b/fief/tasks/email_verification.py @@ -4,12 +4,11 @@ from fief import schemas from fief.logger import logger -from fief.models import EmailVerification from fief.repositories import EmailVerificationRepository from fief.services.email import Null from fief.services.email_template.contexts import VerifyEmailContext from fief.services.email_template.types import EmailTemplateType -from fief.tasks.base import ObjectDoesNotExistTaskError, TaskBase +from fief.tasks.base import TaskBase class OnEmailVerificationRequestedTask(TaskBase): @@ -25,9 +24,13 @@ async def run( ) if email_verification is None: - raise ObjectDoesNotExistTaskError( - EmailVerification, email_verification_id - ) + # Resending or completing verification deletes the queued + # task's record. Retrying cannot restore that obsolete code. + logger.info("Skip obsolete email verification task") + return + if email_verification.is_expired: + logger.info("Skip expired email verification task") + return user = email_verification.user tenant = await self._get_tenant(user.tenant_id) diff --git a/tests/test_tasks_email_verification.py b/tests/test_tasks_email_verification.py index 89fd559c..9097f3ad 100644 --- a/tests/test_tasks_email_verification.py +++ b/tests/test_tasks_email_verification.py @@ -1,7 +1,9 @@ -from unittest.mock import MagicMock +from datetime import UTC, datetime, timedelta +from unittest.mock import AsyncMock, MagicMock, patch import pytest +from fief.repositories import EmailVerificationRepository from fief.services.email import EmailProvider from fief.tasks.email_verification import OnEmailVerificationRequestedTask from tests.data import TestData, email_verification_codes @@ -9,6 +11,52 @@ @pytest.mark.asyncio class TestTasksOnEmailVerificationRequestedTask: + async def test_deleted_verification_is_not_sent( + self, main_session_manager, test_data: TestData + ): + verification = test_data["email_verifications"]["not_verified_email"] + async with main_session_manager() as session: + repository = EmailVerificationRepository(session) + await repository.delete_by_user(verification.user_id) + + provider = MagicMock(spec=EmailProvider) + task = OnEmailVerificationRequestedTask(main_session_manager, provider) + # A resend, completed verification, or user deletion can invalidate a + # queued task. Repeated delivery must also be harmless. + for _ in range(2): + await task.run(str(verification.id), "fixture-code") + provider.send_email.assert_not_called() + + async def test_expired_verification_is_not_sent( + self, main_session_manager, test_data: TestData + ): + verification = test_data["email_verifications"]["not_verified_email"] + async with main_session_manager() as session: + repository = EmailVerificationRepository(session) + row = await repository.get_by_id(verification.id) + assert row is not None + row.expires_at = datetime.now(UTC) - timedelta(seconds=1) + await repository.update(row) + + provider = MagicMock(spec=EmailProvider) + task = OnEmailVerificationRequestedTask(main_session_manager, provider) + await task.run(str(verification.id), "fixture-code") + provider.send_email.assert_not_called() + + async def test_database_failure_still_propagates( + self, main_session_manager, test_data: TestData + ): + verification = test_data["email_verifications"]["not_verified_email"] + provider = MagicMock(spec=EmailProvider) + task = OnEmailVerificationRequestedTask(main_session_manager, provider) + with patch.object( + EmailVerificationRepository, "get_by_id", new_callable=AsyncMock + ) as lookup: + lookup.side_effect = RuntimeError("fixture database unavailable") + with pytest.raises(RuntimeError, match="fixture database unavailable"): + await task.run(str(verification.id), "fixture-code") + provider.send_email.assert_not_called() + async def test_send_verify_email(self, main_session_manager, test_data: TestData): email_provider_mock = MagicMock(spec=EmailProvider) @@ -23,3 +71,14 @@ async def test_send_verify_email(self, main_session_manager, test_data: TestData ) email_provider_mock.send_email.assert_called_once() + + async def test_delivery_failure_still_propagates( + self, main_session_manager, test_data: TestData + ): + provider = MagicMock(spec=EmailProvider) + provider.send_email.side_effect = RuntimeError("fixture delivery unavailable") + task = OnEmailVerificationRequestedTask(main_session_manager, provider) + verification = test_data["email_verifications"]["not_verified_email"] + with pytest.raises(RuntimeError, match="fixture delivery unavailable"): + await task.run(str(verification.id), "fixture-code") + provider.send_email.assert_called_once() diff --git a/tests/test_verification_delivery_flow.py b/tests/test_verification_delivery_flow.py new file mode 100644 index 00000000..ee71fba9 --- /dev/null +++ b/tests/test_verification_delivery_flow.py @@ -0,0 +1,177 @@ +from datetime import UTC, datetime, timedelta +from unittest.mock import MagicMock +from urllib.parse import parse_qs, urlparse + +import pytest +from bs4 import BeautifulSoup + +from fief.models import Brand +from fief.repositories import ( + EmailTemplateRepository, + EmailVerificationRepository, + UserRepository, +) +from fief.services.email import EmailProvider +from fief.settings import settings +from fief.tasks.email_verification import OnEmailVerificationRequestedTask +from fief.tasks.register import OnAfterRegisterTask +from tests.data import session_token_tokens + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "delivery_order", ["old-first", "new-first", "welcome-first", "welcome-last"] +) +async def test_resend_email_can_complete_verification( + delivery_order, + test_client_auth, + test_client_auth_csrf, + csrf_token, + main_session, + main_session_manager, + test_data, + send_task_mock, +): + user = test_data["users"]["not_verified_email"] + prefix = "" if user.tenant.default else f"/{user.tenant.slug}" + cookies = { + settings.session_cookie_name: session_token_tokens["not_verified_email"][0] + } + queued = [] + for _ in range(2): + response = await test_client_auth.get( + f"{prefix}/verify-request", cookies=cookies + ) + assert response.status_code == 302 + queued.append(send_task_mock.call_args.args[1:]) + + provider = MagicMock(spec=EmailProvider) + task = OnEmailVerificationRequestedTask(main_session_manager, provider) + welcome = OnAfterRegisterTask(main_session_manager, MagicMock(spec=EmailProvider)) + if delivery_order == "welcome-first": + await welcome.run(str(user.id)) + for args in reversed(queued) if delivery_order == "new-first" else queued: + await task.run(*args) + if delivery_order == "welcome-last": + await welcome.run(str(user.id)) + + provider.send_email.assert_called_once() + email = provider.send_email.call_args.kwargs + assert email["recipient"][0] == user.email + # Read the actual rendered email, not the code passed to the queue. + delivered_code = ( + BeautifulSoup(email["html"], "html.parser") + .get_text() + .split("VERIFY_EMAIL ")[1] + .strip() + ) + assert delivered_code == queued[-1][1] + response = await test_client_auth_csrf.post( + f"{prefix}/verify-link", + data={"code": delivered_code, "csrf_token": csrf_token}, + ) + assert response.status_code == 302 + assert response.headers["Location"].endswith("/login?verified=1") + verified = await UserRepository(main_session).get_by_id(user.id) + assert verified is not None and verified.email_verified + assert await EmailVerificationRepository(main_session).get_by_user(user.id) == [] + # A duplicate delivery after successful verification must neither fail nor + # create a new code or send another email. + await task.run(*queued[-1]) + provider.send_email.assert_called_once() + + +@pytest.mark.asyncio +async def test_expired_request_can_be_resent_and_completed( + test_client_auth, + test_client_auth_csrf, + csrf_token, + main_session, + main_session_manager, + test_data, + send_task_mock, +): + user = test_data["users"]["not_verified_email"] + repository = EmailVerificationRepository(main_session) + row = await repository.get_by_id( + test_data["email_verifications"]["not_verified_email"].id + ) + row.expires_at = datetime.now(UTC) - timedelta(seconds=1) + await repository.update(row) + provider = MagicMock(spec=EmailProvider) + task = OnEmailVerificationRequestedTask(main_session_manager, provider) + await task.run(str(row.id), "expired-fixture-code") + provider.send_email.assert_not_called() + prefix = "" if user.tenant.default else f"/{user.tenant.slug}" + response = await test_client_auth.get( + f"{prefix}/verify-request", + cookies={ + settings.session_cookie_name: session_token_tokens["not_verified_email"][0], + }, + ) + assert response.status_code == 302 + await task.run(*send_task_mock.call_args.args[1:]) + provider.send_email.assert_called_once() + code = ( + BeautifulSoup(provider.send_email.call_args.kwargs["html"], "html.parser") + .get_text() + .split("VERIFY_EMAIL ")[1] + .strip() + ) + response = await test_client_auth_csrf.post( + f"{prefix}/verify-link", data={"code": code, "csrf_token": csrf_token} + ) + assert response.headers["Location"].endswith("/login?verified=1") + verified = await UserRepository(main_session).get_by_id(user.id) + assert verified is not None and verified.email_verified + + +@pytest.mark.asyncio +@pytest.mark.parametrize("attempts,delivered_attempt", [(1, 0), (2, 0), (2, 1)]) +async def test_welcome_email_without_pending_request_can_complete_verification( + attempts, + delivered_attempt, + test_client_auth_csrf, + csrf_token, + main_session, + main_session_manager, + test_data, +): + user = test_data["users"]["not_verified_email"] + await EmailVerificationRepository(main_session).delete_by_user(user.id) + template = test_data["email_templates"]["welcome"] + template.content = 'Verify email' + await EmailTemplateRepository(main_session).update(template) + brand = Brand( + name="Fixture", + host="members.example.com", + application_url="https://app.example.com", + is_default=True, + tenant_id=user.tenant_id, + ) + main_session.add(brand) + await main_session.commit() + provider = MagicMock(spec=EmailProvider) + task = OnAfterRegisterTask(main_session_manager, provider) + if attempts == 2: + # SMTP can accept a message before the connection fails. A retry must + # leave either email's link usable until verification succeeds. + provider.send_email.side_effect = RuntimeError("fixture delivery interrupted") + with pytest.raises(RuntimeError, match="fixture delivery interrupted"): + await task.run(str(user.id), str(brand.id)) + provider.send_email.side_effect = None + await task.run(str(user.id), str(brand.id)) + assert provider.send_email.call_count == attempts + html = BeautifulSoup( + provider.send_email.call_args_list[delivered_attempt].kwargs["html"], + "html.parser", + ) + link = urlparse(html.find("a", id="verify")["href"]) + code = parse_qs(link.query)["code"][0] + response = await test_client_auth_csrf.post( + link.path, data={"code": code, "csrf_token": csrf_token} + ) + assert response.headers["Location"].endswith("/login?verified=1") + verified = await UserRepository(main_session).get_by_id(user.id) + assert verified.email_verified + assert await EmailVerificationRepository(main_session).get_by_user(user.id) == []