Handle request abspath in the api layer.

A path locating a file in a request has the form `group/path/file`, and
the file on disk is just `root/path/file`. Previously, this discrepency
was handled in file_browser_api layer, at tree construction time.

Building on the previous commit, we move this to be a responsibility of
the ReleaseRequest container. Its abspath() now expected the full path
including group, validates the group part, and returns the abspath on
disk.

Also, in the copy logic, we no longer use abspath to get the destination
path, instead using `root / path`. This means abspath can now verify the
path exists, like workspaces abspath does.

This is much cleaner, and removes some logic and attributes from
PathItem.

airlock/api.py

@@ -92,7 +92,7 @@ def abspath(self, relpath):
         # protect against traversal
         path.resolve().relative_to(root)
 
-        # validate path exists for *workspace* files
+        # validate path exists
         if not path.exists():
             raise ProviderAPI.FileNotFound(path)
 
@@ -160,14 +160,28 @@ def get_url(self, relpath=ROOT_PATH):
         )
 
     def abspath(self, relpath):
+        """Returns abspath to the file on disk.
+
+        The first part of the relpath is the group, so we parse and validate that first.
+        """
+        relpath = UrlPath(relpath)
         root = self.root()
-        path = root / relpath
+
+        group = relpath.parts[0]
+
+        if group not in self.filegroups:
+            raise ProviderAPI.FileNotFound(f"bad group {group} in url {relpath}")
+
+        filepath = relpath.relative_to(group)
+        path = root / filepath
 
         # protect against traversal
         path.resolve().relative_to(root)
 
-        # note: we do not validate path exists for *request* file, as it might
-        # be a destination to copy to.
+        # validate path exists
+        if not path.exists():
+            raise ProviderAPI.FileNotFound(path)
+
         return path
 
     def file_set(self):
@@ -359,7 +373,8 @@ def add_file_to_request(
 
         workspace = self.get_workspace(release_request.workspace)
         src = workspace.abspath(relpath)
-        dst = release_request.abspath(relpath)
+        # manually contruct target path from root
+        dst = release_request.root() / relpath
         dst.parent.mkdir(exist_ok=True, parents=True)
         shutil.copy(src, dst)
 

airlock/file_browser_api.py

@@ -43,20 +43,16 @@ class PathNotFound(Exception):
     # should this node be expanded in the tree?
     expanded: bool = False
 
-    # relative filepath on disk. defaults to path, but allows us to customise it.
-    filepath: UrlPath = None
     # what to display for this node when rendering the tree. Defaults to name,
     # but this allow it to be overridden.
     display_text: str = None
 
     def __post_init__(self):
         # ensure is UrlPath
         self.relpath = UrlPath(self.relpath)
-        if self.filepath is None:
-            self.filepath = self.relpath
 
     def _absolute_path(self):
-        return self.container.abspath(self.filepath)
+        return self.container.abspath(self.relpath)
 
     def is_directory(self):
         """Does this contain other things?"""
@@ -298,8 +294,6 @@ def build_filegroup_tree(file_parts, path, parent):
                 container=container,
                 relpath=child_path,
                 parent=parent,
-                # actual path on disk, striping the group part
-                filepath=child_path.relative_to(child_path.parts[0]),
                 selected=selected,
             )
 

tests/integration/views/test_workspace.py

@@ -159,7 +159,7 @@ def test_workspace_request_file_creates(client_with_user, api):
     filegroup = release_request.filegroups["default"]
     assert filegroup.name == "default"
     assert str(filegroup.files[0].relpath) == "test/path.txt"
-    assert release_request.abspath("test/path.txt").exists()
+    assert release_request.abspath("default/test/path.txt").exists()
 
 
 def test_workspace_request_file_request_already_exists(client_with_user, api):
@@ -178,7 +178,7 @@ def test_workspace_request_file_request_already_exists(client_with_user, api):
     assert response.status_code == 302
     current_release_request = api.get_current_request(workspace.name, user)
     assert current_release_request.id == release_request.id
-    assert release_request.abspath("test/path.txt").exists()
+    assert current_release_request.abspath("default/test/path.txt").exists()
     filegroup = current_release_request.filegroups["default"]
     assert filegroup.name == "default"
     assert str(filegroup.files[0].relpath) == "test/path.txt"

tests/unit/test_api.py

@@ -5,7 +5,7 @@
 import pytest
 
 import old_api
-from airlock.api import ProviderAPI, Status, Workspace
+from airlock.api import ProviderAPI, Status, UrlPath, Workspace
 from airlock.users import User
 from tests import factories
 
@@ -69,7 +69,7 @@ def test_provider_request_release_files(mock_old_api):
     factories.write_request_file(release_request, "group", relpath, "test")
     factories.api.set_status(release_request, Status.APPROVED, checker)
 
-    abspath = release_request.abspath(relpath)
+    abspath = release_request.abspath("group" / relpath)
 
     api = ProviderAPI()
     api.release_files(release_request, checker)
@@ -310,11 +310,10 @@ def test_add_file_to_request_states(status, success, api):
 
     if success:
         api.add_file_to_request(release_request, path, author)
-        assert release_request.abspath(path).exists()
+        assert release_request.abspath("default" / path).exists()
     else:
         with pytest.raises(api.RequestPermissionDenied):
             api.add_file_to_request(release_request, path, author)
-        assert not release_request.abspath(path).exists()
 
 
 def test_request_release_invalid_state():
@@ -326,6 +325,17 @@ def test_request_release_invalid_state():
         )
 
 
+def test_request_release_abspath(api):
+    path = UrlPath("foo/bar.txt")
+    release_request = factories.create_release_request("id")
+    factories.write_request_file(release_request, "default", path)
+
+    with pytest.raises(api.FileNotFound):
+        release_request.abspath("badgroup" / path)
+
+    assert release_request.abspath("default" / path).exists()
+
+
 def setup_empty_release_request():
     author = User(1, "author", ["workspace"], False)
     path = Path("path/file.txt")