From 29a66dc002bda132bdf63f3d86d6a10b77519f36 Mon Sep 17 00:00:00 2001 From: markt Date: Fri, 8 Oct 2010 13:22:04 +0000 Subject: [PATCH] Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=50015 Re-factor dynamic servlet security implementation to make extensions, such as JACC implementations, simpler. Patch provided by David Jencks. git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@1005811 13f79535-47bb-0310-9956-ffa450edef68 --- java/org/apache/catalina/Context.java | 13 ++++ .../apache/catalina/core/ApplicationContext.java | 9 ++- .../core/ApplicationServletRegistration.java | 56 ++------------ java/org/apache/catalina/core/StandardContext.java | 86 ++++++++++++++++++++++ webapps/docs/changelog.xml | 5 ++ 5 files changed, 115 insertions(+), 54 deletions(-) diff --git a/java/org/apache/catalina/Context.java b/java/org/apache/catalina/Context.java index d03f71865..38769834a 100644 --- a/java/org/apache/catalina/Context.java +++ b/java/org/apache/catalina/Context.java @@ -24,8 +24,10 @@ import java.util.Set; import javax.servlet.ServletContainerInitializer; import javax.servlet.ServletContext; +import javax.servlet.ServletSecurityElement; import javax.servlet.descriptor.JspConfigDescriptor; +import org.apache.catalina.core.ApplicationServletRegistration; import org.apache.catalina.deploy.ApplicationParameter; import org.apache.catalina.deploy.ErrorPage; import org.apache.catalina.deploy.FilterDef; @@ -1222,5 +1224,16 @@ public interface Context extends Container { * Is this context using version 2.2 of the Servlet spec? */ boolean isServlet22(); + + /** + * Notification that servlet security has been dynamically set in a + * {@Link ServletRegistration.Dynamic} + * @param registration servlet security was modified for + * @param servletSecurityElement new security constraints for this servlet + * @return urls currently mapped to this registration that are already + * present in web.xml + */ + Set addServletSecurity(ApplicationServletRegistration registration, + ServletSecurityElement servletSecurityElement); } diff --git a/java/org/apache/catalina/core/ApplicationContext.java b/java/org/apache/catalina/core/ApplicationContext.java index d76c720cf..a590c5318 100644 --- a/java/org/apache/catalina/core/ApplicationContext.java +++ b/java/org/apache/catalina/core/ApplicationContext.java @@ -1082,10 +1082,10 @@ public class ApplicationContext } else { wrapper.setServletClass(servlet.getClass().getName()); wrapper.setServlet(servlet); - } - - return new ApplicationServletRegistration(wrapper, context); - } + } + + return context.dynamicServletAdded(wrapper); + } public T createServlet(Class c) @@ -1093,6 +1093,7 @@ public class ApplicationContext try { @SuppressWarnings("unchecked") T servlet = (T) context.getInstanceManager().newInstance(c.getName()); + context.dynamicServletCreated(servlet); return servlet; } catch (IllegalAccessException e) { throw new ServletException(e); diff --git a/java/org/apache/catalina/core/ApplicationServletRegistration.java b/java/org/apache/catalina/core/ApplicationServletRegistration.java index dd4a372bc..4be7e0ead 100644 --- a/java/org/apache/catalina/core/ApplicationServletRegistration.java +++ b/java/org/apache/catalina/core/ApplicationServletRegistration.java @@ -117,10 +117,12 @@ public class ApplicationServletRegistration // Have to add in a separate loop since spec requires no updates at all // if there is an issue - for (Map.Entry entry : initParameters.entrySet()) { - setInitParameter(entry.getKey(), entry.getValue()); + if (conflicts.isEmpty()) { + for (Map.Entry entry : initParameters.entrySet()) { + setInitParameter(entry.getKey(), entry.getValue()); + } } - + return conflicts; } @@ -158,53 +160,7 @@ public class ApplicationServletRegistration getName(), context.getPath())); } - Set conflicts = new HashSet(); - - Collection urlPatterns = getMappings(); - for (String urlPattern : urlPatterns) { - boolean foundConflict = false; - - SecurityConstraint[] securityConstraints = - context.findConstraints(); - for (SecurityConstraint securityConstraint : securityConstraints) { - - SecurityCollection[] collections = - securityConstraint.findCollections(); - for (SecurityCollection collection : collections) { - if (collection.findPattern(urlPattern)) { - // First pattern found will indicate if there is a - // conflict since for any given pattern all matching - // constraints will be from either the descriptor or - // not. It is not permitted to have a mixture - if (collection.isFromDescriptor()) { - // Skip this pattern - foundConflict = true; - } else { - // Need to overwrite constraint for this pattern - // so remove every pattern found - context.removeConstraint(securityConstraint); - } - } - if (foundConflict) { - break; - } - } - if (foundConflict) { - break; - } - } - if (!foundConflict) { - SecurityConstraint[] newSecurityConstraints = - SecurityConstraint.createConstraints(constraint, - urlPattern); - for (SecurityConstraint securityConstraint : - newSecurityConstraints) { - context.addConstraint(securityConstraint); - } - } - } - - return conflicts; + return context.addServletSecurity(this, constraint); } diff --git a/java/org/apache/catalina/core/StandardContext.java b/java/org/apache/catalina/core/StandardContext.java index b80276fc0..1539318b3 100644 --- a/java/org/apache/catalina/core/StandardContext.java +++ b/java/org/apache/catalina/core/StandardContext.java @@ -26,7 +26,9 @@ import java.io.InputStreamReader; import java.net.URL; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.HashMap; +import java.util.HashSet; import java.util.Hashtable; import java.util.Iterator; import java.util.LinkedHashMap; @@ -47,14 +49,17 @@ import javax.management.ObjectName; import javax.naming.NamingException; import javax.naming.directory.DirContext; import javax.servlet.FilterConfig; +import javax.servlet.Servlet; import javax.servlet.ServletContainerInitializer; import javax.servlet.ServletContext; import javax.servlet.ServletContextAttributeListener; import javax.servlet.ServletContextEvent; import javax.servlet.ServletContextListener; import javax.servlet.ServletException; +import javax.servlet.ServletRegistration; import javax.servlet.ServletRequestAttributeListener; import javax.servlet.ServletRequestListener; +import javax.servlet.ServletSecurityElement; import javax.servlet.descriptor.JspConfigDescriptor; import javax.servlet.http.HttpSessionAttributeListener; import javax.servlet.http.HttpSessionListener; @@ -4050,6 +4055,22 @@ public class StandardContext extends ContainerBase return null; } + /** + * hook to register that we need to scan for security annotations. + * @param registration + */ + public ServletRegistration.Dynamic dynamicServletAdded(Wrapper wrapper) { + return new ApplicationServletRegistration(wrapper, this); + } + + /** + * hook to track which registrations need annotation scanning + * @param servlet + */ + public void dynamicServletCreated(Servlet servlet) { + // NOOP - Hook for JACC implementations + } + /** * A helper class to manage the filter mappings in a Context. @@ -5173,6 +5194,71 @@ public class StandardContext extends ContainerBase } + public Set addServletSecurity( + ApplicationServletRegistration registration, + ServletSecurityElement servletSecurityElement) { + + Set conflicts = new HashSet(); + + Collection urlPatterns = registration.getMappings(); + for (String urlPattern : urlPatterns) { + boolean foundConflict = false; + + SecurityConstraint[] securityConstraints = + findConstraints(); + for (SecurityConstraint securityConstraint : securityConstraints) { + + SecurityCollection[] collections = + securityConstraint.findCollections(); + for (SecurityCollection collection : collections) { + if (collection.findPattern(urlPattern)) { + // First pattern found will indicate if there is a + // conflict since for any given pattern all matching + // constraints will be from either the descriptor or + // not. It is not permitted to have a mixture + if (collection.isFromDescriptor()) { + // Skip this pattern + foundConflict = true; + conflicts.add(urlPattern); + } else { + // Need to overwrite constraint for this pattern + // so remove every pattern found + + // TODO spec 13.4.2 appears to say only the + // conflicting pattern is overwritten, not the + // entire security constraint. + removeConstraint(securityConstraint); + } + } + if (foundConflict) { + break; + } + } + if (foundConflict) { + break; + } + } + // TODO spec 13.4.2 appears to say that non-conflicting patterns are + // still used. + // TODO you can't calculate the eventual security constraint now, + // you have to wait until the context is started, since application + // code can add url patterns after calling setSecurity. + if (!foundConflict) { + SecurityConstraint[] newSecurityConstraints = + SecurityConstraint.createConstraints( + servletSecurityElement, + urlPattern); + for (SecurityConstraint securityConstraint : + newSecurityConstraints) { + addConstraint(securityConstraint); + } + } + } + + return conflicts; + + } + /** * Return a File object representing the base directory for the diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 42928366e..ff02e3ee0 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -89,6 +89,11 @@ 49994: As per the Java EE 6 specification, return a new object instance for each JNDI look up of a resource reference. (markt) + + 50015: Re-factor dynamic servlet security implementation to + make extensions, such as JACC implementations, simpler. Patch provided + by David Jencks. (markt) + -- 2.11.0