diff --git a/api_app/_version.py b/api_app/_version.py index 4b2ce7df3..f658d0a64 100644 --- a/api_app/_version.py +++ b/api_app/_version.py @@ -1 +1 @@ -__version__ = "0.4.13" +__version__ = "0.4.14" diff --git a/api_app/api/routes/airlock.py b/api_app/api/routes/airlock.py index 31661bfc3..c0796b0c9 100644 --- a/api_app/api/routes/airlock.py +++ b/api_app/api/routes/airlock.py @@ -8,17 +8,15 @@ from api.dependencies.database import get_repository from api.dependencies.workspaces import get_workspace_by_id_from_path, get_deployed_workspace_by_id_from_path from api.dependencies.airlock import get_airlock_request_by_id_from_path from models.domain.airlock_request import AirlockRequestStatus, AirlockRequestType -from db.repositories.airlock_reviews import AirlockReviewRepository + from models.schemas.airlock_request_url import AirlockRequestTokenInResponse -from models.schemas.airlock_review import AirlockReviewInCreate, AirlockReviewInResponse from db.repositories.airlock_requests import AirlockRequestRepository -from models.schemas.airlock_request import AirlockRequestInCreate, AirlockRequestInResponse, AirlockRequestWithAllowedUserActionsInList +from models.schemas.airlock_request import AirlockRequestInCreate, AirlockRequestInResponse, AirlockRequestWithAllowedUserActionsInList, AirlockReviewInCreate from resources import strings from services.authentication import get_current_workspace_owner_or_researcher_user_or_airlock_manager, get_current_workspace_owner_or_researcher_user, get_current_airlock_manager_user -from .airlock_resource_helpers import save_airlock_review, save_and_publish_event_airlock_request, \ - update_status_and_publish_event_airlock_request, enrich_requests_with_allowed_actions, get_airlock_requests_by_user_and_workspace +from .airlock_resource_helpers import save_and_publish_event_airlock_request, update_and_publish_event_airlock_request, enrich_requests_with_allowed_actions, get_airlock_requests_by_user_and_workspace from services.airlock import validate_user_allowed_to_access_storage_account, \ get_account_by_request, get_airlock_request_container_sas_token, validate_request_status @@ -67,29 +65,27 @@ async def retrieve_airlock_request_by_id(airlock_request=Depends(get_airlock_req @airlock_workspace_router.post("/workspaces/{workspace_id}/requests/{airlock_request_id}/submit", status_code=status.HTTP_200_OK, response_model=AirlockRequestInResponse, name=strings.API_SUBMIT_AIRLOCK_REQUEST, dependencies=[Depends(get_current_workspace_owner_or_researcher_user), Depends(get_workspace_by_id_from_path)]) async def create_submit_request(airlock_request=Depends(get_airlock_request_by_id_from_path), user=Depends(get_current_workspace_owner_or_researcher_user), airlock_request_repo=Depends(get_repository(AirlockRequestRepository)), workspace=Depends(get_workspace_by_id_from_path)) -> AirlockRequestInResponse: - updated_resource = await update_status_and_publish_event_airlock_request(airlock_request, airlock_request_repo, user, AirlockRequestStatus.Submitted, workspace) + updated_resource = await update_and_publish_event_airlock_request(airlock_request, airlock_request_repo, user, AirlockRequestStatus.Submitted, workspace) return AirlockRequestInResponse(airlockRequest=updated_resource) @airlock_workspace_router.post("/workspaces/{workspace_id}/requests/{airlock_request_id}/cancel", status_code=status.HTTP_200_OK, response_model=AirlockRequestInResponse, name=strings.API_CANCEL_AIRLOCK_REQUEST, dependencies=[Depends(get_current_workspace_owner_or_researcher_user), Depends(get_workspace_by_id_from_path)]) async def create_cancel_request(airlock_request=Depends(get_airlock_request_by_id_from_path), user=Depends(get_current_workspace_owner_or_researcher_user), airlock_request_repo=Depends(get_repository(AirlockRequestRepository)), workspace=Depends(get_workspace_by_id_from_path)) -> AirlockRequestInResponse: - updated_resource = await update_status_and_publish_event_airlock_request(airlock_request, airlock_request_repo, user, AirlockRequestStatus.Cancelled, workspace) + updated_resource = await update_and_publish_event_airlock_request(airlock_request, airlock_request_repo, user, AirlockRequestStatus.Cancelled, workspace) return AirlockRequestInResponse(airlockRequest=updated_resource) -@airlock_workspace_router.post("/workspaces/{workspace_id}/requests/{airlock_request_id}/reviews", status_code=status.HTTP_200_OK, response_model=AirlockReviewInResponse, name=strings.API_REVIEW_AIRLOCK_REQUEST, dependencies=[Depends(get_current_airlock_manager_user), Depends(get_workspace_by_id_from_path)]) -async def create_airlock_review(airlock_review_input: AirlockReviewInCreate, airlock_request=Depends(get_airlock_request_by_id_from_path), user=Depends(get_current_airlock_manager_user), airlock_request_repo=Depends(get_repository(AirlockRequestRepository)), airlock_review_repo=Depends(get_repository(AirlockReviewRepository)), workspace=Depends(get_deployed_workspace_by_id_from_path)) -> AirlockReviewInResponse: - # Create the review model and save in cosmos +@airlock_workspace_router.post("/workspaces/{workspace_id}/requests/{airlock_request_id}/review", status_code=status.HTTP_200_OK, response_model=AirlockRequestInResponse, name=strings.API_REVIEW_AIRLOCK_REQUEST, dependencies=[Depends(get_current_airlock_manager_user), Depends(get_workspace_by_id_from_path)]) +async def create_airlock_review(airlock_review_input: AirlockReviewInCreate, airlock_request=Depends(get_airlock_request_by_id_from_path), user=Depends(get_current_airlock_manager_user), airlock_request_repo=Depends(get_repository(AirlockRequestRepository)), workspace=Depends(get_deployed_workspace_by_id_from_path)) -> AirlockRequestInResponse: try: - airlock_review = airlock_review_repo.create_airlock_review_item(airlock_review_input, workspace.id, airlock_request.id) + airlock_review = airlock_request_repo.create_airlock_review_item(airlock_review_input, user) except (ValidationError, ValueError) as e: logging.error(f"Failed creating airlock review model instance: {e}") raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) - # Update the airlock request in cosmos, send a status_changed event - await save_airlock_review(airlock_review, airlock_review_repo, user) + # Store review with new status in cosmos, and send status_changed event review_status = AirlockRequestStatus(airlock_review.reviewDecision.value) - await update_status_and_publish_event_airlock_request(airlock_request, airlock_request_repo, user, review_status, workspace) - return AirlockReviewInResponse(airlock_review=airlock_review) + updated_airlock_request = await update_and_publish_event_airlock_request(airlock_request=airlock_request, airlock_request_repo=airlock_request_repo, user=user, new_status=review_status, workspace=workspace, airlock_review=airlock_review) + return AirlockRequestInResponse(airlockRequest=updated_airlock_request) @airlock_workspace_router.get("/workspaces/{workspace_id}/requests/{airlock_request_id}/link", diff --git a/api_app/api/routes/airlock_resource_helpers.py b/api_app/api/routes/airlock_resource_helpers.py index 3e6e94ce1..e311a5334 100644 --- a/api_app/api/routes/airlock_resource_helpers.py +++ b/api_app/api/routes/airlock_resource_helpers.py @@ -5,10 +5,8 @@ from typing import List from fastapi import HTTPException from starlette import status -from db.repositories.airlock_reviews import AirlockReviewRepository -from models.domain.airlock_review import AirlockReview from db.repositories.airlock_requests import AirlockRequestRepository -from models.domain.airlock_request import AirlockActions, AirlockRequest, AirlockRequestStatus, AirlockRequestType +from models.domain.airlock_request import AirlockActions, AirlockRequest, AirlockRequestStatus, AirlockRequestType, AirlockReview from event_grid.event_sender import send_status_changed_event, send_airlock_notification_event from models.domain.authentication import User from models.domain.workspace import Workspace @@ -44,10 +42,10 @@ async def save_and_publish_event_airlock_request(airlock_request: AirlockRequest raise HTTPException(status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail=strings.EVENT_GRID_GENERAL_ERROR_MESSAGE) -async def update_status_and_publish_event_airlock_request(airlock_request: AirlockRequest, airlock_request_repo: AirlockRequestRepository, user: User, new_status: AirlockRequestStatus, workspace: Workspace, error_message: str = None): +async def update_and_publish_event_airlock_request(airlock_request: AirlockRequest, airlock_request_repo: AirlockRequestRepository, user: User, new_status: AirlockRequestStatus, workspace: Workspace, error_message: str = None, airlock_review: AirlockReview = None): try: logging.debug(f"Updating airlock request item: {airlock_request.id}") - updated_airlock_request = airlock_request_repo.update_airlock_request_status(airlock_request, new_status, user, error_message) + updated_airlock_request = airlock_request_repo.update_airlock_request(airlock_request=airlock_request, new_status=new_status, user=user, error_message=error_message, airlock_review=airlock_review) except Exception as e: logging.error(f'Failed updating airlock_request item {airlock_request}: {e}') # If the validation failed, the error was not related to the saving itself @@ -67,17 +65,6 @@ async def update_status_and_publish_event_airlock_request(airlock_request: Airlo raise HTTPException(status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail=strings.EVENT_GRID_GENERAL_ERROR_MESSAGE) -async def save_airlock_review(airlock_review: AirlockReview, airlock_review_repo: AirlockReviewRepository, user: User): - try: - logging.debug(f"Saving airlock review item: {airlock_review.id}") - airlock_review.user = user - airlock_review.updatedWhen = get_timestamp() - airlock_review_repo.save_item(airlock_review) - except Exception as e: - logging.error(f'Failed saving airlock request {airlock_review}: {e}') - raise HTTPException(status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail=strings.STATE_STORE_ENDPOINT_NOT_RESPONDING) - - def get_timestamp() -> float: return datetime.utcnow().timestamp() diff --git a/api_app/core/config.py b/api_app/core/config.py index 292860b2b..07063b221 100644 --- a/api_app/core/config.py +++ b/api_app/core/config.py @@ -25,7 +25,7 @@ STATE_STORE_DATABASE = "AzureTRE" STATE_STORE_RESOURCES_CONTAINER = "Resources" STATE_STORE_RESOURCE_TEMPLATES_CONTAINER = "ResourceTemplates" STATE_STORE_OPERATIONS_CONTAINER = "Operations" -STATE_STORE_AIRLOCK_RESOURCES_CONTAINER = "AirlockResources" +STATE_STORE_AIRLOCK_REQUESTS_CONTAINER = "Requests" SUBSCRIPTION_ID: str = config("SUBSCRIPTION_ID", default="") RESOURCE_GROUP_NAME: str = config("RESOURCE_GROUP_NAME", default="") diff --git a/api_app/db/repositories/airlock_requests.py b/api_app/db/repositories/airlock_requests.py index ce6c331a6..cb8e13850 100644 --- a/api_app/db/repositories/airlock_requests.py +++ b/api_app/db/repositories/airlock_requests.py @@ -11,19 +11,44 @@ from fastapi import HTTPException from pydantic import parse_obj_as from models.domain.authentication import User from db.errors import EntityDoesNotExist -from db.repositories.airlock_resources import AirlockResourceRepository -from models.domain.airlock_request import AirlockRequest, AirlockRequestStatus, AirlockRequestType -from models.schemas.airlock_request import AirlockRequestInCreate +from models.domain.airlock_request import AirlockRequest, AirlockRequestStatus, AirlockReview, AirlockReviewDecision, AirlockRequestHistoryItem, AirlockRequestType +from models.schemas.airlock_request import AirlockRequestInCreate, AirlockReviewInCreate +from core import config from resources import strings +from db.repositories.base import BaseRepository -class AirlockRequestRepository(AirlockResourceRepository): +class AirlockRequestRepository(BaseRepository): def __init__(self, client: CosmosClient): - super().__init__(client) + super().__init__(client, config.STATE_STORE_AIRLOCK_REQUESTS_CONTAINER) + + @staticmethod + def get_resource_base_spec_params(): + return {"tre_id": config.TRE_ID} + + def get_timestamp(self) -> float: + return datetime.utcnow().timestamp() + + def update_airlock_request_item(self, original_request: AirlockRequest, new_request: AirlockRequest, user: User, request_properties: dict) -> AirlockRequest: + history_item = AirlockRequestHistoryItem( + resourceVersion=original_request.resourceVersion, + updatedWhen=original_request.updatedWhen, + user=original_request.user, + properties=request_properties + ) + new_request.history.append(history_item) + + # now update the request props + new_request.resourceVersion = new_request.resourceVersion + 1 + new_request.user = user + new_request.updatedWhen = self.get_timestamp() + + self.update_item(new_request) + return new_request @staticmethod def airlock_requests_query(): - return 'SELECT * FROM c WHERE c.resourceType = "airlock-request"' + return 'SELECT * FROM c' def validate_status_update(self, current_status: AirlockRequestStatus, new_status: AirlockRequestStatus): # Cannot change status from approved @@ -69,13 +94,14 @@ class AirlockRequestRepository(AirlockResourceRepository): businessJustification=airlock_request_input.businessJustification, requestType=airlock_request_input.requestType, creationTime=datetime.utcnow().timestamp(), - properties=resource_spec_parameters + properties=resource_spec_parameters, + reviews=[] ) return airlock_request def get_airlock_requests(self, workspace_id: str, user_id: str = None, type: AirlockRequestType = None, status: AirlockRequestStatus = None) -> List[AirlockRequest]: - query = self.airlock_requests_query() + f' AND c.workspaceId = "{workspace_id}"' + query = self.airlock_requests_query() + f' where c.workspaceId = "{workspace_id}"' # optional filters if user_id: @@ -100,16 +126,35 @@ class AirlockRequestRepository(AirlockResourceRepository): raise EntityDoesNotExist return parse_obj_as(AirlockRequest, airlock_requests) - def update_airlock_request_status(self, airlock_request: AirlockRequest, new_status: AirlockRequestStatus, user: User, error_message: str = None) -> AirlockRequest: + def update_airlock_request(self, airlock_request: AirlockRequest, new_status: AirlockRequestStatus, user: User, error_message: str = None, airlock_review: AirlockReview = None) -> AirlockRequest: current_status = airlock_request.status if self.validate_status_update(current_status, new_status): updated_request = copy.deepcopy(airlock_request) updated_request.status = new_status if new_status == AirlockRequestStatus.Failed: updated_request.errorMessage = error_message - return self.update_airlock_resource_item(airlock_request, updated_request, user, {"previousStatus": current_status}) + if airlock_review is not None: + if updated_request.reviews is None: + updated_request.reviews = [airlock_review] + else: + updated_request.reviews.append(airlock_review) + return self.update_airlock_request_item(airlock_request, updated_request, user, {"previousStatus": current_status}) else: raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=strings.AIRLOCK_REQUEST_ILLEGAL_STATUS_CHANGE) def get_airlock_request_spec_params(self): return self.get_resource_base_spec_params() + + def create_airlock_review_item(self, airlock_review_input: AirlockReviewInCreate, reviewer: User) -> AirlockReview: + full_airlock_review_id = str(uuid.uuid4()) + airlock_review_decision_from_bool = AirlockReviewDecision.Approved if airlock_review_input.approval else AirlockReviewDecision.Rejected + + airlock_review = AirlockReview( + id=full_airlock_review_id, + dateCreated=self.get_timestamp(), + reviewDecision=airlock_review_decision_from_bool, + decisionExplanation=airlock_review_input.decisionExplanation, + reviewer=reviewer + ) + + return airlock_review diff --git a/api_app/db/repositories/airlock_resources.py b/api_app/db/repositories/airlock_resources.py deleted file mode 100644 index 48f754bf2..000000000 --- a/api_app/db/repositories/airlock_resources.py +++ /dev/null @@ -1,35 +0,0 @@ -from datetime import datetime -from azure.cosmos import CosmosClient -from models.domain.airlock_resource import AirlockResource, AirlockResourceHistoryItem -from models.domain.authentication import User -from core import config -from db.repositories.base import BaseRepository - - -class AirlockResourceRepository(BaseRepository): - def __init__(self, client: CosmosClient): - super().__init__(client, config.STATE_STORE_AIRLOCK_RESOURCES_CONTAINER) - - @staticmethod - def get_resource_base_spec_params(): - return {"tre_id": config.TRE_ID} - - def get_timestamp(self) -> float: - return datetime.utcnow().timestamp() - - def update_airlock_resource_item(self, original_resource: AirlockResource, new_resource: AirlockResource, user: User, resource_properties: dict) -> AirlockResource: - history_item = AirlockResourceHistoryItem( - resourceVersion=original_resource.resourceVersion, - updatedWhen=original_resource.updatedWhen, - user=original_resource.user, - properties=resource_properties - ) - new_resource.history.append(history_item) - - # now update the resource props - new_resource.resourceVersion = new_resource.resourceVersion + 1 - new_resource.user = user - new_resource.updatedWhen = self.get_timestamp() - - self.update_item(new_resource) - return new_resource diff --git a/api_app/db/repositories/airlock_reviews.py b/api_app/db/repositories/airlock_reviews.py deleted file mode 100644 index 2ae09b24e..000000000 --- a/api_app/db/repositories/airlock_reviews.py +++ /dev/null @@ -1,24 +0,0 @@ -import uuid -from azure.cosmos import CosmosClient -from models.domain.airlock_review import AirlockReview, AirlockReviewDecision -from models.schemas.airlock_review import AirlockReviewInCreate -from db.repositories.airlock_resources import AirlockResourceRepository - - -class AirlockReviewRepository(AirlockResourceRepository): - def __init__(self, client: CosmosClient): - super().__init__(client) - - def create_airlock_review_item(self, airlock_review_input: AirlockReviewInCreate, workspace_id: str, request_id: str) -> AirlockReview: - full_airlock_review_id = str(uuid.uuid4()) - airlock_review_decision_from_bool = AirlockReviewDecision.Approved if airlock_review_input.approval else AirlockReviewDecision.Rejected - - airlock_review = AirlockReview( - id=full_airlock_review_id, - workspaceId=workspace_id, - requestId=request_id, - reviewDecision=airlock_review_decision_from_bool, - decisionExplanation=airlock_review_input.decisionExplanation - ) - - return airlock_review diff --git a/api_app/models/domain/airlock_request.py b/api_app/models/domain/airlock_request.py index f07158a95..d6e6df550 100644 --- a/api_app/models/domain/airlock_request.py +++ b/api_app/models/domain/airlock_request.py @@ -3,7 +3,7 @@ from enum import Enum from pydantic import Field from pydantic.schema import Optional from resources import strings -from models.domain.airlock_resource import AirlockResource, AirlockResourceType +from models.domain.azuretremodel import AzureTREModel class AirlockRequestStatus(str, Enum): @@ -35,15 +35,46 @@ class AirlockActions(str, Enum): Submit = strings.AIRLOCK_ACTION_SUBMIT -class AirlockRequest(AirlockResource): +class AirlockReviewDecision(str, Enum): + Approved = strings.AIRLOCK_RESOURCE_STATUS_APPROVAL_INPROGRESS + Rejected = strings.AIRLOCK_RESOURCE_STATUS_REJECTION_INPROGRESS + + +class AirlockReview(AzureTREModel): + """ + Airlock review + """ + id: str = Field(title="Id", description="GUID identifying the review") + reviewer: dict = {} + dateCreated: float = 0 + reviewDecision: AirlockReviewDecision = Field("", title="Airlock review decision") + decisionExplanation: str = Field(False, title="Explanation why the request was approved/rejected") + + +class AirlockRequestHistoryItem(AzureTREModel): + """ + Resource History Item - to preserve history of resource properties + """ + resourceVersion: int + updatedWhen: float + user: dict = {} + properties: dict = {} + + +class AirlockRequest(AzureTREModel): """ Airlock request """ + id: str = Field(title="Id", description="GUID identifying the resource") + resourceVersion: int = 0 + user: dict = {} + updatedWhen: float = 0 + history: List[AirlockRequestHistoryItem] = [] workspaceId: str = Field("", title="Workspace ID", description="Service target Workspace id") - resourceType = AirlockResourceType.AirlockRequest requestType: AirlockRequestType = Field("", title="Airlock request type") files: List[str] = Field([], title="Files of the request") businessJustification: str = Field("Business Justifications", title="Explanation that will be provided to the request reviewer") status = AirlockRequestStatus.Draft creationTime: float = Field(None, title="Creation time of the request") errorMessage: Optional[str] = Field(title="Present only if the request have failed, provides the reason of the failure.") + reviews: Optional[List[AirlockReview]] diff --git a/api_app/models/domain/airlock_resource.py b/api_app/models/domain/airlock_resource.py deleted file mode 100644 index 5cc4572ea..000000000 --- a/api_app/models/domain/airlock_resource.py +++ /dev/null @@ -1,35 +0,0 @@ -from enum import Enum -from typing import List -from pydantic import Field -from models.domain.azuretremodel import AzureTREModel -from resources import strings - - -class AirlockResourceType(str, Enum): - """ - Type of resource to create - """ - AirlockRequest = strings.AIRLOCK_RESOURCE_TYPE_REQUEST - AirlockReview = strings.AIRLOCK_RESOURCE_TYPE_REVIEW - - -class AirlockResourceHistoryItem(AzureTREModel): - """ - Resource History Item - to preserve history of resource properties - """ - resourceVersion: int - updatedWhen: float - user: dict = {} - properties: dict = {} - - -class AirlockResource(AzureTREModel): - """ - Resource request - """ - id: str = Field(title="Id", description="GUID identifying the resource") - resourceType: AirlockResourceType - resourceVersion: int = 0 - user: dict = {} - updatedWhen: float = 0 - history: List[AirlockResourceHistoryItem] = [] diff --git a/api_app/models/domain/airlock_review.py b/api_app/models/domain/airlock_review.py deleted file mode 100644 index 6cd81c990..000000000 --- a/api_app/models/domain/airlock_review.py +++ /dev/null @@ -1,20 +0,0 @@ -from pydantic import Field -from enum import Enum -from models.domain.airlock_resource import AirlockResource, AirlockResourceType -from resources import strings - - -class AirlockReviewDecision(str, Enum): - Approved = strings.AIRLOCK_RESOURCE_STATUS_APPROVAL_INPROGRESS - Rejected = strings.AIRLOCK_RESOURCE_STATUS_REJECTION_INPROGRESS - - -class AirlockReview(AirlockResource): - """ - Airlock review - """ - workspaceId: str = Field("", title="Workspace ID", description="Service target Workspace id") - requestId: str = Field("", title="Airlock Request ID", description="Service target Airlock id") - resourceType = AirlockResourceType.AirlockReview - reviewDecision: AirlockReviewDecision = Field("", title="Airlock review decision") - decisionExplanation: str = Field(False, title="Explanation why the request was approved/rejected") diff --git a/api_app/models/schemas/airlock_request.py b/api_app/models/schemas/airlock_request.py index 9d0a5c9d0..0ad9f5fca 100644 --- a/api_app/models/schemas/airlock_request.py +++ b/api_app/models/schemas/airlock_request.py @@ -2,10 +2,17 @@ import uuid from datetime import datetime from typing import List from pydantic import BaseModel, Field -from models.domain.airlock_resource import AirlockResourceType from models.domain.airlock_request import AirlockActions, AirlockRequest, AirlockRequestType +def get_sample_airlock_review(airlock_review_id: str) -> dict: + return { + "reviewId": airlock_review_id, + "reviewDecision": "Describe why the request was approved/rejected", + "decisionExplanation": "Describe why the request was approved/rejected" + } + + def get_sample_airlock_request(workspace_id: str, airlock_request_id: str) -> dict: return { "requestId": airlock_request_id, @@ -15,7 +22,9 @@ def get_sample_airlock_request(workspace_id: str, airlock_request_id: str) -> di "files": [], "businessJustification": "some business justification", "creationTime": datetime.utcnow().timestamp(), - "resourceType": AirlockResourceType.AirlockRequest + "reviews": [ + get_sample_airlock_review("29990431-5451-40e7-a58a-02e2b7c3d7c8"), + get_sample_airlock_review("02dc0f29-351a-43ec-87e7-3dd2b5177b7f")] } @@ -73,3 +82,16 @@ class AirlockRequestInCreate(BaseModel): "businessJustification": "some business justification" } } + + +class AirlockReviewInCreate(BaseModel): + approval: bool = Field("", title="Airlock review decision", description="Airlock review decision") + decisionExplanation: str = Field("Decision Explanation", title="Explanation of the reviewer for the reviews decision") + + class Config: + schema_extra = { + "example": { + "approval": "True", + "decisionExplanation": "the reason why this request was approved/rejected" + } + } diff --git a/api_app/models/schemas/airlock_review.py b/api_app/models/schemas/airlock_review.py deleted file mode 100644 index 5d26ee48a..000000000 --- a/api_app/models/schemas/airlock_review.py +++ /dev/null @@ -1,38 +0,0 @@ -from pydantic import BaseModel, Field -from models.domain.airlock_review import AirlockReview -from models.domain.airlock_resource import AirlockResourceType - - -def get_sample_airlock_review(workspace_id: str, airlock_request_id: str, airlock_review_id: str) -> dict: - return { - "reviewId": airlock_review_id, - "requestId": airlock_request_id, - "workspaceId": workspace_id, - "reviewDecision": "Describe why the request was approved/rejected", - "decisionExplanation": "Describe why the request was approved/rejected", - "resourceType": AirlockResourceType.AirlockReview - } - - -class AirlockReviewInResponse(BaseModel): - airlock_review: AirlockReview - - class Config: - schema_extra = { - "example": { - "airlock_review": get_sample_airlock_review("933ad738-7265-4b5f-9eae-a1a62928772e", "121e921f-a4aa-44b3-90a9-e8da030495ef", "5c8c3430-b362-4e38-8270-441ca4381739") - } - } - - -class AirlockReviewInCreate(BaseModel): - approval: bool = Field("", title="Airlock review decision", description="Airlock review decision") - decisionExplanation: str = Field("Decision Explanation", title="Explanation of the reviewer for the reviews decision") - - class Config: - schema_extra = { - "example": { - "approval": "True", - "decisionExplanation": "the reason why this request was approved/rejected" - } - } diff --git a/api_app/service_bus/airlock_request_status_update.py b/api_app/service_bus/airlock_request_status_update.py index 72f9a2cf3..ce19631a6 100644 --- a/api_app/service_bus/airlock_request_status_update.py +++ b/api_app/service_bus/airlock_request_status_update.py @@ -7,7 +7,7 @@ from pydantic import ValidationError, parse_obj_as from api.dependencies.database import get_db_client from api.dependencies.airlock import get_airlock_request_by_id_from_path -from api.routes.airlock_resource_helpers import update_status_and_publish_event_airlock_request +from api.routes.airlock_resource_helpers import update_and_publish_event_airlock_request from db.repositories.workspaces import WorkspaceRepository from models.domain.airlock_request import AirlockRequestStatus from db.repositories.airlock_requests import AirlockRequestRepository @@ -65,7 +65,7 @@ async def update_status_in_database(airlock_request_repo: AirlockRequestReposito if airlock_request.status == current_status: workspace = workspace_repo.get_workspace_by_id(airlock_request.workspaceId) # update to new status and send to event grid - await update_status_and_publish_event_airlock_request(airlock_request=airlock_request, airlock_request_repo=airlock_request_repo, user=airlock_request.user, new_status=new_status, workspace=workspace, error_message=error_message) + await update_and_publish_event_airlock_request(airlock_request=airlock_request, airlock_request_repo=airlock_request_repo, user=airlock_request.user, new_status=new_status, workspace=workspace, error_message=error_message) result = True else: error_string = strings.STEP_RESULT_MESSAGE_STATUS_DOES_NOT_MATCH.format(airlock_request_id, current_status, airlock_request.status) diff --git a/api_app/tests_ma/test_api/test_routes/test_airlock.py b/api_app/tests_ma/test_api/test_routes/test_airlock.py index b92f666b8..5d8f0782f 100644 --- a/api_app/tests_ma/test_api/test_routes/test_airlock.py +++ b/api_app/tests_ma/test_api/test_routes/test_airlock.py @@ -1,12 +1,10 @@ import pytest from mock import patch from fastapi import status -from models.domain.airlock_review import AirlockReview, AirlockReviewDecision from db.errors import EntityDoesNotExist, UnableToAccessDatabase from azure.cosmos.exceptions import CosmosResourceNotFoundError -from models.domain.airlock_request import AirlockRequest, AirlockRequestStatus +from models.domain.airlock_request import AirlockRequest, AirlockRequestStatus, AirlockReview, AirlockReviewDecision from azure.core.exceptions import HttpResponseError - from models.domain.workspace import Workspace from resources import strings from services.authentication import get_current_workspace_owner_or_researcher_user, get_current_workspace_owner_or_researcher_user_or_airlock_manager, get_current_airlock_manager_user @@ -43,11 +41,22 @@ def sample_airlock_request_object(status=AirlockRequestStatus.Draft, airlock_req return airlock_request +def sample_airlock_request_object_with_review(status=AirlockRequestStatus.Draft, airlock_request_id=AIRLOCK_REQUEST_ID, workspace_id=WORKSPACE_ID): + airlock_request = AirlockRequest( + id=airlock_request_id, + workspaceId=workspace_id, + businessJustification="test business justification", + requestType="import", + status=status, + reviews=[sample_airlock_review_object()] + ) + return airlock_request + + def sample_airlock_review_object(): airlock_review = AirlockReview( id=AIRLOCK_REVIEW_ID, - workspaceId=WORKSPACE_ID, - requestId=AIRLOCK_REQUEST_ID, + dateCreated=1660231576.328734, reviewDecision=AirlockReviewDecision.Approved, decisionExplanation="test explaination" ) @@ -144,7 +153,7 @@ class TestAirlockRoutesThatRequireOwnerOrResearcherRights(): # [POST] /workspaces/{workspace_id}/requests/{airlock_request_id}/submit @patch("api.routes.airlock.AirlockRequestRepository.read_item_by_id", return_value=sample_airlock_request_object()) - @patch("api.routes.airlock.update_status_and_publish_event_airlock_request", return_value=sample_airlock_request_object(status=AirlockRequestStatus.Submitted)) + @patch("api.routes.airlock.update_and_publish_event_airlock_request", return_value=sample_airlock_request_object(status=AirlockRequestStatus.Submitted)) async def test_post_submit_airlock_request_submitts_airlock_request_returns_200(self, _, __, app, client): response = await client.post(app.url_path_for(strings.API_SUBMIT_AIRLOCK_REQUEST, workspace_id=WORKSPACE_ID, airlock_request_id=AIRLOCK_REQUEST_ID)) assert response.status_code == status.HTTP_200_OK @@ -157,13 +166,13 @@ class TestAirlockRoutesThatRequireOwnerOrResearcherRights(): assert response.status_code == status.HTTP_404_NOT_FOUND @patch("api.routes.airlock.AirlockRequestRepository.read_item_by_id", side_effect=UnableToAccessDatabase) - @patch("api.routes.airlock.update_status_and_publish_event_airlock_request", side_effect=UnableToAccessDatabase) + @patch("api.routes.airlock.update_and_publish_event_airlock_request", side_effect=UnableToAccessDatabase) async def test_post_submit_airlock_request_with_state_store_endpoint_not_responding_returns_503(self, _, __, app, client): response = await client.post(app.url_path_for(strings.API_SUBMIT_AIRLOCK_REQUEST, workspace_id=WORKSPACE_ID, airlock_request_id=AIRLOCK_REQUEST_ID)) assert response.status_code == status.HTTP_503_SERVICE_UNAVAILABLE @patch("api.routes.airlock.AirlockRequestRepository.read_item_by_id", return_value=sample_airlock_request_object()) - @patch("api.routes.airlock.AirlockRequestRepository.update_airlock_request_status") + @patch("api.routes.airlock.AirlockRequestRepository.update_airlock_request") @patch("api.routes.airlock.AirlockRequestRepository.delete_item") @patch("event_grid.event_sender.send_status_changed_event", side_effect=HttpResponseError) async def test_post_submit_airlock_request_with_event_grid_not_responding_returns_503(self, _, __, ___, ____, app, client): @@ -178,7 +187,7 @@ class TestAirlockRoutesThatRequireOwnerOrResearcherRights(): # [POST] /workspaces/{workspace_id}/requests/{airlock_request_id}/cancel @patch("api.routes.airlock.AirlockRequestRepository.read_item_by_id", return_value=sample_airlock_request_object()) - @patch("api.routes.airlock.update_status_and_publish_event_airlock_request", return_value=sample_airlock_request_object(status=AirlockRequestStatus.Cancelled)) + @patch("api.routes.airlock.update_and_publish_event_airlock_request", return_value=sample_airlock_request_object(status=AirlockRequestStatus.Cancelled)) async def test_post_cancel_airlock_request_canceles_request_returns_200(self, _, __, app, client): response = await client.post(app.url_path_for(strings.API_CANCEL_AIRLOCK_REQUEST, workspace_id=WORKSPACE_ID, airlock_request_id=AIRLOCK_REQUEST_ID)) assert response.status_code == status.HTTP_200_OK @@ -239,47 +248,39 @@ class TestAirlockRoutesThatRequireAirlockManagerRights(): with patch("api.routes.airlock.AirlockRequestRepository.create_airlock_request_item", return_value=sample_airlock_request_object()), \ patch("api.routes.workspaces.OperationRepository.resource_has_deployed_operation"), \ patch("api.routes.airlock.AirlockRequestRepository.save_item"), \ - patch("api.routes.airlock.AirlockReviewRepository.save_item"), \ patch("api.dependencies.workspaces.WorkspaceRepository.get_workspace_by_id"): yield app.dependency_overrides = {} # [POST] /workspaces/{workspace_id}/requests/{airlock_request_id}/review @patch("api.routes.airlock.AirlockRequestRepository.read_item_by_id", return_value=sample_airlock_request_object(status=AirlockRequestStatus.InReview)) - @patch("api.routes.airlock.AirlockReviewRepository.create_airlock_review_item", return_value=sample_airlock_review_object()) - @patch("api.routes.airlock.update_status_and_publish_event_airlock_request", return_value=sample_airlock_request_object(status=AirlockRequestStatus.Approved)) - @patch("api.routes.airlock.AirlockReviewRepository.save_item") + @patch("api.routes.airlock.AirlockRequestRepository.create_airlock_review_item", return_value=sample_airlock_review_object()) + @patch("api.routes.airlock.update_and_publish_event_airlock_request", return_value=sample_airlock_request_object_with_review(status=AirlockRequestStatus.Approved)) + @patch("api.routes.airlock.AirlockRequestRepository.save_item") async def test_post_create_airlock_review_approves_airlock_request_returns_200(self, _, __, ___, ____, app, client, sample_airlock_review_input_data): response = await client.post(app.url_path_for(strings.API_REVIEW_AIRLOCK_REQUEST, workspace_id=WORKSPACE_ID, airlock_request_id=AIRLOCK_REQUEST_ID), json=sample_airlock_review_input_data) assert response.status_code == status.HTTP_200_OK - assert response.json()["airlock_review"]["id"] == AIRLOCK_REVIEW_ID - assert response.json()["airlock_review"]["reviewDecision"] == AirlockReviewDecision.Approved + assert response.json()["airlockRequest"]["reviews"][0]["id"] == AIRLOCK_REVIEW_ID + assert response.json()["airlockRequest"]["reviews"][0]["reviewDecision"] == AirlockReviewDecision.Approved @patch("api.routes.airlock.AirlockRequestRepository.read_item_by_id", return_value=sample_airlock_request_object(status=AirlockRequestStatus.InReview)) - @patch("api.routes.airlock.AirlockReviewRepository.create_airlock_review_item", side_effect=ValueError) + @patch("api.routes.airlock.AirlockRequestRepository.create_airlock_review_item", side_effect=ValueError) async def test_post_create_airlock_review_input_is_malformed_returns_400(self, _, __, app, client, sample_airlock_review_input_data): response = await client.post(app.url_path_for(strings.API_REVIEW_AIRLOCK_REQUEST, workspace_id=WORKSPACE_ID, airlock_request_id=AIRLOCK_REQUEST_ID), json=sample_airlock_review_input_data) assert response.status_code == status.HTTP_400_BAD_REQUEST @patch("api.routes.airlock.AirlockRequestRepository.read_item_by_id", return_value=sample_airlock_request_object(status=AirlockRequestStatus.InReview)) - @patch("api.routes.airlock.AirlockReviewRepository.create_airlock_review_item", return_value=sample_airlock_review_object()) - @patch("api.routes.airlock.AirlockReviewRepository.save_item", side_effect=UnableToAccessDatabase) - async def test_post_create_airlock_review_with_state_store_endpoint_not_responding_returns_503(self, _, __, ___, app, client, sample_airlock_review_input_data): - response = await client.post(app.url_path_for(strings.API_REVIEW_AIRLOCK_REQUEST, workspace_id=WORKSPACE_ID, airlock_request_id=AIRLOCK_REQUEST_ID), json=sample_airlock_review_input_data) - assert response.status_code == status.HTTP_503_SERVICE_UNAVAILABLE - - @patch("api.routes.airlock.AirlockRequestRepository.read_item_by_id", return_value=sample_airlock_request_object(status=AirlockRequestStatus.InReview)) - @patch("api.routes.airlock.AirlockReviewRepository.create_airlock_review_item", return_value=sample_airlock_review_object()) - @patch("api.routes.airlock.AirlockReviewRepository.save_item") - @patch("api.routes.airlock.AirlockRequestRepository.update_airlock_request_status") + @patch("api.routes.airlock.AirlockRequestRepository.create_airlock_review_item", return_value=sample_airlock_review_object()) + @patch("api.routes.airlock.AirlockRequestRepository.save_item") + @patch("api.routes.airlock.AirlockRequestRepository.update_airlock_request") @patch("event_grid.event_sender.send_status_changed_event", side_effect=HttpResponseError) async def test_post_create_airlock_review_with_event_grid_not_responding_returns_503(self, _, __, ___, ____, _____, app, client, sample_airlock_review_input_data): response = await client.post(app.url_path_for(strings.API_REVIEW_AIRLOCK_REQUEST, workspace_id=WORKSPACE_ID, airlock_request_id=AIRLOCK_REQUEST_ID), json=sample_airlock_review_input_data) assert response.status_code == status.HTTP_503_SERVICE_UNAVAILABLE @patch("api.routes.airlock.AirlockRequestRepository.read_item_by_id", return_value=sample_airlock_request_object(status=AirlockRequestStatus.InReview)) - @patch("api.routes.airlock.AirlockReviewRepository.create_airlock_review_item", return_value=sample_airlock_review_object()) - @patch("api.routes.airlock.AirlockReviewRepository.save_item") + @patch("api.routes.airlock.AirlockRequestRepository.create_airlock_review_item", return_value=sample_airlock_review_object()) + @patch("api.routes.airlock.AirlockRequestRepository.save_item") @patch("api.routes.airlock.AirlockRequestRepository.validate_status_update", return_value=False) async def test_post_create_airlock_review_with_illegal_status_change_returns_400(self, _, __, ___, ____, app, client, sample_airlock_review_input_data): response = await client.post(app.url_path_for(strings.API_REVIEW_AIRLOCK_REQUEST, workspace_id=WORKSPACE_ID, airlock_request_id=AIRLOCK_REQUEST_ID), json=sample_airlock_review_input_data) diff --git a/api_app/tests_ma/test_api/test_routes/test_airlock_resource_helpers.py b/api_app/tests_ma/test_api/test_routes/test_airlock_resource_helpers.py index 24f102119..baaaa20a7 100644 --- a/api_app/tests_ma/test_api/test_routes/test_airlock_resource_helpers.py +++ b/api_app/tests_ma/test_api/test_routes/test_airlock_resource_helpers.py @@ -3,15 +3,12 @@ import pytest from mock import AsyncMock, patch, MagicMock from models.domain.events import AirlockNotificationData, StatusChangedData -from api.routes.airlock_resource_helpers import save_airlock_review, save_and_publish_event_airlock_request, \ - update_status_and_publish_event_airlock_request, get_airlock_requests_by_user_and_workspace, get_allowed_actions -from db.repositories.airlock_reviews import AirlockReviewRepository +from api.routes.airlock_resource_helpers import save_and_publish_event_airlock_request, \ + update_and_publish_event_airlock_request, get_airlock_requests_by_user_and_workspace, get_allowed_actions from db.repositories.airlock_requests import AirlockRequestRepository from models.domain.workspace import Workspace from tests_ma.test_api.conftest import create_test_user, create_workspace_airlock_manager_user -from models.domain.airlock_review import AirlockReview, AirlockReviewDecision -from models.domain.airlock_resource import AirlockResourceType -from models.domain.airlock_request import AirlockRequest, AirlockRequestStatus, AirlockRequestType, AirlockActions +from models.domain.airlock_request import AirlockRequest, AirlockRequestStatus, AirlockRequestType, AirlockReview, AirlockReviewDecision, AirlockActions from azure.eventgrid import EventGridEvent from api.routes.airlock import create_airlock_review, create_cancel_request, create_submit_request @@ -32,16 +29,9 @@ def airlock_request_repo_mock(): yield AirlockRequestRepository(cosmos_client_mock) -@pytest.fixture -def airlock_review_repo_mock(): - with patch('azure.cosmos.CosmosClient') as cosmos_client_mock: - yield AirlockReviewRepository(cosmos_client_mock) - - def sample_airlock_request(status=AirlockRequestStatus.Draft): airlock_request = AirlockRequest( id=AIRLOCK_REQUEST_ID, - resourceType=AirlockResourceType.AirlockRequest, workspaceId=WORKSPACE_ID, requestType=AirlockRequestType.Import, files=[], @@ -74,9 +64,6 @@ def sample_airlock_notification_event(status="draft"): def sample_airlock_review(review_decision=AirlockReviewDecision.Approved): airlock_review = AirlockReview( id=AIRLOCK_REVIEW_ID, - resourceType=AirlockResourceType.AirlockReview, - workspaceId=WORKSPACE_ID, - requestId=AIRLOCK_REQUEST_ID, reviewDecision=review_decision, decisionExplanation="test explaination" ) @@ -172,24 +159,24 @@ async def test_save_and_publish_event_airlock_request_raises_417_if_email_not_pr @patch("event_grid.helpers.EventGridPublisherClient", return_value=AsyncMock()) @patch("services.aad_authentication.AzureADAuthorization.get_workspace_role_assignment_details", return_value={"researcher_emails": ["researcher@outlook.com"], "owner_emails": ["owner@outlook.com"]}) -async def test_update_status_and_publish_event_airlock_request_updates_item(_, event_grid_publisher_client_mock, - airlock_request_repo_mock): +async def test_update_and_publish_event_airlock_request_updates_item(_, event_grid_publisher_client_mock, + airlock_request_repo_mock): airlock_request_mock = sample_airlock_request() updated_airlock_request_mock = sample_airlock_request(status=AirlockRequestStatus.Submitted) status_changed_event_mock = sample_status_changed_event(status="submitted") airlock_notification_event_mock = sample_airlock_notification_event(status="submitted") - airlock_request_repo_mock.update_airlock_request_status = MagicMock(return_value=updated_airlock_request_mock) + airlock_request_repo_mock.update_airlock_request = MagicMock(return_value=updated_airlock_request_mock) event_grid_sender_client_mock = event_grid_publisher_client_mock.return_value event_grid_sender_client_mock.send = AsyncMock() - actual_updated_airlock_request = await update_status_and_publish_event_airlock_request( + actual_updated_airlock_request = await update_and_publish_event_airlock_request( airlock_request=airlock_request_mock, airlock_request_repo=airlock_request_repo_mock, user=create_test_user(), new_status=AirlockRequestStatus.Submitted, workspace=sample_workspace()) - airlock_request_repo_mock.update_airlock_request_status.assert_called_once() + airlock_request_repo_mock.update_airlock_request.assert_called_once() assert (actual_updated_airlock_request == updated_airlock_request_mock) assert event_grid_sender_client_mock.send.call_count == 2 @@ -201,11 +188,11 @@ async def test_update_status_and_publish_event_airlock_request_updates_item(_, e @patch("services.aad_authentication.AzureADAuthorization.get_workspace_role_assignment_details", return_value={"researcher_emails": ["researcher@outlook.com"], "owner_emails": ["owner@outlook.com"]}) -async def test_update_status_and_publish_event_airlock_request_raises_400_if_status_update_invalid(_, airlock_request_repo_mock): +async def test_update_and_publish_event_airlock_request_raises_400_if_status_update_invalid(_, airlock_request_repo_mock): airlock_request_mock = sample_airlock_request() with pytest.raises(HTTPException) as ex: - await update_status_and_publish_event_airlock_request( + await update_and_publish_event_airlock_request( airlock_request=airlock_request_mock, airlock_request_repo=airlock_request_repo_mock, user=create_test_user(), @@ -217,16 +204,16 @@ async def test_update_status_and_publish_event_airlock_request_raises_400_if_sta @patch("event_grid.helpers.EventGridPublisherClient", return_value=AsyncMock()) @patch("services.aad_authentication.AzureADAuthorization.get_workspace_role_assignment_details", return_value={"researcher_emails": ["researcher@outlook.com"], "owner_emails": ["owner@outlook.com"]}) -async def test_update_status_and_publish_event_airlock_request_raises_503_if_publish_event_fails(_, event_grid_publisher_client_mock, - airlock_request_repo_mock): +async def test_update_and_publish_event_airlock_request_raises_503_if_publish_event_fails(_, event_grid_publisher_client_mock, + airlock_request_repo_mock): airlock_request_mock = sample_airlock_request() updated_airlock_request_mock = sample_airlock_request(status=AirlockRequestStatus.Submitted) - airlock_request_repo_mock.update_airlock_request_status = MagicMock(return_value=updated_airlock_request_mock) + airlock_request_repo_mock.update_airlock_request = MagicMock(return_value=updated_airlock_request_mock) event_grid_sender_client_mock = event_grid_publisher_client_mock.return_value event_grid_sender_client_mock.send = AsyncMock(side_effect=Exception) with pytest.raises(HTTPException) as ex: - await update_status_and_publish_event_airlock_request( + await update_and_publish_event_airlock_request( airlock_request=airlock_request_mock, airlock_request_repo=airlock_request_repo_mock, user=create_test_user(), @@ -235,66 +222,40 @@ async def test_update_status_and_publish_event_airlock_request_raises_503_if_pub assert ex.value.status_code == status.HTTP_503_SERVICE_UNAVAILABLE -async def test_save_airlock_review_saves_item(airlock_review_repo_mock): - airlock_review_mock = sample_airlock_review() - airlock_review_repo_mock.save_item = MagicMock(return_value=None) - - await save_airlock_review( - airlock_review=airlock_review_mock, - airlock_review_repo=airlock_review_repo_mock, - user=create_test_user() - ) - - airlock_review_repo_mock.save_item.assert_called_once_with(airlock_review_mock) - - -async def test_save_airlock_review_raises_503_if_save_to_db_fails(airlock_review_repo_mock): - airlock_review_mock = sample_airlock_review() - airlock_review_repo_mock.save_item = MagicMock(side_effect=Exception) - with pytest.raises(HTTPException) as ex: - await save_airlock_review( - airlock_review=airlock_review_mock, - airlock_review_repo=airlock_review_repo_mock, - user=create_test_user() - ) - - assert ex.value.status_code == status.HTTP_503_SERVICE_UNAVAILABLE - - -async def test_get_airlock_requests_by_user_and_workspace_with_awaiting_current_user_review_and_status_arguments_should_ignore_status(airlock_review_repo_mock): +async def test_get_airlock_requests_by_user_and_workspace_with_awaiting_current_user_review_and_status_arguments_should_ignore_status(airlock_request_repo_mock): workspace = sample_workspace() user = create_workspace_airlock_manager_user() - airlock_review_repo_mock.get_airlock_requests = MagicMock() + airlock_request_repo_mock.get_airlock_requests = MagicMock() - get_airlock_requests_by_user_and_workspace(user=user, workspace=workspace, airlock_request_repo=airlock_review_repo_mock, + get_airlock_requests_by_user_and_workspace(user=user, workspace=workspace, airlock_request_repo=airlock_request_repo_mock, status=AirlockRequestStatus.Approved, awaiting_current_user_review=True) - airlock_review_repo_mock.get_airlock_requests.assert_called_once_with(workspace_id=workspace.id, user_id=None, type=None, status=AirlockRequestStatus.InReview) + airlock_request_repo_mock.get_airlock_requests.assert_called_once_with(workspace_id=workspace.id, user_id=None, type=None, status=AirlockRequestStatus.InReview) -async def test_get_airlock_requests_by_user_and_workspace_with_awaiting_current_user_review_argument_by_non_airlock_manger_should_return_empty_list(airlock_review_repo_mock): +async def test_get_airlock_requests_by_user_and_workspace_with_awaiting_current_user_review_argument_by_non_airlock_manger_should_return_empty_list(airlock_request_repo_mock): user = create_test_user() - airlock_requests = get_airlock_requests_by_user_and_workspace(user=user, workspace=sample_workspace(), airlock_request_repo=airlock_review_repo_mock, awaiting_current_user_review=True) + airlock_requests = get_airlock_requests_by_user_and_workspace(user=user, workspace=sample_workspace(), airlock_request_repo=airlock_request_repo_mock, awaiting_current_user_review=True) assert airlock_requests == [] @pytest.mark.parametrize("role", get_required_roles(endpoint=create_airlock_review)) -async def test_get_airlock_requests_by_user_and_workspace_with_awaiting_current_user_review_argument_requires_same_roles_as_review_endpoint(role, airlock_review_repo_mock): - airlock_review_repo_mock.get_airlock_requests = MagicMock() +async def test_get_airlock_requests_by_user_and_workspace_with_awaiting_current_user_review_argument_requires_same_roles_as_review_endpoint(role, airlock_request_repo_mock): + airlock_request_repo_mock.get_airlock_requests = MagicMock() user = create_test_user() user.roles = [role] - get_airlock_requests_by_user_and_workspace(user=user, workspace=sample_workspace(), airlock_request_repo=airlock_review_repo_mock, awaiting_current_user_review=True) - airlock_review_repo_mock.get_airlock_requests.assert_called_once() + get_airlock_requests_by_user_and_workspace(user=user, workspace=sample_workspace(), airlock_request_repo=airlock_request_repo_mock, awaiting_current_user_review=True) + airlock_request_repo_mock.get_airlock_requests.assert_called_once() -@pytest.mark.parametrize("action, required_roles, airlock_review_repo_mock", [ - (AirlockActions.Review, get_required_roles(endpoint=create_airlock_review), airlock_review_repo_mock), - (AirlockActions.Cancel, get_required_roles(endpoint=create_cancel_request), airlock_review_repo_mock), - (AirlockActions.Submit, get_required_roles(endpoint=create_submit_request), airlock_review_repo_mock)]) -async def test_get_allowed_actions_requires_same_roles_as_endpoint(action, required_roles, airlock_review_repo_mock): - airlock_review_repo_mock.validate_status_update = MagicMock(return_value=True) +@pytest.mark.parametrize("action, required_roles, airlock_request_repo_mock", [ + (AirlockActions.Review, get_required_roles(endpoint=create_airlock_review), airlock_request_repo_mock), + (AirlockActions.Cancel, get_required_roles(endpoint=create_cancel_request), airlock_request_repo_mock), + (AirlockActions.Submit, get_required_roles(endpoint=create_submit_request), airlock_request_repo_mock)]) +async def test_get_allowed_actions_requires_same_roles_as_endpoint(action, required_roles, airlock_request_repo_mock): + airlock_request_repo_mock.validate_status_update = MagicMock(return_value=True) user = create_test_user() for role in required_roles: user.roles = [role] - allowed_actions = get_allowed_actions(request=sample_airlock_request(), user=user, airlock_request_repo=airlock_review_repo_mock) + allowed_actions = get_allowed_actions(request=sample_airlock_request(), user=user, airlock_request_repo=airlock_request_repo_mock) assert action in allowed_actions diff --git a/api_app/tests_ma/test_db/test_repositories/test_airlock_request_repository.py b/api_app/tests_ma/test_db/test_repositories/test_airlock_request_repository.py index 3da56102b..8c3fc7728 100644 --- a/api_app/tests_ma/test_db/test_repositories/test_airlock_request_repository.py +++ b/api_app/tests_ma/test_db/test_repositories/test_airlock_request_repository.py @@ -3,7 +3,6 @@ from mock import patch, MagicMock import pytest from tests_ma.test_api.conftest import create_test_user from models.schemas.airlock_request import AirlockRequestInCreate -from models.domain.airlock_resource import AirlockResourceType from models.domain.airlock_request import AirlockRequest, AirlockRequestStatus, AirlockRequestType from db.repositories.airlock_requests import AirlockRequestRepository @@ -63,12 +62,13 @@ def verify_dictionary_contains_all_enum_values(): def airlock_request_mock(status=AirlockRequestStatus.Draft): airlock_request = AirlockRequest( id=AIRLOCK_REQUEST_ID, - resourceType=AirlockResourceType.AirlockRequest, workspaceId=WORKSPACE_ID, requestType=AirlockRequestType.Import, files=[], businessJustification="some test reason", - status=status + status=status, + reviews=[] + ) return airlock_request @@ -106,29 +106,28 @@ def test_create_airlock_request_item_creates_an_airlock_request_with_the_right_v airlock_request_item_to_create = sample_airlock_request_input airlock_request = airlock_request_repo.create_airlock_request_item(airlock_request_item_to_create, WORKSPACE_ID) - assert airlock_request.resourceType == AirlockResourceType.AirlockRequest assert airlock_request.workspaceId == WORKSPACE_ID @pytest.mark.parametrize("current_status, new_status", get_allowed_status_changes()) -def test_update_airlock_request_status_with_allowed_new_status_should_update_request_status(airlock_request_repo, current_status, new_status, verify_dictionary_contains_all_enum_values): +def test_update_airlock_request_with_allowed_new_status_should_update_request_status(airlock_request_repo, current_status, new_status, verify_dictionary_contains_all_enum_values): user = create_test_user() mock_existing_request = airlock_request_mock(status=current_status) - airlock_request = airlock_request_repo.update_airlock_request_status(mock_existing_request, new_status, user) + airlock_request = airlock_request_repo.update_airlock_request(mock_existing_request, new_status, user) assert airlock_request.status == new_status @pytest.mark.parametrize("current_status, new_status", get_forbidden_status_changes()) -def test_update_airlock_request_status_with_forbidden_status_should_fail_on_validation(airlock_request_repo, current_status, new_status, verify_dictionary_contains_all_enum_values): +def test_update_airlock_request_with_forbidden_status_should_fail_on_validation(airlock_request_repo, current_status, new_status, verify_dictionary_contains_all_enum_values): user = create_test_user() mock_existing_request = airlock_request_mock(status=current_status) with pytest.raises(HTTPException): - airlock_request_repo.update_airlock_request_status(mock_existing_request, new_status, user) + airlock_request_repo.update_airlock_request(mock_existing_request, new_status, user) def test_get_airlock_requests_queries_db(airlock_request_repo): airlock_request_repo.container.query_items = MagicMock() - expected_query = airlock_request_repo.airlock_requests_query() + f' AND c.workspaceId = "{WORKSPACE_ID}"' + expected_query = airlock_request_repo.airlock_requests_query() + f' where c.workspaceId = "{WORKSPACE_ID}"' expected_parameters = [ {"name": "@user_id", "value": None}, {"name": "@status", "value": None}, diff --git a/api_app/tests_ma/test_db/test_repositories/test_airlock_review_repository.py b/api_app/tests_ma/test_db/test_repositories/test_airlock_review_repository.py deleted file mode 100644 index 5bf142124..000000000 --- a/api_app/tests_ma/test_db/test_repositories/test_airlock_review_repository.py +++ /dev/null @@ -1,31 +0,0 @@ -from mock import patch -import pytest -from db.repositories.airlock_reviews import AirlockReviewRepository -from models.domain.airlock_review import AirlockReviewDecision -from models.schemas.airlock_review import AirlockReviewInCreate -from models.domain.airlock_resource import AirlockResourceType - -WORKSPACE_ID = "abc000d3-82da-4bfc-b6e9-9a7853ef753e" -AIRLOCK_REQUEST_ID = "bbc8cae3-588b-4c7d-b27c-2a5feb7cc646" -AIRLOCK_REVIEW_ID = "76709e9f-5e86-4328-abb1-55d0a380f11e" - - -@pytest.fixture -def airlock_review_repo(): - with patch('azure.cosmos.CosmosClient') as cosmos_client_mock: - yield AirlockReviewRepository(cosmos_client_mock) - - -@pytest.fixture -def sample_airlock_review_input(): - return AirlockReviewInCreate(approval=True, decisionExplanation="some decision") - - -def test_create_airlock_review_item_with_the_right_values(sample_airlock_review_input, airlock_review_repo): - airlock__review = airlock_review_repo.create_airlock_review_item(sample_airlock_review_input, WORKSPACE_ID, AIRLOCK_REQUEST_ID) - - assert airlock__review.resourceType == AirlockResourceType.AirlockReview - assert airlock__review.workspaceId == WORKSPACE_ID - assert airlock__review.requestId == AIRLOCK_REQUEST_ID - assert airlock__review.reviewDecision == AirlockReviewDecision.Approved - assert airlock__review.decisionExplanation == "some decision" diff --git a/api_app/tests_ma/test_service_bus/test_airlock_request_status_update.py b/api_app/tests_ma/test_service_bus/test_airlock_request_status_update.py index d4b3084a8..d0c97e171 100644 --- a/api_app/tests_ma/test_service_bus/test_airlock_request_status_update.py +++ b/api_app/tests_ma/test_service_bus/test_airlock_request_status_update.py @@ -4,7 +4,6 @@ import pytest from mock import AsyncMock, patch from models.domain.airlock_request import AirlockRequest, AirlockRequestStatus, AirlockRequestType -from models.domain.airlock_resource import AirlockResourceType from models.domain.workspace import Workspace from service_bus.airlock_request_status_update import receive_step_result_message_and_update_status from db.errors import EntityDoesNotExist @@ -61,13 +60,12 @@ test_sb_step_result_message_with_invalid_status = { def sample_airlock_request(status=AirlockRequestStatus.Submitted): airlock_request = AirlockRequest( id=AIRLOCK_REQUEST_ID, - resourceType=AirlockResourceType.AirlockRequest, workspaceId=WORKSPACE_ID, requestType=AirlockRequestType.Import, files=[], businessJustification="some test reason", status=status, - errorMessage=None + reviews=[] ) return airlock_request @@ -96,12 +94,12 @@ async def test_receiving_good_message(_, app, sb_client, logging_mock, workspace eg_client().send = AsyncMock() expected_airlock_request = sample_airlock_request() airlock_request_repo().get_airlock_request_by_id.return_value = expected_airlock_request - airlock_request_repo().update_airlock_request_status.return_value = sample_airlock_request(status=AirlockRequestStatus.InReview) + airlock_request_repo().update_airlock_request.return_value = sample_airlock_request(status=AirlockRequestStatus.InReview) workspace_repo().get_workspace_by_id.return_value = sample_workspace() await receive_step_result_message_and_update_status(app) airlock_request_repo().get_airlock_request_by_id.assert_called_once_with(test_sb_step_result_message["data"]["request_id"]) - airlock_request_repo().update_airlock_request_status.assert_called_once_with(expected_airlock_request, test_sb_step_result_message["data"]["new_status"], expected_airlock_request.user, None) + airlock_request_repo().update_airlock_request.assert_called_once_with(airlock_request=expected_airlock_request, new_status=test_sb_step_result_message["data"]["new_status"], user=expected_airlock_request.user, error_message=None, airlock_review=None) assert eg_client().send.call_count == 2 logging_mock.assert_not_called() sb_client().get_queue_receiver().complete_message.assert_called_once_with(service_bus_received_message_mock) diff --git a/api_app/tests_ma/test_services/test_airlock.py b/api_app/tests_ma/test_services/test_airlock.py index 0b0ce13cb..d92edbe86 100644 --- a/api_app/tests_ma/test_services/test_airlock.py +++ b/api_app/tests_ma/test_services/test_airlock.py @@ -4,7 +4,6 @@ import pytest from resources import strings from services.airlock import validate_user_allowed_to_access_storage_account, get_required_permission, \ validate_request_status -from models.domain.airlock_resource import AirlockResourceType from models.domain.airlock_request import AirlockRequest, AirlockRequestStatus, AirlockRequestType from tests_ma.test_api.conftest import create_workspace_owner_user, create_workspace_researcher_user @@ -12,12 +11,12 @@ from tests_ma.test_api.conftest import create_workspace_owner_user, create_works def sample_airlock_request(status=AirlockRequestStatus.Draft): airlock_request = AirlockRequest( id="AIRLOCK_REQUEST_ID", - resourceType=AirlockResourceType.AirlockRequest, workspaceId="WORKSPACE_ID", requestType=AirlockRequestType.Import, files=[], businessJustification="some test reason", - status=status + status=status, + reviews=[] ) return airlock_request diff --git a/docs/azure-tre-overview/airlock.md b/docs/azure-tre-overview/airlock.md index 9a455d10d..7d6683405 100644 --- a/docs/azure-tre-overview/airlock.md +++ b/docs/azure-tre-overview/airlock.md @@ -170,7 +170,7 @@ In the TRE Core, the TRE API will provide the airlock API endpoints allowing to |---|---|---| | `POST` | `/api/workspaces/{workspace_id}/requests` | Create an Airlock request (in **Draft**) | | `POST` | `/api/workspaces/{workspace_id}/requests/{airlock_request_id}/link` | Get the url and token to acccess Airlock Request | `POST` | `/api/workspaces/{workspace_id}/requests/{airlock_request_id}/submit` | Submits an Airlock request | -| `POST` | `/api/workspaces/{workspace_id}/requests/{airlock_request_id}/reviews` | Reviews an Airlock request | +| `POST` | `/api/workspaces/{workspace_id}/requests/{airlock_request_id}/review` | Reviews an Airlock request | | `POST` | `/api/workspaces/{workspace_id}/requests/{airlock_request_id}/cancel` | Cancels an Airlock request | container | @@ -180,4 +180,4 @@ Also in the airlock feature we have the **Airlock Processor** which will handle The following sequence diagram detailing the Airlock feature and its event driven behaviour: -[![Airlock flow](../assets/airlock-swimlanes.png)](../assets/airlock-swimlanes.png) \ No newline at end of file +[![Airlock flow](../assets/airlock-swimlanes.png)](../assets/airlock-swimlanes.png) diff --git a/e2e_tests/config.py b/e2e_tests/config.py index 2106f4bbe..740986879 100644 --- a/e2e_tests/config.py +++ b/e2e_tests/config.py @@ -22,3 +22,6 @@ TEST_WORKSPACE_APP_PLAN: str = config("APP_SERVICE_PLAN_SKU", default="") # workspace + workspace service for quicker execution. If they're blank the perf test will create + delete them. PERF_TEST_WORKSPACE_ID: str = config("PERF_TEST_WORKSPACE_ID", default="") PERF_TEST_WORKSPACE_SERVICE_ID: str = config("PERF_TEST_WORKSPACE_SERVICE_ID", default="") + +# Set workspace id of an existing workspace to skip creation of a workspace during E2E tests +TEST_AIRLOCK_WORKSPACE_ID: str = config("TEST_AIRLOCK_WORKSPACE_ID", default="") diff --git a/e2e_tests/test_airlock.py b/e2e_tests/test_airlock.py index 60fe69dd6..4574cc8fd 100644 --- a/e2e_tests/test_airlock.py +++ b/e2e_tests/test_airlock.py @@ -15,26 +15,30 @@ LOGGER = logging.getLogger(__name__) @pytest.mark.airlock @pytest.mark.extended -@pytest.mark.timeout(1600) +@pytest.mark.timeout(2000) async def test_airlock_import_flow(admin_token, verify) -> None: - # 1. create workspace - LOGGER.info("Creating workspace") - payload = { - "templateName": resource_strings.BASE_WORKSPACE, - "properties": { - "display_name": "E2E test airlock flow", - "description": "workspace for E2E airlock flow", - "address_space_size": "small", - "client_id": f"{config.TEST_WORKSPACE_APP_ID}", - "client_secret": f"{config.TEST_WORKSPACE_APP_SECRET}", + if config.TEST_AIRLOCK_WORKSPACE_ID != "": + workspace_id = config.TEST_AIRLOCK_WORKSPACE_ID + workspace_path = f"/workspaces/{workspace_id}" + else: + # 1. create workspace + LOGGER.info("Creating workspace") + payload = { + "templateName": resource_strings.BASE_WORKSPACE, + "properties": { + "display_name": "E2E test airlock flow", + "description": "workspace for E2E airlock flow", + "address_space_size": "small", + "client_id": f"{config.TEST_WORKSPACE_APP_ID}", + "client_secret": f"{config.TEST_WORKSPACE_APP_SECRET}", + } } - } - if config.TEST_WORKSPACE_APP_PLAN != "": - payload["properties"]["app_service_plan_sku"] = config.TEST_WORKSPACE_APP_PLAN + if config.TEST_WORKSPACE_APP_PLAN != "": + payload["properties"]["app_service_plan_sku"] = config.TEST_WORKSPACE_APP_PLAN - workspace_path, workspace_id = await post_resource(payload, resource_strings.API_WORKSPACES, access_token=admin_token, verify=verify) + workspace_path, workspace_id = await post_resource(payload, resource_strings.API_WORKSPACES, access_token=admin_token, verify=verify) workspace_owner_token, scope_uri = await get_workspace_auth_details(admin_token=admin_token, workspace_id=workspace_id, verify=verify) # 2. create airlock request @@ -59,7 +63,7 @@ async def test_airlock_import_flow(admin_token, verify) -> None: # 4. upload blob - # currenly there's no elagant way to check if the container was created yet becasue its an asyc process + # currenly there's no elegant way to check if the container was created yet becasue its an asyc process # it would be better to create another draft_improgress step and wait for the request to change to draft state before # uploading the blob @@ -92,11 +96,12 @@ async def test_airlock_import_flow(admin_token, verify) -> None: "approval": "True", "decisionExplanation": "the reason why this request was approved/rejected" } - request_result = await post_request(payload, f'/api{workspace_path}/requests/{request_id}/reviews', workspace_owner_token, verify, 200) - assert request_result["airlock_review"]["decisionExplanation"] == "the reason why this request was approved/rejected" + request_result = await post_request(payload, f'/api{workspace_path}/requests/{request_id}/review', workspace_owner_token, verify, 200) + assert request_result["airlockRequest"]["reviews"][0]["decisionExplanation"] == "the reason why this request was approved/rejected" await wait_for_status(airlock_strings.APPROVED_STATUS, workspace_owner_token, workspace_path, request_id, verify) - # 7. delete workspace - LOGGER.info("Deleting workspace") - await disable_and_delete_resource(f'/api{workspace_path}', admin_token, verify) + if config.TEST_AIRLOCK_WORKSPACE_ID == "": + # 7. delete workspace + LOGGER.info("Deleting workspace") + await disable_and_delete_resource(f'/api{workspace_path}', admin_token, verify)