From bd2a3725613d8cef351a78ffd3430c34c9fdb6be Mon Sep 17 00:00:00 2001 From: Federico Fissore Date: Thu, 19 Sep 2024 13:21:09 +0200 Subject: [PATCH 1/5] Not accessing `credentials.username` if `credentials` is an instance of `AnonymousCredential` See #694 and #692 --- keyring/cli.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/keyring/cli.py b/keyring/cli.py index 8b3d712c..0b2a0f9f 100644 --- a/keyring/cli.py +++ b/keyring/cli.py @@ -132,12 +132,18 @@ def do_get(self): getattr(self, f'_emit_{self.output_format}')(credential) def _emit_json(self, credential: credentials.Credential): + if isinstance(credential, credentials.AnonymousCredential): + print(json.dumps(dict(password=credential.password))) + return print( json.dumps(dict(username=credential.username, password=credential.password)) ) def _emit_plain(self, credential: credentials.Credential): - if credential.username: + if ( + not isinstance(credential, credentials.AnonymousCredential) + and credential.username + ): print(credential.username) print(credential.password) From 41892e7d7e941b29a9492b616ada39f08d398c1c Mon Sep 17 00:00:00 2001 From: "Jason R. Coombs" Date: Fri, 20 Sep 2024 12:19:27 -0400 Subject: [PATCH 2/5] Add test for get_credential with no username. Captures missed expectation reported in #694 --- tests/test_cli.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/test_cli.py b/tests/test_cli.py index 435c55a3..73901685 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -6,6 +6,7 @@ import pytest from keyring import cli +from keyring import credentials flatten = itertools.chain.from_iterable @@ -36,6 +37,12 @@ def mocked_set(): yield set_password +@pytest.fixture +def mocked_get_credential(): + with mock.patch('keyring.cli.get_credential') as get_credential: + yield get_credential + + def test_set_interactive(monkeypatch, mocked_set): tool = cli.CommandLineTool() tool.service = 'svc' @@ -64,3 +71,16 @@ def test_set_pipe_newline(monkeypatch, mocked_set): monkeypatch.setattr(sys.stdin, 'read', lambda: 'foo123\n') tool.do_set() mocked_set.assert_called_once_with('svc', 'usr', 'foo123') + + +@pytest.mark.xfail(reason="#694") +@pytest.mark.parametrize('format', ['json', 'plain']) +def test_get_anonymous(monkeypatch, mocked_get_credential, format, capsys): + mocked_get_credential.return_value = credentials.AnonymousCredential('s3cret') + tool = cli.CommandLineTool() + tool.service = 'svc' + tool.username = None + tool.get_mode = 'creds' + tool.output_format = format + tool.do_get() + assert 's3cret' in capsys.readouterr().out From ef694691cc42fb87d47cfd1a5c03bbc5010ce35f Mon Sep 17 00:00:00 2001 From: "Jason R. Coombs" Date: Fri, 20 Sep 2024 12:33:28 -0400 Subject: [PATCH 3/5] Move class-specific behaviors into the classes. --- keyring/cli.py | 15 +++------------ keyring/credentials.py | 6 ++++++ 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/keyring/cli.py b/keyring/cli.py index 0b2a0f9f..2c0ba4d3 100644 --- a/keyring/cli.py +++ b/keyring/cli.py @@ -132,20 +132,11 @@ def do_get(self): getattr(self, f'_emit_{self.output_format}')(credential) def _emit_json(self, credential: credentials.Credential): - if isinstance(credential, credentials.AnonymousCredential): - print(json.dumps(dict(password=credential.password))) - return - print( - json.dumps(dict(username=credential.username, password=credential.password)) - ) + print(json.dumps(credential._vars())) def _emit_plain(self, credential: credentials.Credential): - if ( - not isinstance(credential, credentials.AnonymousCredential) - and credential.username - ): - print(credential.username) - print(credential.password) + for val in credential._vars().values(): + print(val) def _get_creds(self) -> credentials.Credential | None: return get_credential(self.service, self.username) diff --git a/keyring/credentials.py b/keyring/credentials.py index 4b28c6c7..6a2cecd8 100644 --- a/keyring/credentials.py +++ b/keyring/credentials.py @@ -13,6 +13,9 @@ def username(self) -> str: ... @abc.abstractproperty def password(self) -> str: ... + def _vars(self) -> dict[str, str]: + return dict(username=self.username, password=self.password) + class SimpleCredential(Credential): """Simple credentials implementation""" @@ -38,6 +41,9 @@ def __init__(self, password: str): def username(self) -> str: raise ValueError("Anonymous credential has no username") + def _vars(self) -> dict[str, str]: + return dict(password=self.password) + class EnvironCredential(Credential): """ From b75609e067a8f2c25c1ff75ca797a5d2a767607a Mon Sep 17 00:00:00 2001 From: "Jason R. Coombs" Date: Fri, 20 Sep 2024 12:34:26 -0400 Subject: [PATCH 4/5] Add news fragment. --- newsfragments/694.bugfix.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 newsfragments/694.bugfix.rst diff --git a/newsfragments/694.bugfix.rst b/newsfragments/694.bugfix.rst new file mode 100644 index 00000000..be4c2559 --- /dev/null +++ b/newsfragments/694.bugfix.rst @@ -0,0 +1 @@ +Fixed ValueError for AnonymousCredentials in CLI. \ No newline at end of file From e9f785146526949edec2ecba15edba1aa82baaca Mon Sep 17 00:00:00 2001 From: "Jason R. Coombs" Date: Fri, 20 Sep 2024 12:38:53 -0400 Subject: [PATCH 5/5] Add another test to cover Credential._vars. --- tests/test_cli.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/test_cli.py b/tests/test_cli.py index de400ee7..87c3a2fc 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -83,3 +83,15 @@ def test_get_anonymous(monkeypatch, mocked_get_credential, format, capsys): tool.output_format = format tool.do_get() assert 's3cret' in capsys.readouterr().out + + +@pytest.mark.parametrize('format', ['json', 'plain']) +def test_get(monkeypatch, mocked_get_credential, format, capsys): + mocked_get_credential.return_value = credentials.SimpleCredential('alice', 's3cret') + tool = cli.CommandLineTool() + tool.service = 'svc' + tool.username = 'alice' + tool.get_mode = 'creds' + tool.output_format = format + tool.do_get() + assert 's3cret' in capsys.readouterr().out