-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Support custom authentication handlers for private pypi. #8250
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Support for custom authentication handlers via --auth-plugin option. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,8 @@ | |
from optparse import SUPPRESS_HELP, Option, OptionGroup | ||
from textwrap import dedent | ||
|
||
from pip._vendor.pkg_resources import iter_entry_points | ||
|
||
from pip._internal.cli.progress_bars import BAR_TYPES | ||
from pip._internal.exceptions import CommandError | ||
from pip._internal.locations import USER_CACHE_DIR, get_src_prefix | ||
|
@@ -920,6 +922,35 @@ def check_list_path_option(options): | |
) # type: Callable[..., Option] | ||
|
||
|
||
discovered_auth_plugins = { | ||
entry_point.name: entry_point | ||
for entry_point | ||
in iter_entry_points('pip.auth_plugins') | ||
} | ||
|
||
|
||
def _handle_auth_plugin(option, opt_str, value, parser): | ||
# type: (Option, str, str, OptionParser) -> None | ||
plugin = discovered_auth_plugins[value].load() | ||
setattr(parser.values, option.dest, plugin.Auth) | ||
|
||
|
||
auth_plugin = partial( | ||
PipOption, | ||
'--auth', '--auth-plugin', | ||
dest='auth_class', | ||
type='choice', | ||
metavar='plugin', | ||
default=None, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we might want to have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I considered this. But didn't want to couple cli and network code. |
||
choices=list(discovered_auth_plugins.keys()), | ||
action='callback', | ||
callback=_handle_auth_plugin, | ||
help='Specify authentication plugin to be used: {}.'.format( | ||
', '.join(discovered_auth_plugins.keys()) or 'no plugins found' | ||
Comment on lines
+948
to
+949
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we can phrase this better in case no plugin found. Personally I'm bad at making concise sentences so I can't offer any suggestion though. |
||
), | ||
) # type: Callable[..., Option] | ||
|
||
|
||
########## | ||
# groups # | ||
########## | ||
|
@@ -948,6 +979,7 @@ def check_list_path_option(options): | |
no_color, | ||
no_python_version_warning, | ||
unstable_feature, | ||
auth_plugin, | ||
] | ||
} # type: Dict[str, Any] | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -238,6 +238,9 @@ def __init__(self, *args, **kwargs): | |||||||||
cache = kwargs.pop("cache", None) | ||||||||||
trusted_hosts = kwargs.pop("trusted_hosts", []) # type: List[str] | ||||||||||
index_urls = kwargs.pop("index_urls", None) | ||||||||||
auth_class = kwargs.pop("auth_class", None) | ||||||||||
if auth_class is None: | ||||||||||
auth_class = MultiDomainBasicAuth | ||||||||||
Comment on lines
+241
to
+243
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That does not work because this named argument is always provided. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I missed that. How about provide it with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried that at first. But None is a better default IMO. This way cli is not coupled with network. Currently MultiDomainBasicAuth is not a plugin, it's the default implementation. |
||||||||||
|
||||||||||
super(PipSession, self).__init__(*args, **kwargs) | ||||||||||
|
||||||||||
|
@@ -249,7 +252,7 @@ def __init__(self, *args, **kwargs): | |||||||||
self.headers["User-Agent"] = user_agent() | ||||||||||
|
||||||||||
# Attach our Authentication handler to the session | ||||||||||
self.auth = MultiDomainBasicAuth(index_urls=index_urls) | ||||||||||
self.auth = auth_class(index_urls=index_urls) | ||||||||||
|
||||||||||
# Create our urllib3.Retry instance which will allow us to customize | ||||||||||
# how we handle retries. | ||||||||||
|
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.
As an user, I would find this to be confusing.
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.
Not sure what's confusing about this. It's quite common for options to have multiple names.
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 find
auth
andauth-plugin
to have different first-impression meaning, but I don't represent all users so no worries, stick to what you think is best.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.
Thanks for clarifying. It's similar to the '--src' option, which has three longer names: '--source', '--source-dir', '--source-directory'. The longer the name - the more informative and clear it is. Shorter names are easier to type. Let's see if others find it confusing, too.
@McSinyx Don't worry. Your thoughts and perceptions are welcome. Thanks for thinking with me.