fix: make errors from openbis more descriptive (#1143)

Author: olevski

components/renku_data_services/errors/errors.py

@@ -186,6 +186,14 @@ class RequestCancelledError(ProgrammingError):
     )
 
 
+@dataclass
+class ThirdPartyAPIError(ProgrammingError):
+    """Raised when a response from a third party API is not what we expected."""
+
+    code: int = 1514
+    message: str = "An external API or service that Renku depends on is not available or is not behaving as expected."
+
+
 def missing_or_unauthorized(resource_type: str | StrEnum, id: str | int | ULID) -> MissingResourceError:
     """Generate a missing resource error with an ambiguous message."""
     return MissingResourceError(

components/renku_data_services/users/core.py

@@ -2,29 +2,31 @@
 
 from datetime import datetime
 
+from renku_data_services.errors import errors
 from renku_data_services.secrets.models import SecretKind, SecretPatch, UnsavedSecret
 from renku_data_services.users import apispec
 
 
-def _validate_expiration_timestamp(expiration_timestamp: datetime | None) -> bool:
-    return expiration_timestamp is None or expiration_timestamp.tzinfo is not None
+def _validate_expiration_timestamp(expiration_timestamp: datetime | None) -> None:
+    if expiration_timestamp is not None and expiration_timestamp.tzinfo is None:
+        raise errors.ValidationError(message="The expiration_timestamp has to contain timezone information.")
 
 
 def validate_unsaved_secret(body: apispec.SecretPost) -> UnsavedSecret:
     """Validate a new secret to be created."""
-    if _validate_expiration_timestamp(body.expiration_timestamp):
-        return UnsavedSecret(
-            name=body.name,
-            secret_value=body.value,
-            kind=SecretKind(body.kind.value),
-            expiration_timestamp=body.expiration_timestamp,
-            default_filename=body.default_filename,
-        )
-    raise ValueError("Expiration timestamp must be specified with a time zone")
+    _validate_expiration_timestamp(body.expiration_timestamp)
+    return UnsavedSecret(
+        name=body.name,
+        secret_value=body.value,
+        kind=SecretKind(body.kind.value),
+        expiration_timestamp=body.expiration_timestamp,
+        default_filename=body.default_filename,
+    )
 
 
 def validate_secret_patch(patch: apispec.SecretPatch) -> SecretPatch:
     """Validate the update to a secret."""
+    _validate_expiration_timestamp(patch.expiration_timestamp)
     return SecretPatch(
         name=patch.name,
         secret_value=patch.value,

components/renku_data_services/utils/core.py

@@ -5,6 +5,7 @@
 import ssl
 from collections.abc import Awaitable, Callable
 from datetime import datetime, timedelta
+from json import JSONDecodeError
 from typing import Any, Concatenate, ParamSpec, Protocol, TypeVar, cast
 from zoneinfo import ZoneInfo
 
@@ -13,6 +14,9 @@
 from sqlalchemy.ext.asyncio import AsyncSession
 
 from renku_data_services import errors
+from renku_data_services.app_config import logging
+
+logger = logging.getLogger(__name__)
 
 
 @functools.lru_cache(1)
@@ -83,91 +87,152 @@ async def transaction_wrapper(self: _WithSessionMaker, *args: _P.args, **kwargs:
     return transaction_wrapper
 
 
-def _get_url(host: str) -> str:
-    return f"https://{host}/openbis/openbis/rmi-application-server-v3.json"
+def _get_openbis_url(openbis_host: str) -> str:
+    return f"https://{openbis_host}/openbis/openbis/rmi-application-server-v3.json"
 
 
-async def _get_openbis_session_token(host: str, login: dict[str, Any], timeout: int) -> str:
+async def _get_openbis_session_token(openbis_host: str, login: dict[str, Any], timeout: int) -> str:
     async with httpx.AsyncClient(verify=get_ssl_context(), timeout=5) as client:
-        response = await client.post(_get_url(host), json=login, timeout=timeout)
-        if response.status_code == 200:
+        response = await client.post(_get_openbis_url(openbis_host), json=login, timeout=timeout)
+        if response.status_code != 200:
+            raise errors.ThirdPartyAPIError(
+                detail="OpenBIS responded with a non-200 status code when attempting to get a session token."
+            )
+        try:
             json: dict[str, str] = response.json()
-            if "result" in json and json["result"] is not None:
-                return json["result"]
-            # No session token was returned. Username and password may be incorrect.
-            raise errors.GeneralBadRequest()
-
-        # An openBIS session token related request failed.
-        raise errors.BaseError()
+        except JSONDecodeError as err:
+            raise errors.ThirdPartyAPIError(
+                detail="Did not receive a json-formatted output when attempting to get a session token from OpenBIS."
+            ) from err
+        if json.get("result") is None:
+            raise errors.ThirdPartyAPIError(
+                detail="The response from OpenBIS was parsed but it does not contain the exepected field(s) "
+                "when attempting to get a session token."
+            )
+        return json["result"]
 
 
 async def get_openbis_session_token_for_anonymous_user(
-    host: str,
+    openbis_host: str,
     timeout: int = 12,
 ) -> str:
     """Requests an openBIS session token for the anonymous user."""
     return await _get_openbis_session_token(
-        host, {"method": "loginAsAnonymousUser", "params": [], "id": "1", "jsonrpc": "2.0"}, timeout
+        openbis_host, {"method": "loginAsAnonymousUser", "params": [], "id": "1", "jsonrpc": "2.0"}, timeout
     )
 
 
 async def get_openbis_session_token(
-    host: str,
+    openbis_host: str,
     username: str,
     password: str,
     timeout: int = 12,
 ) -> str:
     """Requests an openBIS session token with the user's login credentials."""
     return await _get_openbis_session_token(
-        host, {"method": "login", "params": [username, password], "id": "2", "jsonrpc": "2.0"}, timeout
+        openbis_host, {"method": "login", "params": [username, password], "id": "2", "jsonrpc": "2.0"}, timeout
     )
 
 
 async def get_openbis_pat(
-    host: str,
+    openbis_host: str,
     session_id: str,
     personal_access_token_session_name: str = "renku",
     minimum_validity_in_days: int = 2,
     timeout: int = 12,
 ) -> tuple[str, datetime]:
     """Requests an openBIS PAT with an openBIS session ID."""
-    url = _get_url(host)
+    url = _get_openbis_url(openbis_host)
 
     async with httpx.AsyncClient(verify=get_ssl_context(), timeout=5) as client:
         get_server_information = {"method": "getServerInformation", "params": [session_id], "id": "2", "jsonrpc": "2.0"}
         response = await client.post(url, json=get_server_information, timeout=timeout)
-        if response.status_code == 200:
+        if response.status_code != 200:
+            logger.error(
+                f"Received a non-200 response, {response.status_code} from OpenBIS "
+                f"for performing 'getServerInformation'. Reponse content: {response.text}"
+            )
+            raise errors.ThirdPartyAPIError(
+                detail="OpenBIS responded with a non-200 status code when performing 'getServerInformation'."
+            )
+        try:
             json1: dict[str, dict[str, str]] = response.json()
-            if "error" not in json1:
-                personal_access_tokens_max_validity_period = int(
-                    json1["result"]["personal-access-tokens-max-validity-period"]
-                )
-                valid_from = datetime.now(ZoneInfo("Europe/Berlin"))
-                valid_to = valid_from + timedelta(seconds=personal_access_tokens_max_validity_period)
-                validity_in_days = (valid_to - valid_from).days
-                if validity_in_days >= minimum_validity_in_days:
-                    create_personal_access_tokens = {
-                        "method": "createPersonalAccessTokens",
-                        "params": [
-                            session_id,
-                            {
-                                "@type": "as.dto.pat.create.PersonalAccessTokenCreation",
-                                "sessionName": personal_access_token_session_name,
-                                "validFromDate": int(valid_from.timestamp() * 1000),
-                                "validToDate": int(valid_to.timestamp() * 1000),
-                            },
-                        ],
-                        "id": "2",
-                        "jsonrpc": "2.0",
-                    }
-                    response = await client.post(url, json=create_personal_access_tokens, timeout=timeout)
-                    if response.status_code == 200:
-                        json2: dict[str, list[dict[str, str]]] = response.json()
-                        return json2["result"][0]["permId"], valid_to
-                else:
-                    # The maximum allowed validity period of a personal access token is less than
-                    # "minimum_validity_in_days" days.
-                    raise errors.GeneralBadRequest()
-
-        # An openBIS personal access token related request failed.
-        raise errors.BaseError()
+        except JSONDecodeError as err:
+            logger.error(
+                f"Could not parse OpenBIS response for performing 'getServerInformation' into JSON. "
+                f"Response content: {response.text}"
+            )
+            raise errors.ThirdPartyAPIError(
+                detail="Could not parse OpenBIS response about server information into JSON."
+            ) from err
+        if "error" in json1:
+            raise errors.ThirdPartyAPIError(
+                detail=f"The response from OpenBIS for 'getServerInformation' contained errors: {json1['error']}."
+            )
+        if json1.get("result", {}).get("personal-access-tokens-max-validity-period") is None:
+            logger.error(
+                f"The response from OpenBIS for 'getServerInformation' did not contain the expected "
+                "token validity period fields. "
+                f"Response content: {response.text}"
+            )
+            raise errors.ThirdPartyAPIError(
+                detail="The response from OpenBIS for 'getServerInformation' "
+                "did not contain the expected token validity period."
+            )
+        personal_access_tokens_max_validity_period = int(json1["result"]["personal-access-tokens-max-validity-period"])
+        valid_from = datetime.now(ZoneInfo("Europe/Berlin"))
+        valid_to = valid_from + timedelta(seconds=personal_access_tokens_max_validity_period)
+        validity_in_days = (valid_to - valid_from).days
+        if validity_in_days < minimum_validity_in_days:
+            raise errors.ThirdPartyAPIError(
+                detail="The allowed validity of the personal access token from OpenBIS is shorter "
+                f"than the required minimum validity of {minimum_validity_in_days} days"
+            )
+        create_personal_access_tokens = {
+            "method": "createPersonalAccessTokens",
+            "params": [
+                session_id,
+                {
+                    "@type": "as.dto.pat.create.PersonalAccessTokenCreation",
+                    "sessionName": personal_access_token_session_name,
+                    "validFromDate": int(valid_from.timestamp() * 1000),
+                    "validToDate": int(valid_to.timestamp() * 1000),
+                },
+            ],
+            "id": "2",
+            "jsonrpc": "2.0",
+        }
+        response = await client.post(url, json=create_personal_access_tokens, timeout=timeout)
+        if response.status_code != 200:
+            logger.error(
+                "OpenBIS responded with a non-200 status code when creating a personal access token. "
+                f"Status code: {response.status_code}, response content: {response.text}"
+            )
+            raise errors.ThirdPartyAPIError(
+                detail="OpenBIS responded with a non-200 status code when creating a personal access token."
+            )
+        try:
+            json2: dict[str, list[dict[str, str]]] = response.json()
+        except JSONDecodeError as err:
+            logger.error(
+                "Could not parse OpenBIS response for creating a personal access token into JSON."
+                f"Response content: {response.text}"
+            )
+            raise errors.ThirdPartyAPIError(
+                detail="Could not parse OpenBIS response for creating personal access token into JSON."
+            ) from err
+        if (
+            not isinstance(json2.get("result"), list)
+            or len(json2["result"]) == 0
+            or json2["result"][0].get("permId") is None
+        ):
+            logger.error(
+                "The response from OpenBIS did not have the required 'result[0].permId' field in the response "
+                "from creating a personal access token. "
+                f"Response content: {response.text}"
+            )
+            raise errors.ThirdPartyAPIError(
+                detail="The response from OpenBIS did not have the required 'result[0].permId' field in the response "
+                "from creating a personal access token."
+            )
+        return json2["result"][0]["permId"], valid_to

test/bases/renku_data_services/data_api/test_data_connectors.py

@@ -1640,7 +1640,7 @@ async def test_delete_data_connector_secrets(
 @pytest.mark.asyncio
 async def test_create_openbis_data_connector(sanic_client, create_openbis_data_connector, user_headers) -> None:
     openbis_session_token = await get_openbis_session_token_for_anonymous_user(
-        host="openbis-eln-lims.ethz.ch",  # Public openBIS demo instance.
+        openbis_host="openbis-eln-lims.ethz.ch",  # Public openBIS demo instance.
     )
     data_connector = await create_openbis_data_connector(
         "openBIS data connector 1", session_token=openbis_session_token

test/bases/renku_data_services/data_api/test_secret.py

@@ -239,7 +239,7 @@ async def test_get_update_a_secret(sanic_client: SanicASGITestClient, user_heade
     _, response = await sanic_client.patch(
         f"/api/data/user/secrets/{secret["id"]}",
         headers=user_headers,
-        json={"value": "newest-value", "expiration_timestamp": "2029-12-31"},
+        json={"value": "newest-value", "expiration_timestamp": "2029-12-31T00:00:00Z"},
     )
     assert response.status_code == 200, response.text
 

test/bases/renku_data_services/data_api/test_storage.py

@@ -604,7 +604,7 @@ async def test_storage_validate_connection(storage_test_client) -> None:
 @pytest.mark.asyncio
 async def test_openbis_storage_validate_connection(storage_test_client) -> None:
     openbis_session_token = await get_openbis_session_token(
-        host="openbis-eln-lims.ethz.ch",  # Public openBIS demo instance.
+        openbis_host="openbis-eln-lims.ethz.ch",  # Public openBIS demo instance.
         username="observer",
         password="1234",
     )