Small add file permission refactor (#606)

* the UI would not present the option to add a file in the state `UNDER_REVIEW`.
  Although the BLL function would accept it, I think the DAL would correctly reject it
* I believe the function in policies was derived from the BLL function
* Remove unnecessary assert
* Create specific DAL test for re-adding a file to a request
  * this was previously covered implicitly by BLL tests
* Amend policy for adding a file to a request & reuse
* The detail of the check should be in the workspace class (as per file_can_be_updated)

airlock/business_logic.py

@@ -467,6 +467,13 @@ def file_can_be_updated(self, relpath: UrlPath) -> bool:
             WorkspaceFileStatus.WITHDRAWN,
         ]
 
+    def file_can_be_added(self, relpath: UrlPath) -> bool:
+        return self.get_workspace_file_status(relpath) in [
+            WorkspaceFileStatus.UNRELEASED,
+            WorkspaceFileStatus.CONTENT_UPDATED,
+            WorkspaceFileStatus.WITHDRAWN,
+        ]
+
 
 @dataclass(frozen=True)
 class CodeRepo:

airlock/policies.py

@@ -39,6 +39,14 @@ def check_can_edit_request(request: "ReleaseRequest"):
         )
 
 
+def can_add_file_to_request(workspace: "Workspace", relpath: UrlPath):
+    try:
+        check_can_add_file_to_request(workspace, relpath)
+    except exceptions.RequestPermissionDenied:
+        return False
+    return True
+
+
 def check_can_add_file_to_request(workspace: "Workspace", relpath: UrlPath):
     """
     This file can be added to the request.
@@ -53,6 +61,12 @@ def check_can_add_file_to_request(workspace: "Workspace", relpath: UrlPath):
     if workspace.file_has_been_released(relpath):
         raise exceptions.RequestPermissionDenied("Cannot add released file to request")
 
+    if not workspace.file_can_be_added(relpath):
+        status = workspace.get_workspace_file_status(relpath)
+        raise exceptions.RequestPermissionDenied(
+            f"Cannot add file to request if it is in status {status}"
+        )
+
 
 def check_can_update_file_on_request(workspace: "Workspace", relpath: UrlPath):
     """

airlock/views/workspace.py

@@ -9,7 +9,7 @@
 from django.views.decorators.vary import vary_on_headers
 from opentelemetry import trace
 
-from airlock import exceptions, permissions
+from airlock import exceptions, permissions, policies
 from airlock.business_logic import bll
 from airlock.enums import RequestFileType, WorkspaceFileStatus
 from airlock.file_browser_api import get_workspace_tree
@@ -31,12 +31,6 @@
 
 tracer = trace.get_tracer_provider().get_tracer("airlock")
 
-VALID_WORKSPACEFILE_STATES_TO_ADD = [
-    WorkspaceFileStatus.UNRELEASED,
-    WorkspaceFileStatus.CONTENT_UPDATED,
-    WorkspaceFileStatus.WITHDRAWN,
-]
-
 
 def grouped_workspaces(workspaces):
     workspaces_by_project = defaultdict(list)
@@ -95,12 +89,9 @@ def workspace_view(request, workspace_name: str, path: str = ""):
 
     # Only show the add file form button if the multiselect_add condition is true,
     # and also this pathitem is a file that can be added to a request - i.e. it is a
-    # file and it's not already on the current request for the user
-    add_file = (
-        multiselect_add
-        and path_item.is_valid()
-        and workspace.get_workspace_file_status(path_item.relpath)
-        in VALID_WORKSPACEFILE_STATES_TO_ADD
+    # valid file and it's not already on the current request for the user
+    add_file = multiselect_add and policies.can_add_file_to_request(
+        workspace, path_item.relpath
     )
 
     project = workspace.project()
@@ -218,8 +209,9 @@ def multiselect_add_files(request, multiform, workspace):
     for f in multiform.cleaned_data["selected"]:
         workspace.abspath(f)  # validate path
 
-        state = workspace.get_workspace_file_status(UrlPath(f))
-        if state in VALID_WORKSPACEFILE_STATES_TO_ADD:
+        relpath = UrlPath(f)
+        state = workspace.get_workspace_file_status(relpath)
+        if policies.can_add_file_to_request(workspace, relpath):
             files_to_add.append(f)
         elif state == WorkspaceFileStatus.RELEASED:
             files_ignored[f] = "already released"

local_db/data_access.py

@@ -165,9 +165,6 @@ def add_file_to_request(
                 existing_file = RequestFileMetadata.objects.get(
                     request_id=request_id, relpath=relpath
                 )
-                # We should never be able to attempt to add a file to a request
-                # with a different filetype
-                assert existing_file.filetype == filetype
             except RequestFileMetadata.DoesNotExist:
                 # create the RequestFile
                 RequestFileMetadata.objects.create(

tests/local_db/test_data_access.py

@@ -1,9 +1,10 @@
 import pytest
 
 from airlock import exceptions
-from airlock.business_logic import AuditEvent
+from airlock.business_logic import AuditEvent, store_file
 from airlock.enums import (
     AuditEventType,
+    RequestFileType,
     RequestStatus,
     Visibility,
 )
@@ -126,6 +127,43 @@ def test_withdraw_file_from_request_bad_state(status):
         )
 
 
+def test_add_file_to_request_bad_state():
+    workspace = factories.create_workspace("workspace")
+    author = factories.create_user(username="author", workspaces=["workspace"])
+    request_file = factories.request_file()
+    relpath = request_file.path
+    factories.write_workspace_file(workspace, relpath, contents="1234")
+    release_request = factories.create_request_at_status(
+        "workspace",
+        author=author,
+        status=RequestStatus.SUBMITTED,
+        files=[request_file],
+    )
+
+    src = workspace.abspath(relpath)
+    file_id = store_file(release_request, src)
+    manifest = workspace.get_manifest_for_file(relpath)
+
+    with pytest.raises(exceptions.APIException):
+        dal.add_file_to_request(
+            request_id=release_request.id,
+            group_name="group",
+            relpath=UrlPath(relpath),
+            file_id=file_id,
+            filetype=RequestFileType.OUTPUT,
+            timestamp=manifest["timestamp"],
+            commit=manifest["commit"],
+            repo=manifest["repo"],
+            size=manifest["size"],
+            job_id=manifest["job_id"],
+            row_count=manifest["row_count"],
+            col_count=manifest["col_count"],
+            audit=AuditEvent.from_request(
+                release_request, AuditEventType.REQUEST_FILE_ADD, user=author
+            ),
+        )
+
+
 def test_delete_file_from_request_bad_path():
     author = factories.create_user()
     release_request = factories.create_release_request(