refactor: Rename CredentialProvider to ArtifactsHelperCredentialProvider

delilahw · delilahw · commit d6db7e51ea40 · 2024-04-23T09:55:25.000+10:00
Also renames the test files to correspond to the exercised source file.
diff --git a/src/artifacts-helper/codespaces_artifacts_helper_keyring/src/codespaces_artifacts_helper_keyring/__init__.py b/src/artifacts-helper/codespaces_artifacts_helper_keyring/src/codespaces_artifacts_helper_keyring/__init__.py
@@ -1,2 +1,2 @@
-from .artifacts_helper_wrapper import CredentialProvider
+from .artifacts_helper_credential_provider import ArtifactsHelperCredentialProvider
 from .keyring_backend import CodespacesArtifactsHelperKeyringBackend
diff --git a/src/artifacts-helper/codespaces_artifacts_helper_keyring/src/codespaces_artifacts_helper_keyring/artifacts_helper_credential_provider.py b/src/artifacts-helper/codespaces_artifacts_helper_keyring/src/codespaces_artifacts_helper_keyring/artifacts_helper_credential_provider.py
@@ -22,11 +22,11 @@ class Credentials:
     password: str
 
 
-class CredentialProviderError(RuntimeError):
+class ArtifactsHelperCredentialProviderError(RuntimeError):
     pass
 
 
-class CredentialProvider:
+class ArtifactsHelperCredentialProvider:
     DEFAULT_AUTH_HELPER_PATH = "~/ado-auth-helper"
 
     def __init__(
@@ -39,20 +39,18 @@ def __init__(
 
     @staticmethod
     def resolve_auth_helper_path(
-        auth_helper_path: Union[os.PathLike, str], ) -> Optional[str]:
-        return shutil.which(str(Path(auth_helper_path).expanduser()),
-                            mode=os.X_OK)
+        auth_helper_path: Union[os.PathLike, str],
+    ) -> Optional[str]:
+        return shutil.which(str(Path(auth_helper_path).expanduser()), mode=os.X_OK)
 
     @classmethod
-    def auth_helper_installed(
-            cls, auth_helper_path: Union[os.PathLike, str]) -> bool:
+    def auth_helper_installed(cls, auth_helper_path: Union[os.PathLike, str]) -> bool:
         return cls.resolve_auth_helper_path(auth_helper_path) is not None
 
     def get_credentials(self, url) -> Optional[Credentials]:
         # Public feed short circuit: return nothing if not getting credentials for the upload endpoint
         # (which always requires auth) and the endpoint is public (can authenticate without credentials).
-        if not self._is_upload_endpoint(url) and self._can_authenticate(
-                url, None):
+        if not self._is_upload_endpoint(url) and self._can_authenticate(url, None):
             return None
 
         jwt_str = self._get_jwt_from_helper()
@@ -68,13 +66,13 @@ def _is_upload_endpoint(url) -> bool:
 
     def _can_authenticate(self, url, auth) -> bool:
         response = requests.get(url, auth=auth, timeout=self.timeout)
-        return response.status_code < 500 and response.status_code not in (401,
-                                                                           403)
+        return response.status_code < 500 and response.status_code not in (401, 403)
 
     def _get_jwt_from_helper(self) -> str:
         if self.auth_tool_path is None:
-            raise CredentialProviderError(
-                "Failed to get credentials: No authentication tool found")
+            raise ArtifactsHelperCredentialProviderError(
+                "Failed to get credentials: No authentication tool found"
+            )
 
         try:
             p = subprocess.run(
@@ -88,27 +86,29 @@ def _get_jwt_from_helper(self) -> str:
             if stdout:
                 return stdout.strip()
             else:
-                raise CredentialProviderError(
+                raise ArtifactsHelperCredentialProviderError(
                     f"Failed to get credentials: No output from subprocess {self.auth_tool_path}"
                 )
 
         except subprocess.CalledProcessError as e:
-            raise CredentialProviderError(
+            raise ArtifactsHelperCredentialProviderError(
                 f"Failed to get credentials: Process {self.auth_tool_path} exited with code {e.returncode}. Error: {e.stderr}"
             ) from e
         except subprocess.TimeoutExpired as e:
-            raise CredentialProviderError(
+            raise ArtifactsHelperCredentialProviderError(
                 f"Failed to get credentials: Process {self.auth_tool_path} timed out after {self.timeout} seconds"
             ) from e
 
     def _get_credentials_from_jwt(self, jwt_str: str) -> Credentials:
         try:
-            decoded = jwt.decode(jwt_str,
-                                 verify=False,
-                                 options={"verify_signature": False})
+            decoded = jwt.decode(
+                jwt_str, verify=False, options={"verify_signature": False}
+            )
             return Credentials(
                 username=decoded.get("unique_name", decoded.get("upn", None)),
                 password=jwt_str,
             )
         except jwt.PyJWTError as e:
-            raise CredentialProviderError(f"Failed to decode JWT: {e}") from e
+            raise ArtifactsHelperCredentialProviderError(
+                f"Failed to decode JWT: {e}"
+            ) from e
diff --git a/src/artifacts-helper/codespaces_artifacts_helper_keyring/src/codespaces_artifacts_helper_keyring/keyring_backend.py b/src/artifacts-helper/codespaces_artifacts_helper_keyring/src/codespaces_artifacts_helper_keyring/keyring_backend.py
@@ -6,7 +6,7 @@
 from keyring.backend import KeyringBackend
 from keyring.credentials import Credential, SimpleCredential
 
-from .artifacts_helper_wrapper import CredentialProvider
+from .artifacts_helper_credential_provider import ArtifactsHelperCredentialProvider
 
 
 class CodespacesArtifactsHelperKeyringBackend(KeyringBackend):
@@ -17,7 +17,9 @@ class CodespacesArtifactsHelperKeyringBackend(KeyringBackend):
         "pkgs.vsts.me",
     )
 
-    _PROVIDER: Type[CredentialProvider] = CredentialProvider
+    _PROVIDER: Type[ArtifactsHelperCredentialProvider] = (
+        ArtifactsHelperCredentialProvider
+    )
     AUTH_HELPER_PATH = _PROVIDER.DEFAULT_AUTH_HELPER_PATH
 
     @properties.classproperty
diff --git a/src/artifacts-helper/codespaces_artifacts_helper_keyring/tests/test_artifacts_helper_credential_provider.py b/src/artifacts-helper/codespaces_artifacts_helper_keyring/tests/test_artifacts_helper_credential_provider.py
@@ -6,10 +6,10 @@
 
 import pytest
 from codespaces_artifacts_helper_keyring import (
-    CredentialProvider,
+    ArtifactsHelperCredentialProvider,
 )
-from codespaces_artifacts_helper_keyring.artifacts_helper_wrapper import (
-    CredentialProviderError,
+from codespaces_artifacts_helper_keyring.artifacts_helper_credential_provider import (
+    ArtifactsHelperCredentialProviderError,
 )
 
 
@@ -38,61 +38,70 @@ def write_script(self, content: str, shebang: str = "#!/usr/bin/env python3"):
         set_path_executable(self.script_path)
 
     def test_auth_helper_installed_invalid_path(self):
-        assert not CredentialProvider.resolve_auth_helper_path(
+        assert not ArtifactsHelperCredentialProvider.resolve_auth_helper_path(
             self.tmp_dir / "nonexistent"
         )
-        assert not CredentialProvider.auth_helper_installed(
+        assert not ArtifactsHelperCredentialProvider.auth_helper_installed(
             self.tmp_dir / "nonexistent"
         )
 
     def test_auth_helper_installed_and_not_executable(self):
         self.write_script("pass")
         set_path_not_executable(self.script_path)
-        assert not CredentialProvider.resolve_auth_helper_path(self.script_path)
-        assert not CredentialProvider.auth_helper_installed(self.script_path)
+        assert not ArtifactsHelperCredentialProvider.resolve_auth_helper_path(
+            self.script_path
+        )
+        assert not ArtifactsHelperCredentialProvider.auth_helper_installed(
+            self.script_path
+        )
 
     def test_auth_helper_installed_and_executable(self):
         self.write_script("pass")
-        assert CredentialProvider.resolve_auth_helper_path(self.script_path) is not None
-        assert CredentialProvider.auth_helper_installed(self.script_path)
+        assert (
+            ArtifactsHelperCredentialProvider.resolve_auth_helper_path(self.script_path)
+            is not None
+        )
+        assert ArtifactsHelperCredentialProvider.auth_helper_installed(self.script_path)
 
     def test_get_jwt_from_helper(self):
         raw_jwt_value = "raw_jwt_here._-azAZ09"
         self.write_script(f"print('{raw_jwt_value}')")
-        provider = CredentialProvider(self.script_path)
+        provider = ArtifactsHelperCredentialProvider(self.script_path)
         assert provider._get_jwt_from_helper() == raw_jwt_value.strip()
 
     def test_get_jwt_from_helper_not_installed(self):
-        provider = CredentialProvider()
+        provider = ArtifactsHelperCredentialProvider()
         with pytest.raises(
-            CredentialProviderError, match="No authentication tool found"
+            ArtifactsHelperCredentialProviderError, match="No authentication tool found"
         ):
             provider._get_jwt_from_helper()
 
     def test_get_credentials_from_jwt(self):
-        provider = CredentialProvider()
+        provider = ArtifactsHelperCredentialProvider()
         creds = provider._get_credentials_from_jwt(self.TEST_JWT)
         assert creds.username == self.TEST_JWT_USERNAME
         assert creds.password == self.TEST_JWT
 
     def test_get_credentials(self):
         self.write_script(f"print('{self.TEST_JWT}')")
-        provider = CredentialProvider(self.script_path)
+        provider = ArtifactsHelperCredentialProvider(self.script_path)
         creds = provider.get_credentials(self.SUPPORTED_HOST)
         assert creds.username == self.TEST_JWT_USERNAME
         assert creds.password == self.TEST_JWT
 
     def test_get_credentials_invalid_jwt(self):
         self.write_script("print('invalid jwt')")
-        provider = CredentialProvider(self.script_path)
-        with pytest.raises(CredentialProviderError, match="Failed to decode JWT:"):
+        provider = ArtifactsHelperCredentialProvider(self.script_path)
+        with pytest.raises(
+            ArtifactsHelperCredentialProviderError, match="Failed to decode JWT:"
+        ):
             provider.get_credentials(self.SUPPORTED_HOST)
 
     def test_get_crendentials_helper_non_zero_exit(self):
         self.write_script("exit(1)")
-        provider = CredentialProvider(self.script_path)
+        provider = ArtifactsHelperCredentialProvider(self.script_path)
         with pytest.raises(
-            CredentialProviderError,
+            ArtifactsHelperCredentialProviderError,
             match=f"Process .*{self.script_name}.* exited with code 1",
         ):
             provider.get_credentials(self.SUPPORTED_HOST)
diff --git a/src/artifacts-helper/codespaces_artifacts_helper_keyring/tests/test_keyring_backend.py b/src/artifacts-helper/codespaces_artifacts_helper_keyring/tests/test_keyring_backend.py
@@ -9,8 +9,8 @@
 import keyring.errors
 import pytest
 from codespaces_artifacts_helper_keyring import (
+    ArtifactsHelperCredentialProvider,
     CodespacesArtifactsHelperKeyringBackend,
-    CredentialProvider,
 )
 from keyring.credentials import SimpleCredential
 
@@ -19,8 +19,7 @@
 SUPPORTED_HOST = "https://pkgs.dev.azure.com/"
 
 
-class FakeProvider(CredentialProvider):
-
+class FakeProvider(ArtifactsHelperCredentialProvider):
     def get_credentials(self, service):
         return SimpleCredential("user" + service[-4:], "pass" + service[-4:])
 
@@ -68,8 +67,7 @@ def passwords(monkeypatch):
     def mock_get_all_keyring():
         return [CodespacesArtifactsHelperKeyringBackend(), passwords_backend]
 
-    monkeypatch.setattr(keyring.backend, "get_all_keyring",
-                        mock_get_all_keyring)
+    monkeypatch.setattr(keyring.backend, "get_all_keyring", mock_get_all_keyring)
 
     chainer_backend = keyring.backends.chainer.ChainerBackend()
 
@@ -81,8 +79,9 @@ def mock_get_all_keyring():
 
 @pytest.fixture
 def fake_provider(monkeypatch):
-    monkeypatch.setattr(CodespacesArtifactsHelperKeyringBackend, "_PROVIDER",
-                        FakeProvider)
+    monkeypatch.setattr(
+        CodespacesArtifactsHelperKeyringBackend, "_PROVIDER", FakeProvider
+    )
 
 
 def test_get_credential_unsupported_host(only_backend):
@@ -102,8 +101,7 @@ def test_set_password_raises(only_backend):
 
 def test_set_password_fallback(passwords, fake_provider):
     # Ensure we are getting good credentials
-    assert keyring.get_credential(SUPPORTED_HOST + "1234",
-                                  None).password == "pass1234"
+    assert keyring.get_credential(SUPPORTED_HOST + "1234", None).password == "pass1234"
 
     assert keyring.get_password("SYSTEM", "USERNAME") is None
     keyring.set_password("SYSTEM", "USERNAME", "PASSWORD")
@@ -113,8 +111,7 @@ def test_set_password_fallback(passwords, fake_provider):
     assert keyring.get_credential("SYSTEM", "USERNAME").password == "PASSWORD"
 
     # Ensure we are getting good credentials
-    assert keyring.get_credential(SUPPORTED_HOST + "1234",
-                                  None).password == "pass1234"
+    assert keyring.get_credential(SUPPORTED_HOST + "1234", None).password == "pass1234"
 
 
 def test_delete_password_raises(only_backend):
@@ -124,8 +121,7 @@ def test_delete_password_raises(only_backend):
 
 def test_delete_password_fallback(passwords, fake_provider):
     # Ensure we are getting good credentials
-    assert keyring.get_credential(SUPPORTED_HOST + "1234",
-                                  None).password == "pass1234"
+    assert keyring.get_credential(SUPPORTED_HOST + "1234", None).password == "pass1234"
 
     passwords["SYSTEM", "USERNAME"] = "PASSWORD"
     keyring.delete_password("SYSTEM", "USERNAME")

Original file line number	Diff line number	Diff line change
`@@ -1,2 +1,2 @@`
`1`		`-from .artifacts_helper_wrapper import CredentialProvider`
	`1`	`+from .artifacts_helper_credential_provider import ArtifactsHelperCredentialProvider`
`2`	`2`	`from .keyring_backend import CodespacesArtifactsHelperKeyringBackend`