Juan Hernandez has posted comments on this change. Change subject: wip - don't submit: rest session-management ......................................................................
Patch Set 9: (3 inline comments) See my comments inline. I am mostly concerned about the use of the value of the session id. .................................................... File backend/manager/modules/restapi/interface/common/jaxrs/src/main/java/org/ovirt/engine/api/common/security/auth/Challenger.java Line 87: I think that the session id shouldn't be used/checked in this method. The only relevant thing is if the session has been created in a previous request, if it has been created in this request, and if it has to be created or invalidated now, and for that it is enough with HttpServletRequest.getSession(false), HttpSession.isNew() and HttpSession.invalidate(). I mean, the session id itself should be internal: the less we manipulate, the better. The name "jsessionid" should not appear in the code. In addition passing the session id to other components (like the backend) can be dangerous, as it is not guaranteed to be a constant. In some very special situations, for example in a cluster using mod_jk with session replication, the session id can change from one request to the next. This is not likely to happen with our current architecture but it is better to avoid it. What is guaranteed is that the *content* of the session will be preserved, so in this case you would probably need to generate a "backend session id", like before, and store it in the HTTP session. .................................................... File backend/manager/modules/restapi/interface/common/jaxrs/src/main/java/org/ovirt/engine/api/common/security/auth/SessionUtils.java Line 16: public static String JSESSIONID_KEY = "JSESSIONID"; Why do we need to know the name of the cookie? We shouldn't be accessing its value directly. If for some reason you really need the session identifier it is better to call HttpSession.getId(). Line 54: retVal = request.getSession(); You may want to use here the version of the getSession method that takes a boolean as a parameter. If you pass false the session will not be created if it doesn't exist. The default is that if you call this method (or passing true to the other one) the session is automatically created if it doesn't exist. -- To view, visit http://gerrit.ovirt.org/3387 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I19bf58daab2d75c2677d90b09112a1876f5e870e Gerrit-PatchSet: 9 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Oved Ourfali <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Ori Liel <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Roy Golan <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
