From 8ee1394e8cdd0d5796295182a38f0aac9234f896 Mon Sep 17 00:00:00 2001 From: Hiep Le <69354317+hieptl@users.noreply.github.com> Date: Tue, 30 Dec 2025 02:12:14 +0700 Subject: [PATCH] feat: add button to authentication modal to resend verification email (#12179) --- enterprise/server/middleware.py | 1 + enterprise/server/routes/auth.py | 4 +- enterprise/server/routes/email.py | 51 ++- enterprise/server/utils/rate_limit_utils.py | 83 +++++ .../unit/server/routes/test_email_routes.py | 216 ++++++++++++- .../unit/server/test_rate_limit_utils.py | 290 ++++++++++++++++++ enterprise/tests/unit/test_auth_middleware.py | 50 +++ enterprise/tests/unit/test_auth_routes.py | 2 + .../email-verification-modal.test.tsx | 167 +++++++++- .../api/email-service/email-service.api.ts | 35 +++ frontend/src/api/email-service/email.types.ts | 8 + .../waitlist/email-verification-modal.tsx | 33 ++ .../mutation/use-resend-email-verification.ts | 49 +++ frontend/src/hooks/use-email-verification.ts | 73 +++++ frontend/src/routes/root-layout.tsx | 2 + frontend/src/routes/user-settings.tsx | 18 +- frontend/test-utils.tsx | 20 ++ 17 files changed, 1073 insertions(+), 29 deletions(-) create mode 100644 enterprise/server/utils/rate_limit_utils.py create mode 100644 enterprise/tests/unit/server/test_rate_limit_utils.py create mode 100644 frontend/src/api/email-service/email-service.api.ts create mode 100644 frontend/src/api/email-service/email.types.ts create mode 100644 frontend/src/hooks/mutation/use-resend-email-verification.ts diff --git a/enterprise/server/middleware.py b/enterprise/server/middleware.py index 54e3319595..81780a8b10 100644 --- a/enterprise/server/middleware.py +++ b/enterprise/server/middleware.py @@ -159,6 +159,7 @@ class SetAuthCookieMiddleware: '/api/billing/cancel', '/api/billing/customer-setup-success', '/api/billing/stripe-webhook', + '/api/email/resend', '/oauth/device/authorize', '/oauth/device/token', ) diff --git a/enterprise/server/routes/auth.py b/enterprise/server/routes/auth.py index e911538da6..dac7d6871a 100644 --- a/enterprise/server/routes/auth.py +++ b/enterprise/server/routes/auth.py @@ -210,7 +210,9 @@ async def keycloak_callback( from server.routes.email import verify_email await verify_email(request=request, user_id=user_id, is_auth_flow=True) - redirect_url = f'{request.base_url}?email_verification_required=true' + redirect_url = ( + f'{request.base_url}?email_verification_required=true&user_id={user_id}' + ) response = RedirectResponse(redirect_url, status_code=302) return response diff --git a/enterprise/server/routes/email.py b/enterprise/server/routes/email.py index b58adf9a4f..7b7d32a196 100644 --- a/enterprise/server/routes/email.py +++ b/enterprise/server/routes/email.py @@ -7,6 +7,7 @@ from server.auth.constants import KEYCLOAK_CLIENT_ID from server.auth.keycloak_manager import get_keycloak_admin from server.auth.saas_user_auth import SaasUserAuth from server.routes.auth import set_response_cookie +from server.utils.rate_limit_utils import check_rate_limit_by_user_id from openhands.core.logger import openhands_logger as logger from openhands.server.user_auth import get_user_id @@ -28,6 +29,11 @@ class EmailUpdate(BaseModel): return v +class ResendEmailVerificationRequest(BaseModel): + user_id: str | None = None + is_auth_flow: bool = False + + @api_router.post('') async def update_email( email_data: EmailUpdate, request: Request, user_id: str = Depends(get_user_id) @@ -90,11 +96,41 @@ async def update_email( ) -@api_router.put('/verify') +@api_router.put('/resend') async def resend_email_verification( - request: Request, user_id: str = Depends(get_user_id) + request: Request, + body: ResendEmailVerificationRequest | None = None, ): - await verify_email(request=request, user_id=user_id) + # Get user_id from body if provided, otherwise from auth + user_id: str | None = None + if body and body.user_id: + user_id = body.user_id + else: + try: + user_id = await get_user_id(request) + except Exception: + pass + + if not user_id: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail='user_id is required in request body or user must be authenticated', + ) + + # Check rate limit (uses user_id if available, otherwise falls back to IP) + # Use 30 seconds for user-based rate limiting to match frontend cooldown + await check_rate_limit_by_user_id( + request=request, + key_prefix='email_resend', + user_id=user_id, + user_rate_limit_seconds=30, + ip_rate_limit_seconds=60, # 1 minute for IP-based limiting (more lenient) + ) + + # Get is_auth_flow from body if provided, default to False + is_auth_flow = body.is_auth_flow if body else False + + await verify_email(request=request, user_id=user_id, is_auth_flow=is_auth_flow) logger.info(f'Resending verification email for {user_id}') return JSONResponse( @@ -129,11 +165,10 @@ async def verified_email(request: Request): async def verify_email(request: Request, user_id: str, is_auth_flow: bool = False): keycloak_admin = get_keycloak_admin() scheme = 'http' if request.url.hostname == 'localhost' else 'https' - redirect_uri = ( - f'{scheme}://{request.url.netloc}?email_verified=true' - if is_auth_flow - else f'{scheme}://{request.url.netloc}/api/email/verified' - ) + if is_auth_flow: + redirect_uri = f'{scheme}://{request.url.netloc}?email_verified=true' + else: + redirect_uri = f'{scheme}://{request.url.netloc}/api/email/verified' logger.info(f'Redirect URI: {redirect_uri}') await keycloak_admin.a_send_verify_email( user_id=user_id, diff --git a/enterprise/server/utils/rate_limit_utils.py b/enterprise/server/utils/rate_limit_utils.py new file mode 100644 index 0000000000..0d8d2d67cd --- /dev/null +++ b/enterprise/server/utils/rate_limit_utils.py @@ -0,0 +1,83 @@ +from fastapi import HTTPException, Request, status + +from openhands.core.logger import openhands_logger as logger +from openhands.server.shared import sio + +# Rate limiting constants +RATE_LIMIT_USER_SECONDS = 120 # 2 minutes per user_id +RATE_LIMIT_IP_SECONDS = 300 # 5 minutes per IP address + + +async def check_rate_limit_by_user_id( + request: Request, + key_prefix: str, + user_id: str | None, + user_rate_limit_seconds: int = RATE_LIMIT_USER_SECONDS, + ip_rate_limit_seconds: int = RATE_LIMIT_IP_SECONDS, +) -> None: + """ + Check rate limit for requests, using user_id when available, falling back to IP address. + + Uses Redis to store rate limit keys with expiration. If a key already exists, + it means the rate limit is active and the request will be rejected. + + Args: + request: FastAPI Request object + key_prefix: Prefix for the Redis key (e.g., "email_resend") + user_id: User ID if available, None otherwise + user_rate_limit_seconds: Rate limit window in seconds for user_id-based limiting (default: 120) + ip_rate_limit_seconds: Rate limit window in seconds for IP-based limiting (default: 300) + + Raises: + HTTPException: If rate limit is exceeded (429 status code) + """ + try: + redis = sio.manager.redis + if not redis: + # If Redis is unavailable, log warning and allow request (fail open) + logger.warning('Redis unavailable for rate limiting, allowing request') + return + + if user_id: + # Rate limit by user_id (primary method) + rate_limit_key = f'{key_prefix}:{user_id}' + rate_limit_seconds = user_rate_limit_seconds + else: + # Fallback to IP address rate limiting + client_ip = request.client.host if request.client else 'unknown' + rate_limit_key = f'{key_prefix}:ip:{client_ip}' + rate_limit_seconds = ip_rate_limit_seconds + + # Try to set the key with expiration. If it already exists (nx=True fails), + # it means the rate limit is active + created = await redis.set(rate_limit_key, 1, nx=True, ex=rate_limit_seconds) + + if not created: + logger.info( + f'Rate limit exceeded for {rate_limit_key}', + extra={ + 'user_id': user_id, + 'ip': request.client.host if request.client else 'unknown', + }, + ) + # Format error message based on duration + if rate_limit_seconds < 60: + wait_message = f'{rate_limit_seconds} seconds' + elif rate_limit_seconds % 60 == 0: + wait_message = f'{rate_limit_seconds // 60} minute{"s" if rate_limit_seconds // 60 != 1 else ""}' + else: + minutes = rate_limit_seconds // 60 + seconds = rate_limit_seconds % 60 + wait_message = f'{minutes} minute{"s" if minutes != 1 else ""} and {seconds} second{"s" if seconds != 1 else ""}' + + raise HTTPException( + status_code=status.HTTP_429_TOO_MANY_REQUESTS, + detail=f'Too many requests. Please wait {wait_message} before trying again.', + ) + except HTTPException: + # Re-raise HTTPException (rate limit exceeded) + raise + except Exception as e: + # Log error but allow request (fail open) to avoid blocking legitimate users + logger.warning(f'Error checking rate limit: {e}', exc_info=True) + return diff --git a/enterprise/tests/unit/server/routes/test_email_routes.py b/enterprise/tests/unit/server/routes/test_email_routes.py index 8f5ba12e87..bb328b90e4 100644 --- a/enterprise/tests/unit/server/routes/test_email_routes.py +++ b/enterprise/tests/unit/server/routes/test_email_routes.py @@ -1,11 +1,16 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest -from fastapi import Request -from fastapi.responses import RedirectResponse +from fastapi import HTTPException, Request, status +from fastapi.responses import JSONResponse, RedirectResponse from pydantic import SecretStr from server.auth.saas_user_auth import SaasUserAuth -from server.routes.email import verified_email, verify_email +from server.routes.email import ( + ResendEmailVerificationRequest, + resend_email_verification, + verified_email, + verify_email, +) @pytest.fixture @@ -149,3 +154,208 @@ async def test_verified_email_https_scheme(mock_request, mock_user_auth): # Verify secure flag is True for https call_kwargs = mock_set_cookie.call_args.kwargs assert call_kwargs['secure'] is True + + +@pytest.mark.asyncio +async def test_resend_email_verification_with_user_id_from_body_succeeds(mock_request): + """Test resend_email_verification succeeds when user_id is provided in body.""" + # Arrange + user_id = 'test_user_id' + body = ResendEmailVerificationRequest(user_id=user_id, is_auth_flow=False) + mock_keycloak_admin = AsyncMock() + mock_keycloak_admin.a_send_verify_email = AsyncMock() + + with ( + patch('server.routes.email.check_rate_limit_by_user_id') as mock_rate_limit, + patch( + 'server.routes.email.get_keycloak_admin', return_value=mock_keycloak_admin + ), + patch('server.routes.email.logger') as mock_logger, + ): + mock_rate_limit.return_value = None # Rate limit check passes + + # Act + result = await resend_email_verification(request=mock_request, body=body) + + # Assert + assert isinstance(result, JSONResponse) + assert result.status_code == status.HTTP_200_OK + assert 'message' in result.body.decode() + mock_rate_limit.assert_called_once_with( + request=mock_request, + key_prefix='email_resend', + user_id=user_id, + user_rate_limit_seconds=30, + ip_rate_limit_seconds=60, + ) + mock_keycloak_admin.a_send_verify_email.assert_called_once() + # Logger is called multiple times (verify_email and resend_email_verification) + # Check that the resend message was logged + assert any( + 'Resending verification email for' in str(call) + for call in mock_logger.info.call_args_list + ) + + +@pytest.mark.asyncio +async def test_resend_email_verification_with_user_id_from_auth_succeeds(mock_request): + """Test resend_email_verification succeeds when user_id comes from authentication.""" + # Arrange + user_id = 'test_user_id' + mock_keycloak_admin = AsyncMock() + mock_keycloak_admin.a_send_verify_email = AsyncMock() + + with ( + patch( + 'server.routes.email.get_user_id', return_value=user_id + ) as mock_get_user_id, + patch('server.routes.email.check_rate_limit_by_user_id') as mock_rate_limit, + patch( + 'server.routes.email.get_keycloak_admin', return_value=mock_keycloak_admin + ), + ): + mock_rate_limit.return_value = None # Rate limit check passes + + # Act + result = await resend_email_verification(request=mock_request, body=None) + + # Assert + assert isinstance(result, JSONResponse) + assert result.status_code == status.HTTP_200_OK + mock_get_user_id.assert_called_once_with(mock_request) + mock_rate_limit.assert_called_once_with( + request=mock_request, + key_prefix='email_resend', + user_id=user_id, + user_rate_limit_seconds=30, + ip_rate_limit_seconds=60, + ) + + +@pytest.mark.asyncio +async def test_resend_email_verification_without_user_id_returns_400(mock_request): + """Test resend_email_verification returns 400 when user_id is not available.""" + # Arrange + with patch( + 'server.routes.email.get_user_id', side_effect=Exception('Not authenticated') + ): + # Act & Assert + with pytest.raises(HTTPException) as exc_info: + await resend_email_verification(request=mock_request, body=None) + + assert exc_info.value.status_code == status.HTTP_400_BAD_REQUEST + assert 'user_id is required' in exc_info.value.detail + + +@pytest.mark.asyncio +async def test_resend_email_verification_rate_limit_exceeded_returns_429(mock_request): + """Test resend_email_verification returns 429 when rate limit is exceeded.""" + # Arrange + user_id = 'test_user_id' + body = ResendEmailVerificationRequest(user_id=user_id) + + with ( + patch('server.routes.email.check_rate_limit_by_user_id') as mock_rate_limit, + ): + mock_rate_limit.side_effect = HTTPException( + status_code=status.HTTP_429_TOO_MANY_REQUESTS, + detail='Too many requests. Please wait 2 minutes before trying again.', + ) + + # Act & Assert + with pytest.raises(HTTPException) as exc_info: + await resend_email_verification(request=mock_request, body=body) + + assert exc_info.value.status_code == status.HTTP_429_TOO_MANY_REQUESTS + assert 'Too many requests' in exc_info.value.detail + mock_rate_limit.assert_called_once() + + +@pytest.mark.asyncio +async def test_resend_email_verification_with_is_auth_flow_true(mock_request): + """Test resend_email_verification passes is_auth_flow to verify_email.""" + # Arrange + user_id = 'test_user_id' + body = ResendEmailVerificationRequest(user_id=user_id, is_auth_flow=True) + mock_keycloak_admin = AsyncMock() + mock_keycloak_admin.a_send_verify_email = AsyncMock() + + with ( + patch('server.routes.email.check_rate_limit_by_user_id') as mock_rate_limit, + patch( + 'server.routes.email.get_keycloak_admin', return_value=mock_keycloak_admin + ), + ): + mock_rate_limit.return_value = None + + # Act + await resend_email_verification(request=mock_request, body=body) + + # Assert + mock_keycloak_admin.a_send_verify_email.assert_called_once() + call_args = mock_keycloak_admin.a_send_verify_email.call_args + # Verify that verify_email was called with is_auth_flow=True + # We check this indirectly by verifying the redirect_uri + assert 'email_verified=true' in call_args.kwargs['redirect_uri'] + + +@pytest.mark.asyncio +async def test_resend_email_verification_with_is_auth_flow_false(mock_request): + """Test resend_email_verification uses default is_auth_flow=False when not specified.""" + # Arrange + user_id = 'test_user_id' + body = ResendEmailVerificationRequest(user_id=user_id, is_auth_flow=False) + mock_keycloak_admin = AsyncMock() + mock_keycloak_admin.a_send_verify_email = AsyncMock() + + with ( + patch('server.routes.email.check_rate_limit_by_user_id') as mock_rate_limit, + patch( + 'server.routes.email.get_keycloak_admin', return_value=mock_keycloak_admin + ), + ): + mock_rate_limit.return_value = None + + # Act + await resend_email_verification(request=mock_request, body=body) + + # Assert + mock_keycloak_admin.a_send_verify_email.assert_called_once() + call_args = mock_keycloak_admin.a_send_verify_email.call_args + # Verify that verify_email was called with is_auth_flow=False + assert '/api/email/verified' in call_args.kwargs['redirect_uri'] + + +@pytest.mark.asyncio +async def test_resend_email_verification_body_none_uses_auth(mock_request): + """Test resend_email_verification uses auth when body is None.""" + # Arrange + user_id = 'test_user_id' + mock_keycloak_admin = AsyncMock() + mock_keycloak_admin.a_send_verify_email = AsyncMock() + + with ( + patch( + 'server.routes.email.get_user_id', return_value=user_id + ) as mock_get_user_id, + patch('server.routes.email.check_rate_limit_by_user_id') as mock_rate_limit, + patch( + 'server.routes.email.get_keycloak_admin', return_value=mock_keycloak_admin + ), + ): + mock_rate_limit.return_value = None + + # Act + result = await resend_email_verification(request=mock_request, body=None) + + # Assert + assert isinstance(result, JSONResponse) + assert result.status_code == status.HTTP_200_OK + mock_get_user_id.assert_called_once() + mock_rate_limit.assert_called_once_with( + request=mock_request, + key_prefix='email_resend', + user_id=user_id, + user_rate_limit_seconds=30, + ip_rate_limit_seconds=60, + ) diff --git a/enterprise/tests/unit/server/test_rate_limit_utils.py b/enterprise/tests/unit/server/test_rate_limit_utils.py new file mode 100644 index 0000000000..37c83bc6c8 --- /dev/null +++ b/enterprise/tests/unit/server/test_rate_limit_utils.py @@ -0,0 +1,290 @@ +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +from fastapi import HTTPException, Request, status +from server.utils.rate_limit_utils import ( + RATE_LIMIT_IP_SECONDS, + RATE_LIMIT_USER_SECONDS, + check_rate_limit_by_user_id, +) + + +@pytest.fixture +def mock_request(): + """Create a mock request object.""" + request = MagicMock(spec=Request) + request.client = MagicMock() + request.client.host = '192.168.1.1' + return request + + +@pytest.fixture +def mock_redis(): + """Create a mock Redis client.""" + redis = AsyncMock() + redis.set = AsyncMock(return_value=True) # First call succeeds (key doesn't exist) + return redis + + +@pytest.mark.asyncio +async def test_rate_limit_by_user_id_first_request_succeeds(mock_request, mock_redis): + """Test that first request with user_id succeeds and sets rate limit key.""" + # Arrange + user_id = 'test_user_id' + key_prefix = 'email_resend' + + with ( + patch('server.utils.rate_limit_utils.sio') as mock_sio, + patch('server.utils.rate_limit_utils.logger') as mock_logger, + ): + mock_sio.manager.redis = mock_redis + + # Act + await check_rate_limit_by_user_id( + request=mock_request, key_prefix=key_prefix, user_id=user_id + ) + + # Assert + mock_redis.set.assert_called_once_with( + f'{key_prefix}:{user_id}', 1, nx=True, ex=RATE_LIMIT_USER_SECONDS + ) + mock_logger.warning.assert_not_called() + mock_logger.info.assert_not_called() + + +@pytest.mark.asyncio +async def test_rate_limit_by_user_id_second_request_within_window_fails( + mock_request, mock_redis +): + """Test that second request with same user_id within rate limit window fails.""" + # Arrange + user_id = 'test_user_id' + key_prefix = 'email_resend' + mock_redis.set = AsyncMock(return_value=False) # Key already exists + + with ( + patch('server.utils.rate_limit_utils.sio') as mock_sio, + patch('server.utils.rate_limit_utils.logger') as mock_logger, + ): + mock_sio.manager.redis = mock_redis + + # Act & Assert + with pytest.raises(HTTPException) as exc_info: + await check_rate_limit_by_user_id( + request=mock_request, key_prefix=key_prefix, user_id=user_id + ) + + assert exc_info.value.status_code == status.HTTP_429_TOO_MANY_REQUESTS + assert 'Too many requests' in exc_info.value.detail + assert f'{RATE_LIMIT_USER_SECONDS // 60} minutes' in exc_info.value.detail + mock_logger.info.assert_called_once() + + +@pytest.mark.asyncio +async def test_rate_limit_by_ip_when_user_id_is_none(mock_request, mock_redis): + """Test that rate limiting falls back to IP address when user_id is None.""" + # Arrange + key_prefix = 'email_resend' + + with ( + patch('server.utils.rate_limit_utils.sio') as mock_sio, + patch('server.utils.rate_limit_utils.logger') as mock_logger, + ): + mock_sio.manager.redis = mock_redis + + # Act + await check_rate_limit_by_user_id( + request=mock_request, key_prefix=key_prefix, user_id=None + ) + + # Assert + mock_redis.set.assert_called_once_with( + f'{key_prefix}:ip:{mock_request.client.host}', + 1, + nx=True, + ex=RATE_LIMIT_IP_SECONDS, + ) + mock_logger.warning.assert_not_called() + + +@pytest.mark.asyncio +async def test_rate_limit_by_ip_second_request_within_window_fails( + mock_request, mock_redis +): + """Test that second request from same IP within rate limit window fails.""" + # Arrange + key_prefix = 'email_resend' + mock_redis.set = AsyncMock(return_value=False) # Key already exists + + with ( + patch('server.utils.rate_limit_utils.sio') as mock_sio, + ): + mock_sio.manager.redis = mock_redis + + # Act & Assert + with pytest.raises(HTTPException) as exc_info: + await check_rate_limit_by_user_id( + request=mock_request, key_prefix=key_prefix, user_id=None + ) + + assert exc_info.value.status_code == status.HTTP_429_TOO_MANY_REQUESTS + assert f'{RATE_LIMIT_IP_SECONDS // 60} minutes' in exc_info.value.detail + + +@pytest.mark.asyncio +async def test_rate_limit_redis_unavailable_fails_open(mock_request): + """Test that rate limiting fails open when Redis is unavailable.""" + # Arrange + key_prefix = 'email_resend' + user_id = 'test_user_id' + + with ( + patch('server.utils.rate_limit_utils.sio') as mock_sio, + patch('server.utils.rate_limit_utils.logger') as mock_logger, + ): + mock_sio.manager.redis = None # Redis unavailable + + # Act + await check_rate_limit_by_user_id( + request=mock_request, key_prefix=key_prefix, user_id=user_id + ) + + # Assert + mock_logger.warning.assert_called_once_with( + 'Redis unavailable for rate limiting, allowing request' + ) + + +@pytest.mark.asyncio +async def test_rate_limit_redis_exception_fails_open(mock_request, mock_redis): + """Test that rate limiting fails open when Redis raises an exception.""" + # Arrange + key_prefix = 'email_resend' + user_id = 'test_user_id' + mock_redis.set = AsyncMock(side_effect=Exception('Redis connection error')) + + with ( + patch('server.utils.rate_limit_utils.sio') as mock_sio, + patch('server.utils.rate_limit_utils.logger') as mock_logger, + ): + mock_sio.manager.redis = mock_redis + + # Act + await check_rate_limit_by_user_id( + request=mock_request, key_prefix=key_prefix, user_id=user_id + ) + + # Assert + mock_logger.warning.assert_called_once() + assert 'Error checking rate limit' in str(mock_logger.warning.call_args[0][0]) + + +@pytest.mark.asyncio +async def test_rate_limit_custom_key_prefix(mock_request, mock_redis): + """Test that different key prefixes create different rate limit keys.""" + # Arrange + user_id = 'test_user_id' + key_prefix = 'password_reset' + + with patch('server.utils.rate_limit_utils.sio') as mock_sio: + mock_sio.manager.redis = mock_redis + + # Act + await check_rate_limit_by_user_id( + request=mock_request, key_prefix=key_prefix, user_id=user_id + ) + + # Assert + mock_redis.set.assert_called_once_with( + f'{key_prefix}:{user_id}', 1, nx=True, ex=RATE_LIMIT_USER_SECONDS + ) + + +@pytest.mark.asyncio +async def test_rate_limit_custom_rate_limit_seconds(mock_request, mock_redis): + """Test that custom rate limit seconds are used correctly.""" + # Arrange + user_id = 'test_user_id' + key_prefix = 'email_resend' + custom_user_seconds = 60 + custom_ip_seconds = 180 + + with patch('server.utils.rate_limit_utils.sio') as mock_sio: + mock_sio.manager.redis = mock_redis + + # Act + await check_rate_limit_by_user_id( + request=mock_request, + key_prefix=key_prefix, + user_id=user_id, + user_rate_limit_seconds=custom_user_seconds, + ip_rate_limit_seconds=custom_ip_seconds, + ) + + # Assert + mock_redis.set.assert_called_once_with( + f'{key_prefix}:{user_id}', 1, nx=True, ex=custom_user_seconds + ) + + +@pytest.mark.asyncio +async def test_rate_limit_ip_with_unknown_client(mock_request, mock_redis): + """Test that rate limiting handles missing client host gracefully.""" + # Arrange + key_prefix = 'email_resend' + mock_request.client = None # No client information + + with patch('server.utils.rate_limit_utils.sio') as mock_sio: + mock_sio.manager.redis = mock_redis + + # Act + await check_rate_limit_by_user_id( + request=mock_request, key_prefix=key_prefix, user_id=None + ) + + # Assert + mock_redis.set.assert_called_once_with( + f'{key_prefix}:ip:unknown', 1, nx=True, ex=RATE_LIMIT_IP_SECONDS + ) + + +@pytest.mark.asyncio +async def test_rate_limit_different_users_have_separate_limits( + mock_request, mock_redis +): + """Test that different user_ids have separate rate limit keys.""" + # Arrange + key_prefix = 'email_resend' + user_id_1 = 'user_1' + user_id_2 = 'user_2' + + with patch('server.utils.rate_limit_utils.sio') as mock_sio: + mock_sio.manager.redis = mock_redis + + # Act + await check_rate_limit_by_user_id( + request=mock_request, key_prefix=key_prefix, user_id=user_id_1 + ) + await check_rate_limit_by_user_id( + request=mock_request, key_prefix=key_prefix, user_id=user_id_2 + ) + + # Assert + assert mock_redis.set.call_count == 2 + # Extract call arguments properly + call_args_list = [ + (call[0][0], call[0][1], call[1]['nx'], call[1]['ex']) + for call in mock_redis.set.call_args_list + ] + assert ( + f'{key_prefix}:{user_id_1}', + 1, + True, + RATE_LIMIT_USER_SECONDS, + ) in call_args_list + assert ( + f'{key_prefix}:{user_id_2}', + 1, + True, + RATE_LIMIT_USER_SECONDS, + ) in call_args_list diff --git a/enterprise/tests/unit/test_auth_middleware.py b/enterprise/tests/unit/test_auth_middleware.py index 1a0729c88f..86da5b2753 100644 --- a/enterprise/tests/unit/test_auth_middleware.py +++ b/enterprise/tests/unit/test_auth_middleware.py @@ -234,3 +234,53 @@ async def test_middleware_with_other_auth_error(middleware, mock_request): assert 'set-cookie' in result.headers # Logger should be called for non-NoCredentialsError mock_logger.warning.assert_called_once() + + +@pytest.mark.asyncio +async def test_middleware_ignores_email_resend_path( + middleware, mock_request, mock_response +): + """Test middleware ignores /api/email/resend path and doesn't require authentication.""" + # Arrange + mock_request.cookies = {} + mock_request.url = MagicMock() + mock_request.url.hostname = 'localhost' + mock_request.url.path = '/api/email/resend' + mock_call_next = AsyncMock(return_value=mock_response) + + # Act + result = await middleware(mock_request, mock_call_next) + + # Assert + assert result == mock_response + mock_call_next.assert_called_once_with(mock_request) + # Should not raise NoCredentialsError even without auth cookie + + +@pytest.mark.asyncio +async def test_middleware_ignores_email_resend_path_no_tos_check( + middleware, mock_request, mock_response +): + """Test middleware doesn't check TOS for /api/email/resend path.""" + # Arrange + mock_request.cookies = {'keycloak_auth': 'test_cookie'} + mock_request.url = MagicMock() + mock_request.url.hostname = 'localhost' + mock_request.url.path = '/api/email/resend' + mock_call_next = AsyncMock(return_value=mock_response) + + with ( + patch('server.middleware.jwt.decode') as mock_decode, + patch('server.middleware.config') as mock_config, + ): + # Even with accepted_tos=False, should not raise TosNotAcceptedError + mock_decode.return_value = {'accepted_tos': False} + mock_config.jwt_secret.get_secret_value.return_value = 'test_secret' + + # Act + result = await middleware(mock_request, mock_call_next) + + # Assert + assert result == mock_response + mock_call_next.assert_called_once_with(mock_request) + # Should not raise TosNotAcceptedError for this path diff --git a/enterprise/tests/unit/test_auth_routes.py b/enterprise/tests/unit/test_auth_routes.py index 8490d92760..362940b1f7 100644 --- a/enterprise/tests/unit/test_auth_routes.py +++ b/enterprise/tests/unit/test_auth_routes.py @@ -249,6 +249,7 @@ async def test_keycloak_callback_email_not_verified(mock_request): assert isinstance(result, RedirectResponse) assert result.status_code == 302 assert 'email_verification_required=true' in result.headers['location'] + assert 'user_id=test_user_id' in result.headers['location'] mock_verify_email.assert_called_once_with( request=mock_request, user_id='test_user_id', is_auth_flow=True ) @@ -287,6 +288,7 @@ async def test_keycloak_callback_email_not_verified_missing_field(mock_request): assert isinstance(result, RedirectResponse) assert result.status_code == 302 assert 'email_verification_required=true' in result.headers['location'] + assert 'user_id=test_user_id' in result.headers['location'] mock_verify_email.assert_called_once_with( request=mock_request, user_id='test_user_id', is_auth_flow=True ) diff --git a/frontend/__tests__/components/features/waitlist/email-verification-modal.test.tsx b/frontend/__tests__/components/features/waitlist/email-verification-modal.test.tsx index e773461d84..c62f85036b 100644 --- a/frontend/__tests__/components/features/waitlist/email-verification-modal.test.tsx +++ b/frontend/__tests__/components/features/waitlist/email-verification-modal.test.tsx @@ -1,15 +1,34 @@ -import { render, screen } from "@testing-library/react"; -import { it, describe, expect, vi, beforeEach } from "vitest"; +import React from "react"; +import { screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { MemoryRouter } from "react-router"; +import { emailService } from "#/api/email-service/email-service.api"; import { EmailVerificationModal } from "#/components/features/waitlist/email-verification-modal"; +import * as ToastHandlers from "#/utils/custom-toast-handlers"; +import { renderWithProviders, createAxiosError } from "../../../../test-utils"; describe("EmailVerificationModal", () => { + const mockOnClose = vi.fn(); + + const resendEmailVerificationSpy = vi.spyOn( + emailService, + "resendEmailVerification", + ); + const displaySuccessToastSpy = vi.spyOn(ToastHandlers, "displaySuccessToast"); + const displayErrorToastSpy = vi.spyOn(ToastHandlers, "displayErrorToast"); + + const renderWithRouter = (ui: React.ReactElement) => { + return renderWithProviders({ui}); + }; + beforeEach(() => { vi.clearAllMocks(); }); it("should render the email verification message", () => { // Arrange & Act - render(); + renderWithRouter(); // Assert expect( @@ -19,10 +38,150 @@ describe("EmailVerificationModal", () => { it("should render the TermsAndPrivacyNotice component", () => { // Arrange & Act - render(); + renderWithRouter(); // Assert const termsSection = screen.getByTestId("terms-and-privacy-notice"); expect(termsSection).toBeInTheDocument(); }); + + it("should render resend verification button", () => { + // Arrange & Act + renderWithRouter(); + + // Assert + expect( + screen.getByText("SETTINGS$RESEND_VERIFICATION"), + ).toBeInTheDocument(); + }); + + it("should call resendEmailVerification when the button is clicked", async () => { + // Arrange + const userId = "test_user_id"; + resendEmailVerificationSpy.mockResolvedValue({ + message: "Email verification message sent", + }); + renderWithRouter( + , + ); + + // Act + const resendButton = screen.getByText("SETTINGS$RESEND_VERIFICATION"); + await userEvent.click(resendButton); + + // Assert + await waitFor(() => { + expect(resendEmailVerificationSpy).toHaveBeenCalledWith({ + userId, + isAuthFlow: true, + }); + }); + }); + + it("should display success toast when resend succeeds", async () => { + // Arrange + resendEmailVerificationSpy.mockResolvedValue({ + message: "Email verification message sent", + }); + renderWithRouter(); + + // Act + const resendButton = screen.getByText("SETTINGS$RESEND_VERIFICATION"); + await userEvent.click(resendButton); + + // Assert + await waitFor(() => { + expect(displaySuccessToastSpy).toHaveBeenCalledWith( + "SETTINGS$VERIFICATION_EMAIL_SENT", + ); + }); + }); + + it("should display rate limit error message when receiving 429 status", async () => { + // Arrange + const rateLimitError = createAxiosError(429, "Too Many Requests", { + detail: "Too many requests. Please wait 2 minutes before trying again.", + }); + resendEmailVerificationSpy.mockRejectedValue(rateLimitError); + renderWithRouter(); + + // Act + const resendButton = screen.getByText("SETTINGS$RESEND_VERIFICATION"); + await userEvent.click(resendButton); + + // Assert + await waitFor(() => { + expect(displayErrorToastSpy).toHaveBeenCalledWith( + "Too many requests. Please wait 2 minutes before trying again.", + ); + }); + }); + + it("should display generic error message when receiving non-429 error", async () => { + // Arrange + const genericError = createAxiosError(500, "Internal Server Error", { + error: "Internal server error", + }); + resendEmailVerificationSpy.mockRejectedValue(genericError); + renderWithRouter(); + + // Act + const resendButton = screen.getByText("SETTINGS$RESEND_VERIFICATION"); + await userEvent.click(resendButton); + + // Assert + await waitFor(() => { + expect(displayErrorToastSpy).toHaveBeenCalledWith( + "SETTINGS$FAILED_TO_RESEND_VERIFICATION", + ); + }); + }); + + it("should disable button and show sending text while request is pending", async () => { + // Arrange + let resolvePromise: (value: { message: string }) => void; + const pendingPromise = new Promise<{ message: string }>((resolve) => { + resolvePromise = resolve; + }); + resendEmailVerificationSpy.mockReturnValue(pendingPromise); + renderWithRouter(); + + // Act + const resendButton = screen.getByText("SETTINGS$RESEND_VERIFICATION"); + await userEvent.click(resendButton); + + // Assert + await waitFor(() => { + const sendingButton = screen.getByText("SETTINGS$SENDING"); + expect(sendingButton).toBeInTheDocument(); + expect(sendingButton).toBeDisabled(); + }); + + // Cleanup + resolvePromise!({ message: "Email verification message sent" }); + }); + + it("should re-enable button after request completes", async () => { + // Arrange + resendEmailVerificationSpy.mockResolvedValue({ + message: "Email verification message sent", + }); + renderWithRouter(); + + // Act + const resendButton = screen.getByText("SETTINGS$RESEND_VERIFICATION"); + await userEvent.click(resendButton); + + // Assert + await waitFor(() => { + expect(resendEmailVerificationSpy).toHaveBeenCalled(); + }); + // After successful send, the button will be disabled due to cooldown + // So we just verify the mutation was called successfully + await waitFor(() => { + const button = screen.getByRole("button"); + expect(button).toBeDisabled(); // Button is disabled during cooldown + expect(button).toHaveTextContent(/SETTINGS\$RESEND_VERIFICATION/); + }); + }); }); diff --git a/frontend/src/api/email-service/email-service.api.ts b/frontend/src/api/email-service/email-service.api.ts new file mode 100644 index 0000000000..d87b3c392f --- /dev/null +++ b/frontend/src/api/email-service/email-service.api.ts @@ -0,0 +1,35 @@ +import { openHands } from "../open-hands-axios"; +import { + ResendEmailVerificationParams, + ResendEmailVerificationResponse, +} from "./email.types"; + +/** + * Email Service API - Handles all email-related API endpoints + */ +export const emailService = { + /** + * Resend email verification to the user's registered email address + * @param userId - Optional user ID to send verification email for + * @param isAuthFlow - Whether this is part of the authentication flow + * @returns The response message indicating the email was sent + */ + resendEmailVerification: async ({ + userId, + isAuthFlow, + }: ResendEmailVerificationParams): Promise => { + const body: { user_id?: string; is_auth_flow?: boolean } = {}; + if (userId) { + body.user_id = userId; + } + if (isAuthFlow !== undefined) { + body.is_auth_flow = isAuthFlow; + } + const { data } = await openHands.put( + "/api/email/resend", + body, + { withCredentials: true }, + ); + return data; + }, +}; diff --git a/frontend/src/api/email-service/email.types.ts b/frontend/src/api/email-service/email.types.ts new file mode 100644 index 0000000000..e56dbb5ff4 --- /dev/null +++ b/frontend/src/api/email-service/email.types.ts @@ -0,0 +1,8 @@ +export interface ResendEmailVerificationParams { + userId?: string | null; + isAuthFlow?: boolean; +} + +export interface ResendEmailVerificationResponse { + message: string; +} diff --git a/frontend/src/components/features/waitlist/email-verification-modal.tsx b/frontend/src/components/features/waitlist/email-verification-modal.tsx index 820dce3258..fafd11ee4e 100644 --- a/frontend/src/components/features/waitlist/email-verification-modal.tsx +++ b/frontend/src/components/features/waitlist/email-verification-modal.tsx @@ -4,15 +4,34 @@ import OpenHandsLogo from "#/assets/branding/openhands-logo.svg?react"; import { ModalBackdrop } from "#/components/shared/modals/modal-backdrop"; import { ModalBody } from "#/components/shared/modals/modal-body"; import { TermsAndPrivacyNotice } from "#/components/shared/terms-and-privacy-notice"; +import { BrandButton } from "../settings/brand-button"; +import { useEmailVerification } from "#/hooks/use-email-verification"; interface EmailVerificationModalProps { onClose: () => void; + userId?: string | null; } export function EmailVerificationModal({ onClose, + userId, }: EmailVerificationModalProps) { const { t } = useTranslation(); + const { + resendEmailVerification, + isResendingVerification: isResending, + isCooldownActive, + formattedCooldownTime, + } = useEmailVerification(); + + let resendButtonLabel: string; + if (isResending) { + resendButtonLabel = t(I18nKey.SETTINGS$SENDING); + } else if (isCooldownActive) { + resendButtonLabel = `${t(I18nKey.SETTINGS$RESEND_VERIFICATION)} (${formattedCooldownTime})`; + } else { + resendButtonLabel = t(I18nKey.SETTINGS$RESEND_VERIFICATION); + } return ( @@ -24,6 +43,20 @@ export function EmailVerificationModal({ +
+ + resendEmailVerification({ userId, isAuthFlow: true }) + } + isDisabled={isResending || isCooldownActive} + className="w-full font-semibold" + > + {resendButtonLabel} + +
+ diff --git a/frontend/src/hooks/mutation/use-resend-email-verification.ts b/frontend/src/hooks/mutation/use-resend-email-verification.ts new file mode 100644 index 0000000000..2a8b8c4bb1 --- /dev/null +++ b/frontend/src/hooks/mutation/use-resend-email-verification.ts @@ -0,0 +1,49 @@ +import { useMutation } from "@tanstack/react-query"; +import { AxiosError } from "axios"; +import { useTranslation } from "react-i18next"; +import { I18nKey } from "#/i18n/declaration"; +import { emailService } from "#/api/email-service/email-service.api"; +import { + displaySuccessToast, + displayErrorToast, +} from "#/utils/custom-toast-handlers"; +import { retrieveAxiosErrorMessage } from "#/utils/retrieve-axios-error-message"; +import { ResendEmailVerificationParams } from "#/api/email-service/email.types"; + +interface UseResendEmailVerificationOptions { + onSuccess?: () => void; +} + +export const useResendEmailVerification = ( + options?: UseResendEmailVerificationOptions, +) => { + const { t } = useTranslation(); + + return useMutation({ + mutationFn: (params: ResendEmailVerificationParams) => + emailService.resendEmailVerification(params), + onSuccess: () => { + displaySuccessToast(t(I18nKey.SETTINGS$VERIFICATION_EMAIL_SENT)); + options?.onSuccess?.(); + }, + onError: (error: AxiosError) => { + // Check if it's a rate limit error (429) + if (error.response?.status === 429) { + // FastAPI returns errors in { detail: "..." } format + const errorData = error.response.data as + | { detail?: string } + | undefined; + + const rateLimitMessage = + errorData?.detail || + retrieveAxiosErrorMessage(error) || + t(I18nKey.SETTINGS$FAILED_TO_RESEND_VERIFICATION); + + displayErrorToast(rateLimitMessage); + } else { + // For other errors, show the generic error message + displayErrorToast(t(I18nKey.SETTINGS$FAILED_TO_RESEND_VERIFICATION)); + } + }, + }); +}; diff --git a/frontend/src/hooks/use-email-verification.ts b/frontend/src/hooks/use-email-verification.ts index c0068395b5..29ed876295 100644 --- a/frontend/src/hooks/use-email-verification.ts +++ b/frontend/src/hooks/use-email-verification.ts @@ -1,10 +1,12 @@ import React from "react"; import { useSearchParams } from "react-router"; +import { useResendEmailVerification } from "#/hooks/mutation/use-resend-email-verification"; /** * Hook to handle email verification logic from URL query parameters. * Manages the email verification modal state and email verified state * based on query parameters in the URL. + * Also provides functionality to resend email verification. * * @returns An object containing: * - emailVerificationModalOpen: boolean state for modal visibility @@ -12,6 +14,12 @@ import { useSearchParams } from "react-router"; * - emailVerified: boolean state for email verification status * - setEmailVerified: function to control email verification status * - hasDuplicatedEmail: boolean state for duplicate email error status + * - userId: string | null for the user ID from the redirect URL + * - resendEmailVerification: function to resend verification email + * - isResendingVerification: boolean indicating if resend is in progress + * - isCooldownActive: boolean indicating if cooldown is currently active + * - cooldownRemaining: number of milliseconds remaining in cooldown + * - formattedCooldownTime: string formatted as "M:SS" for display */ export function useEmailVerification() { const [searchParams, setSearchParams] = useSearchParams(); @@ -19,6 +27,26 @@ export function useEmailVerification() { React.useState(false); const [emailVerified, setEmailVerified] = React.useState(false); const [hasDuplicatedEmail, setHasDuplicatedEmail] = React.useState(false); + const [userId, setUserId] = React.useState(null); + const [lastSentTimestamp, setLastSentTimestamp] = React.useState< + number | null + >(null); + const [cooldownRemaining, setCooldownRemaining] = React.useState(0); + + const COOLDOWN_DURATION_MS = 30 * 1000; // 30 seconds + + const formatCooldownTime = (ms: number): string => { + const seconds = Math.ceil(ms / 1000); + const minutes = Math.floor(seconds / 60); + const remainingSeconds = seconds % 60; + return `${minutes}:${remainingSeconds.toString().padStart(2, "0")}`; + }; + + const resendEmailVerificationMutation = useResendEmailVerification({ + onSuccess: () => { + setLastSentTimestamp(Date.now()); + }, + }); // Check for email verification query parameters React.useEffect(() => { @@ -27,6 +55,7 @@ export function useEmailVerification() { ); const emailVerifiedParam = searchParams.get("email_verified"); const duplicatedEmailParam = searchParams.get("duplicated_email"); + const userIdParam = searchParams.get("user_id"); let shouldUpdate = false; if (emailVerificationRequired === "true") { @@ -47,17 +76,61 @@ export function useEmailVerification() { shouldUpdate = true; } + if (userIdParam) { + setUserId(userIdParam); + searchParams.delete("user_id"); + shouldUpdate = true; + } + // Clean up the URL by removing parameters if any were found if (shouldUpdate) { setSearchParams(searchParams, { replace: true }); } }, [searchParams, setSearchParams]); + // Update cooldown remaining time + React.useEffect(() => { + if (lastSentTimestamp === null) { + setCooldownRemaining(0); + return undefined; + } + + let timeoutId: NodeJS.Timeout | null = null; + + const updateCooldown = () => { + const elapsed = Date.now() - lastSentTimestamp!; + const remaining = Math.max(0, COOLDOWN_DURATION_MS - elapsed); + setCooldownRemaining(remaining); + + if (remaining > 0) { + // Update every second while cooldown is active + timeoutId = setTimeout(updateCooldown, 1000); + } + }; + + updateCooldown(); + + return () => { + if (timeoutId) { + clearTimeout(timeoutId); + } + }; + }, [lastSentTimestamp, COOLDOWN_DURATION_MS]); + + const isCooldownActive = cooldownRemaining > 0; + const formattedCooldownTime = formatCooldownTime(cooldownRemaining); + return { emailVerificationModalOpen, setEmailVerificationModalOpen, emailVerified, setEmailVerified, hasDuplicatedEmail, + userId, + resendEmailVerification: resendEmailVerificationMutation.mutate, + isResendingVerification: resendEmailVerificationMutation.isPending, + isCooldownActive, + cooldownRemaining, + formattedCooldownTime, }; } diff --git a/frontend/src/routes/root-layout.tsx b/frontend/src/routes/root-layout.tsx index 73da04ea8f..2c5fa735d0 100644 --- a/frontend/src/routes/root-layout.tsx +++ b/frontend/src/routes/root-layout.tsx @@ -98,6 +98,7 @@ export default function MainApp() { setEmailVerificationModalOpen, emailVerified, hasDuplicatedEmail, + userId, } = useEmailVerification(); // Auto-login if login method is stored in local storage @@ -254,6 +255,7 @@ export default function MainApp() { onClose={() => { setEmailVerificationModalOpen(false); }} + userId={userId} /> )} {config.data?.APP_MODE === "oss" && consentFormIsOpen && ( diff --git a/frontend/src/routes/user-settings.tsx b/frontend/src/routes/user-settings.tsx index cddc38466e..3e40d104a8 100644 --- a/frontend/src/routes/user-settings.tsx +++ b/frontend/src/routes/user-settings.tsx @@ -4,6 +4,7 @@ import { useQueryClient } from "@tanstack/react-query"; import { useSettings } from "#/hooks/query/use-settings"; import { openHands } from "#/api/open-hands-axios"; import { displaySuccessToast } from "#/utils/custom-toast-handlers"; +import { useEmailVerification } from "#/hooks/use-email-verification"; // Email validation regex pattern const EMAIL_REGEX = /^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$/; @@ -115,11 +116,12 @@ function UserSettingsScreen() { const [email, setEmail] = useState(""); const [originalEmail, setOriginalEmail] = useState(""); const [isSaving, setIsSaving] = useState(false); - const [isResendingVerification, setIsResendingVerification] = useState(false); const [isEmailValid, setIsEmailValid] = useState(true); const queryClient = useQueryClient(); const pollingIntervalRef = useRef(null); const prevVerificationStatusRef = useRef(undefined); + const { resendEmailVerification, isResendingVerification } = + useEmailVerification(); useEffect(() => { if (settings?.email) { @@ -185,18 +187,8 @@ function UserSettingsScreen() { } }; - const handleResendVerification = async () => { - try { - setIsResendingVerification(true); - await openHands.put("/api/email/verify", {}, { withCredentials: true }); - // Display toast notification instead of setting state - displaySuccessToast(t("SETTINGS$VERIFICATION_EMAIL_SENT")); - } catch (error) { - // eslint-disable-next-line no-console - console.error(t("SETTINGS$FAILED_TO_RESEND_VERIFICATION"), error); - } finally { - setIsResendingVerification(false); - } + const handleResendVerification = () => { + resendEmailVerification({}); }; const isEmailChanged = email !== originalEmail; diff --git a/frontend/test-utils.tsx b/frontend/test-utils.tsx index 87c5ce691b..a55cb7d132 100644 --- a/frontend/test-utils.tsx +++ b/frontend/test-utils.tsx @@ -74,3 +74,23 @@ export const createAxiosNotFoundErrorObject = () => config: {}, }, ); + +export const createAxiosError = ( + status: number, + statusText: string, + data: unknown, +) => + new AxiosError( + `Request failed with status code ${status}`, + "ERR_BAD_REQUEST", + undefined, + undefined, + { + status, + statusText, + data, + headers: {}, + // @ts-expect-error - we only need the response object for this test + config: {}, + }, + );