Add a Middlewares algebra that allows to modify Endpoint

[mleclercq]

Addresses #794 and part of #844

The Middlewares algebra defines methods to change the Request[A] and Response[B] in an Endpoint[A, B].
It also adds methods to add headers to Request and Response and add queryString to Request.

While the Middlewares algebra is separate from the Endpoints algebra, all Endpoints interpreters implements it. To do so, several interpreters had to be changed, especially the client ones in order to be able to retrieve the Request and Response from an Endpoint. For instance in akka-http-client the type of Endpoint is changed from A => Future[B] to:

case class Endpoint[A, B](request: Request[A], response: Response[B]) extends (A => Future[B]) {
  def apply(a: A): Future[B] = ...
}

The AuthenticatedEndpoints algebra demonstrates a usage of these new capabilities. The client and server interpreters for this algebra are independent from the underlying HTTP framework and only uses the new operations provided by Middlewares. Since AuthenticatedEndpoints provides similar features as the BasicAuthentication algebra, it has been deprecated.

[julienrf]

Thank you @mleclercq for this work!
This is interesting and this solves several issues. Please find my comments below.

[julienrf]

Would it be possible to keep BasicAuthentication not deprecated, but to implement it with Middlewares?

[julienrf]

Is the comment automatically indented by scalafmt? Is it possible to align it with the start tag?

[julienrf]

We should distinguish between returning Unauthorized and Forbidden.

When a client does not supply the Authorization header, he should get an Unauthorized response, and when he supplies invalid credentials, he should get a Forbidden response.

[mleclercq]

I'm not certain that the Forbidden status code means "invalid credentials". From experience, I think most servers would return a 401 Unauthorized status code if the client provides invalid credentials.

From https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/403:

The HTTP 403 Forbidden client error status response code indicates that the server understood the request but refuses to authorize it.

This status is similar to 401, but in this case, re-authenticating will make no difference. The access is permanently forbidden and tied to the application logic, such as insufficient rights to a resource.

So I think Forbidden reflects a lack of permission to perform the action. The user is properly authenticated but doesn't have the right to perform the action.

[julienrf]

Yes, that’s how it works in BasicAuthentication: when the request is processed by the server interpreter, it is rejected with an Unauthorized response in case the credentials are missing. If the credentials are provided, the endpoint implementation can signal that the authenticated user lacked the right to access the resource by returning None (in which case the server produces a Forbidden response).

[julienrf]

Why do we need to keep the request and response types here?

[bmeesters]

You need it to be able to implement mapEndpointResponse and mapEndpointRequest` required by the new algebra.

[julienrf]

Yes, but since type Response[A] = DocumentedResponse, the operation mapEndpointResponse could be implemented as a no-op, no?

[bmeesters]

I didn't think about that, but that makes sense.

[mleclercq]

I can change the Endpoint class to be:

  case class Endpoint[A, B](
      request: DocumentedRequest,
      response: List[DocumentedResponse],
      docs: EndpointDocs,
      securityRequirements: Seq[SecurityRequirement]
  )

But I don't think that mapEndpointResponse and mapEndpointRequest could be made as no-op as the final DocumentedEndpoint should reflect the changes made to the request or response.

The thing is that the Request and Response are "sliced and dices" in the DocumentedEndpoint so it would be pretty hard to implement mapEndpointResponse and mapEndpointRequest given a DocumentedEndpoint. So instead, the Endpoint keeps the Request and Response which makes the implementation of mapEndpointResponse and mapEndpointRequest trivial and the DocumentedEndpoint is a lazy val of Endpoint so it always reflects the result of these mappings.

[julienrf]

One concern I have is that we don’t really encapsulate the fact that if the Authorization header is missing, then the server returns an Unauthorized response, otherwise it forwards to the underlying endpoint logic.

IIUC, in the current implementation we always call the underlying endpoint logic. I believe it would be interesting to be able to intercept requests.

Maybe we need something different from mapRequest and mapResponse.

[mleclercq]

Indeed, a client could send a request without the Authorization header and a giant payload and the server would tries to decode the payload and only after reject the request because the header is missing. That's kind of a security vulnerability.

I guess, the implementations of addRequestHeaders and addRequestQueryString could be made such that they first extract the added headers/queryString and call the extraction of the rest of the request only if the former succeeded.

Edit: What I suggested above would not solve that problem. I remember at some point I played with a partialEndpointImplementation operation that takes a PartialFunction[A, B], but I guess even that wouldn't help since it still needs to extract the full A. I'll try to see how this can be addressed...

[julienrf]

I think it would be useful to control whether to call or not the underlying endpoint logic. A bit like what they do in Play, with the ActionFunction hierarchy.

[bmeesters]

Thanks for making this PR @mleclercq. Can you maybe process the review comments? Otherwise I also have some time tomorrow (or otherwise next week) to continue this PR. In case you are short on time.

[agboom]

Would it be sensible to keep the Future in this type? i.e.:

  type Request[A] = A => Future[HttpRequest]

When we handle a request with authentication, the HTTP credentials are validated using a separate microservice. With the old type we could leverage the Future to perform that validation via a separate and get the necessary account data.
I realize this is kind of an edge case, but it would make migration for us easier and since the Future was already there I can imagine that others may depend on this too.

I've pushed a working branch here, the change wasn't too impactful: https://github.com/simacan/endpoints4s/tree/middleware-take-two

[agboom]

I've played around with this a bit and really like the added methods for mapping endpoints and adding constituents! I've left some thoughts below about the details, but in general this PR will solve #794 🎉

[agboom]

Would it be possible to add accessors to this as well, e.g. for the statuscode, url etc.? This would enable us to implement #756 in a composable fashion

[mleclercq]

I'm not sure I see how adding accessors helps with #756.

Currenlty the Endpoint concrete types for client and server interpreters do not keep a reference to the EndpointDocs because it isn't used in anyway. So for these interpreters, mapDocs is a no-op. Only the openapi interpreter actually keeps a reference to EndpointDocs. So if we add accessors in the algebra, all interpreters would need to keep the reference to EndpointDocs.

[agboom]

Sorry, I should've been more clear. I didn't mean an accessor for EndpointDocs, but for other parts of the endpoint, such as the request url, the response statuscode etc. I can try it out in a separate PR or even as part of #756 if that's preferred, so it doesn't hold back this PR.

[codecov]

Codecov Report

Merging #847 (4438a83) into master (5d1095e) will decrease coverage by 1.03%.
The diff coverage is 51.64%.

@@            Coverage Diff             @@
##           master     #847      +/-   ##
==========================================
- Coverage   68.51%   67.48%   -1.04%     
==========================================
  Files         137      139       +2     
  Lines        3656     3847     +191     
  Branches       65       61       -4     
==========================================
+ Hits         2505     2596      +91     
- Misses       1151     1251     +100     

Impacted Files	Coverage Δ
...points4s/akkahttp/client/BasicAuthentication.scala	100.00% <ø> (ø)
...ala/endpoints4s/akkahttp/client/MuxEndpoints.scala	0.00% <0.00%> (ø)
...points4s/akkahttp/server/BasicAuthentication.scala	75.00% <ø> (ø)
.../main/scala/endpoints4s/akkahttp/server/Urls.scala	93.82% <0.00%> (-4.88%)	⬇️
...cala/endpoints4s/algebra/BasicAuthentication.scala	100.00% <ø> (ø)
...src/main/scala/endpoints4s/algebra/Responses.scala	87.50% <0.00%> (-12.50%)	⬇️
...ic/akkahttp-server/src/main/scala/sample/Api.scala	0.00% <0.00%> (ø)
...mples/basic/client/src/main/scala/sample/Api.scala	0.00% <ø> (ø)
...asic/http4s-server/src/main/scala/sample/Api.scala	0.00% <0.00%> (ø)
.../basic/play-server/src/main/scala/sample/Api.scala	0.00% <0.00%> (ø)
... and 46 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d1095e...4438a83. Read the comment docs.

[mleclercq]

Sorry for the delay for responding to the comments. I had a couple of very busy weeks...

I've rebased my branch on @agboom ones which includes the change proposed for akka-client (type Request[A] = A => Future[HttpRequest]).

I did the following changes:

• merge the Middleware algebras in the "main ones".
• rename the withHeaders/QueryString to addHeaders/QueryString as suggested by @agboom.

[julienrf]

Thank you @mleclercq!

This PR introduces several important changes, so we need to make sure this is the right way to go :)

A couple of things that I’m not super happy with:

• as mentioned in a comment above, the fact that we don’t control whether to call or not the logic of the endpoint to which we apply the middleware,
• the fact that we have three traits AuthenticatedEndpoints, AuthenticatedEndpointsServer, and AuthenticatedEndpointsClient (btw why don’t we need an AuthenticatedEndpointsOpenapi?)
  • maybe its possible to move everything in AuthenticatedEndpoints (that would imply using the same concrete type for UsernamePassword)

The operations you have introduced to manipulate requests and responses remember me of the discussion in #47 (so, it also solves a problem we had for some time).

Another thing middlewares should help us achieve is to implement #104. We should try to implement a “telemetry” middleware, as another validation.

[julienrf]

@mleclercq I have implemented my suggestion here, let me know what you think: mleclercq/endpoints@middleware-take-two...julienrf:just-authenticated-endpoints

I have left some comments in the commit: julienrf@a3aac49
An important drawback is that we can’t use different concrete types on client and server interpreters.

[julienrf]

Hi,
Just a heads up that I have made some progress on my branch, I have just pushed a commit that introduces a way to partially provide the logic of endpoints (such as handling authentication things): julienrf@1a5d484

I believe this is going in an interesting direction (you can see how I could re-implement the basic authentication as a middleware). I still need to implement it for all the interpreters (so far I only did OpenAPI and play-client).

Please let me know what you think about it. Maybe it’s introducing unnecessary complexity?

[bmeesters]

Great to see some progress here! I don't think the Middleware approach makes it much more complicated. And it's nice that we can move some things to the algebras that used to live in the interpreters.

I had to look twice to understand how we can have a client/server implementation in the algebra (since I saw the usage before the definition) but the ScalaDoc is pretty clear. Also I think it is important to note that using Middlewares is not required. And we currently have quite complicated code because of a lack of 'middlewares/composability'.

A minor point. I think it would be nice if Credentials and BearerToken to have a shared trait so you can just say an endpoint with auth.

[julienrf]

Hello! I managed to get everything implemented. You can see the result here: master...julienrf:just-authenticated-endpoints

There are still a few things that are missing (tests, examples, and documentation). Also, some things to polish.

More importantly, middlewares are currently made of “synchronous” functions (ie, they can’t return a Future, and they can’t return an Effect in the http4s interpreter). I think supporting asynchronous functions would be doable, but we should also support “effectful” functions in http4s, ideally, which would be a bit more complex to do.

---

Since the diff is quite large, here are the main highlights:

• It is now possible to implement exactly what we used to implement in the BasicAuthentication module, but without having to add any code to the interpreters: everything is now achieved at “the algebra level”, by using middlewares.
• Given an endpoint of type Endpoint[Int, String], you can call withBasicAuth() on it to get an Endpoint[(Int, Credentials), Option[String]]. That resulting endpoint carries authentication credentials in its request, and may return a Forbidden response in case the credentials don’t grant access to the underlying resource.
• The withBasicAuth method is defined here:

  def endpointWithBasicAuth[A, B, AC](
      endpoint: Endpoint[A, B],
      realm: Option[String] = None,
      unauthorizedDocs: Option[String] = None,
      forbiddenDocs: Option[String] = None
  )(implicit tupler: Tupler.Aux[A, Credentials, AC]): Endpoint[AC, Option[B]] = {
    // `endpoint` enriched with request headers and additional error responses
    val enrichedEndpoint: Endpoint[(A, Option[Credentials]), Either[Either[Unit, Unit], B]] =
      endpoint
        .mapRequest(_.addHeaders(basicAuthHeader))
        .mapResponse(response =>
          unauthorizedResponse("Basic", realm, unauthorizedDocs)
            .orElse(forbiddenResponse(forbiddenDocs))
            .orElse(response)
        )
    // add a middleware that takes care of the authentication part
    enrichedEndpoint
      .withMiddleware[AC, Option[B]](authenticationMiddleware)
  }

  What it does is that it first transforms the given Endpoint[A, B] by adding an Authorization header to its request, and by adding alternative response statuses (Unauthorized and Forbidden).
  Then, that modified endpoint is plugged to a middleware that provides the client and server logic around the initial Endpoint[A, B].
  The middleware is defined here:

  private def authenticationMiddleware[A, B, C, `(A, C)`](implicit
      tupler: Tupler.Aux[A, C, `(A, C)`]
  ): Middleware[(A, Option[C]), Either[Either[Unit, Unit], B], `(A, C)`, Option[B]] =
    Middleware(
      serverAction = (aAndCredentials: (A, Option[C])) => {
        val (a, maybeCredentials) = aAndCredentials
        maybeCredentials match {
          case Some(credentials) =>
            // The request contains credentials: invoke the logic of the
            // resulting endpoint, after some transformation
            Middleware.Continue(tupler(a, credentials))
          case None =>
            // No credentials in the request headers: bypass the resulting
            // endpoint’s logic by immediately returning an Unauthorized response
            Middleware.Bypass(Left(Left(())) /* Unauthorized */ )
        }
      },
      fromNewResponse = {
        case Some(b) => Right(b)
        case None    => Left(Right(())) // Forbidden
      },
      fromNewRequest = aAndCredentials => {
        val (a, credentials) = tupler.unapply(aAndCredentials)
        // Always provide the credentials in requests
        (a, Some(credentials))
      },
      toNewResponse = {
        case Right(b)        => Some(b)
        case Left(Right(())) => None
        case Left(Left(()))  => throw new Exception("Unexpected status 'Unauthorized'")
      }
    )

  The serverAction defines what server interpreters should do when they receive requests. If there are no credentials the Unauthorized response is returned (without even going through the logic provided for the endpoint). Otherwise, the credentials are added to the request information. So, it transforms the information carried by requests from (A, Option[Credentials]) to (A, Credentials). The parameters fromNewResponse, fromNewRequest, and toNewResponse define how to map the requests and responses of the resulting endpoint to the requests and responses of the so-called “enriched” endpoint. Here, we say that a response containing None in the resulting endpoint should be mapped to the Forbidden response, for instance.

---

In conclusion, I still want to conduct a few more experiences but so far I am convinced that the changes proposed by @mleclercq should be accepted. As a first step, we could have a PR that only introduces the operations for transforming requests and responses (without the AuthenticatedRequests algebra, which will be superseded by the middlewares in my branch). Does that make sense?

[bmeesters]

Great work @julienrf! Can you explain to me why we need fromNewResponse and toNewResponse? I don't understand yet why we need to have both directions.

Other than that I think two PRs would be good. The mapRequest and mapResponse would already be very useful for us. That said, I also don't mind to review this as a whole. There are already 4 developers that have been involved and could likely do a review. I don't mind taking the time and review the whole PR next week. That said, if we want more time to test/verify the middleware approach we can split it up into two PRs.

In any case I am willing to help and have some time next week.

[julienrf]

Can you explain to me why we need fromNewResponse and toNewResponse?

fromNewResponse is used by servers to transform the response provided by the user logic into the response of the oiriginal endpoint.

toNewResponse is used by clients to transform the response send by the server to the new response type.

I think it will be easier to introduce things gradually, so I am in favor of doing separate PRs. Unfortunately, I won’t have much time in the next two months, so any help is appreciated. Maybe one of you @bmeesters or @mleclercq could create a PR introducing mapRequest, mapResponse, and mapDocs?

[bmeesters]

Ah yes, that explanation for fromNewResponse and toNewResponse makes sense.

If you want a shot @mleclercq at the transformations just let me know. Otherwise I will start on it next week.