Fast Fail for andThen-composed Endpoints does not work

[d-s-d]

Description

When composing two endpoints with the andThen operator, if the first endpoint does not match, the second one is still evaluated. In particular, a json-body is parsed despite authentication has failed.

Steps to reproduce

import io.finch._
import io.finch.circe._
import io.circe.generic.auto._
import io.finch.syntax._
import com.twitter.io.Buf

case class Body(value: String)

val legalBody = """{"value": "test"}"""
val illegalBody = """{"val": "test"}"""

val authE = header("Authorization").handle{ case e: Error.NotPresent => Unauthorized(new Exception("Not Authorized")) }
val bodyE = jsonBody[Body]

val testendpoint = post(authE :: bodyE)

testendpoint(Input.post("/").withBody[Application.Json](Buf.Utf8(legalBody))).awaitValue()
testendpoint(Input.post("/").withBody[Application.Json](Buf.Utf8(illegalBody))).awaitValue()

Expected Behavior

In both cases, the result should be:
Option[com.twitter.util.Try[String :: Body :: shapeless.HNil]] = Some(Throw(java.lang.Exception: Not Authorized))

Actual Behavior

The results are:

Option[com.twitter.util.Try[String :: Body :: shapeless.HNil]] = Some(Throw(java.lang.Exception: Not Authorized))
Option[com.twitter.util.Try[String :: Body :: shapeless.HNil]] = Some(Throw(io.finch.Error$NotParsed: body cannot be converted to $read$Body: Attempt to decode value on failed cursor: DownField(value).))

[spockz]

Depending on which actions are being performed this could not be just wrong but also insecure.

[vkostyukov]

@sergeykolbasov @rpless I'm thinking maybe we could do flatMap instead of a join within Endpoint.product. How do you feel about this?

[rpless]

I feel like this happens because :: is not really sequential, it is independent (and mostly Applicative). Part of me feels like Auth should happen in a Finagle Filter but only matched with a Context lookup in a Finch Endpoint. I think this can be reformulated to be a different Endpoint (not a filter) and documented to work as expected, but its late for me right now. Any chance I can have until tomorrow evening to formulate a response? I've done things like this already for our end-user's API,
If that doesn't pane out let's consider changing it.

[rpless]

Oh also @d-s-d what version of Finch are you using?

[sergeykolbasov]

@vkostyukov you mean introduce Endpoint.flatMap? AFAIR it's impossible without blocking EndpointResult output or change in the current signature of apply, I've tried it multiple times already.

[d-s-d]

Sorry for omitting the version. The above was tested with 0.22.0 and 0.23.0.

[d-s-d]

Considering the Finagle Filter, doesn't the fundamental issue remain? It does not make a difference if the decision is based on a context-object or the header itself, if both endpoints are "evaluated" even though the first one fails, the fundamental problem remains, no?

[d-s-d]

Base on Error Accumulation, I tried something else that does not work either. Forgive me if I made some erroneous assumptions about the API here:

scala> val auth = header("Authorization").handle { case e: Error.NotPresent => throw new Exception("unauth") }
auth: io.finch.Endpoint[String] = header(Authorization)

scala> val sideEffect = path[String].mapOutput(s => {println(s); Ok(s)})
sideEffect: io.finch.Endpoint[String] = :string

scala> val testendpoint = post(auth :: sideEffect)
testendpoint: io.finch.syntax.EndpointMapper[String :: String :: shapeless.HNil] = POST /header(Authorization) :: :string

scala> testendpoint(Input.post("/asdf")).awaitValue()
asdf
res9: Option[com.twitter.util.Try[String :: String :: shapeless.HNil]] = Some(Throw(java.lang.Exception: unauth))

[rpless]

Sorry for omitting the version.

No problem. Given that we've changed to monthly releases the potential for version drift is higher, so we should probably have a template for github issues that includes this as a prompt.

A Finagle Filter might let you get around this, if you can reject invalid auth headers from the filter instead from the Endpoint. You could then set something on the Context and have the Endpoint pull a value from that if you needed it. However, if you need to do this in the endpoint, then its still an issue.

The intention of Error Accumulation for Product Endpoints is to determine all of the things that did not match. In the example above, we have a Product Endpoint of path[String].mapOutput(s => {println(s); Ok(s)}) and header("Authorization").handle { case e: Error.NotPresent => throw new Exception("unauth") } so both will attempt to validate so that we can see all of the problems with the product. The docs do say that it should fail-fast on non-Finch errors, but that hasn't been updated in a year or two, and I don't think that's true anymore. We probably need to change the wording there.

[d-s-d]

A Finagle Filter might let you get around this, if you can reject invalid auth headers from the filter instead from the Endpoint.

True that. Thanks for the clarification.

This works if you have a general authentication mechanism. If you imagine a scenario where different endpoints use different authentication mechanisms, an authenticated client might trigger this behavior on an endpoint that he is not authenticated against. Actually, it's an authorization problem: the filter must not only authenticate, but also authorize the client.

[vkostyukov]

@sergeykolbasov Not Endpoint.apply, but use Rerunnable.flatMap instead of Rerunnable.product here. This will get us the fail-fast semantic monads have.

[sergeykolbasov]

It shouldn't change anything if we lift it to Try. Also, from the original example there will be no exception at all as the authorization endpoint is handled already. The true fail-fast behavior means that we flatMap next endpoint over the Output and with the current signature it's impossible.

…

On Tue, 21 Aug 2018 at 17.24, Vladimir Kostyukov ***@***.***> wrote: @sergeykolbasov <https://github.com/sergeykolbasov> Not Endpoint.apply, but use Rerunnable.flatMap instead of Rerunnable.product here <https://github.com/finagle/finch/blob/master/core/src/main/scala/io/finch/Endpoint.scala#L182>. This will get us the fail-fast semantic monads have. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#978 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACNbQX4A2uTcIvtCeEiUphTJnDK4XUgLks5uTBgugaJpZM4WEQuc> .

[vkostyukov]

We still should be able to implement Endpoint.:: in terms of flatMap on Rerunnable. In fact, this is what an initial implementation of an endpoint looked like. We, however, decided to accumulate errors so we opted in for liftToTry.product. Consider an example:

val ab = param[Int]("a") :: param("b")

With an empty request, this will fail with Errors containing both missing entities. If we go with Rerunnable.flatMap, on the other hand, this will only yield the first observed error.

I'm no longer sure it something that's easy to fix. Perhaps an alternative approach would be to promote a different way of doing an auth?

[spockz]

I'm no longer sure it something that's easy to fix. Perhaps an alternative approach would be to promote a different way of doing an auth?

Instead of having something specific for authentication, could we have a short-circuiting variant of ::? It would solve this issue.

I’m afraid it would still be easy to make a mistake. And we shouldn’t have to many operators. (Like sbt of yesteryear.)

[vkostyukov]

Yeah, I'm not a huge fan of adding a specific combinator for this. I'd rather see us standardizing on either fail-fast of collect-errors approach. The later has been our default for quite a long time yet I'm starting to gain some skepticism around how important it is to accumulate those errors. It may be that people just use body most of the time and don't really worry about threading errors across :: combinators.

Perpahs asking what's people's most popular use case would be a good way forward here.

Maybe @rpless has some insights on where or not the error accumulation is useful in ::?

[rpless]

I'm not a big fan of combinator either.

As for whether error accumulation is useful, I am a heavy user of error accumulation especially in conjunction with should and our own version of validate (similar to what to the one in #808). We tend to use a lot of params and headers and we add additional validation to stop invalid requests from entering our service logic. Being able to see all of the issues with the request immediately is very helpful. That said I've also worked in environments where almost everything outside the path is sent along in the body. I prefer the error accumulation, but we should also try to get a wider set of responses.

[sergeykolbasov]

I feel that it should be the way how it's now with Mapper being executed if there were no errors during request processing through the composition, and we can gurantee that Mapper ain't executed if there was a single error.
So people still can use authorization endpoint i.e., but the real business logic should be executed only under the Mapper.

[vkostyukov]

Maybe the obvious solution would be to structure finch-oauth2 (and, perhaps, other auth packages) differently? One idea is to move the auth logic somewhere under the derived service (ToService, although we'd need to make sure it's decoupled from the actual auth scheme) and populate a request context with AuthInfo on success such that authInfo combinator (from finch-oauth2) only extracts the value that's ready.

A sketch (enable auth on per-endpoint basis)

Bootstrap
  .serveSecured(Endpoint.const("fooo"), OAuth2Scheme(dataHandler))

Or per ToService:

  Bootstrap
    .configure(authScheme = OAuth2Scheme(dataHandler))

I like the later more as we don't have to introduce severSecured, but it's definitely less flexible.

I'm not in love with the fact there are now two places where users would have to "enable" auth, but I surely buy this as an ad-hoc solution until we figure out a better way.

[sergeykolbasov]

@vkostyukov my another thought is to introduce new type FailFastEndpoint <:< Endpoint and do the pattern matching during request flow through :: (or override productWith). In case if self is fail fast and returns error, immediately stop the evaluation and return currently accumulated errors.

I think it should give a transparency over the request execution.

[hderms]

For what it's worth, I think the lack of a way of effectively flatMapping over endpoints is a huge drawback to using Finch, currently. If there's no way to reproduce that semantic behavior, which after looking at the source code in-depth I agree with @sergeykolbasov , I think the documentation around :: should be updated to reflect that all endpoints are run i.e. in a :: b, if a throws an exception or returns an Output error, b will still run, regardless.

@vkostyukov I agree that adding a bunch of combinators could be undesirable, but I think the current state is borderline untenable. If people admit the utility of monadic composition and applicative composition in general purpose FP programming, I think one should admit the utility of those patterns when composing endpoints in Finch. As of right now I think that it's sort of an impedance mismatch between how people would typically want to work with Finch and what is permitted by the library.

If we don't want to add a new combinator I think there should at least be some kind of escape hatch that allows the user to reproduce these semantics, perhaps in a 'hackier' way than would be desired, but that doesn't subvert the goals of the project. It's nice to have well designed, "type-ful" interfaces to solve complex problems with, and in that respect I think Finch is a tremendous success, but I also think that sometimes the real world just doesn't match with the constraints of whatever library someone is using.

I'm not precisely sure what route should be taken but I do think there's a need to make this possible (and it sounds like some projects really make a lot of use of the accumulating errors, so it's hard to say that fail-fast semantics are universally preferred).

@sergeykolbasov I'm not seeing the behavior of a mapper only running when all the endpoints succeed, in fact, that is the behavior that led me to start investigating this myself. Is this potentially something that was fixed in a version of Finch?

[hderms]

To clarify, this does appear to not run the mapper if withAuth returns an error

post("foo" :: withAuth) { user =>
   //only runs with auth
}

but for example:

  val withAuth: Endpoint[IO, HNil] = {
    if (authed) {
       Output.HNil
    } else {
       Unauthorized(new Exception("Not authorized"))
     }
  }

val endpoints = withAuth :: (fooEndpoint :+: barEndpoint)

the mappers on fooEndpoint or barEndpoint will run for the first of fooEndpoint and barEndpoint that matches, regardless of the output of withAuth

I'm not sure if this is a super realistic use case, but it appears to subvert the notion that the Mapper only runs when the previous endpoints succeed