Hi, Mark! I'd updated my jaspic-implementation branch, where I have updated version. Thank you for your comments, almost all of this issues has been fixed.
> The patch triggered a number of basic IDE warnings that should have been already fixed (use of <> and@Override) Could you provide me your eclipse config files for this project? I think it would be most convenient way to fix such kind issues. > The code is completely uncommented. While I can guess at what most of the code is meant to be doing there needs to be some comments to explain what is going on and why. Where appropriate, reference the JASPIC 1.1 spec. I added some Javadocs, however current implementation is not that stable, so I'll continue commenting code when code will be more solid. >All user messages, exception messages etc. should use i18n (StringManager). Fixed. Only "not implemented" exceptions had left, but they will be removed after some time, so I think it's not mandatory to translate them. >There are no unit tests (note most Tomcat unit tests are more like integration tests) I've prepared a couple unit tests and one integration test. >In JaspicAuthenticator.authenticate() request.getLocalName() is not the way to get a unique name for the web application (assuming that is what is required). Has been fixed. Now I get unique name in JASPIC 1.1 style. >In JaspicAuthenticator.getAuthConfig() is a single, shared call back handler the correct model? Would per request/response call back handler instances remove the need for ThreadLocals? >The use of a ThreadLocal JaspicCallbackHandler needs to be justified. Is a ThreadLocal really the only solution? If so there needs to be a very clear comment explaining why. Also, the ThreadLocal must be cleared as soon as it is no longer required. > Will JaspicCallbackHandler.handle(Callback[]) only be called once per authentication? If not, the ThreadLocal is overwritten. All ThreadLocal logic has been replaced with creation of a new instance every time. I'm not sure about performance, but for now it's more convenient. > In JaspicCallbackHandler how is the PrincipalGroupCallback associated with the authenticated Principal? What do you mean under authenticated Principal? Currently, I merge two callback's info into tomcat GenericPrincipal, which contains user principal, user name and roles. Then this GenericPrincipal can be used in Tomcat's internals. I am not sure how to deal with already authenticated Principal's, I need to do some research. >The use of instanceof in PrincipalGroupCallback.addCallback doesn't look right. Why not use separate methods? Fixed. >PrincipalGroupCallback.getPrincipal() has an empty if block. Fixed. -- Thanks, Fjodor
