Hi secureRandom is a performance bottleneck. That's a fact. It can't be set as default.
See for example this blog: http://www.mattnworb.com/post/the-dangers-of-java-security-securerandom/ "... If you don’t read the descriptions carefully enough, you might miss the fact that /dev/random will block when there is not enough entropy data available. This means that in practice, it’s possible for your calls to new SecureRandom() to block for an unknown amount of time...." About MYFACES-3779: well, we are not solving that one here, but the current abstraction is strong enough to support it. About delete StateTokenProcessor: The token can include a mac, is encrypted, compressed, serialized. I think each one of these steps is complex enough to be encapsulated in one isolated class. Maybe we need to consider that StateTokenProcessor must be instantiated by StateCache, not by HtmlResponseStateManager. I don't think we are over engineering here, because the big abstraction are the three methods that does not really change, but if I want to create a new StateCache, maybe I don't want to write those three methods over again, right? If I'm writing a new StateCache instance, I want to focus on how to store the state. Please note MYFACES-4078 expose StateCacheFactory/StateCache to the public since 2.3.0, so these 3 methods in StateCache creates a conflict with this issue I have been working for a long time. This issue makes me feel I'm walking in circles. Keep it simple, follow the facts, the old known algorithm that uses mac +encryption works just fine. The algorithm has been designed to keep the token very small (IntIntKey...), so simplify here undo a previous work in that area. This is a problem where we reach a dead end. Just turn back. But move StateTokenProcessor inside StateCache is a good idea, but a default implementation that behaves like we already know is important. regards, Leonardo Uribe 2017-12-19 16:23 GMT-05:00 Thomas Andraschko <[email protected]>: > Yep, it's a viewstate-id or token, not view-id of course! > If the current secureRandom is not perfect, we can still improve it. > Also without this change discussed here, we would have set secureRandom as > our default (we discussed this in the other topic), so i don't think see a > problem here. > > > I understand your point about https://issues.apache.org/jira > /browse/MYFACES-3779 but quite a lot work must be done to get this > working and is unsure if we will ever work on something. > Also if we would work on it, i'm sure we would have to refactor much more > things. > > Also your statement > "The key point is maybe the state saving mode has something to say about > how the token is processed." > says that its actually one unit. > > We should not over-engineer something which is maybe never required. > maintainability > abstraction - as it's already quite hard the follow our > code in some places. > > > > > 2017-12-19 21:57 GMT+01:00 Leonardo Uribe <[email protected]>: > >> Hi >> >> TA>> Yep, it's not a bug but a good improvement. Personally i would only >> do >> TA>> it in 2.3. >> >> I agree, only 2.3. >> >> TA>> As i already explained, i removed those classes as "sequence" >> view-id >> TA>> generator must not be used anymore. >> >> I'm confused about this term "view-id". It is similar to JSF viewId, >> which >> is the path of the view. Let's do some recap. >> >> In server side state saving we use a token. This token is composed of: >> >> - A session counter. >> - viewId or related hashCode. >> - (optional) a random number. >> >> In JSF 1.1 the full viewId path was saved with a sequence to be able to >> identify the right view state in the collection. Later the viewId was >> changed with a hash code, because the viewId was too large, and we needed >> a smaller token. >> >> The idea behind a random number was the token should not be easily >> guessed. >> Well, it is not a requirement, since the token is mac-encoded and >> encrypted. >> But generate a random number is slow in some systems and JDK versions >> (linux, >> Sun-Oracle JDKs). That's the reason why the random number is not by >> default, >> because it is not possible. >> >> There are fast random number generators but remember, to make it safe it >> should not be possible to guess it, well if you have plans to disable >> encryption. So, there is an option to use a "secureRandom" number >> generator. >> >> TA>> My reasons to merge both classes are that we already have a artifact >> TA>> which handles client side state, and we already have a artifact >> which >> TA>> handles server side state. >> >> TA>> Maybe "Cache" is not a 100% perfect name but it's ok IMO. We could >> TA>> still search for another name, which would be a very very small >> TA>> improvement. There is no reason to e.g. use a >> TA>> ClientSide-StateTokenProcessor with a ServerSide-StateCache. >> TA>> Thereis one big mechanism for server-side state and one for >> TA>> client-side state. How to render it and how to create/restore it, >> TA>> are both sides of the medal. >> >> I can remember this issue: >> >> https://issues.apache.org/jira/browse/MYFACES-3779 >> Mixed mode(Server+client) for state saving >> >> It is a long discussion, but the point is that a mixed mode is possible. >> The work done in the API was meant to go towards that goal. The state >> saving algorithm is something really hard to customize. I strongly feel >> StateTokenProcessor has a lot of sense. The key point is maybe the >> state saving mode has something to say about how the token is processed. >> In this case, the key info is tell the processor that the token must >> not be serialized. >> >> regards, >> >> Leonardo Uribe >> >> 2017-12-19 15:21 GMT-05:00 Thomas Andraschko <[email protected] >> >: >> >>> Yep, it's not a bug but a good improvement. Personally i would only do >>> it in 2.3. >>> >>> As i already explained, i removed those classes as "sequence" view-id >>> generator must not be used anymore. >>> >>> My reasons to merge both classes are that we already have a artifact >>> which handles client side state, and we already have a artifact which >>> handles server side state. >>> Maybe "Cache" is not a 100% perfect name but it's ok IMO. We could still >>> search for another name, which would be a very very small improvement. >>> There is no reason to e.g. use a ClientSide-StateTokenProcessor with a >>> ServerSide-StateCache. >>> Thereis one big mechanism for server-side state and one for client-side >>> state. How to render it and how to create/restore it, are both sides of the >>> medal. >>> >>> >>> >>> 2017-12-19 21:09 GMT+01:00 Leonardo Uribe <[email protected]>: >>> >>>> Hi >>>> >>>> There are some changes in MYFACES-4133 (2.3.x branch) that is required >>>> to be >>>> discussed on dev list and not on jira. >>>> >>>> MYFACES-4133 is all about refactor the existing state API to avoid the >>>> java >>>> token serialization. I have already stated this is an improvement, not >>>> a bug, >>>> but since there is a lot of interest in this point I would like to >>>> expose the >>>> ideas behind the existing API. >>>> >>>> The change proposed removes these classes: >>>> >>>> StateTokenProcessor >>>> IntIntSerializedViewKey >>>> CounterSessionViewStorageFactory >>>> IntByteArraySerializedViewKey >>>> CounterKeyFactory >>>> >>>> And the addition of some methods in StateCache: >>>> >>>> public abstract Object decodeStateToken(FacesContext facesContext, >>>> String token); >>>> public abstract String encodeStateToken(FacesContext facesContext, >>>> Object savedStateObject); >>>> public boolean isStatelessToken(FacesContext facesContext, String token) >>>> >>>> The first problem here is StateCache abstraction. >>>> >>>> "... provides an interface to separate the state caching operations >>>> (saving/restoring) from the renderkit specific stuff that >>>> HtmlResponseStateManager should do. ..." >>>> >>>> Token serialization is HtmlResponseStateManager stuff, specifically >>>> related to >>>> the render step. >>>> >>>> StateTokenProcessor was deleted but this interface had the following >>>> methods: >>>> >>>> public abstract Object decode(FacesContext facesContext, String token); >>>> public abstract String encode(FacesContext facesContext, Object >>>> savedStateObject); >>>> public abstract boolean isStateless(FacesContext facesContext, String >>>> token); >>>> >>>> My first question is what's the reason of merge StateTokenProcessor and >>>> StateCache? What's the advantage? >>>> >>>> Well, I can see one, StateCache has a logic related to client-server >>>> state saving. >>>> Server side state saving uses a counter as token, client side state >>>> saving >>>> encodes the view state inside the token. >>>> >>>> But still I think StateTokenProcessor has life on its own. Am I missing >>>> something? >>>> >>>> regards, >>>> >>>> Leonardo Uribe >>>> >>>> >>>> >>> >> >
