From 309fe87c9050e45d64adbf4419f4b014ff511cff Mon Sep 17 00:00:00 2001 From: Matt Davis Date: Fri, 30 Oct 2026 15:15:06 -0400 Subject: [PATCH 1/2] fix(tasks): skip obsolete email verification deliveries Treat deleted and expired verification requests as completed no-ops. Preserve database and delivery failures for retries, with regression coverage for each case. Fixes OPENSENSOR-FIEF-1 --- fief/tasks/email_verification.py | 13 +++--- tests/test_tasks_email_verification.py | 61 +++++++++++++++++++++++++- 2 files changed, 68 insertions(+), 6 deletions(-) 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() From 3624909e6683531a5e080b6bc2d7890e54eec13e Mon Sep 17 00:00:00 2001 From: Matt Davis Date: Fri, 30 Oct 2026 15:22:44 -0400 Subject: [PATCH 2/2] fix(verification): keep delivered codes valid across welcome tasks Preserve outstanding verification requests when welcome delivery runs or retries. Consume all pending codes on successful verification. Add HTTP flow tests using codes from rendered emails for resend ordering, delayed welcome, expiry, and delivery retries. --- fief/services/user_manager.py | 4 +- fief/tasks/base.py | 8 +- tests/test_verification_delivery_flow.py | 177 +++++++++++++++++++++++ 3 files changed, 183 insertions(+), 6 deletions(-) create mode 100644 tests/test_verification_delivery_flow.py 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/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) == []