Add basic authentication support for http4s #1342
Conversation
github-actions
bot
added
core
|
Please pardon the initial silence here -- I didn't have the time this weekend, I'll try to get to it sometime this week |
There was a problem hiding this comment.
If we are to attempt to actually implement authentication in a general way, I think it is important that we follow the spec, as avoiding generated API churn is an explicit goal of this project.
While you are correct, supplying an additional type parameter is likely the correct direction to go, the similarities between authenticationMiddleware and mapRequest is so great, it is actually possible to implement this feature without a change to guardrail at all by changing the supplied F from IO to Kleisli[IO, Option[AuthContext], *].
As I commented elsewhere in this review, I think the primary concerns that need to be addressed are:
- Instead of enabling/disabling authentication via
Boolean, we need to explicitly thread through which kind of authentication is supplied, per route, with failover - If at all possible, source compatibility with http4s' existing authentication middleware would be very useful, as it would permit code reuse between existing and new implementations, as well as compatibility with already-published http4s middleware providers.
- If this also means switching from
HttpRoutes.of... toAuthedRoutes.ofand composing the two implementations together via<+>, that may help keep reimplementation down, and utilize some of the infrastructure already provided to us by http4s.
- If this also means switching from
Thank you for your initial effort here, it's great to see interest for first-class authentication support in guardrail, at Twilio this was implemented in a bespoke way for our Dropwizard generators, targeting internal Twilio authentication provider libraries. What are your thoughts to my above observations?
| @@ -55,6 +55,7 @@ class Http4sServerGenerator private (version: Http4sVersion)(implicit Cl: Collec | |||
| private def this(Cl: CollectionsLibTerms[ScalaLanguage, Target]) = this(Http4sVersion.V0_23)(Cl) | |||
|
|
|||
| val customExtractionTypeName: Type.Name = Type.Name("E") | |||
| val authContextTypeName: Type.Name = Type.Name("AuthContextT") | |||
There was a problem hiding this comment.
This implies the intent of this type, but isn't defined anywhere
There was a problem hiding this comment.
Additionally, is this really a monad transformer?
There was a problem hiding this comment.
it's definitely not a monad transformer, just an old OOP suffix notation popped up in my mind, will rename
| @@ -50,6 +50,10 @@ object ServerGenerator { | |||
| for { | |||
| resourceName <- formatTypeName(className.lastOption.getOrElse(""), Some("Resource")) | |||
| handlerName <- formatTypeName(className.lastOption.getOrElse(""), Some("Handler")) | |||
| authenticationEnabled = securitySchemes.nonEmpty && routes.exists { | |||
There was a problem hiding this comment.
I think you're right to consider securitySchemes authoritative, but instead of just recording a boolean, we should be able to thread the exact information down into routes.
While it is useful to be able to plug any desired middleware into the route constructor, the more important thing to observe is that the semantics are guided by the oAPI spec, so if there are multiple (consider /basic and /oauth), they need to be handled effectively together.
Without that additional metadata, it's either difficult or impossible to implement the middleware without reverse-engineering the routing layer.
There was a problem hiding this comment.
I think, it's not necessary to have securitySchemes down in the routes class, because there is no guarantee that all of them or any of them are used in routes. A route in the OAS contains security schemes names it requires, this information should be enough (in case we don't care about the schemes types and give a user the responsibility to implement them)
Thank you for the review, I‘ll come back after the holidays |
yes, it is similar, but it's not the same.
I agree, but we don't need to use
I think it's possible to reuse, but it's better not to, because the http4s auth works on completely another level and doesn't have an information about auth kinds and scopes. Probably it could be implemented grouping routes but routes could have several kinds of auth at the same time. It would be hard to group such routes:
I've got an idea while writing this comment, I'll try to experiment with it.
I wouldn't move it to a user code, maybe we can do it internally if it helps
Thank you for the review, I'll try to push it forward whenever I have time |
When I've done this in the past, I've done something along the lines of ctx.fold(respond.NotAuthorized) { auth =>
respond.Ok(auth.username)
}though I appreciate the observation that this isn't always obvious to the user. It does raise a consideration though, which is that if the route only specifies
Good -- So long as we agree that One of the objections I have to
I agree with this -- any careless modification could change fully functional production code to only partially implemented code without any compiler errors, which would be a huge detriment to users. Thanks for calling it out! |
|
Hi again! I've found some time to get back. Another potential problem here is caching, for example in case So, in the latest implementation all decisions should be made on the used code side: it decides what to do with security schemes and scopes, implements caching and whatever optimisations make sense (like always evaluate |
Codecov Report
@@ Coverage Diff @@
## master #1342 +/- ##
==========================================
+ Coverage 82.50% 82.69% +0.18%
==========================================
Files 75 75
Lines 5316 5402 +86
Branches 147 151 +4
==========================================
+ Hits 4386 4467 +81
- Misses 930 935 +5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
This looks really good. I did a few stylistic tweaks:
- avoiding
unsafe*methods - preferring
A => Afunction composition instead of maintaining structurally similar branches - reducing
case (Some(_), Some(_))into a singleOption[(A, B)]
The one sticking point for me is the ergonomics around authMiddleware itself. Looking at a fairly common case of being able to answer a challenge via one or more A => IO[B] functions, as a user I'd expect to be able to (straightforwardly):
- Attempt to execute auth challenges in parallel
- Collect and enforce the "and" and "or" constraints
I'm sure I've gotten lost somewhere here, but I tried to do a mock implementation that would match the type signature. The following is what I ended up with:
def lookupAuthedScopes(req: Request[IO], scopes: List[String]): IO[List[String]] =
IO.pure(
req.headers.get(CIString("dummy-scopes"))
.map(_.head.value.split(",").intersect(scopes))
.toList
.flatten
)
def authMiddleware(schemes: NonEmptyList[NonEmptyMap[Resource.AuthSchemes, List[String]]], req: Request[IO]): IO[Option[List[String]]] = {
println("Schemes:")
schemes.toList.foreach(_.toSortedMap.foreach({ case (scheme, scopes) => println(s" ${scheme.name} -> ${scopes.mkString(", ")}") }))
schemes.collectFirstSomeM[IO, List[String]](_.toSortedMap.toList.flatTraverse({
case (Resource.AuthSchemes.Basic1, scopes) =>
lookupAuthedScopes(req, scopes)
case (Resource.AuthSchemes.Basic2, scopes) =>
lookupAuthedScopes(req, scopes)
}).map(Option(_).filter(_.nonEmpty)))
}This additionally relies on the implementation agreeing with what the specification says in #!/components/securitySchemes, everything's kinda open-ended in that regard.
Do you have any guidance of how you were intending these functions be implemented? The tests are extremely narrow and immediately move away from the NonEmptyList/NonEmptyMap types, in favor of direct equality against known test values, which are not sufficient inspiration for how a user may adapt this to their environment.
|
Playing around a little bit more, in my example I've discovered that schemes.collectFirstSomeM(...)and lookupAuthedScopes(...)almost mesh with the type of If |
Current implementation moves responsibility of processing "and" and "or" to the user side which increases complexity and reduces ergonomics, but from the other hand it gives user more freedom to optimise the check. Do you think it would be better to move parallelisation and running logic to the guardrail side and only request from the user a bunch of authentication functions (
I've reimplemented your example to how I see it could work in a real case: final case class User(id: Long, name: String, scopes: List[String])
def getUserFromDB(id: Long, password: String): IO[Option[User]] =
IO.pure(Some(User(id, "John Smith", List("admin"))))
def base64Decode(in: String): Option[String] = ???
def extractCredentials(req: Request[IO]): Option[(Long, String)] =
req.headers
.get[Authorization]
.collect { case Authorization(Credentials.Token(AuthScheme.Basic, token)) => token }
.flatMap(base64Decode)
.map(_.split(":").toList)
.collect { case id :: password :: Nil => (id.toLong, password) } // unsafe throw
def authMiddleware(schemes: NonEmptyList[NonEmptyMap[Resource.AuthSchemes, List[String]]], req: Request[IO]): IO[Option[User]] = {
println("Schemes:")
schemes.toList.foreach(_.toSortedMap.foreach({ case (scheme, scopes) => println(s" ${scheme.name} -> ${scopes.mkString(", ")}") }))
schemes
.collectFirstSomeM[IO, User](
_.toSortedMap.toList
.map {
case (Resource.AuthSchemes.Basic1, scopes) =>
extractCredentials(req).flatTraverse { case (id, pass) => getUserFromDB(id, pass).map(_.filter(_.scopes.toSet == scopes.toSet)) }
case (Resource.AuthSchemes.Basic2, scopes) =>
extractCredentials(req).flatTraverse { case (id, pass) => getUserFromDB(id, pass).map(_.filter(_.scopes.toSet == scopes.toSet)) }
}
.reduce[IO[Option[User]]] {
case (l, r) =>
l.flatMap {
case Some(_) => r
case None => IO.pure(None)
}
}
)
}Some notes after writing it:
While I wrote this I've realised that the implementation looks a bit complicated, especially when a real use case has only one authentication type. I would suggest to introduce a config parameter like "custom authentication" which would enable current 'complicated' implementation for a complicated case (with "and"s and "or"s, parallelisation requirement and etc.). But replace the current implementation by simplified (from user point of view) one: the code you wrote in the Even in the simplified case the http4s |
Enforcing parallelized calls against some user code seems like it would be easier for the user to implement, which is good, at the expense of some (possibly nonexistent?) case where a user does not want that parallelism.
I wonder if a better strategy wouldn't be to offer some As for ways to improve, I'm interested in your thought about what the user experience of I was experimenting with whether I liked the idea of reducing the Thank you for the dialogue around what end-user code would look like, your implementation clears up a fair amount. Idea for what the singleton scheme code may look like, though to reiterate, I don't believe this is worth the cost of an explosion of complexity in the to-be-designed final case class User(id: Long, name: String, scopes: Set[String])
def getUserFromDB(id: Long, password: String): IO[Option[User]] =
IO.pure(Some(User(id, "John Smith", Set("admin"))))
def base64Decode(in: String): Option[String] = ???
def extractCredentials(req: Request[IO]): Option[(Long, String)] =
req.headers
.get[Authorization]
.collect { case Authorization(Credentials.Token(AuthScheme.Basic, token)) => token }
.flatMap(base64Decode)
.map(_.split(":").toList)
.collect { case id :: password :: Nil => (id.toLong, password) } // unsafe throw
val authMiddleware: ((Resource.AuthSchemes, Set[String]), Request[IO]) => IO[Option[User]] = {
case ((Resource.AuthSchemes.Basic, scopes), req) =>
extractCredentials(req)
.flatTraverse { case (id, pass) =>
getUserFromDB(id, pass)
.map(_.filter(u => scopes.diff(u.scopes).isEmpty)) // If the user contains at least the scopes requested
}
} |
|
I like the latest code example, I think it has enough simplicity and we should consider it as the main solution. For users who have special cases we can provide
|
|
I just merged #1359, which will likely resolve the code coverage issue you're running into in this PR. I'm fine ignoring it for now, since we've been dancing around our coverage target for a while, pushed into the red by the http4s 0.22 and 0.23 tests being split in two until we drop 0.22 support. Feel free to rebase off master if you want to see green, there should be no merge conflicts. |
|
Thanks for notifying, I've rebased the branch and made a couple of steps further:
Possible authentication parameters:
def authenticate[F[_]: Monad, AuthContext](middleware: (AuthSchemes, Set[String], Request[F]) => F[Option[AuthContext]], schemes: NonEmptyList[NonEmptyMap[AuthSchemes, Set[String]]], req: Request[F]): F[Option[AuthContext]] = {
schemes.collectFirstSomeM[F, AuthContext](_.toNel.map({
case (scheme, scopes) =>
middleware(scheme, scopes, req)
}).reduce[F[Option[AuthContext]]]({
case (l, r) =>
l.flatMap({
case Some(_) =>
r
case None =>
Applicative[F].pure(None)
})
}))
}it's based on the code you suggested above with little adjustments. This approach allows to require from user a bit simpler middleware signature: final case class User(id: Long, name: String, scopes: Set[String])
def getUserFromDB(id: Long, password: String): IO[Option[User]] =
IO.pure(Some(User(id, "John Smith", Set("admin"))))
def base64Decode(in: String): Option[String] = ???
def extractCredentials(req: Request[IO]): Option[(Long, String)] =
req.headers
.get[Authorization]
.collect { case Authorization(Credentials.Token(AuthScheme.Basic, token)) => token }
.flatMap(base64Decode)
.map(_.split(":").toList)
.collect { case id :: password :: Nil => (id.toLong, password) } // unsafe throw
val authMiddleware: (Resource.AuthSchemes, Set[String], Request[IO]) => IO[Option[User]] = {
case (Resource.AuthSchemes.Basic, scopes, req) =>
extractCredentials(req)
.flatTraverse { case (id, pass) =>
getUserFromDB(id, pass)
.map(_.filter(u => scopes.diff(u.scopes).isEmpty)) // If the user contains at least the scopes requested
}
}
The next steps could be:
|
| Applicative[F].pure(l) | ||
| }) | ||
| }) | ||
| val nel = el.toNel |
There was a problem hiding this comment.
One caveat of the NonEmptyMap is that it seems to preclude the "Optional Oauth2 Security" example, reproduced here:
security:
- {}
- petstore_auth:
- write:pets
- read:petsThis looks like an oversight from the first foray into authentication, so if you'd like to consider that case out of scope here I'm happy to take that on, if you're running out of time. I appreciate the number of iterations you've taken to this problem so far, just trying to make sure we merge something that isn't going to need more work immediately.
There was a problem hiding this comment.
that's true, I've wrote about the bug above. I would fix it before merging the change because it's not very useful with the problem
There was a problem hiding this comment.
ah, I've checked the link and it's not the bug I mentioned, it's another use case related with optional requirements, anyway, I think it needs to be fixed)
There was a problem hiding this comment.
as I wrote in the previous comment, the swagger library we use under the hood doesn't support this approach to define optional authentication. It seems there is the x-security-optional to cover the case, but it was a list of names which is a bit overwhelming and unclear. To make it a bit more clear, I've changed its type to Boolean and it could be defined just for a route to make its security requirements optional.
For the custom authentication we provide its value to the authenticationMiddleware and let the user to decide what to do with that: produce some special case of its AuthContext or just pass it through inside of the AuthContext to the handler and deal with that there.
For the simple authentication we replace Either by an Option for the handler but the authenticationMiddleware signature wasn't changed
|
Hi again, I'm still here)
|
Before moving forward with the review, I did a small experiment with: diff --git a/modules/sample/src/main/resources/issues/issue1218.yaml b/modules/sample/src/main/resources/issues/issue1218.yaml
index 08a65de4..05751565 100644
--- a/modules/sample/src/main/resources/issues/issue1218.yaml
+++ b/modules/sample/src/main/resources/issues/issue1218.yaml
@@ -11,6 +11,10 @@ paths:
responses:
'204':
description: No response
+ security:
+ - basic:
+ - "bar:basic"
+ - {}
parameters:
- name: refvec
in: query
@@ -125,6 +129,10 @@ paths:
type: integer
format: int64
components:
+ securitySchemes:
+ basic:
+ type: http
+ scheme: basic
schemas:
refvec:
type: arraycombined with diff --git a/modules/core/src/main/scala/dev/guardrail/Common.scala b/modules/core/src/main/scala/dev/guardrail/Common.scala
index 47b5c90b..6445a953 100644
--- a/modules/core/src/main/scala/dev/guardrail/Common.scala
+++ b/modules/core/src/main/scala/dev/guardrail/Common.scala
@@ -72,6 +72,15 @@ object Common {
.filter(_ != "/")
paths = swagger.downField("paths", _.getPaths)
+ () = {
+ import scala.jdk.CollectionConverters._
+ swagger.unwrapTracker.getPaths.asScala.values.toList.flatMap(_.readOperations().asScala.toList).flatMap(_.getSecurity.asScala.toList).foreach(println)
+ }
globalSecurityRequirements = NonEmptyList
.fromList(swagger.downField("security", _.getSecurity).unwrapTracker)
.flatMap(SecurityRequirements(_, SecurityOptional(swagger), SecurityRequirements.Global))which, while not null safe at all and quite unreadable, it does show the expected states: What is not supported? Is this some interaction with the automatic type-conversion in Since the case of Additionally, if |
blast-hardcheese
force-pushed
the
auth-support
branch
from
1b2e048 to
1698c4e
Compare
| val securityRequirements = renderCustomSecurityRequirements(sr) | ||
| val authContextParam = param"${arg.paramName}" | ||
| val authTransformer: Term => Term = inner => q""" | ||
| authenticationMiddleware($securityRequirements, ${sr.optional}, req).flatMap { $authContextParam => |
There was a problem hiding this comment.
This is the last sticking point for me. ${sr.optional} being passed in as a Boolean into authenticationMiddleware is really confusing, as a user. If authentication is optional, shouldn't there just be a recover on whatever $authContextTypeName is returned? Some(Unauthorized) => None?
There was a problem hiding this comment.
I took a stab at resolving this. There's something nagging at me about having both SecurityExposure and AuthImplementation passed in as separate parameters to a bunch of parameters. I've not changed any of the underlying logic, though reducing the number of Boolean's exposed to users is better. I'll come back to this soon (I'd like to get this merged and on its way), but offering here in case anyone is interested in taking a look.
There was a problem hiding this comment.
Haven't found a great way to resolve this, so I'm going to merge as is.
blast-hardcheese
force-pushed
the
auth-support
branch
2 times, most recently
from
241c567 to
1ffdd39
Compare
blast-hardcheese
force-pushed
the
auth-support
branch
2 times, most recently
from
b931ebb to
9539c23
Compare
blast-hardcheese
force-pushed
the
auth-support
branch
from
9539c23 to
c8afc6f
Compare
|
The last concern I had was whether these parameters made sense across Scala/Java, and for all the different supported frameworks. I'm alright with proceeding. |
|
Thanks for your patience, @zeal18, life has been getting in the way of getting 100% behind where we ended up by the end here. |
blast-hardcheese
added
the
enhancement
I thought about it, especially about the I've took a look at some of your improvements but had no chance to check them all, it ok for me by the way) It's nice that it's merged, that means I'll check how the idea works in our project very soon 🤤
thanks, it was nice to work on it! |
This is an idea, how authentication could be implemented. It's enabled automatically based on presents of global
securitySchemesand at least onesecurityrequirement for a handler's route. Checksample-http4s/authenticationfor an example of generated code andHttp4sAuthenticationTestfor a possible usage example