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

WIP: Fix - Cannot access private repository using token #6795

Closed
wants to merge 4 commits into from

Conversation

@booleand
Copy link

commented Jul 27, 2019

This PR addresses a potential bug from 19.2 as discussed here.

Embedding a token in an index_url is not working.

For example:

pip install SomeProject --index-url="https://TOKEN@some.where/somerepo/"

throws an exception: pip._internal.exceptions.DistributionNotFound

Any guidance/feedback would be appreciated!

@chrahunt

This comment has been minimized.

Copy link
Member

commented Jul 27, 2019

Resolves #6796.

@pradyunsg

This comment has been minimized.

Copy link
Member

commented Jul 27, 2019

/cc @zooba since this was modified last in the keyring PR, which is what git bisect points to as well.

booleand added 2 commits Jul 27, 2019
Convert Nonetype arguments to string before calling HTTPBasicAuth
This is for forward compatibility with requests 3.0.0
(see Line 28 in 2e51624)
@booleand

This comment has been minimized.

Copy link
Author

commented Jul 28, 2019

Potential test case

def test_get_single_credential():
    auth = MultiDomainBasicAuth()
    get = auth._get_url_and_credentials

    assert get("http://token@example.com/path") \
        == ('http://example.com/path', 'token', None)
    assert auth.passwords['example.com'] == ('token', None)

To place in:

def test_get_credentials():

@zooba

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

Converting None to "" sounds reasonable to me. Though there may also be issues with the caching if we parse "" out of a URL but are expecting/looking for None.

I've paged all of this code out of my head already, I'm afraid, so without going through and relearning it I can't really offer much help, sorry :)

@pradyunsg

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Hehe. No worries @zooba! Thanks for chiming in.

I'm hoping I'll be able to make time to look into this today or tomorrow.

@pradyunsg

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

@booleand Would you mind if I pick this up from here?

@booleand

This comment has been minimized.

Copy link
Author

commented Jul 30, 2019

@booleand Would you mind if I pick this up from here?

@pradyunsg OK

Just want to mention a few things:

  1. Need to consider which conditional to use on this line as the behavior will be different:

if username is None and password is None:

I suspect there may be edge cases which will result in unexpected behavior. In particular when multiple calls are being made to a MultiDomainBasicAuth object.

  1. Here is a summary of changes in MultiDomainBasicAuth:
  • _get_new_credentials now returns a username even if it doesn't locate a password.

  • _get_url_and_credentials calls _get_new_credentials only when both username and password are empty. Though this may need to be reconsidered depending on requirements.

  • _get_url_and_credentials stores username and/or password when either is found.

  • __call__ now instantiates and calls a HTTPBasicAuth when either username or password is present. It also converts any NoneTypes to an empty string.

  1. Here is a potential test case for MultiDomainBasicAuth.__call__ :
def test_basic_auth_with_url_embedded_token():
    with patch.object(pip._internal.download.HTTPBasicAuth, '__call__') as mock_auth:
        auth = MultiDomainBasicAuth()
        req = MockRequest("http://token@example.com/path")
        auth(req)
    assert mock_auth.called_with('token', '')
@pradyunsg

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Thanks @booleand!

consider which conditional to use on this line

I decided to preserve that conditional as is, and modified what _get_url_and_credentials would return. The guard, as it stands, seems to be correct to me.

summary of changes

Thank you -- these helped. :)

potential test case

I ended up augmenting existing tests: 0832006.

@lock lock bot added the S: auto-locked label Sep 3, 2019

@lock lock bot locked as resolved and limited conversation to collaborators Sep 3, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.