search

jetty / jetty.project

Eclipse Jetty® - Web Container & Clients - supports HTTP/2, HTTP/1.1, HTTP/1.0, websocket, servlets, and more
https://eclipse.dev/jetty
Other
3.84k stars 1.91k forks source link

Use sessionRequest for wrapping HTTP stream instead of original Request #12303

Closed robbie01 closed 1 day ago

robbie01 commented 3 days ago

This fixes flushOnResponseCommit logic, which is currently broken due to SessionHandler.getManagedSession returning null. With this, getManagedSession will return the SessionRequest.

janbartel commented 3 days ago

@robbie01 thanks for the PR. Can you please also modify the SessionHandlerTest so it reveals the problem (and tests that it's solved).

robbie01 commented 3 days ago

@janbartel Added a test case and squashed. Looks like it catches; it fails for request and succeeds for sessionRequest. Please let me know if anything else isn't up to the project's standards.

janbartel commented 3 days ago

@robbie01 this is looking good, thank you! It has even revealed a problem with the core OpenId stuff - whether that's a test setup issue or an implementation issue I'm not sure yet, but I've raised #12307. I think we might need to fix that first before we can commit your fix, otherwise we'll break the build. Stand by.

janbartel commented 3 days ago

@robbie01 actually I discovered the problem was deeper than the OpenId stuff - there was a bug in core security SessionAuthentication: see #12309 and my fix in #12310. When I get that applied to jetty-12.0.x, can you rebase your PR please.

robbie01 commented 2 days ago

@janbartel Absolutely, I'll have that taken care of once it's ready.

janbartel commented 2 days ago

@robbie01 ok, you're good to do a rebase now.

robbie01 commented 2 days ago

@janbartel Done, thanks!