diff --git a/routers/biocommons_register.py b/routers/biocommons_register.py index 3d775744..5a546cef 100644 --- a/routers/biocommons_register.py +++ b/routers/biocommons_register.py @@ -13,9 +13,14 @@ from db.models import BiocommonsGroup, BiocommonsUser from db.setup import get_db_session from routers.errors import RegistrationRoute +from routers.utils import check_existing_user from schemas.biocommons import Auth0UserData, BiocommonsRegisterData from schemas.biocommons_register import BiocommonsRegistrationRequest -from schemas.responses import RegistrationErrorResponse, RegistrationResponse +from schemas.responses import ( + FieldError, + RegistrationErrorResponse, + RegistrationResponse, +) from services.email_queue import enqueue_email logger = logging.getLogger(__name__) @@ -140,7 +145,29 @@ async def register_biocommons_user( logger.error(f"Auth0 registration failed: {e}") # Catch specific errors where possible and return a useful error message if e.response.status_code == 409: - response = RegistrationErrorResponse(message="Username or email already in use") + existing_field = check_existing_user(registration.username, registration.email, auth0_client) + field_errors = [] + if existing_field == "username": + field_errors.append(FieldError(field="username", message="Username is already taken")) + response = RegistrationErrorResponse( + message="Username is already taken", + field_errors=field_errors + ) + elif existing_field == "email": + field_errors.append(FieldError(field="email", message="Email is already taken")) + response = RegistrationErrorResponse( + message="Email is already taken", + field_errors=field_errors + ) + elif existing_field == "both": + field_errors.append(FieldError(field="username", message="Username is already taken")) + field_errors.append(FieldError(field="email", message="Email is already taken")) + response = RegistrationErrorResponse( + message="Username and email are already taken", + field_errors=field_errors + ) + else: + response = RegistrationErrorResponse(message="Username or email is already taken") else: response = RegistrationErrorResponse(message=f"Auth0 error: {str(e.response.text)}") return JSONResponse(status_code=400, content=response.model_dump(mode="json")) diff --git a/routers/sbp_register.py b/routers/sbp_register.py index de5b6ac2..9d4e9f24 100644 --- a/routers/sbp_register.py +++ b/routers/sbp_register.py @@ -11,8 +11,13 @@ from db.models import BiocommonsUser, PlatformEnum from db.setup import get_db_session from routers.errors import RegistrationRoute +from routers.utils import check_existing_user from schemas.biocommons import Auth0UserData, BiocommonsRegisterData -from schemas.responses import RegistrationErrorResponse, RegistrationResponse +from schemas.responses import ( + FieldError, + RegistrationErrorResponse, + RegistrationResponse, +) from schemas.sbp import SBPRegistrationRequest from services.email_queue import enqueue_email @@ -106,7 +111,29 @@ async def register_sbp_user( except HTTPStatusError as e: # Catch specific errors where possible and return a useful error message if e.response.status_code == 409: - response = RegistrationErrorResponse(message="Username or email already in use") + existing_field = check_existing_user(registration.username, registration.email, auth0_client) + field_errors = [] + if existing_field == "username": + field_errors.append(FieldError(field="username", message="Username is already taken")) + response = RegistrationErrorResponse( + message="Username is already taken", + field_errors=field_errors + ) + elif existing_field == "email": + field_errors.append(FieldError(field="email", message="Email is already taken")) + response = RegistrationErrorResponse( + message="Email is already taken", + field_errors=field_errors + ) + elif existing_field == "both": + field_errors.append(FieldError(field="username", message="Username is already taken")) + field_errors.append(FieldError(field="email", message="Email is already taken")) + response = RegistrationErrorResponse( + message="Username and email are already taken", + field_errors=field_errors + ) + else: + response = RegistrationErrorResponse(message="Username or email is already taken") else: response = RegistrationErrorResponse(message=f"Auth0 error: {str(e.response.text)}") return JSONResponse(status_code=400, content=response.model_dump(mode="json")) diff --git a/routers/utils.py b/routers/utils.py index 75ef74b0..5fb52299 100644 --- a/routers/utils.py +++ b/routers/utils.py @@ -1,20 +1,118 @@ -from typing import Annotated +import logging +from typing import Annotated, Literal -from fastapi import APIRouter +from fastapi import APIRouter, Query from fastapi.params import Depends from pydantic import BaseModel from auth0.client import Auth0Client, get_auth0_client from schemas.biocommons import AppId +from schemas.responses import FieldError + +logger = logging.getLogger(__name__) router = APIRouter(prefix="/utils", tags=["utils"]) +def _check_username_exists(username: str, auth0_client: Auth0Client) -> bool: + """Helper function to check if a username exists in Auth0.""" + try: + q = f'username:"{username}"' + res = auth0_client.get_users(q=q) + + users = res if isinstance(res, list) else getattr(res, "users", []) + target = username.lower() + + return any( + getattr(u, "username", "").lower() == target + for u in users + if getattr(u, "username", None) + ) + except Exception as e: + logger.warning(f"Error checking username existence: {e}") + return False + + +def _check_email_exists(email: str, auth0_client: Auth0Client) -> bool: + """Helper function to check if an email exists in Auth0.""" + try: + email_results = auth0_client.search_users_by_email(email) + return len(email_results) > 0 + except Exception as e: + logger.warning(f"Error checking email existence: {e}") + return False + + +def check_existing_user(username: str, email: str, auth0_client: Auth0Client) -> Literal["both", "email", "username"] | None: + """Check if username or email already exists in Auth0. + + Returns: + - "username" if username exists + - "email" if email exists + - "both" if both exist + - None if neither exists + """ + username_exists = _check_username_exists(username, auth0_client) + email_exists = _check_email_exists(email, auth0_client) + + if username_exists and email_exists: + return "both" + elif username_exists: + return "username" + elif email_exists: + return "email" + return None + + class RegistrationInfo(BaseModel): app: AppId = "biocommons" -@router.get("/registration_info") +class AvailabilityResponse(BaseModel): + """Response for checking username/email availability""" + available: bool + field_errors: list[FieldError] = [] + + +@router.get("/register/check-username-availability", response_model=AvailabilityResponse) +async def check_username_availability( + username: Annotated[str, Query(min_length=3, max_length=128)], + auth0_client: Annotated[Auth0Client, Depends(get_auth0_client)], +): + """ + Check if a username is available for registration. + + Returns availability status with field errors if already taken. + """ + exists = _check_username_exists(username, auth0_client) + if exists: + return AvailabilityResponse( + available=False, + field_errors=[FieldError(field="username", message="Username is already taken")] + ) + return AvailabilityResponse(available=True) + + +@router.get("/register/check-email-availability", response_model=AvailabilityResponse) +async def check_email_availability( + email: Annotated[str, Query()], + auth0_client: Annotated[Auth0Client, Depends(get_auth0_client)], +): + """ + Check if an email is available for registration. + + Returns availability status with field errors if already registered. + """ + exists = _check_email_exists(email, auth0_client) + if exists: + return AvailabilityResponse( + available=False, + field_errors=[FieldError(field="email", message="Email is already taken")] + ) + return AvailabilityResponse(available=True) + + +@router.get("/register/registration-info") async def get_registration_info( user_email: str, client: Annotated[Auth0Client, Depends(get_auth0_client)]): diff --git a/tests/test_biocommons_register.py b/tests/test_biocommons_register.py index 09403995..b2569ef9 100644 --- a/tests/test_biocommons_register.py +++ b/tests/test_biocommons_register.py @@ -314,6 +314,69 @@ def test_biocommons_registration_auth0_conflict_error( "User already exists", request=request, response=response ) + mock_auth0_client.get_users.return_value = [Auth0UserDataFactory.build(username="existinguser")] + mock_auth0_client.search_users_by_email.return_value = [] + + registration_data = { + "first_name": "Test", + "last_name": "User", + "email": "existing@example.com", + "username": "existinguser", + "password": "StrongPass1!", + "bundle": "tsi", + } + + response = test_client.post("/biocommons/register", json=registration_data) + + assert response.status_code == 400 + assert response.json()["message"] == "Username is already taken" + + +def test_biocommons_registration_email_conflict_error( + test_client, tsi_group, mock_auth0_client, test_db_session +): + """Test handling of Auth0 conflict error when email exists""" + from httpx import HTTPStatusError, Request, Response + + response = Response(409, json={"error": "user_exists"}) + request = Request("POST", "https://example.com") + mock_auth0_client.create_user.side_effect = HTTPStatusError( + "User already exists", request=request, response=response + ) + + mock_auth0_client.get_users.return_value = [] + mock_auth0_client.search_users_by_email.return_value = [Auth0UserDataFactory.build()] + + registration_data = { + "first_name": "Test", + "last_name": "User", + "email": "existing@example.com", + "username": "newuser", + "password": "StrongPass1!", + "bundle": "tsi", + } + + response = test_client.post("/biocommons/register", json=registration_data) + + assert response.status_code == 400 + assert response.json()["message"] == "Email is already taken" + + +def test_biocommons_registration_both_conflict_error( + test_client, tsi_group, mock_auth0_client, test_db_session +): + """Test handling of Auth0 conflict error when both username and email exist""" + from httpx import HTTPStatusError, Request, Response + + response = Response(409, json={"error": "user_exists"}) + request = Request("POST", "https://example.com") + mock_auth0_client.create_user.side_effect = HTTPStatusError( + "User already exists", request=request, response=response + ) + + mock_auth0_client.get_users.return_value = [Auth0UserDataFactory.build(username="existinguser")] + mock_auth0_client.search_users_by_email.return_value = [Auth0UserDataFactory.build(email="existing@example.com")] + registration_data = { "first_name": "Test", "last_name": "User", @@ -326,7 +389,7 @@ def test_biocommons_registration_auth0_conflict_error( response = test_client.post("/biocommons/register", json=registration_data) assert response.status_code == 400 - assert response.json()["message"] == "Username or email already in use" + assert response.json()["message"] == "Username and email are already taken" def test_biocommons_registration_missing_group_error(test_client, mock_auth0_client): diff --git a/tests/test_sbp_register.py b/tests/test_sbp_register.py index b0bc2db9..6d47016a 100644 --- a/tests/test_sbp_register.py +++ b/tests/test_sbp_register.py @@ -139,10 +139,53 @@ def test_registration_duplicate_user( ) mock_auth0_client.create_user.side_effect = error + mock_auth0_client.get_users.return_value = [] + mock_auth0_client.search_users_by_email.return_value = [Auth0UserDataFactory.build()] + + response = test_client.post("/sbp/register", json=valid_registration_data) + + assert response.status_code == 400 + assert response.json()["message"] == "Email is already taken" + + +def test_registration_duplicate_username( + test_client, valid_registration_data, mock_auth0_client +): + """Test that duplicate username returns specific error message""" + error = httpx.HTTPStatusError( + "User already exists", + request=httpx.Request("POST", "https://api.example.com/data"), + response=httpx.Response(409, text="User already exists"), + ) + mock_auth0_client.create_user.side_effect = error + + mock_auth0_client.get_users.return_value = [Auth0UserDataFactory.build(username=valid_registration_data["username"])] + mock_auth0_client.search_users_by_email.return_value = [] + + response = test_client.post("/sbp/register", json=valid_registration_data) + + assert response.status_code == 400 + assert response.json()["message"] == "Username is already taken" + + +def test_registration_duplicate_both( + test_client, valid_registration_data, mock_auth0_client +): + """Test that duplicate username and email returns specific error message""" + error = httpx.HTTPStatusError( + "User already exists", + request=httpx.Request("POST", "https://api.example.com/data"), + response=httpx.Response(409, text="User already exists"), + ) + mock_auth0_client.create_user.side_effect = error + + mock_auth0_client.get_users.return_value = [Auth0UserDataFactory.build(username=valid_registration_data["username"])] + mock_auth0_client.search_users_by_email.return_value = [Auth0UserDataFactory.build(email=valid_registration_data["email"])] + response = test_client.post("/sbp/register", json=valid_registration_data) assert response.status_code == 400 - assert response.json()["message"] == "Username or email already in use" + assert response.json()["message"] == "Username and email are already taken" def test_registration_auth0_error( diff --git a/tests/test_utils.py b/tests/test_utils.py index 16ea1db2..c40c6b14 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -25,7 +25,7 @@ def test_get_registration_info(override_auth0_client, test_client): user = Auth0UserDataFactory.build(email="user@example.com", app_metadata=app_metadata) override_auth0_client.search_users_by_email.return_value = [user] - resp = test_client.get("/utils/registration_info", params={"user_email": user.email}) + resp = test_client.get("/utils/register/registration-info", params={"user_email": user.email}) assert resp.status_code == 200 data = resp.json() assert data["app"] == "galaxy" @@ -39,7 +39,7 @@ def test_get_registration_info_no_registration_from(override_auth0_client, test_ user = Auth0UserDataFactory.build(email="user@example.com", app_metadata=app_metadata) override_auth0_client.search_users_by_email.return_value = [user] - resp = test_client.get("/utils/registration_info", params={"user_email": user.email}) + resp = test_client.get("/utils/register/registration-info", params={"user_email": user.email}) assert resp.status_code == 200 data = resp.json() assert data["app"] == "biocommons" @@ -52,7 +52,26 @@ def test_get_registration_info_no_user(override_auth0_client, test_client): exists or not) """ override_auth0_client.search_users_by_email.return_value = [] - resp = test_client.get("/utils/registration_info", params={"user_email": "notfound@example.com"}) + resp = test_client.get("/utils/register/registration-info", params={"user_email": "notfound@example.com"}) assert resp.status_code == 200 data = resp.json() assert data["app"] == "biocommons" + + +def test_check_username_exists_exact_match_only(override_auth0_client, test_client): + """Test that username check only matches exact usernames, not partial matches""" + user1 = Auth0UserDataFactory.build(username="testuser") + user2 = Auth0UserDataFactory.build(username="testuser123") + override_auth0_client.get_users.return_value = [user1, user2] + + resp = test_client.get("/utils/register/check-username-availability", params={"username": "testuser"}) + assert resp.status_code == 200 + data = resp.json() + assert data["available"] is False + assert data["field_errors"][0]["message"] == "Username is already taken" + + resp = test_client.get("/utils/register/check-username-availability", params={"username": "testuser99"}) + assert resp.status_code == 200 + data = resp.json() + assert data["available"] is True + assert data["field_errors"] == []