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

Authenticated routes for scala-http4s plugin #1409

Closed
kyri-petrou opened this issue Feb 20, 2022 · 13 comments
Closed

Authenticated routes for scala-http4s plugin #1409

kyri-petrou opened this issue Feb 20, 2022 · 13 comments

Comments

@kyri-petrou
Copy link

Hello there 👋

I'm a huge fan of this project and really appreciative of all the work that went into developing it - you guys rock 🎸

I was wondering whether there are plans to add support for Authenticated routes for the http4s plugin. Please excuse me it's already supported, I couldn't find anything in the docs or source code to indicate as such - if that's the case, a link to some doco or some basic instructions would be very much appreciated!

Thanks again!

@blast-hardcheese
Copy link
Member

@kyri-petrou Great timing! @zeal18 has been diligently working on this over in #1342. It's nearly complete, and if you don't mind switching to running the cli action directly in guardrail's sbt project, you can actually test his branch out to see if it works for you.

It would be great to have some more eyes on the approach we took, as well as to confirm the ergonomics is pleasant to use.

If you clone the guardrail repo, switch to @zeal18's branch, then run:

sbt> cli scala --server --specPath modules/sample/src/main/resources/authentication.yaml --outputPath modules/sample-http4s/target/generated --packageName authentication-simple.server.http4s --framework http4s --auth-implementation simple

you'll get a server generated using one of the sample specs we have for regression tests. By adjusting the locations of the different parameters to this sample CLI call (and temporarily disabling the build tool plugin you're using) you can test the generated code in your own project.

Please do report back and let us know what you think!

@kyri-petrou
Copy link
Author

Ah, perfect! I'll give it a go and let you know how it goes. Are you happy to keep this issue open for me to report any feedback or is there somewhere else I can do that?

@blast-hardcheese
Copy link
Member

@kyri-petrou I'm fine either way -- that issue and #1407 are top-of-mind for me currently, so no matter where you provide feedback (even in the gitter channel) it'll be fine.

The next release will include some minor but necessarily breaking changes to generated code, so even after that PR gets merged, I'll still be playing around with the UX before cutting the next minor release.

@kyri-petrou
Copy link
Author

kyri-petrou commented Feb 21, 2022

Got a couple of questions if that's okay:

  1. Is there a way to reuse existing AuthMiddleware implementations? I can't seem to be able to figure it out, although admittedly, my knowledge of how Kleisi works is not the greatest (to put it mildly)
  2. I can't seem to be able to get global security to work (see here: https://swagger.io/docs/specification/authentication/bearer-authentication/). Is that not implemented yet or am I perhaps doing something wrong?
  3. What is the difference between simple and custom authentication implementations?

@blast-hardcheese
Copy link
Member

  1. I'm working through a patch to that branch that could help with that. It needs a little more thought, but the core idea is to use BasicAuth.challenge instead of BasicAuth.apply and marshalling the Challenge directly
  2. I'll take a look. If you have a simplified specification and expected output, it would help to make sure we're talking about the same thing
  3. simple was an attempt to reduce the complexity of custom, which was the first implementation. The idea is that most will just need standard authentication schemes, so that was implemented as simple vs custom

@kyri-petrou
Copy link
Author

Awesome, thanks. From a UX perspective, I think l to be able to reuse authentication middleware(s), as this will also allow to seamlessly integrate with 3rd party libraries (e.g., http4s-jwt-auth)

As for (2) I've modified authentication.yaml slightly to show a potential usecase where all the routes require an API key to be present (see last 2 lines)

openapi: 3.0.2
info:
  title: Test for security support
  version: 1.0.0
paths:
  /foo:
    post:
      x-jvm-package: auth
      operationId: doFoo
      requestBody:
        required: true
        content:
          application/json:
            schema:
              type: string
      responses:
        200:
          description: ""
          content:
            application/json:
              schema:
                type: string

  /bar:
    post:
      x-jvm-package: auth
      operationId: doBar
      requestBody:
        required: true
        content:
          application/sdp:
            schema:
              type: string
      responses:
        200:
          description: ""
          content:
            application/json:
              schema:
                type: string

  /baz:
    post:
      x-jvm-package: auth
      operationId: doBaz
      requestBody:
        required: true
        content:
          application/sdp:
            schema:
              type: string
      responses:
        200:
          description: ""
          content:
            application/json:
              schema:
                type: string

components:
  securitySchemes:
    ApiKeyAuth:
      type: apiKey
      in: header
      name: X-API-KEY

# The API key is required / applied globally to all operations
security:
  - ApiKeyAuth: []

@zeal18
Copy link
Contributor

zeal18 commented Feb 22, 2022

Awesome, thanks. From a UX perspective, I think l to be able to reuse authentication middleware(s), as this will also allow to seamlessly integrate with 3rd party libraries (e.g., http4s-jwt-auth)

As for (2) I've modified authentication.yaml slightly to show a potential usecase where all the routes require an API key to be present (see last 2 lines)

openapi: 3.0.2
info:
  title: Test for security support
  version: 1.0.0
paths:
  /foo:
    post:
      x-jvm-package: auth
      operationId: doFoo
      requestBody:
        required: true
        content:
          application/json:
            schema:
              type: string
      responses:
        200:
          description: ""
          content:
            application/json:
              schema:
                type: string

  /bar:
    post:
      x-jvm-package: auth
      operationId: doBar
      requestBody:
        required: true
        content:
          application/sdp:
            schema:
              type: string
      responses:
        200:
          description: ""
          content:
            application/json:
              schema:
                type: string

  /baz:
    post:
      x-jvm-package: auth
      operationId: doBaz
      requestBody:
        required: true
        content:
          application/sdp:
            schema:
              type: string
      responses:
        200:
          description: ""
          content:
            application/json:
              schema:
                type: string

components:
  securitySchemes:
    ApiKeyAuth:
      type: apiKey
      in: header
      name: X-API-KEY

# The API key is required / applied globally to all operations
security:
  - ApiKeyAuth: []

I think in this case it's better to switch off guardrail's authentication and use AuthMiddlewares how it's described in the http4s documentation. The main motivation to implement an authentication on the guardrail side is to make it possible to authenticate considering scopes. But scopes are defined on routes level which forces to move authentication to the routes level too. http4s's middlewares approach works on "services" layer (groups of optional routes) which make it impossible to supply scopes information to the AuthMiddleware.

For me, adopting the AuthMiddleware to be compatible with guardrail's auth implementation doesn't make a lot of sense:

  • it loses scopes information which is the main reason to enable guardrail authentication
  • AuthMiddleware makes responses internally which potentially could be inconsistent with the spec's auth-errors http codes and response bodies

Summarising, I would suggest you to use pure http4s AuthMiddleware if:

  • all routes have the same authentication method
  • no scopes are used
  • no auth context is needed to be provided to a handler

and enable guardrail authentication if:

  • different routes have different authentication methods and couldn't (or not wanted) to be split to different groups (separate OpenAPI spec files)
  • there is a requirement to consider different scopes per route
  • a handler requires some data from authentication (for example a user_id extracted from a JWT token)

P.S.: the last point could become another option of the guardrail's authentication: just to enable AuthedRequest and pass it's content to a handler. This one could be used alongside with standard AuthMiddleware approach.

@kyri-petrou
Copy link
Author

P.S.: the last point could become another option of the guardrail's authentication: just to enable AuthedRequest and pass it's content to a handler. This one could be used alongside with standard AuthMiddleware approach.

Yeah I think that would be really good. The main issue I had using an AuthMiddleware from http4s was that the routes were not defined as AthenticatedRoutes. If that can be made available then that would be awesome

@zeal18
Copy link
Contributor

zeal18 commented Feb 23, 2022

Yeah I think that would be really good. The main issue I had using an AuthMiddleware from http4s was that the routes were not defined as AthenticatedRoutes. If that can be made available then that would be awesome

nice, I'll try to workaround this soon

@kyri-petrou
Copy link
Author

I've tried the experimental plugin out quite a bit, and I'm quite happy with it overall!

I think the only other comment I really have is maybe if there was a way to have a default response on unauthenticated calls instead of having to implement the response explicitly in every route, but that's quite minor.

Well done!

@zeal18
Copy link
Contributor

zeal18 commented Mar 17, 2022

Hi @kyri-petrou! I've implemented the idea we discussed above, check it out: #1342

@blast-hardcheese
Copy link
Member

Resolved with #1342, pending release

@kyri-petrou
Copy link
Author

Hi @kyri-petrou! I've implemented the idea we discussed above, check it out: #1342

Oops I must have missed the notification on this one. Awesome, I'll check it out!

Resolved with #1342, pending release

🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants