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/app/auth.py b/app/auth.py index 6ed411fc..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. @@ -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 d8069639..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: @@ -57,16 +57,16 @@ 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 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, + status_code=status.HTTP_403_FORBIDDEN, detail=_("Insufficient permissions"), ) return user @@ -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: @@ -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/main.py b/app/main.py index e16edb33..195b5c88 100644 --- a/app/main.py +++ b/app/main.py @@ -1,6 +1,8 @@ -from fastapi import FastAPI +from fastapi import FastAPI, Request 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: Request, exc: Exception) -> JSONResponse: + return JSONResponse({"detail": _("An unexpected error occurred")}, status_code=500) diff --git a/app/models.py b/app/models.py index f740c6c1..6194ea07 100644 --- a/app/models.py +++ b/app/models.py @@ -1015,7 +1015,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/applications.py b/app/routers/applications.py index 10418bb7..f8f0c118 100644 --- a/app/routers/applications.py +++ b/app/routers/applications.py @@ -1,8 +1,6 @@ -import logging 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 @@ -13,8 +11,6 @@ from app.i18n import _ from app.util import SortOrder, get_order_by -logger = logging.getLogger(__name__) - router = APIRouter() @@ -171,7 +167,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"), ) @@ -182,7 +178,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"), ) @@ -458,7 +454,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) @@ -485,7 +481,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 @@ -576,14 +572,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, @@ -612,6 +601,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/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/guest/applications.py b/app/routers/guest/applications.py index 48aec64c..2c2c32bf 100644 --- a/app/routers/guest/applications.py +++ b/app/routers/guest/applications.py @@ -1,8 +1,6 @@ -import logging 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 @@ -12,8 +10,6 @@ from app.db import get_db, rollback_on_error from app.i18n import _ -logger = logging.getLogger(__name__) - router = APIRouter() @@ -31,7 +27,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), @@ -62,7 +58,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) @@ -105,7 +101,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. @@ -144,7 +140,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) @@ -183,7 +179,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) @@ -217,7 +213,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 = ( @@ -256,7 +252,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): @@ -311,19 +307,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"), ) @@ -362,19 +358,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"), ) @@ -451,19 +447,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"), ) @@ -522,13 +518,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"), ) @@ -536,17 +532,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, @@ -593,7 +582,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"), ) @@ -780,8 +769,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. @@ -801,27 +790,21 @@ 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. - 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/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..4ff2b4ce 100644 --- a/app/routers/lenders.py +++ b/app/routers/lenders.py @@ -45,8 +45,8 @@ async def create_lender( return lender except IntegrityError: raise HTTPException( - status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, - detail=_("Lender already exists"), + status_code=status.HTTP_409_CONFLICT, + detail=_("Lender with that name 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,8 +122,8 @@ async def update_lender( return lender except IntegrityError: raise HTTPException( - status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, - detail=_("Lender already exists"), + status_code=status.HTTP_409_CONFLICT, + detail=_("Lender with that name 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/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/app/routers/users.py b/app/routers/users.py index 18a12362..7ebc0726 100644 --- a/app/routers/users.py +++ b/app/routers/users.py @@ -1,19 +1,14 @@ -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 from app.util import SortOrder, get_object_or_404, get_order_by -logger = logging.getLogger(__name__) - router = APIRouter() @@ -57,8 +52,8 @@ async def create_user( return user except (client.cognito.exceptions.UsernameExistsException, IntegrityError): raise HTTPException( - status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, - detail=_("Username already exists"), + status_code=status.HTTP_409_CONFLICT, + detail=_("User with that email already exists"), ) @@ -75,46 +70,33 @@ 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_422_UNPROCESSABLE_ENTITY, - 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"), + 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")) @@ -138,15 +120,15 @@ 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_408_REQUEST_TIMEOUT, - detail=_("Invalid session for the user, session is expired"), - ) + except client.cognito.exceptions.NotAuthorizedException: raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail=_("There was an error trying to setup mfa"), + 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")) @@ -171,7 +153,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) @@ -184,17 +171,17 @@ def login( mfa_code=payload.temp_password, ) else: - raise NotImplementedError - 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_401_UNAUTHORIZED, - detail=_("Temporal password is expired, please request a new one"), - ) + raise NotImplementedError # Missing MFA challenge + # 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_401_UNAUTHORIZED, - detail=e.response["Error"]["Message"], + status_code=status.HTTP_403_FORBIDDEN, + detail=_("Invalid MFA code"), ) return serializers.LoginResponse( @@ -207,32 +194,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")) @@ -267,20 +245,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")) @@ -363,6 +338,6 @@ async def update_user( return user except IntegrityError: raise HTTPException( - status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, - detail=_("User already exists"), + status_code=status.HTTP_409_CONFLICT, + detail=_("User with that email already exists"), ) diff --git a/app/settings.py b/app/settings.py index b65d144c..95c66ae0 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, 413, range(500, 599)], + ), + FastApiIntegration( + failed_request_status_codes=[400, 413, range(500, 599)], + ), + ], ) diff --git a/app/util.py b/app/util.py index 14787cfa..d1341d37 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 @@ -111,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 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 2fb608a0..76f13a98 100644 --- a/docs/contributing/index.rst +++ b/docs/contributing/index.rst @@ -4,6 +4,7 @@ Contributing .. toctree:: :caption: contents + api sqlalchemy .. seealso:: @@ -160,24 +161,9 @@ 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: +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 `__. -- `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". - -.. note:: - - Some endpoints and enums cannot be made to conform to this style guide, without migrating the database or updating the frontend. +.. seealso:: `Parsing error responses and catching exceptions from AWS services `__ Update requirements ~~~~~~~~~~~~~~~~~~~ @@ -191,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): @@ -211,10 +197,6 @@ Update translations Update API ~~~~~~~~~~ -.. seealso:: :doc:`../api/index` - -Use the parameter ``id`` only for application IDs, to avoid accidental errors. - 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/locale/es/LC_MESSAGES/messages.po b/locale/es/LC_MESSAGES/messages.po index fa6bd145..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:37-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" @@ -27,11 +27,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" @@ -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:58 +#: 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" @@ -157,6 +140,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/models.py:130 msgid "INCORPORATION_DOCUMENT" msgstr "Documento de incorporación" @@ -193,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" @@ -346,45 +331,39 @@ msgstr "" msgid "actividades_organizaciones_extraterritoriales" msgstr "Actividades de organizaciones y entidades extraterritoriales" -#: app/util.py:54 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:109 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" -#: app/util.py:115 +#: app/util.py:116 msgid "File is too large" msgstr "El archivo es muy grande" -#: app/routers/applications.py:175 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:186 tests/routers/test_applications.py:268 +#: app/routers/applications.py:182 msgid "Some documents are not verified" msgstr "Algunos documentos no fueron verificados" -#: app/routers/applications.py:332 +#: app/routers/applications.py:328 msgid "Award not found" msgstr "La adjudicación no ha sido encontrada" -#: app/routers/applications.py:377 +#: app/routers/applications.py:373 msgid "Borrower not found" msgstr "La empresa no ha sido encontrada" -#: app/routers/applications.py:386 +#: app/routers/applications.py:382 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" @@ -414,95 +393,76 @@ msgid "Stage" msgstr "Etapa" #: app/routers/lenders.py:49 app/routers/lenders.py:126 -#: tests/routers/test_lenders.py:98 -msgid "Lender already exists" -msgstr "La entidad financiera ya existe" +msgid "Lender with that name already exists" +msgstr "Ya existe un entidad financiera con ese nombre" -#: app/routers/guest/applications.py:378 app/routers/guest/applications.py:467 +#: 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 tests/routers/test_users.py:68 -msgid "Username already exists" -msgstr "El nombre de usuario ya existe" +#: 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:104 +#: 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:113 app/routers/users.py:193 -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 +#: app/routers/users.py:102 msgid "Password changed" msgstr "Contraseña cambiada" -#: app/routers/users.py:145 +#: 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:149 -msgid "There was an error trying to setup mfa" -msgstr "Ocurrió un error tratando de configurar el MFA" +#: 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:152 +#: app/routers/users.py:134 msgid "MFA configured successfully" msgstr "El MFA fue configurado correctamente" -#: app/routers/users.py:237 tests/routers/test_users.py:77 -#: tests/routers/test_users.py:87 +#: 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:215 msgid "User logged out successfully" msgstr "Usuario deslogueado correctamente" -#: app/routers/users.py:286 +#: 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." -#: 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 +#: 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:327 +#: 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:532 +#: app/routers/guest/applications.py:528 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:586 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:793 +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 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" 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 a570cb7e..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 @@ -160,7 +161,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"]} diff --git a/tests/commands/test_commands.py b/tests/commands/test_commands.py index 2c53b07c..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), - (1, 1), - (app_settings.reminder_days_before_expiration * 86_400, 1), - (app_settings.reminder_days_before_expiration * 86_400 + 1, 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): @@ -26,30 +29,29 @@ 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( ("seconds", "call_count"), [ - (0, 0), - (1, 1), - (app_settings.reminder_days_before_lapsed * 86_400, 1), - (app_settings.reminder_days_before_lapsed * 86_400 + 1, 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): @@ -63,35 +65,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"), [(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) + - timedelta(days=app_settings.days_to_change_to_lapsed) + + timedelta(seconds=seconds) ) session.commit() @@ -99,8 +94,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 +119,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 +126,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): 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/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..e43db055 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.json() == {"detail": _("Lender already exists")} + assert response.status_code == status.HTTP_409_CONFLICT + 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_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..0fd376f0 100644 --- a/tests/routers/test_users.py +++ b/tests/routers/test_users.py @@ -1,4 +1,4 @@ -import logging +import uuid from fastapi import status @@ -31,30 +31,26 @@ 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")} 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")} @@ -64,25 +60,40 @@ 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.json() == {"detail": _("Username already exists")} + assert response.status_code == status.HTTP_409_CONFLICT + assert response.json() == {"detail": _("User with that email already exists")} + + +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_invalid_authorization_header(client, caplog): - caplog.set_level(logging.ERROR) +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")} + + +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