From 1fe9c739806fd1e3b5749c416e5f412d4e4f2bdb Mon Sep 17 00:00:00 2001 From: James McKinney <26463+jpmckinney@users.noreply.github.com> Date: Thu, 22 Aug 2024 21:41:48 -0400 Subject: [PATCH 01/20] test: Tidy command tests --- tests/commands/test_commands.py | 68 +++++++++++++-------------------- 1 file changed, 26 insertions(+), 42 deletions(-) diff --git a/tests/commands/test_commands.py b/tests/commands/test_commands.py index 2c53b07c..41b88ad3 100644 --- a/tests/commands/test_commands.py +++ b/tests/commands/test_commands.py @@ -26,21 +26,20 @@ def test_send_reminders_intro(session, mock_send_templated_email, pending_applic with assert_change(mock_send_templated_email, "call_count", call_count): result = runner.invoke(__main__.app, ["send-reminders"]) - assert_success( - result, - ( + assert_success( + result, f"Sending {call_count} BORROWER_PENDING_APPLICATION_REMINDER...\n" - "Sending 0 BORROWER_PENDING_SUBMIT_REMINDER...\n" - ), - ) + "Sending 0 BORROWER_PENDING_SUBMIT_REMINDER...\n", + ) # If run a second time, reminder is not sent. with assert_change(mock_send_templated_email, "call_count", 0): result = runner.invoke(__main__.app, ["send-reminders"]) - assert_success( - result, "Sending 0 BORROWER_PENDING_APPLICATION_REMINDER...\nSending 0 BORROWER_PENDING_SUBMIT_REMINDER...\n" - ) + assert_success( + result, + "Sending 0 BORROWER_PENDING_APPLICATION_REMINDER...\nSending 0 BORROWER_PENDING_SUBMIT_REMINDER...\n", + ) @pytest.mark.parametrize( @@ -63,35 +62,28 @@ def test_send_reminders_submit(session, mock_send_templated_email, accepted_appl with assert_change(mock_send_templated_email, "call_count", call_count): result = runner.invoke(__main__.app, ["send-reminders"]) - assert_success( - result, - ( + assert_success( + result, "Sending 0 BORROWER_PENDING_APPLICATION_REMINDER...\n" - f"Sending {call_count} BORROWER_PENDING_SUBMIT_REMINDER...\n" - ), - ) + f"Sending {call_count} BORROWER_PENDING_SUBMIT_REMINDER...\n", + ) # If run a second time, reminder is not sent. with assert_change(mock_send_templated_email, "call_count", 0): result = runner.invoke(__main__.app, ["send-reminders"]) - assert_success( - result, "Sending 0 BORROWER_PENDING_APPLICATION_REMINDER...\nSending 0 BORROWER_PENDING_SUBMIT_REMINDER...\n" - ) - + assert_success( + result, + "Sending 0 BORROWER_PENDING_APPLICATION_REMINDER...\nSending 0 BORROWER_PENDING_SUBMIT_REMINDER...\n", + ) -def test_send_reminders_no_applications_to_remind(mock_send_templated_email, pending_application): - with assert_change(mock_send_templated_email, "call_count", 0): - result = runner.invoke(__main__.app, ["send-reminders"]) - assert_success( - result, "Sending 0 BORROWER_PENDING_APPLICATION_REMINDER...\nSending 0 BORROWER_PENDING_SUBMIT_REMINDER...\n" - ) - - -def test_set_lapsed_applications(session, pending_application): - pending_application.created_at = datetime.now(pending_application.tz) - timedelta( - days=app_settings.days_to_change_to_lapsed + 2 +@pytest.mark.parametrize(("seconds", "lapsed"), [(0, True), (1, False)]) +def test_set_lapsed_applications(session, pending_application, seconds, lapsed): + pending_application.created_at = ( + datetime.now(pending_application.tz) + - timedelta(days=app_settings.days_to_change_to_lapsed) + + timedelta(seconds=seconds) ) session.commit() @@ -99,8 +91,8 @@ def test_set_lapsed_applications(session, pending_application): session.expire_all() assert_success(result) - assert pending_application.status == models.ApplicationStatus.LAPSED - assert pending_application.application_lapsed_at is not None + assert (pending_application.status == models.ApplicationStatus.LAPSED) == lapsed + assert (pending_application.application_lapsed_at is not None) == lapsed def test_set_lapsed_applications_no_lapsed(session, pending_application): @@ -124,11 +116,6 @@ def test_set_lapsed_applications_no_lapsed(session, pending_application): def test_send_overdue_reminders( reset_database, session, mock_send_templated_email, started_application, seconds, call_count, overdue ): - # Lapse all applications already in the database. - session.query(models.Application).filter(models.Application.id != started_application.id).update( - {"status": models.ApplicationStatus.LAPSED} - ) - started_application.lender_started_at = datetime.now(started_application.tz) - timedelta(seconds=seconds) session.commit() @@ -136,11 +123,8 @@ def test_send_overdue_reminders( result = runner.invoke(__main__.app, ["sla-overdue-applications"]) session.expire_all() - assert_success(result) - if overdue: - assert started_application.overdued_at is not None - else: - assert started_application.overdued_at is None + assert_success(result) + assert (started_application.overdued_at is not None) == overdue def test_remove_data(session, declined_application): From 9b48b342ec422b5d4a7bc684b3400583213bf9dd Mon Sep 17 00:00:00 2001 From: James McKinney <26463+jpmckinney@users.noreply.github.com> Date: Thu, 22 Aug 2024 22:06:58 -0400 Subject: [PATCH 02/20] chore: Use "admin", not "ocp" --- app/dependencies.py | 4 ++-- app/models.py | 2 +- app/routers/downloads.py | 4 ++-- app/routers/statistics.py | 4 ++-- docs/contributing/index.rst | 1 + 5 files changed, 8 insertions(+), 7 deletions(-) diff --git a/app/dependencies.py b/app/dependencies.py index d8069639..04c86b10 100644 --- a/app/dependencies.py +++ b/app/dependencies.py @@ -64,7 +64,7 @@ async def get_user(username: str = Depends(get_current_user), session: Session = async def get_admin_user(user: models.User = Depends(get_user)) -> models.User: - if not user.is_ocp(): + if not user.is_admin(): raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail=_("Insufficient permissions"), @@ -85,7 +85,7 @@ def raise_if_unauthorized( for role in roles: match role: case models.UserType.OCP: - if user.is_ocp(): + if user.is_admin(): break case models.UserType.FI: if user.lender_id == application.lender_id: diff --git a/app/models.py b/app/models.py index cebcd8fb..70e9def9 100644 --- a/app/models.py +++ b/app/models.py @@ -1014,7 +1014,7 @@ class UserBase(SQLModel): default=datetime.utcnow(), sa_column=Column(DateTime(timezone=True), nullable=False, server_default=func.now()) ) - def is_ocp(self) -> bool: + def is_admin(self) -> bool: return self.type == UserType.OCP diff --git a/app/routers/downloads.py b/app/routers/downloads.py index 0c693393..96435090 100644 --- a/app/routers/downloads.py +++ b/app/routers/downloads.py @@ -41,7 +41,7 @@ async def get_borrower_document( session, type=( models.ApplicationActionType.OCP_DOWNLOAD_DOCUMENT - if user.is_ocp() + if user.is_admin() else models.ApplicationActionType.FI_DOWNLOAD_DOCUMENT ), data={"file_name": document.name}, @@ -116,7 +116,7 @@ async def download_application( session, type=( models.ApplicationActionType.OCP_DOWNLOAD_APPLICATION - if user.is_ocp() + if user.is_admin() else models.ApplicationActionType.FI_DOWNLOAD_APPLICATION ), application_id=application.id, diff --git a/app/routers/statistics.py b/app/routers/statistics.py index 6d3c1256..7c4238fc 100644 --- a/app/routers/statistics.py +++ b/app/routers/statistics.py @@ -16,7 +16,7 @@ "/statistics-ocp", tags=["statistics"], ) -async def get_ocp_statistics_by_lender( +async def get_admin_statistics_by_lender( initial_date: str | None = None, final_date: str | None = None, lender_id: int | None = None, @@ -75,7 +75,7 @@ async def get_ocp_statistics_by_lender( "/statistics-ocp/opt-in", tags=["statistics"], ) -async def get_ocp_statistics_opt_in( +async def get_admin_statistics_opt_in( admin: User = Depends(dependencies.get_admin_user), session: Session = Depends(get_db), ) -> serializers.StatisticOptInResponse: diff --git a/docs/contributing/index.rst b/docs/contributing/index.rst index 0cfecf43..84b9b440 100644 --- a/docs/contributing/index.rst +++ b/docs/contributing/index.rst @@ -181,6 +181,7 @@ In Python code and documentation: - Use "lender", not "FI" or "financial institution". - Use "borrower", not "MSME", "SME" or "small and medium-sized enterprises". +- Use "administrator" or "admin", not "OCP". .. note:: From 8eeb6d6e4398835f0a20f3961ad5a96f015da247 Mon Sep 17 00:00:00 2001 From: James McKinney <26463+jpmckinney@users.noreply.github.com> Date: Thu, 22 Aug 2024 22:21:08 -0400 Subject: [PATCH 03/20] chore: There is no need to raise a 500 error, because FastAPI will do that on its own --- app/routers/applications.py | 10 +--------- app/routers/guest/applications.py | 14 +++----------- app/routers/users.py | 7 ++----- 3 files changed, 6 insertions(+), 25 deletions(-) diff --git a/app/routers/applications.py b/app/routers/applications.py index 10418bb7..38941653 100644 --- a/app/routers/applications.py +++ b/app/routers/applications.py @@ -2,7 +2,6 @@ from datetime import datetime from typing import Any, cast -from botocore.exceptions import ClientError from fastapi import APIRouter, Depends, HTTPException, Query, status from fastapi.encoders import jsonable_encoder from sqlalchemy.orm import Session, joinedload @@ -576,14 +575,7 @@ async def email_borrower( user_id=user.id, ) - try: - message_id = mail.send_mail_request_to_borrower(client.ses, application, payload.message) - except ClientError as e: - logger.exception(e) - return HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail=_("There was an error"), - ) + message_id = mail.send_mail_request_to_borrower(client.ses, application, payload.message) models.Message.create( session, application_id=application.id, diff --git a/app/routers/guest/applications.py b/app/routers/guest/applications.py index 48aec64c..499efe71 100644 --- a/app/routers/guest/applications.py +++ b/app/routers/guest/applications.py @@ -2,7 +2,6 @@ from datetime import datetime from typing import Any, cast -from botocore.exceptions import ClientError from fastapi import APIRouter, BackgroundTasks, Depends, Form, HTTPException, UploadFile, status from fastapi.encoders import jsonable_encoder from sqlalchemy.orm import Session, joinedload @@ -536,17 +535,10 @@ async def update_apps_send_notifications( application.borrower_submitted_at = datetime.now(application.created_at.tzinfo) application.pending_documents = False - try: - mail.send_notification_new_app_to_lender(client.ses, application) - mail.send_notification_new_app_to_ocp(client.ses, application) + mail.send_notification_new_app_to_lender(client.ses, application) + mail.send_notification_new_app_to_ocp(client.ses, application) - message_id = mail.send_application_submission_completed(client.ses, application) - except ClientError as e: - logger.exception(e) - raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail=_("There was an error submitting the application"), - ) + message_id = mail.send_application_submission_completed(client.ses, application) models.Message.create( session, application=application, diff --git a/app/routers/users.py b/app/routers/users.py index 18a12362..2f0b3663 100644 --- a/app/routers/users.py +++ b/app/routers/users.py @@ -144,10 +144,7 @@ def setup_mfa( status_code=status.HTTP_408_REQUEST_TIMEOUT, detail=_("Invalid session for the user, session is expired"), ) - raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail=_("There was an error trying to setup mfa"), - ) + raise return serializers.ResponseBase(detail=_("MFA configured successfully")) @@ -184,7 +181,7 @@ def login( mfa_code=payload.temp_password, ) else: - raise NotImplementedError + raise NotImplementedError # Missing MFA challenge except ClientError as e: # https://boto3.amazonaws.com/v1/documentation/api/latest/guide/error-handling.html#parsing-error-responses-and-catching-exceptions-from-aws-services if e.response["Error"]["Code"] == "ExpiredTemporaryPasswordException": From ed16bbfc629997ff30d17637fc6334c7883bb67b Mon Sep 17 00:00:00 2001 From: James McKinney <26463+jpmckinney@users.noreply.github.com> Date: Thu, 22 Aug 2024 22:54:33 -0400 Subject: [PATCH 04/20] fix: Report 400 (request validation errors) to Sentry. Use 422 instead for expected errors. --- app/routers/guest/applications.py | 34 +++++++++++++++---------------- app/settings.py | 12 +++++++++++ docs/contributing/index.rst | 8 +++++++- 3 files changed, 36 insertions(+), 18 deletions(-) diff --git a/app/routers/guest/applications.py b/app/routers/guest/applications.py index 499efe71..0f2446bb 100644 --- a/app/routers/guest/applications.py +++ b/app/routers/guest/applications.py @@ -61,7 +61,7 @@ async def decline( :param payload: The application decline payload. :return: The application response containing the updated application, borrower, and award. - :raise: HTTPException with status code 400 if the application is not in the PENDING status. + :raise: HTTPException if the application is not in the PENDING status. """ with rollback_on_error(session): borrower_declined_data = vars(payload) @@ -104,7 +104,7 @@ async def rollback_decline( :param payload: The application base payload. :return: The application response containing the updated application, borrower, and award. - :raise: HTTPException with status code 400 if the application is not in the DECLINED status. + :raise: HTTPException if the application is not in the DECLINED status. """ with rollback_on_error(session): # Update application. @@ -143,7 +143,7 @@ async def decline_feedback( :param payload: The application decline feedback payload. :return: The application response containing the updated application, borrower, and award. - :raise: HTTPException with status code 400 if the application is not in the DECLINED status. + :raise: HTTPException if the application is not in the DECLINED status. """ with rollback_on_error(session): borrower_declined_preferences_data = vars(payload) @@ -310,19 +310,19 @@ async def rollback_select_credit_product( :param payload: The application data. :return: The application response containing the updated application, borrower, and award. - :raise: HTTPException with status code 400 if the credit product is not selected or if the lender is already + :raise: HTTPException if the credit product is not selected or if the lender is already assigned. """ with rollback_on_error(session): if not application.credit_product_id: raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=_("Credit product not selected"), ) if application.lender_id: raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=_("Cannot rollback at this stage"), ) @@ -361,19 +361,19 @@ async def confirm_credit_product( :param payload: The application data. :return: The application response containing the updated application, borrower, award, lender, documents, and credit product. - :raise: HTTPException with status code 400 if the credit product is not selected or not found. + :raise: HTTPException if the credit product is not selected or not found. """ with rollback_on_error(session): if not application.credit_product_id: raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=_("Credit product not selected"), ) credit_product = models.CreditProduct.first_by(session, "id", application.credit_product_id) if not credit_product: raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=_("Credit product not found"), ) @@ -450,19 +450,19 @@ async def rollback_confirm_credit_product( :param payload: The application data. :return: The application response containing the updated application, borrower, award, lender, documents, and credit product. - :raise: HTTPException with status code 400 if the credit product is not selected or not found. + :raise: HTTPException if the credit product is not selected or not found. """ with rollback_on_error(session): if not application.credit_product_id: raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=_("Credit product not selected"), ) credit_product = models.CreditProduct.first_by(session, "id", application.credit_product_id) if not credit_product: raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=_("Credit product not found"), ) @@ -521,13 +521,13 @@ async def update_apps_send_notifications( with rollback_on_error(session): if not application.credit_product_id: raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=_("Credit product not selected"), ) if not application.lender: raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=_("Lender not selected"), ) @@ -585,7 +585,7 @@ async def upload_document( new_file, filename = util.validate_file(file) if not application.pending_documents: raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=_("Cannot upload document at this stage"), ) @@ -772,8 +772,8 @@ async def find_alternative_credit_option( :param payload: The payload containing the UUID of the rejected application. :param client: The Cognito client for sending notifications. :return: The newly created application as an alternative credit option. - :raise: HTTPException with status code 400 if the application has already been copied. - :raise: HTTPException with status code 400 if the application is not in the rejected status. + :raise: HTTPException if the application has already been copied. + :raise: HTTPException if the application is not in the rejected status. """ with rollback_on_error(session): # Check if the application has already been copied. diff --git a/app/settings.py b/app/settings.py index b65d144c..6b89d068 100644 --- a/app/settings.py +++ b/app/settings.py @@ -6,6 +6,8 @@ import sentry_sdk from pydantic_settings import BaseSettings, SettingsConfigDict +from sentry_sdk.integrations.fastapi import FastApiIntegration +from sentry_sdk.integrations.starlette import StarletteIntegration def sentry_filter_transactions(event: dict[str, Any], hint: dict[str, Any]) -> dict[str, Any] | None: @@ -184,9 +186,19 @@ class Settings(BaseSettings): ) if app_settings.sentry_dsn: + # https://docs.sentry.io/platforms/python/integrations/fastapi/ sentry_sdk.init( dsn=app_settings.sentry_dsn, before_send=sentry_filter_transactions, # Set traces_sample_rate to 1.0 to capture 100% of transactions for performance monitoring. traces_sample_rate=1.0, + # FastAPI uses 400 for request validation errors, which shouldn't occur unless the frontend is misimplemented. + integrations=[ + StarletteIntegration( + failed_request_status_codes=[400, range(500, 599)], + ), + FastApiIntegration( + failed_request_status_codes=[400, range(500, 599)], + ), + ], ) diff --git a/docs/contributing/index.rst b/docs/contributing/index.rst index 84b9b440..7e219ff2 100644 --- a/docs/contributing/index.rst +++ b/docs/contributing/index.rst @@ -221,7 +221,13 @@ Update API .. seealso:: :doc:`../api/index` -Use the parameter ``id`` only for application IDs, to avoid accidental errors. +- Use the parameter ``id`` only for application IDs, to avoid accidental errors. +- Don't use ``status.HTTP_400_BAD_REQUEST``. FastAPI uses it for `request validation errors `__, which are reported to Sentry. In general, use ``status.HTTP_422_UNPROCESSABLE_ENTITY``, instead. + + .. seealso:: + + - `422 Unprocessable Content `__ (MDN) + - Choosing an HTTP Status Code `__ After making changes, regenerate the OpenAPI document by :ref:`running the server` and: From 984ab042c66bb40d6cb8edc594669426c0d50ba5 Mon Sep 17 00:00:00 2001 From: James McKinney <26463+jpmckinney@users.noreply.github.com> Date: Thu, 22 Aug 2024 23:53:40 -0400 Subject: [PATCH 05/20] chore: Use (and document) appropriate HTTP status codes --- app/dependencies.py | 10 ++++---- app/routers/applications.py | 10 ++++---- app/routers/guest/applications.py | 8 +++--- app/routers/guest/emails.py | 4 +-- app/routers/lenders.py | 8 +++--- app/routers/users.py | 12 ++++----- app/util.py | 3 ++- docs/contributing/api.rst | 20 +++++++++++++++ docs/contributing/index.rst | 31 ++---------------------- docs/contributing/style.rst | 14 +++++++++++ docs/requirements.txt | 2 +- tests/routers/guest/test_applications.py | 6 ++--- tests/routers/test_applications.py | 14 +++++------ tests/routers/test_lenders.py | 10 ++++---- tests/routers/test_users.py | 4 +-- 15 files changed, 82 insertions(+), 74 deletions(-) create mode 100644 docs/contributing/api.rst create mode 100644 docs/contributing/style.rst diff --git a/app/dependencies.py b/app/dependencies.py index 04c86b10..d4b629f5 100644 --- a/app/dependencies.py +++ b/app/dependencies.py @@ -57,7 +57,7 @@ async def get_user(username: str = Depends(get_current_user), session: Session = user = models.User.first_by(session, "external_id", username) if not user: raise HTTPException( - status_code=status.HTTP_403_FORBIDDEN, + status_code=status.HTTP_404_NOT_FOUND, detail=_("User not found"), ) return user @@ -66,7 +66,7 @@ async def get_user(username: str = Depends(get_current_user), session: Session = async def get_admin_user(user: models.User = Depends(get_user)) -> models.User: if not user.is_admin(): raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, + status_code=status.HTTP_403_FORBIDDEN, detail=_("Insufficient permissions"), ) return user @@ -102,14 +102,14 @@ def raise_if_unauthorized( expired_at = application.expired_at if expired_at and expired_at < datetime.now(expired_at.tzinfo): raise HTTPException( - status_code=status.HTTP_409_CONFLICT, + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=_("Application expired"), ) if statuses: if application.status not in statuses: raise HTTPException( - status_code=status.HTTP_409_CONFLICT, + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=_("Application status should not be %(status)s", status=_(application.status)), ) @@ -168,7 +168,7 @@ def _get_application_as_guest_via_uuid(session: Session, uuid: str) -> models.Ap if application.status == models.ApplicationStatus.LAPSED: raise HTTPException( - status_code=status.HTTP_409_CONFLICT, + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=_("Application lapsed"), ) diff --git a/app/routers/applications.py b/app/routers/applications.py index 38941653..ad0dd555 100644 --- a/app/routers/applications.py +++ b/app/routers/applications.py @@ -170,7 +170,7 @@ async def approve_application( not_validated_fields.append(key) if not_validated_fields: raise HTTPException( - status_code=status.HTTP_409_CONFLICT, + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=_("Some borrower data field are not verified"), ) @@ -181,7 +181,7 @@ async def approve_application( not_validated_documents.append(document.type) if not_validated_documents: raise HTTPException( - status_code=status.HTTP_409_CONFLICT, + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=_("Some documents are not verified"), ) @@ -457,7 +457,7 @@ async def get_application( Retrieve an application by its ID. :return: The application with the specified ID and its associated relations. - :raise: HTTPException with status code 401 if the user is not authorized to view the application. + :raise: HTTPException if the user is not authorized to view the application. """ return util.get_modified_data_fields(session, application) @@ -484,7 +484,7 @@ async def start_application( :param id: The ID of the application to start. :return: The started application with its associated relations. - :raise: HTTPException with status code 401 if the user is not authorized to start the application. + :raise: HTTPException if the user is not authorized to start the application. """ with rollback_on_error(session): application.status = models.ApplicationStatus.STARTED @@ -604,6 +604,6 @@ async def previous_contracts( Get the previous awards associated with an application. :return: A list of previous awards associated with the application. - :raise: HTTPException with status code 401 if the user is not authorized to access the application. + :raise: HTTPException if the user is not authorized to access the application. """ return application.previous_awards(session) diff --git a/app/routers/guest/applications.py b/app/routers/guest/applications.py index 0f2446bb..626ef118 100644 --- a/app/routers/guest/applications.py +++ b/app/routers/guest/applications.py @@ -30,7 +30,7 @@ async def application_by_uuid( Retrieve an application by its UUID. :return: The application with the specified UUID and its associated entities. - :raise: HTTPException with status code 404 if the application is expired. + :raise: HTTPException if the application is expired. """ return serializers.ApplicationResponse( application=cast(models.ApplicationRead, application), @@ -182,7 +182,7 @@ async def access_scheme( :param payload: The application data. :return: The application response containing the updated application, borrower, and award. - :raise: HTTPException with status code 404 if the application is expired or not in the PENDING status. + :raise: HTTPException if the application is expired or not in the PENDING status. """ with rollback_on_error(session): application.borrower_accepted_at = datetime.now(application.created_at.tzinfo) @@ -216,7 +216,7 @@ async def credit_product_options( :param payload: The application credit options. :return: The credit product list response containing the available loans and credit lines. - :raise: HTTPException with status code 404 if the application is expired, not in the ACCEPTED status, or if the + :raise: HTTPException if the application is expired, not in the ACCEPTED status, or if the previous lenders are not found. """ base_query = ( @@ -255,7 +255,7 @@ async def select_credit_product( :param payload: The application credit product selection payload. :return: The application response containing the updated application, borrower, award, lender, documents, and credit product. - :raise: HTTPException with status code 404 if the application is expired, not in the ACCEPTED status, or if the + :raise: HTTPException if the application is expired, not in the ACCEPTED status, or if the calculator data is invalid. """ with rollback_on_error(session): diff --git a/app/routers/guest/emails.py b/app/routers/guest/emails.py index 11114f33..aba55140 100644 --- a/app/routers/guest/emails.py +++ b/app/routers/guest/emails.py @@ -77,14 +77,14 @@ async def confirm_email( with rollback_on_error(session): if not application.pending_email_confirmation: raise HTTPException( - status_code=status.HTTP_409_CONFLICT, + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=_("Application is not pending an email confirmation"), ) new_email, token = application.confirmation_email_token.split("---")[:2] if token != payload.confirmation_email_token: raise HTTPException( - status_code=status.HTTP_409_CONFLICT, + status_code=status.HTTP_403_FORBIDDEN, detail=_("Not authorized to modify this application"), ) diff --git a/app/routers/lenders.py b/app/routers/lenders.py index c5292d8c..1c7273f7 100644 --- a/app/routers/lenders.py +++ b/app/routers/lenders.py @@ -45,7 +45,7 @@ async def create_lender( return lender except IntegrityError: raise HTTPException( - status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + status_code=status.HTTP_409_CONFLICT, detail=_("Lender already exists"), ) @@ -90,7 +90,7 @@ async def get_lender( :param lender_id: The ID of the lender to retrieve. :return: The lender with the specified ID. - :raise: HTTPException with status code 404 if the lender is not found. + :raise: HTTPException if the lender is not found. """ return get_object_or_404(session, models.Lender, "id", lender_id) @@ -122,7 +122,7 @@ async def update_lender( return lender except IntegrityError: raise HTTPException( - status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + status_code=status.HTTP_409_CONFLICT, detail=_("Lender already exists"), ) @@ -178,7 +178,7 @@ async def get_credit_product( :param credit_product_id: The ID of the credit product to retrieve. :return: The credit product with the specified ID and its associated lender information. - :raise: HTTPException with status code 404 if the credit product is not found. + :raise: HTTPException if the credit product is not found. """ credit_product = ( models.CreditProduct.filter_by(session, "id", credit_product_id) diff --git a/app/routers/users.py b/app/routers/users.py index 2f0b3663..6f546837 100644 --- a/app/routers/users.py +++ b/app/routers/users.py @@ -57,7 +57,7 @@ async def create_user( return user except (client.cognito.exceptions.UsernameExistsException, IntegrityError): raise HTTPException( - status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + status_code=status.HTTP_409_CONFLICT, detail=_("Username already exists"), ) @@ -109,7 +109,7 @@ def change_password( except ClientError as e: if e.response["Error"]["Code"] == "ExpiredTemporaryPasswordException": raise HTTPException( - status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + status_code=status.HTTP_403_FORBIDDEN, detail=_("Temporal password is expired, please request a new one"), ) raise HTTPException( @@ -141,7 +141,7 @@ def setup_mfa( except ClientError as e: if e.response["Error"]["Code"] == "NotAuthorizedException": raise HTTPException( - status_code=status.HTTP_408_REQUEST_TIMEOUT, + status_code=status.HTTP_403_FORBIDDEN, detail=_("Invalid session for the user, session is expired"), ) raise @@ -186,11 +186,11 @@ def login( # https://boto3.amazonaws.com/v1/documentation/api/latest/guide/error-handling.html#parsing-error-responses-and-catching-exceptions-from-aws-services if e.response["Error"]["Code"] == "ExpiredTemporaryPasswordException": raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, + status_code=status.HTTP_403_FORBIDDEN, detail=_("Temporal password is expired, please request a new one"), ) raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, + status_code=status.HTTP_403_FORBIDDEN, detail=e.response["Error"]["Message"], ) @@ -360,6 +360,6 @@ async def update_user( return user except IntegrityError: raise HTTPException( - status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + status_code=status.HTTP_409_CONFLICT, detail=_("User already exists"), ) diff --git a/app/util.py b/app/util.py index 14787cfa..3c9ccffd 100644 --- a/app/util.py +++ b/app/util.py @@ -51,7 +51,8 @@ def get_object_or_404(session: Session, model: type[T], field: str, value: Any) obj = model.first_by(session, field, value) if not obj: raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, detail=_("%(model_name)s not found", model_name=model.__name__) + status_code=status.HTTP_404_NOT_FOUND, + detail=_("%(model_name)s not found", model_name=model.__name__), ) return obj diff --git a/docs/contributing/api.rst b/docs/contributing/api.rst new file mode 100644 index 00000000..c80f69a9 --- /dev/null +++ b/docs/contributing/api.rst @@ -0,0 +1,20 @@ +API design +========== + +.. seealso:: `Library and Web API `__ + +- Use the parameter ``id`` only for application IDs, to avoid accidental errors. +- Don't use ``status.HTTP_400_BAD_REQUEST``. FastAPI uses it for `request validation errors `__, which are reported to Sentry. Instead, use: + + - ``status.HTTP_403_FORBIDDEN``, if not authenticated or authorized + + .. note:: 401 is only relevant to `HTTP authentication `__. `RFC 7325 `__ states: "The server generating a 401 response MUST send a WWW-Authenticate header field" + + - ``status.HTTP_404_NOT_FOUND``, if the resource is not found + - ``status.HTTP_409_CONFLICT``, if the resource already exists + - ``status.HTTP_422_UNPROCESSABLE_ENTITY``, otherwise + + .. seealso:: + + - `422 Unprocessable Content `__ (MDN) + - Choosing an HTTP Status Code `__ diff --git a/docs/contributing/index.rst b/docs/contributing/index.rst index 7e219ff2..0fd2b076 100644 --- a/docs/contributing/index.rst +++ b/docs/contributing/index.rst @@ -4,6 +4,7 @@ Contributing .. toctree:: :caption: contents + api sqlalchemy .. seealso:: @@ -167,25 +168,7 @@ To delete the image (e.g. when recreating it), run: Make changes ------------ -Read the next pages in this section to learn about style guides, and the :doc:`../api/index` about helper methods and application logic. See also the `OCP Software Development Handbook `__, in particular: - -- `Library and Web API `__ -- `Python `__ - -Style guide -~~~~~~~~~~~ - -- Use lowercase filenames, including ``email_templates/`` files. - -In Python code and documentation: - -- Use "lender", not "FI" or "financial institution". -- Use "borrower", not "MSME", "SME" or "small and medium-sized enterprises". -- Use "administrator" or "admin", not "OCP". - -.. note:: - - Some endpoints and enums cannot be made to conform to this style guide, without migrating the database or updating the frontend. +Read the next pages in this section to learn about style guides, and the :doc:`../api/index` about helper methods and application logic. Read the `OCP Software Development Handbook `__: in particular, `Python `__. Update requirements ~~~~~~~~~~~~~~~~~~~ @@ -219,16 +202,6 @@ Update translations Update API ~~~~~~~~~~ -.. seealso:: :doc:`../api/index` - -- Use the parameter ``id`` only for application IDs, to avoid accidental errors. -- Don't use ``status.HTTP_400_BAD_REQUEST``. FastAPI uses it for `request validation errors `__, which are reported to Sentry. In general, use ``status.HTTP_422_UNPROCESSABLE_ENTITY``, instead. - - .. seealso:: - - - `422 Unprocessable Content `__ (MDN) - - Choosing an HTTP Status Code `__ - After making changes, regenerate the OpenAPI document by :ref:`running the server` and: .. code-block:: bash diff --git a/docs/contributing/style.rst b/docs/contributing/style.rst new file mode 100644 index 00000000..b73816d3 --- /dev/null +++ b/docs/contributing/style.rst @@ -0,0 +1,14 @@ +Style guide +=========== + +- Use lowercase filenames, including ``email_templates/`` files. + +In Python code and documentation: + +- Use "lender", not "FI" or "financial institution". +- Use "borrower", not "MSME", "SME" or "small and medium-sized enterprises". +- Use "administrator" or "admin", not "OCP". + +.. note:: + + Some endpoints and enums cannot be made to conform to this style guide, without migrating the database or updating the frontend. diff --git a/docs/requirements.txt b/docs/requirements.txt index 599b55f4..1beca3d0 100644 --- a/docs/requirements.txt +++ b/docs/requirements.txt @@ -1,4 +1,4 @@ -r ../requirements.txt furo sphinx-design -sphinxcontrib-typer>=0.4.1 +sphinxcontrib-typer>=0.4.2 diff --git a/tests/routers/guest/test_applications.py b/tests/routers/guest/test_applications.py index 68a18b61..8df9ebc1 100644 --- a/tests/routers/guest/test_applications.py +++ b/tests/routers/guest/test_applications.py @@ -18,7 +18,7 @@ def test_application_declined(client, pending_application): response = client.post("/applications/access-scheme", json={"uuid": pending_application.uuid}) - assert response.status_code == status.HTTP_409_CONFLICT + assert response.status_code == status.HTTP_422_UNPROCESSABLE_ENTITY assert response.json() == {"detail": _("Application status should not be %(status)s", status=_("DECLINED"))} @@ -34,7 +34,7 @@ def test_application_rollback_declined(client, session, declined_application): response = client.post("/applications/rollback-decline", json={"uuid": declined_application.uuid}) - assert response.status_code == status.HTTP_409_CONFLICT + assert response.status_code == status.HTTP_422_UNPROCESSABLE_ENTITY assert response.json() == {"detail": _("Application status should not be %(status)s", status=_("PENDING"))} @@ -65,7 +65,7 @@ def test_access_expired_application(client, session, pending_application): # borrower tries to access expired application response = client.get(f"/applications/uuid/{pending_application.uuid}") - assert response.status_code == status.HTTP_409_CONFLICT + assert response.status_code == status.HTTP_422_UNPROCESSABLE_ENTITY assert response.json() == {"detail": _("Application expired")} diff --git a/tests/routers/test_applications.py b/tests/routers/test_applications.py index bd04340b..87c43f29 100644 --- a/tests/routers/test_applications.py +++ b/tests/routers/test_applications.py @@ -20,7 +20,7 @@ def test_reject_application(client, session, lender_header, pending_application) # tries to reject the application before its started response = client.post(f"/applications/{appid}/reject-application", json=payload, headers=lender_header) - assert response.status_code == status.HTTP_409_CONFLICT + assert response.status_code == status.HTTP_422_UNPROCESSABLE_ENTITY assert response.json() == {"detail": _("Application status should not be %(status)s", status=_("PENDING"))} pending_application.status = models.ApplicationStatus.STARTED @@ -63,7 +63,7 @@ def test_approve_application_cycle( # Application should be accepted now so it cannot be accepted again response = client.post("/applications/access-scheme", json={"uuid": pending_application.uuid}) - assert response.status_code == status.HTTP_409_CONFLICT + assert response.status_code == status.HTTP_422_UNPROCESSABLE_ENTITY assert response.json() == {"detail": _("Application status should not be %(status)s", status=_("ACCEPTED"))} response = client.post( @@ -139,7 +139,7 @@ def test_approve_application_cycle( "/applications/confirm-change-email", json={"uuid": pending_application.uuid, "confirmation_email_token": "confirmation_email_token"}, ) - assert response.status_code == status.HTTP_409_CONFLICT + assert response.status_code == status.HTTP_422_UNPROCESSABLE_ENTITY assert response.json() == {"detail": _("Application is not pending an email confirmation")} response = client.post("/applications/submit", json={"uuid": pending_application.uuid}) @@ -153,7 +153,7 @@ def test_approve_application_cycle( data={"uuid": pending_application.uuid, "type": models.BorrowerDocumentType.INCORPORATION_DOCUMENT}, files={"file": (file_to_upload.name, file_to_upload, "image/jpeg")}, ) - assert response.status_code == status.HTTP_409_CONFLICT + assert response.status_code == status.HTTP_422_UNPROCESSABLE_ENTITY assert response.json() == {"detail": _("Application status should not be %(status)s", status=_("SUBMITTED"))} # different lender user tries to start the application @@ -246,7 +246,7 @@ def test_approve_application_cycle( # lender tries to approve the application without verifying legal_name response = client.post(f"/applications/{appid}/approve-application", json=approve_payload, headers=lender_header) - assert response.status_code == status.HTTP_409_CONFLICT + assert response.status_code == status.HTTP_422_UNPROCESSABLE_ENTITY assert response.json() == {"detail": _("Some borrower data field are not verified")} # verify legal_name @@ -264,7 +264,7 @@ def test_approve_application_cycle( # lender tries to approve the application without verifying INCORPORATION_DOCUMENT response = client.post(f"/applications/{appid}/approve-application", json=approve_payload, headers=lender_header) - assert response.status_code == status.HTTP_409_CONFLICT + assert response.status_code == status.HTTP_422_UNPROCESSABLE_ENTITY assert response.json() == {"detail": _("Some documents are not verified")} # verify borrower document @@ -346,7 +346,7 @@ def test_get_applications(client, session, admin_header, lender_header, pending_ session.commit() response = client.get(f"/applications/uuid/{pending_application.uuid}") - assert response.status_code == status.HTTP_409_CONFLICT + assert response.status_code == status.HTTP_422_UNPROCESSABLE_ENTITY assert response.json() == {"detail": _("Application lapsed")} response = client.get("/applications/uuid/123-456") diff --git a/tests/routers/test_lenders.py b/tests/routers/test_lenders.py index 4d483744..47c4369a 100644 --- a/tests/routers/test_lenders.py +++ b/tests/routers/test_lenders.py @@ -44,7 +44,7 @@ def test_create_credit_product(client, admin_header, lender_header, lender): # Lender tries to create credit product response = client.post(f"/lenders/{lender.id}/credit-products", json=create_payload, headers=lender_header) - assert response.status_code == status.HTTP_401_UNAUTHORIZED + assert response.status_code == status.HTTP_403_FORBIDDEN assert response.json() == {"detail": _("Insufficient permissions")} with warnings.catch_warnings(): @@ -94,11 +94,11 @@ def test_create_lender(client, admin_header, lender_header, lender): # tries to create same lender twice response = client.post("/lenders", json=payload, headers=admin_header) - assert response.status_code == status.HTTP_422_UNPROCESSABLE_ENTITY + assert response.status_code == status.HTTP_409_CONFLICT assert response.json() == {"detail": _("Lender already exists")} response = client.post("/lenders", json=payload, headers=lender_header) - assert response.status_code == status.HTTP_401_UNAUTHORIZED + assert response.status_code == status.HTTP_403_FORBIDDEN assert response.json() == {"detail": _("Insufficient permissions")} @@ -137,7 +137,7 @@ def test_create_lender_with_credit_product(client, admin_header, lender_header, # lender user tries to create lender response = client.post("/lenders", json=payload, headers=lender_header) - assert response.status_code == status.HTTP_401_UNAUTHORIZED + assert response.status_code == status.HTTP_403_FORBIDDEN assert response.json() == {"detail": _("Insufficient permissions")} @@ -170,7 +170,7 @@ def test_update_lender(client, admin_header, lender_header, lender): # Lender user tries to update lender response = client.put(f"/lenders/{lender.id}", json=payload, headers=lender_header) - assert response.status_code == status.HTTP_401_UNAUTHORIZED + assert response.status_code == status.HTTP_403_FORBIDDEN assert response.json() == {"detail": _("Insufficient permissions")} response = client.put(f"/lenders/{lender.id}", json={"sla_days": "not_valid_value"}, headers=admin_header) diff --git a/tests/routers/test_users.py b/tests/routers/test_users.py index 16c155ba..ed77b82c 100644 --- a/tests/routers/test_users.py +++ b/tests/routers/test_users.py @@ -31,7 +31,7 @@ def test_create_and_get_user(client, admin_header, lender_header, user_payload): assert_ok(response) response = client.get("/users?page=0&page_size=5&sort_field=created_at&sort_order=desc", headers=lender_header) - assert response.status_code == status.HTTP_401_UNAUTHORIZED + assert response.status_code == status.HTTP_403_FORBIDDEN assert response.json() == {"detail": _("Insufficient permissions")} @@ -64,7 +64,7 @@ def test_duplicate_user(client, admin_header, user_payload): # duplicate user response = client.post("/users", json=user_payload, headers=admin_header) - assert response.status_code == status.HTTP_422_UNPROCESSABLE_ENTITY + assert response.status_code == status.HTTP_409_CONFLICT assert response.json() == {"detail": _("Username already exists")} From a80caaa3b6493ae6a836fd8626da052856af047b Mon Sep 17 00:00:00 2001 From: James McKinney <26463+jpmckinney@users.noreply.github.com> Date: Thu, 22 Aug 2024 23:59:12 -0400 Subject: [PATCH 06/20] feat: Don't do bare "except Exception" and don't report unexpected and unknown errors to the user --- app/routers/guest/applications.py | 28 +++++++++++--------------- app/routers/users.py | 33 +++++++++++-------------------- 2 files changed, 23 insertions(+), 38 deletions(-) diff --git a/app/routers/guest/applications.py b/app/routers/guest/applications.py index 626ef118..0ee3055a 100644 --- a/app/routers/guest/applications.py +++ b/app/routers/guest/applications.py @@ -797,23 +797,17 @@ async def find_alternative_credit_option( ) # Copy the application, changing the uuid, status, and borrower_accepted_at. - try: - new_application = models.Application.create( - session, - award_id=application.award_id, - uuid=util.generate_uuid(application.uuid), - primary_email=application.primary_email, - status=models.ApplicationStatus.ACCEPTED, - award_borrower_identifier=application.award_borrower_identifier, - borrower_id=application.borrower.id, - calculator_data=application.calculator_data, - borrower_accepted_at=datetime.now(application.created_at.tzinfo), - ) - except Exception as e: - raise HTTPException( - status_code=status.HTTP_409_CONFLICT, - detail=_("There was a problem copying the application. %(exception)s", exception=e), - ) + new_application = models.Application.create( + session, + award_id=application.award_id, + uuid=util.generate_uuid(application.uuid), + primary_email=application.primary_email, + status=models.ApplicationStatus.ACCEPTED, + award_borrower_identifier=application.award_borrower_identifier, + borrower_id=application.borrower.id, + calculator_data=application.calculator_data, + borrower_accepted_at=datetime.now(application.created_at.tzinfo), + ) models.ApplicationAction.create( session, diff --git a/app/routers/users.py b/app/routers/users.py index 6f546837..1214dd12 100644 --- a/app/routers/users.py +++ b/app/routers/users.py @@ -112,10 +112,7 @@ def change_password( status_code=status.HTTP_403_FORBIDDEN, detail=_("Temporal password is expired, please request a new one"), ) - raise HTTPException( - status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, - detail=_("There was an error trying to update the password"), - ) + raise return serializers.ResponseBase(detail=_("Password changed")) @@ -189,10 +186,7 @@ def login( status_code=status.HTTP_403_FORBIDDEN, detail=_("Temporal password is expired, please request a new one"), ) - raise HTTPException( - status_code=status.HTTP_403_FORBIDDEN, - detail=e.response["Error"]["Message"], - ) + raise return serializers.LoginResponse( user=user, @@ -264,20 +258,17 @@ def forgot_password( Email the user a temporary password and a reset link. """ - try: - temporary_password = client.generate_password() - - # https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/cognito-idp/client/admin_set_user_password.html - client.cognito.admin_set_user_password( - UserPoolId=app_settings.cognito_pool_id, - Username=payload.username, - Password=temporary_password, - Permanent=False, - ) + temporary_password = client.generate_password() + + # https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/cognito-idp/client/admin_set_user_password.html + client.cognito.admin_set_user_password( + UserPoolId=app_settings.cognito_pool_id, + Username=payload.username, + Password=temporary_password, + Permanent=False, + ) - mail.send_mail_to_reset_password(client.ses, payload.username, temporary_password) - except Exception: - logger.exception("Error resetting password") + mail.send_mail_to_reset_password(client.ses, payload.username, temporary_password) # always return 200 to avoid user enumeration return serializers.ResponseBase(detail=_("An email with a reset link was sent to end user")) From ed46e9df81ab4ca1966e1d1258c21cb8e04d0d3c Mon Sep 17 00:00:00 2001 From: James McKinney <26463+jpmckinney@users.noreply.github.com> Date: Fri, 23 Aug 2024 00:00:20 -0400 Subject: [PATCH 07/20] feat: Improve error messages for integrity errors --- app/routers/lenders.py | 4 ++-- app/routers/users.py | 4 ++-- locale/es/LC_MESSAGES/messages.po | 14 +++++--------- 3 files changed, 9 insertions(+), 13 deletions(-) diff --git a/app/routers/lenders.py b/app/routers/lenders.py index 1c7273f7..4ff2b4ce 100644 --- a/app/routers/lenders.py +++ b/app/routers/lenders.py @@ -46,7 +46,7 @@ async def create_lender( except IntegrityError: raise HTTPException( status_code=status.HTTP_409_CONFLICT, - detail=_("Lender already exists"), + detail=_("Lender with that name already exists"), ) @@ -123,7 +123,7 @@ async def update_lender( except IntegrityError: raise HTTPException( status_code=status.HTTP_409_CONFLICT, - detail=_("Lender already exists"), + detail=_("Lender with that name already exists"), ) diff --git a/app/routers/users.py b/app/routers/users.py index 1214dd12..f6c7e15f 100644 --- a/app/routers/users.py +++ b/app/routers/users.py @@ -58,7 +58,7 @@ async def create_user( except (client.cognito.exceptions.UsernameExistsException, IntegrityError): raise HTTPException( status_code=status.HTTP_409_CONFLICT, - detail=_("Username already exists"), + detail=_("User with that email already exists"), ) @@ -352,5 +352,5 @@ async def update_user( except IntegrityError: raise HTTPException( status_code=status.HTTP_409_CONFLICT, - detail=_("User already exists"), + detail=_("User with that email already exists"), ) diff --git a/locale/es/LC_MESSAGES/messages.po b/locale/es/LC_MESSAGES/messages.po index 979d4ab5..dc3a571e 100644 --- a/locale/es/LC_MESSAGES/messages.po +++ b/locale/es/LC_MESSAGES/messages.po @@ -438,17 +438,17 @@ msgid "Stage" msgstr "Etapa" #: app/routers/lenders.py:49 app/routers/lenders.py:126 -msgid "Lender already exists" -msgstr "La entidad financiera ya existe" +msgid "Lender with that name already exists" +msgstr "La entidad financiera con ese nombre ya existe" #: app/routers/guest/applications.py:378 app/routers/guest/applications.py:467 #: app/routers/lenders.py:192 msgid "Credit product not found" msgstr "Producto crediticio no encontrado" -#: app/routers/users.py:61 -msgid "Username already exists" -msgstr "El nombre de usuario ya existe" +#: app/routers/users.py:61 app/routers/users.py:367 +msgid "User with that email already exists" +msgstr "El usuario con ese correo ya existe" #: app/routers/users.py:104 msgid "Password changed with MFA setup required" @@ -486,10 +486,6 @@ msgstr "Usuario deslogueado correctamente" msgid "An email with a reset link was sent to end user" msgstr "Se envió un correo con el enlace de reseteo de contraseña." -#: app/routers/users.py:367 -msgid "User already exists" -msgstr "El usuario ya existe" - #: app/routers/guest/applications.py:321 app/routers/guest/applications.py:371 #: app/routers/guest/applications.py:460 app/routers/guest/applications.py:526 msgid "Credit product not selected" From 5eec7bffed1d8630fe54272b8c0b9daf15797fed Mon Sep 17 00:00:00 2001 From: James McKinney <26463+jpmckinney@users.noreply.github.com> Date: Fri, 23 Aug 2024 00:00:56 -0400 Subject: [PATCH 08/20] i18n: Regenerate PO file. Correct one typo. --- app/routers/guest/applications.py | 2 +- locale/es/LC_MESSAGES/messages.po | 230 ++++++++++++++---------------- 2 files changed, 105 insertions(+), 127 deletions(-) diff --git a/app/routers/guest/applications.py b/app/routers/guest/applications.py index 0ee3055a..dc5d34df 100644 --- a/app/routers/guest/applications.py +++ b/app/routers/guest/applications.py @@ -793,7 +793,7 @@ async def find_alternative_credit_option( if app_action: raise HTTPException( status_code=status.HTTP_409_CONFLICT, - detail=_("A new application has alredy been created from this one"), + detail=_("A new application has already been created from this one"), ) # Copy the application, changing the uuid, status, and borrower_accepted_at. diff --git a/locale/es/LC_MESSAGES/messages.po b/locale/es/LC_MESSAGES/messages.po index dc3a571e..1625695c 100644 --- a/locale/es/LC_MESSAGES/messages.po +++ b/locale/es/LC_MESSAGES/messages.po @@ -8,7 +8,7 @@ msgid "" msgstr "" "Project-Id-Version: PROJECT VERSION\n" "Report-Msgid-Bugs-To: EMAIL@ADDRESS\n" -"POT-Creation-Date: 2024-08-22 12:34-0400\n" +"POT-Creation-Date: 2024-08-22 23:59-0400\n" "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n" "Last-Translator: FULL NAME \n" "Language: es\n" @@ -20,226 +20,226 @@ msgstr "" "Generated-By: Babel 2.16.0\n" #. BorrowerDocumentType -#: README.rst:125 +#: README.rst:129 msgid "INCORPORATION_DOCUMENT" msgstr "Documento de incorporación" #. BorrowerDocumentType -#: README.rst:126 +#: README.rst:130 msgid "SUPPLIER_REGISTRATION_DOCUMENT" msgstr "Documento de registro de proveedor" #. BorrowerDocumentType -#: README.rst:127 +#: README.rst:131 msgid "BANK_NAME" msgstr "Nombre del banco" #. BorrowerDocumentType -#: README.rst:128 +#: README.rst:132 msgid "BANK_CERTIFICATION_DOCUMENT" msgstr "Certificado bancario" #. BorrowerDocumentType -#: README.rst:129 +#: README.rst:133 msgid "FINANCIAL_STATEMENT" msgstr "Estados financieros" #. BorrowerDocumentType -#: README.rst:130 +#: README.rst:134 msgid "SIGNED_CONTRACT" msgstr "Contrato firmado" #. BorrowerDocumentType -#: README.rst:131 +#: README.rst:135 msgid "SHAREHOLDER_COMPOSITION" msgstr "Composición accionaria" #. BorrowerDocumentType -#: README.rst:132 +#: README.rst:136 msgid "CHAMBER_OF_COMMERCE" msgstr "Cámara de comercio" #. BorrowerDocumentType -#: README.rst:133 +#: README.rst:137 msgid "THREE_LAST_BANK_STATEMENT" msgstr "Últimos tres extractos bancarios" #. ApplicationStatus -#: README.rst:158 +#: README.rst:162 msgid "PENDING" msgstr "Pendiente" #. ApplicationStatus -#: README.rst:162 +#: README.rst:166 msgid "DECLINED" msgstr "Declinado" #. ApplicationStatus -#: README.rst:166 +#: README.rst:170 msgid "ACCEPTED" msgstr "Aceptado" #. ApplicationStatus -#: README.rst:170 +#: README.rst:174 msgid "SUBMITTED" msgstr "Enviado" #. ApplicationStatus -#: README.rst:174 +#: README.rst:178 msgid "STARTED" msgstr "Empezado" #. ApplicationStatus -#: README.rst:179 +#: README.rst:183 msgid "REJECTED" msgstr "Rechazado" #. ApplicationStatus -#: README.rst:183 +#: README.rst:187 msgid "INFORMATION_REQUESTED" msgstr "Información solicitada" #. ApplicationStatus -#: README.rst:187 +#: README.rst:191 msgid "LAPSED" msgstr "Caducado" #. ApplicationStatus -#: README.rst:191 +#: README.rst:195 msgid "APPROVED" msgstr "Precalificado" #. ApplicationStatus -#: README.rst:195 +#: README.rst:199 msgid "CONTRACT_UPLOADED" msgstr "Contrato cargado" #. ApplicationStatus -#: README.rst:199 +#: README.rst:203 msgid "COMPLETED" msgstr "Completado" #. BorrowerSize -#: README.rst:281 +#: README.rst:287 msgid "NOT_INFORMED" msgstr "No informado" #. BorrowerSize -#: README.rst:282 +#: README.rst:288 msgid "MICRO" msgstr "0 a 10" #. BorrowerSize -#: README.rst:283 +#: README.rst:289 msgid "SMALL" msgstr "11 a 50" #. BorrowerSize -#: README.rst:284 +#: README.rst:290 msgid "MEDIUM" msgstr "51 a 200" #. BorrowerSize -#: README.rst:285 +#: README.rst:291 msgid "BIG" msgstr "+ 200" #. BorrowerSector -#: README.rst:289 +#: README.rst:295 msgid "agricultura" msgstr "Agricultura, ganadería, caza, silvicultura y pesca" #. BorrowerSector -#: README.rst:290 +#: README.rst:296 msgid "minas" msgstr "Explotación de minas y canteras" #. BorrowerSector -#: README.rst:291 +#: README.rst:297 msgid "manufactura" msgstr "Industrias manufactureras" #. BorrowerSector -#: README.rst:292 +#: README.rst:298 msgid "electricidad" msgstr "Suministro de electricidad, gas, vapor y aire acondicionado" #. BorrowerSector -#: README.rst:293 +#: README.rst:299 msgid "agua" msgstr "" "Distribución de agua; evacuación y tratamiento de aguas residuales, " "gestión de desechos y actividades de saneamiento ambiental" #. BorrowerSector -#: README.rst:294 +#: README.rst:300 msgid "construccion" msgstr "Construcción" #. BorrowerSector -#: README.rst:295 +#: README.rst:301 msgid "transporte" msgstr "Transporte y almacenamiento" #. BorrowerSector -#: README.rst:296 +#: README.rst:302 msgid "alojamiento" msgstr "Alojamiento y servicios de comida" #. BorrowerSector -#: README.rst:297 +#: README.rst:303 msgid "comunicaciones" msgstr "Información y comunicaciones" #. BorrowerSector -#: README.rst:298 +#: README.rst:304 msgid "actividades_financieras" msgstr "Actividades financieras y de seguros" #. BorrowerSector -#: README.rst:299 +#: README.rst:305 msgid "actividades_inmobiliarias" msgstr "Actividades inmobiliarias" #. BorrowerSector -#: README.rst:300 +#: README.rst:306 msgid "actividades_profesionales" msgstr "Actividades profesionales, científicas y técnicas" #. BorrowerSector -#: README.rst:301 +#: README.rst:307 msgid "actividades_servicios_administrativos" msgstr "Actividades de servicios administrativos y de apoyo" #. BorrowerSector -#: README.rst:302 +#: README.rst:308 msgid "administracion_publica" msgstr "" "Administración pública y defensa; planes de seguridad social de " "afiliación obligatoria" #. BorrowerSector -#: README.rst:303 +#: README.rst:309 msgid "educacion" msgstr "Educación" #. BorrowerSector -#: README.rst:304 +#: README.rst:310 msgid "atencion_salud" msgstr "Actividades de atención de la salud humana y de asistencia social" #. BorrowerSector -#: README.rst:305 +#: README.rst:311 msgid "actividades_artisticas" msgstr "Actividades artísticas, de entretenimiento y recreación" #. BorrowerSector -#: README.rst:306 +#: README.rst:312 msgid "otras_actividades" msgstr "Otras actividades de servicios" #. BorrowerSector -#: README.rst:307 +#: README.rst:313 msgid "actividades_hogares" msgstr "" "Actividades de los hogares individuales en calidad de empleadores; " @@ -247,7 +247,7 @@ msgstr "" " de bienes y servicios para uso propio" #. BorrowerSector -#: README.rst:308 +#: README.rst:314 msgid "actividades_organizaciones_extraterritoriales" msgstr "Actividades de organizaciones y entidades extraterritoriales" @@ -372,43 +372,39 @@ msgstr "Opción de crédito alternativa" msgid "Application updated" msgstr "Aplicación actualizada" -#: app/util.py:54 +#: app/util.py:55 #, python-format msgid "%(model_name)s not found" msgstr "%(model_name)s no encontrado" -#: app/util.py:112 +#: app/util.py:110 msgid "Format not allowed. It must be a PNG, JPEG, or PDF file" msgstr "Formato no permitido. El formato debe ser un PNG, JPEG or archivo PDF" -#: app/util.py:118 +#: app/util.py:116 msgid "File is too large" msgstr "El archivo es muy grande" -#: app/routers/applications.py:175 +#: app/routers/applications.py:174 msgid "Some borrower data field are not verified" msgstr "Algunos campos de datos de la empresa no fueron verificados" -#: app/routers/applications.py:186 +#: app/routers/applications.py:185 msgid "Some documents are not verified" msgstr "Algunos documentos no fueron verificados" -#: app/routers/applications.py:332 +#: app/routers/applications.py:331 msgid "Award not found" msgstr "La adjudicación no ha sido encontrada" -#: app/routers/applications.py:377 +#: app/routers/applications.py:376 msgid "Borrower not found" msgstr "La empresa no ha sido encontrada" -#: app/routers/applications.py:386 +#: app/routers/applications.py:385 msgid "This column cannot be updated" msgstr "Esta columna no puede ser actualizada" -#: app/routers/applications.py:585 -msgid "There was an error" -msgstr "Ocurrió un error" - #: app/routers/downloads.py:88 app/routers/downloads.py:106 msgid "Application Details" msgstr "Detalles de la aplicación" @@ -417,11 +413,11 @@ msgstr "Detalles de la aplicación" msgid "Previous Public Sector Contracts" msgstr "Contratos anteriores con el sector público" -#: app/routers/downloads.py:148 app/utils/tables.py:209 +#: app/routers/downloads.py:148 app/utils/tables.py:211 msgid "National Tax ID" msgstr "Identificación fiscal nacional" -#: app/routers/downloads.py:149 app/utils/tables.py:201 +#: app/routers/downloads.py:149 app/utils/tables.py:203 msgid "Legal Name" msgstr "Nombre legal" @@ -441,12 +437,12 @@ msgstr "Etapa" msgid "Lender with that name already exists" msgstr "La entidad financiera con ese nombre ya existe" -#: app/routers/guest/applications.py:378 app/routers/guest/applications.py:467 +#: app/routers/guest/applications.py:377 app/routers/guest/applications.py:466 #: app/routers/lenders.py:192 msgid "Credit product not found" msgstr "Producto crediticio no encontrado" -#: app/routers/users.py:61 app/routers/users.py:367 +#: app/routers/users.py:61 app/routers/users.py:344 msgid "User with that email already exists" msgstr "El usuario con ese correo ya existe" @@ -454,68 +450,51 @@ msgstr "El usuario con ese correo ya existe" msgid "Password changed with MFA setup required" msgstr "Cambio de contraseña con configuración de MFA requerido" -#: app/routers/users.py:113 app/routers/users.py:193 +#: app/routers/users.py:113 app/routers/users.py:187 msgid "Temporal password is expired, please request a new one" msgstr "La contraseña temporal ha expirado, por favor solicite una nueva" #: app/routers/users.py:117 -msgid "There was an error trying to update the password" -msgstr "Ocurrió un error al tratar de actualizar la contraseña" - -#: app/routers/users.py:120 msgid "Password changed" msgstr "Contraseña cambiada" -#: app/routers/users.py:145 +#: app/routers/users.py:142 msgid "Invalid session for the user, session is expired" msgstr "Sesión inválida para el usuario, la sesión ha expirado" -#: app/routers/users.py:149 -msgid "There was an error trying to setup mfa" -msgstr "Ocurrió un error tratando de configurar el MFA" - -#: app/routers/users.py:152 +#: app/routers/users.py:146 msgid "MFA configured successfully" msgstr "El MFA fue configurado correctamente" -#: app/routers/users.py:237 +#: app/routers/users.py:217 msgid "User logged out successfully" msgstr "Usuario deslogueado correctamente" -#: app/routers/users.py:286 +#: app/routers/users.py:263 msgid "An email with a reset link was sent to end user" msgstr "Se envió un correo con el enlace de reseteo de contraseña." -#: app/routers/guest/applications.py:321 app/routers/guest/applications.py:371 -#: app/routers/guest/applications.py:460 app/routers/guest/applications.py:526 +#: app/routers/guest/applications.py:320 app/routers/guest/applications.py:370 +#: app/routers/guest/applications.py:459 app/routers/guest/applications.py:525 msgid "Credit product not selected" msgstr "El producto crediticio no fue seleccionado" -#: app/routers/guest/applications.py:327 +#: app/routers/guest/applications.py:326 msgid "Cannot rollback at this stage" msgstr "No se puede retroceder en esta etapa" -#: app/routers/guest/applications.py:532 +#: app/routers/guest/applications.py:531 msgid "Lender not selected" msgstr "No se ha seleccionado la entidad financiera" -#: app/routers/guest/applications.py:548 -msgid "There was an error submitting the application" -msgstr "Ocurrió un error enviando la aplicación" - -#: app/routers/guest/applications.py:597 +#: app/routers/guest/applications.py:589 msgid "Cannot upload document at this stage" msgstr "No se pueden subir documentos en esta etapa" -#: app/routers/guest/applications.py:804 -msgid "A new application has alredy been created from this one" +#: app/routers/guest/applications.py:796 +msgid "A new application has already been created from this one" msgstr "Una nueva aplicación ya ha sido creada desde esta" -#: app/routers/guest/applications.py:823 -#, python-format -msgid "There was a problem copying the application. %(exception)s" -msgstr "Ocurrió un problema copiando la aplicación. %(exception)s" - #: app/routers/guest/emails.py:34 msgid "New email is not valid" msgstr "El nuevo correo no es válido" @@ -528,137 +507,136 @@ msgstr "La aplicación no tiene la confirmación de correo pendiente" msgid "Not authorized to modify this application" msgstr "No autorizado a modificar la aplicación en esta etapa" -#: app/utils/tables.py:41 +#: app/utils/tables.py:43 msgid "Financing Options" msgstr "Opciones crediticias" -#: app/utils/tables.py:42 app/utils/tables.py:128 app/utils/tables.py:198 -#: app/utils/tables.py:248 +#: app/utils/tables.py:44 app/utils/tables.py:130 app/utils/tables.py:200 +#: app/utils/tables.py:250 msgid "Data" msgstr "Datos" -#: app/utils/tables.py:45 +#: app/utils/tables.py:47 msgid "Lender" msgstr "Entidad financiera" -#: app/utils/tables.py:49 +#: app/utils/tables.py:51 msgid "Amount requested" msgstr "Monto solicitado" -#: app/utils/tables.py:57 app/utils/tables.py:81 +#: app/utils/tables.py:59 app/utils/tables.py:83 msgid "Type" msgstr "Tipo" -#: app/utils/tables.py:58 +#: app/utils/tables.py:60 msgid "Loan" msgstr "Préstamo" -#: app/utils/tables.py:63 +#: app/utils/tables.py:65 msgid "Payment start date" msgstr "Fecha de inicio de pago" -#: app/utils/tables.py:69 +#: app/utils/tables.py:71 msgid "Repayment terms" msgstr "Periodo de pago" -#: app/utils/tables.py:70 +#: app/utils/tables.py:72 #, python-format msgid "%(repayment_years)s year(s), %(repayment_months)s month(s)" msgstr "%(repayment_years)s año(s), %(repayment_months)s mes(es)" -#: app/utils/tables.py:82 +#: app/utils/tables.py:84 msgid "Credit Line" msgstr "Línea de crédito" -#: app/utils/tables.py:89 +#: app/utils/tables.py:91 msgid "Contract amount" msgstr "Monto del contrato" -#: app/utils/tables.py:95 +#: app/utils/tables.py:97 msgid "Credit amount" msgstr "Monto del crédito" -#: app/utils/tables.py:127 +#: app/utils/tables.py:129 msgid "Award Data" msgstr "Datos de la adjudicación" -#: app/utils/tables.py:131 +#: app/utils/tables.py:133 msgid "View data in SECOP II" msgstr "Ver datos en SECOP II" -#: app/utils/tables.py:135 +#: app/utils/tables.py:137 msgid "Award Title" msgstr "Título de la adjudicación" -#: app/utils/tables.py:139 +#: app/utils/tables.py:141 msgid "Contracting Process ID" msgstr "Identificador del proceso de contratación" -#: app/utils/tables.py:143 +#: app/utils/tables.py:145 msgid "Award Description" msgstr "Descripción de la adjudicación" -#: app/utils/tables.py:147 +#: app/utils/tables.py:149 msgid "Award Date" msgstr "Fecha de la adjudicación" -#: app/utils/tables.py:151 +#: app/utils/tables.py:153 msgid "Award Value Currency & Amount" msgstr "Monto y moneda de la adjudicación" -#: app/utils/tables.py:155 +#: app/utils/tables.py:157 msgid "Contract Start Date" msgstr "Fecha de inicio del contrato" -#: app/utils/tables.py:159 +#: app/utils/tables.py:161 msgid "Contract End Date" msgstr "Fecha de fin del contrato" -#: app/utils/tables.py:163 +#: app/utils/tables.py:165 msgid "Payment Method" msgstr "Método de pago" -#: app/utils/tables.py:167 +#: app/utils/tables.py:169 msgid "Buyer Name" msgstr "Nombre de la entidad compradora" -#: app/utils/tables.py:174 +#: app/utils/tables.py:176 msgid "Procurement Method" msgstr "Método de contratación" -#: app/utils/tables.py:178 +#: app/utils/tables.py:180 msgid "Contract Type" msgstr "Tipo de contrato" -#: app/utils/tables.py:197 +#: app/utils/tables.py:199 msgid "MSME Data" msgstr "Datos de la empresa" -#: app/utils/tables.py:205 +#: app/utils/tables.py:207 msgid "Address" msgstr "Dirección" -#: app/utils/tables.py:213 +#: app/utils/tables.py:215 msgid "Registration Type" msgstr "Tipo de registro" -#: app/utils/tables.py:217 +#: app/utils/tables.py:219 msgid "Size" msgstr "Tamaño" -#: app/utils/tables.py:221 +#: app/utils/tables.py:223 msgid "Sector" msgstr "Sector" -#: app/utils/tables.py:225 +#: app/utils/tables.py:227 msgid "Annual Revenue" msgstr "Valor estimado de facturación anual de su empresa" -#: app/utils/tables.py:229 +#: app/utils/tables.py:231 msgid "Business Email" msgstr "Correo institucional" -#: app/utils/tables.py:247 +#: app/utils/tables.py:249 msgid "MSME Documents" msgstr "Documentos de la empresa" - From 4c43d1b0b8e62403e6327b636ce8ca65e8d7a110 Mon Sep 17 00:00:00 2001 From: James McKinney <26463+jpmckinney@users.noreply.github.com> Date: Fri, 23 Aug 2024 00:20:48 -0400 Subject: [PATCH 09/20] feat: Add general error handler, to return JSON response for unexpected errors (follow-up a80caaa) --- app/main.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/app/main.py b/app/main.py index e16edb33..6bb821ec 100644 --- a/app/main.py +++ b/app/main.py @@ -1,6 +1,8 @@ from fastapi import FastAPI from fastapi.middleware.cors import CORSMiddleware +from fastapi.responses import JSONResponse +from app.i18n import _ from app.routers import applications, downloads, guest, lenders, statistics, users from app.settings import app_settings @@ -21,3 +23,8 @@ app.include_router(downloads.router) app.include_router(lenders.router) app.include_router(statistics.router) + + +@app.exception_handler(500) +async def http_exception_handler(request, exc): + return JSONResponse({"detail": _("An unexpected error occurred")}, status_code=500) From 9dd3fe2d6886a9cdddad09a5ad6d1e86fe4cceed Mon Sep 17 00:00:00 2001 From: James McKinney <26463+jpmckinney@users.noreply.github.com> Date: Fri, 23 Aug 2024 00:22:35 -0400 Subject: [PATCH 10/20] test: Fix test_update_user --- tests/conftest.py | 9 +++++++-- tests/routers/test_users.py | 27 ++++++++++++--------------- 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index da368d4d..787c47dc 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -169,11 +169,16 @@ def unauthorized_lender(session): @pytest.fixture -def admin_header(session, aws_client): +def admin_email(): + return f"ocp-test-{uuid.uuid4()}@open-contracting.org" + + +@pytest.fixture +def admin_header(session, aws_client, admin_email): return create_user( session, aws_client, - email=f"ocp-test-{uuid.uuid4()}@open-contracting.org", + email=admin_email, name="OCP Test User", type=models.UserType.OCP, ) diff --git a/tests/routers/test_users.py b/tests/routers/test_users.py index ed77b82c..633f1bab 100644 --- a/tests/routers/test_users.py +++ b/tests/routers/test_users.py @@ -1,4 +1,5 @@ import logging +import uuid from fastapi import status @@ -36,25 +37,21 @@ def test_create_and_get_user(client, admin_header, lender_header, user_payload): def test_update_user(client, admin_header, lender_header, user_payload): + new_email = f"new-name-{uuid.uuid4()}@example.com" + response = client.post("/users", json=user_payload, headers=admin_header) + data = response.json() + user_id = data["id"] + assert_ok(response) - assert response.json()["name"] == user_payload["name"] + assert data["name"] == user_payload["name"] - # update user 3 since 1 is ocp test user and 2 lender test user - response = client.put( - "/users/3", - json={"email": "new_name@noreply.open-contracting.org"}, - headers=admin_header, - ) + response = client.put(f"/users/{user_id}", json={"email": new_email}, headers=admin_header) assert_ok(response) - assert response.json()["email"] == "new_name@noreply.open-contracting.org" - - response = client.put( - "/users/3", - json={"email": "anoter_email@noreply.open-contracting.org"}, - headers=lender_header, - ) - assert response.status_code == status.HTTP_401_UNAUTHORIZED + assert response.json()["email"] == new_email + + response = client.put(f"/users/{user_id}", json={"email": "another-email@example.com"}, headers=lender_header) + assert response.status_code == status.HTTP_403_FORBIDDEN assert response.json() == {"detail": _("Insufficient permissions")} From 0f0c7be2387bfd24e8e0fbe7d2f2c09813d471e8 Mon Sep 17 00:00:00 2001 From: James McKinney <26463+jpmckinney@users.noreply.github.com> Date: Fri, 23 Aug 2024 00:37:11 -0400 Subject: [PATCH 11/20] fix: Fix logout endpoint --- app/auth.py | 8 +++++++- app/dependencies.py | 2 +- app/routers/users.py | 37 ++++++++++++++----------------------- tests/routers/test_users.py | 12 +++++++++++- 4 files changed, 33 insertions(+), 26 deletions(-) diff --git a/app/auth.py b/app/auth.py index 6ed411fc..3aedc306 100644 --- a/app/auth.py +++ b/app/auth.py @@ -86,7 +86,13 @@ async def __call__(self, request: Request) -> JWTAuthorizationCredentials | None jwt_token = credentials.credentials - message, signature = jwt_token.rsplit(".", 1) + if "." in jwt_token: + message, signature = jwt_token.rsplit(".", 1) + else: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail=_("JWK invalid"), + ) try: jwt_credentials = JWTAuthorizationCredentials( diff --git a/app/dependencies.py b/app/dependencies.py index d4b629f5..3b0e2940 100644 --- a/app/dependencies.py +++ b/app/dependencies.py @@ -25,7 +25,7 @@ def get_aws_client() -> Generator[aws.Client, None, None]: async def get_auth_credentials(request: Request) -> auth.JWTAuthorizationCredentials | None: - return await auth.JWTAuthorization().__call__(request) + return await auth.JWTAuthorization()(request) async def get_current_user(credentials: auth.JWTAuthorizationCredentials = Depends(get_auth_credentials)) -> str: diff --git a/app/routers/users.py b/app/routers/users.py index f6c7e15f..7a32d261 100644 --- a/app/routers/users.py +++ b/app/routers/users.py @@ -1,12 +1,12 @@ import logging from botocore.exceptions import ClientError -from fastapi import APIRouter, Depends, Header, HTTPException, Query, status +from fastapi import APIRouter, Depends, HTTPException, Query, Request, status from fastapi.encoders import jsonable_encoder from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session, joinedload -from app import aws, dependencies, mail, models, parsers, serializers +from app import auth, aws, dependencies, mail, models, parsers, serializers from app.db import get_db, rollback_on_error from app.i18n import _ from app.settings import app_settings @@ -198,32 +198,23 @@ def login( @router.get( "/users/logout", ) -def logout( - authorization: str | None = Header(None), +async def logout( + request: Request, client: aws.Client = Depends(dependencies.get_aws_client), ) -> serializers.ResponseBase: """ Logout the user from all devices in Cognito. - - :param authorization: The Authorization header, like "Bearer ACCESS_TOKEN". """ - - # The Authorization header is not set if the user is already logged out. - if authorization is not None: - try: - # https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/cognito-idp/client/get_user.html - response = client.cognito.get_user(AccessToken=authorization.split(" ")[1]) - # "If `username` isn’t an alias attribute in your user pool, this value must be the `sub` of a local user - # or the username of a user from a third-party IdP." - # https://docs.aws.amazon.com/cognito/latest/developerguide/user-pool-settings-attributes.html - sub = next(attribute["Value"] for attribute in response["UserAttributes"] if attribute["Name"] == "sub") - # https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/cognito-idp/client/admin_user_global_sign_out.html - client.cognito.admin_user_global_sign_out(UserPoolId=app_settings.cognito_pool_id, Username=sub) - # The user is not signed in ("Access Token has expired", "Invalid token", etc.). - except client.cognito.exceptions.NotAuthorizedException: - pass - except ClientError as e: - logger.exception(e) + try: + # get_auth_credentials() + credentials = await auth.JWTAuthorization()(request) + # get_current_user() + username = credentials.claims["username"] + # https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/cognito-idp/client/admin_user_global_sign_out.html + client.cognito.admin_user_global_sign_out(UserPoolId=app_settings.cognito_pool_id, Username=username) + # The user is not signed in. + except (HTTPException, KeyError): + pass return serializers.ResponseBase(detail=_("User logged out successfully")) diff --git a/tests/routers/test_users.py b/tests/routers/test_users.py index 633f1bab..f2f5bd63 100644 --- a/tests/routers/test_users.py +++ b/tests/routers/test_users.py @@ -62,7 +62,17 @@ def test_duplicate_user(client, admin_header, user_payload): # duplicate user response = client.post("/users", json=user_payload, headers=admin_header) assert response.status_code == status.HTTP_409_CONFLICT - assert response.json() == {"detail": _("Username already exists")} + assert response.json() == {"detail": _("User with that email already exists")} + + +def test_logout(client, admin_header, caplog): + caplog.set_level(logging.ERROR) + + response = client.get("/users/logout", headers=admin_header) + + assert response.status_code == status.HTTP_200_OK + assert response.json() == {"detail": "User logged out successfully"} + assert not caplog.records def test_logout_invalid_authorization_header(client, caplog): From 94a39a3c73ffd75225d0c073557e738bdc5cdea7 Mon Sep 17 00:00:00 2001 From: James McKinney <26463+jpmckinney@users.noreply.github.com> Date: Fri, 23 Aug 2024 00:37:24 -0400 Subject: [PATCH 12/20] chore: Remove unused loggers --- app/routers/applications.py | 3 --- app/routers/guest/applications.py | 3 --- app/routers/users.py | 4 ---- 3 files changed, 10 deletions(-) diff --git a/app/routers/applications.py b/app/routers/applications.py index ad0dd555..f8f0c118 100644 --- a/app/routers/applications.py +++ b/app/routers/applications.py @@ -1,4 +1,3 @@ -import logging from datetime import datetime from typing import Any, cast @@ -12,8 +11,6 @@ from app.i18n import _ from app.util import SortOrder, get_order_by -logger = logging.getLogger(__name__) - router = APIRouter() diff --git a/app/routers/guest/applications.py b/app/routers/guest/applications.py index dc5d34df..2c2c32bf 100644 --- a/app/routers/guest/applications.py +++ b/app/routers/guest/applications.py @@ -1,4 +1,3 @@ -import logging from datetime import datetime from typing import Any, cast @@ -11,8 +10,6 @@ from app.db import get_db, rollback_on_error from app.i18n import _ -logger = logging.getLogger(__name__) - router = APIRouter() diff --git a/app/routers/users.py b/app/routers/users.py index 7a32d261..25cd486c 100644 --- a/app/routers/users.py +++ b/app/routers/users.py @@ -1,5 +1,3 @@ -import logging - from botocore.exceptions import ClientError from fastapi import APIRouter, Depends, HTTPException, Query, Request, status from fastapi.encoders import jsonable_encoder @@ -12,8 +10,6 @@ from app.settings import app_settings from app.util import SortOrder, get_object_or_404, get_order_by -logger = logging.getLogger(__name__) - router = APIRouter() From b400be9288a9cf296c46dc1e973cd541dc324b78 Mon Sep 17 00:00:00 2001 From: James McKinney <26463+jpmckinney@users.noreply.github.com> Date: Fri, 23 Aug 2024 00:37:38 -0400 Subject: [PATCH 13/20] test: Fix test expectation for new error message --- tests/routers/test_lenders.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/routers/test_lenders.py b/tests/routers/test_lenders.py index 47c4369a..e43db055 100644 --- a/tests/routers/test_lenders.py +++ b/tests/routers/test_lenders.py @@ -95,7 +95,7 @@ def test_create_lender(client, admin_header, lender_header, lender): # tries to create same lender twice response = client.post("/lenders", json=payload, headers=admin_header) assert response.status_code == status.HTTP_409_CONFLICT - assert response.json() == {"detail": _("Lender already exists")} + assert response.json() == {"detail": _("Lender with that name already exists")} response = client.post("/lenders", json=payload, headers=lender_header) assert response.status_code == status.HTTP_403_FORBIDDEN From 44c059e5db96de69528cad425f42797585b9c547 Mon Sep 17 00:00:00 2001 From: James McKinney <26463+jpmckinney@users.noreply.github.com> Date: Fri, 23 Aug 2024 00:41:57 -0400 Subject: [PATCH 14/20] feat: Log 413 errors to Sentry, to monitor whether 5MB is too small, closes #349 --- app/settings.py | 4 ++-- app/util.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/settings.py b/app/settings.py index 6b89d068..95c66ae0 100644 --- a/app/settings.py +++ b/app/settings.py @@ -195,10 +195,10 @@ class Settings(BaseSettings): # FastAPI uses 400 for request validation errors, which shouldn't occur unless the frontend is misimplemented. integrations=[ StarletteIntegration( - failed_request_status_codes=[400, range(500, 599)], + failed_request_status_codes=[400, 413, range(500, 599)], ), FastApiIntegration( - failed_request_status_codes=[400, range(500, 599)], + failed_request_status_codes=[400, 413, range(500, 599)], ), ], ) diff --git a/app/util.py b/app/util.py index 3c9ccffd..d1341d37 100644 --- a/app/util.py +++ b/app/util.py @@ -112,7 +112,7 @@ def validate_file(file: UploadFile = File(...)) -> tuple[bytes, str | None]: new_file = file.file.read() if len(new_file) >= MAX_FILE_SIZE: # 10MB in bytes raise HTTPException( - status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + status_code=status.HTTP_413_REQUEST_ENTITY_TOO_LARGE, detail=_("File is too large"), ) return new_file, filename From 392da2313c6bd1fef1c7a2af703277ad576dffda Mon Sep 17 00:00:00 2001 From: James McKinney <26463+jpmckinney@users.noreply.github.com> Date: Fri, 23 Aug 2024 00:54:11 -0400 Subject: [PATCH 15/20] fix: Prevent user enumeration on login endpoint --- app/routers/users.py | 7 +++- locale/es/LC_MESSAGES/messages.po | 59 ++++++++++++++++++------------- tests/routers/test_users.py | 26 ++++++++------ 3 files changed, 55 insertions(+), 37 deletions(-) diff --git a/app/routers/users.py b/app/routers/users.py index 25cd486c..0573d603 100644 --- a/app/routers/users.py +++ b/app/routers/users.py @@ -161,7 +161,12 @@ def login( :param payload: The user data including the username, password, and MFA code. :return: The response containing the user information and tokens if the login is successful. """ - user = get_object_or_404(session, models.User, "email", payload.username) + user = models.User.first_by(session, "email", payload.username) + if not user: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, # prevent user enumeration + detail=_("Invalid username or password"), + ) try: response = client.initiate_auth(payload.username, payload.password) diff --git a/locale/es/LC_MESSAGES/messages.po b/locale/es/LC_MESSAGES/messages.po index 1625695c..67236bdf 100644 --- a/locale/es/LC_MESSAGES/messages.po +++ b/locale/es/LC_MESSAGES/messages.po @@ -8,7 +8,7 @@ msgid "" msgstr "" "Project-Id-Version: PROJECT VERSION\n" "Report-Msgid-Bugs-To: EMAIL@ADDRESS\n" -"POT-Creation-Date: 2024-08-22 23:59-0400\n" +"POT-Creation-Date: 2024-08-23 00:49-0400\n" "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n" "Last-Translator: FULL NAME \n" "Language: es\n" @@ -259,11 +259,11 @@ msgstr "La clave pública JWK no fue encontrada" msgid "Wrong authentication method" msgstr "Método de autenticación equivocado" -#: app/auth.py:102 app/auth.py:108 +#: app/auth.py:94 app/auth.py:108 app/auth.py:114 msgid "JWK invalid" msgstr "JWK inválido" -#: app/auth.py:115 +#: app/auth.py:121 msgid "Not authenticated" msgstr "No autenticado" @@ -372,6 +372,10 @@ msgstr "Opción de crédito alternativa" msgid "Application updated" msgstr "Aplicación actualizada" +#: app/main.py:30 +msgid "An unexpected error occurred" +msgstr "Ocurrió un error inesperado" + #: app/util.py:55 #, python-format msgid "%(model_name)s not found" @@ -385,23 +389,23 @@ msgstr "Formato no permitido. El formato debe ser un PNG, JPEG or archivo PDF" msgid "File is too large" msgstr "El archivo es muy grande" -#: app/routers/applications.py:174 +#: app/routers/applications.py:171 msgid "Some borrower data field are not verified" msgstr "Algunos campos de datos de la empresa no fueron verificados" -#: app/routers/applications.py:185 +#: app/routers/applications.py:182 msgid "Some documents are not verified" msgstr "Algunos documentos no fueron verificados" -#: app/routers/applications.py:331 +#: app/routers/applications.py:328 msgid "Award not found" msgstr "La adjudicación no ha sido encontrada" -#: app/routers/applications.py:376 +#: app/routers/applications.py:373 msgid "Borrower not found" msgstr "La empresa no ha sido encontrada" -#: app/routers/applications.py:385 +#: app/routers/applications.py:382 msgid "This column cannot be updated" msgstr "Esta columna no puede ser actualizada" @@ -435,63 +439,67 @@ msgstr "Etapa" #: app/routers/lenders.py:49 app/routers/lenders.py:126 msgid "Lender with that name already exists" -msgstr "La entidad financiera con ese nombre ya existe" +msgstr "Ya existe un entidad financiera con ese nombre" -#: app/routers/guest/applications.py:377 app/routers/guest/applications.py:466 +#: app/routers/guest/applications.py:374 app/routers/guest/applications.py:463 #: app/routers/lenders.py:192 msgid "Credit product not found" msgstr "Producto crediticio no encontrado" -#: app/routers/users.py:61 app/routers/users.py:344 +#: app/routers/users.py:57 app/routers/users.py:347 msgid "User with that email already exists" -msgstr "El usuario con ese correo ya existe" +msgstr "Ya existe un usuario con ese correo" -#: app/routers/users.py:104 +#: app/routers/users.py:100 msgid "Password changed with MFA setup required" msgstr "Cambio de contraseña con configuración de MFA requerido" -#: app/routers/users.py:113 app/routers/users.py:187 +#: app/routers/users.py:109 app/routers/users.py:188 msgid "Temporal password is expired, please request a new one" msgstr "La contraseña temporal ha expirado, por favor solicite una nueva" -#: app/routers/users.py:117 +#: app/routers/users.py:113 msgid "Password changed" msgstr "Contraseña cambiada" -#: app/routers/users.py:142 +#: app/routers/users.py:138 msgid "Invalid session for the user, session is expired" msgstr "Sesión inválida para el usuario, la sesión ha expirado" -#: app/routers/users.py:146 +#: app/routers/users.py:142 msgid "MFA configured successfully" msgstr "El MFA fue configurado correctamente" -#: app/routers/users.py:217 +#: app/routers/users.py:168 +msgid "Invalid username or password" +msgstr "Nombre de usuario o contraseña no válidos" + +#: app/routers/users.py:220 msgid "User logged out successfully" msgstr "Usuario deslogueado correctamente" -#: app/routers/users.py:263 +#: app/routers/users.py:266 msgid "An email with a reset link was sent to end user" msgstr "Se envió un correo con el enlace de reseteo de contraseña." -#: app/routers/guest/applications.py:320 app/routers/guest/applications.py:370 -#: app/routers/guest/applications.py:459 app/routers/guest/applications.py:525 +#: app/routers/guest/applications.py:317 app/routers/guest/applications.py:367 +#: app/routers/guest/applications.py:456 app/routers/guest/applications.py:522 msgid "Credit product not selected" msgstr "El producto crediticio no fue seleccionado" -#: app/routers/guest/applications.py:326 +#: app/routers/guest/applications.py:323 msgid "Cannot rollback at this stage" msgstr "No se puede retroceder en esta etapa" -#: app/routers/guest/applications.py:531 +#: app/routers/guest/applications.py:528 msgid "Lender not selected" msgstr "No se ha seleccionado la entidad financiera" -#: app/routers/guest/applications.py:589 +#: app/routers/guest/applications.py:586 msgid "Cannot upload document at this stage" msgstr "No se pueden subir documentos en esta etapa" -#: app/routers/guest/applications.py:796 +#: app/routers/guest/applications.py:793 msgid "A new application has already been created from this one" msgstr "Una nueva aplicación ya ha sido creada desde esta" @@ -640,3 +648,4 @@ msgstr "Correo institucional" #: app/utils/tables.py:249 msgid "MSME Documents" msgstr "Documentos de la empresa" + diff --git a/tests/routers/test_users.py b/tests/routers/test_users.py index f2f5bd63..5cf91a6a 100644 --- a/tests/routers/test_users.py +++ b/tests/routers/test_users.py @@ -1,4 +1,3 @@ -import logging import uuid from fastapi import status @@ -65,31 +64,36 @@ def test_duplicate_user(client, admin_header, user_payload): assert response.json() == {"detail": _("User with that email already exists")} -def test_logout(client, admin_header, caplog): - caplog.set_level(logging.ERROR) +def test_login_invalid_username(client): + response = client.post("/users/login", json={"username": "nonexistent"}) + assert response.status_code == status.HTTP_403_FORBIDDEN + assert response.json() == {"detail": "Invalid username or password"} + + +def test_logout(client, admin_header): response = client.get("/users/logout", headers=admin_header) assert response.status_code == status.HTTP_200_OK assert response.json() == {"detail": "User logged out successfully"} - assert not caplog.records - -def test_logout_invalid_authorization_header(client, caplog): - caplog.set_level(logging.ERROR) +def test_logout_invalid_authorization_header_no_period(client): response = client.get("/users/logout", headers={"Authorization": "Bearer ACCESS_TOKEN"}) assert_ok(response) assert response.json() == {"detail": _("User logged out successfully")} - assert not caplog.records -def test_logout_no_authorization_header(client, caplog): - caplog.set_level(logging.ERROR) +def test_logout_invalid_authorization_header(client): + response = client.get("/users/logout", headers={"Authorization": "Bearer ACCESS.TOKEN"}) + + assert_ok(response) + assert response.json() == {"detail": _("User logged out successfully")} + +def test_logout_no_authorization_header(client): response = client.get("/users/logout") assert_ok(response) assert response.json() == {"detail": _("User logged out successfully")} - assert not caplog.records From c7a0a9816bb30a54ccf6ee676d1b240209388b6a Mon Sep 17 00:00:00 2001 From: James McKinney <26463+jpmckinney@users.noreply.github.com> Date: Fri, 23 Aug 2024 02:58:48 -0400 Subject: [PATCH 16/20] chore: Satisfy mypy. build: Upgrade moto --- app/auth.py | 2 +- app/main.py | 4 ++-- app/routers/users.py | 4 +--- requirements_dev.txt | 2 +- tests/__init__.py | 16 ++++++++-------- 5 files changed, 13 insertions(+), 15 deletions(-) diff --git a/app/auth.py b/app/auth.py index 3aedc306..859b6454 100644 --- a/app/auth.py +++ b/app/auth.py @@ -68,7 +68,7 @@ def verify_jwk_token(self, jwt_credentials: JWTAuthorizationCredentials) -> bool return alg_obj.verify(msg, prepared_key, sig) - async def __call__(self, request: Request) -> JWTAuthorizationCredentials | None: + async def __call__(self, request: Request) -> JWTAuthorizationCredentials: """ Authenticate and verify the provided JWT token in the request. diff --git a/app/main.py b/app/main.py index 6bb821ec..195b5c88 100644 --- a/app/main.py +++ b/app/main.py @@ -1,4 +1,4 @@ -from fastapi import FastAPI +from fastapi import FastAPI, Request from fastapi.middleware.cors import CORSMiddleware from fastapi.responses import JSONResponse @@ -26,5 +26,5 @@ @app.exception_handler(500) -async def http_exception_handler(request, exc): +async def http_exception_handler(request: Request, exc: Exception) -> JSONResponse: return JSONResponse({"detail": _("An unexpected error occurred")}, status_code=500) diff --git a/app/routers/users.py b/app/routers/users.py index 0573d603..3c869e9f 100644 --- a/app/routers/users.py +++ b/app/routers/users.py @@ -87,9 +87,7 @@ def change_password( client.cognito.admin_update_user_attributes( UserPoolId=app_settings.cognito_pool_id, Username=payload.username, - UserAttributes=[ - {"Name": "email_verified", "Value": "true"}, - ], + UserAttributes=[{"Name": "email_verified", "Value": "true"}], ) if "ChallengeName" in response and response["ChallengeName"] == "MFA_SETUP": diff --git a/requirements_dev.txt b/requirements_dev.txt index a570cb7e..3c31456e 100644 --- a/requirements_dev.txt +++ b/requirements_dev.txt @@ -160,7 +160,7 @@ mdurl==0.1.2 # markdown-it-py minify-html==0.15.0 # via -r requirements.txt -moto[cognitoidp]==5.0.6 +moto[cognitoidp]==5.0.13 # via -r requirements_dev.in mypy==1.11.1 # via -r requirements_dev.in diff --git a/tests/__init__.py b/tests/__init__.py index 870f2d25..948c3ca5 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -44,21 +44,23 @@ def inner() -> Generator[Session, None, None]: def create_user(session, aws_client, *, email, **kwargs): - # Like create_user(). - user = models.User.create(session, email=email, **kwargs) + # create_user() response = aws_client.cognito.admin_create_user( UserPoolId=app_settings.cognito_pool_id, Username=email, - TemporaryPassword=aws_client.generate_password(), + TemporaryPassword="initial-autogenerated-password", MessageAction="SUPPRESS", UserAttributes=[{"Name": "email", "Value": email}], ) + user = models.User.create(session, email=email, **kwargs) user.external_id = response["User"]["Username"] session.commit() - # Like change_password(). + # User is sent a link to Credere frontend's /create-password path. + + # change_password() response = aws_client.initiate_auth(email, "initial-autogenerated-password") - assert response["ChallengeName"] == "NEW_PASSWORD_REQUIRED" + assert response.get("ChallengeName") == "NEW_PASSWORD_REQUIRED", response response = aws_client.respond_to_auth_challenge( username=email, session=response["Session"], @@ -68,9 +70,7 @@ def create_user(session, aws_client, *, email, **kwargs): aws_client.cognito.admin_update_user_attributes( UserPoolId=app_settings.cognito_pool_id, Username=email, - UserAttributes=[ - {"Name": "email_verified", "Value": "true"}, - ], + UserAttributes=[{"Name": "email_verified", "Value": "true"}], ) return {"Authorization": "Bearer " + response["AuthenticationResult"]["AccessToken"]} From 36a24ef859008798606463e486c09cf662b117c4 Mon Sep 17 00:00:00 2001 From: James McKinney <26463+jpmckinney@users.noreply.github.com> Date: Fri, 23 Aug 2024 03:26:00 -0400 Subject: [PATCH 17/20] fix: Handle EnableSoftwareTokenMFAException and CodeMismatchException errors. Remove ExpiredTemporaryPasswordException. #390 --- app/routers/users.py | 95 ++++++++++++++++++------------------- docs/contributing/index.rst | 2 + 2 files changed, 48 insertions(+), 49 deletions(-) diff --git a/app/routers/users.py b/app/routers/users.py index 3c869e9f..7ebc0726 100644 --- a/app/routers/users.py +++ b/app/routers/users.py @@ -1,4 +1,3 @@ -from botocore.exceptions import ClientError from fastapi import APIRouter, Depends, HTTPException, Query, Request, status from fastapi.encoders import jsonable_encoder from sqlalchemy.exc import IntegrityError @@ -71,42 +70,34 @@ def change_password( This endpoint allows users to change their password. It initiates the password change process and handles different scenarios such as new password requirement, MFA setup, and error handling. """ - try: - # This endpoint is only called for new users, to replace the generated password. - response = client.initiate_auth(payload.username, payload.temp_password) - if response["ChallengeName"] == "NEW_PASSWORD_REQUIRED": - response = client.respond_to_auth_challenge( - username=payload.username, - session=response["Session"], - challenge_name="NEW_PASSWORD_REQUIRED", - new_password=payload.password, - ) - - # Verify the user's email. - # https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/cognito-idp/client/admin_update_user_attributes.html - client.cognito.admin_update_user_attributes( - UserPoolId=app_settings.cognito_pool_id, - Username=payload.username, - UserAttributes=[{"Name": "email_verified", "Value": "true"}], + # This endpoint is only called for new users, to replace the generated password. + response = client.initiate_auth(payload.username, payload.temp_password) + if response["ChallengeName"] == "NEW_PASSWORD_REQUIRED": + response = client.respond_to_auth_challenge( + username=payload.username, + session=response["Session"], + challenge_name="NEW_PASSWORD_REQUIRED", + new_password=payload.password, ) - if "ChallengeName" in response and response["ChallengeName"] == "MFA_SETUP": - # https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/cognito-idp/client/associate_software_token.html - associate_response = client.cognito.associate_software_token(Session=response["Session"]) + # Verify the user's email. + # https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/cognito-idp/client/admin_update_user_attributes.html + client.cognito.admin_update_user_attributes( + UserPoolId=app_settings.cognito_pool_id, + Username=payload.username, + UserAttributes=[{"Name": "email_verified", "Value": "true"}], + ) + + if "ChallengeName" in response and response["ChallengeName"] == "MFA_SETUP": + # https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/cognito-idp/client/associate_software_token.html + associate_response = client.cognito.associate_software_token(Session=response["Session"]) - return serializers.ChangePasswordResponse( - detail=_("Password changed with MFA setup required"), - secret_code=associate_response["SecretCode"], - session=associate_response["Session"], - username=payload.username, - ) - except ClientError as e: - if e.response["Error"]["Code"] == "ExpiredTemporaryPasswordException": - raise HTTPException( - status_code=status.HTTP_403_FORBIDDEN, - detail=_("Temporal password is expired, please request a new one"), - ) - raise + return serializers.ChangePasswordResponse( + detail=_("Password changed with MFA setup required"), + secret_code=associate_response["SecretCode"], + session=associate_response["Session"], + username=payload.username, + ) return serializers.ResponseBase(detail=_("Password changed")) @@ -129,13 +120,16 @@ def setup_mfa( client.cognito.verify_software_token( AccessToken=payload.secret, Session=payload.session, UserCode=payload.temp_password ) - except ClientError as e: - if e.response["Error"]["Code"] == "NotAuthorizedException": - raise HTTPException( - status_code=status.HTTP_403_FORBIDDEN, - detail=_("Invalid session for the user, session is expired"), - ) - raise + except client.cognito.exceptions.NotAuthorizedException: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail=_("Invalid session for the user, session is expired"), + ) + except client.cognito.exceptions.EnableSoftwareTokenMFAException: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail=_("Invalid MFA code"), + ) return serializers.ResponseBase(detail=_("MFA configured successfully")) @@ -178,14 +172,17 @@ def login( ) else: raise NotImplementedError # Missing MFA challenge - except ClientError as e: - # https://boto3.amazonaws.com/v1/documentation/api/latest/guide/error-handling.html#parsing-error-responses-and-catching-exceptions-from-aws-services - if e.response["Error"]["Code"] == "ExpiredTemporaryPasswordException": - raise HTTPException( - status_code=status.HTTP_403_FORBIDDEN, - detail=_("Temporal password is expired, please request a new one"), - ) - raise + # The user failed to sign in ("Incorrect username or password", etc.). + except client.cognito.exceptions.NotAuthorizedException: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail=_("Invalid username or password"), + ) + except client.cognito.exceptions.CodeMismatchException: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail=_("Invalid MFA code"), + ) return serializers.LoginResponse( user=user, diff --git a/docs/contributing/index.rst b/docs/contributing/index.rst index 0fd2b076..d4766a0b 100644 --- a/docs/contributing/index.rst +++ b/docs/contributing/index.rst @@ -170,6 +170,8 @@ Make changes Read the next pages in this section to learn about style guides, and the :doc:`../api/index` about helper methods and application logic. Read the `OCP Software Development Handbook `__: in particular, `Python `__. +.. seealso:: `Parsing error responses and catching exceptions from AWS services `__ + Update requirements ~~~~~~~~~~~~~~~~~~~ From ade9c5bf25537031a43a2fdc1f84d728233d3e09 Mon Sep 17 00:00:00 2001 From: James McKinney <26463+jpmckinney@users.noreply.github.com> Date: Fri, 23 Aug 2024 03:33:19 -0400 Subject: [PATCH 18/20] chore: Fix CI, lint and i18n tests --- locale/es/LC_MESSAGES/messages.po | 29 ++++++++++++++++------------- requirements.in | 1 - requirements.txt | 1 - requirements_dev.in | 1 + requirements_dev.txt | 1 + tests/commands/test_commands.py | 8 ++++---- tests/routers/test_users.py | 4 ++-- 7 files changed, 24 insertions(+), 21 deletions(-) diff --git a/locale/es/LC_MESSAGES/messages.po b/locale/es/LC_MESSAGES/messages.po index 67236bdf..dea3a14b 100644 --- a/locale/es/LC_MESSAGES/messages.po +++ b/locale/es/LC_MESSAGES/messages.po @@ -8,7 +8,7 @@ msgid "" msgstr "" "Project-Id-Version: PROJECT VERSION\n" "Report-Msgid-Bugs-To: EMAIL@ADDRESS\n" -"POT-Creation-Date: 2024-08-23 00:49-0400\n" +"POT-Creation-Date: 2024-08-23 03:32-0400\n" "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n" "Last-Translator: FULL NAME \n" "Language: es\n" @@ -446,39 +446,39 @@ msgstr "Ya existe un entidad financiera con ese nombre" msgid "Credit product not found" msgstr "Producto crediticio no encontrado" -#: app/routers/users.py:57 app/routers/users.py:347 +#: app/routers/users.py:56 app/routers/users.py:342 msgid "User with that email already exists" msgstr "Ya existe un usuario con ese correo" -#: app/routers/users.py:100 +#: app/routers/users.py:96 msgid "Password changed with MFA setup required" msgstr "Cambio de contraseña con configuración de MFA requerido" -#: app/routers/users.py:109 app/routers/users.py:188 -msgid "Temporal password is expired, please request a new one" -msgstr "La contraseña temporal ha expirado, por favor solicite una nueva" - -#: app/routers/users.py:113 +#: app/routers/users.py:102 msgid "Password changed" msgstr "Contraseña cambiada" -#: app/routers/users.py:138 +#: app/routers/users.py:126 msgid "Invalid session for the user, session is expired" msgstr "Sesión inválida para el usuario, la sesión ha expirado" -#: app/routers/users.py:142 +#: app/routers/users.py:131 app/routers/users.py:184 +msgid "Invalid MFA code" +msgstr "Código MFA no válido" + +#: app/routers/users.py:134 msgid "MFA configured successfully" msgstr "El MFA fue configurado correctamente" -#: app/routers/users.py:168 +#: app/routers/users.py:160 app/routers/users.py:179 msgid "Invalid username or password" msgstr "Nombre de usuario o contraseña no válidos" -#: app/routers/users.py:220 +#: app/routers/users.py:215 msgid "User logged out successfully" msgstr "Usuario deslogueado correctamente" -#: app/routers/users.py:266 +#: app/routers/users.py:261 msgid "An email with a reset link was sent to end user" msgstr "Se envió un correo con el enlace de reseteo de contraseña." @@ -649,3 +649,6 @@ msgstr "Correo institucional" msgid "MSME Documents" msgstr "Documentos de la empresa" +#~ msgid "Temporal password is expired, please request a new one" +#~ msgstr "La contraseña temporal ha expirado, por favor solicite una nueva" + diff --git a/requirements.in b/requirements.in index e097a013..00d5f862 100644 --- a/requirements.in +++ b/requirements.in @@ -1,7 +1,6 @@ alembic babel boto3 -botocore click email_validator fastapi diff --git a/requirements.txt b/requirements.txt index 49ae41f6..45948798 100644 --- a/requirements.txt +++ b/requirements.txt @@ -18,7 +18,6 @@ boto3==1.26.136 # via -r requirements.in botocore==1.29.136 # via - # -r requirements.in # boto3 # s3transfer certifi==2024.7.4 diff --git a/requirements_dev.in b/requirements_dev.in index 1b71852d..30ec8a7a 100644 --- a/requirements_dev.in +++ b/requirements_dev.in @@ -1,5 +1,6 @@ -r requirements.txt black +botocore boto3-stubs coveralls flake8 diff --git a/requirements_dev.txt b/requirements_dev.txt index 3c31456e..e1bf1414 100644 --- a/requirements_dev.txt +++ b/requirements_dev.txt @@ -32,6 +32,7 @@ boto3-stubs==1.29.7 botocore==1.29.136 # via # -r requirements.txt + # -r requirements_dev.in # boto3 # moto # s3transfer diff --git a/tests/commands/test_commands.py b/tests/commands/test_commands.py index 41b88ad3..f0b7d5d0 100644 --- a/tests/commands/test_commands.py +++ b/tests/commands/test_commands.py @@ -14,9 +14,9 @@ ("seconds", "call_count"), [ (0, 0), - (1, 1), + (2, 1), (app_settings.reminder_days_before_expiration * 86_400, 1), - (app_settings.reminder_days_before_expiration * 86_400 + 1, 0), + (app_settings.reminder_days_before_expiration * 86_400 + 2, 0), ], ) def test_send_reminders_intro(session, mock_send_templated_email, pending_application, seconds, call_count): @@ -46,9 +46,9 @@ def test_send_reminders_intro(session, mock_send_templated_email, pending_applic ("seconds", "call_count"), [ (0, 0), - (1, 1), + (2, 1), (app_settings.reminder_days_before_lapsed * 86_400, 1), - (app_settings.reminder_days_before_lapsed * 86_400 + 1, 0), + (app_settings.reminder_days_before_lapsed * 86_400 + 2, 0), ], ) def test_send_reminders_submit(session, mock_send_templated_email, accepted_application, seconds, call_count): diff --git a/tests/routers/test_users.py b/tests/routers/test_users.py index 5cf91a6a..0fd376f0 100644 --- a/tests/routers/test_users.py +++ b/tests/routers/test_users.py @@ -68,14 +68,14 @@ def test_login_invalid_username(client): response = client.post("/users/login", json={"username": "nonexistent"}) assert response.status_code == status.HTTP_403_FORBIDDEN - assert response.json() == {"detail": "Invalid username or password"} + assert response.json() == {"detail": _("Invalid username or password")} def test_logout(client, admin_header): response = client.get("/users/logout", headers=admin_header) assert response.status_code == status.HTTP_200_OK - assert response.json() == {"detail": "User logged out successfully"} + assert response.json() == {"detail": _("User logged out successfully")} def test_logout_invalid_authorization_header_no_period(client): From eda2ad01ad1b6cf6dfd90098f2043501c9abe54b Mon Sep 17 00:00:00 2001 From: James McKinney <26463+jpmckinney@users.noreply.github.com> Date: Fri, 23 Aug 2024 13:56:13 -0400 Subject: [PATCH 19/20] i18n: Fix pybabel command to run only against app (like it had with babel.cfg) --- .github/workflows/i18n.yml | 2 +- docs/contributing/index.rst | 2 +- locale/es/LC_MESSAGES/messages.po | 60 +++++++++---------------------- 3 files changed, 19 insertions(+), 45 deletions(-) diff --git a/.github/workflows/i18n.yml b/.github/workflows/i18n.yml index 100a71de..6948513d 100644 --- a/.github/workflows/i18n.yml +++ b/.github/workflows/i18n.yml @@ -20,7 +20,7 @@ jobs: - run: pip install -r requirements.txt . - name: Update catalogs run: | - pybabel extract -k '_ i' -o messages.pot . + pybabel extract -k '_ i' -o messages.pot app pybabel update -N -i messages.pot -d locale - name: Count incomplete translations shell: bash diff --git a/docs/contributing/index.rst b/docs/contributing/index.rst index 35daa5d7..76f13a98 100644 --- a/docs/contributing/index.rst +++ b/docs/contributing/index.rst @@ -177,7 +177,7 @@ Update translations .. code-block:: bash - pybabel extract -k '_ i' -o messages.pot . + pybabel extract -k '_ i' -o messages.pot app pybabel update -N -i messages.pot -d locale #. Compile the message catalogs (in development): diff --git a/locale/es/LC_MESSAGES/messages.po b/locale/es/LC_MESSAGES/messages.po index aa29e8fb..4bee4b6a 100644 --- a/locale/es/LC_MESSAGES/messages.po +++ b/locale/es/LC_MESSAGES/messages.po @@ -8,7 +8,7 @@ msgid "" msgstr "" "Project-Id-Version: PROJECT VERSION\n" "Report-Msgid-Bugs-To: EMAIL@ADDRESS\n" -"POT-Creation-Date: 2024-08-23 11:47-0400\n" +"POT-Creation-Date: 2024-08-23 13:55-0400\n" "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n" "Last-Translator: FULL NAME \n" "Language: es\n" @@ -43,45 +43,28 @@ msgstr "Falta el nombre de usuario" msgid "User not found" msgstr "Usuario no encontrado" -#: app/dependencies.py:70 tests/routers/test_lenders.py:48 -#: tests/routers/test_lenders.py:102 tests/routers/test_lenders.py:141 -#: tests/routers/test_lenders.py:174 tests/routers/test_users.py:35 -#: tests/routers/test_users.py:54 +#: app/dependencies.py:70 msgid "Insufficient permissions" msgstr "Permisos insuficientes" -#: app/dependencies.py:98 tests/routers/test_applications.py:104 -#: tests/routers/test_applications.py:109 -#: tests/routers/test_applications.py:162 -#: tests/routers/test_applications.py:167 -#: tests/routers/test_applications.py:172 -#: tests/routers/test_applications.py:182 -#: tests/routers/test_applications.py:187 +#: app/dependencies.py:98 msgid "User is not authorized" msgstr "Usuario no autorizado" -#: app/dependencies.py:106 tests/routers/guest/test_applications.py:69 +#: app/dependencies.py:106 msgid "Application expired" msgstr "La aplicación ha expirado" -#: app/dependencies.py:113 tests/routers/guest/test_applications.py:22 -#: tests/routers/guest/test_applications.py:38 -#: tests/routers/test_applications.py:24 tests/routers/test_applications.py:67 -#: tests/routers/test_applications.py:157 +#: app/dependencies.py:113 #, python-format msgid "Application status should not be %(status)s" msgstr "El estado de la aplicación no debe ser %(status)s" #: app/dependencies.py:126 app/dependencies.py:166 -#: tests/routers/test_applications.py:192 -#: tests/routers/test_applications.py:197 -#: tests/routers/test_applications.py:333 -#: tests/routers/test_applications.py:354 -#: tests/routers/test_applications.py:358 msgid "Application not found" msgstr "La aplicación no ha sido encontrada" -#: app/dependencies.py:172 tests/routers/test_applications.py:350 +#: app/dependencies.py:172 msgid "Application lapsed" msgstr "La aplicación ha caducado" @@ -197,21 +180,19 @@ msgstr "Cámara de comercio" msgid "THREE_LAST_BANK_STATEMENT" msgstr "Últimos tres extractos bancarios" -#: app/models.py:163 tests/routers/guest/test_applications.py:38 -#: tests/routers/test_applications.py:24 tests/test_i18n.py:11 -#: tests/test_i18n.py:16 +#: app/models.py:163 msgid "PENDING" msgstr "Pendiente" -#: app/models.py:167 tests/routers/guest/test_applications.py:22 +#: app/models.py:167 msgid "DECLINED" msgstr "Declinado" -#: app/models.py:171 tests/routers/test_applications.py:67 +#: app/models.py:171 msgid "ACCEPTED" msgstr "Aceptado" -#: app/models.py:175 tests/routers/test_applications.py:157 +#: app/models.py:175 msgid "SUBMITTED" msgstr "Enviado" @@ -350,14 +331,12 @@ msgstr "" msgid "actividades_organizaciones_extraterritoriales" msgstr "Actividades de organizaciones y entidades extraterritoriales" -#: app/util.py:55 tests/routers/test_applications.py:245 -#: tests/routers/test_lenders.py:62 tests/routers/test_lenders.py:77 -#: tests/routers/test_lenders.py:156 tests/routers/test_users.py:27 +#: app/util.py:55 #, python-format msgid "%(model_name)s not found" msgstr "%(model_name)s no encontrado" -#: app/util.py:110 tests/routers/test_applications.py:221 +#: app/util.py:110 msgid "Format not allowed. It must be a PNG, JPEG, or PDF file" msgstr "Formato no permitido. El formato debe ser un PNG, JPEG or archivo PDF" @@ -365,11 +344,11 @@ msgstr "Formato no permitido. El formato debe ser un PNG, JPEG or archivo PDF" msgid "File is too large" msgstr "El archivo es muy grande" -#: app/routers/applications.py:171 tests/routers/test_applications.py:250 +#: app/routers/applications.py:171 msgid "Some borrower data field are not verified" msgstr "Algunos campos de datos de la empresa no fueron verificados" -#: app/routers/applications.py:182 tests/routers/test_applications.py:268 +#: app/routers/applications.py:182 msgid "Some documents are not verified" msgstr "Algunos documentos no fueron verificados" @@ -414,7 +393,6 @@ msgid "Stage" msgstr "Etapa" #: app/routers/lenders.py:49 app/routers/lenders.py:126 -#: tests/routers/test_lenders.py:98 msgid "Lender with that name already exists" msgstr "Ya existe un entidad financiera con ese nombre" @@ -424,7 +402,6 @@ msgid "Credit product not found" msgstr "Producto crediticio no encontrado" #: app/routers/users.py:56 app/routers/users.py:342 -#: tests/routers/test_users.py:64 msgid "User with that email already exists" msgstr "Ya existe un usuario con ese correo" @@ -449,13 +426,10 @@ msgid "MFA configured successfully" msgstr "El MFA fue configurado correctamente" #: app/routers/users.py:160 app/routers/users.py:179 -#: tests/routers/test_users.py:71 msgid "Invalid username or password" msgstr "Nombre de usuario o contraseña no válidos" -#: app/routers/users.py:215 tests/routers/test_users.py:78 -#: tests/routers/test_users.py:85 tests/routers/test_users.py:92 -#: tests/routers/test_users.py:99 +#: app/routers/users.py:215 msgid "User logged out successfully" msgstr "Usuario deslogueado correctamente" @@ -484,11 +458,11 @@ msgstr "No se pueden subir documentos en esta etapa" msgid "A new application has already been created from this one" msgstr "Una nueva aplicación ya ha sido creada desde esta" -#: app/routers/guest/emails.py:34 tests/routers/test_applications.py:117 +#: app/routers/guest/emails.py:34 msgid "New email is not valid" msgstr "El nuevo correo no es válido" -#: app/routers/guest/emails.py:81 tests/routers/test_applications.py:143 +#: app/routers/guest/emails.py:81 msgid "Application is not pending an email confirmation" msgstr "La aplicación no tiene la confirmación de correo pendiente" From e3f80016db05beb50a0c82574debc6fb5fee1714 Mon Sep 17 00:00:00 2001 From: James McKinney <26463+jpmckinney@users.noreply.github.com> Date: Fri, 23 Aug 2024 14:00:02 -0400 Subject: [PATCH 20/20] test: Add 1-2 seconds to offsets to avoid timing issues --- tests/commands/test_commands.py | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/tests/commands/test_commands.py b/tests/commands/test_commands.py index f0b7d5d0..6d3a79dd 100644 --- a/tests/commands/test_commands.py +++ b/tests/commands/test_commands.py @@ -8,15 +8,18 @@ from tests import assert_change, assert_success runner = CliRunner() +# Do 1-2 seconds off the minimum offset, to avoid test failures due to timing issues. +negative_offset = -2 # min 0 +positive_offset = 2 # min 1 @pytest.mark.parametrize( ("seconds", "call_count"), [ - (0, 0), - (2, 1), - (app_settings.reminder_days_before_expiration * 86_400, 1), - (app_settings.reminder_days_before_expiration * 86_400 + 2, 0), + (negative_offset, 0), + (positive_offset, 1), + (app_settings.reminder_days_before_expiration * 86_400 + negative_offset, 1), + (app_settings.reminder_days_before_expiration * 86_400 + positive_offset, 0), ], ) def test_send_reminders_intro(session, mock_send_templated_email, pending_application, seconds, call_count): @@ -45,10 +48,10 @@ def test_send_reminders_intro(session, mock_send_templated_email, pending_applic @pytest.mark.parametrize( ("seconds", "call_count"), [ - (0, 0), - (2, 1), - (app_settings.reminder_days_before_lapsed * 86_400, 1), - (app_settings.reminder_days_before_lapsed * 86_400 + 2, 0), + (negative_offset, 0), + (positive_offset, 1), + (app_settings.reminder_days_before_lapsed * 86_400 + negative_offset, 1), + (app_settings.reminder_days_before_lapsed * 86_400 + positive_offset, 0), ], ) def test_send_reminders_submit(session, mock_send_templated_email, accepted_application, seconds, call_count): @@ -78,7 +81,7 @@ def test_send_reminders_submit(session, mock_send_templated_email, accepted_appl ) -@pytest.mark.parametrize(("seconds", "lapsed"), [(0, True), (1, False)]) +@pytest.mark.parametrize(("seconds", "lapsed"), [(negative_offset, True), (positive_offset, False)]) def test_set_lapsed_applications(session, pending_application, seconds, lapsed): pending_application.created_at = ( datetime.now(pending_application.tz)