diff --git a/taglibs/src/main/java/org/springframework/security/taglibs/authz/AbstractAuthorizeTag.java b/taglibs/src/main/java/org/springframework/security/taglibs/authz/AbstractAuthorizeTag.java index 9fcbae62a2..8092510e26 100644 --- a/taglibs/src/main/java/org/springframework/security/taglibs/authz/AbstractAuthorizeTag.java +++ b/taglibs/src/main/java/org/springframework/security/taglibs/authz/AbstractAuthorizeTag.java @@ -38,7 +38,6 @@ import org.springframework.security.access.expression.ExpressionUtils; import org.springframework.security.access.expression.SecurityExpressionHandler; import org.springframework.security.core.Authentication; import org.springframework.security.core.GrantedAuthority; -import org.springframework.security.core.authority.AuthorityUtils; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.security.web.FilterInvocation; import org.springframework.security.web.access.WebInvocationPrivilegeEvaluator; @@ -56,6 +55,7 @@ import org.springframework.web.context.support.WebApplicationContextUtils; * @author Francois Beausoleil * @author Luke Taylor * @author Rossen Stoyanchev + * @author Rob Winch * @since 3.1.0 */ public abstract class AbstractAuthorizeTag { @@ -130,23 +130,25 @@ public abstract class AbstractAuthorizeTag { } final Collection granted = getPrincipalAuthorities(); + final Set grantedRoles = authoritiesToRoles(granted); if (hasTextAllGranted) { - if (!granted.containsAll(toAuthorities(getIfAllGranted()))) { + final Set requiredRoles = splitRoles(getIfAllGranted()); + if (!grantedRoles.containsAll(requiredRoles)) { return false; } } if (hasTextAnyGranted) { - Set grantedCopy = retainAll(granted, toAuthorities(getIfAnyGranted())); - if (grantedCopy.isEmpty()) { + final Set expectOneOfRoles = splitRoles(getIfAnyGranted()); + if (!containsAnyValue(grantedRoles, expectOneOfRoles)) { return false; } } if (hasTextNotGranted) { - Set grantedCopy = retainAll(granted, toAuthorities(getIfNotGranted())); - if (!grantedCopy.isEmpty()) { + final Set expectNoneOfRoles = splitRoles(getIfNotGranted()); + if (containsAnyValue(expectNoneOfRoles, grantedRoles)) { return false; } } @@ -265,19 +267,33 @@ public abstract class AbstractAuthorizeTag { return currentUser.getAuthorities(); } - private Set toAuthorities(String authorizations) { - final Set requiredAuthorities = new HashSet(); - requiredAuthorities.addAll(AuthorityUtils.commaSeparatedStringToAuthorityList(authorizations)); - return requiredAuthorities; + /** + * Splits the authorityString using "," as a delimiter into a Set. + * @param authorityString + * @return + */ + private Set splitRoles(String authorityString) { + String[] rolesArray = StringUtils.tokenizeToStringArray(authorityString, ","); + Set roles = new HashSet(rolesArray.length); + for(String role : rolesArray) { + roles.add(role); + } + return roles; } - private Set retainAll(final Collection granted, - final Set required) { - Set grantedRoles = authoritiesToRoles(granted); - Set requiredRoles = authoritiesToRoles(required); - grantedRoles.retainAll(requiredRoles); - - return rolesToAuthorities(grantedRoles, granted); + /** + * Returns true if any of the values are contained in toTest. Otherwise, false. + * @param toTest Check this Set to see if any of the values are contained in it. + * @param values The values to check if they are in toTest. + * @return + */ + private boolean containsAnyValue(Set toTest, Collection values) { + for(String value : values) { + if(toTest.contains(value)) { + return true; + } + } + return false; } private Set authoritiesToRoles(Collection c) { @@ -293,19 +309,6 @@ public abstract class AbstractAuthorizeTag { return target; } - private Set rolesToAuthorities(Set grantedRoles, Collection granted) { - Set target = new HashSet(); - for (String role : grantedRoles) { - for (GrantedAuthority authority : granted) { - if (authority.getAuthority().equals(role)) { - target.add(authority); - break; - } - } - } - return target; - } - @SuppressWarnings("unchecked") private SecurityExpressionHandler getExpressionHandler() throws IOException { ApplicationContext appContext = WebApplicationContextUtils diff --git a/taglibs/src/test/java/org/springframework/security/taglibs/authz/AuthorizeTagCustomGrantedAuthorityTests.java b/taglibs/src/test/java/org/springframework/security/taglibs/authz/AuthorizeTagCustomGrantedAuthorityTests.java index fa1d65a5d4..bd96f297e5 100644 --- a/taglibs/src/test/java/org/springframework/security/taglibs/authz/AuthorizeTagCustomGrantedAuthorityTests.java +++ b/taglibs/src/test/java/org/springframework/security/taglibs/authz/AuthorizeTagCustomGrantedAuthorityTests.java @@ -20,6 +20,7 @@ import static org.junit.Assert.*; import org.junit.*; import org.springframework.security.authentication.TestingAuthenticationToken; import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.core.authority.GrantedAuthorityImpl; import org.springframework.security.core.context.SecurityContextHolder; import javax.servlet.jsp.JspException; @@ -73,4 +74,56 @@ public class AuthorizeTagCustomGrantedAuthorityTests { assertTrue("expected", true); } } + + @Test + @SuppressWarnings("serial") + public void testAuthorizeCustomGrantedAuthority() throws JspException { + authorizeTag.setIfAnyGranted(null); + authorizeTag.setIfNotGranted(null); + authorizeTag.setIfAllGranted("ROLE_TEST"); + List authorities = new ArrayList(); + authorities.add(new GrantedAuthority() { + public String getAuthority() { + return "ROLE_TEST"; + } + }); + SecurityContextHolder.getContext().setAuthentication(new TestingAuthenticationToken("abc", "123", authorities)); + assertEquals("Expected to be authorized", Tag.EVAL_BODY_INCLUDE, authorizeTag.doStartTag()); + } + + @Test + @SuppressWarnings("serial") + public void testAuthorizeExtendsGrantedAuthorityImpl() throws JspException { + authorizeTag.setIfAnyGranted(null); + authorizeTag.setIfNotGranted(null); + authorizeTag.setIfAllGranted("ROLE_TEST"); + List authorities = new ArrayList(); + authorities.add(new GrantedAuthorityImpl("ROLE_TEST") {}); + SecurityContextHolder.getContext().setAuthentication(new TestingAuthenticationToken("abc", "123", authorities)); + assertEquals("Expected to be authorized", Tag.EVAL_BODY_INCLUDE, authorizeTag.doStartTag()); + } + + // SEC-1900 + @Test + public void testAuthorizeUsingGrantedAuthorityImpl() throws JspException { + authorizeTag.setIfAnyGranted(null); + authorizeTag.setIfNotGranted(null); + authorizeTag.setIfAllGranted("ROLE_TEST"); + List authorities = new ArrayList(); + authorities.add(new GrantedAuthorityImpl("ROLE_TEST")); + SecurityContextHolder.getContext().setAuthentication(new TestingAuthenticationToken("abc", "123", authorities)); + assertEquals("Expected to be authorized", Tag.EVAL_BODY_INCLUDE, authorizeTag.doStartTag()); + } + + // SEC-1900 + @Test + public void testNotAuthorizeUsingGrantedAuthorityImpl() throws JspException { + authorizeTag.setIfAnyGranted(null); + authorizeTag.setIfNotGranted(null); + authorizeTag.setIfAllGranted("ROLE_ADMIN"); + List authorities = new ArrayList(); + authorities.add(new GrantedAuthorityImpl("ROLE_TEST")); + SecurityContextHolder.getContext().setAuthentication(new TestingAuthenticationToken("abc", "123", authorities)); + assertEquals("Expected to not be authorized", Tag.SKIP_BODY, authorizeTag.doStartTag()); + } }