search

ferristseng / rust-ipfs-api

IPFS HTTP client in Rust
Apache License 2.0
246 stars 68 forks source link

Basic Authentication #116

Closed iamjpotts closed 1 year ago

iamjpotts commented 1 year ago

Add support for passing basic authentication on HTTP requests.

Useful for when an authenticating reverse proxy is in between the client and the IPFS HTTP api, such as Nginx configured with auth_basic - which this PR includes tests for.

Questions for maintainers:

  1. Placement of tests

I noticed that the ipfs_api crate is marked deprecated, but its also not typical to put integration tests in with examples. In this case, it might make sense to do so depending on the future of ipfs_api. Do you prefer the tests in the current location, or in ipfs-api-examples?

  1. Placement of basic auth struct member

Currently, credentials (an Option<(String, String)> containing username/password) is placed on the ActixBackend and HyperBackend structs, but there's also a GlobalOptions with comment "Options valid on any IPFS Api request" - which of these two locations (backends vs GlobalOptions) is the preferred place to set credentials?

iamjpotts commented 1 year ago

@ferristseng this is ready for review.

I've included a second commit to split up the CI into separate parallel jobs since the first commit adds integration tests that spin up and tear down Docker containers.

iamjpotts commented 1 year ago

Rebased the recent commits from master into this branch.

I'm exploring different rust testing libraries to look for ways to parameterize the larger, longer tests into smaller, faster ones.

iamjpotts commented 1 year ago

@ferristseng I used the test-case crate to split up the long-running test against each of the Docker image into separate tests, one per Docker image. This involved creating a new procedural macro crate ipfs-api-versions so the list of supported images could be maintained in a single location.

CI completes in just a few minutes.

ferristseng commented 1 year ago

Will review this when I have some more time over the holidays~

iamjpotts commented 1 year ago

Will review this when I have some more time over the holidays~

I'm interested in hearing @blackghost1987's feedback also if he offers any.

iamjpotts commented 1 year ago

The failure on Fedora is most likely an issue with the Docker client used by the tests.

I was going to say let's add a Fedora job to the CI, but Fedora isn't supported.

I'll build a VM in my environment to repro it and fix it.

iamjpotts commented 1 year ago

Which test (or tests) is failing on Fedora?

Also, text: "..." should be a JSON string. It's probably lengthy, but can you post/attach it here in raw form or as a gist?

iamjpotts commented 1 year ago

Which test (or tests) is failing on Fedora?

Also, text: "..." should be a JSON string. It's probably lengthy, but can you post/attach it here in raw form or as a gist?

Don't need the example anymore, already pushed a fix.

iamjpotts commented 1 year ago

@ferristseng updated and ready for you to try on Fedora again.