Skip to content
This repository was archived by the owner on Jan 21, 2025. It is now read-only.

Add support for Middleware - #98

Merged
mathieucarbou merged 1 commit into
mainfrom
middleware
Sep 14, 2024
Merged

Add support for Middleware#98
mathieucarbou merged 1 commit into
mainfrom
middleware

Conversation

@mathieucarbou

@mathieucarbou mathieucarbou commented Sep 7, 2024

Copy link
Copy Markdown
Owner

This is a first draft of middleware implementation in ESPAsyncWebServer (a.k.a. Espressif Middleware).

Benefits:

  • Remove duplication of authorisation creds on each handler
  • Ability to define "common" request interceptors: executed one after the other with the ability to decide to stop or not the processing
  • Centralise authc and authz in 2 midleware: AuthenticationMiddleware and AuthorizationMiddleware

Drawback:

  • std::list on each handler (more memory used upfront per handler)

Limitations:

  • Only request interception is possible in ESPASyncWebServer, not response modifiers

Backward compatibility:

  • Change is backward-compatible as for the API
  • But a little behaviour changed for AsyncEventSource where you must be sure to call setAuthentication BEFORE authorizeConnect (bot deprecated) if you want to keep the same ordering as before, which is to check for authc before checking authz.

Deprecations

Some methods are now deprecated in favour of middleware usage:

  • setAuthentication
  • authorizeConnect

Comment thread src/ESPAsyncWebServer.h Outdated
@DRSDavidSoft

DRSDavidSoft commented Sep 8, 2024

Copy link
Copy Markdown

@mathieucarbou This looks incredible—elegant and simple, while addressing all the original points! 👍🏻

Now, authentication can be checked in one place instead of several different locations, for WS/SSE and regular routes as well.

Since #97 is locked for discussions, I'll provide my feedback here as requested.

Does utilizing std::list significantly increase memory consumption? It seems that with the current implementation, _middlewares persists with the request until the Handler has finished executing. I assume each Handler itself could potentially free up middlewares in their handleRequest() method to conserve memory, as the middleware phase is already passed. Since the Handler is the last part of the code execution, this could allow for memory to be freed up for response generation.

One quick question:

Only request interception is possible in ESPASyncWebServer, not response modifiers

This is fine, as intercepting requests is often more useful than modifying responses, when designing for embedded applications. Response modifications can typically be handled by the Handlers themselves through custom implementations.

Out of curiosity, though, why is this a limitation? I assume we would need to use some sort of buffer if we wanted to modify the response, to keep the original response. The current implementation supports both "streamed" responses as well as responses where the entire content is already kept in memory. Could a response middleware have been feasible for the latter type of response?

Lastly, could you please add an example file once the PR is complete? The introduction of middlewares in ESPAsyncWebServer is surely incredibly useful, and new users would likely appreciate a quick example to implement in their own projects.

Thank you for adding this! I'll switch to the middleware branch to test it out now. 🙏

@DRSDavidSoft

Copy link
Copy Markdown

So, it appears that the _middlewares are being set, but they have not yet being invoked anywhere, so I guess the plan is to invoke the first attached middleware before the _attachHandler stage. Then we can clear the middlewares to conserve on memory.

@mathieucarbou

Copy link
Copy Markdown
Owner Author

So, it appears that the _middlewares are being set, but they have not yet being invoked anywhere, so I guess the plan is to invoke the first attached middleware before the _attachHandler stage. Then we can clear the middlewares to conserve on memory.

Yes, this is still WIP ;-) I just wanted to share the current state.

@mathieucarbou

mathieucarbou commented Sep 8, 2024

Copy link
Copy Markdown
Owner Author

Since #97 is locked for discussions, I'll provide my feedback here as requested.

This is the goal ;-)

Does utilizing std::list significantly increase memory consumption?

Yes, and std::list was using significantly at other places to. @vortigont can comment here.

  • For message queues and clients list there are many add and removes so this is required. This is a tradeoff between memory and speed
  • Same for headers and params

But for the handlers, rewrites and middlewares, they are usually setup once, not often removed, and there are not a lot of elements. So a vector should be fine. I thought about using a pointer too (since this is merely a forward list), but it greatly increase the code size, so the benefit is lost.

It seems that with the current implementation, _middlewares persists with the request until the Handler has finished executing. I assume each Handler itself could potentially free up middlewares in their handleRequest() method to conserve memory, as the middleware phase is already passed. Since the Handler is the last part of the code execution, this could allow for memory to be freed up for response generation.

No: handlers are like middleware and like filters: stateless objects (1 exception though) setup once, and all the requests go through them. There is no 1 middleware per request. So they have to stay in the handler. Otherwise, for next request, how the handler knows which middleware to execute ?

Out of curiosity, though, why is this a limitation?

This is because in ESpAsyncWS, response creation is the responsibility of the handler, not the library. So the response object does not exist yet when going through the middleware, and even if a middleware calls next() first to run some code AFTER the handler has sent the request, since the handler is responsible to handle the commit of the request, the middleware acting after the send won't be able to affect the response object.

This is still a valid use case, for example you could have a sort of logging middleware that would do:

      addMiddleware([fn](AsyncWebServerRequest* request, ArMiddlewareNext next) {
        long t = millis();
        next();
        Serial.printf("> $s %s %" PRIu32, request->methodToString(), request->url().c_str(), millis() - t);
      });

Lastly, could you please add an example file once the PR is complete?

Of course! Once it will be done I will add examples.

@mathieucarbou

Copy link
Copy Markdown
Owner Author

@DRSDavidSoft : PR changing std::list to std::vector for rewrites and handlers is here: #99

Comment thread src/AsyncWebSocket.cpp Outdated
Comment thread src/ESPAsyncWebServer.h Outdated
@DRSDavidSoft

Copy link
Copy Markdown

Yes, and std::list was using significantly at other places to. @vortigont can comment here.
But for the handlers, rewrites and middlewares, they are usually setup once, not often removed, and there are not a lot of elements. So a vector should be fine.

My knowledge of C++ is rusty here, so I would appreciate to understand how std:vectors help with the memory consumption as opposed to std:list. Thanks @vortigont!

handlers are like middleware and like filters: stateless objects setup once, and all the requests go through them. There is no 1 middleware per request. So they have to stay in the handler. Otherwise, for next request, how the handler knows which middleware to execute ?

You're right! I don't know why I assumed otherwise, I blame it on the lack of morning coffee 😅

If the middlewares are freed, then the subsequent requests wouldn't invoke them. Same for the handlers and rewrites.

(1 exception though)

Also curious about this!

response creation is the responsibility of the handler

That is correct, although it makes me think, what would happen if the Handler doesn't send a response? Never tested it out, curious what would happen!

This is still a valid use case, for example you could have a sort of logging middleware that would do

Yeap, it is still a valid use case. The only limitation is that we wouldn't be able to interact with the response object being sent.

I have some other questions as well:

#ifndef ESP8266
    [[deprecated("Use instead: addMiddleware(AuthenticationMiddleware)")]]
#endif

Is there some reason to deprecate calls like this only for all non-ESP8266 cores?

Additionally it can still be kept as an alias to middlewares and not be deprecated at all. This is just a suggestion, personally I prefer deprecating this for new projects, just a thought.

One thing I forgot to ask: would it be possible to attach some sort of extra data to the request and have it be destroyed at the end of the request? This is the userId or sessionId that I mentioned in the previous comment. Or, rather, anything else.

Maybe one middleware would create an object, populate it with some data, and attach it to the request (or even the other way around, the request would return a pointer or some sort of id that would be saved in that object)

So that, later in the lifetime of the request, we would be able to retrieve the object containing the additional data (that was created during a middleware) and access it data.

The freeing up part can theoretically be done using another middleware at the end.

@mathieucarbou
mathieucarbou force-pushed the middleware branch 2 times, most recently from e1db8d2 to 3974b9b Compare September 8, 2024 10:14
@mathieucarbou

Copy link
Copy Markdown
Owner Author

Yes, and std::list was using significantly at other places to. @vortigont can comment here.
But for the handlers, rewrites and middlewares, they are usually setup once, not often removed, and there are not a lot of elements. So a vector should be fine.

My knowledge of C++ is rusty here, so I would appreciate to understand how std:vectors help with the memory consumption as opposed to std:list. Thanks @vortigont!

There are plenty of writings for that on Internet. These are both lists but implemented with a different structure behind: vector with an array, and list with pointers to next and previous element, so each item holds 2 points on top.

An array usually also requires to shift elements and resize after removal, which is a costly operation that can hardly be done concurrently. When the size is unknown, an array will also require a reallocation, which is costly.

handlers are like middleware and like filters: stateless objects setup once, and all the requests go through them. There is no 1 middleware per request. So they have to stay in the handler. Otherwise, for next request, how the handler knows which middleware to execute ?

You're right! I don't know why I assumed otherwise, I blame it on the lack of morning coffee 😅

If the middlewares are freed, then the subsequent requests wouldn't invoke them. Same for the handlers and rewrites.

(1 exception though)

Also curious about this!

this is linked to the tmp object attached in a post body or file upload.

response creation is the responsibility of the handler

That is correct, although it makes me think, what would happen if the Handler doesn't send a response? Never tested it out, curious what would happen!

this does not change from the current behaviour. I did not check.

This is still a valid use case, for example you could have a sort of logging middleware that would do

Yeap, it is still a valid use case. The only limitation is that we wouldn't be able to interact with the response object being sent.

I have some other questions as well:

#ifndef ESP8266
    [[deprecated("Use instead: addMiddleware(AuthenticationMiddleware)")]]
#endif

Is there some reason to deprecate calls like this only for all non-ESP8266 cores?

ESP8266 does not compile with the [[deprecated("Use instead: addMiddleware(AuthenticationMiddleware)")]]

Additionally it can still be kept as an alias to middlewares and not be deprecated at all. This is just a suggestion, personally I prefer deprecating this for new projects, just a thought.

It has to be deprecated otherwise an AuthenticationMiddleware will be created for each handler. The usage now is to create only one AuthenticationMiddleware in user app which will be added to all handlers. I will also add the ability to add middlewares to the server globally.

One thing I forgot to ask: would it be possible to attach some sort of extra data to the request and have it be destroyed at the end of the request? This is the userId or sessionId that I mentioned in the previous comment. Or, rather, anything else.

What you are looking for is called request attributes / request session (depending on the scope). It is not yet supported in ESPAsyncWS but I can easily add it in this PR.

Maybe one middleware would create an object, populate it with some data, and attach it to the request (or even the other way around, the request would return a pointer or some sort of id that would be saved in that object)

So that, later in the lifetime of the request, we would be able to retrieve the object containing the additional data (that was created during a middleware) and access it data.

The freeing up part can theoretically be done using another middleware at the end.

this is exactly how it works usually, yes. you have an auth middleware that is checking auth, ans set the userId in the request object which is then available for other middleware and handlers

cleanup can be done in the same middleware. example:

      addMiddleware([fn](AsyncWebServerRequest* request, ArMiddlewareNext next) {
        // check auth
        /// userId = ...
        request.setAttribute("userId", userId);
        next(); // further middleware and handler can call String userid = request.getAttribute("userId");
        request.removeAttribute("userId");
      });

free is optional except if you have to use new()

@DRSDavidSoft

Copy link
Copy Markdown

It has to be deprecated otherwise an AuthenticationMiddleware will be created for each handler. The usage now is to create only one AuthenticationMiddleware in user app which will be added to all handlers.

Got it. So it is to avoid creating duplicate middlewares. 👍🏻

I will also add the ability to add middlewares to the server globally.

This also is useful addition, too!

What you are looking for is called request attributes / request session (depending on the scope). It is not yet supported in ESPAsyncWS but I can easily add it in this PR.

Please do! It would be super useful to be able to set and get attributes from different middlewares as well as the request itself, and would make the middleware support at least 10x more useful! 😄

this is exactly how it works usually, yes. you have an auth middleware that is checking auth, and set the userId in the request object which is then available for other middleware and handlers

Wow, this is exactly what I'm looking for! How about making the example file into some sort of complete authentication/authorization handler based on this, I'd like to contribute some code around the authentication logic, if I could.
(For example, a simple session manager that creates a token upon successful login, and on subsequent requests, compares the token against memory.)

cleanup can be done in the same middleware.

That is awesome! The example you provided is so clean and concise, this is for sure the best implementation of middlewares for ESPAsyncWebserver!

Also, using something like this I believe it would be possible to automatically free up any attached attributes at the end of the request's lifecycle.

The great thing about this implementation of request attributes could allow the user to attach a pointer to any kind of data structure for later use! Although, in this case the data needs to be freed manually, I assume.

These are all looking great and extremely useful for application development, wonder why something so useful as middleware support wasn't asked to be added to the ESPAsyncWebServer years ago...

This all makes me excited for PsychicHTTP as well, I'm sure it'd be even more amazing than ESPAsyncWS 😄

@mathieucarbou
mathieucarbou force-pushed the middleware branch 3 times, most recently from f657210 to deb0f83 Compare September 8, 2024 20:36
@mathieucarbou

mathieucarbou commented Sep 8, 2024

Copy link
Copy Markdown
Owner Author

@DRSDavidSoft : FYI #100 (request attributes)

@mathieucarbou mathieucarbou changed the title [WIP] Add Middleware support in ESPAsyncWebServer [WIP] Add support for Middleware Sep 8, 2024
@mathieucarbou
mathieucarbou force-pushed the middleware branch 2 times, most recently from 274b992 to 0bf4831 Compare September 9, 2024 22:10
@mathieucarbou mathieucarbou changed the title [WIP] Add support for Middleware Add support for Middleware Sep 13, 2024
@mathieucarbou
mathieucarbou marked this pull request as ready for review September 13, 2024 21:58
@mathieucarbou
mathieucarbou force-pushed the middleware branch 3 times, most recently from 322e456 to ea34ba1 Compare September 13, 2024 22:14
@mathieucarbou
mathieucarbou marked this pull request as draft September 13, 2024 22:20
@mathieucarbou
mathieucarbou force-pushed the middleware branch 2 times, most recently from fc0f8ac to cec1fa6 Compare September 14, 2024 00:38
@mathieucarbou
mathieucarbou marked this pull request as ready for review September 14, 2024 00:38
@mathieucarbou

Copy link
Copy Markdown
Owner Author

@DRSDavidSoft : the PR is now complete. You can start testing in your app. I will do the same and I will merge in main once testing is done.

You can look at the README in this PR and the SimpleServer sample for doc and use cases.

Finally, this is a complete implementation of middleware: we can act on the request but also on the response, and even replacing it (i.e. rate limit middleware)

  • AsyncMiddlewareFunction: can convert a lambda function (ArMiddlewareCallback) to a middleware
  • AuthenticationMiddleware: to handle basic/digest authentication globally or per handler
  • AuthorizationMiddleware: to handle authorization globally or per handler
  • CorsMiddleware: to handle CORS preflight request globally or per handler
  • ElapsedTimeMiddleware: to measure the time spent processing the request
  • HeaderFilterMiddleware: to filter out headers from the request
  • HeaderFreeMiddleware: to only keep some headers from the request, and remove the others
  • LoggerMiddleware: to log requests globally or per handler with the same pattern as curl
  • RateLimitMiddleware: to limit the number of requests on a windows of time globally or per handler

Comment thread README.md Outdated
Comment thread docs/index.md Outdated
@mathieucarbou
mathieucarbou force-pushed the middleware branch 10 times, most recently from b9ee913 to e9264d7 Compare September 14, 2024 12:15
@mathieucarbou

mathieucarbou commented Sep 14, 2024

Copy link
Copy Markdown
Owner Author

Hello @DRSDavidSoft ,
I've finished testing, works fine os far.
I will merge this PR and issue a release.
If you see something, let me know!

@vortigont : same goes for you ;-)

All the latest changes are mostly additive (except a few things).

@mathieucarbou
mathieucarbou merged commit 92bdd17 into main Sep 14, 2024
@mathieucarbou
mathieucarbou deleted the middleware branch September 14, 2024 14:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants