Fix #5909 Better handle merged RoleInfo

jetty · Jan 26, 2021 · cc4b6ec · cc4b6ec
1 parent 1f1c4ae
commit cc4b6ec
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 20 deletions.
diff --git a/jetty-security/src/main/java/org/eclipse/jetty/security/RoleInfo.java b/jetty-security/src/main/java/org/eclipse/jetty/security/RoleInfo.java
@@ -41,7 +41,7 @@ public class RoleInfo
     /**
      * List of permitted roles
      */
-    private final Set<String> _roles = new CopyOnWriteArraySet<String>();
+    private final Set<String> _roles = new CopyOnWriteArraySet<>();
 
     public RoleInfo()
     {
@@ -140,26 +140,28 @@ public void combine(RoleInfo other)
     {
         if (other._forbidden)
             setForbidden(true);
-        else if (!other._checked) // TODO is this the right way around???
-            setChecked(true);
-        else if (other._isAnyRole)
-            setAnyRole(true);
-        else if (other._isAnyAuth)
-            setAnyAuth(true);
-        else if (!_isAnyRole)
+        else if (other._checked)
         {
-            for (String r : other._roles)
-            {
-                _roles.add(r);
-            }
+            setChecked(true);
+            if (other._isAnyRole)
+                setAnyRole(true);
+            else if (other._isAnyAuth)
+                setAnyAuth(true);
+            else if (!_isAnyRole)
+                _roles.addAll(other._roles);
         }
-
         setUserDataConstraint(other._userDataConstraint);
     }
 
     @Override
     public String toString()
     {
-        return "{RoleInfo" + (_forbidden ? ",F" : "") + (_checked ? ",C" : "") + (_isAnyRole ? ",*" : _roles) + (_userDataConstraint != null ? "," + _userDataConstraint : "") + "}";
+        return String.format("RoleInfo@%x{%s%s%s%s,%s}",
+            hashCode(),
+            (_forbidden ? "Forbidden," : ""),
+            (_checked ? "Checked," : ""),
+            (_isAnyAuth ? "AnyAuth," : ""),
+            (_isAnyRole ? "*" : _roles),
+            _userDataConstraint);
     }
 }
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
@@ -99,7 +99,7 @@ public class ConstraintTest
     private Constraint _authAdminConstraint;
     private Constraint _relaxConstraint;
     private Constraint _loginPageConstraint;
-    private Constraint _allowPostConstraint;
+    private Constraint _noAuthConstraint;
     private Constraint _confidentialDataConstraint;
     private Constraint _anyUserAuthConstraint;
 
@@ -196,12 +196,12 @@ private List<ConstraintMapping> getConstraintMappings()
         mapping4.setPathSpec("/testLoginPage");
         mapping4.setConstraint(_loginPageConstraint);
 
-        _allowPostConstraint = new Constraint();
-        _allowPostConstraint.setAuthenticate(false);
-        _allowPostConstraint.setName("allow forbidden POST");
+        _noAuthConstraint = new Constraint();
+        _noAuthConstraint.setAuthenticate(false);
+        _noAuthConstraint.setName("allow forbidden");
         ConstraintMapping mapping5 = new ConstraintMapping();
         mapping5.setPathSpec("/forbid/post");
-        mapping5.setConstraint(_allowPostConstraint);
+        mapping5.setConstraint(_noAuthConstraint);
         mapping5.setMethod("POST");
         ConstraintMapping mapping5o = new ConstraintMapping();
         mapping5o.setPathSpec("/forbid/post");
@@ -1836,7 +1836,11 @@ public void testForbidTraceAndOptions() throws Exception
         allowOmitOptions.setPathSpec("/");
         allowOmitOptions.setConstraint(_relaxConstraint);
 
-        _security.setConstraintMappings(new ConstraintMapping[] {forbidTrace, allowOmitTrace, forbidOptions, allowOmitOptions});
+        ConstraintMapping someConstraint = new ConstraintMapping();
+        someConstraint.setPathSpec("/some/constaint/*");
+        someConstraint.setConstraint(_noAuthConstraint);
+
+        _security.setConstraintMappings(new ConstraintMapping[] {forbidTrace, allowOmitTrace, forbidOptions, allowOmitOptions, someConstraint});
 
         _security.setAuthenticator(new BasicAuthenticator());
         _server.start();
@@ -1852,6 +1856,12 @@ public void testForbidTraceAndOptions() throws Exception
 
         response = _connector.getResponse("GET /ctx/some/path HTTP/1.0\r\n\r\n");
         assertThat(response, startsWith("HTTP/1.1 200 "));
+
+        response = _connector.getResponse("GET /ctx/some/constraint/info HTTP/1.0\r\n\r\n");
+        assertThat(response, startsWith("HTTP/1.1 200 "));
+
+        response = _connector.getResponse("OPTIONS /ctx/some/constraint/info HTTP/1.0\r\n\r\n");
+        assertThat(response, startsWith("HTTP/1.1 403 "));
     }
 
     private static String authBase64(String authorization)