-
Notifications
You must be signed in to change notification settings - Fork 2.6k
CredentialsProvider class added to support password rotation #2261
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
Merged
Merged
Changes from 18 commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
86da7ff
A CredentialsProvider class has been added to allow the user to add h…
barshaul 5dfddde
Moved CredentialsProvider to a separate file, added type hints
barshaul 243b244
Changed username and password to properties
barshaul af8e560
Added: StaticCredentialProvider, examples, tests
barshaul 2261cb0
Changed private members' prefix to __
barshaul ddfe1ea
fixed linters
barshaul b481067
fixed auth test
barshaul 686d172
fixed credential test
barshaul d1d10af
Raise an error if username or password are passed along with credenti…
barshaul 9de8d21
fixing linters
barshaul def996b
fixing test
barshaul 29c8006
Changed dundered to single per side underscore
barshaul 6b8cf1f
Changed Connection class members username and password to properties …
barshaul c37e0f1
Reverting last commit and adding backward compatibility to 'username'…
barshaul abe6137
Refactored CredentialProvider class
barshaul 6303243
Fixing tuple type to Tuple
barshaul 057ed82
Fixing optional string members in UsernamePasswordCredentialProvider
barshaul ba91b0f
Fixed credential test
barshaul 6223901
Added credential provider support to AsyncRedis
barshaul fac8333
Merge branch 'master' into creds_provider
dvora-h b951e19
linters
dvora-h 72c366d
linters
dvora-h 4b35cb2
linters
dvora-h 4c82551
linters - black
dvora-h File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
from typing import Optional, Tuple, Union | ||
|
||
|
||
class CredentialProvider: | ||
""" | ||
Credentials Provider. | ||
""" | ||
|
||
def get_credentials(self) -> Union[Tuple[str], Tuple[str, str]]: | ||
raise NotImplementedError("get_credentials must be implemented") | ||
|
||
|
||
class UsernamePasswordCredentialProvider(CredentialProvider): | ||
""" | ||
Simple implementation of CredentialProvider that just wraps static | ||
username and password. | ||
""" | ||
|
||
def __init__(self, username: Optional[str] = None, password: Optional[str] = None): | ||
self.username = username or "" | ||
self.password = password or "" | ||
|
||
def get_credentials(self): | ||
if self.username: | ||
return self.username, self.password | ||
return (self.password,) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I'm not sure why this needs to be a class at all – couldn't a provider just be a function
get_credentials(*, username, password)
...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.
Different credential providers will need different arguments to be passed, so we can save within the credential provider object the required args/kwargs.
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.
You're right: a credential provider could indeed just be a
get_credentials()
function with no knowledge of any arguments. 😉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.
Yup, that would do the job. :)
@chayim WDYT?
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.
There's no reason that during the instantiation of all Redis connection objects a function can't be passed in, as opposed to the class being discussed. However, I think the current class implementation is more readable for the community that uses this. I'm partial for erring towards readability, and the clear separation it provides.
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.
@chayim @barshaul Well, this class looks like it's become quite complicated and hard-to-follow.
It has an username/password that may not be used at all, args and kwargs that may not be used at all.
If you want a class, then why not just
and leave the rest (e.g. implementing a specific provider with a specific
get_credentials
) to the users?As an aside – should
get_credentials
maybe also support being async? :)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.
With the changes now done to
Connection
, I really do believe that the implementation I suggest above would be the better fit, and the currentCredentialProvider
is way too generic. I sincerely ask you to revise it.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.
get_credentials
should definitely support an async all, to keep this aligned.On the class inputs side - I don't think it's unreasonable to pass in an optional
CredentialProvider
on a per redis-connection type. I see classes as more useful given state needs, depending on the provider. Maybe instead,CredentialProvider
shouldn't require any specified arguments, instead be a descendent of an ABC that accepts**kwargs
. This could address the same need, and reduce the complexity, but yes - be perhaps generic enough (though perhaps something shared).@akx WDYT needlessly generic? Having a way to do this for systems that are completely different makes it problematic.
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.
@chayim The "needlessly generic" comment was outdated by abe6137, which removed the "username + password + supplier + args + kwargs" mode.