Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BF: do not crash, just skip whenever trying to delete non existing field in the underlying keyring #5892

Merged
merged 1 commit into from
Aug 18, 2021

Conversation

yarikoptic
Copy link
Member

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

…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 datalad#5889
@yarikoptic yarikoptic added credentials Issues concerning credential(-management) semver-patch Increment the patch version when merged UX user experience labels Aug 13, 2021
@yarikoptic yarikoptic added this to the 0.14.8 milestone Aug 13, 2021
@codecov
Copy link

codecov bot commented Aug 13, 2021

Codecov Report

Merging #5892 (ee6d622) into maint (513c753) will decrease coverage by 60.73%.
The diff coverage is 96.96%.

❗ Current head ee6d622 differs from pull request most recent head 3cddd7c. Consider uploading reports for the commit 3cddd7c to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##            maint    #5892       +/-   ##
===========================================
- Coverage   90.34%   29.60%   -60.74%     
===========================================
  Files         300      297        -3     
  Lines       42396    42387        -9     
===========================================
- Hits        38302    12549    -25753     
- Misses       4094    29838    +25744     
Impacted Files Coverage Δ
datalad/support/keyring_.py 90.32% <93.33%> (-0.25%) ⬇️
datalad/downloaders/tests/test_credentials.py 100.00% <100.00%> (ø)
datalad/tests/test_api.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/support/digests.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/tests/test_base.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/tests/test_config.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/ui/tests/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/tests/test__main__.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/tests/test_strings.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/ui/tests/test_base.py 0.00% <0.00%> (-100.00%) ⬇️
... and 248 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 513c753...3cddd7c. Read the comment docs.

@yarikoptic yarikoptic added the merge-if-ok OP considers this work done, and requests review/merge label Aug 13, 2021
Copy link
Member

@mih mih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The introduction of the backend switch is borderline invasive for maint, but it seems to be safe. Thx!

@yarikoptic
Copy link
Member Author

thank you @mih. Yeah -- was thinking the same about maint... but probably will take the chances now instead of RFing (I guess we could mock needed things out from our Keyring). Let's proceed (since approved)

@yarikoptic yarikoptic merged commit ea0f0c3 into datalad:maint Aug 18, 2021
@yarikoptic yarikoptic deleted the bf-delete-credential branch October 8, 2021 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
credentials Issues concerning credential(-management) merge-if-ok OP considers this work done, and requests review/merge semver-patch Increment the patch version when merged UX user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants