On 25/06/2015 20:50, Fjodor Vershinin wrote: > Hi! > Fresh set of patches is ready. > What has been done: > 1) Added engine name to getVirtualServerName() > 2) Implemented method for getting roles directly from Realm, > 3) Authentication provider uses LoginConfig now, which gives us ability to > get different options directly from there. > 4) Added test for validating DIGEST auth module, however one test case is > ignored, because not implemented yet. > 5) Updated some javadocs to make them more specific and clear.
All looks good and patches applied. I added a few comments to some of the patches. Thanks, Mark > > BR, > Fjodor > > 2015-06-23 23:18 GMT+03:00 Mark Thomas <ma...@apache.org>: > >> On 23/06/2015 16:50, Fjodor Vershinin wrote: >>> Hi there! >>> >>> >>>> You still need to address the issue of a unique name for the JASPIC app >>>> context. >>> >>> I see your point. However, tomcat's implementation of uniqueness is >> against >>> JASPIC 1.1 specification. We must somehow document this feature. >> >> I'm reading that part of the spec now. >> >> Currently Tomcat returns the name of the host object (not necessarily >> the DNS host name) for ServletContext.getVirtualServerName(). Reading >> the Servlet spec more carefully, we can change that to >> engine-name/host-name and still be specification compliant. That would >> address the uniqueness issue for JASPIC as well as being a better >> implementation for getVirtualServerName(). >> >>>>> 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. >>> >>> Yes, we must be very careful with security implementations. However, I >>> would decouple JASPIC code from authentication algorithms and put them >> into >>> separate classes. >> >> I'm on the fence on this. I don't see it as a priority unless it is >> blocking something else. I'd file this under "come back to it if there >> is time at the end". >> >>>> 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. >>> >>> >>> I agree, however I was intensively using rebase and squashing for commit >>> rewriting in order to get "feature per commit". I think it depends on >>> architectural tasks - currently we have architectural stuff done, so next >>> commits will require less rewriting. >> >> We don't have to merge into Tomcat until you are ready but it would be >> nice to see how the work is developing. >> >>>> 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. >>> >>> >>> I need this info in order to construct GenericPrincipal using callbacks. >>> Currently, Realm is returning GenericPrincipal, however, implementation >> is >>> hidden behind Principal interface. I need to do casting to get >>> GenericPrincipal object, because Principal doesn't have getRoles() >> method. >> >> I suspect that was the case. Casting is going to be fragile for users >> with custom realm implementation. I think what is required is a new >> method on Realm: >> >> String[] getRoles(Principal) >> >> For the current realms this should be a trivial implementation in >> RealmBase: >> - cast to GenericPrincipal >> - return getRoles() >> >> >>>>> 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. >>> >>> >>> Yes, I agree, that it's better solution. I am not sure about constructing >>> auth modules in ContextConfig. May be we can pass LoginConfig info into >>> provider, and construct modules inside. >> >> That sounds even better to me. >> >> 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
