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

Jetty-12 insertHandler review #9297

Closed
gregw opened this issue Feb 2, 2023 · 2 comments · Fixed by #10792
Closed

Jetty-12 insertHandler review #9297

gregw opened this issue Feb 2, 2023 · 2 comments · Fixed by #10792
Assignees

Comments

@gregw
Copy link
Contributor

gregw commented Feb 2, 2023

Jetty version(s)
12

Enhancement Description

There are several context.xml files (examples, demos and documentation) that configure a context by using insertHandler on the context to insert a GzipHandler (or similar).

Whilst this still works for ee8/ee9, it does not work for ee10, as we wrap at the ServletContextHandler level, so any subsequent wrapping of the request/response is bypassed when we do a Request.as(request, ServletContextRequest.class);

I've had a few tries at working around this, primarily by introducing a Handler.wrap method, that looks for the container of the handler and inserts the wrapper above it. That way, the context itself would be wrapped by the GzipHandler, which works fine. But the problem is that at the point we execute a context.xml file, the contextHandler has not yet been added to the ContextHandlerCollection, thus it cannot be found by a search of the handler tree and we can't wrap it then.

We could perhaps modify the deployer so that it is happy for a context.xml file to return any type of Handler, so long as there is a ContextHandler within it. the xml would then configure the GzipHandler with the ContextHandler contained within it.

Another option is for the ServletHandler to "rebase" the API wrappers on the request/response it receives, so that any intervening wrapping is used. This is doable either as a very specific ee10 servlet mechanism, or even as a general Handler.Wrapper mechanism that signals if a request/response is wrapped so it can know it's outer wrapper.

However, for that to work efficiently, the EE10 SecurityHandler would need to no longer operate on API request/response but instead on core request/response. This is probably desirable anyway as it will give us core security. However, whilst we have core sessions, we don't have a standard way of accessing those, so we'd need to create some mechanism so that core security could see core sessions, even if those sessions are wrapped as API sessions. I think this is doable and probably a good idea anyway, regardless of the insertHandler issue.

@gregw
Copy link
Contributor Author

gregw commented Aug 29, 2023

I think this is now less important as we can indeed wrap within ee10 context.

@gregw gregw changed the title Jetty-12 insertHandler replacement Jetty-12 insertHandler review Oct 26, 2023
@gregw gregw linked a pull request Oct 26, 2023 that will close this issue
@joakime joakime moved this to 🏗 In progress in Jetty 12.0.3 - FROZEN Oct 26, 2023
@joakime joakime moved this to 🏗 In progress in Jetty 12.0.4 - FROZEN Nov 1, 2023
@joakime joakime moved this from 🏗 In progress to ✅ Done in Jetty 12.0.4 - FROZEN Nov 22, 2023
@joakime
Copy link
Contributor

joakime commented Nov 22, 2023

PR mounted

@joakime joakime closed this as completed Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

3 participants