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

Support authentication tokens for uploading to Hackage #9058

Merged
merged 15 commits into from
Sep 15, 2023

Conversation

SebTee
Copy link
Collaborator

@SebTee SebTee commented Jun 22, 2023

Adds support for authenticating uploads using authentication tokens, as discussed in issue #6738.

A new flag --token (-t) has been created. Token authentication takes precedence over username and password meaning that, if a token is set, the username and password flags are ignored.

The token is included in a HTTP header as follows: Authorization: X-ApiKey [token].

QA Notes

These tests must be run using http-transport: curl, wget, powershell, and plain-http.

  • Calling cabal upload -t [valid-token] [./path/to/package.tar.gz] should successfully upload the package to Hackage.

    • The user shouldn't be prompted to enter a username and password.
  • Calling cabal upload -t [invalid-token] [./path/to/package.tar.gz] should fail to upload the package to Hackage.

    • A HTTP 401 error should be returned with the message "Bad auth token".
    • The user shouldn't be prompted to enter a username and password.
  • Calling cabal upload -t [invalid-token] -u [username] -p [password] [./path/to/package.tar.gz] should fail to upload the package to Hackage.

    • Authentication token takes precedence over username and password.
    • A HTTP 401 error should be returned with the message "Bad auth token".
  • Calling cabal upload -t should output a cabal error message reading Error: cabal: option `-t' requires an argument TOKEN.

  • Calling cabal upload [./path/to/package.tar.gz] should default to username and password authentication.

    • If username and password are not set in the config file, prompt the user for a username and password.
    • If username and password are set in the config file, cabal will attempt to upload the package using the username and password for authentication.

Please include the following checklist in your PR:

Bonus points for added automated tests!

Add token flag. If a token is set ignore the username and password.
The token is passed to Hackage in the  Authorization header.
@SebTee
Copy link
Collaborator Author

SebTee commented Jun 22, 2023

Token authentication is working for cabal upload. I'm currently working on getting token authentication working for cabal report.

doc/cabal-commands.rst Outdated Show resolved Hide resolved
@SebTee
Copy link
Collaborator Author

SebTee commented Jun 25, 2023

I've been having a couple issues with cabal report. It doesn't read the token, username or password from the config, you have to set them in the command line. This is also the case for username and password in the cabal-install version 3.10.1.0 release. I've not been able to find an issue for this, does one exist?

The other issue I'm encountering is that I can't seem to work out how to create a build report that cabal report can parse and upload to Hackage. My changes so far allow you to set a token that will be passed to the postHttp function if a build report is found and parsed. But, since I don't have a parsable build report to upload, I've not been able to fully test this.

@andreabedini
Copy link
Collaborator

IIRC there's a hidden and undocumented remote-build-reporting field. You can find it in the code: https://github.com/search?q=repo%3Ahaskell%2Fcabal%20remote-build-reporting&type=code

@gbaz
Copy link
Collaborator

gbaz commented Jun 25, 2023

I think its fine to not worry about testing the report feature. It was sort of stillborn if I recall. I double checked and the hackage builders generates reports as part of the build, but uses its own custom upload machinery.

@SebTee SebTee linked an issue Jun 28, 2023 that may be closed by this pull request
@SebTee SebTee changed the title WIP Support authentication tokens for uploading to Hackage Support authentication tokens for uploading to Hackage Jun 28, 2023
@SebTee SebTee marked this pull request as ready for review June 28, 2023 17:03
@SebTee SebTee requested a review from ffaf1 July 1, 2023 14:19
@ffaf1
Copy link
Collaborator

ffaf1 commented Jul 13, 2023

One way to meaningfully test this would be to have an monad abstracting over IO, which is not the case.

QA notes are very good, but is there any test hackage server we can use instead of having QA people having to upload something to Hackage.

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

Powershell has to have the Authorization token set in the Header
dictionary parameter. Some headers (e.g. User-Agent) have to be set as
a request parameter.
@Mikolaj
Copy link
Member

Mikolaj commented Sep 9, 2023

@gbaz: could (one of the) Hackage maintainers review this PR as well? BTW, I understand that Hackage is immutable, so the QA tests need to upload real sensible packages, not test packages, do they? Or is it fine, because no --publish is used and so these would be candidates? But if so, does it test enough? Are more errors likely get exposed with --publish?

@gbaz
Copy link
Collaborator

gbaz commented Sep 11, 2023

Its fine to test this without --publish, and just upload candidates.

@Mikolaj
Copy link
Member

Mikolaj commented Sep 12, 2023

OK, fine.

I'm re-running the failed CI jobs, since the failure is probably due to the unix package breakage.

@Mikolaj
Copy link
Member

Mikolaj commented Sep 12, 2023

CI passed. @SebTee: once you are satisfied with the state of the PR, please kindly add label squash+merge_me and in 2 days of inactivity the PR is going to be merged.

@SebTee SebTee added squash+merge me Tell Mergify Bot to squash-merge and removed attention: needs-review labels Sep 12, 2023
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 14, 2023
@mergify mergify bot merged commit a0d815c into haskell:master Sep 15, 2023
46 checks passed
@SebTee SebTee deleted the upload-token-auth branch September 16, 2023 11:28
BinderDavid pushed a commit to BinderDavid/cabal that referenced this pull request Sep 19, 2023
* Add token authorization for cabal upload

Add token flag. If a token is set ignore the username and password.
The token is passed to Hackage in the  Authorization header.

* Add token flag to upload documentation

* Add token authentication for cabal report

* Update auth token documentation and changelog

* Add token flag to config integration tests

* Add auth token header to plain-http transport

* Add documentation and reduce wildcard usage

Use Nothing in pattern matching instead of wildcards.

* Add auth token headers to wget and powershell

* Fix auth token header for powershell transport

Powershell has to have the Authorization token set in the Header
dictionary parameter. Some headers (e.g. User-Agent) have to be set as
a request parameter.

* Fix code formatting to comply with fourmolu
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cabal-install: cmd/upload 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
8 participants