The JASPIC 1.1 specification (section 3.8.4) states that the Prinicpal shall be stored in the session after successful authentication by a ServerAuthModule if javax.servlet.http.registerSession is set to a value of true. This works, but if the value is set to false the current code is not compliant as only the existence of the key is checked. Here is the code from AuthenticatorBase: Map map = state.messageInfo.getMap(); if (map != null && map.containsKey("javax.servlet.http.registerSession")) { register(request, response, principal, "JASPIC", null, null, true, true); } else { register(request, response, principal, "JASPIC", null, null); } Furthermore this makes the behavior dependent on the configuration of the authentication valves "cache" property it the property is not set, which is very inflexible. The ServerAuthModule can therefore not decide to NOT cache it in the session. Especially if you have more than one module which both want to handle this differently. I would suggest to evaluate the property if present and if it is set to "false" disable any caching (ignore valve config). This makes it standard compliant and more flexible without changing current behavior in the case of a "true" value. Here is the changed code: if (map != null && map.containsKey("javax.servlet.http.registerSession")) { boolean registerSession = Boolean.valueOf((String)map.get("javax.servlet.http.registerSession")).booleanValue(); register(request, response, principal, "JASPIC", null, null, alwaysUseSession || registerSession, registerSession); } else { register(request, response, principal, "JASPIC", null, null); }
How about: Map map = state.messageInfo.getMap(); if (map != null && Boolean.TRUE.equals(map.containsKey("javax.servlet.http.registerSession"))) { register(request, response, principal, "JASPIC", null, null, true, true); } else { register(request, response, principal, "JASPIC", null, null); } ?? Care to write a PR/patch for this and submit it? Its pretty trivial but you've already done the real work involved in locating the issue and fixing it.
I suppose you meant: Boolean.TRUE.equals(map.get("javax.servlet.http.registerSession")) ? This solves the problem of the false value being interpreted as true, but remains somewhat inflexible if you don't (really don't) want any caching as the register-Method with the 6 parameters (as opposed to that with 8) uses the valve configuration to determine the values for alwaysUseSession and cache. What I was trying to achieve was (3 cases): 1. javax.servlet.http.registerSession set to TRUE -> do caching and always create session (OK) 2. javax.servlet.http.registerSession NOT set -> use valve config for cache and alwaysUseSession (OK) AND 3. javax.servlet.http.registerSession set to FALSE -> don't do caching and use config of valve for alwaysUseSession (MISSING) The third option is important in my opinion. I'm thinking of Bearer authentication. As soon as you don't send the token you should not be authenticated any more (you would now be because of cached value in the session). I will try to produce the patch. Will be my first time but I'll do my best.
(In reply to Robert Rodewald from comment #2) > I suppose you meant: > Boolean.TRUE.equals(map.get("javax.servlet.http.registerSession")) ? Derp. > The third option is important in my opinion. I'm thinking of Bearer > authentication. As soon as you don't send the token you should not be > authenticated any more (you would now be because of cached value in the > session). Aha, I get it, and yes, I agree. Although honestly Bearer authentication should probably not be using a session but there's no need to be sloppy. > I will try to produce the patch. Will be my first time but I'll do my best. Excellent. Ask on the dev@ list if you need any help with that; we're happy to help.
Created attachment 37428 [details] Patch for Bug 64713 - extended evaluation of "jakarta.servlet.http.registerSession" (false value disables caching of principal in session) - removed unnecessary null-check for map (can not be null as per spec and Tomcat implementation)
Created attachment 37429 [details] Combined patch for bugs 64712, 64713 combination of the patches for bug 64712 and 64713 to avoid merge conflicts (hope that helps)
Thanks for this. I'd looked at bug 64712 and come up with a patch for that that also (I thought) addressed this bug but I had missed your case 3. Using getOrDefault is tempting but given the importance of getting this right I'd rather keep the code in sync across versions which means sticking to Java 1.7 methods. It shouldn't take too long to merge my changes with your patch. I should get the fix for this committed shortly.
Fixed in: - master for 10.0.0-M8 onwards - 9.0.x for 9.0.38 onwards - 8.5.x for 8.5.58 onwards