Issue #5824 Durable ConstraintMappings.

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<ConstraintMapping> _constraintMappings = new CopyOnWriteArrayList<>();
+    private final List<ConstraintMapping> _durableConstraintMappings = new CopyOnWriteArrayList<>();
     private final Set<String> _roles = new CopyOnWriteArraySet<>();
     private final PathMap<Map<String, RoleInfo>> _constraintMap = new PathMap<>();
     private boolean _denyUncoveredMethods = false;
@@ -260,12 +262,19 @@ public static List<ConstraintMapping> 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<ConstraintMapping> 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
@@ -308,8 +317,11 @@ public void setConstraintMappings(ConstraintMapping[] constraintMappings)
     @Override
     public void setConstraintMappings(List<ConstraintMapping> constraintMappings, Set<String> roles)
     {
+        _durableConstraintMappings.clear();
         _constraintMappings.clear();
-        _constraintMappings.addAll(constraintMappings);
+
+        if (isInDurableState())
+            _durableConstraintMappings.addAll(constraintMappings);
 
         if (roles == null)
         {
@@ -331,6 +343,7 @@ public void setConstraintMappings(List<ConstraintMapping> constraintMappings, Se
 
         if (isStarted())
         {
+            _constraintMappings.addAll(constraintMappings);    
             for (ConstraintMapping mapping : _constraintMappings)
             {
                 processConstraintMapping(mapping);
@@ -357,7 +370,9 @@ public void setRoles(Set<String> 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 +387,7 @@ public void addConstraintMapping(ConstraintMapping mapping)
 
         if (isStarted())
         {
+            _constraintMappings.add(mapping);
             processConstraintMapping(mapping);
         }
     }
@@ -404,14 +420,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 +434,7 @@ protected void doStop() throws Exception
     {
         super.doStop();
         _constraintMap.clear();
+        _constraintMappings.clear();
     }
 
     /**
@@ -857,4 +868,23 @@ protected Set<String> 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());
+    }
 }

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,52 @@ 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<ConstraintMapping> mappings =  _security.getConstraintMappings();
+        assertThat("after start", getConstraintMappings().size(), Matchers.equalTo(mappings.size()));
+
+        _server.stop();
+
+        //After a stop, just the durable mappings are left
+        mappings = _security.getConstraintMappings();
+        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()));
+
+        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();
+
+        //After a stop, only the durable mappings remain
+        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
      * &#064;ServletSecurity
@@ -655,7 +702,7 @@ public static Stream<Arguments> basicScenarios()
     @MethodSource("basicScenarios")
     public void testBasic(Scenario scenario) throws Exception
     {
-        List<ConstraintMapping> list = new ArrayList<>(_security.getConstraintMappings());
+        List<ConstraintMapping> list = new ArrayList<>(getConstraintMappings());
 
         Constraint constraint6 = new Constraint();
         constraint6.setAuthenticate(true);