Implement digest authentication middleware behind a feature flag

[PabloMansanet]

In #392 , @algesten mentioned that it could be possible to consider default middleware for common features like Digest authentication. Here's a simple implementation, in case it's general enough to add to the main crate.

Some thoughts:

• I realized the structure of the project is pretty nice and flat, so it felt wrong to create a subfolder for particular middlewares. I ended up adding this as a feature-flagged embedded module under digest. Is this a good approach?
• I'm not sure about using external services like httpbin for unit tests, but I saw other tests doing a similar thing so I went with what seemed to be the simplest approach. I'm happy to take directions on how to mock this instead to fully test it offline.
• The recursive solution helps transparently navigate the "back and forth" between server and client without needing any Arc<Mutex<>> state in the middleware itself, but this may be erring on the "too smart" side. I don't directly see any problems it might cause but I could definitely be missing some.

[strohel]

For what it's worth, looks good to me!

Today I've learned that cases in stc/test/* are called integration tests, so calling external sites may be actually appropriate there.

[algesten]

@PabloMansanet welcome to ureq!

This looks good, and I do welcome that we have some standard middleware in the main crate. I left a couple of notes in the code and here are some more.

• Please make changes .github/workflows/test.yml to ensure the new feature flag is being automatically tested.
• Similarly please update test.sh (this is what we use for local dev to roughly do what github actions do).
• We need to update lib.rs documentation for feature flags (and run cargo readme to generate a new README.md)

---

Bigger picture discussion, maybe @jsha could input?

• Feature flags for middleware are good. No need to compile them if they are not wanted.
• These feature flags are different to the others we have because they require the user to both enable the feature flag AND configure the agent with the middleware.
• Maybe we should have a separate section in README.md detailing how to use optional middleware?
• Since we're likely to have a couple more of these. I wonder if we should group them by some prefix. Like mw-digest-auth, mw-basic-auth

[algesten]

Look in the crate for how we construct doc tests against httpbin.org, but actually wrapping them to also work offline.

It would be nice if this test actually checked the returned headers (from httpbin.org) to verify the digest worked as expected.

[PabloMansanet]

@algesten thanks for the hint! I've looked into it, but I'm still not sure how it works. I see that those doctests call the function ureq::is_test(true), but other than that they seem to be contacting httpbin.org normally. Am I missing something? Do you have a file/line number for an example of that httpbin.org wrapping? Thanks!

[PabloMansanet]

Thanks for the review @algesten. All the proposed changes make sense so I'll ping when I'm done with them 🙂

[PabloMansanet]

While testing this middleware on the field, we've noticed an issue that might be difficult to solve with the current Middleware API.

Basically, while this works well for GET requests, anything with a payload is tricky because the middleware has no access to the payload from what I can see. Starting a request with send_form will send the payload without authorization, and then this middleware's recursive call to send will not have the payload available at all.

Is it conceivable to solve this kind of problem through a change in the Middleware API? I imagine the limitations around the current payload are due to Payload::Reader, but I wonder if, for all other Payload enum variants, it would be possible for middlewares to observe the payload.

There was a similar issue around 307/308 responses, and the conclusion seems to be that PUT/POST/DELETE "retries" have to be handled at application level. Is this still the expectation, or can we possibly solve this at the middleware level for non-stream payloads?

[algesten]

Hey! Thanks for fixing those things!

There was a similar issue around 307/308 responses, and the conclusion seems to be that PUT/POST/DELETE "retries" have to be handled at application level. Is this still the expectation, or can we possibly solve this at the middleware level for non-stream payloads?

As you correctly pointed out, this is very similar to handling 307/308 with body. I'm trying to think if there is some quick win here without handling streaming bodies.

let response = next.handle(request.clone())?;

Here we would need something else, maybe something along the lines of:

// Buffer request bodies up to 10k
request.resend_buffer(10_000);

let response = next.handle(request)?;

...

if let Some((mut request, body)) = response.take_resend() {
    request.set("Authorization", "Bar");
    request.resend(body);
}

The idea is that take_resend will only return something if resend_buffer was set on the request and the request body is smaller than that buffer. body above would be some opaque type ResendBody that can only be used as an argument to request.resend().

[algesten]

Oh for the failed lint, do you mind running cargo fmt?

[PabloMansanet]

Done! (running fmt, I mean). Thanks for the response @algesten . I see #473 is in the works, which will likely unblock me from working on this one, so I will wait for that to progress :)

[algesten]

No activity. Feel free to reopen or start new PR.

[algesten]

Didn't mean to close this.

[algesten]

Closing this since we're moving to 3.x