Repository: datalad/datalad

Issue:
Support credential query via config manager
ATM we have a custom way to provide credentials when a keystore is not an option or desirable for whatever reason. However, this method is not without flaws. Summary: https://github.com/datalad/datalad/issues/4981

This issue also argues to query ConfigManager instead of a direct ENV variable to gain a substantial amount of flexibility, e.g. specify via ENV var, or commandline arg (`-c ...`), or from config files already read at system/user/dataset level. Such a setup would also make override credentials accessible to convenience tools like http://docs.datalad.org/projects/mihextras/en/latest/generated/man/datalad-x-configuration.html

This issue is aiming to collect all loose ends and make this move happen.

### Status quo
The ultimate ENV var query is presently made here: https://github.com/datalad/datalad/blob/8defef58239d9c588992eee4878fe2873b920734/datalad/support/keyring_.py#L50

It is lacking a meaningful markup of the ENV var to clearly indicate a credential component (see https://github.com/datalad/datalad/pull/4135)

Importantly, it precedes the an actual keyring query.

The function is actually called by https://github.com/datalad/datalad/blob/8defef58239d9c588992eee4878fe2873b920734/datalad/downloaders/credentials.py#L39

more precisely in three methods

https://github.com/datalad/datalad/blob/8defef58239d9c588992eee4878fe2873b920734/datalad/downloaders/credentials.py#L90

https://github.com/datalad/datalad/blob/8defef58239d9c588992eee4878fe2873b920734/datalad/downloaders/credentials.py#L154

https://github.com/datalad/datalad/blob/8defef58239d9c588992eee4878fe2873b920734/datalad/downloaders/credentials.py#L177

I would argue that this class and its methods are a more appropriate layer to query for credentials, rather than `keyring_`.

### ENV var composition

ATM a query ENV var is built with the rule `'DATALAD_%s_%s' % (name, field)).replace('-', '_')`, where `name` is essentially user-provided and unpredictable (see [handbook](http://handbook.datalad.org/en/latest/beyond_basics/101-146-providers.html?highlight=credential%20%3D%20#example-data-access-to-a-server-that-requires-basic-http-authentication)) and `field` is defined by the credential type. -core defines at least the following, but there may be more:
- `user`
- `password`
- `token`
- `key_id`
- `secret_id`
- `session`
- `expiration`

Following the spirit of https://github.com/datalad/datalad/pull/4135 we could introduce configuration variables of the pattern

`datalad.credential.<name>.<field>`

This would nicely map onto the git config syntax. `name` can contain any character except newline and the null byte (a reasonable constraint for a user provided value). The `field` must only contain alphanum and `-`, again reasonable for a developer-made choice, and requires minimal modification from the status quo `-` <-> `_`.

With a switch to query `ConfigManager` it will no longer be necessary to support any `name` specification via ENV var, because alternatives exist. However, any specification rule (and name mapping) will be uniform across all ENV var use in datalad.

### Plan

- [x] Switch to ConfigManager instead of direct `os.environ` query
- [x] Consult `ConfigManager` in `Credential` not `Keyring`
- [x] Support `datalad.credential.<name>.<field>` config settings (map field names via `_` -> `-`;, do not map `name` in any way, other than what `ConfigManager` does for all variables)

Discussion:

Patch:
diff --git a/datalad/downloaders/credentials.py b/datalad/downloaders/credentials.py
index 59727f1b3..063038936 100644
--- a/datalad/downloaders/credentials.py
+++ b/datalad/downloaders/credentials.py
@@ -31,6 +31,8 @@ from ..ui import ui
 from ..utils import auto_repr
 from ..support.network import iso8601_to_epoch
 
+from datalad import cfg as dlcfg
+
 from logging import getLogger
 lgr = getLogger('datalad.downloaders.credentials')
 
@@ -87,12 +89,18 @@ class Credential(object):
         """Return True if values for all fields of the credential are known"""
         try:
             return all(
-                self._is_field_optional(f) or self._keyring.get(self.name, f) is not None
+                self._is_field_optional(f) or self._get_field_value(f) is not None
                 for f in self._FIELDS)
         except Exception as exc:
             lgr.warning("Failed to query keyring: %s" % exc_str(exc))
             return False
 
+    def _get_field_value(self, field):
+        return dlcfg.get('datalad.credential.{name}.{field}'.format(
+            name=self.name,
+            field=field.replace('_', '-')
+        )) or self._keyring.get(self.name, field)
+
     def _ask_field_value(self, f, instructions=None):
         msg = instructions if instructions else \
             ("You need to authenticate with %r credentials." % self.name +
@@ -148,10 +156,9 @@ class Credential(object):
 
     def __call__(self):
         """Obtain credentials from a keyring and if any is not known -- ask"""
-        name = self.name
         fields = {}
         for f in self._FIELDS:
-            v = self._keyring.get(name, f)
+            v = self._get_field_value(f)
             if not self._is_field_optional(f):
                 while v is None:  # was not known
                     v = self._ask_and_set(f)
@@ -174,7 +181,7 @@ class Credential(object):
             raise ValueError("Unknown field %s. Known are: %s"
                              % (f, self._FIELDS.keys()))
         try:
-            return self._keyring.get(self.name, f)
+            return self._get_field_value(f)
         except:  # MIH: what could even happen? _keyring not a dict?
             return default
 


Test patch:
diff --git a/datalad/downloaders/tests/test_credentials.py b/datalad/downloaders/tests/test_credentials.py
index 37bcc2dee..b5ccbbe7a 100644
--- a/datalad/downloaders/tests/test_credentials.py
+++ b/datalad/downloaders/tests/test_credentials.py
@@ -27,6 +27,7 @@ from ..credentials import (
     CompositeCredential,
     UserPassword,
 )
+from datalad.config import ConfigManager
 
 
 @with_testsui(responses=[
@@ -124,11 +125,32 @@ def test_credentials_from_env():
     assert_equal(cred.get('key_id'), None)
     assert_equal(cred.get('secret_id'), None)
 
-    with patch.dict('os.environ', {'DATALAD_test_s3_key_id': '1'}):
+    def _check1():
         assert_equal(cred.get('key_id'), '1')
         assert_false(cred.is_known)
+
+    def _check2():
+        assert_equal(cred.get('key_id'), '1')
+        assert_equal(cred.get('secret_id'), '2')
+        assert_true(cred.is_known)
+
+    # this is the old way, should still work
+    with patch.dict('os.environ', {'DATALAD_test_s3_key_id': '1'}):
+        _check1()
         with patch.dict('os.environ', {'DATALAD_test_s3_secret_id': '2'}):
-            assert_equal(cred.get('key_id'), '1')
-            assert_equal(cred.get('secret_id'), '2')
-            assert_true(cred.is_known)
+            _check2()
         assert_false(cred.is_known)  # no memory of the past
+
+    # here is the new way
+    import datalad
+    try:
+        with patch.dict('os.environ', {'DATALAD_CREDENTIAL_test__s3_key__id': '1'}):
+            datalad.cfg.reload()
+            _check1()
+            with patch.dict('os.environ', {'DATALAD_CREDENTIAL_test__s3_secret__id': '2'}):
+                datalad.cfg.reload()
+                _check2()
+            datalad.cfg.reload()
+            assert_false(cred.is_known)  # no memory of the past
+    finally:
+        datalad.cfg.reload()