AsyncAuthorizeRequest should consider being replaced with a bounded tower::Service

[lilyball]

Bug Report

Version

tower-http v0.2.5

Crates

tower-http

Description

tower_http::auth::AsyncRequireAuthorization uses a trait AsyncAuthorizeRequest to define the authorization. This trait is very nearly a refinement of tower::Service except it has no equivalent to tower::Service::poll_ready(). This means I can't easily implement async authorization in terms of an underlying tower::Service without manually checking poll_ready() and blocking my future on that, despite the wrapping AsyncRequireAuthorization itself advertising that it is ready. This breaks the whole ready tracking of tower::Service.

I think AsyncAuthorizeRequest needs its own poll_ready() method. It can have a default implementation that just returns Poll::Ready(Ok(())) so as to preserve compatibility. AsyncRequireAuthorization can then poll that in its own poll_ready(). This way I can properly delay readiness until my wrapped service is ready.

---

Beyond that, it would be nice to actually just go ahead and allow tower::Service impls with appropriate bounds in AsyncRequireAuthorization, so I can use existing service combinators to construct something of the appropriate shape without having to define a custom type just for the AsyncAuthorizeRequest impl. This could work by adding a blanket impl of AsyncRequireAuthorization for any T: tower::Service with the right shape¹. This would be a breaking change though as it would produce an error for any type that already implements both AsyncAuthorizeRequest and tower::Service. A nice benefit of this approach is I could then add arbitrary middleware to my auth implementation (such as tracing) or add additional validation with something like tower::ServiceExt::map_response() or populating auth error response bodies with tower::ServiceExt::map_err().

This could also be done in a backwards-compatible way by adding an adaptor that wraps a tower::Service and implements AsyncAuthorizeRequest (along with convenience methods on AsyncRequireAuthorization and AsyncRequireAuthorizationLayer), but this is a bit more awkward. This could be used as a transition plan until tower-http 0.3 though.

Footnotes

1. This would admittedly be mildly odd on the naming front since <T as Service>::Response would actually be a Request and <T as Service>::Error would be a Response but it should work just fine. ↩

[lilyball]

Another downside to not having poll_ready() is if I call it in my future and get back Poll::Ready(Err(_)) then I can't bubble the error up, my only choice is to return something like 500 Internal Server Error.

[davidpdrsn]

I'm curious what your use case is. Maybe just writing a custom middleware from scratch is easier? I'd like to avoid making AsyncAuthorizeRequest more complicated than it already is.

[lilyball]

I realized later after writing this issue that maybe I should just use a custom service impl. I initially saw AsyncAuthorizeRequest and thought "yes that's what I need" and it wasn't until I had implemented most of it that I realized I needed to call poll_ready(). So I filed this issue and wrote the poll_ready() into the future. But at this point I am in fact going ahead and converting this into a tower::Service, as there is no current ecosystem for composing AsyncAuthorizeRequest impls and so the only real benefit I get from using it is not having to write a layer type, which is pretty minor.

I still do think there's an elegance in replacing AsyncAuthorizeRequest with a bounded tower::Service as it's so darn close to being one already and I could then have middleware on the auth impl or use service combinators. But I concede that this is not a deficiency of AsyncAuthorizeRequest today, it's just a potential opportunity to do something neat.

[jplatte]

So while I think there's nothing that needs to be done here, I am curious: would there be any downsides to having this (with a default implementation)? The existing implementations would then just leave the function defaulted while custom ones could do other things, right?

[davidpdrsn]

I suppose not.

[lilyball]

I found a downside last night. The error type would need to be changed. The call to poll_ready() returns a potential error, and so if AsyncRequireAuthorization is polling two services it needs to be able to combine the errors.

I'm still trying to figure out the right solution in my custom service that I'm converting my auth to. Right now I'm leaning to having an error type parameter that defaults to Box<dyn StdError + Send + Sync + 'static> but since it's a parameter it can be overridden if something else works better. For example, if both the inner service and the auth service use Infallible it could be overridden to be Infallible. My thought here was installing this layer on an axum::Router would require it to be Infallible already and so that would force the error type accordingly if possible, and if not then I'll get an error and wrap it in axum::error_handling::HandleError. I'm just afraid this might be confusing.

The other thought I'm considering is just having a structured error type that distinguishes between inner service errors and auth service errors, and then implementing From<MyError<Infallible, Infallible>> for Infallible to make it easy to convert to Infallible if both service error types already are. This would require adding something like .map_err(Into::into) when using with axum of course. I really wish I could have the error type just be the service error if the auth error is Infallible (or vice versa), but I don't believe there's any way to do that.

The third thought I've had is to just either require the auth service to use Infallible as its error already, or to catch errors from it in poll_ready and cache that so it can return an Internal Service Error response on the call, but this is rather poor as it means you can't detect and handle these failures.

[lilyball]

Requiring the auth error type to be Infallible is the simplest strategy, it makes it roughly compatible with the current trait, but it has the same downsides as the third option. It also means using something like hyper::Client would require wrapping it in a tower::service_fn() as it has a real Error type but only ever returns that from call() (the service_fn() would skip call() entirely and just call Client::request instead). Which is to say, it's awkward, but at least would unlock the ability to use service middleware on the auth as long as it doesn't introduce its own errors.

[lilyball]

Oops, service_fn doesn't work because Client::request() still returns an error. So you'd have to then map the response type to remove the error manually.