From 76a2fb94888b49c06ab725af2b6afb19359dd739 Mon Sep 17 00:00:00 2001 From: Rob Winch Date: Thu, 16 Jul 2015 14:33:33 -0500 Subject: [PATCH] SEC-3020: SecurityContextHolderAwareRequestWrapper conditional rolePrefix Previously SecurityContextHolderAwareRequestWrapper always prefixed with rolePrefix. This meant the defaults would never return true for a role that started with the prefix (i.e. ROLE_). We no longer apply the rolePrefix if the value passed in already starts with rolePrefix. --- ...urityContextHolderAwareRequestWrapper.java | 2 +- ...ContextHolderAwareRequestWrapperTests.java | 29 +++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/web/src/main/java/org/springframework/security/web/servletapi/SecurityContextHolderAwareRequestWrapper.java b/web/src/main/java/org/springframework/security/web/servletapi/SecurityContextHolderAwareRequestWrapper.java index dbe3b5c004..9d82990e80 100644 --- a/web/src/main/java/org/springframework/security/web/servletapi/SecurityContextHolderAwareRequestWrapper.java +++ b/web/src/main/java/org/springframework/security/web/servletapi/SecurityContextHolderAwareRequestWrapper.java @@ -150,7 +150,7 @@ public class SecurityContextHolderAwareRequestWrapper extends HttpServletRequest private boolean isGranted(String role) { Authentication auth = getAuthentication(); - if (rolePrefix != null) { + if (rolePrefix != null && role != null && !role.startsWith(rolePrefix)) { role = rolePrefix + role; } diff --git a/web/src/test/java/org/springframework/security/web/servletapi/SecurityContextHolderAwareRequestWrapperTests.java b/web/src/test/java/org/springframework/security/web/servletapi/SecurityContextHolderAwareRequestWrapperTests.java index 91f11a3629..def19cb28f 100644 --- a/web/src/test/java/org/springframework/security/web/servletapi/SecurityContextHolderAwareRequestWrapperTests.java +++ b/web/src/test/java/org/springframework/security/web/servletapi/SecurityContextHolderAwareRequestWrapperTests.java @@ -114,4 +114,33 @@ public class SecurityContextHolderAwareRequestWrapperTests extends TestCase { assertFalse(wrapper.isUserInRole("ROLE_FOOBAR")); // principal is null, so reject assertNull(wrapper.getUserPrincipal()); } + + public void testRolePrefix() { + Authentication auth = new TestingAuthenticationToken("user", "koala", "ROLE_HELLO", + "ROLE_FOOBAR"); + SecurityContextHolder.getContext().setAuthentication(auth); + + MockHttpServletRequest request = new MockHttpServletRequest(); + + SecurityContextHolderAwareRequestWrapper wrapper = new SecurityContextHolderAwareRequestWrapper( + request, "ROLE_"); + + assertTrue(wrapper.isUserInRole("HELLO")); + assertTrue(wrapper.isUserInRole("FOOBAR")); + } + + // SEC-3020 + public void testRolePrefixNotAppliedIfRoleStartsWith() { + Authentication auth = new TestingAuthenticationToken("user", "koala", "ROLE_HELLO", + "ROLE_FOOBAR"); + SecurityContextHolder.getContext().setAuthentication(auth); + + MockHttpServletRequest request = new MockHttpServletRequest(); + + SecurityContextHolderAwareRequestWrapper wrapper = new SecurityContextHolderAwareRequestWrapper( + request, "ROLE_"); + + assertTrue(wrapper.isUserInRole("ROLE_HELLO")); + assertTrue(wrapper.isUserInRole("ROLE_FOOBAR")); + } }