diff --git a/core/src/main/java/org/apache/struts2/interceptor/RolesInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/RolesInterceptor.java index 22f9062d3..f55f1d262 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/RolesInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/RolesInterceptor.java @@ -23,19 +23,20 @@ package org.apache.struts2.interceptor; import com.opensymphony.xwork2.ActionInvocation; import com.opensymphony.xwork2.interceptor.AbstractInterceptor; +import com.opensymphony.xwork2.util.logging.Logger; +import com.opensymphony.xwork2.util.logging.LoggerFactory; import org.apache.struts2.ServletActionContext; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; /** - * This interceptor ensures that the action - * will only be executed if the user has the correct role. + * + * This interceptor ensures that the action will only be executed if the user has the correct role. + * * *

Interceptor parameters: * @@ -49,15 +50,26 @@ import java.util.List; * * * + *

+ * When both allowedRoles and disallowedRoles are configured, then disallowedRoles + * takes precedence, applying the following logic: + * (if ((inRole(role1) || inRole(role2) || ... inRole(roleN)) && + * !inRole(roleA) && !inRole(roleB) && ... !inRole(roleZ)) + * { //permit ... + *

* * - * There are two extensions to the - * existing interceptor: + * + * There are three extensions to the existing interceptor: * * * @@ -76,27 +88,42 @@ import java.util.List; */ public class RolesInterceptor extends AbstractInterceptor { - protected List allowedRoles = new ArrayList(); - protected List disallowedRoles = new ArrayList(); + private static final Logger LOG = LoggerFactory.getLogger(RolesInterceptor.class); + + private boolean isProperlyConfigured = true; + + protected List allowedRoles = Collections.emptyList(); + protected List disallowedRoles = Collections.emptyList(); public void setAllowedRoles(String roles) { - this.allowedRoles = stringToList(roles); + allowedRoles = stringToList(roles); + checkRoles(allowedRoles); } public void setDisallowedRoles(String roles) { - this.disallowedRoles = stringToList(roles); + disallowedRoles = stringToList(roles); + checkRoles(disallowedRoles); + } + + private void checkRoles(List roles){ + if (!areRolesValid(roles)){ + LOG.fatal("An unknown Role was configured: #0", roles.toString()); + isProperlyConfigured = false; + throw new IllegalArgumentException("An unknown role was configured: " + roles); + } } public String intercept(ActionInvocation invocation) throws Exception { HttpServletRequest request = ServletActionContext.getRequest(); HttpServletResponse response = ServletActionContext.getResponse(); - String result; - if (!isAllowed(request, invocation.getAction())) { - result = handleRejection(invocation, response); - } else { - result = invocation.invoke(); + if (!isProperlyConfigured) { + throw new IllegalArgumentException("RolesInterceptor is misconfigured, check logs for erroneous configuration!"); + } + if (!isAllowed(request, invocation.getAction())) { + return handleRejection(invocation, response); + } else { + return invocation.invoke(); } - return result; } /** @@ -119,23 +146,23 @@ public class RolesInterceptor extends AbstractInterceptor { * @return True if allowed, false otherwise */ protected boolean isAllowed(HttpServletRequest request, Object action) { - if (allowedRoles.size() > 0) { - boolean result = false; - for (String role : allowedRoles) { - if (request.isUserInRole(role)) { - result = true; - break; - } - } - return result; - } else if (disallowedRoles.size() > 0) { - for (String role : disallowedRoles) { - if (request.isUserInRole(role)) { - return false; - } + for (String role : disallowedRoles) { + if (request.isUserInRole(role)) { + return false; } } - return true; + + if (allowedRoles.isEmpty()){ + return true; + } + + for (String role : allowedRoles) { + if (request.isUserInRole(role)) { + return true; + } + } + + return false; } /** @@ -151,4 +178,17 @@ public class RolesInterceptor extends AbstractInterceptor { response.sendError(HttpServletResponse.SC_FORBIDDEN); return null; } + + /** + * Extension point for sub-classes to test if configured roles are known valid roles. + * Implementations are encouraged to implement this method to prevent misconfigured roles. + * If this method returns false, the RolesInterceptor will be disabled and block all requests. + * + * @param roles allowed and disallowed roles + * @return whether the roles are valid or not (always true for the default implementation) + */ + protected boolean areRolesValid(List roles){ + return true; + } + } diff --git a/core/src/test/java/org/apache/struts2/interceptor/RolesInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/RolesInterceptorTest.java index f38e55828..8be3e2380 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/RolesInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/RolesInterceptorTest.java @@ -21,8 +21,12 @@ package org.apache.struts2.interceptor; +import java.util.Arrays; +import java.util.HashSet; import java.util.List; +import javax.security.auth.login.FailedLoginException; + import org.apache.struts2.StrutsTestCase; import com.mockobjects.servlet.MockHttpServletRequest; @@ -78,10 +82,93 @@ public class RolesInterceptorTest extends StrutsTestCase { } + public void testIsAllowed_userAllowedAndGuestDisallowed() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest() { + public boolean isUserInRole(String role) { + return "user".equals(role) || "guest".equals(role); + } + }; + + interceptor.setAllowedRoles("user"); //has to be a user + interceptor.setDisallowedRoles("guest"); //and not a guest + assertFalse(interceptor.isAllowed(request, null)); + } + + public void testIsAllowed_adminAllowedExceptManager() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest() { + public boolean isUserInRole(String role) { + return "admin".equals(role); + } + }; + + interceptor.setAllowedRoles("admin");//allow all + interceptor.setDisallowedRoles("manager"); + assertTrue(interceptor.isAllowed(request, null)); + } + + public void testIsAllowed_sameRoleAllowedAndDisallowed() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest() { + public boolean isUserInRole(String role) { + return "admin".equals(role); + } + }; + + interceptor.setAllowedRoles("admin"); + interceptor.setDisallowedRoles("admin"); + assertFalse(interceptor.isAllowed(request, null)); + } + + + public void testIsAllowed_emptyAllowedAndDisallowed() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest() { + public boolean isUserInRole(String role) { + return "admin".equals(role); + } + }; + + interceptor.setAllowedRoles("");//allow all + interceptor.setDisallowedRoles("admin"); + assertFalse(interceptor.isAllowed(request, null)); + } + public void testHandleRejection() throws Exception { MockHttpServletResponse response = new MockHttpServletResponse(); response.setExpectedError(response.SC_FORBIDDEN); interceptor.handleRejection(null, response); response.verify(); } + + public void testAreRolesValid() throws Exception { + RolesInterceptor roleCheckInterceptor = new RolesInterceptor(){ + List validRoles = Arrays.asList(new String[]{"admin","user"}); + @Override + public boolean areRolesValid(List roles){ + return validRoles.containsAll(roles); + } + }; + try { + roleCheckInterceptor.setAllowedRoles("admin, user"); + roleCheckInterceptor.setDisallowedRoles("admin, user"); + } catch (Exception e){ + fail("Valid roles should not throw an exception"); + } + try { + roleCheckInterceptor.setAllowedRoles("hacker, abuser"); + fail("Invalid roles should throw an exception"); + } catch (Exception e){ + //expected + } + try { + roleCheckInterceptor.setAllowedRoles("nonadmin, nonuser"); + fail("Invalid roles should throw an exception"); + } catch (Exception e){ + //expected + } + try { + roleCheckInterceptor.intercept(null); + fail("A misconfigured should always throw an exception"); + } catch (Exception e){ + //expected; + } + } }