Client layer: rebase advertised links, refuse redirects, add calls the filesystems need

CHANGELOG.md

@@ -6,6 +6,17 @@ All notable changes to this project will be documented in this file
  - replace naming of classes and methods using 'Workbench' to 'Bench'
  - replace create_sample, create_container long argument lists with new XXXPost objects
 
+## 2.8.0 (unreleased)
+
+- Security: the client never follows an HTTP redirect, since `requests` would carry the
+  `apiKey` header to the new host.
+- Client-level changes: `_links` URLs are rebased onto the configured base URL, path prefix
+  included, so paging and download links work behind a proxy or port-forward; `doDelete` tolerates a leading slash (fixes
+  `delete_document`); `upload_file` and `upload_attachment_by_global_id` take an optional
+  `filename=`; `list_folder_tree` takes `page_size=`; new `get_workbench_by_id`; Inventory
+  CSV import uses the shared session.
+- Removed a stray debug print from `InventoryClient.upload_attachment_by_global_id`.
+
 ## Unreleased
 
 - Gallery upload routing and section-mismatch handling (PR #56): clearer

rspace_client/client_base.py

@@ -1,6 +1,7 @@
 import logging
 import re
 import requests
+from urllib.parse import urlparse, urlunparse
 
 from typing import Optional
 
@@ -21,6 +22,7 @@
 DEFAULT_MAX_RETRIES = 3
 DEFAULT_BACKOFF_FACTOR = 0.5
 RETRY_STATUSES = (429, 500, 502, 503, 504)
+REDIRECT_STATUSES = (301, 302, 303, 307, 308)
 
 
 class _RSpaceRetry(Retry):
@@ -119,6 +121,26 @@ def __exit__(self, exc_type, exc_value, traceback):
     def _is_absolute_url(url):
         return url.startswith("http://") or url.startswith("https://")
 
+    def _rebase_url(self, url):
+        """
+        Point an absolute URL at the server this client was configured with.
+
+        RSpace builds the ``_links`` in its responses from its own configured base URL, which
+        is not necessarily the address the client used: behind a reverse proxy, in a container,
+        or through a port-forward the advertised host is unreachable. Following such a link
+        verbatim fails with a connection error, so keep the path and query and substitute our
+        own scheme and host.
+
+        Either side may also sit under a path prefix (a proxy serving RSpace at ``/rspace``), so
+        the part of the path from ``/api/`` on is re-anchored under our own base path, the same
+        base every relative request is built from. A link without ``/api/`` keeps its path.
+        """
+        link = urlparse(url)
+        ours = urlparse(self.rspace_url)
+        api_at = link.path.find("/api/")
+        path = link.path if api_at < 0 else ours.path.rstrip("/") + link.path[api_at:]
+        return urlunparse((ours.scheme, ours.netloc, path, link.params, link.query, link.fragment))
+
     def _get_headers(self, content_type="application/json"):
         headers = {"apiKey": self.api_key}
         if content_type is not None:
@@ -176,6 +198,17 @@ def _check_status(response):
             raise AuthenticationError(
                 "Error code: 401, {}".format(ClientBase._get_error_detail(response))
             )
+        if response.status_code in REDIRECT_STATUSES:
+            # Redirects are never followed: requests keeps custom headers such as apiKey
+            # across a redirect to another host, which would hand the key to that host.
+            raise ApiError(
+                "Error code: {}, the server redirected to {!r}; rspace-client does not follow "
+                "redirects, so point the client at the final RSpace URL".format(
+                    response.status_code, response.headers.get("Location")
+                ),
+                response_status_code=response.status_code,
+                response=response,
+            )
 
         try:
             response.raise_for_status()
@@ -205,7 +238,7 @@ def doDelete(self, path, resource_id):
         """
         numeric_id = self._get_numeric_record_id(resource_id)
         return self.retrieve_api_results(
-            "/{}/{}".format(path, numeric_id),
+            "/{}/{}".format(path.strip("/"), numeric_id),  # tolerate callers passing "/documents"
             content_type=None,
             request_type="DELETE",
         )
@@ -222,23 +255,22 @@ def retrieve_api_results(
         :param content_type: content type
         :return: parsed JSON response as a dictionary
         """
-        url = endpoint
-        if not self._is_absolute_url(endpoint):
-            url = self._get_api_url() + endpoint
+        url = self._rebase_url(endpoint) if self._is_absolute_url(endpoint) else self._get_api_url() + endpoint
 
         headers = self._get_headers(content_type)
         try:
             if request_type == "GET":
                 response = self._session.get(
-                    url, params=params, headers=headers, timeout=self.timeout
+                    url, params=params, headers=headers, timeout=self.timeout, allow_redirects=False
                 )
             elif (
                 request_type == "PUT"
                 or request_type == "POST"
                 or request_type == "DELETE"
             ):
                 response = self._session.request(
-                    request_type, url, json=params, headers=headers, timeout=self.timeout
+                    request_type, url, json=params, headers=headers, timeout=self.timeout,
+                    allow_redirects=False
                 )
             else:
                 raise ValueError(
@@ -271,6 +303,7 @@ def _post_multipart(self, endpoint, files=None, data=None):
                 data=data,
                 headers=self._get_headers(),
                 timeout=self.timeout,
+                allow_redirects=False,
             )
             return self._handle_response(response)
         except (requests.exceptions.ConnectionError, requests.exceptions.Timeout) as e:
@@ -288,6 +321,7 @@ def _get_raw_response(self, url, params=None, accept="application/octet-stream")
                 params=params,
                 headers={"apiKey": self.api_key, "Accept": accept},
                 timeout=self.timeout,
+                allow_redirects=False,
             )
         except (requests.exceptions.ConnectionError, requests.exceptions.Timeout) as e:
             raise RSpaceConnectionError(e)
@@ -340,10 +374,11 @@ def download_link_to_file(self, url, filename, chunk_size=8192):
         :param filename: file path to save the file to or an already opened file object
         :param chunk_size: size of the chunks to download at a time, default is 8192
         """
+        url = self._rebase_url(url) if self._is_absolute_url(url) else url
         headers = {"apiKey": self.api_key, "Accept": "application/octet-stream"}
         try:
             with self._session.get(
-                url, headers=headers, stream=True, timeout=self.timeout
+                url, headers=headers, stream=True, timeout=self.timeout, allow_redirects=False
             ) as response:
                 self._check_status(response)
                 if isinstance(filename, str):

rspace_client/eln/eln.py

@@ -347,13 +347,14 @@ def download_file(self, file_id, filename, chunk_size=128):
             f"{url_base}/files/{numeric_file_id}/file", filename, chunk_size
         )
 
-    def upload_file(self, file, folder_id=None, caption=None):
+    def upload_file(self, file, folder_id=None, caption=None, filename=None):
         """
         Upload a file to the gallery. More information on
         https://community.researchspace.com/public/apiDocs (or your own instance's /public/apiDocs).
         :param file: open file object
         :param folder_id: folder id of the destination folder
         :param caption: optional caption
+        :param filename: name to store the file under; defaults to the file object's own name
         :return: parsed response as a dictionary
         """
         data = {}
@@ -365,7 +366,8 @@ def upload_file(self, file, folder_id=None, caption=None):
         if caption is not None:
             data["caption"] = caption
 
-        return self._post_multipart("/files", files={"file": file}, data=data)
+        part = file if filename is None else (filename, file)
+        return self._post_multipart("/files", files={"file": part}, data=data)
 
     def update_file(self, file, fileId):
         """
@@ -793,12 +795,14 @@ def get_folder(self, folder_id):
         numeric_folder_id = self._get_numeric_record_id(folder_id)
         return self.retrieve_api_results("/folders/{}".format(numeric_folder_id))
 
-    def list_folder_tree(self, folder_id=None, typesToInclude=[]):
+    def list_folder_tree(self, folder_id=None, typesToInclude=[], page_size=None):
         """
         Lists contents of a folder by its ID.
         :param folder_id. Optional folderId. If none, will return listing of Home Folder
         :param typesToInclude: An optional list of any of 'folder', 'notebook' or 'document'. Results
          will be restricted to these types
+        :param page_size: optional number of records per page (the server default applies otherwise);
+         follow the response's 'next' link for further pages
         :return a paginated folder listing
         """
         url = ""
@@ -817,6 +821,8 @@ def list_folder_tree(self, folder_id=None, typesToInclude=[]):
                     'typesToInclude must be contain "document", "notebook" and/or "folder"'
                 )
             params["typesToInclude"] = ",".join(typesToInclude)
+        if page_size is not None:
+            params["pageSize"] = int(page_size)
         return self.retrieve_api_results(url, params)
 
     # Groups methods

rspace_client/inv/inv.py

@@ -1817,13 +1817,44 @@ def download_attachment_by_id(self, attachment_id: Union[str, int], file_path: U
             f"{url_base}/files/{attachment_id}/file", file_path, chunk_size
         )
 
-    def upload_attachment_by_global_id(self, record_global_id: str, file: BinaryIO) -> dict:
+    def upload_attachment_by_global_id(self, record_global_id: str, file: BinaryIO,
+                                       filename: Optional[str] = None) -> dict:
+        """Attach a file to a record or an attachment field. ``filename`` overrides the name
+        taken from the file object."""
         return self._post_multipart(
             "/files",
-            files={"file": file},
+            files={"file": file if filename is None else (filename, file)},
             data={"fileSettings": json.dumps({"parentGlobalId": record_global_id})},
         )
 
+    def attach_gallery_file_by_global_id(self, parent_global_id: str, media_file_global_id: str) -> dict:
+        """
+        Attach a file that is already in the Gallery to an Inventory record or attachment field.
+
+        This is the "link from Gallery" the web interface offers alongside "upload new". No
+        bytes move: the resulting Inventory file references the Gallery file, and its
+        ``mediaFileGlobalId`` names it. Deleting the attachment leaves the Gallery file alone.
+
+        Parameters
+        ----------
+        parent_global_id : str
+            A sample (SA...), subsample (SS...), container (IC...), instrument (IN...) or an
+            attachment-type field (SF...). Benches and sample templates are refused by the
+            server.
+        media_file_global_id : str
+            The Gallery file to link (GL...).
+
+        Returns
+        -------
+        Dict of the created InventoryFile, whose ``mediaFileGlobalId`` is the linked file.
+        """
+        return self.retrieve_api_results(
+            "/attachments",
+            request_type="POST",
+            params={"parentGlobalId": Id(parent_global_id).as_global_id(),
+                    "mediaFileGlobalId": Id(media_file_global_id).as_global_id()},
+        )
+
     def split_subsample(
         self,
         subsample: Union[int, str, dict],
@@ -1981,6 +2012,22 @@ def get_workbenches(self) -> Sequence[dict]:
         result = self.retrieve_api_results("/workbenches")
         return [wb for wb in result["containers"]]
 
+    def get_workbench_by_id(self, workbench_id: Union[str, int], include_content: bool = False) -> dict:
+        """
+        Gets a workbench (bench) by id. Benches are containers but must be read through
+        /workbenches: /containers/{id} answers 422 for a bench.
+
+        Parameters
+        ----------
+        workbench_id : Union[str, int]
+            Numeric id or global id (BE...) of the bench.
+        include_content : bool
+            Include the bench's locations and their content (subsamples, containers).
+        """
+        numeric_id = self._get_numeric_record_id(workbench_id)
+        return self.retrieve_api_results(
+            f"/workbenches/{numeric_id}?includeContent={'true' if include_content else 'false'}")
+
     ## ---------------------------------------------------------------------
     ## CSV import
     ## ---------------------------------------------------------------------
@@ -1994,13 +2041,9 @@ def _multipart_post(self, endpoint: str, files: dict, data: dict) -> dict:
         multipart Content-Type (with boundary) when ``files`` is supplied, and
         adds each entry of ``data`` as an additional form field.
         """
-        response = requests.post(
-            self._get_api_url() + endpoint,
-            files=files,
-            data=data,
-            headers=self._get_headers(),
-        )
-        return self._handle_response(response)
+        # through the shared session, so timeout, retries, redirect refusal and error
+        # handling are the same as for every other request
+        return self._post_multipart(endpoint, files=files, data=data)
 
     def parse_csv_import_file(
         self, file: BinaryIO, record_type: Union[str, ImportRecordType]