Skip to content

Commit 094bf1b

Browse files
bist220jzheaux
authored andcommitted
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
1 parent 6666534 commit 094bf1b

File tree

2 files changed

+32
-5
lines changed

2 files changed

+32
-5
lines changed

core/src/main/java/org/springframework/security/authorization/AuthorityAuthorizationManager.java

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2022 the original author or authors.
2+
* Copyright 2002-2023 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -46,12 +46,15 @@ private AuthorityAuthorizationManager(String... authorities) {
4646
/**
4747
* Creates an instance of {@link AuthorityAuthorizationManager} with the provided
4848
* authority.
49-
* @param role the authority to check for prefixed with "ROLE_"
49+
* @param role the authority to check for prefixed with "ROLE_". Role should not start
50+
* with "ROLE_" since it is automatically prepended already.
5051
* @param <T> the type of object being authorized
5152
* @return the new instance
5253
*/
5354
public static <T> AuthorityAuthorizationManager<T> hasRole(String role) {
5455
Assert.notNull(role, "role cannot be null");
56+
Assert.isTrue(!role.startsWith(ROLE_PREFIX), () -> role + " should not start with " + ROLE_PREFIX + " since "
57+
+ ROLE_PREFIX + " is automatically prepended when using hasRole. Consider using hasAuthority instead.");
5558
return hasAuthority(ROLE_PREFIX + role);
5659
}
5760

@@ -70,7 +73,8 @@ public static <T> AuthorityAuthorizationManager<T> hasAuthority(String authority
7073
/**
7174
* Creates an instance of {@link AuthorityAuthorizationManager} with the provided
7275
* authorities.
73-
* @param roles the authorities to check for prefixed with "ROLE_"
76+
* @param roles the authorities to check for prefixed with "ROLE_". Each role should
77+
* not start with "ROLE_" since it is automatically prepended already.
7478
* @param <T> the type of object being authorized
7579
* @return the new instance
7680
*/
@@ -109,7 +113,11 @@ public static <T> AuthorityAuthorizationManager<T> hasAnyAuthority(String... aut
109113
private static String[] toNamedRolesArray(String rolePrefix, String[] roles) {
110114
String[] result = new String[roles.length];
111115
for (int i = 0; i < roles.length; i++) {
112-
result[i] = rolePrefix + roles[i];
116+
String role = roles[i];
117+
Assert.isTrue(!role.startsWith(rolePrefix), () -> role + " should not start with " + rolePrefix + " since "
118+
+ rolePrefix
119+
+ " is automatically prepended when using hasAnyRole. Consider using hasAnyAuthority instead.");
120+
result[i] = rolePrefix + role;
113121
}
114122
return result;
115123
}

core/src/test/java/org/springframework/security/authorization/AuthorityAuthorizationManagerTests.java

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2021 the original author or authors.
2+
* Copyright 2002-2023 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -41,6 +41,15 @@ public void hasRoleWhenNullThenException() {
4141
.withMessage("role cannot be null");
4242
}
4343

44+
@Test
45+
public void hasRoleWhenContainRoleWithRolePrefixThenException() {
46+
String ROLE_PREFIX = "ROLE_";
47+
String ROLE_USER = ROLE_PREFIX + "USER";
48+
assertThatIllegalArgumentException().isThrownBy(() -> AuthorityAuthorizationManager.hasRole(ROLE_USER))
49+
.withMessage(ROLE_USER + " should not start with " + ROLE_PREFIX + " since " + ROLE_PREFIX
50+
+ " is automatically prepended when using hasRole. Consider using hasAuthority instead.");
51+
}
52+
4453
@Test
4554
public void hasAuthorityWhenNullThenException() {
4655
assertThatIllegalArgumentException().isThrownBy(() -> AuthorityAuthorizationManager.hasAuthority(null))
@@ -73,6 +82,16 @@ public void hasAnyRoleWhenCustomRolePrefixNullThenException() {
7382
.withMessage("rolePrefix cannot be null");
7483
}
7584

85+
@Test
86+
public void hasAnyRoleWhenContainRoleWithRolePrefixThenException() {
87+
String ROLE_PREFIX = "ROLE_";
88+
String ROLE_USER = ROLE_PREFIX + "USER";
89+
assertThatIllegalArgumentException()
90+
.isThrownBy(() -> AuthorityAuthorizationManager.hasAnyRole(new String[] { ROLE_USER }))
91+
.withMessage(ROLE_USER + " should not start with " + ROLE_PREFIX + " since " + ROLE_PREFIX
92+
+ " is automatically prepended when using hasAnyRole. Consider using hasAnyAuthority instead.");
93+
}
94+
7695
@Test
7796
public void hasAnyAuthorityWhenNullThenException() {
7897
assertThatIllegalArgumentException().isThrownBy(() -> AuthorityAuthorizationManager.hasAnyAuthority(null))

0 commit comments

Comments
 (0)