New config `datalad.credentials.force-ask` to force interactive credential updates #5777

mih · 2021-06-30T09:09:01Z

This change introduces a new configuration setting:

 % datalad x-configuration
 ...
 # Force (re-)entry of credentials
 # Should DataLad prompt for credential (re-)entry? This can be used to
 # update previously stored credentials.
 datalad.credentials.force-ask=False
 ...

It enables seamless updates of stored credentials. Importantly, users
are no longer required to learn where credentials are stored, and how to
update them in that store. It makes instructions provided in the
handbook on how to learn python keyring superfluous. Credential
updates are simply entered at the same place where the original
credentials were provided.

This is not a perfect solution (can get messy when there are a
number of credentials involved in a single datalad operation, and only a
single one needs updating), but it addresses a substantial usability
issue for a broad range of use cases in a straightforward manner.

Besides being compatible with the usual ways of DataLad configuration
handling, this also works for plain git-annex calls that trigger
DataLad's credential handling, e.g.:

git -c datalad.credentials.force-ask=no annex get T1w_acpc_dc.nii.gz

Fixes #5775

This is related to #396 by addressing the "update" use case.

bpoldrack

Looks straight forward to me.

datalad/interface/common_cfg.py

codecov · 2021-06-30T09:52:06Z

Codecov Report

Merging #5777 (461d7cc) into master (9ff9ce3) will decrease coverage by 2.01%.
The diff coverage is 100.00%.

❗ Current head 461d7cc differs from pull request most recent head 70c8056. Consider uploading reports for the commit 70c8056 to get more accurate results

@@            Coverage Diff             @@
##           master    #5777      +/-   ##
==========================================
- Coverage   90.62%   88.60%   -2.02%     
==========================================
  Files         309      306       -3     
  Lines       42381    42356      -25     
==========================================
- Hits        38406    37531     -875     
- Misses       3975     4825     +850

Impacted Files	Coverage Δ
datalad/distribution/get.py	`97.90% <ø> (ø)`
datalad/distribution/tests/test_get.py	`100.00% <ø> (ø)`
datalad/interface/common_cfg.py	`100.00% <ø> (ø)`
datalad/cmd_protocols.py	`94.44% <100.00%> (-5.56%)`	⬇️
datalad/distribution/tests/test_publish.py	`100.00% <100.00%> (ø)`
datalad/distribution/tests/test_update.py	`98.95% <100.00%> (-1.05%)`	⬇️
datalad/downloaders/credentials.py	`85.63% <100.00%> (+0.16%)`	⬆️
datalad/downloaders/tests/test_credentials.py	`100.00% <100.00%> (ø)`
datalad/plugin/wtf.py	`0.00% <0.00%> (-100.00%)`	⬇️
datalad/plugin/addurls.py	`0.00% <0.00%> (-100.00%)`	⬇️
... and 82 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ff9ce3...70c8056. Read the comment docs.

yarikoptic · 2021-06-30T14:49:31Z

can't lookup atm, but I think we contemplated a command like datalad credentials which would list known providers/credentials and interface forced re-entry. But I guess it is orthogonal/complimentary to this PR, which in general I do not mind, but haven't thought yet about possible corner cases etc.

edit 20210715: found it #4981 (comment)

mih · 2021-06-30T15:48:35Z

OK, I will put this back in draft mode. It does not work: parallel operation!

git -c datalad.credential.force-ask=yes annex get -J2 T1w_acpc_dc.nii.gz ribbon.nii.gz

asks twice.

Conceptual issue in PR

mih · 2021-07-02T05:36:40Z

So thinking about this for a bit more. It might just be a matter of documenting that parallel operations and this flag do not work conveniently with each other. Any other solution would yield in a undesirably complex entangling of credential queries and download (or other) operations.

A legitimate Q is whether this is worth pursuing at all, rather than implementing #5777 (comment) (credential command): I think it is, because a big problem is the identification of the relevant credentials that need an update. With a get call this is relatively straightforward. But the credential naming implemented by DataLad is completely opaque right now, and not even communicated to the use upon initial storage in a credential store. On top of that come the problem that the effective credential store can change over time and with configuration.

Bottom line: I think this is a useful feature, even with a coming credential command.

christian-monch · 2021-07-15T11:27:57Z

[...]
Bottom line: I think this is a useful feature, even with a coming credential command.

Without going into technical details, I agree with the rationale given here. Especially since the credential command might not be here before a few months have passed

bpoldrack · 2021-07-15T12:08:25Z

Bottom line: I think this is a useful feature, even with a coming credential command.

I tend to agree, but it may need reconsideration of config naming. We have command specific configs like datalad.clone.something. If we are introducing datalad.credential.some and then later a command credential, then the config better be something that makes sense for that command to respect or not for the config to not "conflict" in the first place.
Not quite sure how to avoid future user confusion.

The feature itself seems to be worthwhile with or w/o the command, though.

mih · 2021-07-15T12:53:26Z

I tend to agree, but it may need reconsideration of config naming. We have command specific configs like datalad.clone.something. If we are introducing datalad.credential.some and then later a command credential, then the config better be something that makes sense for that command to respect or not for the config to not "conflict" in the first place.
Not quite sure how to avoid future user confusion.

Do you have a suggestion for a change?

yarikoptic · 2021-07-15T18:11:19Z

I tend to agree, but it may need reconsideration of config naming. We have command specific configs like datalad.clone.something. If we are introducing datalad.credential.some and then later a command credential, then the config better be something that makes sense for that command to respect or not for the config to not "conflict" in the first place.
Not quite sure how to avoid future user confusion.

Do you have a suggestion for a change?

make it datalad.credentials config and datalad credentials command. At least until some brave soul would do big UI RF towards something like #4539 (comment) . More argumentation is above

This change introduces a new configuration setting: ``` % datalad x-configuration ... # Force (re-)entry of credentials # Should DataLad prompt for credential (re-)entry? This can be used to # update previously stored credentials. datalad.credentials.force-ask=False ... ``` It enables seamless updates of stored credentials. Importantly, users are no longer required to learn where credentials are stored, and how to update them in that store. It makes instructions provided in the handbook on how to learn python `keyring` superfluous. Credential updates are simply entered at the same place where the original credentials were provided. This is not a perfect solution (can get messy when there are a number of credentials involved in a single datalad operation, and only a single one needs updating), but it addresses a substantial usability issue for a broad range of use cases in a straightforward manner. Besides being compatible with the usual ways of DataLad configuration handling, this also works for plain git-annex calls that trigger DataLad's credential handling, e.g.: ``` git -c datalad.credentials.force-ask=no annex get T1w_acpc_dc.nii.gz ``` Fixes datalad#5775

mih · 2021-07-16T08:11:11Z

Bowed to the majority of expressed opinions, and force-pushed an amendment that renames the config setting to plural.

mih · 2021-07-17T09:53:36Z

Took out of draft mode, ready to merge. Test failure is unrelated #5794

yarikoptic

I think it might be worth to tune up PR title to become more specific, before merge

mih · 2021-07-17T12:53:42Z

Title is updated.

mih added the credentials Issues concerning credential(-management) label Jun 30, 2021

mih added this to the 0.15.0 milestone Jun 30, 2021

bpoldrack previously approved these changes Jun 30, 2021

View reviewed changes

datalad/interface/common_cfg.py Outdated Show resolved Hide resolved

mih marked this pull request as draft June 30, 2021 15:49

mih force-pushed the bf-5775 branch from b290f2a to 70c8056 Compare July 16, 2021 08:10

mih marked this pull request as ready for review July 17, 2021 09:52

yarikoptic approved these changes Jul 17, 2021

View reviewed changes

yarikoptic added the semver-patch Increment the patch version when merged label Jul 17, 2021

mih changed the title ~~NF: Support credential updates~~ New config datalad.credentials.force-ask to force interactive credential updates Jul 17, 2021

yarikoptic merged commit 0b2d882 into datalad:master Jul 17, 2021

mih deleted the bf-5775 branch July 27, 2021 04:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New config `datalad.credentials.force-ask` to force interactive credential updates #5777

New config `datalad.credentials.force-ask` to force interactive credential updates #5777

mih commented Jun 30, 2021 •

edited

bpoldrack left a comment

codecov bot commented Jun 30, 2021 •

edited

yarikoptic commented Jun 30, 2021 •

edited

mih commented Jun 30, 2021

mih commented Jul 2, 2021 •

edited

christian-monch commented Jul 15, 2021

bpoldrack commented Jul 15, 2021

mih commented Jul 15, 2021

yarikoptic commented Jul 15, 2021

mih commented Jul 16, 2021

mih commented Jul 17, 2021

yarikoptic left a comment

mih commented Jul 17, 2021

New config datalad.credentials.force-ask to force interactive credential updates #5777

New config datalad.credentials.force-ask to force interactive credential updates #5777

Conversation

mih commented Jun 30, 2021 • edited

bpoldrack left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 30, 2021 • edited

Codecov Report

yarikoptic commented Jun 30, 2021 • edited

mih commented Jun 30, 2021

mih commented Jul 2, 2021 • edited

christian-monch commented Jul 15, 2021

bpoldrack commented Jul 15, 2021

mih commented Jul 15, 2021

yarikoptic commented Jul 15, 2021

mih commented Jul 16, 2021

mih commented Jul 17, 2021

yarikoptic left a comment

Choose a reason for hiding this comment

mih commented Jul 17, 2021

New config `datalad.credentials.force-ask` to force interactive credential updates #5777

New config `datalad.credentials.force-ask` to force interactive credential updates #5777

mih commented Jun 30, 2021 •

edited

codecov bot commented Jun 30, 2021 •

edited

yarikoptic commented Jun 30, 2021 •

edited

mih commented Jul 2, 2021 •

edited