Issue #5824 Changes after review

Signed-off-by: Jan Bartel <janb@webtide.com>
jetty · Dec 30, 2020 · 65e53aa · 65e53aa
1 parent 60b3508
commit 65e53aa
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 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
@@ -307,12 +307,15 @@ public void setConstraintMappings(ConstraintMapping[] constraintMappings)
     @Override
     public void setConstraintMappings(List<ConstraintMapping> constraintMappings, Set<String> 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<String> 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
@@ -246,9 +246,12 @@ public void testConstraints() throws Exception
     @Test
     public void testDurableConstraints() throws Exception
     {
+        List<ConstraintMapping> mappings =  _security.getConstraintMappings();
+        assertThat("before start", getConstraintMappings().size(), Matchers.equalTo(mappings.size()));
+
         _server.start();
 
-        List<ConstraintMapping> 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()));
     }
 
     /**