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

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

Open
pcai opened this issue Jul 6, 2024 · 4 comments
Open

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

pcai opened this issue Jul 6, 2024 · 4 comments

Comments

@pcai
Copy link
Member

pcai commented Jul 6, 2024

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
Copy link

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
Copy link
Member Author

pcai commented Jul 17, 2024

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
Copy link

@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
Copy link
Member Author

pcai commented Jul 17, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants