diff --git a/src/nsls2api/api/v1/user_api.py b/src/nsls2api/api/v1/user_api.py index 39f54085..7a28c32f 100644 --- a/src/nsls2api/api/v1/user_api.py +++ b/src/nsls2api/api/v1/user_api.py @@ -9,6 +9,7 @@ bnlpeople_service, person_service, ) +from nsls2api.services.bnlpeople_service import AmbiguousPersonLookupError from nsls2api.services.ldap_service import get_user_info, shape_ldap_response router = fastapi.APIRouter() @@ -16,52 +17,60 @@ @router.get("/person/username/{username}", response_model=Person) async def get_person_from_username(username: str): - bnl_person = await bnlpeople_service.get_person_by_username(username) - print(bnl_person) - if bnl_person: - person = Person( - firstname=bnl_person.FirstName, - lastname=bnl_person.LastName, - email=bnl_person.BNLEmail, - bnl_id=bnl_person.EmployeeNumber, - institution=bnl_person.Institution, - username=bnl_person.ActiveDirectoryName, - cyber_agreement_signed=bnl_person.CyberAgreementSigned, - ) - # If the person is an Employee then set their institution to BNL - if ( - bnl_person.EmployeeStatus == "Active" - and bnl_person.EmployeeType == "Employee" - ): - person.bnl_employee = True - person.institution = "Brookhaven National Laboratory" - return person - else: - return fastapi.responses.JSONResponse( - {"error": f"No people with username {username} found."}, + try: + bnl_person = await bnlpeople_service.get_person_by_username(username) + except LookupError as e: + raise HTTPException( status_code=404, - ) + detail=f"No person with username {username} was found.", + ) from None + + person = Person( + firstname=bnl_person.FirstName, + lastname=bnl_person.LastName, + email=bnl_person.BNLEmail, + bnl_id=bnl_person.EmployeeNumber, + institution=bnl_person.Institution, + username=bnl_person.ActiveDirectoryName, + cyber_agreement_signed=bnl_person.CyberAgreementSigned, + ) + # If the person is an Employee then set their institution to BNL + if ( + bnl_person.EmployeeStatus == "Active" + and bnl_person.EmployeeType == "Employee" + ): + person.bnl_employee = True + person.institution = "Brookhaven National Laboratory" + return person -@router.get("/person/email/{email}") +@router.get("/person/email/{email}", response_model=Person) async def get_person_from_email(email: str): - bnl_person = await bnlpeople_service.get_person_by_email(email) - if bnl_person: - person = Person( - firstname=bnl_person.FirstName, - lastname=bnl_person.LastName, - email=bnl_person.BNLEmail, - bnl_id=bnl_person.EmployeeNumber, - institution=bnl_person.Institution, - username=bnl_person.ActiveDirectoryName, - cyber_agreement_signed=bnl_person.CyberAgreementSigned, - ) - return person - else: - return fastapi.responses.JSONResponse( - {"error": f"No people with username {email} found."}, + try: + bnl_person = await bnlpeople_service.get_person_by_email(email) + except LookupError as e: + raise HTTPException( status_code=404, - ) + detail=f"No person with email {email} was found.", + ) from None + + person = Person( + firstname=bnl_person.FirstName, + lastname=bnl_person.LastName, + email=bnl_person.BNLEmail, + bnl_id=bnl_person.EmployeeNumber, + institution=bnl_person.Institution, + username=bnl_person.ActiveDirectoryName, + cyber_agreement_signed=bnl_person.CyberAgreementSigned, + ) + # If the person is an Employee then set their institution to BNL + if ( + bnl_person.EmployeeStatus == "Active" + and bnl_person.EmployeeType == "Employee" + ): + person.bnl_employee = True + person.institution = "Brookhaven National Laboratory" + return person # TODO: Add back into schema if we decide to use this endpoint. diff --git a/src/nsls2api/services/bnlpeople_service.py b/src/nsls2api/services/bnlpeople_service.py index 20b7a71f..a4d0ae6a 100644 --- a/src/nsls2api/services/bnlpeople_service.py +++ b/src/nsls2api/services/bnlpeople_service.py @@ -9,6 +9,11 @@ base_url = "https://api.bnl.gov/BNLPeople" +class AmbiguousPersonLookupError(Exception): + """Raised when a person lookup returns multiple results (data integrity issue).""" + pass + + async def _call_bnlpeople_webservice(url: str): return await _call_async_webservice_with_client(url, client=httpx_client_wrapper()) @@ -19,12 +24,20 @@ async def get_all_people(): return people -async def get_person_by_username(username: str) -> BNLPerson | None: +async def get_person_by_username(username: str) -> BNLPerson: url = f"{base_url}/api/BNLPeople?accountName={username}" person = await _call_bnlpeople_webservice(url) - if len(person) == 0 or len(person) > 1: - raise LookupError( - f"BNL People could not find a person with a username of '{username}'" + if len(person) == 0: + logger.warning( + f"BNL People API could not find a person with a username of '{username}'" + ) + raise LookupError(f"BNL People API could not find a person with a username of '{username}'") + if len(person) > 1: + logger.error( + f"BNL People API returned {len(person)} people for username '{username}' - ambiguous result" + ) + raise AmbiguousPersonLookupError( + f"BNL People API returned {len(person)} people for username '{username}' - ambiguous result" ) return BNLPerson(**person[0]) @@ -44,7 +57,7 @@ async def get_username_by_id(lifenumber: str) -> str | None: # logger.debug(person) if len(person) == 0 or len(person) > 1: logger.warning( - f"BNL People could not find a person with an employee/life number of '{lifenumber}'" + f"BNL People API could not find a person with an employee/life number of '{lifenumber}'" ) return None @@ -67,17 +80,25 @@ async def get_person_by_id(lifenumber: str) -> BNLPerson | None: if len(person) == 0 or len(person) > 1: raise LookupError( - f"BNL People could not find a person with an employee/life number of '{lifenumber}'" + f"BNL People API could not find a person with an employee/life number of '{lifenumber}'" ) return BNLPerson(**person[0]) -async def get_person_by_email(email: str) -> BNLPerson | None: +async def get_person_by_email(email: str) -> BNLPerson: url = f"{base_url}/api/BNLPeople?email={email}" person = await _call_bnlpeople_webservice(url) - if len(person) == 0 or len(person) > 1: - raise LookupError( - f"BNL People could not find a person with an email of '{email}'" + if len(person) == 0: + logger.warning( + f"BNL People API could not find a person with an email of '{email}'" + ) + raise LookupError(f"BNL People API could not find a person with an email of '{email}'") + if len(person) > 1: + logger.error( + f"BNL People API returned {len(person)} people for email '{email}' - ambiguous result" + ) + raise AmbiguousPersonLookupError( + f"BNL People API returned {len(person)} people for email '{email}' - ambiguous result" ) return BNLPerson(**person[0]) @@ -89,7 +110,7 @@ async def get_people_by_department( people = await _call_bnlpeople_webservice(url) if len(people) == 0: raise LookupError( - f"BNL People could not find a person with the department code of '{department_code}'" + f"BNL People API could not find a person with the department code of '{department_code}'" ) people_in_department = [BNLPerson(**p) for p in people] return people_in_department diff --git a/src/nsls2api/services/person_service.py b/src/nsls2api/services/person_service.py index ee6c4f5a..aa3cae15 100644 --- a/src/nsls2api/services/person_service.py +++ b/src/nsls2api/services/person_service.py @@ -13,6 +13,7 @@ n2sn_service, proposal_service, ) +from nsls2api.services.bnlpeople_service import AmbiguousPersonLookupError from nsls2api.services.pass_service import get_proposals_by_person @@ -37,22 +38,11 @@ async def diagnostic_details_by_username(username: str) -> Person | None: ) ad_groups = await n2sn_service.get_groups_by_username(username) proposals = await get_proposals_by_person(bnl_person.EmployeeNumber) - except LookupError as error: + except (LookupError, AmbiguousPersonLookupError) as error: raise LookupError( f"Error obtaining diagnostic details for username of {username}" ) from error - print(bnl_person) - print("-------") - - print(ad_person) - print("-------") - - print(ad_groups) - print("-------") - - print(proposals) - print("-------") person = Person( firstname=bnl_person.FirstName, diff --git a/src/nsls2api/services/proposal_service.py b/src/nsls2api/services/proposal_service.py index 17fa544b..83ee428d 100644 --- a/src/nsls2api/services/proposal_service.py +++ b/src/nsls2api/services/proposal_service.py @@ -28,6 +28,7 @@ facility_service, pass_service, ) +from nsls2api.services.bnlpeople_service import AmbiguousPersonLookupError async def get_locked_proposals( @@ -810,8 +811,7 @@ async def generate_fake_test_proposal( is_pi=True, ) user_list.append(user) - except LookupError: - logger.error(f"Could not find user {add_specific_user} in BNLPeople.") + except (LookupError, AmbiguousPersonLookupError): return None fake_proposal_id = await generate_fake_proposal_id() diff --git a/src/nsls2api/tests/api/test_user_api.py b/src/nsls2api/tests/api/test_user_api.py new file mode 100644 index 00000000..02c0ab58 --- /dev/null +++ b/src/nsls2api/tests/api/test_user_api.py @@ -0,0 +1,223 @@ +import pytest +from httpx import ASGITransport, AsyncClient +from unittest.mock import AsyncMock, patch + +from nsls2api.main import app + + +@pytest.mark.anyio +async def test_get_person_by_username_not_found(): + """Test that requesting a non-existent username returns 404.""" + with patch( + "nsls2api.services.bnlpeople_service._call_bnlpeople_webservice", + new_callable=AsyncMock, + return_value=[], + ): + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://test" + ) as ac: + response = await ac.get("/v1/person/username/nonexistent_user_xyz123") + + assert response.status_code == 404 + response_json = response.json() + assert "detail" in response_json + assert "No person with username nonexistent_user_xyz123 was found." in response_json["detail"] + + +@pytest.mark.anyio +async def test_get_person_by_username_multiple_found(): + """Test that multiple people with same username returns 500 via exception handler.""" + # Mock API response with 2 people + mock_api_response = [ + { + "FirstName": "John", + "LastName": "Doe", + "BNLEmail": "john.doe@bnl.gov", + "EmployeeNumber": "123456", + "Institution": "Brookhaven National Laboratory", + "ActiveDirectoryName": "jdoe", + "CyberAgreementSigned": None, + "EmployeeStatus": "Active", + "EmployeeType": "Employee", + "FacilityCode": None, + "Facility": None, + }, + { + "FirstName": "John", + "LastName": "Doe", + "BNLEmail": "john.doe@bnl.gov", + "EmployeeNumber": "789012", + "Institution": "Brookhaven National Laboratory", + "ActiveDirectoryName": "jdoe", + "CyberAgreementSigned": None, + "EmployeeStatus": "Active", + "EmployeeType": "Employee", + "FacilityCode": None, + "Facility": None, + }, + ] + + with patch( + "nsls2api.services.bnlpeople_service._call_bnlpeople_webservice", + new_callable=AsyncMock, + return_value=mock_api_response, + ): + async with AsyncClient( + transport=ASGITransport(app=app, raise_app_exceptions=False), + base_url="http://test", + ) as ac: + response = await ac.get("/v1/person/username/jdoe") + + assert response.status_code == 500 + assert response.text == "Internal Server Error" + + +@pytest.mark.anyio +async def test_get_person_by_username_success(): + """Test that valid username returns 200 with person data.""" + mock_api_response = [ + { + "FirstName": "John", + "LastName": "Doe", + "BNLEmail": "john.doe@bnl.gov", + "EmployeeNumber": "123456", + "Institution": "Brookhaven National Laboratory", + "ActiveDirectoryName": "jdoe", + "CyberAgreementSigned": None, + "EmployeeStatus": "Active", + "EmployeeType": "Employee", + "FacilityCode": None, + "Facility": None, + } + ] + + with patch( + "nsls2api.services.bnlpeople_service._call_bnlpeople_webservice", + new_callable=AsyncMock, + return_value=mock_api_response, + ): + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://test" + ) as ac: + response = await ac.get("/v1/person/username/jdoe") + + assert response.status_code == 200 + response_json = response.json() + assert response_json["firstname"] == "John" + assert response_json["lastname"] == "Doe" + assert response_json["email"] == "john.doe@bnl.gov" + assert response_json["username"] == "jdoe" + assert response_json["bnl_employee"] is True + + +# ============================================================================ +# EMAIL TESTS +# ============================================================================ + + +@pytest.mark.anyio +async def test_get_person_by_email_not_found(): + """Test that requesting a non-existent email returns 404.""" + with patch( + "nsls2api.services.bnlpeople_service._call_bnlpeople_webservice", + new_callable=AsyncMock, + return_value=[], + ): + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://test" + ) as ac: + response = await ac.get("/v1/person/email/nonexistent@example.com") + + assert response.status_code == 404 + response_json = response.json() + assert "detail" in response_json + assert "No person with email nonexistent@example.com was found." in response_json["detail"] + + +@pytest.mark.anyio +async def test_get_person_by_email_multiple_found(): + """Test that multiple people with same email returns 500 via exception handler.""" + # Mock API response with 2 people + mock_api_response = [ + { + "FirstName": "Jane", + "LastName": "Smith", + "BNLEmail": "jane@example.com", + "EmployeeNumber": "123456", + "Institution": "Brookhaven National Laboratory", + "ActiveDirectoryName": "jsmith", + "CyberAgreementSigned": None, + "EmployeeStatus": "Active", + "EmployeeType": "Employee", + "FacilityCode": None, + "Facility": None, + }, + { + "FirstName": "Jane", + "LastName": "Smith", + "BNLEmail": "jane@example.com", + "EmployeeNumber": "654321", + "Institution": "Brookhaven National Laboratory", + "ActiveDirectoryName": "jsmith2", + "CyberAgreementSigned": None, + "EmployeeStatus": "Active", + "EmployeeType": "Employee", + "FacilityCode": None, + "Facility": None, + }, + ] + + with patch( + "nsls2api.services.bnlpeople_service._call_bnlpeople_webservice", + new_callable=AsyncMock, + return_value=mock_api_response, + ): + async with AsyncClient( + transport=ASGITransport(app=app, raise_app_exceptions=False), + base_url="http://test", + ) as ac: + response = await ac.get("/v1/person/email/jane@example.com") + + assert response.status_code == 500 + assert response.text == "Internal Server Error" + + +@pytest.mark.anyio +async def test_get_person_by_email_success(): + """Test that valid email returns 200 with person data.""" + mock_api_response = [ + { + "FirstName": "Jane", + "LastName": "Smith", + "BNLEmail": "jane.smith@bnl.gov", + "EmployeeNumber": "654321", + "Institution": "Brookhaven National Laboratory", + "ActiveDirectoryName": "jsmith", + "CyberAgreementSigned": None, + "EmployeeStatus": "Active", + "EmployeeType": "Employee", + "FacilityCode": None, + "Facility": None, + } + ] + + with patch( + "nsls2api.services.bnlpeople_service._call_bnlpeople_webservice", + new_callable=AsyncMock, + return_value=mock_api_response, + ): + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://test" + ) as ac: + response = await ac.get("/v1/person/email/jane.smith@bnl.gov") + + assert response.status_code == 200 + response_json = response.json() + assert response_json["firstname"] == "Jane" + assert response_json["lastname"] == "Smith" + assert response_json["email"] == "jane.smith@bnl.gov" + assert response_json["username"] == "jsmith" + assert response_json["bnl_employee"] is True + + +