From 876b3dde0c8c2749d7cfe8feebecc22822d8b84f Mon Sep 17 00:00:00 2001 From: Aadesh-Baral Date: Tue, 22 Aug 2023 16:34:51 +0545 Subject: [PATCH 01/10] Use newly defined Forbidden exception to handle 403 errors --- backend/api/campaigns/resources.py | 43 +++--- backend/api/comments/resources.py | 29 ++-- backend/api/interests/resources.py | 59 +++----- backend/api/issues/resources.py | 2 + backend/api/licenses/resources.py | 2 + backend/api/notifications/resources.py | 23 +-- backend/api/organisations/campaigns.py | 11 +- backend/api/organisations/resources.py | 36 ++--- backend/api/projects/actions.py | 103 +++++++------ backend/api/projects/campaigns.py | 19 +-- backend/api/projects/resources.py | 109 +++++++------- backend/api/projects/teams.py | 82 ++++++----- backend/api/system/authentication.py | 5 +- backend/api/system/banner.py | 6 +- backend/api/tasks/actions.py | 135 ++++++++---------- backend/api/tasks/resources.py | 10 +- backend/api/teams/actions.py | 48 +++---- backend/api/teams/resources.py | 27 ++-- backend/api/users/actions.py | 4 +- backend/api/users/resources.py | 2 - backend/error_messages.json | 11 ++ backend/services/grid/split_service.py | 14 +- backend/services/mapping_service.py | 34 +++-- backend/services/messaging/chat_service.py | 28 ++-- backend/services/messaging/message_service.py | 7 +- backend/services/organisation_service.py | 6 +- backend/services/project_admin_service.py | 41 +++--- backend/services/project_service.py | 17 ++- backend/services/team_service.py | 20 ++- backend/services/users/user_service.py | 9 +- backend/services/validator_service.py | 26 ++-- 31 files changed, 456 insertions(+), 512 deletions(-) diff --git a/backend/api/campaigns/resources.py b/backend/api/campaigns/resources.py index 466090ce00..08f3965912 100644 --- a/backend/api/campaigns/resources.py +++ b/backend/api/campaigns/resources.py @@ -1,6 +1,7 @@ from flask_restful import Resource, request, current_app from schematics.exceptions import DataError +from backend.exceptions import Forbidden from backend.models.dtos.campaign_dto import CampaignDTO, NewCampaignDTO from backend.services.campaign_service import CampaignService from backend.services.organisation_service import OrganisationService @@ -115,15 +116,11 @@ def patch(self, campaign_id): 500: description: Internal Server Error """ - try: - orgs_dto = OrganisationService.get_organisations_managed_by_user_as_dto( - token_auth.current_user() - ) - if len(orgs_dto.organisations) < 1: - raise ValueError("User not a Org Manager") - except ValueError as e: - error_msg = f"CampaignsRestAPI PATCH: {str(e)}" - return {"Error": error_msg, "SubCode": "UserNotPermitted"}, 403 + orgs_dto = OrganisationService.get_organisations_managed_by_user_as_dto( + token_auth.current_user() + ) + if len(orgs_dto.organisations) < 1: + raise Forbidden(sub_code="USER_NOT_ORG_MANAGER") try: campaign_dto = CampaignDTO(request.get_json()) @@ -179,15 +176,11 @@ def delete(self, campaign_id): 500: description: Internal Server Error """ - try: - orgs_dto = OrganisationService.get_organisations_managed_by_user_as_dto( - token_auth.current_user() - ) - if len(orgs_dto.organisations) < 1: - raise ValueError("User not a Org Manager") - except ValueError as e: - error_msg = f"CampaignsRestAPI DELETE: {str(e)}" - return {"Error": error_msg, "SubCode": "UserNotPermitted"}, 403 + orgs_dto = OrganisationService.get_organisations_managed_by_user_as_dto( + token_auth.current_user() + ) + if len(orgs_dto.organisations) < 1: + raise Forbidden(sub_code="USER_NOT_ORG_MANAGER") campaign = CampaignService.get_campaign(campaign_id) CampaignService.delete_campaign(campaign.id) @@ -268,15 +261,11 @@ def post(self): 500: description: Internal Server Error """ - try: - orgs_dto = OrganisationService.get_organisations_managed_by_user_as_dto( - token_auth.current_user() - ) - if len(orgs_dto.organisations) < 1: - raise ValueError("User not a Org Manager") - except ValueError as e: - error_msg = f"CampaignsAllAPI POST: {str(e)}" - return {"Error": error_msg, "SubCode": "UserNotPermitted"}, 403 + orgs_dto = OrganisationService.get_organisations_managed_by_user_as_dto( + token_auth.current_user() + ) + if len(orgs_dto.organisations) < 1: + raise Forbidden(sub_code="USER_NOT_ORG_MANAGER") try: campaign_dto = NewCampaignDTO(request.get_json()) diff --git a/backend/api/comments/resources.py b/backend/api/comments/resources.py index 0e031360ef..4350558139 100644 --- a/backend/api/comments/resources.py +++ b/backend/api/comments/resources.py @@ -1,6 +1,7 @@ from flask_restful import Resource, request, current_app from schematics.exceptions import DataError +from backend.exceptions import Forbidden from backend.models.dtos.message_dto import ChatMessageDTO from backend.models.dtos.mapping_dto import TaskCommentDTO from backend.services.messaging.chat_service import ChatService @@ -53,7 +54,7 @@ def post(self, project_id): """ authenticated_user_id = token_auth.current_user() if UserService.is_user_blocked(authenticated_user_id): - return {"Error": "User is on read only mode", "SubCode": "ReadOnly"}, 403 + raise Forbidden(sub_code="USER_BLOCKED") try: chat_dto = ChatMessageDTO(request.get_json()) @@ -67,13 +68,10 @@ def post(self, project_id): "SubCode": "InvalidData", }, 400 - try: - project_messages = ChatService.post_message( - chat_dto, project_id, authenticated_user_id - ) - return project_messages.to_primitive(), 201 - except ValueError as e: - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 + project_messages = ChatService.post_message( + chat_dto, project_id, authenticated_user_id + ) + return project_messages.to_primitive(), 201 def get(self, project_id): """ @@ -155,13 +153,10 @@ def delete(self, project_id, comment_id): description: Internal Server Error """ authenticated_user_id = token_auth.current_user() - try: - ChatService.delete_project_chat_by_id( - project_id, comment_id, authenticated_user_id - ) - return {"Success": "Comment deleted"}, 200 - except ValueError as e: - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 + ChatService.delete_project_chat_by_id( + project_id, comment_id, authenticated_user_id + ) + return {"Success": "Comment deleted"}, 200 class CommentsTasksRestAPI(Resource): @@ -222,7 +217,7 @@ def post(self, project_id, task_id): """ authenticated_user_id = token_auth.current_user() if UserService.is_user_blocked(authenticated_user_id): - return {"Error": "User is on read only mode", "SubCode": "ReadOnly"}, 403 + raise Forbidden(sub_code="USER_BLOCKED") try: task_comment = TaskCommentDTO(request.get_json()) @@ -237,7 +232,7 @@ def post(self, project_id, task_id): try: task = MappingService.add_task_comment(task_comment) return task.to_primitive(), 201 - except MappingServiceError: + except MappingServiceError: # FLAGGED: UNREACHABLE CODE return {"Error": "Task update failed"}, 403 def get(self, project_id, task_id): diff --git a/backend/api/interests/resources.py b/backend/api/interests/resources.py index d1c0a0e1c6..dbe39ce238 100644 --- a/backend/api/interests/resources.py +++ b/backend/api/interests/resources.py @@ -1,6 +1,7 @@ from flask_restful import Resource, current_app, request from schematics.exceptions import DataError +from backend.exceptions import Forbidden from backend.models.dtos.interests_dto import InterestDTO from backend.services.interests_service import InterestService from backend.services.organisation_service import OrganisationService @@ -9,6 +10,7 @@ from sqlalchemy.exc import IntegrityError INTEREST_NOT_FOUND = "Interest Not Found" +# FLAGGED FOR PERMISSIONS REVIEW class InterestsAllAPI(Resource): @@ -49,15 +51,11 @@ def post(self): 500: description: Internal Server Error """ - try: - orgs_dto = OrganisationService.get_organisations_managed_by_user_as_dto( - token_auth.current_user() - ) - if len(orgs_dto.organisations) < 1: - raise ValueError("User not a Org Manager") - except ValueError as e: - error_msg = f"InterestsAllAPI POST: {str(e)}" - return {"Error": error_msg, "SubCode": "UserNotPermitted"}, 403 + orgs_dto = OrganisationService.get_organisations_managed_by_user_as_dto( + token_auth.current_user() + ) + if len(orgs_dto.organisations) < 1: + raise Forbidden(sub_code="USER_NOT_ORG_MANAGER") try: interest_dto = InterestDTO(request.get_json()) @@ -133,15 +131,12 @@ def get(self, interest_id): 500: description: Internal Server Error """ - try: - orgs_dto = OrganisationService.get_organisations_managed_by_user_as_dto( - token_auth.current_user() - ) - if len(orgs_dto.organisations) < 1: - raise ValueError("User not a Org Manager") - except ValueError as e: - error_msg = f"InterestsRestAPI GET: {str(e)}" - return {"Error": error_msg, "SubCode": "UserNotPermitted"}, 403 + # FLAGGED + orgs_dto = OrganisationService.get_organisations_managed_by_user_as_dto( + token_auth.current_user() + ) + if len(orgs_dto.organisations) < 1: + raise Forbidden(sub_code="USER_NOT_ORG_MANAGER") interest = InterestService.get(interest_id) return interest.to_primitive(), 200 @@ -191,15 +186,11 @@ def patch(self, interest_id): 500: description: Internal Server Error """ - try: - orgs_dto = OrganisationService.get_organisations_managed_by_user_as_dto( - token_auth.current_user() - ) - if len(orgs_dto.organisations) < 1: - raise ValueError("User not a Org Manager") - except ValueError as e: - error_msg = f"InterestsRestAPI PATCH: {str(e)}" - return {"Error": error_msg, "SubCode": "UserNotPermitted"}, 403 + orgs_dto = OrganisationService.get_organisations_managed_by_user_as_dto( + token_auth.current_user() + ) + if len(orgs_dto.organisations) < 1: + raise Forbidden(sub_code="USER_NOT_ORG_MANAGER") try: interest_dto = InterestDTO(request.get_json()) @@ -245,15 +236,11 @@ def delete(self, interest_id): 500: description: Internal Server Error """ - try: - orgs_dto = OrganisationService.get_organisations_managed_by_user_as_dto( - token_auth.current_user() - ) - if len(orgs_dto.organisations) < 1: - raise ValueError("User not a Org Manager") - except ValueError as e: - error_msg = f"InterestsRestAPI DELETE: {str(e)}" - return {"Error": error_msg, "SubCode": "UserNotPermitted"}, 403 + orgs_dto = OrganisationService.get_organisations_managed_by_user_as_dto( + token_auth.current_user() + ) + if len(orgs_dto.organisations) < 1: + raise Forbidden(sub_code="USER_NOT_ORG_MANAGER") InterestService.delete(interest_id) return {"Success": "Interest deleted"}, 200 diff --git a/backend/api/issues/resources.py b/backend/api/issues/resources.py index 3a60deadf6..ac6939b906 100644 --- a/backend/api/issues/resources.py +++ b/backend/api/issues/resources.py @@ -7,6 +7,8 @@ ISSUE_NOT_FOUND = "Mapping-issue category not found" +# FLAGGED FOR DELETION + class IssuesRestAPI(Resource): def get(self, category_id): diff --git a/backend/api/licenses/resources.py b/backend/api/licenses/resources.py index 4b3e78066d..e1dcb6df30 100644 --- a/backend/api/licenses/resources.py +++ b/backend/api/licenses/resources.py @@ -5,6 +5,8 @@ from backend.services.license_service import LicenseService from backend.services.users.authentication_service import token_auth, tm +# FLAGGED FOR PERMISSIONS REVIEW + class LicensesRestAPI(Resource): @tm.pm_only() diff --git a/backend/api/notifications/resources.py b/backend/api/notifications/resources.py index 6575fad152..6618aeb749 100644 --- a/backend/api/notifications/resources.py +++ b/backend/api/notifications/resources.py @@ -1,8 +1,5 @@ from flask_restful import Resource, request -from backend.services.messaging.message_service import ( - MessageService, - MessageServiceError, -) +from backend.services.messaging.message_service import MessageService from backend.services.notification_service import NotificationService from backend.services.users.authentication_service import token_auth, tm @@ -41,13 +38,10 @@ def get(self, message_id): 500: description: Internal Server Error """ - try: - user_message = MessageService.get_message_as_dto( - message_id, token_auth.current_user() - ) - return user_message.to_primitive(), 200 - except MessageServiceError as e: - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 + user_message = MessageService.get_message_as_dto( + message_id, token_auth.current_user() + ) + return user_message.to_primitive(), 200 @tm.pm_only(False) @token_auth.login_required @@ -82,11 +76,8 @@ def delete(self, message_id): 500: description: Internal Server Error """ - try: - MessageService.delete_message(message_id, token_auth.current_user()) - return {"Success": "Message deleted"}, 200 - except MessageServiceError as e: - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 + MessageService.delete_message(message_id, token_auth.current_user()) + return {"Success": "Message deleted"}, 200 class NotificationsAllAPI(Resource): diff --git a/backend/api/organisations/campaigns.py b/backend/api/organisations/campaigns.py index 9abaf40440..5a83007fc6 100644 --- a/backend/api/organisations/campaigns.py +++ b/backend/api/organisations/campaigns.py @@ -1,5 +1,6 @@ from flask_restful import Resource +from backend.exceptions import Forbidden from backend.services.campaign_service import CampaignService from backend.services.organisation_service import OrganisationService from backend.services.users.authentication_service import token_auth @@ -63,10 +64,7 @@ def post(self, organisation_id, campaign_id): ) return {"Success": message}, 200 else: - return { - "Error": "User is not a manager of the organisation", - "SubCode": "UserNotPermitted", - }, 403 + raise Forbidden(sub_code="USER_NOT_ORG_MANAGER") def get(self, organisation_id): """ @@ -149,7 +147,4 @@ def delete(self, organisation_id, campaign_id): 200, ) else: - return { - "Error": "User is not a manager of the organisation", - "SubCode": "UserNotPermitted", - }, 403 + raise Forbidden(sub_code="USER_NOT_ORG_MANAGER") diff --git a/backend/api/organisations/resources.py b/backend/api/organisations/resources.py index 86e2badb0b..4245248e02 100644 --- a/backend/api/organisations/resources.py +++ b/backend/api/organisations/resources.py @@ -2,11 +2,12 @@ from flask_restful import Resource, request, current_app from schematics.exceptions import DataError +from backend.exceptions import Forbidden from backend.models.dtos.organisation_dto import ( NewOrganisationDTO, UpdateOrganisationDTO, ) -from backend.models.postgis.user import User +from backend.models.postgis.user import User, UserRole from backend.services.organisation_service import ( OrganisationService, OrganisationServiceError, @@ -121,11 +122,8 @@ def post(self): description: Internal Server Error """ request_user = User.get_by_id(token_auth.current_user()) - if request_user.role != 1: - return { - "Error": "Only admin users can create organisations.", - "SubCode": "OnlyAdminAccess", - }, 403 + if request_user.role != UserRole.ADMIN.value: + raise Forbidden(sub_code="USER_NOT_ADMIN") try: organisation_dto = NewOrganisationDTO(request.get_json()) @@ -179,14 +177,11 @@ def delete(self, organisation_id): if not OrganisationService.can_user_manage_organisation( organisation_id, token_auth.current_user() ): - return { - "Error": "User is not an admin for the org", - "SubCode": "UserNotOrgAdmin", - }, 403 + raise Forbidden(sub_code="USER_NOT_ORG_MANAGER") try: OrganisationService.delete_organisation(organisation_id) return {"Success": "Organisation deleted"}, 200 - except OrganisationServiceError: + except OrganisationServiceError: # FLAGGED: STATUS CODE return { "Error": "Organisation has some projects", "SubCode": "OrgHasProjects", @@ -303,15 +298,13 @@ def patch(self, organisation_id): if not OrganisationService.can_user_manage_organisation( organisation_id, token_auth.current_user() ): - return { - "Error": "User is not an admin for the org", - "SubCode": "UserNotOrgAdmin", - }, 403 + raise Forbidden(sub_code="USER_NOT_ORG_MANAGER") try: organisation_dto = UpdateOrganisationDTO(request.get_json()) organisation_dto.organisation_id = organisation_id + # Don't update organisation type and subscription_tier if request user is not an admin - if User.get_by_id(token_auth.current_user()).role != 1: + if User.get_by_id(token_auth.current_user()).role != UserRole.ADMIN.value: org = OrganisationService.get_organisation_by_id(organisation_id) organisation_dto.type = OrganisationType(org.type).name organisation_dto.subscription_tier = org.subscription_tier @@ -323,7 +316,7 @@ def patch(self, organisation_id): try: OrganisationService.update_organisation(organisation_dto) return {"Status": "Updated"}, 200 - except OrganisationServiceError as e: + except OrganisationServiceError as e: # FLAGGED: STATUS CODE return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 402 @@ -415,12 +408,9 @@ def get(self): manager_user_id = None if manager_user_id is not None and not authenticated_user_id: - return ( - { - "Error": "Unauthorized - Filter by manager_user_id is not allowed to unauthenticated requests", - "SubCode": "LoginToFilterManager", - }, - 403, + raise Forbidden( # FLAGGED: STATUS CODE 401? + sub_code="USER_NOT_AUTHENTICATED", + message="Filter by manager_user_id is not allowed to unauthenticated requests", ) # Validate abbreviated. diff --git a/backend/api/projects/actions.py b/backend/api/projects/actions.py index 23bcb73e6f..b2e7d30957 100644 --- a/backend/api/projects/actions.py +++ b/backend/api/projects/actions.py @@ -3,13 +3,11 @@ from flask_restful import Resource, request, current_app from schematics.exceptions import DataError +from backend.exceptions import Forbidden from backend.models.dtos.message_dto import MessageDTO from backend.models.dtos.grid_dto import GridDTO from backend.services.project_service import ProjectService -from backend.services.project_admin_service import ( - ProjectAdminService, - ProjectAdminServiceError, -) +from backend.services.project_admin_service import ProjectAdminService from backend.services.grid.grid_service import GridService from backend.services.messaging.message_service import MessageService from backend.services.users.authentication_service import token_auth, tm @@ -65,14 +63,12 @@ def post(self, project_id): username = request.get_json()["username"] except Exception: return {"Error": "Username not provided", "SubCode": "InvalidData"}, 400 - try: - authenticated_user_id = token_auth.current_user() - ProjectAdminService.transfer_project_to( - project_id, authenticated_user_id, username - ) - return {"Success": "Project Transferred"}, 200 - except (ValueError, ProjectAdminServiceError) as e: - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 + + authenticated_user_id = token_auth.current_user() + ProjectAdminService.transfer_project_to( + project_id, authenticated_user_id, username + ) + return {"Success": "Project Transferred"}, 200 class ProjectsActionsMessageContributorsAPI(Resource): @@ -128,7 +124,9 @@ def post(self, project_id): message_dto.from_user_id = authenticated_user_id message_dto.validate() except DataError as e: - current_app.logger.error(f"Error validating request: {str(e)}") + current_app.logger.error( + f"Error validating request: {str(e)}" + ) # FLAGGED FOR LOG LEVEL return { "Error": "Unable to send message to mappers", "SubCode": "InvalidData", @@ -137,10 +135,11 @@ def post(self, project_id): if not ProjectAdminService.is_user_action_permitted_on_project( authenticated_user_id, project_id ): - return { - "Error": "User is not a manager of the project", - "SubCode": "UserPermissionError", - }, 403 + raise Forbidden( + sub_code="USER_NOT_PROJECT_MANAGER", + project_id=project_id, + user_id=authenticated_user_id, + ) threading.Thread( target=MessageService.send_message_to_all_contributors, args=(project_id, message_dto), @@ -183,23 +182,24 @@ def post(self, project_id): 500: description: Internal Server Error """ - try: - authenticated_user_id = token_auth.current_user() - if not ProjectAdminService.is_user_action_permitted_on_project( - authenticated_user_id, project_id - ): - raise ValueError() - except ValueError: - return { - "Error": "User is not a manager of the project", - "SubCode": "UserPermissionError", - }, 403 + authenticated_user_id = token_auth.current_user() + if not ProjectAdminService.is_user_action_permitted_on_project( + authenticated_user_id, project_id + ): + raise Forbidden( + sub_code="USER_NOT_PROJECT_MANAGER", + project_id=project_id, + user_id=authenticated_user_id, + ) try: ProjectService.set_project_as_featured(project_id) return {"Success": True}, 200 except ValueError as e: - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 + return { + "Error": str(e).split("-")[1], + "SubCode": str(e).split("-")[0], + }, 403 # FLAGGED FOR STATUS CODE: 409? class ProjectsActionsUnFeatureAPI(Resource): @@ -237,23 +237,24 @@ def post(self, project_id): 500: description: Internal Server Error """ - try: - authenticated_user_id = token_auth.current_user() - if not ProjectAdminService.is_user_action_permitted_on_project( - authenticated_user_id, project_id - ): - raise ValueError() - except ValueError: - return { - "Error": "User is not a manager of the project", - "SubCode": "UserPermissionError", - }, 403 + authenticated_user_id = token_auth.current_user() + if not ProjectAdminService.is_user_action_permitted_on_project( + authenticated_user_id, project_id + ): + raise Forbidden( + sub_code="USER_NOT_PROJECT_MANAGER", + project_id=project_id, + user_id=authenticated_user_id, + ) try: ProjectService.unset_project_as_featured(project_id) return {"Success": True}, 200 except ValueError as e: - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 + return { + "Error": str(e).split("-")[1], + "SubCode": str(e).split("-")[0], + }, 403 # FLAGGED FOR STATUS CODE: 409 class ProjectsActionsSetInterestsAPI(Resource): @@ -301,17 +302,15 @@ def post(self, project_id): 500: description: Internal Server Error """ - try: - authenticated_user_id = token_auth.current_user() - if not ProjectAdminService.is_user_action_permitted_on_project( - authenticated_user_id, project_id - ): - raise ValueError() - except ValueError: - return { - "Error": "User is not a manager of the project", - "SubCode": "UserPermissionError", - }, 403 + authenticated_user_id = token_auth.current_user() + if not ProjectAdminService.is_user_action_permitted_on_project( + authenticated_user_id, project_id + ): + raise Forbidden( + sub_code="USER_NOT_PROJECT_MANAGER", + project_id=project_id, + user_id=authenticated_user_id, + ) data = request.get_json() project_interests = InterestService.create_or_update_project_interests( diff --git a/backend/api/projects/campaigns.py b/backend/api/projects/campaigns.py index 6d29087970..164d2a487a 100644 --- a/backend/api/projects/campaigns.py +++ b/backend/api/projects/campaigns.py @@ -1,6 +1,7 @@ from flask_restful import Resource, current_app from schematics.exceptions import DataError +from backend.exceptions import Forbidden from backend.models.dtos.campaign_dto import CampaignProjectDTO from backend.services.campaign_service import CampaignService from backend.services.project_admin_service import ProjectAdminService @@ -52,10 +53,11 @@ def post(self, project_id, campaign_id): if not ProjectAdminService.is_user_action_permitted_on_project( authenticated_user_id, project_id ): - return { - "Error": "User is not a manager of the project", - "SubCode": "UserPermissionError", - }, 403 + raise Forbidden( + sub_code="USER_NOT_PROJECT_MANAGER", + project_id=project_id, + user_id=authenticated_user_id, + ) try: campaign_project_dto = CampaignProjectDTO() campaign_project_dto.campaign_id = campaign_id @@ -145,10 +147,11 @@ def delete(self, project_id, campaign_id): if not ProjectAdminService.is_user_action_permitted_on_project( authenticated_user_id, project_id ): - return { - "Error": "User is not a manager of the project", - "SubCode": "UserPermissionError", - }, 403 + raise Forbidden( + sub_code="USER_NOT_PROJECT_MANAGER", + project_id=project_id, + user_id=authenticated_user_id, + ) CampaignService.delete_project_campaign(project_id, campaign_id) return {"Success": "Campaigns Deleted"}, 200 diff --git a/backend/api/projects/resources.py b/backend/api/projects/resources.py index 3831ccb071..29fb8228d1 100644 --- a/backend/api/projects/resources.py +++ b/backend/api/projects/resources.py @@ -1,9 +1,12 @@ import geojson import io +from distutils.util import strtobool + from flask import send_file from flask_restful import Resource, current_app, request from schematics.exceptions import DataError -from distutils.util import strtobool + +from backend.exceptions import NotFound, Forbidden from backend.models.dtos.project_dto import ( DraftProjectDTO, ProjectDTO, @@ -18,7 +21,6 @@ from backend.services.project_service import ( ProjectService, ProjectServiceError, - NotFound, ) from backend.services.users.user_service import UserService from backend.services.organisation_service import OrganisationService @@ -100,24 +102,15 @@ def get(self, project_id): abbreviated, ) - if project_dto: - project_dto = project_dto.to_primitive() - if as_file: - return send_file( - io.BytesIO(geojson.dumps(project_dto).encode("utf-8")), - mimetype="application/json", - as_attachment=True, - download_name=f"project_{str(project_id)}.json", - ) - - return project_dto, 200 - else: - return { - "Error": "User not permitted: Private Project", - "SubCode": "PrivateProject", - }, 403 - except ProjectServiceError as e: - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 + project_dto = project_dto.to_primitive() + if as_file: + return send_file( + io.BytesIO(geojson.dumps(project_dto).encode("utf-8")), + mimetype="application/json", + as_attachment=True, + download_name=f"project_{str(project_id)}.json", + ) + return project_dto, 200 finally: # this will try to unlock tasks that have been locked too long try: @@ -204,8 +197,6 @@ def post(self): draft_project_dto ) return {"projectId": draft_project_id}, 201 - except ProjectAdminServiceError as e: - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 except (InvalidGeoJson, InvalidData) as e: return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 400 @@ -248,10 +239,11 @@ def head(self, project_id): token_auth.current_user(), project_id ) except ValueError: - return { - "Error": "User is not a manager of the project", - "SubCode": "UserPermissionError", - }, 403 + raise Forbidden( + sub_code="USER_NOT_PROJECT_MANAGER", + project_id=project_id, + user_id=token_auth.current_user(), + ) project_dto = ProjectAdminService.get_project_dto_for_admin(project_id) return project_dto.to_primitive(), 200 @@ -383,10 +375,11 @@ def patch(self, project_id): if not ProjectAdminService.is_user_action_permitted_on_project( authenticated_user_id, project_id ): - return { - "Error": "User is not a manager of the project", - "SubCode": "UserPermissionError", - }, 403 + raise Forbidden( + sub_code="USER_NOT_PROJECT_MANAGER", + project_id=project_id, + user_id=authenticated_user_id, + ) try: project_dto = ProjectDTO(request.get_json()) project_dto.project_id = project_id @@ -401,7 +394,10 @@ def patch(self, project_id): except InvalidGeoJson as e: return {"Invalid GeoJson": str(e)}, 400 except ProjectAdminServiceError as e: - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 + return { + "Error": str(e).split("-")[1], + "SubCode": str(e).split("-")[0], + }, 403 # FLAGGED FOR STATUS CODE @token_auth.login_required def delete(self, project_id): @@ -437,22 +433,19 @@ def delete(self, project_id): 500: description: Internal Server Error """ - try: - authenticated_user_id = token_auth.current_user() - if not ProjectAdminService.is_user_action_permitted_on_project( - authenticated_user_id, project_id - ): - raise ValueError() - except ValueError: - return { - "Error": "User is not a manager of the project", - "SubCode": "UserPermissionError", - }, 403 + authenticated_user_id = token_auth.current_user() + # FLAGGED: CONFLICTING PERMISSION CHECK WITH SERVICE FUNCTION + if not ProjectAdminService.is_user_action_permitted_on_project( + authenticated_user_id, project_id + ): + raise Forbidden( + sub_code="USER_NOT_PROJECT_MANAGER", user_id=authenticated_user_id + ) try: ProjectAdminService.delete_project(project_id, authenticated_user_id) return {"Success": "Project deleted"}, 200 - except ProjectAdminServiceError as e: + except ProjectAdminServiceError as e: # FLAGGED FOR STATUS CODE return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 @@ -733,10 +726,9 @@ def get(self): authenticated_user_id ) if len(orgs_dto.organisations) < 1: - return { - "Error": "User is not a manager of the project", - "SubCode": "UserPermissionError", - }, 403 + raise Forbidden( + sub_code="USER_NOT_ORG_MANAGER", user_id=authenticated_user_id + ) try: search_dto = ProjectSearchBBoxDTO() @@ -806,10 +798,9 @@ def get(self): authenticated_user_id ) if len(orgs_dto.organisations) < 1: - return { - "Error": "User is not a manager of the project", - "SubCode": "UserPermissionError", - }, 403 + raise Forbidden( + sub_code="USER_NOT_ORG_MANAGER", user_id=authenticated_user_id + ) search_dto = self.setup_search_dto() admin_projects = ProjectAdminService.get_projects_for_admin( @@ -952,8 +943,6 @@ def get(self, project_id): ) return project_dto, 200 - except ProjectServiceError as e: - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 finally: # this will try to unlock tasks that have been locked too long try: @@ -997,13 +986,15 @@ def get(self, project_id): 500: description: Internal Server Error """ + authenticated_user_id = token_auth.current_user() if not ProjectAdminService.is_user_action_permitted_on_project( - token_auth.current_user(), project_id + authenticated_user_id, project_id ): - return { - "Error": "User is not a manager of the project", - "SubCode": "UserPermissionError", - }, 403 + raise Forbidden( + sub_code="USER_NOT_PROJECT_MANAGER", + project_id=project_id, + user_id=authenticated_user_id, + ) project_dto = ProjectAdminService.get_project_dto_for_admin(project_id) return project_dto.to_primitive(), 200 @@ -1088,7 +1079,7 @@ def get(self, project_id): try: priority_areas = ProjectService.get_project_priority_areas(project_id) return priority_areas, 200 - except ProjectServiceError: + except ProjectServiceError: # FLAGGED: NEVER REACHING CODE return {"Error": "Unable to fetch project"}, 403 diff --git a/backend/api/projects/teams.py b/backend/api/projects/teams.py index 3dbb12a337..9a20072ec8 100644 --- a/backend/api/projects/teams.py +++ b/backend/api/projects/teams.py @@ -1,6 +1,7 @@ from flask_restful import Resource, request, current_app from schematics.exceptions import DataError +from backend.exceptions import Forbidden from backend.services.team_service import TeamService, TeamServiceError from backend.services.project_admin_service import ProjectAdminService from backend.services.project_service import ProjectService @@ -32,7 +33,7 @@ def get(self, project_id): responses: 200: description: Teams listed successfully - 403: + 403: # TODO: Check if this is the correct error code description: Forbidden, if user is not authenticated 404: description: Not found @@ -91,11 +92,12 @@ def post(self, team_id, project_id): 500: description: Internal Server Error """ - if not TeamService.is_user_team_manager(team_id, token_auth.current_user()): + authenticated_user = token_auth.current_user() + if not TeamService.is_user_team_manager(team_id, authenticated_user): return { "Error": "User is not an admin or a manager for the team", "SubCode": "UserPermissionError", - }, 401 + }, 401 # FLAGGED FOR STATUS CODE try: role = request.get_json(force=True)["role"] @@ -103,25 +105,25 @@ def post(self, team_id, project_id): current_app.logger.error(f"Error validating request: {str(e)}") return {"Error": str(e), "SubCode": "InvalidData"}, 400 - try: - if not ProjectAdminService.is_user_action_permitted_on_project( - token_auth.current_user, project_id - ): - raise ValueError() - TeamService.add_team_project(team_id, project_id, role) - return ( - { - "Success": "Team {} assigned to project {} with role {}".format( - team_id, project_id, role - ) - }, - 201, + if not ProjectAdminService.is_user_action_permitted_on_project( + token_auth.current_user, project_id + ): + raise Forbidden( + sub_code="USER_NOT_PROJECT_MANAGER", + project_id=project_id, + team_id=team_id, + user_id=authenticated_user, ) - except ValueError: - return { - "Error": "User is not a manager of the project", - "SubCode": "UserPermissionError", - }, 403 + + TeamService.add_team_project(team_id, project_id, role) + return ( + { + "Success": "Team {} assigned to project {} with role {}".format( + team_id, project_id, role + ) + }, + 201, + ) @token_auth.login_required def patch(self, team_id, project_id): @@ -162,9 +164,9 @@ def patch(self, team_id, project_id): 201: description: Team project assignment created 401: - description: Forbidden, if user is not a manager of the project + description: Unauthenticated user 403: - description: Forbidden, if user is not authenticated + description: Forbidden, if user is not a manager of the project 404: description: Not found 500: @@ -180,16 +182,12 @@ def patch(self, team_id, project_id): if not ProjectAdminService.is_user_action_permitted_on_project( token_auth.current_user, project_id ): - raise ValueError() + raise Forbidden(sub_code="USER_NOT_PROJECT_MANAGER") + TeamService.change_team_role(team_id, project_id, role) return {"Status": "Team role updated successfully."}, 200 - except ValueError: - return { - "Error": "User is not a manager of the project", - "SubCode": "UserPermissionError", - }, 403 except TeamServiceError as e: - return str(e), 402 + return str(e), 402 # FLAGGED FOR STATUS CODE/UNREACHABLE @token_auth.login_required def delete(self, team_id, project_id): @@ -225,15 +223,15 @@ def delete(self, team_id, project_id): 500: description: Internal Server Error """ - try: - if not ProjectAdminService.is_user_action_permitted_on_project( - token_auth.current_user, project_id - ): - raise ValueError() - TeamService.delete_team_project(team_id, project_id) - return {"Success": True}, 200 - except ValueError: - return { - "Error": "User is not a manager of the project", - "SubCode": "UserPermissionError", - }, 403 + authenticated_user_id = token_auth.current_user() + if not ProjectAdminService.is_user_action_permitted_on_project( + authenticated_user_id, project_id + ): + raise Forbidden( + sub_code="USER_NOT_PROJECT_MANAGER", + project_id=project_id, + team_id=team_id, + user_id=authenticated_user_id, + ) + TeamService.delete_team_project(team_id, project_id) + return {"Success": True}, 200 diff --git a/backend/api/system/authentication.py b/backend/api/system/authentication.py index 58453ddad6..6363999daa 100644 --- a/backend/api/system/authentication.py +++ b/backend/api/system/authentication.py @@ -161,4 +161,7 @@ def get(self): return {"Status": "OK"}, 200 except AuthServiceError as e: - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 + return { + "Error": str(e).split("-")[1], + "SubCode": str(e).split("-")[0], + }, 403 # FLAGGED FOR STATUS CODE diff --git a/backend/api/system/banner.py b/backend/api/system/banner.py index 272bc3e12d..2c8c982e5d 100644 --- a/backend/api/system/banner.py +++ b/backend/api/system/banner.py @@ -2,6 +2,7 @@ from flask_restful import Resource, request from schematics.exceptions import DataError +from backend.exceptions import Forbidden from backend.models.postgis.banner import Banner from backend.models.dtos.banner_dto import BannerDTO from backend.models.postgis.statuses import UserRole @@ -83,10 +84,7 @@ def patch(self): authenticated_user_id = token_auth.current_user() authenticated_user = UserService.get_user_by_id(authenticated_user_id) if authenticated_user.role != UserRole.ADMIN.value: - return { - "Error": "Banner can only be updated by system admins", - "SubCode": "OnlyAdminAccess", - }, 403 + raise Forbidden(sub_code="USER_NOT_ADMIN", user_id=authenticated_user_id) banner_dto.message = Banner.to_html( banner_dto.message diff --git a/backend/api/tasks/actions.py b/backend/api/tasks/actions.py index 10878ecffc..599b43a9af 100644 --- a/backend/api/tasks/actions.py +++ b/backend/api/tasks/actions.py @@ -1,7 +1,7 @@ from flask_restful import Resource, current_app, request from schematics.exceptions import DataError -from backend.exceptions import NotFound +from backend.exceptions import NotFound, Forbidden from backend.models.dtos.grid_dto import SplitTaskDTO from backend.models.postgis.utils import InvalidGeoJson from backend.services.grid.split_service import SplitService, SplitServiceError @@ -95,7 +95,7 @@ def post(self, project_id, task_id): ProjectService.exists(project_id) # Check if project exists task = MappingService.lock_task_for_mapping(lock_task_dto) return task.to_primitive(), 200 - except MappingServiceError as e: + except MappingServiceError as e: # FLAGGED FOR STATUS CODE return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 except UserLicenseError: return { @@ -181,7 +181,7 @@ def post(self, project_id, task_id): ProjectService.exists(project_id) # Check if project exists task = MappingService.stop_mapping_task(stop_task) return task.to_primitive(), 200 - except MappingServiceError as e: + except MappingServiceError as e: # FLAGGED FOR STATUS CODE return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 @@ -260,11 +260,11 @@ def post(self, project_id, task_id): ProjectService.exists(project_id) # Check if project exists task = MappingService.unlock_task_after_mapping(mapped_task) return task.to_primitive(), 200 - except MappingServiceError as e: + except MappingServiceError as e: # FLAGGED FOR STATUS CODE return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 - except NotFound as e: + except NotFound as e: # FLAGGED IF THIS CATCH IS NEEDED return e.to_dict() - except Exception as e: + except Exception as e: # FLAGGED IF THIS CATCH IS NEEDED error_msg = f"Task Lock API - unhandled error: {str(e)}" current_app.logger.critical(error_msg) return { @@ -328,7 +328,7 @@ def post(self, project_id, task_id): project_id, task_id, token_auth.current_user(), preferred_locale ) return task.to_primitive(), 200 - except MappingServiceError as e: + except MappingServiceError as e: # FLAGGED FOR STATUS CODE return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 @@ -403,7 +403,7 @@ def post(self, project_id): ProjectService.exists(project_id) # Check if project exists tasks = ValidatorService.lock_tasks_for_validation(validator_dto) return tasks.to_primitive(), 200 - except ValidatorServiceError as e: + except ValidatorServiceError as e: # FLAGGED FOR STATUS CODE return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 except UserLicenseError: return { @@ -481,7 +481,7 @@ def post(self, project_id): ProjectService.exists(project_id) # Check if project exists tasks = ValidatorService.stop_validating_tasks(validated_dto) return tasks.to_primitive(), 200 - except ValidatorServiceError as e: + except ValidatorServiceError as e: # FLAGGED FOR STATUS CODE return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 @@ -553,7 +553,7 @@ def post(self, project_id): ProjectService.exists(project_id) # Check if project exists tasks = ValidatorService.unlock_tasks_after_validation(validated_dto) return tasks.to_primitive(), 200 - except ValidatorServiceError as e: + except ValidatorServiceError as e: # FLAGGED FOR STATUS CODE return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 @@ -590,17 +590,15 @@ def post(self, project_id): 500: description: Internal Server Error """ - try: - authenticated_user_id = token_auth.current_user() - if not ProjectAdminService.is_user_action_permitted_on_project( - authenticated_user_id, project_id - ): - raise ValueError() - except ValueError: - return { - "Error": "User is not a manager of the project", - "SubCode": "UserPermissionError", - }, 403 + authenticated_user_id = token_auth.current_user() + if not ProjectAdminService.is_user_action_permitted_on_project( + authenticated_user_id, project_id + ): + raise Forbidden( + sub_code="USER_NOT_PROJECT_MANAGER", + project_id=project_id, + user_id=authenticated_user_id, + ) MappingService.map_all_tasks(project_id, authenticated_user_id) return {"Success": "All tasks mapped"}, 200 @@ -639,17 +637,15 @@ def post(self, project_id): 500: description: Internal Server Error """ - try: - authenticated_user_id = token_auth.current_user() - if not ProjectAdminService.is_user_action_permitted_on_project( - authenticated_user_id, project_id - ): - raise ValueError() - except ValueError: - return { - "Error": "User is not a manager of the project", - "SubCode": "UserPermissionError", - }, 403 + authenticated_user_id = token_auth.current_user() + if not ProjectAdminService.is_user_action_permitted_on_project( + authenticated_user_id, project_id + ): + raise Forbidden( + sub_code="USER_NOT_PROJECT_MANAGER", + project_id=project_id, + user_id=authenticated_user_id, + ) ValidatorService.validate_all_tasks(project_id, authenticated_user_id) return {"Success": "All tasks validated"}, 200 @@ -688,17 +684,15 @@ def post(self, project_id): 500: description: Internal Server Error """ - try: - authenticated_user_id = token_auth.current_user() - if not ProjectAdminService.is_user_action_permitted_on_project( - authenticated_user_id, project_id - ): - raise ValueError() - except ValueError: - return { - "Error": "User is not a manager of the project", - "SubCode": "UserPermissionError", - }, 403 + authenticated_user_id = token_auth.current_user() + if not ProjectAdminService.is_user_action_permitted_on_project( + authenticated_user_id, project_id + ): + raise Forbidden( + sub_code="USER_NOT_PROJECT_MANAGER", + project_id=project_id, + user_id=authenticated_user_id, + ) ValidatorService.invalidate_all_tasks(project_id, authenticated_user_id) return {"Success": "All tasks invalidated"}, 200 @@ -737,17 +731,15 @@ def post(self, project_id): 500: description: Internal Server Error """ - try: - authenticated_user_id = token_auth.current_user() - if not ProjectAdminService.is_user_action_permitted_on_project( - authenticated_user_id, project_id - ): - raise ValueError() - except ValueError: - return { - "Error": "User is not a manager of the project", - "SubCode": "UserPermissionError", - }, 403 + authenticated_user_id = token_auth.current_user() + if not ProjectAdminService.is_user_action_permitted_on_project( + authenticated_user_id, project_id + ): + raise Forbidden( + sub_code="USER_NOT_PROJECT_MANAGER", + project_id=project_id, + user_id=authenticated_user_id, + ) MappingService.reset_all_badimagery(project_id, authenticated_user_id) return {"Success": "All bad imagery tasks marked ready for mapping"}, 200 @@ -786,18 +778,15 @@ def post(self, project_id): 500: description: Internal Server Error """ - try: - authenticated_user_id = token_auth.current_user() - authenticated_user_id = token_auth.current_user() - if not ProjectAdminService.is_user_action_permitted_on_project( - authenticated_user_id, project_id - ): - raise ValueError() - except ValueError: - return { - "Error": "User is not a manager of the project", - "SubCode": "UserPermissionError", - }, 403 + authenticated_user_id = token_auth.current_user() + if not ProjectAdminService.is_user_action_permitted_on_project( + authenticated_user_id, project_id + ): + raise Forbidden( + sub_code="USER_NOT_PROJECT_MANAGER", + project_id=project_id, + user_id=authenticated_user_id, + ) ProjectAdminService.reset_all_tasks(project_id, authenticated_user_id) return {"Success": "All tasks reset"}, 200 @@ -868,9 +857,9 @@ def post(self, project_id, task_id): ProjectService.exists(project_id) # Check if project exists tasks = SplitService.split_task(split_task_dto) return tasks.to_primitive(), 200 - except SplitServiceError as e: + except SplitServiceError as e: # FLAGGED FOR STATUS CODE return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 - except InvalidGeoJson as e: + except InvalidGeoJson as e: # FLAGGED FOR STATUS CODE return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 @@ -945,7 +934,7 @@ def post(self, project_id): ProjectService.exists(project_id) # Check if project exists MappingService.extend_task_lock_time(extend_dto) return {"Success": "Successfully extended task expiry"}, 200 - except MappingServiceError as e: + except MappingServiceError as e: # FLAGGED FOR STATUS CODE return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 @@ -1017,8 +1006,6 @@ def post(self, project_id): "Error": "Unable to revert tasks", "SubCode": "InvalidData", }, 400 - try: - ValidatorService.revert_user_tasks(revert_dto) - return {"Success": "Successfully reverted tasks"}, 200 - except ValidatorServiceError as e: - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 + + ValidatorService.revert_user_tasks(revert_dto) + return {"Success": "Successfully reverted tasks"}, 200 diff --git a/backend/api/tasks/resources.py b/backend/api/tasks/resources.py index 90a04905cd..34ba83c2ec 100644 --- a/backend/api/tasks/resources.py +++ b/backend/api/tasks/resources.py @@ -5,13 +5,12 @@ from flask_restful import Resource, current_app, request from schematics.exceptions import DataError +from backend.exceptions import Forbidden from backend.services.mapping_service import MappingService from backend.models.dtos.grid_dto import GridDTO - from backend.services.users.authentication_service import token_auth, tm from backend.services.users.user_service import UserService from backend.services.validator_service import ValidatorService - from backend.services.project_service import ProjectService, ProjectServiceError from backend.services.grid.grid_service import GridService from backend.models.postgis.statuses import UserRole @@ -116,7 +115,7 @@ def get(self, project_id): ) return tasks_json, 200 - except ProjectServiceError as e: + except ProjectServiceError as e: # FLAGGED: CHECK IF THIS EXCEPTION IS RAISED return {"Error": str(e)}, 403 @token_auth.login_required @@ -167,10 +166,7 @@ def delete(self, project_id): user_id = token_auth.current_user() user = UserService.get_user_by_id(user_id) if user.role != UserRole.ADMIN.value: - return { - "Error": "This endpoint action is restricted to ADMIN users.", - "SubCode": "OnlyAdminAccess", - }, 403 + raise Forbidden(sub_code="USER_NOT_ADMIN", user_id=user_id) tasks_ids = request.get_json().get("tasks") if tasks_ids is None: diff --git a/backend/api/teams/actions.py b/backend/api/teams/actions.py index 27a5799afd..6925d10263 100644 --- a/backend/api/teams/actions.py +++ b/backend/api/teams/actions.py @@ -2,10 +2,10 @@ from schematics.exceptions import DataError import threading +from backend.exceptions import Forbidden from backend.models.dtos.message_dto import MessageDTO from backend.services.team_service import ( TeamService, - TeamJoinNotAllowed, TeamServiceError, ) from backend.services.users.authentication_service import token_auth, tm @@ -129,12 +129,10 @@ def patch(self, team_id): ) return {"Success": "True"}, 200 else: - return ( - { - "Error": "You don't have permissions to approve this join team request", - "SubCode": "ApproveJoinError", - }, - 403, + raise Forbidden( + sub_code="USER_NOT_TEAM_MANAGER", + team_id=team_id, + user_id=authenticated_user_id, ) elif request_type == "invite-response": TeamService.accept_reject_invitation_request( @@ -199,12 +197,9 @@ def post(self, team_id): "SubCode": "InvalidData", }, 400 - try: - authenticated_user_id = token_auth.current_user() - TeamService.add_user_to_team(team_id, authenticated_user_id, username, role) - return {"Success": "User added to the team"}, 200 - except TeamJoinNotAllowed as e: - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 + authenticated_user_id = token_auth.current_user() + TeamService.add_user_to_team(team_id, authenticated_user_id, username, role) + return {"Success": "User added to the team"}, 200 class TeamsActionsLeaveAPI(Resource): @@ -261,14 +256,10 @@ def post(self, team_id): TeamService.leave_team(team_id, username) return {"Success": "User removed from the team"}, 200 else: - return ( - { - "Error": "You don't have permissions to remove {} from this team.".format( - username - ), - "SubCode": "RemoveUserError", - }, - 403, + raise Forbidden( + sub_code="USER_NOT_TEAM_MANAGER", + team_id=team_id, + user_id=authenticated_user_id, ) @@ -329,7 +320,12 @@ def post(self, team_id): team_id, authenticated_user_id ) if not is_manager: - raise ValueError + raise Forbidden( + sub_code="USER_NOT_TEAM_MANAGER", + team_id=team_id, + user_id=authenticated_user_id, + ) + message_dto.from_user_id = authenticated_user_id message_dto.validate() if not message_dto.message.strip() or not message_dto.subject.strip(): @@ -342,12 +338,6 @@ def post(self, team_id): "Error": "Request payload did not match validation", "SubCode": "InvalidData", }, 400 - except ValueError: - return { - "Error": "Unauthorised to send message to team members", - "SubCode": "UserNotPermitted", - }, 403 - try: threading.Thread( target=TeamService.send_message_to_all_team_members, @@ -355,5 +345,5 @@ def post(self, team_id): ).start() return {"Success": "Message sent successfully"}, 200 - except ValueError as e: + except ValueError as e: # FLAGGED: CHECK IS THIS EVER RAISED return {"Error": str(e)}, 403 diff --git a/backend/api/teams/resources.py b/backend/api/teams/resources.py index 3f5c13b848..8718c86161 100644 --- a/backend/api/teams/resources.py +++ b/backend/api/teams/resources.py @@ -1,6 +1,7 @@ from flask_restful import Resource, request, current_app from schematics.exceptions import DataError +from backend.exceptions import Forbidden from backend.models.dtos.team_dto import ( NewTeamDTO, UpdateTeamDTO, @@ -86,10 +87,11 @@ def patch(self, team_id): ) and not OrganisationService.can_user_manage_organisation( team.organisation_id, authenticated_user_id ): - return { - "Error": "User is not a admin or a manager for the team", - "SubCode": "UserNotTeamManager", - }, 403 + raise Forbidden( + sub_code="USER_NOT_TEAM_MANAGER", + team_id=team_id, + user_id=authenticated_user_id, + ) except DataError as e: current_app.logger.error(f"error validating request: {str(e)}") return {"Error": str(e), "SubCode": "InvalidData"}, 400 @@ -139,8 +141,6 @@ def get(self, team_id): team_dto = TeamService.get_team_as_dto(team_id, user_id, omit_members) return team_dto.to_primitive(), 200 - # TODO: Add delete API then do front end services and ui work - @token_auth.login_required def delete(self, team_id): """ @@ -175,11 +175,13 @@ def delete(self, team_id): 500: description: Internal Server Error """ - if not TeamService.is_user_team_manager(team_id, token_auth.current_user()): - return { - "Error": "User is not a manager for the team", - "SubCode": "UserNotTeamManager", - }, 401 + authenticated_user_id = token_auth.current_user() + if not TeamService.is_user_team_manager(team_id, authenticated_user_id): + raise Forbidden( + sub_code="USER_NOT_TEAM_MANAGER", + team_id=team_id, + user_id=authenticated_user_id, + ) TeamService.delete_team(team_id) return {"Success": "Team deleted"}, 200 @@ -365,7 +367,6 @@ def post(self): team_id = TeamService.create_team(team_dto) return {"teamId": team_id}, 201 else: - error_msg = "User not permitted to create team for the Organisation" - return {"Error": error_msg, "SubCode": "CreateTeamNotPermitted"}, 403 + raise Forbidden(sub_code="USER_NOT_ORG_MANAGER") except TeamServiceError as e: return str(e), 400 diff --git a/backend/api/users/actions.py b/backend/api/users/actions.py index 0bd77f8c9d..e97c3678ee 100644 --- a/backend/api/users/actions.py +++ b/backend/api/users/actions.py @@ -183,6 +183,8 @@ def patch(self, username, role): responses: 200: description: Role set + 400: + description: Bad Request - Client Error 401: description: Unauthorized - Invalid credentials 403: @@ -196,7 +198,7 @@ def patch(self, username, role): UserService.add_role_to_user(token_auth.current_user(), username, role) return {"Success": "Role Added"}, 200 except UserServiceError as e: - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 + return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 400 class UsersActionsSetExpertModeAPI(Resource): diff --git a/backend/api/users/resources.py b/backend/api/users/resources.py index ab08d78fd9..603209c002 100644 --- a/backend/api/users/resources.py +++ b/backend/api/users/resources.py @@ -372,8 +372,6 @@ def get(self, username): description: Recommended projects found 401: description: Unauthorized - Invalid credentials - 403: - description: Forbidden 404: description: No recommended projects found 500: diff --git a/backend/error_messages.json b/backend/error_messages.json index f2afe220cf..a3c0a2e048 100644 --- a/backend/error_messages.json +++ b/backend/error_messages.json @@ -23,6 +23,17 @@ "BAD_REQUEST": "The request was invalid or cannot be otherwise served.", "UNAUTHORIZED": "Authentication credentials were missing or incorrect.", "FORBIDDEN": "The request is understood, but it has been refused or access is not allowed.", + "USER_NOT_ADMIN": "This action is only allowed for system administrators.", + "USER_NOT_ORG_MANAGER": "This action is only allowed for organisation managers or system administrators.", + "USER_NOT_PROJECT_MANAGER": "This action is only allowed for users with project manage permissions.", + "USER_NOT_TEAM_MANAGER": "This action is only allowed for users with team manage permissions.", + "INVALID_NEW_PROJECT_OWNER": "New project owner must be project's organisation manager or system admin", + "USER_ACTION_NOT_PERMITTED": "This user does not have the required permissions to perform this action.", + "DRAFT_PROJECT_NOT_ALLOWED": "This is a draft project and you do not have the required permissions to perform this action.", + "PRIVATE_PROJECT_NOT_ALLOWED": "This is a private project and you do not have the required permissions to perform this action.", + "USER_BLOCKED": "Your account has been blocked and is currently in read-only mode. Please contact support for further assistance.", + "MESSAGE_NOT_FOR_USER": "The requested message doesn't belong to the requesting user thus the action is not permitted.", + "COMMENT_NOT_OF_USER": "The requested comment doesn't belong to the requesting user thus the action is not permitted.", "CONFLICT": "The request could not be completed due to a conflict with the current state of the resource.", "INTERNAL_SERVER_ERROR": "An unexpected error occurred while processing the request. Please contact the system administrator." } diff --git a/backend/services/grid/split_service.py b/backend/services/grid/split_service.py index fe57b04015..ac99ac649e 100644 --- a/backend/services/grid/split_service.py +++ b/backend/services/grid/split_service.py @@ -186,17 +186,19 @@ def split_task(split_task_dto: SplitTaskDTO) -> TaskDTOs: if ( original_task.zoom and original_task.zoom >= 18 ) or original_task_area_m < 25000: - raise SplitServiceError("SmallToSplit- Task is too small to be split") + raise SplitServiceError( + "SmallToSplit- Task is too small to be split" + ) # FLAGGED STATUS CODE: 409 # check its locked for mapping by the current user if TaskStatus(original_task.task_status) != TaskStatus.LOCKED_FOR_MAPPING: raise SplitServiceError( - "LockToSplit- Status must be LOCKED_FOR_MAPPING to split" + "LockToSplit- Status must be LOCKED_FOR_MAPPING to split" # FLAGGED STATUS CODE: 409 ) if original_task.locked_by != split_task_dto.user_id: raise SplitServiceError( - "SplitOtherUserTask- Attempting to split a task owned by another user" + "SplitOtherUserTask- Attempting to split a task owned by another user" # FLAGGED STATUS CODE: 423 ) # create new geometries from the task geometry @@ -205,7 +207,9 @@ def split_task(split_task_dto: SplitTaskDTO) -> TaskDTOs: original_task.x, original_task.y, original_task.zoom, original_task ) except Exception as e: - raise SplitServiceError(f"Error splitting task{str(e)}") + raise SplitServiceError( + f"Error splitting task{str(e)}" + ) # FLAGGED STATUS CODE: 500 # create new tasks from the new geojson i = Task.get_max_task_id_for_project(split_task_dto.project_id) @@ -217,7 +221,7 @@ def split_task(split_task_dto: SplitTaskDTO) -> TaskDTOs: if not new_geometry.intersects(original_geometry): raise InvalidGeoJson( "SplitGeoJsonError- New split task does not intersect original task" - ) + ) # FLAGGED STATUS CODE: 400 # insert new tasks into database i = i + 1 diff --git a/backend/services/mapping_service.py b/backend/services/mapping_service.py index ca8c1a47bd..d5223c6dce 100644 --- a/backend/services/mapping_service.py +++ b/backend/services/mapping_service.py @@ -4,7 +4,7 @@ from flask import current_app from geoalchemy2 import shape -from backend.exceptions import NotFound +from backend.exceptions import NotFound, Forbidden from backend.models.dtos.mapping_dto import ( ExtendLockTimeDTO, TaskDTO, @@ -74,7 +74,7 @@ def _is_task_undoable(logged_in_user_id: int, task: Task) -> bool: if last_action.user_id == int(logged_in_user_id) or is_user_permitted: return True - return False + return False # FLAGGED: Split out for permission and state @staticmethod def lock_task_for_mapping(lock_task_dto: LockTaskDTO) -> TaskDTO: @@ -88,7 +88,7 @@ def lock_task_for_mapping(lock_task_dto: LockTaskDTO) -> TaskDTO: if task.locked_by != lock_task_dto.user_id: if not task.is_mappable(): - raise MappingServiceError( + raise MappingServiceError( # FLAGGED FOR STATUS CODE: 423 "InvalidTaskState- Task in invalid state for mapping" ) @@ -97,17 +97,21 @@ def lock_task_for_mapping(lock_task_dto: LockTaskDTO) -> TaskDTO: ) if not user_can_map: if error_reason == MappingNotAllowed.USER_NOT_ACCEPTED_LICENSE: - raise UserLicenseError("User must accept license to map this task") + raise UserLicenseError( + "User must accept license to map this task" + ) # FLAGGED FOR STATUS CODE: 409 elif error_reason == MappingNotAllowed.USER_NOT_ON_ALLOWED_LIST: - raise MappingServiceError( - "UserNotAllowed- User not on allowed list" + raise Forbidden( + sub_code="USER_BLOCKED", user_id=lock_task_dto.user_id ) elif error_reason == MappingNotAllowed.PROJECT_NOT_PUBLISHED: - raise MappingServiceError( - "ProjectNotPublished- Project is not published" + raise Forbidden( + sub_code="DRAFT_PROJECT_NOT_ALLOWED", + project_id=lock_task_dto.project_id, + user_id=lock_task_dto.user_id, ) elif error_reason == MappingNotAllowed.USER_ALREADY_HAS_TASK_LOCKED: - raise MappingServiceError( + raise MappingServiceError( # FLAGGED FOR STATUS CODE: 409 "UserAlreadyHasTaskLocked- User already has task locked" ) else: @@ -134,7 +138,7 @@ def unlock_task_after_mapping(mapped_task: MappedTaskDTO) -> TaskDTO: ]: raise MappingServiceError( "InvalidUnlockState- Can only set status to MAPPED, BADIMAGERY, READY after mapping" - ) + ) # FLAGGED FOR STATUS CODE: 409 # Update stats around the change of state last_state = TaskHistory.get_last_status( @@ -180,7 +184,7 @@ def get_task_locked_by_user(project_id: int, task_id: int, user_id: int) -> Task :raises: MappingServiceError """ task = MappingService.get_task(task_id, project_id) - if task is None: + if task is None: # FLAGGED: CHECK IF THIS CONDITION IS EVER REACHED raise NotFound( sub_code="TASK_NOT_FOUND", project_id=project_id, task_id=task_id ) @@ -188,11 +192,11 @@ def get_task_locked_by_user(project_id: int, task_id: int, user_id: int) -> Task if current_state != TaskStatus.LOCKED_FOR_MAPPING: raise MappingServiceError( "LockBeforeUnlocking- Status must be LOCKED_FOR_MAPPING to unlock" - ) + ) # FLAGGED FOR STATUS CODE: 409 if task.locked_by != user_id: raise MappingServiceError( "TaskNotOwned- Attempting to unlock a task owned by another user" - ) + ) # FLAGGED FOR STATUS CODE: 423 return task @staticmethod @@ -444,11 +448,11 @@ def lock_time_can_be_extended(project_id, task_id, user_id): ]: raise MappingServiceError( f"TaskStatusNotLocked- Task {task_id} status is not LOCKED_FOR_MAPPING or LOCKED_FOR_VALIDATION." - ) + ) # FLAGGED FOR STATUS CODE: 409 if task.locked_by != user_id: raise MappingServiceError( "LockedByAnotherUser- Task is locked by another user." - ) + ) # FLAGGED FOR STATUS CODE: 423 @staticmethod def extend_task_lock_time(extend_dto: ExtendLockTimeDTO): diff --git a/backend/services/messaging/chat_service.py b/backend/services/messaging/chat_service.py index 825be2123f..8b84101878 100644 --- a/backend/services/messaging/chat_service.py +++ b/backend/services/messaging/chat_service.py @@ -2,7 +2,7 @@ from flask import current_app from backend import db -from backend.exceptions import NotFound +from backend.exceptions import NotFound, Forbidden from backend.models.dtos.message_dto import ChatMessageDTO, ProjectChatDTO from backend.models.postgis.project_chat import ProjectChat from backend.models.postgis.project_info import ProjectInfo @@ -26,6 +26,8 @@ def post_message( project_name = ProjectInfo.get_dto_for_locale( project_id, project.default_locale ).name + + # Check if user is allowed to post message is_allowed_user = True is_manager_permission = ProjectAdminService.is_user_action_permitted_on_project( authenticated_user_id, project_id @@ -37,7 +39,11 @@ def post_message( ProjectStatus(project.status) == ProjectStatus.DRAFT and not is_manager_permission ): - raise ValueError("UserNotPermitted- User not permitted to post Comment") + raise Forbidden( + sub_code="DRAFT_PROJECT_NOT_ALLOWED", + project_id=project_id, + user_id=authenticated_user_id, + ) if project.private: is_allowed_user = False @@ -77,7 +83,11 @@ def post_message( # Ensure we return latest messages after post return ProjectChat.get_messages(chat_dto.project_id, 1, 5) else: - raise ValueError("UserNotPermitted- User not permitted to post Comment") + raise Forbidden( + sub_code="PRIVATE_PROJECT_NOT_ALLOWED", + project_id=project_id, + user_id=authenticated_user_id, + ) @staticmethod def get_messages(project_id: int, page: int, per_page: int) -> ProjectChatDTO: @@ -112,12 +122,12 @@ def get_project_chat_by_id(project_id: int, comment_id: int) -> ProjectChat: def delete_project_chat_by_id(project_id: int, comment_id: int, user_id: int): """Deletes a message from a project chat ---------------------------------------- - :param project_id: The id of the project the message belongs to - :param message_id: The message id to delete :param user_id: The id of the requesting user + :param project_id: The id of the project the message belongs to + :param comment_id: The message id to delete ---------------------------------------- :raises NotFound: When the message is not found - :raises Unauthorized: When the user is not allowed to delete the message + :raises Forbidden: When the user is not allowed to delete the message ---------------------------------------- returns: None """ @@ -145,6 +155,8 @@ def delete_project_chat_by_id(project_id: int, comment_id: int, user_id: int): db.session.delete(chat_message) db.session.commit() else: - raise ValueError( - "DeletePermissionError- User not allowed to delete message" + raise Forbidden( + sub_code="COMMENT_NOT_OF_USER", + project_id=project_id, + comment_id=comment_id, ) diff --git a/backend/services/messaging/message_service.py b/backend/services/messaging/message_service.py index a74dba6167..3726b6c8e2 100644 --- a/backend/services/messaging/message_service.py +++ b/backend/services/messaging/message_service.py @@ -9,7 +9,7 @@ from markdown import markdown from backend import db, create_app -from backend.exceptions import NotFound +from backend.exceptions import NotFound, Forbidden from backend.models.dtos.message_dto import MessageDTO, MessagesDTO from backend.models.dtos.stats_dto import Pagination from backend.models.postgis.message import Message, MessageType @@ -754,9 +754,8 @@ def get_message(message_id: int, user_id: int) -> Message: raise NotFound(sub_code="MESSAGE_NOT_FOUND", message_id=message_id) if message.to_user_id != int(user_id): - raise MessageServiceError( - "AccessOtherUserMessage- " - + f"User {user_id} attempting to access another users message {message_id}" + raise Forbidden( + sub_code="MESSAGE_NOT_FOR_USER", message_id=message_id, user_id=user_id ) return message diff --git a/backend/services/organisation_service.py b/backend/services/organisation_service.py index e1eef65c9e..3b37851cf1 100644 --- a/backend/services/organisation_service.py +++ b/backend/services/organisation_service.py @@ -143,7 +143,7 @@ def delete_organisation(organisation_id: int): else: raise OrganisationServiceError( "Organisation has projects, cannot be deleted" - ) + ) # FLAGGED STATUS CODE: 409 @staticmethod def get_organisations(manager_user_id: int): @@ -277,7 +277,7 @@ def assert_validate_name(org: Organisation, name: str): """Validates that the organisation name doesn't exist""" if org.name != name and Organisation.get_organisation_by_name(name) is not None: raise OrganisationServiceError( - f"NameExists- Organisation name already exists: {name}" + f"NameExists- Organisation name already exists: {name}" # FLAGGED STATUS CODE : 409 ) @staticmethod @@ -286,7 +286,7 @@ def assert_validate_users(organisation_dto: OrganisationDTO): if organisation_dto.managers and len(organisation_dto.managers) == 0: raise OrganisationServiceError( "MustHaveAdmin- Must have at least one admin" - ) + ) # FLAGGED: STATUS CODE 409 if organisation_dto.managers and len(organisation_dto.managers) > 0: managers = [] diff --git a/backend/services/project_admin_service.py b/backend/services/project_admin_service.py index 13e34bb6fb..32d880e55b 100644 --- a/backend/services/project_admin_service.py +++ b/backend/services/project_admin_service.py @@ -3,7 +3,7 @@ import geojson from flask import current_app -from backend.exceptions import NotFound +from backend.exceptions import NotFound, Forbidden from backend.models.dtos.project_dto import ( DraftProjectDTO, ProjectDTO, @@ -57,12 +57,7 @@ def create_draft_project(draft_project_dto: DraftProjectDTO) -> int: # First things first, we need to validate that the author_id is a PM. issue #1715 if not (is_admin or is_org_manager): - user = UserService.get_user_by_id(user_id) - raise ( - ProjectAdminServiceError( - f"NotPermittedToCreate- User {user.username} is not permitted to create project" - ) - ) + raise Forbidden(sub_code="USER_NOT_ORG_MANAGER", user_id=user_id) # If we're cloning we'll copy all the project details from the clone, otherwise create brand new project if draft_project_dto.cloneFromProjectId: @@ -131,15 +126,16 @@ def update_project(project_dto: ProjectDTO, authenticated_user_id: int): ProjectAdminService._validate_imagery_licence(project_dto.license_id) # To be handled before reaching this function - if ProjectAdminService.is_user_action_permitted_on_project( + if ProjectAdminService.is_user_action_permitted_on_project( # FLAGGED: ALREADY CHECKED IN VIEW FUNCTION authenticated_user_id, project_id ): project = ProjectAdminService._get_project_by_id(project_id) project.update(project_dto) else: - raise ValueError( - str(project_id) - + " :Project can only be updated by admins or by the owner" + raise Forbidden( + sub_code="USER_NOT_PROJECT_MANAGER", + user_id=authenticated_user_id, + project_id=project_id, ) return project @@ -150,7 +146,7 @@ def _validate_imagery_licence(license_id: int): try: LicenseService.get_license_as_dto(license_id) except NotFound: - raise ProjectAdminServiceError( + raise ProjectAdminServiceError( # FLAGGED WHY NOT NOTFOUND? f"RequireLicenseId- LicenseId {license_id} not found" ) @@ -173,8 +169,8 @@ def delete_project(project_id: int, authenticated_user_id: int): "HasMappedTasks- Project has mapped tasks, cannot be deleted" ) else: - raise ProjectAdminServiceError( - "DeletePermissionError- User does not have permissions to delete project" + raise Forbidden( + sub_code="USER_NOT_ORG_MANAGER", user_id=authenticated_user_id ) @staticmethod @@ -257,7 +253,7 @@ def _validate_default_locale(default_locale, project_info_locales): break if default_info is None: - raise ProjectAdminServiceError( + raise ProjectAdminServiceError( # FLAGGED: SHOULD BE VALIDATED IN DTO? 400 "InfoForLocaleRequired- Project Info for Default Locale not provided" ) @@ -267,10 +263,10 @@ def _validate_default_locale(default_locale, project_info_locales): if not value: raise ( - ProjectAdminServiceError( + ProjectAdminServiceError( # FLAGGED: SHOULD BE VALIDATED IN DTO? f"MissingRequiredAttribute- {attr} not provided for Default Locale" ) - ) + ) # Flagged : for status code 400 return True # Indicates valid default locale for unit testing @@ -299,8 +295,8 @@ def transfer_project_to(project_id: int, transfering_user_id: int, username: str project.organisation_id, transfering_user_id ) if not (is_admin or is_author or is_org_manager): - raise ProjectAdminServiceError( - "TransferPermissionError- User does not have permissions to transfer project" + raise Forbidden( + sub_code="USER_NOT_ORG_MANAGER", user_id=transfering_user_id ) # Check permissions for the new owner - must be project's org manager @@ -309,12 +305,7 @@ def transfer_project_to(project_id: int, transfering_user_id: int, username: str ) is_new_owner_admin = UserService.is_user_an_admin(new_owner.id) if not (is_new_owner_org_manager or is_new_owner_admin): - error_message = ( - "InvalidNewOwner- New owner must be project's org manager or TM admin" - ) - if current_app: - current_app.logger.debug(error_message) - raise ValueError(error_message) + raise Forbidden(sub_code="INVALID_NEW_PROJECT_OWNER", user_id=new_owner.id) else: transferred_by = User.get_by_id(transfering_user_id).username project.author_id = new_owner.id diff --git a/backend/services/project_service.py b/backend/services/project_service.py index 7d6dd1f7e3..18611326c0 100644 --- a/backend/services/project_service.py +++ b/backend/services/project_service.py @@ -2,7 +2,7 @@ from cachetools import TTLCache, cached from flask import current_app -from backend.exceptions import NotFound +from backend.exceptions import NotFound, Forbidden from backend.models.dtos.mapping_dto import TaskDTOs from backend.models.dtos.project_dto import ( ProjectDTO, @@ -202,7 +202,7 @@ def get_project_dto_for_mapper( """ project = ProjectService.get_project_by_id(project_id) # if project is public and is not draft, we don't need to check permissions - if not project.private and not project.status == ProjectStatus.DRAFT.value: + if not project.private and project.status != ProjectStatus.DRAFT.value: return project.as_dto_for_mapping(current_user_id, locale, abbrev) is_allowed_user = True @@ -219,11 +219,14 @@ def get_project_dto_for_mapper( if project.status == ProjectStatus.DRAFT.value: if not is_manager_permission: is_allowed_user = False - raise ProjectServiceError("ProjectNotFetched- Unable to fetch project") + raise Forbidden( + sub_code="DRAFT_PROJECT_NOT_ALLOWED", + project_id=project_id, + user_id=current_user_id, + ) # Private Projects - allowed_users, admins, org admins & # assigned teams (mappers, validators, project managers), authors permitted - if project.private and not is_manager_permission: is_allowed_user = False if current_user_id: @@ -252,7 +255,11 @@ def get_project_dto_for_mapper( if is_allowed_user or is_manager_permission or is_team_member: return project.as_dto_for_mapping(current_user_id, locale, abbrev) else: - return None + raise Forbidden( + sub_code="PRIVATE_PROJECT_NOT_ALLOWED", + project_id=project_id, + user_id=current_user_id, + ) @staticmethod def get_project_tasks( diff --git a/backend/services/team_service.py b/backend/services/team_service.py index 292360e3b8..7e34c83d13 100644 --- a/backend/services/team_service.py +++ b/backend/services/team_service.py @@ -3,7 +3,7 @@ from markdown import markdown from backend import create_app, db -from backend.exceptions import NotFound +from backend.exceptions import NotFound, Forbidden from backend.models.dtos.team_dto import ( TeamDTO, NewTeamDTO, @@ -39,14 +39,6 @@ def __init__(self, message): current_app.logger.debug(message) -class TeamJoinNotAllowed(Exception): - """Custom Exception to notify bad user level on joining team""" - - def __init__(self, message): - if current_app: - current_app.logger.debug(message) - - class TeamService: @staticmethod def request_to_join_team(team_id: int, user_id: int): @@ -92,7 +84,11 @@ def add_user_to_team( ): is_manager = TeamService.is_user_team_manager(team_id, requesting_user) if not is_manager: - raise TeamServiceError("User is not allowed to add member to the team") + raise Forbidden( + sub_code="USER_NOT_TEAM_MANAGER", + team_id=team_id, + user_id=requesting_user, + ) team = TeamService.get_team_by_id(team_id) from_user = UserService.get_user_by_id(requesting_user) to_user = UserService.get_user_by_username(username) @@ -467,7 +463,9 @@ def update_team(team_dto: TeamDTO) -> Team: return team @staticmethod - def assert_validate_organisation(org_id: int): + def assert_validate_organisation( + org_id: int, + ): # FLAGGED: IS THIS FUNCTION REQUIRED? """Makes sure an organisation exists""" try: OrganisationService.get_organisation_by_id(org_id) diff --git a/backend/services/users/user_service.py b/backend/services/users/user_service.py index a2c2550874..f4f7e00467 100644 --- a/backend/services/users/user_service.py +++ b/backend/services/users/user_service.py @@ -4,7 +4,7 @@ from sqlalchemy.sql.expression import literal from sqlalchemy import func, or_, desc, and_, distinct, cast, Time, column -from backend.exceptions import NotFound +from backend.exceptions import NotFound, Forbidden from backend import db from backend.models.dtos.project_dto import ProjectFavoritesDTO, ProjectSearchResultsDTO from backend.models.dtos.user_dto import ( @@ -679,7 +679,7 @@ def add_role_to_user(admin_user_id: int, username: str, role: str): :param role: The requested role :raises UserServiceError """ - try: + try: # FLAGGED CHECK IN DTO requested_role = UserRole[role.upper()] except KeyError: raise UserServiceError( @@ -691,10 +691,7 @@ def add_role_to_user(admin_user_id: int, username: str, role: str): admin_role = UserRole(admin.role) if admin_role != UserRole.ADMIN and requested_role == UserRole.ADMIN: - raise UserServiceError( - "NeedAdminRole- You must be an Admin to assign Admin role" - ) - + raise Forbidden(sub_code="USER_NOT_ADMIN", user_id=admin_user_id) user = UserService.get_user_by_username(username) user.set_user_role(requested_role) diff --git a/backend/services/validator_service.py b/backend/services/validator_service.py index 598839c993..7214fcda8c 100644 --- a/backend/services/validator_service.py +++ b/backend/services/validator_service.py @@ -1,7 +1,7 @@ from flask import current_app from sqlalchemy import text -from backend.exceptions import NotFound +from backend.exceptions import NotFound, Forbidden from backend.models.dtos.mapping_dto import TaskDTOs from backend.models.dtos.stats_dto import Pagination from backend.models.dtos.validator_dto import ( @@ -67,7 +67,7 @@ def lock_tasks_for_validation(validation_dto: LockForValidationDTO) -> TaskDTOs: ]: raise ValidatorServiceError( f"NotReadyForValidation- Task {task_id} is not MAPPED, BADIMAGERY or INVALIDATED" - ) + ) # FLAGGED STATUS CODE: 409 user_can_validate = ValidatorService._user_can_validate_task( validation_dto.user_id, task.mapped_by ) @@ -75,7 +75,7 @@ def lock_tasks_for_validation(validation_dto: LockForValidationDTO) -> TaskDTOs: raise ValidatorServiceError( "CannotValidateMappedTask-" + "Tasks cannot be validated by the same user who marked task as mapped or badimagery" - ) + ) # FLAGGED STATUS CODE: 409 tasks_to_lock.append(task) @@ -89,17 +89,19 @@ def lock_tasks_for_validation(validation_dto: LockForValidationDTO) -> TaskDTOs: elif error_reason == ValidatingNotAllowed.USER_NOT_ON_ALLOWED_LIST: raise ValidatorServiceError( "UserNotAllowed- Validation not allowed because: User not on allowed list" - ) + ) # FLAGGED STATUS CODE: 403 elif error_reason == ValidatingNotAllowed.PROJECT_NOT_PUBLISHED: - raise ValidatorServiceError( - "ProjectNotPublished- Validation not allowed because: Project not published" + raise Forbidden( + sub_code="DRAFT_PROJECT_NOT_ALLOWED", + project_id=validation_dto.project_id, + user_id=validation_dto.user_id, ) elif error_reason == ValidatingNotAllowed.USER_ALREADY_HAS_TASK_LOCKED: user_tasks = Task.get_locked_tasks_for_user(validation_dto.user_id) if set(user_tasks.locked_tasks) != set(validation_dto.task_ids): raise ValidatorServiceError( "UserAlreadyHasTaskLocked- User already has a task locked" - ) + ) # FLAGGED FOR STATUS CODE: 409 else: raise ValidatorServiceError( f"ValidtionNotAllowed- Validation not allowed because: {error_reason}" @@ -269,12 +271,12 @@ def get_tasks_locked_by_user(project_id: int, unlock_tasks, user_id: int): if current_state != TaskStatus.LOCKED_FOR_VALIDATION: raise ValidatorServiceError( f"NotLockedForValidation- Task {unlock_task.task_id} is not LOCKED_FOR_VALIDATION" - ) + ) # FLAGGED STATUS CODE: 409 if task.locked_by != user_id: raise ValidatorServiceError( "TaskNotOwned- Attempting to unlock a task owned by another user" - ) + ) # FLAGGED STATUS CODE: 409 if hasattr(unlock_task, "status"): # we know what status we ate going to be setting to on unlock @@ -435,6 +437,8 @@ def revert_user_tasks(revert_dto: RevertUserTasksDTO): revert_dto.preferred_locale, ) else: - raise ValidatorServiceError( - "UserActionNotPermitted- User not permitted to revert tasks" + raise Forbidden( + sub_code="USER_NOT_PROJECT_MANAGER", + project_id=revert_dto.project_id, + user_id=revert_dto.action_by, ) From 176578fc189db9eca7495fc63fa1f459f39a42aa Mon Sep 17 00:00:00 2001 From: Aadesh-Baral Date: Thu, 24 Aug 2023 14:42:27 +0545 Subject: [PATCH 02/10] Fix test cases after using Forbidden exception class to raise 403 errors --- .../api/campaigns/test_resources.py | 21 ++++--------- .../api/comments/test_resources.py | 15 ++++------ .../api/interests/test_resources.py | 28 +++++------------ .../api/notifications/test_resources.py | 8 ++--- .../api/organisations/test_campaigns.py | 14 +++------ .../api/organisations/test_resources.py | 17 ++++------- .../api/projects/test_resources.py | 14 +++++---- .../integration/api/system/test_banner.py | 2 +- .../integration/api/tasks/test_actions.py | 20 ++++++++----- .../integration/api/teams/test_actions.py | 30 +++++-------------- .../integration/api/teams/test_resources.py | 22 +++++--------- .../integration/api/users/test_actions.py | 4 +-- .../services/messaging/test_chat_service.py | 8 ++--- .../services/test_project_admin_service.py | 11 +++---- .../services/test_project_service.py | 14 ++++----- .../services/test_validation_service.py | 4 +-- 16 files changed, 92 insertions(+), 140 deletions(-) diff --git a/tests/backend/integration/api/campaigns/test_resources.py b/tests/backend/integration/api/campaigns/test_resources.py index 0e2cfbf154..13a774efda 100644 --- a/tests/backend/integration/api/campaigns/test_resources.py +++ b/tests/backend/integration/api/campaigns/test_resources.py @@ -85,12 +85,9 @@ def test_update_existent_campaign_by_non_admin_fails(self): }, headers={"Authorization": self.non_admin_token}, ) - response_body = response.get_json() + response_body = response.get_json()["error"] self.assertEqual(response.status_code, 403) - self.assertEqual( - response_body["Error"], "CampaignsRestAPI PATCH: User not a Org Manager" - ) - self.assertEqual(response_body["SubCode"], "UserNotPermitted") + self.assertEqual(response_body["sub_code"], "USER_NOT_ORG_MANAGER") def test_update_existent_campaign_by_unauthenticated_user_fails(self): """ @@ -165,12 +162,9 @@ def test_delete_campaign_by_non_admin_fails(self): f"{self.endpoint_url}{self.test_campaign.id}/", headers={"Authorization": self.non_admin_token}, ) - response_body = response.get_json() + response_body = response.get_json()["error"] self.assertEqual(response.status_code, 403) - self.assertEqual( - response_body["Error"], "CampaignsRestAPI DELETE: User not a Org Manager" - ) - self.assertEqual(response_body["SubCode"], "UserNotPermitted") + self.assertEqual(response_body["sub_code"], "USER_NOT_ORG_MANAGER") def test_delete_campaign_by_unauthenticated_user_fails(self): """ @@ -261,12 +255,9 @@ def test_create_new_campaign_by_non_admin_fails(self): }, headers={"Authorization": non_admin_token}, ) - response_body = response.get_json() + response_body = response.get_json()["error"] self.assertEqual(response.status_code, 403) - self.assertEqual( - response_body["Error"], "CampaignsAllAPI POST: User not a Org Manager" - ) - self.assertEqual(response_body["SubCode"], "UserNotPermitted") + self.assertEqual(response_body["sub_code"], "USER_NOT_ORG_MANAGER") def test_create_new_campaign_by_unauthenticated_user_fails(self): """ diff --git a/tests/backend/integration/api/comments/test_resources.py b/tests/backend/integration/api/comments/test_resources.py index 4bb5603b00..9076049c7a 100644 --- a/tests/backend/integration/api/comments/test_resources.py +++ b/tests/backend/integration/api/comments/test_resources.py @@ -53,10 +53,9 @@ def test_post_comment_to_project_chat_by_blocked_user_fails(self): headers={"Authorization": generate_encoded_token(self.test_user.id)}, json={"message": TEST_MESSAGE}, ) - response_body = response.get_json() + response_body = response.get_json()["error"] self.assertEqual(response.status_code, 403) - self.assertEqual(response_body["Error"], "User is on read only mode") - self.assertEqual(response_body["SubCode"], "ReadOnly") + self.assertEqual(response_body["sub_code"], "USER_BLOCKED") def test_invalid_request_to_post_comment_to_project_chat_fails(self): """ @@ -191,10 +190,9 @@ def test_returns_403_if_user_not_allowed_to_delete_comment(self): self.endpoint_url, headers={"Authorization": self.test_user_token} ) # Assert - response_body = response.get_json() + response_body = response.get_json()["error"] self.assertEqual(response.status_code, 403) - self.assertEqual(response_body["Error"], " User not allowed to delete message") - self.assertEqual(response_body["SubCode"], "DeletePermissionError") + self.assertEqual(response_body["sub_code"], "COMMENT_NOT_OF_USER") def test_comment_can_be_deleted_by_author(self): """Test that endpoint returns 200 when deleting a comment of a project by the comment author""" @@ -267,10 +265,9 @@ def test_post_comment_to_task_chat_by_blocked_user_fails(self): headers={"Authorization": generate_encoded_token(self.test_user.id)}, json={"comment": TEST_MESSAGE}, ) - response_body = response.get_json() + response_body = response.get_json()["error"] self.assertEqual(response.status_code, 403) - self.assertEqual(response_body["Error"], "User is on read only mode") - self.assertEqual(response_body["SubCode"], "ReadOnly") + self.assertEqual(response_body["sub_code"], "USER_BLOCKED") def test_post_comment_to_task_chat_using_an_invalid_request_fails(self): """ diff --git a/tests/backend/integration/api/interests/test_resources.py b/tests/backend/integration/api/interests/test_resources.py index 2fd7c4d885..5e90670309 100644 --- a/tests/backend/integration/api/interests/test_resources.py +++ b/tests/backend/integration/api/interests/test_resources.py @@ -42,12 +42,9 @@ def test_create_a_new_interest_by_non_admin_fails(self): headers={"Authorization": self.session_token}, json={"name": NEW_INTEREST_NAME}, ) - response_body = response.get_json() + response_body = response.get_json()["error"] self.assertEqual(response.status_code, 403) - self.assertEqual( - response_body["Error"], "InterestsAllAPI POST: User not a Org Manager" - ) - self.assertEqual(response_body["SubCode"], "UserNotPermitted") + self.assertEqual(response_body["sub_code"], "USER_NOT_ORG_MANAGER") def test_create_a_new_interest_using_invalid_data_fails(self): """ @@ -152,12 +149,9 @@ def test_get_an_interest_by_non_admin_fails(self): self.endpoint_url, headers={"Authorization": self.session_token}, ) - response_body = response.get_json() + response_body = response.get_json()["error"] self.assertEqual(response.status_code, 403) - self.assertEqual( - response_body["Error"], "InterestsRestAPI GET: User not a Org Manager" - ) - self.assertEqual(response_body["SubCode"], "UserNotPermitted") + self.assertEqual(response_body["sub_code"], "USER_NOT_ORG_MANAGER") def test_get_a_non_existent_interest_fails(self): """ @@ -210,12 +204,9 @@ def test_update_an_interest_by_non_admin_fails(self): headers={"Authorization": self.session_token}, json={"name": NEW_INTEREST_NAME}, ) - response_body = response.get_json() + response_body = response.get_json()["error"] self.assertEqual(response.status_code, 403) - self.assertEqual( - response_body["Error"], "InterestsRestAPI PATCH: User not a Org Manager" - ) - self.assertEqual(response_body["SubCode"], "UserNotPermitted") + self.assertEqual(response_body["sub_code"], "USER_NOT_ORG_MANAGER") def test_update_a_non_existent_interest_fails(self): """ @@ -297,12 +288,9 @@ def test_delete_an_interest_by_non_admin_fails(self): self.endpoint_url, headers={"Authorization": self.session_token}, ) - response_body = response.get_json() + response_body = response.get_json()["error"] self.assertEqual(response.status_code, 403) - self.assertEqual( - response_body["Error"], "InterestsRestAPI DELETE: User not a Org Manager" - ) - self.assertEqual(response_body["SubCode"], "UserNotPermitted") + self.assertEqual(response_body["sub_code"], "USER_NOT_ORG_MANAGER") def test_delete_a_non_existent_interest_fails(self): """ diff --git a/tests/backend/integration/api/notifications/test_resources.py b/tests/backend/integration/api/notifications/test_resources.py index 07f0d55417..0bccb57fce 100644 --- a/tests/backend/integration/api/notifications/test_resources.py +++ b/tests/backend/integration/api/notifications/test_resources.py @@ -55,9 +55,9 @@ def test_get_message_returns_403(self): self.url, headers={"Authorization": self.test_sender_token}, ) - response_body = response.get_json() + response_body = response.get_json()["error"] self.assertEqual(response.status_code, 403) - self.assertEqual(response_body["SubCode"], "AccessOtherUserMessage") + self.assertEqual(response_body["sub_code"], "MESSAGE_NOT_FOR_USER") def test_get_message_returns_404(self): """ @@ -104,9 +104,9 @@ def test_delete_message_returns_403(self): self.url, headers={"Authorization": self.test_sender_token}, ) - response_body = response.get_json() + response_body = response.get_json()["error"] self.assertEqual(response.status_code, 403) - self.assertEqual(response_body["SubCode"], "AccessOtherUserMessage") + self.assertEqual(response_body["sub_code"], "MESSAGE_NOT_FOR_USER") def test_delete_message_returns_404(self): """ diff --git a/tests/backend/integration/api/organisations/test_campaigns.py b/tests/backend/integration/api/organisations/test_campaigns.py index b0e1aeb861..337a503e94 100644 --- a/tests/backend/integration/api/organisations/test_campaigns.py +++ b/tests/backend/integration/api/organisations/test_campaigns.py @@ -96,12 +96,9 @@ def test_non_org_admin_assigns_new_campaign_to_org_fails(self): }, headers={"Authorization": self.test_user_session_token}, ) - response_body = response.get_json() + response_body = response.get_json()["error"] self.assertEqual(response.status_code, 403) - self.assertEqual( - response_body["Error"], "User is not a manager of the organisation" - ) - self.assertEqual(response_body["SubCode"], "UserNotPermitted") + self.assertEqual(response_body["sub_code"], "USER_NOT_ORG_MANAGER") # get def test_get_organisation_campaigns_passes(self): @@ -142,12 +139,9 @@ def test_delete_organisation_campaign_non_admin_fails(self): f"/api/v2/organisations/{self.test_org.id}/campaigns/{self.test_campaign.id}/", headers={"Authorization": self.test_user_session_token}, ) - response_body = response.get_json() + response_body = response.get_json()["error"] self.assertEqual(response.status_code, 403) - self.assertEqual( - response_body["Error"], "User is not a manager of the organisation" - ) - self.assertEqual(response_body["SubCode"], "UserNotPermitted") + self.assertEqual(response_body["sub_code"], "USER_NOT_ORG_MANAGER") def test_delete_non_existent_organisation_campaign_fails(self): """Tests that the endpoint returns 404 when the org admin deletes a non existent campaign""" diff --git a/tests/backend/integration/api/organisations/test_resources.py b/tests/backend/integration/api/organisations/test_resources.py index 8aec9f9f65..cab3529b0c 100644 --- a/tests/backend/integration/api/organisations/test_resources.py +++ b/tests/backend/integration/api/organisations/test_resources.py @@ -208,12 +208,9 @@ def test_create_org_with_non_admin_fails(self): "managers": [TEST_USERNAME], }, ) - response_body = response.get_json() + response_body = response.get_json()["error"] self.assertEqual(response.status_code, 403) - self.assertEqual( - response_body["Error"], "Only admin users can create organisations." - ) - self.assertEqual(response_body["SubCode"], "OnlyAdminAccess") + self.assertEqual(response_body["sub_code"], "USER_NOT_ADMIN") # get organisation def test_get_org_when_omitManagerList_is_false_passes(self): @@ -271,10 +268,9 @@ def test_delete_item_by_non_admin_user_fails(self): f"/api/v2/organisations/{self.test_org.id}/", headers={"Authorization": non_admin_token}, ) - response_body = response.get_json() + response_body = response.get_json()["error"] self.assertEqual(response.status_code, 403) - self.assertEqual(response_body["Error"], "User is not an admin for the org") - self.assertEqual(response_body["SubCode"], "UserNotOrgAdmin") + self.assertEqual(response_body["sub_code"], "USER_NOT_ORG_MANAGER") def test_delete_org_with_projects_fails(self): """ @@ -327,10 +323,9 @@ def test_update_org_details_by_non_admin_fails(self): "managers": [TEST_USERNAME], }, ) - response_body = response.get_json() + response_body = response.get_json()["error"] self.assertEqual(response.status_code, 403) - self.assertEqual(response_body["Error"], "User is not an admin for the org") - self.assertEqual(response_body["SubCode"], "UserNotOrgAdmin") + self.assertEqual(response_body["sub_code"], "USER_NOT_ORG_MANAGER") def test_update_org_details_with_invalid_data_fails(self): """ diff --git a/tests/backend/integration/api/projects/test_resources.py b/tests/backend/integration/api/projects/test_resources.py index 5cbf1b267d..0e152963e5 100644 --- a/tests/backend/integration/api/projects/test_resources.py +++ b/tests/backend/integration/api/projects/test_resources.py @@ -89,7 +89,7 @@ def test_delete_project_returns_403_on_if_request_by_unauthorized_user(self): ) # Assert self.assertEqual(response.status_code, 403) - self.assertEqual(response.json["SubCode"], "UserPermissionError") + self.assertEqual(response.json["error"]["sub_code"], "USER_NOT_PROJECT_MANAGER") def test_project_with_mapped_tasks_cannot_be_deleted(self): "Test project with mapped tasks cannot be deleted" @@ -187,7 +187,7 @@ def test_create_project_returns_403_on_if_request_by_unauthorized_user(self): ) # Assert self.assertEqual(response.status_code, 403) - self.assertEqual(response.json["SubCode"], "NotPermittedToCreate") + self.assertEqual(response.json["error"]["sub_code"], "USER_NOT_ORG_MANAGER") def test_create_project_returns_400_if_invalid_json(self): "Test returns 400 if invalid json" @@ -369,7 +369,9 @@ def test_draft_project_can_only_be_accessed_by_user_with_PM_permissions(self): ) # Assert self.assertEqual(response.status_code, 403) - self.assertEqual(response.json["SubCode"], "ProjectNotFetched") + self.assertEqual( + response.json["error"]["sub_code"], "DRAFT_PROJECT_NOT_ALLOWED" + ) # Authenticated user with that is a admin self.test_user.role = UserRole.ADMIN.value @@ -404,7 +406,9 @@ def test_private_project_requires_atleast_PM_permissions(self): ) # Assert self.assertEqual(response.status_code, 403) - self.assertEqual(response.json["SubCode"], "PrivateProject") + self.assertEqual( + response.json["error"]["sub_code"], "PRIVATE_PROJECT_NOT_ALLOWED" + ) # Authenticated user with that is a admin # Arrange @@ -539,7 +543,7 @@ def test_patch_project_returns_403_if_user_is_not_project_manager(self): ) # Assert self.assertEqual(response.status_code, 403) - self.assertEqual(response.json["SubCode"], "UserPermissionError") + self.assertEqual(response.json["error"]["sub_code"], "USER_NOT_PROJECT_MANAGER") def test_patch_project_returns_400_if_invalid_body(self): "Test patch project returns 400 if invalid body." diff --git a/tests/backend/integration/api/system/test_banner.py b/tests/backend/integration/api/system/test_banner.py index 426397de5c..1deace790b 100644 --- a/tests/backend/integration/api/system/test_banner.py +++ b/tests/backend/integration/api/system/test_banner.py @@ -48,7 +48,7 @@ def test_patch_banner(self): ) # Assert self.assertEqual(response.status_code, 403) - self.assertEqual(response.json["SubCode"], "OnlyAdminAccess") + self.assertEqual(response.json["error"]["sub_code"], "USER_NOT_ADMIN") # Test returns 200 OK for admin users test_user.role = 1 diff --git a/tests/backend/integration/api/tasks/test_actions.py b/tests/backend/integration/api/tasks/test_actions.py index 4d7bb034aa..8bbd1411e3 100644 --- a/tests/backend/integration/api/tasks/test_actions.py +++ b/tests/backend/integration/api/tasks/test_actions.py @@ -58,7 +58,7 @@ def test_map_all_tasks_returns_403_for_non_PM_role_request(self, mock_pm_role): ) # Assert self.assertEqual(response.status_code, 403) - self.assertEqual(response.json["SubCode"], "UserPermissionError") + self.assertEqual(response.json["error"]["sub_code"], "USER_NOT_PROJECT_MANAGER") @patch.object(ProjectAdminService, "is_user_action_permitted_on_project") def test_map_all_tasks_is_allowed_for_user_with_pm_role(self, mock_pm_role): @@ -116,7 +116,7 @@ def test_validate_all_tasks_returns_403_for_non_PM_role_request(self, mock_pm_ro ) # Assert self.assertEqual(response.status_code, 403) - self.assertEqual(response.json["SubCode"], "UserPermissionError") + self.assertEqual(response.json["error"]["sub_code"], "USER_NOT_PROJECT_MANAGER") @patch.object(ProjectAdminService, "is_user_action_permitted_on_project") def test_validate_all_tasks_is_allowed_for_user_with_pm_role(self, mock_pm_role): @@ -173,7 +173,7 @@ def test_invalidate_all_tasks_returns_403_for_non_PM_role_request( ) # Assert self.assertEqual(response.status_code, 403) - self.assertEqual(response.json["SubCode"], "UserPermissionError") + self.assertEqual(response.json["error"]["sub_code"], "USER_NOT_PROJECT_MANAGER") @patch.object(ProjectAdminService, "is_user_action_permitted_on_project") def test_invalidate_all_tasks_is_allowed_for_user_with_pm_role(self, mock_pm_role): @@ -233,7 +233,7 @@ def test_reset_all_badimagery_tasks_returns_403_for_non_PM_role_request( ) # Assert self.assertEqual(response.status_code, 403) - self.assertEqual(response.json["SubCode"], "UserPermissionError") + self.assertEqual(response.json["error"]["sub_code"], "USER_NOT_PROJECT_MANAGER") @patch.object(ProjectAdminService, "is_user_action_permitted_on_project") def test_reset_all_badimagery_tasks_is_allowed_for_user_with_pm_role( @@ -287,7 +287,7 @@ def test_reset_all_tasks_returns_403_for_non_PM_role_request(self, mock_pm_role) ) # Assert self.assertEqual(response.status_code, 403) - self.assertEqual(response.json["SubCode"], "UserPermissionError") + self.assertEqual(response.json["error"]["sub_code"], "USER_NOT_PROJECT_MANAGER") @patch.object(ProjectAdminService, "is_user_action_permitted_on_project") def test_reset_all_tasks_is_allowed_for_user_with_pm_role(self, mock_pm_role): @@ -381,7 +381,9 @@ def test_mapping_lock_returns_403_for_if_user_not_allowed_to_map( ) # Assert self.assertEqual(response.status_code, 403) - self.assertEqual(response.json["SubCode"], "ProjectNotPublished") + self.assertEqual( + response.json["error"]["sub_code"], "DRAFT_PROJECT_NOT_ALLOWED" + ) @patch.object(ProjectAdminService, "is_user_action_permitted_on_project") def test_mapping_lock_returns_403_if_task_in_invalid_state_for_mapping( @@ -823,7 +825,9 @@ def test_validation_lock_returns_403_if_user_not_permitted_to_validate(self): ) # Assert self.assertEqual(response.status_code, 403) - self.assertEqual(response.json["SubCode"], "ProjectNotPublished") + self.assertEqual( + response.json["error"]["sub_code"], "DRAFT_PROJECT_NOT_ALLOWED" + ) def test_validation_lock_returns_409_if_user_hasnt_accepted_license(self): """Test returns 409 if user hasn't accepted license.""" @@ -1660,7 +1664,7 @@ def test_returns_403_if_user_doesnot_have_PM_permission(self): ) # Assert self.assertEqual(response.status_code, 403) - self.assertEqual(response.json["SubCode"], "UserActionNotPermitted") + self.assertEqual(response.json["error"]["sub_code"], "USER_NOT_PROJECT_MANAGER") def set_task_status(self, task, status, user_id): """Set task status.""" diff --git a/tests/backend/integration/api/teams/test_actions.py b/tests/backend/integration/api/teams/test_actions.py index 931ba0412c..00d3ce3cde 100644 --- a/tests/backend/integration/api/teams/test_actions.py +++ b/tests/backend/integration/api/teams/test_actions.py @@ -136,13 +136,9 @@ def test_handle_join_request_by_non_admin_fails(self): "username": self.test_user.username, }, ) - response_body = response.get_json() + response_body = response.get_json()["error"] self.assertEqual(response.status_code, 403) - self.assertEqual( - response_body["Error"], - "You don't have permissions to approve this join team request", - ) - self.assertEqual(response_body["SubCode"], "ApproveJoinError") + self.assertEqual(response_body["sub_code"], "USER_NOT_TEAM_MANAGER") def test_handle_join_request_to_non_existent_team_fails(self): """ @@ -238,12 +234,9 @@ def test_add_members_to_team_by_non_admin_fails(self): headers={"Authorization": self.session_token}, ) response_body = response.get_json() - self.assertEqual(response.status_code, 500) + self.assertEqual(response.status_code, 403) error_resp = response_body["error"] - self.assertEqual(error_resp["sub_code"], "INTERNAL_SERVER_ERROR") - self.assertTrue( - "User is not allowed to add member to the team" in error_resp["message"] - ) + self.assertEqual(error_resp["sub_code"], "USER_NOT_TEAM_MANAGER") def test_add_non_existent_members_to_team_fails(self): """ @@ -342,13 +335,9 @@ def test_remove_members_from_team_by_non_admin_fails(self): }, headers={"Authorization": self.session_token}, ) - response_body = response.get_json() + response_body = response.get_json()["error"] self.assertEqual(response.status_code, 403) - self.assertEqual( - response_body["Error"], - f"You don't have permissions to remove {TEST_ADMIN_USERNAME} from this team.", - ) - self.assertEqual(response_body["SubCode"], "RemoveUserError") + self.assertEqual(response_body["sub_code"], "USER_NOT_TEAM_MANAGER") def test_remove_non_existent_members_from_team_fails(self): """ @@ -436,12 +425,9 @@ def test_message_team_members_by_non_admin_fails(self): json={"message": TEST_MESSAGE, "subject": TEST_SUBJECT}, headers={"Authorization": self.session_token}, ) - response_body = response.get_json() + response_body = response.get_json()["error"] self.assertEqual(response.status_code, 403) - self.assertEqual( - response_body["Error"], "Unauthorised to send message to team members" - ) - self.assertEqual(response_body["SubCode"], "UserNotPermitted") + self.assertEqual(response_body["sub_code"], "USER_NOT_TEAM_MANAGER") def test_message_team_members_with_invalid_data_fails(self): """ diff --git a/tests/backend/integration/api/teams/test_resources.py b/tests/backend/integration/api/teams/test_resources.py index 5ff3baeaa5..e2c772e514 100644 --- a/tests/backend/integration/api/teams/test_resources.py +++ b/tests/backend/integration/api/teams/test_resources.py @@ -93,12 +93,9 @@ def test_update_team_by_non_admin_fails(self): headers={"Authorization": self.test_user_token}, json={"name": NEW_TEAM_NAME}, ) - response_body = response.get_json() + response_body = response.get_json()["error"] self.assertEqual(response.status_code, 403) - self.assertEqual( - response_body["Error"], "User is not a admin or a manager for the team" - ) - self.assertEqual(response_body["SubCode"], "UserNotTeamManager") + self.assertEqual(response_body["sub_code"], "USER_NOT_TEAM_MANAGER") def test_update_team_with_invalid_data_fails(self): """ @@ -140,10 +137,9 @@ def test_delete_team_by_non_admin_fails(self): response = self.client.delete( self.endpoint_url, headers={"Authorization": self.test_user_token} ) - response_body = response.get_json() - self.assertEqual(response.status_code, 401) - self.assertEqual(response_body["Error"], "User is not a manager for the team") - self.assertEqual(response_body["SubCode"], "UserNotTeamManager") + response_body = response.get_json()["error"] + self.assertEqual(response.status_code, 403) + self.assertEqual(response_body["sub_code"], "USER_NOT_TEAM_MANAGER") def test_delete_non_existent_team_by_id_fails(self): """ @@ -209,13 +205,9 @@ def test_create_new_team_non_admin_fails(self): }, headers={"Authorization": self.non_admin_token}, ) - response_body = response.get_json() + response_body = response.get_json()["error"] self.assertEqual(response.status_code, 403) - self.assertEqual( - response_body["Error"], - "User not permitted to create team for the Organisation", - ) - self.assertEqual(response_body["SubCode"], "CreateTeamNotPermitted") + self.assertEqual(response_body["sub_code"], "USER_NOT_ORG_MANAGER") def test_create_new_team_existent_org_passes(self): """ diff --git a/tests/backend/integration/api/users/test_actions.py b/tests/backend/integration/api/users/test_actions.py index 0dcc76cfba..99def8c212 100644 --- a/tests/backend/integration/api/users/test_actions.py +++ b/tests/backend/integration/api/users/test_actions.py @@ -257,7 +257,7 @@ def test_returns_403_if_user_not_admin(self): ) # Assert self.assertEqual(response.status_code, 403) - self.assertEqual(response.json["SubCode"], "NeedAdminRole") + self.assertEqual(response.json["error"]["sub_code"], "USER_NOT_ADMIN") def test_returns_403_if_unknown_user_role(self): """Test API returns 403 if unknown user role is provided""" @@ -267,7 +267,7 @@ def test_returns_403_if_unknown_user_role(self): headers={"Authorization": self.admin_session_token}, ) # Assert - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 400) self.assertEqual(response.json["SubCode"], "UnknownAddRole") def test_returns_404_if_user_not_found(self): diff --git a/tests/backend/integration/services/messaging/test_chat_service.py b/tests/backend/integration/services/messaging/test_chat_service.py index 1a52724718..a787640f55 100644 --- a/tests/backend/integration/services/messaging/test_chat_service.py +++ b/tests/backend/integration/services/messaging/test_chat_service.py @@ -1,7 +1,7 @@ import threading from unittest.mock import patch -from backend.exceptions import NotFound +from backend.exceptions import NotFound, Forbidden from backend.models.postgis.statuses import ProjectStatus from backend.models.postgis.team import TeamRoles, TeamMemberFunctions from tests.backend.base import BaseTestCase @@ -44,7 +44,7 @@ def test_post_message_raises_error_if_user_not_manager_in_draft_project(self): self.chat_dto.user_id = self.test_user.id self.chat_dto.username = self.test_user.username # Act/Assert - with self.assertRaises(ValueError): + with self.assertRaises(Forbidden): ChatService.post_message( self.chat_dto, self.canned_project.id, self.test_user.id ) @@ -81,7 +81,7 @@ def test_post_message_raises_error_if_user_not_member_of_allowed_team_in_private self.canned_project.status = ProjectStatus.PUBLISHED.value self.canned_project.private = True # Act/Assert - with self.assertRaises(ValueError): + with self.assertRaises(Forbidden): ChatService.post_message( self.chat_dto, self.canned_project.id, self.test_user.id ) @@ -143,7 +143,7 @@ def test_delete_project_chat_by_id_raises_error_if_user_is_not_comment_author_an ) comment = comments.chat[0] # Act/Assert - with self.assertRaises(ValueError): + with self.assertRaises(Forbidden): ChatService.delete_project_chat_by_id( self.canned_project.id, comment.id, self.test_user.id ) diff --git a/tests/backend/integration/services/test_project_admin_service.py b/tests/backend/integration/services/test_project_admin_service.py index e5515a6704..8f88a004fd 100644 --- a/tests/backend/integration/services/test_project_admin_service.py +++ b/tests/backend/integration/services/test_project_admin_service.py @@ -2,7 +2,8 @@ import json from flask import current_app -from backend.models.postgis.project import Project, User, NotFound, ProjectPriority +from backend.exceptions import NotFound, Forbidden +from backend.models.postgis.project import Project, User, ProjectPriority from backend.models.postgis.statuses import UserRole, ProjectDifficulty from backend.services.messaging.message_service import MessageService from backend.services.team_service import TeamService @@ -48,7 +49,7 @@ def test_create_draft_project_raises_error_if_user_not_admin_or_org_manager( mock_user_get.return_value = return_canned_user() # Act/Assert - with self.assertRaises(ProjectAdminServiceError): + with self.assertRaises(Forbidden): ProjectAdminService.create_draft_project(draft_project_dto) @patch.object(UserService, "is_user_an_admin") @@ -384,7 +385,7 @@ def test_updating_a_project_with_different_roles_raises_error( stub_user.role = UserRole.MAPPER.value mock_user.return_value = stub_user # Act/Assert - with self.assertRaises(ValueError): + with self.assertRaises(Forbidden): ProjectAdminService.update_project(dto, mock_user.id) def test_updating_a_project_with_valid_project_info(self): @@ -438,7 +439,7 @@ def test_project_transfer_to_(self, mock_send_message): current_app.logger.debug( "Testing error is raised if initiating user is not permitted to transfer project" ) - with self.assertRaises(ProjectAdminServiceError): + with self.assertRaises(Forbidden): ProjectAdminService.transfer_project_to( test_project.id, test_user.id, test_manager.username ) @@ -447,7 +448,7 @@ def test_project_transfer_to_(self, mock_send_message): current_app.logger.debug( "Testing error is raised if transferred to user who is not a manager of the organisation" ) - with self.assertRaises(ValueError): + with self.assertRaises(Forbidden): ProjectAdminService.transfer_project_to( test_project.id, test_manager.id, test_user.username ) diff --git a/tests/backend/integration/services/test_project_service.py b/tests/backend/integration/services/test_project_service.py index 615b532d5c..e671d1bc53 100644 --- a/tests/backend/integration/services/test_project_service.py +++ b/tests/backend/integration/services/test_project_service.py @@ -1,8 +1,9 @@ from unittest.mock import patch +from backend.exceptions import Forbidden from backend.models.postgis.statuses import ProjectStatus, UserRole from backend.services.project_admin_service import ProjectAdminService -from backend.services.project_service import ProjectService, ProjectServiceError +from backend.services.project_service import ProjectService from backend.services.team_service import TeamService from tests.backend.base import BaseTestCase from tests.backend.helpers.test_helpers import create_canned_project, return_canned_user @@ -31,7 +32,7 @@ def test_get_project_dto_for_mapper_returns_project_dto(self): def test_get_project_dto_for_mapper_raises_error_if_draft_project(self): # Project status is already set as draft while creating test project so no need to change it's status # Act/Assert - with self.assertRaises(ProjectServiceError): + with self.assertRaises(Forbidden): ProjectService.get_project_dto_for_mapper( self.test_project.id, self.test_mapper.id ) @@ -42,11 +43,10 @@ def test_get_project_dto_for_mapper_returns_none_if_private_project(self): self.test_project.status = ProjectStatus.PUBLISHED.value self.test_project.private = True # Act - project_dto = ProjectService.get_project_dto_for_mapper( - self.test_project.id, self.test_mapper.id - ) - # Assert - self.assertIsNone(project_dto) + with self.assertRaises(Forbidden): + ProjectService.get_project_dto_for_mapper( + self.test_project.id, self.test_mapper.id + ) def test_get_project_dto_for_mapper_returns_private_and_draft_project_dto_for_adimn( self, diff --git a/tests/backend/integration/services/test_validation_service.py b/tests/backend/integration/services/test_validation_service.py index dc8edd1452..2645fd558c 100644 --- a/tests/backend/integration/services/test_validation_service.py +++ b/tests/backend/integration/services/test_validation_service.py @@ -1,6 +1,6 @@ +from backend.exceptions import Forbidden from backend.services.validator_service import ( ValidatorService, - ValidatorServiceError, TaskStatus, Task, ) @@ -70,7 +70,7 @@ def test_revert_user_tasks_requires_user_with_PM_permission_for_successful_opera revert_dto.action_by = self.test_user.id revert_dto.action = "VALIDATED" # Act/Assert - with self.assertRaises(ValidatorServiceError): + with self.assertRaises(Forbidden): ValidatorService.revert_user_tasks(revert_dto) def test_revert_user_tasks_revert_validated_task_to_mapped_status(self): From ac5b7c0e4c7708a27075c2826fe4c735dc167059 Mon Sep 17 00:00:00 2001 From: Aadesh-Baral Date: Mon, 28 Aug 2023 13:17:58 +0545 Subject: [PATCH 03/10] Revise: Corrected 403 Status Code Return for Exceptions Unrelated to Permissions. --- backend/api/comments/resources.py | 7 ++--- backend/api/issues/resources.py | 2 -- backend/api/licenses/resources.py | 2 -- backend/api/organisations/resources.py | 15 ++++------ backend/api/projects/actions.py | 4 +-- backend/api/projects/resources.py | 18 ++++-------- backend/api/projects/teams.py | 10 ++++--- backend/api/system/authentication.py | 2 +- backend/api/tasks/actions.py | 36 ++++++++++++------------ backend/api/tasks/resources.py | 35 +++++++++++------------ backend/api/teams/actions.py | 14 ++++----- backend/error_messages.json | 3 +- backend/services/mapping_service.py | 6 ++-- backend/services/organisation_service.py | 6 ++-- backend/services/validator_service.py | 7 +++-- 15 files changed, 75 insertions(+), 92 deletions(-) diff --git a/backend/api/comments/resources.py b/backend/api/comments/resources.py index 4350558139..2c6f0e2cbb 100644 --- a/backend/api/comments/resources.py +++ b/backend/api/comments/resources.py @@ -229,11 +229,8 @@ def post(self, project_id, task_id): current_app.logger.error(f"Error validating request: {str(e)}") return {"Error": "Unable to add comment", "SubCode": "InvalidData"}, 400 - try: - task = MappingService.add_task_comment(task_comment) - return task.to_primitive(), 201 - except MappingServiceError: # FLAGGED: UNREACHABLE CODE - return {"Error": "Task update failed"}, 403 + task = MappingService.add_task_comment(task_comment) + return task.to_primitive(), 201 def get(self, project_id, task_id): """ diff --git a/backend/api/issues/resources.py b/backend/api/issues/resources.py index ac6939b906..3a60deadf6 100644 --- a/backend/api/issues/resources.py +++ b/backend/api/issues/resources.py @@ -7,8 +7,6 @@ ISSUE_NOT_FOUND = "Mapping-issue category not found" -# FLAGGED FOR DELETION - class IssuesRestAPI(Resource): def get(self, category_id): diff --git a/backend/api/licenses/resources.py b/backend/api/licenses/resources.py index e1dcb6df30..4b3e78066d 100644 --- a/backend/api/licenses/resources.py +++ b/backend/api/licenses/resources.py @@ -5,8 +5,6 @@ from backend.services.license_service import LicenseService from backend.services.users.authentication_service import token_auth, tm -# FLAGGED FOR PERMISSIONS REVIEW - class LicensesRestAPI(Resource): @tm.pm_only() diff --git a/backend/api/organisations/resources.py b/backend/api/organisations/resources.py index 4245248e02..c198c32aa8 100644 --- a/backend/api/organisations/resources.py +++ b/backend/api/organisations/resources.py @@ -2,7 +2,7 @@ from flask_restful import Resource, request, current_app from schematics.exceptions import DataError -from backend.exceptions import Forbidden +from backend.exceptions import Forbidden, Unauthorized from backend.models.dtos.organisation_dto import ( NewOrganisationDTO, UpdateOrganisationDTO, @@ -181,11 +181,11 @@ def delete(self, organisation_id): try: OrganisationService.delete_organisation(organisation_id) return {"Success": "Organisation deleted"}, 200 - except OrganisationServiceError: # FLAGGED: STATUS CODE + except OrganisationServiceError: return { "Error": "Organisation has some projects", "SubCode": "OrgHasProjects", - }, 403 + }, 409 @token_auth.login_required(optional=True) def get(self, organisation_id): @@ -316,8 +316,8 @@ def patch(self, organisation_id): try: OrganisationService.update_organisation(organisation_dto) return {"Status": "Updated"}, 200 - except OrganisationServiceError as e: # FLAGGED: STATUS CODE - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 402 + except OrganisationServiceError as e: + return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 409 class OrganisationsStatsAPI(Resource): @@ -408,10 +408,7 @@ def get(self): manager_user_id = None if manager_user_id is not None and not authenticated_user_id: - raise Forbidden( # FLAGGED: STATUS CODE 401? - sub_code="USER_NOT_AUTHENTICATED", - message="Filter by manager_user_id is not allowed to unauthenticated requests", - ) + raise Unauthorized(sub_code="UNAUTHORIZED_MANAGER_FILTER") # Validate abbreviated. omit_managers = bool(strtobool(request.args.get("omitManagerList", "false"))) diff --git a/backend/api/projects/actions.py b/backend/api/projects/actions.py index b2e7d30957..9fc3803d37 100644 --- a/backend/api/projects/actions.py +++ b/backend/api/projects/actions.py @@ -199,7 +199,7 @@ def post(self, project_id): return { "Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0], - }, 403 # FLAGGED FOR STATUS CODE: 409? + }, 409 class ProjectsActionsUnFeatureAPI(Resource): @@ -254,7 +254,7 @@ def post(self, project_id): return { "Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0], - }, 403 # FLAGGED FOR STATUS CODE: 409 + }, 403 class ProjectsActionsSetInterestsAPI(Resource): diff --git a/backend/api/projects/resources.py b/backend/api/projects/resources.py index 29fb8228d1..028bfd6c96 100644 --- a/backend/api/projects/resources.py +++ b/backend/api/projects/resources.py @@ -18,10 +18,7 @@ ProjectSearchServiceError, BBoxTooBigError, ) -from backend.services.project_service import ( - ProjectService, - ProjectServiceError, -) +from backend.services.project_service import ProjectService from backend.services.users.user_service import UserService from backend.services.organisation_service import OrganisationService from backend.services.users.authentication_service import token_auth @@ -397,7 +394,7 @@ def patch(self, project_id): return { "Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0], - }, 403 # FLAGGED FOR STATUS CODE + }, 400 @token_auth.login_required def delete(self, project_id): @@ -445,8 +442,8 @@ def delete(self, project_id): try: ProjectAdminService.delete_project(project_id, authenticated_user_id) return {"Success": "Project deleted"}, 200 - except ProjectAdminServiceError as e: # FLAGGED FOR STATUS CODE - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 + except ProjectAdminServiceError as e: + return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 409 class ProjectSearchBase(Resource): @@ -1076,11 +1073,8 @@ def get(self, project_id): 500: description: Internal Server Error """ - try: - priority_areas = ProjectService.get_project_priority_areas(project_id) - return priority_areas, 200 - except ProjectServiceError: # FLAGGED: NEVER REACHING CODE - return {"Error": "Unable to fetch project"}, 403 + priority_areas = ProjectService.get_project_priority_areas(project_id) + return priority_areas, 200 class ProjectsQueriesFeaturedAPI(Resource): diff --git a/backend/api/projects/teams.py b/backend/api/projects/teams.py index 9a20072ec8..e75fe06fbd 100644 --- a/backend/api/projects/teams.py +++ b/backend/api/projects/teams.py @@ -94,10 +94,12 @@ def post(self, team_id, project_id): """ authenticated_user = token_auth.current_user() if not TeamService.is_user_team_manager(team_id, authenticated_user): - return { - "Error": "User is not an admin or a manager for the team", - "SubCode": "UserPermissionError", - }, 401 # FLAGGED FOR STATUS CODE + raise Forbidden( + sub_code="USER_NOT_TEAM_MANAGER", + project_id=project_id, + team_id=team_id, + user_id=authenticated_user, + ) try: role = request.get_json(force=True)["role"] diff --git a/backend/api/system/authentication.py b/backend/api/system/authentication.py index 6363999daa..e88eba7aa0 100644 --- a/backend/api/system/authentication.py +++ b/backend/api/system/authentication.py @@ -164,4 +164,4 @@ def get(self): return { "Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0], - }, 403 # FLAGGED FOR STATUS CODE + }, 400 diff --git a/backend/api/tasks/actions.py b/backend/api/tasks/actions.py index 599b43a9af..0c0b542a3a 100644 --- a/backend/api/tasks/actions.py +++ b/backend/api/tasks/actions.py @@ -95,8 +95,8 @@ def post(self, project_id, task_id): ProjectService.exists(project_id) # Check if project exists task = MappingService.lock_task_for_mapping(lock_task_dto) return task.to_primitive(), 200 - except MappingServiceError as e: # FLAGGED FOR STATUS CODE - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 + except MappingServiceError as e: + return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 409 except UserLicenseError: return { "Error": "User not accepted license terms", @@ -181,8 +181,8 @@ def post(self, project_id, task_id): ProjectService.exists(project_id) # Check if project exists task = MappingService.stop_mapping_task(stop_task) return task.to_primitive(), 200 - except MappingServiceError as e: # FLAGGED FOR STATUS CODE - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 + except MappingServiceError as e: + return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 409 class TasksActionsMappingUnlockAPI(Resource): @@ -260,8 +260,8 @@ def post(self, project_id, task_id): ProjectService.exists(project_id) # Check if project exists task = MappingService.unlock_task_after_mapping(mapped_task) return task.to_primitive(), 200 - except MappingServiceError as e: # FLAGGED FOR STATUS CODE - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 + except MappingServiceError as e: + return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 409 except NotFound as e: # FLAGGED IF THIS CATCH IS NEEDED return e.to_dict() except Exception as e: # FLAGGED IF THIS CATCH IS NEEDED @@ -403,8 +403,8 @@ def post(self, project_id): ProjectService.exists(project_id) # Check if project exists tasks = ValidatorService.lock_tasks_for_validation(validator_dto) return tasks.to_primitive(), 200 - except ValidatorServiceError as e: # FLAGGED FOR STATUS CODE - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 + except ValidatorServiceError as e: + return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 409 except UserLicenseError: return { "Error": "User not accepted license terms", @@ -481,8 +481,8 @@ def post(self, project_id): ProjectService.exists(project_id) # Check if project exists tasks = ValidatorService.stop_validating_tasks(validated_dto) return tasks.to_primitive(), 200 - except ValidatorServiceError as e: # FLAGGED FOR STATUS CODE - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 + except ValidatorServiceError as e: + return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 409 class TasksActionsValidationUnlockAPI(Resource): @@ -553,8 +553,8 @@ def post(self, project_id): ProjectService.exists(project_id) # Check if project exists tasks = ValidatorService.unlock_tasks_after_validation(validated_dto) return tasks.to_primitive(), 200 - except ValidatorServiceError as e: # FLAGGED FOR STATUS CODE - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 + except ValidatorServiceError as e: + return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 409 class TasksActionsMapAllAPI(Resource): @@ -857,10 +857,10 @@ def post(self, project_id, task_id): ProjectService.exists(project_id) # Check if project exists tasks = SplitService.split_task(split_task_dto) return tasks.to_primitive(), 200 - except SplitServiceError as e: # FLAGGED FOR STATUS CODE - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 - except InvalidGeoJson as e: # FLAGGED FOR STATUS CODE - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 + except SplitServiceError as e: + return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 409 + except InvalidGeoJson as e: + return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 400 class TasksActionsExtendAPI(Resource): @@ -934,8 +934,8 @@ def post(self, project_id): ProjectService.exists(project_id) # Check if project exists MappingService.extend_task_lock_time(extend_dto) return {"Success": "Successfully extended task expiry"}, 200 - except MappingServiceError as e: # FLAGGED FOR STATUS CODE - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 + except MappingServiceError as e: + return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 409 class TasksActionsReverUserTaskstAPI(Resource): diff --git a/backend/api/tasks/resources.py b/backend/api/tasks/resources.py index 34ba83c2ec..1feaa6fa2b 100644 --- a/backend/api/tasks/resources.py +++ b/backend/api/tasks/resources.py @@ -95,28 +95,25 @@ def get(self, project_id): 500: description: Internal Server Error """ - try: - tasks = request.args.get("tasks") if request.args.get("tasks") else None - as_file = ( - strtobool(request.args.get("as_file")) - if request.args.get("as_file") - else True - ) + tasks = request.args.get("tasks") if request.args.get("tasks") else None + as_file = ( + strtobool(request.args.get("as_file")) + if request.args.get("as_file") + else True + ) - tasks_json = ProjectService.get_project_tasks(int(project_id), tasks) + tasks_json = ProjectService.get_project_tasks(int(project_id), tasks) - if as_file: - tasks_json = str(tasks_json).encode("utf-8") - return send_file( - io.BytesIO(tasks_json), - mimetype="application/json", - as_attachment=True, - download_name=f"{str(project_id)}-tasks.geojson", - ) + if as_file: + tasks_json = str(tasks_json).encode("utf-8") + return send_file( + io.BytesIO(tasks_json), + mimetype="application/json", + as_attachment=True, + download_name=f"{str(project_id)}-tasks.geojson", + ) - return tasks_json, 200 - except ProjectServiceError as e: # FLAGGED: CHECK IF THIS EXCEPTION IS RAISED - return {"Error": str(e)}, 403 + return tasks_json, 200 @token_auth.login_required def delete(self, project_id): diff --git a/backend/api/teams/actions.py b/backend/api/teams/actions.py index 6925d10263..06e1b4edff 100644 --- a/backend/api/teams/actions.py +++ b/backend/api/teams/actions.py @@ -338,12 +338,10 @@ def post(self, team_id): "Error": "Request payload did not match validation", "SubCode": "InvalidData", }, 400 - try: - threading.Thread( - target=TeamService.send_message_to_all_team_members, - args=(team_id, team.name, message_dto), - ).start() - return {"Success": "Message sent successfully"}, 200 - except ValueError as e: # FLAGGED: CHECK IS THIS EVER RAISED - return {"Error": str(e)}, 403 + threading.Thread( + target=TeamService.send_message_to_all_team_members, + args=(team_id, team.name, message_dto), + ).start() + + return {"Success": "Message sent successfully"}, 200 diff --git a/backend/error_messages.json b/backend/error_messages.json index a3c0a2e048..ba2d128dbf 100644 --- a/backend/error_messages.json +++ b/backend/error_messages.json @@ -21,7 +21,8 @@ "USER_NOT_IN_TEAM": "The requested user is not a member of the team.", "USER_NOT_FOUND": "The requested user was not found on the server.", "BAD_REQUEST": "The request was invalid or cannot be otherwise served.", - "UNAUTHORIZED": "Authentication credentials were missing or incorrect.", + "UNAUTHORIZED": "You are not authorized to perform this action. Please login and try again.", + "UNAUTHORIZED_MANAGER_FILTER": "You need to be authenticated to filter organisations by manager.", "FORBIDDEN": "The request is understood, but it has been refused or access is not allowed.", "USER_NOT_ADMIN": "This action is only allowed for system administrators.", "USER_NOT_ORG_MANAGER": "This action is only allowed for organisation managers or system administrators.", diff --git a/backend/services/mapping_service.py b/backend/services/mapping_service.py index d5223c6dce..05efde2771 100644 --- a/backend/services/mapping_service.py +++ b/backend/services/mapping_service.py @@ -70,7 +70,7 @@ def _is_task_undoable(logged_in_user_id: int, task: Task) -> bool: # User requesting task made the last change, so they are allowed to undo it. is_user_permitted, _ = ProjectService.is_user_permitted_to_validate( task.project_id, logged_in_user_id - ) + ) # FLAGGED: Make use of error_reason if last_action.user_id == int(logged_in_user_id) or is_user_permitted: return True @@ -138,7 +138,7 @@ def unlock_task_after_mapping(mapped_task: MappedTaskDTO) -> TaskDTO: ]: raise MappingServiceError( "InvalidUnlockState- Can only set status to MAPPED, BADIMAGERY, READY after mapping" - ) # FLAGGED FOR STATUS CODE: 409 + ) # FLAGGED FOR STATUS CODE: 400 # Update stats around the change of state last_state = TaskHistory.get_last_status( @@ -196,7 +196,7 @@ def get_task_locked_by_user(project_id: int, task_id: int, user_id: int) -> Task if task.locked_by != user_id: raise MappingServiceError( "TaskNotOwned- Attempting to unlock a task owned by another user" - ) # FLAGGED FOR STATUS CODE: 423 + ) # FLAGGED FOR STATUS CODE: 409 return task @staticmethod diff --git a/backend/services/organisation_service.py b/backend/services/organisation_service.py index 3b37851cf1..e1eef65c9e 100644 --- a/backend/services/organisation_service.py +++ b/backend/services/organisation_service.py @@ -143,7 +143,7 @@ def delete_organisation(organisation_id: int): else: raise OrganisationServiceError( "Organisation has projects, cannot be deleted" - ) # FLAGGED STATUS CODE: 409 + ) @staticmethod def get_organisations(manager_user_id: int): @@ -277,7 +277,7 @@ def assert_validate_name(org: Organisation, name: str): """Validates that the organisation name doesn't exist""" if org.name != name and Organisation.get_organisation_by_name(name) is not None: raise OrganisationServiceError( - f"NameExists- Organisation name already exists: {name}" # FLAGGED STATUS CODE : 409 + f"NameExists- Organisation name already exists: {name}" ) @staticmethod @@ -286,7 +286,7 @@ def assert_validate_users(organisation_dto: OrganisationDTO): if organisation_dto.managers and len(organisation_dto.managers) == 0: raise OrganisationServiceError( "MustHaveAdmin- Must have at least one admin" - ) # FLAGGED: STATUS CODE 409 + ) if organisation_dto.managers and len(organisation_dto.managers) > 0: managers = [] diff --git a/backend/services/validator_service.py b/backend/services/validator_service.py index 7214fcda8c..132d9e0416 100644 --- a/backend/services/validator_service.py +++ b/backend/services/validator_service.py @@ -87,9 +87,10 @@ def lock_tasks_for_validation(validation_dto: LockForValidationDTO) -> TaskDTOs: if error_reason == ValidatingNotAllowed.USER_NOT_ACCEPTED_LICENSE: raise UserLicenseError("User must accept license to map this task") elif error_reason == ValidatingNotAllowed.USER_NOT_ON_ALLOWED_LIST: - raise ValidatorServiceError( - "UserNotAllowed- Validation not allowed because: User not on allowed list" - ) # FLAGGED STATUS CODE: 403 + raise Forbidden( + sub_code="USER_BLOCKED", + user_id=validation_dto.user_id, + ) elif error_reason == ValidatingNotAllowed.PROJECT_NOT_PUBLISHED: raise Forbidden( sub_code="DRAFT_PROJECT_NOT_ALLOWED", From 5e46c2ba3057bd95717c25ab784add86c571a6df Mon Sep 17 00:00:00 2001 From: Aadesh-Baral Date: Mon, 28 Aug 2023 13:22:43 +0545 Subject: [PATCH 04/10] Fix failing test cases after revision of 403 status codes. --- .../api/organisations/test_resources.py | 6 +- .../api/projects/test_resources.py | 2 +- .../api/system/test_authentication.py | 10 +- .../integration/api/tasks/test_actions.py | 108 +++++++++--------- 4 files changed, 63 insertions(+), 63 deletions(-) diff --git a/tests/backend/integration/api/organisations/test_resources.py b/tests/backend/integration/api/organisations/test_resources.py index cab3529b0c..c1e6a40c8f 100644 --- a/tests/backend/integration/api/organisations/test_resources.py +++ b/tests/backend/integration/api/organisations/test_resources.py @@ -74,13 +74,13 @@ def test_get_all_org_includes_managers_if_user_is_authenticated(self): response_body[0]["managers"][0]["username"], self.test_author.username ) - def test_get_all_org_raises_error_if_filter_by_manager_id__on_unauthenticated_request( + def test_get_all_org_raises_error_if_filter_by_manager_id_on_unauthenticated_request( self, ): "Test 403 is returned if filter by manager id on unauthenticated request" response = self.client.get(f"{self.endpoint_url}?manager_user_id=2") - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 401) def test_get_all_org_includes_stats_if_omit_stats_set_false(self): """Test stats are not returned is omitOrgStats is set to False""" @@ -283,7 +283,7 @@ def test_delete_org_with_projects_fails(self): headers={"Authorization": self.session_token}, ) response_body = response.get_json() - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 409) self.assertEqual(response_body["Error"], "Organisation has some projects") self.assertEqual(response_body["SubCode"], "OrgHasProjects") diff --git a/tests/backend/integration/api/projects/test_resources.py b/tests/backend/integration/api/projects/test_resources.py index 0e152963e5..3fb801d20a 100644 --- a/tests/backend/integration/api/projects/test_resources.py +++ b/tests/backend/integration/api/projects/test_resources.py @@ -101,7 +101,7 @@ def test_project_with_mapped_tasks_cannot_be_deleted(self): self.url, headers={"Authorization": self.author_session_token} ) # Assert - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 409) self.assertEqual(response.json["SubCode"], "HasMappedTasks") def test_org_manager_can_delete_project(self): diff --git a/tests/backend/integration/api/system/test_authentication.py b/tests/backend/integration/api/system/test_authentication.py index 62ff06316c..95fbbf5f4c 100644 --- a/tests/backend/integration/api/system/test_authentication.py +++ b/tests/backend/integration/api/system/test_authentication.py @@ -210,7 +210,7 @@ def test_returns_404_if_user_does_not_exist(self): self.assertEqual(error_details["sub_code"], USER_NOT_FOUND_SUB_CODE) self.assertEqual(error_details["message"], USER_NOT_FOUND_MESSAGE) - def test_returns_403_if_invalid_email_token(self): + def test_returns_400_if_invalid_email_token(self): # Arrange user = return_canned_user(USERNAME, USER_ID) user.create() @@ -219,7 +219,7 @@ def test_returns_403_if_invalid_email_token(self): self.url, query_string={"token": "1234", "username": USERNAME} ) # Assert - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 400) self.assertEqual(response.json["SubCode"], "BadSignature") self.assertEqual(response.json["Error"], " Bad Token Signature") @@ -239,11 +239,11 @@ def test_returns_403_if_expired_email_token(self, mock_is_valid_token): self.url, query_string={"token": token, "username": USERNAME} ) # Assert - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 400) self.assertEqual(response.json["SubCode"], "ExpiredToken") self.assertEqual(response.json["Error"], " Token has expired") - def test_returns_403_if_token_and_email_mismatch(self): + def test_returns_400_if_token_and_email_mismatch(self): # Arrange user = return_canned_user(USERNAME, USER_ID) user.email_address = "test_email_2" @@ -257,7 +257,7 @@ def test_returns_403_if_token_and_email_mismatch(self): self.url, query_string={"token": token, "username": USERNAME} ) # Assert - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 400) self.assertEqual(response.json["SubCode"], "InvalidEmail") self.assertEqual(response.json["Error"], " Email address does not match token") diff --git a/tests/backend/integration/api/tasks/test_actions.py b/tests/backend/integration/api/tasks/test_actions.py index 8bbd1411e3..f9629e1401 100644 --- a/tests/backend/integration/api/tasks/test_actions.py +++ b/tests/backend/integration/api/tasks/test_actions.py @@ -386,10 +386,10 @@ def test_mapping_lock_returns_403_for_if_user_not_allowed_to_map( ) @patch.object(ProjectAdminService, "is_user_action_permitted_on_project") - def test_mapping_lock_returns_403_if_task_in_invalid_state_for_mapping( + def test_mapping_lock_returns_409_if_task_in_invalid_state_for_mapping( self, mock_pm_role ): - """Test returns 403 if task is in invalid state for mapping. i.e. not in READY or INVALIDATED state.""" + """Test returns 409 if task is in invalid state for mapping. i.e. not in READY or INVALIDATED state.""" # Arrange mock_pm_role.return_value = True # Act @@ -398,7 +398,7 @@ def test_mapping_lock_returns_403_if_task_in_invalid_state_for_mapping( ) # Assert # As Task 1 is in MAPPED state, it should not be allowed to be locked for mapping. - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 409) self.assertEqual(response.json["SubCode"], "InvalidTaskState") # Arrange @@ -411,7 +411,7 @@ def test_mapping_lock_returns_403_if_task_in_invalid_state_for_mapping( ) # Assert # As Task 1 is in VALIDATED state, it should not be allowed to be locked for mapping. - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 409) self.assertEqual(response.json["SubCode"], "InvalidTaskState") @patch.object(UserService, "has_user_accepted_license") @@ -513,8 +513,8 @@ def test_mapping_unlock_returns_404_for_invalid_task_id(self): self.assertEqual(response.status_code, 404) self.assertEqual(response.json["error"]["sub_code"], TASK_NOT_FOUND_SUB_CODE) - def test_mapping_unlock_returns_403_if_task_not_locked_for_mapping(self): - """Test returns 403 if task is not locked for mapping.""" + def test_mapping_unlock_returns_409_if_task_not_locked_for_mapping(self): + """Test returns 409 if task is not locked for mapping.""" # Act response = self.client.post( self.url, @@ -522,11 +522,11 @@ def test_mapping_unlock_returns_403_if_task_not_locked_for_mapping(self): json={"status": "MAPPED"}, ) # Assert - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 409) self.assertEqual(response.json["SubCode"], "LockBeforeUnlocking") - def test_mapping_unlock_returns_403_if_task_locked_by_other_user(self): - """Test returns 403 if task is locked by other user.""" + def test_mapping_unlock_returns_409_if_task_locked_by_other_user(self): + """Test returns 409 if task is locked by other user.""" # Arrange task = Task.get(1, self.test_project.id) task.task_status = TaskStatus.LOCKED_FOR_MAPPING.value @@ -539,11 +539,11 @@ def test_mapping_unlock_returns_403_if_task_locked_by_other_user(self): json={"status": "MAPPED"}, ) # Assert - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 409) self.assertEqual(response.json["SubCode"], "TaskNotOwned") - def test_returns_403_if_invalid_new_state_passed(self): - """Test returns 403 if invalid new state passed as new task state should be READY, MAPPED or BADIMAGERY.""" + def test_returns_409_if_invalid_new_state_passed(self): + """Test returns 409 if invalid new state passed as new task state should be READY, MAPPED or BADIMAGERY.""" # Arrange task = Task.get(1, self.test_project.id) task.task_status = TaskStatus.LOCKED_FOR_MAPPING.value @@ -556,7 +556,7 @@ def test_returns_403_if_invalid_new_state_passed(self): json={"status": "INVALIDATED"}, ) # Assert - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 409) self.assertEqual(response.json["SubCode"], "InvalidUnlockState") def test_mapping_unlock_returns_200_on_success(self): @@ -664,11 +664,11 @@ def test_mapping_stop_returns_403_if_task_not_locked_for_mapping(self): headers={"Authorization": self.user_session_token}, ) # Assert - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 409) self.assertEqual(response.json["SubCode"], "LockBeforeUnlocking") - def test_mapping_stop_returns_403_if_task_locked_by_other_user(self): - """Test returns 403 if task locked by other user.""" + def test_mapping_stop_returns_409_if_task_locked_by_other_user(self): + """Test returns 409 if task locked by other user.""" # Arrange task = Task.get(1, self.test_project.id) task.task_status = TaskStatus.LOCKED_FOR_MAPPING.value @@ -680,7 +680,7 @@ def test_mapping_stop_returns_403_if_task_locked_by_other_user(self): headers={"Authorization": self.user_session_token}, ) # Assert - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 409) self.assertEqual(response.json["SubCode"], "TaskNotOwned") def test_mapping_stop_returns_200_on_success(self): @@ -780,8 +780,8 @@ def test_validation_lock_returns_404_for_invalid_task_id(self): self.assertEqual(response.status_code, 404) self.assertEqual(response.json["error"]["sub_code"], TASK_NOT_FOUND_SUB_CODE) - def test_validation_lock_returns_403_if_task_not_ready_for_validation(self): - """Test returns 403 if task not ready for validation.""" + def test_validation_lock_returns_409_if_task_not_ready_for_validation(self): + """Test returns 409 if task not ready for validation.""" # Arrange task = Task.get(1, self.test_project.id) task.task_status = TaskStatus.READY.value # not ready for validation @@ -793,13 +793,13 @@ def test_validation_lock_returns_403_if_task_not_ready_for_validation(self): json={"taskIds": [1]}, ) # Assert - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 409) self.assertEqual(response.json["SubCode"], "NotReadyForValidation") - def test_validation_lock_returns_403_if_mapped_by_same_user_and_user_not_admin( + def test_validation_lock_returns_409_if_mapped_by_same_user_and_user_not_admin( self, ): - """Test returns 403 if mapped by same user.""" + """Test returns 409 if mapped by same user.""" # Arrange task = Task.get(1, self.test_project.id) task.task_status = TaskStatus.MAPPED.value @@ -812,7 +812,7 @@ def test_validation_lock_returns_403_if_mapped_by_same_user_and_user_not_admin( json={"taskIds": [1]}, ) # Assert - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 409) self.assertEqual(response.json["SubCode"], "CannotValidateMappedTask") def test_validation_lock_returns_403_if_user_not_permitted_to_validate(self): @@ -871,10 +871,10 @@ def test_validation_lock_returns_403_if_user_not_on_allowed_list( ) # Assert self.assertEqual(response.status_code, 403) - self.assertEqual(response.json["SubCode"], "UserNotAllowed") + self.assertEqual(response.json["error"]["sub_code"], "USER_BLOCKED") - def test_validation_lock_returns_403_if_user_has_already_locked_other_task(self): - """Test returns 403 if user has already locked other task.""" + def test_validation_lock_returns_409_if_user_has_already_locked_other_task(self): + """Test returns 409 if user has already locked other task.""" # Arrange self.test_project.status = ProjectStatus.PUBLISHED.value self.test_project.save() @@ -889,7 +889,7 @@ def test_validation_lock_returns_403_if_user_has_already_locked_other_task(self) json={"taskIds": [1]}, ) # Assert - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 409) self.assertEqual(response.json["SubCode"], "UserAlreadyHasTaskLocked") @patch.object(ProjectService, "is_user_permitted_to_validate") @@ -980,8 +980,8 @@ def test_validation_unlock_returns_404_if_task_not_found(self): self.assertEqual(response.status_code, 404) self.assertEqual(response.json["error"]["sub_code"], TASK_NOT_FOUND_SUB_CODE) - def test_validation_unlock_returns_403_if_task_not_locked_for_validation(self): - """Test returns 403 if task not locked for validation.""" + def test_validation_unlock_returns_409_if_task_not_locked_for_validation(self): + """Test returns 409 if task not locked for validation.""" # Act response = self.client.post( self.url, @@ -989,7 +989,7 @@ def test_validation_unlock_returns_403_if_task_not_locked_for_validation(self): json={"validatedTasks": [{"taskId": 1, "status": "VALIDATED"}]}, ) # Assert - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 409) self.assertEqual(response.json["SubCode"], "NotLockedForValidation") @staticmethod @@ -1002,8 +1002,8 @@ def lock_task_for_validation(task_id, project_id, user_id, mapped_by=None): task.mapped_by = mapped_by task.update() - def test_validation_unlock_returns_403_if_task_locked_by_other_user(self): - """Test returns 403 if task locked by other user.""" + def test_validation_unlock_returns_409_if_task_locked_by_other_user(self): + """Test returns 409 if task locked by other user.""" # Arrange TestTasksActionsValidationUnlockAPI.lock_task_for_validation( 1, self.test_project.id, self.test_author.id @@ -1015,7 +1015,7 @@ def test_validation_unlock_returns_403_if_task_locked_by_other_user(self): json={"validatedTasks": [{"taskId": 1, "status": "VALIDATED"}]}, ) # Assert - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 409) self.assertEqual(response.json["SubCode"], "TaskNotOwned") def test_validation_unlock_returns_400_if_invalid_state_passsed(self): @@ -1164,8 +1164,8 @@ def test_validation_stop_returns_404_if_task_not_found(self): self.assertEqual(response.status_code, 404) self.assertEqual(response.json["error"]["sub_code"], TASK_NOT_FOUND_SUB_CODE) - def test_validation_stop_returns_403_if_task_not_locked_for_validation(self): - """Test returns 403 if task not locked for validation.""" + def test_validation_stop_returns_409_if_task_not_locked_for_validation(self): + """Test returns 409 if task not locked for validation.""" # Arrange TestTasksActionsValidationUnlockAPI.lock_task_for_validation( 1, self.test_project.id, self.test_user.id @@ -1178,11 +1178,11 @@ def test_validation_stop_returns_403_if_task_not_locked_for_validation(self): ) # Assert # Since task 2 is not locked for validation, we should get a 403 even though task 1 is locked for validation - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 409) self.assertEqual(response.json["SubCode"], "NotLockedForValidation") - def test_validation_stop_returns_403_if_task_locked_by_other_user(self): - """Test returns 403 if task locked by other user.""" + def test_validation_stop_returns_409_if_task_locked_by_other_user(self): + """Test returns 409 if task locked by other user.""" # Arrange TestTasksActionsValidationUnlockAPI.lock_task_for_validation( 1, self.test_project.id, self.test_author.id, self.test_author.id @@ -1194,7 +1194,7 @@ def test_validation_stop_returns_403_if_task_locked_by_other_user(self): json={"resetTasks": [{"taskId": 1}]}, ) # Assert - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 409) self.assertEqual(response.json["SubCode"], "TaskNotOwned") def test_validation_stop_returns_200_if_task_locked_by_user(self): @@ -1287,8 +1287,8 @@ def test_returns_404_if_task_not_found(self): self.assertEqual(response.status_code, 404) self.assertEqual(response.json["error"]["sub_code"], TASK_NOT_FOUND_SUB_CODE) - def test_returns_403_if_task_too_small_to_split(self): - """Test returns 403 if task too small to split.""" + def test_returns_409_if_task_too_small_to_split(self): + """Test returns 409 if task too small to split.""" # Arrange task = Task.get(1, self.test_project.id) task.zoom = 18 @@ -1299,21 +1299,21 @@ def test_returns_403_if_task_too_small_to_split(self): headers={"Authorization": self.author_session_token}, ) # Assert - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 409) self.assertEqual(response.json["SubCode"], "SmallToSplit") - def test_returns_403_if_task_not_locked_for_mapping(self): - """Test returns 403 if task not locked for mapping.""" + def test_returns_409_if_task_not_locked_for_mapping(self): + """Test returns 409 if task not locked for mapping.""" # Since task should be locked for mapping to split, we should get a 403 response = self.client.post( self.url, headers={"Authorization": self.author_session_token}, ) - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 409) self.assertEqual(response.json["SubCode"], "LockToSplit") - def test_returns_403_if_task_locked_by_other_user(self): - """Test returns 403 if task locked by other user.""" + def test_returns_409_if_task_locked_by_other_user(self): + """Test returns 409 if task locked by other user.""" # Arrange test_user = return_canned_user("test user", 1111111) test_user.create() @@ -1325,7 +1325,7 @@ def test_returns_403_if_task_locked_by_other_user(self): headers={"Authorization": self.author_session_token}, ) # Assert - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 409) self.assertEqual(response.json["SubCode"], "SplitOtherUserTask") def test_returns_200_if_task_locked_by_user(self): @@ -1549,8 +1549,8 @@ def test_returns_404_if_task_not_found(self): self.assertEqual(response.status_code, 404) self.assertEqual(response.json["error"]["sub_code"], TASK_NOT_FOUND_SUB_CODE) - def test_returns_403_if_task_not_locked(self): - """Test returns 403 if task not locked.""" + def test_returns_409_if_task_not_locked(self): + """Test returns 409 if task not locked.""" # Task should be locked for mapping or validation to extend # Act response = self.client.post( @@ -1559,11 +1559,11 @@ def test_returns_403_if_task_not_locked(self): json={"taskIds": [1]}, ) # Assert - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 409) self.assertEqual(response.json["SubCode"], "TaskStatusNotLocked") - def test_returns_403_if_task_is_not_locked_by_requesting_user(self): - """Test returns 403 if task is not locked by requesting user.""" + def test_returns_409_if_task_is_not_locked_by_requesting_user(self): + """Test returns 409 if task is not locked by requesting user.""" # Task should be locked for mapping or validation to extend # Arrange task = Task.get(1, self.test_project.id) @@ -1575,7 +1575,7 @@ def test_returns_403_if_task_is_not_locked_by_requesting_user(self): json={"taskIds": [1]}, ) # Assert - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 409) self.assertEqual(response.json["SubCode"], "LockedByAnotherUser") def test_returns_200_if_task_locked_by_requesting_user(self): From 2f279c233d54515f1e17554ab509ef321bfd856f Mon Sep 17 00:00:00 2001 From: Aadesh-Baral Date: Mon, 28 Aug 2023 13:27:22 +0545 Subject: [PATCH 05/10] Resolve Permission Conflict during Project Deletion. ----------------------------------- The current setup for deleting a project involves conflicting permission checks. In the resource class function, users with a Project Manager (PM) role are allowed to delete projects. However, in the service function responsible for project deletion, the check only permits organization managers or system administrators to perform this action. To address this inconsistency in permission checks, this commit streamlines the process. It eliminates the permission check within the service function, thereby enabling users with PM roles within a project to successfully initiate project deletions. --- backend/api/projects/resources.py | 1 - backend/services/project_admin_service.py | 19 ++++--------------- 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/backend/api/projects/resources.py b/backend/api/projects/resources.py index 028bfd6c96..fdb53b3a31 100644 --- a/backend/api/projects/resources.py +++ b/backend/api/projects/resources.py @@ -431,7 +431,6 @@ def delete(self, project_id): description: Internal Server Error """ authenticated_user_id = token_auth.current_user() - # FLAGGED: CONFLICTING PERMISSION CHECK WITH SERVICE FUNCTION if not ProjectAdminService.is_user_action_permitted_on_project( authenticated_user_id, project_id ): diff --git a/backend/services/project_admin_service.py b/backend/services/project_admin_service.py index 32d880e55b..9f4f162206 100644 --- a/backend/services/project_admin_service.py +++ b/backend/services/project_admin_service.py @@ -155,22 +155,11 @@ def delete_project(project_id: int, authenticated_user_id: int): """Deletes project if it has no completed tasks""" project = ProjectAdminService._get_project_by_id(project_id) - is_admin = UserService.is_user_an_admin(authenticated_user_id) - user_orgs = OrganisationService.get_organisations_managed_by_user_as_dto( - authenticated_user_id - ) - is_org_manager = len(user_orgs.organisations) > 0 - - if is_admin or is_org_manager: - if project.can_be_deleted(): - project.delete() - else: - raise ProjectAdminServiceError( - "HasMappedTasks- Project has mapped tasks, cannot be deleted" - ) + if project.can_be_deleted(): + project.delete() else: - raise Forbidden( - sub_code="USER_NOT_ORG_MANAGER", user_id=authenticated_user_id + raise ProjectAdminServiceError( + "HasMappedTasks- Project has mapped tasks, cannot be deleted" ) @staticmethod From 60f391de430739efe150225f94a1f42d33d28c54 Mon Sep 17 00:00:00 2001 From: Aadesh-Baral Date: Mon, 28 Aug 2023 14:35:19 +0545 Subject: [PATCH 06/10] Split checks for tasks status and user permission in function so that correct error response can be returned --- backend/api/tasks/actions.py | 4 +-- backend/error_messages.json | 1 + backend/services/mapping_service.py | 31 ++++++++++++------- .../integration/api/tasks/test_actions.py | 12 +++---- .../unit/services/test_mapping_service.py | 6 ++-- 5 files changed, 33 insertions(+), 21 deletions(-) diff --git a/backend/api/tasks/actions.py b/backend/api/tasks/actions.py index 0c0b542a3a..017dc8e0f9 100644 --- a/backend/api/tasks/actions.py +++ b/backend/api/tasks/actions.py @@ -328,8 +328,8 @@ def post(self, project_id, task_id): project_id, task_id, token_auth.current_user(), preferred_locale ) return task.to_primitive(), 200 - except MappingServiceError as e: # FLAGGED FOR STATUS CODE - return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 + except MappingServiceError as e: + return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 409 class TasksActionsValidationLockAPI(Resource): diff --git a/backend/error_messages.json b/backend/error_messages.json index ba2d128dbf..1a193c8b12 100644 --- a/backend/error_messages.json +++ b/backend/error_messages.json @@ -27,6 +27,7 @@ "USER_NOT_ADMIN": "This action is only allowed for system administrators.", "USER_NOT_ORG_MANAGER": "This action is only allowed for organisation managers or system administrators.", "USER_NOT_PROJECT_MANAGER": "This action is only allowed for users with project manage permissions.", + "USER_NOT_VALIDATOR": "This action is only allowed for users with validation permissions in the project.", "USER_NOT_TEAM_MANAGER": "This action is only allowed for users with team manage permissions.", "INVALID_NEW_PROJECT_OWNER": "New project owner must be project's organisation manager or system admin", "USER_ACTION_NOT_PERMITTED": "This user does not have the required permissions to perform this action.", diff --git a/backend/services/mapping_service.py b/backend/services/mapping_service.py index 05efde2771..9c86c5af39 100644 --- a/backend/services/mapping_service.py +++ b/backend/services/mapping_service.py @@ -59,22 +59,31 @@ def get_task_as_dto( @staticmethod def _is_task_undoable(logged_in_user_id: int, task: Task) -> bool: """Determines if the current task status can be undone by the logged in user""" - # Test to see if user can undo status on this task - if logged_in_user_id and TaskStatus(task.task_status) not in [ + # Check if task status is in a state that can be undone + if TaskStatus(task.task_status) in [ TaskStatus.LOCKED_FOR_MAPPING, TaskStatus.LOCKED_FOR_VALIDATION, TaskStatus.READY, ]: - last_action = TaskHistory.get_last_action(task.project_id, task.id) - - # User requesting task made the last change, so they are allowed to undo it. - is_user_permitted, _ = ProjectService.is_user_permitted_to_validate( - task.project_id, logged_in_user_id - ) # FLAGGED: Make use of error_reason - if last_action.user_id == int(logged_in_user_id) or is_user_permitted: - return True + raise MappingServiceError( + "UndoNotAllowed- Task is in a state that cannot be undone" + ) + # Test to see if user can undo status on this task + last_action = TaskHistory.get_last_action(task.project_id, task.id) - return False # FLAGGED: Split out for permission and state + # User requesting task made the last change, so they are allowed to undo it. + is_user_permitted, _ = ProjectService.is_user_permitted_to_validate( + task.project_id, logged_in_user_id + ) + if last_action.user_id == int(logged_in_user_id) or is_user_permitted: + return True + else: + raise Forbidden( + sub_code="USER_NOT_VALIDATOR", + user_id=logged_in_user_id, + project_id=task.project_id, + task_id=task.id, + ) @staticmethod def lock_task_for_mapping(lock_task_dto: LockTaskDTO) -> TaskDTO: diff --git a/tests/backend/integration/api/tasks/test_actions.py b/tests/backend/integration/api/tasks/test_actions.py index f9629e1401..a7209de04a 100644 --- a/tests/backend/integration/api/tasks/test_actions.py +++ b/tests/backend/integration/api/tasks/test_actions.py @@ -1388,10 +1388,10 @@ def test_returns_404_if_task_not_found(self): self.assertEqual(response.status_code, 404) self.assertEqual(response.json["error"]["sub_code"], TASK_NOT_FOUND_SUB_CODE) - def test_returns_403_if_task_in_invalid_state_for_undo(self): - """Test returns 403 if task in invalid state for undo.""" + def test_returns_409_if_task_in_invalid_state_for_undo(self): + """Test returns 409 if task in invalid state for undo.""" # Since task cannot be in READY, LOCKED_FOR_VALIDATION or LOCKED_FOR_MAPPING state for undo, - # we should get a 403 + # we should get a 409 # Arrange task = Task.get(1, self.test_project.id) task.task_status = TaskStatus.READY.value @@ -1402,8 +1402,8 @@ def test_returns_403_if_task_in_invalid_state_for_undo(self): headers={"Authorization": self.user_session_token}, ) # Assert - self.assertEqual(response.status_code, 403) - self.assertEqual(response.json["SubCode"], "UndoPermissionError") + self.assertEqual(response.status_code, 409) + self.assertEqual(response.json["SubCode"], "UndoNotAllowed") @staticmethod def validate_task(task_id, project_id, user_id): @@ -1428,7 +1428,7 @@ def test_returns_403_if_user_not_permitted_for_undo(self): ) # Assert self.assertEqual(response.status_code, 403) - self.assertEqual(response.json["SubCode"], "UndoPermissionError") + self.assertEqual(response.json["error"]["sub_code"], "USER_NOT_VALIDATOR") @staticmethod def assert_undo_response(response, project_id, last_status, username, new_status): diff --git a/tests/backend/unit/services/test_mapping_service.py b/tests/backend/unit/services/test_mapping_service.py index 21700db801..ac6e3d3e05 100644 --- a/tests/backend/unit/services/test_mapping_service.py +++ b/tests/backend/unit/services/test_mapping_service.py @@ -1,4 +1,6 @@ from unittest.mock import patch, MagicMock + +from backend.exceptions import Forbidden from backend.services.mapping_service import ( MappingService, Task, @@ -231,7 +233,7 @@ def test_task_is_not_undoable_if_last_change_not_made_by_you( # Act mock_project.return_value = (False, None) - is_undoable = MappingService._is_task_undoable(1, task) # Assert - self.assertFalse(is_undoable) + with self.assertRaises(Forbidden): + MappingService._is_task_undoable(1, task) From 77b3d5fbefb4a9c412618e3d9f715e2b95650bdb Mon Sep 17 00:00:00 2001 From: Aadesh-Baral Date: Mon, 28 Aug 2023 16:02:31 +0545 Subject: [PATCH 07/10] Enhanced Error Messaging for Different Scenarios with Separate Codes -------------------------------------------------------------------- Previously, we utilized the same error reason, namely "USER_NOT_ON_ALLOWED_LIST," for two distinct situations: 1. When a user, who is blocked, attempts to map/validate a task. 2. When a user lacks the necessary permissions to map/validate. To enhance clarity and precision in error reporting, we have introduced a distinct error reason for the scenario where a user is blocked. This modification allows us to deliver more accurate and contextually relevant error messages, ultimately improving the user experience and troubleshooting process. --- backend/models/postgis/statuses.py | 2 ++ backend/services/mapping_service.py | 7 ++++- backend/services/project_service.py | 6 ++--- backend/services/validator_service.py | 4 ++- .../integration/api/tasks/test_actions.py | 26 +++++++++++++++++++ .../unit/services/test_project_service.py | 4 +-- 6 files changed, 42 insertions(+), 7 deletions(-) diff --git a/backend/models/postgis/statuses.py b/backend/models/postgis/statuses.py index ef78498948..5922ba7168 100644 --- a/backend/models/postgis/statuses.py +++ b/backend/models/postgis/statuses.py @@ -73,6 +73,7 @@ class MappingNotAllowed(Enum): USER_NOT_TEAM_MEMBER = 105 PROJECT_HAS_NO_TEAM = 106 NOT_A_MAPPING_TEAM = 107 + USER_IS_BLOCKED = 108 class ValidatingNotAllowed(Enum): @@ -87,6 +88,7 @@ class ValidatingNotAllowed(Enum): USER_NOT_TEAM_MEMBER = 106 PROJECT_HAS_NO_TEAM = 107 USER_ALREADY_HAS_TASK_LOCKED = 108 + USER_IS_BLOCKED = 109 class UserGender(Enum): diff --git a/backend/services/mapping_service.py b/backend/services/mapping_service.py index 9c86c5af39..4b2570cb6c 100644 --- a/backend/services/mapping_service.py +++ b/backend/services/mapping_service.py @@ -109,10 +109,15 @@ def lock_task_for_mapping(lock_task_dto: LockTaskDTO) -> TaskDTO: raise UserLicenseError( "User must accept license to map this task" ) # FLAGGED FOR STATUS CODE: 409 - elif error_reason == MappingNotAllowed.USER_NOT_ON_ALLOWED_LIST: + elif error_reason == MappingNotAllowed.USER_IS_BLOCKED: raise Forbidden( sub_code="USER_BLOCKED", user_id=lock_task_dto.user_id ) + elif error_reason == MappingNotAllowed.USER_NOT_ON_ALLOWED_LIST: + raise Forbidden( + sub_code="USER_ACTION_NOT_PERMITTED", + user_id=lock_task_dto.user_id, + ) elif error_reason == MappingNotAllowed.PROJECT_NOT_PUBLISHED: raise Forbidden( sub_code="DRAFT_PROJECT_NOT_ALLOWED", diff --git a/backend/services/project_service.py b/backend/services/project_service.py index 18611326c0..be05cecf76 100644 --- a/backend/services/project_service.py +++ b/backend/services/project_service.py @@ -331,7 +331,7 @@ def evaluate_mapping_permission( project_id, allowed_roles, user_id ) - # mapping_permission = 1(level),2(teams),3(teamsAndLevel) + # mapping_permission: 1(level),2(teams),3(teamsAndLevel) if mapping_permission == MappingPermission.TEAMS.value: if not is_team_member: return False, MappingNotAllowed.USER_NOT_TEAM_MEMBER @@ -350,7 +350,7 @@ def evaluate_mapping_permission( def is_user_permitted_to_map(project_id: int, user_id: int): """Check if the user is allowed to map the on the project in scope""" if UserService.is_user_blocked(user_id): - return False, MappingNotAllowed.USER_NOT_ON_ALLOWED_LIST + return False, MappingNotAllowed.USER_IS_BLOCKED project = ProjectService.get_project_by_id(project_id) if project.license_id: @@ -435,7 +435,7 @@ def evaluate_validation_permission( def is_user_permitted_to_validate(project_id, user_id): """Check if the user is allowed to validate on the project in scope""" if UserService.is_user_blocked(user_id): - return False, ValidatingNotAllowed.USER_NOT_ON_ALLOWED_LIST + return False, ValidatingNotAllowed.USER_IS_BLOCKED project = ProjectService.get_project_by_id(project_id) if project.license_id: diff --git a/backend/services/validator_service.py b/backend/services/validator_service.py index 132d9e0416..5d620541da 100644 --- a/backend/services/validator_service.py +++ b/backend/services/validator_service.py @@ -86,9 +86,11 @@ def lock_tasks_for_validation(validation_dto: LockForValidationDTO) -> TaskDTOs: if not user_can_validate: if error_reason == ValidatingNotAllowed.USER_NOT_ACCEPTED_LICENSE: raise UserLicenseError("User must accept license to map this task") + elif error_reason == ValidatingNotAllowed.USER_IS_BLOCKED: + raise Forbidden(sub_code="USER_BLOCKED", user_id=validation_dto.user_id) elif error_reason == ValidatingNotAllowed.USER_NOT_ON_ALLOWED_LIST: raise Forbidden( - sub_code="USER_BLOCKED", + sub_code="USER_ACTION_NOT_PERMITTED", user_id=validation_dto.user_id, ) elif error_reason == ValidatingNotAllowed.PROJECT_NOT_PUBLISHED: diff --git a/tests/backend/integration/api/tasks/test_actions.py b/tests/backend/integration/api/tasks/test_actions.py index a7209de04a..13885e1254 100644 --- a/tests/backend/integration/api/tasks/test_actions.py +++ b/tests/backend/integration/api/tasks/test_actions.py @@ -871,6 +871,32 @@ def test_validation_lock_returns_403_if_user_not_on_allowed_list( ) # Assert self.assertEqual(response.status_code, 403) + self.assertEqual( + response.json["error"]["sub_code"], "USER_ACTION_NOT_PERMITTED" + ) + + @patch.object(ProjectService, "is_user_permitted_to_validate") + def test_validation_lock_returns_403_if_user_is_blocked( + self, mock_validate_permitted + ): + """Test returns 403 if user not on allowed list.""" + # Arrange + mock_validate_permitted.return_value = ( + False, + ValidatingNotAllowed.USER_IS_BLOCKED, + ) + task = Task.get(1, self.test_project.id) + task.task_status = TaskStatus.MAPPED.value + task.mapped_by = self.test_author.id + task.update() + # Act + response = self.client.post( + self.url, + headers={"Authorization": self.user_session_token}, + json={"taskIds": [1]}, + ) + # Assert + self.assertEqual(response.status_code, 403) self.assertEqual(response.json["error"]["sub_code"], "USER_BLOCKED") def test_validation_lock_returns_409_if_user_has_already_locked_other_task(self): diff --git a/tests/backend/unit/services/test_project_service.py b/tests/backend/unit/services/test_project_service.py index f986f61258..d931d17f0f 100644 --- a/tests/backend/unit/services/test_project_service.py +++ b/tests/backend/unit/services/test_project_service.py @@ -119,7 +119,7 @@ def test_user_allowed_to_map( mock_user_blocked.return_value = True allowed, reason = ProjectService.is_user_permitted_to_map(1, 1) self.assertFalse(allowed) - self.assertEqual(reason, MappingNotAllowed.USER_NOT_ON_ALLOWED_LIST) + self.assertEqual(reason, MappingNotAllowed.USER_IS_BLOCKED) @patch.object(ProjectAdminService, "is_user_action_permitted_on_project") @patch.object(UserService, "is_user_blocked") @@ -169,7 +169,7 @@ def test_user_permitted_to_validate( mock_user_blocked.return_value = True allowed, reason = ProjectService.is_user_permitted_to_validate(1, 1) self.assertFalse(allowed) - self.assertEqual(reason, ValidatingNotAllowed.USER_NOT_ON_ALLOWED_LIST) + self.assertEqual(reason, ValidatingNotAllowed.USER_IS_BLOCKED) # Unpublished project stub_project.status = ProjectStatus.DRAFT.value From 5dcde8bfe23c0c54e24302c2d994fb67672df6ca Mon Sep 17 00:00:00 2001 From: Aadesh-Baral Date: Tue, 29 Aug 2023 13:51:10 +0545 Subject: [PATCH 08/10] Remove user permission ncheck on service function to update project as this is already checked in resource function --- backend/api/projects/resources.py | 2 +- backend/services/project_admin_service.py | 16 ++----- .../services/test_project_admin_service.py | 47 ++----------------- 3 files changed, 9 insertions(+), 56 deletions(-) diff --git a/backend/api/projects/resources.py b/backend/api/projects/resources.py index fdb53b3a31..3abe82b346 100644 --- a/backend/api/projects/resources.py +++ b/backend/api/projects/resources.py @@ -386,7 +386,7 @@ def patch(self, project_id): return {"Error": "Unable to update project", "SubCode": "InvalidData"}, 400 try: - ProjectAdminService.update_project(project_dto, authenticated_user_id) + ProjectAdminService.update_project(project_dto) return {"Status": "Updated"}, 200 except InvalidGeoJson as e: return {"Invalid GeoJson": str(e)}, 400 diff --git a/backend/services/project_admin_service.py b/backend/services/project_admin_service.py index 9f4f162206..4623aeb975 100644 --- a/backend/services/project_admin_service.py +++ b/backend/services/project_admin_service.py @@ -114,7 +114,7 @@ def get_project_dto_for_admin(project_id: int) -> ProjectDTO: return project.as_dto_for_admin(project_id) @staticmethod - def update_project(project_dto: ProjectDTO, authenticated_user_id: int): + def update_project(project_dto: ProjectDTO): project_id = project_dto.project_id if project_dto.project_status == ProjectStatus.PUBLISHED.name: @@ -125,18 +125,8 @@ def update_project(project_dto: ProjectDTO, authenticated_user_id: int): if project_dto.license_id: ProjectAdminService._validate_imagery_licence(project_dto.license_id) - # To be handled before reaching this function - if ProjectAdminService.is_user_action_permitted_on_project( # FLAGGED: ALREADY CHECKED IN VIEW FUNCTION - authenticated_user_id, project_id - ): - project = ProjectAdminService._get_project_by_id(project_id) - project.update(project_dto) - else: - raise Forbidden( - sub_code="USER_NOT_PROJECT_MANAGER", - user_id=authenticated_user_id, - project_id=project_id, - ) + project = ProjectAdminService._get_project_by_id(project_id) + project.update(project_dto) return project diff --git a/tests/backend/integration/services/test_project_admin_service.py b/tests/backend/integration/services/test_project_admin_service.py index 8f88a004fd..fc95d71664 100644 --- a/tests/backend/integration/services/test_project_admin_service.py +++ b/tests/backend/integration/services/test_project_admin_service.py @@ -287,7 +287,7 @@ def test_update_published_project_with_incomplete_default_locale_raises_error( mock_user.return_value = stub_admin_user # Act / Assert with self.assertRaises(ProjectAdminServiceError): - ProjectAdminService.update_project(dto, mock_user.id) + ProjectAdminService.update_project(dto) @patch.object(User, "get_by_id") @patch.object(Project, "update") @@ -310,7 +310,7 @@ def test_updating_a_private_project_with_no_allowed_users_raises_error( # Act try: - ProjectAdminService.update_project(dto, mock_user.id) + ProjectAdminService.update_project(dto) # Assert except ProjectAdminServiceError: self.fail("update_project raised an exception when setting it as private") @@ -349,44 +349,7 @@ def test_update_project_with_non_existant_license_raises_error( mock_user.return_value = stub_admin_user # Act / Assert with self.assertRaises(ProjectAdminServiceError): - ProjectAdminService.update_project(dto, mock_user.id) - - @patch.object(User, "get_by_id") - @patch.object(Project, "update") - @patch.object(Project, "get") - def test_updating_a_project_with_different_roles_raises_error( - self, mock_project, mock_project2, mock_user - ): - # Arrange - stub_project = Project() - stub_project.status = ProjectStatus.DRAFT.value - - mock_project.return_value = stub_project - - locales = [] - info = ProjectInfoDTO() - info.locale = "en" - info.name = "Test" - locales.append(info) - - dto = ProjectDTO() - dto.project_id = 1 - dto.default_locale = "en" - dto.project_status = ProjectStatus.DRAFT.name - dto.project_priority = ProjectPriority.LOW.name - dto.difficulty = ProjectDifficulty.EASY.name - dto.mapping_types = ["ROADS"] - dto.mapping_editors = ["ID"] - dto.validation_editors = ["ID"] - dto.project_info_locales = locales - - stub_user = User() - stub_user.username = "mapper" - stub_user.role = UserRole.MAPPER.value - mock_user.return_value = stub_user - # Act/Assert - with self.assertRaises(Forbidden): - ProjectAdminService.update_project(dto, mock_user.id) + ProjectAdminService.update_project(dto) def test_updating_a_project_with_valid_project_info(self): locales = [] @@ -398,7 +361,7 @@ def test_updating_a_project_with_valid_project_info(self): info.instructions = "Test instructions" locales.append(info) - test_project, test_user = create_canned_project() + test_project, _ = create_canned_project() dto = ProjectDTO() dto.project_id = test_project.id @@ -411,7 +374,7 @@ def test_updating_a_project_with_valid_project_info(self): dto.validation_editors = ["ID"] dto.project_info_locales = locales # Act - updated_project = ProjectAdminService.update_project(dto, test_user.id) + updated_project = ProjectAdminService.update_project(dto) # Assert self.assertEqual( updated_project.difficulty, ProjectDifficulty[dto.difficulty.upper()].value From 992c1ddf61d9b7ff350b9794b99781f32a060584 Mon Sep 17 00:00:00 2001 From: Aadesh-Baral Date: Tue, 29 Aug 2023 13:56:05 +0545 Subject: [PATCH 09/10] Remove nerver reaching except blocks. --- backend/api/projects/teams.py | 17 +++++++---------- backend/api/tasks/actions.py | 11 +---------- backend/services/mapping_service.py | 4 ---- 3 files changed, 8 insertions(+), 24 deletions(-) diff --git a/backend/api/projects/teams.py b/backend/api/projects/teams.py index e75fe06fbd..fc88df55f1 100644 --- a/backend/api/projects/teams.py +++ b/backend/api/projects/teams.py @@ -2,7 +2,7 @@ from schematics.exceptions import DataError from backend.exceptions import Forbidden -from backend.services.team_service import TeamService, TeamServiceError +from backend.services.team_service import TeamService from backend.services.project_admin_service import ProjectAdminService from backend.services.project_service import ProjectService from backend.services.users.authentication_service import token_auth @@ -180,16 +180,13 @@ def patch(self, team_id, project_id): current_app.logger.error(f"Error validating request: {str(e)}") return {"Error": str(e), "SubCode": "InvalidData"}, 400 - try: - if not ProjectAdminService.is_user_action_permitted_on_project( - token_auth.current_user, project_id - ): - raise Forbidden(sub_code="USER_NOT_PROJECT_MANAGER") + if not ProjectAdminService.is_user_action_permitted_on_project( + token_auth.current_user, project_id + ): + raise Forbidden(sub_code="USER_NOT_PROJECT_MANAGER") - TeamService.change_team_role(team_id, project_id, role) - return {"Status": "Team role updated successfully."}, 200 - except TeamServiceError as e: - return str(e), 402 # FLAGGED FOR STATUS CODE/UNREACHABLE + TeamService.change_team_role(team_id, project_id, role) + return {"Status": "Team role updated successfully."}, 200 @token_auth.login_required def delete(self, team_id, project_id): diff --git a/backend/api/tasks/actions.py b/backend/api/tasks/actions.py index 017dc8e0f9..16741a99d6 100644 --- a/backend/api/tasks/actions.py +++ b/backend/api/tasks/actions.py @@ -1,7 +1,7 @@ from flask_restful import Resource, current_app, request from schematics.exceptions import DataError -from backend.exceptions import NotFound, Forbidden +from backend.exceptions import Forbidden from backend.models.dtos.grid_dto import SplitTaskDTO from backend.models.postgis.utils import InvalidGeoJson from backend.services.grid.split_service import SplitService, SplitServiceError @@ -262,15 +262,6 @@ def post(self, project_id, task_id): return task.to_primitive(), 200 except MappingServiceError as e: return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 409 - except NotFound as e: # FLAGGED IF THIS CATCH IS NEEDED - return e.to_dict() - except Exception as e: # FLAGGED IF THIS CATCH IS NEEDED - error_msg = f"Task Lock API - unhandled error: {str(e)}" - current_app.logger.critical(error_msg) - return { - "Error": "Task unlock failed", - "SubCode": "InternalServerError", - }, 500 finally: # Refresh mapper level after mapping UserService.check_and_update_mapper_level(authenticated_user_id) diff --git a/backend/services/mapping_service.py b/backend/services/mapping_service.py index 4b2570cb6c..2a5c362c7c 100644 --- a/backend/services/mapping_service.py +++ b/backend/services/mapping_service.py @@ -198,10 +198,6 @@ def get_task_locked_by_user(project_id: int, task_id: int, user_id: int) -> Task :raises: MappingServiceError """ task = MappingService.get_task(task_id, project_id) - if task is None: # FLAGGED: CHECK IF THIS CONDITION IS EVER REACHED - raise NotFound( - sub_code="TASK_NOT_FOUND", project_id=project_id, task_id=task_id - ) current_state = TaskStatus(task.task_status) if current_state != TaskStatus.LOCKED_FOR_MAPPING: raise MappingServiceError( From a7d90344ae9e6bcfd70aca8bc6f6be07ec5393c3 Mon Sep 17 00:00:00 2001 From: Aadesh-Baral Date: Wed, 30 Aug 2023 11:10:40 +0545 Subject: [PATCH 10/10] Handle new error response format on frontend. ----------------------- After the introduction of new exception classes on backend the error response format is changed. As in new error response error subcode should be accessed on error.sub_code this commit handles this case. Also change in subcode to identify project is private and cannot be accessed by user has been addressed. --- frontend/src/network/tests/server-handlers.js | 2 +- frontend/src/utils/promise.js | 2 +- frontend/src/views/project.js | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/frontend/src/network/tests/server-handlers.js b/frontend/src/network/tests/server-handlers.js index c565d4cfb7..fdad0844f1 100644 --- a/frontend/src/network/tests/server-handlers.js +++ b/frontend/src/network/tests/server-handlers.js @@ -372,7 +372,7 @@ const faultyHandlers = [ return res.once( ctx.status(403), ctx.json({ - SubCode: `PrivateProject`, + error:{sub_code: `PRIVATE_PROJECT_NOT_ALLOWED`} }), ); }), diff --git a/frontend/src/utils/promise.js b/frontend/src/utils/promise.js index a9af63507a..2ee009b416 100644 --- a/frontend/src/utils/promise.js +++ b/frontend/src/utils/promise.js @@ -24,7 +24,7 @@ export async function handleErrors(response) { .clone() .json() .then((res) => { - text = res.SubCode || response.statusText; + text = res.error?.sub_code || res.SubCode || response.statusText; }); throw Error(text); } diff --git a/frontend/src/views/project.js b/frontend/src/views/project.js index 321bbd8826..ac80dd3fb8 100644 --- a/frontend/src/views/project.js +++ b/frontend/src/views/project.js @@ -198,7 +198,7 @@ export const ProjectDetailPage = () => { )} {error && ( <> - {error.message === 'PrivateProject' ? ( + {error.message === 'PRIVATE_PROJECT_NOT_ALLOWED' ? ( ) : (