Skip to content
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

feat: add support for http4s AuthedRoutes middleware #374

Open
wants to merge 1 commit into
base: master
from

Conversation

@ingarabr
Copy link

commented Aug 27, 2019

http4s include a middleware for authenticating users. To enable this feature you need to add the security configuration to the openApi specification (see server3 for an example) and include the --http4sAuthedRoutes argument when generating the code. Note, this is only the server side
implementation. The client already has the option to pass headers.

Upgraded http4s since AuthedRoutes isn't available in 0.20.0. I also bumped into #179 so that's why some of the code is commented out.

Signed-off-by: Ingar Abrahamsen ingar.abrahamsen@tapad.com

Contributing to Twilio

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.

@ingarabr ingarabr force-pushed the ingarabr:authed-routes branch 2 times, most recently from a16f7f7 to 2605867 Aug 27, 2019

feat: add support for http4s AuthedRoutes middleware
http4s include a middleware for authenticating users. To enable this
feature you need to add the `secutity` configuration to the openApi
specification (see server3 for an example) and include the `--http4sAuthedRoutes`
argument when generating the code. Note, this is only the server-side
implementation. The client already has the option to pass headers.

Signed-off-by: Ingar Abrahamsen <ingar.abrahamsen@tapad.com>

@ingarabr ingarabr force-pushed the ingarabr:authed-routes branch from 2605867 to d933962 Aug 27, 2019

@blast-hardcheese
Copy link
Collaborator

left a comment

Please pardon the delay on reviewing this, and thank you for your effort so far.

type: array
items:
$ref: '#/definitions/SecureAddress'
# '401':

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Sep 2, 2019

Collaborator

Was this commented out due to #179? If so, the fix has been merged, hopefully this can get uncommented

@@ -1,7 +1,7 @@
package com.twilio.guardrail

case class Context(framework: Option[String], tracing: Boolean)
case class Context(framework: Option[String], tracing: Boolean, http4sAuthedRoutes: Boolean)

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Sep 2, 2019

Collaborator

This PR represents a significant time investment, I appreciate what you've done so far. One of the design principles that may not have been clear is that the core of guardrail does not actually know anything about implementation details of any libraries; as a result, having a top-level context boolean that only applies to a single framework is undesirable.

An alternate strategy could be what we do in the dropwizard generator, where we build SecurityRequirements both globally (set top-level in the OpenAPI specification) as well as locally (configured for an individual operation).

You could then use the presence or absence of SecurityRequirements to enable or disable this functionality.

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Sep 2, 2019

Collaborator

One other consideration here is that as this is a global boolean, there's no convenient way to combine authed and unauthed paths in the same configuration. By using this security context configuration, it could be turned on and off by route.

This comment has been minimized.

Copy link
@ingarabr

ingarabr Sep 3, 2019

Author

I expected some feedback here. As you mention, it didn't fit so well with the rest of the design. The intention here was to not change the behavior of the existing solution if you didn't want to opt-in to it. I'll do another pass on it and see what I can find a better way to solve it.

val authedRoutes =
if (authedRouteTerms.isEmpty) None
else Some(q"""
def authedRoutes[U](..${routesParams}): AuthedRoutes[U, F] = AuthedRoutes.of {

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Sep 2, 2019

Collaborator

Instead of defining a new authedRoutes function and requiring folk to call it, why not alter the return type of routes to yield a tuple of the non-authed routes as well as the authed routes?

If we take this approach, when a user switches on authed functionality, the compiler error makes it very obvious what needs to happen next.

This comment has been minimized.

Copy link
@ingarabr

ingarabr Sep 3, 2019

Author

👍 Should be easy to change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.