From 75183e8413c736b89b9efc88146c384413527000 Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Tue, 9 Feb 2021 09:44:53 +0100 Subject: [PATCH] Issue #5909 - Better handling of merged RoleInfo during omitted method constraints (#5917) * Fix #5909 Better handle merged RoleInfo Signed-off-by: Jan Bartel Co-authored-by: gregw --- .../org/eclipse/jetty/security/RoleInfo.java | 30 +-- .../jetty/security/ConstraintTest.java | 199 ++++++++++++------ 2 files changed, 147 insertions(+), 82 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 index f8ba12f23ac4..eba7b981c6e3 100644 --- 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 _roles = new CopyOnWriteArraySet(); + private final Set _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._isAnyAuth) + setAnyAuth(true); + if (other._isAnyRole) + setAnyRole(true); + _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 index 5353eef9e887..a686581f3e48 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 @@ -31,6 +31,7 @@ import java.util.Set; import java.util.concurrent.TimeUnit; import java.util.function.Consumer; +import java.util.logging.Logger; import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.stream.Stream; @@ -74,6 +75,7 @@ import static java.nio.charset.StandardCharsets.ISO_8859_1; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.in; import static org.hamcrest.Matchers.is; @@ -92,9 +94,17 @@ public class ConstraintTest private LocalConnector _connector; private ConstraintSecurityHandler _security; private HttpConfiguration _config; + private Constraint _forbidConstraint; + private Constraint _authAnyRoleConstraint; + private Constraint _authAdminConstraint; + private Constraint _relaxConstraint; + private Constraint _loginPageConstraint; + private Constraint _noAuthConstraint; + private Constraint _confidentialDataConstraint; + private Constraint _anyUserAuthConstraint; @BeforeEach - public void startServer() + public void setupServer() { _server = new Server(); _connector = new LocalConnector(_server); @@ -143,98 +153,80 @@ public Set getKnownRoles() private List getConstraintMappings() { - Constraint constraint0 = new Constraint(); - constraint0.setAuthenticate(true); - constraint0.setName("forbid"); + _forbidConstraint = new Constraint(); + _forbidConstraint.setAuthenticate(true); + _forbidConstraint.setName("forbid"); ConstraintMapping mapping0 = new ConstraintMapping(); mapping0.setPathSpec("/forbid/*"); - mapping0.setConstraint(constraint0); + mapping0.setConstraint(_forbidConstraint); - Constraint constraint1 = new Constraint(); - constraint1.setAuthenticate(true); - constraint1.setName("auth"); - constraint1.setRoles(new String[]{Constraint.ANY_ROLE}); + _authAnyRoleConstraint = new Constraint(); + _authAnyRoleConstraint.setAuthenticate(true); + _authAnyRoleConstraint.setName("auth"); + _authAnyRoleConstraint.setRoles(new String[]{Constraint.ANY_ROLE}); ConstraintMapping mapping1 = new ConstraintMapping(); mapping1.setPathSpec("/auth/*"); - mapping1.setConstraint(constraint1); + mapping1.setConstraint(_authAnyRoleConstraint); - Constraint constraint2 = new Constraint(); - constraint2.setAuthenticate(true); - constraint2.setName("admin"); - constraint2.setRoles(new String[]{"administrator"}); + _authAdminConstraint = new Constraint(); + _authAdminConstraint.setAuthenticate(true); + _authAdminConstraint.setName("admin"); + _authAdminConstraint.setRoles(new String[]{"administrator"}); ConstraintMapping mapping2 = new ConstraintMapping(); mapping2.setPathSpec("/admin/*"); - mapping2.setConstraint(constraint2); + mapping2.setConstraint(_authAdminConstraint); mapping2.setMethod("GET"); - - Constraint constraint3 = new Constraint(); - constraint3.setAuthenticate(false); - constraint3.setName("relax"); + ConstraintMapping mapping2o = new ConstraintMapping(); + mapping2o.setPathSpec("/admin/*"); + mapping2o.setConstraint(_forbidConstraint); + mapping2o.setMethodOmissions(new String[]{"GET"}); + + _relaxConstraint = new Constraint(); + _relaxConstraint.setAuthenticate(false); + _relaxConstraint.setName("relax"); ConstraintMapping mapping3 = new ConstraintMapping(); mapping3.setPathSpec("/admin/relax/*"); - mapping3.setConstraint(constraint3); + mapping3.setConstraint(_relaxConstraint); - Constraint constraint4 = new Constraint(); - constraint4.setAuthenticate(true); - constraint4.setName("loginpage"); - constraint4.setRoles(new String[]{"administrator"}); + _loginPageConstraint = new Constraint(); + _loginPageConstraint.setAuthenticate(true); + _loginPageConstraint.setName("loginpage"); + _loginPageConstraint.setRoles(new String[]{"administrator"}); ConstraintMapping mapping4 = new ConstraintMapping(); mapping4.setPathSpec("/testLoginPage"); - mapping4.setConstraint(constraint4); + mapping4.setConstraint(_loginPageConstraint); - Constraint constraint5 = new Constraint(); - constraint5.setAuthenticate(false); - constraint5.setName("allow forbidden POST"); + _noAuthConstraint = new Constraint(); + _noAuthConstraint.setAuthenticate(false); + _noAuthConstraint.setName("allow forbidden"); ConstraintMapping mapping5 = new ConstraintMapping(); mapping5.setPathSpec("/forbid/post"); - mapping5.setConstraint(constraint5); + mapping5.setConstraint(_noAuthConstraint); mapping5.setMethod("POST"); - - Constraint constraint6 = new Constraint(); - constraint6.setAuthenticate(false); - constraint6.setName("data constraint"); - constraint6.setDataConstraint(2); + ConstraintMapping mapping5o = new ConstraintMapping(); + mapping5o.setPathSpec("/forbid/post"); + mapping5o.setConstraint(_forbidConstraint); + mapping5o.setMethodOmissions(new String[]{"POST"}); + + _confidentialDataConstraint = new Constraint(); + _confidentialDataConstraint.setAuthenticate(false); + _confidentialDataConstraint.setName("data constraint"); + _confidentialDataConstraint.setDataConstraint(Constraint.DC_CONFIDENTIAL); ConstraintMapping mapping6 = new ConstraintMapping(); mapping6.setPathSpec("/data/*"); - mapping6.setConstraint(constraint6); + mapping6.setConstraint(_confidentialDataConstraint); - Constraint constraint7 = new Constraint(); - constraint7.setAuthenticate(true); - constraint7.setName("** constraint"); - constraint7.setRoles(new String[]{ + _anyUserAuthConstraint = new Constraint(); + _anyUserAuthConstraint.setAuthenticate(true); + _anyUserAuthConstraint.setName("** constraint"); + _anyUserAuthConstraint.setRoles(new String[]{ Constraint.ANY_AUTH, "user" }); //the "user" role is superfluous once ** has been defined ConstraintMapping mapping7 = new ConstraintMapping(); mapping7.setPathSpec("/starstar/*"); - mapping7.setConstraint(constraint7); - - return Arrays.asList(mapping0, mapping1, mapping2, mapping3, mapping4, mapping5, mapping6, mapping7); - } - - @Test - public void testConstraints() throws Exception - { - List mappings = new ArrayList<>(_security.getConstraintMappings()); - - assertTrue(mappings.get(0).getConstraint().isForbidden()); - assertFalse(mappings.get(1).getConstraint().isForbidden()); - assertFalse(mappings.get(2).getConstraint().isForbidden()); - assertFalse(mappings.get(3).getConstraint().isForbidden()); + mapping7.setConstraint(_anyUserAuthConstraint); - assertFalse(mappings.get(0).getConstraint().isAnyRole()); - assertTrue(mappings.get(1).getConstraint().isAnyRole()); - assertFalse(mappings.get(2).getConstraint().isAnyRole()); - assertFalse(mappings.get(3).getConstraint().isAnyRole()); - - assertFalse(mappings.get(0).getConstraint().hasRole("administrator")); - assertTrue(mappings.get(1).getConstraint().hasRole("administrator")); - assertTrue(mappings.get(2).getConstraint().hasRole("administrator")); - assertFalse(mappings.get(3).getConstraint().hasRole("administrator")); - - assertTrue(mappings.get(0).getConstraint().getAuthenticate()); - assertTrue(mappings.get(1).getConstraint().getAuthenticate()); - assertTrue(mappings.get(2).getConstraint().getAuthenticate()); - assertFalse(mappings.get(3).getConstraint().getAuthenticate()); + return Arrays.asList(mapping0, mapping1, mapping2, mapping2o, mapping3, mapping4, mapping5, mapping5o, mapping6, mapping7); } /** @@ -1798,7 +1790,78 @@ public void testRelaxedMethod() throws Exception assertThat(response, startsWith("HTTP/1.1 200 ")); response = _connector.getResponse("GET /ctx/forbid/post HTTP/1.0\r\n\r\n"); - assertThat(response, startsWith("HTTP/1.1 200 ")); // This is so stupid, but it is the S P E C + assertThat(response, startsWith("HTTP/1.1 403 ")); + } + + @Test + public void testUncoveredMethod() throws Exception + { + ConstraintMapping specificMethod = new ConstraintMapping(); + specificMethod.setMethod("GET"); + specificMethod.setPathSpec("/specific/method"); + specificMethod.setConstraint(_forbidConstraint); + _security.addConstraintMapping(specificMethod); + _security.setAuthenticator(new BasicAuthenticator()); + Logger.getAnonymousLogger().info("Uncovered method for /specific/method is expected"); + _server.start(); + + assertThat(_security.getPathsWithUncoveredHttpMethods(), contains("/specific/method")); + + String response; + response = _connector.getResponse("GET /ctx/specific/method HTTP/1.0\r\n\r\n"); + assertThat(response, startsWith("HTTP/1.1 403 ")); + + response = _connector.getResponse("POST /ctx/specific/method HTTP/1.0\r\n\r\n"); + assertThat(response, startsWith("HTTP/1.1 200 ")); // This is so stupid, but it is the S P E C + } + + @Test + public void testForbidTraceAndOptions() throws Exception + { + ConstraintMapping forbidTrace = new ConstraintMapping(); + forbidTrace.setMethod("TRACE"); + forbidTrace.setPathSpec("/"); + forbidTrace.setConstraint(_forbidConstraint); + ConstraintMapping allowOmitTrace = new ConstraintMapping(); + allowOmitTrace.setMethodOmissions(new String[] {"TRACE"}); + allowOmitTrace.setPathSpec("/"); + allowOmitTrace.setConstraint(_relaxConstraint); + + ConstraintMapping forbidOptions = new ConstraintMapping(); + forbidOptions.setMethod("OPTIONS"); + forbidOptions.setPathSpec("/"); + forbidOptions.setConstraint(_forbidConstraint); + ConstraintMapping allowOmitOptions = new ConstraintMapping(); + allowOmitOptions.setMethodOmissions(new String[] {"OPTIONS"}); + allowOmitOptions.setPathSpec("/"); + allowOmitOptions.setConstraint(_relaxConstraint); + + 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(); + + assertThat(_security.getPathsWithUncoveredHttpMethods(), Matchers.empty()); + + String response; + response = _connector.getResponse("TRACE /ctx/some/path HTTP/1.0\r\n\r\n"); + assertThat(response, startsWith("HTTP/1.1 403 ")); + + response = _connector.getResponse("OPTIONS /ctx/some/path HTTP/1.0\r\n\r\n"); + assertThat(response, startsWith("HTTP/1.1 403 ")); + + 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)