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

Implements secure value loading method for multiple Windows credentials #191

Merged
merged 46 commits into from
Aug 29, 2023

Conversation

samadpls
Copy link
Contributor

@samadpls samadpls commented Jun 23, 2023

What It Does

resolved #134
How to Test

Review Checklist
I certify that I have:

Additional Comments

@codecov
Copy link

codecov bot commented Jun 23, 2023

Codecov Report

Patch coverage: 95.12% and project coverage change: +1.74% 🎉

Comparison is base (558edfb) 79.35% compared to head (58178c0) 81.10%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #191      +/-   ##
==========================================
+ Coverage   79.35%   81.10%   +1.74%     
==========================================
  Files          32       33       +1     
  Lines        1589     1752     +163     
==========================================
+ Hits         1261     1421     +160     
- Misses        328      331       +3     
Flag Coverage Δ
unittests 81.10% <95.12%> (+1.74%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/core/zowe/core_for_zowe_sdk/constants.py 100.00% <ø> (ø)
src/core/zowe/core_for_zowe_sdk/zosmf_profile.py 34.24% <33.33%> (-0.97%) ⬇️
.../core/zowe/core_for_zowe_sdk/credential_manager.py 92.30% <92.30%> (ø)
tests/unit/test_zowe_core.py 99.33% <99.05%> (-0.18%) ⬇️
src/core/zowe/core_for_zowe_sdk/__init__.py 100.00% <100.00%> (ø)
src/core/zowe/core_for_zowe_sdk/config_file.py 87.83% <100.00%> (+2.20%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aadityasinha-dotcom aadityasinha-dotcom changed the title Implementws secure value loading method for multiple Windows credentials Implements secure value loading method for multiple Windows credentials Jun 25, 2023
@samadpls samadpls requested a review from t1m0thyj July 4, 2023 16:19
samadpls added 6 commits July 6, 2023 00:23
… & handled None case in `keyring.get_password`

Signed-off-by: samadpls <[email protected]>
Signed-off-by: samadpls <[email protected]>
Signed-off-by: samadpls <[email protected]>
Signed-off-by: samadpls <[email protected]>
Signed-off-by: samadpls <[email protected]>
Signed-off-by: samadpls <[email protected]>
@t1m0thyj t1m0thyj requested a review from zFernand0 July 7, 2023 14:14
src/core/zowe/core_for_zowe_sdk/config_file.py Outdated Show resolved Hide resolved
src/core/zowe/core_for_zowe_sdk/config_file.py Outdated Show resolved Hide resolved
src/core/zowe/core_for_zowe_sdk/config_file.py Outdated Show resolved Hide resolved
src/core/zowe/core_for_zowe_sdk/config_file.py Outdated Show resolved Hide resolved
src/core/zowe/core_for_zowe_sdk/config_file.py Outdated Show resolved Hide resolved
src/core/zowe/core_for_zowe_sdk/config_file.py Outdated Show resolved Hide resolved
@samadpls samadpls requested a review from t1m0thyj July 21, 2023 18:47
Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

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

The new CredentialManager implementation is looking good 👍 I like the usage of warnings.filterwarnings - clever way to prevent the keyring package from spamming the console 🙂

It looks like most of the methods for credential management are duplicated in config_file.py and credential_manager.py. I assume this is temporary so that tests will pass, and the next step would be to consolidate into a single implementation in the CredentialManager class?

src/core/zowe/core_for_zowe_sdk/config_file.py Outdated Show resolved Hide resolved
src/core/zowe/core_for_zowe_sdk/config_file.py Outdated Show resolved Hide resolved
src/core/zowe/core_for_zowe_sdk/config_file.py Outdated Show resolved Hide resolved
src/core/zowe/core_for_zowe_sdk/config_file.py Outdated Show resolved Hide resolved
@zFernand0 zFernand0 requested a review from t1m0thyj August 9, 2023 15:05
@samadpls samadpls requested a review from t1m0thyj August 17, 2023 13:31
Signed-off-by: samadpls <[email protected]>
Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

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

LGTM and thanks for adding tests 👍

Just have one comment about unused imports 😋

Signed-off-by: samadpls <[email protected]>
Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

LGTM! 😋
Clean implementation and easy to use.

I left one comment about a small bug in the _retrieve_credentials method where we are calling decode on a str 😋

Signed-off-by: samadpls <[email protected]>
Signed-off-by: samadpls <[email protected]>
Signed-off-by: samadpls <[email protected]>
Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

LGTM! 😋

@zFernand0 zFernand0 merged commit 4bc1a86 into main Aug 29, 2023
@zFernand0 zFernand0 deleted the multi-credentials branch August 29, 2023 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Load secure values from multiple credential entries on Windows
3 participants