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

Fix handling of tokens (single part credentials) in URLs #6818

Merged
merged 6 commits into from
Aug 4, 2019

Conversation

pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Jul 30, 2019

Closes #6796

Supersedes and closes #6795

@pradyunsg pradyunsg added this to the 19.2 milestone Jul 30, 2019
@pradyunsg pradyunsg self-assigned this Jul 30, 2019
@pradyunsg pradyunsg force-pushed the fix/URL-authentication-handling branch from 6abc49d to 0832006 Compare July 30, 2019 14:57
@pradyunsg pradyunsg changed the title Fix URL authentication handling Fix URL credential handling Jul 30, 2019
@pradyunsg pradyunsg force-pushed the fix/URL-authentication-handling branch 2 times, most recently from 5265af7 to 6dd087b Compare July 30, 2019 15:32
@pradyunsg
Copy link
Member Author

CI driven development! \o/

news/6795.bugfix Outdated
@@ -0,0 +1 @@
Fix bugs in handling of credentials in URLs.
Copy link
Member

Choose a reason for hiding this comment

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

I would be more specific by mentioning the case of “a username in the URL with no password (e.g. a token)” since that is the case that everyone has been reporting and would want to know is fixed when reading the release notes. Also, if it’s too vague people might think it might also include the quoting issue, which was also reported but we’re not fixing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed.

@pradyunsg pradyunsg changed the title Fix URL credential handling Fix handling of tokens (single part credentials) in URLs Jul 31, 2019
@pradyunsg
Copy link
Member Author

Could someone else review this PR? I'm inclined to merge it but I just wanna make sure that no one has any outstanding issues here.

To that end, if no one raises any concerns by Sunday morning IST, I'll merge this (and make a pip 19.2.2 release the next week).

@booleand
Copy link

booleand commented Aug 3, 2019

@pradyunsg There are commits which show me as the author, yet I did not make the changes.

This could cause confusion when trying to maintain the code base. For example if someone needed to track down the original author of a commit.

This applies to all of ccac31a and b5b70ad.

It also applies to the commit message of 5036631 which forms part of the commit. The original commit is booleand@d07b7a3

@pradyunsg
Copy link
Member Author

Ah, yes. I rebased off your PR, which is why git credited you as the author.

If you mind, I'm happy to change things up to remove the attribution.

@booleand
Copy link

booleand commented Aug 3, 2019

no worries.

Yes, it makes sense to remove the attribution.

@cjerdonek
Copy link
Member

To that end, if no one raises any concerns by Sunday morning IST, I'll merge this (and make a pip 19.2.2 release the next week).

@pradyunsg Could you also include the fix for issue #6804 (PR #6827) when you release, which was the other 19.2 regression? (I'm assuming the PR can be merged by then as the fix is trivial.)

Also, since there have been 19.3-specific changes to master since the last release, I'm wondering if the fixes should be cherry-picked prior to release (to minimize the chance of introducing new regressions).

@pradyunsg
Copy link
Member Author

I'm wondering if the fixes should be cherry-picked prior to release (to minimize the chance of introducing new regressions).

Yep yep. I'll be cherry-picking the specific regressions that we want to fix; not releasing off master.

The earlier bugfix releases blocking master or going straight off master, was mostly because I was being lazy. :)

@pradyunsg pradyunsg force-pushed the fix/URL-authentication-handling branch from ef7d429 to 0a6c1c7 Compare August 4, 2019 06:58
@pradyunsg
Copy link
Member Author

Doing a squash-merge to make cherry-picking easier.

@pradyunsg pradyunsg merged commit 7d29841 into pypa:master Aug 4, 2019
@pradyunsg pradyunsg deleted the fix/URL-authentication-handling branch August 4, 2019 14:26
pradyunsg added a commit to pradyunsg/pip that referenced this pull request Aug 11, 2019
@lock lock bot added the auto-locked Outdated issues that have been locked by automation 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.
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pip >=19.2 cannot install packages via custom URLs with "one element" auth
4 participants