From 7955ebcb3de5caa6d3351b524cb328743ea3bc6c Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Wed, 23 Dec 2020 18:32:54 +0100 Subject: [PATCH 1/5] Issue #5824 Durable ConstraintMappings. Signed-off-by: Jan Bartel --- .../security/ConstraintSecurityHandler.java | 43 ++++++++++++++----- .../jetty/security/ConstraintTest.java | 43 +++++++++++++++++++ 2 files changed, 76 insertions(+), 10 deletions(-) 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 5396e0b363f9..a82984f798f8 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 @@ -42,6 +42,7 @@ import org.eclipse.jetty.server.HttpConfiguration; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.Response; +import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.UserIdentity; import org.eclipse.jetty.server.handler.ContextHandler; import org.eclipse.jetty.util.URIUtil; @@ -64,6 +65,7 @@ public class ConstraintSecurityHandler extends SecurityHandler implements Constr private static final String OMISSION_SUFFIX = ".omission"; private static final String ALL_METHODS = "*"; private final List _constraintMappings = new CopyOnWriteArrayList<>(); + private final List _durableConstraintMappings = new CopyOnWriteArrayList<>(); private final Set _roles = new CopyOnWriteArraySet<>(); private final PathMap> _constraintMap = new PathMap<>(); private boolean _denyUncoveredMethods = false; @@ -308,8 +310,11 @@ public void setConstraintMappings(ConstraintMapping[] constraintMappings) @Override public void setConstraintMappings(List constraintMappings, Set roles) { + _durableConstraintMappings.clear(); _constraintMappings.clear(); - _constraintMappings.addAll(constraintMappings); + + if (isInDurableState()) + _durableConstraintMappings.addAll(constraintMappings); if (roles == null) { @@ -331,6 +336,7 @@ public void setConstraintMappings(List constraintMappings, Se if (isStarted()) { + _constraintMappings.addAll(constraintMappings); for (ConstraintMapping mapping : _constraintMappings) { processConstraintMapping(mapping); @@ -357,7 +363,9 @@ public void setRoles(Set roles) @Override public void addConstraintMapping(ConstraintMapping mapping) { - _constraintMappings.add(mapping); + if (isInDurableState()) + _durableConstraintMappings.add(mapping); + if (mapping.getConstraint() != null && mapping.getConstraint().getRoles() != null) { //allow for lazy role naming: if a role is named in a security constraint, try and @@ -372,6 +380,7 @@ public void addConstraintMapping(ConstraintMapping mapping) if (isStarted()) { + _constraintMappings.add(mapping); processConstraintMapping(mapping); } } @@ -404,14 +413,8 @@ public void addRole(String role) @Override protected void doStart() throws Exception { - _constraintMap.clear(); - if (_constraintMappings != null) - { - for (ConstraintMapping mapping : _constraintMappings) - { - processConstraintMapping(mapping); - } - } + _constraintMappings.addAll(_durableConstraintMappings); + _constraintMappings.stream().forEach(m -> processConstraintMapping(m)); //Servlet Spec 3.1 pg 147 sec 13.8.4.2 log paths for which there are uncovered http methods checkPathsWithUncoveredHttpMethods(); @@ -424,6 +427,7 @@ protected void doStop() throws Exception { super.doStop(); _constraintMap.clear(); + _constraintMappings.clear(); } /** @@ -857,4 +861,23 @@ protected Set getOmittedMethods(String omission) } return methods; } + + /** + * Constraints can be added to the ConstraintSecurityHandler before the + * associated context is started. These constraints should persist across + * a stop/start. Others can be added after the associated context is starting + * (eg by a web.xml/web-fragment.xml, annotation or javax.servlet api call) - + * these should not be persisted across a stop/start as they will be re-added on + * the restart. + * + * @return true if the context with which this ConstraintSecurityHandler + * has not yet started, or if there is no context, the server has not yet started. + */ + private boolean isInDurableState() + { + ContextHandler context = ContextHandler.getContextHandler(null); + Server server = getServer(); + + return (context == null && server == null) || (context != null && !context.isRunning()) || (context == null && server != null && !server.isRunning()); + } } 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..d60c22afe51c 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 @@ -64,6 +64,7 @@ import org.eclipse.jetty.util.TypeUtil; import org.eclipse.jetty.util.security.Constraint; import org.eclipse.jetty.util.security.Password; +import org.hamcrest.Matchers; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -236,6 +237,48 @@ public void testConstraints() throws Exception assertFalse(mappings.get(3).getConstraint().getAuthenticate()); } + /** + * Test that constraint mappings added before the context starts are + * retained, but those that are added after the context starts are not. + * + * @throws Exception + */ + @Test + public void testDurableConstraints() throws Exception + { + _server.start(); + + List mappings = _security.getConstraintMappings(); + assertThat("after start", getConstraintMappings().size(), Matchers.equalTo(mappings.size())); + + _server.stop(); + + mappings = _security.getConstraintMappings(); + assertThat("after restart", 0, Matchers.equalTo(mappings.size())); + + _server.start(); + mappings = _security.getConstraintMappings(); + assertThat("after restart", getConstraintMappings().size(), Matchers.equalTo(mappings.size())); + + ConstraintMapping mapping = new ConstraintMapping(); + mapping.setPathSpec("/xxxx/*"); + Constraint constraint = new Constraint(); + constraint.setAuthenticate(false); + constraint.setName("transient"); + mapping.setConstraint(constraint); + + _security.addConstraintMapping(mapping); + + mappings = _security.getConstraintMappings(); + assertThat("after addition", getConstraintMappings().size() + 1, Matchers.equalTo(mappings.size())); + + _server.stop(); + _server.start(); + + mappings = _security.getConstraintMappings(); + assertThat("after addition", getConstraintMappings().size(), Matchers.equalTo(mappings.size())); + } + /** * Equivalent of Servlet Spec 3.1 pg 132, sec 13.4.1.1, Example 13-1 * @ServletSecurity From 9d328ce78bad6f3e353ef5398b7acac6ad810d72 Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Wed, 23 Dec 2020 21:56:43 +0100 Subject: [PATCH 2/5] Issue #5824 Fix ConstraintTest Signed-off-by: Jan Bartel --- .../test/java/org/eclipse/jetty/security/ConstraintTest.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 d60c22afe51c..af91c6a0ab80 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 @@ -214,6 +214,9 @@ private List getConstraintMappings() @Test public void testConstraints() throws Exception { + //constraint mappings are not available until the server is started. + _server.start(); + List mappings = new ArrayList<>(_security.getConstraintMappings()); assertTrue(mappings.get(0).getConstraint().isForbidden()); @@ -698,7 +701,7 @@ public static Stream basicScenarios() @MethodSource("basicScenarios") public void testBasic(Scenario scenario) throws Exception { - List list = new ArrayList<>(_security.getConstraintMappings()); + List list = new ArrayList<>(getConstraintMappings()); Constraint constraint6 = new Constraint(); constraint6.setAuthenticate(true); From 39d7083b47052ab7cbaf671fe822b58b8a773018 Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Thu, 24 Dec 2020 15:27:39 +0100 Subject: [PATCH 3/5] Issue #5824 Return only durable mappings before ConstraintSecurityHandler start Signed-off-by: Jan Bartel --- .../jetty/security/ConstraintSecurityHandler.java | 11 +++++++++-- .../org/eclipse/jetty/security/ConstraintTest.java | 9 +++++---- 2 files changed, 14 insertions(+), 6 deletions(-) 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 a82984f798f8..17a911454a9a 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 @@ -262,12 +262,19 @@ public static List createConstraintsWithMappingsForPath(Strin } /** - * @return Returns the constraintMappings. + * @return only the durable mappings if we are not yet started, otherwise + * return both the durables and transients. */ @Override public List getConstraintMappings() { - return _constraintMappings; + //if we've started, then we've processed both the durable and + //transient constraint mappings + if (isRunning()) + return _constraintMappings; + else + //otherwise, we only have durables + return _durableConstraintMappings; } @Override 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 af91c6a0ab80..439ce9b8be38 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 @@ -214,9 +214,6 @@ private List getConstraintMappings() @Test public void testConstraints() throws Exception { - //constraint mappings are not available until the server is started. - _server.start(); - List mappings = new ArrayList<>(_security.getConstraintMappings()); assertTrue(mappings.get(0).getConstraint().isForbidden()); @@ -256,10 +253,13 @@ public void testDurableConstraints() throws Exception _server.stop(); + //After a stop, just the durable mappings are left mappings = _security.getConstraintMappings(); - assertThat("after restart", 0, Matchers.equalTo(mappings.size())); + assertThat("after stop", getConstraintMappings().size(), Matchers.equalTo(mappings.size())); _server.start(); + + //Verify the constraints are just the durables mappings = _security.getConstraintMappings(); assertThat("after restart", getConstraintMappings().size(), Matchers.equalTo(mappings.size())); @@ -278,6 +278,7 @@ public void testDurableConstraints() throws Exception _server.stop(); _server.start(); + //After a stop, only the durable mappings remain mappings = _security.getConstraintMappings(); assertThat("after addition", getConstraintMappings().size(), Matchers.equalTo(mappings.size())); } From 60b350876c398bff3cac9b28c3a3cfac0e2ea762 Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Wed, 30 Dec 2020 11:51:03 +0100 Subject: [PATCH 4/5] Issue #5824 Change after review Signed-off-by: Jan Bartel --- .../security/ConstraintSecurityHandler.java | 28 +++++-------------- 1 file changed, 7 insertions(+), 21 deletions(-) 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 17a911454a9a..f14c9fd1a889 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 @@ -261,20 +261,10 @@ public static List createConstraintsWithMappingsForPath(Strin return mappings; } - /** - * @return only the durable mappings if we are not yet started, otherwise - * return both the durables and transients. - */ @Override public List getConstraintMappings() { - //if we've started, then we've processed both the durable and - //transient constraint mappings - if (isRunning()) - return _constraintMappings; - else - //otherwise, we only have durables - return _durableConstraintMappings; + return _constraintMappings; } @Override @@ -319,6 +309,7 @@ public void setConstraintMappings(List constraintMappings, Se { _durableConstraintMappings.clear(); _constraintMappings.clear(); + _constraintMappings.addAll(constraintMappings); if (isInDurableState()) _durableConstraintMappings.addAll(constraintMappings); @@ -343,11 +334,7 @@ public void setConstraintMappings(List constraintMappings, Se if (isStarted()) { - _constraintMappings.addAll(constraintMappings); - for (ConstraintMapping mapping : _constraintMappings) - { - processConstraintMapping(mapping); - } + _constraintMappings.stream().forEach(m -> processConstraintMapping(m)); } } @@ -373,6 +360,8 @@ public void addConstraintMapping(ConstraintMapping mapping) if (isInDurableState()) _durableConstraintMappings.add(mapping); + _constraintMappings.add(mapping); + if (mapping.getConstraint() != null && mapping.getConstraint().getRoles() != null) { //allow for lazy role naming: if a role is named in a security constraint, try and @@ -386,10 +375,7 @@ public void addConstraintMapping(ConstraintMapping mapping) } if (isStarted()) - { - _constraintMappings.add(mapping); processConstraintMapping(mapping); - } } /** @@ -420,7 +406,6 @@ public void addRole(String role) @Override protected void doStart() throws Exception { - _constraintMappings.addAll(_durableConstraintMappings); _constraintMappings.stream().forEach(m -> processConstraintMapping(m)); //Servlet Spec 3.1 pg 147 sec 13.8.4.2 log paths for which there are uncovered http methods @@ -435,7 +420,8 @@ protected void doStop() throws Exception super.doStop(); _constraintMap.clear(); _constraintMappings.clear(); - } + _constraintMappings.addAll(_durableConstraintMappings); + } /** * Create and combine the constraint with the existing processed From 65e53aa0b196364321816c41cd9431070c65b4e2 Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Wed, 30 Dec 2020 17:42:06 +0100 Subject: [PATCH 5/5] Issue #5824 Changes after review Signed-off-by: Jan Bartel --- .../security/ConstraintSecurityHandler.java | 11 +++++--- .../jetty/security/ConstraintTest.java | 28 ++++++++++++++++++- 2 files changed, 34 insertions(+), 5 deletions(-) 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 f14c9fd1a889..faba57d8fcc2 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 @@ -307,12 +307,15 @@ public void setConstraintMappings(ConstraintMapping[] constraintMappings) @Override public void setConstraintMappings(List constraintMappings, Set roles) { - _durableConstraintMappings.clear(); + _constraintMappings.clear(); - _constraintMappings.addAll(constraintMappings); + _constraintMappings.addAll(constraintMappings); + _durableConstraintMappings.clear(); if (isInDurableState()) + { _durableConstraintMappings.addAll(constraintMappings); + } if (roles == null) { @@ -357,11 +360,11 @@ public void setRoles(Set roles) @Override public void addConstraintMapping(ConstraintMapping mapping) { + _constraintMappings.add(mapping); + if (isInDurableState()) _durableConstraintMappings.add(mapping); - _constraintMappings.add(mapping); - if (mapping.getConstraint() != null && mapping.getConstraint().getRoles() != null) { //allow for lazy role naming: if a role is named in a security constraint, try and 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 439ce9b8be38..baf04618ddfa 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 @@ -246,9 +246,12 @@ public void testConstraints() throws Exception @Test public void testDurableConstraints() throws Exception { + List mappings = _security.getConstraintMappings(); + assertThat("before start", getConstraintMappings().size(), Matchers.equalTo(mappings.size())); + _server.start(); - List mappings = _security.getConstraintMappings(); + mappings = _security.getConstraintMappings(); assertThat("after start", getConstraintMappings().size(), Matchers.equalTo(mappings.size())); _server.stop(); @@ -263,6 +266,7 @@ public void testDurableConstraints() throws Exception mappings = _security.getConstraintMappings(); assertThat("after restart", getConstraintMappings().size(), Matchers.equalTo(mappings.size())); + //Add a non-durable constraint ConstraintMapping mapping = new ConstraintMapping(); mapping.setPathSpec("/xxxx/*"); Constraint constraint = new Constraint(); @@ -281,6 +285,28 @@ public void testDurableConstraints() throws Exception //After a stop, only the durable mappings remain mappings = _security.getConstraintMappings(); assertThat("after addition", getConstraintMappings().size(), Matchers.equalTo(mappings.size())); + + //test that setConstraintMappings replaces all existing mappings whether durable or not + + //test setConstraintMappings in durable state + _server.stop(); + _security.setConstraintMappings(Collections.singletonList(mapping)); + mappings = _security.getConstraintMappings(); + assertThat("after set during stop", 1, Matchers.equalTo(mappings.size())); + _server.start(); + mappings = _security.getConstraintMappings(); + assertThat("after set after start", 1, Matchers.equalTo(mappings.size())); + + //test setConstraintMappings not in durable state + _server.stop(); + _server.start(); + assertThat("no change after start", 1, Matchers.equalTo(mappings.size())); + _security.setConstraintMappings(getConstraintMappings()); + mappings = _security.getConstraintMappings(); + assertThat("durables lost", getConstraintMappings().size(), Matchers.equalTo(mappings.size())); + _server.stop(); + mappings = _security.getConstraintMappings(); + assertThat("no mappings", 0, Matchers.equalTo(mappings.size())); } /**