From 98bdfa74cfddf8d7191daf4633e4f9bf1527c38a Mon Sep 17 00:00:00 2001 From: Michael Hanke Date: Wed, 22 Feb 2023 16:42:05 +0100 Subject: [PATCH] New `memory_keyring` fixture This fixture provides an isolated keyring in-memory-only backend for tests that needs to store/retrieve credentials. The fixture is function-scope, hence the backend is destroyed at the end of each test. Closes https://github.com/datalad/datalad/issues/7295 --- .../commands/tests/test_credentials.py | 32 +++++------ datalad_next/conftest.py | 47 +++++++++++++++ datalad_next/credman/tests/test_credman.py | 57 +++++++------------ datalad_next/tests/utils.py | 1 - 4 files changed, 81 insertions(+), 56 deletions(-) diff --git a/datalad_next/commands/tests/test_credentials.py b/datalad_next/commands/tests/test_credentials.py index c49b7ae3..2a781cf1 100644 --- a/datalad_next/commands/tests/test_credentials.py +++ b/datalad_next/commands/tests/test_credentials.py @@ -19,7 +19,6 @@ from datalad_next.exceptions import IncompleteResultsError from datalad_next.utils import external_versions from datalad_next.tests.utils import ( - MemoryKeyring, assert_in, assert_in_results, assert_raises, @@ -74,12 +73,11 @@ def test_normalize_specs(): assert_raises(ValueError, normalize_specs, error) -def test_credentials(): +def test_credentials(memory_keyring): # we want all tests to bypass the actual system keyring - with patch('datalad.support.keyring_.keyring', MemoryKeyring()): - check_credentials_cli() - check_interactive_entry_set() - check_interactive_entry_get() + check_credentials_cli() + check_interactive_entry_set() + check_interactive_entry_get() def test_errorhandling_smoketest(): @@ -185,16 +183,16 @@ def test_result_renderer(): )) -def test_extreme_credential_name(): +def test_extreme_credential_name(memory_keyring): cred = Credentials() extreme = 'ΔЙקم๗あ |/;&%b5{}"' - with patch('datalad.support.keyring_.keyring', MemoryKeyring()): - assert_in_results( - cred('set', - name=extreme, - # use CLI style spec to exercise more code - spec=[f'someprop={extreme}', f'secret={extreme}'], - ), - cred_someprop=extreme, - cred_secret=extreme, - ) + assert_in_results( + cred( + 'set', + name=extreme, + # use CLI style spec to exercise more code + spec=[f'someprop={extreme}', f'secret={extreme}'], + ), + cred_someprop=extreme, + cred_secret=extreme, + ) diff --git a/datalad_next/conftest.py b/datalad_next/conftest.py index 1b42ad02..d69e433b 100644 --- a/datalad_next/conftest.py +++ b/datalad_next/conftest.py @@ -1 +1,48 @@ +import pytest + from datalad.conftest import setup_package + + +@pytest.fixture(autouse=False, scope="function") +def memory_keyring(): + """Patch keyring to temporarily use a backend that only stores in memory + + No credential read or write actions will impact any existing credential + store of any configured backend. + + The patched-in backend is yielded by the fixture. It offers a ``store`` + attribute, which is a ``dict`` that uses keys of the pattern:: + + (datalad-, ) + + and the associated secrets as values. For non-legacy credentials the + ```` is uniformly ``'secret'``. For legacy credentials + other values are also used, including fields that are not actually + secrets. + """ + import keyring + import keyring.backend + + class MemoryKeyring(keyring.backend.KeyringBackend): + # high priority + priority = 1000 + + def __init__(self): + self.store = {} + + def set_password(self, servicename, username, password): + self.store[(servicename, username)] = password + + def get_password(self, servicename, username): + return self.store.get((servicename, username)) + + def delete_password(self, servicename, username): + del self.store[(servicename, username)] + + old_backend = keyring.get_keyring() + new_backend = MemoryKeyring() + keyring.set_keyring(new_backend) + + yield new_backend + + keyring.set_keyring(old_backend) diff --git a/datalad_next/credman/tests/test_credman.py b/datalad_next/credman/tests/test_credman.py index 59151e36..07523bf8 100644 --- a/datalad_next/credman/tests/test_credman.py +++ b/datalad_next/credman/tests/test_credman.py @@ -18,7 +18,6 @@ _get_cred_cfg_var, ) from datalad_next.tests.utils import ( - MemoryKeyring, assert_in, assert_not_in, assert_raises, @@ -31,13 +30,7 @@ from datalad_next.utils import chpwd -def test_credmanager(): - # we want all tests to bypass the actual system keyring - with patch('datalad.support.keyring_.keyring', MemoryKeyring()): - check_credmanager() - - -def check_credmanager(): +def test_credmanager(memory_keyring): cfg = ConfigManager() credman = CredentialManager(cfg) # doesn't work with thing air @@ -172,13 +165,7 @@ def test_credman_local(path=None): credman.remove('notstupid') -def test_query(): - # we want all tests to bypass the actual system keyring - with patch('datalad.support.keyring_.keyring', MemoryKeyring()): - check_query() - - -def check_query(): +def test_query(memory_keyring): cfg = ConfigManager() credman = CredentialManager(cfg) # set a bunch of credentials with a common realm AND timestamp @@ -228,13 +215,7 @@ def test_credman_get(): assert 'mysecret' == res['secret'] -def test_credman_obtain(): - # we want all tests to bypass the actual system keyring - with patch('datalad.support.keyring_.keyring', MemoryKeyring()): - check_obtain() - - -def check_obtain(): +def test_credman_obtain(memory_keyring): credman = CredentialManager(ConfigManager()) # senseless, but valid call # could not possibly report a credential without any info @@ -295,27 +276,27 @@ def check_obtain(): """ -def test_legacy_credentials(tmp_path): - # we will use a dataset to host a legacy provider config - ds = Dataset(tmp_path).create(result_renderer='disabled') - provider_path = ds.pathobj / '.datalad' / 'providers' / 'mylegacycred.cfg' - provider_path.parent.mkdir(parents=True, exist_ok=True) - provider_path.write_text(legacy_provider_cfg) +def test_legacy_credentials(memory_keyring, tmp_path): # - the legacy code will only ever pick up a dataset credential, when # PWD is inside a dataset # - we want all tests to bypass the actual system keyring # 'datalad.downloaders.credentials.keyring_' is what the UserPassword # credential will use to store the credential # - 'datalad.support.keyring_' is what credman uses - # - we need to make them one and the same thing - tmp_keyring = MemoryKeyring() - with chpwd(tmp_path), \ - patch('datalad.downloaders.credentials.keyring_', tmp_keyring), \ - patch('datalad.support.keyring_.keyring', tmp_keyring): - check_legacy_credentials(ds) + # - we need to make them one and the same thing, and the memory_keyring + # fixture does this by replacing the keyring backend for the runtime + # of the test + with chpwd(tmp_path): + check_legacy_credentials(memory_keyring, tmp_path) + +def check_legacy_credentials(memory_keyring, tmp_path): + # we will use a dataset to host a legacy provider config + ds = Dataset(tmp_path).create(result_renderer='disabled') + provider_path = ds.pathobj / '.datalad' / 'providers' / 'mylegacycred.cfg' + provider_path.parent.mkdir(parents=True, exist_ok=True) + provider_path.write_text(legacy_provider_cfg) -def check_legacy_credentials(ds): # shortcut cname = 'credmanuniquetestcredentialsetup' @@ -355,11 +336,11 @@ def check_legacy_credentials(ds): # the legacy backend, in order to keep old code working # with the old info # confirm starting point: legacy code keeps user in secret store - assert credman._keyring.entries[cname]['user'] == 'mike' + assert memory_keyring.store[(f'datalad-{cname}', 'user')] == 'mike' assert ds.config.get(f'datalad.credential.{cname}.user') is None credman.set(cname, **cred) # it remains there - assert credman._keyring.entries[cname]['user'] == 'mike' + assert memory_keyring.store[(f'datalad-{cname}', 'user')] == 'mike' # but is also migrated assert ds.config.get(f'datalad.credential.{cname}.user') == 'mike' # NOTE: This setup is not without problems. Users will update @@ -367,7 +348,7 @@ def check_legacy_credentials(ds): # however, I did not come up with a better approach that gradually # brings users over. credman.set(cname, user='newmike', secret='othersecret') - assert credman._keyring.entries[cname]['user'] == 'mike' + assert memory_keyring.store[(f'datalad-{cname}', 'user')] == 'mike' # finally check that the update is reported now cred = credman.get(cname) assert cred['user'] == 'newmike' diff --git a/datalad_next/tests/utils.py b/datalad_next/tests/utils.py index 687a8628..1835ff53 100644 --- a/datalad_next/tests/utils.py +++ b/datalad_next/tests/utils.py @@ -40,7 +40,6 @@ ) from datalad.tests.test_utils_testrepos import BasicGitTestRepo from datalad.cli.tests.test_main import run_main -from datalad.support.keyring_ import MemoryKeyring from datalad_next.utils import ( CredentialManager, optional_args,