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

Support custom authentication handlers for private pypi. #8250

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/4475.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Support for custom authentication handlers via --auth-plugin option.
32 changes: 32 additions & 0 deletions src/pip/_internal/cli/cmdoptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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',
Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I find auth and auth-plugin to have different first-impression meaning, but I don't represent all users so no worries, stick to what you think is best.

Copy link
Author

@fedorbirjukov fedorbirjukov May 17, 2020

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.

dest='auth_class',
type='choice',
metavar='plugin',
default=None,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might want to have MultiDomainBasicAuth here.

Copy link
Author

Choose a reason for hiding this comment

The 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
Copy link
Contributor

@McSinyx McSinyx May 16, 2020

Choose a reason for hiding this comment

The 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 #
##########
Expand Down Expand Up @@ -948,6 +979,7 @@ def check_list_path_option(options):
no_color,
no_python_version_warning,
unstable_feature,
auth_plugin,
]
} # type: Dict[str, Any]

Expand Down
1 change: 1 addition & 0 deletions src/pip/_internal/cli/req_command.py
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ def _build_session(self, options, retries=None, timeout=None):
retries=retries if retries is not None else options.retries,
trusted_hosts=options.trusted_hosts,
index_urls=self._get_index_urls(options),
auth_class=options.auth_class,
)

# Handle custom ca-bundles from the user
Expand Down
5 changes: 4 additions & 1 deletion src/pip/_internal/network/session.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
auth_class = kwargs.pop("auth_class", None)
if auth_class is None:
auth_class = MultiDomainBasicAuth
auth_class = kwargs.pop("auth_class", MultiDomainBasicAuth)

Copy link
Author

Choose a reason for hiding this comment

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

That does not work because this named argument is always provided.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I missed that. How about provide it with MultiDomainBasicAuth as suggested below?

Copy link
Author

Choose a reason for hiding this comment

The 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)

Expand All @@ -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.
Expand Down