On Sep 18, 2010, at 1:14 AM, Ivan wrote: > > > 2010/9/18 David Jencks <[email protected]> > > On Sep 18, 2010, at 12:24 AM, Ivan wrote: > >> >> >> 2010/9/18 David Jencks <[email protected]> >> >> On Sep 17, 2010, at 5:36 PM, Ivan wrote: >> >>> >>> >>> 2010/9/18 David Jencks <[email protected]> >>> >>> On Sep 17, 2010, at 12:44 AM, Ivan wrote: >>> >>> > Hi, >>> > While looking at some Servlet Security JIRAs, I begun some code >>> > refactors on the SpecSecurityBuilder, including : >>> > a. Add more Info class for the security configurations, and serialize >>> > those in the .ser file, with them, it would avoid the xml parsing on the >>> > startup time and make the codes look simple >>> >>> excellent idea! >>> >>> > b. Use ServletContext more in the SpecSecurityBuilder, as it is more >>> > helpful for some calculations, such as get the mapping urls for the >>> > target servlet. >>> >>> I'm not sure what you mean here, but I haven't looked closely at >>> SpecSecurityBuilder. Could you be more specific? >>> >>> e.g. Use the method ServetContext.getServletRegistration().getMapping >>> could eaisly get all the url patterns of the target servlet. >> >> I don't think this will be ideal. It won't work except for servlets where >> the security is set through the servletRegistration rather than through >> web.xml, since the url patterns in web.xml don't have to be servlet >> mappings. For jetty, I think a more "native" solution will work better, see >> below. >> >> I might double check this, but from the Java Doc, it did not write >> explicitly that they are only for new added servlets. > > right, but my point is that url patterns in a constraint in web.xml may not > appear explicitly in any servlet mapping. > >> Anyway, if it works, we could use it as an util method, if not, we could >> calculate the mapping from the info classes. >> Another thing that could be refractor is that to make >> specsecuritybuilder is a pure specsecuritybuilder, currently, it is also >> used to handle how to scan servletsecurity annotation, and detect which >> security configuration has high priority. >> >> >> >>> >>> > >>> > To make these functions work, especially the option b. it requires to >>> > enable declarative security in Jetty integration, generally speaking, >>> > will adopt the same way as Tomcat integration does, >>> > a. create a Wrapper class for ServletContextHandler.Context class, so >>> > that we could monitor those new added dynamic servlets. One thing might >>> > be care is that the codes need to distinguish the servlets from web.xml, >>> > as they are also added by ServletContext now in Jetty. >>> > b. Add a EventListener to ServletContextHandler, it will be >>> > resposible for the security calculation and fill it into >>> > ApplicationPolicyConfigurationManager. >>> > >>> >>> I think you mean "declarative security for servlets added by the addServlet >>> methods on ServletContext"? Jetty will want to deal with that too, so I >>> think putting something in the jetty code that calls out to a security >>> builder of some kind (we can install our own) is the best plan here. Then >>> we shouldn't need more wrapping. Maybe I don't understand exactly what you >>> mean? What would the event listener do? >>> >>> In the Tomcat integration, a JACCListener is attached to the web context, >>> while it receives the initialize event, a specsecuritybuilder is created, >>> while the started event is received, it will build the permissions and fill >>> the result to the policymanager. >>> >>> We need wrapper here, especially in Jetty now, since all the servlets are >>> registered dynamically, for jetty itself, it will not know what to return >>> for some methods, e,g, ServletRegistration.Dynamic.setServletSecurity >>> method, it needs to return all the un-affected url patterns. Also, since >>> most of the security related issues are handled by Geronimo, there is no >>> need to call the initial codes. >> >> Right now this is not implemented in jetty at all. I think we should define >> an interface >> >> ConstraintStore { >> public Set<String> setServletSecurity(String servletName, >> ServletSecurityElement securityElement); >> } >> >> that the ServletRegistration will delegate to. Our implementation can >> implement it based on the info tree from the deployment descriptor. When we >> start the context we can convert the ServletSecurityElements and url >> mappings to info, and proceed to generate the permissions. >> >> >> setServletSecurity definitely needs to be delegated, but it is not enought. >> Other methods like "createServlet/addServlet/.." might also be required. > > We already provide the implementation for creating servlet instances. We can > take the opportunity to scan for the security annotation as well. > >> Actually, the declaritive security is much complicated, especially whether >> or not need to take care the ServletSecurity annotation. I quoted some code >> comments from my working copy ; >> /** >> * a. The security constraints in the portable deployment >> descriptor are of the highest priority, >> * b .None security annotations will take effect on the URL >> patterns explicitly configured in the portable deployment desciptor, >> * but for those URL patterns are not configured, the security >> annotations should take effect, except for META-COMPLETE is set with TRUE >> * c. All the dynamic added servlets should take care the >> ServletSecurity annotation, and it seems to have >> nothing to do with the META-COMPLETE flag, two exceptions >> are : >> c1. Users create the servlet by themselves >> c2. ServletRegistration.Dynamic.setServletConstraint is >> called >> * d. For those URL patterns added by >> ServletContext.getServletRegistration().addMappping, and those target >> servlets are configured >> * in the portable deployment plan, ServletSecurity annotation >> should also be taken care, except for META-COMPLETE is set with TRUE >> */ > > it's definitely even more complicated than it was in ee5 where it was bad > enough :-) > > As I understand the javadoc we can: > > 1. convert the web.xml constraints to info as soon as we know the complete > web.xml. > 2. if web.xml was not marked metadata complete, we scan all servlets for the > security annotation and track the results by servlet name. > 3. we wait for people to call setServletSecurity and track those by name too, > merging the info by url pattern, where "last wins" > 4. we merge the servlet name to security info map with the info from the > web.xml, where the web.xml wins. > > > Generally speaking, that is what we do now, mightbe some differences on > those detailed handlering. > Another missing point, if the > ServletContext.getServletRegistration.addMapping works for all the servlets, > including the ones in the web.xml (It should be), we also need to apply the > servletsecurity annotation to those new added url mapping.
Maybe I'm not understanding :-) If all the security constraint info is in the web.xml, calling ServletRegistration.addMapping has no effect on on security constraints. Calling ServletRegistration.addMapping has an effect on constraints only if there is a security annotation (not suppressed by metadataComplete = true) or if you call setServletSecurity. Agreed? > Hope I could create a patch tomorrow, anyway, the codes are the most > useful tool for expressing the ideas. And will apreciate your comments ;-) looking forward to it.... definitely clearer that trying to talk about it :-) thanks david jencks > > We can't merge security constraint info from annotations into web.xml because > it can be overridden by this setServletSecurity method, whereas constraints > originally in web.xml can't be. > >> >> Some similar codes are in Tomcat integration, e.g. >> >> https://svn.apache.org/repos/asf/geronimo/server/trunk/plugins/tomcat/geronimo-tomcat7/src/main/java/org/apache/geronimo/tomcat/core/GeronimoApplicationServletRegistrationAdapter.java >> >> https://svn.apache.org/repos/asf/geronimo/server/trunk/plugins/tomcat/geronimo-tomcat7/src/main/java/org/apache/geronimo/tomcat/core/GeronimoApplicationContext.java >> I am not sure how to implement these without creating >> wrapper/adapter for the ServletContext/ServletRegistration, or I miss >> anything somewhere ? > > I'll try to take a look soon. > >> >> It looks to me as if the return value from the spec setServletSecurity is of >> limited use because you can set the security element on your servlet >> registration before you set any servlet mappings, so you wont find out >> about any conflicts with the xml dd. >> >> Even if it is of limited use, think that we need to obey the java doc >> :-) > > of course :-) > > thanks > david jencks > >> >> >> >> thanks >> david jencks >> >> >> >>> >>> >>> > Thoughts ? >>> > To David. I found you did some code changes for Jetty now, and >>> > wonder whether you have bugun some simliar work ? >>> >>> I was thinking about doing something like this but haven't started >>> anything. I did look a little bit into configuring tomcat using the info >>> tree rather than letting tomcat read the web.xml. I've found a bunch of >>> tomcat problems and spec inconsistencies. I haven't gotten to security >>> configuration yet. >>> >>> >>> thanks >>> david jencks >>> >>> > Thanks ! >>> > -- >>> > Ivan >>> >>> >>> >>> >>> -- >>> Ivan >> >> >> >> >> -- >> Ivan > > > > > -- > Ivan
