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

authmiddleware to consume the entire request #1530

Merged
merged 3 commits into from Nov 9, 2017

Conversation

Projects
None yet
3 participants
@jmcardon
Member

jmcardon commented Nov 8, 2017

This pr aims to address this issue

import cats._
import cats.data._
import org.http4s._
import org.http4s.dsl.io._
import cats.effect.IO
import cats.implicits._
import org.http4s.implicits._
import org.http4s.server.AuthMiddleware

case class User(id: Int)

val authedService: AuthedService[User, IO] = AuthedService[User, IO] {
  case GET -> Root / "asd" as user =>
    Ok()
}

val middleware: AuthMiddleware[IO, User] =
  AuthMiddleware[IO, User](Kleisli(_ => OptionT.none))

val service1 = middleware(authedService)

val service2: HttpService[IO] = HttpService[IO] {
  case GET -> Root =>
    Ok()
}


(middleware(authedService) <+> service2).orNotFound(Request[IO]()).unsafeRunSync()

For security reasons, going through an AuthMiddleware should consume an entire request: i.e, it should never fall through to the other service. This should return 404, but it currently returns ok.

This pr adjusts that behavior in the following manner:

  • If the user is authenticated, but the route is unmatched, the request will fall through to not found
  • if the user is not authenticated, it will be Unauthorized
  • service composition with <+> for authmiddleware'd service will never reach any service composed
    to the right of the authed service

jmcardon added some commits Nov 8, 2017

@rossabaker

Looks good. Some of the tests go a bit beyond the description of the tests, but I think all the cases are correct.

service.compose(Kleisli((req: Request[F]) => authUser(req).map(AuthedRequest(_, req))))
Kleisli((r: Request[F]) => authUser(r).map(AuthedRequest(_, r)))
.andThen(service.mapF(o => OptionT.liftF(o.fold(Response[F](Status.NotFound))(identity))))
.mapF(o => OptionT.liftF(o.fold(Response[F](Status.Unauthorized))(identity)))

This comment has been minimized.

@rossabaker

rossabaker Nov 9, 2017

Member

Should these canned responses come with a default message? I've never had a good feel for that. It feels like yes, but then people fight about the content type, and it's a bit anglocentric.

@rossabaker

rossabaker Nov 9, 2017

Member

Should these canned responses come with a default message? I've never had a good feel for that. It feels like yes, but then people fight about the content type, and it's a bit anglocentric.

This comment has been minimized.

@jmcardon

jmcardon Nov 9, 2017

Member

I'd rather they not. If people wanted this, they could use the method that deals with the explicit error in authmiddleware.
I'm partial to simply the 401, but I'll let others take the wheel.

@jmcardon

jmcardon Nov 9, 2017

Member

I'd rather they not. If people wanted this, they could use the method that deals with the explicit error in authmiddleware.
I'm partial to simply the 401, but I'll let others take the wheel.

val service = middleware(authedService)
service.orNotFound(Request[IO](method = Method.POST)) must returnStatus(Ok)

This comment has been minimized.

@rossabaker

rossabaker Nov 9, 2017

Member

Is this to test your test?

@rossabaker

rossabaker Nov 9, 2017

Member

Is this to test your test?

This comment has been minimized.

@jmcardon

jmcardon Nov 9, 2017

Member

None of the prior tests actually test the default apply in authmiddleware. They test the second apply with explicit error.

I figured it doesn't hurt to test whether it passes through anyway.

@jmcardon

jmcardon Nov 9, 2017

Member

None of the prior tests actually test the default apply in authmiddleware. They test the second apply with explicit error.

I figured it doesn't hurt to test whether it passes through anyway.

This comment has been minimized.

@jmcardon

jmcardon Nov 9, 2017

Member

I moved this to a separate test, actually.

@jmcardon

jmcardon Nov 9, 2017

Member

I moved this to a separate test, actually.

Show outdated Hide outdated ...ala/org/http4s/server/middleware/authentication/AuthMiddlewareSpec.scala Outdated
@rossabaker

👍 on green

@aeons

aeons approved these changes Nov 9, 2017

@aeons aeons merged commit 4d8a3df into http4s:master Nov 9, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

parsnips added a commit to parsnips/tsec that referenced this pull request Dec 22, 2017

Example of composing routes with differing auth.
api2 can never be hit due to
http4s/http4s#1530

Alternatives?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment