From 964c4ce61d58d6a7fb1949408390461f968a7854 Mon Sep 17 00:00:00 2001 From: Ihor Kalnytskyi Date: Sun, 5 May 2024 22:35:11 +0300 Subject: [PATCH 01/12] Use 'ruff.line-length=100' to lint & format Even though I'm still trying to wrap my lines at 80 characters, over the years I realized that blindly following the 80 characters rather harms than helps. That's why I want to bump the line-length limit to 100 characters. --- pyproject.toml | 2 +- src/httpie_credential_store/_auth.py | 8 ++------ src/httpie_credential_store/_keychain.py | 5 +---- tests/test_keychain_password_store.py | 4 +--- tests/test_keychain_shell.py | 7 ++----- tests/test_keychain_system.py | 3 +-- tests/test_plugin.py | 24 ++++++------------------ 7 files changed, 14 insertions(+), 39 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 5e829ad..70c8338 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -28,5 +28,5 @@ requires = ["poetry>=0.12"] build-backend = "poetry.masonry.api" [tool.ruff] -line-length = 88 +line-length = 100 target-version = "py38" diff --git a/src/httpie_credential_store/_auth.py b/src/httpie_credential_store/_auth.py index 8d8efeb..7070f58 100644 --- a/src/httpie_credential_store/_auth.py +++ b/src/httpie_credential_store/_auth.py @@ -119,9 +119,7 @@ class HTTPMultipleAuth(requests.auth.AuthBase, AuthProvider): name = "multiple" def __init__(self, *, providers): - self._providers = [ - get_auth(provider.pop("provider"), **provider) for provider in providers - ] + self._providers = [get_auth(provider.pop("provider"), **provider) for provider in providers] def __call__(self, request): for provider in self._providers: @@ -129,9 +127,7 @@ def __call__(self, request): return request -_PROVIDERS = { - provider_cls.name: provider_cls for provider_cls in AuthProvider.__subclasses__() -} +_PROVIDERS = {provider_cls.name: provider_cls for provider_cls in AuthProvider.__subclasses__()} def get_auth(provider, **kwargs): diff --git a/src/httpie_credential_store/_keychain.py b/src/httpie_credential_store/_keychain.py index 649f832..4096118 100644 --- a/src/httpie_credential_store/_keychain.py +++ b/src/httpie_credential_store/_keychain.py @@ -64,10 +64,7 @@ def get(self, *, service, username): return secret -_PROVIDERS = { - provider_cls.name: provider_cls - for provider_cls in KeychainProvider.__subclasses__() -} +_PROVIDERS = {provider_cls.name: provider_cls for provider_cls in KeychainProvider.__subclasses__()} def get_keychain(provider): diff --git a/tests/test_keychain_password_store.py b/tests/test_keychain_password_store.py index a5bf0a4..b2de039 100644 --- a/tests/test_keychain_password_store.py +++ b/tests/test_keychain_password_store.py @@ -117,6 +117,4 @@ def test_secret_not_found(testkeychain): with pytest.raises(LookupError) as excinfo: testkeychain.get(name="testservice/testuser") - assert str(excinfo.value) == ( - "password-store: no secret found: 'testservice/testuser'" - ) + assert str(excinfo.value) == ("password-store: no secret found: 'testservice/testuser'") diff --git a/tests/test_keychain_shell.py b/tests/test_keychain_shell.py index e2d899e..c6d6f87 100644 --- a/tests/test_keychain_shell.py +++ b/tests/test_keychain_shell.py @@ -44,14 +44,11 @@ def test_secret_not_found(testkeychain, tmpdir): testkeychain.get(command=f"cat {secrettxt.strpath}") assert str(excinfo.value) == ( - f"No secret found: Command 'cat {secrettxt.strpath}' returned " - f"non-zero exit status 1." + f"No secret found: Command 'cat {secrettxt.strpath}' returned " f"non-zero exit status 1." ) -@pytest.mark.parametrize( - ["args", "kwargs"], [pytest.param(["echo p@ss"], {}, id="args")] -) +@pytest.mark.parametrize(["args", "kwargs"], [pytest.param(["echo p@ss"], {}, id="args")]) def test_keywords_only_arguments(testkeychain, args, kwargs): with pytest.raises(TypeError): testkeychain.get(*args, **kwargs) diff --git a/tests/test_keychain_system.py b/tests/test_keychain_system.py index da4ae88..ea7e472 100644 --- a/tests/test_keychain_system.py +++ b/tests/test_keychain_system.py @@ -55,8 +55,7 @@ def test_secret_not_found(testkeychain): assert testkeychain.get(service="testsvc", username="testuser") assert str(excinfo.value) == ( - "No secret found for 'testsvc' service and 'testuser' username " - "in 'system' keychain." + "No secret found for 'testsvc' service and 'testuser' username " "in 'system' keychain." ) diff --git a/tests/test_plugin.py b/tests/test_plugin.py index c861335..8a4fc7b 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -303,9 +303,7 @@ def test_creds_auth_token_scheme(httpie_run, set_credentials, creds_auth_type): @responses.activate -def test_creds_auth_token_keychain( - httpie_run, set_credentials, creds_auth_type, tmpdir -): +def test_creds_auth_token_keychain(httpie_run, set_credentials, creds_auth_type, tmpdir): """The plugin retrieves secrets from keychain for HTTP token auth.""" secrettxt = tmpdir.join("secret.txt") @@ -360,9 +358,7 @@ def test_creds_auth_header(httpie_run, set_credentials, creds_auth_type): @responses.activate -def test_creds_auth_header_keychain( - httpie_run, set_credentials, creds_auth_type, tmpdir -): +def test_creds_auth_header_keychain(httpie_run, set_credentials, creds_auth_type, tmpdir): """The plugin retrieves secrets from keychain for HTTP header auth.""" secrettxt = tmpdir.join("secret.txt") @@ -429,9 +425,7 @@ def test_creds_auth_multiple_token_header(httpie_run, set_credentials, creds_aut @responses.activate -def test_creds_auth_multiple_header_header( - httpie_run, set_credentials, creds_auth_type -): +def test_creds_auth_multiple_header_header(httpie_run, set_credentials, creds_auth_type): """The plugin supports usage of the same auth provider twice.""" set_credentials( @@ -632,9 +626,7 @@ def test_creds_auth_missing( "http://example.com/", id="no-protocol", ), - pytest.param( - r"example", "http://example.com/", "http://example.com/", id="part" - ), + pytest.param(r"example", "http://example.com/", "http://example.com/", id="part"), pytest.param( r"example.com", "http://example.com/foo/bar", @@ -819,9 +811,7 @@ def test_creds_lookup_by_id(httpie_run, set_credentials): @responses.activate -def test_creds_lookup_by_id_error( - httpie_run, set_credentials, httpie_stderr, creds_auth_type -): +def test_creds_lookup_by_id_error(httpie_run, set_credentials, httpie_stderr, creds_auth_type): """The plugin raises error if no credentials found.""" set_credentials( @@ -955,9 +945,7 @@ def test_creds_permissions_not_enough( @responses.activate -def test_creds_auth_no_database( - httpie_run, credentials_file, httpie_stderr, creds_auth_type -): +def test_creds_auth_no_database(httpie_run, credentials_file, httpie_stderr, creds_auth_type): """The plugin raises error if credentials file does not exist.""" httpie_run(["-A", creds_auth_type, "http://example.com"]) From e66d0c185051d9a5f0f19a8e7da000fa91f59c1a Mon Sep 17 00:00:00 2001 From: Ihor Kalnytskyi Date: Sun, 5 May 2024 22:43:49 +0300 Subject: [PATCH 02/12] Turn most Ruff lint rules on Default Ruff configuration tries to be as close as possible to Flake8, so that the former can be considered a drop-in replacement of the latter. Ruff, however, comes with many batteries included, and there are many useful rules out there. This patch enables most of them, leaving our the one the code base is not ready for for future. --- pyproject.toml | 11 ++++- src/httpie_credential_store/__init__.py | 1 - src/httpie_credential_store/_auth.py | 17 ++++--- src/httpie_credential_store/_keychain.py | 13 ++++-- src/httpie_credential_store/_plugin.py | 4 +- src/httpie_credential_store/_store.py | 14 +++--- tests/conftest.py | 9 ++-- tests/test_keychain_password_store.py | 18 ++++---- tests/test_keychain_shell.py | 6 +-- tests/test_keychain_system.py | 8 ++-- tests/test_plugin.py | 59 +++++++++++------------- 11 files changed, 86 insertions(+), 74 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 70c8338..1016978 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -17,7 +17,6 @@ keyring = ">= 23.5" [tool.poetry.dev-dependencies] pytest = "^7.1" responses = "^0.20" -mock = "^4.0" [tool.poetry.plugins."httpie.plugins.auth.v1"] credential-store = "httpie_credential_store:CredentialStoreAuthPlugin" @@ -30,3 +29,13 @@ build-backend = "poetry.masonry.api" [tool.ruff] line-length = 100 target-version = "py38" + +[tool.ruff.lint] +select = ["ALL"] +ignore = ["ANN", "D", "TRY", "S", "PTH", "PLR", "COM", "PT005", "ISC001", "INP001"] + +[tool.ruff.lint.per-file-ignores] +"tests/*" = ["INP001"] + +[tool.ruff.lint.isort] +known-first-party = ["httpie_credential_store"] diff --git a/src/httpie_credential_store/__init__.py b/src/httpie_credential_store/__init__.py index eb934b8..3549e96 100644 --- a/src/httpie_credential_store/__init__.py +++ b/src/httpie_credential_store/__init__.py @@ -2,5 +2,4 @@ from ._plugin import CredentialStoreAuthPlugin, CredsAuthPlugin - __all__ = ["CredentialStoreAuthPlugin", "CredsAuthPlugin"] diff --git a/src/httpie_credential_store/_auth.py b/src/httpie_credential_store/_auth.py index 7070f58..2fea0f9 100644 --- a/src/httpie_credential_store/_auth.py +++ b/src/httpie_credential_store/_auth.py @@ -8,7 +8,6 @@ from ._keychain import get_keychain - # These patterns are copied over from built-in `http.client` implementation, # and are more lenient than RFC definitions for backwards compatibility # reasons. @@ -45,7 +44,7 @@ class HTTPBasicAuth(requests.auth.HTTPBasicAuth, AuthProvider): name = "basic" def __init__(self, *, username, password): - super(HTTPBasicAuth, self).__init__(username, get_secret(password)) + super().__init__(username, get_secret(password)) class HTTPDigestAuth(requests.auth.HTTPDigestAuth, AuthProvider): @@ -54,7 +53,7 @@ class HTTPDigestAuth(requests.auth.HTTPDigestAuth, AuthProvider): name = "digest" def __init__(self, *, username, password): - super(HTTPDigestAuth, self).__init__(username, get_secret(password)) + super().__init__(username, get_secret(password)) class HTTPHeaderAuth(requests.auth.AuthBase, AuthProvider): @@ -67,18 +66,20 @@ def __init__(self, *, name, value): self._value = get_secret(value) if not is_legal_header_name(self._name): - raise ValueError( + error_message = ( f"HTTP header authentication provider received invalid " f"header name: {self._name!r}. Please remove illegal " f"characters and try again." ) + raise ValueError(error_message) if is_illegal_header_value(self._value): - raise ValueError( + error_message = ( f"HTTP header authentication provider received invalid " f"header value: {self._value!r}. Please remove illegal " f"characters and try again." ) + raise ValueError(error_message) def __call__(self, request): request.headers[self._name] = self._value @@ -95,18 +96,20 @@ def __init__(self, *, token, scheme="Bearer"): self._token = get_secret(token) if is_illegal_header_value(self._scheme): - raise ValueError( + error_message = ( f"HTTP token authentication provider received scheme that " f"contains illegal characters: {self._scheme!r}. Please " f"remove these characters and try again." ) + raise ValueError(error_message) if is_illegal_header_value(self._token): - raise ValueError( + error_message = ( f"HTTP token authentication provider received token that " f"contains illegal characters: {self._token!r}. Please " f"remove these characters and try again." ) + raise ValueError(error_message) def __call__(self, request): request.headers["Authorization"] = f"{self._scheme} {self._token}" diff --git a/src/httpie_credential_store/_keychain.py b/src/httpie_credential_store/_keychain.py index 4096118..1c35dbc 100644 --- a/src/httpie_credential_store/_keychain.py +++ b/src/httpie_credential_store/_keychain.py @@ -28,7 +28,8 @@ def get(self, *, command): try: return subprocess.check_output(command, shell=True).decode("UTF-8") except subprocess.CalledProcessError as exc: - raise LookupError(f"No secret found: {exc}") + error_message = f"No secret found: {exc}" + raise LookupError(error_message) from exc class PasswordStoreKeychain(ShellKeychain): @@ -40,10 +41,11 @@ def get(self, *, name): try: # password-store may store securely extra information along with a # password. Nevertheless, a password is always a first line. - text = super(PasswordStoreKeychain, self).get(command=f"pass {name}") + text = super().get(command=f"pass {name}") return text.splitlines()[0] - except LookupError: - raise LookupError(f"password-store: no secret found: '{name}'") + except LookupError as exc: + error_message = f"password-store: no secret found: '{name}'" + raise LookupError(error_message) from exc class SystemKeychain(KeychainProvider): @@ -57,10 +59,11 @@ def __init__(self): def get(self, *, service, username): secret = self._keyring.get_password(service, username) if not secret: - raise LookupError( + error_message = ( f"No secret found for '{service}' service and '{username}' " f"username in '{self.name}' keychain." ) + raise LookupError(error_message) return secret diff --git a/src/httpie_credential_store/_plugin.py b/src/httpie_credential_store/_plugin.py index ba60f62..8bcbbab 100644 --- a/src/httpie_credential_store/_plugin.py +++ b/src/httpie_credential_store/_plugin.py @@ -1,7 +1,7 @@ """HTTPie Credential Store Auth Plugin.""" -import requests import httpie.plugins +import requests from ._store import get_credential_store @@ -23,6 +23,8 @@ class CredentialStoreAuthPlugin(httpie.plugins.AuthPlugin): auth_parse = False # do not parse '-a' content def get_auth(self, username=None, password=None): + _ = username + _ = password credential_id = self.raw_auth class CredentialStoreAuth(requests.auth.AuthBase): diff --git a/src/httpie_credential_store/_store.py b/src/httpie_credential_store/_store.py index 64246db..0845bb6 100644 --- a/src/httpie_credential_store/_store.py +++ b/src/httpie_credential_store/_store.py @@ -1,6 +1,5 @@ """Credentials are managed here.""" -import io import json import os import re @@ -12,7 +11,7 @@ from ._auth import get_auth -class CredentialStore(object): +class CredentialStore: """Credential store, manages your credentials.""" def __init__(self, credentials): @@ -42,10 +41,11 @@ def get_credential_store(name, directory=httpie.config.DEFAULT_CONFIG_DIR): credential_file = os.path.join(directory, name) if not os.path.exists(credential_file): - raise FileNotFoundError( + error_message = ( f"Credentials file '{credential_file}' is not found; " f"please create one and try again." ) + raise FileNotFoundError(error_message) mode = stat.S_IMODE(os.stat(credential_file).st_mode) @@ -56,20 +56,22 @@ def get_credential_store(name, directory=httpie.config.DEFAULT_CONFIG_DIR): # ignore this platform for a while. if sys.platform != "win32": if mode & 0o077 > 0o000: - raise PermissionError( + error_message = ( f"Permissions '{mode:04o}' for '{credential_file}' are too " f"open; please ensure your credentials file is NOT accessible " f"by others." ) + raise PermissionError(error_message) if mode & 0o400 != 0o400: - raise PermissionError( + error_message = ( f"Permissions '{mode:04o}' for '{credential_file}' are too " f"close; please ensure your credentials file CAN be read by " f"you." ) + raise PermissionError(error_message) - with io.open(credential_file, encoding="UTF-8") as f: + with open(credential_file, encoding="UTF-8") as f: credentials = json.load(f) return CredentialStore(credentials) diff --git a/tests/conftest.py b/tests/conftest.py index c1af556..0deca9e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,7 +1,7 @@ import os import tempfile +import unittest.mock as mock -import mock import pytest @@ -18,6 +18,7 @@ def _httpie_config_dir(): # HTTPie package is imported and that's why the very same value must be # used for all tests (session scope). Otherwise, tests may fail because # they will look for credentials file in different directory. - with tempfile.TemporaryDirectory() as tmpdir: - with mock.patch.dict(os.environ, {"HTTPIE_CONFIG_DIR": tmpdir}): - yield tmpdir + with tempfile.TemporaryDirectory() as tmpdir, mock.patch.dict( + os.environ, {"HTTPIE_CONFIG_DIR": tmpdir} + ): + yield tmpdir diff --git a/tests/test_keychain_password_store.py b/tests/test_keychain_password_store.py index b2de039..e862545 100644 --- a/tests/test_keychain_password_store.py +++ b/tests/test_keychain_password_store.py @@ -10,7 +10,6 @@ import py import pytest - _is_windows = sys.platform == "win32" _is_macos = sys.platform == "darwin" @@ -32,13 +31,13 @@ # built-in 'tmpdir' fixture produces too long names. That's why on macOS we # override 'tmpdir' fixture to return much shorter path to a temporary # directory. - @pytest.fixture(scope="function") + @pytest.fixture() def tmpdir(): with tempfile.TemporaryDirectory() as path: yield py.path.local(path) -@pytest.fixture(scope="function") +@pytest.fixture() def gpg_key_id(monkeypatch, tmpdir): """Return a Key ID of just generated GPG key.""" @@ -72,11 +71,12 @@ def gpg_key_id(monkeypatch, tmpdir): key = re.search(r"\s+([0-9A-F]{40})\s+", keys) if not key: - raise RuntimeError("cannot generate a GPG key") + error_message = "cannot generate a GPG key" + raise RuntimeError(error_message) return key.group(1) -@pytest.fixture(scope="function", autouse=True) +@pytest.fixture(autouse=True) def password_store_dir(monkeypatch, tmpdir): """Set password-store home directory to a temporary one.""" @@ -89,7 +89,7 @@ def password_store_dir(monkeypatch, tmpdir): return passstore.strpath -@pytest.fixture(scope="function") +@pytest.fixture() def testkeychain(): """Keychain instance under test.""" @@ -104,8 +104,8 @@ def testkeychain(): def test_secret_retrieved(testkeychain, gpg_key_id): """The keychain returns stored secret, no bullshit.""" - subprocess.run(f"pass init {gpg_key_id}", shell=True) - subprocess.run("pass generate testservice/testuser 14", shell=True) + subprocess.run(f"pass init {gpg_key_id}", shell=True, check=False) + subprocess.run("pass generate testservice/testuser 14", shell=True, check=False) secret = testkeychain.get(name="testservice/testuser") assert len(secret) == 14 @@ -117,4 +117,4 @@ def test_secret_not_found(testkeychain): with pytest.raises(LookupError) as excinfo: testkeychain.get(name="testservice/testuser") - assert str(excinfo.value) == ("password-store: no secret found: 'testservice/testuser'") + assert str(excinfo.value) == "password-store: no secret found: 'testservice/testuser'" diff --git a/tests/test_keychain_shell.py b/tests/test_keychain_shell.py index c6d6f87..57517c7 100644 --- a/tests/test_keychain_shell.py +++ b/tests/test_keychain_shell.py @@ -5,7 +5,7 @@ import pytest -@pytest.fixture(scope="function") +@pytest.fixture() def testkeychain(): """Keychain instance under test.""" @@ -44,11 +44,11 @@ def test_secret_not_found(testkeychain, tmpdir): testkeychain.get(command=f"cat {secrettxt.strpath}") assert str(excinfo.value) == ( - f"No secret found: Command 'cat {secrettxt.strpath}' returned " f"non-zero exit status 1." + f"No secret found: Command 'cat {secrettxt.strpath}' returned non-zero exit status 1." ) -@pytest.mark.parametrize(["args", "kwargs"], [pytest.param(["echo p@ss"], {}, id="args")]) +@pytest.mark.parametrize(("args", "kwargs"), [pytest.param(["echo p@ss"], {}, id="args")]) def test_keywords_only_arguments(testkeychain, args, kwargs): with pytest.raises(TypeError): testkeychain.get(*args, **kwargs) diff --git a/tests/test_keychain_system.py b/tests/test_keychain_system.py index ea7e472..ebf9356 100644 --- a/tests/test_keychain_system.py +++ b/tests/test_keychain_system.py @@ -19,7 +19,7 @@ def set_password(self, service, username, password): self._keyring[(service, username)] = password -@pytest.fixture(scope="function", autouse=True) +@pytest.fixture(autouse=True) def keyring_backend(): """Temporary set in-memory keyring as current backend.""" @@ -29,7 +29,7 @@ def keyring_backend(): keyring.set_keyring(prev_backend) -@pytest.fixture(scope="function") +@pytest.fixture() def testkeychain(): """Keychain instance under test.""" @@ -55,12 +55,12 @@ def test_secret_not_found(testkeychain): assert testkeychain.get(service="testsvc", username="testuser") assert str(excinfo.value) == ( - "No secret found for 'testsvc' service and 'testuser' username " "in 'system' keychain." + "No secret found for 'testsvc' service and 'testuser' username in 'system' keychain." ) @pytest.mark.parametrize( - ["args", "kwargs"], + ("args", "kwargs"), [ pytest.param(["testsvc", "testuser"], {}, id="args"), pytest.param(["testsvc"], {"username": "testuser"}, id="args-kwargs"), diff --git a/tests/test_plugin.py b/tests/test_plugin.py index 8a4fc7b..6b0cf52 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -6,17 +6,15 @@ import pathlib import re import sys - from urllib.request import parse_http_list, parse_keqv_list import pytest import responses - _is_windows = sys.platform == "win32" -class _DigestAuthHeader(object): +class _DigestAuthHeader: """Assert that a given Authorization header has expected digest parameters.""" def __init__(self, parameters): @@ -29,7 +27,7 @@ def __eq__(self, authorization_header_value): return True -class _RegExp(object): +class _RegExp: """Assert that a given string meets some expectations.""" def __init__(self, pattern, flags=0): @@ -42,7 +40,7 @@ def __repr__(self): return self._regex.pattern -@pytest.fixture(scope="function", autouse=True) +@pytest.fixture(autouse=True) def httpie_config_dir(_httpie_config_dir): """Return a path to HTTPie configuration directory.""" @@ -55,33 +53,33 @@ def httpie_config_dir(_httpie_config_dir): path.unlink() -@pytest.fixture(scope="function") +@pytest.fixture() def credentials_file(httpie_config_dir): """Return a path to credentials file.""" return os.path.join(httpie_config_dir, "credentials.json") -@pytest.fixture(scope="function") +@pytest.fixture() def set_credentials(credentials_file): """Render given credentials to credentials.json.""" def render(credentials, mode=0o600): - with io.open(credentials_file, "wt", encoding="UTF-8") as f: + with open(credentials_file, "w", encoding="UTF-8") as f: f.write(json.dumps(credentials, indent=4)) os.chmod(credentials_file, mode) return render -@pytest.fixture(scope="function") +@pytest.fixture() def httpie_stderr(): """Return captured standard error stream of HTTPie.""" return io.StringIO() -@pytest.fixture(scope="function") +@pytest.fixture() def httpie_run(httpie_stderr): """Run HTTPie from within this process.""" @@ -89,17 +87,17 @@ def main(args): # Imports of HTTPie internals must be local because otherwise they # won't take into account patched HTTPIE_CONFIG_DIR environment # variable. - import httpie.core import httpie.context + import httpie.core - args = ["http", "--ignore-stdin"] + args + args = ["http", "--ignore-stdin", *args] env = httpie.context.Environment(stderr=httpie_stderr) return httpie.core.main(args, env=env) return main -@pytest.fixture(scope="function", params=["credential-store", "creds"]) +@pytest.fixture(params=["credential-store", "creds"]) def creds_auth_type(request): """All possible aliases.""" @@ -133,7 +131,7 @@ def test_creds_auth_deactivated_by_default(httpie_run): @responses.activate -def test_creds_auth_basic(httpie_run, set_credentials, creds_auth_type, httpie_stderr): +def test_creds_auth_basic(httpie_run, set_credentials, creds_auth_type): """The plugin works for HTTP basic auth.""" set_credentials( @@ -158,9 +156,7 @@ def test_creds_auth_basic(httpie_run, set_credentials, creds_auth_type, httpie_s @responses.activate -def test_creds_auth_basic_keychain( - httpie_run, set_credentials, creds_auth_type, tmpdir, httpie_stderr -): +def test_creds_auth_basic_keychain(httpie_run, set_credentials, creds_auth_type, tmpdir): """The plugin retrieves secrets from keychain for HTTP basic auth.""" secrettxt = tmpdir.join("secret.txt") @@ -199,14 +195,11 @@ def test_creds_auth_digest(httpie_run, set_credentials, creds_auth_type): "http://example.com", status=401, headers={ - "WWW-Authenticate": "Digest " - + ",".join( - [ - "realm=auth.example.com", - 'qop="auth,auth-int"', - "nonce=dcd98b7102dd2f0e8b11d0f600bfb0c093", - "opaque=5ccc069c403ebaf9f0171e9517f40e41", - ] + "WWW-Authenticate": ( + "Digest realm=auth.example.com" + ',qop="auth,auth-int"' + ",nonce=dcd98b7102dd2f0e8b11d0f600bfb0c093" + ",opaque=5ccc069c403ebaf9f0171e9517f40e41" ) }, ) @@ -510,7 +503,7 @@ def test_creds_auth_multiple_token_header_keychain( @responses.activate @pytest.mark.parametrize( - ["auth", "error_pattern"], + ("auth", "error_pattern"), [ pytest.param( {"provider": "basic"}, @@ -594,7 +587,7 @@ def test_creds_auth_missing( @responses.activate @pytest.mark.parametrize( - ["regexp", "url", "normalized_url"], + ("regexp", "url", "normalized_url"), [ pytest.param( r"http://example.com/", @@ -745,7 +738,7 @@ def test_creds_lookup_many_credentials(httpie_run, set_credentials, creds_auth_t @responses.activate @pytest.mark.parametrize( - ["regexp", "url"], + ("regexp", "url"), [ pytest.param(r"http://example.com/", "https://example.com/", id="http-https"), pytest.param(r"https://example.com", "http://example.com/", id="https-http"), @@ -840,7 +833,7 @@ def test_creds_lookup_by_id_error(httpie_run, set_credentials, httpie_stderr, cr @responses.activate @pytest.mark.skipif(_is_windows, reason="no support for permissions on windows") @pytest.mark.parametrize( - ["mode"], + "mode", [ pytest.param(0o700, id="0700"), pytest.param(0o600, id="0600"), @@ -876,7 +869,7 @@ def test_creds_permissions_safe(httpie_run, set_credentials, mode, creds_auth_ty @responses.activate @pytest.mark.skipif(_is_windows, reason="no support for permissions on windows") @pytest.mark.parametrize( - ["mode"], + "mode", [ pytest.param(0o607, id="0607"), pytest.param(0o606, id="0606"), @@ -917,7 +910,7 @@ def test_creds_permissions_unsafe( @responses.activate @pytest.mark.skipif(_is_windows, reason="no support for permissions on windows") @pytest.mark.parametrize( - ["mode"], + "mode", [ pytest.param(0o300, id="0300"), pytest.param(0o200, id="0200"), @@ -959,7 +952,7 @@ def test_creds_auth_no_database(httpie_run, credentials_file, httpie_stderr, cre @responses.activate @pytest.mark.parametrize( - ["auth", "error"], + ("auth", "error"), [ pytest.param( {"provider": "header", "name": "X-Auth", "value": "p@ss\n"}, @@ -996,7 +989,7 @@ def test_creds_auth_header_value_illegal_characters( @responses.activate @pytest.mark.parametrize( - ["auth", "error"], + ("auth", "error"), [ pytest.param( {"provider": "header", "name": "X-Auth\n", "value": "p@ss"}, From 0eff03f0751d09a0c0785c51957983d4a4273be8 Mon Sep 17 00:00:00 2001 From: Ihor Kalnytskyi Date: Mon, 6 May 2024 18:26:23 +0300 Subject: [PATCH 03/12] Skip password-store tests if binary is not found Some platforms, such as Windows, may not have password-store available for them, and some systems, such as my home machine, doesn't have it permanently installed. Instead of failing a test run when password-store is not available, it's better to skip those tests. --- tests/test_keychain_password_store.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/tests/test_keychain_password_store.py b/tests/test_keychain_password_store.py index e862545..22f5080 100644 --- a/tests/test_keychain_password_store.py +++ b/tests/test_keychain_password_store.py @@ -2,6 +2,7 @@ import os import re +import shutil import subprocess import sys import tempfile @@ -10,16 +11,12 @@ import py import pytest -_is_windows = sys.platform == "win32" _is_macos = sys.platform == "darwin" -# On Windows, password-store is only supported through Cygwin. There's no much -# sense even to try make these tests green on Windows because I doubt there -# will ever be password-store users on that operating system. pytestmark = pytest.mark.skipif( - _is_windows, - reason="password-store is not supported on windows", + not shutil.which("pass"), + reason="password-store is not found", ) From b369ee2a033706a324d0b5c4a3b74df8dbf27419 Mon Sep 17 00:00:00 2001 From: Ihor Kalnytskyi Date: Mon, 6 May 2024 19:10:41 +0300 Subject: [PATCH 04/12] Use tmp_path instead of tmpdir in tests The `tmpdir` fixture is deprecated because it returns the legacy path type. It's not recommended to use `tmp_path` as it returns the path type from the standard library. --- tests/conftest.py | 16 ++++++-------- tests/test_keychain_password_store.py | 31 +++++++++++---------------- tests/test_keychain_shell.py | 20 ++++++++--------- tests/test_plugin.py | 29 ++++++++++++------------- 4 files changed, 43 insertions(+), 53 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 0deca9e..bb63428 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,12 +1,8 @@ -import os -import tempfile -import unittest.mock as mock - import pytest @pytest.fixture(scope="session", autouse=True) -def _httpie_config_dir(): +def _httpie_config_dir(tmp_path_factory: pytest.TempPathFactory): """Set path to HTTPie configuration directory.""" # HTTPie can optionally read a path to configuration directory from @@ -14,11 +10,11 @@ def _httpie_config_dir(): # configuration, HTTPIE_CONFIG_DIR environment variable is patched to point # to a temporary directory instead. But here's the thing, HTTPie is not ran # in subprocess in these tests, and so the environment variable is read - # only once on first package import. That's why it must be set before + # just once on first package import. That's why it must be set before # HTTPie package is imported and that's why the very same value must be # used for all tests (session scope). Otherwise, tests may fail because # they will look for credentials file in different directory. - with tempfile.TemporaryDirectory() as tmpdir, mock.patch.dict( - os.environ, {"HTTPIE_CONFIG_DIR": tmpdir} - ): - yield tmpdir + with pytest.MonkeyPatch.context() as monkeypatch: + tmp_path = tmp_path_factory.mktemp(".httpie") + monkeypatch.setenv("HTTPIE_CONFIG_DIR", str(tmp_path)) + yield tmp_path diff --git a/tests/test_keychain_password_store.py b/tests/test_keychain_password_store.py index 22f5080..fb8d80c 100644 --- a/tests/test_keychain_password_store.py +++ b/tests/test_keychain_password_store.py @@ -1,6 +1,6 @@ """Tests password-store keychain provider.""" -import os +import pathlib import re import shutil import subprocess @@ -8,7 +8,6 @@ import tempfile import textwrap -import py import pytest _is_macos = sys.platform == "darwin" @@ -25,23 +24,23 @@ # temporary directory and generate-key template pointed to a file in # temporary directory too, it complains about using too long names. It's # not clear why 'gpg' complains about too long names, but it's clear that - # built-in 'tmpdir' fixture produces too long names. That's why on macOS we - # override 'tmpdir' fixture to return much shorter path to a temporary + # built-in 'tmp_path' fixture produces too long names. That's why on macOS we + # override 'tmp_path' fixture to return much shorter path to a temporary # directory. @pytest.fixture() - def tmpdir(): + def tmp_path(): with tempfile.TemporaryDirectory() as path: - yield py.path.local(path) + yield pathlib.Path(path) @pytest.fixture() -def gpg_key_id(monkeypatch, tmpdir): +def gpg_key_id(monkeypatch, tmp_path): """Return a Key ID of just generated GPG key.""" - gpghome = tmpdir.join(".gnupg") - gpgtemplate = tmpdir.join("gpg-template") + gpghome = tmp_path.joinpath(".gnupg") + gpgtemplate = tmp_path.joinpath("gpg-template") - monkeypatch.setitem(os.environ, "GNUPGHOME", gpghome.strpath) + monkeypatch.setenv("GNUPGHOME", str(gpghome)) gpgtemplate.write_text( textwrap.dedent( """ @@ -74,16 +73,12 @@ def gpg_key_id(monkeypatch, tmpdir): @pytest.fixture(autouse=True) -def password_store_dir(monkeypatch, tmpdir): +def password_store_dir(monkeypatch, tmp_path): """Set password-store home directory to a temporary one.""" - passstore = tmpdir.join(".password-store") - monkeypatch.setitem( - os.environ, - "PASSWORD_STORE_DIR", - passstore.strpath, - ) - return passstore.strpath + passstore = tmp_path.joinpath(".password-store") + monkeypatch.setenv("PASSWORD_STORE_DIR", str(passstore)) + return passstore @pytest.fixture() diff --git a/tests/test_keychain_shell.py b/tests/test_keychain_shell.py index 57517c7..bef4eeb 100644 --- a/tests/test_keychain_shell.py +++ b/tests/test_keychain_shell.py @@ -17,34 +17,34 @@ def testkeychain(): return _keychain.ShellKeychain() -def test_secret_retrieved(testkeychain, tmpdir): +def test_secret_retrieved(testkeychain, tmp_path): """The keychain returns stored secret, no bullshit.""" - secrettxt = tmpdir.join("secret.txt") + secrettxt = tmp_path.joinpath("secret.txt") secrettxt.write_text("p@ss", encoding="UTF-8") - assert testkeychain.get(command=f"cat {secrettxt.strpath}") == "p@ss" + assert testkeychain.get(command=f"cat {secrettxt}") == "p@ss" -def test_secret_retrieved_pipe(testkeychain, tmpdir): +def test_secret_retrieved_pipe(testkeychain, tmp_path): """The keychain returns stored secret even when pipes are used.""" - secrettxt = tmpdir.join("secret.txt") + secrettxt = tmp_path.joinpath("secret.txt") secrettxt.write_text("p@ss\nextra", encoding="UTF-8") - command = rf"cat {secrettxt.strpath} | head -n 1 | tr -d {os.linesep!r}" + command = rf"cat {secrettxt} | head -n 1 | tr -d {os.linesep!r}" assert testkeychain.get(command=command) == "p@ss" -def test_secret_not_found(testkeychain, tmpdir): +def test_secret_not_found(testkeychain, tmp_path): """LookupError is raised when no secrets are found in the keychain.""" - secrettxt = tmpdir.join("secret.txt") + secrettxt = tmp_path.joinpath("secret.txt") with pytest.raises(LookupError) as excinfo: - testkeychain.get(command=f"cat {secrettxt.strpath}") + testkeychain.get(command=f"cat {secrettxt}") assert str(excinfo.value) == ( - f"No secret found: Command 'cat {secrettxt.strpath}' returned non-zero exit status 1." + f"No secret found: Command 'cat {secrettxt}' returned non-zero exit status 1." ) diff --git a/tests/test_plugin.py b/tests/test_plugin.py index 6b0cf52..f27ff91 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -3,7 +3,6 @@ import io import json import os -import pathlib import re import sys from urllib.request import parse_http_list, parse_keqv_list @@ -49,7 +48,7 @@ def httpie_config_dir(_httpie_config_dir): # Since we cannot use new directory for HTTPie configuration for every test # (see reasons in `_httpie_config_dir` fixture), we must at least ensure # that there's no side effect between tests by emptying the directory. - for path in pathlib.Path(_httpie_config_dir).iterdir(): + for path in _httpie_config_dir.iterdir(): path.unlink() @@ -156,10 +155,10 @@ def test_creds_auth_basic(httpie_run, set_credentials, creds_auth_type): @responses.activate -def test_creds_auth_basic_keychain(httpie_run, set_credentials, creds_auth_type, tmpdir): +def test_creds_auth_basic_keychain(httpie_run, set_credentials, creds_auth_type, tmp_path): """The plugin retrieves secrets from keychain for HTTP basic auth.""" - secrettxt = tmpdir.join("secret.txt") + secrettxt = tmp_path.joinpath("secret.txt") secrettxt.write_text("p@ss", encoding="UTF-8") set_credentials( @@ -171,7 +170,7 @@ def test_creds_auth_basic_keychain(httpie_run, set_credentials, creds_auth_type, "username": "user", "password": { "keychain": "shell", - "command": f"cat {secrettxt.strpath}", + "command": f"cat {secrettxt}", }, }, } @@ -296,10 +295,10 @@ def test_creds_auth_token_scheme(httpie_run, set_credentials, creds_auth_type): @responses.activate -def test_creds_auth_token_keychain(httpie_run, set_credentials, creds_auth_type, tmpdir): +def test_creds_auth_token_keychain(httpie_run, set_credentials, creds_auth_type, tmp_path): """The plugin retrieves secrets from keychain for HTTP token auth.""" - secrettxt = tmpdir.join("secret.txt") + secrettxt = tmp_path.joinpath("secret.txt") secrettxt.write_text("token-can-be-anything", encoding="UTF-8") set_credentials( @@ -310,7 +309,7 @@ def test_creds_auth_token_keychain(httpie_run, set_credentials, creds_auth_type, "provider": "token", "token": { "keychain": "shell", - "command": f"cat {secrettxt.strpath}", + "command": f"cat {secrettxt}", }, }, } @@ -351,10 +350,10 @@ def test_creds_auth_header(httpie_run, set_credentials, creds_auth_type): @responses.activate -def test_creds_auth_header_keychain(httpie_run, set_credentials, creds_auth_type, tmpdir): +def test_creds_auth_header_keychain(httpie_run, set_credentials, creds_auth_type, tmp_path): """The plugin retrieves secrets from keychain for HTTP header auth.""" - secrettxt = tmpdir.join("secret.txt") + secrettxt = tmp_path.joinpath("secret.txt") secrettxt.write_text("value-can-be-anything", encoding="UTF-8") set_credentials( @@ -366,7 +365,7 @@ def test_creds_auth_header_keychain(httpie_run, set_credentials, creds_auth_type "name": "X-Auth", "value": { "keychain": "shell", - "command": f"cat {secrettxt.strpath}", + "command": f"cat {secrettxt}", }, }, } @@ -455,11 +454,11 @@ def test_creds_auth_multiple_header_header(httpie_run, set_credentials, creds_au @responses.activate def test_creds_auth_multiple_token_header_keychain( - httpie_run, set_credentials, creds_auth_type, tmpdir + httpie_run, set_credentials, creds_auth_type, tmp_path ): """The plugin retrieves secrets from keychains for combination of auths.""" - tokentxt, secrettxt = tmpdir.join("token.txt"), tmpdir.join("secret.txt") + tokentxt, secrettxt = tmp_path.joinpath("token.txt"), tmp_path.joinpath("secret.txt") tokentxt.write_text("token-can-be-anything", encoding="UTF-8") secrettxt.write_text("secret-can-be-anything", encoding="UTF-8") @@ -474,7 +473,7 @@ def test_creds_auth_multiple_token_header_keychain( "provider": "token", "token": { "keychain": "shell", - "command": f"cat {tokentxt.strpath}", + "command": f"cat {tokentxt}", }, "scheme": "JWT", }, @@ -483,7 +482,7 @@ def test_creds_auth_multiple_token_header_keychain( "name": "X-Auth", "value": { "keychain": "shell", - "command": f"cat {secrettxt.strpath}", + "command": f"cat {secrettxt}", }, }, ], From 2a13a7b459eb3b5d28048684655835a085b0da95 Mon Sep 17 00:00:00 2001 From: Ihor Kalnytskyi Date: Tue, 7 May 2024 13:06:05 +0300 Subject: [PATCH 05/12] Use 2 empty lines after the import block This is my personal preference I wasn't sure how to deal with ever since I migrated to Ruff. Apparently, Ruff provides configuration options to achieve what I want. --- pyproject.toml | 2 ++ src/httpie_credential_store/__init__.py | 1 + src/httpie_credential_store/_auth.py | 1 + tests/test_keychain_password_store.py | 1 + tests/test_plugin.py | 2 ++ 5 files changed, 7 insertions(+) diff --git a/pyproject.toml b/pyproject.toml index 1016978..831c2dd 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -39,3 +39,5 @@ ignore = ["ANN", "D", "TRY", "S", "PTH", "PLR", "COM", "PT005", "ISC001", "INP00 [tool.ruff.lint.isort] known-first-party = ["httpie_credential_store"] +lines-after-imports = 2 +lines-between-types = 1 diff --git a/src/httpie_credential_store/__init__.py b/src/httpie_credential_store/__init__.py index 3549e96..eb934b8 100644 --- a/src/httpie_credential_store/__init__.py +++ b/src/httpie_credential_store/__init__.py @@ -2,4 +2,5 @@ from ._plugin import CredentialStoreAuthPlugin, CredsAuthPlugin + __all__ = ["CredentialStoreAuthPlugin", "CredsAuthPlugin"] diff --git a/src/httpie_credential_store/_auth.py b/src/httpie_credential_store/_auth.py index 2fea0f9..a9e1b12 100644 --- a/src/httpie_credential_store/_auth.py +++ b/src/httpie_credential_store/_auth.py @@ -8,6 +8,7 @@ from ._keychain import get_keychain + # These patterns are copied over from built-in `http.client` implementation, # and are more lenient than RFC definitions for backwards compatibility # reasons. diff --git a/tests/test_keychain_password_store.py b/tests/test_keychain_password_store.py index fb8d80c..9e29035 100644 --- a/tests/test_keychain_password_store.py +++ b/tests/test_keychain_password_store.py @@ -10,6 +10,7 @@ import pytest + _is_macos = sys.platform == "darwin" diff --git a/tests/test_plugin.py b/tests/test_plugin.py index f27ff91..4e914d8 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -5,11 +5,13 @@ import os import re import sys + from urllib.request import parse_http_list, parse_keqv_list import pytest import responses + _is_windows = sys.platform == "win32" From cf0a583fe3ab81462c4f6e188d13f8628eded573 Mon Sep 17 00:00:00 2001 From: Ihor Kalnytskyi Date: Tue, 7 May 2024 13:24:32 +0300 Subject: [PATCH 06/12] Decrease the size of the testing matrix This plugin claims to support 5 Python versions: 3.8 to 3.12, and 3 operating systems: Linux, macOS and Windows. By testing all possible combinations, it requires running 15 test jobs in parallel. Unfortunately, Windows & macOS runners aren't fast to be found, and sometimes one has to wait a significant amount of time for a pull request to become "green". This is wasteful for me, as a sole contributor, and GitHub, as a company that provides compute resources for free. In practice there's almost no benefits of testing all python versions on Windows and macOS platforms as long as we test them elsewhere (Linux). In most cases, either the python version is not supported regardless of the platform it runs on, or the platform is not supported regardless of the python version you run on it. This patch decreases the testing matrix size by running tests on Windows and macOS runners just once, using the latest available python version. --- .github/workflows/ci.yml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5f90212..8e78093 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -22,8 +22,13 @@ jobs: test: strategy: matrix: - os: [ubuntu-latest, macos-latest, windows-latest] + os: [ubuntu-latest] python-version: ["3.8", "3.9", "3.10", "3.11", "3.12"] + include: + - os: macos-latest + python-version: "3.12" + - os: windows-latest + python-version: "3.12" runs-on: ${{ matrix.os }} steps: From 6c94556f9701c01254cfb7e6bfec8aaf1f85f3f3 Mon Sep 17 00:00:00 2001 From: Ihor Kalnytskyi Date: Tue, 7 May 2024 14:17:55 +0300 Subject: [PATCH 07/12] Modernize tox-poetry integration I have never been a fan of poetry, but since this project is already using it, let's keep it this way. What we have to do though is to make sure we use the up-to-date poetry configuration and best practices. This patch moves dependency definitions out from tox.ini to pyproject.toml (poetry groups). This should help us to maintain dependencies in one place only. --- .github/workflows/ci.yml | 2 +- pyproject.toml | 19 ++++++++++++++----- tox.ini | 18 +++++++++--------- 3 files changed, 24 insertions(+), 15 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8e78093..ed50284 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -49,4 +49,4 @@ jobs: if: matrix.os == 'macos-latest' - name: Run pytest - run: pipx run tox -e py3 + run: pipx run tox -e test diff --git a/pyproject.toml b/pyproject.toml index 831c2dd..a84dcfc 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,3 +1,7 @@ +[build-system] +requires = ["poetry-core"] +build-backend = "poetry.core.masonry.api" + [tool.poetry] name = "httpie-credential-store" version = "3.1.0" @@ -14,7 +18,16 @@ python = "^3.8" httpie = "^3.1" keyring = ">= 23.5" -[tool.poetry.dev-dependencies] +[tool.poetry.group.lint] +optional = true + +[tool.poetry.group.lint.dependencies] +ruff = "^0.4.2" + +[tool.poetry.group.test] +optional = true + +[tool.poetry.group.test.dependencies] pytest = "^7.1" responses = "^0.20" @@ -22,10 +35,6 @@ responses = "^0.20" credential-store = "httpie_credential_store:CredentialStoreAuthPlugin" creds = "httpie_credential_store:CredsAuthPlugin" -[build-system] -requires = ["poetry>=0.12"] -build-backend = "poetry.masonry.api" - [tool.ruff] line-length = 100 target-version = "py38" diff --git a/tox.ini b/tox.ini index 342314f..88931be 100644 --- a/tox.ini +++ b/tox.ini @@ -1,17 +1,17 @@ [tox] -envlist = py3, lint +envlist = lint, test [testenv] +allowlist_externals = poetry deps = poetry skip_install = true -commands = - poetry install -v - poetry run pytest -vv [testenv:lint] -skip_install = true -deps = - ruff >= 0.4.2, < 0.5.0 +commands_pre = poetry install --only lint commands = - ruff check {args:.} - ruff format --check --diff {args:.} + ruff check {posargs:.} + ruff format --check --diff {posargs:.} + +[testenv:test] +commands_pre = poetry install --with test +commands = poetry run pytest -vv {posargs:.} From 29f118b99de5cccef368b7910d60bb11c56884db Mon Sep 17 00:00:00 2001 From: Ihor Kalnytskyi Date: Tue, 7 May 2024 14:31:22 +0300 Subject: [PATCH 08/12] Bump used gh actions There are 16 deprecation warnings generated by gh actions today. We better bump used versions to make sure things won't get broken unexpectedly. --- .github/workflows/cd.yml | 6 ++++-- .github/workflows/ci.yml | 10 ++++++---- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/.github/workflows/cd.yml b/.github/workflows/cd.yml index 63d0d5c..d09ac23 100644 --- a/.github/workflows/cd.yml +++ b/.github/workflows/cd.yml @@ -10,10 +10,12 @@ jobs: runs-on: ubuntu-latest steps: - name: Set up sources - uses: actions/checkout@v2 + uses: actions/checkout@v4 - name: Set up Python - uses: actions/setup-python@v2 + uses: actions/setup-python@v5 + with: + python-version: "3.12" - name: Publish to PyPI env: diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ed50284..bd8ae76 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -11,10 +11,12 @@ jobs: runs-on: ubuntu-latest steps: - name: Set up sources - uses: actions/checkout@v2 + uses: actions/checkout@v4 - name: Set up Python - uses: actions/setup-python@v2 + uses: actions/setup-python@v5 + with: + python-version: "3.12" - name: Run ruff run: pipx run tox -e lint @@ -33,10 +35,10 @@ jobs: runs-on: ${{ matrix.os }} steps: - name: Set up sources - uses: actions/checkout@v2 + uses: actions/checkout@v4 - name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@v2 + uses: actions/setup-python@v5 with: python-version: ${{ matrix.python-version }} From 4650c324d81232ee6d330bba1718a021e400670e Mon Sep 17 00:00:00 2001 From: Ihor Kalnytskyi Date: Tue, 7 May 2024 14:50:24 +0300 Subject: [PATCH 09/12] Show pytest/ruff errors in actions output system The GitHub Actions CI/CD can show errors in pull request per line. This is much better than reading stdout/stderr outputs. This patch adds a pytest plugin that can generate gh actions reports, and opts Ruff into github actions output when ran on CI. --- .github/workflows/ci.yml | 2 ++ pyproject.toml | 1 + tox.ini | 2 ++ 3 files changed, 5 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bd8ae76..55cf049 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -20,6 +20,8 @@ jobs: - name: Run ruff run: pipx run tox -e lint + env: + RUFF_OUTPUT_FORMAT: github test: strategy: diff --git a/pyproject.toml b/pyproject.toml index a84dcfc..04195c3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -30,6 +30,7 @@ optional = true [tool.poetry.group.test.dependencies] pytest = "^7.1" responses = "^0.20" +pytest-github-actions-annotate-failures = "*" [tool.poetry.plugins."httpie.plugins.auth.v1"] credential-store = "httpie_credential_store:CredentialStoreAuthPlugin" diff --git a/tox.ini b/tox.ini index 88931be..65d1741 100644 --- a/tox.ini +++ b/tox.ini @@ -11,7 +11,9 @@ commands_pre = poetry install --only lint commands = ruff check {posargs:.} ruff format --check --diff {posargs:.} +passenv = RUFF_OUTPUT_FORMAT [testenv:test] commands_pre = poetry install --with test commands = poetry run pytest -vv {posargs:.} +passenv = GITHUB_ACTIONS From 9a7820505e57d143754bd7c3e5383e8615cca06b Mon Sep 17 00:00:00 2001 From: Ihor Kalnytskyi Date: Tue, 7 May 2024 15:33:25 +0300 Subject: [PATCH 10/12] password-store: check returned secret in the test The test we have today doesn't check retrieved secret except for its length. Even though in most cases testing the length most likely will be enough, it's not robust enough. This patch makes the test better by creating a known secret in password-store, retrieving it and then testing that the secret is expected. --- tests/test_keychain_password_store.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/tests/test_keychain_password_store.py b/tests/test_keychain_password_store.py index 9e29035..83a53d7 100644 --- a/tests/test_keychain_password_store.py +++ b/tests/test_keychain_password_store.py @@ -97,17 +97,16 @@ def testkeychain(): def test_secret_retrieved(testkeychain, gpg_key_id): """The keychain returns stored secret, no bullshit.""" - subprocess.run(f"pass init {gpg_key_id}", shell=True, check=False) - subprocess.run("pass generate testservice/testuser 14", shell=True, check=False) + subprocess.run(["pass", "init", gpg_key_id], check=True) + subprocess.run(["pass", "insert", "--echo", "service/user"], input=b"f00b@r", check=True) - secret = testkeychain.get(name="testservice/testuser") - assert len(secret) == 14 + assert testkeychain.get(name="service/user") == "f00b@r" def test_secret_not_found(testkeychain): """LookupError is raised when no secrets are found in the keychain.""" with pytest.raises(LookupError) as excinfo: - testkeychain.get(name="testservice/testuser") + testkeychain.get(name="service/user") - assert str(excinfo.value) == "password-store: no secret found: 'testservice/testuser'" + assert str(excinfo.value) == "password-store: no secret found: 'service/user'" From 3a694f7f35a9fd92bf39aba4f6295a8b793d7fec Mon Sep 17 00:00:00 2001 From: Ihor Kalnytskyi Date: Tue, 7 May 2024 17:46:37 +0300 Subject: [PATCH 11/12] Turn on flake8-bandit lint rules Almost all of them are checked now, with few exceptions that tend to return false positives. --- pyproject.toml | 5 +++-- tests/test_keychain_password_store.py | 12 +++--------- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 04195c3..a73ecf6 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -42,10 +42,11 @@ target-version = "py38" [tool.ruff.lint] select = ["ALL"] -ignore = ["ANN", "D", "TRY", "S", "PTH", "PLR", "COM", "PT005", "ISC001", "INP001"] +ignore = ["ANN", "D", "PTH", "PLR", "PT005", "ISC001", "INP001", "S603", "S607", "COM812"] [tool.ruff.lint.per-file-ignores] -"tests/*" = ["INP001"] +"src/httpie_credential_store/_keychain.py" = ["S602"] +"tests/*" = ["S101", "INP001"] [tool.ruff.lint.isort] known-first-party = ["httpie_credential_store"] diff --git a/tests/test_keychain_password_store.py b/tests/test_keychain_password_store.py index 83a53d7..195a181 100644 --- a/tests/test_keychain_password_store.py +++ b/tests/test_keychain_password_store.py @@ -57,14 +57,8 @@ def gpg_key_id(monkeypatch, tmp_path): encoding="UTF-8", ) - subprocess.check_output( - f"gpg --batch --generate-key {gpgtemplate}", - shell=True, - stderr=subprocess.STDOUT, - ) - keys = subprocess.check_output( - "gpg --list-secret-keys", shell=True, stderr=subprocess.STDOUT - ).decode("UTF-8") + subprocess.check_call(["gpg", "--batch", "--generate-key", gpgtemplate]) + keys = subprocess.check_output(["gpg", "--list-secret-keys"], text=True) key = re.search(r"\s+([0-9A-F]{40})\s+", keys) if not key: @@ -97,7 +91,7 @@ def testkeychain(): def test_secret_retrieved(testkeychain, gpg_key_id): """The keychain returns stored secret, no bullshit.""" - subprocess.run(["pass", "init", gpg_key_id], check=True) + subprocess.check_call(["pass", "init", gpg_key_id]) subprocess.run(["pass", "insert", "--echo", "service/user"], input=b"f00b@r", check=True) assert testkeychain.get(name="service/user") == "f00b@r" From 2259ce9b9f2dec9d58bad156e973f87dc9cf2b09 Mon Sep 17 00:00:00 2001 From: Ihor Kalnytskyi Date: Tue, 7 May 2024 18:04:57 +0300 Subject: [PATCH 12/12] password-store: retrieve secret w/o shell Do not use shell to retrieve a secret from password-store because (a) it's less secure, and (b) it's one extra executable invocation. --- src/httpie_credential_store/_keychain.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/httpie_credential_store/_keychain.py b/src/httpie_credential_store/_keychain.py index 1c35dbc..9c8a5f6 100644 --- a/src/httpie_credential_store/_keychain.py +++ b/src/httpie_credential_store/_keychain.py @@ -26,7 +26,7 @@ class ShellKeychain(KeychainProvider): def get(self, *, command): try: - return subprocess.check_output(command, shell=True).decode("UTF-8") + return subprocess.check_output(command, shell=True, text=True) except subprocess.CalledProcessError as exc: error_message = f"No secret found: {exc}" raise LookupError(error_message) from exc @@ -41,9 +41,9 @@ def get(self, *, name): try: # password-store may store securely extra information along with a # password. Nevertheless, a password is always a first line. - text = super().get(command=f"pass {name}") + text = subprocess.check_output(["pass", name], text=True) return text.splitlines()[0] - except LookupError as exc: + except subprocess.CalledProcessError as exc: error_message = f"password-store: no secret found: '{name}'" raise LookupError(error_message) from exc