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

cabal-install: update curl transport to support Basic authentication #10089

Merged

Conversation

frasertweedale
Copy link
Contributor

@frasertweedale frasertweedale commented Jun 9, 2024

This PR updates the curl transport to support HTTP Basic authentication scheme. There are a
couple of preliminary small refactors. Best review the commits independently and in order.

Targeting branch 3.12 because I couldn't build master (GHC panic, don't worry about it).
It rebases cleanly onto master.

QA Notes

Ensure that authenticated operations using username and password against hackage.haskell.org succeed, when using the Curl transport.

Checklist

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • Manual QA notes have been included.
  • Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

@mergify mergify bot mentioned this pull request Jun 9, 2024
7 tasks
@frasertweedale frasertweedale force-pushed the fix/curl-transport-support-basic branch 3 times, most recently from 6f13072 to 9693c34 Compare June 9, 2024 12:54
@ulysses4ever
Copy link
Collaborator

I don't think we can merge it into 3.12. Please, target master.

@andreabedini andreabedini force-pushed the fix/curl-transport-support-basic branch from 9693c34 to 5fc9672 Compare June 10, 2024 08:44
@andreabedini andreabedini changed the base branch from 3.12 to master June 10, 2024 08:44
@andreabedini
Copy link
Collaborator

@frasertweedale I rebased your PR on top of master. I hope it's ok.

@frasertweedale
Copy link
Contributor Author

@andreabedini that's fine. Note that we do want to backport this change for maintenance releases on older branches (you know the reasons).

If you prefer a variant of the patch for backports with the minimal changes and without the refactors, I can prepare that. Or else, I suppose it lands in master first and then the backports can be created (is there automation for that?)

Copy link
Collaborator

@andreabedini andreabedini left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

@ffaf1 ffaf1 left a comment

Choose a reason for hiding this comment

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

good afaics

@ulysses4ever
Copy link
Collaborator

Please, disregard my previous message, I was wrong about the branch.

Let me put a label on this so that it gets merged.

@ulysses4ever ulysses4ever added the squash+merge me Tell Mergify Bot to squash-merge label Aug 8, 2024
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Aug 10, 2024
Extract a bunch of string equality checks for the URI scheme to
top-level functions.
"They're the same picture".  Thus, refactor the *transport supports
https* checks.
Allow the curl transport to use Basic authentication, if and only if
the url scheme is HTTPS (i.e. TLS will be used).  Retain the
existing behaviour (force Digest scheme) for insecure requests.

This change is required to support upcoming hackage-server changes.
The wget transport already supports Basic authentication.
@geekosaur geekosaur force-pushed the fix/curl-transport-support-basic branch from 36faf8b to 6364221 Compare August 22, 2024 01:07
@mergify mergify bot merged commit fb2ac8c into haskell:master Aug 22, 2024
50 checks passed
@frasertweedale frasertweedale deleted the fix/curl-transport-support-basic branch August 22, 2024 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants