[WIP] AuthedContext implementation #184

eklavya · 2017-07-08T17:59:13Z

An uninformed and naive attempt at implementing an AuthedRhoService for #14
The code is very likely poor quality and needs many changes, I hope it's not too bad 😛

The test is not working at the moment. Will need help with that.
Tested separately with a dummy API, seems to work with http4s authentication middleware.

eklavya · 2017-07-08T18:08:32Z

One more point, the auth part is not visible to swagger (what headers or auth info is required).

bryce-anderson · 2017-07-11T03:47:35Z

Have you seen the function

def genericRequestHeaderCapture[R](f: Request => ResultResponse[R]): TypedHeader[R :: HNil]

and the >>> operator? I think you could build a lot of what you need using those tools. That should make this much less extensive and fit in a bit better with the current dsl. In my minds eye, you end up with something like this

object UserAuth {
  case class User(name: String, id: UUID)

  implicit val userAuthInfo = new AuthInfo[User] {
     ...
  }
}

object Auth {
  def authRule[T: AuthInfo]: TypedHeader[T :: HNil] = 
    genericRequestHeaderCapture { request =>
      auth stuff...
    }
}

val myService = new RhoService {
  GET / "auth" >>> Auth.authRule[UserAuth.User] |>> { user: UserAuth.User =>
    whatever...
  }
}

eklavya · 2017-07-11T04:27:55Z

Ok, yeah that would be a lot less everything 👅
However, how can I mix that with AuthedService. The main issue is being able to interoperate with the authentication middleware of http4s.

bryce-anderson · 2017-07-11T14:45:02Z

Sounds like I didn't fully understand what you're trying to do. I'll take another look at it this evening.

bryce-anderson

I don't know what exactly is going on, but it seems to me that the main goal is to turn a rho generated HttpService into a AuthedService, eg Kleisli[Task, Request, Response] => Kleisli[Task, (U, Request), Response] (basically an inverse AuthMiddleware[U]) by stuffing the U into an attribute entry and pulling it back out in the service definition. The first part can be done via composition and I think the nicest way to do the second part would be via a path rule made with genericRequestHeaderCapture, but a simple extractor is fine too.

bryce-anderson · 2017-07-15T16:21:46Z

core/src/main/scala/org/http4s/rho/CompileService.scala

+    * @param ev [[AuthInfo]] instance for U
+    * @return An `AuthedService`
+    */
+  def foldAuthedServices[U](routes: Seq[RhoRoute.Tpe], filter: RhoMiddleware = identity)(implicit ev: AuthInfo[U]): AuthedService[U] = {


Why can't this live in AuthedServiceBuilder? Furthermore, it seems like it's only purpose is to add the entry to the attribute map so couldn't it be

val rhoService = foldServices(routes, filter) Service.lift { case AuthedRequest(auth, req) => rhoService(req.withAttribute ...) }

I just wanted to conform to the overall existing structure.

bryce-anderson · 2017-07-15T16:29:33Z

core/src/main/scala/org/http4s/rho/AuthInfo.scala

+  *
+  * @tparam U Authentication info type for ex: User(id: UUID...)
+  */
+trait AuthInfo[U] {


Why do we need to serialize and then deserialize the type U when stuffing U in and pulling it out of the attribute map? Why not just put type U in there?

Didn't realise attributeMap allows to add any T !

bryce-anderson · 2017-07-15T16:31:40Z

core/src/main/scala/org/http4s/rho/AuthedRhoService.scala

+    new AuthedRhoService(serviceBuilder.routes().map { prefix /: _ })(ev)
+  }
+
+  def withAuth[R](req: Request)(f: U => R): R = {


Why do we need a closure here? It doesn't do anything but extract U, so why not make it a simple getter:

def getUath[U](req: Request): U = ...

No reason, just an artefact of my usages in the project 👅

eklavya · 2017-07-15T18:08:52Z

@bryce-anderson thanks for taking the time to review. I have addressed the review comments, any stupidity is purely unintentional 😄

bryce-anderson

I think we're getting closer.

bryce-anderson · 2017-07-15T19:33:13Z

core/src/main/scala/org/http4s/rho/AuthedServiceBuilder.scala

+import scala.collection.immutable.VectorBuilder
+
+/** CompileService which accumulates routes and can build a `AuthedService` */
+final class AuthedServiceBuilder[U] private(internalRoutes: VectorBuilder[RhoRoute.Tpe]) extends ServiceBuilder(internalRoutes) {


I don't think we need this class at all: the important part

def toAuthedService(filter: RhoMiddleware = identity): AuthedService[U] = { val rhoService = toService(filter) Service.lift { case AuthedRequest(authInfo, req) => rhoService(req.withAttribute[U](authKey, authInfo)) } }

can be inlined into AuthedRhoService.

bryce-anderson · 2017-07-15T19:37:04Z

core/src/main/scala/org/http4s/rho/AuthedRhoService.scala

+  * @param routes Routes to prepend before elements in the constructor.
+  *
+  */
+class AuthedRhoService[U](routes: Seq[RhoRoute[_ <: HList]] = Vector.empty)


I'm not convinced this needs to be a full DSL: all you really want is the toAuthedService and getAuth[U] functions which you could just use from an instance of this without having all the noise of another DSL class. This also helps to mitigate the AttributeKey issue described below.

I am not sure if I understand what you mean.

What I mean is that function composition gives you the vast majority of stuff you need. For example, the class below would work for you:

class AuthContext[T] { private val key = AttributeKey[U]("key") def getUser(req: Request): T = req.attributes.get(key).get def transform(rhoService: Service[Request, Response]): Service[AuthedRequest[U], Response] = Service.lift { case AuthedRequest(authInfo, req) => rhoService(req.withAttribute[U](key, authInfo)) } }

Then you just need to have this instance's getUser method when you construct your RhoService so you can actually get your user, and then use the same instance to transform it into an AuthedService. Note that this isn't a rho specific tool. The only downside is that nobody is stopping you from forgetting the second part and then getUser will explode since it clearly won't find the user in the attribute map. That last part is the only thing that AuthedRhoService really buys us.

Is the downside enough incentive to have a minimal DSL on AuthedRhoService?

In fact I think, it would be very unpleasant (maybe even impossible) to do it without making it a DSL. You can't call getAuth inside a RhoService which you need to give to transform.

It's totally possible and not that bad:

object MyAuth extends AuthContext[User] object MyService extends RhoService { import MyAuth._ GET/ "hello" |>> (req: Request) => { val user = getAuth(req) ??? } }

You are right !! Making changes.

bryce-anderson · 2017-07-15T19:39:35Z

core/src/main/scala/org/http4s/rho/AuthedRhoService.scala

+    * @return A new [[AuthedRhoService]] that contains the routes of the other service appended
+    *         the the routes contained in this service.
+    */
+  final def and(other: AuthedRhoService[U]): AuthedRhoService[U] = new AuthedRhoService(this.getRoutes ++ other.getRoutes)


This is going to have problems with the referential equality of AttributeKey: when you define a route, you use the getAuth of the DSL instance where it is defined. However, the conversion of the routes to an AuthedService uses the key from the DSL instance performing the compilation, resulting in different keys.

bryce-anderson · 2017-07-15T19:41:14Z

core/src/main/scala/org/http4s/rho/ServiceBuilder.scala

@@ -7,7 +7,7 @@ import shapeless.HList
 import scala.collection.immutable.VectorBuilder

 /** CompileService which accumulates routes and can build a `HttpService` */
-final class ServiceBuilder private(internalRoutes: VectorBuilder[RhoRoute.Tpe]) extends CompileService[RhoRoute.Tpe] {
+class ServiceBuilder (internalRoutes: VectorBuilder[RhoRoute.Tpe]) extends CompileService[RhoRoute.Tpe] {


See the comments above about why we don't need to extend this. If you're going to change this file at all lets make the class private as well.

bryce-anderson

I like where this landed. My only points of discussion are names: AuthedRhoService isn't really a service. How about AuthedContext? Along those lines, does this belong in rho? Nothing in here is rho specific other than the names and that it turns a RhoService into an AuthedService, where it could just as well receive a generic HttpService. I'm not against it, but it could just as well live in http4s, albeit that projects style tends to frown on things like Option.get etc.

eklavya · 2017-07-18T02:44:05Z

The only reason for using RhoService instead of HttpService was just to remove one more toService call on user side. Calling get here removes some boilerplate on the user side. In my opinion it's not too bad since a wrong implementation (where service is not an AuthedService) would fail pretty soon in a project and is a done deal (you do it only once).

I don't completely agree that this is not rho specific because it's all about making rho work with the auth middleware of http4s. That said, this solves my purpose and it's ok if it's not merged. I did see two issues about this so maybe this is needed elsewhere too. But it's entirely your call :)

bryce-anderson · 2017-07-18T04:57:11Z

I'm happy to merge it into rho and if others find utility outside, we can talk about moving it to http4s proper. I can merge it as is, or if you're interested, we could integrate this with the rho dsl using a custom path rule:

final val auth: TypedHeader[U :: HNil] = rho.genericRequestHeaderCapture { req =>
    req.attributes.get(authKey) match {
      case Some(user) => SuccessResponse(user)
      case None => FailureResponse.error("Invalid auth configuration")
    }
  }

Then we can use it in the dsl like such:

object MyService extends RhoService {
  import MyAuth._

  GET +? param("foo", "bar") >>> auth |>> { (req: Request, foo: String, user: User) =>
    if (user.name == "Test User") {
      Ok(s"just root with parameter 'foo=$foo'")
    } else {
      BadRequest("This should not have happened.")
    }
  }
}

With even a little more work, that custom rule could probably include the metadata necessary for integration with the generated swagger docs, but that might be a significant amount of more work 😄.

If you're not interested in the dsl integration I can take care of it later. Thanks for the good work, and sorry this has taken so long but I think we've arrived at a really good commit.

eklavya · 2017-07-18T05:36:23Z

Thanks @bryce-anderson 😄
I can put more effort on this, sure.

eklavya · 2017-07-18T06:02:11Z

I copy pasted your DSL code. I have no idea how to capture what http4s auth middleware is doing though.

bryce-anderson · 2017-07-19T03:15:53Z

Merged! Thanks for the PR, and once again sorry for the lengthy back and forth but I think it paid off.

zizhengtai · 2017-07-19T03:25:01Z

Thanks guys. Will this be usable in a snapshot version perhaps? Authentication is the only thing that prevents us from using Rho at the moment.

eklavya · 2017-07-19T03:31:47Z

@bryce-anderson thanks for your time and patience 😄

A release with this would be really helpful.

Please let me know if you would like me to work on the swagger thing.

bryce-anderson · 2017-07-19T03:56:30Z

Let me check when the next milestone release is for http4s proper: if it's close I'd like to wait for it.

For http4s#14 AuthedRhoService implementation

ede94be

bryce-anderson suggested changes Jul 15, 2017

View reviewed changes

changes for review comments

6c71744

bryce-anderson reviewed Jul 15, 2017

View reviewed changes

eklavya added 2 commits July 17, 2017 20:48

changes for review comments

eeabac8

make ServiceBuilder private again

1e850a6

bryce-anderson reviewed Jul 18, 2017

View reviewed changes

bryce-anderson approved these changes Jul 18, 2017

View reviewed changes

Rename AuthedRhoService to AuthedContext

b090e9d

eklavya force-pushed the issue/14 branch from 960ded5 to b090e9d Compare July 18, 2017 02:47

eklavya changed the title ~~[WIP] AuthedRhoService implementation~~ [WIP] AuthedContext implementation Jul 18, 2017

Add auth DSL to AuthedContext

1eea6d5

bryce-anderson approved these changes Jul 19, 2017

View reviewed changes

bryce-anderson merged commit ba44d82 into http4s:master Jul 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] AuthedContext implementation #184

[WIP] AuthedContext implementation #184

eklavya commented Jul 8, 2017

eklavya commented Jul 8, 2017

bryce-anderson commented Jul 11, 2017

eklavya commented Jul 11, 2017

bryce-anderson commented Jul 11, 2017

bryce-anderson left a comment •

edited

Loading

bryce-anderson Jul 15, 2017

eklavya Jul 15, 2017

bryce-anderson Jul 15, 2017

eklavya Jul 15, 2017

bryce-anderson Jul 15, 2017

eklavya Jul 15, 2017

eklavya commented Jul 15, 2017

bryce-anderson left a comment

bryce-anderson Jul 15, 2017

bryce-anderson Jul 15, 2017

eklavya Jul 17, 2017

bryce-anderson Jul 17, 2017

eklavya Jul 17, 2017

eklavya Jul 17, 2017

bryce-anderson Jul 17, 2017

eklavya Jul 17, 2017

bryce-anderson Jul 15, 2017

bryce-anderson Jul 15, 2017

bryce-anderson left a comment

eklavya commented Jul 18, 2017

bryce-anderson commented Jul 18, 2017 •

edited

Loading

eklavya commented Jul 18, 2017

eklavya commented Jul 18, 2017

bryce-anderson commented Jul 19, 2017

zizhengtai commented Jul 19, 2017 •

edited

Loading

eklavya commented Jul 19, 2017

bryce-anderson commented Jul 19, 2017

[WIP] AuthedContext implementation #184

[WIP] AuthedContext implementation #184

Conversation

eklavya commented Jul 8, 2017

eklavya commented Jul 8, 2017

bryce-anderson commented Jul 11, 2017

eklavya commented Jul 11, 2017

bryce-anderson commented Jul 11, 2017

bryce-anderson left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eklavya commented Jul 15, 2017

bryce-anderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryce-anderson left a comment

Choose a reason for hiding this comment

eklavya commented Jul 18, 2017

bryce-anderson commented Jul 18, 2017 • edited Loading

eklavya commented Jul 18, 2017

eklavya commented Jul 18, 2017

bryce-anderson commented Jul 19, 2017

zizhengtai commented Jul 19, 2017 • edited Loading

eklavya commented Jul 19, 2017

bryce-anderson commented Jul 19, 2017

bryce-anderson left a comment •

edited

Loading

bryce-anderson commented Jul 18, 2017 •

edited

Loading

zizhengtai commented Jul 19, 2017 •

edited

Loading