-
Notifications
You must be signed in to change notification settings - Fork 28
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
Conversation
Signed-off-by: samadpls <[email protected]>
Codecov ReportPatch coverage:
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
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
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]>
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]>
…edential` Signed-off-by: samadpls <[email protected]>
Signed-off-by: samadpls <[email protected]>
… & 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]>
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]>
There was a problem hiding this 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?
Signed-off-by: Abdul Samad Siddiqui <[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]>
Signed-off-by: samadpls <[email protected]>
Signed-off-by: samadpls <[email protected]>
Signed-off-by: samadpls <[email protected]>
Signed-off-by: samadpls <[email protected]>
There was a problem hiding this 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]>
There was a problem hiding this 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]>
Signed-off-by: samadpls <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 😋
What It Does
resolved #134
How to Test
Review Checklist
I certify that I have:
Additional Comments