From 094bf1b52750faaf8115f87bfef68f575c49264f Mon Sep 17 00:00:00 2001 From: bist Date: Wed, 22 Feb 2023 01:25:39 +0530 Subject: [PATCH] Validate hasRole Input There are no check for role prefix in AuthorizeHttpRequestsConfigurer#XXXrole methods. This PR adds check for the same. Now the configuration will fail if role/s start with prefix for hasRole and hasAnyRole methods. Closes #12581 --- .../AuthorityAuthorizationManager.java | 16 ++++++++++---- .../AuthorityAuthorizationManagerTests.java | 21 ++++++++++++++++++- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/springframework/security/authorization/AuthorityAuthorizationManager.java b/core/src/main/java/org/springframework/security/authorization/AuthorityAuthorizationManager.java index 43af8a5cb8..484ca956f2 100644 --- a/core/src/main/java/org/springframework/security/authorization/AuthorityAuthorizationManager.java +++ b/core/src/main/java/org/springframework/security/authorization/AuthorityAuthorizationManager.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2023 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -46,12 +46,15 @@ public final class AuthorityAuthorizationManager implements AuthorizationMana /** * Creates an instance of {@link AuthorityAuthorizationManager} with the provided * authority. - * @param role the authority to check for prefixed with "ROLE_" + * @param role the authority to check for prefixed with "ROLE_". Role should not start + * with "ROLE_" since it is automatically prepended already. * @param the type of object being authorized * @return the new instance */ public static AuthorityAuthorizationManager hasRole(String role) { Assert.notNull(role, "role cannot be null"); + Assert.isTrue(!role.startsWith(ROLE_PREFIX), () -> role + " should not start with " + ROLE_PREFIX + " since " + + ROLE_PREFIX + " is automatically prepended when using hasRole. Consider using hasAuthority instead."); return hasAuthority(ROLE_PREFIX + role); } @@ -70,7 +73,8 @@ public final class AuthorityAuthorizationManager implements AuthorizationMana /** * Creates an instance of {@link AuthorityAuthorizationManager} with the provided * authorities. - * @param roles the authorities to check for prefixed with "ROLE_" + * @param roles the authorities to check for prefixed with "ROLE_". Each role should + * not start with "ROLE_" since it is automatically prepended already. * @param the type of object being authorized * @return the new instance */ @@ -109,7 +113,11 @@ public final class AuthorityAuthorizationManager implements AuthorizationMana private static String[] toNamedRolesArray(String rolePrefix, String[] roles) { String[] result = new String[roles.length]; for (int i = 0; i < roles.length; i++) { - result[i] = rolePrefix + roles[i]; + String role = roles[i]; + Assert.isTrue(!role.startsWith(rolePrefix), () -> role + " should not start with " + rolePrefix + " since " + + rolePrefix + + " is automatically prepended when using hasAnyRole. Consider using hasAnyAuthority instead."); + result[i] = rolePrefix + role; } return result; } diff --git a/core/src/test/java/org/springframework/security/authorization/AuthorityAuthorizationManagerTests.java b/core/src/test/java/org/springframework/security/authorization/AuthorityAuthorizationManagerTests.java index ce5d40604b..8c00a752c0 100644 --- a/core/src/test/java/org/springframework/security/authorization/AuthorityAuthorizationManagerTests.java +++ b/core/src/test/java/org/springframework/security/authorization/AuthorityAuthorizationManagerTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2021 the original author or authors. + * Copyright 2002-2023 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -41,6 +41,15 @@ public class AuthorityAuthorizationManagerTests { .withMessage("role cannot be null"); } + @Test + public void hasRoleWhenContainRoleWithRolePrefixThenException() { + String ROLE_PREFIX = "ROLE_"; + String ROLE_USER = ROLE_PREFIX + "USER"; + assertThatIllegalArgumentException().isThrownBy(() -> AuthorityAuthorizationManager.hasRole(ROLE_USER)) + .withMessage(ROLE_USER + " should not start with " + ROLE_PREFIX + " since " + ROLE_PREFIX + + " is automatically prepended when using hasRole. Consider using hasAuthority instead."); + } + @Test public void hasAuthorityWhenNullThenException() { assertThatIllegalArgumentException().isThrownBy(() -> AuthorityAuthorizationManager.hasAuthority(null)) @@ -73,6 +82,16 @@ public class AuthorityAuthorizationManagerTests { .withMessage("rolePrefix cannot be null"); } + @Test + public void hasAnyRoleWhenContainRoleWithRolePrefixThenException() { + String ROLE_PREFIX = "ROLE_"; + String ROLE_USER = ROLE_PREFIX + "USER"; + assertThatIllegalArgumentException() + .isThrownBy(() -> AuthorityAuthorizationManager.hasAnyRole(new String[] { ROLE_USER })) + .withMessage(ROLE_USER + " should not start with " + ROLE_PREFIX + " since " + ROLE_PREFIX + + " is automatically prepended when using hasAnyRole. Consider using hasAnyAuthority instead."); + } + @Test public void hasAnyAuthorityWhenNullThenException() { assertThatIllegalArgumentException().isThrownBy(() -> AuthorityAuthorizationManager.hasAnyAuthority(null))