[Feat] Request overriding _attachHandler() for custom authentication middleware

[DRSDavidSoft]

Hi again,

I would like to apologize beforehand for the wall of text below, but I was wondering if you could help me with some aspects of implementing this. 😅

I'm reworking my custom authentication handler, and I need to detect whether my Authentication handler needs to be called or not. One of the conditions is that the route actually exist, as I don't want to protect non-existent routes (think of robots.txt, favicon.ico, need to get 404).

Some context

Upon closer look it seems that ESPAsyncWebServer:

• Handles the routes that are assigned using .on('/url', ...) by creating a AsyncWebHandler of type AsyncCallbackWebHandler and add it to the list of _handlers that canHandle() the request.
• There is aslo onNotFound() that modify a special handler called _catchAllHandler on how it needs to behave when no other handlers are found, e.g. no routes are matched.

This is excellent, because to quote the docs:

[...] the server goes through all attached Handlers (in the order they were added) trying to find one that canHandle the given request. If none are found, the default (catch-all) handler is attached.

Now my AuthenticationHandler needs something to detect if the Handler that comes after is the _catchAllHandler or not. This means that I need to override the _attachHandler() (the method that is running through all Handlers to find one that canHandle()) to modify its logic a bit so that if there is a handler that can handle the request, the custom AuthenticationHandler is called first, otherwise the catch all handler is called, where we can respond to OPTIONS requests and for other requests send a 404.

Proposed change

Can you please either:

• Allow _attachHandler() to be overridable
• Introduce a new pointer function that would be called in the appropriate moment so that I can attach the AuthenticationHandler (prefrered)

Or, introduce a new method (such as addMiddleware(), or addHandler()) with a custom parameter) that would change the order that the handler can be attached.

Alternative approach

To avoid asking a potential XY Problem, let's see what the ideal approach for this problem could be.

I am trying to implement this custom handler, as this project doesn't rely on a username/password combo for authentication, instead it relies on a auth token (that optionally can be acquired through a login process with a username/password, but it's irrelevant to the authentication step). Think of it as an API key.

Now, ideally, there would be one place to manage and put the actual authentication logic, but it appears that this is spread all through the code.

• Regular requests

  if ((_username != "" && _password != "") && !request->authenticate(_username.c_str(), _password.c_str()))
    return request->requestAuthentication();

• WebSockets

  if ((_username.length() && _password.length()) && !request->authenticate(_username.c_str(), _password.c_str())) {
    return request->requestAuthentication();
  }

Each handler is separately looking for the username/password, and if set, individually trying to authenticate and respond with a requestAuthentication().

WebSocket and SSE go one step even further by implementing a custom _handshakeHandler():

• Add Custom Handshake Handlers for Websocket and EventSource Plugins me-no-dev/ESPAsyncWebServer#684

This is actually useful, but besides being dandy and all, has some drawbacks:

• The authentication method is restricted only to a username/password combo, with an additional token-based auth only for WS/SSE step
• The check for authentication is made in several places, and no control is given to the user regarding how, when and in which conditions it should be called
• There is a hard coded response code of 401 always being returned in case of an un-authenticated request, disallowing the user from using other types of responses (e.g. a 302 redirect, or a JSON response)

I would be happy if the request->authenticate() and request->requestAuthentication() part was actually overridable, I bet the original onHandshake() of WS/SSE wouldn't even be needed to be implemented if this was the case.

However, as this is not the case, the next best thing would be to completely forgo the built-in authentication handler in AsyncCallbackWebHandler/AsyncWebSocket and the one for Server Events, and instead implement a custom one from scratch; however it needs to be called in the right moment, hence the original request above.

With that being said, I would also appreciate it, if there was a re-factor to move all the duplicated request->authenticate()/request->requestAuthentication() calls to a shared method that would be overridable by the user, allowing us to attach the custom authentication handler.

Once again I am sorry for this wall of text, but these were my thoughts when reviewing the code for the library, and as I have previously said, I'm invested in a good and clear solutions that can be benefited from in the long run, instead of just patching the library for my own use case.

Thanks for taking time to read this, @mathieucarbou, I deeply appreciate your thoughts and feedback. 👍

[mathieucarbou]

I went through the same issues: there is no middleware support in ESPAsyncWS and the way it is coded it won't be added : the design does not allow it.

I added middleware support in Arduino WebServer (but PR still in review) and Psychic though (still in dev).

For ESPAsycnWS, the only option to put the auth code at a common place to not repeat all the String/password password duplications would be to add support for custom RequestFilter (aka middleware) (a new class) which could be added on some handlers, or globally to the server, would execute before them and could return true or false depending whether they allow the processing of the request to go further or not. Something like that.

I will have a look how feasible it is.

[DRSDavidSoft]

Thanks, I'd also appreciate it if the duplicated calls like this would be turned into a dedicated function:

if ((_username != "" && _password != "") && !request->authenticate(_username.c_str(), _password.c_str()))
  return request->requestAuthentication();

That would be awesome as it can be made overridable by the user. We can call it handleAuthentication(), and it would deal with the authentication using AsyncWebServer's built in methods, or using user's custom supplied once.

I'm also open to the _attachHandler() thing.

Oh by the way, one important thing to consider is that middlewares would allow attaching custom bits of information (such as userId or sessionId) to the request. With the current design it's not possible to attach any data to the request for further processing later... if something like this was possible, it would reduce duplicated calls later to re-acquire the same data.

[mathieucarbou]

accessing the internals like overriding _attachHandler is often not a good design because internals can change.

usually, generally, overriding is not a good idea because it creates a hard dependency over how the super class is made and works.

[DRSDavidSoft]

Good point, so the handleAuthentication() way seems morre feasible, correct?

[mathieucarbou]

Good point, so the handleAuthentication() way seems morre feasible, correct?

I don't like to specialis that to auth.

I will see if this is possible to add a sort of middleware feature, or improved filters instead. current filters decide if the handler is active or not, we need something in between.

With that, you could have a AuthcRequestFilter, AuthzRequestFilter, CorsRequestFilter, etc etc.

The only kind of filters not possible with ESPAsyncWS are the filters both:

1. acting on the response headers
2. and allowing the process to continue to the handlers

This is not possible because the handlers are responsible to create the response object and commit it. So no middleware can act on response headers in ESPASyncWS.

Psychic had the same design, which we are changing now in v2. But it breaks a lot of API. So that is why in ESPAsyncWS, middleware support can be added but can only be limited to request interception.

[DRSDavidSoft]

Oh, that's great, thank you! I hope implementing something like this can be done with minimal changes, so that it wouldn't require much development cost 👍🏻

Do you plan on keeping the old auths:

if ((_username != "" && _password != "") && !request->authenticate(_username.c_str(), _password.c_str()))
  return request->requestAuthentication();

or can you also move it to a handler of its own? The duplicated-ness makes it a great candidate to move them, without breaking existing backwards compatibility.

I'm so excited for PsychicHTTP BTW, as it seems when we eventually migrate to it, a lot of nice to have features will be present in it!

[mathieucarbou]

Do you plan on keeping the old auths:

I will see what is possible without breaking the current API.

[mathieucarbou]

@DRSDavidSoft : i have opened a PR here: #98
Please leave your comment / discussion in the PR to centralise the discussion there around middleware.
I will lock this conversation to avoid any double channels.