Merge pull request #86 from NERSC/85-client-silently-ignores-pem-file-if-user-expansion-is-needed

Author: tylern4

examples/check_current_and_past_jobs.ipynb

@@ -17,8 +17,6 @@
             "source": [
                 "from sfapi_client import Client\n",
                 "from sfapi_client.compute import Machine\n",
-                "from sfapi_client.paths import RemotePath\n",
-                "from pathlib import Path\n",
                 "\n",
                 "user_name = \"elvis\"\n",
                 "\n",

examples/run_job_and_check_status.ipynb

@@ -10,7 +10,7 @@
 from authlib.jose import JsonWebKey
 
 from .compute import Machine, AsyncCompute
-from ..exceptions import SfApiError
+from ..exceptions import ClientKeyError
 from .._models import (
     Changelog as ChangelogItem,
     Config as ConfItem,
@@ -231,7 +231,7 @@ def __init__(
 
         :param client_id: The client ID
         :param secret: The client secret
-        :param key: The path to the client secret file
+        :param key: Full path to the client secret file, or path relative to `~` from the expanduser
         :param api_base_url: The API base URL
         :param token_url: The token URL
         :param access_token: An existing access token
@@ -311,10 +311,13 @@ async def close(self):
     async def __aexit__(self, type, value, traceback):
         await self.close()
 
-    def _read_client_secret_from_file(self, name):
-        if name is not None and Path(name).exists():
+    def _read_client_secret_from_file(self, name: Optional[Union[str, Path]]):
+        if name is None:
+            return
+        _path = Path(name).expanduser().resolve()
+        if _path.exists():
             # If the user gives a full path, then use it
-            key_path = Path(name)
+            key_path = _path
         else:
             # If not let's search in ~/.superfacility for the name or any key
             nickname = "" if name is None else name
@@ -326,12 +329,14 @@ def _read_client_secret_from_file(self, name):
 
         # We have no credentials
         if key_path is None or key_path.is_dir():
-            return
+            raise ClientKeyError(
+                f"no key found at key_path: {_path} or in ~/.superfacility/{name}*"
+            )
 
         # Check that key is read only in case it's not
         # 0o100600 means chmod 600
         if key_path.stat().st_mode != 0o100600:
-            raise SfApiError(
+            raise ClientKeyError(
                 f"Incorrect permissions on the key. To fix run: chmod 600 {key_path}"
             )
 
@@ -351,7 +356,7 @@ def _read_client_secret_from_file(self, name):
 
         # Validate we got a correct looking client_id
         if len(self._client_id) != 13:
-            raise SfApiError(f"client_id not found in file {key_path}")
+            raise ClientKeyError(f"client_id not found in file {key_path}")
 
     @tenacity.retry(
         retry=tenacity.retry_if_exception_type(httpx.TimeoutException)

src/sfapi_client/_async/client.py

@@ -10,7 +10,7 @@
 from authlib.jose import JsonWebKey
 
 from .compute import Machine, Compute
-from ..exceptions import SfApiError
+from ..exceptions import ClientKeyError
 from .._models import (
     Changelog as ChangelogItem,
     Config as ConfItem,
@@ -231,7 +231,7 @@ def __init__(
 
         :param client_id: The client ID
         :param secret: The client secret
-        :param key: The path to the client secret file
+        :param key: Full path to the client secret file, or path relative to `~` from the expanduser
         :param api_base_url: The API base URL
         :param token_url: The token URL
         :param access_token: An existing access token
@@ -260,7 +260,7 @@ def __enter__(self):
 
     def _http_client(self):
         headers = {"accept": "application/json"}
-        # If we have a client_id then we need to use OAuth2 client
+        # If we have a client_id then we need to use the OAuth2 client
         if self._client_id is not None:
             if self.__http_client is None:
                 # Create a new session if we haven't already
@@ -311,10 +311,13 @@ def close(self):
     def __exit__(self, type, value, traceback):
         self.close()
 
-    def _read_client_secret_from_file(self, name):
-        if name is not None and Path(name).exists():
+    def _read_client_secret_from_file(self, name: Optional[Union[str, Path]]):
+        if name is None:
+            return
+        _path = Path(name).expanduser().resolve()
+        if _path.exists():
             # If the user gives a full path, then use it
-            key_path = Path(name)
+            key_path = _path
         else:
             # If not let's search in ~/.superfacility for the name or any key
             nickname = "" if name is None else name
@@ -326,12 +329,14 @@ def _read_client_secret_from_file(self, name):
 
         # We have no credentials
         if key_path is None or key_path.is_dir():
-            return
+            raise ClientKeyError(
+                f"no key found at key_path: {_path} or in ~/.superfacility/{name}*"
+            )
 
         # Check that key is read only in case it's not
         # 0o100600 means chmod 600
         if key_path.stat().st_mode != 0o100600:
-            raise SfApiError(
+            raise ClientKeyError(
                 f"Incorrect permissions on the key. To fix run: chmod 600 {key_path}"
             )
 
@@ -351,7 +356,7 @@ def _read_client_secret_from_file(self, name):
 
         # Validate we got a correct looking client_id
         if len(self._client_id) != 13:
-            raise SfApiError(f"client_id not found in file {key_path}")
+            raise ClientKeyError(f"client_id not found in file {key_path}")
 
     @tenacity.retry(
         retry=tenacity.retry_if_exception_type(httpx.TimeoutException)
@@ -501,6 +506,7 @@ def resources(self) -> Resources:
 
 # Ensure that the job models are built, we need to import here to
 # avoid circular imports
-from .jobs import JobSacct, JobSqueue
+from .jobs import JobSacct, JobSqueue  # noqa: E402
+
 JobSqueue.model_rebuild()
-JobSacct.model_rebuild()
+JobSacct.model_rebuild()

src/sfapi_client/_sync/client.py

@@ -165,7 +165,10 @@ def download(self, binary=False) -> IO[AnyStr]:
 
     @staticmethod
     def _ls(
-        compute: "Compute", path, directory=False, filter_dots=True  # noqa: F821
+        compute: "Compute",  # noqa: F821
+        path,
+        directory=False,
+        filter_dots=True,  # noqa: F821
     ) -> List["RemotePath"]:  # noqa: F821
         r = compute.client.get(f"utilities/ls/{compute.name}/{path}")
 

src/sfapi_client/_sync/paths.py

@@ -5,3 +5,12 @@ class SfApiError(Exception):
 
     def __init__(self, message):
         self.message = message
+
+
+class ClientKeyError(Exception):
+    """
+    Exception indicating an error occurred reading the client keys
+    """
+
+    def __init__(self, message):
+        self.message = message

src/sfapi_client/exceptions.py

@@ -2,6 +2,8 @@
 import random
 import string
 from typing import Optional, Union, Dict
+from pathlib import Path
+from cryptography.hazmat.primitives.asymmetric import rsa
 
 import pytest
 from authlib.jose import JsonWebKey
@@ -174,3 +176,44 @@ def async_authenticated_client(api_base_url, token_url, client_id, client_secret
 @pytest.fixture
 def access_token():
     return settings.ACCESS_TOKEN
+
+
+@pytest.fixture
+def fake_key_file(tmp_path_factory):
+    try:
+        tmp_path_factory._basetemp = Path().home()
+        key_path = tmp_path_factory.mktemp(".sfapi_test1", numbered=False) / "key.pem"
+
+        # Make a fake key for testing
+        key_path.write_text(
+            f"""abcdefghijlmo
+    -----BEGIN RSA PRIVATE KEY-----
+    {rsa.generate_private_key(public_exponent=65537, key_size=2048)}
+    -----END RSA PRIVATE KEY-----
+    """
+        )
+        key_path.chmod(0o100600)
+        yield key_path
+    finally:
+        # make sure to cleanup the test since we put a file in ~/.sfapi_test
+        temp_path = Path().home() / ".sfapi_test1"
+        if temp_path.exists():
+            (temp_path / "key.pem").unlink(missing_ok=True)
+            temp_path.rmdir()
+
+
+@pytest.fixture
+def empty_key_file(tmp_path_factory):
+    try:
+        tmp_path_factory._basetemp = Path().home()
+        key_path = tmp_path_factory.mktemp(".sfapi_test2", numbered=False) / "nokey.pem"
+        # Makes an empty key
+        key_path.write_text("")
+        key_path.chmod(0o100600)
+        yield key_path
+    finally:
+        # make sure to cleanup the test since we put a file in ~/.sfapi_test
+        temp_path = Path().home() / ".sfapi_test2"
+        if temp_path.exists():
+            (temp_path / "nokey.pem").unlink(missing_ok=True)
+            temp_path.rmdir()

tests/conftest.py

@@ -0,0 +1,33 @@
+import pytest
+
+from sfapi_client import Client
+from sfapi_client.exceptions import ClientKeyError
+
+
+@pytest.mark.public
+def test_wrong_key_data(fake_key_file, test_machine):
+    with Client(key=fake_key_file) as client:
+        with pytest.raises(Exception):
+            # Raises OAuthError when trying to read incorrect PEM
+            client.compute(test_machine)
+
+
+@pytest.mark.public
+def test_empty_key_file(empty_key_file):
+    with pytest.raises(ClientKeyError):
+        # Raise ClientKeyError for emtpy key file
+        Client(key=empty_key_file)
+
+
+@pytest.mark.public
+def test_no_key_file_path():
+    with pytest.raises(ClientKeyError):
+        # Raise error when there is no key present
+        Client(key="~/name")
+
+
+@pytest.mark.public
+def test_no_key_file_name():
+    with pytest.raises(ClientKeyError):
+        # Raise error when searching for keys
+        Client(key="name")