OF-2717: Allow plugins to register/unregister web context

[guusdk]

The embedded webserver (Jetty) that powers the HTTP-Bind (BOSH) and Websocket endpoints can be used by plugins, for them to add web content of their own.

To do so, a plugin can dynamically add a 'handler' when it gets loaded in Openfire (that handler should be removed again when the plugin gets unloaded).

Due to changes to the Jetty API (which got updated from 10 to 12, using EE8), this mechanism was broken.

Prior to the upgrade, a org.eclipse.jetty.server.handler.HandlerList was used to maintain the collection of plugin-provided contexts. After upgrading to Jetty 12, this was replaced with a org.eclipse.jetty.server.handler.ContextHandlerCollection. This was part of 99fe5f9#diff-55709739e79c9b3141bb106d4e4cf428d2bc54e46acf59450c3d9e8cf988385b

The methods used by plugins to add and remove 'handlers' were not modified in that change. The 'handler' passed to them remained of type org.eclipse.jetty.server.Handler. This compiles fine, but seems to be a problem.

When plugins (which are stand-alone projects with their own source code repositories) get updated to use Jetty 12 instead of 10, their representation of their web-context switches from org.eclipse.jetty.webapp.WebAppContext to the Jetty 12 EE8 org.eclipse.jetty.ee8.webapp.WebAppContext. The former is compatible with the old implementation in Openfire, but the latter is not.

This commit adds new methods to register and unregister (Jetty 12 EE8) web contexts.

I considered leaving the original methods (possibly marked as being deprecated) in hopes of providing backwards compatibility. However old plugins that are compiled against Jetty 10 won't find the Jetty classes that they need in Openfire anyway (as Jetty is a 'provided' dependency, which now got updated), causing ClassCastExceptions to be thrown. That makes retaining the original add/remove methods (for backwards compatibility) rather pointless.

[guusdk]

What does not sit well with me is that when adding a org.eclipse.jetty.ee8.nested.ContextHandler to org.eclipse.jetty.server.handler.ContextHandlerCollection, we can do that 'directly' (by virtue of it being a Supplier, I guess).

When removing that same ContextHandler, we need a bit of magic to make the round peg fit the square hole (by calling ContextHandler#get() to get an object that's compatible with the API of org.eclipse.jetty.server.handler.ContextHandlerCollection#removeHandler(). This makes me wonder if we're using the right approach here. @joakime, if you're willing/able to have a look: I would love your take on this.

[joakime]

Does your use of org.eclipse.jetty.ee8.nested.ContextHandler use any ee8 specific features? (like servlets? or servlet apis?)

Can it be a non servlet implementation?
if so, use the org.eclipse.jetty.server.handler.ContextHandler version instead.

If you must use servlet features, then you are bound by the servlet environment that is provided by the ee8 environment.

[guusdk]

Thanks @joakime! From your feedback, I think I can conclude that there's not a better way of registering/unregistering contexts. That weird non-symmetry between registration and de-registration made me question if this is the way this API is intended to be used. Is that fair to say?

This Openfire code is part of a public API against which third-party developers can code, so there's no telling what they might use. It's certainly not unreasonable to assume that some of them want to use servlet APIs.

Does it make sense to expose both options explicitly?

[joakime]

Either leave this as a org.eclipse.jetty.server.Handler or provide a new addJettyHandler(org.eclipse.jetty.server.Handler handler) as a secondary implementation allowing your users to enable other jetty features (like GzipHandler, CompressionHandler, QoSHandler, RewriteHandler, etc)

[joakime]

It there any reason you don't want your users to just add Servlets or Filters to your existing context instead?

[guusdk]

This has historically been done to somewhat isolate the functionality that is being provided by individual plugins, both from each-other, as well as from the functionality that's provided by Openfire itself on that web context.

[joakime]

Do you prevent two plugins from using the same overlapping context selection rules?
A ContextHandler is selected from the ContextHandlerCollection based on ...

• The context-path declared on the ContextHandler
• The incoming path-in-context for request when it reaches the ContextHandlerCollection
• The virtual-host declarations (either direct, or named)

To oversimplify this ...
If more than one ContextHandler entry results from the selection list above, then the resulting list is sorted, and then each entry is given the request to process following Jetty Core Handler rules, stopping when one of the handlers declares itself to be the one handling the generation of the response.

Is that something you attempt to prevent? or let the plugins do what they want?

[guusdk]

For the web-based content that is provided through the API that we're discussing in this PR, I don't think that there is an explicit check that plugin-provided context doesn't overlap. It is expected that plugins provide their own unique context, eg:

CONTEXT_ROOT = "jsxc"; // Expected to be unique - typically the name of the plugin.

...

context = new WebAppContext( null, pluginDirectory.getPath() + File.separator + "classes/", "/" + CONTEXT_ROOT );

Although possibly not ideal, I believe that this is 'good enough'. We've been doing this for close to two decades now. I'm not aware of any issue that stems from a naming collision.

[joakime]

Just to expand on what I said before ...

The org.eclipse.jetty.server.handler.ContextHandlerCollection holds non-environment core org.eclipse.jetty.server.handler.ContextHandler instances.

The ee8 org.eclipse.jetty.ee8.nested.ContextHandler is not core context handler.
As it attempts to maintain the servlet-ness of the ContextHandler found in Jetty 10/11.

When you upgrade to ee10 or ee11 (with Jetty 12.1.x) then that "servlet-ness" goes away, as there's no old Jetty version to attempt to maintain behavior with.

[guusdk]

I must admit that my lack of knowhow makes it a bit hard to follow. Maybe it's easier to illustrate things with an example.

One of the Openfire plugins that adds content to Jetty is a plugin that adds a web-based chat client to Openfire. This plugin (called the JSXC-plugin) uses Jetty in two distinct ways:

• the plugin contains JSPs (pre-compiled) that add to very similar JSPs provided by Openfire, that make up a web-based administrative console. The plugin-provided JSPs allow for configuration of the web-client that's being added by the plugin.
• the webclient itself is a Javascript plugin, that is configured through a bit of JSON. That JSON is dynamically generated by a servlet.

My approach to make the pre-existing plugin code compatible with Jetty 12 (as well as the change in this PR) can be found here: igniterealtime/openfire-jsxc-plugin#25

[joakime]

I must admit that my lack of knowhow makes it a bit hard to follow. Maybe it's easier to illustrate things with an example.

One of the Openfire plugins that adds content to Jetty is a plugin that adds a web-based chat client to Openfire. This plugin (called the JSXC-plugin) uses Jetty in two distinct ways:

* the plugin contains JSPs (pre-compiled) that add to very similar JSPs provided by Openfire, that make up a web-based administrative console. The plugin-provided JSPs allow for configuration of the web-client that's being added by the plugin.

JSPs would mean you are either using a ServletContextHandler or a WebAppContext.
I say this, because JSP requires a (jakarta|javax).servlet.ServletContext to function.
A raw ContextHandler is insufficient for JSP to function.

[guusdk]

Openfire uses two different mechanisms:

JSPs provided by a plugin that are intended to be part of the admin console are not added through the context that's being added by the API that's the subject of this PR. Instead, the per-compilation generates a web.xml like the one below. They are then merged into a custom servlet that has already been initialized.

<?xml version="1.0" encoding="UTF-8"?>
<web-app xmlns="http://xmlns.jcp.org/xml/ns/javaee"
         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:schemaLocation="http://xmlns.jcp.org/xml/ns/javaee http://xmlns.jcp.org/xml/ns/javaee/web-app_3_1.xsd"
         version="3.1">
<context-param><param-name>org.eclipse.jetty.ee8.jsp.precompiled</param-name><param-value>true</param-value></context-param>

<!--
Automatically created by Apache Tomcat JspC.
-->


    <servlet>
        <servlet-name>org.jivesoftware.openfire.plugin.jsxc.jsxc_002dconfig_jsp</servlet-name>
        <servlet-class>org.jivesoftware.openfire.plugin.jsxc.jsxc_002dconfig_jsp</servlet-class>
    </servlet>

    <servlet-mapping>
        <servlet-name>org.jivesoftware.openfire.plugin.jsxc.jsxc_002dconfig_jsp</servlet-name>
        <url-pattern>/jsxc-config.jsp</url-pattern>
    </servlet-mapping>

<!--
End of content automatically created by Apache Tomcat JspC.
-->

</web-app>

This all is separate from the optional extension point that's the subject of this PR, through which a plugin could add ... whatever they want ... to Jetty, in order to make use of its functionality. In practice, that's mostly going to be servlet-based functionality, like the bit in JSXC that I pointed to in my previous commit. It uses something like this:

// Add the Webchat sources to the same context as the one that's providing the BOSH interface.
context = new WebAppContext( null, pluginDirectory.getPath() + File.separator + "classes/", "/" + CONTEXT_ROOT );
context.setClassLoader( this.getClass().getClassLoader() );

HttpBindManager.getInstance().addJettyHandler( context );

To make available a web-app (as defined here). This happens on a different different Jetty embedded server instance than the one that serves the admin console (as the webapp serves user-facing, public content, while the admin console serves restricted, administrative content).

[hqv126]

Is the following alternative approach, which may have been mentioned in part in previous comments on this PR, of keeping, the more generic addJettyHandler(Handler handler) and removeJettyHandler(Handler handler) methods something we can go with? Plugins that use WebAppContext can simply call get() to pass a Handler instance, like so:

HttpBindManager.getInstance().addJettyHandler( context.get() );

This approach works with our current Openfire implementation and will also remain compatible if the changes in this PR are accepted.

[guusdk]

Is there benefit in providing both the 'old' method, as well as the new one as introduced in this PR?

[guusdk]

As suggested by @joakime and @hqv126, this PR has been updated (force-push) to retain the old add/remove methods that are based on the generic Handler.

As I expect many/most plugins to use a WebAppContext - which each plugin will need to update to Jetty12 EE8 - I've added a convenience overload for those methods that take instances of that class directly.