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

Enabled MultiViews setup of FacesViews breaks websockets when welcome file is declared #402

Closed
skuntsel opened this Issue Sep 21, 2017 · 6 comments

Comments

Projects
None yet
2 participants
@skuntsel
Copy link

skuntsel commented Sep 21, 2017

No description provided.

@skuntsel skuntsel changed the title Enabled MultiViews setup of FacesViews breaks websockets when welcome file are declared Enabled MultiViews setup of FacesViews breaks websockets when welcome file is declared Sep 21, 2017

@skuntsel

This comment has been minimized.

Copy link

skuntsel commented Sep 21, 2017

When MultiViews setup of FacesViews is enabled, e.g.:

<context-param>
    <param-name>org.omnifaces.FACES_VIEWS_SCAN_PATHS</param-name>
    <param-value>/*.xhtml/*</param-value>
</context-param>

and when welcome file is declared, e.g.:

<welcome-file-list>
    <welcome-file>index.xhtml</welcome-file>
</welcome-file-list>

in web deployment desriptor, websocket configuration becomes broken, e.g. @ServerEndpoint("/foo") is never reched.

This appears to be an oversight in FacesViews source code, because FacesViewsForwardingFilter overzealosuly kicks in on all URLs, including /foo, theats it as a path parameter injectable via @Param and forwards it to the welcome file / modifies the request accordingly. Thus, the websocket filter that does the protocol upgrade is never reached (e.g. undertow's JsrWebSocketFilter used by WildF ly 10.1.0 that is designed to run after all user filters) and protocol upgrade request is never satisfied.\

Basically, new WebSocket("ws://context-path/foo") request yields response HTTP status code 200 (and the ordinary response) instead of the expected 101.

Should FacesViewsForwardingFilter do a basic check for upgrade request header, request.getHeader("Upgrade") != null, before doing its job and otherwise continue the filter chain, it would be sufficient to fix this use case.

@skuntsel

This comment has been minimized.

Copy link

skuntsel commented Sep 21, 2017

As soon as the Filter is added programatically during application startup it is not possible to bypass it to let the request reach the targeted filter because ServletContext doesn't offer the opportunity to remove/disable the filter configured by Omnifaces in e.g. @WebListener. Overriding it in web deployment descriptors leads to NPE during omnifaces initialization, and is the expected behaviour. Basically, filter initialization is tightly coupled.

As a workaround you could theoretically fork omnifaces source code and modify META-INF/services/javax.servlet/ServletContainerInitializer file with your.package.WebsocketApplicationInitializer and do the filter initialization job therein, getting inspiration from the original source code, with the filter being:

public class WebsocketAwareFacesViewsForwardingFilter extends FacesViewsForwardingFilter {

    @Override
    public void doFilter(HttpServletRequest request, HttpServletResponse response, HttpSession session, FilterChain chain) throws ServletException, IOException {
        if(request.getHeader("Upgrade") != null) {
            chain.doFilter(request, response);
        } else {
            super.doFilter(request, response, session, chain);
        }
    }
    
}

but this definitely seems to be a worng way to go.

The other alternative to consider is to give up using welcome files, but this .may be unwanted, especially for existing applications.

@skuntsel

This comment has been minimized.

Copy link

skuntsel commented Sep 21, 2017

As a feature request for a new release it would be great if there were a context parameter holding a FQN of the filter that had to be added programmatically that would fall back to the standard one in case of an error, like:

   <context-param>
       <param-name>org.omnifaces.FACES_VIEWS_FILTER_IMPLEMENTATION_NAME</param-name>
       <param-value>your.package.WebsocketAwareFacesViewsForwardingFilter</param-value>
   </context-param>

with the implementing class subclassing or wrapping the standard one for some extra checks that could emerge during development.

Another highly efficient setting with this setup could be explicit statement of paths that would not trigger the filter. This could be highly beneficial, because basically the filter with this setup is mapped on /*, bypassing only a subset of resources, and it disallows developers to provide for a dedicated path for different frameworks utilizing HTTP protocol, like, for instance, REST API.

It could have the directory flavour like WEB-INF/faces-views-exclusions or context parameter falour like

<context-param>
    <param-name>org.omnifaces.FACES_VIEWS_SCAN_PATHS_EXCLUSION</param-name>
    <param-value>/api/*</param-value>
</context-param>

@skuntsel skuntsel closed this Sep 21, 2017

@skuntsel skuntsel reopened this Sep 21, 2017

@BalusC

This comment has been minimized.

Copy link
Member

BalusC commented Sep 21, 2017

Confirmed.

If you happen to use MultiViews only on a specific page, e.g. /pages.xhtml, then you can also reconfigure SCAN_PATHS context param as below:

<param-value>/*.xhtml, /pages/*</param-value>
@BalusC

This comment has been minimized.

Copy link
Member

BalusC commented Sep 21, 2017

I tried to reproduce it with OmniFaces' own <o:socket> but couldn't. Upon investigation it appears that I never noticed this before because the <o:socket> uses an URI template of /omnifaces.push/{channel} - note the period - and this passes through the FacesViewsForwardingFilter unchanged.

Your request is however sound. I'll look for implementing an exclude pattern in FacesViews context param like

<param-value>/*.xhtml/*, !/api</param-value>
@BalusC

This comment has been minimized.

Copy link
Member

BalusC commented Sep 21, 2017

Give today's 2.6.5-SNAPSHOT a try.

@BalusC BalusC closed this in 161890d Sep 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment