Finally, attach a patch file to *GERONIMO-5624<https://issues.apache.org/jira/browse/GERONIMO-5624>:-) *
2010/9/19 David Jencks <[email protected]> > > 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 > > > -- Ivan
