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.

Sign up for GitHub

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

Jump to bottom

Add cli flag --force-keyring #11029

Closed
wants to merge 5 commits into from
Closed

Add cli flag --force-keyring #11029

wants to merge 5 commits into from

Conversation

Darsstar
Copy link
Contributor

@Darsstar Darsstar commented Apr 12, 2022 •
edited by pradyunsg

Fixes #11020
Fixes #11658

Currently, Pip is conservative and does not query keyring at all when --no-input is used because the keyring might require user interaction such as prompting the user on the console.

These commits add a flag to pinky swear that the configured keyring backend does not require any user interaction. It defaults to False, making this opt-in behaviour.

Tools such as Pipx and Pipenv use Pip and have their own progress indicator that hides output from the subprocess Pip runs in. They should pass --no-input in my opinion, but Pip should provide some mechanism to still query the keyring in that case. Just not by default. So here we are.

@Darsstar Darsstar mentioned this pull request Apr 12, 2022
Closed
1 task
@pfmoore
Copy link
Member

pfmoore commented Apr 12, 2022

If you supply the flag, but the keyring backend does prompt for user input (somehow, maybe via a GUI), what happens? I can imagine people using this flag getting very confused when the pip process "hangs" and reporting issues. In my experience, authentication prompts are notorious for "mostly" not needing user interaction, but then unexpectedly popping up prompts. The git client definitely does this - upgrading git often causes re-prompts, for example. And a keyring that periodically prompts for security reasons seems perfectly plausible to me. So it would be easy for a user to think that it's OK to supply this flag, but be wrong.

Also, the paragraph in the documentation about pipx/pipenv isn't at all clear to me. It seems to suggest that if you use those tools, you should always set this flag - which frankly seems like the wrong message to be giving.

Maybe this would be better handled as a --force-keyring flag, which overrides pip's current behaviour of disabling the keyring on --no-input. That's pretty easy to document ("Always query the keyring, regardless of pip's --no-input option. Note that this may cause problems if the keyring expects to be able to prompt the user interactively and no interactive user is available.") and puts the responsibility completely on the user to ensure that their keyring backend works as expected.

@Darsstar
Copy link
Contributor Author

Darsstar commented Apr 12, 2022 •
edited

That does sound like a more sensible name and way to document the feature.

Time for a break tohopefully quicky stop being disapointed in myself not for realizing this myself after @uranusjr suggested --force-system-keyring. It was staring me right in the face and I wan't able to see it because I concentrated too much on system... Sorry!

@Darsstar Darsstar force-pushed the keyring-does-not-prompt branch 2 times, most recently from 89a4822 to 94a0941 Compare April 18, 2022 11:19
@Darsstar Darsstar changed the title Add cli flag --keyring-does-not-prompt Add cli flag --force-keyring Apr 18, 2022
@Darsstar
Copy link
Contributor Author

I renamed the flag to --force-keyring and hopefully improved the documentation changes.

@uranusjr
Copy link
Member

uranusjr commented Apr 18, 2022 •
edited

The flag name isn’t changed in documentation. Edit: Need to change the config value and environment variable name as well.

@Darsstar
Copy link
Contributor Author

The flag name isn’t changed in documentation. Edit: Need to change the config value and environment variable name as well.

Woops. Fixed it.

uranusjr
uranusjr approved these changes Apr 19, 2022
View reviewed changes
Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

Implementation looks good. Don’t have much opinion on the design, leaving it for others to comment on.

@Darsstar Darsstar mentioned this pull request Apr 20, 2022
Closed
@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label May 25, 2022
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label May 30, 2022
@Darsstar
Copy link
Contributor Author

leaving it for others to comment on.

@uranusjr It has been two months without any sign of Pip contributors doing so without any asking them to. Would pinging people or a team be reasonable at this point? (The latter is not something I can do as far as I can tell.)

@pfmoore
Copy link
Member

pfmoore commented Jun 20, 2022

I'm much the same as @uranusjr - the implementation looks OK, but I don't have much opinion on the design. Basically, I don't mind if someone else merges this, but I won't do so myself.

@Darsstar Darsstar mentioned this pull request Jun 27, 2022
Closed
3 tasks
@uranusjr uranusjr added this to the 22.3 milestone Aug 12, 2022
@Darsstar Darsstar mentioned this pull request Aug 21, 2022
Closed
@Darsstar Darsstar mentioned this pull request Sep 2, 2022
Merged
2 tasks
@pfmoore
Copy link
Member

pfmoore commented Oct 15, 2022

Moving this to the 23.0 milestone.

@pfmoore pfmoore modified the milestones: 22.3, 23.0 Oct 15, 2022
@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label Nov 10, 2022
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Nov 11, 2022
@Darsstar Darsstar mentioned this pull request Dec 13, 2022
Closed
1 task
@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label Dec 14, 2022
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Dec 14, 2022
Darsstar and others added 3 commits December 14, 2022 11:18
@Darsstar
Add failing test for the use case of supplying --no-input and having …
51475ec 
…keyring queried for credentials.

Since there is currently no possible way to get pip to query the keyring when --no-input is used I am not marking the new scenario xfail. That should change once there is some way to do that.
Add cli flag --force-keyring.
497761c 
Currently, Pip is conservative and does not query keyring at all when `--no-input` is used because the keyring might require user interaction such as prompting the user on the console.

This commit adds a flag to force keyring usage if it is installed. It defaults to `False`, making this opt-in behaviour.

Tools such as Pipx and Pipenv use Pip and have their own progress indicator that hides output from the subprocess Pip runs in. They should pass `--no-input` in my opinion, but Pip should provide some mechanism to still query the keyring in that case. Just not by default. So here we are.
@Darsstar
Parameterized tests/functional/test_install_config.py::test_prompt_fo…
629de94 
…r_keyring_if_needed to add a few --no-input and --force-keyring scenarios
@Darsstar Darsstar mentioned this pull request Dec 19, 2022
Closed
@Darsstar Darsstar mentioned this pull request Jan 5, 2023
Merged
@pradyunsg
Copy link
Member

pradyunsg commented Jan 9, 2023 •
edited

I don't have the bandwidth to review/decide what to do on this PR and, TBH, I'm not too sure about breaking behaviour change here without a migration period (x-ref https://pip.pypa.io/en/stable/development/release-process/#deprecation-policy). What do we want to do here folks?

@Darsstar
Copy link
Contributor Author

Darsstar commented Jan 9, 2023 •
edited

If I do as @uranusjr suggests in #11698 then this PR will become irrelevant and only --keyring-provider with the default of auto (current behaviour) should not require a deprecation period as far as I can tell.

I can have that refactor done in the next couple of days. Not that that will magically give you more bandwith to deal with that PR... Could you at least take a quick look at #11658 which is about a tiny bug in new keyring related functionality on HEAD?

@pradyunsg
Copy link
Member

pradyunsg commented Jan 9, 2023 •
edited

only --keyring-provider with the default of auto (current behaviour) should not require a deprecation period as far as I can tell.

Oh, this sounds great and I agree. :)

Could you at least take a quick look at #11658 which is about a tiny bug in new keyring related functionality on HEAD?

Have added it to the 23.0 milestone for now. That's a relatively easy fix, I think but if we can cover that as part of this PR, that works for me too. :)

@Darsstar
Copy link
Contributor Author

only --keyring-provider with the default of auto (current behaviour) should not require a deprecation period as far as I can tell.

Oh, this sounds great and I agree. :)

I'm done with the refactoring.

I'm going to selfishly leave this PR open for now since this one is on de 23.0 milestone and the other is not so it serves as a reminder.

@pradyunsg pradyunsg modified the milestones: 23.0, 23.1 Jan 28, 2023
@pradyunsg
Copy link
Member

Given the lack of a response from the other maintainers here, and my lack of familiarity with this chunk of logic; I've extracted #11759 to fix the issue blocking the upcoming release based on the fix by @Darsstar related to that here.

@pradyunsg pradyunsg mentioned this pull request Jan 28, 2023
Merged
@Darsstar
Copy link
Contributor Author

Closing in favor of #11698

@Darsstar Darsstar closed this Jan 30, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@q0w q0w q0w left review comments

@uranusjr uranusjr uranusjr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
23.1
6 participants
@Darsstar @pfmoore @uranusjr @pradyunsg @q0w @pypa-bot