Skip to content

Commit

Permalink
Refactor of fix for #5810
Browse files Browse the repository at this point in the history 
 + Converted `ConstraintSecurityHandler` to use `PathMappings` so it can handle non standard PathSpecs
  • Loading branch information
@gregw
gregw committed Jan 13, 2021
1 parent 77fe316 commit f4e4a10
Show file tree
Hide file tree
Showing 6 changed files with 55 additions and 46 deletions.
4 changes: 2 additions & 2 deletions jetty-security/src/main/java/org/eclipse/jetty/security/ConstraintMapping.java
View file
Expand Up @@ -74,11 +74,11 @@ public void setMethod(String method)
}

/**
* @return Returns the pathSpec.
* @return Returns the pathSpec as a String only if it is a {@link ServletPathSpec}
*/
public String getPathSpec()
{
return _pathSpec.getDeclaration();
return (ServletPathSpec.class.isInstance(_pathSpec)) ? _pathSpec.getDeclaration() : null;
}

/**
Expand Down
56 changes: 25 additions & 31 deletions jetty-security/src/main/java/org/eclipse/jetty/security/ConstraintSecurityHandler.java
View file
Expand Up @@ -39,7 +39,8 @@
import javax.servlet.annotation.ServletSecurity.TransportGuarantee;

import org.eclipse.jetty.http.HttpStatus;
import org.eclipse.jetty.http.PathMap;
import org.eclipse.jetty.http.pathmap.MappedResource;
import org.eclipse.jetty.http.pathmap.PathMappings;
import org.eclipse.jetty.http.pathmap.PathSpec;
import org.eclipse.jetty.http.pathmap.ServletPathSpec;
import org.eclipse.jetty.server.HttpConfiguration;
Expand Down Expand Up @@ -69,8 +70,7 @@ public class ConstraintSecurityHandler extends SecurityHandler implements Constr
private final List<ConstraintMapping> _constraintMappings = new CopyOnWriteArrayList<>();
private final Set<String> _roles = new CopyOnWriteArraySet<>();

// TODO convert to PathMappings
private final PathMap<Map<String, RoleInfo>> _constraintMap = new PathMap<>();
private final PathMappings<Map<String, RoleInfo>> _constraintMap = new PathMappings<>();
private boolean _denyUncoveredMethods = false;

public static Constraint createConstraint()
Expand Down Expand Up @@ -390,9 +390,9 @@ public void addRole(String role)
if (isStarted() && modified)
{
// Add the new role to currently defined any role role infos
for (Map<String, RoleInfo> map : _constraintMap.values())
for (MappedResource<Map<String, RoleInfo>> map : _constraintMap)
{
for (RoleInfo info : map.values())
for (RoleInfo info : map.getResource().values())
{
if (info.isAnyRole())
info.addRole(role);
Expand All @@ -407,14 +407,8 @@ public void addRole(String role)
@Override
protected void doStart() throws Exception
{
_constraintMap.clear();
if (_constraintMappings != null)
{
for (ConstraintMapping mapping : _constraintMappings)
{
processConstraintMapping(mapping);
}
}
_constraintMap.reset();
_constraintMappings.forEach(this::processConstraintMapping);

//Servlet Spec 3.1 pg 147 sec 13.8.4.2 log paths for which there are uncovered http methods
checkPathsWithUncoveredHttpMethods();
Expand All @@ -426,7 +420,7 @@ protected void doStart() throws Exception
protected void doStop() throws Exception
{
super.doStop();
_constraintMap.clear();
_constraintMap.reset();
}

/**
Expand All @@ -437,13 +431,12 @@ protected void doStop() throws Exception
*/
protected void processConstraintMapping(ConstraintMapping mapping)
{
// TODO handle all path spec types
Map<String, RoleInfo> mappings = _constraintMap.get(mapping.getPathSpec());
Map<String, RoleInfo> mappings = _constraintMap.get(mapping.toPathSpec());

if (mappings == null)
{
mappings = new HashMap<>();
_constraintMap.put(mapping.getPathSpec(), mappings);
_constraintMap.put(mapping.toPathSpec(), mappings);
}
RoleInfo allMethodsRoleInfo = mappings.get(ALL_METHODS);
if (allMethodsRoleInfo != null && allMethodsRoleInfo.isForbidden())
Expand Down Expand Up @@ -584,7 +577,8 @@ else if (mapping.getConstraint().isAnyAuth())
@Override
protected RoleInfo prepareConstraintInfo(String pathInContext, Request request)
{
Map<String, RoleInfo> mappings = _constraintMap.match(pathInContext);
MappedResource<Map<String, RoleInfo>> mapped = _constraintMap.getMatch(pathInContext);
Map<String, RoleInfo> mappings = mapped == null ? null : mapped.getResource();

if (mappings != null)
{
Expand Down Expand Up @@ -731,7 +725,7 @@ public void dump(Appendable out, String indent) throws IOException
{
dumpObjects(out, indent,
DumpableCollection.from("roles", _roles),
DumpableCollection.from("constraints", _constraintMap.entrySet()));
DumpableCollection.from("constraints", _constraintMap));
}

/**
Expand All @@ -755,10 +749,10 @@ public boolean isDenyUncoveredHttpMethods()
@Override
public boolean checkPathsWithUncoveredHttpMethods()
{
Set<String> paths = getPathsWithUncoveredHttpMethods();
Set<PathSpec> paths = getPathsWithUncoveredHttpMethods();
if (paths != null && !paths.isEmpty())
{
for (String p : paths)
for (PathSpec p : paths)
{
LOG.warn("{} has uncovered http methods for path: {}", ContextHandler.getCurrentContext(), p);
}
Expand All @@ -777,16 +771,16 @@ public boolean checkPathsWithUncoveredHttpMethods()
*
* @return list of paths for which there are uncovered methods
*/
public Set<String> getPathsWithUncoveredHttpMethods()
public Set<PathSpec> getPathsWithUncoveredHttpMethods()
{
//if automatically denying uncovered methods, there are no uncovered methods
if (_denyUncoveredMethods)
return Collections.emptySet();

Set<String> uncoveredPaths = new HashSet<>();
for (Entry<String,Map<String, RoleInfo>> entry : _constraintMap.entrySet())
Set<PathSpec> uncoveredPathSpecs = new HashSet<>();
for (MappedResource<Map<String, RoleInfo>> entry : _constraintMap)
{
Map<String, RoleInfo> methodMappings = entry.getValue();
Map<String, RoleInfo> methodMappings = entry.getResource();

//Each key is either:
// : an exact method name
Expand All @@ -795,7 +789,8 @@ public Set<String> getPathsWithUncoveredHttpMethods()
if (methodMappings.get(ALL_METHODS) != null)
continue; //can't be any uncovered methods for this url path

boolean hasOmissions = omissionsExist(entry.getKey(), methodMappings);
// TODO handle or ignore non standard pathspec
boolean hasOmissions = omissionsExist(methodMappings);

for (String method : methodMappings.keySet())
{
Expand All @@ -805,30 +800,29 @@ public Set<String> getPathsWithUncoveredHttpMethods()
for (String m : omittedMethods)
{
if (!methodMappings.containsKey(m))
uncoveredPaths.add(entry.getKey());
uncoveredPathSpecs.add(entry.getPathSpec());
}
}
else
{
//an exact method name
if (!hasOmissions)
//an http-method does not have http-method-omission to cover the other method names
uncoveredPaths.add(entry.getKey());
uncoveredPathSpecs.add(entry.getPathSpec());
}
}
}
return uncoveredPaths;
return uncoveredPathSpecs;
}

/**
* Check if any http method omissions exist in the list of method
* to auth info mappings.
*
* @param path the path
* @param methodMappings the method mappings
* @return true if omission exist
*/
protected boolean omissionsExist(String path, Map<String, RoleInfo> methodMappings)
protected boolean omissionsExist(Map<String, RoleInfo> methodMappings)
{
if (methodMappings == null)
return false;
Expand Down
8 changes: 5 additions & 3 deletions jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java
View file
Expand Up @@ -46,6 +46,8 @@
import org.eclipse.jetty.http.HttpHeader;
import org.eclipse.jetty.http.HttpStatus;
import org.eclipse.jetty.http.HttpTester;
import org.eclipse.jetty.http.pathmap.PathSpec;
import org.eclipse.jetty.http.pathmap.ServletPathSpec;
import org.eclipse.jetty.security.authentication.BasicAuthenticator;
import org.eclipse.jetty.security.authentication.DigestAuthenticator;
import org.eclipse.jetty.security.authentication.FormAuthenticator;
Expand Down Expand Up @@ -405,7 +407,7 @@ public void testUncoveredHttpMethodDetection() throws Exception
_security.setAuthenticator(new BasicAuthenticator());
_server.start();

Set<String> uncoveredPaths = _security.getPathsWithUncoveredHttpMethods();
Set<PathSpec> uncoveredPaths = _security.getPathsWithUncoveredHttpMethods();
assertTrue(uncoveredPaths.isEmpty()); //no uncovered methods

//Test only an explicitly named method, no omissions to cover other methods
Expand All @@ -422,7 +424,7 @@ public void testUncoveredHttpMethodDetection() throws Exception
uncoveredPaths = _security.getPathsWithUncoveredHttpMethods();
assertNotNull(uncoveredPaths);
assertEquals(1, uncoveredPaths.size());
assertThat("/user/*", is(in(uncoveredPaths)));
assertThat(new ServletPathSpec("/user/*"), is(in(uncoveredPaths)));

//Test an explicitly named method with an http-method-omission to cover all other methods
Constraint constraint2a = new Constraint();
Expand Down Expand Up @@ -450,7 +452,7 @@ public void testUncoveredHttpMethodDetection() throws Exception
_security.addConstraintMapping(mapping3);
uncoveredPaths = _security.getPathsWithUncoveredHttpMethods();
assertNotNull(uncoveredPaths);
assertThat("/omit/*", is(in(uncoveredPaths)));
assertThat(new ServletPathSpec("/omit/*"), is(in(uncoveredPaths)));

_security.setDenyUncoveredHttpMethods(true);
uncoveredPaths = _security.getPathsWithUncoveredHttpMethods();
Expand Down
6 changes: 4 additions & 2 deletions jetty-security/src/test/java/org/eclipse/jetty/security/SpecExampleConstraintTest.java
View file
Expand Up @@ -27,6 +27,8 @@
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import org.eclipse.jetty.http.pathmap.PathSpec;
import org.eclipse.jetty.http.pathmap.ServletPathSpec;
import org.eclipse.jetty.security.authentication.BasicAuthenticator;
import org.eclipse.jetty.server.Connector;
import org.eclipse.jetty.server.LocalConnector;
Expand Down Expand Up @@ -237,9 +239,9 @@ public void testUncoveredHttpMethodDetection() throws Exception
_security.setAuthenticator(new BasicAuthenticator());
_server.start();

Set<String> paths = _security.getPathsWithUncoveredHttpMethods();
Set<PathSpec> paths = _security.getPathsWithUncoveredHttpMethods();
assertEquals(1, paths.size());
assertEquals("/*", paths.iterator().next());
assertEquals(new ServletPathSpec("/*"), paths.iterator().next());
}

@Test
Expand Down
11 changes: 11 additions & 0 deletions jetty-servlet/src/main/java/org/eclipse/jetty/servlet/Mapping.java
View file
Expand Up @@ -19,6 +19,7 @@
package org.eclipse.jetty.servlet;

import java.util.Arrays;
import java.util.Collection;
import java.util.Objects;
import java.util.stream.Stream;

Expand Down Expand Up @@ -107,6 +108,16 @@ public void setPathSpecs(PathSpec[] pathSpecs)
: Arrays.stream(pathSpecs).filter(Objects::nonNull).toArray(PathSpec[]::new);
}

/**
* @param pathSpecs The pathSpecs to set, which are assumed to be {@link ServletPathSpec}s
*/
public void setPathSpecs(Collection<PathSpec> pathSpecs)
{
_pathSpecs = (pathSpecs == null)
? new PathSpec[]{}
: pathSpecs.stream().filter(Objects::nonNull).toArray(PathSpec[]::new);
}

public Source getSource()
{
return _source;
Expand Down
16 changes: 8 additions & 8 deletions jetty-webapp/src/main/java/org/eclipse/jetty/webapp/StandardDescriptorProcessor.java
View file
Expand Up @@ -1185,7 +1185,7 @@ public ServletMapping addServletMapping(String servletName, XmlParser.Node node,
mapping.setServletName(servletName);
mapping.setDefault(descriptor instanceof DefaultsDescriptor);

List<String> paths = new ArrayList<String>();
List<PathSpec> pathspecs = new ArrayList<>();
Iterator<XmlParser.Node> iter = node.iterator("url-pattern");
while (iter.hasNext())
{
Expand All @@ -1198,13 +1198,13 @@ public ServletMapping addServletMapping(String servletName, XmlParser.Node node,
while (listItor.hasNext() && !found)
{
ServletMapping sm = listItor.next();
if (sm.getPathSpecs() != null)
if (sm.hasPathSpecs())
{
for (String ps : sm.getPathSpecs())
for (PathSpec ps : sm.toPathSpecs())
{
//The same path has been mapped multiple times, either to a different servlet or the same servlet.
//If its a different servlet, this is only valid to do if the old mapping was from a default descriptor.
if (p.equals(ps) && (sm.isDefault() || servletName.equals(sm.getServletName())))
if (ps.is(p) && (sm.isDefault() || servletName.equals(sm.getServletName())))
{
if (sm.isDefault())
{
Expand All @@ -1216,7 +1216,7 @@ public ServletMapping addServletMapping(String servletName, XmlParser.Node node,

//remove ps from the path specs on the existing mapping
//if the mapping now has no pathspecs, remove it
String[] updatedPaths = ArrayUtil.removeFromArray(sm.getPathSpecs(), ps);
PathSpec[] updatedPaths = ArrayUtil.removeFromArray(sm.toPathSpecs(), ps);

if (updatedPaths == null || updatedPaths.length == 0)
{
Expand All @@ -1237,11 +1237,11 @@ public ServletMapping addServletMapping(String servletName, XmlParser.Node node,
}
}

paths.add(p);
pathspecs.add(new ServletPathSpec(p));
context.getMetaData().setOrigin(servletName + ".servlet.mapping." + p, descriptor);
}

mapping.setPathSpecs(paths.toArray(new String[paths.size()]));
mapping.setPathSpecs(pathspecs.toArray(new PathSpec[0]));
if (LOG.isDebugEnabled())
LOG.debug("Added mapping {} ", mapping);
_servletMappings.add(mapping);
Expand Down Expand Up @@ -1393,7 +1393,7 @@ public void visitJspConfig(WebAppContext context, Descriptor descriptor, XmlPars
//no paths in jsp servlet mapping, we will add all of ours
if (LOG.isDebugEnabled())
LOG.debug("Adding all paths from jsp-config to jsp servlet mapping");
jspMapping.setPathSpecs(jspPaths.toArray(new PathSpec[0]));
jspMapping.setPathSpecs(jspPaths);
}
else
{
Expand Down

0 comments on commit f4e4a10

Please sign in to comment.