Use URL-encoded username for keyring if no URL-encoded password

[BakerNet]

Summary

Currently, the middleware will not check keyring or netrc if there is any URL-encoded credentials. pip will check keyring and netrc if there is no URL-encoded password, and use the URL-encoded username for keyring. This updates the middleware to match pip functionality in this way.

See issue:

• How to set username for keyring cli based pip authorization (Azure artifacts)  #2563

Test Plan

See included unit tests

Note: Introduced cfg-branching functionality to get_keyring_subprocess_auth in order to bypass subprocess calls in unit tests.

[BakerNet]

Flaky tests? Only change in my branch was a comment and failing tests are in unrelated parts of the codebase.
Unable to rerun failed checks on my end, btw.

[charliermarsh]

I think GitHub has been having issues today.

[zanieb]

This is on my to-do list, it's been slow because of all the cfg changes for testing — we've generally avoided doing something like this so far. Without an in-depth review, I'd say some sort of pluggable Provider interface is preferable.

[jenshnielsen]

@BakerNet Thanks for the PR. I can confirm that this resolved the direct issue. If I try to install a package it will now call keyring with the correct username allowing me to get a token as expected. However, it seems to hit a code path where this still fails when downloading transitive dependencies.

E.g. installing numpy works but installing scipy (that depends on numpy) fails like this:

DEBUG Adding transitive dependency: numpy>=1.22.4, <1.29.0
DEBUG No cache entry for: https://pkgs.dev.azure.com/someorg/_packaging/somefeed/pypi/simple/numpy/
DEBUG Request already has an authorization header with URL-encoded password: https://pkgs.dev.azure.com/someorg/_packaging/somefeed/pypi/simple/numpy/
error: HTTP status client error (401 Unauthorized) for url (https://pkgs.dev.azure.com/someorg/_packaging/somefeed/pypi/simple/numpy/)

[BakerNet]

@BakerNet Thanks for the PR. I can confirm that this resolved the direct issue. If I try to install a package it will now call keyring with the correct username allowing me to get a token as expected. However, it seems to hit a code path where this still fails when downloading transitive dependencies.

E.g. installing numpy works but installing scipy (that depends on numpy) fails like this:

DEBUG Adding transitive dependency: numpy>=1.22.4, <1.29.0
DEBUG No cache entry for: https://pkgs.dev.azure.com/someorg/_packaging/somefeed/pypi/simple/numpy/
DEBUG Request already has an authorization header with URL-encoded password: https://pkgs.dev.azure.com/someorg/_packaging/somefeed/pypi/simple/numpy/
error: HTTP status client error (401 Unauthorized) for url (https://pkgs.dev.azure.com/someorg/_packaging/somefeed/pypi/simple/numpy/)

Thanks for testing and sharing! I'm pretty sure I know what's going on here and will get a fix in later today.

[BakerNet]

This is on my to-do list, it's been slow because of all the cfg changes for testing — we've generally avoided doing something like this so far. Without an in-depth review, I'd say some sort of pluggable Provider interface is preferable.

I have an idea to propose:
We could make the keyring check function a method on the KeyringProvider enum, and add a clap-skipped enum variant for test suite.
So instead of using cfg(test) we would match self

#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)]
#[cfg_attr(feature = "clap", derive(clap::ValueEnum))]
pub enum KeyringProvider {
    /// Will not use keyring for authentication
    #[default]
    Disabled,
    /// Will use keyring CLI command for authentication
    Subprocess,
    // /// Not yet implemented
    // Auto,
    // /// Not implemented yet.  Maybe use <https://docs.rs/keyring/latest/keyring/> for this?
    // Import,
    #[cfg_attr(feature = "clap", clap(skip))]
    TestStub,
}

in method:

let output = match self {
    Self::Subprocess => match Command::new("keyring")
        ... ,
    Self::Disabled => Ok(None),
    Self::TestStub => Ok(Some("mypassword".to_string())),
}

[BakerNet]

@jenshnielsen would you mind testing this again to check if my fix works on your end when you have time?

[jenshnielsen]

@BakerNet Thanks this does seem to resolve the issue. I can install packages including their dependencies now. I am still seeing intermittent 401 errors that go away if I rerun the same command. I have not been able to figure out exactly which code path triggers it or if its even related

[zanieb]

I'm tackling this in a more comprehensive overhaul over in #2976 if you're interested in looking at it.

[zanieb]

Thanks for taking the time to put up a pull request, this was really helpful for understanding the desired behavior. I'm closing in favor of #2976 which makes some other changes.