jsr372-experts@javaserverfaces-spec-public.java.net

[jsr372-experts] Re: [jsr372-experts mirror] Re: Re: Re: Re: problems with jsf.push.init(...) specification and token generation

From: Bauke Scholtz <balusc_at_gmail.com>
Date: Fri, 23 Dec 2016 19:42:57 +0100

FYI: I just added javax.faces.WEBSOCKET_ENDPOINT_PORT,
ViewHandler#getWebsocketURL() and ExternalContext#encodeWebsocketURL() to
API and removed port argument from <f:websocket> and jsf.push.init() as per
https://java.net/projects/mojarra/sources/git/revision/37a93d7b4e95f96350ab191520aa8bc9568a9862

Note that initial UIWebsocket used javax.faces.Output component family but
this has been changed to javax.faces.Script as it's actually not a
ValueHolder.

Cheers, B

On Thu, Dec 15, 2016 at 9:53 AM, Bauke Scholtz <balusc_at_gmail.com> wrote:

> Hi,
>
> Coming back to channelToken, I wonder the necessity that the
> getWebsocketURL() API should take care of that part (in this specific case,
> appending it as query string). The websocket implementation has to extract
> it from the URL somehow (in this specific case, obtaining the query
> string). But if the API or a wrapper does it a bit differently, then the
> whole thing may break. It'd better be and stay an implementation detail.
>
> Cheers, B
>
> On Thu, Dec 8, 2016 at 8:10 AM, Bauke Scholtz <balusc_at_gmail.com> wrote:
>
>> Hi,
>>
>> API looks good. However, is there any technical reason you use
>> getServerName() instead of extracting it from getRequestURL()? The latter
>> is more reliable as this represents the URL that actually hit the server
>> instead of a server environment dependent configuration setting.
>>
>> Cheers, B
>>
>> On Thu, Dec 8, 2016, 04:55 Leonardo Uribe <leonardo.uribe_at_irian.at>
>> wrote:
>>
>>> Hi Bauke
>>>
>>> I have this declaration in MyFaces:
>>>
>>> public String getWebsocketURL(FacesContext context, String channel,
>>> String channelToken)
>>>
>>> This is the way I have used it in MyFaces implementation of f:websocket :
>>>
>>> In the Renderer instance:
>>>
>>> sb.append("'"+facesContext.getExternalContext().encodeWebsocketURL(
>>> facesContext.getApplication().getViewHandler().getWebsocketU
>>> RL(
>>> facesContext, component.getChannel(), (String)
>>> component.getValue()))+"'");
>>>
>>> In ViewHandler:
>>>
>>> /**
>>> * Return a JSF URL that represents a websocket connection for the
>>> passed channel and channelToken
>>> *
>>> * @since 2.3
>>> * @param context
>>> * @param channel
>>> * @param channelToken
>>> * @return
>>> */
>>> public String getWebsocketURL(FacesContext context, String channel,
>>> String channelToken)
>>> {
>>> String url = context.getExternalContext().getRequestContextPath()
>>> +
>>> "/javax.faces.push/"+channel+"?"+channelToken;
>>> return url;
>>> }
>>>
>>> In ExternalContext
>>>
>>> public String encodeWebsocketURL(String baseUrl)
>>> {
>>> Integer port = WebConfigParamUtils.getIntegerInitParameter(
>>> _currentFacesContext.getExternalContext(),
>>> ViewHandler.WEBSOCKET_PORT);
>>> port = (port == 0) ? null : port;
>>> if (port != null &&
>>> !port.equals(_currentFacesContext.getExternalContext().getRe
>>> questServerPort()))
>>> {
>>> String scheme = _currentFacesContext.getExtern
>>> alContext().getRequestScheme();
>>> String serverName = _currentFacesContext.getExtern
>>> alContext().getRequestServerName();
>>> String url;
>>> try
>>> {
>>> url = new URL(scheme, serverName, port,
>>> baseUrl).toExternalForm();
>>> url = url.replaceFirst("http", "ws");
>>> return ((HttpServletResponse)
>>> _servletResponse).encodeURL(url);
>>> }
>>> catch (MalformedURLException ex)
>>> {
>>> //If cannot build the url, return the base one unchanged
>>> return baseUrl;
>>> }
>>> }
>>> else
>>> {
>>> return baseUrl;
>>> }
>>> }
>>>
>>> I think there are two different concepts: the "channelName" and the
>>> "channelToken". The "channelName" or "channel" is the symbolic name to
>>> identify a channel, but the "channelToken" is meant to setup the context
>>> of
>>> that channel. You can join both in a single string, but that means you
>>> are
>>> writing a part of the url (the "?" between both).
>>>
>>> ExternalContext.encodeWebsocketURL(String baseUrl) is a point where the
>>> implementation could add a host/port if required. The idea is both
>>>
>>> /test-webapp-cdi/javax.faces.push/clock?e82c26b5ccc2a5651571de6818cbfab0
>>>
>>> and
>>>
>>> ws://localhost:4566/test-webapp-cdi/javax.faces.push/clock?e
>>> 82c26b5ccc2a5651571de6818cbfab0
>>>
>>> are valid encoded urls.
>>>
>>> regards,
>>>
>>> Leonardo Uribe
>>>
>>> 2016-12-07 4:09 GMT-05:00 Bauke Scholtz <balusc_at_gmail.com>:
>>>
>>> Hi all and specifically Leo,
>>>
>>> How about this ViewHandler#getWebsocketURL() spec? I just copypasted
>>> from getResourceURL() and altered where necessary. I myself only find the
>>> wording of "will select the argument xxx for" somewhat unclear, but it is
>>> as how those other getXxxURL() are specified.
>>>
>>> /**
>>> * <p class="changed_added_2_3">If the value returned from this
>>> * method is used as the <code>file</code> argument to the
>>> * four-argument constructor for <code>java.net.URL</code> (assuming
>>> * appropriate values are used for the first three arguments), then
>>> * a client making a websocket push handshake request to the
>>> <code>toExternalForm()</code> of
>>> * that <code>URL</code> will select the argument
>>> <code>channel</code>
>>> * for connecting the websocket push channel in the current view.
>>> * It must match the {_at_link PushContext#URI_PREFIX} of the
>>> endpoint.</p>
>>> *
>>> * @param context {_at_link FacesContext} for the current request.
>>> * @param channel The channel name of the websocket.
>>> *
>>> * @throws NullPointerException if <code>context</code> or
>>> * <code>channel</code> is <code>null</code>.
>>> *
>>> * @return the websocket URL.
>>> * @see PushContext#URI_PREFIX
>>> */
>>> public abstract String getWebsocketURL(FacesContext context, String
>>> channel);
>>>
>>>
>>> A reference implementation would be this:
>>>
>>> @Override
>>> public String getWebsocketURL(FacesContext context, String channel) {
>>> requireNonNull(context, "context");
>>> requireNonNull(channel, "channel");
>>>
>>> String contextPath = context.getExternalContext().g
>>> etRequestContextPath();
>>> return contextPath + PushContext.URI_PREFIX + "/" + channel;
>>> }
>>>
>>>
>>> Cheers, B
>>>
>>>
>>> On Tue, Dec 6, 2016 at 4:25 PM, Bauke Scholtz <balusc_at_gmail.com> wrote:
>>>
>>> Hi,
>>>
>>> As per https://java.net/projects/mojarra/sources/git/revision/acb65
>>> 8925ed54206f0844f7eb88d5035218e22b3 the WebsocketHandler has been
>>> turned into UIWebsocket implementing ClientBehaviorHolder. Now f:ajax is
>>> supported as per below javadoc update:
>>>
>>> * <h3 id="ui"><a href="#ui">Ajax support</a></h3>
>>> * <p>
>>> * In case you'd like to perform complex UI updates depending on the
>>> received push message, then you can nest
>>> * <code>&lt;f:ajax&gt;</code> inside <code>&lt;f:websocket&gt;</code>.
>>> Here's an example:
>>> * <pre>
>>> * &lt;h:panelGroup id="foo"&gt;
>>> * ... (some complex UI here) ...
>>> * &lt;/h:panelGroup&gt;
>>> *
>>> * &lt;h:form&gt;
>>> * &lt;f:websocket channel="someChannel" scope="view"&gt;
>>> * &lt;f:ajax event="someEvent" listener="#{bean.pushed}"
>>> render=":foo" /&gt;
>>> * &lt;/f:websocket&gt;
>>> * &lt;/h:form&gt;
>>> * </pre>
>>> * <p>
>>> * Here, the push message simply represents the ajax event name. You can
>>> use any custom event name.
>>> * <pre>
>>> * someChannel.send("someEvent");
>>> * </pre>
>>>
>>> And, jsf.push.init/open/close now take client ID instead of channel name.
>>>
>>> One thing that's still open is having a ViewHandler#getWebsocketURL(). I
>>> will work on that later this week.
>>>
>>> Cheers, B
>>>
>>> On Fri, Oct 28, 2016 at 5:54 AM, Leonardo Uribe <leonardo.uribe_at_irian.at
>>> > wrote:
>>>
>>> Hi
>>>
>>> Good to know that. Thanks!
>>>
>>> regards,
>>>
>>> Leonardo Uribe
>>>
>>>
>>> 2016-10-27 3:07 GMT-05:00 Bauke Scholtz <balusc_at_gmail.com>:
>>>
>>> I have reopened https://java.net/jira/browse/JAVASERVERFACES_SPEC_
>>> PUBLIC-1396
>>>
>>> Cheers, B
>>>
>>> On Fri, Oct 21, 2016 at 4:08 AM, Leonardo Uribe <leonardo.uribe_at_irian.at
>>> > wrote:
>>>
>>> Hi
>>>
>>> I just wanted to mention the declaration of jsf.push.init(...) that
>>> works better in my personal opinion, based on the previous
>>> discussions:
>>>
>>> jsf.push.init(clientId, url, onopen, onmessage, onclose,
>>> behaviorScripts, connected);
>>>
>>> Example:
>>>
>>> <div>
>>> <h:outputText id="currentTime" value="#{clockBean.currentTime}"/>
>>> </div>
>>>
>>> <f:websocket channel="clock" scope="application"
>>> onopen="socketOpen"
>>> onclose="socketClose">
>>> <f:ajax event="showCurrentTime"
>>> execute="currentTime"
>>> render="currentTime"/>
>>> </f:websocket>
>>>
>>> Generates this script:
>>>
>>> <script type="text/javascript"><!--
>>> jsf.push.init('mainForm:j_id_p',
>>> '/test-webapp-cdi/javax.faces.
>>> push/clock?e82c26b5ccc2a5651571de6818cbfab0',
>>> socketOpen,
>>> null,
>>> socketClose,
>>> {showCurrentTime:[function(event){
>>> jsf.ajax.request('mainForm:j_i
>>> d_p',event,{execute:'mainForm:currentTime
>>> ',render:'mainForm:currentTime ','javax.faces.behavior.event'
>>> :'showCurrentTime'})}
>>> ]},true);
>>> //--></script>
>>>
>>> Only if the port is set in the global web config param, the url will
>>> include it full.
>>>
>>> The only thing to mention is the channel name and the channel token are
>>> derived from the URL, which means the URL should always end in that way
>>> if it is overriden (portlet case usually overrides everything).
>>> The other option is pass both as parameters of jsf.push.init(...), but
>>> maybe
>>> that's not necessary, better to keep it simple.
>>>
>>> regards,
>>>
>>> Leonardo Uribe
>>>
>>>
>>> 2016-10-18 15:33 GMT-05:00 Leonardo Uribe <leonardo.uribe_at_irian.at>:
>>>
>>> Hi
>>>
>>> BS>> As to clustering, do server and WS specific specifics really have
>>> to end up in JSF specification too?
>>>
>>> No, instead we do what we can with what we have. This is not something
>>> that we can fix from here.
>>>
>>> But we could ask websocket api EG about this for clarification.
>>>
>>> BS>> As to auto-reconnect (not auto-connect), yes I agree that some
>>> API-provided configuration settings
>>> BS>> would be useful. Mojarra's implementation does 25 attempts with an
>>> incremental interval
>>> BS>> of 500ms, starting with 0ms. I only wonder what parts should be
>>> configureable by API. The
>>> BS>> amount of reconnect attempts? If zero or negative, then it's
>>> considered as disabled.
>>>
>>> I was thinking about use a exponential backoff algorithm. But the
>>> problem here is it depends of what
>>> the developer wants. If he/she wants real time, the reconnection could
>>> be different. The key point is
>>> this could be a candidate parameter to be decided at deployment stage.
>>> If this is a config parameter,
>>> you can override it in web.xml without change the code. But if this is
>>> not considered relevant for
>>> the spec, we can just let this as an implementation detail.
>>>
>>> This is a design decision. My only intention here is to mention it.
>>>
>>> regards,
>>>
>>> Leonardo Uribe
>>>
>>>
>>> 2016-10-15 1:19 GMT-05:00 Bauke Scholtz <balusc_at_gmail.com>:
>>>
>>> Hi,
>>>
>>> As to clustering, do server and WS specific specifics really have to end
>>> up in JSF specification too?
>>>
>>> As to auto-reconnect (not auto-connect), yes I agree that some
>>> API-provided configuration settings would be useful. Mojarra's
>>> implementation does 25 attempts with an incremental interval of 500ms,
>>> starting with 0ms. I only wonder what parts should be configureable by API.
>>> The amount of reconnect attempts? If zero or negative, then it's considered
>>> as disabled.
>>>
>>> Cheers, B
>>>
>>>
>>> On Fri, Oct 14, 2016 at 8:06 PM, Leonardo Uribe <leonardo.uribe_at_irian.at
>>> > wrote:
>>>
>>> Hi
>>>
>>> BS>> As to clustering, see writeObject() and readObject() methods of
>>> OmniFaces
>>> BS>> o:socket SocketChannelManager class (on which Mojarra's f:websocket
>>> BS>> WebsocketChannelManager class is largely based).
>>>
>>> Please note I'm writing an implementation of f:websocket from scratch,
>>> so I'm
>>> trying do not see omnifaces code. I have checked the methods and anyway
>>> it
>>> doesn't solve the problem related to Session migration, because this
>>> code is
>>> not detecting when the Session instance has been migrated. But it
>>> supports the
>>> point that the token is not application scope, because you need to move
>>> the related data using a session scope bean. But as I said, this is
>>> implementation specific, doesn't change the spec.
>>>
>>> BS>> As to dropping "connected", I don't understand why you consider it
>>> an
>>> BS>> application wide setting instead of a socket specific setting. How
>>> do you
>>> BS>> want to let the client have control over manually opening a
>>> specific
>>> BS>> socket via jsf.push.open("channel") during e.g. an onclick event
>>> only?
>>> BS>> See also javax.faces.push.Push javadoc and o:socket showcase.
>>>
>>> My mistake here, I just read this in the javadoc:
>>>
>>> "... You can use the optional connected attribute to control whether to
>>> auto-connect the web socket or not. ..."
>>>
>>> But I was thinking about how the websockets connects in case of a
>>> disconnection between the client and the server.
>>>
>>> In other words, what I wanted to say is the way how the client attempts
>>> to
>>> reconnect to the server is something that you would like to control
>>> using a
>>> web config param. How many attemps, the timeout and so on. Maybe pass
>>> these config as an array in jsf.push.init(...) is a good idea.
>>>
>>> Please note I haven't implemented this attribute on my code yet, so I'm
>>> reporting on the mailing list what I'm seeing. I have not checked the
>>> showcase, because I do not want to bias my own judgment. The idea is
>>> the documentation in the spec should be good enough to give an idea
>>> about how it works. That's the way we ensure spec's quality. So, I have
>>> not digested every single line, but I'm on the way.
>>>
>>> regards,
>>>
>>> Leonardo Uribe
>>>
>>> 2016-10-14 2:39 GMT-05:00 Bauke Scholtz <balusc_at_gmail.com>:
>>>
>>> Hi,
>>>
>>> As to clustering, see writeObject() and readObject() methods of
>>> OmniFaces o:socket SocketChannelManager class (on which Mojarra's
>>> f:websocket WebsocketChannelManager class is largely based).
>>>
>>> As to dropping "connected", I don't understand why you consider it an
>>> application wide setting instead of a socket specific setting. How do you
>>> want to let the client have control over manually opening a specific socket
>>> via jsf.push.open("channel") during e.g. an onclick event only? See also
>>> javax.faces.push.Push javadoc and o:socket showcase.
>>>
>>> Cheers, B
>>>
>>> On Fri, Oct 14, 2016 at 4:19 AM, Leonardo Uribe <leonardo.uribe_at_irian.at
>>> > wrote:
>>>
>>> Hi
>>>
>>> BS>> I do agree that the full url should be generated by the server
>>> BS>> and passed to the client. I will look into it.
>>>
>>> Ok, thanks. I just want to add that the "port" attribute is something
>>> that
>>> should be set using a global web config param, because this is common
>>> for all the application and depends on the deployment configuration.
>>> Please also consider if "connect" can be a global web config param
>>> instead
>>> an attribute too.
>>>
>>> BS>> The "token" is application scoped not session scoped.
>>>
>>> The logic behind this is something I have been thinking for a long time.
>>>
>>> Before start, I want to be clear that the suggested proposal is provide
>>> a point in the spec to allow customize the token generation by security
>>> purposes. The way how a token is used is considered an implementation
>>> specific concern, but in order to enrich the understanding of this
>>> component I'll put my thoughts about this here.
>>>
>>>
>>> There is a precedent that suggest an application scope token is not a
>>> good
>>> idea. See:
>>>
>>> - https://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-694
>>> Current Flash specification breaks clustering
>>> - https://java.net/jira/browse/JAVASERVERFACES-1449
>>> Modify flash to support clustering
>>>
>>> The description says: "... Currently the flash implementation is
>>> entirely
>>> stored in the Application scope. This makes it unclusterable. Make it so
>>> data stored in the flash is cluster friendly. ..."
>>>
>>> Now, as you already know, f:websocket requires to register an Endpoint
>>> and in that place there is no HttpSession (there is on handshake),
>>> so from that point the only valid CDI beans are application scoped.
>>> So, the active websocket Session instances should be registered in an
>>> application scoped bean.
>>>
>>> There are two points to keep in mind:
>>>
>>> - Clustering relies on session replication, and application scope is not
>>> replicated among nodes.
>>> - Application scope means a potential for memory leaks, if the map does
>>> not
>>> have proper cleanup measures.
>>>
>>> If you make the token application scope, that means multiple views share
>>> the
>>> same token, but it also means there is no way to keep track of each
>>> connection in the application scope bean. So, at the end you will end up
>>> with
>>> stale session instances, and that's not good.
>>>
>>> If the websocket lifetime is shorter than the view lifetime, it is only
>>> natural that the token should be view scope. That means, for each
>>> combination
>>> of channel, scope and user it should be one channel token per view.
>>>
>>> The channel token is generated on render response time, so the way to do
>>> it
>>> is generate the token and store it in a view scope bean. If @PreDestroy
>>> is
>>> properly implemented for view scope beans, we can rely on that to
>>> destroy in
>>> a ordered way the token and the associated websocket Session instances
>>> from
>>> the application scope bean. If the session is destroyed, @PreDestroy will
>>> be called anyway, so with this we can avoid the memory leak.
>>>
>>> If you see the problem from a "cluster" point of view, if the token is
>>> application scope and the session is migrated to another node, the token
>>> will
>>> not be registered in the application scope of the target node, so the
>>> connection will not be accepted. Please note there is no CDI session
>>> scope
>>> in Endpoint.onOpen, but there is on the handshake request, so the
>>> solution
>>> is to register the valid tokens on session scope, so they can be
>>> validated
>>> later in that point. It could be variants to avoid store the tokens on
>>> session scope, but the problem is this feature is "CDI centric", so it is
>>> hard to initialize a FacesContext instance on the Endpoint. It can be
>>> done,
>>> but we need more "plumbing" to do it. In that case, we need a method
>>> on ViewHandler to validate the websocket token (remember
>>> ResponseStateManager is tied to RenderKit, and here there is view and
>>> no renderkit reference).
>>>
>>> Websocket spec says this about clustering:
>>>
>>> "... Websocket implementations that are part of a distributed container
>>> may
>>> need to migrate websocket sessions from one node to another in the case
>>> of a
>>> failover. ..."
>>>
>>> This blows up any storage of Session into an application scope bean,
>>> because
>>> it says the session can be moved between nodes, so in that case, the
>>> migrated
>>> Session will not be registered into the application scope bean. But this
>>> can be accepted, because probably in that case the websocket connection
>>> could be reopened, and the problem silently dissapear. This could be a
>>> potential websocket spec pitfall, where the spec says that the websocket
>>> sessions can be moved between nodes, but does not provide a way to
>>> identify
>>> when this has happened and take proper actions (like register the
>>> Session in
>>> the application scope bean).
>>>
>>>
>>>
>>>
>>> In conclusion, the important thing about the websocket token is that it
>>> helps
>>> to identify the connection and its associated properties. If two
>>> f:websocket
>>> declarations share the same channel, user and scope, both should have the
>>> same token "at least" at view level. I think both implementations can
>>> agree
>>> on that assumption.
>>>
>>> No, thinking on jsf.push.init(...) I would like to propose this
>>> structure for
>>> the EG consideration:
>>>
>>> jsf.push.init(clientId, url, onopen, onmessage, onclose,
>>> behaviorScripts);
>>>
>>> removing "port" and "connected" (use web config parameters for that).
>>>
>>> The clientId helps to identify from which tag these properties belong,
>>> and
>>> make possible to reuse the same websocket connection among f:websocket
>>> declarations. I'll explain this in another email, this is a work in
>>> progress,
>>> but for now I'm sure we could agree with this function declaration.
>>>
>>> regards,
>>>
>>> Leonardo Uribe
>>>
>>> 2016-10-13 2:10 GMT-05:00 Bauke Scholtz <balusc_at_gmail.com>:
>>>
>>> Hi,
>>>
>>> The "token" is application scoped not session scoped. I do agree that
>>> the full url should be generated by the server and passed to the client. I
>>> will look into it.
>>>
>>> Cheers ,B
>>>
>>> On Tue, Oct 4, 2016, 05:36 Leonardo Uribe <leonardo.uribe_at_irian.at>
>>> wrote:
>>>
>>> Hi
>>>
>>> Following a more exhaustive review of f:websockets, I have found some
>>> inconsistencies related to the way how a connection is specified.
>>>
>>>
>>>
>>> Please note there is no mention of jsf.push.init(...) in the
>>> javascript documentation, but the package jsf.push is there.
>>>
>>> Here is an example:
>>>
>>> <script type="text/javascript">
>>> jsf.push.init(8080,'clock?1e4ac202-2b21-434f-8431-de5d5e4d37fe',
>>> socketOpen,socketListener,socketClose,true);</script>
>>>
>>> If you use "jsf" javascript package, every method there should be
>>> specified
>>> without exceptions. I know this feature is a "work in progress" but I
>>> just
>>> wanted to notice this fact.
>>>
>>>
>>>
>>>
>>> Other thing to notice is there is a session token used to identify the
>>> connection. This is something related to state management, so I was
>>> expecting
>>> a method inside ResponseStateManager, in the same way there is a method
>>> there called:
>>>
>>> public String getCryptographicallyStrongTokenFromSession(FacesContext
>>> context)
>>>
>>> to handle protected views. Something like:
>>>
>>> public String getCryptographicallyStrongToke
>>> nForWebsocketSession(FacesContext context)
>>>
>>> It is only the token generation what we need here, not the websocket
>>> session handling.
>>>
>>>
>>>
>>> At last, there is a problem with passing params to assemble the url used
>>> for the connection in the client. It has been a long standing policy in
>>> JSF
>>> to generate urls on the server and pass them to the client. But in the
>>> javascript documentation a new variable was added called:
>>>
>>> jsf.contextpath
>>>
>>> with description "... The result of calling
>>> ExternalContext.getRequestContextPath(). ..."
>>>
>>> It looks like something recently added, so I guess it should be something
>>> related to jsf.push.init(...) script.
>>>
>>> The problem here is there is no way for the server to override this path,
>>> and in cases like portlets for example, it is necessary to provide a way
>>> to override this part properly. So the url used to connect must be built
>>> fully on the server and use the known ViewHandler - ExternalContext
>>> pattern. Additionally, embed this variable in the js file breaks any
>>> caching strategy. If the content is dynamic, embed it as an inline
>>> script,
>>> but in this case it is better to avoid that.
>>>
>>> regards,
>>>
>>> Leonardo Uribe
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>