keyring2.0 #11823
keyring2.0 #11823
Conversation
|
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
| if index_info: | ||
| index_url, _, index_url_user_password = index_info | ||
| logger.debug("Found index url %s", index_url) | ||
| def split_index_url_on_url_and_credentials(url): |
There was a problem hiding this comment.
Moved logic to a function to remove the comment
| @@ -266,27 +265,57 @@ def _get_new_credentials( | |||
| logger.debug("Found credentials in index url for %s", netloc) | |||
| return index_url_user_password | |||
|
|
|||
| # Get creds from netrc if we still don't have them | |||
| if allow_netrc: | |||
There was a problem hiding this comment.
We don't need flags allow_netrc/allow_keyring anymore
|
|
||
| return username, password | ||
|
|
||
| def _find_key_ring_credentials( |
There was a problem hiding this comment.
Order of getting keyring user:
- Username from artifact url
- Username from index url
- Username from pip.conf (or command line argument)
| return kr_auth | ||
|
|
||
| return username, password | ||
| kr_auth = get_keyring_auth(netloc, key_ring_user) |
There was a problem hiding this comment.
Maybe here we should change the order and first try to take credentials for some particular netloc.
| @@ -346,107 +375,4 @@ def __call__(self, req: Request) -> Request: | |||
| if username is not None and password is not None: | |||
| # Send the basic auth with this request | |||
| req = HTTPBasicAuth(username, password)(req) | |||
|
|
|||
| # Attach a hook to handle 401 responses | |||
| req.register_hook("response", self.handle_401) | |||
There was a problem hiding this comment.
Removed completely all logic that asks user to type username.
| @@ -199,18 +198,6 @@ def ask(message: str, options: Iterable[str]) -> str: | |||
| return response | |||
|
|
|||
|
|
|||
| def ask_input(message: str) -> str: | |||
There was a problem hiding this comment.
we use this function only for prompting username for keyring. So i removed them too.
| @@ -884,6 +884,13 @@ def _handle_config_settings( | |||
| help="Don't periodically check PyPI to determine whether a new version " | |||
| "of pip is available for download. Implied with --no-index.", | |||
| ) | |||
| default_key_ring_user: Callable[..., Option] = partial( | |||
There was a problem hiding this comment.
Maybe this argument could be usefull for case where companies use internal pypi. And for their users they just could add an additional section for pip.conf:
[global]
index-url = https://pypi-mirror.mangin.com/simple
extra-index-url = https://pypi-private.mangin.com/simple
default-key-ring-user = aleksandr.mangin
There was a problem hiding this comment.
Can this not work if the user name is added to the index URL?
index-url = https://aleksandr.mangin@pypi-private.mangin.com/simple(I know it does not work now but since we’re doing an overhaul I think we can make it work?)
There was a problem hiding this comment.
Thank you for your reply.
yep, i checked these scenarious:
- login in the index-url
- login in the config
- login in args.
Would we like to keep default_key_ring_user? I think it increase flexiability. And be honest for CI it's the best options. But if you disagree I could revert this change.
P.S. I just would like to know about oppinion of PyPi contibuters how we would like to fix this issue.
There was a problem hiding this comment.
P.S. When we'll agree on what should be done. I'll add all unit tests ;) I just don't want to do useless job.
There was a problem hiding this comment.
Personally I feel the extra config is slightly unintuitive. Yes it’s convenient if you know it’s there, but I suspect most people won’t know about it and would be confused seeing the default user kicking in unexpectedly. An explicit username in each URL is more obvious IMO.
There was a problem hiding this comment.
Okay, let's assume that we remove it.
And now we have two scenarios:
- Local user tries to install/remove a package
- Jenkins or any other CI integration tries to install/remove a package.
With first scenario everything is obvious:
- user can just create a file ~/.pip/pip.conf
- and add there something like this:
[global]
index-url = https://aleksandr.mangin@pypi-mirror.mangin.com/simple
extra-index-url = https://aleksandr.mangin@pypi-private.mangin.com/simple
- and add credentials to keyring.
With jenkins user the situation could be a little bit more difficult.
- Let's assume that we have a jenkins job that allows us to build some services. So we have many PyPi users.
- In this case we should receive from the world credentials that we are going to use with PyPi user
- Generate pip.conf file
[global]
index-url = https://system-userN@pypi-mirror.mangin.com/simple
extra-index-url = https://system-userN@pypi-private.mangin.com/simple
- Add credentials to keyring
I think it's a pretty hard to use keyring for system users. But with local users I agree we don't need this parameter.
Action: rollback changes with one more extra-parameter.
What do you think about adding one more source of credentials: Environment variables
- Let's assume that we have a jenkins job that allows us to build some services. So we have many PyPi users.
- In this case we should receive from the world credentials that we are going to use with PyPi user
- Set environment variable PIP_USER/PIP_PASSWORD
There was a problem hiding this comment.
If you’re going to use environment variables, you can just set the entire PIP_INDEX_URL value.
There was a problem hiding this comment.
Adding a bunch of configuration knobs to pip feels off to me, given that keyring already has support for discovering the username. Is #12748 sufficient?
github-actions
bot
added
the
needs rebase or merge
Hey there,
I would like to improve the integration with keyring. Now keyring requires typing username in some cases. Because by default keyring would like to know username and service to fetch credentials and in some cases we don't know username => we ask to type it.
Moreover in the current integration we use keyring only if we can't authorize using different way. In this case we always will generate 401 error on pypi repository. It could create some issues with monitoring on pypi repository's side. I removed this logic too.
My change allows to fetch username from:
I really believe that this change will simplify integration with internal pip for some companies that uses their own mirrors and would like to force authentication.
I'm looking forward for your feedback
TODO: