template,echo: add ability to use per-handler middlewares

[Snaipe]

This is essentially the same thing as #259, but for echo.

I could not find a way to access the OAuth scope of the route from the normal
middlewares, as the value gets set by ServerInterfaceWrapper. With this change,
it's now possible to add a middleware to check that the scope is valid prior
to running the actual handler.

This commit introduces a new Middlewares field to ServerInterfaceWrapper,
which is a slice of echo.MiddlewareFunc, that gets registered as per-handler
middlewares. It also adds a new registration function, RegisterHandlersWithOptions,
to allow users to specify these middlewares.

[Snaipe]

@deepmap-marcinr The change should not be breaking -- can you point out where it does? I'd rather try to keep this backward-compatible.

[deepmap-marcinr]

You're right, I misread, this isn't a breaking change.

It's kind of odd to call the middleware functions yourself, because it bypasses the echo middleware chain, and these middleware functions won't work like other middleware, in that they can't abort the the request because they happen independent of that chain.

When you register a handler with Echo, it's possible to specify per-handler middleware already, so we need to tie into that somehow.

I've written the generated code in many layers, so you can use whichever layer works for you, but I've not put that much thought into registering handlers. Clearly, I can't foresee how everyone will want to configure things, so I just did the basic thing so far - register all global functions onto a preconfigured echo.Echo or router, which contains middleware.

[Snaipe]

The main motivation for the change is that oapi-codegen generates handlers that set into the context the auth scope for the route, which happens a bit too late -- I think per-handler middlewares run before the generated handler stub too, so it's no different than regular middlewares -- which means that any middleware that checks the scope in the JWT can't have any access to the route scope value since it's not set by that point.

Maybe a better fix to address that would be to change the generator so that RegisterHandlers registers per-route middlewares and set their individual scopes in their respective contexts? If so then it might be possible to call Use() with the scope checker after RegisterHandlers.

[apamildner]

How's this progressing? @deepmap-marcinr I'm having the exact same problem. It would be way more convenient to have my JWT parsing middleware run which parse the scopes from the token. Then, we can compare them with the scopes defined for the given route before hitting the handler. Right now we have to do this check in all the handlers manually instead.

[deepmap-marcinr]

I've solved this problem by having my middleware look up the required scopes in the OpenAPI spec, and do that comparison, versus doing per-handler middleware. Let me see about merging this....

[deepmap-marcinr]

Here is why I am reluctant to merge this.

The routers such as echo support per-handler middleware, we're not using that functionality, but rather invoking the middleware ourselves in the generated code. This will be very different for different routers.

I think the answer may be to have a more flexible registration function which allows you to pass through middleware into the underlying router.

Maybe I'll come up with an example how to create a JWT handler which resolves paths/scopes correctly.

In our own production code, some services don't match the current RegisterHandlers behavior, so we simply register handlers ourselves, not using that function.

[apamildner]

I've added my own attempt at solving this, I'd like to hear what you guys think: #392
@deepmap-marcinr @Snaipe
It's my first contribution so please bear with me 👍

[Snaipe]

@apamildner RegisterHandlersWithOAuth2ScopeValidation seems overly specific for the exact problem being solved -- there has been other times where setting context in the handler function directly is too late and leaves no room for middleware inspection

@deepmap-marcinr I gave this some more thoughts and realized that the only good way to address this was to move the ctx.Set for scopes into its own dedicated middleware, and registering it as a per-handler one before any user-provided middlewares. Care to take another look?

[Snaipe]

@deepmap-marcinr gentle reminder to review these new commits 🙂

[deepmap-marcinr]

Since you are putting all the same middleware functions on each handler, how is this different from?

e := echo.New()
g := e.Group("", <your middleware list>)
api.RegisterHandlers(g)

This seems to accomplish the same exact thing without any changes to the generated code.

[Snaipe]

The problem is that the generated code currently populates the context with oauth scopes too late to be used by authorization middlewares. Even when moving the ctx.Set from the handler to a generated middleware, any middleware installed before RegisterHandlers would still not have any way to access it due to the registration order.

These new commits simply allows us to write a middleware that accepts or rejects requests based on whether the provided token contains the right scopes or not, which is simply not possible currently. We'd also like to eventually follow-up in another PR with some commits to add some additional context like accepted media-types in order to perform media-type negotiation from a middleware, all of which is unfortunately not possible if these ctx.Set stay in the handler function. (or, well, I guess we could manually add some boilerplate code that does the auth check + negotiation & whatever else is needed at the top of each and every handler, but this really sucks and goes against the spirit of what middlewares are for. Yuck!)

[apamildner]

@Snaipe I got around this problem by simply writing my own template file. The framework supports this out of the box, so it solved everything for me. Just extended the register.tmpl file as described in the docs. Then I could make the per handler middleware which accepts scopes during the templating where they are still available for each path.

[deepmap-marcinr]

I'm still missing something, I think. How is this different?

mw := myMiddleware()
api.RegisterHandlers(e, mw...)

versus

mw := my Middleware()
g := e.Group("", mw...)
api.RegisterHandlers(g)

We don't add any middleware by default, none whatsoever, so the order is up to you, however you populate that middleware list.

If we extend all the register functions to accept a middleware list, all that happens is that it gets appended to the existing middleware list on the echo.Ctx or Group that you call as your target. In this PR, all the middleware passed to each handler is the same.

I can see wanting to specify per handler middleware that's different for each handler, and yes, that is currently not supported via the codegen (or this PR).

[Snaipe]

I'm still missing something, I think. How is this different?

mw := myMiddleware()
api.RegisterHandlers(e, mw...)

versus

mw := my Middleware()
g := e.Group("", mw...)
api.RegisterHandlers(g)

Sorry, I don't think we're on the same page; as I mentioned before the auth scopes is set in the echo context in the generated handler function. The only way to possibly access it via a middleware is to install a generated middleware that sets the context and then add any middleware that the user might have.

In more practical terms, take the following openapi definition:

# openapi.yaml
openapi: 3.0.0

components:
  securitySchemes:
    auth:
      type: openIdConnect
      openIdConnectUrl: https://acme/.well-known/openid-configuration

security:
- auth: [some-auth-scope]

paths:

  /foo:
    get:
      security:
      - auth: [some-auth-scope]

And this go file:

package main

import (
	"fmt"

	"github.com/labstack/echo/v4"
)

const AuthScopes = "authscopes"

//go:generate go run github.com/deepmap/oapi-codegen/cmd/oapi-codegen -generate server -package main -o service_gen.go openapi.yaml

type Service struct {}

func (Service) GetFoo(c echo.Context) error {
	fmt.Println(c.Request().URL.Path, "scope in handler: ", c.Get(AuthScopes))
	return nil
}

func main() {
	e := echo.New()

	printScope := func(next echo.HandlerFunc) echo.HandlerFunc {
		return func(c echo.Context) error {
			fmt.Println(c.Request().URL.Path, "scope in middleware: ", c.Get(AuthScopes))
			return next(c)
		}
	}

	RegisterHandlers(e.Group("vanilla", printScope), Service{})
	RegisterHandlersWithOptions(e.Group("modified"), Service{}, RegisterOptions{
		Middlewares: []echo.MiddlewareFunc{printScope},
	})

	e.Logger.Fatal(e.Start(":4321"))
}

Generate, run, and test:

$ go generate .
$ go run . & 
⇨ http server started on [::]:4321
$ curl localhost:4321/vanilla/foo
/vanilla/foo scope in middleware:  <nil>
/vanilla/foo scope in handler:  [some-auth-scope]
$ curl localhost:4321/modified/foo
/modified/foo scope in middleware:  [some-auth-scope]
/modified/foo scope in handler:  [some-auth-scope]

You can clearly see that this just doesn't work for /vanilla. This cannot possibly work, because if the context is set in the handler function, it's already too late to do anything useful.

As far as I can tell, the only two ways to access that context is either:

1. manually call some user-provided middlewares inside the handler function, after the context is set, which is what the Chi generated code currently does, and was the first version of this PR, but had issues.
2. move the context set into a per-route middleware, and install it before any user-provided one, which is what this PR currently does. Because the context is set in a per-route middleware, all of the provided user middleware have to be installed as per-route middleware.

In both cases, user-provided middlewares need to run after whatever piece of code sets the context.

[deepmap-marcinr]

I'm looking over your example code right now. Please have a look at an example of how we use JWT auth with oapi-codegen in production. This is subtle enough that I decided to make a simple example of an authenticated API with per-handler scopes.

https://github.com/deepmap/oapi-codegen/tree/master/examples/authenticated-api

[deepmap-marcinr]

Who sets authscopes on ctx before it gets to handler or middleware?

[Snaipe]

As far as I see the authentication example is not the problem we have -- in fact we have something like this to check the JWT token against our OIDC provider. The problem is that we have some boilerplate code at the top of all of our handlers to check that the token has the right scopes for the endpoint, and we'd like to refactor that into a middleware.

Who sets authscopes on ctx before it gets to handler or middleware?

The generated code does, based on the scopes defined in the yaml file.

[Snaipe]

Oh I see, the example does get the scopes, but it does so via GetSwagger() in CreateMiddleware, that's rather confusing. Okay, having access to the spec indirectly should work for that purpose.

[deepmap-marcinr]

Oh, now I see too. Your custom generated code isn't in this discussion or in the CL, so I was really confused what you were discussing, since it looks like you're using a different approach than we considered.

[deepmap-marcinr]

So, you've generated some code, which sets authscopes on ctx in a deeper nesting level than some middleware that you are trying to run, right?

[Snaipe]

Well the generated code comes from the server target of oapi-codegen: https://github.com/deepmap/oapi-codegen/blob/master/pkg/codegen/templates/echo/echo-wrappers.tmpl#L29

The issue being that this value is fairly useless by itself.

[deepmap-marcinr]

I see. I had completely forgotten that code. I must have overlooked it in some PR.

[deepmap-marcinr]

Well, I dislike the code we have in our codegen now :), and I understand the need for your PR, as the generated code is at the innermost nesting level, so middleware wont have it available without some trickery, as you're doing.

Given that we have off-the-shelf middleware that can give you the request scopes, I really don't feel this is the answer. We're fixing an oapi-codegen error with another issue. What if we created a simple standalone middleware that you can put early into your middleware chain which set authscopes in whatever order you liked, based on loading the Swagger specification.

I think having the spec at runtime is invaluable, because you can do request validation entirely in middleware, and never need to check request structure yourself, as you can rely on middleware for that.

[Snaipe]

Yeah, I think I can see a better path for these kinds of middleware via GetSwagger(), I'll refactor our current codebase to make use of it. Thanks for the example!

[deepmap-marcinr]

Thanks for understanding. I really try to keep our code as dumb as possible, as being too smart causes more pain long term.