Add support for pulling username from keyring subprocess provider

[jfly]

This fixes #12543.

Somewhat confusingly, when using using PIP_KEYRING_PROVIDER=import,
pip was able to fetch both a username and a password from keyring, but
when using PIP_KEYRING_PROVIDER=subprocess, it needed the username.

I had to rework the existing tests quite a bit to fit with this new
behavior, as it's now OK to ask for a username/password for an index
even if you don't have a username. This is why KeyringSubprocessResult
now subclasses KeyringModuleV2. While I was in here, I opted to remove
the "fixtures" values from KeyringModuleV2 by introducing this new
add_credential contextmanager. IMO, they were hurting the readability
of our tests: you had to jump around quite a bit to see what the
contents of the keyring would be during our tests. It also forced all
our tests to play nicely with the same fixtures values, which IMO was an
unnecessary constraint.

Note: per the discussion
here,
I've opted not to implement any "feature detection" to see if the
keyring subprocess supports --mode=creds. For the record, this
feature was added in
jaraco/keyring@7830a64,
which landed in keyring v25.2.0.

[jfly]

@Darsstar, I would love your feedback on this, as you seem to be the primary maintainer of pip's keyring support.

[Darsstar]

'Maintainer' is more than I am willing to claim, but I do keep an eye on the feature I contributed to, hopefully, ease the maintenance on it.

I could personally live with the breaking change. But feature detection would be nice.

The getcreds subcommand is actually even more recent, v25.2.1, so please don't use that for feature detection.

[jfly]

The getcreds subcommand is actually even more recent, v25.2.1

Ok, so I was a little confused. But I think you are too? It looks like jaraco/keyring#678 went through some iteration before getting merged up, and all the commits got included in the merge:

1. getcreds was introduced in jaraco/keyring@0a104c8
2. getcreds was removed and replaced with --mode=creds in jaraco/keyring@7830a64

Both commits have landed in keyring v25.2.0+, but commit 1 (the "getcreds" command) never actually landed in a release without commit 2 (the --mode=creds) command.

I could personally live with the breaking change. But feature detection would be nice. [...] please don't use that [getcreds] for feature detection.

Agreed. I guess I'll wait for feedback from a pip maintainer before I go to the effort of implementing feature detection.

[Darsstar]

But I think you are too?

Yeah... I would have sworn I saw a newer commit adding it 'back' after 25.2.0...

[jfly]

For the record, I'm aware of the test failures in test_keyring_cli_get_password. I can fix them, but would like to get feedback on the "do we need feature detection of the keyring subprocess?" question before I do, as it'll affect how I update that test to pass.

[pypa-bot]

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 master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

[jfly]

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 master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

Oops, I accidentally pushed a WIP commit to fix the tests.

Again, I'd like to get some feedback on the "do we need feature detection of the keyring subprocess?" question before I finalize this work, as it'll affect how I update that test to pass.

@Darsstar are you the right person to talk to about this?

[Darsstar]

No, I'm just a user. A user who is fine with the minor churn no feature detection would cause.

[jfly]

Urg, so this got ugly. I was hoping to use this nicer syntax for opening multiple context managers at once, but apparently it's not supported on Python 3.8:

Suggested change

	with keyring_subprocess.add_credential("example.com", "example", "!netloc"):
	with keyring_subprocess.add_credential(
	"http://example.com/path2/", "saved-user1", "pw1"
	):
	with keyring_subprocess.add_credential(
	"http://example.com/path2/", "saved-user2", "pw2"
	):
	with (
	keyring_subprocess.add_credential("example.com", "example", "!netloc"),
	keyring_subprocess.add_credential("http://example.com/path2/", "saved-user1", "pw1"),
	keyring_subprocess.add_credential("http://example.com/path2/", "saved-user2", "pw2"),
	):

I could rework this add_credential method to instead take an array of triplets or something, but IMO that makes the api kind of messy.

I'm inclined to leave this the way it is (and clean it up when we drop support for versions of python that don't support the above syntax), but I'm happy to approach this however folks prefer.

[jfly]

I found the pre-existing code hard to grok. This line in particular really confounds me: why does keyring_provider influence how we set up the test environment? I'd expect that to be only dependent upon keyring_provider_implementation (which is the change I've made in this PR).

[Darsstar]

After your refactoring keyring won't be importable when keyring_provider in [None, "auto"] and keyring_provider_implementation == "disabled". (first subcondition is false, second is true)
Which might be relevant because of:

        if keyring_provider_implementation == "disabled" or (
            not interactive and keyring_provider in [None, "auto"]
        ):
            request.applymarker(pytest.mark.xfail())

So the test should fail, so far so good.
I'm starting to suspect I did this so keyring would be installed when keyring_provider == "auto" and keyring_provider_implementation == "disabled" when what I actually was thinking about is the keyring_provider == "disabled" and keyring_provider_implementation == "import" case.

Maybe it is an idea to ask for verbose output and check for the Keyring provider set: lines to make that more explicit? Although, that maght be better as a seperate test.

[jfly]

After your refactoring keyring won't be importable when keyring_provider in [None, "auto"] and keyring_provider_implementation == "disabled".

Yes, this was intentional. Perhaps I'm misunderstanding these variable names? I read them as:

• keyring_provider_implementation: this is the way i'm going to set up my environment for this test (is keyring installed in this venv, is is discoverable via PATH in a different venv, or is it not installed at all)
• keyring_provider: this is the keyring provider i'm going to request when i invoke pip

So, it's intentional that if keyring_provider_implementation == "disabled", then yeah, keyring won't be imprtable.

But again, it's very possible I'm misunderstanding the intent of these tests. Guidance appreciated.

Maybe it is an idea to ask for verbose output and check for the Keyring provider set: lines to make that more explicit?

Yeah, I personally not in love with this elaborate usage of parameterize + xfail. Tests can fail for all sorts of reasons other than the expected reason! I think it would be better to assert on the expected failures. (IMO, that would be better to do in a separate PR)

[jfly]

I've completed the implementation and have gotten the tests passing.

@Darsstar if you're up for giving me a review, I'd sure appreciate it!