Skip to content

Commit

Permalink
Issue #5909 - Better handling of merged RoleInfo during omitted metho…
Browse files Browse the repository at this point in the history 
…d constraints (#5917)

* Fix #5909 Better handle merged RoleInfo

Signed-off-by: Jan Bartel <janb@webtide.com>

Co-authored-by: gregw <gregw@webtide.com>
  • Loading branch information
@janbartel @gregw
janbartel and gregw committed Feb 9, 2021
1 parent f768e2e commit 75183e8
Show file tree
Hide file tree
Showing 2 changed files with 147 additions and 82 deletions.
30 changes: 16 additions & 14 deletions jetty-security/src/main/java/org/eclipse/jetty/security/RoleInfo.java
View file
Expand Up @@ -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()
{
Expand Down Expand Up @@ -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);
}
}
199 changes: 131 additions & 68 deletions jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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);
Expand Down Expand Up @@ -143,98 +153,80 @@ public Set<String> getKnownRoles()

private List<ConstraintMapping> 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<ConstraintMapping> 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);
}

/**
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit 75183e8

Please sign in to comment.