Skip to content

Commit

Permalink
Process all ServletSecurity annotations at web application start rather
Browse files Browse the repository at this point in the history 
than at servlet load time to ensure constraints are applied
consistently.

git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@1823310 13f79535-47bb-0310-9956-ffa450edef68
  • Loading branch information
@markt-asf
markt-asf committed Feb 6, 2018
1 parent 723ea6a commit 3e54b2a
Show file tree
Hide file tree
Showing 10 changed files with 80 additions and 78 deletions.
18 changes: 10 additions & 8 deletions java/org/apache/catalina/Wrapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down
8 changes: 0 additions & 8 deletions java/org/apache/catalina/authenticator/AuthenticatorBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -479,13 +478,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);
Expand Down
18 changes: 17 additions & 1 deletion java/org/apache/catalina/core/ApplicationContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,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.HttpServletMapping;
import javax.servlet.http.HttpSessionAttributeListener;
Expand All @@ -67,6 +69,7 @@
import org.apache.catalina.Wrapper;
import org.apache.catalina.connector.Connector;
import org.apache.catalina.mapper.MappingData;
import org.apache.catalina.util.Introspection;
import org.apache.catalina.util.ServerInfo;
import org.apache.catalina.util.URLEncoder;
import org.apache.tomcat.util.ExceptionUtils;
Expand Down Expand Up @@ -931,11 +934,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) {
Expand All @@ -944,7 +955,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;
}


Expand Down
7 changes: 7 additions & 0 deletions java/org/apache/catalina/core/ApplicationServletRegistration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ public class ApplicationServletRegistration

private final Wrapper wrapper;
private final Context context;
private ServletSecurityElement constraint;

public ApplicationServletRegistration(Wrapper wrapper,
Context context) {
Expand Down Expand Up @@ -160,6 +161,7 @@ public Set<String> setServletSecurity(ServletSecurityElement constraint) {
getName(), context.getName()));
}

this.constraint = constraint;
return context.addServletSecurity(this, constraint);
}

Expand Down Expand Up @@ -194,6 +196,11 @@ public Set<String> addMapping(String... urlPatterns) {
context.addServletMappingDecoded(
UDecoder.URLDecode(urlPattern, StandardCharsets.UTF_8), wrapper.getName());
}

if (constraint != null) {
context.addServletSecurity(this, constraint);
}

return Collections.emptySet();
}

Expand Down
30 changes: 18 additions & 12 deletions java/org/apache/catalina/core/StandardContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4343,28 +4343,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.
*/
Expand Down Expand Up @@ -5639,8 +5647,6 @@ public Set<String> addServletSecurity(
newSecurityConstraints) {
addConstraint(securityConstraint);
}

checkConstraintsForUncoveredMethods(newSecurityConstraints);
}
}

Expand Down
43 changes: 2 additions & 41 deletions java/org/apache/catalina/core/StandardWrapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -43,11 +41,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;
Expand Down Expand Up @@ -257,8 +253,6 @@ public StandardWrapper() {
*/
protected boolean enabled = true;

protected volatile boolean servletSecurityAnnotationScanRequired = false;

private boolean overridable = false;

/**
Expand Down Expand Up @@ -616,7 +610,7 @@ public void setServlet(Servlet servlet) {
*/
@Override
public void setServletSecurityAnnotationScanRequired(boolean b) {
this.servletSecurityAnnotationScanRequired = b;
// NO-OP
}

// --------------------------------------------------------- Public Methods
Expand Down Expand Up @@ -1075,8 +1069,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
Expand Down Expand Up @@ -1119,40 +1111,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 {
Expand Down
9 changes: 4 additions & 5 deletions java/org/apache/catalina/startup/ContextConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
3 changes: 3 additions & 0 deletions java/org/apache/catalina/startup/Tomcat.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -967,6 +967,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) {
Expand Down
17 changes: 14 additions & 3 deletions java/org/apache/catalina/startup/WebAnnotationSet.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
}
}
}
Expand Down
5 changes: 5 additions & 0 deletions webapps/docs/changelog.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,11 @@
<bug>62067</bug>: Correctly apply security constraints mapped to the
context root using a URL pattern of <code>&quot;&quot;</code>. (markt)
</fix>
<fix>
Process all <code>ServletSecurity</code> annotations at web application
start rather than at servlet load time to ensure constraints are applied
consistently. (markt)
</fix>
</changelog>
</subsection>
<subsection name="Coyote">
Expand Down

0 comments on commit 3e54b2a

Please sign in to comment.