BF: do not crash, just skip whenever trying to delete non existing pa…

…ssw in keyring

Fixing bug leading to a crash: We have pairs or more in composite
credentials. If user saved some but not all of them, then upon renew whenever
we try to delete them all we would crash here since keyring cannot delete the
ones it does not yet know.  So the easiest (although adhoc due to need to match
msg) is to handle such an exception.

To get it tested, I had to use a "real" keyring's backend (so they are called),
and for that I had to RF our Keyring helper class to be capable of being
provided a keyring backend instance (or in other words -- a "keyring").  An odd
logic abit I left behind is that even though we are now explicitly asking for
what keyring will be used, so we could store/use that instance if no explicit
backend is provided, but for consistency with current behavior (since to go
against maint), decided to keep it as is and keep using module level bound
interfaces of keyring module in case if no backend instance was provided.

So now ._keyring  could be either keyring module (no backend instance provided,
or actual keyring backend if it was provided (used only in the tests).

Added test also tests basic assumption that keyring is storing the name
somewhere, and deletes entire section whenever we delete entire credential.
Relies on "ad-hoc" way to specify the filename. Somewhat inquired in
jaraco/keyrings.alt#45 on a better way.

Closes #5889

datalad/downloaders/tests/test_credentials.py

@@ -15,9 +15,13 @@
     assert_in,
     assert_raises,
     assert_true,
+    ok_file_has_content,
     SkipTest,
+    skip_if,
+    with_tempfile,
     with_testsui,
 )
+from datalad.support.external_versions import external_versions
 from datalad.support.keyring_ import (
     Keyring,
     MemoryKeyring,
@@ -58,6 +62,7 @@ def test_cred1_enter_new():
     assert_equal(keyring.get('name', 'user'), 'user2')
     assert_equal(keyring.get('name', 'password'), 'newpassword')
 
+
 @with_testsui(responses=['password1'])
 def test_cred1_call():
     keyring = MemoryKeyring()
@@ -132,3 +137,32 @@ def test_credentials_from_env():
             assert_equal(cred.get('secret_id'), '2')
             assert_true(cred.is_known)
         assert_false(cred.is_known)  # no memory of the past
+
+
+@skip_if(not external_versions['keyrings.alt'])
+@with_tempfile
+def test_delete_not_crashing(path):
+    # although in above test we just use/interact with Keyring without specifying
+    # any custom one, there we do not change it so I guess it is ok. Here we want
+    # a real keyring backend which we will alter
+    from keyrings.alt.file import PlaintextKeyring
+    kb = PlaintextKeyring()
+    kb.filename = path
+
+    keyring = Keyring(keyring_backend=kb)
+    cred = UserPassword("test1", keyring=keyring)
+
+    cred.set(user="user1", password="password")
+    ok_file_has_content(path, ".*test1.*", re_=True)  # keyring backend saves where we expect
+
+    # manually delete one component of the credential
+    cred._keyring.delete(cred.name, next(iter(cred._FIELDS)))
+
+    # now delete entire credential -- we must not crash
+    cred.delete()
+    try:
+        ok_file_has_content(path, ".*test1.*", re_=True)  # keyring backend saves where we expect
+        raise AssertionError("keyring still has our key")
+    except AssertionError:
+        pass
+

datalad/support/keyring_.py

@@ -19,13 +19,22 @@ class Keyring(object):
 
     It also delays import of keyring which takes 300ms I guess due to all plugins etc
     """
-    def __init__(self):
-        self.__keyring = None
+    def __init__(self, keyring_backend=None):
+        """
+
+        Parameters
+        ----------
+        keyring_backend: keyring.backend.KeyringBackend, optional
+          Specific keyring to use.  If not provided, the one returned by
+          `keyring.get_keyring()` is used
+        """
+        self.__keyring = keyring_backend
+        self.__keyring_mod = None
 
     @property
     def _keyring(self):
-        if self.__keyring is None:
-            # Setup logging for keyring if we are debugging, althought keyring's logging
+        if self.__keyring_mod is None:
+            # Setup logging for keyring if we are debugging, although keyring's logging
             # is quite scarce ATM
             from datalad.log import lgr
             import logging
@@ -36,8 +45,14 @@ def _keyring(self):
                 keyring_lgr.handlers = lgr.handlers
             lgr.debug("Importing keyring")
             import keyring
-            self.__keyring = keyring
-            the_keyring = keyring.get_keyring()
+            self.__keyring_mod = keyring
+
+        if self.__keyring is None:
+            from datalad.log import lgr
+            # we use module bound interfaces whenever we were not provided a dedicated
+            # backend
+            self.__keyring = self.__keyring_mod
+            the_keyring = self.__keyring_mod.get_keyring()
             if the_keyring.name.lower().startswith('null '):
                 lgr.warning(
                     "Keyring module returned '%s', no credentials will be provided",
@@ -64,7 +79,12 @@ def set(self, name, field, value):
     def delete(self, name, field=None):
         if field is None:
             raise NotImplementedError("Deletion of all fields associated with a name")
-        return self._keyring.delete_password(self._get_service_name(name), field)
+        try:
+            return self.__keyring.delete_password(self._get_service_name(name), field)
+        except self.__keyring_mod.errors.PasswordDeleteError as exc:
+            if 'not found' in str(exc):
+                return
+            raise
 
 
 class MemoryKeyring(object):