From f4e4a101d5f9df7751daac8b9e2d43723a1373b0 Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 13 Jan 2021 10:23:26 +0100 Subject: [PATCH] Refactor of fix for #5810 + Converted `ConstraintSecurityHandler` to use `PathMappings` so it can handle non standard PathSpecs --- .../jetty/security/ConstraintMapping.java | 4 +- .../security/ConstraintSecurityHandler.java | 56 +++++++++---------- .../jetty/security/ConstraintTest.java | 8 ++- .../security/SpecExampleConstraintTest.java | 6 +- .../org/eclipse/jetty/servlet/Mapping.java | 11 ++++ .../webapp/StandardDescriptorProcessor.java | 16 +++--- 6 files changed, 55 insertions(+), 46 deletions(-) diff --git a/jetty-security/src/main/java/org/eclipse/jetty/security/ConstraintMapping.java b/jetty-security/src/main/java/org/eclipse/jetty/security/ConstraintMapping.java index d7fd3e41b256..a511af688771 100644 --- a/jetty-security/src/main/java/org/eclipse/jetty/security/ConstraintMapping.java +++ b/jetty-security/src/main/java/org/eclipse/jetty/security/ConstraintMapping.java @@ -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; } /** diff --git a/jetty-security/src/main/java/org/eclipse/jetty/security/ConstraintSecurityHandler.java b/jetty-security/src/main/java/org/eclipse/jetty/security/ConstraintSecurityHandler.java index daca0f2e2bc4..c115375ad76e 100644 --- a/jetty-security/src/main/java/org/eclipse/jetty/security/ConstraintSecurityHandler.java +++ b/jetty-security/src/main/java/org/eclipse/jetty/security/ConstraintSecurityHandler.java @@ -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; @@ -69,8 +70,7 @@ public class ConstraintSecurityHandler extends SecurityHandler implements Constr private final List _constraintMappings = new CopyOnWriteArrayList<>(); private final Set _roles = new CopyOnWriteArraySet<>(); - // TODO convert to PathMappings - private final PathMap> _constraintMap = new PathMap<>(); + private final PathMappings> _constraintMap = new PathMappings<>(); private boolean _denyUncoveredMethods = false; public static Constraint createConstraint() @@ -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 map : _constraintMap.values()) + for (MappedResource> map : _constraintMap) { - for (RoleInfo info : map.values()) + for (RoleInfo info : map.getResource().values()) { if (info.isAnyRole()) info.addRole(role); @@ -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(); @@ -426,7 +420,7 @@ protected void doStart() throws Exception protected void doStop() throws Exception { super.doStop(); - _constraintMap.clear(); + _constraintMap.reset(); } /** @@ -437,13 +431,12 @@ protected void doStop() throws Exception */ protected void processConstraintMapping(ConstraintMapping mapping) { - // TODO handle all path spec types - Map mappings = _constraintMap.get(mapping.getPathSpec()); + Map 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()) @@ -584,7 +577,8 @@ else if (mapping.getConstraint().isAnyAuth()) @Override protected RoleInfo prepareConstraintInfo(String pathInContext, Request request) { - Map mappings = _constraintMap.match(pathInContext); + MappedResource> mapped = _constraintMap.getMatch(pathInContext); + Map mappings = mapped == null ? null : mapped.getResource(); if (mappings != null) { @@ -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)); } /** @@ -755,10 +749,10 @@ public boolean isDenyUncoveredHttpMethods() @Override public boolean checkPathsWithUncoveredHttpMethods() { - Set paths = getPathsWithUncoveredHttpMethods(); + Set 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); } @@ -777,16 +771,16 @@ public boolean checkPathsWithUncoveredHttpMethods() * * @return list of paths for which there are uncovered methods */ - public Set getPathsWithUncoveredHttpMethods() + public Set getPathsWithUncoveredHttpMethods() { //if automatically denying uncovered methods, there are no uncovered methods if (_denyUncoveredMethods) return Collections.emptySet(); - Set uncoveredPaths = new HashSet<>(); - for (Entry> entry : _constraintMap.entrySet()) + Set uncoveredPathSpecs = new HashSet<>(); + for (MappedResource> entry : _constraintMap) { - Map methodMappings = entry.getValue(); + Map methodMappings = entry.getResource(); //Each key is either: // : an exact method name @@ -795,7 +789,8 @@ public Set 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()) { @@ -805,7 +800,7 @@ public Set getPathsWithUncoveredHttpMethods() for (String m : omittedMethods) { if (!methodMappings.containsKey(m)) - uncoveredPaths.add(entry.getKey()); + uncoveredPathSpecs.add(entry.getPathSpec()); } } else @@ -813,22 +808,21 @@ public Set getPathsWithUncoveredHttpMethods() //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 methodMappings) + protected boolean omissionsExist(Map methodMappings) { if (methodMappings == null) return false; diff --git a/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java b/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java index 9dfd30810200..de96831a1848 100644 --- a/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java +++ b/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java @@ -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; @@ -405,7 +407,7 @@ public void testUncoveredHttpMethodDetection() throws Exception _security.setAuthenticator(new BasicAuthenticator()); _server.start(); - Set uncoveredPaths = _security.getPathsWithUncoveredHttpMethods(); + Set uncoveredPaths = _security.getPathsWithUncoveredHttpMethods(); assertTrue(uncoveredPaths.isEmpty()); //no uncovered methods //Test only an explicitly named method, no omissions to cover other methods @@ -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(); @@ -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(); diff --git a/jetty-security/src/test/java/org/eclipse/jetty/security/SpecExampleConstraintTest.java b/jetty-security/src/test/java/org/eclipse/jetty/security/SpecExampleConstraintTest.java index 125c8f492e8c..bc80b8e97cf3 100644 --- a/jetty-security/src/test/java/org/eclipse/jetty/security/SpecExampleConstraintTest.java +++ b/jetty-security/src/test/java/org/eclipse/jetty/security/SpecExampleConstraintTest.java @@ -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; @@ -237,9 +239,9 @@ public void testUncoveredHttpMethodDetection() throws Exception _security.setAuthenticator(new BasicAuthenticator()); _server.start(); - Set paths = _security.getPathsWithUncoveredHttpMethods(); + Set paths = _security.getPathsWithUncoveredHttpMethods(); assertEquals(1, paths.size()); - assertEquals("/*", paths.iterator().next()); + assertEquals(new ServletPathSpec("/*"), paths.iterator().next()); } @Test diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/Mapping.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/Mapping.java index 6bb8402eacf3..5f047d027c71 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/Mapping.java +++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/Mapping.java @@ -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; @@ -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 pathSpecs) + { + _pathSpecs = (pathSpecs == null) + ? new PathSpec[]{} + : pathSpecs.stream().filter(Objects::nonNull).toArray(PathSpec[]::new); + } + public Source getSource() { return _source; diff --git a/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/StandardDescriptorProcessor.java b/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/StandardDescriptorProcessor.java index 714df8b48c3f..5879f7562fe4 100644 --- a/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/StandardDescriptorProcessor.java +++ b/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/StandardDescriptorProcessor.java @@ -1185,7 +1185,7 @@ public ServletMapping addServletMapping(String servletName, XmlParser.Node node, mapping.setServletName(servletName); mapping.setDefault(descriptor instanceof DefaultsDescriptor); - List paths = new ArrayList(); + List pathspecs = new ArrayList<>(); Iterator iter = node.iterator("url-pattern"); while (iter.hasNext()) { @@ -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()) { @@ -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) { @@ -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); @@ -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 {