On 22/06/2015 20:57, Fjodor Vershinin wrote: > Hi! > There are new bunch of patches ready in my github repo: > https://github.com/fjodorver/tomcat/commits/feature/jaspic-implementation
Thanks. Patches applied. I've added comments to some of the patches. > My report for previous week + today: You still need to address the issue of a unique name for the JASPIC app context. > 1) I have prepared mechanism for registration embedded JASPIC modules > 2) Callback handler is singleton now > 3) Implemented JAAS Subject's support (it turned out, that it is mandatory). > 4) BASIC and DIGEST authenticators has been ported to JASPIC > I think these modules need to be carefully refactored though, then I will > prepare some tests. Why do you think these modules need to be refactored? Given the security nature of this code and that what you have currently is largely copied directly from the existing implementations, I'd be wary of making any changes without a good reason for doing so. > 5) Fixed some bugs in implementation, such as lack of session caching > 6) Currently, I am working on some javadoc's, but I'll commit them later. Remember, little and often is better than a few larger code dumps. The recent commits have been fine but I would prefer to see 1 or 2 commits a day rather than a batch of 10+ commits once a week. > Some problems I have: > 1) I need some convenient way to get user roles from Realm. I assume, that > every Principal is GenericPrincipal, but I guess that's not right. What for? The best way to handle this depends on why/where that information is needed. > 2) We need find a easy way for configuring embedded JASPIC modules. For > example, form authentication requires login page and error page. I think > that these parameters can be passed to JASPIC provider directly, but I'm > not sure. Currently the ContextConfig registers a new TomcatAuthConfigProvider for each web application. The TomcatAuthConfigProvider creates (lazily) a TomcatAuthConfig. The TomcatAuthConfig creates (lazily) TomcatServerAuthContext with all available modules. The TomcatAuthConfig then looks up the authentication type obtained from the request and maps it to the right module. Initialising all the modules when - typically - only one is required looks wrong to me. I'd expect the ContextConfig to specify (possibly even create and configure) the required modules and pass those to the TomcatAuthConfigProvider instance for the web application. Mark > 3) Arjan, can you have a look at current implementation and give some > comments on current implementation. > > 2015-06-17 12:47 GMT+03:00 Mark Thomas <ma...@apache.org>: > >> On 17/06/2015 08:32, Fjodor Vershinin wrote: >>> Could you provide me your eclipse config files for this project? I think >> it >>> would be most convenient way to fix such kind issues. >> >> This is something that would have been covered during community bonding. >> >> http://svn.apache.org/viewvc/tomcat/trunk/res/ide-support/ >> >>> I added some Javadocs, however current implementation is not that stable, >>> so I'll continue commenting code when code will be more solid. >> >> Comments in the code are just as importantas the Javadoc. I'm not too >> bothered about ensuring every public method is fully documented with >> Javadoc. The important thing is that there are enough comments for >> someone to understand the code. >> >>>> 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. >> >> Yes, that is fine. No need to use i18n for temporary code. Do make sure >> there is a TODO marker there so nothing gets missed. >> >>>> 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. >> >> That is better but it is still not unique. It is rare but Tomcat >> instances can be configured with multiple services and those services >> may have host names and contexts paths duplicated between them. You >> really need to find a way to include the engine name as well. You can't >> use the address:port since there may be multiple connectors with >> different addresses and/or ports. >> >> I'd ignore the request and use the fact that Valves have a Container and >> that that Container will have a reference to its ancestors. The >> >>> 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. >> >> I'm not sure about performance either. My general approach is to focus >> on functional correctness and worry about performance once I have >> something that is working. Tuning a working implementation is a lot >> easier than fixing a tuned but broken implementation. I do try to avoid >> any obvious performance pitfalls as I go along but the bulk of tuning >> will happen latter. Premature optimisation nearly always causes more >> problems than it solves. >> >>>> 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. >> >> What I mean is how is this linked in to the Realm (which is currently >> unused) since it is the Realm that creates the Pincipal. >> >> I am encouraged by the most recent round of patches. Progress is being >> made and I can see the direction development is heading in. >> >> I look forward to the next set of patches. >> >> Mark >> >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org >> For additional commands, e-mail: dev-h...@tomcat.apache.org >> >> > > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
