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

Improving ProtocolUpgradeHandler priority checking #2676

Open
tsegismont opened this issue Nov 7, 2024 · 0 comments
Open

Improving ProtocolUpgradeHandler priority checking #2676

tsegismont opened this issue Nov 7, 2024 · 0 comments
Assignees
Milestone

Comments

@tsegismont
Copy link
Contributor

In Vert.x Web there is a handler priority verification mechanism.

There are different priorities (e.g. authentication, protocol upgrade, user) and, when several handlers are installed on the same route, Vert.x Web throws an exception if the lower priority handler is set after a higher priority handler.

For example:

    router.route("/product*")
      .handler(basicAuthHandler) // BasicAuthHandler
      .handler(productProxyHandler); // ProxyHandler

Leads to:

java.lang.IllegalStateException: Cannot add [PROTOCOL_UPGRADE] handler to route with [AUTHENTICATION] handler at index 0
	at io.vertx.ext.web.impl.RouteState.addContextHandler(RouteState.java:572)
	at io.vertx.ext.web.impl.RouteImpl.handler(RouteImpl.java:143)

As a workaround, the handlers can be set on two different routes with the same configuration:

    router.route("/product*")
      .handler(basicAuthHandler);

    router.route("/product*")
      .handler(productProxyHandler);

In this example, the reason of the failure is that ProxyHandler has priority PROTOCOL_UPGRADE (because it can proxy websockets) but it's added after BasicAuthHandler, which has AUTHENTICATION priority.

A blunt move might be to revert some of the changes introduced in #2616 (i.e. no longer make ProxyHandlerImpl implement the ProtocolUpgradeHandler interface).

But, in this case, it makes sense to do the same with the other ProtocolUpgradeHandler, which is the GraphQLWSHandler.
Indeed, in both cases there are good reasons for adding an auth handler when needed.

Any thoughts on this @pmlopes ?

@tsegismont tsegismont self-assigned this Nov 7, 2024
@tsegismont tsegismont added this to the 5.0.0 milestone Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

1 participant