From 2349801827f09fb6582a8afdeca704294106ad9a Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Tue, 6 Feb 2018 12:15:40 +0000 Subject: [PATCH] Process all ServletSecurity annotations at web application start rather than at servlet load time to ensure constraints are applied consistently. git-svn-id: https://svn.apache.org/repos/asf/tomcat/tc8.5.x/trunk@1823314 13f79535-47bb-0310-9956-ffa450edef68 --- java/org/apache/catalina/Wrapper.java | 18 ++++---- .../authenticator/AuthenticatorBase.java | 8 ---- .../catalina/core/ApplicationContext.java | 18 +++++++- .../core/ApplicationServletRegistration.java | 7 +++ .../apache/catalina/core/StandardContext.java | 30 +++++++------ .../apache/catalina/core/StandardWrapper.java | 43 +------------------ .../catalina/startup/ContextConfig.java | 9 ++-- java/org/apache/catalina/startup/Tomcat.java | 3 ++ .../catalina/startup/WebAnnotationSet.java | 17 ++++++-- webapps/docs/changelog.xml | 5 +++ 10 files changed, 80 insertions(+), 78 deletions(-) diff --git a/java/org/apache/catalina/Wrapper.java b/java/org/apache/catalina/Wrapper.java index 4f366dd734a7..ec9a6e54063b 100644 --- a/java/org/apache/catalina/Wrapper.java +++ b/java/org/apache/catalina/Wrapper.java @@ -368,21 +368,23 @@ public void setMultipartConfigElement( public void setEnabled(boolean enabled); /** - * Set the flag that indicates - * {@link javax.servlet.annotation.ServletSecurity} annotations must be - * scanned when the Servlet is first used. + * This method is no longer used. All implementations should be NO-OPs. * - * @param b The new value of the flag + * @param b Unused. + * + * @deprecated This will be removed in Tomcat 9. */ + @Deprecated public void setServletSecurityAnnotationScanRequired(boolean b); /** - * Scan for (if necessary) and process (if found) the - * {@link javax.servlet.annotation.ServletSecurity} annotations for the - * Servlet associated with this wrapper. + * This method is no longer used. All implementations should be NO-OPs. + * + * @throws ServletException Never thrown * - * @throws ServletException if an annotation scanning error occurs + * @deprecated This will be removed in Tomcat 9. */ + @Deprecated public void servletSecurityAnnotationScan() throws ServletException; /** diff --git a/java/org/apache/catalina/authenticator/AuthenticatorBase.java b/java/org/apache/catalina/authenticator/AuthenticatorBase.java index c8516d2a14bc..fb85ce10f8bf 100644 --- a/java/org/apache/catalina/authenticator/AuthenticatorBase.java +++ b/java/org/apache/catalina/authenticator/AuthenticatorBase.java @@ -52,7 +52,6 @@ import org.apache.catalina.Session; import org.apache.catalina.TomcatPrincipal; import org.apache.catalina.Valve; -import org.apache.catalina.Wrapper; import org.apache.catalina.authenticator.jaspic.CallbackHandlerImpl; import org.apache.catalina.authenticator.jaspic.MessageInfoImpl; import org.apache.catalina.connector.Request; @@ -481,13 +480,6 @@ public void invoke(Request request, Response response) throws IOException, Servl boolean authRequired = isContinuationRequired(request); - // The Servlet may specify security constraints through annotations. - // Ensure that they have been processed before constraints are checked - Wrapper wrapper = request.getWrapper(); - if (wrapper != null) { - wrapper.servletSecurityAnnotationScan(); - } - Realm realm = this.context.getRealm(); // Is this request URI subject to a security constraint? SecurityConstraint[] constraints = realm.findSecurityConstraints(request, this.context); diff --git a/java/org/apache/catalina/core/ApplicationContext.java b/java/org/apache/catalina/core/ApplicationContext.java index 207b94311638..b7c50756771b 100644 --- a/java/org/apache/catalina/core/ApplicationContext.java +++ b/java/org/apache/catalina/core/ApplicationContext.java @@ -51,8 +51,10 @@ import javax.servlet.ServletRegistration.Dynamic; import javax.servlet.ServletRequestAttributeListener; import javax.servlet.ServletRequestListener; +import javax.servlet.ServletSecurityElement; import javax.servlet.SessionCookieConfig; import javax.servlet.SessionTrackingMode; +import javax.servlet.annotation.ServletSecurity; import javax.servlet.descriptor.JspConfigDescriptor; import javax.servlet.http.HttpSessionAttributeListener; import javax.servlet.http.HttpSessionIdListener; @@ -69,6 +71,7 @@ import org.apache.catalina.connector.Connector; import org.apache.catalina.mapper.MappingData; import org.apache.catalina.servlet4preview.http.ServletMapping; +import org.apache.catalina.util.Introspection; import org.apache.catalina.util.ServerInfo; import org.apache.catalina.util.URLEncoder; import org.apache.tomcat.util.ExceptionUtils; @@ -935,11 +938,19 @@ private ServletRegistration.Dynamic addServlet(String servletName, String servle } } + ServletSecurity annotation = null; if (servlet == null) { wrapper.setServletClass(servletClass); + Class clazz = Introspection.loadClass(context, servletClass); + if (clazz != null) { + annotation = clazz.getAnnotation(ServletSecurity.class); + } } else { wrapper.setServletClass(servlet.getClass().getName()); wrapper.setServlet(servlet); + if (context.wasCreatedDynamicServlet(servlet)) { + annotation = servlet.getClass().getAnnotation(ServletSecurity.class); + } } if (initParams != null) { @@ -948,7 +959,12 @@ private ServletRegistration.Dynamic addServlet(String servletName, String servle } } - return context.dynamicServletAdded(wrapper); + ServletRegistration.Dynamic registration = + new ApplicationServletRegistration(wrapper, context); + if (annotation != null) { + registration.setServletSecurity(new ServletSecurityElement(annotation)); + } + return registration; } diff --git a/java/org/apache/catalina/core/ApplicationServletRegistration.java b/java/org/apache/catalina/core/ApplicationServletRegistration.java index 6b98849bff1c..47eda6114508 100644 --- a/java/org/apache/catalina/core/ApplicationServletRegistration.java +++ b/java/org/apache/catalina/core/ApplicationServletRegistration.java @@ -46,6 +46,7 @@ public class ApplicationServletRegistration private final Wrapper wrapper; private final Context context; + private ServletSecurityElement constraint; public ApplicationServletRegistration(Wrapper wrapper, Context context) { @@ -160,6 +161,7 @@ public Set setServletSecurity(ServletSecurityElement constraint) { getName(), context.getName())); } + this.constraint = constraint; return context.addServletSecurity(this, constraint); } @@ -194,6 +196,11 @@ public Set addMapping(String... urlPatterns) { context.addServletMappingDecoded( UDecoder.URLDecode(urlPattern, StandardCharsets.UTF_8), wrapper.getName()); } + + if (constraint != null) { + context.addServletSecurity(this, constraint); + } + return Collections.emptySet(); } diff --git a/java/org/apache/catalina/core/StandardContext.java b/java/org/apache/catalina/core/StandardContext.java index eac4f509a446..f9487408d0ed 100644 --- a/java/org/apache/catalina/core/StandardContext.java +++ b/java/org/apache/catalina/core/StandardContext.java @@ -4443,28 +4443,36 @@ public String getRealPath(String path) { } /** - * Hook to register that we need to scan for security annotations. - * @param wrapper The wrapper for the Servlet that was added - * @return the associated registration + * Create a servlet registration. + * + * @param wrapper The wrapper for which the registration should be created. + * + * @return An appropriate registration + * + * @deprecated This will be removed in Tomcat 9. The registration should be + * created directly. */ + @Deprecated public ServletRegistration.Dynamic dynamicServletAdded(Wrapper wrapper) { - Servlet s = wrapper.getServlet(); - if (s != null && createdServlets.contains(s)) { - // Mark the wrapper to indicate annotations need to be scanned - wrapper.setServletSecurityAnnotationScanRequired(true); - } return new ApplicationServletRegistration(wrapper, this); } /** - * Hook to track which registrations need annotation scanning - * @param servlet the Servlet to add + * Hook to track which Servlets were created via + * {@link ServletContext#createServlet(Class)}. + * + * @param servlet the created Servlet */ public void dynamicServletCreated(Servlet servlet) { createdServlets.add(servlet); } + public boolean wasCreatedDynamicServlet(Servlet servlet) { + return createdServlets.contains(servlet); + } + + /** * A helper class to manage the filter mappings in a Context. */ @@ -5741,8 +5749,6 @@ public Set addServletSecurity( newSecurityConstraints) { addConstraint(securityConstraint); } - - checkConstraintsForUncoveredMethods(newSecurityConstraints); } } diff --git a/java/org/apache/catalina/core/StandardWrapper.java b/java/org/apache/catalina/core/StandardWrapper.java index adda7091f2c7..1af91c4b1209 100644 --- a/java/org/apache/catalina/core/StandardWrapper.java +++ b/java/org/apache/catalina/core/StandardWrapper.java @@ -14,8 +14,6 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - - package org.apache.catalina.core; import java.io.PrintStream; @@ -42,11 +40,9 @@ import javax.servlet.ServletConfig; import javax.servlet.ServletContext; import javax.servlet.ServletException; -import javax.servlet.ServletSecurityElement; import javax.servlet.SingleThreadModel; import javax.servlet.UnavailableException; import javax.servlet.annotation.MultipartConfig; -import javax.servlet.annotation.ServletSecurity; import org.apache.catalina.Container; import org.apache.catalina.ContainerServlet; @@ -256,8 +252,6 @@ public StandardWrapper() { */ protected boolean enabled = true; - protected volatile boolean servletSecurityAnnotationScanRequired = false; - private boolean overridable = false; /** @@ -619,7 +613,7 @@ public void setServlet(Servlet servlet) { */ @Override public void setServletSecurityAnnotationScanRequired(boolean b) { - this.servletSecurityAnnotationScanRequired = b; + // NO-OP } // --------------------------------------------------------- Public Methods @@ -1078,8 +1072,6 @@ public synchronized Servlet loadServlet() throws ServletException { } } - processServletSecurityAnnotation(servlet.getClass()); - // Special handling for ContainerServlet instances // Note: The InstanceManager checks if the application is permitted // to load ContainerServlets @@ -1122,40 +1114,9 @@ public synchronized Servlet loadServlet() throws ServletException { */ @Override public void servletSecurityAnnotationScan() throws ServletException { - if (getServlet() == null) { - Class clazz = null; - try { - clazz = ((Context) getParent()).getLoader().getClassLoader().loadClass( - getServletClass()); - processServletSecurityAnnotation(clazz); - } catch (ClassNotFoundException e) { - // Safe to ignore. No class means no annotations to process - } - } else { - if (servletSecurityAnnotationScanRequired) { - processServletSecurityAnnotation(getServlet().getClass()); - } - } + // NO-OP } - private void processServletSecurityAnnotation(Class clazz) { - // Calling this twice isn't harmful so no syncs - servletSecurityAnnotationScanRequired = false; - - Context ctxt = (Context) getParent(); - - if (ctxt.getIgnoreAnnotations()) { - return; - } - - ServletSecurity secAnnotation = - clazz.getAnnotation(ServletSecurity.class); - if (secAnnotation != null) { - ctxt.addServletSecurity( - new ApplicationServletRegistration(this, ctxt), - new ServletSecurityElement(secAnnotation)); - } - } private synchronized void initServlet(Servlet servlet) throws ServletException { diff --git a/java/org/apache/catalina/startup/ContextConfig.java b/java/org/apache/catalina/startup/ContextConfig.java index da5833021a1d..53cf1431f27d 100644 --- a/java/org/apache/catalina/startup/ContextConfig.java +++ b/java/org/apache/catalina/startup/ContextConfig.java @@ -344,15 +344,14 @@ protected void authenticatorConfig() { LoginConfig loginConfig = context.getLoginConfig(); SecurityConstraint constraints[] = context.findConstraints(); - if (context.getIgnoreAnnotations() && - (constraints == null || constraints.length ==0) && + if ((constraints == null || constraints.length ==0) && !context.getPreemptiveAuthentication()) { + // No need for an authenticator return; } else { if (loginConfig == null) { - // Not metadata-complete or security constraints present, need - // an authenticator to support @ServletSecurity annotations - // and/or constraints + // Security constraints present. Need an authenticator to + // support them. loginConfig = DUMMY_LOGIN_CONFIG; context.setLoginConfig(loginConfig); } diff --git a/java/org/apache/catalina/startup/Tomcat.java b/java/org/apache/catalina/startup/Tomcat.java index c516aa77093c..4ac44d80541a 100644 --- a/java/org/apache/catalina/startup/Tomcat.java +++ b/java/org/apache/catalina/startup/Tomcat.java @@ -970,6 +970,9 @@ public void lifecycleEvent(LifecycleEvent event) { if (event.getType().equals(Lifecycle.CONFIGURE_START_EVENT)) { context.setConfigured(true); + // Process annotations + WebAnnotationSet.loadApplicationAnnotations(context); + // LoginConfig is required to process @ServletSecurity // annotations if (context.getLoginConfig() == null) { diff --git a/java/org/apache/catalina/startup/WebAnnotationSet.java b/java/org/apache/catalina/startup/WebAnnotationSet.java index 398cd38066bd..852bda83392e 100644 --- a/java/org/apache/catalina/startup/WebAnnotationSet.java +++ b/java/org/apache/catalina/startup/WebAnnotationSet.java @@ -23,10 +23,13 @@ import javax.annotation.Resources; import javax.annotation.security.DeclareRoles; import javax.annotation.security.RunAs; +import javax.servlet.ServletSecurityElement; +import javax.servlet.annotation.ServletSecurity; import org.apache.catalina.Container; import org.apache.catalina.Context; import org.apache.catalina.Wrapper; +import org.apache.catalina.core.ApplicationServletRegistration; import org.apache.catalina.util.Introspection; import org.apache.tomcat.util.descriptor.web.ContextEnvironment; import org.apache.tomcat.util.descriptor.web.ContextResource; @@ -136,9 +139,17 @@ protected static void loadApplicationServletAnnotations(Context context) { * Ref JSR 250, equivalent to the run-as element in * the deployment descriptor */ - RunAs annotation = clazz.getAnnotation(RunAs.class); - if (annotation != null) { - wrapper.setRunAs(annotation.value()); + RunAs runAs = clazz.getAnnotation(RunAs.class); + if (runAs != null) { + wrapper.setRunAs(runAs.value()); + } + + // Process ServletSecurity annotation + ServletSecurity servletSecurity = clazz.getAnnotation(ServletSecurity.class); + if (servletSecurity != null) { + context.addServletSecurity( + new ApplicationServletRegistration(wrapper, context), + new ServletSecurityElement(servletSecurity)); } } } diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index ae977d703498..a6904d22bee4 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -77,6 +77,11 @@ When using Tomcat embedded, only perform Authenticator configuration once during web application start. (markt) + + Process all ServletSecurity annotations at web application + start rather than at servlet load time to ensure constraints are applied + consistently. (markt) +