savonrb / httpi

Common interface for Ruby's HTTP clients
http://httpirb.com
MIT License
301 stars 151 forks source link

Reimplement Rack::Auth::Digest in tests #250

Open pcai opened 2 months ago

pcai commented 2 months ago

background: https://www.rfc-editor.org/rfc/rfc2617

rack 3.0 deprecates the server-side implementation of HTTP digest authentication (Rack::Auth::Digest) and 3.1 removes it. the old implementation is here:

https://github.com/rack/rack/blob/3-0-stable/lib/rack/auth/digest.rb

Our integration tests relied on this and start failing when bundled with rack 3.1 because the class is missing.

Fortunately, the implementation is not used in the library, only tests, so we'll just skip the tests for now. This will allow us to loosen the deps so that httpi can be used with rack 3.1

ruyrocha commented 2 months ago

Is this still an issue? I'm afraid I got no errors while running the integration tests:

~/G/httpi (main)> bundle exec rspec spec/integration/

Randomized with seed 46340
....D, [2024-07-16T00:40:12.827309 #50113] DEBUG -- : Server does not support NTLM/Negotiate. Trying NTLM anyway
.............................*................../home/ruy/.asdf/installs/ruby/3.3.4/lib/ruby/gems/3.3.0/gems/curb-0.9.11/lib/curl/easy.rb:67: warning: undefining the allocator of T_DATA class Curl::Multi
......./home/ruy/GitHub/httpi/lib/httpi/adapter/curb.rb:41: warning: undefining the allocator of T_DATA class Curl::Upload
................................................

Pending: (Failures listed here are expected and do not affect your suite's status)

  1) HTTPI::Adapter::HTTP http requests supports chunked response
     # Needs investigation
     # ./spec/integration/http_spec.rb:99

Finished in 16.83 seconds (files took 0.165 seconds to load)
107 examples, 0 failures, 1 pending

Randomized with seed 46340

Coverage report generated for RSpec to /home/ruy/GitHub/httpi/coverage. 780 / 979 LOC (79.67%) covered.
```shell
 ~/G/httpi (main)> g diff

spec/integration/curb_spec.rb:83: describe HTTPI::Adapter::Curb do 
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
│ 83 │      end                                                                                                            │ 83 │      end
│ 84 │                                                                                                                     │ 84 │
│ 85 │      # Rack::Auth::Digest is removed in Rack 3.1                                                                    │ 85 │      # Rack::Auth::Digest is removed in Rack 3.1
│ 86 │      xit "supports digest authentication" do                                                                        │ 86 │      it "supports digest authentication" do
│ 87 │        request = HTTPI::Request.new(@server.url + "digest-auth")                                                    │ 87 │        request = HTTPI::Request.new(@server.url + "digest-auth")
│ 88 │        request.auth.digest("admin", "secret")                                                                       │ 88 │        request.auth.digest("admin", "secret")
│ 89 │                                                                                                                     │ 89 │

spec/integration/httpclient_spec.rb:80: describe HTTPI::Adapter::HTTPClient do 
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
│ 80 │    end                                                                                                              │ 80 │    end
│ 81 │                                                                                                                     │ 81 │
│ 82 │    # Rack::Auth::Digest is removed in Rack 3.1                                                                      │ 82 │    # Rack::Auth::Digest is removed in Rack 3.1
│ 83 │    xit "supports digest authentication" do                                                                          │ 83 │    it "supports digest authentication" do
│ 84 │      request = HTTPI::Request.new(@server.url + "digest-auth")                                                      │ 84 │      request = HTTPI::Request.new(@server.url + "digest-auth")
│ 85 │      request.auth.digest("admin", "secret")                                                                         │ 85 │      request.auth.digest("admin", "secret")
│ 86 │                                                                                                                     │ 86 │
 ~/G/httpi (main)> 
pcai commented 2 months ago

I made a PR to skip the tests for now so that the suite is still green, I linked to the commit above. If you undo that patch those tests will fail.

ruyrocha commented 2 months ago

@pcai yep, you're right. This comment has more context on why it was removed, and looks like the upgrade path is to fallback to basic auth. Are you OK with that?

pcai commented 2 months ago

I’d be more than happy to completely remove digest auth support entirely.

I initially wrote this issue as conservatively as possible but without rigorously evaluating whether that was the best course of action. It’s not even clear to me whether any code specifically exists to support it in httpi